Hi Julien,
> On 18 Aug 2022, at 15:31, Julien Grall <jul...@xen.org> wrote:
>
> Hi Bertrand,
>
> On 18/08/2022 14:44, Bertrand Marquis wrote:
>>> On 18 Aug 2022, at 11:55, Jens Wiklander <jens.wiklan...@linaro.org> 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?
I was more thinking of pushing this until we have something compatible with 1.2
but I get the argument so ok.
>
>>>
>>> [1] https://developer.arm.com/documentation/den0028/c/?lang=en
>>>
>>> Reviewed-by: Luca Fancellu <luca.fance...@arm.com>
>>> Signed-off-by: Jens Wiklander <jens.wiklan...@linaro.org>
>>> ---
>>> 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).
Adding “by SMC call" would make this more clear
>
>>> + 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.
>
Right
>>> +
>>> + /* 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).
Right instruction is also incrementing sp, my bad.
Cheers
Bertrand
>
> Cheers,
>
> --
> Julien Grall
>
