Re: [PATCH 06/11] target/arm: Extract halt-to-exec transition out of arm_cpu_exec_halt()

["Philippe Mathieu-Daudé" <philmd@oss.qualcomm.com>]

On 20/8/26 09:29, Philippe Mathieu-Daudé wrote:
> On 2026-08-19 23:40, Peter Maydell wrote:
>> On Wed, 19 Aug 2026 at 21:14, Richard Henderson
>> <richard.henderson@linaro.org> wrote:
>>>
>>> On 8/19/26 07:56, Philippe Mathieu-Daudé wrote:
>>>> Refactor the target-specific halt-to-execution transition
>>>> logic (disable WFE/WFI timers, clear the halt_reason flag)
>>>> into a separate arm_cpu_transition_halt_to_exec() function.
>>>>
>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@oss.qualcomm.com>
>>>> ---
>>>>    target/arm/cpu.c | 22 +++++++++++++++-------
>>>>    1 file changed, 15 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
>>>> index 77aa78f00e2..8e9b584559d 100644
>>>> --- a/target/arm/cpu.c
>>>> +++ b/target/arm/cpu.c
>>>> @@ -870,18 +870,26 @@ static bool 
>>>> arm_cpu_internal_is_big_endian(CPUState *cs)
>>>>    }
>>>>
>>>>    #ifdef CONFIG_TCG
>>>> +static void arm_cpu_transition_halt_to_exec(CPUState *cs)
>>>> +{
>>>> +    ARMCPU *cpu = ARM_CPU(cs);
>>>> +
>>>> +    assert(cpu_has_work(cs));
>>>
>>> In reply to patch 11 you suggest removing this.
>>> I wonder why you added it in the first place.
>>
>> Does the accel loop enforce that we hold the BQL when
>> arm_cpu_exec_halt() is called?
> 
> For MTTCG:
> 
> mttcg_cpu_thread_fn()
> {
>    bql_lock();
>    while (1)
>      qemu_process_cpu_events(cpu);
>      if (cpu_can_run(cpu)) {
>        bql_unlock();
>        tcg_cpu_exec(cpu);
>          cpu_exec(cpu);
>            if (cpu->halted) {
>              cpu_has_work_after_processing_async_events(cpu);
>                process_async_events(cpu);
>                  x86_cpu_process_async_events(); // APIC takes BQL
>                cpu_has_work(cpu);
>                transition_halt_to_exec(cpu);
>                  arm_cpu_transition_halt_to_exec(); // timer_lock
>                  x86_cpu_transition_halt_to_exec();
>          ...
>        bql_lock();
>        ...
> }
> 
> void x86_cpu_process_async_events()
> {
>      /* (BQL not held) */
>      if (cpu_test_interrupt()) {
>          bql_lock();
>          apic_poll_irq();
>            apic_sync_vapic();
>            apic_update_irq();
>              cpu_interrupt();
>          cpu_reset_interrupt(cpu, CPU_INTERRUPT_POLL);
>          bql_unlock();
>      }
> }


>> If not then the assertion
>> is racy, because something might get in and e.g. lower
>> an IRQ line so that cpu_has_work() is no longer true
>> (which would be fine -- it just means we made the decision
>> to wake up and that won vs the incoming interrupt).
>>
>> On the one hand, if we don't have the BQL in has_work and
>> exec_halt then we should be a lot more careful about how
>> we code them (e.g. use of the right kind of atomics, and
>> there's no way the call to do_interrupt_all() in the x86
>> exec_halt can be safe without the BQL, surely).
>>
>> On the other hand, if we do have the BQL in exec_halt
>> then why is the x86 implementation explicitly taking
>> the BQL when it calls apic_poll_irq()?

Trying harder, my understanding is apic_poll_irq() only requires
the BQL for the apic_update_irq() call, not apic_sync_vapic().

So maybe only apic_sync_vapic() belongs to process_async_events(),
leaving apic_update_irq() to transition_halt_to_exec().

Paolo does that sound like going in the good direction?