|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH 4/6] xen/arm: Rewrite arm_smccc_smc() for arm64
Hi Andrew,
> On 31 Aug 2026, at 14:19, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:
>
> PSCI v1.0 says that x4 thru x17 may be clobbered. PSCI v1.1 says they are
> strictly preserved unless they contain return values.
I think the PSCI reference here and in the rest of the commit message and the
patch
should not be used and should be SMCCC references instead (which would also be
more coherent with the fact that this code is not PSCI but SMCCC and is used by
others than PSCI).
PSCI does not define the preservation rules, this is defined in SMCCC spec and
we
have a way to detect the SMCCC version.
SMCCC defines the following rules:
- 1.0: x4-x17 have an unpredictable return state.
- 1.1: x4-x17 must be preserved.
- 1.2: x4-x17 may additionally be defined as result registers, otherwise they
remain preserved.
So the commit message and comments should consistently say SMCCC instead of
PSCI.
Other than this the code is right i think.
Cheers
Bertrand
>
> Xen deals with this by having __arm_smccc_1_0_smc() as an out-of-line
> function, but this causes awful code generation in arm_smccc_smc().
> cpus_have_const_cap() is opaque to the optimiser, so we end up with one basic
> block doing the reasonably-ok arm_smccc_1_1_smc() code generation and a second
> basic block setting up all 8 input registers even when they're not needed,
> spilling or discarding x8 thru x17, and calling an out-of-line function.
>
> Remove __arm_smccc_1_0_smc() entirely, and rewrite arm_smccc_smc() to declare
> x4 thru x17 as clobbered.
>
> This fully inlines the SMC, is a single basic block which instructs the
> compiler to spill or discard the potentially clobbered registers, and only
> sets up the necessary number of arguments for the call.
>
> No functional change.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> CC: Julien Grall <julien@xxxxxxx>
> CC: Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
> CC: Bertrand Marquis <bertrand.marquis@xxxxxxx>
> CC: Michal Orzel <michal.orzel@xxxxxxx>
> CC: Jan Setje-Eilers <Jan.SetjeEilers@xxxxxxxxxx>
>
> Bloat-o-meter reports:
>
> add/remove: 0/1 grow/shrink: 0/10 up/down: 0/-795 (-795)
> Function old new delta
> symbols_sorted_offsets 23832 23824 -8
> symbols_names 42962 42943 -19
> symbols_addresses 35128 35104 -24
> __arm_smccc_1_0_smc 32 - -32
> call_psci_cpu_off 144 76 -68
> seattle_system_reset 92 16 -76
> seattle_system_off 92 16 -76
> call_psci_system_reset 112 32 -80
> call_psci_system_off 112 32 -80
> call_psci_cpu_on 252 124 -128
> psci_init 628 424 -204
>
> An alternative way to do this would be to have x8 thru x17 in the clobber list
> rather than the output list which would reduce the source size, but this form
> is more amenable to having PSCI v1.2 worked into it too.
> ---
> xen/arch/arm/arm64/smc.S | 16 ------
> xen/arch/arm/include/asm/smccc.h | 95 ++++++++++++++++----------------
> 2 files changed, 48 insertions(+), 63 deletions(-)
>
> diff --git a/xen/arch/arm/arm64/smc.S b/xen/arch/arm/arm64/smc.S
> index 68b05e8ddd12..65b4eabe4f87 100644
> --- a/xen/arch/arm/arm64/smc.S
> +++ b/xen/arch/arm/arm64/smc.S
> @@ -13,22 +13,6 @@
> * GNU General Public License for more details.
> */
>
> -/*
> - * void __arm_smccc_1_0_smc(register_t a0, register_t a1, register_t a2,
> - * register_t a3, register_t a4, register_t a5,
> - * register_t a6, register_t a7,
> - * struct arm_smccc_res *res)
> - */
> -FUNC(__arm_smccc_1_0_smc)
> - smc #0
> - ldr x4, [sp]
> - cbz x4, 1f /* No need to store the result */
> - stp x0, x1, [x4, #SMCCC_RES_a0]
> - stp x2, x3, [x4, #SMCCC_RES_a2]
> -1:
> - ret
> -END(__arm_smccc_1_0_smc)
> -
> /*
> * void arm_smccc_1_2_smc(const struct arm_smccc_1_2_regs *args,
> * struct arm_smccc_1_2_regs *res)
> diff --git a/xen/arch/arm/include/asm/smccc.h
> b/xen/arch/arm/include/asm/smccc.h
> index 5fe54013ac83..8920c54b09a6 100644
> --- a/xen/arch/arm/include/asm/smccc.h
> +++ b/xen/arch/arm/include/asm/smccc.h
> @@ -16,9 +16,6 @@
> #ifndef __ASM_ARM_SMCCC_H__
> #define __ASM_ARM_SMCCC_H__
>
> -#include <asm/alternative.h>
> -#include <asm/cpufeature.h>
> -
> #define SMCCC_VERSION_MAJOR_SHIFT 16
> #define SMCCC_VERSION_MINOR_MASK \
> ((1U << SMCCC_VERSION_MAJOR_SHIFT) - 1)
> @@ -57,6 +54,9 @@
> #ifndef __ASSEMBLER__
>
> #include <xen/macros.h>
> +#include <xen/types.h>
> +
> +#include <asm/asm_defns.h>
>
> extern uint32_t smccc_ver;
>
> @@ -160,6 +160,8 @@ struct arm_smccc_res {
> #define ___declare_args(count, ...) __declare_arg_ ## count(__VA_ARGS__)
> #define __declare_args(count, ...) ___declare_args(count, __VA_ARGS__)
>
> +#ifdef CONFIG_ARM_32
> +
> /*
> * arm_smccc_1_1_smc() - make an SMCCC v1.1 compliant SMC call
> *
> @@ -199,7 +201,6 @@ struct arm_smccc_res {
> * The calling convention for arm32 is the same for both SMCCC v1.0 and
> * v1.1.
> */
> -#ifdef CONFIG_ARM_32
> #define arm_smccc_smc(...) arm_smccc_1_1_smc(__VA_ARGS__)
>
> /* Make an SMCCC v1.1 compliant SMC call with guest register state. */
> @@ -218,53 +219,53 @@ static inline void arm_smccc_guest_smc(struct
> cpu_user_regs *regs)
>
> #else /* CONFIG_ARM_64 */
>
> -void __arm_smccc_1_0_smc(register_t a0, register_t a1, register_t a2,
> - register_t a3, register_t a4, register_t a5,
> - register_t a6, register_t a7,
> - struct arm_smccc_res *res);
> -
> -/* Macros to handle variadic parameter for SMCCC v1.0 helper */
> -#define __arm_smccc_1_0_smc_7(a0, a1, a2, a3, a4, a5, a6, a7, res) \
> - __arm_smccc_1_0_smc(a0, a1, a2, a3, a4, a5, a6, a7, res)
> -
> -#define __arm_smccc_1_0_smc_6(a0, a1, a2, a3, a4, a5, a6, res) \
> - __arm_smccc_1_0_smc_7(a0, a1, a2, a3, a4, a5, a6, 0, res)
> -
> -#define __arm_smccc_1_0_smc_5(a0, a1, a2, a3, a4, a5, res) \
> - __arm_smccc_1_0_smc_6(a0, a1, a2, a3, a4, a5, 0, res)
> -
> -#define __arm_smccc_1_0_smc_4(a0, a1, a2, a3, a4, res) \
> - __arm_smccc_1_0_smc_5(a0, a1, a2, a3, a4, 0, res)
> -
> -#define __arm_smccc_1_0_smc_3(a0, a1, a2, a3, res) \
> - __arm_smccc_1_0_smc_4(a0, a1, a2, a3, 0, res)
> -
> -#define __arm_smccc_1_0_smc_2(a0, a1, a2, res) \
> - __arm_smccc_1_0_smc_3(a0, a1, a2, 0, res)
> -
> -#define __arm_smccc_1_0_smc_1(a0, a1, res) \
> - __arm_smccc_1_0_smc_2(a0, a1, 0, res)
> -
> -#define __arm_smccc_1_0_smc_0(a0, res) \
> - __arm_smccc_1_0_smc_1(a0, 0, res)
> -
> -#define ___arm_smccc_1_0_smc_count(count, ...) \
> - __arm_smccc_1_0_smc_ ## count(__VA_ARGS__)
> -
> -#define __arm_smccc_1_0_smc_count(count, ...) \
> - ___arm_smccc_1_0_smc_count(count, __VA_ARGS__)
> -
> -#define arm_smccc_1_0_smc(...) \
> - __arm_smccc_1_0_smc_count(__count_args(__VA_ARGS__), __VA_ARGS__)
> -
> +/*
> + * Make an SMCCC call compatible with both PSCI v1.1 and v1.0.
> + *
> + * PSCI v1.1 says that x4 through x17 are strictly preserved unless they
> + * contain return data. PSCI v1.0 says they clobbered.
> + *
> + * Xen doesn't make PSCI v1.1 calls which expect more than 4 return
> registers,
> + * so imply list x4 through x17 as clobbered.
> + */
> #define arm_smccc_smc(...) \
> do { \
> - if ( cpus_have_const_cap(ARM_SMCCC_1_1) ) \
> - arm_smccc_1_1_smc(__VA_ARGS__); \
> - else \
> - arm_smccc_1_0_smc(__VA_ARGS__); \
> + register unsigned long r0 ASM_REG(0); \
> + register unsigned long r1 ASM_REG(1); \
> + register unsigned long r2 ASM_REG(2); \
> + register unsigned long r3 ASM_REG(3); \
> + /* Potentially clobbered in PSCI 1.0 */ \
> + register unsigned long c4 ASM_REG(4); \
> + register unsigned long c5 ASM_REG(5); \
> + register unsigned long c6 ASM_REG(6); \
> + register unsigned long c7 ASM_REG(7); \
> + register unsigned long c8 ASM_REG(8); \
> + register unsigned long c9 ASM_REG(9); \
> + register unsigned long c10 ASM_REG(10); \
> + register unsigned long c11 ASM_REG(11); \
> + register unsigned long c12 ASM_REG(12); \
> + register unsigned long c13 ASM_REG(13); \
> + register unsigned long c14 ASM_REG(14); \
> + register unsigned long c15 ASM_REG(15); \
> + register unsigned long c16 ASM_REG(16); \
> + register unsigned long c17 ASM_REG(17); \
> + __declare_args(__count_args(__VA_ARGS__), __VA_ARGS__); \
> + asm volatile ( \
> + "smc #0" \
> + : "=r" (r0), "=r" (r1), "=r" (r2), "=r" (r3), \
> + "=r" (c4), "=r" (c5), "=r" (c6), "=r" (c7), \
> + "=r" (c8), "=r" (c9), "=r" (c10), "=r" (c11), \
> + "=r" (c12), "=r" (c13), "=r" (c14), "=r" (c15), \
> + "=r" (c16), "=r" (c17) \
> + : PASTE(__constraint_read_, \
> + __count_args(__VA_ARGS__)) \
> + : "memory" ); \
> + if ( ___res ) \
> + *___res = (struct arm_smccc_res){ r0, r1, r2, r3 }; \
> } while ( 0 )
>
> +#define arm_smccc_1_1_smc(...) arm_smccc_smc(__VA_ARGS__)
> +
> /* Make an SMCCC v1.1 compliant SMC call with guest register state. */
> static inline void arm_smccc_guest_smc(struct cpu_user_regs *regs)
> {
> --
> 2.39.5
>
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |