Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   =?utf-8?B?QmrDtnJuIFTDtnBlbA==?= <bjorn@kernel.org>
To:     Nam Cao <namcaov@gmail.com>,
        Paul Walmsley <paul.walmsley@sifive.com>,
        Palmer Dabbelt <palmer@dabbelt.com>,
        Albert Ou <aou@eecs.berkeley.edu>,
        linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org
Cc:     Nam Cao <namcaov@gmail.com>, charlie@rivosinc.com
Subject: Re: [PATCH] riscv: correct riscv_insn_is_c_jr() and
 riscv_insn_is_c_jalr()
In-Reply-To: <20230731183925.152145-1-namcaov@gmail.com>
References: <20230731183925.152145-1-namcaov@gmail.com>
Date:   Mon, 14 Aug 2023 14:07:50 +0200
Message-ID: <87o7j9g73d.fsf@all.your.base.are.belong.to.us>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Nam Cao <namcaov@gmail.com> writes:

> The instructions c.jr and c.jalr must have rs1 !=3D 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 !=3D 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) =3D=3D RVG_OPCODE_BRANCH;
>  }
>=20=20
> +static __always_inline bool riscv_insn_is_c_jr(u32 code)
> +{
> +	return (code & RVC_MASK_C_JR) =3D=3D RVC_MATCH_C_JR &&
> +	       (code & RVC_INSN_J_RS1_MASK) !=3D 0;
> +}
> +
> +static __always_inline bool riscv_insn_is_c_jalr(u32 code)
> +{
> +	return (code & RVC_MASK_C_JALR) =3D=3D RVC_MATCH_C_JALR &&
> +	       (code & RVC_INSN_J_RS1_MASK) !=3D 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 hea=
der")

(No need for a respin, b4 will pick up the Fixes above.)


Bj=C3=B6rn