[PATCH v2 02/13] RISC-V: detach funct-values from their offset

Tue Nov 29 14:47:52 PST 2022

On Mon, Nov 28, 2022 at 11:26:21AM +0100, Heiko Stuebner wrote:
> From: Heiko Stuebner <heiko.stuebner at vrull.eu>
> 
> Only carry the actual values for the funct3, funct4, etc instruction
> fields without directly including the shift to the target position.

"Only carry the actual values..." didn't really make sense to me until I
actually looked at the diff. I'm not sure what "better" wording to
suggest though... "Rather than defining funct3 (...) etc instruction
fields, pre-shifted to their target positions, define the values
themselves."

Is that better? I don't know, but at least I didn't say "I hate this"
without trying to help :)

> Instead use macros to move the values to their target positions
> when needed.
> 
> This at the same time also reduces the use of magic numbers,
> one would need a spec manual to understand.

I'm always down to avoid having to read a spec :)

> Signed-off-by: Heiko Stuebner <heiko.stuebner at vrull.eu>
> ---
>  arch/riscv/include/asm/parse_asm.h | 100 +++++++++++++++++------------
>  1 file changed, 59 insertions(+), 41 deletions(-)
> 
> diff --git a/arch/riscv/include/asm/parse_asm.h b/arch/riscv/include/asm/parse_asm.h
> index c52b8ae02cfe..e3f87da108f4 100644
> --- a/arch/riscv/include/asm/parse_asm.h
> +++ b/arch/riscv/include/asm/parse_asm.h
> @@ -5,6 +5,15 @@
>  
>  #include <linux/bits.h>
>  
> +#define RV_INSN_FUNCT3_MASK	GENMASK(14, 12)
> +#define RV_INSN_FUNCT3_OPOFF	12
> +#define RV_INSN_OPCODE_MASK	GENMASK(6, 0)
> +#define RV_INSN_OPCODE_OPOFF	0
> +#define RV_INSN_FUNCT12_OPOFF	20
> +
> +#define RV_ENCODE_FUNCT3(f_)	(RVG_FUNCT3_##f_ << RV_INSN_FUNCT3_OPOFF)
> +#define RV_ENCODE_FUNCT12(f_)	(RVG_FUNCT12_##f_ << RV_INSN_FUNCT12_OPOFF)
> +
>  /* The bit field of immediate value in I-type instruction */
>  #define RV_I_IMM_SIGN_OPOFF	31
>  #define RV_I_IMM_11_0_OPOFF	20
> @@ -84,6 +93,15 @@
>  #define RVC_B_IMM_2_1_MASK	GENMASK(1, 0)
>  #define RVC_B_IMM_5_MASK	GENMASK(0, 0)
>  
> +#define RVC_INSN_FUNCT4_MASK	GENMASK(15, 12)
> +#define RVC_INSN_FUNCT4_OPOFF	12
> +#define RVC_INSN_FUNCT3_MASK	GENMASK(15, 13)
> +#define RVC_INSN_FUNCT3_OPOFF	13
> +#define RVC_INSN_J_RS2_MASK	GENMASK(6, 2)

Since I won't feel comfortable reviewing this without checking the
specs, I did and god is Table 1.1 in the v1.7 compressed spec
horrifically formatted. The numbers aren't even consistently spaced.
My poor OCD! I thought they made these things with adoc or LaTeX...

