[PATCH 0/2] Small fixups for the Zbb string functions

[PATCH 0/2] Small fixups for the Zbb string functions

[Heiko Stuebner]

From: Heiko Stuebner <heiko.stuebner@vrull.eu>

I didn't manage to address Andrews comments on v5 [0] in time
before the series got applied, so here are two addon comments
that hopefully do his comments justice now.


[0] https://lore.kernel.org/r/20230117122447.y6tdsmsxqdwf76ri@orel

Heiko Stuebner (2):
  RISC-V: fix ordering of Zbb extension
  RISC-V: improve string-function assembly

 arch/riscv/kernel/cpu.c  |  2 +-
 arch/riscv/lib/strcmp.S  |  6 ++++--
 arch/riscv/lib/strlen.S  | 10 +++++-----
 arch/riscv/lib/strncmp.S | 16 +++++++---------
 4 files changed, 17 insertions(+), 17 deletions(-)

-- 
2.39.0

[PATCH 1/2] RISC-V: fix ordering of Zbb extension

[Heiko Stuebner]

From: Heiko Stuebner <heiko.stuebner@vrull.eu>

As Andrew reported,
    Zb* comes after Zi* according 27.11 "Subset Naming Convention"
so fix the ordering accordingly.

Reported-by: Andrew Jones <ajones@ventanamicro.com>
Signed-off-by: Heiko Stuebner <heiko.stuebner@vrull.eu>
---
 arch/riscv/kernel/cpu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
index 420228e219f7..8400f0cc9704 100644
--- a/arch/riscv/kernel/cpu.c
+++ b/arch/riscv/kernel/cpu.c
@@ -185,9 +185,9 @@ arch_initcall(riscv_cpuinfo_init);
  * New entries to this struct should follow the ordering rules described above.
  */
 static struct riscv_isa_ext_data isa_ext_arr[] = {
-	__RISCV_ISA_EXT_DATA(zbb, RISCV_ISA_EXT_ZBB),
 	__RISCV_ISA_EXT_DATA(zicbom, RISCV_ISA_EXT_ZICBOM),
 	__RISCV_ISA_EXT_DATA(zihintpause, RISCV_ISA_EXT_ZIHINTPAUSE),
+	__RISCV_ISA_EXT_DATA(zbb, RISCV_ISA_EXT_ZBB),
 	__RISCV_ISA_EXT_DATA(sscofpmf, RISCV_ISA_EXT_SSCOFPMF),
 	__RISCV_ISA_EXT_DATA(sstc, RISCV_ISA_EXT_SSTC),
 	__RISCV_ISA_EXT_DATA(svinval, RISCV_ISA_EXT_SVINVAL),
-- 
2.39.0

Re: [PATCH 1/2] RISC-V: fix ordering of Zbb extension

[Conor Dooley]

Hey Heiko,

On 8 February 2023 22:53:27 GMT, Heiko Stuebner <heiko@sntech.de> wrote:
>From: Heiko Stuebner <heiko.stuebner@vrull.eu>
>
>As Andrew reported,
>    Zb* comes after Zi* according 27.11 "Subset Naming Convention"
>so fix the ordering accordingly.
>
>Reported-by: Andrew Jones <ajones@ventanamicro.com>
>Signed-off-by: Heiko Stuebner <heiko.stuebner@vrull.eu>

The whole "getting it wrong immediately after fixing it up" ;)

Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

>---
> arch/riscv/kernel/cpu.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
>index 420228e219f7..8400f0cc9704 100644
>--- a/arch/riscv/kernel/cpu.c
>+++ b/arch/riscv/kernel/cpu.c
>@@ -185,9 +185,9 @@ arch_initcall(riscv_cpuinfo_init);
>  * New entries to this struct should follow the ordering rules described above.
>  */
> static struct riscv_isa_ext_data isa_ext_arr[] = {
>-	__RISCV_ISA_EXT_DATA(zbb, RISCV_ISA_EXT_ZBB),
> 	__RISCV_ISA_EXT_DATA(zicbom, RISCV_ISA_EXT_ZICBOM),
> 	__RISCV_ISA_EXT_DATA(zihintpause, RISCV_ISA_EXT_ZIHINTPAUSE),
>+	__RISCV_ISA_EXT_DATA(zbb, RISCV_ISA_EXT_ZBB),
> 	__RISCV_ISA_EXT_DATA(sscofpmf, RISCV_ISA_EXT_SSCOFPMF),
> 	__RISCV_ISA_EXT_DATA(sstc, RISCV_ISA_EXT_SSTC),
> 	__RISCV_ISA_EXT_DATA(svinval, RISCV_ISA_EXT_SVINVAL),

Re: [PATCH 1/2] RISC-V: fix ordering of Zbb extension

[Heiko Stübner]

Am Donnerstag, 9. Februar 2023, 00:20:10 CET schrieb Conor Dooley:
> Hey Heiko,
> 
> On 8 February 2023 22:53:27 GMT, Heiko Stuebner <heiko@sntech.de> wrote:
> >From: Heiko Stuebner <heiko.stuebner@vrull.eu>
> >
> >As Andrew reported,
> >    Zb* comes after Zi* according 27.11 "Subset Naming Convention"
> >so fix the ordering accordingly.
> >
> >Reported-by: Andrew Jones <ajones@ventanamicro.com>
> >Signed-off-by: Heiko Stuebner <heiko.stuebner@vrull.eu>
> 
> The whole "getting it wrong immediately after fixing it up" ;)
> 
> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

I'm still hopefully that I'll learn at some point that "b" comes after "i",
at least with riscv extensions. Decades of sorting the other way around are
hard to break :-D .

> >---
> > arch/riscv/kernel/cpu.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> >diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
> >index 420228e219f7..8400f0cc9704 100644
> >--- a/arch/riscv/kernel/cpu.c
> >+++ b/arch/riscv/kernel/cpu.c
> >@@ -185,9 +185,9 @@ arch_initcall(riscv_cpuinfo_init);
> >  * New entries to this struct should follow the ordering rules described above.
> >  */
> > static struct riscv_isa_ext_data isa_ext_arr[] = {
> >-	__RISCV_ISA_EXT_DATA(zbb, RISCV_ISA_EXT_ZBB),
> > 	__RISCV_ISA_EXT_DATA(zicbom, RISCV_ISA_EXT_ZICBOM),
> > 	__RISCV_ISA_EXT_DATA(zihintpause, RISCV_ISA_EXT_ZIHINTPAUSE),
> >+	__RISCV_ISA_EXT_DATA(zbb, RISCV_ISA_EXT_ZBB),
> > 	__RISCV_ISA_EXT_DATA(sscofpmf, RISCV_ISA_EXT_SSCOFPMF),
> > 	__RISCV_ISA_EXT_DATA(sstc, RISCV_ISA_EXT_SSTC),
> > 	__RISCV_ISA_EXT_DATA(svinval, RISCV_ISA_EXT_SVINVAL),
> 

Re: [PATCH 1/2] RISC-V: fix ordering of Zbb extension

[Andrew Jones]

On Wed, Feb 08, 2023 at 11:20:10PM +0000, Conor Dooley wrote:
> Hey Heiko,
> 
> On 8 February 2023 22:53:27 GMT, Heiko Stuebner <heiko@sntech.de> wrote:
> >From: Heiko Stuebner <heiko.stuebner@vrull.eu>
> >
> >As Andrew reported,
> >    Zb* comes after Zi* according 27.11 "Subset Naming Convention"
> >so fix the ordering accordingly.
> >
> >Reported-by: Andrew Jones <ajones@ventanamicro.com>
> >Signed-off-by: Heiko Stuebner <heiko.stuebner@vrull.eu>
> 
> The whole "getting it wrong immediately after fixing it up" ;)

Hi Conor,

Do you know any patchwork savvy people that could whip up a check
for this array? :-)

drew

Re: [PATCH 1/2] RISC-V: fix ordering of Zbb extension

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 845 bytes --]

On Thu, Feb 09, 2023 at 09:25:20AM +0100, Andrew Jones wrote:
> On Wed, Feb 08, 2023 at 11:20:10PM +0000, Conor Dooley wrote:
> > Hey Heiko,
> > 
> > On 8 February 2023 22:53:27 GMT, Heiko Stuebner <heiko@sntech.de> wrote:
> > >From: Heiko Stuebner <heiko.stuebner@vrull.eu>
> > >
> > >As Andrew reported,
> > >    Zb* comes after Zi* according 27.11 "Subset Naming Convention"
> > >so fix the ordering accordingly.
> > >
> > >Reported-by: Andrew Jones <ajones@ventanamicro.com>
> > >Signed-off-by: Heiko Stuebner <heiko.stuebner@vrull.eu>
> > 
> > The whole "getting it wrong immediately after fixing it up" ;)
> 
> Hi Conor,
> 
> Do you know any patchwork savvy people that could whip up a check
> for this array? :-)

Maybe that is more of a checkpatch type thing?

Either way, I'll put it on the todo list I suppose!

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

Re: [PATCH 1/2] RISC-V: fix ordering of Zbb extension

[Andrew Jones]

On Thu, Feb 09, 2023 at 09:03:50AM +0000, Conor Dooley wrote:
> On Thu, Feb 09, 2023 at 09:25:20AM +0100, Andrew Jones wrote:
> > On Wed, Feb 08, 2023 at 11:20:10PM +0000, Conor Dooley wrote:
> > > Hey Heiko,
> > > 
> > > On 8 February 2023 22:53:27 GMT, Heiko Stuebner <heiko@sntech.de> wrote:
> > > >From: Heiko Stuebner <heiko.stuebner@vrull.eu>
> > > >
> > > >As Andrew reported,
> > > >    Zb* comes after Zi* according 27.11 "Subset Naming Convention"
> > > >so fix the ordering accordingly.
> > > >
> > > >Reported-by: Andrew Jones <ajones@ventanamicro.com>
> > > >Signed-off-by: Heiko Stuebner <heiko.stuebner@vrull.eu>
> > > 
> > > The whole "getting it wrong immediately after fixing it up" ;)
> > 
> > Hi Conor,
> > 
> > Do you know any patchwork savvy people that could whip up a check
> > for this array? :-)
> 
> Maybe that is more of a checkpatch type thing?

I think this is too specific for general checkpatch. I once proposed on
the KVM mailing list that checkpatch should gain support for plugins,
allowing specific directories to provide a .checkpatch script, or
whatever, where it puts its own checks. I never followed-up by actually
proposing that to checkpatch maintainers though.

> 
> Either way, I'll put it on the todo list I suppose!

In the absence of checkpatch plugins, I think subsystem-specific
patch management tools, like patchwork, are the next best place
to put specific checks. But, I agree it's a bit late. It'd be better
if the developers could run the checks themselves before posting.

Thanks,
drew

Re: [PATCH 1/2] RISC-V: fix ordering of Zbb extension

[Andrew Jones]

On Wed, Feb 08, 2023 at 11:53:27PM +0100, Heiko Stuebner wrote:
> From: Heiko Stuebner <heiko.stuebner@vrull.eu>
> 
> As Andrew reported,
>     Zb* comes after Zi* according 27.11 "Subset Naming Convention"
> so fix the ordering accordingly.
> 
> Reported-by: Andrew Jones <ajones@ventanamicro.com>
> Signed-off-by: Heiko Stuebner <heiko.stuebner@vrull.eu>
> ---
>  arch/riscv/kernel/cpu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
> index 420228e219f7..8400f0cc9704 100644
> --- a/arch/riscv/kernel/cpu.c
> +++ b/arch/riscv/kernel/cpu.c
> @@ -185,9 +185,9 @@ arch_initcall(riscv_cpuinfo_init);
>   * New entries to this struct should follow the ordering rules described above.
>   */
>  static struct riscv_isa_ext_data isa_ext_arr[] = {
> -	__RISCV_ISA_EXT_DATA(zbb, RISCV_ISA_EXT_ZBB),
>  	__RISCV_ISA_EXT_DATA(zicbom, RISCV_ISA_EXT_ZICBOM),
>  	__RISCV_ISA_EXT_DATA(zihintpause, RISCV_ISA_EXT_ZIHINTPAUSE),
> +	__RISCV_ISA_EXT_DATA(zbb, RISCV_ISA_EXT_ZBB),
>  	__RISCV_ISA_EXT_DATA(sscofpmf, RISCV_ISA_EXT_SSCOFPMF),
>  	__RISCV_ISA_EXT_DATA(sstc, RISCV_ISA_EXT_SSTC),
>  	__RISCV_ISA_EXT_DATA(svinval, RISCV_ISA_EXT_SVINVAL),
> -- 
> 2.39.0
>

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

[PATCH 2/2] RISC-V: improve string-function assembly

[Heiko Stuebner]

From: Heiko Stuebner <heiko.stuebner@vrull.eu>

Adapt the suggestions for the assembly string functions that Andrew
suggested but that I didn't manage to include into the series that
got applied.

This includes improvements to two comments, removal of unneeded labels
and moving one instruction slightly higher to contradict an
explanatory comment.

Suggested-by: Andrew Jones <ajones@ventanamicro.com>
Signed-off-by: Heiko Stuebner <heiko.stuebner@vrull.eu>
---
 arch/riscv/lib/strcmp.S  |  6 ++++--
 arch/riscv/lib/strlen.S  | 10 +++++-----
 arch/riscv/lib/strncmp.S | 16 +++++++---------
 3 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/arch/riscv/lib/strcmp.S b/arch/riscv/lib/strcmp.S
index 8148b6418f61..fb186bf28f07 100644
--- a/arch/riscv/lib/strcmp.S
+++ b/arch/riscv/lib/strcmp.S
@@ -40,7 +40,9 @@ SYM_FUNC_START(strcmp)
 	ret
 
 /*
- * Variant of strcmp using the ZBB extension if available
+ * Variant of strcmp using the ZBB extension if available.
+ * The code was published as part of the bitmanip manual
+ * in Appendix A.
  */
 #ifdef CONFIG_RISCV_ISA_ZBB
 strcmp_zbb:
@@ -57,7 +59,7 @@ strcmp_zbb:
 	 *   a1 - string2
 	 *
 	 * Clobbers
-	 *   t0, t1, t2, t3, t4, t5
+	 *   t0, t1, t2, t3, t4
 	 */
 
 	or	t2, a0, a1
diff --git a/arch/riscv/lib/strlen.S b/arch/riscv/lib/strlen.S
index 0f9dbf93301a..898466f67000 100644
--- a/arch/riscv/lib/strlen.S
+++ b/arch/riscv/lib/strlen.S
@@ -96,7 +96,7 @@ strlen_zbb:
 	 * of valid bytes in this chunk.
 	 */
 	srli	a0, t1, 3
-	bgtu	t3, a0, 3f
+	bgtu	t3, a0, 2f
 
 	/* Prepare for the word comparison loop. */
 	addi	t2, t0, SZREG
@@ -112,20 +112,20 @@ strlen_zbb:
 	addi	t0, t0, SZREG
 	orc.b	t1, t1
 	beq	t1, t3, 1b
-2:
+
 	not	t1, t1
 	CZ	t1, t1
+	srli	t1, t1, 3
 
-	/* Get number of processed words.  */
+	/* Get number of processed bytes. */
 	sub	t2, t0, t2
 
 	/* Add number of characters in the first word.  */
 	add	a0, a0, t2
-	srli	t1, t1, 3
 
 	/* Add number of characters in the last word.  */
 	add	a0, a0, t1
-3:
+2:
 	ret
 
 .option pop
diff --git a/arch/riscv/lib/strncmp.S b/arch/riscv/lib/strncmp.S
index 7940ddab2d48..e36f5a6e1b16 100644
--- a/arch/riscv/lib/strncmp.S
+++ b/arch/riscv/lib/strncmp.S
@@ -70,7 +70,7 @@ strncmp_zbb:
 	li	t5, -1
 	and	t2, t2, SZREG-1
 	add	t4, a0, a2
-	bnez	t2, 4f
+	bnez	t2, 3f
 
 	/* Adjust limit for fast-path.  */
 	andi	t6, t4, -SZREG
@@ -114,23 +114,21 @@ strncmp_zbb:
 	ret
 
 	/* Simple loop for misaligned strings.  */
-3:
-	/* Restore limit for slow-path.  */
 	.p2align 3
-4:
-	bge	a0, t4, 6f
+3:
+	bge	a0, t4, 5f
 	lbu	t0, 0(a0)
 	lbu	t1, 0(a1)
 	addi	a0, a0, 1
 	addi	a1, a1, 1
-	bne	t0, t1, 5f
-	bnez	t0, 4b
+	bne	t0, t1, 4f
+	bnez	t0, 3b
 
-5:
+4:
 	sub	a0, t0, t1
 	ret
 
-6:
+5:
 	li	a0, 0
 	ret
 
-- 
2.39.0

Re: [PATCH 2/2] RISC-V: improve string-function assembly

[Andrew Jones]

On Wed, Feb 08, 2023 at 11:53:28PM +0100, Heiko Stuebner wrote:
> From: Heiko Stuebner <heiko.stuebner@vrull.eu>
> 
> Adapt the suggestions for the assembly string functions that Andrew
> suggested but that I didn't manage to include into the series that
> got applied.
> 
> This includes improvements to two comments, removal of unneeded labels
> and moving one instruction slightly higher to contradict an
> explanatory comment.
> 
> Suggested-by: Andrew Jones <ajones@ventanamicro.com>
> Signed-off-by: Heiko Stuebner <heiko.stuebner@vrull.eu>
> ---
>  arch/riscv/lib/strcmp.S  |  6 ++++--
>  arch/riscv/lib/strlen.S  | 10 +++++-----
>  arch/riscv/lib/strncmp.S | 16 +++++++---------
>  3 files changed, 16 insertions(+), 16 deletions(-)
>

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

Re: [PATCH 0/2] Small fixups for the Zbb string functions

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 422 bytes --]

