Paolo Bonzini <[email protected]> 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 <[email protected]>
>
> 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
