[PATCH 12/26] hw/display/apple-gfx: Task memory mapping cleanup

[Phil Dennis-Jordan]

macOS' PV Graphics framework first requests a contiguous host-virtual
address range ("PGTask"), and subsequently requests guest-physical
memory ranges to be mapped into subranges of the tasks.

It is easier (and less threading error prone) to use the host's Mach
memory subsystem on the host to perform this subsequent remapping,
rather than tracking the mapping state directly.

Additionally, there is absolutely no need to pick a magic number
for a virtual memory base address - we can simply allocate some
appropriate virtual memory pages and let the OS choose an available
address range.

Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
---
 hw/display/apple-gfx.m  | 106 +++++++++++++++-------------------------
 hw/display/trace-events |   2 +-
 2 files changed, 40 insertions(+), 68 deletions(-)

diff --git a/hw/display/apple-gfx.m b/hw/display/apple-gfx.m
index 6537e32806..a23f731ddc 100644
--- a/hw/display/apple-gfx.m
+++ b/hw/display/apple-gfx.m
@@ -26,6 +26,7 @@
 #include "monitor/monitor.h"
 #include "qapi/error.h"
 #include "migration/blocker.h"
+#include <mach/mach_vm.h>
 #import <ParavirtualizedGraphics/ParavirtualizedGraphics.h>
 
 #define TYPE_APPLE_GFX          "apple-gfx"
@@ -35,13 +36,6 @@
     { .x = 1280, .y = 1024 },
 };
 
-/*
- * We have to map PVG memory into our address space. Use the one below
- * as base start address. In normal linker setups it points to a free
- * memory range.
- */
-#define APPLE_GFX_BASE_VA ((void *)(uintptr_t)0x500000000000UL)
-
 /*
  * ParavirtualizedGraphics.Framework only ships header files for the x86
  * variant which does not include IOSFC descriptors and host devices. We add
@@ -68,18 +62,9 @@ -(uint32_t)mmioReadAtOffset:(size_t) offset;
 -(void)mmioWriteAtOffset:(size_t) offset value:(uint32_t)value;
 @end
 
-typedef struct AppleGFXMR {
-    QTAILQ_ENTRY(AppleGFXMR) node;
-    hwaddr pa;
-    void *va;
-    uint64_t len;
-} AppleGFXMR;
-
-typedef QTAILQ_HEAD(, AppleGFXMR) AppleGFXMRList;
-
 typedef struct AppleGFXTask {
     QTAILQ_ENTRY(AppleGFXTask) node;
-    void *mem;
+    mach_vm_address_t address;
     uint64_t len;
 } AppleGFXTask;
 
@@ -95,7 +80,6 @@ -(void)mmioWriteAtOffset:(size_t) offset 
value:(uint32_t)value;
     id<PGDevice> pgdev;
     id<PGDisplay> pgdisp;
     PGIOSurfaceHostDevice *pgiosfc;
-    AppleGFXMRList mrs;
     AppleGFXTaskList tasks;
     QemuConsole *con;
     void *vram;
@@ -115,37 +99,24 @@ -(void)mmioWriteAtOffset:(size_t) offset 
value:(uint32_t)value;
 
 static AppleGFXTask *apple_gfx_new_task(AppleGFXState *s, uint64_t len)
 {
-    void *base = APPLE_GFX_BASE_VA;
+    mach_vm_address_t task_mem;
     AppleGFXTask *task;
+    kern_return_t r;
 
-    QTAILQ_FOREACH(task, &s->tasks, node) {
-        if ((task->mem + task->len) > base) {
-            base = task->mem + task->len;
-        }
+    r = mach_vm_allocate(mach_task_self(), &task_mem, len, VM_FLAGS_ANYWHERE);
+    if (r != KERN_SUCCESS || task_mem == 0) {
+        return NULL;
     }
 
     task = g_new0(AppleGFXTask, 1);
 
+    task->address = task_mem;
     task->len = len;
-    task->mem = base;
     QTAILQ_INSERT_TAIL(&s->tasks, task, node);
 
     return task;
 }
 
-static AppleGFXMR *apple_gfx_mapMemory(AppleGFXState *s, AppleGFXTask *task,
-                                       uint64_t voff, uint64_t phys, uint64_t 
len)
-{
-    AppleGFXMR *mr = g_new0(AppleGFXMR, 1);
-
-    mr->pa = phys;
-    mr->len = len;
-    mr->va = task->mem + voff;
-    QTAILQ_INSERT_TAIL(&s->mrs, mr, node);
-
-    return mr;
-}
-
 static uint64_t apple_gfx_read(void *opaque, hwaddr offset, unsigned size)
 {
     AppleGFXState *s = opaque;
@@ -411,7 +382,7 @@ static void apple_gfx_realize(DeviceState *dev, Error 
**errp)
 
     desc.createTask = ^(uint64_t vmSize, void * _Nullable * _Nonnull 
baseAddress) {
         AppleGFXTask *task = apple_gfx_new_task(s, vmSize);
-        *baseAddress = task->mem;
+        *baseAddress = (void*)task->address;
         trace_apple_gfx_create_task(vmSize, *baseAddress);
         return (PGTask_t *)task;
     };
@@ -420,60 +391,62 @@ static void apple_gfx_realize(DeviceState *dev, Error 
**errp)
         AppleGFXTask *task = (AppleGFXTask *)_task;
         trace_apple_gfx_destroy_task(task);
         QTAILQ_REMOVE(&s->tasks, task, node);
+        mach_vm_deallocate(mach_task_self(), task->address, task->len);
         g_free(task);
     };
 
     desc.mapMemory = ^(PGTask_t * _Nonnull _task, uint32_t rangeCount, 
uint64_t virtualOffset, bool readOnly, PGPhysicalMemoryRange_t * _Nonnull 
ranges) {
         AppleGFXTask *task = (AppleGFXTask*)_task;
-       mach_port_t mtask = mach_task_self();
+        kern_return_t r;
+        mach_vm_address_t target, source;
         trace_apple_gfx_map_memory(task, rangeCount, virtualOffset, readOnly);
         for (int i = 0; i < rangeCount; i++) {
             PGPhysicalMemoryRange_t *range = &ranges[i];
             MemoryRegion *tmp_mr;
             /* TODO: Bounds checks? r/o? */
             bql_lock();
-            AppleGFXMR *mr = apple_gfx_mapMemory(s, task, virtualOffset,
-                                                 range->physicalAddress,
-                                                 range->physicalLength);
 
-            trace_apple_gfx_map_memory_range(i, range->physicalAddress, 
range->physicalLength, mr->va);
+            trace_apple_gfx_map_memory_range(i, range->physicalAddress,
+                                             range->physicalLength);
 
-            vm_address_t target = (vm_address_t)mr->va;
-            uint64_t mask = 0;
-            bool anywhere = false;
-            vm_address_t source = (vm_address_t)gpa2hva(&tmp_mr, mr->pa, 
mr->len, NULL);
+            target = task->address + virtualOffset;
+            source = (mach_vm_address_t)gpa2hva(&tmp_mr,
+                                                range->physicalAddress,
+                                                range->physicalLength, NULL);
             vm_prot_t cur_protection = 0;
             vm_prot_t max_protection = 0;
-            kern_return_t retval = vm_remap(mtask, &target, mr->len, mask,
-                                            anywhere, mtask, source, false,
-                                            &cur_protection, &max_protection,
-                                            VM_INHERIT_DEFAULT);
-            trace_apple_gfx_remap(retval, source, target);
-            g_assert(retval == KERN_SUCCESS);
+            // Map guest RAM at range->physicalAddress into PG task memory 
range
+            r = mach_vm_remap(mach_task_self(),
+                              &target, range->physicalLength, vm_page_size - 1,
+                              VM_FLAGS_FIXED | VM_FLAGS_OVERWRITE,
+                              mach_task_self(),
+                              source, false /* shared mapping, no copy */,
+                              &cur_protection, &max_protection,
+                              VM_INHERIT_COPY);
+            trace_apple_gfx_remap(r, source, target);
+            g_assert(r == KERN_SUCCESS);
 
             bql_unlock();
 
-            virtualOffset += mr->len;
+            virtualOffset += range->physicalLength;
         }
         return (bool)true;
     };
 
     desc.unmapMemory = ^(PGTask_t * _Nonnull _task, uint64_t virtualOffset, 
uint64_t length) {
         AppleGFXTask *task = (AppleGFXTask *)_task;
-        AppleGFXMR *mr, *next;
+        kern_return_t r;
+        mach_vm_address_t range_address;
 
         trace_apple_gfx_unmap_memory(task, virtualOffset, length);
-        bql_lock();
-        QTAILQ_FOREACH_SAFE(mr, &s->mrs, node, next) {
-            if (mr->va >= (task->mem + virtualOffset) &&
-                (mr->va + mr->len) <= (task->mem + virtualOffset + length)) {
-                vm_address_t addr = (vm_address_t)mr->va;
-                vm_deallocate(mach_task_self(), addr, mr->len);
-                QTAILQ_REMOVE(&s->mrs, mr, node);
-                g_free(mr);
-            }
-        }
-        bql_unlock();
+
+        /* Replace task memory range with fresh pages, undoing the mapping
+         * from guest RAM. */
+        range_address = task->address + virtualOffset;
+        r = mach_vm_allocate(mach_task_self(), &range_address, length,
+                             VM_FLAGS_FIXED | VM_FLAGS_OVERWRITE);
+        g_assert(r == KERN_SUCCESS);
+
         return (bool)true;
     };
 
@@ -582,7 +555,6 @@ static void apple_gfx_realize(DeviceState *dev, Error 
**errp)
         [[PGIOSurfaceHostDevice alloc] initWithDescriptor:iosfc_desc];
     [iosfc_desc release];
 
-    QTAILQ_INIT(&s->mrs);
     QTAILQ_INIT(&s->tasks);
 
     create_fb(s);
diff --git a/hw/display/trace-events b/hw/display/trace-events
index 98e1a3a3a0..4b897554c9 100644
--- a/hw/display/trace-events
+++ b/hw/display/trace-events
@@ -198,7 +198,7 @@ apple_gfx_write(uint64_t offset, uint64_t val) 
"offset=0x%"PRIx64" val=0x%"PRIx6
 apple_gfx_create_task(uint32_t vm_size, void *va) "vm_size=0x%x base_addr=%p"
 apple_gfx_destroy_task(void *task) "task=%p"
 apple_gfx_map_memory(void *task, uint32_t range_count, uint64_t 
virtual_offset, uint32_t read_only) "task=%p range_count=0x%x 
virtual_offset=0x%"PRIx64" read_only=%d"
-apple_gfx_map_memory_range(uint32_t i, uint64_t phys_addr, uint64_t phys_len, 
void *va) "[%d] phys_addr=0x%"PRIx64" phys_len=0x%"PRIx64" va=%p"
+apple_gfx_map_memory_range(uint32_t i, uint64_t phys_addr, uint64_t phys_len) 
"[%d] phys_addr=0x%"PRIx64" phys_len=0x%"PRIx64
 apple_gfx_remap(uint64_t retval, uint64_t source, uint64_t target) 
"retval=%"PRId64" source=0x%"PRIx64" target=0x%"PRIx64
 apple_gfx_unmap_memory(void *task, uint64_t virtual_offset, uint64_t length) 
"task=%p virtual_offset=0x%"PRIx64" length=0x%"PRIx64
 apple_gfx_read_memory(uint64_t phys_address, uint64_t length, void *dst) 
"phys_addr=0x%"PRIx64" length=0x%"PRIx64" dest=%p"
-- 
2.39.3 (Apple Git-146)