Re: [PATCH 0/3] hvf x86 correctness and efficiency improvements

[Paolo Bonzini <pbonzini@redhat.com>]

On 9/22/23 16:09, Phil Dennis-Jordan wrote:
> Patch 1 enables the INVTSC CPUID bit when running with hvf. This can 
> enable some optimisations in the guest OS, and I've not found any reason 
> it shouldn't be allowed for hvf based hosts.

It can be enabled, but it should include a migration blocker.  In fact, 
probably HVF itself should include a migration blocker because QEMU 
doesn't support TSC scaling.

> Patch 2 fixes hvf_kick_vcpu_thread so it actually forces a VM exit instead of
> doing nothing. I guess this previously didn't cause any huge issues because
> hvf's hv_vcpu_run() would exit so extremely frequently on its own accord.

No, it's because signal delivery should already kick the vCPU out of 
guest mode.  I guess it does with hv_vcpu_run(), but not with 
hv_vcpu_run_until()?

hv_vcpu_interrupt() is a problematic API, in that it is not clear how it 
handles races with hv_vcpu_run().  In particular, whether this causes an 
immediate vmexit or not:

            thread 1                         thread 2
            -----------------------          -----------------------
                                             <CPU not in guest mode>
            hv_vcpu_interrupt()
                                             hv_vcpu_run() (or run_until)

Not that the current code is any better; there is no guarantee that the 
signal is delivered before hv_vcpu_run() is called.  However, if as you 
said hv_vcpu_run() will exit often on its own accord but 
hv_vcpu_run_until() does not, then this could cause difficult to 
reproduce bugs by switching to the latter.

> The temp variable is needed because the pointer expected by the hv_vcpu_interrupt()
> call doesn't match the fd field's type in the hvf accel's struct AccelCPUState.
> I'm unsure if it would be better to change that struct field to the relevant
> architecture's handle types, hv_vcpuid_t (x86, unsigned int) and hv_vcpu_t
> (aarch64, uint64_t), perhaps via an intermediate typedef?

I think fd and the HVF type should be placed in an anonymous union.

Thanks,

Paolo