[PATCH v2 0/3] arm64/sve: Trivial optimisation for 128 bit SVE vectors

[PATCH v2 0/3] arm64/sve: Trivial optimisation for 128 bit SVE vectors

[Mark Brown]

This series is a combination of factoring out some duplicated code and a
very minor optimisation to the performance of handling converting FPSIMD
state to SVE in the live registers for 128 bit SVE vectors.

v2:
 - Combine P and FFR flushing into a single macro.

Mark Brown (3):
  arm64/sve: Split _sve_flush macro into separate Z and predicate
    flushes
  arm64/sve: Use the sve_flush macros in sve_load_from_fpsimd_state()
  arm64/sve: Skip flushing Z registers with 128 bit vectors

 arch/arm64/include/asm/fpsimd.h       |  2 +-
 arch/arm64/include/asm/fpsimdmacros.h |  4 +++-
 arch/arm64/kernel/entry-fpsimd.S      | 19 ++++++++++++-------
 arch/arm64/kernel/fpsimd.c            |  6 ++++--
 4 files changed, 20 insertions(+), 11 deletions(-)


base-commit: 6efb943b8616ec53a5e444193dccf1af9ad627b5
-- 
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/3] arm64/sve: Split _sve_flush macro into separate Z and predicate flushes

[Mark Brown]

Trivial refactoring to support further work, no change to generated code.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
 arch/arm64/include/asm/fpsimdmacros.h | 4 +++-
 arch/arm64/kernel/entry-fpsimd.S      | 3 ++-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/include/asm/fpsimdmacros.h b/arch/arm64/include/asm/fpsimdmacros.h
index a2563992d2dc..059204477ce6 100644
--- a/arch/arm64/include/asm/fpsimdmacros.h
+++ b/arch/arm64/include/asm/fpsimdmacros.h
@@ -213,8 +213,10 @@
 	mov	v\nz\().16b, v\nz\().16b
 .endm
 
-.macro sve_flush
+.macro sve_flush_z
  _for n, 0, 31, _sve_flush_z	\n
+.endm
+.macro sve_flush_p_ffr
  _for n, 0, 15, _sve_pfalse	\n
 		_sve_wrffr	0
 .endm
diff --git a/arch/arm64/kernel/entry-fpsimd.S b/arch/arm64/kernel/entry-fpsimd.S
index 3ecec60d3295..7921d58427c2 100644
--- a/arch/arm64/kernel/entry-fpsimd.S
+++ b/arch/arm64/kernel/entry-fpsimd.S
@@ -72,7 +72,8 @@ SYM_FUNC_END(sve_load_from_fpsimd_state)
 
 /* Zero all SVE registers but the first 128-bits of each vector */
 SYM_FUNC_START(sve_flush_live)
-	sve_flush
+	sve_flush_z
+	sve_flush_p_ffr
 	ret
 SYM_FUNC_END(sve_flush_live)
 
-- 
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/3] arm64/sve: Use the sve_flush macros in sve_load_from_fpsimd_state()

[Mark Brown]

This makes the code a bit clearer and as a result we can also make the
indentation more normal, there is no change to the generated code.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
 arch/arm64/kernel/entry-fpsimd.S | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/kernel/entry-fpsimd.S b/arch/arm64/kernel/entry-fpsimd.S
index 7921d58427c2..dd8382e5ce82 100644
--- a/arch/arm64/kernel/entry-fpsimd.S
+++ b/arch/arm64/kernel/entry-fpsimd.S
@@ -63,11 +63,10 @@ SYM_FUNC_END(sve_set_vq)
  * and the rest zeroed. All the other SVE registers will be zeroed.
  */
 SYM_FUNC_START(sve_load_from_fpsimd_state)
-		sve_load_vq	x1, x2, x3
-		fpsimd_restore	x0, 8
- _for n, 0, 15, _sve_pfalse	\n
-		_sve_wrffr	0
-		ret
+	sve_load_vq	x1, x2, x3
+	fpsimd_restore	x0, 8
+	sve_flush_p_ffr
+	ret
 SYM_FUNC_END(sve_load_from_fpsimd_state)
 
 /* Zero all SVE registers but the first 128-bits of each vector */
-- 
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 3/3] arm64/sve: Skip flushing Z registers with 128 bit vectors

[Mark Brown]

When the SVE vector length is 128 bits then there are no bits in the Z
registers which are not shared with the V registers so we can skip them
when zeroing state not shared with FPSIMD, this results in a minor
performance improvement.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
 arch/arm64/include/asm/fpsimd.h  | 2 +-
 arch/arm64/kernel/entry-fpsimd.S | 9 +++++++--
 arch/arm64/kernel/fpsimd.c       | 6 ++++--
 3 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/include/asm/fpsimd.h b/arch/arm64/include/asm/fpsimd.h
index 2599504674b5..c072161d5c65 100644
--- a/arch/arm64/include/asm/fpsimd.h
+++ b/arch/arm64/include/asm/fpsimd.h
@@ -69,7 +69,7 @@ static inline void *sve_pffr(struct thread_struct *thread)
 extern void sve_save_state(void *state, u32 *pfpsr);
 extern void sve_load_state(void const *state, u32 const *pfpsr,
 			   unsigned long vq_minus_1);
-extern void sve_flush_live(void);
+extern void sve_flush_live(unsigned long vq_minus_1);
 extern void sve_load_from_fpsimd_state(struct user_fpsimd_state const *state,
 				       unsigned long vq_minus_1);
 extern unsigned int sve_get_vl(void);
diff --git a/arch/arm64/kernel/entry-fpsimd.S b/arch/arm64/kernel/entry-fpsimd.S
index dd8382e5ce82..87ef25836963 100644
--- a/arch/arm64/kernel/entry-fpsimd.S
+++ b/arch/arm64/kernel/entry-fpsimd.S
@@ -69,10 +69,15 @@ SYM_FUNC_START(sve_load_from_fpsimd_state)
 	ret
 SYM_FUNC_END(sve_load_from_fpsimd_state)
 
-/* Zero all SVE registers but the first 128-bits of each vector */
+/*
+ * Zero all SVE registers but the first 128-bits of each vector
+ *
+ * x0 = VQ - 1
+ */
 SYM_FUNC_START(sve_flush_live)
+	cbz		x0, 1f	// A VQ-1 of 0 is 128 bits so no extra Z state
 	sve_flush_z
-	sve_flush_p_ffr
+1:	sve_flush_p_ffr
 	ret
 SYM_FUNC_END(sve_flush_live)
 
diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
index ad3dd34a83cf..e57b23f95284 100644
--- a/arch/arm64/kernel/fpsimd.c
+++ b/arch/arm64/kernel/fpsimd.c
@@ -957,8 +957,10 @@ void do_sve_acc(unsigned int esr, struct pt_regs *regs)
 	 * disabling the trap, otherwise update our in-memory copy.
 	 */
 	if (!test_thread_flag(TIF_FOREIGN_FPSTATE)) {
-		sve_set_vq(sve_vq_from_vl(current->thread.sve_vl) - 1);
-		sve_flush_live();
+		unsigned long vq_minus_one =
+			sve_vq_from_vl(current->thread.sve_vl) - 1;
+		sve_set_vq(vq_minus_one);
+		sve_flush_live(vq_minus_one);
 		fpsimd_bind_task_to_cpu();
 	} else {
 		fpsimd_to_sve(current);
-- 
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 1/3] arm64/sve: Split _sve_flush macro into separate Z and predicate flushes

[Dave Martin]

On Tue, May 11, 2021 at 05:04:44PM +0100, Mark Brown wrote:
> Trivial refactoring to support further work, no change to generated code.
> 
> Signed-off-by: Mark Brown <broonie@kernel.org>

Reviewed-by: Dave Martin <Dave.Martin@arm.com>