> +#define RVC_INSN_OPCODE_MASK	GENMASK(1, 0)
> +#define RVC_ENCODE_FUNCT3(f_)	(RVC_FUNCT3_##f_ << RVC_INSN_FUNCT3_OPOFF)
> +#define RVC_ENCODE_FUNCT4(f_)	(RVC_FUNCT4_##f_ << RVC_INSN_FUNCT4_OPOFF)
> +
>  /* The register offset in RVC op=C0 instruction */
>  #define RVC_C0_RS1_OPOFF	7
>  #define RVC_C0_RS2_OPOFF	2
> @@ -113,52 +131,52 @@
>  /* parts of funct3 code for I, M, A extension*/
>  #define RVG_FUNCT3_JALR		0x0
>  #define RVG_FUNCT3_BEQ		0x0
> -#define RVG_FUNCT3_BNE		0x1000
> -#define RVG_FUNCT3_BLT		0x4000
> -#define RVG_FUNCT3_BGE		0x5000
> -#define RVG_FUNCT3_BLTU		0x6000
> -#define RVG_FUNCT3_BGEU		0x7000
> +#define RVG_FUNCT3_BNE		0x1
> +#define RVG_FUNCT3_BLT		0x4
> +#define RVG_FUNCT3_BGE		0x5
> +#define RVG_FUNCT3_BLTU		0x6
> +#define RVG_FUNCT3_BGEU		0x7
>  
>  /* parts of funct3 code for C extension*/
> -#define RVC_FUNCT3_C_BEQZ	0xc000
> -#define RVC_FUNCT3_C_BNEZ	0xe000
> -#define RVC_FUNCT3_C_J		0xa000
> -#define RVC_FUNCT3_C_JAL	0x2000
> -#define RVC_FUNCT4_C_JR		0x8000
> -#define RVC_FUNCT4_C_JALR	0xf000
> +#define RVC_FUNCT3_C_BEQZ	0x6
> +#define RVC_FUNCT3_C_BNEZ	0x7
> +#define RVC_FUNCT3_C_J		0x5
> +#define RVC_FUNCT3_C_JAL	0x1
> +#define RVC_FUNCT4_C_JR		0x8
> +#define RVC_FUNCT4_C_JALR	0x9
>  
> -#define RVG_FUNCT12_SRET	0x10200000
> +#define RVG_FUNCT12_SRET	0x102

