Hi,

> 2026年7月20日 17:04,Bibo Mao <[email protected]> 写道:
> 
> 
> 
> On 2026/7/20 下午4:49, Miao Wang wrote:
>> Hi,
>>> 2026年7月20日 16:34,Bibo Mao <[email protected]> 写道:
>>> 
>>> 
>>> 
>>> On 2026/7/16 上午4:48, Miao Wang via B4 Relay wrote:
>>>> From: Miao Wang <[email protected]>
>>>> 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 <[email protected]>
>>>> ---
>>>> 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/[email protected]
>>>> ---
>>>>  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 
>>>> fb03424ffa8cd5e5d250531cc2a36fa789fc25a9..126c3ad276cc44311942542e086bcabe41d91b63
>>>>  100644
>>>> --- a/target/loongarch/cpu.c
>>>> +++ b/target/loongarch/cpu.c
>>>> @@ -57,12 +57,27 @@ 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;
>>>> @@ -71,12 +86,8 @@ 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));
>>> This is a good method with async_run_on_cpu, there is one small issue with 
>>> irq == 0 interrupt injection where -irq/irq are both 0.
>> Since according to Section 7.4.6. of Loongarch Reference Manual Vol1,
>> bit [1:0] stands for the software interrupt SW1 and SW0, I expect that
>> these two irqs should not be manipulated by hardware sources and thus
>> the irq here should be larger than 2. Can you further explain under
>> what condition the irq line 0 and 1 of a cpu core would be connected
>> to external outputs.
> loongarch_cpu_set_irq() is not only for external interrupts, percpu interrupt 
> injection also uses loongarch_cpu_set_irq() such as IRQ_TIMER etc.

The users of loongarch_cpu_set_irq() are limited. The main usage is
as the callback of qdev_init_gpio_in(). Other usage is for the timer,
where the irq number is IRQ_TIMER, i.e. 11.

> With SW1 and SW0, if the CPU set bit0 of estat, it will inject SW0 to this 
> CPU itself. And de-assert the irq if the CPU clear bit0 of estat.

When SW1 and/or SW0 are set by the guest code running in the emulated
CPU, the handling of the software IRQs in the qemu side does not
involve loongarch_cpu_set_irq(). As a result, I believe that it is
safe to assume that the second parameter of loongarch_cpu_set_irq(),
i.e. irq should not be smaller than 2. I also suggest enforce this
check at the entrance of loongarch_cpu_set_irq().

Cheers,

Miao Wang


Reply via email to