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

["Alex Bennée" <alex.bennee@linaro.org>]

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