Re: [Xen-devel] [PATCH v5 2/7] xen/arm: zynqmp: Forward plaform specific firmware calls

[Julien Grall <Julien.Grall@xxxxxxx>]

On 11/12/2018 20:19, Stefano Stabellini wrote:
> On Tue, 11 Dec 2018, Julien Grall wrote:
>> Hi Stefano,
>>
>> On 11/12/2018 18:50, Stefano Stabellini wrote:
>>> On Tue, 11 Dec 2018, Julien Grall wrote:
>>>> Hi Stefano,
>>>>
>>>> On 03/12/2018 21:03, Stefano Stabellini wrote:
>>>>> From: "Edgar E. Iglesias" <edgar.iglesias@xxxxxxxxxx>
>>>>>
>>>>> From: Edgar E. Iglesias <edgar.iglesias@xxxxxxxxxx>
>>>>>
>>>>> Introduce zynqmp_eemi: a function responsible for implementing access
>>>>> controls over the firmware calls. Only calls that are allowed are
>>>>> forwarded to the firmware.
>>>>>
>>>>> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xxxxxxxxxx>
>>>>> Signed-off-by: Stefano Stabellini <stefanos@xxxxxxxxxx>
>>>>>
>>>>> ---
>>>>> Changes in v4:
>>>>> - fix typo
>>>>> - add header guard
>>>>> - add emacs magic
>>>>> - remove #includes that will only be used later
>>>>> - add copyright notice to header
>>>>> - remove SMCCC 1.1 check
>>>>> ---
>>>>>     xen/arch/arm/platforms/Makefile                    |  1 +
>>>>>     xen/arch/arm/platforms/xilinx-zynqmp-eemi.c        | 34
>>>>> ++++++++++++++++++++++
>>>>>     xen/arch/arm/platforms/xilinx-zynqmp.c             | 11 +++++++
>>>>>     xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h | 30
>>>>> +++++++++++++++++++
>>>>>     4 files changed, 76 insertions(+)
>>>>>     create mode 100644 xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
>>>>>     create mode 100644 xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h
>>>>>
>>>>> diff --git a/xen/arch/arm/platforms/Makefile
>>>>> b/xen/arch/arm/platforms/Makefile
>>>>> index a79bdb9..fe8e0c7 100644
>>>>> --- a/xen/arch/arm/platforms/Makefile
>>>>> +++ b/xen/arch/arm/platforms/Makefile
>>>>> @@ -9,3 +9,4 @@ obj-y += sunxi.o
>>>>>     obj-$(CONFIG_ARM_64) += thunderx.o
>>>>>     obj-$(CONFIG_ARM_64) += xgene-storm.o
>>>>>     obj-$(CONFIG_MPSOC_PLATFORM)  += xilinx-zynqmp.o
>>>>> +obj-$(CONFIG_MPSOC_PLATFORM)  += xilinx-zynqmp-eemi.o
>>>>> diff --git a/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
>>>>> b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
>>>>> new file mode 100644
>>>>> index 0000000..369bb3f
>>>>> --- /dev/null
>>>>> +++ b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
>>>>> @@ -0,0 +1,34 @@
>>>>> +/*
>>>>> + * xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
>>>>> + *
>>>>> + * Xilinx ZynqMP EEMI API
>>>>> + *
>>>>> + * Copyright (c) 2018 Xilinx Inc.
>>>>> + * Written by Edgar E. Iglesias <edgar.iglesias@xxxxxxxxxx>
>>>>> + *
>>>>> + * This program is free software; you can redistribute it and/or
>>>>> + * modify it under the terms and conditions of the GNU General Public
>>>>> + * License, version 2, as published by the Free Software Foundation.
>>>>> + *
>>>>> + * This program is distributed in the hope that it will be useful,
>>>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>>>> + * GNU General Public License for more details.
>>>>> + */
>>>>> +
>>>>> +#include <asm/regs.h>
>>>>> +#include <asm/platforms/xilinx-zynqmp-eemi.h>
>>>>> +
>>>>> +bool zynqmp_eemi(struct cpu_user_regs *regs)
>>>>> +{
>>>>> +    return false;
>>>>> +}
>>>>> +
>>>>> +/*
>>>>> + * Local variables:
>>>>> + * mode: C
>>>>> + * c-file-style: "BSD"
>>>>> + * c-basic-offset: 4
>>>>> + * indent-tabs-mode: nil
>>>>> + * End:
>>>>> + */
>>>>> diff --git a/xen/arch/arm/platforms/xilinx-zynqmp.c
>>>>> b/xen/arch/arm/platforms/xilinx-zynqmp.c
>>>>> index d8ceded..9c174d2 100644
>>>>> --- a/xen/arch/arm/platforms/xilinx-zynqmp.c
>>>>> +++ b/xen/arch/arm/platforms/xilinx-zynqmp.c
>>>>> @@ -18,6 +18,8 @@
>>>>>      */
>>>>>       #include <asm/platform.h>
>>>>> +#include <asm/platforms/xilinx-zynqmp-eemi.h>
>>>>> +#include <asm/smccc.h>
>>>>>       static const char * const zynqmp_dt_compat[] __initconst =
>>>>>     {
>>>>> @@ -32,8 +34,17 @@ static const struct dt_device_match
>>>>> zynqmp_blacklist_dev[] __initconst =
>>>>>         { /* sentinel */ },
>>>>>     };
>>>>>     +static bool zynqmp_smc(struct cpu_user_regs *regs)
>>>>> +{
>>>>> +    if ( !is_64bit_domain(current->domain) )
>>>>
>>>> Please document why you only expose eemi to 64-bit domain. What if the
>>>> user
>>>> start with 32-bit Dom0?
>>>
>>> I'll add a in-code comment saying that only 64-bit guests are supported
>>> by the current implementation -- the 32 bit EEMI ABI is not yet covered.
>>
>> 64-bit guests is allowed to use both SMC32 and SMC64. There are actually some
>> call that can only be done using SMC32 convention (see patch #6). So why
>> forbid 32-bit domain when you allow 64-bit using SMC32?
> 
> I understand. It really looks like this check is a mistake. In fact, it
> doesn't matter if a domain is 32bit or 64bit, the only thing that
> matters for this code is if the EEMI calls are SMC32 and SMC64. So
> instead of is_64bit_domain, I should check that SMC64 is used. Right?

This would be wrong because you at least need to handle UID, Call count 
and version using SMC32 as mandated by the SMCCC.

The right solution is to deal with the full identifier in patch #6 
rather than just the function ID.

Cheers,

-- 
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel