From: Miao Wang <shankerwangmiao@gmail.com> The CSR_ESTAT register of a CPU can be read and written by both the CPU thread and other threads (e.g., the interrupt controller thread). Currently the possible readers and writers of CSR_ESTAT are: - Readers - tcg generated by trans_csrrd(, CSR_ESTAT) - loongarch_cpu_has_work() - Writers - tcg generated by trans_csrxchg(, CSR_ESTAT) - helper_csrwr_estat() - helper_csrrd_msgir() - loongarch_cpu_set_irq() - loongarch_cpu_do_interrupt() - loongarch_cpu_exec_interrupt() The access from the CPU thread is not synchronized with the access from other threads, which may lead to data races. To fix this, use atomic operations to read and write CSR_ESTAT. The data race has been identified while running the test cases from dracut, which is using QEMU to boot a LoongArch guest. By running the tests repeatedly (about 30 times) in the following conditions, the guest will hang in the middle of booting: - Host architecture: LoongArch64 or Aarch64 - Guest kernel: 7.1.3+deb14-loong64 - Number of vCPUs: 1 or 2 - CPU Features: max, la464,msgint=off,ptw=off, or la464,msgint=off,ptw=on - Accelerator: tcg When the guest hangs, the guest kernel log shows various errors related to RCU stalls or other Soft Lockup or Hard Lockup issues. When running with 1 vCPU and the guest hangs, the guest kernel directly hangs without any messages and stucks at idle_exit. With this patch, the guest can boot successfully without any hangs during repeated runs of the test cases. Signed-off-by: Miao Wang <shankerwangmiao@gmail.com> --- target/loongarch/cpu.c | 8 +++---- target/loongarch/internals.h | 27 +++++++++++++++++++++ target/loongarch/tcg/csr_helper.c | 28 +++++++++++++++++----- target/loongarch/tcg/helper.h | 2 ++ .../tcg/insn_trans/trans_privileged.c.inc | 13 ++++++++-- target/loongarch/tcg/tcg_cpu.c | 12 ++++------ 6 files changed, 71 insertions(+), 19 deletions(-) diff --git a/target/loongarch/cpu.c b/target/loongarch/cpu.c index XXXXXXX..XXXXXXX 100644 --- a/target/loongarch/cpu.c +++ b/target/loongarch/cpu.c @@ -XXX,XX +XXX,XX @@ void loongarch_cpu_set_irq(void *opaque, int irq, int level) if (kvm_enabled()) { kvm_loongarch_set_interrupt(cpu, irq, level); } else if (tcg_enabled()) { - sys->CSR_ESTAT = deposit64(sys->CSR_ESTAT, irq, 1, level != 0); - if (FIELD_EX64(sys->CSR_ESTAT, CSR_ESTAT, IS)) { + cpu_csr_estat_deposit64(sys, irq, 1, level != 0); + if (FIELD_EX64(cpu_csr_estat_get(sys), CSR_ESTAT, IS)) { cpu_interrupt(cs, CPU_INTERRUPT_HARD); } else { cpu_reset_interrupt(cs, CPU_INTERRUPT_HARD); @@ -XXX,XX +XXX,XX @@ bool cpu_loongarch_hw_interrupts_pending(CPULoongArchState *env) uint32_t status; CPUSysState *sys = env_sys(env); - pending = FIELD_EX64(sys->CSR_ESTAT, CSR_ESTAT, IS); + pending = FIELD_EX64(cpu_csr_estat_get(sys), CSR_ESTAT, IS); status = FIELD_EX64(sys->CSR_ECFG, CSR_ECFG, LIE); return (pending & status) != 0; @@ -XXX,XX +XXX,XX @@ static void loongarch_cpu_reset_hold(Object *obj, ResetType type) sys->CSR_ECFG = FIELD_DP64(sys->CSR_ECFG, CSR_ECFG, VS, 0); sys->CSR_ECFG = FIELD_DP64(sys->CSR_ECFG, CSR_ECFG, LIE, 0); - sys->CSR_ESTAT = sys->CSR_ESTAT & (~MAKE_64BIT_MASK(0, 2)); + qatomic_and(&sys->CSR_ESTAT, ~MAKE_64BIT_MASK(0, 2)); sys->CSR_RVACFG = FIELD_DP64(sys->CSR_RVACFG, CSR_RVACFG, RBITS, 0); sys->CSR_CPUID = cs->cpu_index; sys->CSR_TCFG = FIELD_DP64(sys->CSR_TCFG, CSR_TCFG, EN, 0); diff --git a/target/loongarch/internals.h b/target/loongarch/internals.h index XXXXXXX..XXXXXXX 100644 --- a/target/loongarch/internals.h +++ b/target/loongarch/internals.h @@ -XXX,XX +XXX,XX @@ int ieee_ex_to_loongarch(int xcpt); void restore_fp_status(CPULoongArchState *env); #endif +static inline uint64_t cpu_csr_estat_get(CPUSysState *sys) +{ + return qatomic_read(&sys->CSR_ESTAT); +} + +#define cpu_csr_estat_set(sys, field, val) \ + do { \ + uint64_t _estat = cpu_csr_estat_get(sys); \ + uint64_t _new = FIELD_DP64(_estat, CSR_ESTAT, field, (val)); \ + if (qatomic_cmpxchg(&(sys)->CSR_ESTAT, _estat, _new) == _estat) { \ + break; \ + } \ + } while (1) + +static inline uint64_t cpu_csr_estat_deposit64(CPUSysState *sys, int start, + int len, uint64_t val) +{ + do { + uint64_t _estat = cpu_csr_estat_get(sys); + uint64_t _new = deposit64(_estat, start, len, val); + if (qatomic_cmpxchg(&(sys)->CSR_ESTAT, _estat, _new) == _estat) { + return _estat; + } + } while (1); +} + + #ifndef CONFIG_USER_ONLY extern const VMStateDescription vmstate_loongarch_cpu; diff --git a/target/loongarch/tcg/csr_helper.c b/target/loongarch/tcg/csr_helper.c index XXXXXXX..XXXXXXX 100644 --- a/target/loongarch/tcg/csr_helper.c +++ b/target/loongarch/tcg/csr_helper.c @@ -XXX,XX +XXX,XX @@ target_ulong helper_csrrd_msgir(CPULoongArchState *env) return irq; } - sys->CSR_ESTAT = FIELD_DP64(sys->CSR_ESTAT, CSR_ESTAT, MSGINT, 0); + cpu_csr_estat_set(sys, MSGINT, 0); } else { /* bit 31 set 1 for no invalid irq */ irq = BIT(31); @@ -XXX,XX +XXX,XX @@ target_ulong helper_csrrd_msgir(CPULoongArchState *env) return irq; } -target_ulong helper_csrwr_estat(CPULoongArchState *env, target_ulong val) +target_ulong helper_csrrd_estat(CPULoongArchState *env) { CPUSysState *sys = env_sys(env); - int64_t old_v = sys->CSR_ESTAT; + return cpu_csr_estat_get(sys); +} - /* Only IS[1:0] can be written */ - sys->CSR_ESTAT = deposit64(sys->CSR_ESTAT, 0, 2, val); +target_ulong helper_csrwr_estat(CPULoongArchState *env, target_ulong val) +{ + CPUSysState *sys = env_sys(env); + return cpu_csr_estat_deposit64(sys, 0, 2, val); +} - return old_v; +target_ulong helper_csrxhg_estat(CPULoongArchState *env, target_ulong val, + target_ulong mask) +{ + CPUSysState *sys = env_sys(env); + mask &= MAKE_64BIT_MASK(0, 2); + val &= mask; + do { + uint64_t _estat = cpu_csr_estat_get(sys); + uint64_t _new = (_estat & ~mask) | val; + if (qatomic_cmpxchg(&(sys)->CSR_ESTAT, _estat, _new) == _estat) { + return _estat; + } + } while (1); } target_ulong helper_csrwr_asid(CPULoongArchState *env, target_ulong val) diff --git a/target/loongarch/tcg/helper.h b/target/loongarch/tcg/helper.h index XXXXXXX..XXXXXXX 100644 --- a/target/loongarch/tcg/helper.h +++ b/target/loongarch/tcg/helper.h @@ -XXX,XX +XXX,XX @@ DEF_HELPER_1(csrrd_pgd, i64, env) DEF_HELPER_1(csrrd_cpuid, i64, env) DEF_HELPER_1(csrrd_tval, i64, env) DEF_HELPER_1(csrrd_msgir, i64, env) +DEF_HELPER_1(csrrd_estat, i64, env) DEF_HELPER_2(csrwr_stlbps, i64, env, tl) DEF_HELPER_2(csrwr_estat, i64, env, tl) +DEF_HELPER_3(csrxhg_estat, i64, env, tl, tl) DEF_HELPER_2(csrwr_asid, i64, env, tl) DEF_HELPER_2(csrwr_tcfg, i64, env, tl) DEF_HELPER_2(csrwr_ticlr, i64, env, tl) diff --git a/target/loongarch/tcg/insn_trans/trans_privileged.c.inc b/target/loongarch/tcg/insn_trans/trans_privileged.c.inc index XXXXXXX..XXXXXXX 100644 --- a/target/loongarch/tcg/insn_trans/trans_privileged.c.inc +++ b/target/loongarch/tcg/insn_trans/trans_privileged.c.inc @@ -XXX,XX +XXX,XX @@ static bool set_csr_trans_func(unsigned int csr_num, GenCSRRead readfn, void loongarch_csr_translate_init(void) { SET_CSR_FUNC(STLBPS, NULL, gen_helper_csrwr_stlbps); - SET_CSR_FUNC(ESTAT, NULL, gen_helper_csrwr_estat); + SET_CSR_FUNC(ESTAT, gen_helper_csrrd_estat, gen_helper_csrwr_estat); SET_CSR_FUNC(ASID, NULL, gen_helper_csrwr_asid); SET_CSR_FUNC(PGD, gen_helper_csrrd_pgd, NULL); SET_CSR_FUNC(PWCL, NULL, gen_helper_csrwr_pwcl); @@ -XXX,XX +XXX,XX @@ static bool trans_csrxchg(DisasContext *ctx, arg_csrxchg *a) return false; } - /* So far only readonly csrs have readfn. */ + if (a->csr == LOONGARCH_CSR_ESTAT) { + src1 = gpr_src(ctx, a->rd, EXT_NONE); + mask = gpr_src(ctx, a->rj, EXT_NONE); + oldv = tcg_temp_new(); + gen_helper_csrxhg_estat(oldv, tcg_env, src1, mask); + gen_set_gpr(a->rd, oldv, EXT_NONE); + return true; + } + + /* So far only readonly csrs have readfn, except ESTAT. */ assert(csr->readfn == NULL); src1 = gpr_src(ctx, a->rd, EXT_NONE); diff --git a/target/loongarch/tcg/tcg_cpu.c b/target/loongarch/tcg/tcg_cpu.c index XXXXXXX..XXXXXXX 100644 --- a/target/loongarch/tcg/tcg_cpu.c +++ b/target/loongarch/tcg/tcg_cpu.c @@ -XXX,XX +XXX,XX @@ static void loongarch_cpu_do_interrupt(CPUState *cs) sys->CSR_TLBRERA = FIELD_DP64(sys->CSR_TLBRERA, CSR_TLBRERA, PC, (env->pc >> 2)); } else { - sys->CSR_ESTAT = FIELD_DP64(sys->CSR_ESTAT, CSR_ESTAT, ECODE, - EXCODE_MCODE(cause)); - sys->CSR_ESTAT = FIELD_DP64(sys->CSR_ESTAT, CSR_ESTAT, ESUBCODE, - EXCODE_SUBCODE(cause)); + cpu_csr_estat_set(sys, ECODE, EXCODE_MCODE(cause)); + cpu_csr_estat_set(sys, ESUBCODE, EXCODE_SUBCODE(cause)); sys->CSR_PRMD = FIELD_DP64(sys->CSR_PRMD, CSR_PRMD, PPLV, FIELD_EX64(sys->CSR_CRMD, CSR_CRMD, PLV)); sys->CSR_PRMD = FIELD_DP64(sys->CSR_PRMD, CSR_PRMD, PIE, @@ -XXX,XX +XXX,XX @@ static void loongarch_cpu_do_interrupt(CPUState *cs) if (cs->exception_index == EXCCODE_INT) { /* Interrupt */ uint32_t vector = 0; - uint32_t pending = FIELD_EX64(sys->CSR_ESTAT, CSR_ESTAT, IS); + uint32_t pending = FIELD_EX64(cpu_csr_estat_get(sys), CSR_ESTAT, IS); pending &= FIELD_EX64(sys->CSR_ECFG, CSR_ECFG, LIE); /* Find the highest-priority interrupt. */ @@ -XXX,XX +XXX,XX @@ static void loongarch_cpu_do_interrupt(CPUState *cs) TARGET_FMT_lx "\n", __func__, env->pc, sys->CSR_ERA, cause, sys->CSR_BADV, sys->CSR_DERA, vector, - sys->CSR_ECFG, sys->CSR_ESTAT); + sys->CSR_ECFG, cpu_csr_estat_get(sys)); qemu_plugin_vcpu_interrupt_cb(cs, last_pc); } else { if (tlbfill) { @@ -XXX,XX +XXX,XX @@ static void loongarch_cpu_do_interrupt(CPUState *cs) "BADI " TARGET_FMT_lx " SYS_NUM " TARGET_FMT_lu " cpu %d asid " TARGET_FMT_lx "\n", __func__, env->pc, tlbfill ? sys->CSR_TLBRERA : sys->CSR_ERA, - cause, tlbfill ? "(refill)" : "", sys->CSR_ESTAT, + cause, tlbfill ? "(refill)" : "", cpu_csr_estat_get(sys), sys->CSR_ECFG, tlbfill ? sys->CSR_TLBRBADV : sys->CSR_BADV, sys->CSR_BADI, env->gpr[11], cs->cpu_index, --- base-commit: 499039798cdad7d86b787fec0eaf1da4151c0f05 change-id: 20260707-loong-race-63639e8f5afb Best regards, -- Miao Wang <shankerwangmiao@gmail.com> From: Miao Wang <shankerwangmiao@gmail.com> The CSR_ESTAT register of a CPU can be read and written by both the CPU thread and other threads (e.g., the interrupt controller thread). Currently the possible readers and writers of CSR_ESTAT are: - Readers - tcg generated by trans_csrrd(, CSR_ESTAT) - loongarch_cpu_has_work() - Writers - tcg generated by trans_csrxchg(, CSR_ESTAT) - helper_csrwr_estat() - helper_csrrd_msgir() - loongarch_cpu_set_irq() - loongarch_cpu_do_interrupt() - loongarch_cpu_exec_interrupt() The access from the CPU thread is not synchronized with the access from other threads, which may lead to data races. The above readers and writers shall all run on the corresponding CPU thread except for loongarch_cpu_set_irq(). To fix this, the access to CSR_ESTAT in loongarch_cpu_set_irq() is moved to the CPU thread by using async_run_on_cpu(). The data race has been identified while running the test cases from dracut, which is using QEMU to boot a LoongArch guest. By running the tests repeatedly (about 30 times) in the following conditions, the guest will hang in the middle of booting: - Host architecture: LoongArch64 or Aarch64 - Guest kernel: 7.1.3+deb14-loong64 - Number of vCPUs: 1 or 2 - CPU Features: max, la464,msgint=off,ptw=off, or la464,msgint=off,ptw=on - Accelerator: tcg When the guest hangs, the guest kernel log shows various errors related to RCU stalls or other Soft Lockup or Hard Lockup issues. When running with 1 vCPU and the guest hangs, the guest kernel directly hangs without any messages and stucks at idle_exit. With this patch, the guest can boot successfully without any hangs during repeated runs of the test cases. Signed-off-by: Miao Wang <shankerwangmiao@gmail.com> --- Changes in v2: - Simplify the changes to move the access to CSR_ESTAT from the only unsynchronized loongarch_cpu_set_irq() to the CPU thread using async_run_on_cpu() to avoid the race condition. - Link to v1: https://lore.kernel.org/qemu-devel/20260714-loong-race-v1-1-54111549c95e@gmail.com --- target/loongarch/cpu.c | 27 +++++++++++++++++++-------- 1 file changed, 19 insertions(+), 8 deletions(-) diff --git a/target/loongarch/cpu.c b/target/loongarch/cpu.c index XXXXXXX..XXXXXXX 100644 --- a/target/loongarch/cpu.c +++ b/target/loongarch/cpu.c @@ -XXX,XX +XXX,XX @@ static vaddr loongarch_cpu_get_pc(CPUState *cs) #ifndef CONFIG_USER_ONLY #include "hw/loongarch/virt.h" +static void do_set_cpu_estat(CPUState *cs, run_on_cpu_data data) +{ + CPULoongArchState *env = cpu_env(cs); + CPUSysState *sys = env_sys(env); + + int irq = data.host_int; + int level = irq >= 0 ? 1 : 0; + irq = level ? irq : -irq; + + sys->CSR_ESTAT = deposit64(sys->CSR_ESTAT, irq, 1, level != 0); + if (FIELD_EX64(sys->CSR_ESTAT, CSR_ESTAT, IS)) { + cpu_interrupt(cs, CPU_INTERRUPT_HARD); + } else { + cpu_reset_interrupt(cs, CPU_INTERRUPT_HARD); + } +} + void loongarch_cpu_set_irq(void *opaque, int irq, int level) { LoongArchCPU *cpu = opaque; - CPULoongArchState *env = &cpu->env; CPUState *cs = CPU(cpu); - CPUSysState *sys = env_sys(env); if (irq < 0 || irq >= N_IRQS) { return; @@ -XXX,XX +XXX,XX @@ void loongarch_cpu_set_irq(void *opaque, int irq, int level) if (kvm_enabled()) { kvm_loongarch_set_interrupt(cpu, irq, level); } else if (tcg_enabled()) { - sys->CSR_ESTAT = deposit64(sys->CSR_ESTAT, irq, 1, level != 0); - if (FIELD_EX64(sys->CSR_ESTAT, CSR_ESTAT, IS)) { - cpu_interrupt(cs, CPU_INTERRUPT_HARD); - } else { - cpu_reset_interrupt(cs, CPU_INTERRUPT_HARD); - } + async_run_on_cpu(cs, do_set_cpu_estat, + RUN_ON_CPU_HOST_INT(level ? irq : -irq)); } } --- base-commit: 499039798cdad7d86b787fec0eaf1da4151c0f05 change-id: 20260707-loong-race-63639e8f5afb Best regards, -- Miao Wang <shankerwangmiao@gmail.com>