[RFC PATCH] target/arm: hold bql when SEV sets the event_register

[RFC PATCH] target/arm: hold bql when SEV sets the event_register

[Alex Bennée]

There is a race in qemu_process_cpu_events and sleeping on halt_cond
where we can miss the setting of the event_register which would cause
arm_cpu_has_work to report we shouldn't sleep. As by definition the
bql has to be held when we go to sleep against halt_cond guard the
kicking of the cpus by the SEV helper with the BQL to ensure things
are properly serialised.

The other places that set event_register are fine as they are either
in the same vCPU context or are already serialised by the BQL.

Also use qemu_cond_wait_bql like the other halt_cond users.

Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/4660
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 system/cpus.c              | 2 +-
 target/arm/tcg/op_helper.c | 3 +++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/system/cpus.c b/system/cpus.c
index e11a5aab6a6..985732217fb 100644
--- a/system/cpus.c
+++ b/system/cpus.c
@@ -412,7 +412,7 @@ void qemu_process_cpu_events(CPUState *cpu)
             slept = true;
             qemu_plugin_vcpu_idle_cb(cpu);
         }
-        qemu_cond_wait(cpu->halt_cond, &bql);
+        qemu_cond_wait_bql(cpu->halt_cond);
     }
     if (slept) {
         qemu_plugin_vcpu_resume_cb(cpu);
diff --git a/target/arm/tcg/op_helper.c b/target/arm/tcg/op_helper.c
index af56eff9f18..7c1fdab79d7 100644
--- a/target/arm/tcg/op_helper.c
+++ b/target/arm/tcg/op_helper.c
@@ -471,6 +471,9 @@ void HELPER(wfit)(CPUARMState *env, uint32_t rd)
 void HELPER(sev)(CPUARMState *env)
 {
     CPUState *cs = env_cpu(env);
+
+    BQL_LOCK_GUARD();
+
     CPU_FOREACH(cs) {
         ARMCPU *target_cpu = ARM_CPU(cs);
         target_cpu->env.event_register = true;
-- 
2.47.3

Re: [RFC PATCH] target/arm: hold bql when SEV sets the event_register

[Alex Bennée]

Alex Bennée <alex.bennee@linaro.org> writes:

> There is a race in qemu_process_cpu_events and sleeping on halt_cond
> where we can miss the setting of the event_register which would cause
> arm_cpu_has_work to report we shouldn't sleep. As by definition the
> bql has to be held when we go to sleep against halt_cond guard the
> kicking of the cpus by the SEV helper with the BQL to ensure things
> are properly serialised.
>
> The other places that set event_register are fine as they are either
> in the same vCPU context or are already serialised by the BQL.
>
> Also use qemu_cond_wait_bql like the other halt_cond users.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/4660

This also fixes 4670 

> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  system/cpus.c              | 2 +-
>  target/arm/tcg/op_helper.c | 3 +++
>  2 files changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/system/cpus.c b/system/cpus.c
> index e11a5aab6a6..985732217fb 100644
> --- a/system/cpus.c
> +++ b/system/cpus.c
> @@ -412,7 +412,7 @@ void qemu_process_cpu_events(CPUState *cpu)
>              slept = true;
>              qemu_plugin_vcpu_idle_cb(cpu);
>          }
> -        qemu_cond_wait(cpu->halt_cond, &bql);
> +        qemu_cond_wait_bql(cpu->halt_cond);
>      }
>      if (slept) {
>          qemu_plugin_vcpu_resume_cb(cpu);
> diff --git a/target/arm/tcg/op_helper.c b/target/arm/tcg/op_helper.c
> index af56eff9f18..7c1fdab79d7 100644
> --- a/target/arm/tcg/op_helper.c
> +++ b/target/arm/tcg/op_helper.c
> @@ -471,6 +471,9 @@ void HELPER(wfit)(CPUARMState *env, uint32_t rd)
>  void HELPER(sev)(CPUARMState *env)
>  {
>      CPUState *cs = env_cpu(env);
> +
> +    BQL_LOCK_GUARD();
> +
>      CPU_FOREACH(cs) {
>          ARMCPU *target_cpu = ARM_CPU(cs);
>          target_cpu->env.event_register = true;

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [RFC PATCH] target/arm: hold bql when SEV sets the event_register

[Paolo Bonzini]

On 10/12/26 16:52, Alex Bennée wrote:
>> diff --git a/target/arm/tcg/op_helper.c b/target/arm/tcg/op_helper.c
>> index af56eff9f18..7c1fdab79d7 100644
>> --- a/target/arm/tcg/op_helper.c
>> +++ b/target/arm/tcg/op_helper.c
>> @@ -471,6 +471,9 @@ void HELPER(wfit)(CPUARMState *env, uint32_t rd)
>>   void HELPER(sev)(CPUARMState *env)
>>   {
>>       CPUState *cs = env_cpu(env);
>> +
>> +    BQL_LOCK_GUARD();
>> +
>>       CPU_FOREACH(cs) {
>>           ARMCPU *target_cpu = ARM_CPU(cs);
>>           target_cpu->env.event_register = true;
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

The use of cmpxchg in wfxt_timer_cb is fishy though.

Clearing HALT_WFE should be the task of the target CPU, not the timer. 
The timer_mod() should be *after* halt_reason(*), and then you can just 
do a qatomic_read() instead of a cmpxchg(**) in wfxt_timer_cb().

However, I don't understand at all why wfxt_timer_cb needs to set 
event_register to true.  It seems to me that a wfet + wfe sequence, 
where the first wfet times out, *should* halt again on the second wfe; 
but as the code is written it doesn't.

I think wfxt_timer_cb only needs to call cpu_interrupt().

Thanks,

Paolo

(*) always, even if the halt reason is not HALT_WFE, if only for consistency

(**) you don't need a lock or acquire/release here, because the timer 
callback always sees everything that happened before timer_mod().  In a 
sense, timer_mod() *is* the release operation and starting the callback 
is the acquire operation.

Re: [RFC PATCH] target/arm: hold bql when SEV sets the event_register

[Alex Bennée]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 10/12/26 16:52, Alex Bennée wrote:
>>> diff --git a/target/arm/tcg/op_helper.c b/target/arm/tcg/op_helper.c
>>> index af56eff9f18..7c1fdab79d7 100644
>>> --- a/target/arm/tcg/op_helper.c
>>> +++ b/target/arm/tcg/op_helper.c
>>> @@ -471,6 +471,9 @@ void HELPER(wfit)(CPUARMState *env, uint32_t rd)
>>>   void HELPER(sev)(CPUARMState *env)
>>>   {
>>>       CPUState *cs = env_cpu(env);
>>> +
>>> +    BQL_LOCK_GUARD();
>>> +
>>>       CPU_FOREACH(cs) {
>>>           ARMCPU *target_cpu = ARM_CPU(cs);
>>>           target_cpu->env.event_register = true;
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
>
> The use of cmpxchg in wfxt_timer_cb is fishy though.
>
> Clearing HALT_WFE should be the task of the target CPU, not the timer.
> The timer_mod() should be *after* halt_reason(*), and then you can
> just do a qatomic_read() instead of a cmpxchg(**) in wfxt_timer_cb().
>
> However, I don't understand at all why wfxt_timer_cb needs to set
> event_register to true.  It seems to me that a wfet + wfe sequence,
> where the first wfet times out, *should* halt again on the second wfe;
> but as the code is written it doesn't.
>
> I think wfxt_timer_cb only needs to call cpu_interrupt().

Something like:

--8<---------------cut here---------------start------------->8---
modified    target/arm/cpu.c
@@ -877,25 +877,22 @@ bool arm_cpu_exec_halt(CPUState *cs)
     bool leave_halt = cpu_has_work(cs);
 
     if (leave_halt) {
-        /* We're about to come out of WFI/WFE: disable the WFxT timer */
         ARMCPU *cpu = ARM_CPU(cs);
+
+        qatomic_set(&cpu->env.halt_reason, NOT_HALTED);
+
         if (cpu->wfxt_timer) {
             timer_del(cpu->wfxt_timer);
         }
-        /* clear the halt reason */
-        qatomic_set(&cpu->env.halt_reason, NOT_HALTED);
     }
     return leave_halt;
 }
 #endif
 
 /*
- * Unlike almost everything else that messes with the halt_reason and
- * event_register details the timer callbacks are not in the vCPU
- * context.
- *
- * To prevent races we atomically consume a HALT_WFE and set the event
- * register. Either way we trigger the an exit event.
+ * Unlike almost everything else that messes with the halt_reason the
+ * timer callbacks are not in the vCPU context so we must use atomics
+ * to check it.
  */
 static void arm_wfxt_timer_cb(void *opaque)
 {
@@ -903,17 +900,15 @@ static void arm_wfxt_timer_cb(void *opaque)
     CPUState *cs = CPU(cpu);
     CPUARMState *env = &cpu->env;
 
-    if (qatomic_cmpxchg(&env->halt_reason, HALT_WFE, NOT_HALTED)) {
-        qatomic_set(&env->event_register, true);
+    if (qatomic_read(&env->halt_reason) == HALT_WFE) {
+        /*
+         * We expect the CPU to be halted; this will cause arm_cpu_is_work()
+         * to return true (so we will come out of halt even with no other
+         * pending interrupt), and the TCG accelerator's cpu_exec_interrupt()
+         * function auto-clears the CPU_INTERRUPT_EXITTB flag for us.
+         */
+        cpu_interrupt(cs, CPU_INTERRUPT_EXITTB);
     }
-
-    /*
-     * We expect the CPU to be halted; this will cause arm_cpu_is_work()
-     * to return true (so we will come out of halt even with no other
-     * pending interrupt), and the TCG accelerator's cpu_exec_interrupt()
-     * function auto-clears the CPU_INTERRUPT_EXITTB flag for us.
-     */
-    cpu_interrupt(cs, CPU_INTERRUPT_EXITTB);
 }
 #endif
--8<---------------cut here---------------end--------------->8---

?

>
> Thanks,
>
> Paolo
>
> (*) always, even if the halt reason is not HALT_WFE, if only for consistency
>
> (**) you don't need a lock or acquire/release here, because the timer
> callback always sees everything that happened before timer_mod().  In
> a sense, timer_mod() *is* the release operation and starting the
> callback is the acquire operation.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [RFC PATCH] target/arm: hold bql when SEV sets the event_register

[Paolo Bonzini]

[-- Attachment #1: Type: text/plain, Size: 3081 bytes --]

Il lun 12 ott 2026, 20:47 Alex Bennée <alex.bennee@linaro.org> ha scritto:

> Paolo Bonzini <pbonzini@redhat.com> writes:
>
>
> > I think wfxt_timer_cb only needs to call cpu_interrupt().
>
> Something like:
>

No, the cpu_interrupt must be unconditional due to the WFIT instruction.
Your patch puts it under qatomic_read(&env->halt_reason) == HALT_WFE.

>
> +
> +        qatomic_set(&cpu->env.halt_reason, NOT_HALTED);
> +
>          if (cpu->wfxt_timer) {
>              timer_del(cpu->wfxt_timer);
>          }
> -        /* clear the halt reason */
> -        qatomic_set(&cpu->env.halt_reason, NOT_HALTED);
>

I was thinking about the ordering before timer_mod, in the
wfe/wfi/wfet/wfit helpers. However it is not relevant if the timer callback
does not check halt_reason.

Paolo

     }
>      return leave_halt;
>  }
>  #endif
>
>  /*
> - * Unlike almost everything else that messes with the halt_reason and
> - * event_register details the timer callbacks are not in the vCPU
> - * context.
> - *
> - * To prevent races we atomically consume a HALT_WFE and set the event
> - * register. Either way we trigger the an exit event.
> + * Unlike almost everything else that messes with the halt_reason the
> + * timer callbacks are not in the vCPU context so we must use atomics
> + * to check it.
>   */
>  static void arm_wfxt_timer_cb(void *opaque)
>  {
> @@ -903,17 +900,15 @@ static void arm_wfxt_timer_cb(void *opaque)
>      CPUState *cs = CPU(cpu);
>      CPUARMState *env = &cpu->env;
>
> -    if (qatomic_cmpxchg(&env->halt_reason, HALT_WFE, NOT_HALTED)) {
> -        qatomic_set(&env->event_register, true);
> +    if (qatomic_read(&env->halt_reason) == HALT_WFE) {
> +        /*
> +         * We expect the CPU to be halted; this will cause
> arm_cpu_is_work()
> +         * to return true (so we will come out of halt even with no other
> +         * pending interrupt), and the TCG accelerator's
> cpu_exec_interrupt()
> +         * function auto-clears the CPU_INTERRUPT_EXITTB flag for us.
> +         */
> +        cpu_interrupt(cs, CPU_INTERRUPT_EXITTB);
>      }
> -
> -    /*
> -     * We expect the CPU to be halted; this will cause arm_cpu_is_work()
> -     * to return true (so we will come out of halt even with no other
> -     * pending interrupt), and the TCG accelerator's cpu_exec_interrupt()
> -     * function auto-clears the CPU_INTERRUPT_EXITTB flag for us.
> -     */
> -    cpu_interrupt(cs, CPU_INTERRUPT_EXITTB);
>  }
>  #endif
> --8<---------------cut here---------------end--------------->8---
>
> ?
>
> >
> > Thanks,
> >
> > Paolo
> >
> > (*) always, even if the halt reason is not HALT_WFE, if only for
> consistency
> >
> > (**) you don't need a lock or acquire/release here, because the timer
> > callback always sees everything that happened before timer_mod().  In
> > a sense, timer_mod() *is* the release operation and starting the
> > callback is the acquire operation.
>
> --
> Alex Bennée
> Virtualisation Tech Lead @ Linaro
>
>

[-- Attachment #2: Type: text/html, Size: 4419 bytes --]

Re: [RFC PATCH] target/arm: hold bql when SEV sets the event_register

[Peter Maydell]

On Mon, 12 Oct 2026 at 14:48, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> There is a race in qemu_process_cpu_events and sleeping on halt_cond
> where we can miss the setting of the event_register which would cause
> arm_cpu_has_work to report we shouldn't sleep. As by definition the
> bql has to be held when we go to sleep against halt_cond guard the
> kicking of the cpus by the SEV helper with the BQL to ensure things
> are properly serialised.
>
> The other places that set event_register are fine as they are either
> in the same vCPU context or are already serialised by the BQL.
>
> Also use qemu_cond_wait_bql like the other halt_cond users.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/4660
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  system/cpus.c              | 2 +-
>  target/arm/tcg/op_helper.c | 3 +++
>  2 files changed, 4 insertions(+), 1 deletion(-)

This should be

Cc: qemu-stable@nongnu.org
Fixes: d238858bff6 ("target/arm: Implement WFE, SEV and SEVONPEND for Cortex-M")

right?

thanks
-- PMM