> ---
>  arch/arm64/include/asm/fpsimdmacros.h | 4 +++-
>  arch/arm64/kernel/entry-fpsimd.S      | 3 ++-
>  2 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/fpsimdmacros.h b/arch/arm64/include/asm/fpsimdmacros.h
> index a2563992d2dc..059204477ce6 100644
> --- a/arch/arm64/include/asm/fpsimdmacros.h
> +++ b/arch/arm64/include/asm/fpsimdmacros.h
> @@ -213,8 +213,10 @@
>  	mov	v\nz\().16b, v\nz\().16b
>  .endm
>  
> -.macro sve_flush
> +.macro sve_flush_z
>   _for n, 0, 31, _sve_flush_z	\n
> +.endm
> +.macro sve_flush_p_ffr
>   _for n, 0, 15, _sve_pfalse	\n
>  		_sve_wrffr	0
>  .endm
> diff --git a/arch/arm64/kernel/entry-fpsimd.S b/arch/arm64/kernel/entry-fpsimd.S
> index 3ecec60d3295..7921d58427c2 100644
> --- a/arch/arm64/kernel/entry-fpsimd.S
> +++ b/arch/arm64/kernel/entry-fpsimd.S
> @@ -72,7 +72,8 @@ SYM_FUNC_END(sve_load_from_fpsimd_state)
>  
>  /* Zero all SVE registers but the first 128-bits of each vector */
>  SYM_FUNC_START(sve_flush_live)
> -	sve_flush
> +	sve_flush_z
> +	sve_flush_p_ffr
>  	ret
>  SYM_FUNC_END(sve_flush_live)
>  
> -- 
> 2.20.1
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

Re: [PATCH v2 2/3] arm64/sve: Use the sve_flush macros in sve_load_from_fpsimd_state()

[Dave Martin]

On Tue, May 11, 2021 at 05:04:45PM +0100, Mark Brown wrote:
> This makes the code a bit clearer and as a result we can also make the
> indentation more normal, there is no change to the generated code.
> 
> Signed-off-by: Mark Brown <broonie@kernel.org>

Reviewed-by: Dave Martin <Dave.Martin@arm.com>

> ---
>  arch/arm64/kernel/entry-fpsimd.S | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm64/kernel/entry-fpsimd.S b/arch/arm64/kernel/entry-fpsimd.S
> index 7921d58427c2..dd8382e5ce82 100644
> --- a/arch/arm64/kernel/entry-fpsimd.S
> +++ b/arch/arm64/kernel/entry-fpsimd.S
> @@ -63,11 +63,10 @@ SYM_FUNC_END(sve_set_vq)
>   * and the rest zeroed. All the other SVE registers will be zeroed.
>   */
>  SYM_FUNC_START(sve_load_from_fpsimd_state)
> -		sve_load_vq	x1, x2, x3
> -		fpsimd_restore	x0, 8
> - _for n, 0, 15, _sve_pfalse	\n
> -		_sve_wrffr	0
> -		ret
> +	sve_load_vq	x1, x2, x3
> +	fpsimd_restore	x0, 8
> +	sve_flush_p_ffr
> +	ret
>  SYM_FUNC_END(sve_load_from_fpsimd_state)
>  
>  /* Zero all SVE registers but the first 128-bits of each vector */
> -- 
> 2.20.1
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

Re: [PATCH v2 3/3] arm64/sve: Skip flushing Z registers with 128 bit vectors

[Dave Martin]

