Re: [PATCH v5 1/9] xen/arm: smccc: add support for SMCCCv1.2 extended input/output registers

[Jens Wiklander <jens.wiklander@xxxxxxxxxx>]

On Thu, Aug 18, 2022 at 3:45 PM Bertrand Marquis
<Bertrand.Marquis@xxxxxxx> wrote:
>
> Hi Jens,
>
> > On 18 Aug 2022, at 11:55, Jens Wiklander <jens.wiklander@xxxxxxxxxx> wrote:
> >
> > SMCCC v1.2 [1] AArch64 allows x0-x17 to be used as both parameter
> > registers and result registers for the SMC and HVC instructions.
> >
> > Arm Firmware Framework for Armv8-A specification makes use of x0-x7 as
> > parameter and result registers.
> >
> > Let us add new interface to support this extended set of input/output
> > registers.
> >
> > This is based on 3fdc0cb59d97 ("arm64: smccc: Add support for SMCCCv1.2
> > extended input/output registers") by Sudeep Holla from the Linux kernel
> >
> > The SMCCC version reported to the VM is bumped to 1.2 in order to support
> > handling FF-A messages.
>
> With this patch, you add something so that you could call SMCCCv1.2 but in 
> practice you are not using it anywhere.
> I do not think this patch should bump the version we present to guests.
>
> >
> > [1] https://developer.arm.com/documentation/den0028/c/?lang=en
> >
> > Reviewed-by: Luca Fancellu <luca.fancellu@xxxxxxx>
> > Signed-off-by: Jens Wiklander <jens.wiklander@xxxxxxxxxx>
> > ---
> > xen/arch/arm/arm64/asm-offsets.c |  9 +++++++
> > xen/arch/arm/arm64/smc.S         | 43 ++++++++++++++++++++++++++++++++
> > xen/arch/arm/include/asm/smccc.h | 40 +++++++++++++++++++++++++++++
> > xen/arch/arm/vsmc.c              |  2 +-
> > 4 files changed, 93 insertions(+), 1 deletion(-)
> >
> > diff --git a/xen/arch/arm/arm64/asm-offsets.c 
> > b/xen/arch/arm/arm64/asm-offsets.c
> > index 280ddb55bfd4..1721e1ed26e1 100644
> > --- a/xen/arch/arm/arm64/asm-offsets.c
> > +++ b/xen/arch/arm/arm64/asm-offsets.c
> > @@ -56,6 +56,15 @@ void __dummy__(void)
> >    BLANK();
> >    OFFSET(SMCCC_RES_a0, struct arm_smccc_res, a0);
> >    OFFSET(SMCCC_RES_a2, struct arm_smccc_res, a2);
> > +   OFFSET(ARM_SMCCC_1_2_REGS_X0_OFFS, struct arm_smccc_1_2_regs, a0);
> > +   OFFSET(ARM_SMCCC_1_2_REGS_X2_OFFS, struct arm_smccc_1_2_regs, a2);
> > +   OFFSET(ARM_SMCCC_1_2_REGS_X4_OFFS, struct arm_smccc_1_2_regs, a4);
> > +   OFFSET(ARM_SMCCC_1_2_REGS_X6_OFFS, struct arm_smccc_1_2_regs, a6);
> > +   OFFSET(ARM_SMCCC_1_2_REGS_X8_OFFS, struct arm_smccc_1_2_regs, a8);
> > +   OFFSET(ARM_SMCCC_1_2_REGS_X10_OFFS, struct arm_smccc_1_2_regs, a10);
> > +   OFFSET(ARM_SMCCC_1_2_REGS_X12_OFFS, struct arm_smccc_1_2_regs, a12);
> > +   OFFSET(ARM_SMCCC_1_2_REGS_X14_OFFS, struct arm_smccc_1_2_regs, a14);
> > +   OFFSET(ARM_SMCCC_1_2_REGS_X16_OFFS, struct arm_smccc_1_2_regs, a16);
> > }
> >
> > /*
> > diff --git a/xen/arch/arm/arm64/smc.S b/xen/arch/arm/arm64/smc.S
> > index 91bae62dd4d2..c546192e7f2d 100644
> > --- a/xen/arch/arm/arm64/smc.S
> > +++ b/xen/arch/arm/arm64/smc.S
> > @@ -27,3 +27,46 @@ ENTRY(__arm_smccc_1_0_smc)
> >         stp     x2, x3, [x4, #SMCCC_RES_a2]
> > 1:
> >         ret
> > +
> > +
>
> Please only add one line only here

OK, I'll fix.

Thanks,
Jens

>
> > +/*
> > + * void arm_smccc_1_2_smc(const struct arm_smccc_1_2_regs *args,
> > + *                        struct arm_smccc_1_2_regs *res)
> > + */
> > +ENTRY(arm_smccc_1_2_smc)
> > +    /* Save `res` and free a GPR that won't be clobbered */
>
> The comment here should be fixed, you are clobbering x19 hence you need to 
> save it.
>
> > +    stp     x1, x19, [sp, #-16]!
> > +
> > +    /* Ensure `args` won't be clobbered while loading regs in next step */
> > +    mov      x19, x0
>
> You do not need to save args (and no code is restoring it).
>
> > +
> > +    /* Load the registers x0 - x17 from the struct arm_smccc_1_2_regs */
> > +    ldp      x0, x1, [x19, #ARM_SMCCC_1_2_REGS_X0_OFFS]
> > +    ldp      x2, x3, [x19, #ARM_SMCCC_1_2_REGS_X2_OFFS]
> > +    ldp      x4, x5, [x19, #ARM_SMCCC_1_2_REGS_X4_OFFS]
> > +    ldp      x6, x7, [x19, #ARM_SMCCC_1_2_REGS_X6_OFFS]
> > +    ldp      x8, x9, [x19, #ARM_SMCCC_1_2_REGS_X8_OFFS]
> > +    ldp      x10, x11, [x19, #ARM_SMCCC_1_2_REGS_X10_OFFS]
> > +    ldp      x12, x13, [x19, #ARM_SMCCC_1_2_REGS_X12_OFFS]
> > +    ldp      x14, x15, [x19, #ARM_SMCCC_1_2_REGS_X14_OFFS]
> > +    ldp      x16, x17, [x19, #ARM_SMCCC_1_2_REGS_X16_OFFS]
> > +
> > +    smc #0
> > +
> > +    /* Load the `res` from the stack */
> > +    ldr      x19, [sp]
> > +
> > +    /* Store the registers x0 - x17 into the result structure */
> > +    stp      x0, x1, [x19, #ARM_SMCCC_1_2_REGS_X0_OFFS]
> > +    stp      x2, x3, [x19, #ARM_SMCCC_1_2_REGS_X2_OFFS]
> > +    stp      x4, x5, [x19, #ARM_SMCCC_1_2_REGS_X4_OFFS]
> > +    stp      x6, x7, [x19, #ARM_SMCCC_1_2_REGS_X6_OFFS]
> > +    stp      x8, x9, [x19, #ARM_SMCCC_1_2_REGS_X8_OFFS]
> > +    stp      x10, x11, [x19, #ARM_SMCCC_1_2_REGS_X10_OFFS]
> > +    stp      x12, x13, [x19, #ARM_SMCCC_1_2_REGS_X12_OFFS]
> > +    stp      x14, x15, [x19, #ARM_SMCCC_1_2_REGS_X14_OFFS]
> > +    stp      x16, x17, [x19, #ARM_SMCCC_1_2_REGS_X16_OFFS]
> > +
> > +    /* Restore original x19 */
> > +    ldp     xzr, x19, [sp], #16
>
> You should use ldr and just load x19 value here.
>
> > +    ret
> > diff --git a/xen/arch/arm/include/asm/smccc.h 
> > b/xen/arch/arm/include/asm/smccc.h
> > index b3dbeecc90ad..b5e3f67eb34e 100644
> > --- a/xen/arch/arm/include/asm/smccc.h
> > +++ b/xen/arch/arm/include/asm/smccc.h
> > @@ -33,6 +33,7 @@
> >
> > #define ARM_SMCCC_VERSION_1_0   SMCCC_VERSION(1, 0)
> > #define ARM_SMCCC_VERSION_1_1   SMCCC_VERSION(1, 1)
> > +#define ARM_SMCCC_VERSION_1_2   SMCCC_VERSION(1, 2)
> >
> > /*
> >  * This file provides common defines for ARM SMC Calling Convention as
> > @@ -265,6 +266,45 @@ void __arm_smccc_1_0_smc(register_t a0, register_t a1, 
> > register_t a2,
> >         else                                                    \
> >             arm_smccc_1_0_smc(__VA_ARGS__);                     \
> >     } while ( 0 )
> > +
> > +/**
> > + * struct arm_smccc_1_2_regs - Arguments for or Results from SMC call
> > + * @a0-a17 argument values from registers 0 to 17
> > + */
> > +struct arm_smccc_1_2_regs {
> > +    unsigned long a0;
> > +    unsigned long a1;
> > +    unsigned long a2;
> > +    unsigned long a3;
> > +    unsigned long a4;
> > +    unsigned long a5;
> > +    unsigned long a6;
> > +    unsigned long a7;
> > +    unsigned long a8;
> > +    unsigned long a9;
> > +    unsigned long a10;
> > +    unsigned long a11;
> > +    unsigned long a12;
> > +    unsigned long a13;
> > +    unsigned long a14;
> > +    unsigned long a15;
> > +    unsigned long a16;
> > +    unsigned long a17;
> > +};
> > +
> > +/**
> > + * arm_smccc_1_2_smc() - make SMC calls
> > + * @args: arguments passed via struct arm_smccc_1_2_regs
> > + * @res: result values via struct arm_smccc_1_2_regs
> > + *
> > + * This function is used to make SMC calls following SMC Calling Convention
> > + * v1.2 or above. The content of the supplied param are copied from the
> > + * structure to registers prior to the SMC instruction. The return values
> > + * are updated with the content from registers on return from the SMC
> > + * instruction.
> > + */
> > +void arm_smccc_1_2_smc(const struct arm_smccc_1_2_regs *args,
> > +                       struct arm_smccc_1_2_regs *res);
> > #endif /* CONFIG_ARM_64 */
> >
> > #endif /* __ASSEMBLY__ */
> > diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c
> > index 676740ef1520..6f90c08a6304 100644
> > --- a/xen/arch/arm/vsmc.c
> > +++ b/xen/arch/arm/vsmc.c
> > @@ -93,7 +93,7 @@ static bool handle_arch(struct cpu_user_regs *regs)
> >     switch ( fid )
> >     {
> >     case ARM_SMCCC_VERSION_FID:
> > -        set_user_reg(regs, 0, ARM_SMCCC_VERSION_1_1);
> > +        set_user_reg(regs, 0, ARM_SMCCC_VERSION_1_2);
>
> As said for the commit message, I do not see what changes are making Xen 
> providing 1.2 interface at this stage.
>
> Regards
> Bertrand
>
> >         return true;
> >
> >     case ARM_SMCCC_ARCH_FEATURES_FID:
> > --
> > 2.31.1
> >
>