* [PATCH] riscv, lib: Fix Zbb strncmp
@ 2023-02-28 18:42 Björn Töpel
2023-02-28 19:12 ` Conor Dooley
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Björn Töpel @ 2023-02-28 18:42 UTC (permalink / raw)
To: Paul Walmsley, Palmer Dabbelt, Albert Ou, linux-riscv,
Christoph Muellner, Heiko Stuebner
Cc: Björn Töpel, Conor Dooley, Guenter Roeck, Andrew Jones
From: Björn Töpel <bjorn@rivosinc.com>
The Zbb optimized strncmp has two parts; a fast path that does XLEN/8B
per iteration, and a slow that does one byte per iteration.
The idea is to compare aligned XLEN chunks for most of strings, and do
the remainder tail in the slow path.
The Zbb strncmp has two issues in the fast path:
Incorrect remainder handling (wrong compare): Assume that the string
length is 9. On 64b systems, the fast path should do one iteration,
and one iteration in the slow path. Instead, both were done in the
fast path, which lead to incorrect results. An example:
strncmp("/dev/vda", "/dev/", 5);
Correct by changing "bgt" to "bge".
Missing NULL checks in the second string: This could lead to incorrect
results for:
strncmp("/dev/vda", "/dev/vda\0", 8);
Correct by adding an additional check.
Fixes: b6fcdb191e36 ("RISC-V: add zbb support to string functions")
Suggested-by: Heiko Stuebner <heiko.stuebner@vrull.eu>
Signed-off-by: Björn Töpel <bjorn@rivosinc.com>
---
arch/riscv/lib/strncmp.S | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/arch/riscv/lib/strncmp.S b/arch/riscv/lib/strncmp.S
index ee49595075be..efd3f3150a54 100644
--- a/arch/riscv/lib/strncmp.S
+++ b/arch/riscv/lib/strncmp.S
@@ -78,11 +78,13 @@ strncmp_zbb:
/* Main loop for aligned string. */
.p2align 3
1:
- bgt a0, t6, 3f
+ bge a0, t6, 3f
REG_L t0, 0(a0)
REG_L t1, 0(a1)
orc.b t3, t0
bne t3, t5, 2f
+ orc.b t3, t1
+ bne t3, t5, 2f
addi a0, a0, SZREG
addi a1, a1, SZREG
beq t0, t1, 1b
base-commit: eb9be8310c58c166f9fae3b71c0ad9d6741b4897
--
2.37.2
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] riscv, lib: Fix Zbb strncmp
2023-02-28 18:42 [PATCH] riscv, lib: Fix Zbb strncmp Björn Töpel
@ 2023-02-28 19:12 ` Conor Dooley
2023-02-28 19:41 ` Björn Töpel
2023-02-28 19:53 ` Guenter Roeck
2023-02-28 21:18 ` Guenter Roeck
2023-03-01 3:20 ` patchwork-bot+linux-riscv
2 siblings, 2 replies; 7+ messages in thread
From: Conor Dooley @ 2023-02-28 19:12 UTC (permalink / raw)
To: Björn Töpel
Cc: Paul Walmsley, Palmer Dabbelt, Albert Ou, linux-riscv,
Christoph Muellner, Heiko Stuebner, Björn Töpel,
Conor Dooley, Guenter Roeck, Andrew Jones
[-- Attachment #1.1: Type: text/plain, Size: 1368 bytes --]
On Tue, Feb 28, 2023 at 07:42:10PM +0100, Björn Töpel wrote:
> From: Björn Töpel <bjorn@rivosinc.com>
>
> The Zbb optimized strncmp has two parts; a fast path that does XLEN/8B
> per iteration, and a slow that does one byte per iteration.
>
> The idea is to compare aligned XLEN chunks for most of strings, and do
> the remainder tail in the slow path.
>
> The Zbb strncmp has two issues in the fast path:
>
> Incorrect remainder handling (wrong compare): Assume that the string
> length is 9. On 64b systems, the fast path should do one iteration,
> and one iteration in the slow path. Instead, both were done in the
> fast path, which lead to incorrect results. An example:
>
> strncmp("/dev/vda", "/dev/", 5);
>
> Correct by changing "bgt" to "bge".
>
> Missing NULL checks in the second string: This could lead to incorrect
> results for:
>
> strncmp("/dev/vda", "/dev/vda\0", 8);
>
> Correct by adding an additional check.
>
> Fixes: b6fcdb191e36 ("RISC-V: add zbb support to string functions")
> Suggested-by: Heiko Stuebner <heiko.stuebner@vrull.eu>
> Signed-off-by: Björn Töpel <bjorn@rivosinc.com>
With your new selftest stuff & on a JH7110:
Tested-by: Conor Dooley <conor.dooley@microchip.com>
Also, given the reporter, this should be:
Reported-by: Guenter Roeck <linux@roeck-us.net>
no?
Thanks,
Conor.
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
[-- Attachment #2: Type: text/plain, Size: 161 bytes --]
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] riscv, lib: Fix Zbb strncmp
2023-02-28 19:12 ` Conor Dooley
@ 2023-02-28 19:41 ` Björn Töpel
2023-02-28 19:53 ` Guenter Roeck
1 sibling, 0 replies; 7+ messages in thread
From: Björn Töpel @ 2023-02-28 19:41 UTC (permalink / raw)
To: Conor Dooley
Cc: Paul Walmsley, Palmer Dabbelt, Albert Ou, linux-riscv,
Christoph Muellner, Heiko Stuebner, Björn Töpel,
Conor Dooley, Guenter Roeck, Andrew Jones
Conor Dooley <conor@kernel.org> writes:
> On Tue, Feb 28, 2023 at 07:42:10PM +0100, Björn Töpel wrote:
>> From: Björn Töpel <bjorn@rivosinc.com>
>>
>> The Zbb optimized strncmp has two parts; a fast path that does XLEN/8B
>> per iteration, and a slow that does one byte per iteration.
>>
>> The idea is to compare aligned XLEN chunks for most of strings, and do
>> the remainder tail in the slow path.
>>
>> The Zbb strncmp has two issues in the fast path:
>>
>> Incorrect remainder handling (wrong compare): Assume that the string
>> length is 9. On 64b systems, the fast path should do one iteration,
>> and one iteration in the slow path. Instead, both were done in the
>> fast path, which lead to incorrect results. An example:
>>
>> strncmp("/dev/vda", "/dev/", 5);
>>
>> Correct by changing "bgt" to "bge".
>>
>> Missing NULL checks in the second string: This could lead to incorrect
>> results for:
>>
>> strncmp("/dev/vda", "/dev/vda\0", 8);
>>
>> Correct by adding an additional check.
>>
>> Fixes: b6fcdb191e36 ("RISC-V: add zbb support to string functions")
>> Suggested-by: Heiko Stuebner <heiko.stuebner@vrull.eu>
>> Signed-off-by: Björn Töpel <bjorn@rivosinc.com>
>
> With your new selftest stuff & on a JH7110:
> Tested-by: Conor Dooley <conor.dooley@microchip.com>
> Also, given the reporter, this should be:
> Reported-by: Guenter Roeck <linux@roeck-us.net>
> no?
Indeed. Sorry, Guenter, for missing that!
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] riscv, lib: Fix Zbb strncmp
2023-02-28 19:12 ` Conor Dooley
2023-02-28 19:41 ` Björn Töpel
@ 2023-02-28 19:53 ` Guenter Roeck
1 sibling, 0 replies; 7+ messages in thread
From: Guenter Roeck @ 2023-02-28 19:53 UTC (permalink / raw)
To: Conor Dooley, Björn Töpel
Cc: Paul Walmsley, Palmer Dabbelt, Albert Ou, linux-riscv,
Christoph Muellner, Heiko Stuebner, Björn Töpel,
Conor Dooley, Andrew Jones
On 2/28/23 11:12, Conor Dooley wrote:
> On Tue, Feb 28, 2023 at 07:42:10PM +0100, Björn Töpel wrote:
>> From: Björn Töpel <bjorn@rivosinc.com>
>>
>> The Zbb optimized strncmp has two parts; a fast path that does XLEN/8B
>> per iteration, and a slow that does one byte per iteration.
>>
>> The idea is to compare aligned XLEN chunks for most of strings, and do
>> the remainder tail in the slow path.
>>
>> The Zbb strncmp has two issues in the fast path:
>>
>> Incorrect remainder handling (wrong compare): Assume that the string
>> length is 9. On 64b systems, the fast path should do one iteration,
>> and one iteration in the slow path. Instead, both were done in the
>> fast path, which lead to incorrect results. An example:
>>
>> strncmp("/dev/vda", "/dev/", 5);
>>
>> Correct by changing "bgt" to "bge".
>>
>> Missing NULL checks in the second string: This could lead to incorrect
>> results for:
>>
>> strncmp("/dev/vda", "/dev/vda\0", 8);
>>
>> Correct by adding an additional check.
>>
>> Fixes: b6fcdb191e36 ("RISC-V: add zbb support to string functions")
>> Suggested-by: Heiko Stuebner <heiko.stuebner@vrull.eu>
>> Signed-off-by: Björn Töpel <bjorn@rivosinc.com>
>
> With your new selftest stuff & on a JH7110:
> Tested-by: Conor Dooley <conor.dooley@microchip.com>
> Also, given the reporter, this should be:
> Reported-by: Guenter Roeck <linux@roeck-us.net>
> no?
>
No worries. I'll give the patch a try.
Guenter
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] riscv, lib: Fix Zbb strncmp
2023-02-28 18:42 [PATCH] riscv, lib: Fix Zbb strncmp Björn Töpel
2023-02-28 19:12 ` Conor Dooley
@ 2023-02-28 21:18 ` Guenter Roeck
2023-02-28 21:55 ` Palmer Dabbelt
2023-03-01 3:20 ` patchwork-bot+linux-riscv
2 siblings, 1 reply; 7+ messages in thread
From: Guenter Roeck @ 2023-02-28 21:18 UTC (permalink / raw)
To: Björn Töpel, Paul Walmsley, Palmer Dabbelt, Albert Ou,
linux-riscv, Christoph Muellner, Heiko Stuebner
Cc: Björn Töpel, Conor Dooley, Andrew Jones
On 2/28/23 10:42, Björn Töpel wrote:
> From: Björn Töpel <bjorn@rivosinc.com>
>
> The Zbb optimized strncmp has two parts; a fast path that does XLEN/8B
> per iteration, and a slow that does one byte per iteration.
>
> The idea is to compare aligned XLEN chunks for most of strings, and do
> the remainder tail in the slow path.
>
> The Zbb strncmp has two issues in the fast path:
>
> Incorrect remainder handling (wrong compare): Assume that the string
> length is 9. On 64b systems, the fast path should do one iteration,
> and one iteration in the slow path. Instead, both were done in the
> fast path, which lead to incorrect results. An example:
>
> strncmp("/dev/vda", "/dev/", 5);
>
> Correct by changing "bgt" to "bge".
>
> Missing NULL checks in the second string: This could lead to incorrect
> results for:
>
> strncmp("/dev/vda", "/dev/vda\0", 8);
>
> Correct by adding an additional check.
>
> Fixes: b6fcdb191e36 ("RISC-V: add zbb support to string functions")
> Suggested-by: Heiko Stuebner <heiko.stuebner@vrull.eu>
> Signed-off-by: Björn Töpel <bjorn@rivosinc.com>
Tested-by: Guenter Roeck <linux@roeck-us.net>
> ---
> arch/riscv/lib/strncmp.S | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/arch/riscv/lib/strncmp.S b/arch/riscv/lib/strncmp.S
> index ee49595075be..efd3f3150a54 100644
> --- a/arch/riscv/lib/strncmp.S
> +++ b/arch/riscv/lib/strncmp.S
> @@ -78,11 +78,13 @@ strncmp_zbb:
> /* Main loop for aligned string. */
> .p2align 3
> 1:
> - bgt a0, t6, 3f
> + bge a0, t6, 3f
> REG_L t0, 0(a0)
> REG_L t1, 0(a1)
> orc.b t3, t0
> bne t3, t5, 2f
> + orc.b t3, t1
> + bne t3, t5, 2f
> addi a0, a0, SZREG
> addi a1, a1, SZREG
> beq t0, t1, 1b
>
> base-commit: eb9be8310c58c166f9fae3b71c0ad9d6741b4897
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] riscv, lib: Fix Zbb strncmp
2023-02-28 21:18 ` Guenter Roeck
@ 2023-02-28 21:55 ` Palmer Dabbelt
0 siblings, 0 replies; 7+ messages in thread
From: Palmer Dabbelt @ 2023-02-28 21:55 UTC (permalink / raw)
To: linux
Cc: bjorn, Paul Walmsley, aou, linux-riscv, christoph.muellner,
heiko.stuebner, Bjorn Topel, Conor Dooley, ajones
On Tue, 28 Feb 2023 13:18:12 PST (-0800), linux@roeck-us.net wrote:
> On 2/28/23 10:42, Björn Töpel wrote:
>> From: Björn Töpel <bjorn@rivosinc.com>
>>
>> The Zbb optimized strncmp has two parts; a fast path that does XLEN/8B
>> per iteration, and a slow that does one byte per iteration.
>>
>> The idea is to compare aligned XLEN chunks for most of strings, and do
>> the remainder tail in the slow path.
>>
>> The Zbb strncmp has two issues in the fast path:
>>
>> Incorrect remainder handling (wrong compare): Assume that the string
>> length is 9. On 64b systems, the fast path should do one iteration,
>> and one iteration in the slow path. Instead, both were done in the
>> fast path, which lead to incorrect results. An example:
>>
>> strncmp("/dev/vda", "/dev/", 5);
>>
>> Correct by changing "bgt" to "bge".
>>
>> Missing NULL checks in the second string: This could lead to incorrect
>> results for:
>>
>> strncmp("/dev/vda", "/dev/vda\0", 8);
>>
>> Correct by adding an additional check.
>>
>> Fixes: b6fcdb191e36 ("RISC-V: add zbb support to string functions")
>> Suggested-by: Heiko Stuebner <heiko.stuebner@vrull.eu>
>> Signed-off-by: Björn Töpel <bjorn@rivosinc.com>
>
> Tested-by: Guenter Roeck <linux@roeck-us.net>
Thanks guys, I'm going to apply this ASAP as it's a pretty concrete
breakage in new code from this merge window. I just stuck it in my
staging branch, it should show up on for-next after the next batch of
tests.
>
>> ---
>> arch/riscv/lib/strncmp.S | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/riscv/lib/strncmp.S b/arch/riscv/lib/strncmp.S
>> index ee49595075be..efd3f3150a54 100644
>> --- a/arch/riscv/lib/strncmp.S
>> +++ b/arch/riscv/lib/strncmp.S
>> @@ -78,11 +78,13 @@ strncmp_zbb:
>> /* Main loop for aligned string. */
>> .p2align 3
>> 1:
>> - bgt a0, t6, 3f
>> + bge a0, t6, 3f
>> REG_L t0, 0(a0)
>> REG_L t1, 0(a1)
>> orc.b t3, t0
>> bne t3, t5, 2f
>> + orc.b t3, t1
>> + bne t3, t5, 2f
>> addi a0, a0, SZREG
>> addi a1, a1, SZREG
>> beq t0, t1, 1b
>>
>> base-commit: eb9be8310c58c166f9fae3b71c0ad9d6741b4897
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] riscv, lib: Fix Zbb strncmp
2023-02-28 18:42 [PATCH] riscv, lib: Fix Zbb strncmp Björn Töpel
2023-02-28 19:12 ` Conor Dooley
2023-02-28 21:18 ` Guenter Roeck
@ 2023-03-01 3:20 ` patchwork-bot+linux-riscv
2 siblings, 0 replies; 7+ messages in thread
From: patchwork-bot+linux-riscv @ 2023-03-01 3:20 UTC (permalink / raw)
To: =?utf-8?b?QmrDtnJuIFTDtnBlbCA8Ympvcm5Aa2VybmVsLm9yZz4=?=
Cc: linux-riscv, paul.walmsley, palmer, aou, christoph.muellner,
heiko.stuebner, bjorn, conor.dooley, linux, ajones
Hello:
This patch was applied to riscv/linux.git (for-next)
by Palmer Dabbelt <palmer@rivosinc.com>:
On Tue, 28 Feb 2023 19:42:10 +0100 you wrote:
> From: Björn Töpel <bjorn@rivosinc.com>
>
> The Zbb optimized strncmp has two parts; a fast path that does XLEN/8B
> per iteration, and a slow that does one byte per iteration.
>
> The idea is to compare aligned XLEN chunks for most of strings, and do
> the remainder tail in the slow path.
>
> [...]
Here is the summary with links:
- riscv, lib: Fix Zbb strncmp
https://git.kernel.org/riscv/c/81a1dd10b072
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-03-01 3:20 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-02-28 18:42 [PATCH] riscv, lib: Fix Zbb strncmp Björn Töpel
2023-02-28 19:12 ` Conor Dooley
2023-02-28 19:41 ` Björn Töpel
2023-02-28 19:53 ` Guenter Roeck
2023-02-28 21:18 ` Guenter Roeck
2023-02-28 21:55 ` Palmer Dabbelt
2023-03-01 3:20 ` patchwork-bot+linux-riscv
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.