On Tue, May 11, 2021 at 05:04:46PM +0100, Mark Brown wrote:
> When the SVE vector length is 128 bits then there are no bits in the Z
> registers which are not shared with the V registers so we can skip them
> when zeroing state not shared with FPSIMD, this results in a minor
> performance improvement.
> 
> Signed-off-by: Mark Brown <broonie@kernel.org>
> ---
>  arch/arm64/include/asm/fpsimd.h  | 2 +-
>  arch/arm64/kernel/entry-fpsimd.S | 9 +++++++--
>  arch/arm64/kernel/fpsimd.c       | 6 ++++--
>  3 files changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/fpsimd.h b/arch/arm64/include/asm/fpsimd.h
> index 2599504674b5..c072161d5c65 100644
> --- a/arch/arm64/include/asm/fpsimd.h
> +++ b/arch/arm64/include/asm/fpsimd.h
> @@ -69,7 +69,7 @@ static inline void *sve_pffr(struct thread_struct *thread)
>  extern void sve_save_state(void *state, u32 *pfpsr);
>  extern void sve_load_state(void const *state, u32 const *pfpsr,
>  			   unsigned long vq_minus_1);
> -extern void sve_flush_live(void);
> +extern void sve_flush_live(unsigned long vq_minus_1);
>  extern void sve_load_from_fpsimd_state(struct user_fpsimd_state const *state,
>  				       unsigned long vq_minus_1);
>  extern unsigned int sve_get_vl(void);
> diff --git a/arch/arm64/kernel/entry-fpsimd.S b/arch/arm64/kernel/entry-fpsimd.S
> index dd8382e5ce82..87ef25836963 100644
> --- a/arch/arm64/kernel/entry-fpsimd.S
> +++ b/arch/arm64/kernel/entry-fpsimd.S
> @@ -69,10 +69,15 @@ SYM_FUNC_START(sve_load_from_fpsimd_state)
>  	ret
>  SYM_FUNC_END(sve_load_from_fpsimd_state)
>  
> -/* Zero all SVE registers but the first 128-bits of each vector */
> +/*
> + * Zero all SVE registers but the first 128-bits of each vector
> + *
> + * x0 = VQ - 1

This does require that ZCR_EL1.LEN has already been set to match x0, and
is not changed again before entering userspace.

It would be a good idea to at least describe this in a comment so that
this doesn't get forgotten later on, but there's a limit to how
foolproof this low-level backend code needs to be...

> + */
>  SYM_FUNC_START(sve_flush_live)
> +	cbz		x0, 1f	// A VQ-1 of 0 is 128 bits so no extra Z state
>  	sve_flush_z
> -	sve_flush_p_ffr
> +1:	sve_flush_p_ffr
>  	ret
>  SYM_FUNC_END(sve_flush_live)
>  
> diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
> index ad3dd34a83cf..e57b23f95284 100644
> --- a/arch/arm64/kernel/fpsimd.c
> +++ b/arch/arm64/kernel/fpsimd.c
> @@ -957,8 +957,10 @@ void do_sve_acc(unsigned int esr, struct pt_regs *regs)
>  	 * disabling the trap, otherwise update our in-memory copy.
>  	 */
>  	if (!test_thread_flag(TIF_FOREIGN_FPSTATE)) {
> -		sve_set_vq(sve_vq_from_vl(current->thread.sve_vl) - 1);
> -		sve_flush_live();
> +		unsigned long vq_minus_one =
> +			sve_vq_from_vl(current->thread.sve_vl) - 1;
> +		sve_set_vq(vq_minus_one);
> +		sve_flush_live(vq_minus_one);
>  		fpsimd_bind_task_to_cpu();
>  	} else {
>  		fpsimd_to_sve(current);
> -- 
> 2.20.1

With a comment added as outlined above,

Reviewed-by: Dave Martin <Dave.Martin@arm.com>

Cheers
---Dave

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

Re: [PATCH v2 3/3] arm64/sve: Skip flushing Z registers with 128 bit vectors

[Mark Brown]

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

On Wed, May 12, 2021 at 02:49:09PM +0100, Dave Martin wrote:
> On Tue, May 11, 2021 at 05:04:46PM +0100, Mark Brown wrote:

> > +/*
> > + * Zero all SVE registers but the first 128-bits of each vector
> > + *
> > + * x0 = VQ - 1

> This does require that ZCR_EL1.LEN has already been set to match x0, and
> is not changed again before entering userspace.

> It would be a good idea to at least describe this in a comment so that
> this doesn't get forgotten later on, but there's a limit to how
> foolproof this low-level backend code needs to be...

Similar concerns exist for huge chunks of the existing SVE code (eg, the
no further changes on vector length constraint is pretty much universal
and is I'd say largely more of a "make sure you handle the register
contents" thing on anything that changes the vector length).

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

Re: [PATCH v2 0/3] arm64/sve: Trivial optimisation for 128 bit SVE vectors

[Catalin Marinas]

On Tue, May 11, 2021 at 05:04:43PM +0100, Mark Brown wrote:
> This series is a combination of factoring out some duplicated code and a
> very minor optimisation to the performance of handling converting FPSIMD
> state to SVE in the live registers for 128 bit SVE vectors.
> 
> v2:
>  - Combine P and FFR flushing into a single macro.
> 
> Mark Brown (3):
>   arm64/sve: Split _sve_flush macro into separate Z and predicate
>     flushes
>   arm64/sve: Use the sve_flush macros in sve_load_from_fpsimd_state()
>   arm64/sve: Skip flushing Z registers with 128 bit vectors

The series makes sense to me:

Acked-by: Catalin Marinas <catalin.marinas@arm.com>

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