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

[Julien Grall <julien@xxxxxxx>]

Hi Bertrand,

On 18/08/2022 14:44, Bertrand Marquis wrote:
> > 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.
IMHO, this is better to add it here rather than in a FFA specific patch. 
Otherwise, one could raise the question of why we are adding wrapper 
when they are not used?
> > [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
>
> > +/*
> > + * 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.
The comment is correct. x19 is one of the few registers that will not be 
clobbered by the SMC call. But we still need a register below to store 
'args', so we need to free it (what you call clobber).
> > +    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).
The next instruction will overwrite x0. So if you don't save 'x0' to 
'x19' then you will not be able to load the rest of the registers.
> > +
> > +    /* 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.
AFAIU, this would mean an extra instruction to increment sp by 8 
(covering the xzr register).
Cheers,

--
Julien Grall