| Message ID | 20230731183925.152145-1-namcaov@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 79bc3f85c51fc352f8e684ba6b626f677a3aa230 |
| Headers | show |
| Series | riscv: correct riscv_insn_is_c_jr() and riscv_insn_is_c_jalr() | expand |
| Context | Check | Description |
|---|---|---|
| conchuod/cover_letter | success | Single patches do not need cover letters |
| conchuod/tree_selection | success | Guessed tree name to be for-next at HEAD 471aba2e4760 |
| conchuod/fixes_present | success | Fixes tag not required for -next series |
| conchuod/maintainers_pattern | success | MAINTAINERS pattern errors before the patch: 4 and now 4 |
| conchuod/verify_signedoff | success | Signed-off-by tag matches author and committer |
| conchuod/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| conchuod/build_rv64_clang_allmodconfig | success | Errors and warnings before: 9 this patch: 9 |
| conchuod/module_param | success | Was 0 now: 0 |
| conchuod/build_rv64_gcc_allmodconfig | success | Errors and warnings before: 9 this patch: 9 |
| conchuod/build_rv32_defconfig | success | Build OK |
| conchuod/dtb_warn_rv64 | success | Errors and warnings before: 3 this patch: 3 |
| conchuod/header_inline | success | No static functions without inline keyword in header files |
| conchuod/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 33 lines checked |
| conchuod/build_rv64_nommu_k210_defconfig | success | Build OK |
| conchuod/verify_fixes | success | No Fixes tag |
| conchuod/build_rv64_nommu_virt_defconfig | success | Build OK |
On Mon, Jul 31, 2023 at 08:39:25PM +0200, Nam Cao wrote: > The instructions c.jr and c.jalr must have rs1 != 0, but > riscv_insn_is_c_jr() and riscv_insn_is_c_jalr() do not check for this. So, > riscv_insn_is_c_jr() can match a reserved encoding, while > riscv_insn_is_c_jalr() can match the c.ebreak instruction. > > Rewrite them with check for rs1 != 0. > > Signed-off-by: Nam Cao <namcaov@gmail.com> > --- > arch/riscv/include/asm/insn.h | 15 +++++++++++++-- > 1 file changed, 13 insertions(+), 2 deletions(-) > > diff --git a/arch/riscv/include/asm/insn.h b/arch/riscv/include/asm/insn.h > index 4e1505cef8aa..fce00400c9bc 100644 > --- a/arch/riscv/include/asm/insn.h > +++ b/arch/riscv/include/asm/insn.h > @@ -110,6 +110,7 @@ > #define RVC_INSN_FUNCT4_OPOFF 12 > #define RVC_INSN_FUNCT3_MASK GENMASK(15, 13) > #define RVC_INSN_FUNCT3_OPOFF 13 > +#define RVC_INSN_J_RS1_MASK GENMASK(11, 7) > #define RVC_INSN_J_RS2_MASK GENMASK(6, 2) > #define RVC_INSN_OPCODE_MASK GENMASK(1, 0) > #define RVC_ENCODE_FUNCT3(f_) (RVC_FUNCT3_##f_ << RVC_INSN_FUNCT3_OPOFF) > @@ -245,8 +246,6 @@ __RISCV_INSN_FUNCS(c_jal, RVC_MASK_C_JAL, RVC_MATCH_C_JAL) > __RISCV_INSN_FUNCS(auipc, RVG_MASK_AUIPC, RVG_MATCH_AUIPC) > __RISCV_INSN_FUNCS(jalr, RVG_MASK_JALR, RVG_MATCH_JALR) > __RISCV_INSN_FUNCS(jal, RVG_MASK_JAL, RVG_MATCH_JAL) > -__RISCV_INSN_FUNCS(c_jr, RVC_MASK_C_JR, RVC_MATCH_C_JR) > -__RISCV_INSN_FUNCS(c_jalr, RVC_MASK_C_JALR, RVC_MATCH_C_JALR) > __RISCV_INSN_FUNCS(c_j, RVC_MASK_C_J, RVC_MATCH_C_J) > __RISCV_INSN_FUNCS(beq, RVG_MASK_BEQ, RVG_MATCH_BEQ) > __RISCV_INSN_FUNCS(bne, RVG_MASK_BNE, RVG_MATCH_BNE) > @@ -273,6 +272,18 @@ static __always_inline bool riscv_insn_is_branch(u32 code) > return (code & RV_INSN_OPCODE_MASK) == RVG_OPCODE_BRANCH; > } > > +static __always_inline bool riscv_insn_is_c_jr(u32 code) > +{ > + return (code & RVC_MASK_C_JR) == RVC_MATCH_C_JR && > + (code & RVC_INSN_J_RS1_MASK) != 0; > +} > + > +static __always_inline bool riscv_insn_is_c_jalr(u32 code) > +{ > + return (code & RVC_MASK_C_JALR) == RVC_MATCH_C_JALR && > + (code & RVC_INSN_J_RS1_MASK) != 0; > +} > + > #define RV_IMM_SIGN(x) (-(((x) >> 31) & 1)) > #define RVC_IMM_SIGN(x) (-(((x) >> 12) & 1)) > #define RV_X(X, s, mask) (((X) >> (s)) & (mask)) > -- > 2.34.1 > Good catch. You can add: Reviewed-by: Charlie Jenkins <charlie@rivosinc.com> > > _______________________________________________ > linux-riscv mailing list > linux-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv
Nam Cao <namcaov@gmail.com> writes: > The instructions c.jr and c.jalr must have rs1 != 0, but > riscv_insn_is_c_jr() and riscv_insn_is_c_jalr() do not check for this. So, > riscv_insn_is_c_jr() can match a reserved encoding, while > riscv_insn_is_c_jalr() can match the c.ebreak instruction. > > Rewrite them with check for rs1 != 0. > > Signed-off-by: Nam Cao <namcaov@gmail.com> > --- > arch/riscv/include/asm/insn.h | 15 +++++++++++++-- > 1 file changed, 13 insertions(+), 2 deletions(-) > > diff --git a/arch/riscv/include/asm/insn.h b/arch/riscv/include/asm/insn.h > index 4e1505cef8aa..fce00400c9bc 100644 > --- a/arch/riscv/include/asm/insn.h > +++ b/arch/riscv/include/asm/insn.h > @@ -110,6 +110,7 @@ > #define RVC_INSN_FUNCT4_OPOFF 12 > #define RVC_INSN_FUNCT3_MASK GENMASK(15, 13) > #define RVC_INSN_FUNCT3_OPOFF 13 > +#define RVC_INSN_J_RS1_MASK GENMASK(11, 7) > #define RVC_INSN_J_RS2_MASK GENMASK(6, 2) > #define RVC_INSN_OPCODE_MASK GENMASK(1, 0) > #define RVC_ENCODE_FUNCT3(f_) (RVC_FUNCT3_##f_ << RVC_INSN_FUNCT3_OPOFF) > @@ -245,8 +246,6 @@ __RISCV_INSN_FUNCS(c_jal, RVC_MASK_C_JAL, RVC_MATCH_C_JAL) > __RISCV_INSN_FUNCS(auipc, RVG_MASK_AUIPC, RVG_MATCH_AUIPC) > __RISCV_INSN_FUNCS(jalr, RVG_MASK_JALR, RVG_MATCH_JALR) > __RISCV_INSN_FUNCS(jal, RVG_MASK_JAL, RVG_MATCH_JAL) > -__RISCV_INSN_FUNCS(c_jr, RVC_MASK_C_JR, RVC_MATCH_C_JR) > -__RISCV_INSN_FUNCS(c_jalr, RVC_MASK_C_JALR, RVC_MATCH_C_JALR) > __RISCV_INSN_FUNCS(c_j, RVC_MASK_C_J, RVC_MATCH_C_J) > __RISCV_INSN_FUNCS(beq, RVG_MASK_BEQ, RVG_MATCH_BEQ) > __RISCV_INSN_FUNCS(bne, RVG_MASK_BNE, RVG_MATCH_BNE) > @@ -273,6 +272,18 @@ static __always_inline bool riscv_insn_is_branch(u32 code) > return (code & RV_INSN_OPCODE_MASK) == RVG_OPCODE_BRANCH; > } > > +static __always_inline bool riscv_insn_is_c_jr(u32 code) > +{ > + return (code & RVC_MASK_C_JR) == RVC_MATCH_C_JR && > + (code & RVC_INSN_J_RS1_MASK) != 0; > +} > + > +static __always_inline bool riscv_insn_is_c_jalr(u32 code) > +{ > + return (code & RVC_MASK_C_JALR) == RVC_MATCH_C_JALR && > + (code & RVC_INSN_J_RS1_MASK) != 0; > +} > + Nice one! In the future, add a Fixes-tag for these kind of changes! Fixes: ec5f90877516 ("RISC-V: Move riscv_insn_is_* macros into a common header") (No need for a respin, b4 will pick up the Fixes above.) Björn
On Mon, Aug 14, 2023 at 02:07:50PM +0200, Björn Töpel wrote: > In the future, add a Fixes-tag for these kind of changes! > Fixes: ec5f90877516 ("RISC-V: Move riscv_insn_is_* macros into a common header") > > (No need for a respin, b4 will pick up the Fixes above.) Thanks! I will keep that in mind in the future. Best regards, Nam
Hello: This patch was applied to riscv/linux.git (fixes) by Palmer Dabbelt <palmer@rivosinc.com>: On Mon, 31 Jul 2023 20:39:25 +0200 you wrote: > The instructions c.jr and c.jalr must have rs1 != 0, but > riscv_insn_is_c_jr() and riscv_insn_is_c_jalr() do not check for this. So, > riscv_insn_is_c_jr() can match a reserved encoding, while > riscv_insn_is_c_jalr() can match the c.ebreak instruction. > > Rewrite them with check for rs1 != 0. > > [...] Here is the summary with links: - riscv: correct riscv_insn_is_c_jr() and riscv_insn_is_c_jalr() https://git.kernel.org/riscv/c/79bc3f85c51f You are awesome, thank you!
diff --git a/arch/riscv/include/asm/insn.h b/arch/riscv/include/asm/insn.h index 4e1505cef8aa..fce00400c9bc 100644 --- a/arch/riscv/include/asm/insn.h +++ b/arch/riscv/include/asm/insn.h @@ -110,6 +110,7 @@ #define RVC_INSN_FUNCT4_OPOFF 12 #define RVC_INSN_FUNCT3_MASK GENMASK(15, 13) #define RVC_INSN_FUNCT3_OPOFF 13 +#define RVC_INSN_J_RS1_MASK GENMASK(11, 7) #define RVC_INSN_J_RS2_MASK GENMASK(6, 2) #define RVC_INSN_OPCODE_MASK GENMASK(1, 0) #define RVC_ENCODE_FUNCT3(f_) (RVC_FUNCT3_##f_ << RVC_INSN_FUNCT3_OPOFF) @@ -245,8 +246,6 @@ __RISCV_INSN_FUNCS(c_jal, RVC_MASK_C_JAL, RVC_MATCH_C_JAL) __RISCV_INSN_FUNCS(auipc, RVG_MASK_AUIPC, RVG_MATCH_AUIPC) __RISCV_INSN_FUNCS(jalr, RVG_MASK_JALR, RVG_MATCH_JALR) __RISCV_INSN_FUNCS(jal, RVG_MASK_JAL, RVG_MATCH_JAL) -__RISCV_INSN_FUNCS(c_jr, RVC_MASK_C_JR, RVC_MATCH_C_JR) -__RISCV_INSN_FUNCS(c_jalr, RVC_MASK_C_JALR, RVC_MATCH_C_JALR) __RISCV_INSN_FUNCS(c_j, RVC_MASK_C_J, RVC_MATCH_C_J) __RISCV_INSN_FUNCS(beq, RVG_MASK_BEQ, RVG_MATCH_BEQ) __RISCV_INSN_FUNCS(bne, RVG_MASK_BNE, RVG_MATCH_BNE) @@ -273,6 +272,18 @@ static __always_inline bool riscv_insn_is_branch(u32 code) return (code & RV_INSN_OPCODE_MASK) == RVG_OPCODE_BRANCH; } +static __always_inline bool riscv_insn_is_c_jr(u32 code) +{ + return (code & RVC_MASK_C_JR) == RVC_MATCH_C_JR && + (code & RVC_INSN_J_RS1_MASK) != 0; +} + +static __always_inline bool riscv_insn_is_c_jalr(u32 code) +{ + return (code & RVC_MASK_C_JALR) == RVC_MATCH_C_JALR && + (code & RVC_INSN_J_RS1_MASK) != 0; +} + #define RV_IMM_SIGN(x) (-(((x) >> 31) & 1)) #define RVC_IMM_SIGN(x) (-(((x) >> 12) & 1)) #define RV_X(X, s, mask) (((X) >> (s)) & (mask))
The instructions c.jr and c.jalr must have rs1 != 0, but riscv_insn_is_c_jr() and riscv_insn_is_c_jalr() do not check for this. So, riscv_insn_is_c_jr() can match a reserved encoding, while riscv_insn_is_c_jalr() can match the c.ebreak instruction. Rewrite them with check for rs1 != 0. Signed-off-by: Nam Cao <namcaov@gmail.com> --- arch/riscv/include/asm/insn.h | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-)