[PATCH] riscv, kprobes: Stricter c.jr/c.jalr decoding

[PATCH] riscv, kprobes: Stricter c.jr/c.jalr decoding

[Björn Töpel]

From: Björn Töpel <bjorn@rivosinc.com>

In the compressed instruction extension, c.jr, c.jalr, c.mv, and c.add
is encoded the following way (each instruction is 16b):

---+-+-----------+-----------+--
100 0 rs1[4:0]!=0       00000 10 : c.jr
100 1 rs1[4:0]!=0       00000 10 : c.jalr
100 0  rd[4:0]!=0 rs2[4:0]!=0 10 : c.mv
100 1  rd[4:0]!=0 rs2[4:0]!=0 10 : c.add

The following logic is used to decode c.jr and c.jalr:

  insn & 0xf007 == 0x8002 => instruction is an c.jr
  insn & 0xf007 == 0x9002 => instruction is an c.jalr

When 0xf007 is used to mask the instruction, c.mv can be incorrectly
decoded as c.jr, and c.add as c.jalr.

Correct the decoding by changing the mask from 0xf007 to 0xf07f.

Fixes: c22b0bcb1dd0 ("riscv: Add kprobes supported")
Signed-off-by: Björn Töpel <bjorn@rivosinc.com>
---
 arch/riscv/kernel/probes/simulate-insn.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/riscv/kernel/probes/simulate-insn.h b/arch/riscv/kernel/probes/simulate-insn.h
index cb6ff7dccb92..de8474146a9b 100644
--- a/arch/riscv/kernel/probes/simulate-insn.h
+++ b/arch/riscv/kernel/probes/simulate-insn.h
@@ -31,9 +31,9 @@ __RISCV_INSN_FUNCS(fence,	0x7f, 0x0f);
 	} while (0)
 
 __RISCV_INSN_FUNCS(c_j,		0xe003, 0xa001);
-__RISCV_INSN_FUNCS(c_jr,	0xf007, 0x8002);
+__RISCV_INSN_FUNCS(c_jr,	0xf07f, 0x8002);
 __RISCV_INSN_FUNCS(c_jal,	0xe003, 0x2001);
-__RISCV_INSN_FUNCS(c_jalr,	0xf007, 0x9002);
+__RISCV_INSN_FUNCS(c_jalr,	0xf07f, 0x9002);
 __RISCV_INSN_FUNCS(c_beqz,	0xe003, 0xc001);
 __RISCV_INSN_FUNCS(c_bnez,	0xe003, 0xe001);
 __RISCV_INSN_FUNCS(c_ebreak,	0xffff, 0x9002);

base-commit: 1b929c02afd37871d5afb9d498426f83432e71c2
-- 
2.37.2


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

[PATCH] riscv, kprobes: Stricter c.jr/c.jalr decoding

[Björn Töpel]

From: Björn Töpel <bjorn@rivosinc.com>

In the compressed instruction extension, c.jr, c.jalr, c.mv, and c.add
is encoded the following way (each instruction is 16b):

---+-+-----------+-----------+--
100 0 rs1[4:0]!=0       00000 10 : c.jr
100 1 rs1[4:0]!=0       00000 10 : c.jalr
100 0  rd[4:0]!=0 rs2[4:0]!=0 10 : c.mv
100 1  rd[4:0]!=0 rs2[4:0]!=0 10 : c.add

The following logic is used to decode c.jr and c.jalr:

  insn & 0xf007 == 0x8002 => instruction is an c.jr
  insn & 0xf007 == 0x9002 => instruction is an c.jalr

When 0xf007 is used to mask the instruction, c.mv can be incorrectly
decoded as c.jr, and c.add as c.jalr.

Correct the decoding by changing the mask from 0xf007 to 0xf07f.

Fixes: c22b0bcb1dd0 ("riscv: Add kprobes supported")
Signed-off-by: Björn Töpel <bjorn@rivosinc.com>
---
 arch/riscv/kernel/probes/simulate-insn.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/riscv/kernel/probes/simulate-insn.h b/arch/riscv/kernel/probes/simulate-insn.h
index cb6ff7dccb92..de8474146a9b 100644
--- a/arch/riscv/kernel/probes/simulate-insn.h
+++ b/arch/riscv/kernel/probes/simulate-insn.h
@@ -31,9 +31,9 @@ __RISCV_INSN_FUNCS(fence,	0x7f, 0x0f);
 	} while (0)
 
 __RISCV_INSN_FUNCS(c_j,		0xe003, 0xa001);
-__RISCV_INSN_FUNCS(c_jr,	0xf007, 0x8002);
+__RISCV_INSN_FUNCS(c_jr,	0xf07f, 0x8002);
 __RISCV_INSN_FUNCS(c_jal,	0xe003, 0x2001);
-__RISCV_INSN_FUNCS(c_jalr,	0xf007, 0x9002);
+__RISCV_INSN_FUNCS(c_jalr,	0xf07f, 0x9002);
 __RISCV_INSN_FUNCS(c_beqz,	0xe003, 0xc001);
 __RISCV_INSN_FUNCS(c_bnez,	0xe003, 0xe001);
 __RISCV_INSN_FUNCS(c_ebreak,	0xffff, 0x9002);

base-commit: 1b929c02afd37871d5afb9d498426f83432e71c2
-- 
2.37.2

Re: [PATCH] riscv, kprobes: Stricter c.jr/c.jalr decoding

[Conor Dooley]

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

Hey Bjorn,

On Mon, Jan 02, 2023 at 05:07:48PM +0100, Björn Töpel wrote:
> From: Björn Töpel <bjorn@rivosinc.com>
> 
> In the compressed instruction extension, c.jr, c.jalr, c.mv, and c.add
> is encoded the following way (each instruction is 16b):
> 
> ---+-+-----------+-----------+--
> 100 0 rs1[4:0]!=0       00000 10 : c.jr
> 100 1 rs1[4:0]!=0       00000 10 : c.jalr
> 100 0  rd[4:0]!=0 rs2[4:0]!=0 10 : c.mv
> 100 1  rd[4:0]!=0 rs2[4:0]!=0 10 : c.add
> 
> The following logic is used to decode c.jr and c.jalr:
> 
>   insn & 0xf007 == 0x8002 => instruction is an c.jr
>   insn & 0xf007 == 0x9002 => instruction is an c.jalr
> 
> When 0xf007 is used to mask the instruction, c.mv can be incorrectly
> decoded as c.jr, and c.add as c.jalr.
> 
> Correct the decoding by changing the mask from 0xf007 to 0xf07f.
> 
> Fixes: c22b0bcb1dd0 ("riscv: Add kprobes supported")
> Signed-off-by: Björn Töpel <bjorn@rivosinc.com>
> ---
>  arch/riscv/kernel/probes/simulate-insn.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/riscv/kernel/probes/simulate-insn.h b/arch/riscv/kernel/probes/simulate-insn.h
> index cb6ff7dccb92..de8474146a9b 100644
> --- a/arch/riscv/kernel/probes/simulate-insn.h
> +++ b/arch/riscv/kernel/probes/simulate-insn.h
> @@ -31,9 +31,9 @@ __RISCV_INSN_FUNCS(fence,	0x7f, 0x0f);
>  	} while (0)
>  
>  __RISCV_INSN_FUNCS(c_j,		0xe003, 0xa001);
> -__RISCV_INSN_FUNCS(c_jr,	0xf007, 0x8002);

Hmm, I wonder where the mask originally came from!
I had a look at the compressed spec, of which the version google gave to
me was v1.9 [1], and Table 1.6 in that (Instruction listing for RVC,
Quadrant 2) seems to list them all together.
Tedious it may be, but future instruction decoding bits probably need
more scrutiny as Drew found another clearly wrong bit a few weeks ago
[2].

Anyways, I checked against the doc and the new versions look good to
me. How'd you spot this, and did you check the other masks?
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

[1] - https://riscv.org/wp-content/uploads/2015/11/riscv-compressed-spec-v1.9.pdf
[2] - https://lore.kernel.org/linux-riscv/20221223221332.4127602-2-heiko@sntech.de/

> +__RISCV_INSN_FUNCS(c_jr,	0xf07f, 0x8002);
>  __RISCV_INSN_FUNCS(c_jal,	0xe003, 0x2001);
> -__RISCV_INSN_FUNCS(c_jalr,	0xf007, 0x9002);
> +__RISCV_INSN_FUNCS(c_jalr,	0xf07f, 0x9002);
>  __RISCV_INSN_FUNCS(c_beqz,	0xe003, 0xc001);
>  __RISCV_INSN_FUNCS(c_bnez,	0xe003, 0xe001);
>  __RISCV_INSN_FUNCS(c_ebreak,	0xffff, 0x9002);

Worth noting that this code is gone in riscv/for-next thanks to Heiko's
de-duplication:
https://lore.kernel.org/linux-riscv/20221223221332.4127602-7-heiko@sntech.de/

> 
> base-commit: 1b929c02afd37871d5afb9d498426f83432e71c2
> -- 
> 2.37.2
> 

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

Re: [PATCH] riscv, kprobes: Stricter c.jr/c.jalr decoding

[Conor Dooley]

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

Hey Bjorn,

On Mon, Jan 02, 2023 at 05:07:48PM +0100, Björn Töpel wrote:
> From: Björn Töpel <bjorn@rivosinc.com>
> 
> In the compressed instruction extension, c.jr, c.jalr, c.mv, and c.add
> is encoded the following way (each instruction is 16b):
> 
> ---+-+-----------+-----------+--
> 100 0 rs1[4:0]!=0       00000 10 : c.jr
> 100 1 rs1[4:0]!=0       00000 10 : c.jalr
> 100 0  rd[4:0]!=0 rs2[4:0]!=0 10 : c.mv
> 100 1  rd[4:0]!=0 rs2[4:0]!=0 10 : c.add
> 
> The following logic is used to decode c.jr and c.jalr:
> 
>   insn & 0xf007 == 0x8002 => instruction is an c.jr
>   insn & 0xf007 == 0x9002 => instruction is an c.jalr
> 
> When 0xf007 is used to mask the instruction, c.mv can be incorrectly
> decoded as c.jr, and c.add as c.jalr.
> 
> Correct the decoding by changing the mask from 0xf007 to 0xf07f.
> 
> Fixes: c22b0bcb1dd0 ("riscv: Add kprobes supported")
> Signed-off-by: Björn Töpel <bjorn@rivosinc.com>
> ---
>  arch/riscv/kernel/probes/simulate-insn.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/riscv/kernel/probes/simulate-insn.h b/arch/riscv/kernel/probes/simulate-insn.h
> index cb6ff7dccb92..de8474146a9b 100644
> --- a/arch/riscv/kernel/probes/simulate-insn.h
> +++ b/arch/riscv/kernel/probes/simulate-insn.h
> @@ -31,9 +31,9 @@ __RISCV_INSN_FUNCS(fence,	0x7f, 0x0f);
>  	} while (0)
>  
>  __RISCV_INSN_FUNCS(c_j,		0xe003, 0xa001);
> -__RISCV_INSN_FUNCS(c_jr,	0xf007, 0x8002);

Hmm, I wonder where the mask originally came from!
I had a look at the compressed spec, of which the version google gave to
me was v1.9 [1], and Table 1.6 in that (Instruction listing for RVC,
Quadrant 2) seems to list them all together.
Tedious it may be, but future instruction decoding bits probably need
more scrutiny as Drew found another clearly wrong bit a few weeks ago
[2].

Anyways, I checked against the doc and the new versions look good to
me. How'd you spot this, and did you check the other masks?
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

[1] - https://riscv.org/wp-content/uploads/2015/11/riscv-compressed-spec-v1.9.pdf
[2] - https://lore.kernel.org/linux-riscv/20221223221332.4127602-2-heiko@sntech.de/

> +__RISCV_INSN_FUNCS(c_jr,	0xf07f, 0x8002);
>  __RISCV_INSN_FUNCS(c_jal,	0xe003, 0x2001);
> -__RISCV_INSN_FUNCS(c_jalr,	0xf007, 0x9002);
> +__RISCV_INSN_FUNCS(c_jalr,	0xf07f, 0x9002);
>  __RISCV_INSN_FUNCS(c_beqz,	0xe003, 0xc001);
>  __RISCV_INSN_FUNCS(c_bnez,	0xe003, 0xe001);
>  __RISCV_INSN_FUNCS(c_ebreak,	0xffff, 0x9002);

Worth noting that this code is gone in riscv/for-next thanks to Heiko's
de-duplication:
https://lore.kernel.org/linux-riscv/20221223221332.4127602-7-heiko@sntech.de/

> 
> base-commit: 1b929c02afd37871d5afb9d498426f83432e71c2
> -- 
> 2.37.2
> 

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

Re: [PATCH] riscv, kprobes: Stricter c.jr/c.jalr decoding

[Björn Töpel]

Conor Dooley <conor@kernel.org> writes:

> Hey Bjorn,
>
> On Mon, Jan 02, 2023 at 05:07:48PM +0100, Björn Töpel wrote:
>> From: Björn Töpel <bjorn@rivosinc.com>
>> 
>> In the compressed instruction extension, c.jr, c.jalr, c.mv, and c.add
>> is encoded the following way (each instruction is 16b):
>> 
>> ---+-+-----------+-----------+--
>> 100 0 rs1[4:0]!=0       00000 10 : c.jr
>> 100 1 rs1[4:0]!=0       00000 10 : c.jalr
>> 100 0  rd[4:0]!=0 rs2[4:0]!=0 10 : c.mv
>> 100 1  rd[4:0]!=0 rs2[4:0]!=0 10 : c.add
>> 
>> The following logic is used to decode c.jr and c.jalr:
>> 
>>   insn & 0xf007 == 0x8002 => instruction is an c.jr
>>   insn & 0xf007 == 0x9002 => instruction is an c.jalr
>> 
>> When 0xf007 is used to mask the instruction, c.mv can be incorrectly
>> decoded as c.jr, and c.add as c.jalr.
>> 
>> Correct the decoding by changing the mask from 0xf007 to 0xf07f.
>> 
>> Fixes: c22b0bcb1dd0 ("riscv: Add kprobes supported")
>> Signed-off-by: Björn Töpel <bjorn@rivosinc.com>
>> ---
>>  arch/riscv/kernel/probes/simulate-insn.h | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>> 
>> diff --git a/arch/riscv/kernel/probes/simulate-insn.h b/arch/riscv/kernel/probes/simulate-insn.h
>> index cb6ff7dccb92..de8474146a9b 100644
>> --- a/arch/riscv/kernel/probes/simulate-insn.h
>> +++ b/arch/riscv/kernel/probes/simulate-insn.h
>> @@ -31,9 +31,9 @@ __RISCV_INSN_FUNCS(fence,	0x7f, 0x0f);
>>  	} while (0)
>>  
>>  __RISCV_INSN_FUNCS(c_j,		0xe003, 0xa001);
>> -__RISCV_INSN_FUNCS(c_jr,	0xf007, 0x8002);
>
> Hmm, I wonder where the mask originally came from!

I think it's just a simple bug -- missing that "rs2" must be zero.

> I had a look at the compressed spec, of which the version google gave to
> me was v1.9 [1], and Table 1.6 in that (Instruction listing for RVC,
> Quadrant 2) seems to list them all together.
> Tedious it may be, but future instruction decoding bits probably need
> more scrutiny as Drew found another clearly wrong bit a few weeks ago
> [2].
>
> Anyways, I checked against the doc and the new versions look good to
> me. How'd you spot this, and did you check the other masks?

I got hit by it when testing the optprobe series (c.mv was rejected as
c.jr).

Skimmed the other masks quickly, but will take another look.

> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
>
> [1] -
> https://riscv.org/wp-content/uploads/2015/11/riscv-compressed-spec-v1.9.pdf

C-ext is part of the unpriv spec:
https://github.com/riscv/riscv-isa-manual/releases

> [2] - https://lore.kernel.org/linux-riscv/20221223221332.4127602-2-heiko@sntech.de/
>
>> +__RISCV_INSN_FUNCS(c_jr,	0xf07f, 0x8002);
>>  __RISCV_INSN_FUNCS(c_jal,	0xe003, 0x2001);
>> -__RISCV_INSN_FUNCS(c_jalr,	0xf007, 0x9002);
>> +__RISCV_INSN_FUNCS(c_jalr,	0xf07f, 0x9002);
>>  __RISCV_INSN_FUNCS(c_beqz,	0xe003, 0xc001);
>>  __RISCV_INSN_FUNCS(c_bnez,	0xe003, 0xe001);
>>  __RISCV_INSN_FUNCS(c_ebreak,	0xffff, 0x9002);
>
> Worth noting that this code is gone in riscv/for-next thanks to Heiko's
> de-duplication:
> https://lore.kernel.org/linux-riscv/20221223221332.4127602-7-heiko@sntech.de/

Yay!


Björn

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

Re: [PATCH] riscv, kprobes: Stricter c.jr/c.jalr decoding

[Björn Töpel]

Conor Dooley <conor@kernel.org> writes:

> Hey Bjorn,
>
> On Mon, Jan 02, 2023 at 05:07:48PM +0100, Björn Töpel wrote:
>> From: Björn Töpel <bjorn@rivosinc.com>
>> 
>> In the compressed instruction extension, c.jr, c.jalr, c.mv, and c.add
>> is encoded the following way (each instruction is 16b):
>> 
>> ---+-+-----------+-----------+--
>> 100 0 rs1[4:0]!=0       00000 10 : c.jr
>> 100 1 rs1[4:0]!=0       00000 10 : c.jalr
>> 100 0  rd[4:0]!=0 rs2[4:0]!=0 10 : c.mv
>> 100 1  rd[4:0]!=0 rs2[4:0]!=0 10 : c.add
>> 
>> The following logic is used to decode c.jr and c.jalr:
>> 
>>   insn & 0xf007 == 0x8002 => instruction is an c.jr
>>   insn & 0xf007 == 0x9002 => instruction is an c.jalr
>> 
>> When 0xf007 is used to mask the instruction, c.mv can be incorrectly
>> decoded as c.jr, and c.add as c.jalr.
>> 
>> Correct the decoding by changing the mask from 0xf007 to 0xf07f.
>> 
>> Fixes: c22b0bcb1dd0 ("riscv: Add kprobes supported")
>> Signed-off-by: Björn Töpel <bjorn@rivosinc.com>
>> ---
>>  arch/riscv/kernel/probes/simulate-insn.h | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>> 
>> diff --git a/arch/riscv/kernel/probes/simulate-insn.h b/arch/riscv/kernel/probes/simulate-insn.h
>> index cb6ff7dccb92..de8474146a9b 100644
>> --- a/arch/riscv/kernel/probes/simulate-insn.h
>> +++ b/arch/riscv/kernel/probes/simulate-insn.h
>> @@ -31,9 +31,9 @@ __RISCV_INSN_FUNCS(fence,	0x7f, 0x0f);
>>  	} while (0)
>>  
>>  __RISCV_INSN_FUNCS(c_j,		0xe003, 0xa001);
>> -__RISCV_INSN_FUNCS(c_jr,	0xf007, 0x8002);
>
> Hmm, I wonder where the mask originally came from!

I think it's just a simple bug -- missing that "rs2" must be zero.

> I had a look at the compressed spec, of which the version google gave to
> me was v1.9 [1], and Table 1.6 in that (Instruction listing for RVC,
> Quadrant 2) seems to list them all together.
> Tedious it may be, but future instruction decoding bits probably need
> more scrutiny as Drew found another clearly wrong bit a few weeks ago
> [2].
>
> Anyways, I checked against the doc and the new versions look good to
> me. How'd you spot this, and did you check the other masks?

I got hit by it when testing the optprobe series (c.mv was rejected as
c.jr).

Skimmed the other masks quickly, but will take another look.

> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
>
> [1] -
> https://riscv.org/wp-content/uploads/2015/11/riscv-compressed-spec-v1.9.pdf

C-ext is part of the unpriv spec:
https://github.com/riscv/riscv-isa-manual/releases

> [2] - https://lore.kernel.org/linux-riscv/20221223221332.4127602-2-heiko@sntech.de/
>
>> +__RISCV_INSN_FUNCS(c_jr,	0xf07f, 0x8002);
>>  __RISCV_INSN_FUNCS(c_jal,	0xe003, 0x2001);
>> -__RISCV_INSN_FUNCS(c_jalr,	0xf007, 0x9002);
>> +__RISCV_INSN_FUNCS(c_jalr,	0xf07f, 0x9002);
>>  __RISCV_INSN_FUNCS(c_beqz,	0xe003, 0xc001);
>>  __RISCV_INSN_FUNCS(c_bnez,	0xe003, 0xe001);
>>  __RISCV_INSN_FUNCS(c_ebreak,	0xffff, 0x9002);
>
> Worth noting that this code is gone in riscv/for-next thanks to Heiko's
> de-duplication:
> https://lore.kernel.org/linux-riscv/20221223221332.4127602-7-heiko@sntech.de/

Yay!


Björn

Re: [PATCH] riscv, kprobes: Stricter c.jr/c.jalr decoding

[Conor Dooley]

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

On Tue, Jan 03, 2023 at 07:44:49AM +0100, Björn Töpel wrote:
> Conor Dooley <conor@kernel.org> writes:
> > On Mon, Jan 02, 2023 at 05:07:48PM +0100, Björn Töpel wrote:
> >> From: Björn Töpel <bjorn@rivosinc.com>
> >> diff --git a/arch/riscv/kernel/probes/simulate-insn.h b/arch/riscv/kernel/probes/simulate-insn.h
> >> index cb6ff7dccb92..de8474146a9b 100644
> >> --- a/arch/riscv/kernel/probes/simulate-insn.h
> >> +++ b/arch/riscv/kernel/probes/simulate-insn.h
> >> @@ -31,9 +31,9 @@ __RISCV_INSN_FUNCS(fence,	0x7f, 0x0f);
> >>  	} while (0)
> >>  
> >>  __RISCV_INSN_FUNCS(c_j,		0xe003, 0xa001);
> >> -__RISCV_INSN_FUNCS(c_jr,	0xf007, 0x8002);
> >
> > Hmm, I wonder where the mask originally came from!
> 
> I think it's just a simple bug -- missing that "rs2" must be zero.
> 
> > I had a look at the compressed spec, of which the version google gave to
> > me was v1.9 [1], and Table 1.6 in that (Instruction listing for RVC,
> > Quadrant 2) seems to list them all together.

> > [1] -
> > https://riscv.org/wp-content/uploads/2015/11/riscv-compressed-spec-v1.9.pdf
> 
> C-ext is part of the unpriv spec:
> https://github.com/riscv/riscv-isa-manual/releases

Yah, I was trying to see if there was some period piece which was
misleading that would have explained the mask. I looked again & the v1.7
spec doesn't have that table, but also has no reason to suggest the
current mask either. Guess it was just a mistake :)

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

Re: [PATCH] riscv, kprobes: Stricter c.jr/c.jalr decoding

[Conor Dooley]

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

On Tue, Jan 03, 2023 at 07:44:49AM +0100, Björn Töpel wrote:
> Conor Dooley <conor@kernel.org> writes:
> > On Mon, Jan 02, 2023 at 05:07:48PM +0100, Björn Töpel wrote:
> >> From: Björn Töpel <bjorn@rivosinc.com>
> >> diff --git a/arch/riscv/kernel/probes/simulate-insn.h b/arch/riscv/kernel/probes/simulate-insn.h
> >> index cb6ff7dccb92..de8474146a9b 100644
> >> --- a/arch/riscv/kernel/probes/simulate-insn.h
> >> +++ b/arch/riscv/kernel/probes/simulate-insn.h
> >> @@ -31,9 +31,9 @@ __RISCV_INSN_FUNCS(fence,	0x7f, 0x0f);
> >>  	} while (0)
> >>  
> >>  __RISCV_INSN_FUNCS(c_j,		0xe003, 0xa001);
> >> -__RISCV_INSN_FUNCS(c_jr,	0xf007, 0x8002);
> >
> > Hmm, I wonder where the mask originally came from!
> 
> I think it's just a simple bug -- missing that "rs2" must be zero.
> 
> > I had a look at the compressed spec, of which the version google gave to
> > me was v1.9 [1], and Table 1.6 in that (Instruction listing for RVC,
> > Quadrant 2) seems to list them all together.

> > [1] -
> > https://riscv.org/wp-content/uploads/2015/11/riscv-compressed-spec-v1.9.pdf
> 
> C-ext is part of the unpriv spec:
> https://github.com/riscv/riscv-isa-manual/releases

Yah, I was trying to see if there was some period piece which was
misleading that would have explained the mask. I looked again & the v1.7
spec doesn't have that table, but also has no reason to suggest the
current mask either. Guess it was just a mistake :)

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

Re: [PATCH] riscv, kprobes: Stricter c.jr/c.jalr decoding

[Guo Ren]

Thx for catching it.

On Tue, Jan 3, 2023 at 12:07 AM Björn Töpel <bjorn@kernel.org> wrote:
>
> From: Björn Töpel <bjorn@rivosinc.com>
>
> In the compressed instruction extension, c.jr, c.jalr, c.mv, and c.add
> is encoded the following way (each instruction is 16b):
>
> ---+-+-----------+-----------+--
> 100 0 rs1[4:0]!=0       00000 10 : c.jr
> 100 1 rs1[4:0]!=0       00000 10 : c.jalr
> 100 0  rd[4:0]!=0 rs2[4:0]!=0 10 : c.mv
> 100 1  rd[4:0]!=0 rs2[4:0]!=0 10 : c.add
Yes, I forgot the c.mv & c.add effect.

Reviewed-by: Guo Ren <guoren@kernel.org>

>
> The following logic is used to decode c.jr and c.jalr:
>
>   insn & 0xf007 == 0x8002 => instruction is an c.jr
>   insn & 0xf007 == 0x9002 => instruction is an c.jalr
>
> When 0xf007 is used to mask the instruction, c.mv can be incorrectly
> decoded as c.jr, and c.add as c.jalr.
>
> Correct the decoding by changing the mask from 0xf007 to 0xf07f.
>
> Fixes: c22b0bcb1dd0 ("riscv: Add kprobes supported")
> Signed-off-by: Björn Töpel <bjorn@rivosinc.com>
> ---
>  arch/riscv/kernel/probes/simulate-insn.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/riscv/kernel/probes/simulate-insn.h b/arch/riscv/kernel/probes/simulate-insn.h
> index cb6ff7dccb92..de8474146a9b 100644
> --- a/arch/riscv/kernel/probes/simulate-insn.h
> +++ b/arch/riscv/kernel/probes/simulate-insn.h
> @@ -31,9 +31,9 @@ __RISCV_INSN_FUNCS(fence,     0x7f, 0x0f);
>         } while (0)
>
>  __RISCV_INSN_FUNCS(c_j,                0xe003, 0xa001);
> -__RISCV_INSN_FUNCS(c_jr,       0xf007, 0x8002);
> +__RISCV_INSN_FUNCS(c_jr,       0xf07f, 0x8002);
>  __RISCV_INSN_FUNCS(c_jal,      0xe003, 0x2001);
> -__RISCV_INSN_FUNCS(c_jalr,     0xf007, 0x9002);
> +__RISCV_INSN_FUNCS(c_jalr,     0xf07f, 0x9002);
>  __RISCV_INSN_FUNCS(c_beqz,     0xe003, 0xc001);
>  __RISCV_INSN_FUNCS(c_bnez,     0xe003, 0xe001);
>  __RISCV_INSN_FUNCS(c_ebreak,   0xffff, 0x9002);
>
> base-commit: 1b929c02afd37871d5afb9d498426f83432e71c2
> --
> 2.37.2
>


-- 
Best Regards
 Guo Ren

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

Re: [PATCH] riscv, kprobes: Stricter c.jr/c.jalr decoding

[Guo Ren]

Thx for catching it.

On Tue, Jan 3, 2023 at 12:07 AM Björn Töpel <bjorn@kernel.org> wrote:
>
> From: Björn Töpel <bjorn@rivosinc.com>
>
> In the compressed instruction extension, c.jr, c.jalr, c.mv, and c.add
> is encoded the following way (each instruction is 16b):
>
> ---+-+-----------+-----------+--
> 100 0 rs1[4:0]!=0       00000 10 : c.jr
> 100 1 rs1[4:0]!=0       00000 10 : c.jalr
> 100 0  rd[4:0]!=0 rs2[4:0]!=0 10 : c.mv
> 100 1  rd[4:0]!=0 rs2[4:0]!=0 10 : c.add
Yes, I forgot the c.mv & c.add effect.

Reviewed-by: Guo Ren <guoren@kernel.org>

