Re: [PATCH v1 2/2] arm64: smccc: Support SMCCC v1.3 SVE register saving hint

[Marc Zyngier <maz@kernel.org>]

Hi Mark,

On Tue, 18 May 2021 11:09:19 +0100,
Mark Brown <broonie@kernel.org> wrote:
> 
> SMCCC v1.2 requires that all SVE state be preserved over SMC calls which
> introduces substantial overhead in the common case where there is no SVE
> state in the registers. To avoid this SMCCC v1.3 introduces a flag which
> allows the caller to say that there is no state that needs to be preserved
> in the registers. Make use of this flag, setting it if the SMCCC version
> indicates support for it and the TIF_ flags indicate that there is no live
> SVE state in the registers, this avoids placing any constraints on when
> SMCCC calls can be done or triggering extra saving and reloading of SVE
> register state in the kernel.
> 
> This would be straightforward enough except for the rather entertaining
> inline assembly we use to do SMCCC v1.1 calls to allow us to take advantage
> of the limited number of registers it clobbers. Deal with this by having a
> slightly non-standard function which we call immediately before issuing the
> SMCCC call to make our checks. This causes an extra function call on any
> system built with SVE support, and extra checks when SVE is detected at
> runtime, but these costs are expected to be reasonable in the context of
> doing a SMCCC call in the first place.
> 
> Signed-off-by: Mark Brown <broonie@kernel.org>
> ---
>  arch/arm64/kernel/smccc-call.S | 39 ++++++++++++++++++++++++++++++++++
>  drivers/firmware/smccc/smccc.c |  4 ++++
>  include/linux/arm-smccc.h      | 22 +++++++++++++++++--
>  3 files changed, 63 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/kernel/smccc-call.S b/arch/arm64/kernel/smccc-call.S
> index d62447964ed9..217bfb03a6c9 100644
> --- a/arch/arm64/kernel/smccc-call.S
> +++ b/arch/arm64/kernel/smccc-call.S
> @@ -7,8 +7,47 @@
>  
>  #include <asm/asm-offsets.h>
>  #include <asm/assembler.h>
> +#include <asm/thread_info.h>
> +
> +/*
> + * If we have SMCCC v1.3 and (as is likely) no SVE state in
> + * the registers then set the SMCCC hint bit to say there's no
> + * need to preserve it.  Do this by directly adjusting the SMCCC
> + * function value which is already stored in x0 ready to be called.
> + *
> + * Since we need scratch registers but wish to avoid having to handle
> + * the stack we expect the caller to preserve x15 and x16 if needed,
> + * the only callers are expected to be the call below and the inline
> + * asm in linux/arm-smccc.h for SMCCC 1.1 and later calls.
> + */
> +SYM_CODE_START(__smccc_sve_check)
> +	BTI_C
> +
> +alternative_if ARM64_SVE
> +
> +	adrp	x15, smccc_has_sve_hint

The adrp instruction will give you a 4kB aligned address, which
results in 1 chance out of 512 to point to the right location. The
adr_l macro is probably what you want.

> +	ldr	x15, [x15]
> +	cbz	x15, 1f
> +
> +	get_current_task x15
> +	ldr	x15, [x15, #TSK_TI_FLAGS]
> +	and	x16, x15, #TIF_FOREIGN_FPSTATE	// Any live FP state?
> +	cbnz	x16, 1f
> +	mov	x16, #TIF_SVE			// Does that state include SVE?
> +	and	x16, x15, x16
> +	cbnz	x16, 2f
> +
> +1:	orr	x0, x0, ARM_SMCCC_1_3_SVE_HINT
> +alternative_else_nop_endif
> +
> +2:	ret
> +SYM_CODE_END(__smccc_sve_check)
> +EXPORT_SYMBOL(__smccc_sve_check)
>  
>  	.macro SMCCC instr
> +alternative_if ARM64_SVE
> +	bl	__smccc_sve_check
> +alternative_else_nop_endif
>  	\instr	#0
>  	ldr	x4, [sp]
>  	stp	x0, x1, [x4, #ARM_SMCCC_RES_X0_OFFS]
> diff --git a/drivers/firmware/smccc/smccc.c b/drivers/firmware/smccc/smccc.c
> index 028f81d702cc..9f937b125ab0 100644
> --- a/drivers/firmware/smccc/smccc.c
> +++ b/drivers/firmware/smccc/smccc.c
> @@ -15,6 +15,7 @@ static u32 smccc_version = ARM_SMCCC_VERSION_1_0;
>  static enum arm_smccc_conduit smccc_conduit = SMCCC_CONDUIT_NONE;
>  
>  bool __ro_after_init smccc_trng_available = false;
> +u64 __ro_after_init smccc_has_sve_hint = false;
>  
>  void __init arm_smccc_version_init(u32 version, enum arm_smccc_conduit conduit)
>  {
> @@ -22,6 +23,9 @@ void __init arm_smccc_version_init(u32 version, enum arm_smccc_conduit conduit)
>  	smccc_conduit = conduit;
>  
>  	smccc_trng_available = smccc_probe_trng();
> +	if (IS_ENABLED(CONFIG_ARM64_SVE) &&
> +	    smccc_version >= ARM_SMCCC_VERSION_1_3)
> +		smccc_has_sve_hint = true;
>  }
>  
>  enum arm_smccc_conduit arm_smccc_1_1_get_conduit(void)
> diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h
> index 6861489a1890..611f3bc9c131 100644
> --- a/include/linux/arm-smccc.h
> +++ b/include/linux/arm-smccc.h
> @@ -63,6 +63,9 @@
>  #define ARM_SMCCC_VERSION_1_0		0x10000
>  #define ARM_SMCCC_VERSION_1_1		0x10001
>  #define ARM_SMCCC_VERSION_1_2		0x10002
> +#define ARM_SMCCC_VERSION_1_3		0x10003
> +
> +#define ARM_SMCCC_1_3_SVE_HINT		0x10000
>  
>  #define ARM_SMCCC_VERSION_FUNC_ID					\
>  	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,				\
> @@ -216,6 +219,8 @@ u32 arm_smccc_get_version(void);
>  
>  void __init arm_smccc_version_init(u32 version, enum arm_smccc_conduit conduit);
>  
> +extern u64 smccc_has_sve_hint;
> +
>  /**
>   * struct arm_smccc_res - Result from SMC/HVC call
>   * @a0-a3 result values from registers 0 to 3
> @@ -297,6 +302,18 @@ asmlinkage void __arm_smccc_hvc(unsigned long a0, unsigned long a1,
>  
>  #endif
>  
> +#ifdef CONFIG_ARM64_SVE
> +
> +#define SMCCC_SVE_CHECK "bl __smccc_sve_check \n"
> +#define smccc_sve_clobbers "x15", "x16", "lr",
> +
> +#else
> +
> +#define SMCCC_SVE_CHECK
> +#define smccc_sve_clobbers
> +
> +#endif
> +
>  #define ___count_args(_0, _1, _2, _3, _4, _5, _6, _7, _8, x, ...) x
>  
>  #define __count_args(...)						\
> @@ -364,7 +381,7 @@ asmlinkage void __arm_smccc_hvc(unsigned long a0, unsigned long a1,
>  
>  #define ___constraints(count)						\
>  	: __constraint_read_ ## count					\
> -	: "memory"
> +	: smccc_sve_clobbers "memory"
>  #define __constraints(count)	___constraints(count)
>  
>  /*
> @@ -379,7 +396,8 @@ asmlinkage void __arm_smccc_hvc(unsigned long a0, unsigned long a1,
>  		register unsigned long r2 asm("r2");			\
>  		register unsigned long r3 asm("r3"); 			\
>  		__declare_args(__count_args(__VA_ARGS__), __VA_ARGS__);	\
> -		asm volatile(inst "\n" :				\
> +		asm volatile(SMCCC_SVE_CHECK				\

What happens when this is called from a context where
__smccc_sve_check isn't mapped, nor does "current" mean anything? See
arch/arm64/kvm/hyp/nvhe/psci-relay.c for an example.

> +			     inst "\n" :				\
>  			     "=r" (r0), "=r" (r1), "=r" (r2), "=r" (r3)	\
>  			     __constraints(__count_args(__VA_ARGS__)));	\
>  		if (___res)						\

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel