|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v2 1/2] xen/arm: smccc: add support for SMCCCv1.2 extended input/output registers
On Fri, Jun 10, 2022 at 05:41:33PM -0700, Stefano Stabellini wrote:
> On Thu, 9 Jun 2022, Jens Wiklander wrote:
> > SMCCC v1.2 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
> >
> > Signed-off-by: Jens Wiklander <jens.wiklander@xxxxxxxxxx>
> > ---
> > xen/arch/arm/arm64/smc.S | 43 ++++++++++++++++++++++++++++++++
> > xen/arch/arm/include/asm/smccc.h | 42 +++++++++++++++++++++++++++++++
> > xen/arch/arm/vsmc.c | 2 +-
> > 3 files changed, 86 insertions(+), 1 deletion(-)
> >
> > diff --git a/xen/arch/arm/arm64/smc.S b/xen/arch/arm/arm64/smc.S
> > index 91bae62dd4d2..1570bc8eb9d4 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
> > +
> > +
> > +/*
> > + * 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 */
> > + stp x1, x19, [sp, #-16]!
> > +
> > + /* Ensure `args` won't be clobbered while loading regs in next step */
> > + mov x19, x0
> > +
> > + /* Load the registers x0 - x17 from the struct arm_smccc_1_2_regs */
> > + ldp x0, x1, [x19, #0]
> > + ldp x2, x3, [x19, #16]
> > + ldp x4, x5, [x19, #32]
> > + ldp x6, x7, [x19, #48]
> > + ldp x8, x9, [x19, #64]
> > + ldp x10, x11, [x19, #80]
> > + ldp x12, x13, [x19, #96]
> > + ldp x14, x15, [x19, #112]
> > + ldp x16, x17, [x19, #128]
> > +
> > + smc #0
> > +
> > + /* Load the `res` from the stack */
> > + ldr x19, [sp]
> > +
> > + /* Store the registers x0 - x17 into the result structure */
> > + stp x0, x1, [x19, #0]
> > + stp x2, x3, [x19, #16]
> > + stp x4, x5, [x19, #32]
> > + stp x6, x7, [x19, #48]
> > + stp x8, x9, [x19, #64]
> > + stp x10, x11, [x19, #80]
> > + stp x12, x13, [x19, #96]
> > + stp x14, x15, [x19, #112]
> > + stp x16, x17, [x19, #128]
>
> I noticed that in the original commit the offsets are declared as
> ARM_SMCCC_1_2_REGS_X0_OFFS, etc. In Xen we could add them to
> xen/arch/arm/arm64/asm-offsets.c given that they are only used in asm.
>
> That said, there isn't a huge value in declaring them given that they
> are always read and written in order and there is nothing else in the
> struct, so I am fine either way.
>
> I am also happy to have them declared if other maintainers prefer it
> that way.
OK, I'll update with asm-offsets.c since Julien asked for that too.
>
>
> > + /* Restore original x19 */
> > + ldp xzr, x19, [sp], #16
> > + ret
> > diff --git a/xen/arch/arm/include/asm/smccc.h
> > b/xen/arch/arm/include/asm/smccc.h
> > index b3dbeecc90ad..316adf968e74 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
> > @@ -217,6 +218,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
>
> Spurious change
>
>
> > void __arm_smccc_1_0_smc(register_t a0, register_t a1, register_t a2,
> > @@ -265,8 +267,48 @@ 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;
> > +};
> > #endif /* CONFIG_ARM_64 */
> >
> > +/**
> > + * 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);
> > +
>
> As arm_smccc_1_2_smc is not implemented in ARM32 it is better to place
> the declaration inside the #ifdef CONFIG_ARM_64.
I'll fix.
>
>
> > #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);
> > return true;
>
> This is going to be a problem for ARM32 given that ARM_SMCCC_VERSION_1_2
> is unimplemented on ARM32. If there is an ARM32 implementation in Linux
> for ARM_SMCCC_VERSION_1_2 you might as well import it too.
>
> Otherwise we'll have to abstract it away, e.g.:
>
> #ifdef CONFIG_ARM_64
> #define ARM_VSMCCC_VERSION ARM_SMCCC_VERSION_1_2
> #else
> #define ARM_VSMCCC_VERSION ARM_SMCCC_VERSION_1_1
> #endif
I couldn't find an ARM32 implementation for ARM_SMCCC_VERSION_1_2. But
I'm not sure it's needed at this point. From what I've understood r4-17
are either preserved or updated by the function ID in question. So
claiming ARM_SMCCC_VERSION_1_2 shouldn't break anything. The FF-A
functions will mostly update r4-r7, but we don't use FF-A with ARM32
yet. I'l update with you're proposal if that's what you prefer.
Thanks,
Jens
>
> > case ARM_SMCCC_ARCH_FEATURES_FID:
> > --
> > 2.31.1
> >
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |