[PATCH 1/3] include: sbi: Use array for struct sbi_trap_regs and GET/SET macros
Anup Patel
anup at brainfault.org
Tue Jul 22 03:45:56 PDT 2025
On Thu, Jul 10, 2025 at 4:59 AM Jessica Clarke <jrtc27 at jrtc27.com> wrote:
>
> Rather than hand-rolling scaled pointer arithmetic with casts and
> shifts, let the compiler do so by indexing an array of GPRs, taking
> advantage of the language's type system to scale based on whatever type
> the register happens to be. This makes it easier to support CHERI where
> the registers are capabilities, not plain integers, and so this pointer
> arithmetic would need to change (and currently REGBYTES is both the size
> of a register and the size of an integer word upstream).
>
> Signed-off-by: Jessica Clarke <jrtc27 at jrtc27.com>
LGTM.
Reviewed-by: Anup Patel <anup at brainfault.org>
Applied this patch to the riscv/opensbi repo.
Thanks,
Anup
> ---
> include/sbi/riscv_encoding.h | 25 ++----
> include/sbi/sbi_trap.h | 148 ++++++++++++++++++++---------------
> 2 files changed, 92 insertions(+), 81 deletions(-)
>
> diff --git a/include/sbi/riscv_encoding.h b/include/sbi/riscv_encoding.h
> index e1256e3..f4df1ae 100644
> --- a/include/sbi/riscv_encoding.h
> +++ b/include/sbi/riscv_encoding.h
> @@ -1291,6 +1291,8 @@
> #define SHIFT_FUNCT3 12
>
> #define MASK_RS1 0xf8000
> +#define MASK_RS2 0x1f00000
> +#define MASK_RD 0xf80
>
> #define MASK_CSR 0xfff00000
> #define SHIFT_CSR 20
> @@ -1356,28 +1358,17 @@
> #define SHIFT_RIGHT(x, y) \
> ((y) < 0 ? ((x) << -(y)) : ((x) >> (y)))
>
> -#define REG_MASK \
> - ((1 << (5 + LOG_REGBYTES)) - (1 << LOG_REGBYTES))
> -
> -#define REG_OFFSET(insn, pos) \
> - (SHIFT_RIGHT((insn), (pos) - LOG_REGBYTES) & REG_MASK)
> -
> -#define REG_PTR(insn, pos, regs) \
> - (ulong *)((ulong)(regs) + REG_OFFSET(insn, pos))
> -
> #define GET_FUNC3(insn) ((insn & MASK_FUNCT3) >> SHIFT_FUNCT3)
> #define GET_RM(insn) GET_FUNC3(insn)
> -#define GET_RS1_NUM(insn) ((insn & MASK_RS1) >> 15)
> +#define GET_RS1_NUM(insn) ((insn & MASK_RS1) >> SH_RS1)
> +#define GET_RS2_NUM(insn) ((insn & MASK_RS2) >> SH_RS2)
> +#define GET_RS1S_NUM(insn) RVC_RS1S(insn)
> +#define GET_RS2S_NUM(insn) RVC_RS2S(insn)
> +#define GET_RS2C_NUM(insn) RVC_RS2(insn)
> +#define GET_RD_NUM(insn) ((insn & MASK_RD) >> SH_RD)
> #define GET_CSR_NUM(insn) ((insn & MASK_CSR) >> SHIFT_CSR)
> #define GET_AQRL(insn) ((insn & MASK_AQRL) >> SHIFT_AQRL)
>
> -#define GET_RS1(insn, regs) (*REG_PTR(insn, SH_RS1, regs))
> -#define GET_RS2(insn, regs) (*REG_PTR(insn, SH_RS2, regs))
> -#define GET_RS1S(insn, regs) (*REG_PTR(RVC_RS1S(insn), 0, regs))
> -#define GET_RS2S(insn, regs) (*REG_PTR(RVC_RS2S(insn), 0, regs))
> -#define GET_RS2C(insn, regs) (*REG_PTR(insn, SH_RS2C, regs))
> -#define GET_SP(regs) (*REG_PTR(2, 0, regs))
> -#define SET_RD(insn, regs, val) (*REG_PTR(insn, SH_RD, regs) = (val))
> #define IMM_I(insn) ((s32)(insn) >> 20)
> #define IMM_S(insn) (((s32)(insn) >> 25 << 5) | \
> (s32)(((insn) >> 7) & 0x1f))
> diff --git a/include/sbi/sbi_trap.h b/include/sbi/sbi_trap.h
> index 5eec4da..731a0c9 100644
> --- a/include/sbi/sbi_trap.h
> +++ b/include/sbi/sbi_trap.h
> @@ -127,70 +127,75 @@
>
> /** Representation of register state at time of trap/interrupt */
> struct sbi_trap_regs {
> - /** zero register state */
> - unsigned long zero;
> - /** ra register state */
> - unsigned long ra;
> - /** sp register state */
> - unsigned long sp;
> - /** gp register state */
> - unsigned long gp;
> - /** tp register state */
> - unsigned long tp;
> - /** t0 register state */
> - unsigned long t0;
> - /** t1 register state */
> - unsigned long t1;
> - /** t2 register state */
> - unsigned long t2;
> - /** s0 register state */
> - unsigned long s0;
> - /** s1 register state */
> - unsigned long s1;
> - /** a0 register state */
> - unsigned long a0;
> - /** a1 register state */
> - unsigned long a1;
> - /** a2 register state */
> - unsigned long a2;
> - /** a3 register state */
> - unsigned long a3;
> - /** a4 register state */
> - unsigned long a4;
> - /** a5 register state */
> - unsigned long a5;
> - /** a6 register state */
> - unsigned long a6;
> - /** a7 register state */
> - unsigned long a7;
> - /** s2 register state */
> - unsigned long s2;
> - /** s3 register state */
> - unsigned long s3;
> - /** s4 register state */
> - unsigned long s4;
> - /** s5 register state */
> - unsigned long s5;
> - /** s6 register state */
> - unsigned long s6;
> - /** s7 register state */
> - unsigned long s7;
> - /** s8 register state */
> - unsigned long s8;
> - /** s9 register state */
> - unsigned long s9;
> - /** s10 register state */
> - unsigned long s10;
> - /** s11 register state */
> - unsigned long s11;
> - /** t3 register state */
> - unsigned long t3;
> - /** t4 register state */
> - unsigned long t4;
> - /** t5 register state */
> - unsigned long t5;
> - /** t6 register state */
> - unsigned long t6;
> + union {
> + unsigned long gprs[32];
> + struct {
> + /** zero register state */
> + unsigned long zero;
> + /** ra register state */
> + unsigned long ra;
> + /** sp register state */
> + unsigned long sp;
> + /** gp register state */
> + unsigned long gp;
> + /** tp register state */
> + unsigned long tp;
> + /** t0 register state */
> + unsigned long t0;
> + /** t1 register state */
> + unsigned long t1;
> + /** t2 register state */
> + unsigned long t2;
> + /** s0 register state */
> + unsigned long s0;
> + /** s1 register state */
> + unsigned long s1;
> + /** a0 register state */
> + unsigned long a0;
> + /** a1 register state */
> + unsigned long a1;
> + /** a2 register state */
> + unsigned long a2;
> + /** a3 register state */
> + unsigned long a3;
> + /** a4 register state */
> + unsigned long a4;
> + /** a5 register state */
> + unsigned long a5;
> + /** a6 register state */
> + unsigned long a6;
> + /** a7 register state */
> + unsigned long a7;
> + /** s2 register state */
> + unsigned long s2;
> + /** s3 register state */
> + unsigned long s3;
> + /** s4 register state */
> + unsigned long s4;
> + /** s5 register state */
> + unsigned long s5;
> + /** s6 register state */
> + unsigned long s6;
> + /** s7 register state */
> + unsigned long s7;
> + /** s8 register state */
> + unsigned long s8;
> + /** s9 register state */
> + unsigned long s9;
> + /** s10 register state */
> + unsigned long s10;
> + /** s11 register state */
> + unsigned long s11;
> + /** t3 register state */
> + unsigned long t3;
> + /** t4 register state */
> + unsigned long t4;
> + /** t5 register state */
> + unsigned long t5;
> + /** t6 register state */
> + unsigned long t6;
> + };
> + };
> /** mepc register state */
> unsigned long mepc;
> /** mstatus register state */
> @@ -199,6 +204,21 @@ struct sbi_trap_regs {
> unsigned long mstatusH;
> };
>
> +_Static_assert(
> + sizeof(((struct sbi_trap_regs *)0)->gprs) ==
> + offsetof(struct sbi_trap_regs, t6) +
> + sizeof(((struct sbi_trap_regs *)0)->t6),
> + "struct sbi_trap_regs's layout differs between gprs and named members");
> +
> +#define REG_VAL(idx, regs) ((regs)->gprs[(idx)])
> +
> +#define GET_RS1(insn, regs) REG_VAL(GET_RS1_NUM(insn), regs)
> +#define GET_RS2(insn, regs) REG_VAL(GET_RS2_NUM(insn), regs)
> +#define GET_RS1S(insn, regs) REG_VAL(GET_RS1S_NUM(insn), regs)
> +#define GET_RS2S(insn, regs) REG_VAL(GET_RS2S_NUM(insn), regs)
> +#define GET_RS2C(insn, regs) REG_VAL(GET_RS2C_NUM(insn), regs)
> +#define SET_RD(insn, regs, val) (REG_VAL(GET_RD_NUM(insn), regs) = (val))
> +
> /** Representation of trap details */
> struct sbi_trap_info {
> /** cause Trap exception cause */
> --
> 2.34.1
>
>
> --
> opensbi mailing list
> opensbi at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi
More information about the opensbi
mailing list