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

[Julien Grall <Julien.Grall@xxxxxxx>]

Hi Stefano,

On 17/12/2018 18:17, Stefano Stabellini wrote:
> On Mon, 17 Dec 2018, Julien Grall wrote:
>> Hi,
>>
>> On 14/12/2018 19:11, 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 v6:
>>> - remove is_domain_64 check
>>> - add check for smccc 1.1
>>> - code style
>>>
>>> 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             | 14 +++++++++
>>>    xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h | 30
>>> +++++++++++++++++++
>>>    4 files changed, 79 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 bd724a1..01608f8 100644
>>> --- a/xen/arch/arm/platforms/Makefile
>>> +++ b/xen/arch/arm/platforms/Makefile
>>> @@ -9,3 +9,4 @@ obj-$(CONFIG_ALL_PLAT)   += sunxi.o
>>>    obj-$(CONFIG_ALL64_PLAT) += thunderx.o
>>>    obj-$(CONFIG_ALL64_PLAT) += 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..b1e67fd 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,20 @@ static const struct dt_device_match
>>> zynqmp_blacklist_dev[] __initconst =
>>>        { /* sentinel */ },
>>>    };
>>>    +static bool zynqmp_smc(struct cpu_user_regs *regs)
>>> +{
>>> +    /*
>>> +     * ZynqMP firmware is based on SMCCC 1.1. If SMCCC 1.1 is not
>>> +     * available something is wrong, don't try to handle it.
>>> +     */
>>
>> Why not just denying booting Xen on such platform? I guess we would need to
>> add a callback (e.g presmp_init) in the platform for that purpose.
> 
> Yes, we would need a new callback. I wasn't too keen on adding one more.

Checking at every SMC is a bit pointless :). That's platform specific 
code, so I don't much mind the solution here.

> 
> The other reason for doing it this way is that even if the user doesn't
> have the right firmware version (I cannot imagine what version could it
> be), it makes sense to stop any firmware related functions, including
> EEMI, but continue booting nonetheless. I guess I should also have added
> a clear warning to say that firmware functionalities have been disabled
> because wrong firmware or no-firmware is present.

Most likely the hardware domain would not behave correctly. So I am not 
sure that worth trying to continue in Xen.

> 
> What do you think?

The warning would definitely be an improvement. Anyway as I said above, 
that's platform specific code. So I don't much care whether this is call 
everytime.

Cheers,

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