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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Mon, 17 Dec 2018, Julien Grall wrote:
> 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.

I gave it a look, but I think it is not worth adding a new hook for
this, moreover we would have to change the implementation of .smc
dynamically which is not supported (PLATFORM_START structs are
read-only). I think it is not worth the trouble. I'll add a warning
printed only once to this patch.

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