[PATCH v3] target/loongarch: Fix data race in CSR_ESTAT

Bibo Mao posted 1 patch 3 weeks, 4 days ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20260901034833.2215072-1-maobibo@loongson.cn
Maintainers: Song Gao <17746591750@163.com>, Bibo Mao <maobibo@loongson.cn>, Xianglai Li <lixianglai@loongson.cn>
target/loongarch/cpu.c | 25 +++++++++++++++++++------
1 file changed, 19 insertions(+), 6 deletions(-)
[PATCH v3] target/loongarch: Fix data race in CSR_ESTAT
Posted by Bibo Mao 3 weeks, 4 days ago
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:

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().

Signed-off-by: Miao Wang <shankerwangmiao@gmail.com>
Signed-off-by: Bibo Mao <maobibo@loongson.cn>
---
Changes in v3:
  1. Combine irq and level into 32 bit int type.
  2. Rename do_set_cpu_estat() with loongarch_cpu_self_set_irq().

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.
---
 target/loongarch/cpu.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/target/loongarch/cpu.c b/target/loongarch/cpu.c
index 84a130956d..3bfc9a8034 100644
--- a/target/loongarch/cpu.c
+++ b/target/loongarch/cpu.c
@@ -75,13 +75,26 @@ void loongarch_cpu_update_irq(LoongArchCPU *cpu, uint64_t old)
     }
 }
 
-void loongarch_cpu_set_irq(void *opaque, int irq, int level)
+static void loongarch_cpu_self_set_irq(CPUState *cs, run_on_cpu_data data)
 {
-    LoongArchCPU *cpu = opaque;
-    CPULoongArchState *env = &cpu->env;
+    LoongArchCPU *cpu = LOONGARCH_CPU(cs);
+    CPULoongArchState *env = cpu_env(cs);
     CPUSysState *sys = env_sys(env);
+    int irq, level;
     uint64_t old;
 
+    irq = data.host_int & ~BIT(31);
+    level = (data.host_int >> 31) & 1;
+    old = sys->CSR_ESTAT;
+    sys->CSR_ESTAT = deposit64(sys->CSR_ESTAT, irq, 1, level != 0);
+    loongarch_cpu_update_irq(cpu, old);
+}
+
+void loongarch_cpu_set_irq(void *opaque, int irq, int level)
+{
+    LoongArchCPU *cpu = opaque;
+    CPUState *cs = CPU(cpu);
+
     if (irq < 0 || irq >= N_IRQS) {
         return;
     }
@@ -89,9 +102,9 @@ 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()) {
-        old = sys->CSR_ESTAT;
-        sys->CSR_ESTAT = deposit64(sys->CSR_ESTAT, irq, 1, level != 0);
-        loongarch_cpu_update_irq(cpu, old);
+        irq |= (level & 1) << 31;
+        async_run_on_cpu(cs, loongarch_cpu_self_set_irq,
+                         RUN_ON_CPU_HOST_INT(irq));
     }
 }
 

base-commit: d2e570cc0f97b936902a5b1b86b73c0f5998b475
-- 
2.54.0
Re: [PATCH v3] target/loongarch: Fix data race in CSR_ESTAT
Posted by lixianglai 1 week, 3 days ago
> 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:
>
> 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().
>
> Signed-off-by: Miao Wang <shankerwangmiao@gmail.com>
> Signed-off-by: Bibo Mao <maobibo@loongson.cn>
> ---
> Changes in v3:
>    1. Combine irq and level into 32 bit int type.
>    2. Rename do_set_cpu_estat() with loongarch_cpu_self_set_irq().
>
> 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.
> ---
>   target/loongarch/cpu.c | 25 +++++++++++++++++++------
>   1 file changed, 19 insertions(+), 6 deletions(-)
>
> diff --git a/target/loongarch/cpu.c b/target/loongarch/cpu.c
> index 84a130956d..3bfc9a8034 100644
> --- a/target/loongarch/cpu.c
> +++ b/target/loongarch/cpu.c
> @@ -75,13 +75,26 @@ void loongarch_cpu_update_irq(LoongArchCPU *cpu, uint64_t old)
>       }
>   }
>   
> -void loongarch_cpu_set_irq(void *opaque, int irq, int level)
> +static void loongarch_cpu_self_set_irq(CPUState *cs, run_on_cpu_data data)
>   {
> -    LoongArchCPU *cpu = opaque;
> -    CPULoongArchState *env = &cpu->env;
> +    LoongArchCPU *cpu = LOONGARCH_CPU(cs);
> +    CPULoongArchState *env = cpu_env(cs);
>       CPUSysState *sys = env_sys(env);
> +    int irq, level;
>       uint64_t old;
>   
> +    irq = data.host_int & ~BIT(31);
> +    level = (data.host_int >> 31) & 1;
> +    old = sys->CSR_ESTAT;
> +    sys->CSR_ESTAT = deposit64(sys->CSR_ESTAT, irq, 1, level != 0);
> +    loongarch_cpu_update_irq(cpu, old);
> +}
> +
> +void loongarch_cpu_set_irq(void *opaque, int irq, int level)
> +{
> +    LoongArchCPU *cpu = opaque;
> +    CPUState *cs = CPU(cpu);
> +
>       if (irq < 0 || irq >= N_IRQS) {
>           return;
>       }
> @@ -89,9 +102,9 @@ 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()) {
> -        old = sys->CSR_ESTAT;
> -        sys->CSR_ESTAT = deposit64(sys->CSR_ESTAT, irq, 1, level != 0);
> -        loongarch_cpu_update_irq(cpu, old);
> +        irq |= (level & 1) << 31;
> +        async_run_on_cpu(cs, loongarch_cpu_self_set_irq,
> +                         RUN_ON_CPU_HOST_INT(irq));
>       }
>   }
>   
>
> base-commit: d2e570cc0f97b936902a5b1b86b73c0f5998b475


  Reviewed-by:  Xianglai Li <lixianglai@loongson.cn>