public inbox for linux-riscv@lists.infradead.org
 help / color / mirror / Atom feed
From: Conor Dooley <conor@kernel.org>
To: Heiko Stuebner <heiko@sntech.de>
Cc: linux-riscv@lists.infradead.org, palmer@dabbelt.com,
	christoph.muellner@vrull.eu, prabhakar.csengg@gmail.com,
	philipp.tomsich@vrull.eu, ajones@ventanamicro.com,
	emil.renner.berthing@canonical.com,
	Heiko Stuebner <heiko.stuebner@vrull.eu>
Subject: Re: [PATCH v2 02/13] RISC-V: detach funct-values from their offset
Date: Tue, 29 Nov 2022 22:47:52 +0000	[thread overview]
Message-ID: <Y4aMGFOuc8kPL/I+@spud> (raw)
In-Reply-To: <20221128102632.435174-3-heiko@sntech.de>

On Mon, Nov 28, 2022 at 11:26:21AM +0100, Heiko Stuebner wrote:
> From: Heiko Stuebner <heiko.stuebner@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@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@microchip.com>


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

  reply	other threads:[~2022-11-29 22:48 UTC|newest]

Thread overview: 48+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-11-28 10:26 [PATCH v2 0/13] Zbb string optimizations and call support in alternatives Heiko Stuebner
2022-11-28 10:26 ` [PATCH v2 01/13] RISC-V: add prefix to all constants/macros in parse_asm.h Heiko Stuebner
2022-11-29 22:19   ` Conor Dooley
2022-11-30 12:12     ` Heiko Stübner
2022-11-30 14:10       ` Andrew Jones
2022-11-28 10:26 ` [PATCH v2 02/13] RISC-V: detach funct-values from their offset Heiko Stuebner
2022-11-29 22:47   ` Conor Dooley [this message]
2022-11-29 23:01   ` Conor Dooley
2022-11-30 14:04     ` Heiko Stübner
2022-11-30 14:16       ` Andrew Jones
2022-11-30 14:19         ` Heiko Stübner
2022-11-30 14:51   ` Andrew Jones
2022-11-28 10:26 ` [PATCH v2 03/13] RISC-V: add ebreak instructions to definitions Heiko Stuebner
2022-11-29 22:56   ` Conor Dooley
2022-11-30 15:08   ` Andrew Jones
2022-11-28 10:26 ` [PATCH v2 04/13] RISC-V: Move riscv_insn_is_* macros into a common header Heiko Stuebner
2022-11-29 23:09   ` Conor Dooley
2022-11-29 23:14     ` Conor Dooley
2022-11-30 14:53     ` Heiko Stübner
2022-11-30 15:44   ` Andrew Jones
2022-11-28 10:26 ` [PATCH v2 05/13] RISC-V: rename parse_asm.h to insn.h Heiko Stuebner
2022-11-29 23:13   ` Conor Dooley
2022-11-30 15:47   ` Andrew Jones
2022-11-28 10:26 ` [PATCH v2 06/13] RISC-V: kprobes: use central defined funct3 constants Heiko Stuebner
2022-11-29 23:22   ` Conor Dooley
2022-11-30 19:18     ` Heiko Stübner
2022-11-30 15:51   ` Andrew Jones
2022-11-28 10:26 ` [PATCH v2 07/13] RISC-V: add auipc elements to parse_asm header Heiko Stuebner
2022-11-29 23:36   ` Conor Dooley
2022-11-30 14:43     ` Heiko Stübner
2022-11-30 15:56   ` Andrew Jones
2022-11-28 10:26 ` [PATCH v2 08/13] RISC-V: add U-type imm parsing " Heiko Stuebner
2022-11-29 23:38   ` Conor Dooley
2022-11-30 19:27     ` Heiko Stübner
2022-11-30 19:41       ` Conor Dooley
2022-11-30 16:02   ` Andrew Jones
2022-11-28 10:26 ` [PATCH v2 09/13] RISC-V: add rd reg " Heiko Stuebner
2022-11-29 23:41   ` Conor Dooley
2022-11-30 16:12   ` Andrew Jones
2022-11-28 10:26 ` [PATCH v2 10/13] RISC-V: fix auipc-jalr addresses in patched alternatives Heiko Stuebner
2022-11-30 16:37   ` Conor Dooley
2022-11-28 10:26 ` [PATCH v2 11/13] efi/riscv: libstub: mark when compiling libstub Heiko Stuebner
2022-11-28 10:26 ` [PATCH v2 12/13] RISC-V: add infrastructure to allow different str* implementations Heiko Stuebner
2022-11-28 10:26 ` [PATCH v2 13/13] RISC-V: add zbb support to string functions Heiko Stuebner
2022-11-30 16:53   ` Conor Dooley
2022-11-30 17:14   ` Conor Dooley
2022-11-30 21:28     ` Heiko Stübner
2022-11-29 22:02 ` [PATCH v2 0/13] Zbb string optimizations and call support in alternatives Conor Dooley

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=Y4aMGFOuc8kPL/I+@spud \
    --to=conor@kernel.org \
    --cc=ajones@ventanamicro.com \
    --cc=christoph.muellner@vrull.eu \
    --cc=emil.renner.berthing@canonical.com \
    --cc=heiko.stuebner@vrull.eu \
    --cc=heiko@sntech.de \
    --cc=linux-riscv@lists.infradead.org \
    --cc=palmer@dabbelt.com \
    --cc=philipp.tomsich@vrull.eu \
    --cc=prabhakar.csengg@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox