Re: [Xen-devel] [PATCH 4/7] arm: smccc: handle SMCs according to SMCCC

[Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx>]

Hi Julien,

On 11.08.17 00:09, Julien Grall wrote:
> On 10/08/2017 21:09, Volodymyr Babchuk wrote:
> > Hi,
> >
> > On 10.08.17 21:18, Julien Grall wrote:
> > > Hi,
> > >
> > > On 10/08/17 18:40, Volodymyr Babchuk wrote:
> > > > On 10.08.17 19:11, Julien Grall wrote:
> > > > > On 10/08/17 16:33, Volodymyr Babchuk wrote:
> > > > > > Hi Julien,
> > > > > >
> > > > > > On 09.08.17 13:10, Julien Grall wrote:
> > > > > > > Hi Volodymyr,
> > > > > > >
> > > > > > > CC "THE REST" maintainers to get an opinion on the public headers.
> > > > > > >
> > > > > > > On 08/08/17 21:08, Volodymyr Babchuk wrote:
> > > > > > > > SMCCC (SMC Call Convention) describes how to handle both HVCs and
> > > > > > > > SMCs.
> > > > > > > > SMCCC states that both HVC and SMC are valid conduits to call to a
> > > > > > > > different
> > > > > > > > firmware functions. Thus, for example PSCI calls can be made 
> > > > > > > > both by
> > > > > > > > SMC or HVC. Also SMCCC defines function number coding for such
> > > > > > > > calls.
> > > > > > > > Besides functional calls there are query calls, which allows
> > > > > > > > underling
> > > > > > > > OS determine version, UID and number of functions provided by
> > > > > > > > service
> > > > > > > > provider.
> > > > > > > >
> > > > > > > > This patch adds new file `vsmc.c`, which handles both generic SMCs
> > > > > > > > and HVC according to SMC. At this moment it implements only one
> > > > > > > > service: Standard Hypervisor Service.
> > > > > > > >
> > > > > > > > Standard Hypervisor Service only supports query calls, so caller 
> > > > > > > > can
> > > > > > > > ask about hypervisor UID and determine that it is XEN running.
> > > > > > > >
> > > > > > > > This change allows more generic handling for SMCs and HVCs and it
> > > > > > > > can
> > > > > > > > be easily extended to support new services and functions.
> > > > > > > >
> > > > > > > > But, before SMC is forwarded to standard SMCCC handler, it can be
> > > > > > > > routed
> > > > > > > > to a domain monitor, if one is installed.
> > > > > > > >
> > > > > > > > Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx>
> > > > > > > > Reviewed-by: Oleksandr Andrushchenko
> > > > > > > > <oleksandr_andrushchenko@xxxxxxxx>
> > > > > > > > Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> > > > > > > > ---
> > > > > > > >  - Updated description to indicate that this patch affects only SMC
> > > > > > > > call path.
> > > > > > > >  - added "xen_" prefix to definitions in
> > > > > > > > include/public/arch-arm/smc.h
> > > > > > > >  - moved do_trap_smc() into vsmc.c from traps.c
> > > > > > > >  - replaced all tabs with spaces
> > > > > > > I would have really appreciated a summary of the discussion we 
> > > > > > > had on
> > > > > > > the previous version regarding the bindings. This is a real blocker
> > > > > > > for this series and should not be ignored.
> > > > > > While I agree that question about bindings is important, I can't
> > > > > > see how
> > > > > > it affects this patch series. This patch series does not break
> > > > > > anything.
> > > > > > Because
> > > > > >
> > > > > > 1. This series add only new feature: generic hypervisor service
> > > > > > with no
> > > > > > immediate use. All ARM guests are already aware that they are
> > > > > > running on
> > > > > > XEN. All ARM guests know that they call *only* PSCI.
> > > > > >
> > > > > > 2. I see importance of this patch series for embedded platforms, 
> > > > > > where
> > > > > > developer exactly knows what software of which version he/she will
> > > > > > run.
> > > > > > I doubt that server platforms will need something beyond PSCI, 
> > > > > > which I
> > > > > > preserved as is.
> > > > > I disagree here. SMCCC could be used to provide Dom0 a way to interact
> > > > > with the firmware if necessary.
> > > > AFAIK, Dom0 usually is built with particular version of XEN in mind (or
> > > > at least minimal XEN version).
> > > That's not true. Dom0 is a generic kernel able to probe everything
> > > from the firmware tables and Xen interface. I use the exact Linux
> > > kernel on multiple platforms with no issue.
> > Okay, I was speaking about embedded use case.
> Bear in mind that Xen is not only embedded. When you upstream a new 
> feature you have to think about how this could be used by anyone.
> > > The Hypervisor and Dom0 (Linux, FreeBSD) are different projects with
> > > different release cadence. We provide a stable interface that is
> > > backward compatible (returning error code for non-existent
> > > hypercalls). So a Linux released in 10 years should still work on Xen
> > > 4.10 and adapt to the features available.
> > I just want to clarify this. At least four hypercalls are not backward
> > compatible: platform_op, sysctl, domctl and flask_op. I had a problem
> > with this, when played with MiniOS-based monitor. Sure, your kernel will
> > boot up, but some well known XEN services will be absent.
> > This have nothing with SMC bindings problem, though.
> I am not sure to follow here... Were they ever be supported on ARM? If 
> not, then it is no a backward compatibility issue. You cannot assume 
> that all the hypercalls existing on x86 will be available on ARM.
> For a list of known available/supported hypercalls for ARM see:
> public/include/arch-arm.h.
>
> Also bear in mind that some hypercalls are not part of the stable ABI. 
> For instance domctls are not stable and it is well-known that you have 
> to rebuild your app for every new Xen versions...
> But domctls will never be used in Linux Kernel as they only meant to be 
> used to control a domain.
> > > > > > A I'm not denying importance of SMC bindings, but I think it is not
> > > > > > blocker for my patches. We can add bindings later, when there will be
> > > > > > consensus on how they should look. In meantime SMC handler can be 
> > > > > > used
> > > > > > by anyone who knows that is available.
> > > > > I am not in favor on getting something merged in Xen until we agree on
> > > > > the way for the guest to know it is there.
> > > > I think that SMC implementation will be the same, regardless the way we
> > > > can tell guest that it is available. At this time guests can safely
> > > > assume that SMCCC is not implemented in XEN. This wouldn't break
> > > > anything.
> > > So you suggest to merge this series but says "The guest should not
> > > assume the presence of SMC"? This is rather a bit odd...
> > Basically yes. This is one of the options. If guest knowns for sure that
> > SMCCC is available - it can use it. If it is unsure - then it does not
> > use it.
> > In meantime we can develop a generic way to tell guest that SMCCC is
> > present on a platform.
> > I just don't want to bloat up this patch series. There are already 7
> > patches and looks like there will be more.
> Let's be honest, a 7 patches series is quite small :P.
Aaaah, okay :)

> > But I can add another patch/two with bindings if you insist. This is not
> > a big deal. I just have concerns about this right now.
> >
> > > > > It means you have to carry hack in your kernel in order to use SMC.
> > > > > Maybe this is fine for you, but I don't want to make this assumption
> > > > > on Xen upstream today.
> > > > Modern linux kernel uses SMC for PSCI calls and OP-TEE calls. In both
> > > > cases it reads DT to get conduit method (SMC/HVC). So there are already
> > > > bindings for generic uses. Other uses are platform-specific (okay,
> > > > probably there can be a problem).
> > > >
> > > > > This is a change in the interface that should be notified to the
> > > > > guest. If we expose it without providing a bindings (or something), we
> > > > > have no way to revert/disable it. Imagine we want to disable SMC in
> > > > > the future.
> > > > Natural way was to disable Security Extensions in PFR1 register. This
> > > > was not done and now we have curious situation: guest thinks that 
> > > > SMC is
> > > > available, but then it gets Undefined Instruction exception when it
> > > > tries to invoke SMC.
> > > Security extensions does not mean that SMCCC is present... Even if we
> > > had returned an error, you don't know if it was from the SMCCC
> > > protocol or my foo protocol.
> > > It would be valid (though a bit odd) for a firmware to inject an
> > > undefined instruction if it is unable to understand the SMC.
> > Okay, I can see logic there. This is a strange way to return an error,
> > but it is not prohibited in reference manual at least.
> >
> > > The main problem today is neither HVC #0 nor SMC #0 are SMCCC
> > > compliant. They are let's call it XEN Protocol compliant. KVM has the
> > > same problem.
> > > So you clearly need a way to say: "HVCs and SMCs are SMCCC compliant,
> > > you can go ahead probing the different components".
> > Yes, I agree with this. I just trying to say, that it is not only XEN
> > problem, it is platform-wide.
> I am well aware about it... and kept saying that from the beginning. 
> Except that nobody really thought about it because embedded are mostly 
> focused to get things working for a specific set of configuration.
Yes, I see. Okay, I'm curious on how such thinks should be worked out. 
E.g. XEN ML is not the best place to discuss platform-wide questions.
> > > As I said, I am not asking you to write the binding. I am asking you
> > > to have in mind other use cases. If the binding takes to much time,
> > > then a solution would be to add a XENFEAT_*.
> > I can write DT binding. Problem is that this binding should be adopted
> > by all OSes, hypervisors and firmwares. More on this below.
> >
> > > > > How a guest will know that
> > > > >      - until Xen 4.10 SMC was not existing,
> > > > >      - between Xen 4.10 and Xen 4.x you can use them
> > > > >      - after Xen 4.y they can be disabled.
> > > > It is a broader question: how software can know that SMCCC is available
> > > > on a platform? Not SMC, but SMCCC as a protocol. Probably, there should
> > > > be some generic way to tell Linux/Windows/XEN/Windriver Hypervisor/etc
> > > > that they can rely on SMCCC spec. I think that it is question to ARM
> > > > guys (including you), because this affects all ARM machines.
> > > I spent quite some time to explain that in an e-mail to the previous
> > > version... See the thread [1].
> > Yes, I've seen this thread, naturally. But I can't find there answer to
> > my question.
> Because there is none of the moment and it is still in discussion...
>
> > As I said, this binding (or any other way to tell software about SMCCC)
> > should not be XEN-specific. As you mentioned, KVM have the exactly same
> > problem. I think, bare-metal platforms suffer from similar issue. So, in
> > my opinion, it will be great to have agreement on SMCCC discovery from
> > all players. So, when I boot my kernel on any ARM machine, it can tell
> > for sure if SMCCC is supported. What to you think?
> There are already discussion going-on. It is known and taken care.
Are you referring to that thread at [1]? Or to some internal discussion?

> > We can stick to XEN-only approach, like XENFEAT_* or "xen,smccc" in DT.
> > But is this right?
> > > > > All changes should be detected through the firmware tables (DT, ACPI)
> > > > > or another Xen method (i.e XENFEAT_*). For instance, the guest has to
> > > > > parse the firmware tables in order to know PSCI is available.
> > > > Yep. The same does OP-TEE code. It parses DT to get conduit. Probably,
> > > > this is wrong approach. Should all SMCCC calls use the same conduit?.
> > > > Should platform provide conduit for each SMCCC service owner?
> > > Technically a guest should only use HVC. I am happy to allow the SMC
> > > to support unmodified OS that don't use DT.
> > > Cheers,
> > >
> > > [1]
> > > https://lists.xenproject.org/archives/html/xen-devel/2017-08/msg00068.html 
> Cheers,
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel