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

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

[Phil Dennis-Jordan]

This is a series of semi-related patches for the x86 macOS Hypervisor.framework
(hvf) accelerator backend. The intention is to make VMs run slightly more
efficiently on macOS host machines. They have been subject to some months of
CI workloads with macOS guest VMs without issues and they seem to give a few
percent performance improvement. (Though this varies greatly with the type of
workload.)

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.

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. 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?

Patch 3, which replaces the call to hv_vcpu_run() with the more modern
hv_vcpu_run_until() for running the guest vCPU. The newer API is available
from macOS 10.15 host systems onwards. This call causes significantly fewer
VM exits, which also means we really need that exit-forcing interrupt from
patch 2. The reduction in VM exits means less overhead from exits and less
contention on the BQL. Using hv_vcpu_run_until() is also a prerequisite for
using certain newer hvf features, though this patchset doesn't use any.

Patches 2 & 3 must therefore be applied in that order, patch 1 is independent.

This work has been sponsored by Sauce Labs Inc.

Phil Dennis-Jordan (3):
  i386: hvf: Adds support for INVTSC cpuid bit
  i386: hvf: In kick_vcpu use hv_vcpu_interrupt to force exit
  i386: hvf: Updates API usage to use modern vCPU run function

 target/i386/hvf/hvf.c       | 26 +++++++++++++++++++++++++-
 target/i386/hvf/x86_cpuid.c |  4 ++++
 2 files changed, 29 insertions(+), 1 deletion(-)

-- 
2.36.1

[PATCH 1/3] i386: hvf: Adds support for INVTSC cpuid bit

[Phil Dennis-Jordan]

This patch adds the INVTSC bit to the Hypervisor.framework accelerator's
CPUID bit passthrough allow-list. Previously, specifying +invtsc in the CPU
configuration would fail with the following warning despite the host CPU
advertising the feature:

qemu-system-x86_64: warning: host doesn't support requested feature:
CPUID.80000007H:EDX.invtsc [bit 8]

x86 macOS itself relies on a fixed rate TSC for its own Mach absolute time
timestamp mechanism, so there's no reason we can't enable this bit for guests.

Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
---
 target/i386/hvf/x86_cpuid.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/target/i386/hvf/x86_cpuid.c b/target/i386/hvf/x86_cpuid.c
index 7323a7a94b..d2721bd2a7 100644
--- a/target/i386/hvf/x86_cpuid.c
+++ b/target/i386/hvf/x86_cpuid.c
@@ -145,6 +145,10 @@ uint32_t hvf_get_supported_cpuid(uint32_t func, uint32_t idx,
                 CPUID_EXT3_3DNOWPREFETCH | CPUID_EXT3_OSVW | CPUID_EXT3_XOP |
                 CPUID_EXT3_FMA4 | CPUID_EXT3_TBM;
         break;
+    case 0x80000007:
+        edx &= CPUID_APM_INVTSC;
+        eax = ebx = ecx = 0;
+        break;
     default:
         return 0;
     }
-- 
2.36.1

[PATCH 2/3] i386: hvf: In kick_vcpu use hv_vcpu_interrupt to force exit

[Phil Dennis-Jordan]

When interrupting a vCPU thread, this patch actually tells the hypervisor to
stop running guest code on that vCPU.

Calling hv_vcpu_interrupt actually forces a vCPU exit, analogously to
hv_vcpus_exit on aarch64.

Previously, hvf_kick_vcpu_thread relied upon hv_vcpu_run returning very
frequently, including many spurious exits, which made it less of a problem that
nothing was actively done to stop the vCPU thread running guest code.
The newer, more efficient hv_vcpu_run_until exits much more rarely, so a true
"kick" is needed.

Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
---
 target/i386/hvf/hvf.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/target/i386/hvf/hvf.c b/target/i386/hvf/hvf.c
index cb2cd0b02f..55bd7d2af8 100644
--- a/target/i386/hvf/hvf.c
+++ b/target/i386/hvf/hvf.c
@@ -209,7 +209,10 @@ static inline bool apic_bus_freq_is_known(CPUX86State *env)
 
 void hvf_kick_vcpu_thread(CPUState *cpu)
 {
+    hv_vcpuid_t hvf_vcpuid;
     cpus_kick_thread(cpu);
+    hvf_vcpuid = cpu->accel->fd;
+    hv_vcpu_interrupt(&hvf_vcpuid, 1);
 }
 
 int hvf_arch_init(void)
-- 
2.36.1

[PATCH 3/3] i386: hvf: Updates API usage to use modern vCPU run function

[Phil Dennis-Jordan]

macOS 10.15 introduced the more efficient hv_vcpu_run_until() function
to supersede hv_vcpu_run(). According to the documentation, there is no
longer any reason to use the latter on modern host OS versions.

Observed behaviour of the newer function is that as documented, it exits
much less frequently - and most of the original function’s exits seem to
have been effectively pointless.

Another reason to use the new function is that it is a prerequisite for
using newer features such as in-kernel APIC support. (Not covered by
this patch.)

This change implements the upgrade by selecting one of three code paths
at compile time: two static code paths for the new and old functions
respectively, when building for targets where the new function is either
not available, or where the built executable won’t run on older
platforms lacking the new function anyway. The third code path selects
dynamically based on runtime detected availability of the weakly-linked
symbol.

Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
---
 target/i386/hvf/hvf.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/target/i386/hvf/hvf.c b/target/i386/hvf/hvf.c
index 55bd7d2af8..e4c785c686 100644
--- a/target/i386/hvf/hvf.c
+++ b/target/i386/hvf/hvf.c
@@ -410,6 +410,27 @@ static void hvf_cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
     }
 }
 
+static hv_return_t hvf_vcpu_run(hv_vcpuid_t vcpu_id)
+{
+    /*
+     * hv_vcpu_run_until is available and recommended from macOS 10.15+.
+     * Test for availability at runtime and fall back to hv_vcpu_run() only
+     * where necessary.
+     */
+#ifndef MAC_OS_X_VERSION_10_15
+    return hv_vcpu_run(vcpu_id);
+#elif MAC_OS_X_VERSION_MIN_REQUIRED >= MAC_OS_X_VERSION_10_15
+    return hv_vcpu_run_until(vcpu_id, HV_DEADLINE_FOREVER);
+#else /* MAC_OS_X_VERSION_MIN_REQUIRED < MAC_OS_X_VERSION_10_15 */
+    /* 10.15 SDK or newer, but could be < 10.15 at runtime */
+    if (__builtin_available(macOS 10.15, *)) {
+        return hv_vcpu_run(vcpu_id);
+    } else {
+        return hv_vcpu_run_until(vcpu_id, HV_DEADLINE_FOREVER);
+    }
+#endif
+}
+
 int hvf_vcpu_exec(CPUState *cpu)
 {
     X86CPU *x86_cpu = X86_CPU(cpu);
@@ -438,7 +459,7 @@ int hvf_vcpu_exec(CPUState *cpu)
             return EXCP_HLT;
         }
 
-        hv_return_t r  = hv_vcpu_run(cpu->accel->fd);
+        hv_return_t r = hvf_vcpu_run(cpu->accel->fd);
         assert_hvf_ok(r);
 
         /* handle VMEXIT */
-- 
2.36.1

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

[Phil Dennis-Jordan]

Ping - let me know if there's anything particularly controversial,
unclear, etc. about these patches or if I can do anything to make
reviewing easier.

Thanks!


On Fri, 22 Sept 2023 at 16:09, Phil Dennis-Jordan <phil@philjordan.eu> wrote:
>
> This is a series of semi-related patches for the x86 macOS Hypervisor.framework
> (hvf) accelerator backend. The intention is to make VMs run slightly more
> efficiently on macOS host machines. They have been subject to some months of
> CI workloads with macOS guest VMs without issues and they seem to give a few
> percent performance improvement. (Though this varies greatly with the type of
> workload.)
>
> 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.
>
> 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. 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?
>
> Patch 3, which replaces the call to hv_vcpu_run() with the more modern
> hv_vcpu_run_until() for running the guest vCPU. The newer API is available
> from macOS 10.15 host systems onwards. This call causes significantly fewer
> VM exits, which also means we really need that exit-forcing interrupt from
> patch 2. The reduction in VM exits means less overhead from exits and less
> contention on the BQL. Using hv_vcpu_run_until() is also a prerequisite for
> using certain newer hvf features, though this patchset doesn't use any.
>
> Patches 2 & 3 must therefore be applied in that order, patch 1 is independent.
>
> This work has been sponsored by Sauce Labs Inc.
>
> Phil Dennis-Jordan (3):
>   i386: hvf: Adds support for INVTSC cpuid bit
>   i386: hvf: In kick_vcpu use hv_vcpu_interrupt to force exit
>   i386: hvf: Updates API usage to use modern vCPU run function
>
>  target/i386/hvf/hvf.c       | 26 +++++++++++++++++++++++++-
>  target/i386/hvf/x86_cpuid.c |  4 ++++
>  2 files changed, 29 insertions(+), 1 deletion(-)
>
> --
> 2.36.1
>

Re: [PATCH 1/3] i386: hvf: Adds support for INVTSC cpuid bit

[Roman Bolshakov]

On Fri, Sep 22, 2023 at 04:09:12PM +0200, Phil Dennis-Jordan wrote:
> This patch adds the INVTSC bit to the Hypervisor.framework accelerator's
> CPUID bit passthrough allow-list. Previously, specifying +invtsc in the CPU
> configuration would fail with the following warning despite the host CPU
> advertising the feature:
> 
> qemu-system-x86_64: warning: host doesn't support requested feature:
> CPUID.80000007H:EDX.invtsc [bit 8]
> 
> x86 macOS itself relies on a fixed rate TSC for its own Mach absolute time
> timestamp mechanism, so there's no reason we can't enable this bit for guests.
> 

Reviewed-by: Roman Bolshakov <roman@roolebo.dev>
Tested-by: Roman Bolshakov <roman@roolebo.dev>

Regards,
Roman

Re: [PATCH 2/3] i386: hvf: In kick_vcpu use hv_vcpu_interrupt to force exit

[Roman Bolshakov]

On Fri, Sep 22, 2023 at 04:09:13PM +0200, Phil Dennis-Jordan wrote:
> When interrupting a vCPU thread, this patch actually tells the hypervisor to
> stop running guest code on that vCPU.
> 
> Calling hv_vcpu_interrupt actually forces a vCPU exit, analogously to
> hv_vcpus_exit on aarch64.
> 
> Previously, hvf_kick_vcpu_thread relied upon hv_vcpu_run returning very
> frequently, including many spurious exits, which made it less of a problem that
> nothing was actively done to stop the vCPU thread running guest code.
> The newer, more efficient hv_vcpu_run_until exits much more rarely, so a true
> "kick" is needed.
> 

Hi Phil,

I see severe performance regression with the patch on a Windows XP
guest. The display is not refreshed properly like a broken LVDS panel,
only some horizontal lines appear on it. My test laptop for x86 hvf is
MBA 2015 with the latest Big Sur. What are you runing QEMU/HVF on?

FWIW. I recall a few years ago I submitted a similar patch that does
something similar but addresses a few more issues:
https://patchwork.kernel.org/project/qemu-devel/patch/20200729124832.79375-1-r.bolshakov@yadro.com/

I don't remember why it never got merged.

Regards,
Roman

Re: [PATCH 2/3] i386: hvf: In kick_vcpu use hv_vcpu_interrupt to force exit

[Phil Dennis-Jordan]

[-- Attachment #1: Type: text/plain, Size: 2467 bytes --]

On Sun, 8 Oct 2023 at 20:23, Roman Bolshakov <roman@roolebo.dev> wrote:

> On Fri, Sep 22, 2023 at 04:09:13PM +0200, Phil Dennis-Jordan wrote:
> > When interrupting a vCPU thread, this patch actually tells the
> hypervisor to
> > stop running guest code on that vCPU.
> >
> > Calling hv_vcpu_interrupt actually forces a vCPU exit, analogously to
> > hv_vcpus_exit on aarch64.
> >
> > Previously, hvf_kick_vcpu_thread relied upon hv_vcpu_run returning very
> > frequently, including many spurious exits, which made it less of a
> problem that
> > nothing was actively done to stop the vCPU thread running guest code.
> > The newer, more efficient hv_vcpu_run_until exits much more rarely, so a
> true
> > "kick" is needed.
> >
>

Hi Roman,

Thanks for the review and test of this patch and the preceding one!


> I see severe performance regression with the patch on a Windows XP
> guest. The display is not refreshed properly like a broken LVDS panel,
> only some horizontal lines appear on it.


OK, that's interesting - I've been running into that sort of issue while
trying to integrate HVF's APIC implementation into Qemu. I assume that's
with patch 3/3 applied as well? The fact you've repro'd it with just these
patch would explain why I've not been able to fix it on the APIC side…

My test laptop for x86 hvf is
> MBA 2015 with the latest Big Sur. What are you runing QEMU/HVF on?
>

Most of the testing has been with 2018 Mac Mini (Intel Coffee Lake) hosts
running Big Sur (11), Monterey (12), and Ventura (13), and using macOS
guests. I've also sanity-checked with a 2015 MBP (Broadwell) Monterey host
with various macOS guests as well as Linux and FreeBSD guests. Guess I
should have tried Windows guests too, sorry about the regression!


> FWIW. I recall a few years ago I submitted a similar patch that does
> something similar but addresses a few more issues:
>
> https://patchwork.kernel.org/project/qemu-devel/patch/20200729124832.79375-1-r.bolshakov@yadro.com/
>
> I don't remember why it never got merged.
>

Looks like the VM kick might be a more complex undertaking than I was
anticipating. I'll try to repro the problem you ran into, and then look
over your original patch and make sense of it. Hopefully an updated version
of your 'kick' implementation will work well in combination with the
newer hv_vcpu_run_until() API from patch 3/3. I'll keep you posted.

Thanks again,
Phil

[-- Attachment #2: Type: text/html, Size: 3589 bytes --]

Re: [PATCH 2/3] i386: hvf: In kick_vcpu use hv_vcpu_interrupt to force exit

[Roman Bolshakov]

On Sun, Oct 08, 2023 at 08:39:08PM +0200, Phil Dennis-Jordan wrote:
> On Sun, 8 Oct 2023 at 20:23, Roman Bolshakov <roman@roolebo.dev> wrote:
> 
> > On Fri, Sep 22, 2023 at 04:09:13PM +0200, Phil Dennis-Jordan wrote:
> > > When interrupting a vCPU thread, this patch actually tells the
> > hypervisor to
> > > stop running guest code on that vCPU.
> > >
> > > Calling hv_vcpu_interrupt actually forces a vCPU exit, analogously to
> > > hv_vcpus_exit on aarch64.
> > >
> > > Previously, hvf_kick_vcpu_thread relied upon hv_vcpu_run returning very
> > > frequently, including many spurious exits, which made it less of a
> > problem that
> > > nothing was actively done to stop the vCPU thread running guest code.
> > > The newer, more efficient hv_vcpu_run_until exits much more rarely, so a
> > true
> > > "kick" is needed.
> > >
> >
> 
> Hi Roman,
> 
> Thanks for the review and test of this patch and the preceding one!
> 

My pleasure. Thanks for submitting the patch and confirming the need of
the feature on x86 HVF.

> > I see severe performance regression with the patch on a Windows XP
> > guest. The display is not refreshed properly like a broken LVDS panel,
> > only some horizontal lines appear on it.
> 
> 
> I assume that's with patch 3/3 applied as well? The fact you've
> repro'd it with just these patch would explain why I've not been able
> to fix it on the APIC side…
> 

Yes, I applied with patch 3/3 and then retested only with the first two
patches.

> > FWIW. I recall a few years ago I submitted a similar patch that does
> > something similar but addresses a few more issues:
> >
> > https://patchwork.kernel.org/project/qemu-devel/patch/20200729124832.79375-1-r.bolshakov@yadro.com/
> >
> > I don't remember why it never got merged.
> >
> 
> Looks like the VM kick might be a more complex undertaking than I was
> anticipating. I'll try to repro the problem you ran into, and then look
> over your original patch and make sense of it. Hopefully an updated version
> of your 'kick' implementation will work well in combination with the
> newer hv_vcpu_run_until() API from patch 3/3. I'll keep you posted.
> 

Apparently I left a note that some interrupts weren't delivered even
with my patch and I was not able figure out the reason back then. I had
another attempt to debug this two weeks later after I submitted v4 and I
can find a WIP branch on github where I added a Debug Registers support
patch and some tracepoints:

https://github.com/qemu/qemu/compare/master...roolebo:qemu:hvf-debug-kick

Perhaps that's where we should start from besides the obvious need of
rebase.

With regards to hv_vcpu_run_until() I can find the following thread:
https://lists.nongnu.org/archive/html/qemu-devel/2020-06/msg09468.html

> hv_vcpu_run_until() was also evaluated on macOS 10.15.5 but it degrades
> VM performance significantly compared to explicit setting of
> VMX-preepmtion timer value and hv_vcpu_run(). The performance issue was
> observed on Broadwell-based MacBook Air and Ivy Bridge-based MacBook
> Pro.
> 
> macOS 11.0 Beta deprecated hv_vcpu_run() and introduced a special
> declaration for hv_vcpu_run_until(), that's not available 10.15 -
> HV_DEADLINE_FOREVER (UINT64_MAX, which is bigger than maximum value of
> VMX-preeemption counter). Perhaps the performance issue is addressed
> there.

All discussion with Paolo might be helpful, particurlarly:
https://lists.nongnu.org/archive/html/qemu-devel/2020-06/msg09893.html

> So, I've tried Big Sur Beta and it has exactly the same performance
> issue with hv_vcpu_run_until() while hv_vcpu_run() works as good as it
> worked on 10.15.5. I've submitted FB7827341 to Apple wrt the issue.

In November 2020, Apple responded to FB7827341 that there's an issue on
QEMU side.

Regards,
Roman

Re: [PATCH 2/3] i386: hvf: In kick_vcpu use hv_vcpu_interrupt to force exit

[Phil Dennis-Jordan]

[-- Attachment #1: Type: text/plain, Size: 3327 bytes --]

On Sun, 8 Oct 2023 at 21:19, Roman Bolshakov <roman@roolebo.dev> wrote:

> > I assume that's with patch 3/3 applied as well? The fact you've
> > repro'd it with just these patch would explain why I've not been able
> > to fix it on the APIC side…
> >
>
> Yes, I applied with patch 3/3 and then retested only with the first two
> patches.
>

OK, interesting that it would happen without patch 3 as well.


> > > FWIW. I recall a few years ago I submitted a similar patch that does
> > > something similar but addresses a few more issues:
> > >
> > >
> https://patchwork.kernel.org/project/qemu-devel/patch/20200729124832.79375-1-r.bolshakov@yadro.com/
> > >
> > > I don't remember why it never got merged.
> > >
> >
> > Looks like the VM kick might be a more complex undertaking than I was
> > anticipating. I'll try to repro the problem you ran into, and then look
> > over your original patch and make sense of it. Hopefully an updated
> version
> > of your 'kick' implementation will work well in combination with the
> > newer hv_vcpu_run_until() API from patch 3/3. I'll keep you posted.
> >
>
> Apparently I left a note that some interrupts weren't delivered even
> with my patch and I was not able figure out the reason back then. I had
> another attempt to debug this two weeks later after I submitted v4 and I
> can find a WIP branch on github where I added a Debug Registers support
> patch and some tracepoints:
>
> https://github.com/qemu/qemu/compare/master...roolebo:qemu:hvf-debug-kick
>
> Perhaps that's where we should start from besides the obvious need of
> rebase.
>

Sounds good, I'll take a look at those changes and try to work out what to
do next.


> With regards to hv_vcpu_run_until() I can find the following thread:
> https://lists.nongnu.org/archive/html/qemu-devel/2020-06/msg09468.html
>
> > hv_vcpu_run_until() was also evaluated on macOS 10.15.5 but it degrades
> > VM performance significantly compared to explicit setting of
> > VMX-preepmtion timer value and hv_vcpu_run(). The performance issue was
> > observed on Broadwell-based MacBook Air and Ivy Bridge-based MacBook
> > Pro.
> >
> > macOS 11.0 Beta deprecated hv_vcpu_run() and introduced a special
> > declaration for hv_vcpu_run_until(), that's not available 10.15 -
> > HV_DEADLINE_FOREVER (UINT64_MAX, which is bigger than maximum value of
> > VMX-preeemption counter). Perhaps the performance issue is addressed
> > there.
>
> All discussion with Paolo might be helpful, particurlarly:
> https://lists.nongnu.org/archive/html/qemu-devel/2020-06/msg09893.html
>
> > So, I've tried Big Sur Beta and it has exactly the same performance
> > issue with hv_vcpu_run_until() while hv_vcpu_run() works as good as it
> > worked on 10.15.5. I've submitted FB7827341 to Apple wrt the issue.
>
> In November 2020, Apple responded to FB7827341 that there's an issue on
> QEMU side.
>

Hmm, that's interesting. I'll need to work my way through that thread, but
I'll point out that in my testing with SMP guests, I measured a performance
*improvement* with the hv_vcpu_run_until() API (and the forever deadline)
versus hv_vcpu_run(), as it significantly reduced BQL contention - with so
many VMEXITs, vCPU threads were spending a lot of time waiting for the lock.

[-- Attachment #2: Type: text/html, Size: 4807 bytes --]

Re: [PATCH 2/3] i386: hvf: In kick_vcpu use hv_vcpu_interrupt to force exit

[Phil Dennis-Jordan]

Hi Roman,

A quick update on my progress on this so far:

- I've managed to repro the graphical issue; it turns up with the
switch to hv_vcpu_run_until() (patch 3/3). I can't repro with just the
changes to the kick. (patch 2/3; It would have surprised me if that
had caused it.)

- Thanks for linking to the old mailing list threads for background on
the issues.

- I've rebased/ported your more sophisticated vCPU kick implementation
to the latest upstream master branch. This doesn't seem to cause any
obvious regressions, but it also doesn't appear to fix the issue with
hv_vcpu_run_until(). My branch is here,
https://github.com/pmj/qemu/tree/hvf-kick-modern and the updated
version of your patch here:
https://github.com/qemu/qemu/commit/fadc716c5bb15345a49e08eecf7ab1077782975c

- I've done some tracing, and I so far can't spot any undelivered or
slow kicks. Each kick is paired with a return from hvf_vcpu_exec with
reason EXCP_INTERRUPT, and this happens within less than a
millisecond, typically around 200µs. I'm seeing kicks being delivered
at all the various stages we're expecting them apart from VMX
preemption timer exits, at least so far. But those only target a tiny
window and so should be statistically quite rare.

- I'm starting to entertain the idea that hv_vcpu_run_until() exhibits
some other difference compared to hv_vcpu_run() which is causing the
graphical issues, rather than a problem with interrupt delivery.
Unfortunately, I'm not familiar with how drawing works on legacy
(S)VGA graphics, and what effect might cause it to end up with these
dropped scanline updates. I might try to find some kind of OSDev
example code that draws to (S)VGA and hopefully lets me repro and
perhaps debug the problem in isolation.

Any ideas/thoughts?

Thanks,
Phil



On Sun, 8 Oct 2023 at 21:30, Phil Dennis-Jordan <phil@philjordan.eu> wrote:
>
>
> On Sun, 8 Oct 2023 at 21:19, Roman Bolshakov <roman@roolebo.dev> wrote:
>>
>> > I assume that's with patch 3/3 applied as well? The fact you've
>> > repro'd it with just these patch would explain why I've not been able
>> > to fix it on the APIC side…
>> >
>>
>> Yes, I applied with patch 3/3 and then retested only with the first two
>> patches.
>
>
> OK, interesting that it would happen without patch 3 as well.
>
>>
>> > > FWIW. I recall a few years ago I submitted a similar patch that does
>> > > something similar but addresses a few more issues:
>> > >
>> > > https://patchwork.kernel.org/project/qemu-devel/patch/20200729124832.79375-1-r.bolshakov@yadro.com/
>> > >
>> > > I don't remember why it never got merged.
>> > >
>> >
>> > Looks like the VM kick might be a more complex undertaking than I was
>> > anticipating. I'll try to repro the problem you ran into, and then look
>> > over your original patch and make sense of it. Hopefully an updated version
>> > of your 'kick' implementation will work well in combination with the
>> > newer hv_vcpu_run_until() API from patch 3/3. I'll keep you posted.
>> >
>>
>> Apparently I left a note that some interrupts weren't delivered even
>> with my patch and I was not able figure out the reason back then. I had
>> another attempt to debug this two weeks later after I submitted v4 and I
>> can find a WIP branch on github where I added a Debug Registers support
>> patch and some tracepoints:
>>
>> https://github.com/qemu/qemu/compare/master...roolebo:qemu:hvf-debug-kick
>>
>> Perhaps that's where we should start from besides the obvious need of
>> rebase.
>
>
> Sounds good, I'll take a look at those changes and try to work out what to do next.
>
>>
>> With regards to hv_vcpu_run_until() I can find the following thread:
>> https://lists.nongnu.org/archive/html/qemu-devel/2020-06/msg09468.html
>>
>> > hv_vcpu_run_until() was also evaluated on macOS 10.15.5 but it degrades
>> > VM performance significantly compared to explicit setting of
>> > VMX-preepmtion timer value and hv_vcpu_run(). The performance issue was
>> > observed on Broadwell-based MacBook Air and Ivy Bridge-based MacBook
>> > Pro.
>> >
>> > macOS 11.0 Beta deprecated hv_vcpu_run() and introduced a special
>> > declaration for hv_vcpu_run_until(), that's not available 10.15 -
>> > HV_DEADLINE_FOREVER (UINT64_MAX, which is bigger than maximum value of
>> > VMX-preeemption counter). Perhaps the performance issue is addressed
>> > there.
>>
>> All discussion with Paolo might be helpful, particurlarly:
>> https://lists.nongnu.org/archive/html/qemu-devel/2020-06/msg09893.html
>>
>> > So, I've tried Big Sur Beta and it has exactly the same performance
>> > issue with hv_vcpu_run_until() while hv_vcpu_run() works as good as it
>> > worked on 10.15.5. I've submitted FB7827341 to Apple wrt the issue.
>>
>> In November 2020, Apple responded to FB7827341 that there's an issue on
>> QEMU side.
>
>
> Hmm, that's interesting. I'll need to work my way through that thread, but I'll point out that in my testing with SMP guests, I measured a performance *improvement* with the hv_vcpu_run_until() API (and the forever deadline) versus hv_vcpu_run(), as it significantly reduced BQL contention - with so many VMEXITs, vCPU threads were spending a lot of time waiting for the lock.
>
>

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

[Paolo Bonzini]

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

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

[Phil Dennis-Jordan]

[-- Attachment #1: Type: text/plain, Size: 4716 bytes --]

Hi Paolo,


On Mon, 16 Oct 2023 at 16:39, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> 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.

I didn't think Qemu's HVF backend supported migration in any form at this
point anyway? Or do you mean machine model versioning of the default
setting?

> > 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()?

It's possible! At any rate, hv_vcpu_run() only seems to run for VERY short
time slices in practice. I'll try gathering some data on exit reasons/kick
delivery latencies with the various combinations of kick methods and vcpu
run APIs if that helps.

> 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)

Roman brought me up to speed on this issue in the thread here:
https://lists.gnu.org/archive/html/qemu-devel/2023-10/msg02156.html
including a patch and discussion on the subject from 2020. I've now updated
Roman's old patch which addressed it thanks to your feedback at the time
but which never got merged. (Judging by the list archives, v4 just never
got reviewed.) Details on my latest progress here:

https://lists.gnu.org/archive/html/qemu-devel/2023-10/msg04752.html


> 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.

At least with Roman's updated kick patch, according to tracing points I
inserted locally, I'm not seeing any slow or undelivered kicks with
hv_vcpu_run_until. However, I'm still observing the (S)VGA issues to which
Roman drew my attention at the same time, so I'm wondering if the issues
we're seeing with hv_vcpu_run_until aren't (all) related to interrupt
delivery. (To be clear, switching to hv_vcpu_run_until() WITHOUT
hv_vcpu_interrupt() causes some very obvious problems where the vCPU simply
doesn't exit at all for long periods.)

From my point of view, there are at least 2 strong incentives for switching
to hv_vcpu_run_until():

1. hv_vcpu_run() exits very frequently, and often there is actually nothing
for the VMM to do except call hv_vcpu_run() again. With Qemu's current hvf
backend, each exit causes a BQL acquisition, and VMs with a bunch of vCPUs
rapidly become limited by BQL contention according to my profiling.

2. The HVF in macOS 12 and newer contains an in-kernel APIC implementation,
in a similar vein to KVM’s kernel irqchip. This should further reduce
apparent VM exits, but using it is conditional on using hv_vcpu_run_until().
Calling hv_vcpu_run() errors out with that enabled. Integrating that into
Qemu is still a work in progress, but I’ve got pretty far. (most of the
APIC and IOAPIC suites in kvm-unit-tests pass)

For those 2 reasons I’m pretty motivated to make things work with
hv_vcpu_run_until()
one way or another.

> > 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.

That’s a good idea, I’ll put a patch together doing exactly that.

Thanks,
Phil

[-- Attachment #2: Type: text/html, Size: 6351 bytes --]

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

[Paolo Bonzini]

On Mon, Oct 16, 2023 at 6:45 PM Phil Dennis-Jordan <lists@philjordan.eu> wrote:
>
> Hi Paolo,
>
>
> On Mon, 16 Oct 2023 at 16:39, Paolo Bonzini <pbonzini@redhat.com> wrote:
> >
> > 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.
>
> I didn't think Qemu's HVF backend supported migration in any form at this point anyway? Or do you mean machine model versioning of the default setting?

If it doesn't support migration, it needs to register a migration blocker.

> switching to hv_vcpu_run_until() WITHOUT hv_vcpu_interrupt()
> causes some very obvious problems where the vCPU simply
> doesn't exit at all for long periods.)

Yes, that makes sense. It looks like hv_vcpu_run_until() has an
equivalent of a "do ... while (errno == EINTR)" loop inside it.

> 1. hv_vcpu_run() exits very frequently, and often there is actually
> nothing for the VMM to do except call hv_vcpu_run() again. With
> Qemu's current hvf backend, each exit causes a BQL acquisition,
> and VMs with a bunch of vCPUs rapidly become limited by BQL
> contention according to my profiling.

Yes, that should be fixed anyway, but I agree it is a separate issue.

Paolo

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

[Phil Dennis-Jordan]

On Mon, 16 Oct 2023 at 18:49, Paolo Bonzini <pbonzini@redhat.com> wrote:
> > switching to hv_vcpu_run_until() WITHOUT hv_vcpu_interrupt()
> > causes some very obvious problems where the vCPU simply
> > doesn't exit at all for long periods.)
>
> Yes, that makes sense. It looks like hv_vcpu_run_until() has an
> equivalent of a "do ... while (errno == EINTR)" loop inside it.

Thinking about this some more, I wonder if it's worth putting together
some test code to check empirically how signals and hv_vcpu_interrupt
interact with each of the 2 vcpu_run implementations. It should be
pretty straightforward to establish whether calling hv_vcpu_interrupt
*before* hv_vcpu_run*() causes an instant exit when it is called, and
whether the signal causes hv_vcpu_run() to exit. (Based on observed
behaviour, I'm already pretty confident hv_vcpu_run_until() ignores
signals until it exits for other reasons.)

> > 1. hv_vcpu_run() exits very frequently, and often there is actually
> > nothing for the VMM to do except call hv_vcpu_run() again. With
> > Qemu's current hvf backend, each exit causes a BQL acquisition,
> > and VMs with a bunch of vCPUs rapidly become limited by BQL
> > contention according to my profiling.
>
> Yes, that should be fixed anyway, but I agree it is a separate issue.

I've locally got a bunch of very messy patches for reducing BQL
acquisition in the x86 HVF vCPU loop. I found it difficult to make
much of a real difference however, and the code gets a lot harder to
follow.

- Decoding instructions that need emulating can be done outside the
lock. Actually running the instruction typically ends up causing an
action that needs it though, so the win isn't that big.
- With hv_vcpu_run() there are a bunch of (EPT fault) exits that don't
really need anything in particular to be done. Or instead of detecting
those outside the lock you can switch to hv_vcpu_run_until() which
avoids those exits altogether.
- KVM's vCPU loop calls into MMIO faults without the BQL, but they
acquire it internally I think?
- Going from xAPIC to x2APIC reduces the number of exits, and using
MSRs is a bit quicker than walking the memory hierarchy, so that
definitely helps too, but the APIC implementation still needs the BQL
held, and untangling that is probably harder than switching to an
in-kernel APIC.

Beyond that: there's a good chance that turning the BQL into a
read-write lock would help, but that's a much bigger undertaking than
I'm currently willing to entertain!


Re hvf fd:
> I think fd and the HVF type should be placed in an anonymous union.

Taking a look at the code and thinking through the implications, I'm
not actually sure what the intention of the union is? IIRC Qemu is
built with strict aliasing rules disabled, but there seems little
point in actively using union aliasing here? We can just as easily
modify this block at the top of hvf_int.h:

#ifdef __aarch64__
#include <Hypervisor/Hypervisor.h>
#else
#include <Hypervisor/hv.h>
#endif

to something like:

#ifdef __aarch64__
#include <Hypervisor/Hypervisor.h>
typedef hv_vcpu_t hvf_vcpu_id;
#else
#include <Hypervisor/hv.h>
typedef hv_vcpuid_t hvf_vcpu_id;
#endif

And then:

struct AccelCPUState {
    hvf_vcpu_id fd;
    void *exit;
    …

Or perhaps this variant, if we want AccelCPUState to have consistent
size/alignment properties, we can use a union after all, but never
actually use the fd_padding field:

struct AccelCPUState {
    union {
        hvf_vcpu_id fd;
        uint64_t fd_padding;
    };
    void *exit;
    …

(Or is that what you were thinking of with the union idea in the first place?)


Thanks,
Phil

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

[Paolo Bonzini]

On Mon, Oct 16, 2023 at 10:05 PM Phil Dennis-Jordan <lists@philjordan.eu> wrote:
> > I think fd and the HVF type should be placed in an anonymous union.
>
> Taking a look at the code and thinking through the implications, I'm
> not actually sure what the intention of the union is

Nevermind, I got confused that AccelCPUState depends on what the
actual accelerator is. So yeah, you can just change the type of "fd";
it was an int only because in the past the field was defined for all
accelerators.

Paolo

Re: [PATCH 2/3] i386: hvf: In kick_vcpu use hv_vcpu_interrupt to force exit

[Phil Dennis-Jordan]

Hi Roman, hi Paolo,

Just an update on my investigation of the hv_vcpu_run ->
hv_vcpu_run_until issue. The graphical issues with the Windows XP VM
appear to be caused by the dirty memory page system not working as
expected. The emulated (Cirrus) VGA adapter uses dirty page tracking
to perform partial screen updates, so when pages aren't marked as
dirty, they don't get updated on the host console.

This got me digging into how dirty memory tracking is actually
implemented in the Qemu hvf backend, and basically, it should never
have worked in the first place. When we get a write fault, the code
marks the *whole* 'logged' memory range as writable rather than just
the page that's just been dirtied. It just so happens that hv_vcpu_run
was causing EPT fault exits on those pages even after marking them
writable (?), and hv_vcpu_run_until() no longer does that. So
basically, this has been a Qemu bug masked by undesirable
hv_vcpu_run() behaviour. I'll start putting together a fix for this.

I'm also hoping to settle the hv_vcpu_interrupt() race condition
question empirically - if we can avoid the complicated signal/vmexit
race avoidance logic with atomic flags, that will make the code rather
simpler.

Phil

Re: [PATCH 2/3] i386: hvf: In kick_vcpu use hv_vcpu_interrupt to force exit

[Roman Bolshakov]

On Fri, Oct 20, 2023 at 05:12:13PM +0200, Phil Dennis-Jordan wrote:
> Hi Roman, hi Paolo,
> 

Hi Phil,

Pardon for not responding earlier. I was travelling the last three weeks.

I appreciate the time you spent on the rebase. I have compiled it and
observed the same issue with SVGA like with your third patch.

> Just an update on my investigation of the hv_vcpu_run ->
> hv_vcpu_run_until issue. The graphical issues with the Windows XP VM
> appear to be caused by the dirty memory page system not working as
> expected. The emulated (Cirrus) VGA adapter uses dirty page tracking
> to perform partial screen updates, so when pages aren't marked as
> dirty, they don't get updated on the host console.
> 

That sounds awesome, I think you have tracked it down correctly. I have
also looked at SVGA code and the only idea I had is dirty tracking is
somehow not working properly.

I observed similar issue when tried to add GDB stub for x86 hvf. The
changes from GDB stub produced no apparent effect on the guest -
breakpoints were there, in the memory but did not stop the guest and so
on. I got lost why it didn't work back then.

> This got me digging into how dirty memory tracking is actually
> implemented in the Qemu hvf backend, and basically, it should never
> have worked in the first place. When we get a write fault, the code
> marks the *whole* 'logged' memory range as writable rather than just
> the page that's just been dirtied. It just so happens that hv_vcpu_run
> was causing EPT fault exits on those pages even after marking them
> writable (?), and hv_vcpu_run_until() no longer does that. So
> basically, this has been a Qemu bug masked by undesirable
> hv_vcpu_run() behaviour. I'll start putting together a fix for this.
> 

Sounds good, have you got anything to test or review? Meanwhile, I'll
review the pending patches you sent.

Best regards,
Roman

Re: [PATCH 2/3] i386: hvf: In kick_vcpu use hv_vcpu_interrupt to force exit

[Phil Dennis-Jordan]

[-- Attachment #1: Type: text/plain, Size: 1664 bytes --]

Hi Roman,

On Sun, 5 Nov 2023 at 16:21, Roman Bolshakov <roman@roolebo.dev> wrote:

> > This got me digging into how dirty memory tracking is actually
> > implemented in the Qemu hvf backend, and basically, it should never
> > have worked in the first place. When we get a write fault, the code
> > marks the *whole* 'logged' memory range as writable rather than just
> > the page that's just been dirtied. It just so happens that hv_vcpu_run
> > was causing EPT fault exits on those pages even after marking them
> > writable (?), and hv_vcpu_run_until() no longer does that. So
> > basically, this has been a Qemu bug masked by undesirable
> > hv_vcpu_run() behaviour. I'll start putting together a fix for this.
> >
>
> Sounds good, have you got anything to test or review? Meanwhile, I'll
> review the pending patches you sent.
>

Sorry, I've likewise been busy with other things the last 2-3 weeks.

As far as I'm aware, we don't actually know 100% for certain if there's a
race condition when using hv_vcpu_interrupt(), right? (As I mentioned, the
patches with hv_vcpu_run_until and a hv_vcpu_interrupt-only kick have been
running without issue for months on dozens to hundreds of VMs.) So before
we add the complexity of the hybrid hv_vcpu_interrupt & signal-based kick
to the codebase, I'd like to test out hv_vcpu_interrupt's behaviour in
isolation and actually force the edge cases we're worried about. But yeah,
I haven't got around to doing that yet. :-) I'm hoping to take a crack at
it later this week or next week, probably using
https://github.com/mist64/hvdos as a starting point.

Thanks for reviewing and testing the first set of patches!

Phil

[-- Attachment #2: Type: text/html, Size: 2236 bytes --]