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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 23 Aug 2018, Julien Grall wrote:
> Hi Stefano,
> 
> 2018 01:01 AM, Stefano Stabellini wrote:
> > From: "Edgar E. Iglesias" <edgar.iglesias@xxxxxxxxxx>
> > 
> > From: Edgar E. Iglesias <edgar.iglesias@xxxxxxxxxx>
> > 
> > Introduce zynqmp_eemi: a function resposible for implementing access
> 
> s/resposible/responsible/

I'll fix


> > 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>
> > ---
> >   xen/arch/arm/platforms/Makefile                    |  1 +
> >   xen/arch/arm/platforms/xilinx-zynqmp-eemi.c        | 38
> > ++++++++++++++++++++++
> >   xen/arch/arm/platforms/xilinx-zynqmp.c             | 14 ++++++++
> >   xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h |  3 ++
> >   4 files changed, 56 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 80e555c..703f915 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_ARM_64) += xilinx-zynqmp.o
> > +obj-$(CONFIG_ARM_64) += 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..c3a19e9
> > --- /dev/null
> > +++ b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
> > @@ -0,0 +1,38 @@
> > +/*
> > + * 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 <xen/iocap.h>
> > +#include <xen/sched.h>
> > +#include <xen/types.h>
> > +#include <asm/smccc.h>
> > +#include <asm/regs.h>
> > +#include <asm/platforms/xilinx-zynqmp-eemi.h>
> 
> Can you please introduce headers when they are actually used?

I'll do


> > +
> > +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..c318cf5 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)
> > +{
> > +    if ( !is_64bit_domain(current->domain) )
> > +        return false;
> > +    /* Only support platforms with SMCCC >= 1.1 */
> > +    if ( smccc_ver < SMCCC_VERSION(1, 1) )
> > +        return false;
> 
> Do you really need to call this at every SMC call?

Do you mean that we could skip the checks? We could do the checks at
init time, but then we would have to memorize the result for each
domain. Not sure it is worth it.


> > +
> > +    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..6630dc8
> > --- /dev/null
> > +++ b/xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h
> > @@ -0,0 +1,3 @@
> 
> Missing copyright headers and guard.

The guard is missing, I'll add it. Regarding the copyright, this is an
header file, none of the others in the same directory have the copyright
header -- are you sure we need it?


> > +#include <asm/processor.h>
> > +
> > +extern bool zynqmp_eemi(struct cpu_user_regs *regs);
> > 
> 
> Missing emacs magics.

Yeah.. Good point. One of these days I'll send a xen.git wide series to
remove it for everywhere :-D

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