On Wed, Feb 08, 2023 at 11:53:26PM +0100, Heiko Stuebner wrote:
> From: Heiko Stuebner <heiko.stuebner@vrull.eu>
> 
> I didn't manage to address Andrews comments on v5 [0] in time
> before the series got applied, so here are two addon comments
> that hopefully do his comments justice now.

Running this on my jh7110 at the moment, so I suppose this is:
Tested-by: Conor Dooley <conor.dooley@microchip.com>

;)

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

Re: [PATCH 0/2] Small fixups for the Zbb string functions

[patchwork-bot+linux-riscv]

Hello:

This series was applied to riscv/linux.git (for-next)
by Palmer Dabbelt <palmer@rivosinc.com>:

On Wed,  8 Feb 2023 23:53:26 +0100 you wrote:
> From: Heiko Stuebner <heiko.stuebner@vrull.eu>
> 
> I didn't manage to address Andrews comments on v5 [0] in time
> before the series got applied, so here are two addon comments
> that hopefully do his comments justice now.
> 
> 
> [...]

Here is the summary with links:
  - [1/2] RISC-V: fix ordering of Zbb extension
    https://git.kernel.org/riscv/c/1eac28201ac0
  - [2/2] RISC-V: improve string-function assembly
    (no matching commit)

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: (subset) [PATCH 0/2] Small fixups for the Zbb string functions

[Palmer Dabbelt]

On Wed, 08 Feb 2023 23:53:26 +0100, Heiko Stuebner wrote:
> From: Heiko Stuebner <heiko.stuebner@vrull.eu>
> 
> I didn't manage to address Andrews comments on v5 [0] in time
> before the series got applied, so here are two addon comments
> that hopefully do his comments justice now.
> 
> 
> [...]

Applied, thanks!

[2/2] RISC-V: improve string-function assembly
      https://git.kernel.org/palmer/c/6934cf8a3e0b

Best regards,
-- 
Palmer Dabbelt <palmer@rivosinc.com>

Re: [PATCH 0/2] Small fixups for the Zbb string functions

[patchwork-bot+linux-riscv]

Hello:

This series was applied to riscv/linux.git (for-next)
by Palmer Dabbelt <palmer@rivosinc.com>:

On Wed,  8 Feb 2023 23:53:26 +0100 you wrote:
> From: Heiko Stuebner <heiko.stuebner@vrull.eu>
> 
> I didn't manage to address Andrews comments on v5 [0] in time
> before the series got applied, so here are two addon comments
> that hopefully do his comments justice now.
> 
> 
> [...]

Here is the summary with links:
  - [1/2] RISC-V: fix ordering of Zbb extension
    (no matching commit)
  - [2/2] RISC-V: improve string-function assembly
    https://git.kernel.org/riscv/c/6934cf8a3e0b

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html