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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 24 Aug 2018, Julien Grall wrote:
> > > > +
> > > > +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

I think we can assume SMCCCv1.1, we don't need to support SMCCCv1.0 with
EEMI. I'll remove the check.


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

I'll add the copyright notice

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