[PATCH v2 0/2] arm64: DC {ZVA,GVA,GZVA} shouldn't be used when DCZID_EL0.DZP == 1

[PATCH v2 0/2] arm64: DC {ZVA,GVA,GZVA} shouldn't be used when DCZID_EL0.DZP == 1

[Reiji Watanabe]

DCZID_EL0.DZP indicates whether or not use of DC {ZVA,GVA,GZVA} is
prohibited. However, clear_page(), mte_set_mem_tag_range(), and
mte_zero_clear_page_tags() use those instructions without checking it.

Fix those functions not to use DC {ZVA,GVA,GZVA} when DCZID_EL0.DZP == 1.

v2:
 - Fix mte_set_mem_tag_range() and mte_zero_clear_page_tags() not to use
   DC {GVA,GZVA} when DCZID_EL0.DZP == 1. [Mark]
 - Fix a typo of the comment in clear_page() [Mark]
 - Fix a loop of for stp in clear_page() for more consistency with
   the existing loop  [Mark]

v1: https://lore.kernel.org/all/20211026034844.1393437-1-reijiw@google.com/

Reiji Watanabe (2):
  arm64: clear_page() shouldn't use DC ZVA when DCZID_EL0.DZP == 1
  arm64: mte: DC {GVA,GZVA} shouldn't be used when DCZID_EL0.DZP == 1

 arch/arm64/include/asm/mte-kasan.h |  8 +++++---
 arch/arm64/lib/clear_page.S        | 10 ++++++++++
 arch/arm64/lib/mte.S               |  8 +++++++-
 3 files changed, 22 insertions(+), 4 deletions(-)

-- 
2.34.0.rc0.344.g81b53c2807-goog


_______________________________________________
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: clear_page() shouldn't use DC ZVA when DCZID_EL0.DZP == 1

[Reiji Watanabe]

Currently, clear_page() uses DC ZVA instruction unconditionally.  But it
should make sure that DCZID_EL0.DZP, which indicates whether or not use
of DC ZVA instruction is prohibited, is zero when using the instruction.
Use STP as memset does instead when DCZID_EL0.DZP == 1.

Fixes: f27bb139c387 ("arm64: Miscellaneous library functions")
Signed-off-by: Reiji Watanabe <reijiw@google.com>
---
 arch/arm64/lib/clear_page.S | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm64/lib/clear_page.S b/arch/arm64/lib/clear_page.S
index b84b179edba3..ea3a6c927fe8 100644
--- a/arch/arm64/lib/clear_page.S
+++ b/arch/arm64/lib/clear_page.S
@@ -16,6 +16,7 @@
  */
 SYM_FUNC_START_PI(clear_page)
 	mrs	x1, dczid_el0
+	tbnz	x1, #4, 2f	/* Branch if DC ZVA is prohibited */
 	and	w1, w1, #0xf
 	mov	x2, #4
 	lsl	x1, x2, x1
@@ -25,5 +26,14 @@ SYM_FUNC_START_PI(clear_page)
 	tst	x0, #(PAGE_SIZE - 1)
 	b.ne	1b
 	ret
