* [PATCH 1/3] include: sbi: Use array for struct sbi_trap_regs and GET/SET macros
@ 2025-07-09 23:29 Jessica Clarke
2025-07-09 23:29 ` [PATCH 2/3] firmware: Replace sole uses of REGBYTES with __SIZEOF_LONG__ Jessica Clarke
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Jessica Clarke @ 2025-07-09 23:29 UTC (permalink / raw)
To: opensbi; +Cc: Jessica Clarke
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@jrtc27.com>
---
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@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/3] firmware: Replace sole uses of REGBYTES with __SIZEOF_LONG__
2025-07-09 23:29 [PATCH 1/3] include: sbi: Use array for struct sbi_trap_regs and GET/SET macros Jessica Clarke
@ 2025-07-09 23:29 ` Jessica Clarke
2025-07-22 10:46 ` Anup Patel
2025-07-09 23:29 ` [PATCH 3/3] include: sbi: Remove unused (LOG_)REGBYTES Jessica Clarke
2025-07-22 10:45 ` [PATCH 1/3] include: sbi: Use array for struct sbi_trap_regs and GET/SET macros Anup Patel
2 siblings, 1 reply; 6+ messages in thread
From: Jessica Clarke @ 2025-07-09 23:29 UTC (permalink / raw)
To: opensbi; +Cc: Jessica Clarke
This code has nothing to do with the ISA's registers, it's about the
format of ELF relocations. As such, __SIZEOF_LONG__, being a language /
ABI-level property, is a more appropriate constant to use. This also
makes it easier to support CHERI, where general-purpose registers are
extended to be capabilities, not just integers, and so the register size
is not the same as the machine word size. This also happens to make it
more correct for RV64ILP32, where the registers are 64-bit integers but
the ABI is 32-bit (both for long and for the ELF format), though
properly supporting that ABI is not part of the motivation here, just a
consequence of improving the code for CHERI.
Signed-off-by: Jessica Clarke <jrtc27@jrtc27.com>
---
firmware/fw_base.S | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/firmware/fw_base.S b/firmware/fw_base.S
index 2498797..aa8e264 100644
--- a/firmware/fw_base.S
+++ b/firmware/fw_base.S
@@ -80,17 +80,17 @@ _sc_fail:
lla t1, __rel_dyn_end
beq t0, t1, _relocate_done
2:
- REG_L t5, REGBYTES(t0) /* t5 <-- relocation info:type */
+ REG_L t5, __SIZEOF_LONG__(t0) /* t5 <-- relocation info:type */
li t3, R_RISCV_RELATIVE /* reloc type R_RISCV_RELATIVE */
bne t5, t3, 3f
REG_L t3, 0(t0)
- REG_L t5, (REGBYTES * 2)(t0) /* t5 <-- addend */
+ REG_L t5, (__SIZEOF_LONG__ * 2)(t0) /* t5 <-- addend */
add t5, t5, t2
add t3, t3, t2
REG_S t5, 0(t3) /* store runtime address to the GOT entry */
3:
- addi t0, t0, (REGBYTES * 3)
+ addi t0, t0, (__SIZEOF_LONG__ * 3)
blt t0, t1, 2b
_relocate_done:
/* At this point we are running from link address */
--
2.34.1
--
opensbi mailing list
opensbi@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 3/3] include: sbi: Remove unused (LOG_)REGBYTES
2025-07-09 23:29 [PATCH 1/3] include: sbi: Use array for struct sbi_trap_regs and GET/SET macros Jessica Clarke
2025-07-09 23:29 ` [PATCH 2/3] firmware: Replace sole uses of REGBYTES with __SIZEOF_LONG__ Jessica Clarke
@ 2025-07-09 23:29 ` Jessica Clarke
2025-07-22 10:47 ` Anup Patel
2025-07-22 10:45 ` [PATCH 1/3] include: sbi: Use array for struct sbi_trap_regs and GET/SET macros Anup Patel
2 siblings, 1 reply; 6+ messages in thread
From: Jessica Clarke @ 2025-07-09 23:29 UTC (permalink / raw)
To: opensbi; +Cc: Jessica Clarke
These are no longer used, so remove them.
Signed-off-by: Jessica Clarke <jrtc27@jrtc27.com>
---
include/sbi/riscv_encoding.h | 7 -------
1 file changed, 7 deletions(-)
diff --git a/include/sbi/riscv_encoding.h b/include/sbi/riscv_encoding.h
index f4df1ae..40a854e 100644
--- a/include/sbi/riscv_encoding.h
+++ b/include/sbi/riscv_encoding.h
@@ -1317,13 +1317,6 @@
#define INSN_LEN(insn) (INSN_IS_16BIT(insn) ? 2 : 4)
-#if __riscv_xlen == 64
-#define LOG_REGBYTES 3
-#else
-#define LOG_REGBYTES 2
-#endif
-#define REGBYTES (1 << LOG_REGBYTES)
-
#define SH_VSEW 3
#define SH_VIEW 12
#define SH_VD 7
--
2.34.1
--
opensbi mailing list
opensbi@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/3] include: sbi: Use array for struct sbi_trap_regs and GET/SET macros
2025-07-09 23:29 [PATCH 1/3] include: sbi: Use array for struct sbi_trap_regs and GET/SET macros Jessica Clarke
2025-07-09 23:29 ` [PATCH 2/3] firmware: Replace sole uses of REGBYTES with __SIZEOF_LONG__ Jessica Clarke
2025-07-09 23:29 ` [PATCH 3/3] include: sbi: Remove unused (LOG_)REGBYTES Jessica Clarke
@ 2025-07-22 10:45 ` Anup Patel
2 siblings, 0 replies; 6+ messages in thread
From: Anup Patel @ 2025-07-22 10:45 UTC (permalink / raw)
To: Jessica Clarke; +Cc: opensbi
On Thu, Jul 10, 2025 at 4:59 AM Jessica Clarke <jrtc27@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@jrtc27.com>
LGTM.
Reviewed-by: Anup Patel <anup@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@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi
--
opensbi mailing list
opensbi@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/3] firmware: Replace sole uses of REGBYTES with __SIZEOF_LONG__
2025-07-09 23:29 ` [PATCH 2/3] firmware: Replace sole uses of REGBYTES with __SIZEOF_LONG__ Jessica Clarke
@ 2025-07-22 10:46 ` Anup Patel
0 siblings, 0 replies; 6+ messages in thread
From: Anup Patel @ 2025-07-22 10:46 UTC (permalink / raw)
To: Jessica Clarke; +Cc: opensbi
On Thu, Jul 10, 2025 at 4:59 AM Jessica Clarke <jrtc27@jrtc27.com> wrote:
>
> This code has nothing to do with the ISA's registers, it's about the
> format of ELF relocations. As such, __SIZEOF_LONG__, being a language /
> ABI-level property, is a more appropriate constant to use. This also
> makes it easier to support CHERI, where general-purpose registers are
> extended to be capabilities, not just integers, and so the register size
> is not the same as the machine word size. This also happens to make it
> more correct for RV64ILP32, where the registers are 64-bit integers but
> the ABI is 32-bit (both for long and for the ELF format), though
> properly supporting that ABI is not part of the motivation here, just a
> consequence of improving the code for CHERI.
>
> Signed-off-by: Jessica Clarke <jrtc27@jrtc27.com>
LGTM.
Reviewed-by: Anup Patel <anup@brainfault.org>
Applied this patch to the riscv/opensbi repo.
Thanks,
Anup
> ---
> firmware/fw_base.S | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/firmware/fw_base.S b/firmware/fw_base.S
> index 2498797..aa8e264 100644
> --- a/firmware/fw_base.S
> +++ b/firmware/fw_base.S
> @@ -80,17 +80,17 @@ _sc_fail:
> lla t1, __rel_dyn_end
> beq t0, t1, _relocate_done
> 2:
> - REG_L t5, REGBYTES(t0) /* t5 <-- relocation info:type */
> + REG_L t5, __SIZEOF_LONG__(t0) /* t5 <-- relocation info:type */
> li t3, R_RISCV_RELATIVE /* reloc type R_RISCV_RELATIVE */
> bne t5, t3, 3f
> REG_L t3, 0(t0)
> - REG_L t5, (REGBYTES * 2)(t0) /* t5 <-- addend */
> + REG_L t5, (__SIZEOF_LONG__ * 2)(t0) /* t5 <-- addend */
> add t5, t5, t2
> add t3, t3, t2
> REG_S t5, 0(t3) /* store runtime address to the GOT entry */
>
> 3:
> - addi t0, t0, (REGBYTES * 3)
> + addi t0, t0, (__SIZEOF_LONG__ * 3)
> blt t0, t1, 2b
> _relocate_done:
> /* At this point we are running from link address */
> --
> 2.34.1
>
>
> --
> opensbi mailing list
> opensbi@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi
--
opensbi mailing list
opensbi@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 3/3] include: sbi: Remove unused (LOG_)REGBYTES
2025-07-09 23:29 ` [PATCH 3/3] include: sbi: Remove unused (LOG_)REGBYTES Jessica Clarke
@ 2025-07-22 10:47 ` Anup Patel
0 siblings, 0 replies; 6+ messages in thread
From: Anup Patel @ 2025-07-22 10:47 UTC (permalink / raw)
To: Jessica Clarke; +Cc: opensbi
On Thu, Jul 10, 2025 at 4:59 AM Jessica Clarke <jrtc27@jrtc27.com> wrote:
>
> These are no longer used, so remove them.
>
> Signed-off-by: Jessica Clarke <jrtc27@jrtc27.com>
LGTM.
Reviewed-by: Anup Patel <anup@brainfault.org>
Applied this patch to the riscv/opensbi repo.
Thanks,
Anup
> ---
> include/sbi/riscv_encoding.h | 7 -------
> 1 file changed, 7 deletions(-)
>
> diff --git a/include/sbi/riscv_encoding.h b/include/sbi/riscv_encoding.h
> index f4df1ae..40a854e 100644
> --- a/include/sbi/riscv_encoding.h
> +++ b/include/sbi/riscv_encoding.h
> @@ -1317,13 +1317,6 @@
>
> #define INSN_LEN(insn) (INSN_IS_16BIT(insn) ? 2 : 4)
>
> -#if __riscv_xlen == 64
> -#define LOG_REGBYTES 3
> -#else
> -#define LOG_REGBYTES 2
> -#endif
> -#define REGBYTES (1 << LOG_REGBYTES)
> -
> #define SH_VSEW 3
> #define SH_VIEW 12
> #define SH_VD 7
> --
> 2.34.1
>
>
> --
> opensbi mailing list
> opensbi@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi
--
opensbi mailing list
opensbi@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-07-22 11:00 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-07-09 23:29 [PATCH 1/3] include: sbi: Use array for struct sbi_trap_regs and GET/SET macros Jessica Clarke
2025-07-09 23:29 ` [PATCH 2/3] firmware: Replace sole uses of REGBYTES with __SIZEOF_LONG__ Jessica Clarke
2025-07-22 10:46 ` Anup Patel
2025-07-09 23:29 ` [PATCH 3/3] include: sbi: Remove unused (LOG_)REGBYTES Jessica Clarke
2025-07-22 10:47 ` Anup Patel
2025-07-22 10:45 ` [PATCH 1/3] include: sbi: Use array for struct sbi_trap_regs and GET/SET macros Anup Patel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox