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

[Julien Grall <julien.grall@arm.com>]

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@epam.com>
>>>>>>> Reviewed-by: Oleksandr Andrushchenko
>>>>>>> <oleksandr_andrushchenko@epam.com>
>>>>>>> Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>>>>> ---
>>>>>>>  - 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.

> 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.

>
>> 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.

>
> 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,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel