Re: [PATCH 2/8] accel/hvf: check exit_request before running the vCPU

[Paolo Bonzini <pbonzini@redhat.com>]

On 8/10/25 01:09, Richard Henderson wrote:
> On 8/9/25 04:58, Paolo Bonzini wrote:
>> This is done by all other accelerators, but was missing for
>> Hypervisor.framework.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>   target/arm/hvf/hvf.c  | 4 ++++
>>   target/i386/hvf/hvf.c | 4 ++++
>>   2 files changed, 8 insertions(+)
>>
>> diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c
>> index b77db99079e..478bc75fee6 100644
>> --- a/target/arm/hvf/hvf.c
>> +++ b/target/arm/hvf/hvf.c
>> @@ -1927,6 +1927,10 @@ int hvf_vcpu_exec(CPUState *cpu)
>>       flush_cpu_state(cpu);
>>       bql_unlock();
>> +    /* Corresponding store-release is in cpu_exit. */
>> +    if (qatomic_load_acquire(&cpu->exit_request)) {
>> +        hv_vcpus_exit(&cpu->accel->fd, 1);
>> +    }
>>       r = hv_vcpu_run(cpu->accel->fd);
> 
> This looks odd.
> 
> hv_vcpus_exit: "Forces an immediate exit of a set of vCPUs of the VM".
> 
> But we know for a fact fd isn't running, because that's to start in the 
> next line.  I suppose this must set a flag so that the kernel's 
> hv_vcpu_run exits immediately with CANCELED?
> 
> Does executing of hv_vcpu_run buy us anything else?  Is it less 
> confusing to simply return 0, modulo bql fiddling?
> 
>> diff --git a/target/i386/hvf/hvf.c b/target/i386/hvf/hvf.c
>> index 8445cadecec..b7c4b849cdf 100644
>> --- a/target/i386/hvf/hvf.c
>> +++ b/target/i386/hvf/hvf.c
>> @@ -749,6 +749,10 @@ int hvf_vcpu_exec(CPUState *cpu)
>>               return EXCP_HLT;
>>           }
>> +        /* Corresponding store-release is in cpu_exit. */
>> +        if (qatomic_load_acquire(&cpu->exit_request)) {
>> +            hv_vcpu_interrupt(&cpu->accel->fd, 1);
>> +        }
>>           hv_return_t r = hv_vcpu_run_until(cpu->accel->fd, 
>> HV_DEADLINE_FOREVER);
>>           assert_hvf_ok(r);
> 
> This, similarly, I guess returns EXCP_INTERRUPT.  Is that better or 
> worse than 0?

The advantage is that you can reuse the code for when another thread 
kicks you out of the execution loop.  For x86, for example, the effects 
of hvf_inject_interrupts() are undone by hv_vcpu_run() + hvf_store_events().

But on second thought this patch is not needed.  The kick is handled 
completely by Apple code and that's where any synchronization must 
happen.  In fact, there should be no need for hvf_kick_vcpu_thread() to 
call cpus_kick_thread() even (though I'd leave it to more HVF-literate 
people like Phil to do that).

Paolo