>
> The following logic is used to decode c.jr and c.jalr:
>
>   insn & 0xf007 == 0x8002 => instruction is an c.jr
>   insn & 0xf007 == 0x9002 => instruction is an c.jalr
>
> When 0xf007 is used to mask the instruction, c.mv can be incorrectly
> decoded as c.jr, and c.add as c.jalr.
>
> Correct the decoding by changing the mask from 0xf007 to 0xf07f.
>
> Fixes: c22b0bcb1dd0 ("riscv: Add kprobes supported")
> Signed-off-by: Björn Töpel <bjorn@rivosinc.com>
> ---
>  arch/riscv/kernel/probes/simulate-insn.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/riscv/kernel/probes/simulate-insn.h b/arch/riscv/kernel/probes/simulate-insn.h
> index cb6ff7dccb92..de8474146a9b 100644
> --- a/arch/riscv/kernel/probes/simulate-insn.h
> +++ b/arch/riscv/kernel/probes/simulate-insn.h
> @@ -31,9 +31,9 @@ __RISCV_INSN_FUNCS(fence,     0x7f, 0x0f);
>         } while (0)
>
>  __RISCV_INSN_FUNCS(c_j,                0xe003, 0xa001);
> -__RISCV_INSN_FUNCS(c_jr,       0xf007, 0x8002);
> +__RISCV_INSN_FUNCS(c_jr,       0xf07f, 0x8002);
>  __RISCV_INSN_FUNCS(c_jal,      0xe003, 0x2001);
> -__RISCV_INSN_FUNCS(c_jalr,     0xf007, 0x9002);
> +__RISCV_INSN_FUNCS(c_jalr,     0xf07f, 0x9002);
>  __RISCV_INSN_FUNCS(c_beqz,     0xe003, 0xc001);
>  __RISCV_INSN_FUNCS(c_bnez,     0xe003, 0xe001);
>  __RISCV_INSN_FUNCS(c_ebreak,   0xffff, 0x9002);
>
> base-commit: 1b929c02afd37871d5afb9d498426f83432e71c2
> --
> 2.37.2
>


-- 
Best Regards
 Guo Ren

Re: [PATCH] riscv, kprobes: Stricter c.jr/c.jalr decoding

[Palmer Dabbelt]

On Mon, 2 Jan 2023 17:07:48 +0100, Björn Töpel wrote:
> From: Björn Töpel <bjorn@rivosinc.com>
> 
> In the compressed instruction extension, c.jr, c.jalr, c.mv, and c.add
> is encoded the following way (each instruction is 16b):
> 
> ---+-+-----------+-----------+--
> 100 0 rs1[4:0]!=0       00000 10 : c.jr
> 100 1 rs1[4:0]!=0       00000 10 : c.jalr
> 100 0  rd[4:0]!=0 rs2[4:0]!=0 10 : c.mv
> 100 1  rd[4:0]!=0 rs2[4:0]!=0 10 : c.add
> 
> [...]

Applied, thanks!

[1/1] riscv, kprobes: Stricter c.jr/c.jalr decoding
      https://git.kernel.org/palmer/c/b2d473a6019e

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

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

Re: [PATCH] riscv, kprobes: Stricter c.jr/c.jalr decoding

[patchwork-bot+linux-riscv]

Hello:

This patch was applied to riscv/linux.git (fixes)
by Palmer Dabbelt <palmer@rivosinc.com>:

On Mon,  2 Jan 2023 17:07:48 +0100 you wrote:
> From: Björn Töpel <bjorn@rivosinc.com>
> 
> In the compressed instruction extension, c.jr, c.jalr, c.mv, and c.add
> is encoded the following way (each instruction is 16b):
> 
> ---+-+-----------+-----------+--
> 100 0 rs1[4:0]!=0       00000 10 : c.jr
> 100 1 rs1[4:0]!=0       00000 10 : c.jalr
> 100 0  rd[4:0]!=0 rs2[4:0]!=0 10 : c.mv
> 100 1  rd[4:0]!=0 rs2[4:0]!=0 10 : c.add
> 
> [...]

Here is the summary with links:
  - riscv, kprobes: Stricter c.jr/c.jalr decoding
    https://git.kernel.org/riscv/c/b2d473a6019e

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

Re: [PATCH] riscv, kprobes: Stricter c.jr/c.jalr decoding

[patchwork-bot+linux-riscv]

Hello:

This patch was applied to riscv/linux.git (fixes)
by Palmer Dabbelt <palmer@rivosinc.com>:

On Mon,  2 Jan 2023 17:07:48 +0100 you wrote:
> From: Björn Töpel <bjorn@rivosinc.com>
> 
> In the compressed instruction extension, c.jr, c.jalr, c.mv, and c.add
> is encoded the following way (each instruction is 16b):
> 
> ---+-+-----------+-----------+--
> 100 0 rs1[4:0]!=0       00000 10 : c.jr
> 100 1 rs1[4:0]!=0       00000 10 : c.jalr
> 100 0  rd[4:0]!=0 rs2[4:0]!=0 10 : c.mv
> 100 1  rd[4:0]!=0 rs2[4:0]!=0 10 : c.add
> 
> [...]

Here is the summary with links:
  - riscv, kprobes: Stricter c.jr/c.jalr decoding
    https://git.kernel.org/riscv/c/b2d473a6019e

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