[PATCH v2 0/4] hvf x86 correctness and efficiency improvements part 1

[PATCH v2 0/4] hvf x86 correctness and efficiency improvements part 1

[Phil Dennis-Jordan]

This is a series of semi-related patches for the x86 macOS Hypervisor.framework
(hvf) accelerator backend. The intention is to (1) fix bugs and (2) move the
hvf backend to use more modern and efficient APIs in Hypervisor.framework.

The eventual goal is to replace the main hv_vcpu_run() call with
hv_vcpu_run_until(); the final 2 patches for this were included in v1 of this
series, but I've omitted them until part 2 this time around, as discussion
raised some questions about edge cases and race conditions when forcing VM
exits. I'm still working on getting solid answers on those questions and will
only then follow up with corresponding patches. Until then, this first set
of changes lays some groundwork.

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 now also includes setting a migration
blocker when the feature is active.

Patch 2 fixes a bunch of compile warnings that kept littering my build logs,
so I finally caved and fixed them. As far as I can tell, these were all
ancient typos.

Patch 3 sorts out the minor mess of hvf vCPU IDs/handles. The aarch64 and
x86-64 versions of Hypervisor.framework's APIs use different integral types
(uint64_t vs unsigned int) when referencing vCPUs, so this changes the code
to use the correct one depending on build arch instead of awkward pointer
casts. (There's currently only one instance of such a cast, but the patches
in part 2 would have added more, so I'm fixing this preemptively.)

Patch 4 fixes dirty page tracking for the x86 hvf backend. This has
previously only accidentally worked because hv_vcpu_run() makes spurious EPT
fault exits. Switching to hv_vcpu_run_until() surfaces this issue when using
an emulated VGA adapter for example, as those use dirty page tracking to do
partial screen updates.

changelog:
v2:
 - Migration blocker when INVTSC is set (Thanks Paolo for pointing that out!)
 - Dirty page tracking fix (Thanks Roman for noticing the regression in
   observed behaviour on certain VMs, which led me to debugging this issue.)
 - vCPU handle type cleanup (Based on discussion with Paolo)
 - Added fixes for existing compile warnings.
 - Split patch series into 2 parts.

This work has been sponsored by Sauce Labs Inc.

Phil Dennis-Jordan (4):
  i386: hvf: Adds support for INVTSC cpuid bit
  hvf: Fixes some compilation warnings
  hvf: Consistent types for vCPU handles
  i386/hvf: Fixes dirty memory tracking by page granularity RX->RWX
    change

 accel/hvf/hvf-accel-ops.c    |  2 +-
 include/sysemu/hvf_int.h     |  4 +++-
 target/i386/hvf/hvf.c        | 23 +++++++++++++++++++++--
 target/i386/hvf/vmx.h        |  3 +--
 target/i386/hvf/x86_cpuid.c  |  4 ++++
 target/i386/hvf/x86_decode.c |  2 +-
 target/i386/hvf/x86_emu.c    |  4 ++--
 7 files changed, 33 insertions(+), 9 deletions(-)

-- 
2.36.1

[PATCH v2 1/4] 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.
When the feature is enabled, a migration blocker is installed.

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

diff --git a/target/i386/hvf/hvf.c b/target/i386/hvf/hvf.c
index cb2cd0b02f..43d64574ad 100644
--- a/target/i386/hvf/hvf.c
+++ b/target/i386/hvf/hvf.c
@@ -49,6 +49,8 @@
 #include "qemu/osdep.h"
 #include "qemu/error-report.h"
 #include "qemu/memalign.h"
+#include "qapi/error.h"
+#include "migration/blocker.h"
 
 #include "sysemu/hvf.h"
 #include "sysemu/hvf_int.h"
@@ -74,6 +76,8 @@
 #include "qemu/accel.h"
 #include "target/i386/cpu.h"
 
+static Error *invtsc_mig_blocker;
+
 void vmx_update_tpr(CPUState *cpu)
 {
     /* TODO: need integrate APIC handling */
@@ -221,6 +225,8 @@ int hvf_arch_init_vcpu(CPUState *cpu)
 {
     X86CPU *x86cpu = X86_CPU(cpu);
     CPUX86State *env = &x86cpu->env;
+    Error *local_err = NULL;
+    int r;
     uint64_t reqCap;
 
     init_emu();
@@ -238,6 +244,18 @@ int hvf_arch_init_vcpu(CPUState *cpu)
         }
     }
 
+    if ((env->features[FEAT_8000_0007_EDX] & CPUID_APM_INVTSC) &&
+        invtsc_mig_blocker == NULL) {
+        error_setg(&invtsc_mig_blocker,
+                   "State blocked by non-migratable CPU device (invtsc flag)");
+        r = migrate_add_blocker(&invtsc_mig_blocker, &local_err);
+        if (r < 0) {
+            error_report_err(local_err);
+            return r;
+        }
+    }
+
+
     if (hv_vmx_read_capability(HV_VMX_CAP_PINBASED,
         &hvf_state->hvf_caps->vmx_cap_pinbased)) {
         abort();
diff --git a/target/i386/hvf/x86_cpuid.c b/target/i386/hvf/x86_cpuid.c
index 9380b90496..e56cd8411b 100644
--- a/target/i386/hvf/x86_cpuid.c
+++ b/target/i386/hvf/x86_cpuid.c
@@ -146,6 +146,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

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

[Roman Bolshakov]

On Sat, Oct 21, 2023 at 10:05:15PM +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.
> When the feature is enabled, a migration blocker is installed.
> 

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

Thanks,
Roman

[PATCH v2 2/4] hvf: Fixes some compilation warnings

[Phil Dennis-Jordan]

A bunch of function definitions used empty parentheses instead of (void) syntax, yielding the following warning when building with clang on macOS:

warning: a function declaration without a prototype is deprecated in all versions of C [-Wstrict-prototypes]

In addition to fixing these function headers, it also fixes what appears to be a typo causing a variable to be unused after initialisation.

warning: variable 'entry_ctls' set but not used [-Wunused-but-set-variable]

Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
---
 target/i386/hvf/vmx.h        | 3 +--
 target/i386/hvf/x86_decode.c | 2 +-
 target/i386/hvf/x86_emu.c    | 4 ++--
 3 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/target/i386/hvf/vmx.h b/target/i386/hvf/vmx.h
index 0fffcfa46c..3954ef883d 100644
--- a/target/i386/hvf/vmx.h
+++ b/target/i386/hvf/vmx.h
@@ -95,8 +95,7 @@ static void enter_long_mode(hv_vcpuid_t vcpu, uint64_t cr0, uint64_t efer)
     efer |= MSR_EFER_LMA;
     wvmcs(vcpu, VMCS_GUEST_IA32_EFER, efer);
     entry_ctls = rvmcs(vcpu, VMCS_ENTRY_CTLS);
-    wvmcs(vcpu, VMCS_ENTRY_CTLS, rvmcs(vcpu, VMCS_ENTRY_CTLS) |
-          VM_ENTRY_GUEST_LMA);
+    wvmcs(vcpu, VMCS_ENTRY_CTLS, entry_ctls | VM_ENTRY_GUEST_LMA);
 
     uint64_t guest_tr_ar = rvmcs(vcpu, VMCS_GUEST_TR_ACCESS_RIGHTS);
     if ((efer & MSR_EFER_LME) &&
diff --git a/target/i386/hvf/x86_decode.c b/target/i386/hvf/x86_decode.c
index 3728d7705e..a4a28f113f 100644
--- a/target/i386/hvf/x86_decode.c
+++ b/target/i386/hvf/x86_decode.c
@@ -2111,7 +2111,7 @@ uint32_t decode_instruction(CPUX86State *env, struct x86_decode *decode)
     return decode->len;
 }
 
-void init_decoder()
+void init_decoder(void)
 {
     int i;
     
diff --git a/target/i386/hvf/x86_emu.c b/target/i386/hvf/x86_emu.c
index ccda568478..852f766161 100644
--- a/target/i386/hvf/x86_emu.c
+++ b/target/i386/hvf/x86_emu.c
@@ -1410,7 +1410,7 @@ static struct cmd_handler {
 
 static struct cmd_handler _cmd_handler[X86_DECODE_CMD_LAST];
 
-static void init_cmd_handler()
+static void init_cmd_handler(void)
 {
     int i;
     for (i = 0; i < ARRAY_SIZE(handlers); i++) {
@@ -1482,7 +1482,7 @@ bool exec_instruction(CPUX86State *env, struct x86_decode *ins)
     return true;
 }
 
-void init_emu()
+void init_emu(void)
 {
     init_cmd_handler();
 }
-- 
2.36.1

Re: [PATCH v2 2/4] hvf: Fixes some compilation warnings

[Roman Bolshakov]

On Sat, Oct 21, 2023 at 10:05:16PM +0200, Phil Dennis-Jordan wrote:
> A bunch of function definitions used empty parentheses instead of (void) syntax, yielding the following warning when building with clang on macOS:
> 
> warning: a function declaration without a prototype is deprecated in all versions of C [-Wstrict-prototypes]
> 
> In addition to fixing these function headers, it also fixes what appears to be a typo causing a variable to be unused after initialisation.
> 
> warning: variable 'entry_ctls' set but not used [-Wunused-but-set-variable]
> 

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

Thanks,
Roman

[PATCH v2 3/4] hvf: Consistent types for vCPU handles

[Phil Dennis-Jordan]

macOS Hypervisor.framework uses different types for identifying vCPUs,
hv_vcpu_t or hv_vcpuid_t, depending on host architecture. They are not just
differently named typedefs for the same primitive type, but reference
different-width integers.

Instead of using an integer type and casting where necessary, this change
introduces a typedef which resolves to the active architecture’s hvf typedef.
It also removes a now-unnecessary cast.

Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
---
 accel/hvf/hvf-accel-ops.c | 2 +-
 include/sysemu/hvf_int.h  | 4 +++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/accel/hvf/hvf-accel-ops.c b/accel/hvf/hvf-accel-ops.c
index abe7adf7ee..165e54ea27 100644
--- a/accel/hvf/hvf-accel-ops.c
+++ b/accel/hvf/hvf-accel-ops.c
@@ -400,7 +400,7 @@ static int hvf_init_vcpu(CPUState *cpu)
     r = hv_vcpu_create(&cpu->accel->fd,
                        (hv_vcpu_exit_t **)&cpu->accel->exit, NULL);
 #else
-    r = hv_vcpu_create((hv_vcpuid_t *)&cpu->accel->fd, HV_VCPU_DEFAULT);
+    r = hv_vcpu_create(&cpu->accel->fd, HV_VCPU_DEFAULT);
 #endif
     cpu->vcpu_dirty = 1;
     assert_hvf_ok(r);
diff --git a/include/sysemu/hvf_int.h b/include/sysemu/hvf_int.h
index 718beddcdd..7980c90825 100644
--- a/include/sysemu/hvf_int.h
+++ b/include/sysemu/hvf_int.h
@@ -13,8 +13,10 @@
 
 #ifdef __aarch64__
 #include <Hypervisor/Hypervisor.h>
+typedef hv_vcpu_t hvf_vcpuid;
 #else
 #include <Hypervisor/hv.h>
+typedef hv_vcpuid_t hvf_vcpuid;
 #endif
 
 /* hvf_slot flags */
@@ -50,7 +52,7 @@ struct HVFState {
 extern HVFState *hvf_state;
 
 struct AccelCPUState {
-    uint64_t fd;
+    hvf_vcpuid fd;
     void *exit;
     bool vtimer_masked;
     sigset_t unblock_ipi_mask;
-- 
2.36.1

Re: [PATCH v2 3/4] hvf: Consistent types for vCPU handles

[Roman Bolshakov]

On Sat, Oct 21, 2023 at 10:05:17PM +0200, Phil Dennis-Jordan wrote:
> macOS Hypervisor.framework uses different types for identifying vCPUs,
> hv_vcpu_t or hv_vcpuid_t, depending on host architecture. They are not just
> differently named typedefs for the same primitive type, but reference
> different-width integers.
> 
> Instead of using an integer type and casting where necessary, this change
> introduces a typedef which resolves to the active architecture’s hvf typedef.
> It also removes a now-unnecessary cast.
> 

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

Regards,
Roman

[PATCH v2 4/4] i386/hvf: Fixes dirty memory tracking by page granularity RX->RWX change

[Phil Dennis-Jordan]

When using x86 macOS Hypervisor.framework as accelerator, detection of
dirty memory regions is implemented by marking logged memory region
slots as read-only in the EPT, then setting the dirty flag when a
guest write causes a fault. The area marked dirty should then be marked
writable in order for subsequent writes to succeed without a VM exit.

However, dirty bits are tracked on a per-page basis, whereas the fault
handler was marking the whole logged memory region as writable. This
change fixes the fault handler so only the protection of the single
faulting page is marked as dirty.

(Note: the dirty page tracking appeared to work despite this error
because HVF’s hv_vcpu_run() function generated unnecessary EPT fault
exits, which ended up causing the dirty marking handler to run even
when the memory region had been marked RW. When using
hv_vcpu_run_until(), a change planned for a subsequent commit, these
spurious exits no longer occur, so dirty memory tracking malfunctions.)

Additionally, the dirty page is set to permit code execution, the same
as all other guest memory; changing memory protection from RX to RW not
RWX appears to have been an oversight.

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

diff --git a/target/i386/hvf/hvf.c b/target/i386/hvf/hvf.c
index 43d64574ad..a15ee469c3 100644
--- a/target/i386/hvf/hvf.c
+++ b/target/i386/hvf/hvf.c
@@ -135,9 +135,10 @@ static bool ept_emulation_fault(hvf_slot *slot, uint64_t gpa, uint64_t ept_qual)
 
     if (write && slot) {
         if (slot->flags & HVF_SLOT_LOG) {
+            uint64_t dirty_page_start = gpa & ~(TARGET_PAGE_SIZE - 1u);
             memory_region_set_dirty(slot->region, gpa - slot->start, 1);
-            hv_vm_protect((hv_gpaddr_t)slot->start, (size_t)slot->size,
-                          HV_MEMORY_READ | HV_MEMORY_WRITE);
+            hv_vm_protect(dirty_page_start, TARGET_PAGE_SIZE,
+                          HV_MEMORY_READ | HV_MEMORY_WRITE | HV_MEMORY_EXEC);
         }
     }
 
-- 
2.36.1

Re: [PATCH v2 4/4] i386/hvf: Fixes dirty memory tracking by page granularity RX->RWX change

[Roman Bolshakov]

On Sat, Oct 21, 2023 at 10:05:18PM +0200, Phil Dennis-Jordan wrote:
> When using x86 macOS Hypervisor.framework as accelerator, detection of
> dirty memory regions is implemented by marking logged memory region
> slots as read-only in the EPT, then setting the dirty flag when a
> guest write causes a fault. The area marked dirty should then be marked
> writable in order for subsequent writes to succeed without a VM exit.
> 
> However, dirty bits are tracked on a per-page basis, whereas the fault
> handler was marking the whole logged memory region as writable. This
> change fixes the fault handler so only the protection of the single
> faulting page is marked as dirty.
> 
> (Note: the dirty page tracking appeared to work despite this error
> because HVF’s hv_vcpu_run() function generated unnecessary EPT fault
> exits, which ended up causing the dirty marking handler to run even
> when the memory region had been marked RW. When using
> hv_vcpu_run_until(), a change planned for a subsequent commit, these
> spurious exits no longer occur, so dirty memory tracking malfunctions.)
> 
> Additionally, the dirty page is set to permit code execution, the same
> as all other guest memory; changing memory protection from RX to RW not
> RWX appears to have been an oversight.
> 

Hi Phil, I don't observe a problem with SVGA if I apply CPU kick patch
on top of it. Thanks for fixing this,

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

Regards,
Roman