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

[Julien Grall <julien.grall@xxxxxxx>]

Hi,

On 24/08/18 00:56, Stefano Stabellini wrote:
> 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.
I was mostly speaking about smccc_ver. It feels strange to always check 
the SMCCC version when calling SMC as the value will never change.
I can see two solutions here:
        1) Provide a different .smc callback depending on the SMCCC version
        2) Provide wrappers to either call SMCCv1.1 or SMCCCv1.0

> > > +
> > > +    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?
Technically any file require the copyright. This was not followed in the 
the header side until recently.
Cheers,

--
Julien Grall

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