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

[Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx>]

Hi,

On 27.08.18 18:29, Julien Grall wrote:
> 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.
Ah, yes, true.

I checked how static keys are working. Seems, they use asm goto feature. 
Now I think: can we optimize this more? Something like that:
#define arm_smccc_smc(...)
    do {

            asm goto (ALTERNATIVE(nop",
                                  "b %l[smccc_1_1]",
                                  ARM_SMCCC_1_1)); 

            arm_smccc_1_0_smc(__VA_ARGS__);
            break;
smccc_1_1:
            arm_smccc_1_1_smc(__VA_ARGS__);
   } while ( 0 )

This will save use additional conditional branch.

--
Volodymyr Babchuk

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