|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v2 4/5] xen/arm: Rewrite arm_smccc_smc() for arm64
Hi Andrew,
> On 4 Sep 2026, at 13:12, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:
>
> SMCCC v1.0 says that x4 through x17 may be clobbered. SMCCC v1.1 says they
> are strictly preserved, and SMCCC v1.2 permits them to contain extra return
> values.
>
> 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 through x17, and calling an out-of-line function.
>
> Remove __arm_smccc_1_0_smc() entirely, and rewrite arm_smccc_smc() to declare
> x4 through 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>
Lot better thanks for the fixes.
Reviewed-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>
Cheers
Bertrand
> ---
> 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>
>
> v2:
> * s/PSCI/SMCCC/g
> * Rewrite the comment for the new arm_smccc_smc().
>
> 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 SMCCC v1.2 worked into it too.
> ---
> xen/arch/arm/arm64/smc.S | 16 ------
> xen/arch/arm/include/asm/smccc.h | 92 ++++++++++++++++----------------
> 2 files changed, 45 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..4ed2a40ed0ac 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,50 @@ 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 SMC call compatible with both SMCCC v1.1 and v1.0.
> + *
> + * SMCCC v1.0 says that x4 through x17 are clobbered. SMCCC v1.1 says they
> + * are strictly preserved. Always mark 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 SMCCC v1.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 |