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

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.

What do you think?


> > +    if ( !cpus_have_const_cap(ARM_SMCCC_1_1) )
> > +        return false;
> > +    return zynqmp_eemi(regs);
> > +}
> > +
> >   PLATFORM_START(xilinx_zynqmp, "Xilinx ZynqMP")
> >       .compatible = zynqmp_dt_compat,
> > +    .smc = zynqmp_smc,
> >       .blacklist_dev = zynqmp_blacklist_dev,
> >   PLATFORM_END
> >   diff --git a/xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h
> > b/xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h
> > new file mode 100644
> > index 0000000..43cefb5
> > --- /dev/null
> > +++ b/xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h
> > @@ -0,0 +1,30 @@
> > +/*
> > + * Copyright (c) 2018 Xilinx Inc.
> > + *
> > + * 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.
> > + */
> > +
> > +#ifndef __ASM_ARM_PLATFORMS_ZYNQMP_H
> > +#define __ASM_ASM_PLATFORMS_ZYNQMP_H
> > +
> > +#include <asm/processor.h>
> > +
> > +extern bool zynqmp_eemi(struct cpu_user_regs *regs);
> > +
> > +#endif /* __ASM_ARM_PLATFORMS_ZYNQMP_H */
> > +
> > +/*
> > + * Local variables:
> > + * mode: C
> > + * c-file-style: "BSD"
> > + * c-basic-offset: 4
> > + * indent-tabs-mode: nil
> > + * End:
> > + */
> > 
> 
> Cheers,
> 
> -- 
> Julien Grall
> 

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