Dirty-tracking in HVF is not properly implemented.

On Intel Macs, Ubuntu ISO boot menu does not show properly.

On Apple Silicon, using bochs-display may cause the guest crashes because
the guest may uses load/store instructions on framebuffer which causes
vmexits and the exception register does not contain enough information
(ESR_EL2.ISV = 0) for QEMU to emulate the memory operation.

The strategy to log the dirty pages is to write-protect the memory regions
that are being dirty-tracked.

When the guest is trapped to the host because of memory write, check whether
the address being written is being dirty-tracked.

If it is being dirty-tracked, restore the write permission of the page and
mark the accessed page dirty, and resume the guest without increasing
program counter, and then the same instruction will be execute again.

This patch fixes the problem and make the dirty-tracking work properly.

Buglink: https://bugs.launchpad.net/qemu/+bug/1827005
Signed-off-by: Yan-Jie Wang <ubzeme@gmail.com>
---
 accel/hvf/hvf-mem.c      | 62 ++++++++++++++++++++++++++++++++++++----
 include/sysemu/hvf_int.h | 14 +--------
 target/arm/hvf/hvf.c     |  5 ++++
 target/i386/hvf/hvf.c    | 25 ++++------------
 4 files changed, 68 insertions(+), 38 deletions(-)

diff --git a/accel/hvf/hvf-mem.c b/accel/hvf/hvf-mem.c
index b8e9f30e4c..896e718374 100644
--- a/accel/hvf/hvf-mem.c
+++ b/accel/hvf/hvf-mem.c
@@ -30,9 +30,21 @@
 
 #define HVF_NUM_SLOTS 32
 
+/* HVFSlot flags */
+#define HVF_SLOT_LOG (1 << 0)
+#define HVF_SLOT_READONLY (1 << 1)
+
+typedef struct HVFSlot {
+    hwaddr start;
+    hwaddr size;  /* 0 if the slot is free */
+    hwaddr offset;  /* offset within memory region */
+    uint32_t flags;
+    MemoryRegion *region;
+} HVFSlot;
+
 static HVFSlot memslots[HVF_NUM_SLOTS];
 
-HVFSlot *hvf_find_overlap_slot(hwaddr start, hwaddr size)
+static HVFSlot *hvf_find_overlap_slot(hwaddr start, hwaddr size)
 {
     HVFSlot *slot;
     int x;
@@ -194,7 +206,7 @@ static void hvf_set_dirty_tracking(MemoryRegionSection *section, bool on)
 static void hvf_log_start(MemoryListener *listener,
                           MemoryRegionSection *section, int old, int new)
 {
-    if (old != 0) {
+    if (old == new) {
         return;
     }
 
@@ -211,12 +223,12 @@ static void hvf_log_stop(MemoryListener *listener,
     hvf_set_dirty_tracking(section, 0);
 }
 
-static void hvf_log_sync(MemoryListener *listener,
+static void hvf_log_clear(MemoryListener *listener,
                          MemoryRegionSection *section)
 {
     /*
-     * sync of dirty pages is handled elsewhere; just make sure we keep
-     * tracking the region.
+     * The dirty bits are being cleared.
+     * Make the section write-protected again.
      */
     hvf_set_dirty_tracking(section, 1);
 }
@@ -240,9 +252,47 @@ static MemoryListener hvf_memory_listener = {
     .region_del = hvf_region_del,
     .log_start = hvf_log_start,
     .log_stop = hvf_log_stop,
-    .log_sync = hvf_log_sync,
+    .log_clear = hvf_log_clear,
 };
 
+
+/*
+ * The function is called when the guest is accessing memory causing vmexit.
+ * Check whether the guest can access the memory directly and
+ * also mark the accessed page being written dirty
+ * if the page is being dirty-tracked.
+ *
+ * Return true if the access is within the mapped region,
+ * otherwise return false.
+ */
+bool hvf_access_memory(hwaddr address, bool write)
+{
+    HVFSlot *slot;
+    hv_return_t ret;
+    hwaddr start, size;
+
+    slot = hvf_find_overlap_slot(address, 1);
+
+    if (!slot || (write && slot->flags & HVF_SLOT_READONLY)) {
+        /* MMIO or unmapped area, return false */
+        return false;
+    }
+
+    if (write && (slot->flags & HVF_SLOT_LOG)) {
+        /* The slot is being dirty-tracked. Mark the accessed page dirty. */
+        start = address & qemu_real_host_page_mask;
+        size = qemu_real_host_page_size;
+
+        memory_region_set_dirty(slot->region,
+                                start - slot->start + slot->offset, size);
+        ret = hv_vm_protect(start, size,
+                    HV_MEMORY_READ | HV_MEMORY_WRITE | HV_MEMORY_EXEC);
+        assert_hvf_ok(ret);
+    }
+
+    return true;
+}
+
 void hvf_init_memslots(void)
 {
     memory_listener_register(&hvf_memory_listener, &address_space_memory);
diff --git a/include/sysemu/hvf_int.h b/include/sysemu/hvf_int.h
index 0aafbc9357..16e5faf0ff 100644
--- a/include/sysemu/hvf_int.h
+++ b/include/sysemu/hvf_int.h
@@ -17,18 +17,6 @@
 #include <Hypervisor/hv.h>
 #endif
 
-/* HVFSlot flags */
-#define HVF_SLOT_LOG (1 << 0)
-#define HVF_SLOT_READONLY (1 << 1)
-
-typedef struct HVFSlot {
-    hwaddr start;
-    hwaddr size;  /* 0 if the slot is free */
-    hwaddr offset;  /* offset within memory region */
-    uint32_t flags;
-    MemoryRegion *region;
-} HVFSlot;
-
 typedef struct hvf_vcpu_caps {
     uint64_t vmx_cap_pinbased;
     uint64_t vmx_cap_procbased;
@@ -58,11 +46,11 @@ int hvf_arch_init(void);
 int hvf_arch_init_vcpu(CPUState *cpu);
 void hvf_arch_vcpu_destroy(CPUState *cpu);
 int hvf_vcpu_exec(CPUState *);
-HVFSlot *hvf_find_overlap_slot(hwaddr, hwaddr);
 int hvf_put_registers(CPUState *);
 int hvf_get_registers(CPUState *);
 void hvf_kick_vcpu_thread(CPUState *cpu);
 
+bool hvf_access_memory(hwaddr address, bool write);
 void hvf_init_memslots(void);
 
 #endif
diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c
index 4d4ddab348..398ad50a29 100644
--- a/target/arm/hvf/hvf.c
+++ b/target/arm/hvf/hvf.c
@@ -1202,6 +1202,11 @@ int hvf_vcpu_exec(CPUState *cpu)
             break;
         }
 
+        if (iswrite &&
+            hvf_access_memory(hvf_exit->exception.physical_address, 1)) {
+            break;
+        }
+
         assert(isv);
 
         if (iswrite) {
diff --git a/target/i386/hvf/hvf.c b/target/i386/hvf/hvf.c
index 2ddb4fc825..c4c544dc54 100644
--- a/target/i386/hvf/hvf.c
+++ b/target/i386/hvf/hvf.c
@@ -113,7 +113,7 @@ void hvf_handle_io(CPUArchState *env, uint16_t port, void *buffer,
     }
 }
 
-static bool ept_emulation_fault(HVFSlot *slot, uint64_t gpa, uint64_t ept_qual)
+static bool ept_emulation_fault(uint64_t gpa, uint64_t ept_qual)
 {
     int read, write;
 
@@ -129,14 +129,6 @@ static bool ept_emulation_fault(HVFSlot *slot, uint64_t gpa, uint64_t ept_qual)
         return false;
     }
 
-    if (write && slot) {
-        if (slot->flags & HVF_SLOT_LOG) {
-            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);
-        }
-    }
-
     /*
      * The EPT violation must have been caused by accessing a
      * guest-physical address that is a translation of a guest-linear
@@ -147,14 +139,11 @@ static bool ept_emulation_fault(HVFSlot *slot, uint64_t gpa, uint64_t ept_qual)
         return false;
     }
 
-    if (!slot) {
-        return true;
+    if (hvf_access_memory(gpa, write)) {
+        return false;
     }
-    if (!memory_region_is_ram(slot->region) &&
-        !(read && memory_region_is_romd(slot->region))) {
-        return true;
-    }
-    return false;
+
+    return true;
 }
 
 void hvf_arch_vcpu_destroy(CPUState *cpu)
@@ -469,7 +458,6 @@ int hvf_vcpu_exec(CPUState *cpu)
         /* Need to check if MMIO or unmapped fault */
         case EXIT_REASON_EPT_FAULT:
         {
-            HVFSlot *slot;
             uint64_t gpa = rvmcs(cpu->hvf->fd, VMCS_GUEST_PHYSICAL_ADDRESS);
 
             if (((idtvec_info & VMCS_IDT_VEC_VALID) == 0) &&
@@ -477,9 +465,8 @@ int hvf_vcpu_exec(CPUState *cpu)
                 vmx_set_nmi_blocking(cpu);
             }
 
-            slot = hvf_find_overlap_slot(gpa, 1);
             /* mmio */
-            if (ept_emulation_fault(slot, gpa, exit_qual)) {
+            if (ept_emulation_fault(gpa, exit_qual)) {
                 struct x86_decode decode;
 
                 load_regs(cpu);
-- 
2.32.0 (Apple Git-132)

On Wed, 2 Mar 2022 at 13:04, Yan-Jie Wang <ubzeme@gmail.com> wrote:
>
> Dirty-tracking in HVF is not properly implemented.
>
> On Intel Macs, Ubuntu ISO boot menu does not show properly.
>
> On Apple Silicon, using bochs-display may cause the guest crashes because
> the guest may uses load/store instructions on framebuffer which causes
> vmexits and the exception register does not contain enough information
> (ESR_EL2.ISV = 0) for QEMU to emulate the memory operation.
>
> The strategy to log the dirty pages is to write-protect the memory regions
> that are being dirty-tracked.
>
> When the guest is trapped to the host because of memory write, check whether
> the address being written is being dirty-tracked.
>
> If it is being dirty-tracked, restore the write permission of the page and
> mark the accessed page dirty, and resume the guest without increasing
> program counter, and then the same instruction will be execute again.
>
> This patch fixes the problem and make the dirty-tracking work properly.
>
> Buglink: https://bugs.launchpad.net/qemu/+bug/1827005
> Signed-off-by: Yan-Jie Wang <ubzeme@gmail.com>
> ---
>  accel/hvf/hvf-mem.c      | 62 ++++++++++++++++++++++++++++++++++++----
>  include/sysemu/hvf_int.h | 14 +--------
>  target/arm/hvf/hvf.c     |  5 ++++
>  target/i386/hvf/hvf.c    | 25 ++++------------
>  4 files changed, 68 insertions(+), 38 deletions(-)
>


> +/*
> + * The function is called when the guest is accessing memory causing vmexit.
> + * Check whether the guest can access the memory directly and
> + * also mark the accessed page being written dirty
> + * if the page is being dirty-tracked.
> + *
> + * Return true if the access is within the mapped region,
> + * otherwise return false.
> + */
> +bool hvf_access_memory(hwaddr address, bool write)
> +{
> +    HVFSlot *slot;
> +    hv_return_t ret;
> +    hwaddr start, size;
> +
> +    slot = hvf_find_overlap_slot(address, 1);

What happens if the guest does an unaligned 4 byte access
such that byte 1 is in one slot and bytes 2-4 are in a
different slot, or not covered by a slot at all ?

> diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c
> index 4d4ddab348..398ad50a29 100644
> --- a/target/arm/hvf/hvf.c
> +++ b/target/arm/hvf/hvf.c
> @@ -1202,6 +1202,11 @@ int hvf_vcpu_exec(CPUState *cpu)
>              break;
>          }
>
> +        if (iswrite &&
> +            hvf_access_memory(hvf_exit->exception.physical_address, 1)) {

hvf_access_memory() can return true even if it has not changed the
protection flags on the memory, in which case we'll go into an
infinite loop of taking the fault again.

> +            break;
> +        }
> +
>          assert(isv);
>
>          if (iswrite)

thanks
-- PMM