Re: [PATCH v8 12/19] hvf: Simplify post reset/init/loadvm hooks

Hey Philippe,

On 30.07.26 18:00, Philippe Mathieu-Daudé wrote:
> Hi Alex,
>
> (old patch committed as bac969ef30e8a8b73acbeb6d68abff6f68b2056c)
>
> On 19/5/21 22:22, Alexander Graf wrote:
>> The hooks we have that call us after reset, init and loadvm really all
>> just want to say "The reference of all register state is in the QEMU
>> vcpu struct, please push it".
>>
>> We already have a working pushing mechanism though called 
>> cpu->vcpu_dirty,
>> so we can just reuse that for all of the above, syncing state 
>> properly the
>> next time we actually execute a vCPU.
>>
>> This fixes PSCI resets on ARM, as they modify CPU state even after the
>> post init call has completed, but before we execute the vCPU again.
>>
>> To also make the scheme work for x86, we have to make sure we don't
>> move stale eflags into our env when the vcpu state is dirty.
>>
>> Signed-off-by: Alexander Graf <[email protected]>
>> Reviewed-by: Roman Bolshakov <[email protected]>
>> Tested-by: Roman Bolshakov <[email protected]>
>> ---
>>   accel/hvf/hvf-accel-ops.c | 27 +++++++--------------------
>>   target/i386/hvf/x86hvf.c  |  5 ++++-
>>   2 files changed, 11 insertions(+), 21 deletions(-)
>>
>> diff --git a/accel/hvf/hvf-accel-ops.c b/accel/hvf/hvf-accel-ops.c
>> index ded918c443..d1691be989 100644
>> --- a/accel/hvf/hvf-accel-ops.c
>> +++ b/accel/hvf/hvf-accel-ops.c
>> @@ -205,39 +205,26 @@ static void hvf_cpu_synchronize_state(CPUState 
>> *cpu)
>>       }
>>   }
>>   -static void do_hvf_cpu_synchronize_post_reset(CPUState *cpu,
>> -                                              run_on_cpu_data arg)
>> +static void do_hvf_cpu_synchronize_set_dirty(CPUState *cpu,
>> +                                             run_on_cpu_data arg)
>>   {
>> -    hvf_put_registers(cpu);
>> -    cpu->vcpu_dirty = false;
>> +    /* QEMU state is the reference, push it to HVF now and on next 
>> entry */
>> +    cpu->vcpu_dirty = true;
>>   }
>>     static void hvf_cpu_synchronize_post_reset(CPUState *cpu)
>>   {
>> -    run_on_cpu(cpu, do_hvf_cpu_synchronize_post_reset, 
>> RUN_ON_CPU_NULL);
>> -}
>> -
>> -static void do_hvf_cpu_synchronize_post_init(CPUState *cpu,
>> -                                             run_on_cpu_data arg)
>> -{
>> -    hvf_put_registers(cpu);
>> -    cpu->vcpu_dirty = false;
>> +    run_on_cpu(cpu, do_hvf_cpu_synchronize_set_dirty, RUN_ON_CPU_NULL);
>>   }
>>     static void hvf_cpu_synchronize_post_init(CPUState *cpu)
>>   {
>> -    run_on_cpu(cpu, do_hvf_cpu_synchronize_post_init, RUN_ON_CPU_NULL);
>
> Since we are not calling hvf_put_registers() anymore, any reason to keep
> using run_on_cpu()? I think it is safe to update the field out of the
> vCPU thread context, it will be consumed by the thread during the next
> inner loop run, no need to schedule to the vCPU work queue. But I'm out
> of my confort zone here so might be wrong.


Your reasoning makes sense. But there could be dragons lurking 
somewhere, like broken "info registers" views, HVF depending on specific 
register contents for paused CPUs, etc.

I don't see a way to reason about this except to empirically try.


Alex