Hi,

This patch is causing a seg fault in rmw_xireg_aia():


$ gdb --args ./build/qemu-system-riscv64 -M riscv-server-ref --nographic


(gdb) bt
#0  0x0000aaaaab3bda20 in rmw_xireg_aia
    (env=0xaaaaac194d80, csrno=849, isel=114, val=0x0, new_val=0, 
wr_mask=18446744073709551615)
    at ../target/riscv/csr.c:2735
#1  0x0000aaaaab3be320 in rmw_xireg (env=0xaaaaac194d80, csrno=849, val=0x0, 
new_val=0, wr_mask=18446744073709551615)
    at ../target/riscv/csr.c:2950
#2  0x0000aaaaab3c5628 in riscv_csrrw_do64
    (env=0xaaaaac194d80, csrno=849, ret_value=0x0, new_value=0, 
write_mask=18446744073709551615, ra=281472560986724)
    at ../target/riscv/csr.c:5813
#3  0x0000aaaaab3c5884 in riscv_csrrw
    (env=0xaaaaac194d80, csrno=849, ret_value=0x0, new_value=0, 
write_mask=18446744073709551615, ra=281472560986724)
    at ../target/riscv/csr.c:5871
#4  0x0000aaaaab3cb670 in helper_csrw (env=0xaaaaac194d80, csr=849, src=0) at 
../target/riscv/op_helper.c:75


Down there:



On 5/20/2026 9:53 AM, Anton Johansson via qemu development wrote:
In hw/ the relevant RISCVIMSICState fields
eidelivery, eithreshold, eistate are uint32_t.

Signed-off-by: Anton Johansson <[email protected]>
Reviewed-by: Pierrick Bouvier <[email protected]>
Acked-by: Alistair Francis <[email protected]>
---
  target/riscv/cpu.h        | 42 ++++++++++++++++++++-------------------
  hw/intc/riscv_imsic.c     | 34 +++++++++++++++----------------
  target/riscv/cpu_helper.c | 12 ++++-------
  target/riscv/csr.c        | 24 ++++++++++++----------
  4 files changed, 57 insertions(+), 55 deletions(-)

diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 10afc617f8..9823d7d581 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -200,6 +200,24 @@ FIELD(VTYPE, VMA, 7, 1)
  FIELD(VTYPE, ALTFMT, 8, 1)
  FIELD(VTYPE, RESERVED, 9, sizeof(uint64_t) * 8 - 10)
+#ifndef CONFIG_USER_ONLY
+/* machine specific AIA ireg read-modify-write callback */
+#define AIA_MAKE_IREG(__isel, __priv, __virt, __vgein, __xlen)                 
\
+    ((uint32_t)((((__xlen) & 0xff) << 24) |                                    
\
+                (((__vgein) & 0x3f) << 20) |                                   
\
+                (((__virt) & 0x1) << 18) |                                     
\
+                (((__priv) & 0x3) << 16) |                                     
\
+                  (__isel & 0xffff)))
+#define AIA_IREG_ISEL(__ireg) ((__ireg) & 0xffff)
+#define AIA_IREG_PRIV(__ireg) (((__ireg) >> 16) & 0x3)
+#define AIA_IREG_VIRT(__ireg) (((__ireg) >> 18) & 0x1)
+#define AIA_IREG_VGEIN(__ireg) (((__ireg) >> 20) & 0x3f)
+#define AIA_IREG_XLEN(__ireg) (((__ireg) >> 24) & 0xff)
+
+typedef int (*aia_ireg_rmw_fn)(void *arg, uint32_t reg, uint64_t *val,
+                               uint64_t new_val, uint64_t write_mask);
+#endif
+
  typedef struct PMUCTRState {
      /* Current value of a counter */
      uint64_t mhpmcounter_val;
@@ -465,20 +483,8 @@ struct CPUArchState {
      void *rdtime_fn_arg;
/* machine specific AIA ireg read-modify-write callback */
-#define AIA_MAKE_IREG(__isel, __priv, __virt, __vgein, __xlen) \
-    ((((__xlen) & 0xff) << 24) | \
-     (((__vgein) & 0x3f) << 20) | \
-     (((__virt) & 0x1) << 18) | \
-     (((__priv) & 0x3) << 16) | \
-     (__isel & 0xffff))
-#define AIA_IREG_ISEL(__ireg)                  ((__ireg) & 0xffff)
-#define AIA_IREG_PRIV(__ireg)                  (((__ireg) >> 16) & 0x3)
-#define AIA_IREG_VIRT(__ireg)                  (((__ireg) >> 18) & 0x1)
-#define AIA_IREG_VGEIN(__ireg)                 (((__ireg) >> 20) & 0x3f)
-#define AIA_IREG_XLEN(__ireg)                  (((__ireg) >> 24) & 0xff)
-    int (*aia_ireg_rmw_fn[4])(void *arg, target_ulong reg,
-        target_ulong *val, target_ulong new_val, target_ulong write_mask);
-    void *aia_ireg_rmw_fn_arg[4];
+    aia_ireg_rmw_fn aia_ireg_rmw_cb[4];
+    void *aia_ireg_rmw_cb_arg[4];
/* True if in debugger mode. */
      bool debugger;
@@ -646,12 +652,8 @@ void riscv_cpu_interrupt(CPURISCVState *env);
  #define BOOL_TO_MASK(x) (-!!(x)) /* helper for riscv_cpu_update_mip value */
  void riscv_cpu_set_rdtime_fn(CPURISCVState *env, uint64_t (*fn)(void *),
                               void *arg);
-void riscv_cpu_set_aia_ireg_rmw_fn(CPURISCVState *env, privilege_mode_t priv,
-                                   int (*rmw_fn)(void *arg,
-                                                 target_ulong reg,
-                                                 target_ulong *val,
-                                                 target_ulong new_val,
-                                                 target_ulong write_mask),
+void riscv_cpu_set_aia_ireg_rmw_cb(CPURISCVState *env, privilege_mode_t priv,
+                                   aia_ireg_rmw_fn rmw_fn,
                                     void *rmw_fn_arg);
RISCVException smstateen_acc_ok(CPURISCVState *env, int index, uint64_t bit);
diff --git a/hw/intc/riscv_imsic.c b/hw/intc/riscv_imsic.c
index 7c9a012033..3ce9f146c0 100644
--- a/hw/intc/riscv_imsic.c
+++ b/hw/intc/riscv_imsic.c
@@ -88,11 +88,11 @@ static void riscv_imsic_update(RISCVIMSICState *imsic, 
uint32_t page)
  }
static int riscv_imsic_eidelivery_rmw(RISCVIMSICState *imsic, uint32_t page,
-                                      target_ulong *val,
-                                      target_ulong new_val,
-                                      target_ulong wr_mask)
+                                      uint64_t *val,
+                                      uint64_t new_val,
+                                      uint64_t wr_mask)
  {
-    target_ulong old_val = imsic->eidelivery[page];
+    uint32_t old_val = imsic->eidelivery[page];
if (val) {
          *val = old_val;
@@ -106,11 +106,11 @@ static int riscv_imsic_eidelivery_rmw(RISCVIMSICState 
*imsic, uint32_t page,
  }
static int riscv_imsic_eithreshold_rmw(RISCVIMSICState *imsic, uint32_t page,
-                                      target_ulong *val,
-                                      target_ulong new_val,
-                                      target_ulong wr_mask)
+                                      uint64_t *val,
+                                      uint64_t new_val,
+                                      uint64_t wr_mask)
  {
-    target_ulong old_val = imsic->eithreshold[page];
+    uint32_t old_val = imsic->eithreshold[page];
if (val) {
          *val = old_val;
@@ -124,8 +124,8 @@ static int riscv_imsic_eithreshold_rmw(RISCVIMSICState 
*imsic, uint32_t page,
  }
static int riscv_imsic_topei_rmw(RISCVIMSICState *imsic, uint32_t page,
-                                 target_ulong *val, target_ulong new_val,
-                                 target_ulong wr_mask)
+                                 uint64_t *val, uint64_t new_val,
+                                 uint64_t wr_mask)
  {
      uint32_t base, topei = riscv_imsic_topei(imsic, page);
@@ -149,11 +149,11 @@ static int riscv_imsic_topei_rmw(RISCVIMSICState *imsic, uint32_t page, static int riscv_imsic_eix_rmw(RISCVIMSICState *imsic,
                                 uint32_t xlen, uint32_t page,
-                               uint32_t num, bool pend, target_ulong *val,
-                               target_ulong new_val, target_ulong wr_mask)
+                               uint32_t num, bool pend, uint64_t *val,
+                               uint64_t new_val, uint64_t wr_mask)
  {
      uint32_t i, base, prev;
-    target_ulong mask;
+    uint64_t mask;
      uint32_t state = (pend) ? IMSIC_EISTATE_PENDING : IMSIC_EISTATE_ENABLED;
if (xlen != 32) {
@@ -178,7 +178,7 @@ static int riscv_imsic_eix_rmw(RISCVIMSICState *imsic,
              continue;
          }
- mask = (target_ulong)1 << i;
+        mask = 1ull << i;
          if (wr_mask & mask) {
              if (new_val & mask) {
                  prev = qatomic_fetch_or(&imsic->eistate[base + i], state);
@@ -197,8 +197,8 @@ static int riscv_imsic_eix_rmw(RISCVIMSICState *imsic,
      return 0;
  }
-static int riscv_imsic_rmw(void *arg, target_ulong reg, target_ulong *val,
-                           target_ulong new_val, target_ulong wr_mask)
+static int riscv_imsic_rmw(void *arg, uint32_t reg, uint64_t *val,
+                           uint64_t new_val, uint64_t wr_mask)
  {
      RISCVIMSICState *imsic = arg;
      uint32_t isel, priv, virt, vgein, xlen, page;
@@ -383,7 +383,7 @@ static void riscv_imsic_realize(DeviceState *dev, Error 
**errp)
          }
if (!kvm_irqchip_in_kernel()) {
-            riscv_cpu_set_aia_ireg_rmw_fn(env, (imsic->mmode) ? PRV_M : PRV_S,
+            riscv_cpu_set_aia_ireg_rmw_cb(env, (imsic->mmode) ? PRV_M : PRV_S,
                                            riscv_imsic_rmw, imsic);
          }
      }
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index e7c0ff49d0..90c126fdc2 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -816,17 +816,13 @@ void riscv_cpu_set_rdtime_fn(CPURISCVState *env, uint64_t 
(*fn)(void *),
      env->rdtime_fn_arg = arg;
  }
-void riscv_cpu_set_aia_ireg_rmw_fn(CPURISCVState *env, privilege_mode_t priv,
-                                   int (*rmw_fn)(void *arg,
-                                                 target_ulong reg,
-                                                 target_ulong *val,
-                                                 target_ulong new_val,
-                                                 target_ulong write_mask),
+void riscv_cpu_set_aia_ireg_rmw_cb(CPURISCVState *env, privilege_mode_t priv,
+                                   aia_ireg_rmw_fn rmw_fn,
                                     void *rmw_fn_arg)
  {
      if (priv <= PRV_M) {
-        env->aia_ireg_rmw_fn[priv] = rmw_fn;
-        env->aia_ireg_rmw_fn_arg[priv] = rmw_fn_arg;
+        env->aia_ireg_rmw_cb[priv] = rmw_fn;
+        env->aia_ireg_rmw_cb_arg[priv] = rmw_fn_arg;
      }
  }
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 4bab7ec29f..837212b55f 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -2655,6 +2655,7 @@ static RISCVException rmw_xireg_aia(CPURISCVState *env, 
int csrno,
      uint8_t *iprio;
      privilege_mode_t priv;
      uint32_t vgein;
+    uint64_t wide_val;
/* VS-mode CSR number passed in has already been translated */
      switch (csrno) {
@@ -2699,16 +2700,17 @@ static RISCVException rmw_xireg_aia(CPURISCVState *env, 
int csrno,
          }
      } else if (ISELECT_IMSIC_FIRST <= isel && isel <= ISELECT_IMSIC_LAST) {
          /* IMSIC registers only available when machine implements it. */
-        if (env->aia_ireg_rmw_fn[priv]) {
+        if (env->aia_ireg_rmw_cb[priv]) {
              /* Selected guest interrupt file should not be zero */
              if (virt && (!vgein || env->geilen < vgein)) {
                  goto done;
              }
              /* Call machine specific IMSIC register emulation */
-            ret = env->aia_ireg_rmw_fn[priv](env->aia_ireg_rmw_fn_arg[priv],
+            ret = env->aia_ireg_rmw_cb[priv](env->aia_ireg_rmw_cb_arg[priv],
                                      AIA_MAKE_IREG(isel, priv, virt, vgein,
                                                    riscv_cpu_mxl_bits(env)),
-                                    val, new_val, wr_mask);
+                                    &wide_val, new_val, wr_mask);
+            *val = wide_val;

Here.  We can't just do *val = wide_val because there's no guarantee that *val,
i.e. the 'ret_value' of riscv_csr_op_fn(), is not NULL:

typedef RISCVException (*riscv_csr_op_fn)(CPURISCVState *env, int csrno,
                                          target_ulong *ret_value,  <--------
                                          target_ulong new_value,
                                          target_ulong write_mask);

This fixes it:

   if (val) {
       *val = wide_val;
   }


Note that this is the same pattern used in other predicates.  E.g
rmw_xiselect() does:


    if (val) {
        *val = *iselect;
    }



rmw_xtopei down there needs the same:

          }
      } else {
          isel_reserved = true;
@@ -2941,6 +2943,7 @@ static RISCVException rmw_xtopei(CPURISCVState *env, int 
csrno,
      int ret = -EINVAL;
      privilege_mode_t priv;
      uint32_t vgein;
+    uint64_t wide_val;
/* Translate CSR number for VS-mode */
      csrno = aia_xlate_vs_csrno(env, csrno);
@@ -2966,7 +2969,7 @@ static RISCVException rmw_xtopei(CPURISCVState *env, int 
csrno,
      };
/* IMSIC CSRs only available when machine implements IMSIC. */
-    if (!env->aia_ireg_rmw_fn[priv]) {
+    if (!env->aia_ireg_rmw_cb[priv]) {
          goto done;
      }
@@ -2979,10 +2982,11 @@ static RISCVException rmw_xtopei(CPURISCVState *env, int csrno,
      }
/* Call machine specific IMSIC register emulation for TOPEI */
-    ret = env->aia_ireg_rmw_fn[priv](env->aia_ireg_rmw_fn_arg[priv],
+    ret = env->aia_ireg_rmw_cb[priv](env->aia_ireg_rmw_cb_arg[priv],
                      AIA_MAKE_IREG(ISELECT_IMSIC_TOPEI, priv, virt, vgein,
                                    riscv_cpu_mxl_bits(env)),
-                    val, new_val, wr_mask);
+                    &wide_val, new_val, wr_mask);
+    *val = wide_val;

This needs to be:

    if (val) {
        *val = wide_val;
    }



Thanks,
Daniel

done:
      if (ret) {
@@ -4486,7 +4490,7 @@ static RISCVException read_vstopi(CPURISCVState *env, int 
csrno,
                                    target_ulong *val)
  {
      int irq, ret;
-    target_ulong topei;
+    uint64_t topei = 0;
      uint64_t vseip, vsgein;
      uint32_t iid, iprio, hviid, hviprio, gein;
      uint32_t s, scount = 0, siid[VSTOPI_NUM_SRCS], siprio[VSTOPI_NUM_SRCS];
@@ -4501,13 +4505,13 @@ static RISCVException read_vstopi(CPURISCVState *env, 
int csrno,
          if (gein <= env->geilen && vseip) {
              siid[scount] = IRQ_S_EXT;
              siprio[scount] = IPRIO_MMAXIPRIO + 1;
-            if (env->aia_ireg_rmw_fn[PRV_S]) {
+            if (env->aia_ireg_rmw_cb[PRV_S]) {
                  /*
                   * Call machine specific IMSIC register emulation for
                   * reading TOPEI.
                   */
-                ret = env->aia_ireg_rmw_fn[PRV_S](
-                        env->aia_ireg_rmw_fn_arg[PRV_S],
+                ret = env->aia_ireg_rmw_cb[PRV_S](
+                        env->aia_ireg_rmw_cb_arg[PRV_S],
                          AIA_MAKE_IREG(ISELECT_IMSIC_TOPEI, PRV_S, true, gein,
                                        riscv_cpu_mxl_bits(env)),
                          &topei, 0, 0);


Reply via email to