Re: [RFC PATCH 24/35] target/arm: remove event_register check from arm_cpu_has_work

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

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

> Peter Maydell <peter.maydell@linaro.org> writes:
>
>> On Fri, 20 Mar 2026 at 13:16, Alex Bennée <alex.bennee@linaro.org> wrote:
>>>
>>> The generation of events are not indications of work to be done and
>>> importantly for the case of WFI instructions not a reason to wake up.
>>> Remove the check.
>>
>> But an event *is* a reason for WFE to wake up, which is why this
>> check is here. In particular, the thing that tells accel/tcg
>> that the CPU should come out of halt (via cpu_handle_halt())
>> is whether cpu_exec_halt / cpu_has_work returns true. If we
>> don't check the event register here then another CPU setting
>> the event register won't cause this one to come out of WFE.
>
> So for the same-CPU case we do because we signal cpu_interrupt(cs,
> CPU_INTERRUPT_EXITTB) which is enough for has_work to return true but I
> can see the failure mode for cross-cpu SEV's when we kick all vCPUs in
> the helper would get stuck.
>
>> If we want to have WFE and WFI wakeup events to be separated
>> such that only WFE wakeup events resume a WFE and only
>> WFI wakeup events resume a WFI then we need to do something
>> more clever here, so that we track whether we're halted for
>> WFI, WFE, PSCI-power-off or whatever and our has_work function
>> can check the right condition.
>
> I guess we can add a cpu->halt_reason? I've already had to add a
> cpu->waiting_for_event for arm_wfxt_timer_cb and I guess it would be
> better to generalise that.
>
>> (Side question: are the only
>> cases where our arm CPUs halt WFE, WFI, power-off, or are
>> there any others I forgot about?)
>
> The only others I can find are debug events which all have their own
> logic (and we don't have "external" debug events modelled). YIELD is
> still a NOP and not really a sleep.
>
>> Incidentally, looking at the code above it will need fixing
>> for A-profile anyway, because a CPU in PSCI-power-off should
>> not be woken up by a WFE event. We get away with doing the
>> WFE event register check first because M-profile doesn't use
>> the PSCI powerdown stuff.
>
> Ok, that seals it.

I'm thinking something like this:

--8<---------------cut here---------------start------------->8---
modified    target/arm/cpu.h
@@ -257,6 +257,19 @@ typedef enum ARMFPStatusFlavour {
 } ARMFPStatusFlavour;
 #define FPST_COUNT  10
 
+/**
+ * ARMHaltReason - the reason we have entered halt state
+ *
+ * To be able to correctly wake up via arm_cpu_has_work() we need to
+ * track the reason we went to sleep.
+ */
+typedef enum {
+    NOT_HALTED = 0,
+    HALT_PSCI,
+    HALT_WFI,
+    HALT_WFE
+} ARMHaltReason;
+
 typedef struct CPUArchState {
     /* Regs for current mode.  */
     uint32_t regs[16];
@@ -760,6 +773,9 @@ typedef struct CPUArchState {
     /* Optional fault info across tlb lookup. */
     ARMMMUFaultInfo *tlb_fi;
 
+    /* Reason the CPU is halted */
+    ARMHaltReason halt_reason;
+
     /*
      * The event register is shared by all ARM profiles (A/R/M),
      * so it is stored in the top-level CPU state.
modified    target/arm/arm-powerctl.c
@@ -78,6 +78,7 @@ static void arm_set_cpu_on_async_work(CPUState *target_cpu_state,
 
     /* Finally set the power status */
     assert(bql_locked());
+    target_cpu->env.halt_reason = NOT_HALTED;
     target_cpu->power_state = PSCI_ON;
 }
 
@@ -240,6 +241,7 @@ static void arm_set_cpu_off_async_work(CPUState *target_cpu_state,
 
     assert(bql_locked());
     target_cpu->power_state = PSCI_OFF;
+    target_cpu->env.halt_reason = HALT_PSCI;
     target_cpu_state->halted = 1;
     target_cpu_state->exception_index = EXCP_HLT;
 }
modified    target/arm/cpu.c
@@ -144,18 +144,36 @@ static bool arm_cpu_has_work(CPUState *cs)
 {
     ARMCPU *cpu = ARM_CPU(cs);
 
-    if (arm_feature(&cpu->env, ARM_FEATURE_M)) {
-        if (cpu->env.event_register) {
-            return true;
-        }
+    /*
+     * Only another PSCI call can wake the CPU up in which case the
+     * power_state would be set by reset.
+     */
+    if (cpu->power_state == PSCI_OFF) {
+        g_assert(cpu->env.halt_reason == HALT_PSCI);
+        return false;
+    }
+
+    /*
+     * A wake-up event should only wake us if we are halted on a WFE
+     */
+    if (cpu->env.halt_reason == HALT_WFE && cpu->env.event_register) {
+        cpu->env.halt_reason = NOT_HALTED;
+        return true;
+    }
+
+    /*
+     * Otherwise pretty much any IRQ would wake us up
+     */
+    if (cpu_test_interrupt(cs,
+                           CPU_INTERRUPT_FIQ | CPU_INTERRUPT_HARD
+                           | CPU_INTERRUPT_NMI | CPU_INTERRUPT_VINMI | CPU_INTERRUPT_VFNMI
+                           | CPU_INTERRUPT_VFIQ | CPU_INTERRUPT_VIRQ | CPU_INTERRUPT_VSERR
+                           | CPU_INTERRUPT_EXITTB)) {
+        cpu->env.halt_reason = NOT_HALTED;
+        return true;
     }
 
-    return (cpu->power_state != PSCI_OFF)
-        && cpu_test_interrupt(cs,
-               CPU_INTERRUPT_FIQ | CPU_INTERRUPT_HARD
-               | CPU_INTERRUPT_NMI | CPU_INTERRUPT_VINMI | CPU_INTERRUPT_VFNMI
-               | CPU_INTERRUPT_VFIQ | CPU_INTERRUPT_VIRQ | CPU_INTERRUPT_VSERR
-               | CPU_INTERRUPT_EXITTB);
+    return false;
 }
 #endif /* !CONFIG_USER_ONLY */
 
modified    target/arm/tcg/op_helper.c
@@ -403,6 +403,7 @@ void HELPER(wfi)(CPUARMState *env, uint32_t insn_len)
                         target_el);
     }
 
+    env->halt_reason = HALT_WFI;
     cs->exception_index = EXCP_HLT;
     cs->halted = 1;
     cpu_loop_exit(cs);
@@ -464,6 +465,7 @@ void HELPER(wfit)(CPUARMState *env, uint32_t rd)
     } else {
         timer_mod(cpu->wfxt_timer, nexttick);
     }
+    env->halt_reason = HALT_WFI;
     cs->exception_index = EXCP_HLT;
     cs->halted = 1;
     cpu_loop_exit(cs);
@@ -508,6 +510,7 @@ void HELPER(wfe)(CPUARMState *env)
             return;
         }
 
+        env->halt_reason = HALT_WFE;
         cs->exception_index = EXCP_HLT;
         cs->halted = 1;
         cpu_loop_exit(cs);
--8<---------------cut here---------------end--------------->8---


>
>>
>> thanks
>> -- PMM

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro