[PATCH v2 0/2] arm64: Implement SMCCC v1.3 SVE register saving hint

[PATCH v2 0/2] arm64: Implement SMCCC v1.3 SVE register saving hint

[Mark Brown]

SMCCC v1.3 provides support for a flag which allows the caller to say
that there is no state that needs to be preserved in the SVE registers,
meaning that the called code can skip doing this, especially with larger
vector lengths this can save a noticable amount of work in the event
that the state needs to be saved. Implement support for this, using the
TIF_ flags to report if there is live SVE state present.

This overlaps with Sudeep's patch "arm64: smccc: Add support for
SMCCCv1.2 extended input/output registers"[1] although there shouldn't
be any conflicts, that adds some new SMCCC calls for v1.2 - the new
functions for v1.2 calls will need updating to take advantage of the
hint bit.

v2:
 - Use ldr_l rather than adrp/ldr so we load the hint flag rather than
   the start of the 4K block the hint flag happens to be in.
 - Disable the optimisation when running in nVHE so we don't try to jump
   to a function that isn't mapped or look at a current task that isn't
   meaningful.
 - Use _TIF for a bitfield like we should've done first time round.

[1] https://lore.kernel.org/r/20210518163618.43950-1-sudeep.holla@arm.com

Mark Brown (2):
  arm64: asmlinkage: Enable use of BTI_C macro in SYM_CODE
  arm64: smccc: Support SMCCC v1.3 SVE register saving hint

 arch/arm64/include/asm/linkage.h |  4 ++++
 arch/arm64/kernel/smccc-call.S   | 38 ++++++++++++++++++++++++++++++++
 drivers/firmware/smccc/smccc.c   |  4 ++++
 include/linux/arm-smccc.h        | 23 +++++++++++++++++--
 4 files changed, 67 insertions(+), 2 deletions(-)


base-commit: d07f6ca923ea0927a1024dfccafc5b53b61cfecc
-- 
2.20.1


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

[PATCH v2 1/2] arm64: asmlinkage: Enable use of BTI_C macro in SYM_CODE

[Mark Brown]

SYM_CODE sections may require BTI landing pads but won't have them
automatically generated by the symbol definiton macros. Make an empty
version of the BTI_C macro available when BTI is disabled so that they
can more easily create the landing pads if they need them.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
 arch/arm64/include/asm/linkage.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm64/include/asm/linkage.h b/arch/arm64/include/asm/linkage.h
index ba89a9af820a..d98f9af8b8ec 100644
--- a/arch/arm64/include/asm/linkage.h
+++ b/arch/arm64/include/asm/linkage.h
@@ -42,6 +42,10 @@
 	SYM_START(name, SYM_L_WEAK, SYM_A_NONE)		\
 	BTI_C
 
+#else
+
+#define BTI_C
+
 #endif
 
 /*
-- 
2.20.1


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

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

[Mark Brown]

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.

Since we can't check the TIF_ flags for current and would need to map
the helper function separately when in the nVHE hypervisor we don't
attempt to do the optimisation for it, this could be done as a followup.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
 arch/arm64/kernel/smccc-call.S | 38 ++++++++++++++++++++++++++++++++++
 drivers/firmware/smccc/smccc.c |  4 ++++
 include/linux/arm-smccc.h      | 23 ++++++++++++++++++--
 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..2713ea37b536 100644
--- a/arch/arm64/kernel/smccc-call.S
+++ b/arch/arm64/kernel/smccc-call.S
@@ -7,8 +7,46 @@
 
 #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
+
+	ldr_l	x15, smccc_has_sve_hint
+	cbz	x15, 2f
+
+	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..9e974a9f8993 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,19 @@ asmlinkage void __arm_smccc_hvc(unsigned long a0, unsigned long a1,
 
 #endif
 
+/* nVHE hypervisor doesn't have a current thread so needs separate checks */
+#if defined(CONFIG_ARM64_SVE) && !defined(__KVM_NVHE_HYPERVISOR__)
+
+#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 +382,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 +397,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				\
+			     inst "\n" :				\
 			     "=r" (r0), "=r" (r1), "=r" (r2), "=r" (r3)	\
 			     __constraints(__count_args(__VA_ARGS__)));	\
 		if (___res)						\
-- 
2.20.1


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

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

[Ard Biesheuvel]

Hi Mark,

Some more comments on this patch below. Apologies for not mentioning
these the first time around, I only commented on Marc's reply and did
not look carefully at the rest.

On Tue, 18 May 2021 at 20:35, 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.
>
> Since we can't check the TIF_ flags for current and would need to map
> the helper function separately when in the nVHE hypervisor we don't
> attempt to do the optimisation for it, this could be done as a followup.
>
> Signed-off-by: Mark Brown <broonie@kernel.org>
> ---
>  arch/arm64/kernel/smccc-call.S | 38 ++++++++++++++++++++++++++++++++++
>  drivers/firmware/smccc/smccc.c |  4 ++++
>  include/linux/arm-smccc.h      | 23 ++++++++++++++++++--
>  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..2713ea37b536 100644
> --- a/arch/arm64/kernel/smccc-call.S
> +++ b/arch/arm64/kernel/smccc-call.S
> @@ -7,8 +7,46 @@
>
>  #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,

You should use x16 and x17 here. The calling convention dictates that
the linker may use x16 and x17 in a veneer (or PLT lookup sequence) if
the branch to  __smccc_sve_check() goes out of range, and even if our
PLT code only uses x16 at this time, it is better to follow the AAPCS
here.

> + * 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
> +

Any reason not to use SYM_FUNC_START() here?

> +alternative_if ARM64_SVE
> +
> +       ldr_l   x15, smccc_has_sve_hint
> +       cbz     x15, 2f
> +
> +       get_current_task x15
> +       ldr     x15, [x15, #TSK_TI_FLAGS]
> +       and     x16, x15, #_TIF_FOREIGN_FPSTATE // Any live FP state?
> +       cbnz    x16, 1f

You could use tbnz here, with the actual shift in TIF_FOREIGN_FPSTATE

> +       mov     x16, #_TIF_SVE                  // Does that state include SVE?
> +       and     x16, x15, x16
> +       cbnz    x16, 2f
> +

Same but with TIF_SVE. That way, you don't need to corrupt x15 at all
(but we should still mark x16 and x17 as clobbered)

> +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

Do we need the redundant alternatives here? Could we use an
ALTERNATIVE() below instead, and drop the one in __smccc_sve_check?

>         \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..9e974a9f8993 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,19 @@ asmlinkage void __arm_smccc_hvc(unsigned long a0, unsigned long a1,
>
>  #endif
>
> +/* nVHE hypervisor doesn't have a current thread so needs separate checks */
> +#if defined(CONFIG_ARM64_SVE) && !defined(__KVM_NVHE_HYPERVISOR__)
> +
> +#define SMCCC_SVE_CHECK "bl __smccc_sve_check \n"

I.e., make this

#define SMCCC_SVE_CHECK ALTERNATIVE("bl __smccc_sve_check \n", "nop", ARM64_SVE)

(or whichever way around they should go in this particular case)


> +#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 +382,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 +397,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                            \
> +                            inst "\n" :                                \
>                              "=r" (r0), "=r" (r1), "=r" (r2), "=r" (r3) \
>                              __constraints(__count_args(__VA_ARGS__))); \
>                 if (___res)                                             \
> --
> 2.20.1
>

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

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

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1952 bytes --]

On Wed, May 19, 2021 at 09:05:41AM +0200, Ard Biesheuvel wrote:
> On Tue, 18 May 2021 at 20:35, Mark Brown <broonie@kernel.org> wrote:

> > + * 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,

> You should use x16 and x17 here. The calling convention dictates that
> the linker may use x16 and x17 in a veneer (or PLT lookup sequence) if
> the branch to  __smccc_sve_check() goes out of range, and even if our
> PLT code only uses x16 at this time, it is better to follow the AAPCS
> here.

Oh, of course - not sure why I didn't use x17 TBH.

> > + * 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

> Any reason not to use SYM_FUNC_START() here?

Not with the changes you've suggested, 

> > +alternative_if ARM64_SVE
> > +
> > +       ldr_l   x15, smccc_has_sve_hint
> > +       cbz     x15, 2f
> > +
> > +       get_current_task x15
> > +       ldr     x15, [x15, #TSK_TI_FLAGS]
> > +       and     x16, x15, #_TIF_FOREIGN_FPSTATE // Any live FP state?
> > +       cbnz    x16, 1f

> You could use tbnz here, with the actual shift in TIF_FOREIGN_FPSTATE

Ah, yes - I'd misremembered what tbnz uses and thought it didn't apply.
That does help a lot.

> >         .macro SMCCC instr
> > +alternative_if ARM64_SVE
> > +       bl      __smccc_sve_check
> > +alternative_else_nop_endif

> Do we need the redundant alternatives here? Could we use an
> ALTERNATIVE() below instead, and drop the one in __smccc_sve_check?

We do need the one right there, but with the use of ALTERNATIVE() in the
inline asm we'd be able to drop the one in __smccc_sve_check.  Or for
that matter we could not bother with alternatives at all if we're happy
that the overhead were OK.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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