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@linaro.org>
---
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]
+
+ /* 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
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);
+
+
#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;
case ARM_SMCCC_ARCH_FEATURES_FID:
--
2.31.1On 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@linaro.org>
> ---
> 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.
> + /* 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.
> #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
> case ARM_SMCCC_ARCH_FEATURES_FID:
> --
> 2.31.1
>
Hi Stefano,
On 11/06/2022 01:41, Stefano Stabellini wrote:
>> #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 don't understand why you want to tie the virtual and host SMCCC version.
In theory, it would be possible for us to implement a subsystem to fully
emulate, lets say, FFA. We would to tell the VM that we are v1.2
compliant but we would not need the helper as no calls would be forwarded.
When a 32-bit guest is running on Xen Arm64, we are going to say that
SMCCC v1.2 will be available. This is not much different from running a
32-bit guest on 32-bit hardware. So I think we should expose 1.2 unless
we think there is a problem in the Xen 32-bit specific code.
Cheers,
--
Julien Grall
On Wed, 15 Jun 2022, Julien Grall wrote:
> On 11/06/2022 01:41, Stefano Stabellini wrote:
> > > #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 don't understand why you want to tie the virtual and host SMCCC version.
>
> In theory, it would be possible for us to implement a subsystem to fully
> emulate, lets say, FFA. We would to tell the VM that we are v1.2 compliant but
> we would not need the helper as no calls would be forwarded.
>
> When a 32-bit guest is running on Xen Arm64, we are going to say that SMCCC
> v1.2 will be available. This is not much different from running a 32-bit guest
> on 32-bit hardware.
In a few places (especially platform specific code, such as Xilinx EEMI)
the guest SMC call traps in Xen, then Xen repeats the same SMC call to
the firmware.
I realize this is not a good reason to keep virtual SMCCC 1.1 because
the firmware could support SMCCC 1.0 or older. Some argument conversions
are to be expected. In reality I have been working with SMCCC 1.1
virtual and SMCCC 1.1 firmware for a long time so the problem didn't
exist, and I didn't really think it through :-)
> So I think we should expose 1.2 unless we think there is a
> problem in the Xen 32-bit specific code.
Yeah, I am OK with that.
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@linaro.org>
> > ---
> > 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
> >
Hi,
On 15/06/2022 16:58, Jens Wiklander wrote:
> On Fri, Jun 10, 2022 at 05:41:33PM -0700, Stefano Stabellini wrote:
>>> #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.
So in Xen, we always take a snapshot of the registers on entry to the
hypervisor and only touch it when necessary. Therefore, it doesn't
matter whether we claim to be complaient with 1.1 or 1.2 based on the
argument passing convention.
However, the spec is not only about arguments. For instance, SMCCC v1.1
also added some mandatory functions (e.g. detection the version). I
haven't looked closely on whether the SMCCC v1.2 introduced such thing.
Can you confirm what mandatory feature comes with 1.2?
Furthermore, your commit message explain why arm_smccc_1_2_smc() was
introduced. But it seems to miss some words about exposing SMCCC v2.1 to
the VM.
In general, I think it is better to split the host support from the VM
support. The two are technically not independent (caller vs
implementation) and there are different problematics for each (see above
for an example). I don't think there are a lot to add here, so I would
be OK to keep it in the same patch with a few words.
Lastly, can you provide a link to the spec in the commit message? This
would help to find the pdf easily. I think in this case, you are using
ARM DEN 0028C (this is not the latest but describe 1.2):
https://developer.arm.com/documentation/den0028/c/?lang=en
Cheers,
--
Julien Grall
On Wed, Jun 15, 2022 at 08:01:28PM +0100, Julien Grall wrote:
> Hi,
>
> On 15/06/2022 16:58, Jens Wiklander wrote:
> > On Fri, Jun 10, 2022 at 05:41:33PM -0700, Stefano Stabellini wrote:
> > > > #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.
>
> So in Xen, we always take a snapshot of the registers on entry to the
> hypervisor and only touch it when necessary. Therefore, it doesn't matter
> whether we claim to be complaient with 1.1 or 1.2 based on the argument
> passing convention.
>
> However, the spec is not only about arguments. For instance, SMCCC v1.1 also
> added some mandatory functions (e.g. detection the version). I haven't
> looked closely on whether the SMCCC v1.2 introduced such thing. Can you
> confirm what mandatory feature comes with 1.2?
There's a nice summary in a table at the end of the C version of DEN0028
you linked below. For SMCCC v1.2:
Argument/Result register set:
Permits calls to use R4—R7 as return register (Section 4.1).
Permits calls to use X4—X17 as return registers(Section3.1).
Permits calls to use X8—X17 as argument registers (Section 3.1).
Introduces:
SMCCC_ARCH_SOC_ID (Section 7.4)
Deprecates:
UID, Revision Queries on Arm Architecture Service (Section 6.2)
Count Query on all services (Section 6.2)
As far a I can tell nothing mandatory is introduced with version 1.2.
>
> Furthermore, your commit message explain why arm_smccc_1_2_smc() was
> introduced. But it seems to miss some words about exposing SMCCC v2.1 to the
> VM.
>
> In general, I think it is better to split the host support from the VM
> support. The two are technically not independent (caller vs implementation)
> and there are different problematics for each (see above for an example). I
> don't think there are a lot to add here, so I would be OK to keep it in the
> same patch with a few words.
>
> Lastly, can you provide a link to the spec in the commit message? This would
> help to find the pdf easily. I think in this case, you are using
> ARM DEN 0028C (this is not the latest but describe 1.2):
>
> https://developer.arm.com/documentation/den0028/c/?lang=en
I'll update the commit message.
Thanks,
Jens
Hi Jens,
On 15/06/2022 23:09, Jens Wiklander wrote:
> On Wed, Jun 15, 2022 at 08:01:28PM +0100, Julien Grall wrote:
>> Hi,
>>
>> On 15/06/2022 16:58, Jens Wiklander wrote:
>>> On Fri, Jun 10, 2022 at 05:41:33PM -0700, Stefano Stabellini wrote:
>>>>> #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.
>>
>> So in Xen, we always take a snapshot of the registers on entry to the
>> hypervisor and only touch it when necessary. Therefore, it doesn't matter
>> whether we claim to be complaient with 1.1 or 1.2 based on the argument
>> passing convention.
>>
>> However, the spec is not only about arguments. For instance, SMCCC v1.1 also
>> added some mandatory functions (e.g. detection the version). I haven't
>> looked closely on whether the SMCCC v1.2 introduced such thing. Can you
>> confirm what mandatory feature comes with 1.2?
>
> There's a nice summary in a table at the end of the C version of DEN0028
> you linked below. For SMCCC v1.2:
> Argument/Result register set:
> Permits calls to use R4—R7 as return register (Section 4.1).
> Permits calls to use X4—X17 as return registers(Section3.1).
> Permits calls to use X8—X17 as argument registers (Section 3.1).
> Introduces:
> SMCCC_ARCH_SOC_ID (Section 7.4)
> Deprecates:
> UID, Revision Queries on Arm Architecture Service (Section 6.2)
> Count Query on all services (Section 6.2)
Thanks for posting here!
>
> As far a I can tell nothing mandatory is introduced with version 1.2.
Agree. So it is safe to expose 1.2 unconditionally to the VMs.
Cheers,
--
Julien Grall
Hi Stefano, On 11/06/2022 01:41, Stefano Stabellini wrote: > On Thu, 9 Jun 2022, Jens Wiklander wrote: >> + /* 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. While we don't support big-endian in Xen (yet?), the offsets will be inverted. Furthermore, I prefer to avoid open-coded value (in particular when they are related to offset). They are unlikely going to change, but at least we have the compiler that will compute them for us (so less chance for a problem). Cheers, -- Julien Grall
On Sat, 11 Jun 2022, Julien Grall wrote: > Hi Stefano, > > On 11/06/2022 01:41, Stefano Stabellini wrote: > > On Thu, 9 Jun 2022, Jens Wiklander wrote: > > > + /* 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. > > While we don't support big-endian in Xen (yet?), the offsets will be inverted. > > Furthermore, I prefer to avoid open-coded value (in particular when they are > related to offset). They are unlikely going to change, but at least we have > the compiler that will compute them for us (so less chance for a problem). I am OK with that
© 2016 - 2026 Red Hat, Inc.