Re: [PATCH v6 01/10] arm64: smccc: add support for SMCCCv1.2 x0-x17 registers

Abdellatif El Khlifi
2022-11-08T11:13:52Z

On Mon, Oct 24, 2022 at 04:19:18PM +0200, Jens Wiklander wrote:
> On Thu, Oct 13, 2022 at 11:38:48AM +0100, Abdellatif El Khlifi wrote:
> > add support for x0-x17 registers used by the SMC calls
> >
> > In SMCCC v1.2 [1] arguments are passed in registers x1-x17.
> > Results are returned in x0-x17.
> >
> > This work is inspired from the following kernel commit:
> >
> > arm64: smccc: Add support for SMCCCv1.2 extended input/output registers
> >
> > [1]: https://documentation-service.arm.com/static/5f8edaeff86e16515cdbe4c6?token=
> >
> > Signed-off-by: Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
> > Cc: Tom Rini <trini@konsulko.com>
> > Cc: Simon Glass <sjg@chromium.org>
> > Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > Cc: Jens Wiklander <jens.wiklander@linaro.org>
> >
> > ---
> >
> > Changelog:
> > ===============
> >
> > v4:
> >
> > * rename the commit title and improve description
> > new commit title: the current
> >
> > v3:
> >
> > * port x0-x17 registers support from linux kernel as defined by SMCCCv1.2
> > commit title:
> > arm64: smccc: add Xn registers support used by SMC calls
> >
> > arch/arm/cpu/armv8/smccc-call.S | 53 +++++++++++++++++++++++++++++++++
> > arch/arm/lib/asm-offsets.c | 14 +++++++++
> > include/linux/arm-smccc.h | 43 ++++++++++++++++++++++++++
> > 3 files changed, 110 insertions(+)
> >
> > diff --git a/arch/arm/cpu/armv8/smccc-call.S b/arch/arm/cpu/armv8/smccc-call.S
> > index dc92b28777..ec6f299bc9 100644
> > --- a/arch/arm/cpu/armv8/smccc-call.S
> > +++ b/arch/arm/cpu/armv8/smccc-call.S
> > @@ -1,6 +1,8 @@
> > /* SPDX-License-Identifier: GPL-2.0 */
> > /*
> > * Copyright (c) 2015, Linaro Limited
> > + * (C) Copyright 2022 ARM Limited
> > + * Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
> > */
> > #include <linux/linkage.h>
> > #include <linux/arm-smccc.h>
> > @@ -45,3 +47,54 @@ ENDPROC(__arm_smccc_smc)
> > ENTRY(__arm_smccc_hvc)
> > SMCCC hvc
> > ENDPROC(__arm_smccc_hvc)
> > +
> > +#ifdef CONFIG_ARM64
> > +
> > + .macro SMCCC_1_2 instr
> > + /* 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, #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]
> > +
> > + \instr #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
> > + ret
> > + .endm
> > +
> > +/*
> > + * 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)
> > + SMCCC_1_2 smc
> > +ENDPROC(arm_smccc_1_2_smc)
> > +
> > +#endif
> > diff --git a/arch/arm/lib/asm-offsets.c b/arch/arm/lib/asm-offsets.c
> > index 22fd541f9a..1bc2d90faa 100644
> > --- a/arch/arm/lib/asm-offsets.c
> > +++ b/arch/arm/lib/asm-offsets.c
> > @@ -9,6 +9,9 @@
> > * generate asm statements containing #defines,
> > * compile this file to assembler, and then extract the
> > * #defines from the assembly-language output.
> > + *
> > + * (C) Copyright 2022 ARM Limited
> > + * Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
> > */
> >
> > #include <common.h>
> > @@ -117,6 +120,17 @@ int main(void)
> > DEFINE(ARM_SMCCC_RES_X2_OFFS, offsetof(struct arm_smccc_res, a2));
> > DEFINE(ARM_SMCCC_QUIRK_ID_OFFS, offsetof(struct arm_smccc_quirk, id));
> > DEFINE(ARM_SMCCC_QUIRK_STATE_OFFS, offsetof(struct arm_smccc_quirk, state));
> > + #ifdef CONFIG_ARM64
> > + DEFINE(ARM_SMCCC_1_2_REGS_X0_OFFS, offsetof(struct arm_smccc_1_2_regs, a0));
> > + DEFINE(ARM_SMCCC_1_2_REGS_X2_OFFS, offsetof(struct arm_smccc_1_2_regs, a2));
> > + DEFINE(ARM_SMCCC_1_2_REGS_X4_OFFS, offsetof(struct arm_smccc_1_2_regs, a4));
> > + DEFINE(ARM_SMCCC_1_2_REGS_X6_OFFS, offsetof(struct arm_smccc_1_2_regs, a6));
> > + DEFINE(ARM_SMCCC_1_2_REGS_X8_OFFS, offsetof(struct arm_smccc_1_2_regs, a8));
> > + DEFINE(ARM_SMCCC_1_2_REGS_X10_OFFS, offsetof(struct arm_smccc_1_2_regs, a10));
> > + DEFINE(ARM_SMCCC_1_2_REGS_X12_OFFS, offsetof(struct arm_smccc_1_2_regs, a12));
> > + DEFINE(ARM_SMCCC_1_2_REGS_X14_OFFS, offsetof(struct arm_smccc_1_2_regs, a14));
> > + DEFINE(ARM_SMCCC_1_2_REGS_X16_OFFS, offsetof(struct arm_smccc_1_2_regs, a16));
> > + #endif
>
> We don't need another level of indentation here, please follow the
> established style in this file.
>
> With that fixed please add
> Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>

Thanks, done in v7.

>
> Cheers,
> Jens
>
> > #endif
> >
> > return 0;
> > diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h
> > index e1d09884a1..9105031d55 100644
> > --- a/include/linux/arm-smccc.h
> > +++ b/include/linux/arm-smccc.h
> > @@ -1,6 +1,8 @@
> > /* SPDX-License-Identifier: GPL-2.0 */
> > /*
> > * Copyright (c) 2015, Linaro Limited
> > + * (C) Copyright 2022 ARM Limited
> > + * Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
> > */
> > #ifndef __LINUX_ARM_SMCCC_H
> > #define __LINUX_ARM_SMCCC_H
> > @@ -70,6 +72,47 @@ struct arm_smccc_res {
> > unsigned long a3;
> > };
> >
> > +#ifdef CONFIG_ARM64
> > +/**
> > + * 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.
> > + */
> > +asmlinkage void arm_smccc_1_2_smc(const struct arm_smccc_1_2_regs *args,
> > + struct arm_smccc_1_2_regs *res);
> > +#endif
> > +
> > /**
> > * struct arm_smccc_quirk - Contains quirk information
> > * @id: quirk identification
> > --
> > 2.17.1
> >