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