+
+2:	stp	xzr, xzr, [x0]
+	stp	xzr, xzr, [x0, #16]
+	stp	xzr, xzr, [x0, #32]
+	stp	xzr, xzr, [x0, #48]
+	add	x0, x0, #64
+	tst	x0, #(PAGE_SIZE - 1)
+	b.ne	2b
+	ret
 SYM_FUNC_END_PI(clear_page)
 EXPORT_SYMBOL(clear_page)
-- 
2.34.0.rc0.344.g81b53c2807-goog


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

Re: [PATCH v2 1/2] arm64: clear_page() shouldn't use DC ZVA when DCZID_EL0.DZP == 1

[Robin Murphy]

On 2021-11-08 07:11, Reiji Watanabe wrote:
> Currently, clear_page() uses DC ZVA instruction unconditionally.  But it
> should make sure that DCZID_EL0.DZP, which indicates whether or not use
> of DC ZVA instruction is prohibited, is zero when using the instruction.
> Use STP as memset does instead when DCZID_EL0.DZP == 1.

Reviewed-by: Robin Murphy <robin.murphy@arm.com>

FWIW I did eventually figure out the "pre-bias" trick from v1 thanks to 
Mark's nod toward the original context, but a quick survey of various 
optimisation guides implied that the explicit add should generally be 
preferred over post-index writeback anyway, so I think we're all good here.

Cheers,
Robin.

> Fixes: f27bb139c387 ("arm64: Miscellaneous library functions")
> Signed-off-by: Reiji Watanabe <reijiw@google.com>
> ---
>   arch/arm64/lib/clear_page.S | 10 ++++++++++
>   1 file changed, 10 insertions(+)
> 
> diff --git a/arch/arm64/lib/clear_page.S b/arch/arm64/lib/clear_page.S
> index b84b179edba3..ea3a6c927fe8 100644
> --- a/arch/arm64/lib/clear_page.S
> +++ b/arch/arm64/lib/clear_page.S
> @@ -16,6 +16,7 @@
>    */
>   SYM_FUNC_START_PI(clear_page)
>   	mrs	x1, dczid_el0
> +	tbnz	x1, #4, 2f	/* Branch if DC ZVA is prohibited */
>   	and	w1, w1, #0xf
>   	mov	x2, #4
>   	lsl	x1, x2, x1
> @@ -25,5 +26,14 @@ SYM_FUNC_START_PI(clear_page)
>   	tst	x0, #(PAGE_SIZE - 1)
>   	b.ne	1b
>   	ret
> +
> +2:	stp	xzr, xzr, [x0]
> +	stp	xzr, xzr, [x0, #16]
> +	stp	xzr, xzr, [x0, #32]
> +	stp	xzr, xzr, [x0, #48]
> +	add	x0, x0, #64
> +	tst	x0, #(PAGE_SIZE - 1)
> +	b.ne	2b
> +	ret
>   SYM_FUNC_END_PI(clear_page)
>   EXPORT_SYMBOL(clear_page)
> 

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

Re: [PATCH v2 1/2] arm64: clear_page() shouldn't use DC ZVA when DCZID_EL0.DZP == 1

[Reiji Watanabe]

On Tue, Nov 16, 2021 at 3:00 PM Robin Murphy <robin.murphy@arm.com> wrote:
>
> On 2021-11-08 07:11, Reiji Watanabe wrote:
> > Currently, clear_page() uses DC ZVA instruction unconditionally.  But it
> > should make sure that DCZID_EL0.DZP, which indicates whether or not use
> > of DC ZVA instruction is prohibited, is zero when using the instruction.
> > Use STP as memset does instead when DCZID_EL0.DZP == 1.
>
> Reviewed-by: Robin Murphy <robin.murphy@arm.com>
>
> FWIW I did eventually figure out the "pre-bias" trick from v1 thanks to
> Mark's nod toward the original context, but a quick survey of various
> optimisation guides implied that the explicit add should generally be
> preferred over post-index writeback anyway, so I think we're all good here.

Thank you for the review!
The original code, which used *pre*-index (not post-index) addressing,
made no significant difference in page_clear performance on my test
environment from the current code.

Now, I am looking at creating v3 patches to use stnp instead of stp
in page_clear (NOTE: DC ZVA shows much better performance on my test
system than stp/stnp).

Although using stnp didn't show significant difference in clear_page()
performance on my test system from stp (no significant difference in
cache-misses, cache_refill, cache_wb, or cache_allocate event counter
either), using stnp should be more appropriate for page_clear than stp,
and I understand it could show better performance on some CPUs.

Thanks,
Reiji




>
> Cheers,
> Robin.
>
> > Fixes: f27bb139c387 ("arm64: Miscellaneous library functions")
> > Signed-off-by: Reiji Watanabe <reijiw@google.com>
> > ---
> >   arch/arm64/lib/clear_page.S | 10 ++++++++++
> >   1 file changed, 10 insertions(+)
> >
> > diff --git a/arch/arm64/lib/clear_page.S b/arch/arm64/lib/clear_page.S
> > index b84b179edba3..ea3a6c927fe8 100644
> > --- a/arch/arm64/lib/clear_page.S
> > +++ b/arch/arm64/lib/clear_page.S
> > @@ -16,6 +16,7 @@
> >    */
> >   SYM_FUNC_START_PI(clear_page)
> >       mrs     x1, dczid_el0
> > +     tbnz    x1, #4, 2f      /* Branch if DC ZVA is prohibited */
> >       and     w1, w1, #0xf
> >       mov     x2, #4
> >       lsl     x1, x2, x1
> > @@ -25,5 +26,14 @@ SYM_FUNC_START_PI(clear_page)
> >       tst     x0, #(PAGE_SIZE - 1)
> >       b.ne    1b
> >       ret
> > +
> > +2:   stp     xzr, xzr, [x0]
> > +     stp     xzr, xzr, [x0, #16]
> > +     stp     xzr, xzr, [x0, #32]
> > +     stp     xzr, xzr, [x0, #48]
> > +     add     x0, x0, #64
> > +     tst     x0, #(PAGE_SIZE - 1)
> > +     b.ne    2b
> > +     ret
> >   SYM_FUNC_END_PI(clear_page)
> >   EXPORT_SYMBOL(clear_page)
> >

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

Re: [PATCH v2 1/2] arm64: clear_page() shouldn't use DC ZVA when DCZID_EL0.DZP == 1

[Robin Murphy]

On 2021-11-18 08:18, Reiji Watanabe wrote:
> On Tue, Nov 16, 2021 at 3:00 PM Robin Murphy <robin.murphy@arm.com> wrote:
>>
>> On 2021-11-08 07:11, Reiji Watanabe wrote:
>>> Currently, clear_page() uses DC ZVA instruction unconditionally.  But it
>>> should make sure that DCZID_EL0.DZP, which indicates whether or not use
>>> of DC ZVA instruction is prohibited, is zero when using the instruction.
>>> Use STP as memset does instead when DCZID_EL0.DZP == 1.
>>
>> Reviewed-by: Robin Murphy <robin.murphy@arm.com>
>>
>> FWIW I did eventually figure out the "pre-bias" trick from v1 thanks to
>> Mark's nod toward the original context, but a quick survey of various
>> optimisation guides implied that the explicit add should generally be
>> preferred over post-index writeback anyway, so I think we're all good here.
> 
> Thank you for the review!
> The original code, which used *pre*-index (not post-index) addressing,

Oops, in the context I think I meant writeback in general anyway. This 
is what happens when a sudden urge to review random patches at 11PM 
strikes :)

> made no significant difference in page_clear performance on my test
> environment from the current code.
> 
> Now, I am looking at creating v3 patches to use stnp instead of stp
> in page_clear (NOTE: DC ZVA shows much better performance on my test
> system than stp/stnp).
> 
> Although using stnp didn't show significant difference in clear_page()
> performance on my test system from stp (no significant difference in
> cache-misses, cache_refill, cache_wb, or cache_allocate event counter
> either), using stnp should be more appropriate for page_clear than stp,
> and I understand it could show better performance on some CPUs.

Indeed - certainly most Arm Ltd. cores tend to be good at spotting the 
store pattern and switching into write-streaming mode automatically - 
but semantically, STNP probably is appropriate for the great majority of 
clear_page() usage. Feel free to keep my review tag with that change.

Thanks,
Robin.

_______________________________________________
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: mte: DC {GVA,GZVA} shouldn't be used when DCZID_EL0.DZP == 1

[Reiji Watanabe]

Currently, mte_set_mem_tag_range() and mte_zero_clear_page_tags() use
DC {GVA,GZVA} unconditionally.  But, they should make sure that
DCZID_EL0.DZP, which indicates whether or not use of those instructions
is prohibited, is zero when using those instructions.
Use ST{G,ZG} instead when DCZID_EL0.DZP == 1.

Fixes: 013bb59dbb7c ("arm64: mte: handle tags zeroing at page allocation time")
Fixes: 3d0cca0b02ac ("kasan: speed up mte_set_mem_tag_range")
Signed-off-by: Reiji Watanabe <reijiw@google.com>
---
 arch/arm64/include/asm/mte-kasan.h | 8 +++++---
 arch/arm64/lib/mte.S               | 8 +++++++-
 2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/include/asm/mte-kasan.h b/arch/arm64/include/asm/mte-kasan.h
index 22420e1f8c03..26e013e540ae 100644
--- a/arch/arm64/include/asm/mte-kasan.h
+++ b/arch/arm64/include/asm/mte-kasan.h
@@ -84,10 +84,12 @@ static inline void __dc_gzva(u64 p)
 static inline void mte_set_mem_tag_range(void *addr, size_t size, u8 tag,
 					 bool init)
 {
-	u64 curr, mask, dczid_bs, end1, end2, end3;
+	u64 curr, mask, dczid, dczid_bs, dczid_dzp, end1, end2, end3;
 
 	/* Read DC G(Z)VA block size from the system register. */
-	dczid_bs = 4ul << (read_cpuid(DCZID_EL0) & 0xf);
+	dczid = read_cpuid(DCZID_EL0);
+	dczid_bs = 4ul << (dczid & 0xf);
+	dczid_dzp = (dczid >> 4) & 1;
 
 	curr = (u64)__tag_set(addr, tag);
 	mask = dczid_bs - 1;
@@ -106,7 +108,7 @@ static inline void mte_set_mem_tag_range(void *addr, size_t size, u8 tag,
 	 */
 #define SET_MEMTAG_RANGE(stg_post, dc_gva)		\
 	do {						\
-		if (size >= 2 * dczid_bs) {		\
+		if (!dczid_dzp && size >= 2 * dczid_bs) {\
 			do {				\
 				curr = stg_post(curr);	\
 			} while (curr < end1);		\
diff --git a/arch/arm64/lib/mte.S b/arch/arm64/lib/mte.S
index e83643b3995f..e62c048af337 100644
--- a/arch/arm64/lib/mte.S
+++ b/arch/arm64/lib/mte.S
@@ -43,17 +43,23 @@ SYM_FUNC_END(mte_clear_page_tags)
  *	x0 - address to the beginning of the page
  */
 SYM_FUNC_START(mte_zero_clear_page_tags)
+	and	x0, x0, #(1 << MTE_TAG_SHIFT) - 1	// clear the tag
 	mrs	x1, dczid_el0
+	tbnz	x1, #4, 2f	// Branch if DC GZVA is prohibited
 	and	w1, w1, #0xf
 	mov	x2, #4
 	lsl	x1, x2, x1
-	and	x0, x0, #(1 << MTE_TAG_SHIFT) - 1	// clear the tag
 
 1:	dc	gzva, x0
 	add	x0, x0, x1
 	tst	x0, #(PAGE_SIZE - 1)
 	b.ne	1b
 	ret
+
+2:	stzg	x0, [x0], #16
+	tst	x0, #(PAGE_SIZE - 1)
+	b.ne	2b
+	ret
 SYM_FUNC_END(mte_zero_clear_page_tags)
 
 /*
-- 
2.34.0.rc0.344.g81b53c2807-goog


_______________________________________________
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: mte: DC {GVA,GZVA} shouldn't be used when DCZID_EL0.DZP == 1

[Catalin Marinas]

On Sun, Nov 07, 2021 at 11:11:49PM -0800, Reiji Watanabe wrote:
> diff --git a/arch/arm64/lib/mte.S b/arch/arm64/lib/mte.S
> index e83643b3995f..e62c048af337 100644
> --- a/arch/arm64/lib/mte.S
> +++ b/arch/arm64/lib/mte.S
> @@ -43,17 +43,23 @@ SYM_FUNC_END(mte_clear_page_tags)
>   *	x0 - address to the beginning of the page
>   */
>  SYM_FUNC_START(mte_zero_clear_page_tags)
> +	and	x0, x0, #(1 << MTE_TAG_SHIFT) - 1	// clear the tag
>  	mrs	x1, dczid_el0
> +	tbnz	x1, #4, 2f	// Branch if DC GZVA is prohibited
>  	and	w1, w1, #0xf
>  	mov	x2, #4
>  	lsl	x1, x2, x1
> -	and	x0, x0, #(1 << MTE_TAG_SHIFT) - 1	// clear the tag
>  
>  1:	dc	gzva, x0
>  	add	x0, x0, x1
>  	tst	x0, #(PAGE_SIZE - 1)
>  	b.ne	1b
>  	ret
> +
> +2:	stzg	x0, [x0], #16

Nitpick: MTE_GRANULE_SIZE instead of 16.

> +	tst	x0, #(PAGE_SIZE - 1)
> +	b.ne	2b
> +	ret
>  SYM_FUNC_END(mte_zero_clear_page_tags)

We can use stz2g here since we know it's always a PAGE_SIZE and an even
number of tag granules.

-- 
Catalin

_______________________________________________
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: mte: DC {GVA,GZVA} shouldn't be used when DCZID_EL0.DZP == 1

[Catalin Marinas]

On Fri, Dec 03, 2021 at 06:29:48PM +0000, Catalin Marinas wrote:
> On Sun, Nov 07, 2021 at 11:11:49PM -0800, Reiji Watanabe wrote:
> > diff --git a/arch/arm64/lib/mte.S b/arch/arm64/lib/mte.S
> > index e83643b3995f..e62c048af337 100644
> > --- a/arch/arm64/lib/mte.S
> > +++ b/arch/arm64/lib/mte.S
> > @@ -43,17 +43,23 @@ SYM_FUNC_END(mte_clear_page_tags)
> >   *	x0 - address to the beginning of the page
> >   */
> >  SYM_FUNC_START(mte_zero_clear_page_tags)
> > +	and	x0, x0, #(1 << MTE_TAG_SHIFT) - 1	// clear the tag
> >  	mrs	x1, dczid_el0
> > +	tbnz	x1, #4, 2f	// Branch if DC GZVA is prohibited
> >  	and	w1, w1, #0xf
> >  	mov	x2, #4
> >  	lsl	x1, x2, x1
> > -	and	x0, x0, #(1 << MTE_TAG_SHIFT) - 1	// clear the tag
> >  
> >  1:	dc	gzva, x0
> >  	add	x0, x0, x1
> >  	tst	x0, #(PAGE_SIZE - 1)
> >  	b.ne	1b
> >  	ret
> > +
> > +2:	stzg	x0, [x0], #16
> 
> Nitpick: MTE_GRANULE_SIZE instead of 16.
> 
> > +	tst	x0, #(PAGE_SIZE - 1)
> > +	b.ne	2b
> > +	ret
> >  SYM_FUNC_END(mte_zero_clear_page_tags)
> 
> We can use stz2g here since we know it's always a PAGE_SIZE and an even
> number of tag granules.

I should have replied on v3. The comment is the same.

-- 
Catalin

_______________________________________________
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: mte: DC {GVA,GZVA} shouldn't be used when DCZID_EL0.DZP == 1

[Reiji Watanabe]

On Fri, Dec 3, 2021 at 10:52 AM Catalin Marinas <catalin.marinas@arm.com> wrote:
>
> On Fri, Dec 03, 2021 at 06:29:48PM +0000, Catalin Marinas wrote:
> > On Sun, Nov 07, 2021 at 11:11:49PM -0800, Reiji Watanabe wrote:
> > > diff --git a/arch/arm64/lib/mte.S b/arch/arm64/lib/mte.S
> > > index e83643b3995f..e62c048af337 100644
> > > --- a/arch/arm64/lib/mte.S
> > > +++ b/arch/arm64/lib/mte.S
> > > @@ -43,17 +43,23 @@ SYM_FUNC_END(mte_clear_page_tags)
> > >   * x0 - address to the beginning of the page
> > >   */
> > >  SYM_FUNC_START(mte_zero_clear_page_tags)
> > > +   and     x0, x0, #(1 << MTE_TAG_SHIFT) - 1       // clear the tag
> > >     mrs     x1, dczid_el0
> > > +   tbnz    x1, #4, 2f      // Branch if DC GZVA is prohibited
> > >     and     w1, w1, #0xf
> > >     mov     x2, #4
> > >     lsl     x1, x2, x1
> > > -   and     x0, x0, #(1 << MTE_TAG_SHIFT) - 1       // clear the tag
> > >
> > >  1: dc      gzva, x0
> > >     add     x0, x0, x1
> > >     tst     x0, #(PAGE_SIZE - 1)
> > >     b.ne    1b
> > >     ret
> > > +
> > > +2: stzg    x0, [x0], #16
> >
> > Nitpick: MTE_GRANULE_SIZE instead of 16.

I will fix it.

> >
> > > +   tst     x0, #(PAGE_SIZE - 1)
> > > +   b.ne    2b
> > > +   ret
> > >  SYM_FUNC_END(mte_zero_clear_page_tags)
> >
> > We can use stz2g here since we know it's always a PAGE_SIZE and an even
> > number of tag granules.

Ah, yes, I will fix it using stz2g.


> I should have replied on v3. The comment is the same.

Thank you for the review and I will be working for v4 !

Regards,
Reiji

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