Re: [Xen-devel] [PATCH 5/6] xen/arm: smccc: Add wrapper to automatically select the calling convention

[Julien Grall <julien.grall@xxxxxxx>]

On 27/08/2018 15:15, Volodymyr Babchuk wrote:
> Hi Julien,
Hi,

> On 24.08.18 19:58, Julien Grall wrote:
> > Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
> > ---
> >   xen/arch/arm/psci.c              | 4 ++++
> >   xen/include/asm-arm/cpufeature.h | 3 ++-
> >   xen/include/asm-arm/smccc.h      | 8 ++++++++
> >   3 files changed, 14 insertions(+), 1 deletion(-)
> >
> > diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c
> > index 3cf5ecf0f3..941eec921b 100644
> > --- a/xen/arch/arm/psci.c
> > +++ b/xen/arch/arm/psci.c
> > @@ -21,6 +21,7 @@
> >   #include <xen/types.h>
> >   #include <xen/mm.h>
> >   #include <xen/smp.h>
> > +#include <asm/cpufeature.h>
> >   #include <asm/psci.h>
> >   #include <asm/acpi.h>
> > @@ -118,6 +119,9 @@ static void __init psci_init_smccc(void)
> >               smccc_ver = ret;
> >       }
> > +    if ( smccc_ver >= SMCCC_VERSION(1, 1) )
> > +        cpus_set_cap(ARM_SMCCC_1_1);
> > +
> >       printk(XENLOG_INFO "Using SMC Calling Convention v%u.%u\n",
> >              SMCCC_VERSION_MAJOR(smccc_ver), 
> > SMCCC_VERSION_MINOR(smccc_ver));
> >   }
> > diff --git a/xen/include/asm-arm/cpufeature.h 
> > b/xen/include/asm-arm/cpufeature.h
> > index 9c297c521c..c9c4046f5f 100644
> > --- a/xen/include/asm-arm/cpufeature.h
> > +++ b/xen/include/asm-arm/cpufeature.h
> > @@ -44,8 +44,9 @@
> >   #define SKIP_CTXT_SWITCH_SERROR_SYNC 6
> >   #define ARM_HARDEN_BRANCH_PREDICTOR 7
> >   #define ARM_SSBD 8
> > +#define ARM_SMCCC_1_1 9
> > -#define ARM_NCAPS           9
> > +#define ARM_NCAPS           10
> >   #ifndef __ASSEMBLY__
> > diff --git a/xen/include/asm-arm/smccc.h b/xen/include/asm-arm/smccc.h
> > index 1ed6cbaa48..7c39c530e2 100644
> > --- a/xen/include/asm-arm/smccc.h
> > +++ b/xen/include/asm-arm/smccc.h
> > @@ -213,6 +213,7 @@ struct arm_smccc_res {
> >    */
> >   #ifdef CONFIG_ARM_32
> >   #define arm_smccc_1_0_smc(...) arm_smccc_1_1_smc(__VA_ARGS__)
> > +#define arm_smccc_smc(...) arm_smccc_1_1_smc(__VA_ARGS__)
> >   #else
> >   void __arm_smccc_1_0_smc(register_t a0, register_t a1, register_t a2,
> > @@ -254,6 +255,13 @@ void __arm_smccc_1_0_smc(register_t a0, 
> > register_t a1, register_t a2,
> >   #define 
> > arm_smccc_1_0_smc(...)                                              \
> >           __arm_smccc_1_0_smc_count(__count_args(__VA_ARGS__), 
> > __VA_ARGS__)
> > +#define arm_smccc_smc(...)                                      \
> > +    do {                                                        \
> > +        if ( !cpus_have_const_cap(ARM_SMCCC_1_1) )              \
> > +            arm_smccc_1_0_smc(__VA_ARGS__);                     \
> > +        else                                                    \
> > +            arm_smccc_1_1_smc(__VA_ARGS__);                     \
> > +    } while ( 0 )
> >   #endif /* CONFIG_ARM_64 */
> This will generate lots of code for every arm_smccc_smc(). Can we have 
> function pointer arm_smccc_smc instead and assign it to either 
> arm_smccc_1_1_smc() or arm_asmccc_1_0_smc() at boot?
> I know that currently we have no function arm_smccc_1_1_smc() because it 
> is being constructed inline every time. But we can write it explicitly 
> and then have one indirect call to (maybe cached) function instead of
> lots inlined code with conditional branches.
This is indeed increasing the size of the function, but with a better 
performance when you perform an SMC with 1.1.
The main goal of SMCCC 1.1 is to limit the number of registers saved 
when calling an SMC. So if you provide provide a wrapper using a 
function, then you are better off sticking with SMCCC 1.0.
The idea of this code is to provide a faster call on the presence of 
SMCCC 1.1 while providing a fallback for other case. The function 
cpus_have_const_cap is implemented using an ALTERNATIVE (similar to 
static key) that will make if close to a NOP. I am saying close because 
this is not quite a static key as we don't have it in Xen. Instead, you 
replace a memory load by a constant.
Cheers,

--
Julien Grall

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