Re: [RFC PATCH v2 5/8] arm64/sve: Implement an helper to flush SVE registers

From: Dave Martin <Dave.Martin@arm.com>
To: Julien Grall <julien.grall@arm.com>
Cc: Anton.Kirilov@arm.com, catalin.marinas@arm.com,
	will.deacon@arm.com, oleg@redhat.com, zhang.lei@jp.fujitsu.com,
	alex.bennee@linaro.org, linux-arm-kernel@lists.infradead.org,
	Daniel.Kiss@arm.com
Subject: Re: [RFC PATCH v2 5/8] arm64/sve: Implement an helper to flush SVE registers
Date: Tue, 25 Jun 2019 10:37:35 +0100	[thread overview]
Message-ID: <20190625093735.GX2790@e103592.cambridge.arm.com> (raw)
In-Reply-To: <20772a47-5cd3-0566-1e29-839e39961def@arm.com>

On Mon, Jun 24, 2019 at 05:28:56PM +0100, Julien Grall wrote:
> Hi Dave,
> 
> On 6/21/19 4:33 PM, Dave Martin wrote:
> >On Thu, Jun 13, 2019 at 05:16:53PM +0100, Julien Grall wrote:
> >>Introduce a new helper that will zero all SVE registers but the first
> >>128-bits of each vector.
> >
> >Maybe mention that the helper will be used by a subsequent patch.  For
> >now, it's dead code.
> >
> >Maybe also mention briefly what this will be used for: i.e., to avoid
> >the costly store/munge/reload sequences currently used by things like
> >do_sve_acc().
> 
> How about the following commit message:
> 
> "Introduce a new helper that will zero all SVE registers but the first
> 128-bits of each vector. This will be used in subsequent patch to avoid the
> costly store/munge/reload sequences used in place such as do_sve_acc()."

Sure, that works.

> 
> >
> >>Signed-off-by: Julien Grall <julien.grall@arm.com>
> >>
> >>---
> >>     Changes in v2:
> >>         - Fix typo in the commit title
> >>---
> >>  arch/arm64/include/asm/fpsimd.h       |  3 +++
> >>  arch/arm64/include/asm/fpsimdmacros.h | 19 +++++++++++++++++++
> >>  arch/arm64/kernel/entry-fpsimd.S      |  7 +++++++
> >>  3 files changed, 29 insertions(+)
> >>
> >>diff --git a/arch/arm64/include/asm/fpsimd.h b/arch/arm64/include/asm/fpsimd.h
> >>index df62bbd33a9a..fda3544c9606 100644
> >>--- a/arch/arm64/include/asm/fpsimd.h
> >>+++ b/arch/arm64/include/asm/fpsimd.h
> >>@@ -83,6 +83,9 @@ 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);
> >>+
> >
> >We probably don't need the extra blank lines here... these operations
> >are all closely related low-level backend functions.
> 
> Sure.

[...]

> >>diff --git a/arch/arm64/kernel/entry-fpsimd.S b/arch/arm64/kernel/entry-fpsimd.S
> >>index 12d4958e6429..17121a51c41f 100644
> >>--- a/arch/arm64/kernel/entry-fpsimd.S
> >>+++ b/arch/arm64/kernel/entry-fpsimd.S
> >>@@ -57,4 +57,11 @@ ENTRY(sve_get_vl)
> >>  	_sve_rdvl	0, 1
> >>  	ret
> >>  ENDPROC(sve_get_vl)
> >>+
> >>+/* Zero all SVE registers but the first 128-bits of each vector */
> >>+ENTRY(sve_flush_live)
> >>+	sve_flush
> >>+	ret
> >>+ENDPROC(sve_flush_live)
> >>+
> >>  #endif /* CONFIG_ARM64_SVE */
> >
> >The extra blank line makes this more readable, but in the interests
> >of symmetry can you also add a blank after the corresponding #ifdef?
> 
> I would prefer to do this change in a separate patch. So I will drop the
> newline here.

OK, sounds reasonable.

> 
> >
> >[...]
> >
> >With those addressed as appropriate,
> >
> >Reviewed-by: Dave Martin <Dave.Martin@arm.com>

Feel free to keep this tag.

Cheers
---Dave

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