On 03/06/26, Daniel Henrique Barboza wrote: > 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
Agh, good catch and thank you Daniel! I'll address this in a v8. Alistair, if it's easier for you to do this change on your end, feel free to do so and ignore my v8. Unsure of the praxis in this situation. -- Anton Johansson rev.ng Labs Srl.
