* Remove mac_slot and use hvf_slot only. The function of the two structures
  are similar.

* Refactor function hvf_set_phys_mem():
 - Remove unnecessary checks because any modified memory sections
   will be removed first (region_del called first) before being added.
   Therefore, new sections do not overlap with existing sections.
 - Try to align memory sections first before giving up sections that are not
   aligned to host page size.

Signed-off-by: Yan-Jie Wang <ubzeme@gmail.com>
---
 accel/hvf/hvf-accel-ops.c |   1 -
 accel/hvf/hvf-mem.c       | 211 +++++++++++++++++++-------------------
 include/sysemu/hvf_int.h  |   8 +-
 3 files changed, 107 insertions(+), 113 deletions(-)

diff --git a/accel/hvf/hvf-accel-ops.c b/accel/hvf/hvf-accel-ops.c
index 50a563bfe0..c77f142f2b 100644
--- a/accel/hvf/hvf-accel-ops.c
+++ b/accel/hvf/hvf-accel-ops.c
@@ -49,7 +49,6 @@
 
 #include "qemu/osdep.h"
 #include "qemu/main-loop.h"
-#include "exec/address-spaces.h"
 #include "exec/exec-all.h"
 #include "sysemu/cpus.h"
 #include "sysemu/hvf.h"
diff --git a/accel/hvf/hvf-mem.c b/accel/hvf/hvf-mem.c
index 3712731ed9..32452696b6 100644
--- a/accel/hvf/hvf-mem.c
+++ b/accel/hvf/hvf-mem.c
@@ -28,12 +28,16 @@
 
 /* Memory slots */
 
+#define HVF_NUM_SLOTS 32
+
+static hvf_slot memslots[HVF_NUM_SLOTS];
+
 hvf_slot *hvf_find_overlap_slot(uint64_t start, uint64_t size)
 {
     hvf_slot *slot;
     int x;
-    for (x = 0; x < hvf_state->num_slots; ++x) {
-        slot = &hvf_state->slots[x];
+    for (x = 0; x < HVF_NUM_SLOTS; ++x) {
+        slot = &memslots[x];
         if (slot->size && start < (slot->start + slot->size) &&
             (start + size) > slot->start) {
             return slot;
@@ -42,128 +46,130 @@ hvf_slot *hvf_find_overlap_slot(uint64_t start, uint64_t size)
     return NULL;
 }
 
-struct mac_slot {
-    int present;
-    uint64_t size;
-    uint64_t gpa_start;
-    uint64_t gva;
-};
-
-struct mac_slot mac_slots[32];
-
-static int do_hvf_set_memory(hvf_slot *slot, hv_memory_flags_t flags)
+static hvf_slot *hvf_find_free_slot(void)
 {
-    struct mac_slot *macslot;
-    hv_return_t ret;
-
-    macslot = &mac_slots[slot->slot_id];
-
-    if (macslot->present) {
-        if (macslot->size != slot->size) {
-            macslot->present = 0;
-            ret = hv_vm_unmap(macslot->gpa_start, macslot->size);
-            assert_hvf_ok(ret);
+    hvf_slot *slot;
+    int x;
+    for (x = 0; x < HVF_NUM_SLOTS; x++) {
+        slot = &memslots[x];
+        if (!slot->size) {
+            return slot;
         }
     }
 
-    if (!slot->size) {
-        return 0;
-    }
-
-    macslot->present = 1;
-    macslot->gpa_start = slot->start;
-    macslot->size = slot->size;
-    ret = hv_vm_map(slot->mem, slot->start, slot->size, flags);
-    assert_hvf_ok(ret);
-    return 0;
+    return NULL;
+}
+
+/*
+ * Hypervisor.framework requires that the host virtual address,
+ * the guest physical address and the size of memory regions are aligned
+ * to the host page size.
+ *
+ * The function here tries to align the guest physical address and the size.
+ *
+ * The return value is the aligned size.
+ * The aligned guest physical address will be written to `start'.
+ * The delta between the aligned address and the original address will be
+ * written to `delta'.
+ */
+static hwaddr hvf_align_section(MemoryRegionSection *section,
+                              hwaddr *start, hwaddr *delta)
+{
+    hwaddr unaligned, _start, size, _delta;
+
+    unaligned = section->offset_within_address_space;
+    size = int128_get64(section->size);
+    _start = ROUND_UP(unaligned, qemu_real_host_page_size);
+    _delta = _start - unaligned;
+    size = (size - _delta) & qemu_real_host_page_mask;
+
+    *start = _start;
+    *delta = _delta;
+
+    return size;
 }
 
 static void hvf_set_phys_mem(MemoryRegionSection *section, bool add)
 {
-    hvf_slot *mem;
+    hvf_slot *slot;
+    hwaddr start, size, offset, delta;
+    uint8_t *host_addr;
     MemoryRegion *area = section->mr;
-    bool writeable = !area->readonly && !area->rom_device;
+    bool readonly, dirty_tracking;
     hv_memory_flags_t flags;
-    uint64_t page_size = qemu_real_host_page_size;
+    hv_return_t ret;
 
-    if (!memory_region_is_ram(area)) {
-        if (writeable) {
+    if (add && !memory_region_is_ram(area) && !memory_region_is_romd(area)) {
+        /*
+         * If the memory region is not RAM and is in ROMD mode,
+         * do not map it to the guest.
+         */
+        return;
+    }
+
+    size = hvf_align_section(section, &start, &delta);
+
+    if (!size) {
+        /* The size is 0 after aligned. Do not map the region */
+        return;
+    }
+
+    if (add) {
+        /* add memory region */
+        offset = section->offset_within_region + delta;
+        host_addr = memory_region_get_ram_ptr(area) + offset;
+
+        if (!QEMU_PTR_IS_ALIGNED(host_addr, qemu_real_host_page_size)) {
+            /* The host virtual address is not aligned. It cannot be mapped */
             return;
-        } else if (!memory_region_is_romd(area)) {
-            /*
-             * If the memory device is not in romd_mode, then we actually want
-             * to remove the hvf memory slot so all accesses will trap.
-             */
-             add = false;
         }
-    }
 
-    if (!QEMU_IS_ALIGNED(int128_get64(section->size), page_size) ||
-        !QEMU_IS_ALIGNED(section->offset_within_address_space, page_size)) {
-        /* Not page aligned, so we can not map as RAM */
-        add = false;
-    }
+        dirty_tracking = !!memory_region_get_dirty_log_mask(area);
+        readonly = memory_region_is_rom(area) || memory_region_is_romd(area);
 
-    mem = hvf_find_overlap_slot(
-            section->offset_within_address_space,
-            int128_get64(section->size));
-
-    if (mem && add) {
-        if (mem->size == int128_get64(section->size) &&
-            mem->start == section->offset_within_address_space &&
-            mem->mem == (memory_region_get_ram_ptr(area) +
-            section->offset_within_region)) {
-            return; /* Same region was attempted to register, go away. */
-        }
-    }
-
-    /* Region needs to be reset. set the size to 0 and remap it. */
-    if (mem) {
-        mem->size = 0;
-        if (do_hvf_set_memory(mem, 0)) {
-            error_report("Failed to reset overlapping slot");
+        /* setup a slot */
+        slot = hvf_find_free_slot();
+        if (!slot) {
+            error_report("No free slots");
             abort();
         }
-    }
 
-    if (!add) {
-        return;
-    }
+        slot->start = start;
+        slot->size = size;
+        slot->offset = offset;
+        slot->flags = 0;
+        slot->region = area;
 
-    if (area->readonly ||
-        (!memory_region_is_ram(area) && memory_region_is_romd(area))) {
-        flags = HV_MEMORY_READ | HV_MEMORY_EXEC;
+        if (readonly) {
+            slot->flags |= HVF_SLOT_READONLY;
+        }
+
+        if (dirty_tracking) {
+            slot->flags |= HVF_SLOT_LOG;
+        }
+
+        /* set Hypervisor.framework memory mapping flags */
+        if (readonly || dirty_tracking) {
+            flags = HV_MEMORY_READ | HV_MEMORY_EXEC;
+        } else {
+            flags = HV_MEMORY_READ | HV_MEMORY_WRITE | HV_MEMORY_EXEC;
+        }
+
+        ret = hv_vm_map(host_addr, start, size, flags);
+        assert_hvf_ok(ret);
     } else {
-        flags = HV_MEMORY_READ | HV_MEMORY_WRITE | HV_MEMORY_EXEC;
-    }
+        /* remove memory region */
+        slot = hvf_find_overlap_slot(start, size);
 
-    /* Now make a new slot. */
-    int x;
+        if (slot) {
+            ret = hv_vm_unmap(start, size);
+            assert_hvf_ok(ret);
 
-    for (x = 0; x < hvf_state->num_slots; ++x) {
-        mem = &hvf_state->slots[x];
-        if (!mem->size) {
-            break;
+            slot->size = 0;
         }
     }
-
-    if (x == hvf_state->num_slots) {
-        error_report("No free slots");
-        abort();
-    }
-
-    mem->size = int128_get64(section->size);
-    mem->mem = memory_region_get_ram_ptr(area) + section->offset_within_region;
-    mem->start = section->offset_within_address_space;
-    mem->region = area;
-
-    if (do_hvf_set_memory(mem, flags)) {
-        error_report("Error registering new memory slot");
-        abort();
-    }
 }
 
-
 static void hvf_set_dirty_tracking(MemoryRegionSection *section, bool on)
 {
     hvf_slot *slot;
@@ -239,14 +245,5 @@ static MemoryListener hvf_memory_listener = {
 
 void hvf_init_memslots(void)
 {
-    int x;
-    HVFState *s = hvf_state;
-
-    s->num_slots = ARRAY_SIZE(s->slots);
-    for (x = 0; x < s->num_slots; ++x) {
-        s->slots[x].size = 0;
-        s->slots[x].slot_id = x;
-    }
-
     memory_listener_register(&hvf_memory_listener, &address_space_memory);
 }
diff --git a/include/sysemu/hvf_int.h b/include/sysemu/hvf_int.h
index cef20d750d..8ee31a16ac 100644
--- a/include/sysemu/hvf_int.h
+++ b/include/sysemu/hvf_int.h
@@ -19,12 +19,12 @@
 
 /* hvf_slot flags */
 #define HVF_SLOT_LOG (1 << 0)
+#define HVF_SLOT_READONLY (1 << 1)
 
 typedef struct hvf_slot {
     uint64_t start;
-    uint64_t size;
-    uint8_t *mem;
-    int slot_id;
+    uint64_t size;  /* 0 if the slot is free */
+    uint64_t offset;  /* offset within memory region */
     uint32_t flags;
     MemoryRegion *region;
 } hvf_slot;
@@ -40,8 +40,6 @@ typedef struct hvf_vcpu_caps {
 
 struct HVFState {
     AccelState parent;
-    hvf_slot slots[32];
-    int num_slots;
 
     hvf_vcpu_caps *hvf_caps;
     uint64_t vtimer_offset;
-- 
2.32.0 (Apple Git-132)

On Wed, 2 Mar 2022 at 13:04, Yan-Jie Wang <ubzeme@gmail.com> wrote:
>
> * Remove mac_slot and use hvf_slot only. The function of the two structures
>   are similar.
>
> * Refactor function hvf_set_phys_mem():
>  - Remove unnecessary checks because any modified memory sections
>    will be removed first (region_del called first) before being added.
>    Therefore, new sections do not overlap with existing sections.
>  - Try to align memory sections first before giving up sections that are not
>    aligned to host page size.
>
> Signed-off-by: Yan-Jie Wang <ubzeme@gmail.com>
> ---
>  accel/hvf/hvf-accel-ops.c |   1 -
>  accel/hvf/hvf-mem.c       | 211 +++++++++++++++++++-------------------
>  include/sysemu/hvf_int.h  |   8 +-
>  3 files changed, 107 insertions(+), 113 deletions(-)
>
> diff --git a/accel/hvf/hvf-accel-ops.c b/accel/hvf/hvf-accel-ops.c
> index 50a563bfe0..c77f142f2b 100644
> --- a/accel/hvf/hvf-accel-ops.c
> +++ b/accel/hvf/hvf-accel-ops.c
> @@ -49,7 +49,6 @@
>
>  #include "qemu/osdep.h"
>  #include "qemu/main-loop.h"
> -#include "exec/address-spaces.h"
>  #include "exec/exec-all.h"
>  #include "sysemu/cpus.h"
>  #include "sysemu/hvf.h"

I think this deletion of the #include line should have been in patch 1?

> diff --git a/accel/hvf/hvf-mem.c b/accel/hvf/hvf-mem.c
> index 3712731ed9..32452696b6 100644
> --- a/accel/hvf/hvf-mem.c
> +++ b/accel/hvf/hvf-mem.c
> @@ -28,12 +28,16 @@
>
>  /* Memory slots */
>
> +#define HVF_NUM_SLOTS 32
> +
> +static hvf_slot memslots[HVF_NUM_SLOTS];
> +
>  hvf_slot *hvf_find_overlap_slot(uint64_t start, uint64_t size)
>  {
>      hvf_slot *slot;
>      int x;
> -    for (x = 0; x < hvf_state->num_slots; ++x) {
> -        slot = &hvf_state->slots[x];
> +    for (x = 0; x < HVF_NUM_SLOTS; ++x) {
> +        slot = &memslots[x];
>          if (slot->size && start < (slot->start + slot->size) &&
>              (start + size) > slot->start) {
>              return slot;
> @@ -42,128 +46,130 @@ hvf_slot *hvf_find_overlap_slot(uint64_t start, uint64_t size)
>      return NULL;
>  }
>
> -struct mac_slot {
> -    int present;
> -    uint64_t size;
> -    uint64_t gpa_start;
> -    uint64_t gva;
> -};
> -
> -struct mac_slot mac_slots[32];
> -
> -static int do_hvf_set_memory(hvf_slot *slot, hv_memory_flags_t flags)
> +static hvf_slot *hvf_find_free_slot(void)
>  {
> -    struct mac_slot *macslot;
> -    hv_return_t ret;
> -
> -    macslot = &mac_slots[slot->slot_id];
> -
> -    if (macslot->present) {
> -        if (macslot->size != slot->size) {
> -            macslot->present = 0;
> -            ret = hv_vm_unmap(macslot->gpa_start, macslot->size);
> -            assert_hvf_ok(ret);
> +    hvf_slot *slot;
> +    int x;
> +    for (x = 0; x < HVF_NUM_SLOTS; x++) {
> +        slot = &memslots[x];
> +        if (!slot->size) {
> +            return slot;
>          }
>      }
>
> -    if (!slot->size) {
> -        return 0;
> -    }
> -
> -    macslot->present = 1;
> -    macslot->gpa_start = slot->start;
> -    macslot->size = slot->size;
> -    ret = hv_vm_map(slot->mem, slot->start, slot->size, flags);
> -    assert_hvf_ok(ret);
> -    return 0;
> +    return NULL;
> +}
> +
> +/*
> + * Hypervisor.framework requires that the host virtual address,
> + * the guest physical address and the size of memory regions are aligned
> + * to the host page size.
> + *
> + * The function here tries to align the guest physical address and the size.
> + *
> + * The return value is the aligned size.
> + * The aligned guest physical address will be written to `start'.
> + * The delta between the aligned address and the original address will be
> + * written to `delta'.
> + */
> +static hwaddr hvf_align_section(MemoryRegionSection *section,
> +                              hwaddr *start, hwaddr *delta)

Indentation is slightly wrong here.

> +{
> +    hwaddr unaligned, _start, size, _delta;

Don't use variable names with leading underscores, please.

> +
> +    unaligned = section->offset_within_address_space;
> +    size = int128_get64(section->size);
> +    _start = ROUND_UP(unaligned, qemu_real_host_page_size);
> +    _delta = _start - unaligned;
> +    size = (size - _delta) & qemu_real_host_page_mask;
> +
> +    *start = _start;
> +    *delta = _delta;
> +
> +    return size;
>  }

> -    if (area->readonly ||
> -        (!memory_region_is_ram(area) && memory_region_is_romd(area))) {
> -        flags = HV_MEMORY_READ | HV_MEMORY_EXEC;
> +        if (readonly) {
> +            slot->flags |= HVF_SLOT_READONLY;
> +        }
> +
> +        if (dirty_tracking) {
> +            slot->flags |= HVF_SLOT_LOG;
> +        }
> +
> +        /* set Hypervisor.framework memory mapping flags */
> +        if (readonly || dirty_tracking) {
> +            flags = HV_MEMORY_READ | HV_MEMORY_EXEC;
> +        } else {
> +            flags = HV_MEMORY_READ | HV_MEMORY_WRITE | HV_MEMORY_EXEC;
> +        }
> +
> +        ret = hv_vm_map(host_addr, start, size, flags);
> +        assert_hvf_ok(ret);
>      } else {
> -        flags = HV_MEMORY_READ | HV_MEMORY_WRITE | HV_MEMORY_EXEC;
> -    }
> +        /* remove memory region */
> +        slot = hvf_find_overlap_slot(start, size);
>
> -    /* Now make a new slot. */
> -    int x;
> +        if (slot) {
> +            ret = hv_vm_unmap(start, size);
> +            assert_hvf_ok(ret);
>

This 'remove memory region' logic doesn't look quite right. In the old
code, we unmap the entirety of the memory range covered by the old
slot (this happens in do_hvf_set_memory() where it calls hv_vm_unmap()
using the gpa_start and size in the macslot we're about to reuse).
In the new code, we only unmap memory covered by the start/size
we've just calculated, but we then mark the whole slot as unused.
Shouldn't we be unmapping (slot->start, slot->size) here ?

Maybe it's actually the case that we can guarantee start == slot->start
and size == slot->size ? But in that case "find an overlapping slot"
seems like the wrong operation and actually what we're doing is
finding the exact matching slot.

The rest of the logic here looks OK, I think.

-- PMM