On Thu, 2026-10-15 at 02:15 +0800, wangyang wrote: > helper_cbo_zero() aligns the address down to the cache-block size > before > probe_write(), and accel/tcg/user-exec.c:probe_access_internal() > reports > that aligned address to cpu_loop_exit_sigsegv(), so a store fault > from a > legal cbo.zero (e.g. on a read-only page) reports the aligned block > base > as si_addr instead of the rs1 value supplied by the program. Native > RISC-V Linux reports the exact rs1 address. Keep the original address > in > CPUState and use it when the fault is reported.
I find this confusing. Can you please describe the issue in terms of the RISC-V spec, not what Linux does. For example: The RISC-V spec states that for cbo.zero: """ On faults, the faulting virtual address is considered to be the value in rs1, rather than the base address of the cache block. """ QEMU currently does... This commit fixes this by... > > The saved address must not outlive the instruction that produced it. > The > helper alone cannot guarantee that, because a store trigger or > breakpoint on the cbo.zero access leaves through the debug path > instead > of one of the four fault consumers, and a later unrelated fault would > then report the stale cbo.zero address. Also drop the pending > override > on every trap delivery: at the top of riscv_cpu_do_interrupt() and in > riscv_cpu_debug_excp_handler() for system mode, and right after > cpu_exec() returns in the linux-user cpu loop. > > Tested: the read-only-page reproducer reports rs1 for offsets 1, 65 > and > 129; an mcontrol store-trigger reproducer reports the follow-up > load's > own access-fault address instead of the earlier cbo.zero rs1. Please add the test as a unit test as part of the patch or the series. > > Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/4151 > Suggested-by: David Harris <[email protected]> > Signed-off-by: wangyang <[email protected]> > --- > accel/tcg/user-exec.c | 7 ++++++- > include/hw/core/cpu.h | 2 ++ > linux-user/riscv/cpu_loop.c | 3 +++ > target/riscv/tcg/cpu_helper.c | 27 ++++++++++++++++++++++++--- > target/riscv/tcg/debug.c | 3 +++ > target/riscv/tcg/op_helper.c | 5 +++++ > 6 files changed, 43 insertions(+), 4 deletions(-) > > diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c > index a35aca7889..13c3845811 100644 > --- a/accel/tcg/user-exec.c > +++ b/accel/tcg/user-exec.c > @@ -787,7 +787,12 @@ static int probe_access_internal(CPUArchState > *env, vaddr addr, > return TLB_INVALID_MASK; > } > > - cpu_loop_exit_sigsegv(env_cpu(env), addr, access_type, maperr, > ra); > + CPUState *cpu = env_cpu(env); > + if (cpu->exception_addr_valid) { > + addr = cpu->exception_addr; > + cpu->exception_addr_valid = false; > + } > + cpu_loop_exit_sigsegv(cpu, addr, access_type, maperr, ra); > } > > int probe_access_flags(CPUArchState *env, vaddr addr, int size, > diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h > index ae66f5e33e..1fd68367f6 100644 > --- a/include/hw/core/cpu.h > +++ b/include/hw/core/cpu.h > @@ -560,6 +560,8 @@ struct CPUState { > uint32_t tcg_cflags; > uint32_t halted; > int32_t exception_index; > + vaddr exception_addr; > + bool exception_addr_valid; This seems overly complex for one instruction > > bool vcpu_dirty; > AccelCPUState *accel; > diff --git a/linux-user/riscv/cpu_loop.c b/linux- > user/riscv/cpu_loop.c > index 6f27a06da4..dbf1d42d33 100644 > --- a/linux-user/riscv/cpu_loop.c > +++ b/linux-user/riscv/cpu_loop.c > @@ -38,6 +38,9 @@ void cpu_loop(CPURISCVState *env) > cpu_exec_end(cs); > qemu_process_cpu_events(cs); > > + /* A trap ends the pending cbo.zero address override window. > */ > + cs->exception_addr_valid = false; > + > switch (trapnr) { > case EXCP_INTERRUPT: > /* just indicate that signals should be handled asap */ > diff --git a/target/riscv/tcg/cpu_helper.c > b/target/riscv/tcg/cpu_helper.c > index 07d9222652..4513c9bdbe 100644 > --- a/target/riscv/tcg/cpu_helper.c > +++ b/target/riscv/tcg/cpu_helper.c > @@ -1537,7 +1537,12 @@ static void raise_mmu_exception(CPURISCVState > *env, target_ulong address, > default: > g_assert_not_reached(); > } > - env->badaddr = address; > + if (cs->exception_addr_valid) { > + env->badaddr = cs->exception_addr; > + cs->exception_addr_valid = false; > + } else { > + env->badaddr = address; > + } > env->two_stage_lookup = two_stage; > env->two_stage_indirect_lookup = two_stage_indirect; > } > @@ -1588,7 +1593,12 @@ void riscv_cpu_do_transaction_failed(CPUState > *cs, hwaddr physaddr, > cs->exception_index = RISCV_EXCP_INST_ACCESS_FAULT; > } > > - env->badaddr = addr; > + if (cs->exception_addr_valid) { > + env->badaddr = cs->exception_addr; > + cs->exception_addr_valid = false; > + } else { > + env->badaddr = addr; > + } > env->two_stage_lookup = mmuidx_2stage(mmu_idx); > env->two_stage_indirect_lookup = false; > cpu_loop_exit_restore(cs, retaddr); > @@ -1621,7 +1631,12 @@ void riscv_cpu_do_unaligned_access(CPUState > *cs, vaddr addr, > default: > g_assert_not_reached(); > } > - env->badaddr = addr; > + if (cs->exception_addr_valid) { > + env->badaddr = cs->exception_addr; > + cs->exception_addr_valid = false; > + } else { > + env->badaddr = addr; > + } > env->two_stage_lookup = mmuidx_2stage(mmu_idx); > env->two_stage_indirect_lookup = false; > cpu_loop_exit_restore(cs, retaddr); > @@ -2047,6 +2062,12 @@ void riscv_cpu_do_interrupt(CPUState *cs) > { > RISCVCPU *cpu = RISCV_CPU(cs); > CPURISCVState *env = &cpu->env; > + > + /* > + * A trap ends the pending cbo.zero address override window, so > drop > + * any saved rs1 that a store trigger or breakpoint left behind. > + */ > + cs->exception_addr_valid = false; > bool virt = env->virt_enabled; > bool write_gva = false; > bool always_storeamo = (env->excp_uw2 & > RISCV_UW2_ALWAYS_STORE_AMO); > diff --git a/target/riscv/tcg/debug.c b/target/riscv/tcg/debug.c > index e4d719a1da..6962557251 100644 > --- a/target/riscv/tcg/debug.c > +++ b/target/riscv/tcg/debug.c > @@ -973,6 +973,9 @@ void riscv_cpu_debug_excp_handler(CPUState *cs) > RISCVCPU *cpu = RISCV_CPU(cs); > CPURISCVState *env = &cpu->env; > > + /* A debug exit ends the cbo.zero address override window too. > */ > + cs->exception_addr_valid = false; > + > if (cs->watchpoint_hit) { > if (cs->watchpoint_hit->flags & BP_CPU) { > do_trigger_action(env, DBG_ACTION_BP); > diff --git a/target/riscv/tcg/op_helper.c > b/target/riscv/tcg/op_helper.c > index b67c7718db..3fc993a207 100644 > --- a/target/riscv/tcg/op_helper.c > +++ b/target/riscv/tcg/op_helper.c > @@ -188,9 +188,13 @@ void helper_cbo_zero(CPURISCVState *env, > target_ulong address) > int mmu_idx = riscv_env_mmu_index(env, false); > uintptr_t ra = GETPC(); > void *mem; > + CPUState *cs = env_cpu(env); > > check_zicbo_envcfg(env, MENVCFG_CBZE, ra); > > + cs->exception_addr = address; > + cs->exception_addr_valid = true; Can't we just try a `probe_write() using the rs1 address first? That will check the address and if it faults report rs1. Obviously that falls apart if the fault is not rs1, but somewhere else in the cacheblock, but does that actually happen? Adding your tests as part of this patch would help figure that out. Alistair > + > /* Mask off low-bits to align-down to the cache-block. */ > address &= ~(cbozlen - 1); > > @@ -218,6 +222,7 @@ void helper_cbo_zero(CPURISCVState *env, > target_ulong address) > cpu_stb_mmuidx_ra(env, address + i, 0, mmu_idx, ra); > } > } > + cs->exception_addr_valid = false; > } > > /*