>  
> -#define RVG_MATCH_JALR		(RVG_FUNCT3_JALR | RVG_OPCODE_JALR)
> +#define RVG_MATCH_JALR		(RV_ENCODE_FUNCT3(JALR) | RVG_OPCODE_JALR)
>  #define RVG_MATCH_JAL		(RVG_OPCODE_JAL)
> -#define RVG_MATCH_BEQ		(RVG_FUNCT3_BEQ | RVG_OPCODE_BRANCH)
> -#define RVG_MATCH_BNE		(RVG_FUNCT3_BNE | RVG_OPCODE_BRANCH)
> -#define RVG_MATCH_BLT		(RVG_FUNCT3_BLT | RVG_OPCODE_BRANCH)
> -#define RVG_MATCH_BGE		(RVG_FUNCT3_BGE | RVG_OPCODE_BRANCH)
> -#define RVG_MATCH_BLTU		(RVG_FUNCT3_BLTU | RVG_OPCODE_BRANCH)
> -#define RVG_MATCH_BGEU		(RVG_FUNCT3_BGEU | RVG_OPCODE_BRANCH)
> -#define RVG_MATCH_SRET		(RVG_FUNCT12_SRET | RVG_OPCODE_SYSTEM)
> -#define RVC_MATCH_C_BEQZ	(RVC_FUNCT3_C_BEQZ | RVC_OPCODE_C1)
> -#define RVC_MATCH_C_BNEZ	(RVC_FUNCT3_C_BNEZ | RVC_OPCODE_C1)
> -#define RVC_MATCH_C_J		(RVC_FUNCT3_C_J | RVC_OPCODE_C1)
> -#define RVC_MATCH_C_JAL		(RVC_FUNCT3_C_JAL | RVC_OPCODE_C1)
> -#define RVC_MATCH_C_JR		(RVC_FUNCT4_C_JR | RVC_OPCODE_C2)
> -#define RVC_MATCH_C_JALR	(RVC_FUNCT4_C_JALR | RVC_OPCODE_C2)
> -
> -#define RVG_MASK_JALR		0x707f
> -#define RVG_MASK_JAL		0x7f
> -#define RVC_MASK_C_JALR		0xf07f
> -#define RVC_MASK_C_JR		0xf07f
> -#define RVC_MASK_C_JAL		0xe003
> -#define RVC_MASK_C_J		0xe003
> -#define RVG_MASK_BEQ		0x707f
> -#define RVG_MASK_BNE		0x707f
> -#define RVG_MASK_BLT		0x707f
> -#define RVG_MASK_BGE		0x707f
> -#define RVG_MASK_BLTU		0x707f
> -#define RVG_MASK_BGEU		0x707f
> -#define RVC_MASK_C_BEQZ		0xe003
> -#define RVC_MASK_C_BNEZ		0xe003
> +#define RVG_MATCH_BEQ		(RV_ENCODE_FUNCT3(BEQ) | RVG_OPCODE_BRANCH)
> +#define RVG_MATCH_BNE		(RV_ENCODE_FUNCT3(BNE) | RVG_OPCODE_BRANCH)
> +#define RVG_MATCH_BLT		(RV_ENCODE_FUNCT3(BLT) | RVG_OPCODE_BRANCH)
> +#define RVG_MATCH_BGE		(RV_ENCODE_FUNCT3(BGE) | RVG_OPCODE_BRANCH)
> +#define RVG_MATCH_BLTU		(RV_ENCODE_FUNCT3(BLTU) | RVG_OPCODE_BRANCH)
> +#define RVG_MATCH_BGEU		(RV_ENCODE_FUNCT3(BGEU) | RVG_OPCODE_BRANCH)
> +#define RVG_MATCH_SRET		(RV_ENCODE_FUNCT12(SRET) | RVG_OPCODE_SYSTEM)
> +#define RVC_MATCH_C_BEQZ	(RVC_ENCODE_FUNCT3(C_BEQZ) | RVC_OPCODE_C1)
> +#define RVC_MATCH_C_BNEZ	(RVC_ENCODE_FUNCT3(C_BNEZ) | RVC_OPCODE_C1)
> +#define RVC_MATCH_C_J		(RVC_ENCODE_FUNCT3(C_J) | RVC_OPCODE_C1)
> +#define RVC_MATCH_C_JAL		(RVC_ENCODE_FUNCT3(C_JAL) | RVC_OPCODE_C1)
> +#define RVC_MATCH_C_JR		(RVC_ENCODE_FUNCT4(C_JR) | RVC_OPCODE_C2)
> +#define RVC_MATCH_C_JALR	(RVC_ENCODE_FUNCT4(C_JALR) | RVC_OPCODE_C2)
> +
> +#define RVG_MASK_JALR		(RV_INSN_FUNCT3_MASK | RV_INSN_OPCODE_MASK)
> +#define RVG_MASK_JAL		(RV_INSN_OPCODE_MASK)
> +#define RVC_MASK_C_JALR		(RVC_INSN_FUNCT4_MASK | RVC_INSN_J_RS2_MASK | RVC_INSN_OPCODE_MASK)
> +#define RVC_MASK_C_JR		(RVC_INSN_FUNCT4_MASK | RVC_INSN_J_RS2_MASK | RVC_INSN_OPCODE_MASK)
> +#define RVC_MASK_C_JAL		(RVC_INSN_FUNCT3_MASK | RVC_INSN_OPCODE_MASK)
> +#define RVC_MASK_C_J		(RVC_INSN_FUNCT3_MASK | RVC_INSN_OPCODE_MASK)
> +#define RVG_MASK_BEQ		(RV_INSN_FUNCT3_MASK | RV_INSN_OPCODE_MASK)
> +#define RVG_MASK_BNE		(RV_INSN_FUNCT3_MASK | RV_INSN_OPCODE_MASK)
> +#define RVG_MASK_BLT		(RV_INSN_FUNCT3_MASK | RV_INSN_OPCODE_MASK)
> +#define RVG_MASK_BGE		(RV_INSN_FUNCT3_MASK | RV_INSN_OPCODE_MASK)
> +#define RVG_MASK_BLTU		(RV_INSN_FUNCT3_MASK | RV_INSN_OPCODE_MASK)
> +#define RVG_MASK_BGEU		(RV_INSN_FUNCT3_MASK | RV_INSN_OPCODE_MASK)
> +#define RVC_MASK_C_BEQZ		(RVC_INSN_FUNCT3_MASK | RVC_INSN_OPCODE_MASK)
> +#define RVC_MASK_C_BNEZ		(RVC_INSN_FUNCT3_MASK | RVC_INSN_OPCODE_MASK)
>  #define RVG_MASK_SRET		0xffffffff
>  
>  #define __INSN_LENGTH_MASK	_UL(0x3)

My eyes got tired near the end of this, but I had a good through and
spot checked against the values in the spec docs and all came up good
for me. I definitely like the cases where you manage to avoid using
defines that'd require an RE effort to figure out what the actual fields
are.
Reviewed-by: Conor Dooley <conor.dooley at microchip.com>