(no subject)

[Phil Dennis-Jordan]

Date: Mon, 15 Jul 2024 21:07:12 +0200
Subject: [PATCH 00/26] hw/display/apple-gfx: New macOS PV Graphics device
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This sequence of patches integrates the paravirtualised graphics device
implemented by macOS's ParavirtualizedGraphics.Framework into Qemu.
Combined with the guest drivers which ship with macOS versions 11 and up,
this allows the guest OS to use the host's GPU for hardware accelerated
3D graphics, GPGPU compute (both using the 'Metal' graphics API), and
window compositing.

Some background:
----------------

The device exposed by the ParavirtualizedGraphics.Framework's (henceforth
PVG) public API consists of a PCI device with a single memory-mapped BAR;
the VMM is expected to pass reads and writes through to the framework, and
to forward interrupts emenating from it to the guest VM.

The bulk of data exchange between host and guest occurs via shared memory,
however. For this purpose, PVG makes callbacks to VMM code for allocating,
mapping, unmapping, and deallocating "task" memory ranges. Each task
represents a contiguous host virtual address range, and PVG expects the
VMM to map specific guest system memory ranges to these host addresses via
subsequent map callbacks. Multiple tasks can exist at a time, each with
many mappings.

Data is exchanged via an undocumented, Apple-proprietary protocol. The
PVG API only acts as a facilitator for establishing the communication
mechanism. This is perhaps not ideal, and among other things means it
only works on macOS hosts, but it's the only serious option we've got for
good performance and quality graphics with macOS guests at this time.

The first iterations of this PVG integration into Qemu were developed
by Alexander Graf as part of his "vmapple" machine patch series for
supporting aarch64 macOS guests, and posted to qemu-devel in June and
August 2023:

https://lore.kernel.org/all/20230830161425.91946-1-graf@amazon.com/T/

This integration mimics the "vmapple"/"apple-gfx" variant of the PVG device
used by Apple's own VMM, Virtualization.framework. This variant does not use
PCI but acts as a direct MMIO system device; there are two MMIO ranges, one
behaving identically to the PCI BAR, while the other's functionality is
exposed by private APIs in the PVG framework. It is only available on aarch64
macOS hosts.

I had prior to this simultaneously and independently developed my own PVG
integration for Qemu using the public PCI device APIs, with x86-64 and
corresponding macOS guests and hosts as the target. After some months of
use in production, I was slowly reviewing the code and readying it for
upstreaming around the time Alexander posted his vmapple patches.

I ended up reviewing the vmapple PVG code in detail; I identified a number
of issues with it (mainly thanks to my prior trial-and-error working with
the framework) but overall I thought it a better basis for refinement
than my own version:

 - It implemented the vmapple variant of the device. I thought it better to
   port the part I understood well (PCI variant) to this than trying to port
   the part I didn't understand well (MMIO vmapple variant) to my own code.
 - The code was already tidier than my own.

It also became clear in out-of-band communication that Alexander would
probably not end up having the time to see the patch through to inclusion,
and was happy for me to start making changes and to integrate my PCI code.

It's taken a while, but I'm happy with the result and think it will be a
welcome addition for anyone running macOS VMs.

What doesn't work:
------------------

 * State (de-)serialisation and thus migration. There is no fundamental
   technical obstacle to this. PVG supports saving and loading device state.
   I have simply not had the resources to implement (and crucially, test it)
   it yet.
 * Setting the list of display modes via a property is currently only
   implemented on the PCI version, which is the only one readily testable
   without the out-of-tree vmapple patches. (See review notes for patch 24)
 * End-to-end GPU-only rendering. After the host GPU has rendered the guest's
   screen, the framebuffer is copied into a system memory buffer (surface).
   When using the Qemu Cocoa UI, this buffer is drawn by the CPU into a GPU
   texture used for hardware window compositing. It would be vastly more
   efficient to retain the Metal texture and pass it directly through to the
   Cocoa window. We currently have no mechanism for doing so; it would need
   to be similar to the end-to-end OpenGL rendering support, with the added
   complication that Metal textures are Objective-C types and would need to
   traverse the plain C code of the Qemu display subsystem.
 * Dirty region detection. Similarly, the whole framebuffer is marked modified
   even if there has only been a small change. This hurts network data volume
   when using VNC.
 * Multi-head support. PVG allows "connecting" more than one virtual display.
   This integration currently always uses exactly 1 display.
 * The vmapple/aarch64 variant of the device is only testable with Alexander's
   vmapple machine type patch set. I've been maintaining this out-of-tree and
   have made a few improvements, but it doesn't yet run smoothly. (Graphics
   work fine with this code, issues are with other devices.) I can push my
   current draft to a git forge if anyone wants to test with them. I'm
   definitely hoping to eventually resolve the remaining problems and submit
   a revised version of that patch set as well.


I think we can live without these for the moment, and I'd prefer to work on
them only if and when the baseline functionality has been merged.

Patch review notes:
-------------------

I have aimed to submit the patches roughly in order of descending importance
and increasing debatability. From patch 18 it becomes usable and useful in
practice without further modification. Patches 19-23 fix issues that only occur
in certain host configurations or that can otherwise be worked around. Patch
24 is more of an RFC; patch 26 is needed if recently submitted Cocoa UI patches
end up being merged.

Brief meta-discussion of specific patches and groups of patches:

 01:    I have retained Alexander Graf's original patch intact as far as
        possible as patch 01. My only modifications are those required for
        fixing rebase conflicts.
 02:    Resolves build errors caused by upstream API changes since original
        patch submission.
 03:    This moves the device code to hw/display from its original hw/vmapple.
        With the PCI variant added later, it doesn't make much sense to put
        this inside a machine type directory.
 04-13: These patches address issues identified during code review in the
        original e-mail threads as well as my own review.
 14-15: These patches split the monolithic source file into a common core and
        vmapple/mmio specific parts.
        NOTE: checkpatch.pl complains about #ifdef __OBJC__ in the header file.
        This is needed for allowing the file to be #included from pure C.
        Ensuring that isn't currently critical, but I expect it to be useful
        in future.
 16-17: Preparation for x86-64. The x86-64 variant of PVG seems to behave
        slightly differently than the aarch64 version in terms of threading
        and control flow edge cases. We need to do some things asynchronously
        to avoid deadlock and performance problems.
 18:    The PCI variant. This builds on the split from 14/15 and the async
        changes to create the PVG PCI device which works with x86-64 macOS
        guests.
 19-23: Fixes for various additional problems and limitations. Without these
        the PVG device will only work with the Cocoa UI, on Macs which
        have an integrated (shared memory) GPU only, and mouse cursors will
        be slightly off, for example.
 24:    QOM property for specifying the display mode list (resolutions) the
        device will report to the guest. I checked other display devices and
        found none supported this, though I personally find it very useful.
        I'm wondering whether this should be a more generic feature optionally
        usable by any display device in Qemu?
 25:    Adding myself as maintainer for the PVG code, and reviewer for HVF.
 26:    Required if/when Akihiko Odaki's pending patch for removing
        dpy_cursor_define_supported is merged.

I don't know if it's useful/wise to keep these all as separate patches.
I'm happy to squash any number or groups of them. Personally, I find
smaller patches easier to review, and the git blame history acts as
a sort of documentation, so I've kept them for now.

Alexander Graf (1):
  hw/vmapple/apple-gfx: Introduce ParavirtualizedGraphics.Framework
    support

Phil Dennis-Jordan (25):
  hw/vmapple/apple-gfx: BQL renaming update
  hw/display/apple-gfx: Moved from hw/vmapple/
  hw/display/apple-gfx: uses DEFINE_TYPES macro
  hw/display/apple-gfx: native -> little endian memory ops
  hw/display/apple-gfx: Removes dead/superfluous code
  hw/display/apple-gfx: Makes set_mode thread & memory safe
  hw/display/apple-gfx: Adds migration blocker
  hw/display/apple-gfx: Wraps ObjC autorelease code in pool
  hw/display/apple-gfx: Fixes ObjC new/init misuse, plugs leaks
  hw/display/apple-gfx: Uses ObjC category extension for private
    property
  hw/display/apple-gfx: Task memory mapping cleanup
  hw/display/apple-gfx: Defines PGTask_s struct instead of casting
  hw/display/apple-gfx: Refactoring of realize function
  hw/display/apple-gfx: Separates generic & vmapple-specific
    functionality
  hw/display/apple-gfx: Asynchronous MMIO writes on x86-64
  hw/display/apple-gfx: Asynchronous rendering and graphics update
  hw/display/apple-gfx: Adds PCI implementation
  ui/cocoa: Adds non-app runloop on main thread mode
  hw/display/apple-gfx: Fixes cursor hotspot handling
  hw/display/apple-gfx: Implements texture syncing for non-UMA GPUs
  hw/display/apple-gfx: Replaces magic number with queried MMIO length
  hw/display/apple-gfx: Host GPU picking improvements
  hw/display/apple-gfx: Adds configurable mode list
  MAINTAINERS: Add myself as maintainer for apple-gfx, reviewer for HVF
  hw/display/apple-gfx: Removes UI pointer support check

 MAINTAINERS                    |   7 +
 hw/display/Kconfig             |  13 +
 hw/display/apple-gfx-pci.m     | 166 +++++++++
 hw/display/apple-gfx-vmapple.m | 194 ++++++++++
 hw/display/apple-gfx.h         |  72 ++++
 hw/display/apple-gfx.m         | 659 +++++++++++++++++++++++++++++++++
 hw/display/meson.build         |   3 +
 hw/display/trace-events        |  26 ++
 include/qemu-main.h            |   2 +
 meson.build                    |   4 +
 ui/cocoa.m                     |  15 +-
 11 files changed, 1159 insertions(+), 2 deletions(-)
 create mode 100644 hw/display/apple-gfx-pci.m
 create mode 100644 hw/display/apple-gfx-vmapple.m
 create mode 100644 hw/display/apple-gfx.h
 create mode 100644 hw/display/apple-gfx.m

-- 
2.39.3 (Apple Git-146)

[PATCH 01/26] hw/vmapple/apple-gfx: Introduce ParavirtualizedGraphics.Framework support

[Phil Dennis-Jordan]

From: Alexander Graf <graf@amazon.com>

MacOS provides a framework (library) that allows any vmm to implement a
paravirtualized 3d graphics passthrough to the host metal stack called
ParavirtualizedGraphics.Framework (PVG). The library abstracts away
almost every aspect of the paravirtualized device model and only provides
and receives callbacks on MMIO access as well as to share memory address
space between the VM and PVG.

This patch implements a QEMU device that drives PVG for the VMApple
variant of it.

Signed-off-by: Alexander Graf <graf@amazon.com>

Cherry-pick/rebase conflict fixes:
Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
---
 hw/vmapple/Kconfig      |   4 +
 hw/vmapple/apple-gfx.m  | 578 ++++++++++++++++++++++++++++++++++++++++
 hw/vmapple/meson.build  |   1 +
 hw/vmapple/trace-events |  23 ++
 meson.build             |   4 +
 5 files changed, 610 insertions(+)
 create mode 100644 hw/vmapple/Kconfig
 create mode 100644 hw/vmapple/apple-gfx.m
 create mode 100644 hw/vmapple/meson.build
 create mode 100644 hw/vmapple/trace-events

diff --git a/hw/vmapple/Kconfig b/hw/vmapple/Kconfig
new file mode 100644
index 0000000000..5835790a31
--- /dev/null
+++ b/hw/vmapple/Kconfig
@@ -0,0 +1,4 @@
+
+config VMAPPLE_PVG
+    bool
+
diff --git a/hw/vmapple/apple-gfx.m b/hw/vmapple/apple-gfx.m
new file mode 100644
index 0000000000..f75da5c610
--- /dev/null
+++ b/hw/vmapple/apple-gfx.m
@@ -0,0 +1,578 @@
+/*
+ * QEMU Apple ParavirtualizedGraphics.framework device
+ *
+ * Copyright © 2023 Amazon.com, Inc. or its affiliates. All Rights Reserved.
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ * ParavirtualizedGraphics.framework is a set of libraries that macOS provides
+ * which implements 3d graphics passthrough to the host as well as a
+ * proprietary guest communication channel to drive it. This device model
+ * implements support to drive that library from within QEMU.
+ */
+
+#include "qemu/osdep.h"
+#include "hw/irq.h"
+#include "migration/vmstate.h"
+#include "qemu/log.h"
+#include "qemu/module.h"
+#include "trace.h"
+#include "hw/sysbus.h"
+#include "hw/pci/msi.h"
+#include "crypto/hash.h"
+#include "sysemu/cpus.h"
+#include "ui/console.h"
+#include "monitor/monitor.h"
+#import <ParavirtualizedGraphics/ParavirtualizedGraphics.h>
+
+#define TYPE_APPLE_GFX          "apple-gfx"
+
+#define MAX_MRS 512
+
+static const PGDisplayCoord_t apple_gfx_modes[] = {
+    { .x = 1440, .y = 1080 },
+    { .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
+ * their definitions here so that we can also work with the ARM version.
+ */
+typedef bool(^IOSFCRaiseInterrupt)(uint32_t vector);
+typedef bool(^IOSFCUnmapMemory)(void *a, void *b, void *c, void *d, void *e, void *f);
+typedef bool(^IOSFCMapMemory)(uint64_t phys, uint64_t len, bool ro, void **va, void *e, void *f);
+
+@interface PGDeviceDescriptorExt : PGDeviceDescriptor
+@property (readwrite, nonatomic) bool usingIOSurfaceMapper;
+@end
+
+@interface PGIOSurfaceHostDeviceDescriptor : NSObject
+-(PGIOSurfaceHostDeviceDescriptor *)init;
+@property (readwrite, nonatomic, copy, nullable) IOSFCMapMemory mapMemory;
+@property (readwrite, nonatomic, copy, nullable) IOSFCUnmapMemory unmapMemory;
+@property (readwrite, nonatomic, copy, nullable) IOSFCRaiseInterrupt raiseInterrupt;
+@end
+
+@interface PGIOSurfaceHostDevice : NSObject
+-(void)initWithDescriptor:(PGIOSurfaceHostDeviceDescriptor *) desc;
+-(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;
+    uint64_t len;
+} AppleGFXTask;
+
+typedef QTAILQ_HEAD(, AppleGFXTask) AppleGFXTaskList;
+
+typedef struct AppleGFXState {
+    /* Private */
+    SysBusDevice parent_obj;
+
+    /* Public */
+    qemu_irq irq_gfx;
+    qemu_irq irq_iosfc;
+    MemoryRegion iomem_gfx;
+    MemoryRegion iomem_iosfc;
+    id<PGDevice> pgdev;
+    id<PGDisplay> pgdisp;
+    PGIOSurfaceHostDevice *pgiosfc;
+    AppleGFXMRList mrs;
+    AppleGFXTaskList tasks;
+    QemuConsole *con;
+    void *vram;
+    id<MTLDevice> mtl;
+    id<MTLTexture> texture;
+    bool handles_frames;
+    bool new_frame;
+    bool cursor_show;
+    DisplaySurface *surface;
+    QEMUCursor *cursor;
+} AppleGFXState;
+
+
+OBJECT_DECLARE_SIMPLE_TYPE(AppleGFXState, APPLE_GFX)
+
+static AppleGFXTask *apple_gfx_new_task(AppleGFXState *s, uint64_t len)
+{
+    void *base = APPLE_GFX_BASE_VA;
+    AppleGFXTask *task;
+
+    QTAILQ_FOREACH(task, &s->tasks, node) {
+        if ((task->mem + task->len) > base) {
+            base = task->mem + task->len;
+        }
+    }
+
+    task = g_new0(AppleGFXTask, 1);
+
+    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;
+    uint64_t res = 0;
+
+    switch (offset) {
+    default:
+        res = [s->pgdev mmioReadAtOffset:offset];
+        break;
+    }
+
+    trace_apple_gfx_read(offset, res);
+
+    return res;
+}
+
+static void apple_gfx_write(void *opaque, hwaddr offset, uint64_t val, unsigned size)
+{
+    AppleGFXState *s = opaque;
+
+    trace_apple_gfx_write(offset, val);
+
+    qemu_mutex_unlock_iothread();
+    [s->pgdev mmioWriteAtOffset:offset value:val];
+    qemu_mutex_lock_iothread();
+}
+
+static const MemoryRegionOps apple_gfx_ops = {
+    .read = apple_gfx_read,
+    .write = apple_gfx_write,
+    .endianness = DEVICE_NATIVE_ENDIAN,
+    .valid = {
+        .min_access_size = 4,
+        .max_access_size = 8,
+    },
+    .impl = {
+        .min_access_size = 4,
+        .max_access_size = 4,
+    },
+};
+
+static uint64_t apple_iosfc_read(void *opaque, hwaddr offset, unsigned size)
+{
+    AppleGFXState *s = opaque;
+    uint64_t res = 0;
+
+    qemu_mutex_unlock_iothread();
+    res = [s->pgiosfc mmioReadAtOffset:offset];
+    qemu_mutex_lock_iothread();
+
+    trace_apple_iosfc_read(offset, res);
+
+    return res;
+}
+
+static void apple_iosfc_write(void *opaque, hwaddr offset, uint64_t val, unsigned size)
+{
+    AppleGFXState *s = opaque;
+
+    trace_apple_iosfc_write(offset, val);
+
+    [s->pgiosfc mmioWriteAtOffset:offset value:val];
+}
+
+static const MemoryRegionOps apple_iosfc_ops = {
+    .read = apple_iosfc_read,
+    .write = apple_iosfc_write,
+    .endianness = DEVICE_NATIVE_ENDIAN,
+    .valid = {
+        .min_access_size = 4,
+        .max_access_size = 8,
+    },
+    .impl = {
+        .min_access_size = 4,
+        .max_access_size = 8,
+    },
+};
+
+static void apple_gfx_fb_update_display(void *opaque)
+{
+    AppleGFXState *s = opaque;
+
+    if (!s->new_frame || !s->handles_frames) {
+        return;
+    }
+
+    s->new_frame = false;
+
+    BOOL r;
+    uint32_t width = surface_width(s->surface);
+    uint32_t height = surface_height(s->surface);
+    MTLRegion region = MTLRegionMake2D(0, 0, width, height);
+    id<MTLCommandQueue> commandQueue = [s->mtl newCommandQueue];
+    id<MTLCommandBuffer> mipmapCommandBuffer = [commandQueue commandBuffer];
+
+    r = [s->pgdisp encodeCurrentFrameToCommandBuffer:mipmapCommandBuffer
+                                             texture:s->texture
+                                              region:region];
+
+    if (r != YES) {
+        return;
+    }
+
+    id<MTLBlitCommandEncoder> blitCommandEncoder = [mipmapCommandBuffer blitCommandEncoder];
+    [blitCommandEncoder generateMipmapsForTexture:s->texture];
+    [blitCommandEncoder endEncoding];
+    [mipmapCommandBuffer commit];
+    [mipmapCommandBuffer waitUntilCompleted];
+    [s->texture getBytes:s->vram bytesPerRow:(width * 4)
+                                 bytesPerImage: (width * height * 4)
+                                 fromRegion: region
+                                 mipmapLevel: 0
+                                 slice: 0];
+
+    /* Need to render cursor manually if not supported by backend */
+    if (!dpy_cursor_define_supported(s->con) && s->cursor && s->cursor_show) {
+        pixman_image_t *image =
+            pixman_image_create_bits(PIXMAN_a8r8g8b8,
+                                     s->cursor->width,
+                                     s->cursor->height,
+                                     (uint32_t *)s->cursor->data,
+                                     s->cursor->width * 4);
+
+        pixman_image_composite(PIXMAN_OP_OVER,
+                               image, NULL, s->surface->image,
+                               0, 0, 0, 0, s->pgdisp.cursorPosition.x,
+                               s->pgdisp.cursorPosition.y, s->cursor->width,
+                               s->cursor->height);
+
+        pixman_image_unref(image);
+    }
+
+    dpy_gfx_update_full(s->con);
+
+    [commandQueue release];
+}
+
+static const GraphicHwOps apple_gfx_fb_ops = {
+    .gfx_update = apple_gfx_fb_update_display,
+};
+
+static void update_cursor(AppleGFXState *s)
+{
+    dpy_mouse_set(s->con, s->pgdisp.cursorPosition.x, s->pgdisp.cursorPosition.y, s->cursor_show);
+
+    /* Need to render manually if cursor is not natively supported */
+    if (!dpy_cursor_define_supported(s->con)) {
+        s->new_frame = true;
+    }
+}
+
+static void set_mode(AppleGFXState *s, uint32_t width, uint32_t height)
+{
+    void *vram = g_malloc0(width * height * 4);
+    void *old_vram = s->vram;
+    DisplaySurface *surface;
+    MTLTextureDescriptor *textureDescriptor;
+    id<MTLTexture> old_texture = s->texture;
+
+    if (s->surface &&
+        width == surface_width(s->surface) &&
+        height == surface_height(s->surface)) {
+        return;
+    }
+    surface = qemu_create_displaysurface_from(width, height, PIXMAN_LE_a8r8g8b8,
+                                              width * 4, vram);
+    s->surface = surface;
+    dpy_gfx_replace_surface(s->con, surface);
+    s->vram = vram;
+    g_free(old_vram);
+
+    textureDescriptor = [MTLTextureDescriptor texture2DDescriptorWithPixelFormat:MTLPixelFormatBGRA8Unorm
+                                              width:width
+                                              height:height
+                                              mipmapped:NO];
+    textureDescriptor.usage = s->pgdisp.minimumTextureUsage;
+    s->texture = [s->mtl newTextureWithDescriptor:textureDescriptor];
+
+    if (old_texture) {
+        [old_texture release];
+    }
+}
+
+static void create_fb(AppleGFXState *s)
+{
+
+    s->con = graphic_console_init(NULL, 0, &apple_gfx_fb_ops, s);
+    set_mode(s, 1440, 1080);
+
+    s->cursor_show = true;
+}
+
+static void apple_gfx_reset(DeviceState *d)
+{
+}
+
+static void apple_gfx_init(Object *obj)
+{
+    AppleGFXState *s = APPLE_GFX(obj);
+
+    memory_region_init_io(&s->iomem_gfx, obj, &apple_gfx_ops, s, TYPE_APPLE_GFX, 0x4000);
+    memory_region_init_io(&s->iomem_iosfc, obj, &apple_iosfc_ops, s, TYPE_APPLE_GFX, 0x10000);
+    sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->iomem_gfx);
+    sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->iomem_iosfc);
+    sysbus_init_irq(SYS_BUS_DEVICE(s), &s->irq_gfx);
+    sysbus_init_irq(SYS_BUS_DEVICE(s), &s->irq_iosfc);
+}
+
+static void apple_gfx_realize(DeviceState *dev, Error **errp)
+{
+    AppleGFXState *s = APPLE_GFX(dev);
+    PGDeviceDescriptor *desc = [PGDeviceDescriptor new];
+    PGDisplayDescriptor *disp_desc = [PGDisplayDescriptor new];
+    PGIOSurfaceHostDeviceDescriptor *iosfc_desc = [PGIOSurfaceHostDeviceDescriptor new];
+    PGDeviceDescriptorExt *desc_ext = (PGDeviceDescriptorExt *)desc;
+    PGDisplayMode *modes[ARRAY_SIZE(apple_gfx_modes)];
+    int i;
+
+    for (i = 0; i < ARRAY_SIZE(apple_gfx_modes); i++) {
+        modes[i] = [PGDisplayMode new];
+        [modes[i] initWithSizeInPixels:apple_gfx_modes[i] refreshRateInHz:60.];
+    }
+
+    s->mtl = MTLCreateSystemDefaultDevice();
+
+    desc.device = s->mtl;
+    desc_ext.usingIOSurfaceMapper = true;
+
+    desc.createTask = ^(uint64_t vmSize, void * _Nullable * _Nonnull baseAddress) {
+        AppleGFXTask *task = apple_gfx_new_task(s, vmSize);
+        *baseAddress = task->mem;
+        trace_apple_gfx_create_task(vmSize, *baseAddress);
+        return (PGTask_t *)task;
+    };
+
+    desc.destroyTask = ^(PGTask_t * _Nonnull _task) {
+        AppleGFXTask *task = (AppleGFXTask *)_task;
+        trace_apple_gfx_destroy_task(task);
+        QTAILQ_REMOVE(&s->tasks, task, node);
+        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();
+        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? */
+            qemu_mutex_lock_iothread();
+            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);
+
+            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);
+            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);
+
+            qemu_mutex_unlock_iothread();
+
+            virtualOffset += mr->len;
+        }
+        return (bool)true;
+    };
+
+    desc.unmapMemory = ^(PGTask_t * _Nonnull _task, uint64_t virtualOffset, uint64_t length) {
+        AppleGFXTask *task = (AppleGFXTask *)_task;
+        AppleGFXMR *mr, *next;
+
+        trace_apple_gfx_unmap_memory(task, virtualOffset, length);
+        qemu_mutex_lock_iothread();
+        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);
+            }
+        }
+        qemu_mutex_unlock_iothread();
+        return (bool)true;
+    };
+
+    desc.readMemory = ^(uint64_t physicalAddress, uint64_t length, void * _Nonnull dst) {
+        trace_apple_gfx_read_memory(physicalAddress, length, dst);
+        cpu_physical_memory_read(physicalAddress, dst, length);
+        return (bool)true;
+    };
+
+    desc.raiseInterrupt = ^(uint32_t vector) {
+        bool locked;
+
+        trace_apple_gfx_raise_irq(vector);
+        locked = qemu_mutex_iothread_locked();
+        if (!locked) {
+            qemu_mutex_lock_iothread();
+        }
+        qemu_irq_pulse(s->irq_gfx);
+        if (!locked) {
+            qemu_mutex_unlock_iothread();
+        }
+    };
+
+    desc.addTraceRange = ^(PGPhysicalMemoryRange_t * _Nonnull range, PGTraceRangeHandler _Nonnull handler) {
+        /* Never saw this called. Return a bogus pointer so we catch access. */
+        return (PGTraceRange_t *)(void *)(uintptr_t)0x4242;
+    };
+
+    desc.removeTraceRange = ^(PGTraceRange_t * _Nonnull range) {
+        /* Never saw this called. Nothing to do. */
+    };
+    s->pgdev = PGNewDeviceWithDescriptor(desc);
+
+    [disp_desc init];
+    disp_desc.name = @"QEMU display";
+    disp_desc.sizeInMillimeters = NSMakeSize(400., 300.); /* A 20" display */
+    disp_desc.queue = dispatch_get_main_queue();
+    disp_desc.newFrameEventHandler = ^(void) {
+        trace_apple_gfx_new_frame();
+
+        /* Tell QEMU gfx stack that a new frame arrived */
+        s->handles_frames = true;
+        s->new_frame = true;
+    };
+    disp_desc.modeChangeHandler = ^(PGDisplayCoord_t sizeInPixels, OSType pixelFormat) {
+        trace_apple_gfx_mode_change(sizeInPixels.x, sizeInPixels.y);
+        set_mode(s, sizeInPixels.x, sizeInPixels.y);
+    };
+    disp_desc.cursorGlyphHandler = ^(NSBitmapImageRep *glyph, PGDisplayCoord_t hotSpot) {
+        uint32_t bpp = glyph.bitsPerPixel;
+        uint64_t width = glyph.pixelsWide;
+        uint64_t height = glyph.pixelsHigh;
+
+        trace_apple_gfx_cursor_set(bpp, width, height);
+
+        if (s->cursor) {
+            cursor_unref(s->cursor);
+        }
+        s->cursor = cursor_alloc(width, height);
+
+        /* TODO handle different bpp */
+        if (bpp == 32) {
+            memcpy(s->cursor->data, glyph.bitmapData, glyph.bytesPerPlane);
+            dpy_cursor_define(s->con, s->cursor);
+            update_cursor(s);
+        }
+    };
+    disp_desc.cursorShowHandler = ^(BOOL show) {
+        trace_apple_gfx_cursor_show(show);
+        s->cursor_show = show;
+        update_cursor(s);
+    };
+    disp_desc.cursorMoveHandler = ^(void) {
+        trace_apple_gfx_cursor_move();
+        update_cursor(s);
+    };
+
+    s->pgdisp = [s->pgdev newDisplayWithDescriptor:disp_desc port:0 serialNum:1234];
+    s->pgdisp.modeList = [NSArray arrayWithObjects:modes count:ARRAY_SIZE(apple_gfx_modes)];
+
+    [iosfc_desc init];
+    iosfc_desc.mapMemory = ^(uint64_t phys, uint64_t len, bool ro, void **va, void *e, void *f) {
+        trace_apple_iosfc_map_memory(phys, len, ro, va, e, f);
+        MemoryRegion *tmp_mr;
+        *va = gpa2hva(&tmp_mr, phys, len, NULL);
+        return (bool)true;
+    };
+
+    iosfc_desc.unmapMemory = ^(void *a, void *b, void *c, void *d, void *e, void *f) {
+        trace_apple_iosfc_unmap_memory(a, b, c, d, e, f);
+        return (bool)true;
+    };
+
+    iosfc_desc.raiseInterrupt = ^(uint32_t vector) {
+        trace_apple_iosfc_raise_irq(vector);
+        bool locked = qemu_mutex_iothread_locked();
+        if (!locked) {
+            qemu_mutex_lock_iothread();
+        }
+        qemu_irq_pulse(s->irq_iosfc);
+        if (!locked) {
+            qemu_mutex_unlock_iothread();
+        }
+        return (bool)true;
+    };
+
+    s->pgiosfc = [PGIOSurfaceHostDevice new];
+    [s->pgiosfc initWithDescriptor:iosfc_desc];
+
+    QTAILQ_INIT(&s->mrs);
+    QTAILQ_INIT(&s->tasks);
+
+    create_fb(s);
+}
+
+static void apple_gfx_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->reset = apple_gfx_reset;
+    dc->realize = apple_gfx_realize;
+}
+
+static TypeInfo apple_gfx_info = {
+    .name          = TYPE_APPLE_GFX,
+    .parent        = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(AppleGFXState),
+    .class_init    = apple_gfx_class_init,
+    .instance_init = apple_gfx_init,
+};
+
+static void apple_gfx_register_types(void)
+{
+    type_register_static(&apple_gfx_info);
+}
+
+type_init(apple_gfx_register_types)
diff --git a/hw/vmapple/meson.build b/hw/vmapple/meson.build
new file mode 100644
index 0000000000..f9ab2194d9
--- /dev/null
+++ b/hw/vmapple/meson.build
@@ -0,0 +1 @@
+system_ss.add(when: 'CONFIG_VMAPPLE_PVG',  if_true: [files('apple-gfx.m'), pvg, metal])
diff --git a/hw/vmapple/trace-events b/hw/vmapple/trace-events
new file mode 100644
index 0000000000..497f64064b
--- /dev/null
+++ b/hw/vmapple/trace-events
@@ -0,0 +1,23 @@
+# See docs/devel/tracing.rst for syntax documentation.
+
+# apple-gfx.m
+apple_gfx_read(uint64_t offset, uint64_t res) "offset=0x%"PRIx64" res=0x%"PRIx64
+apple_gfx_write(uint64_t offset, uint64_t val) "offset=0x%"PRIx64" val=0x%"PRIx64
+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_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"
+apple_gfx_raise_irq(uint32_t vector) "vector=0x%x"
+apple_gfx_new_frame(void) ""
+apple_gfx_mode_change(uint64_t x, uint64_t y) "x=%"PRId64" y=%"PRId64
+apple_gfx_cursor_set(uint32_t bpp, uint64_t width, uint64_t height) "bpp=%d width=%"PRId64" height=0x%"PRId64
+apple_gfx_cursor_show(uint32_t show) "show=%d"
+apple_gfx_cursor_move(void) ""
+apple_iosfc_read(uint64_t offset, uint64_t res) "offset=0x%"PRIx64" res=0x%"PRIx64
+apple_iosfc_write(uint64_t offset, uint64_t val) "offset=0x%"PRIx64" val=0x%"PRIx64
+apple_iosfc_map_memory(uint64_t phys, uint64_t len, uint32_t ro, void *va, void *e, void *f) "phys=0x%"PRIx64" len=0x%"PRIx64" ro=%d va=%p e=%p f=%p"
+apple_iosfc_unmap_memory(void *a, void *b, void *c, void *d, void *e, void *f) "a=%p b=%p c=%p d=%p e=%p f=%p"
+apple_iosfc_raise_irq(uint32_t vector) "vector=0x%x"
diff --git a/meson.build b/meson.build
index 6a93da48e1..fd00a6c0d5 100644
--- a/meson.build
+++ b/meson.build
@@ -710,6 +710,8 @@ socket = []
 version_res = []
 coref = []
 iokit = []
+pvg = []
+metal = []
 emulator_link_args = []
 midl = not_found
 widl = not_found
@@ -731,6 +733,8 @@ elif host_os == 'darwin'
   coref = dependency('appleframeworks', modules: 'CoreFoundation')
   iokit = dependency('appleframeworks', modules: 'IOKit', required: false)
   host_dsosuf = '.dylib'
+  pvg = dependency('appleframeworks', modules: 'ParavirtualizedGraphics')
+  metal = dependency('appleframeworks', modules: 'Metal')
 elif host_os == 'sunos'
   socket = [cc.find_library('socket'),
             cc.find_library('nsl'),
-- 
2.39.3 (Apple Git-146)

[PATCH 02/26] hw/vmapple/apple-gfx: BQL renaming update

[Phil Dennis-Jordan]

Since the original apple-gfx code was written, the BQL functions have
been renamed. This change updates the apple-gfx code to use the new
names.

Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
---
 hw/vmapple/apple-gfx.m | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/hw/vmapple/apple-gfx.m b/hw/vmapple/apple-gfx.m
index f75da5c610..3b437e2519 100644
--- a/hw/vmapple/apple-gfx.m
+++ b/hw/vmapple/apple-gfx.m
@@ -168,9 +168,9 @@ static void apple_gfx_write(void *opaque, hwaddr offset, uint64_t val, unsigned
 
     trace_apple_gfx_write(offset, val);
 
-    qemu_mutex_unlock_iothread();
+    bql_unlock();
     [s->pgdev mmioWriteAtOffset:offset value:val];
-    qemu_mutex_lock_iothread();
+    bql_lock();
 }
 
 static const MemoryRegionOps apple_gfx_ops = {
@@ -192,9 +192,9 @@ static uint64_t apple_iosfc_read(void *opaque, hwaddr offset, unsigned size)
     AppleGFXState *s = opaque;
     uint64_t res = 0;
 
-    qemu_mutex_unlock_iothread();
+    bql_unlock();
     res = [s->pgiosfc mmioReadAtOffset:offset];
-    qemu_mutex_lock_iothread();
+    bql_lock();
 
     trace_apple_iosfc_read(offset, res);
 
@@ -396,7 +396,7 @@ static void apple_gfx_realize(DeviceState *dev, Error **errp)
             PGPhysicalMemoryRange_t *range = &ranges[i];
             MemoryRegion *tmp_mr;
             /* TODO: Bounds checks? r/o? */
-            qemu_mutex_lock_iothread();
+            bql_lock();
             AppleGFXMR *mr = apple_gfx_mapMemory(s, task, virtualOffset,
                                                  range->physicalAddress,
                                                  range->physicalLength);
@@ -416,7 +416,7 @@ static void apple_gfx_realize(DeviceState *dev, Error **errp)
             trace_apple_gfx_remap(retval, source, target);
             g_assert(retval == KERN_SUCCESS);
 
-            qemu_mutex_unlock_iothread();
+            bql_unlock();
 
             virtualOffset += mr->len;
         }
@@ -428,7 +428,7 @@ static void apple_gfx_realize(DeviceState *dev, Error **errp)
         AppleGFXMR *mr, *next;
 
         trace_apple_gfx_unmap_memory(task, virtualOffset, length);
-        qemu_mutex_lock_iothread();
+        bql_lock();
         QTAILQ_FOREACH_SAFE(mr, &s->mrs, node, next) {
             if (mr->va >= (task->mem + virtualOffset) &&
                 (mr->va + mr->len) <= (task->mem + virtualOffset + length)) {
@@ -438,7 +438,7 @@ static void apple_gfx_realize(DeviceState *dev, Error **errp)
                 g_free(mr);
             }
         }
-        qemu_mutex_unlock_iothread();
+        bql_unlock();
         return (bool)true;
     };
 
@@ -452,13 +452,13 @@ static void apple_gfx_realize(DeviceState *dev, Error **errp)
         bool locked;
 
         trace_apple_gfx_raise_irq(vector);
-        locked = qemu_mutex_iothread_locked();
+        locked = bql_locked();
         if (!locked) {
-            qemu_mutex_lock_iothread();
+            bql_lock();
         }
         qemu_irq_pulse(s->irq_gfx);
         if (!locked) {
-            qemu_mutex_unlock_iothread();
+            bql_unlock();
         }
     };
 
@@ -534,13 +534,13 @@ static void apple_gfx_realize(DeviceState *dev, Error **errp)
 
     iosfc_desc.raiseInterrupt = ^(uint32_t vector) {
         trace_apple_iosfc_raise_irq(vector);
-        bool locked = qemu_mutex_iothread_locked();
+        bool locked = bql_locked();
         if (!locked) {
-            qemu_mutex_lock_iothread();
+            bql_lock();
         }
         qemu_irq_pulse(s->irq_iosfc);
         if (!locked) {
-            qemu_mutex_unlock_iothread();
+            bql_unlock();
         }
         return (bool)true;
     };
-- 
2.39.3 (Apple Git-146)

[PATCH 03/26] hw/display/apple-gfx: Moved from hw/vmapple/

[Phil Dennis-Jordan]

The macOS PV graphics device is useful outside the (as yet incomplete)
vmapple machine type, so this patch moves it to hw/display.

Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
---
 hw/display/Kconfig                  |  4 ++++
 hw/{vmapple => display}/apple-gfx.m |  0
 hw/display/meson.build              |  1 +
 hw/display/trace-events             | 23 +++++++++++++++++++++++
 hw/vmapple/Kconfig                  |  4 ----
 hw/vmapple/meson.build              |  1 -
 hw/vmapple/trace-events             | 23 -----------------------
 7 files changed, 28 insertions(+), 28 deletions(-)
 rename hw/{vmapple => display}/apple-gfx.m (100%)
 delete mode 100644 hw/vmapple/Kconfig
 delete mode 100644 hw/vmapple/meson.build
 delete mode 100644 hw/vmapple/trace-events

diff --git a/hw/display/Kconfig b/hw/display/Kconfig
index a4552c8ed7..13cd256c06 100644
--- a/hw/display/Kconfig
+++ b/hw/display/Kconfig
@@ -143,3 +143,7 @@ config XLNX_DISPLAYPORT
 
 config DM163
     bool
+
+config MAC_PVG
+    bool
+
diff --git a/hw/vmapple/apple-gfx.m b/hw/display/apple-gfx.m
similarity index 100%
rename from hw/vmapple/apple-gfx.m
rename to hw/display/apple-gfx.m
diff --git a/hw/display/meson.build b/hw/display/meson.build
index 7db05eace9..713786bd07 100644
--- a/hw/display/meson.build
+++ b/hw/display/meson.build
@@ -65,6 +65,7 @@ system_ss.add(when: 'CONFIG_ARTIST', if_true: files('artist.c'))
 
 system_ss.add(when: 'CONFIG_ATI_VGA', if_true: [files('ati.c', 'ati_2d.c', 'ati_dbg.c'), pixman])
 
+system_ss.add(when: 'CONFIG_MAC_PVG', if_true: [files('apple-gfx.m'), pvg, metal])
 
 if config_all_devices.has_key('CONFIG_VIRTIO_GPU')
   virtio_gpu_ss = ss.source_set()
diff --git a/hw/display/trace-events b/hw/display/trace-events
index 781f8a3320..98e1a3a3a0 100644
--- a/hw/display/trace-events
+++ b/hw/display/trace-events
@@ -191,3 +191,26 @@ dm163_bits_ppi(unsigned dest_width) "dest_width : %u"
 dm163_leds(int led, uint32_t value) "led %d: 0x%x"
 dm163_channels(int channel, uint8_t value) "channel %d: 0x%x"
 dm163_refresh_rate(uint32_t rr) "refresh rate %d"
+
+# apple-gfx.m
+apple_gfx_read(uint64_t offset, uint64_t res) "offset=0x%"PRIx64" res=0x%"PRIx64
+apple_gfx_write(uint64_t offset, uint64_t val) "offset=0x%"PRIx64" val=0x%"PRIx64
+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_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"
+apple_gfx_raise_irq(uint32_t vector) "vector=0x%x"
+apple_gfx_new_frame(void) ""
+apple_gfx_mode_change(uint64_t x, uint64_t y) "x=%"PRId64" y=%"PRId64
+apple_gfx_cursor_set(uint32_t bpp, uint64_t width, uint64_t height) "bpp=%d width=%"PRId64" height=0x%"PRId64
+apple_gfx_cursor_show(uint32_t show) "show=%d"
+apple_gfx_cursor_move(void) ""
+apple_iosfc_read(uint64_t offset, uint64_t res) "offset=0x%"PRIx64" res=0x%"PRIx64
+apple_iosfc_write(uint64_t offset, uint64_t val) "offset=0x%"PRIx64" val=0x%"PRIx64
+apple_iosfc_map_memory(uint64_t phys, uint64_t len, uint32_t ro, void *va, void *e, void *f) "phys=0x%"PRIx64" len=0x%"PRIx64" ro=%d va=%p e=%p f=%p"
+apple_iosfc_unmap_memory(void *a, void *b, void *c, void *d, void *e, void *f) "a=%p b=%p c=%p d=%p e=%p f=%p"
+apple_iosfc_raise_irq(uint32_t vector) "vector=0x%x"
+
diff --git a/hw/vmapple/Kconfig b/hw/vmapple/Kconfig
deleted file mode 100644
index 5835790a31..0000000000
--- a/hw/vmapple/Kconfig
+++ /dev/null
@@ -1,4 +0,0 @@
-
-config VMAPPLE_PVG
-    bool
-
diff --git a/hw/vmapple/meson.build b/hw/vmapple/meson.build
deleted file mode 100644
index f9ab2194d9..0000000000
--- a/hw/vmapple/meson.build
+++ /dev/null
@@ -1 +0,0 @@
-system_ss.add(when: 'CONFIG_VMAPPLE_PVG',  if_true: [files('apple-gfx.m'), pvg, metal])
diff --git a/hw/vmapple/trace-events b/hw/vmapple/trace-events
deleted file mode 100644
index 497f64064b..0000000000
--- a/hw/vmapple/trace-events
+++ /dev/null
@@ -1,23 +0,0 @@
-# See docs/devel/tracing.rst for syntax documentation.
-
-# apple-gfx.m
-apple_gfx_read(uint64_t offset, uint64_t res) "offset=0x%"PRIx64" res=0x%"PRIx64
-apple_gfx_write(uint64_t offset, uint64_t val) "offset=0x%"PRIx64" val=0x%"PRIx64
-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_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"
-apple_gfx_raise_irq(uint32_t vector) "vector=0x%x"
-apple_gfx_new_frame(void) ""
-apple_gfx_mode_change(uint64_t x, uint64_t y) "x=%"PRId64" y=%"PRId64
-apple_gfx_cursor_set(uint32_t bpp, uint64_t width, uint64_t height) "bpp=%d width=%"PRId64" height=0x%"PRId64
-apple_gfx_cursor_show(uint32_t show) "show=%d"
-apple_gfx_cursor_move(void) ""
-apple_iosfc_read(uint64_t offset, uint64_t res) "offset=0x%"PRIx64" res=0x%"PRIx64
-apple_iosfc_write(uint64_t offset, uint64_t val) "offset=0x%"PRIx64" val=0x%"PRIx64
-apple_iosfc_map_memory(uint64_t phys, uint64_t len, uint32_t ro, void *va, void *e, void *f) "phys=0x%"PRIx64" len=0x%"PRIx64" ro=%d va=%p e=%p f=%p"
-apple_iosfc_unmap_memory(void *a, void *b, void *c, void *d, void *e, void *f) "a=%p b=%p c=%p d=%p e=%p f=%p"
-apple_iosfc_raise_irq(uint32_t vector) "vector=0x%x"
-- 
2.39.3 (Apple Git-146)

[PATCH 04/26] hw/display/apple-gfx: uses DEFINE_TYPES macro

[Phil Dennis-Jordan]

Switches the device definition to the more modern macro variants.

Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
---
 hw/display/apple-gfx.m | 23 +++++++++--------------
 1 file changed, 9 insertions(+), 14 deletions(-)

diff --git a/hw/display/apple-gfx.m b/hw/display/apple-gfx.m
index 3b437e2519..87bcdcd98e 100644
--- a/hw/display/apple-gfx.m
+++ b/hw/display/apple-gfx.m
@@ -86,10 +86,8 @@ -(void)mmioWriteAtOffset:(size_t) offset value:(uint32_t)value;
 typedef QTAILQ_HEAD(, AppleGFXTask) AppleGFXTaskList;
 
 typedef struct AppleGFXState {
-    /* Private */
     SysBusDevice parent_obj;
 
-    /* Public */
     qemu_irq irq_gfx;
     qemu_irq irq_iosfc;
     MemoryRegion iomem_gfx;
@@ -562,17 +560,14 @@ static void apple_gfx_class_init(ObjectClass *klass, void *data)
     dc->realize = apple_gfx_realize;
 }
 
-static TypeInfo apple_gfx_info = {
-    .name          = TYPE_APPLE_GFX,
-    .parent        = TYPE_SYS_BUS_DEVICE,
-    .instance_size = sizeof(AppleGFXState),
-    .class_init    = apple_gfx_class_init,
-    .instance_init = apple_gfx_init,
+static TypeInfo apple_gfx_types[] = {
+    {
+        .name          = TYPE_APPLE_GFX,
+        .parent        = TYPE_SYS_BUS_DEVICE,
+        .instance_size = sizeof(AppleGFXState),
+        .class_init    = apple_gfx_class_init,
+        .instance_init = apple_gfx_init,
+    }
 };
 
-static void apple_gfx_register_types(void)
-{
-    type_register_static(&apple_gfx_info);
-}
-
-type_init(apple_gfx_register_types)
+DEFINE_TYPES(apple_gfx_types)
-- 
2.39.3 (Apple Git-146)

[PATCH 05/26] hw/display/apple-gfx: native -> little endian memory ops

[Phil Dennis-Jordan]

This is considered best practice.

Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
---
 hw/display/apple-gfx.m | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/display/apple-gfx.m b/hw/display/apple-gfx.m
index 87bcdcd98e..8b0459f969 100644
--- a/hw/display/apple-gfx.m
+++ b/hw/display/apple-gfx.m
@@ -174,7 +174,7 @@ static void apple_gfx_write(void *opaque, hwaddr offset, uint64_t val, unsigned
 static const MemoryRegionOps apple_gfx_ops = {
     .read = apple_gfx_read,
     .write = apple_gfx_write,
-    .endianness = DEVICE_NATIVE_ENDIAN,
+    .endianness = DEVICE_LITTLE_ENDIAN,
     .valid = {
         .min_access_size = 4,
         .max_access_size = 8,
-- 
2.39.3 (Apple Git-146)

[PATCH 06/26] hw/display/apple-gfx: Removes dead/superfluous code

[Phil Dennis-Jordan]

This removes a few chunks of code that do nothing useful.

 * MAX_MRS was not used anywhere.
 * The MMIO offset switch block had only a default case.
 * Generating mip maps for the texture is a waste of time as the
   texture is only used for read-out to a system memory buffer.
 * The trace range callbacks are used only when the device is
   handled by the (2D-only) UEFI guest driver, not by the macOS
   guest driver, hence the lack of observed use in the vmapple
   machine which does not use UEFI. Making these no-ops could
   potentially be harmful, whereas leaving them unimplemented
   is explicitly supported according to the framework header
   docs.

Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
---
 hw/display/apple-gfx.m | 17 +----------------
 1 file changed, 1 insertion(+), 16 deletions(-)

diff --git a/hw/display/apple-gfx.m b/hw/display/apple-gfx.m
index 8b0459f969..b10c060d9a 100644
--- a/hw/display/apple-gfx.m
+++ b/hw/display/apple-gfx.m
@@ -28,8 +28,6 @@
 
 #define TYPE_APPLE_GFX          "apple-gfx"
 
-#define MAX_MRS 512
-
 static const PGDisplayCoord_t apple_gfx_modes[] = {
     { .x = 1440, .y = 1080 },
     { .x = 1280, .y = 1024 },
@@ -149,11 +147,7 @@ static uint64_t apple_gfx_read(void *opaque, hwaddr offset, unsigned size)
     AppleGFXState *s = opaque;
     uint64_t res = 0;
 
-    switch (offset) {
-    default:
-        res = [s->pgdev mmioReadAtOffset:offset];
-        break;
-    }
+    res = [s->pgdev mmioReadAtOffset:offset];
 
     trace_apple_gfx_read(offset, res);
 
@@ -248,7 +242,6 @@ static void apple_gfx_fb_update_display(void *opaque)
     }
 
     id<MTLBlitCommandEncoder> blitCommandEncoder = [mipmapCommandBuffer blitCommandEncoder];
-    [blitCommandEncoder generateMipmapsForTexture:s->texture];
     [blitCommandEncoder endEncoding];
     [mipmapCommandBuffer commit];
     [mipmapCommandBuffer waitUntilCompleted];
@@ -460,14 +453,6 @@ static void apple_gfx_realize(DeviceState *dev, Error **errp)
         }
     };
 
-    desc.addTraceRange = ^(PGPhysicalMemoryRange_t * _Nonnull range, PGTraceRangeHandler _Nonnull handler) {
-        /* Never saw this called. Return a bogus pointer so we catch access. */
-        return (PGTraceRange_t *)(void *)(uintptr_t)0x4242;
-    };
-
-    desc.removeTraceRange = ^(PGTraceRange_t * _Nonnull range) {
-        /* Never saw this called. Nothing to do. */
-    };
     s->pgdev = PGNewDeviceWithDescriptor(desc);
 
     [disp_desc init];
-- 
2.39.3 (Apple Git-146)

[PATCH 07/26] hw/display/apple-gfx: Makes set_mode thread & memory safe

[Phil Dennis-Jordan]

When the set_mode callback was invoked outside of the BQL, there
could be a race condition swapping out the resized render target
texture and VRAM. set_mode may be called inside or out of the
BQL depending on context (reentrant from a MMIO write or not)
so we need to check locking state first.

Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
---
 hw/display/apple-gfx.m | 54 +++++++++++++++++++++++++++++++-----------
 1 file changed, 40 insertions(+), 14 deletions(-)

diff --git a/hw/display/apple-gfx.m b/hw/display/apple-gfx.m
index b10c060d9a..39aba8d143 100644
--- a/hw/display/apple-gfx.m
+++ b/hw/display/apple-gfx.m
@@ -290,34 +290,60 @@ static void update_cursor(AppleGFXState *s)
 
 static void set_mode(AppleGFXState *s, uint32_t width, uint32_t height)
 {
-    void *vram = g_malloc0(width * height * 4);
+    void *vram = NULL;
     void *old_vram = s->vram;
     DisplaySurface *surface;
     MTLTextureDescriptor *textureDescriptor;
-    id<MTLTexture> old_texture = s->texture;
+    id<MTLTexture> old_texture = nil;
+    id<MTLTexture> texture = nil;
+    bool locking_required = false;
 
+    locking_required = !bql_locked();
+    if (locking_required) {
+        bql_lock();
+    }
     if (s->surface &&
         width == surface_width(s->surface) &&
         height == surface_height(s->surface)) {
+        if (locking_required) {
+            bql_unlock();
+        }
         return;
     }
+    if (locking_required) {
+        bql_unlock();
+    }
+
+    vram = g_malloc0(width * height * 4);
     surface = qemu_create_displaysurface_from(width, height, PIXMAN_LE_a8r8g8b8,
                                               width * 4, vram);
-    s->surface = surface;
-    dpy_gfx_replace_surface(s->con, surface);
-    s->vram = vram;
-    g_free(old_vram);
 
-    textureDescriptor = [MTLTextureDescriptor texture2DDescriptorWithPixelFormat:MTLPixelFormatBGRA8Unorm
-                                              width:width
-                                              height:height
-                                              mipmapped:NO];
-    textureDescriptor.usage = s->pgdisp.minimumTextureUsage;
-    s->texture = [s->mtl newTextureWithDescriptor:textureDescriptor];
+    @autoreleasepool {
+        textureDescriptor =
+            [MTLTextureDescriptor
+                texture2DDescriptorWithPixelFormat:MTLPixelFormatBGRA8Unorm
+                                             width:width
+                                            height:height
+                                         mipmapped:NO];
+        textureDescriptor.usage = s->pgdisp.minimumTextureUsage;
+        texture = [s->mtl newTextureWithDescriptor:textureDescriptor];
+    }
 
-    if (old_texture) {
-        [old_texture release];
+    if (locking_required) {
+        bql_lock();
+    }
+    old_vram = s->vram;
+    s->vram = vram;
+    s->surface = surface;
+    dpy_gfx_replace_surface(s->con, surface);
+    old_texture = s->texture;
+    s->texture = texture;
+    if (locking_required) {
+        bql_unlock();
     }
+
+    g_free(old_vram);
+    [old_texture release];
 }
 
 static void create_fb(AppleGFXState *s)
-- 
2.39.3 (Apple Git-146)

[PATCH 08/26] hw/display/apple-gfx: Adds migration blocker

[Phil Dennis-Jordan]

Although the underlying ParavirtualizedGraphics.framework does support
state (de-)serialisation, this is currently not yet integrated. We
therefore add a migration blocker for now.

Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
---
 hw/display/apple-gfx.m | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/hw/display/apple-gfx.m b/hw/display/apple-gfx.m
index 39aba8d143..c97bd40cb5 100644
--- a/hw/display/apple-gfx.m
+++ b/hw/display/apple-gfx.m
@@ -24,6 +24,8 @@
 #include "sysemu/cpus.h"
 #include "ui/console.h"
 #include "monitor/monitor.h"
+#include "qapi/error.h"
+#include "migration/blocker.h"
 #import <ParavirtualizedGraphics/ParavirtualizedGraphics.h>
 
 #define TYPE_APPLE_GFX          "apple-gfx"
@@ -109,6 +111,8 @@ -(void)mmioWriteAtOffset:(size_t) offset value:(uint32_t)value;
 
 OBJECT_DECLARE_SIMPLE_TYPE(AppleGFXState, APPLE_GFX)
 
+static Error *apple_gfx_mig_blocker;
+
 static AppleGFXTask *apple_gfx_new_task(AppleGFXState *s, uint64_t len)
 {
     void *base = APPLE_GFX_BASE_VA;
@@ -362,6 +366,8 @@ static void apple_gfx_reset(DeviceState *d)
 static void apple_gfx_init(Object *obj)
 {
     AppleGFXState *s = APPLE_GFX(obj);
+    Error *local_err = NULL;
+    int r;
 
     memory_region_init_io(&s->iomem_gfx, obj, &apple_gfx_ops, s, TYPE_APPLE_GFX, 0x4000);
     memory_region_init_io(&s->iomem_iosfc, obj, &apple_iosfc_ops, s, TYPE_APPLE_GFX, 0x10000);
@@ -369,6 +375,16 @@ static void apple_gfx_init(Object *obj)
     sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->iomem_iosfc);
     sysbus_init_irq(SYS_BUS_DEVICE(s), &s->irq_gfx);
     sysbus_init_irq(SYS_BUS_DEVICE(s), &s->irq_iosfc);
+
+    /* TODO: PVG framework supports serialising device state: integrate it! */
+    if (apple_gfx_mig_blocker == NULL) {
+        error_setg(&apple_gfx_mig_blocker,
+                  "Migration state blocked by apple-gfx display device");
+        r = migrate_add_blocker(&apple_gfx_mig_blocker, &local_err);
+        if (r < 0) {
+            error_report_err(local_err);
+        }
+    }
 }
 
 static void apple_gfx_realize(DeviceState *dev, Error **errp)
-- 
2.39.3 (Apple Git-146)

[PATCH 09/26] hw/display/apple-gfx: Wraps ObjC autorelease code in pool

[Phil Dennis-Jordan]

Most Objective-C methods return objects with autorelease semantics.
These objects will have their retain count decremented once control
exits the scope of the current autorelease pool. A number of areas
in the code call such methods with autorelease semantics, but can't
make any guarantees about the presence of a surrounding pool.

We therefore wrap some code in explicit pool scopes to prevent
leaks.

Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
---
 hw/display/apple-gfx.m | 88 ++++++++++++++++++++++--------------------
 1 file changed, 46 insertions(+), 42 deletions(-)

diff --git a/hw/display/apple-gfx.m b/hw/display/apple-gfx.m
index c97bd40cb5..7f0a231d26 100644
--- a/hw/display/apple-gfx.m
+++ b/hw/display/apple-gfx.m
@@ -228,54 +228,56 @@ static void apple_gfx_fb_update_display(void *opaque)
         return;
     }
 
-    s->new_frame = false;
+    @autoreleasepool {
+        s->new_frame = false;
 
-    BOOL r;
-    uint32_t width = surface_width(s->surface);
-    uint32_t height = surface_height(s->surface);
-    MTLRegion region = MTLRegionMake2D(0, 0, width, height);
-    id<MTLCommandQueue> commandQueue = [s->mtl newCommandQueue];
-    id<MTLCommandBuffer> mipmapCommandBuffer = [commandQueue commandBuffer];
+        BOOL r;
+        uint32_t width = surface_width(s->surface);
+        uint32_t height = surface_height(s->surface);
+        MTLRegion region = MTLRegionMake2D(0, 0, width, height);
+        id<MTLCommandQueue> commandQueue = [s->mtl newCommandQueue];
+        id<MTLCommandBuffer> mipmapCommandBuffer = [commandQueue commandBuffer];
 
-    r = [s->pgdisp encodeCurrentFrameToCommandBuffer:mipmapCommandBuffer
-                                             texture:s->texture
-                                              region:region];
+        r = [s->pgdisp encodeCurrentFrameToCommandBuffer:mipmapCommandBuffer
+                                                 texture:s->texture
+                                                  region:region];
 
-    if (r != YES) {
-        return;
-    }
+        if (r != YES) {
+            return;
+        }
 
-    id<MTLBlitCommandEncoder> blitCommandEncoder = [mipmapCommandBuffer blitCommandEncoder];
-    [blitCommandEncoder endEncoding];
-    [mipmapCommandBuffer commit];
-    [mipmapCommandBuffer waitUntilCompleted];
-    [s->texture getBytes:s->vram bytesPerRow:(width * 4)
-                                 bytesPerImage: (width * height * 4)
-                                 fromRegion: region
-                                 mipmapLevel: 0
-                                 slice: 0];
-
-    /* Need to render cursor manually if not supported by backend */
-    if (!dpy_cursor_define_supported(s->con) && s->cursor && s->cursor_show) {
-        pixman_image_t *image =
-            pixman_image_create_bits(PIXMAN_a8r8g8b8,
-                                     s->cursor->width,
-                                     s->cursor->height,
-                                     (uint32_t *)s->cursor->data,
-                                     s->cursor->width * 4);
-
-        pixman_image_composite(PIXMAN_OP_OVER,
-                               image, NULL, s->surface->image,
-                               0, 0, 0, 0, s->pgdisp.cursorPosition.x,
-                               s->pgdisp.cursorPosition.y, s->cursor->width,
-                               s->cursor->height);
-
-        pixman_image_unref(image);
-    }
+        id<MTLBlitCommandEncoder> blitCommandEncoder = [mipmapCommandBuffer blitCommandEncoder];
+        [blitCommandEncoder endEncoding];
+        [mipmapCommandBuffer commit];
+        [mipmapCommandBuffer waitUntilCompleted];
+        [s->texture getBytes:s->vram bytesPerRow:(width * 4)
+                                     bytesPerImage: (width * height * 4)
+                                     fromRegion: region
+                                     mipmapLevel: 0
+                                     slice: 0];
+
+        /* Need to render cursor manually if not supported by backend */
+        if (!dpy_cursor_define_supported(s->con) && s->cursor && s->cursor_show) {
+            pixman_image_t *image =
+                pixman_image_create_bits(PIXMAN_a8r8g8b8,
+                                         s->cursor->width,
+                                         s->cursor->height,
+                                         (uint32_t *)s->cursor->data,
+                                         s->cursor->width * 4);
+
+            pixman_image_composite(PIXMAN_OP_OVER,
+                                   image, NULL, s->surface->image,
+                                   0, 0, 0, 0, s->pgdisp.cursorPosition.x,
+                                   s->pgdisp.cursorPosition.y, s->cursor->width,
+                                   s->cursor->height);
+
+            pixman_image_unref(image);
+        }
 
-    dpy_gfx_update_full(s->con);
+        dpy_gfx_update_full(s->con);
 
-    [commandQueue release];
+        [commandQueue release];
+    }
 }
 
 static const GraphicHwOps apple_gfx_fb_ops = {
@@ -389,6 +391,7 @@ static void apple_gfx_init(Object *obj)
 
 static void apple_gfx_realize(DeviceState *dev, Error **errp)
 {
+    @autoreleasepool {
     AppleGFXState *s = APPLE_GFX(dev);
     PGDeviceDescriptor *desc = [PGDeviceDescriptor new];
     PGDisplayDescriptor *disp_desc = [PGDisplayDescriptor new];
@@ -577,6 +580,7 @@ static void apple_gfx_realize(DeviceState *dev, Error **errp)
     QTAILQ_INIT(&s->tasks);
 
     create_fb(s);
+    }
 }
 
 static void apple_gfx_class_init(ObjectClass *klass, void *data)
-- 
2.39.3 (Apple Git-146)

[PATCH 10/26] hw/display/apple-gfx: Fixes ObjC new/init misuse, plugs leaks

[Phil Dennis-Jordan]

The creation of Objective-C objects is split between 'alloc' and
'init...' methods. The single 'new' method is shorthand for alloc+init.
Calling 'new' then 'init...' is therefore redundant. The instances of
new + initWith... are replaced with the alloc/initWith... pattern and
the zero-argument 'init' call is removed from instances of the
new + init pattern.

'new' and alloc+init return an object with a retain count of 1. The
change therefore also inserts release calls for temporary objects
created in this way to prevent them leaking.

Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
---
 hw/display/apple-gfx.m | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/hw/display/apple-gfx.m b/hw/display/apple-gfx.m
index 7f0a231d26..073741ede5 100644
--- a/hw/display/apple-gfx.m
+++ b/hw/display/apple-gfx.m
@@ -63,7 +63,7 @@ -(PGIOSurfaceHostDeviceDescriptor *)init;
 @end
 
 @interface PGIOSurfaceHostDevice : NSObject
--(void)initWithDescriptor:(PGIOSurfaceHostDeviceDescriptor *) desc;
+-(instancetype)initWithDescriptor:(PGIOSurfaceHostDeviceDescriptor *) desc;
 -(uint32_t)mmioReadAtOffset:(size_t) offset;
 -(void)mmioWriteAtOffset:(size_t) offset value:(uint32_t)value;
 @end
@@ -401,8 +401,8 @@ static void apple_gfx_realize(DeviceState *dev, Error **errp)
     int i;
 
     for (i = 0; i < ARRAY_SIZE(apple_gfx_modes); i++) {
-        modes[i] = [PGDisplayMode new];
-        [modes[i] initWithSizeInPixels:apple_gfx_modes[i] refreshRateInHz:60.];
+        modes[i] =
+            [[PGDisplayMode alloc] initWithSizeInPixels:apple_gfx_modes[i] refreshRateInHz:60.];
     }
 
     s->mtl = MTLCreateSystemDefaultDevice();
@@ -499,8 +499,9 @@ static void apple_gfx_realize(DeviceState *dev, Error **errp)
     };
 
     s->pgdev = PGNewDeviceWithDescriptor(desc);
+    [desc release];
+    desc = nil;
 
-    [disp_desc init];
     disp_desc.name = @"QEMU display";
     disp_desc.sizeInMillimeters = NSMakeSize(400., 300.); /* A 20" display */
     disp_desc.queue = dispatch_get_main_queue();
@@ -545,9 +546,14 @@ static void apple_gfx_realize(DeviceState *dev, Error **errp)
     };
 
     s->pgdisp = [s->pgdev newDisplayWithDescriptor:disp_desc port:0 serialNum:1234];
+    [disp_desc release];
     s->pgdisp.modeList = [NSArray arrayWithObjects:modes count:ARRAY_SIZE(apple_gfx_modes)];
 
-    [iosfc_desc init];
+    for (i = 0; i < ARRAY_SIZE(apple_gfx_modes); i++) {
+        [modes[i] release];
+        modes[i] = nil;
+    }
+
     iosfc_desc.mapMemory = ^(uint64_t phys, uint64_t len, bool ro, void **va, void *e, void *f) {
         trace_apple_iosfc_map_memory(phys, len, ro, va, e, f);
         MemoryRegion *tmp_mr;
@@ -573,8 +579,9 @@ static void apple_gfx_realize(DeviceState *dev, Error **errp)
         return (bool)true;
     };
 
-    s->pgiosfc = [PGIOSurfaceHostDevice new];
-    [s->pgiosfc initWithDescriptor:iosfc_desc];
+    s->pgiosfc =
+        [[PGIOSurfaceHostDevice alloc] initWithDescriptor:iosfc_desc];
+    [iosfc_desc release];
 
     QTAILQ_INIT(&s->mrs);
     QTAILQ_INIT(&s->tasks);
-- 
2.39.3 (Apple Git-146)

[PATCH 11/26] hw/display/apple-gfx: Uses ObjC category extension for private property

[Phil Dennis-Jordan]

The vmapple mmio implementation of the PG device uses an IOService
mapper for sharing video memory between host and guest. This is only
exposed via a private property on the PGDeviceDescriptor class. The
code has so far declared this in a dummy derived class; the idiomatic
Objective-C way of exposing private APIs on a class is via a category
extension. This change does exactly that; this also avoids the extra
cast to access the property.

Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
---
 hw/display/apple-gfx.m | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/hw/display/apple-gfx.m b/hw/display/apple-gfx.m
index 073741ede5..6537e32806 100644
--- a/hw/display/apple-gfx.m
+++ b/hw/display/apple-gfx.m
@@ -51,7 +51,7 @@
 typedef bool(^IOSFCUnmapMemory)(void *a, void *b, void *c, void *d, void *e, void *f);
 typedef bool(^IOSFCMapMemory)(uint64_t phys, uint64_t len, bool ro, void **va, void *e, void *f);
 
-@interface PGDeviceDescriptorExt : PGDeviceDescriptor
+@interface PGDeviceDescriptor (IOSurfaceMapper)
 @property (readwrite, nonatomic) bool usingIOSurfaceMapper;
 @end
 
@@ -396,7 +396,6 @@ static void apple_gfx_realize(DeviceState *dev, Error **errp)
     PGDeviceDescriptor *desc = [PGDeviceDescriptor new];
     PGDisplayDescriptor *disp_desc = [PGDisplayDescriptor new];
     PGIOSurfaceHostDeviceDescriptor *iosfc_desc = [PGIOSurfaceHostDeviceDescriptor new];
-    PGDeviceDescriptorExt *desc_ext = (PGDeviceDescriptorExt *)desc;
     PGDisplayMode *modes[ARRAY_SIZE(apple_gfx_modes)];
     int i;
 
@@ -408,7 +407,7 @@ static void apple_gfx_realize(DeviceState *dev, Error **errp)
     s->mtl = MTLCreateSystemDefaultDevice();
 
     desc.device = s->mtl;
-    desc_ext.usingIOSurfaceMapper = true;
+    desc.usingIOSurfaceMapper = true;
 
     desc.createTask = ^(uint64_t vmSize, void * _Nullable * _Nonnull baseAddress) {
         AppleGFXTask *task = apple_gfx_new_task(s, vmSize);
-- 
2.39.3 (Apple Git-146)

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

[PATCH 13/26] hw/display/apple-gfx: Defines PGTask_s struct instead of casting

[Phil Dennis-Jordan]

The struct PGTask_s is only forward-declared by the macOS
ParavirtualizedGraphics.framework and treated as opaque, which
means client code can safely provide its own definition.

This change does exactly that, renaming struct AppleGFXTask to
PGTask_s, but keeping the original typedef name. The upshot are
improved type safety and fewer casts.

Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
---
 hw/display/apple-gfx.m | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/hw/display/apple-gfx.m b/hw/display/apple-gfx.m
index a23f731ddc..39e33ed999 100644
--- a/hw/display/apple-gfx.m
+++ b/hw/display/apple-gfx.m
@@ -62,13 +62,13 @@ -(uint32_t)mmioReadAtOffset:(size_t) offset;
 -(void)mmioWriteAtOffset:(size_t) offset value:(uint32_t)value;
 @end
 
-typedef struct AppleGFXTask {
-    QTAILQ_ENTRY(AppleGFXTask) node;
+typedef struct PGTask_s { // Name matches forward declaration in PG header
+    QTAILQ_ENTRY(PGTask_s) node;
     mach_vm_address_t address;
     uint64_t len;
 } AppleGFXTask;
 
-typedef QTAILQ_HEAD(, AppleGFXTask) AppleGFXTaskList;
+typedef QTAILQ_HEAD(, PGTask_s) AppleGFXTaskList;
 
 typedef struct AppleGFXState {
     SysBusDevice parent_obj;
@@ -384,19 +384,19 @@ static void apple_gfx_realize(DeviceState *dev, Error **errp)
         AppleGFXTask *task = apple_gfx_new_task(s, vmSize);
         *baseAddress = (void*)task->address;
         trace_apple_gfx_create_task(vmSize, *baseAddress);
-        return (PGTask_t *)task;
+        return task;
     };
 
-    desc.destroyTask = ^(PGTask_t * _Nonnull _task) {
-        AppleGFXTask *task = (AppleGFXTask *)_task;
+    desc.destroyTask = ^(AppleGFXTask * _Nonnull 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;
+    desc.mapMemory = ^(AppleGFXTask * _Nonnull task, uint32_t rangeCount,
+                       uint64_t virtualOffset, bool readOnly,
+                       PGPhysicalMemoryRange_t * _Nonnull ranges) {
         kern_return_t r;
         mach_vm_address_t target, source;
         trace_apple_gfx_map_memory(task, rangeCount, virtualOffset, readOnly);
@@ -433,8 +433,8 @@ static void apple_gfx_realize(DeviceState *dev, Error **errp)
         return (bool)true;
     };
 
-    desc.unmapMemory = ^(PGTask_t * _Nonnull _task, uint64_t virtualOffset, uint64_t length) {
-        AppleGFXTask *task = (AppleGFXTask *)_task;
+    desc.unmapMemory = ^(AppleGFXTask * _Nonnull task, uint64_t virtualOffset,
+                         uint64_t length) {
         kern_return_t r;
         mach_vm_address_t range_address;
 
-- 
2.39.3 (Apple Git-146)

[PATCH 14/26] hw/display/apple-gfx: Refactoring of realize function

[Phil Dennis-Jordan]

The apple_gfx_realize function was very long, with different
sections of the code doing mostly unrelated things.

This change groups some of the functionality into helper
functions, which hopefully makes the code easier to understand.

There are also some code formatting fixes in the general
vicinity of the refactored code.

Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
---
 hw/display/apple-gfx.m | 120 ++++++++++++++++++++++++++---------------
 1 file changed, 76 insertions(+), 44 deletions(-)

diff --git a/hw/display/apple-gfx.m b/hw/display/apple-gfx.m
index 39e33ed999..f9046f41a0 100644
--- a/hw/display/apple-gfx.m
+++ b/hw/display/apple-gfx.m
@@ -360,26 +360,9 @@ static void apple_gfx_init(Object *obj)
     }
 }
 
-static void apple_gfx_realize(DeviceState *dev, Error **errp)
+static void apple_gfx_register_task_mapping_handlers(AppleGFXState *s,
+                                                     PGDeviceDescriptor *desc)
 {
-    @autoreleasepool {
-    AppleGFXState *s = APPLE_GFX(dev);
-    PGDeviceDescriptor *desc = [PGDeviceDescriptor new];
-    PGDisplayDescriptor *disp_desc = [PGDisplayDescriptor new];
-    PGIOSurfaceHostDeviceDescriptor *iosfc_desc = [PGIOSurfaceHostDeviceDescriptor new];
-    PGDisplayMode *modes[ARRAY_SIZE(apple_gfx_modes)];
-    int i;
-
-    for (i = 0; i < ARRAY_SIZE(apple_gfx_modes); i++) {
-        modes[i] =
-            [[PGDisplayMode alloc] initWithSizeInPixels:apple_gfx_modes[i] refreshRateInHz:60.];
-    }
-
-    s->mtl = MTLCreateSystemDefaultDevice();
-
-    desc.device = s->mtl;
-    desc.usingIOSurfaceMapper = true;
-
     desc.createTask = ^(uint64_t vmSize, void * _Nullable * _Nonnull baseAddress) {
         AppleGFXTask *task = apple_gfx_new_task(s, vmSize);
         *baseAddress = (void*)task->address;
@@ -450,29 +433,18 @@ static void apple_gfx_realize(DeviceState *dev, Error **errp)
         return (bool)true;
     };
 
-    desc.readMemory = ^(uint64_t physicalAddress, uint64_t length, void * _Nonnull dst) {
+    desc.readMemory = ^(uint64_t physicalAddress, uint64_t length,
+                        void * _Nonnull dst) {
         trace_apple_gfx_read_memory(physicalAddress, length, dst);
         cpu_physical_memory_read(physicalAddress, dst, length);
         return (bool)true;
     };
 
-    desc.raiseInterrupt = ^(uint32_t vector) {
-        bool locked;
-
-        trace_apple_gfx_raise_irq(vector);
-        locked = bql_locked();
-        if (!locked) {
-            bql_lock();
-        }
-        qemu_irq_pulse(s->irq_gfx);
-        if (!locked) {
-            bql_unlock();
-        }
-    };
+}
 
-    s->pgdev = PGNewDeviceWithDescriptor(desc);
-    [desc release];
-    desc = nil;
+static PGDisplayDescriptor *apple_gfx_prepare_display_handlers(AppleGFXState *s)
+{
+    PGDisplayDescriptor *disp_desc = [PGDisplayDescriptor new];
 
     disp_desc.name = @"QEMU display";
     disp_desc.sizeInMillimeters = NSMakeSize(400., 300.); /* A 20" display */
@@ -484,7 +456,8 @@ static void apple_gfx_realize(DeviceState *dev, Error **errp)
         s->handles_frames = true;
         s->new_frame = true;
     };
-    disp_desc.modeChangeHandler = ^(PGDisplayCoord_t sizeInPixels, OSType pixelFormat) {
+    disp_desc.modeChangeHandler = ^(PGDisplayCoord_t sizeInPixels,
+                                    OSType pixelFormat) {
         trace_apple_gfx_mode_change(sizeInPixels.x, sizeInPixels.y);
         set_mode(s, sizeInPixels.x, sizeInPixels.y);
     };
@@ -517,15 +490,35 @@ static void apple_gfx_realize(DeviceState *dev, Error **errp)
         update_cursor(s);
     };
 
-    s->pgdisp = [s->pgdev newDisplayWithDescriptor:disp_desc port:0 serialNum:1234];
-    [disp_desc release];
-    s->pgdisp.modeList = [NSArray arrayWithObjects:modes count:ARRAY_SIZE(apple_gfx_modes)];
+    return disp_desc;
+}
+
+static NSArray<PGDisplayMode*>* apple_gfx_prepare_display_mode_array(void)
+{
+    PGDisplayMode *modes[ARRAY_SIZE(apple_gfx_modes)];
+    NSArray<PGDisplayMode*>* mode_array = nil;
+    int i;
+
+    for (i = 0; i < ARRAY_SIZE(apple_gfx_modes); i++) {
+        modes[i] =
+            [[PGDisplayMode alloc] initWithSizeInPixels:apple_gfx_modes[i] refreshRateInHz:60.];
+    }
+
+    mode_array = [NSArray arrayWithObjects:modes count:ARRAY_SIZE(apple_gfx_modes)];
 
     for (i = 0; i < ARRAY_SIZE(apple_gfx_modes); i++) {
         [modes[i] release];
         modes[i] = nil;
     }
 
+    return mode_array;
+}
+
+static PGIOSurfaceHostDevice *apple_gfx_prepare_iosurface_host_device(AppleGFXState *s)
+{
+    PGIOSurfaceHostDeviceDescriptor *iosfc_desc = [PGIOSurfaceHostDeviceDescriptor new];
+    PGIOSurfaceHostDevice *iosfc_host_dev = nil;
+
     iosfc_desc.mapMemory = ^(uint64_t phys, uint64_t len, bool ro, void **va, void *e, void *f) {
         trace_apple_iosfc_map_memory(phys, len, ro, va, e, f);
         MemoryRegion *tmp_mr;
@@ -551,13 +544,52 @@ static void apple_gfx_realize(DeviceState *dev, Error **errp)
         return (bool)true;
     };
 
-    s->pgiosfc =
-        [[PGIOSurfaceHostDevice alloc] initWithDescriptor:iosfc_desc];
+    iosfc_host_dev = [[PGIOSurfaceHostDevice alloc] initWithDescriptor:iosfc_desc];
     [iosfc_desc release];
+    return iosfc_host_dev;
+}
+
+static void apple_gfx_realize(DeviceState *dev, Error **errp)
+{
+    @autoreleasepool {
+        AppleGFXState *s = APPLE_GFX(dev);
+        PGDeviceDescriptor *desc = [PGDeviceDescriptor new];
+        PGDisplayDescriptor *disp_desc = nil;
+
+        QTAILQ_INIT(&s->tasks);
+        s->mtl = MTLCreateSystemDefaultDevice();
+
+        desc.device = s->mtl;
+        desc.usingIOSurfaceMapper = true;
+
+        apple_gfx_register_task_mapping_handlers(s, desc);
+
+        desc.raiseInterrupt = ^(uint32_t vector) {
+            bool locked;
+
+            trace_apple_gfx_raise_irq(vector);
+            locked = bql_locked();
+            if (!locked) {
+                bql_lock();
+            }
+            qemu_irq_pulse(s->irq_gfx);
+            if (!locked) {
+                bql_unlock();
+            }
+        };
+
+        s->pgdev = PGNewDeviceWithDescriptor(desc);
+        [desc release];
+        desc = nil;
+
+        disp_desc = apple_gfx_prepare_display_handlers(s);
+        s->pgdisp = [s->pgdev newDisplayWithDescriptor:disp_desc port:0 serialNum:1234];
+        [disp_desc release];
+        s->pgdisp.modeList = apple_gfx_prepare_display_mode_array();
 
-    QTAILQ_INIT(&s->tasks);
+        s->pgiosfc = apple_gfx_prepare_iosurface_host_device(s);
 
-    create_fb(s);
+        create_fb(s);
     }
 }
 
-- 
2.39.3 (Apple Git-146)

[PATCH 15/26] hw/display/apple-gfx: Separates generic & vmapple-specific functionality

[Phil Dennis-Jordan]

The apple-gfx paravirtualised graphics device can be used in a
MMIO mode with the 'vmapple' arm64 machine model, but also as
a PCI device, especially on x86-64 VMs. There are some
significant differences between these implementations, but
even more shared functionality.

This change prepares for a PCI based implementation by
splitting out the vmapple-specific code into a separate
code file.

Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
---
 hw/display/Kconfig             |   4 +
 hw/display/apple-gfx-vmapple.m | 194 +++++++++++++++++++++++++++++
 hw/display/apple-gfx.h         |  47 +++++++
 hw/display/apple-gfx.m         | 221 +++------------------------------
 hw/display/meson.build         |   3 +-
 hw/display/trace-events        |   2 +
 6 files changed, 266 insertions(+), 205 deletions(-)
 create mode 100644 hw/display/apple-gfx-vmapple.m
 create mode 100644 hw/display/apple-gfx.h

diff --git a/hw/display/Kconfig b/hw/display/Kconfig
index 13cd256c06..e3d10bf6ff 100644
--- a/hw/display/Kconfig
+++ b/hw/display/Kconfig
@@ -146,4 +146,8 @@ config DM163
 
 config MAC_PVG
     bool
+    default y
 
+config MAC_PVG_VMAPPLE
+    bool
+    depends on MAC_PVG
diff --git a/hw/display/apple-gfx-vmapple.m b/hw/display/apple-gfx-vmapple.m
new file mode 100644
index 0000000000..6af8b7a292
--- /dev/null
+++ b/hw/display/apple-gfx-vmapple.m
@@ -0,0 +1,194 @@
+#include "apple-gfx.h"
+#include "monitor/monitor.h"
+#include "hw/sysbus.h"
+#include "hw/irq.h"
+#include "trace.h"
+#import <ParavirtualizedGraphics/ParavirtualizedGraphics.h>
+
+_Static_assert(__aarch64__, "");
+
+/*
+ * ParavirtualizedGraphics.Framework only ships header files for the x86
+ * variant which does not include IOSFC descriptors and host devices. We add
+ * their definitions here so that we can also work with the ARM version.
+ */
+typedef bool(^IOSFCRaiseInterrupt)(uint32_t vector);
+typedef bool(^IOSFCUnmapMemory)(
+    void *a, void *b, void *c, void *d, void *e, void *f);
+typedef bool(^IOSFCMapMemory)(
+    uint64_t phys, uint64_t len, bool ro, void **va, void *e, void *f);
+
+@interface PGDeviceDescriptor (IOSurfaceMapper)
+@property (readwrite, nonatomic) bool usingIOSurfaceMapper;
+@end
+
+@interface PGIOSurfaceHostDeviceDescriptor : NSObject
+-(PGIOSurfaceHostDeviceDescriptor *)init;
+@property (readwrite, nonatomic, copy, nullable) IOSFCMapMemory mapMemory;
+@property (readwrite, nonatomic, copy, nullable) IOSFCUnmapMemory unmapMemory;
+@property (readwrite, nonatomic, copy, nullable) IOSFCRaiseInterrupt raiseInterrupt;
+@end
+
+@interface PGIOSurfaceHostDevice : NSObject
+-(instancetype)initWithDescriptor:(PGIOSurfaceHostDeviceDescriptor *) desc;
+-(uint32_t)mmioReadAtOffset:(size_t) offset;
+-(void)mmioWriteAtOffset:(size_t) offset value:(uint32_t)value;
+@end
+
+typedef struct AppleGFXVmappleState {
+    SysBusDevice parent_obj;
+
+    AppleGFXState common;
+
+    qemu_irq irq_gfx;
+    qemu_irq irq_iosfc;
+    MemoryRegion iomem_iosfc;
+    PGIOSurfaceHostDevice *pgiosfc;
+} AppleGFXVmappleState;
+
+OBJECT_DECLARE_SIMPLE_TYPE(AppleGFXVmappleState, APPLE_GFX_VMAPPLE)
+
+
+static uint64_t apple_iosfc_read(void *opaque, hwaddr offset, unsigned size)
+{
+    AppleGFXVmappleState *s = opaque;
+    uint64_t res = 0;
+
+    bql_unlock();
+    res = [s->pgiosfc mmioReadAtOffset:offset];
+    bql_lock();
+
+    trace_apple_iosfc_read(offset, res);
+
+    return res;
+}
+
+static void apple_iosfc_write(
+    void *opaque, hwaddr offset, uint64_t val, unsigned size)
+{
+    AppleGFXVmappleState *s = opaque;
+
+    trace_apple_iosfc_write(offset, val);
+
+    [s->pgiosfc mmioWriteAtOffset:offset value:val];
+}
+
+static const MemoryRegionOps apple_iosfc_ops = {
+    .read = apple_iosfc_read,
+    .write = apple_iosfc_write,
+    .endianness = DEVICE_LITTLE_ENDIAN,
+    .valid = {
+        .min_access_size = 4,
+        .max_access_size = 8,
+    },
+    .impl = {
+        .min_access_size = 4,
+        .max_access_size = 8,
+    },
+};
+
+static PGIOSurfaceHostDevice *apple_gfx_prepare_iosurface_host_device(
+    AppleGFXVmappleState *s)
+{
+    PGIOSurfaceHostDeviceDescriptor *iosfc_desc =
+        [PGIOSurfaceHostDeviceDescriptor new];
+    PGIOSurfaceHostDevice *iosfc_host_dev = nil;
+
+    iosfc_desc.mapMemory =
+        ^(uint64_t phys, uint64_t len, bool ro, void **va, void *e, void *f) {
+            trace_apple_iosfc_map_memory(phys, len, ro, va, e, f);
+            MemoryRegion *tmp_mr;
+            *va = gpa2hva(&tmp_mr, phys, len, NULL);
+            return (bool)true;
+        };
+
+    iosfc_desc.unmapMemory =
+        ^(void *a, void *b, void *c, void *d, void *e, void *f) {
+            trace_apple_iosfc_unmap_memory(a, b, c, d, e, f);
+            return (bool)true;
+        };
+
+    iosfc_desc.raiseInterrupt = ^(uint32_t vector) {
+        trace_apple_iosfc_raise_irq(vector);
+        bool locked = bql_locked();
+        if (!locked) {
+            bql_lock();
+        }
+        qemu_irq_pulse(s->irq_iosfc);
+        if (!locked) {
+            bql_unlock();
+        }
+        return (bool)true;
+    };
+
+    iosfc_host_dev =
+        [[PGIOSurfaceHostDevice alloc] initWithDescriptor:iosfc_desc];
+    [iosfc_desc release];
+    return iosfc_host_dev;
+}
+
+static void apple_gfx_vmapple_realize(DeviceState *dev, Error **errp)
+{
+    @autoreleasepool {
+        AppleGFXVmappleState *s = APPLE_GFX_VMAPPLE(dev);
+
+        PGDeviceDescriptor *desc = [PGDeviceDescriptor new];
+        desc.usingIOSurfaceMapper = true;
+        desc.raiseInterrupt = ^(uint32_t vector) {
+            bool locked;
+
+            trace_apple_gfx_raise_irq(vector);
+            locked = bql_locked();
+            if (!locked) {
+                bql_lock();
+            }
+            qemu_irq_pulse(s->irq_gfx);
+            if (!locked) {
+                bql_unlock();
+            }
+        };
+
+        s->pgiosfc = apple_gfx_prepare_iosurface_host_device(s);
+
+        apple_gfx_common_realize(&s->common, desc);
+        [desc release];
+        desc = nil;
+    }
+}
+
+static void apple_gfx_vmapple_reset(DeviceState *d)
+{
+}
+
+static void apple_gfx_vmapple_init(Object *obj)
+{
+    AppleGFXVmappleState *s = APPLE_GFX_VMAPPLE(obj);
+
+    apple_gfx_common_init(obj, &s->common, TYPE_APPLE_GFX_VMAPPLE);
+
+    memory_region_init_io(&s->iomem_iosfc, obj, &apple_iosfc_ops, s,
+                          TYPE_APPLE_GFX_VMAPPLE, 0x10000);
+    sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->common.iomem_gfx);
+    sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->iomem_iosfc);
+    sysbus_init_irq(SYS_BUS_DEVICE(s), &s->irq_gfx);
+    sysbus_init_irq(SYS_BUS_DEVICE(s), &s->irq_iosfc);
+}
+
+static void apple_gfx_vmapple_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->reset = apple_gfx_vmapple_reset;
+    dc->realize = apple_gfx_vmapple_realize;
+}
+
+static TypeInfo apple_gfx_vmapple_types[] = {
+    {
+        .name          = TYPE_APPLE_GFX_VMAPPLE,
+        .parent        = TYPE_SYS_BUS_DEVICE,
+        .instance_size = sizeof(AppleGFXVmappleState),
+        .class_init    = apple_gfx_vmapple_class_init,
+        .instance_init = apple_gfx_vmapple_init,
+    }
+};
+DEFINE_TYPES(apple_gfx_vmapple_types)
diff --git a/hw/display/apple-gfx.h b/hw/display/apple-gfx.h
new file mode 100644
index 0000000000..fa7fea6368
--- /dev/null
+++ b/hw/display/apple-gfx.h
@@ -0,0 +1,47 @@
+#ifndef QEMU_APPLE_GFX_H
+#define QEMU_APPLE_GFX_H
+
+#define TYPE_APPLE_GFX_VMAPPLE      "apple-gfx-vmapple"
+#define TYPE_APPLE_GFX_PCI          "apple-gfx-pci"
+
+#include "qemu/typedefs.h"
+
+typedef struct AppleGFXState AppleGFXState;
+
+void apple_gfx_common_init(Object *obj, AppleGFXState *s, const char* obj_name);
+
+#ifdef __OBJC__
+
+#include "qemu/osdep.h"
+#include "exec/memory.h"
+#include "ui/surface.h"
+
+@class PGDeviceDescriptor;
+@protocol PGDevice;
+@protocol PGDisplay;
+@protocol MTLDevice;
+@protocol MTLTexture;
+
+typedef QTAILQ_HEAD(, PGTask_s) AppleGFXTaskList;
+
+struct AppleGFXState {
+    MemoryRegion iomem_gfx;
+    id<PGDevice> pgdev;
+    id<PGDisplay> pgdisp;
+    AppleGFXTaskList tasks;
+    QemuConsole *con;
+    void *vram;
+    id<MTLDevice> mtl;
+    id<MTLTexture> texture;
+    bool handles_frames;
+    bool new_frame;
+    bool cursor_show;
+    DisplaySurface *surface;
+    QEMUCursor *cursor;
+};
+
+void apple_gfx_common_realize(AppleGFXState *s, PGDeviceDescriptor *desc);
+
+#endif /* __OBJC__ */
+
+#endif
diff --git a/hw/display/apple-gfx.m b/hw/display/apple-gfx.m
index f9046f41a0..806feb58fa 100644
--- a/hw/display/apple-gfx.m
+++ b/hw/display/apple-gfx.m
@@ -12,16 +12,9 @@
  * implements support to drive that library from within QEMU.
  */
 
-#include "qemu/osdep.h"
-#include "hw/irq.h"
-#include "migration/vmstate.h"
-#include "qemu/log.h"
-#include "qemu/module.h"
+#include "apple-gfx.h"
 #include "trace.h"
-#include "hw/sysbus.h"
-#include "hw/pci/msi.h"
-#include "crypto/hash.h"
-#include "sysemu/cpus.h"
+#include "qemu/main-loop.h"
 #include "ui/console.h"
 #include "monitor/monitor.h"
 #include "qapi/error.h"
@@ -29,72 +22,17 @@
 #include <mach/mach_vm.h>
 #import <ParavirtualizedGraphics/ParavirtualizedGraphics.h>
 
-#define TYPE_APPLE_GFX          "apple-gfx"
-
 static const PGDisplayCoord_t apple_gfx_modes[] = {
     { .x = 1440, .y = 1080 },
     { .x = 1280, .y = 1024 },
 };
 
-/*
- * ParavirtualizedGraphics.Framework only ships header files for the x86
- * variant which does not include IOSFC descriptors and host devices. We add
- * their definitions here so that we can also work with the ARM version.
- */
-typedef bool(^IOSFCRaiseInterrupt)(uint32_t vector);
-typedef bool(^IOSFCUnmapMemory)(void *a, void *b, void *c, void *d, void *e, void *f);
-typedef bool(^IOSFCMapMemory)(uint64_t phys, uint64_t len, bool ro, void **va, void *e, void *f);
-
-@interface PGDeviceDescriptor (IOSurfaceMapper)
-@property (readwrite, nonatomic) bool usingIOSurfaceMapper;
-@end
-
-@interface PGIOSurfaceHostDeviceDescriptor : NSObject
--(PGIOSurfaceHostDeviceDescriptor *)init;
-@property (readwrite, nonatomic, copy, nullable) IOSFCMapMemory mapMemory;
-@property (readwrite, nonatomic, copy, nullable) IOSFCUnmapMemory unmapMemory;
-@property (readwrite, nonatomic, copy, nullable) IOSFCRaiseInterrupt raiseInterrupt;
-@end
-
-@interface PGIOSurfaceHostDevice : NSObject
--(instancetype)initWithDescriptor:(PGIOSurfaceHostDeviceDescriptor *) desc;
--(uint32_t)mmioReadAtOffset:(size_t) offset;
--(void)mmioWriteAtOffset:(size_t) offset value:(uint32_t)value;
-@end
-
 typedef struct PGTask_s { // Name matches forward declaration in PG header
     QTAILQ_ENTRY(PGTask_s) node;
     mach_vm_address_t address;
     uint64_t len;
 } AppleGFXTask;
 
-typedef QTAILQ_HEAD(, PGTask_s) AppleGFXTaskList;
-
-typedef struct AppleGFXState {
-    SysBusDevice parent_obj;
-
-    qemu_irq irq_gfx;
-    qemu_irq irq_iosfc;
-    MemoryRegion iomem_gfx;
-    MemoryRegion iomem_iosfc;
-    id<PGDevice> pgdev;
-    id<PGDisplay> pgdisp;
-    PGIOSurfaceHostDevice *pgiosfc;
-    AppleGFXTaskList tasks;
-    QemuConsole *con;
-    void *vram;
-    id<MTLDevice> mtl;
-    id<MTLTexture> texture;
-    bool handles_frames;
-    bool new_frame;
-    bool cursor_show;
-    DisplaySurface *surface;
-    QEMUCursor *cursor;
-} AppleGFXState;
-
-
-OBJECT_DECLARE_SIMPLE_TYPE(AppleGFXState, APPLE_GFX)
-
 static Error *apple_gfx_mig_blocker;
 
 static AppleGFXTask *apple_gfx_new_task(AppleGFXState *s, uint64_t len)
@@ -154,43 +92,6 @@ static void apple_gfx_write(void *opaque, hwaddr offset, uint64_t val, unsigned
     },
 };
 
-static uint64_t apple_iosfc_read(void *opaque, hwaddr offset, unsigned size)
-{
-    AppleGFXState *s = opaque;
-    uint64_t res = 0;
-
-    bql_unlock();
-    res = [s->pgiosfc mmioReadAtOffset:offset];
-    bql_lock();
-
-    trace_apple_iosfc_read(offset, res);
-
-    return res;
-}
-
-static void apple_iosfc_write(void *opaque, hwaddr offset, uint64_t val, unsigned size)
-{
-    AppleGFXState *s = opaque;
-
-    trace_apple_iosfc_write(offset, val);
-
-    [s->pgiosfc mmioWriteAtOffset:offset value:val];
-}
-
-static const MemoryRegionOps apple_iosfc_ops = {
-    .read = apple_iosfc_read,
-    .write = apple_iosfc_write,
-    .endianness = DEVICE_NATIVE_ENDIAN,
-    .valid = {
-        .min_access_size = 4,
-        .max_access_size = 8,
-    },
-    .impl = {
-        .min_access_size = 4,
-        .max_access_size = 8,
-    },
-};
-
 static void apple_gfx_fb_update_display(void *opaque)
 {
     AppleGFXState *s = opaque;
@@ -325,29 +226,18 @@ static void set_mode(AppleGFXState *s, uint32_t width, uint32_t height)
 
 static void create_fb(AppleGFXState *s)
 {
-
     s->con = graphic_console_init(NULL, 0, &apple_gfx_fb_ops, s);
     set_mode(s, 1440, 1080);
 
     s->cursor_show = true;
 }
 
-static void apple_gfx_reset(DeviceState *d)
+void apple_gfx_common_init(Object *obj, AppleGFXState *s, const char* obj_name)
 {
-}
-
-static void apple_gfx_init(Object *obj)
-{
-    AppleGFXState *s = APPLE_GFX(obj);
     Error *local_err = NULL;
     int r;
 
-    memory_region_init_io(&s->iomem_gfx, obj, &apple_gfx_ops, s, TYPE_APPLE_GFX, 0x4000);
-    memory_region_init_io(&s->iomem_iosfc, obj, &apple_iosfc_ops, s, TYPE_APPLE_GFX, 0x10000);
-    sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->iomem_gfx);
-    sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->iomem_iosfc);
-    sysbus_init_irq(SYS_BUS_DEVICE(s), &s->irq_gfx);
-    sysbus_init_irq(SYS_BUS_DEVICE(s), &s->irq_iosfc);
+    memory_region_init_io(&s->iomem_gfx, obj, &apple_gfx_ops, s, obj_name, 0x4000);
 
     /* TODO: PVG framework supports serialising device state: integrate it! */
     if (apple_gfx_mig_blocker == NULL) {
@@ -514,101 +404,24 @@ static void apple_gfx_register_task_mapping_handlers(AppleGFXState *s,
     return mode_array;
 }
 
-static PGIOSurfaceHostDevice *apple_gfx_prepare_iosurface_host_device(AppleGFXState *s)
-{
-    PGIOSurfaceHostDeviceDescriptor *iosfc_desc = [PGIOSurfaceHostDeviceDescriptor new];
-    PGIOSurfaceHostDevice *iosfc_host_dev = nil;
-
-    iosfc_desc.mapMemory = ^(uint64_t phys, uint64_t len, bool ro, void **va, void *e, void *f) {
-        trace_apple_iosfc_map_memory(phys, len, ro, va, e, f);
-        MemoryRegion *tmp_mr;
-        *va = gpa2hva(&tmp_mr, phys, len, NULL);
-        return (bool)true;
-    };
-
-    iosfc_desc.unmapMemory = ^(void *a, void *b, void *c, void *d, void *e, void *f) {
-        trace_apple_iosfc_unmap_memory(a, b, c, d, e, f);
-        return (bool)true;
-    };
-
-    iosfc_desc.raiseInterrupt = ^(uint32_t vector) {
-        trace_apple_iosfc_raise_irq(vector);
-        bool locked = bql_locked();
-        if (!locked) {
-            bql_lock();
-        }
-        qemu_irq_pulse(s->irq_iosfc);
-        if (!locked) {
-            bql_unlock();
-        }
-        return (bool)true;
-    };
-
-    iosfc_host_dev = [[PGIOSurfaceHostDevice alloc] initWithDescriptor:iosfc_desc];
-    [iosfc_desc release];
-    return iosfc_host_dev;
-}
-
-static void apple_gfx_realize(DeviceState *dev, Error **errp)
+void apple_gfx_common_realize(AppleGFXState *s, PGDeviceDescriptor *desc)
 {
-    @autoreleasepool {
-        AppleGFXState *s = APPLE_GFX(dev);
-        PGDeviceDescriptor *desc = [PGDeviceDescriptor new];
-        PGDisplayDescriptor *disp_desc = nil;
+    PGDisplayDescriptor *disp_desc = nil;
 
-        QTAILQ_INIT(&s->tasks);
-        s->mtl = MTLCreateSystemDefaultDevice();
+    QTAILQ_INIT(&s->tasks);
+    s->mtl = MTLCreateSystemDefaultDevice();
 
-        desc.device = s->mtl;
-        desc.usingIOSurfaceMapper = true;
+    desc.device = s->mtl;
 
-        apple_gfx_register_task_mapping_handlers(s, desc);
+    apple_gfx_register_task_mapping_handlers(s, desc);
 
-        desc.raiseInterrupt = ^(uint32_t vector) {
-            bool locked;
+    s->pgdev = PGNewDeviceWithDescriptor(desc);
 
-            trace_apple_gfx_raise_irq(vector);
-            locked = bql_locked();
-            if (!locked) {
-                bql_lock();
-            }
-            qemu_irq_pulse(s->irq_gfx);
-            if (!locked) {
-                bql_unlock();
-            }
-        };
+    disp_desc = apple_gfx_prepare_display_handlers(s);
+    s->pgdisp = [s->pgdev newDisplayWithDescriptor:disp_desc
+                                              port:0 serialNum:1234];
+    [disp_desc release];
+    s->pgdisp.modeList = apple_gfx_prepare_display_mode_array();
 
-        s->pgdev = PGNewDeviceWithDescriptor(desc);
-        [desc release];
-        desc = nil;
-
-        disp_desc = apple_gfx_prepare_display_handlers(s);
-        s->pgdisp = [s->pgdev newDisplayWithDescriptor:disp_desc port:0 serialNum:1234];
-        [disp_desc release];
-        s->pgdisp.modeList = apple_gfx_prepare_display_mode_array();
-
-        s->pgiosfc = apple_gfx_prepare_iosurface_host_device(s);
-
-        create_fb(s);
-    }
+    create_fb(s);
 }
-
-static void apple_gfx_class_init(ObjectClass *klass, void *data)
-{
-    DeviceClass *dc = DEVICE_CLASS(klass);
-
-    dc->reset = apple_gfx_reset;
-    dc->realize = apple_gfx_realize;
-}
-
-static TypeInfo apple_gfx_types[] = {
-    {
-        .name          = TYPE_APPLE_GFX,
-        .parent        = TYPE_SYS_BUS_DEVICE,
-        .instance_size = sizeof(AppleGFXState),
-        .class_init    = apple_gfx_class_init,
-        .instance_init = apple_gfx_init,
-    }
-};
-
-DEFINE_TYPES(apple_gfx_types)
diff --git a/hw/display/meson.build b/hw/display/meson.build
index 713786bd07..70d855749c 100644
--- a/hw/display/meson.build
+++ b/hw/display/meson.build
@@ -65,7 +65,8 @@ system_ss.add(when: 'CONFIG_ARTIST', if_true: files('artist.c'))
 
 system_ss.add(when: 'CONFIG_ATI_VGA', if_true: [files('ati.c', 'ati_2d.c', 'ati_dbg.c'), pixman])
 
-system_ss.add(when: 'CONFIG_MAC_PVG', if_true: [files('apple-gfx.m'), pvg, metal])
+system_ss.add(when: 'CONFIG_MAC_PVG',         if_true: [files('apple-gfx.m'), pvg, metal])
+system_ss.add(when: 'CONFIG_MAC_PVG_VMAPPLE', if_true: [files('apple-gfx-vmapple.m'), pvg, metal])
 
 if config_all_devices.has_key('CONFIG_VIRTIO_GPU')
   virtio_gpu_ss = ss.source_set()
diff --git a/hw/display/trace-events b/hw/display/trace-events
index 4b897554c9..e35582d659 100644
--- a/hw/display/trace-events
+++ b/hw/display/trace-events
@@ -208,6 +208,8 @@ apple_gfx_mode_change(uint64_t x, uint64_t y) "x=%"PRId64" y=%"PRId64
 apple_gfx_cursor_set(uint32_t bpp, uint64_t width, uint64_t height) "bpp=%d width=%"PRId64" height=0x%"PRId64
 apple_gfx_cursor_show(uint32_t show) "show=%d"
 apple_gfx_cursor_move(void) ""
+
+# apple-gfx-vmapple.m
 apple_iosfc_read(uint64_t offset, uint64_t res) "offset=0x%"PRIx64" res=0x%"PRIx64
 apple_iosfc_write(uint64_t offset, uint64_t val) "offset=0x%"PRIx64" val=0x%"PRIx64
 apple_iosfc_map_memory(uint64_t phys, uint64_t len, uint32_t ro, void *va, void *e, void *f) "phys=0x%"PRIx64" len=0x%"PRIx64" ro=%d va=%p e=%p f=%p"
-- 
2.39.3 (Apple Git-146)

[PATCH 16/26] hw/display/apple-gfx: Asynchronous MMIO writes on x86-64

[Phil Dennis-Jordan]

This change ensures that the MMIO write calls into the PVG
framework are performed asynchronously on a background dispatch
queue. Without this, we rapidly run into re-entrant MMIO issues.

This problem only seems to exist on x86-64 hosts. Conversely,
doing it async on arm64/vmapple causes other issues, so we're
left with 2 different implementations.

Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
---
 hw/display/apple-gfx.m | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/hw/display/apple-gfx.m b/hw/display/apple-gfx.m
index 806feb58fa..48463e5a1f 100644
--- a/hw/display/apple-gfx.m
+++ b/hw/display/apple-gfx.m
@@ -67,15 +67,28 @@ static uint64_t apple_gfx_read(void *opaque, hwaddr offset, unsigned size)
     return res;
 }
 
-static void apple_gfx_write(void *opaque, hwaddr offset, uint64_t val, unsigned size)
+static void apple_gfx_write(void *opaque, hwaddr offset, uint64_t val,
+                            unsigned size)
 {
     AppleGFXState *s = opaque;
 
     trace_apple_gfx_write(offset, val);
 
+#ifdef __x86_64__
+    id<PGDevice> dev = s->pgdev;
+    dispatch_queue_t bg_queue = NULL;
+
+    bg_queue = dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0ul);
+    [dev retain];
+    dispatch_async(bg_queue, ^{
+        [dev mmioWriteAtOffset:offset value:val];
+        [dev release];
+    });
+#else
     bql_unlock();
     [s->pgdev mmioWriteAtOffset:offset value:val];
     bql_lock();
+#endif
 }
 
 static const MemoryRegionOps apple_gfx_ops = {
-- 
2.39.3 (Apple Git-146)

[PATCH 17/26] hw/display/apple-gfx: Asynchronous rendering and graphics update

[Phil Dennis-Jordan]

This change avoids doing expensive rendering while holding the BQL.
Rendering with the lock held is not only inefficient, it can also
cause deadlocks when the PV Graphics framework’s encode... method
causes a (synchronous) call to a callback, which in turn tries to
acquire the BQL.

Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
---
 hw/display/apple-gfx.h |  15 +++-
 hw/display/apple-gfx.m | 193 +++++++++++++++++++++++++++--------------
 2 files changed, 138 insertions(+), 70 deletions(-)

diff --git a/hw/display/apple-gfx.h b/hw/display/apple-gfx.h
index fa7fea6368..9d6d40795e 100644
--- a/hw/display/apple-gfx.h
+++ b/hw/display/apple-gfx.h
@@ -15,12 +15,14 @@ void apple_gfx_common_init(Object *obj, AppleGFXState *s, const char* obj_name);
 #include "qemu/osdep.h"
 #include "exec/memory.h"
 #include "ui/surface.h"
+#include <dispatch/dispatch.h>
 
 @class PGDeviceDescriptor;
 @protocol PGDevice;
 @protocol PGDisplay;
 @protocol MTLDevice;
 @protocol MTLTexture;
+@protocol MTLCommandQueue;
 
 typedef QTAILQ_HEAD(, PGTask_s) AppleGFXTaskList;
 
@@ -30,14 +32,21 @@ struct AppleGFXState {
     id<PGDisplay> pgdisp;
     AppleGFXTaskList tasks;
     QemuConsole *con;
-    void *vram;
     id<MTLDevice> mtl;
-    id<MTLTexture> texture;
+    id<MTLCommandQueue> mtl_queue;
     bool handles_frames;
     bool new_frame;
     bool cursor_show;
-    DisplaySurface *surface;
     QEMUCursor *cursor;
+
+    dispatch_queue_t render_queue;
+    /* The following fields should only be accessed from render_queue: */
+    bool gfx_update_requested;
+    bool new_frame_ready;
+    int32_t pending_frames;
+    void *vram;
+    DisplaySurface *surface;
+    id<MTLTexture> texture;
 };
 
 void apple_gfx_common_realize(AppleGFXState *s, PGDeviceDescriptor *desc);
diff --git a/hw/display/apple-gfx.m b/hw/display/apple-gfx.m
index 48463e5a1f..5855d1d7f5 100644
--- a/hw/display/apple-gfx.m
+++ b/hw/display/apple-gfx.m
@@ -35,6 +35,9 @@
 
 static Error *apple_gfx_mig_blocker;
 
+static void apple_gfx_render_frame_completed(AppleGFXState *s, void *vram,
+                                             id<MTLTexture> texture);
+
 static AppleGFXTask *apple_gfx_new_task(AppleGFXState *s, uint64_t len)
 {
     mach_vm_address_t task_mem;
@@ -105,41 +108,63 @@ static void apple_gfx_write(void *opaque, hwaddr offset, uint64_t val,
     },
 };
 
-static void apple_gfx_fb_update_display(void *opaque)
+static void apple_gfx_render_new_frame(AppleGFXState *s)
 {
-    AppleGFXState *s = opaque;
-
-    if (!s->new_frame || !s->handles_frames) {
+    BOOL r;
+    void *vram = s->vram;
+    uint32_t width = surface_width(s->surface);
+    uint32_t height = surface_height(s->surface);
+    MTLRegion region = MTLRegionMake2D(0, 0, width, height);
+    id<MTLCommandBuffer> command_buffer = [s->mtl_queue commandBuffer];
+    id<MTLTexture> texture = s->texture;
+    r = [s->pgdisp encodeCurrentFrameToCommandBuffer:command_buffer
+                                             texture:texture
+                                              region:region];
+    if (!r) {
         return;
     }
+    [texture retain];
+
+    [command_buffer retain];
+    [command_buffer addCompletedHandler:
+        ^(id<MTLCommandBuffer> cb)
+        {
+            dispatch_async(s->render_queue, ^{
+                apple_gfx_render_frame_completed(s, vram, texture);
+                [texture release];
+            });
+            [command_buffer release];
+        }];
+    [command_buffer commit];
+}
 
-    @autoreleasepool {
-        s->new_frame = false;
-
-        BOOL r;
-        uint32_t width = surface_width(s->surface);
-        uint32_t height = surface_height(s->surface);
-        MTLRegion region = MTLRegionMake2D(0, 0, width, height);
-        id<MTLCommandQueue> commandQueue = [s->mtl newCommandQueue];
-        id<MTLCommandBuffer> mipmapCommandBuffer = [commandQueue commandBuffer];
-
-        r = [s->pgdisp encodeCurrentFrameToCommandBuffer:mipmapCommandBuffer
-                                                 texture:s->texture
-                                                  region:region];
+static void copy_mtl_texture_to_surface_mem(id<MTLTexture> texture, void *vram)
+{
+    /* TODO: Skip this entirely on a pure Metal or headless/guest-only
+     * rendering path, else use a blit command encoder? Needs careful
+     * (double?) buffering design. */
+    size_t width = texture.width, height = texture.height;
+    MTLRegion region = MTLRegionMake2D(0, 0, width, height);
+    [texture getBytes:vram
+          bytesPerRow:(width * 4)
+        bytesPerImage:(width * height * 4)
+           fromRegion:region
+          mipmapLevel:0
+                slice:0];
+}
 
-        if (r != YES) {
-            return;
-        }
+static void apple_gfx_render_frame_completed(AppleGFXState *s, void *vram,
+                                             id<MTLTexture> texture)
+{
+    --s->pending_frames;
+    assert(s->pending_frames >= 0);
 
-        id<MTLBlitCommandEncoder> blitCommandEncoder = [mipmapCommandBuffer blitCommandEncoder];
-        [blitCommandEncoder endEncoding];
-        [mipmapCommandBuffer commit];
-        [mipmapCommandBuffer waitUntilCompleted];
-        [s->texture getBytes:s->vram bytesPerRow:(width * 4)
-                                     bytesPerImage: (width * height * 4)
-                                     fromRegion: region
-                                     mipmapLevel: 0
-                                     slice: 0];
+    if (vram != s->vram) {
+        /* Display mode has changed, drop this old frame. */
+        assert(texture != s->texture);
+        g_free(vram);
+    } else {
+        copy_mtl_texture_to_surface_mem(texture, vram);
 
         /* Need to render cursor manually if not supported by backend */
         if (!dpy_cursor_define_supported(s->con) && s->cursor && s->cursor_show) {
@@ -159,14 +184,40 @@ static void apple_gfx_fb_update_display(void *opaque)
             pixman_image_unref(image);
         }
 
-        dpy_gfx_update_full(s->con);
-
-        [commandQueue release];
+        if (s->gfx_update_requested) {
+            s->gfx_update_requested = false;
+            dpy_gfx_update_full(s->con);
+            graphic_hw_update_done(s->con);
+            s->new_frame_ready = false;
+        } else {
+            s->new_frame_ready = true;
+        }
+    }
+    if (s->pending_frames > 0) {
+        apple_gfx_render_new_frame(s);
     }
 }
 
+static void apple_gfx_fb_update_display(void *opaque)
+{
+    AppleGFXState *s = opaque;
+
+    dispatch_async(s->render_queue, ^{
+        if (s->pending_frames > 0) {
+            s->gfx_update_requested = true;
+        } else {
+            if (s->new_frame_ready) {
+                dpy_gfx_update_full(s->con);
+                s->new_frame_ready = false;
+            }
+            graphic_hw_update_done(s->con);
+        }
+    });
+}
+
 static const GraphicHwOps apple_gfx_fb_ops = {
     .gfx_update = apple_gfx_fb_update_display,
+    .gfx_update_async = true,
 };
 
 static void update_cursor(AppleGFXState *s)
@@ -182,28 +233,23 @@ static void update_cursor(AppleGFXState *s)
 static void set_mode(AppleGFXState *s, uint32_t width, uint32_t height)
 {
     void *vram = NULL;
-    void *old_vram = s->vram;
     DisplaySurface *surface;
     MTLTextureDescriptor *textureDescriptor;
-    id<MTLTexture> old_texture = nil;
     id<MTLTexture> texture = nil;
-    bool locking_required = false;
-
-    locking_required = !bql_locked();
-    if (locking_required) {
-        bql_lock();
-    }
-    if (s->surface &&
-        width == surface_width(s->surface) &&
-        height == surface_height(s->surface)) {
-        if (locking_required) {
-            bql_unlock();
-        }
+    __block bool no_change = false;
+
+    dispatch_sync(s->render_queue,
+        ^{
+            if (s->surface &&
+                width == surface_width(s->surface) &&
+                height == surface_height(s->surface)) {
+                no_change = true;
+            }
+        });
+
+    if (no_change) {
         return;
     }
-    if (locking_required) {
-        bql_unlock();
-    }
 
     vram = g_malloc0(width * height * 4);
     surface = qemu_create_displaysurface_from(width, height, PIXMAN_LE_a8r8g8b8,
@@ -220,21 +266,23 @@ static void set_mode(AppleGFXState *s, uint32_t width, uint32_t height)
         texture = [s->mtl newTextureWithDescriptor:textureDescriptor];
     }
 
-    if (locking_required) {
-        bql_lock();
-    }
-    old_vram = s->vram;
-    s->vram = vram;
-    s->surface = surface;
-    dpy_gfx_replace_surface(s->con, surface);
-    old_texture = s->texture;
-    s->texture = texture;
-    if (locking_required) {
-        bql_unlock();
-    }
+    dispatch_sync(s->render_queue,
+        ^{
+            id<MTLTexture> old_texture = nil;
+            void *old_vram = s->vram;
+            s->vram = vram;
+            s->surface = surface;
 
-    g_free(old_vram);
-    [old_texture release];
+            dpy_gfx_replace_surface(s->con, surface);
+
+            old_texture = s->texture;
+            s->texture = texture;
+            [old_texture release];
+
+            if (s->pending_frames == 0) {
+                g_free(old_vram);
+            }
+        });
 }
 
 static void create_fb(AppleGFXState *s)
@@ -354,10 +402,18 @@ static void apple_gfx_register_task_mapping_handlers(AppleGFXState *s,
     disp_desc.queue = dispatch_get_main_queue();
     disp_desc.newFrameEventHandler = ^(void) {
         trace_apple_gfx_new_frame();
-
-        /* Tell QEMU gfx stack that a new frame arrived */
-        s->handles_frames = true;
-        s->new_frame = true;
+        dispatch_async(s->render_queue, ^{
+            /* Drop frames if we get too far ahead. */
+            if (s->pending_frames >= 2)
+                return;
+            ++s->pending_frames;
+            if (s->pending_frames > 1) {
+                return;
+            }
+            @autoreleasepool {
+                apple_gfx_render_new_frame(s);
+            }
+        });
     };
     disp_desc.modeChangeHandler = ^(PGDisplayCoord_t sizeInPixels,
                                     OSType pixelFormat) {
@@ -422,7 +478,10 @@ void apple_gfx_common_realize(AppleGFXState *s, PGDeviceDescriptor *desc)
     PGDisplayDescriptor *disp_desc = nil;
 
     QTAILQ_INIT(&s->tasks);
+    s->render_queue = dispatch_queue_create("apple-gfx.render",
+                                            DISPATCH_QUEUE_SERIAL);
     s->mtl = MTLCreateSystemDefaultDevice();
+    s->mtl_queue = [s->mtl newCommandQueue];
 
     desc.device = s->mtl;
 
-- 
2.39.3 (Apple Git-146)

[PATCH 18/26] hw/display/apple-gfx: Adds PCI implementation

[Phil Dennis-Jordan]

This change wires up the PCI variant of the paravirtualised
graphics device, mainly useful for x86-64 macOS guests, implemented
by macOS's ParavirtualizedGraphics.framework. It builds on the shared
code previously split from the vmapple MMIO variant of the device.

Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
---
 hw/display/Kconfig         |   5 ++
 hw/display/apple-gfx-pci.m | 125 +++++++++++++++++++++++++++++++++++++
 hw/display/meson.build     |   1 +
 3 files changed, 131 insertions(+)
 create mode 100644 hw/display/apple-gfx-pci.m

diff --git a/hw/display/Kconfig b/hw/display/Kconfig
index e3d10bf6ff..8a78a60670 100644
--- a/hw/display/Kconfig
+++ b/hw/display/Kconfig
@@ -151,3 +151,8 @@ config MAC_PVG
 config MAC_PVG_VMAPPLE
     bool
     depends on MAC_PVG
+
+config MAC_PVG_PCI
+    bool
+    depends on MAC_PVG && PCI
+    default y if PCI_DEVICES
diff --git a/hw/display/apple-gfx-pci.m b/hw/display/apple-gfx-pci.m
new file mode 100644
index 0000000000..bdbab35eed
--- /dev/null
+++ b/hw/display/apple-gfx-pci.m
@@ -0,0 +1,125 @@
+#include "apple-gfx.h"
+#include "hw/pci/pci_device.h"
+#include "hw/pci/msi.h"
+#include "qapi/error.h"
+#include "trace.h"
+#import <ParavirtualizedGraphics/ParavirtualizedGraphics.h>
+
+typedef struct AppleGFXPCIState {
+    PCIDevice parent_obj;
+
+    AppleGFXState common;
+} AppleGFXPCIState;
+
+OBJECT_DECLARE_SIMPLE_TYPE(AppleGFXPCIState, APPLE_GFX_PCI)
+
+static const char* apple_gfx_pci_option_rom_path = NULL;
+
+static void apple_gfx_init_option_rom_path(void)
+{
+    NSURL *option_rom_url = PGCopyOptionROMURL();
+    const char *option_rom_path = option_rom_url.fileSystemRepresentation;
+    if (option_rom_url.fileURL && option_rom_path != NULL) {
+        apple_gfx_pci_option_rom_path = g_strdup(option_rom_path);
+    }
+    [option_rom_url release];
+}
+
+static void apple_gfx_pci_init(Object *obj)
+{
+    AppleGFXPCIState *s = APPLE_GFX_PCI(obj);
+
+    if (!apple_gfx_pci_option_rom_path) {
+        /* Done on device not class init to avoid -daemonize ObjC fork crash */
+        PCIDeviceClass *pci = PCI_DEVICE_CLASS(object_get_class(obj));
+        apple_gfx_init_option_rom_path();
+        pci->romfile = apple_gfx_pci_option_rom_path;
+    }
+
+    apple_gfx_common_init(obj, &s->common, TYPE_APPLE_GFX_PCI);
+}
+
+static void apple_gfx_pci_interrupt(PCIDevice *dev, AppleGFXPCIState *s,
+                                    uint32_t vector)
+{
+    bool msi_ok;
+    trace_apple_gfx_raise_irq(vector);
+
+    msi_ok = msi_enabled(dev);
+    if (msi_ok) {
+        msi_notify(dev, vector);
+    }
+}
+
+static void apple_gfx_pci_realize(PCIDevice *dev, Error **errp)
+{
+    AppleGFXPCIState *s = APPLE_GFX_PCI(dev);
+    Error *err = NULL;
+    int ret;
+
+    pci_register_bar(dev, PG_PCI_BAR_MMIO,
+                     PCI_BASE_ADDRESS_SPACE_MEMORY, &s->common.iomem_gfx);
+
+    ret = msi_init(dev, 0x0 /* config offset; 0 = find space */,
+                   PG_PCI_MAX_MSI_VECTORS, true /* msi64bit */,
+                   false /*msi_per_vector_mask*/, &err);
+    if (ret != 0) {
+        error_propagate(errp, err);
+        return;
+    }
+
+    @autoreleasepool {
+        PGDeviceDescriptor *desc = [PGDeviceDescriptor new];
+        desc.raiseInterrupt = ^(uint32_t vector) {
+            apple_gfx_pci_interrupt(dev, s, vector);
+        };
+
+        apple_gfx_common_realize(&s->common, desc);
+        [desc release];
+        desc = nil;
+    }
+}
+
+static void apple_gfx_pci_reset(DeviceState *dev)
+{
+    AppleGFXPCIState *s = APPLE_GFX_PCI(dev);
+    if (@available(macOS 12,*)) {
+        [s->common.pgdev reset];
+    } else {
+        // TODO: tear down and bring back up
+    }
+}
+
+static void apple_gfx_pci_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    PCIDeviceClass *pci = PCI_DEVICE_CLASS(klass);
+
+    dc->reset = apple_gfx_pci_reset;
+    dc->desc = "macOS Paravirtualized Graphics PCI Display Controller";
+    dc->hotpluggable = false;
+    set_bit(DEVICE_CATEGORY_DISPLAY, dc->categories);
+
+    pci->vendor_id = PG_PCI_VENDOR_ID;
+    pci->device_id = PG_PCI_DEVICE_ID;
+    pci->class_id = PCI_CLASS_DISPLAY_OTHER;
+    pci->realize = apple_gfx_pci_realize;
+
+    // TODO: Property for setting mode list
+}
+
+static TypeInfo apple_gfx_pci_types[] = {
+    {
+        .name          = TYPE_APPLE_GFX_PCI,
+        .parent        = TYPE_PCI_DEVICE,
+        .instance_size = sizeof(AppleGFXPCIState),
+        .class_init    = apple_gfx_pci_class_init,
+        .instance_init = apple_gfx_pci_init,
+        .interfaces = (InterfaceInfo[]) {
+            { INTERFACE_PCIE_DEVICE },
+            { },
+        },
+    }
+};
+DEFINE_TYPES(apple_gfx_pci_types)
+
diff --git a/hw/display/meson.build b/hw/display/meson.build
index 70d855749c..ceb7bb0761 100644
--- a/hw/display/meson.build
+++ b/hw/display/meson.build
@@ -67,6 +67,7 @@ system_ss.add(when: 'CONFIG_ATI_VGA', if_true: [files('ati.c', 'ati_2d.c', 'ati_
 
 system_ss.add(when: 'CONFIG_MAC_PVG',         if_true: [files('apple-gfx.m'), pvg, metal])
 system_ss.add(when: 'CONFIG_MAC_PVG_VMAPPLE', if_true: [files('apple-gfx-vmapple.m'), pvg, metal])
+system_ss.add(when: 'CONFIG_MAC_PVG_PCI',     if_true: [files('apple-gfx-pci.m'), pvg, metal])
 
 if config_all_devices.has_key('CONFIG_VIRTIO_GPU')
   virtio_gpu_ss = ss.source_set()
-- 
2.39.3 (Apple Git-146)

[PATCH 19/26] ui/cocoa: Adds non-app runloop on main thread mode

[Phil Dennis-Jordan]

Various system frameworks on macOS and other Apple platforms
require a main runloop to be processing events on the process’s
main thread. The Cocoa UI’s requirement to run the process as a
Cocoa application automatically enables this runloop, but it
can be useful to have the runloop handling events even without
the Cocoa UI active.

This change adds a non-app runloop mode to the cocoa_main
function. This can be requested by other code, while the Cocoa UI
additionally enables app mode. This arrangement ensures there is
only one qemu_main function switcheroo, and the Cocoa UI’s app
mode requirement and other subsystems’ runloop requests don’t
conflict with each other.

The main runloop is required for the AppleGFX PV graphics device,
so the runloop request call has been added to its initialisation.

Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
---
 hw/display/apple-gfx.m |  3 +++
 include/qemu-main.h    |  2 ++
 ui/cocoa.m             | 15 +++++++++++++--
 3 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/hw/display/apple-gfx.m b/hw/display/apple-gfx.m
index 5855d1d7f5..437294d0fb 100644
--- a/hw/display/apple-gfx.m
+++ b/hw/display/apple-gfx.m
@@ -14,6 +14,7 @@
 
 #include "apple-gfx.h"
 #include "trace.h"
+#include "qemu-main.h"
 #include "qemu/main-loop.h"
 #include "ui/console.h"
 #include "monitor/monitor.h"
@@ -309,6 +310,8 @@ void apple_gfx_common_init(Object *obj, AppleGFXState *s, const char* obj_name)
             error_report_err(local_err);
         }
     }
+
+    cocoa_enable_runloop_on_main_thread();
 }
 
 static void apple_gfx_register_task_mapping_handlers(AppleGFXState *s,
diff --git a/include/qemu-main.h b/include/qemu-main.h
index 940960a7db..da4516e69e 100644
--- a/include/qemu-main.h
+++ b/include/qemu-main.h
@@ -8,4 +8,6 @@
 int qemu_default_main(void);
 extern int (*qemu_main)(void);
 
+void cocoa_enable_runloop_on_main_thread(void);
+
 #endif /* QEMU_MAIN_H */
diff --git a/ui/cocoa.m b/ui/cocoa.m
index 2935247cdd..ccef2aa93c 100644
--- a/ui/cocoa.m
+++ b/ui/cocoa.m
@@ -1941,6 +1941,7 @@ static void cocoa_clipboard_request(QemuClipboardInfo *info,
     exit(status);
 }
 
+static bool run_as_cocoa_app = false;
 static int cocoa_main(void)
 {
     QemuThread thread;
@@ -1953,7 +1954,11 @@ static int cocoa_main(void)
 
     // Start the main event loop
     COCOA_DEBUG("Main thread: entering OSX run loop\n");
-    [NSApp run];
+    if (run_as_cocoa_app) {
+        [NSApp run];
+    } else {
+        CFRunLoopRun();
+    }
     COCOA_DEBUG("Main thread: left OSX run loop, which should never happen\n");
 
     abort();
@@ -2012,13 +2017,19 @@ static void cocoa_refresh(DisplayChangeListener *dcl)
     [pool release];
 }
 
+void cocoa_enable_runloop_on_main_thread(void)
+{
+    qemu_main = cocoa_main;
+}
+
 static void cocoa_display_init(DisplayState *ds, DisplayOptions *opts)
 {
     NSAutoreleasePool * pool = [[NSAutoreleasePool alloc] init];
 
     COCOA_DEBUG("qemu_cocoa: cocoa_display_init\n");
 
-    qemu_main = cocoa_main;
+    run_as_cocoa_app = true;
+    cocoa_enable_runloop_on_main_thread();
 
     // Pull this console process up to being a fully-fledged graphical
     // app with a menubar and Dock icon
-- 
2.39.3 (Apple Git-146)

[PATCH 20/26] hw/display/apple-gfx: Fixes cursor hotspot handling

[Phil Dennis-Jordan]

The ParavirtualizedGraphics framework provides the cursor's
hotspot, this change actually passes that information through to
Qemu's cursor handling.

This change also seizes the opportunity to make other cursor
related code conform to coding standards.

Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
---
 hw/display/apple-gfx.m | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/hw/display/apple-gfx.m b/hw/display/apple-gfx.m
index 437294d0fb..bc9722b420 100644
--- a/hw/display/apple-gfx.m
+++ b/hw/display/apple-gfx.m
@@ -223,7 +223,8 @@ static void apple_gfx_fb_update_display(void *opaque)
 
 static void update_cursor(AppleGFXState *s)
 {
-    dpy_mouse_set(s->con, s->pgdisp.cursorPosition.x, s->pgdisp.cursorPosition.y, s->cursor_show);
+    dpy_mouse_set(s->con, s->pgdisp.cursorPosition.x,
+                  s->pgdisp.cursorPosition.y, s->cursor_show);
 
     /* Need to render manually if cursor is not natively supported */
     if (!dpy_cursor_define_supported(s->con)) {
@@ -423,7 +424,8 @@ static void apple_gfx_register_task_mapping_handlers(AppleGFXState *s,
         trace_apple_gfx_mode_change(sizeInPixels.x, sizeInPixels.y);
         set_mode(s, sizeInPixels.x, sizeInPixels.y);
     };
-    disp_desc.cursorGlyphHandler = ^(NSBitmapImageRep *glyph, PGDisplayCoord_t hotSpot) {
+    disp_desc.cursorGlyphHandler = ^(NSBitmapImageRep *glyph,
+                                     PGDisplayCoord_t hotSpot) {
         uint32_t bpp = glyph.bitsPerPixel;
         uint64_t width = glyph.pixelsWide;
         uint64_t height = glyph.pixelsHigh;
@@ -434,6 +436,8 @@ static void apple_gfx_register_task_mapping_handlers(AppleGFXState *s,
             cursor_unref(s->cursor);
         }
         s->cursor = cursor_alloc(width, height);
+        s->cursor->hot_x = hotSpot.x;
+        s->cursor->hot_y = hotSpot.y;
 
         /* TODO handle different bpp */
         if (bpp == 32) {
-- 
2.39.3 (Apple Git-146)

[PATCH 21/26] hw/display/apple-gfx: Implements texture syncing for non-UMA GPUs

[Phil Dennis-Jordan]

Renderable Metal textures are handled differently depending on
whether the GPU uses a unified memory architecture (no physical
distinction between VRAM and system RAM, CPU and GPU share the
memory bus) or not. (Traditional discrete GPU with its own VRAM)

In the discrete GPU case, textures must be explicitly
synchronised to the CPU or the GPU before use after being
modified by the other. In this case, we sync after the PV
graphics framework has rendered the next frame into the
texture using the GPU so that we can read out its contents using
the CPU. This fixes the issue where the guest screen stayed
black on AMD Radeon GPUs.

Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
---
 hw/display/apple-gfx.h |  1 +
 hw/display/apple-gfx.m | 10 +++++++++-
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/hw/display/apple-gfx.h b/hw/display/apple-gfx.h
index 9d6d40795e..995ecf7f4a 100644
--- a/hw/display/apple-gfx.h
+++ b/hw/display/apple-gfx.h
@@ -43,6 +43,7 @@ struct AppleGFXState {
     /* The following fields should only be accessed from render_queue: */
     bool gfx_update_requested;
     bool new_frame_ready;
+    bool using_managed_texture_storage;
     int32_t pending_frames;
     void *vram;
     DisplaySurface *surface;
diff --git a/hw/display/apple-gfx.m b/hw/display/apple-gfx.m
index bc9722b420..801ae4ad51 100644
--- a/hw/display/apple-gfx.m
+++ b/hw/display/apple-gfx.m
@@ -125,7 +125,12 @@ static void apple_gfx_render_new_frame(AppleGFXState *s)
         return;
     }
     [texture retain];
-
+    if (s->using_managed_texture_storage) {
+        /* "Managed" textures exist in both VRAM and RAM and must be synced. */
+        id<MTLBlitCommandEncoder> blit = [command_buffer blitCommandEncoder];
+        [blit synchronizeResource:texture];
+        [blit endEncoding];
+    }
     [command_buffer retain];
     [command_buffer addCompletedHandler:
         ^(id<MTLCommandBuffer> cb)
@@ -268,6 +273,9 @@ static void set_mode(AppleGFXState *s, uint32_t width, uint32_t height)
         texture = [s->mtl newTextureWithDescriptor:textureDescriptor];
     }
 
+    s->using_managed_texture_storage =
+        (texture.storageMode == MTLStorageModeManaged);
+
     dispatch_sync(s->render_queue,
         ^{
             id<MTLTexture> old_texture = nil;
-- 
2.39.3 (Apple Git-146)

[PATCH 22/26] hw/display/apple-gfx: Replaces magic number with queried MMIO length

[Phil Dennis-Jordan]

Rather than specifying the length of the device's MMIO range as an
unnamed literal constant (which is at least documented as a comment in
the framework headers), we query the PVG framework's API for the
recommended value. This also avoids problems in future, in case the
currently documented value for the default changes.

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

diff --git a/hw/display/apple-gfx.m b/hw/display/apple-gfx.m
index 801ae4ad51..cbcbaf0006 100644
--- a/hw/display/apple-gfx.m
+++ b/hw/display/apple-gfx.m
@@ -303,12 +303,26 @@ static void create_fb(AppleGFXState *s)
     s->cursor_show = true;
 }
 
+static size_t apple_gfx_get_default_mmio_range_size(void)
+{
+    size_t mmio_range_size;
+    @autoreleasepool {
+        PGDeviceDescriptor *desc = [PGDeviceDescriptor new];
+        mmio_range_size = desc.mmioLength;
+        [desc release];
+    }
+    return mmio_range_size;
+}
+
 void apple_gfx_common_init(Object *obj, AppleGFXState *s, const char* obj_name)
 {
     Error *local_err = NULL;
     int r;
+    size_t mmio_range_size = apple_gfx_get_default_mmio_range_size();
 
-    memory_region_init_io(&s->iomem_gfx, obj, &apple_gfx_ops, s, obj_name, 0x4000);
+    trace_apple_gfx_common_init(obj_name, mmio_range_size);
+    memory_region_init_io(&s->iomem_gfx, obj, &apple_gfx_ops, s, obj_name,
+                          mmio_range_size);
 
     /* TODO: PVG framework supports serialising device state: integrate it! */
     if (apple_gfx_mig_blocker == NULL) {
diff --git a/hw/display/trace-events b/hw/display/trace-events
index e35582d659..1809a358e3 100644
--- a/hw/display/trace-events
+++ b/hw/display/trace-events
@@ -208,6 +208,7 @@ apple_gfx_mode_change(uint64_t x, uint64_t y) "x=%"PRId64" y=%"PRId64
 apple_gfx_cursor_set(uint32_t bpp, uint64_t width, uint64_t height) "bpp=%d width=%"PRId64" height=0x%"PRId64
 apple_gfx_cursor_show(uint32_t show) "show=%d"
 apple_gfx_cursor_move(void) ""
+apple_gfx_common_init(const char *device_name, size_t mmio_size) "device: %s; MMIO size: %zu bytes"
 
 # apple-gfx-vmapple.m
 apple_iosfc_read(uint64_t offset, uint64_t res) "offset=0x%"PRIx64" res=0x%"PRIx64
-- 
2.39.3 (Apple Git-146)

[PATCH 23/26] hw/display/apple-gfx: Host GPU picking improvements

[Phil Dennis-Jordan]

During startup of the PV graphics device, we need to specify the
host GPU to use for PV acceleration of the guest's graphics
operations.

On a host system, this is trivial: pick the only one. The
MTLCreateSystemDefaultDevice() function will do the right thing
in this case.

It gets a little more complicated on systems with more than one
GPU; first and foremost, this applies to x86-64 MacBook Pros
with 15/16" displays. However, with eGPUs, in theory any x86-64
Mac can gain one or more additional GPUs. In these cases, the
default is often not ideal - usually, discrete GPUs are selected.
In my tests, performance tends to be best with iGPUs, however,
and they are usually also best in terms of energy consumption.

Ideally, we will want to allow the user to manually select a GPU
if they so choose. In this patch, I am interested in picking a
sensible default. Instead of the built-in default logic, it is
now:

 1. Select a GPU with unified memory (iGPU)
 2. If (1) fails, select a GPU that is built-in, not an eGPU.
 3. If (2) fails, fall back to system default.

Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
---
 hw/display/apple-gfx.m | 28 +++++++++++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)

diff --git a/hw/display/apple-gfx.m b/hw/display/apple-gfx.m
index cbcbaf0006..6c92f2579b 100644
--- a/hw/display/apple-gfx.m
+++ b/hw/display/apple-gfx.m
@@ -502,6 +502,32 @@ static void apple_gfx_register_task_mapping_handlers(AppleGFXState *s,
     return mode_array;
 }
 
+static id<MTLDevice> copy_suitable_metal_device(void)
+{
+    id<MTLDevice> dev = nil;
+    NSArray<id<MTLDevice>> *devs = MTLCopyAllDevices();
+
+    /* Prefer a unified memory GPU. Failing that, pick a non-removable GPU. */
+    for (size_t i = 0; i < devs.count; ++i) {
+        if (devs[i].hasUnifiedMemory) {
+            dev = devs[i];
+            break;
+        }
+        if (!devs[i].removable) {
+            dev = devs[i];
+        }
+    }
+
+    if (dev != nil) {
+        [dev retain];
+    } else {
+        dev = MTLCreateSystemDefaultDevice();
+    }
+    [devs release];
+
+    return dev;
+}
+
 void apple_gfx_common_realize(AppleGFXState *s, PGDeviceDescriptor *desc)
 {
     PGDisplayDescriptor *disp_desc = nil;
@@ -509,7 +535,7 @@ void apple_gfx_common_realize(AppleGFXState *s, PGDeviceDescriptor *desc)
     QTAILQ_INIT(&s->tasks);
     s->render_queue = dispatch_queue_create("apple-gfx.render",
                                             DISPATCH_QUEUE_SERIAL);
-    s->mtl = MTLCreateSystemDefaultDevice();
+    s->mtl = copy_suitable_metal_device();
     s->mtl_queue = [s->mtl newCommandQueue];
 
     desc.device = s->mtl;
-- 
2.39.3 (Apple Git-146)

[PATCH 24/26] hw/display/apple-gfx: Adds configurable mode list

[Phil Dennis-Jordan]

This change adds a property 'display_modes' on the graphics device
which permits specifying a list of display modes. (screen resolution
and refresh rate)

PCI variant of apple-gfx only for the moment.

Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
---
 hw/display/apple-gfx-pci.m |  43 ++++++++++-
 hw/display/apple-gfx.h     |  17 ++++-
 hw/display/apple-gfx.m     | 151 ++++++++++++++++++++++++++++++++++---
 3 files changed, 198 insertions(+), 13 deletions(-)

diff --git a/hw/display/apple-gfx-pci.m b/hw/display/apple-gfx-pci.m
index bdbab35eed..a8205093ab 100644
--- a/hw/display/apple-gfx-pci.m
+++ b/hw/display/apple-gfx-pci.m
@@ -1,6 +1,7 @@
 #include "apple-gfx.h"
 #include "hw/pci/pci_device.h"
 #include "hw/pci/msi.h"
+#include "hw/qdev-properties.h"
 #include "qapi/error.h"
 #include "trace.h"
 #import <ParavirtualizedGraphics/ParavirtualizedGraphics.h>
@@ -90,6 +91,46 @@ static void apple_gfx_pci_reset(DeviceState *dev)
     }
 }
 
+static void apple_gfx_pci_get_display_modes(Object *obj, Visitor *v,
+                                            const char *name, void *opaque,
+                                            Error **errp)
+{
+    Property *prop = opaque;
+    AppleGFXDisplayModeList *mode_list = object_field_prop_ptr(obj, prop);
+
+    apple_gfx_get_display_modes(mode_list, v, name, errp);
+}
+
+static void apple_gfx_pci_set_display_modes(Object *obj, Visitor *v,
+                                            const char *name, void *opaque,
+                                            Error **errp)
+{
+    Property *prop = opaque;
+    AppleGFXDisplayModeList *mode_list = object_field_prop_ptr(obj, prop);
+
+    apple_gfx_set_display_modes(mode_list, v, name, errp);
+}
+
+const PropertyInfo apple_gfx_pci_prop_display_modes = {
+    .name  = "display_modes",
+    .description =
+        "Colon-separated list of display modes; "
+        "<width>x<height>@<refresh-rate>; the first mode is considered "
+        "'native'. Example: 3840x2160@60:2560x1440@60:1920x1080@60",
+    .get   = apple_gfx_pci_get_display_modes,
+    .set   = apple_gfx_pci_set_display_modes,
+};
+
+#define DEFINE_PROP_DISPLAY_MODES(_name, _state, _field) \
+    DEFINE_PROP(_name, _state, _field, apple_gfx_pci_prop_display_modes, \
+                AppleGFXDisplayModeList)
+
+static Property apple_gfx_pci_properties[] = {
+    DEFINE_PROP_DISPLAY_MODES("display-modes", AppleGFXPCIState,
+                              common.display_modes),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static void apple_gfx_pci_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -105,7 +146,7 @@ static void apple_gfx_pci_class_init(ObjectClass *klass, void *data)
     pci->class_id = PCI_CLASS_DISPLAY_OTHER;
     pci->realize = apple_gfx_pci_realize;
 
-    // TODO: Property for setting mode list
+    device_class_set_props(dc, apple_gfx_pci_properties);
 }
 
 static TypeInfo apple_gfx_pci_types[] = {
diff --git a/hw/display/apple-gfx.h b/hw/display/apple-gfx.h
index 995ecf7f4a..baad4a9865 100644
--- a/hw/display/apple-gfx.h
+++ b/hw/display/apple-gfx.h
@@ -5,14 +5,28 @@
 #define TYPE_APPLE_GFX_PCI          "apple-gfx-pci"
 
 #include "qemu/typedefs.h"
+#include "qemu/osdep.h"
 
 typedef struct AppleGFXState AppleGFXState;
 
+typedef struct AppleGFXDisplayMode {
+    uint16_t width_px;
+    uint16_t height_px;
+    uint16_t refresh_rate_hz;
+} AppleGFXDisplayMode;
+
+typedef struct AppleGFXDisplayModeList {
+    GArray *modes;
+} AppleGFXDisplayModeList;
+
 void apple_gfx_common_init(Object *obj, AppleGFXState *s, const char* obj_name);
+void apple_gfx_get_display_modes(AppleGFXDisplayModeList *mode_list, Visitor *v,
+                                 const char *name, Error **errp);
+void apple_gfx_set_display_modes(AppleGFXDisplayModeList *mode_list, Visitor *v,
+                                 const char *name, Error **errp);
 
 #ifdef __OBJC__
 
-#include "qemu/osdep.h"
 #include "exec/memory.h"
 #include "ui/surface.h"
 #include <dispatch/dispatch.h>
@@ -38,6 +52,7 @@ struct AppleGFXState {
     bool new_frame;
     bool cursor_show;
     QEMUCursor *cursor;
+    AppleGFXDisplayModeList display_modes;
 
     dispatch_queue_t render_queue;
     /* The following fields should only be accessed from render_queue: */
diff --git a/hw/display/apple-gfx.m b/hw/display/apple-gfx.m
index 6c92f2579b..e0ad784022 100644
--- a/hw/display/apple-gfx.m
+++ b/hw/display/apple-gfx.m
@@ -16,6 +16,9 @@
 #include "trace.h"
 #include "qemu-main.h"
 #include "qemu/main-loop.h"
+#include "qemu/cutils.h"
+#include "qapi/visitor.h"
+#include "qapi/error.h"
 #include "ui/console.h"
 #include "monitor/monitor.h"
 #include "qapi/error.h"
@@ -23,9 +26,10 @@
 #include <mach/mach_vm.h>
 #import <ParavirtualizedGraphics/ParavirtualizedGraphics.h>
 
-static const PGDisplayCoord_t apple_gfx_modes[] = {
-    { .x = 1440, .y = 1080 },
-    { .x = 1280, .y = 1024 },
+static const AppleGFXDisplayMode apple_gfx_default_modes[] = {
+    { 1920, 1080, 60 },
+    { 1440, 1080, 60 },
+    { 1280, 1024, 60 },
 };
 
 typedef struct PGTask_s { // Name matches forward declaration in PG header
@@ -298,7 +302,6 @@ static void set_mode(AppleGFXState *s, uint32_t width, uint32_t height)
 static void create_fb(AppleGFXState *s)
 {
     s->con = graphic_console_init(NULL, 0, &apple_gfx_fb_ops, s);
-    set_mode(s, 1440, 1080);
 
     s->cursor_show = true;
 }
@@ -481,20 +484,24 @@ static void apple_gfx_register_task_mapping_handlers(AppleGFXState *s,
     return disp_desc;
 }
 
-static NSArray<PGDisplayMode*>* apple_gfx_prepare_display_mode_array(void)
+static NSArray<PGDisplayMode*>* apple_gfx_create_display_mode_array(
+    const AppleGFXDisplayMode display_modes[], int display_mode_count)
 {
-    PGDisplayMode *modes[ARRAY_SIZE(apple_gfx_modes)];
+    PGDisplayMode **modes = alloca(sizeof(modes[0]) * display_mode_count);
     NSArray<PGDisplayMode*>* mode_array = nil;
     int i;
 
-    for (i = 0; i < ARRAY_SIZE(apple_gfx_modes); i++) {
+    for (i = 0; i < display_mode_count; i++) {
+        const AppleGFXDisplayMode *mode = &display_modes[i];
+        PGDisplayCoord_t mode_size = { mode->width_px, mode->height_px };
         modes[i] =
-            [[PGDisplayMode alloc] initWithSizeInPixels:apple_gfx_modes[i] refreshRateInHz:60.];
+            [[PGDisplayMode alloc] initWithSizeInPixels:mode_size
+                                        refreshRateInHz:mode->refresh_rate_hz];
     }
 
-    mode_array = [NSArray arrayWithObjects:modes count:ARRAY_SIZE(apple_gfx_modes)];
+    mode_array = [NSArray arrayWithObjects:modes count:display_mode_count];
 
-    for (i = 0; i < ARRAY_SIZE(apple_gfx_modes); i++) {
+    for (i = 0; i < display_mode_count; i++) {
         [modes[i] release];
         modes[i] = nil;
     }
@@ -531,6 +538,8 @@ static void apple_gfx_register_task_mapping_handlers(AppleGFXState *s,
 void apple_gfx_common_realize(AppleGFXState *s, PGDeviceDescriptor *desc)
 {
     PGDisplayDescriptor *disp_desc = nil;
+    const AppleGFXDisplayMode *display_modes = apple_gfx_default_modes;
+    int num_display_modes = ARRAY_SIZE(apple_gfx_default_modes);
 
     QTAILQ_INIT(&s->tasks);
     s->render_queue = dispatch_queue_create("apple-gfx.render",
@@ -548,7 +557,127 @@ void apple_gfx_common_realize(AppleGFXState *s, PGDeviceDescriptor *desc)
     s->pgdisp = [s->pgdev newDisplayWithDescriptor:disp_desc
                                               port:0 serialNum:1234];
     [disp_desc release];
-    s->pgdisp.modeList = apple_gfx_prepare_display_mode_array();
+
+    if (s->display_modes.modes != NULL && s->display_modes.modes->len > 0) {
+        display_modes =
+            &g_array_index(s->display_modes.modes, AppleGFXDisplayMode, 0);
+        num_display_modes = s->display_modes.modes->len;
+    }
+    s->pgdisp.modeList =
+        apple_gfx_create_display_mode_array(display_modes, num_display_modes);
 
     create_fb(s);
 }
+
+void apple_gfx_get_display_modes(AppleGFXDisplayModeList *mode_list, Visitor *v,
+                                 const char *name, Error **errp)
+{
+    GArray *modes = mode_list->modes;
+    /* 3 uint16s (max 5 digits) and 3 separator characters per mode + nul. */
+    size_t buffer_size = (5 + 1) * 3 * modes->len + 1;
+
+    char *buffer = alloca(buffer_size);
+    char *pos = buffer;
+
+    unsigned used = 0;
+    buffer[0] = '\0';
+    for (guint i = 0; i < modes->len; ++i)
+    {
+        AppleGFXDisplayMode *mode =
+            &g_array_index(modes, AppleGFXDisplayMode, i);
+        int rc = snprintf(pos, buffer_size - used,
+                          "%s%"PRIu16"x%"PRIu16"@%"PRIu16,
+                          i > 0 ? ":" : "",
+                          mode->width_px, mode->height_px,
+                          mode->refresh_rate_hz);
+        used += rc;
+        pos += rc;
+        assert(used < buffer_size);
+    }
+
+    pos = buffer;
+    visit_type_str(v, name, &pos, errp);
+}
+
+void apple_gfx_set_display_modes(AppleGFXDisplayModeList *mode_list, Visitor *v,
+                                 const char *name, Error **errp)
+{
+    Error *local_err = NULL;
+    const char *endptr;
+    char *str;
+    int ret;
+    unsigned int val;
+    uint32_t num_modes;
+    GArray *modes;
+    uint32_t mode_idx;
+
+    visit_type_str(v, name, &str, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+
+    // Count colons to estimate modes. No leading/trailing colons so start at 1.
+    num_modes = 1;
+    for (size_t i = 0; str[i] != '\0'; ++i)
+    {
+        if (str[i] == ':') {
+            ++num_modes;
+        }
+    }
+
+    modes = g_array_sized_new(false, true, sizeof(AppleGFXDisplayMode), num_modes);
+
+    endptr = str;
+    for (mode_idx = 0; mode_idx < num_modes; ++mode_idx)
+    {
+        AppleGFXDisplayMode mode = {};
+        if (mode_idx > 0)
+        {
+            if (*endptr != ':') {
+                goto separator_error;
+            }
+            ++endptr;
+        }
+
+        ret = qemu_strtoui(endptr, &endptr, 10, &val);
+        if (ret || val > UINT16_MAX || val == 0) {
+            error_setg(errp, "width of '%s' must be a decimal integer number "
+                       "of pixels in the range 1..65535", name);
+            goto out;
+        }
+        mode.width_px = val;
+        if (*endptr != 'x') {
+            goto separator_error;
+        }
+
+        ret = qemu_strtoui(endptr + 1, &endptr, 10, &val);
+        if (ret || val > UINT16_MAX || val == 0) {
+            error_setg(errp, "height of '%s' must be a decimal integer number "
+                       "of pixels in the range 1..65535", name);
+            goto out;
+        }
+        mode.height_px = val;
+        if (*endptr != '@') {
+            goto separator_error;
+        }
+
+        ret = qemu_strtoui(endptr + 1, &endptr, 10, &val);
+        if (ret) {
+            error_setg(errp, "refresh rate of '%s'"
+                       " must be a non-negative decimal integer (Hertz)", name);
+        }
+        mode.refresh_rate_hz = val;
+        g_array_append_val(modes, mode);
+    }
+
+    mode_list->modes = modes;
+    goto out;
+
+separator_error:
+    error_setg(errp, "Each display mode takes the format "
+               "'<width>x<height>@<rate>', modes are separated by colons. (:)");
+out:
+    g_free(str);
+    return;
+}
-- 
2.39.3 (Apple Git-146)

[PATCH 25/26] MAINTAINERS: Add myself as maintainer for apple-gfx, reviewer for HVF

[Phil Dennis-Jordan]

I'm happy to take responsibility for the macOS PV graphics code. As
HVF patches don't seem to get much attention at the moment, I'm also
adding myself as designated reviewer for HVF and x86 HVF to try and
improve that.

I anticipate that the resulting workload should be covered by the
funding I'm receiving for improving Qemu in combination with macOS. As
of right now this runs out at the end of 2024; I expect the workload on
apple-gfx should be relatively minor and manageable in my spare time
beyond that. I may have to remove myself from more general HVF duties
once the contract runs out if it's more than I can manage.

Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
---
 MAINTAINERS | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 7d9811458c..b870bd6ad5 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -511,6 +511,7 @@ F: target/arm/hvf/
 X86 HVF CPUs
 M: Cameron Esfahani <dirty@apple.com>
 M: Roman Bolshakov <rbolshakov@ddn.com>
+R: Phil Dennis-Jordan <phil@philjordan.eu>
 W: https://wiki.qemu.org/Features/HVF
 S: Maintained
 F: target/i386/hvf/
@@ -518,6 +519,7 @@ F: target/i386/hvf/
 HVF
 M: Cameron Esfahani <dirty@apple.com>
 M: Roman Bolshakov <rbolshakov@ddn.com>
+R: Phil Dennis-Jordan <phil@philjordan.eu>
 W: https://wiki.qemu.org/Features/HVF
 S: Maintained
 F: accel/hvf/
@@ -2624,6 +2626,11 @@ F: hw/display/edid*
 F: include/hw/display/edid.h
 F: qemu-edid.c
 
+macOS PV Graphics (apple-gfx)
+M: Phil Dennis-Jordan <phil@philjordan.eu>
+S: Maintained
+F: hw/display/apple-gfx*
+
 PIIX4 South Bridge (i82371AB)
 M: Hervé Poussineau <hpoussin@reactos.org>
 M: Philippe Mathieu-Daudé <philmd@linaro.org>
-- 
2.39.3 (Apple Git-146)

[PATCH 26/26] hw/display/apple-gfx: Removes UI pointer support check

[Phil Dennis-Jordan]

The dpy_cursor_define_supported() check is destined for removal
when the last UI frontend without cursor support (cocoa) gains
cursor integration. This patch is required to reconcile with
that change.

Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
---
 hw/display/apple-gfx.m | 24 ------------------------
 1 file changed, 24 deletions(-)

diff --git a/hw/display/apple-gfx.m b/hw/display/apple-gfx.m
index e0ad784022..a4789be275 100644
--- a/hw/display/apple-gfx.m
+++ b/hw/display/apple-gfx.m
@@ -175,25 +175,6 @@ static void apple_gfx_render_frame_completed(AppleGFXState *s, void *vram,
         g_free(vram);
     } else {
         copy_mtl_texture_to_surface_mem(texture, vram);
-
-        /* Need to render cursor manually if not supported by backend */
-        if (!dpy_cursor_define_supported(s->con) && s->cursor && s->cursor_show) {
-            pixman_image_t *image =
-                pixman_image_create_bits(PIXMAN_a8r8g8b8,
-                                         s->cursor->width,
-                                         s->cursor->height,
-                                         (uint32_t *)s->cursor->data,
-                                         s->cursor->width * 4);
-
-            pixman_image_composite(PIXMAN_OP_OVER,
-                                   image, NULL, s->surface->image,
-                                   0, 0, 0, 0, s->pgdisp.cursorPosition.x,
-                                   s->pgdisp.cursorPosition.y, s->cursor->width,
-                                   s->cursor->height);
-
-            pixman_image_unref(image);
-        }
-
         if (s->gfx_update_requested) {
             s->gfx_update_requested = false;
             dpy_gfx_update_full(s->con);
@@ -234,11 +215,6 @@ static void update_cursor(AppleGFXState *s)
 {
     dpy_mouse_set(s->con, s->pgdisp.cursorPosition.x,
                   s->pgdisp.cursorPosition.y, s->cursor_show);
-
-    /* Need to render manually if cursor is not natively supported */
-    if (!dpy_cursor_define_supported(s->con)) {
-        s->new_frame = true;
-    }
 }
 
 static void set_mode(AppleGFXState *s, uint32_t width, uint32_t height)
-- 
2.39.3 (Apple Git-146)

Re: [PATCH 16/26] hw/display/apple-gfx: Asynchronous MMIO writes on x86-64

[Philippe Mathieu-Daudé]

Hi Phil,

On 15/7/24 23:06, Phil Dennis-Jordan wrote:
> This change ensures that the MMIO write calls into the PVG
> framework are performed asynchronously on a background dispatch
> queue. Without this, we rapidly run into re-entrant MMIO issues.
> 
> This problem only seems to exist on x86-64 hosts. Conversely,
> doing it async on arm64/vmapple causes other issues,

Such as?

> so we're
> left with 2 different implementations.
> 
> Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
> ---
>   hw/display/apple-gfx.m | 15 ++++++++++++++-
>   1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/display/apple-gfx.m b/hw/display/apple-gfx.m
> index 806feb58fa..48463e5a1f 100644
> --- a/hw/display/apple-gfx.m
> +++ b/hw/display/apple-gfx.m
> @@ -67,15 +67,28 @@ static uint64_t apple_gfx_read(void *opaque, hwaddr offset, unsigned size)
>       return res;
>   }
>   
> -static void apple_gfx_write(void *opaque, hwaddr offset, uint64_t val, unsigned size)
> +static void apple_gfx_write(void *opaque, hwaddr offset, uint64_t val,
> +                            unsigned size)
>   {
>       AppleGFXState *s = opaque;
>   
>       trace_apple_gfx_write(offset, val);
>   
> +#ifdef __x86_64__
> +    id<PGDevice> dev = s->pgdev;
> +    dispatch_queue_t bg_queue = NULL;
> +
> +    bg_queue = dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0ul);
> +    [dev retain];
> +    dispatch_async(bg_queue, ^{
> +        [dev mmioWriteAtOffset:offset value:val];
> +        [dev release];
> +    });
> +#else
>       bql_unlock();
>       [s->pgdev mmioWriteAtOffset:offset value:val];
>       bql_lock();
> +#endif
>   }
>   
>   static const MemoryRegionOps apple_gfx_ops = {

Re:

[Akihiko Odaki]

On 2024/07/16 6:06, Phil Dennis-Jordan wrote:
> Date: Mon, 15 Jul 2024 21:07:12 +0200
> Subject: [PATCH 00/26] hw/display/apple-gfx: New macOS PV Graphics device
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
> 
> This sequence of patches integrates the paravirtualised graphics device
> implemented by macOS's ParavirtualizedGraphics.Framework into Qemu.
> Combined with the guest drivers which ship with macOS versions 11 and up,
> this allows the guest OS to use the host's GPU for hardware accelerated
> 3D graphics, GPGPU compute (both using the 'Metal' graphics API), and
> window compositing.
> 
> Some background:
> ----------------
> 
> The device exposed by the ParavirtualizedGraphics.Framework's (henceforth
> PVG) public API consists of a PCI device with a single memory-mapped BAR;
> the VMM is expected to pass reads and writes through to the framework, and
> to forward interrupts emenating from it to the guest VM.
> 
> The bulk of data exchange between host and guest occurs via shared memory,
> however. For this purpose, PVG makes callbacks to VMM code for allocating,
> mapping, unmapping, and deallocating "task" memory ranges. Each task
> represents a contiguous host virtual address range, and PVG expects the
> VMM to map specific guest system memory ranges to these host addresses via
> subsequent map callbacks. Multiple tasks can exist at a time, each with
> many mappings.
> 
> Data is exchanged via an undocumented, Apple-proprietary protocol. The
> PVG API only acts as a facilitator for establishing the communication
> mechanism. This is perhaps not ideal, and among other things means it
> only works on macOS hosts, but it's the only serious option we've got for
> good performance and quality graphics with macOS guests at this time.
> 
> The first iterations of this PVG integration into Qemu were developed
> by Alexander Graf as part of his "vmapple" machine patch series for
> supporting aarch64 macOS guests, and posted to qemu-devel in June and
> August 2023:
> 
> https://lore.kernel.org/all/20230830161425.91946-1-graf@amazon.com/T/
> 
> This integration mimics the "vmapple"/"apple-gfx" variant of the PVG device
> used by Apple's own VMM, Virtualization.framework. This variant does not use
> PCI but acts as a direct MMIO system device; there are two MMIO ranges, one
> behaving identically to the PCI BAR, while the other's functionality is
> exposed by private APIs in the PVG framework. It is only available on aarch64
> macOS hosts.
> 
> I had prior to this simultaneously and independently developed my own PVG
> integration for Qemu using the public PCI device APIs, with x86-64 and
> corresponding macOS guests and hosts as the target. After some months of
> use in production, I was slowly reviewing the code and readying it for
> upstreaming around the time Alexander posted his vmapple patches.
> 
> I ended up reviewing the vmapple PVG code in detail; I identified a number
> of issues with it (mainly thanks to my prior trial-and-error working with
> the framework) but overall I thought it a better basis for refinement
> than my own version:
> 
>   - It implemented the vmapple variant of the device. I thought it better to
>     port the part I understood well (PCI variant) to this than trying to port
>     the part I didn't understand well (MMIO vmapple variant) to my own code.
>   - The code was already tidier than my own.
> 
> It also became clear in out-of-band communication that Alexander would
> probably not end up having the time to see the patch through to inclusion,
> and was happy for me to start making changes and to integrate my PCI code.

Hi,

Thanks for continuing his effort.

Please submit a patch series that includes his patches. Please also 
merge fixes for his patches into them. This saves the effort to review 
the obsolete code and keeps git bisect working.

Regards,
Akihiko Odaki

Re: hw/display/apple-gfx

[Philippe Mathieu-Daudé]

On 16/7/24 08:07, Akihiko Odaki wrote:
> On 2024/07/16 6:06, Phil Dennis-Jordan wrote:
>> Date: Mon, 15 Jul 2024 21:07:12 +0200
>> Subject: [PATCH 00/26] hw/display/apple-gfx: New macOS PV Graphics device
>> MIME-Version: 1.0
>> Content-Type: text/plain; charset=UTF-8
>> Content-Transfer-Encoding: 8bit
>>
>> This sequence of patches integrates the paravirtualised graphics device
>> implemented by macOS's ParavirtualizedGraphics.Framework into Qemu.
>> Combined with the guest drivers which ship with macOS versions 11 and up,
>> this allows the guest OS to use the host's GPU for hardware accelerated
>> 3D graphics, GPGPU compute (both using the 'Metal' graphics API), and
>> window compositing.


> Hi,
> 
> Thanks for continuing his effort.

Yes!

> Please submit a patch series that includes his patches. Please also 
> merge fixes for his patches into them. This saves the effort to review 
> the obsolete code and keeps git bisect working.

Should be as easy as squashing patches 1-6, right?

Re: hw/display/apple-gfx

[Akihiko Odaki]

On 2024/07/16 15:38, Philippe Mathieu-Daudé wrote:
> On 16/7/24 08:07, Akihiko Odaki wrote:
>> On 2024/07/16 6:06, Phil Dennis-Jordan wrote:
>>> Date: Mon, 15 Jul 2024 21:07:12 +0200
>>> Subject: [PATCH 00/26] hw/display/apple-gfx: New macOS PV Graphics 
>>> device
>>> MIME-Version: 1.0
>>> Content-Type: text/plain; charset=UTF-8
>>> Content-Transfer-Encoding: 8bit
>>>
>>> This sequence of patches integrates the paravirtualised graphics device
>>> implemented by macOS's ParavirtualizedGraphics.Framework into Qemu.
>>> Combined with the guest drivers which ship with macOS versions 11 and 
>>> up,
>>> this allows the guest OS to use the host's GPU for hardware accelerated
>>> 3D graphics, GPGPU compute (both using the 'Metal' graphics API), and
>>> window compositing.
> 
> 
>> Hi,
>>
>> Thanks for continuing his effort.
> 
> Yes!
> 
>> Please submit a patch series that includes his patches. Please also 
>> merge fixes for his patches into them. This saves the effort to review 
>> the obsolete code and keeps git bisect working.
> 
> Should be as easy as squashing patches 1-6, right?

The cover letter says:

 > 04-13: These patches address issues identified during code review in
 > the original e-mail threads as well as my own review.

So these patches are certainly to be squashed. There are other patches 
titled "fixes" or "refactoring", which should also be squashed. I expect 
squashing them will reduce the number of patches (and code to review) 
drastically.

Regards,
Akihiko Odaki

Re: [PATCH 16/26] hw/display/apple-gfx: Asynchronous MMIO writes on x86-64

[Phil Dennis-Jordan]

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

On Mon, 15 Jul 2024 at 23:26, Philippe Mathieu-Daudé <philmd@linaro.org>
wrote:

> Hi Phil,
>
> On 15/7/24 23:06, Phil Dennis-Jordan wrote:
> > This change ensures that the MMIO write calls into the PVG
> > framework are performed asynchronously on a background dispatch
> > queue. Without this, we rapidly run into re-entrant MMIO issues.
> >
> > This problem only seems to exist on x86-64 hosts. Conversely,
> > doing it async on arm64/vmapple causes other issues,
>
> Such as?
>

Sorry for being vague. I've just refreshed my memory by testing with async
MMIO writes on aarch64, and the guest never manages to initialise the
display at all. I've admittedly not attempted to debug through this in any
significant way, though with PVG being something of a black box I'm not
sure it's worth it. It works reliably on x86-64 with async writes, and on
aarch64 with sync writes.

I'll add comments to the #ifdefs for v2.

Thanks,
Phil

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

Re: [PATCH 16/26] hw/display/apple-gfx: Asynchronous MMIO writes on x86-64

[BALATON Zoltan]

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

On Tue, 16 Jul 2024, Phil Dennis-Jordan wrote:
> On Mon, 15 Jul 2024 at 23:26, Philippe Mathieu-Daudé <philmd@linaro.org>
> wrote:
>
>> Hi Phil,
>>
>> On 15/7/24 23:06, Phil Dennis-Jordan wrote:
>>> This change ensures that the MMIO write calls into the PVG
>>> framework are performed asynchronously on a background dispatch
>>> queue. Without this, we rapidly run into re-entrant MMIO issues.
>>>
>>> This problem only seems to exist on x86-64 hosts. Conversely,
>>> doing it async on arm64/vmapple causes other issues,
>>
>> Such as?
>>
>
> Sorry for being vague. I've just refreshed my memory by testing with async
> MMIO writes on aarch64, and the guest never manages to initialise the
> display at all. I've admittedly not attempted to debug through this in any
> significant way, though with PVG being something of a black box I'm not
> sure it's worth it. It works reliably on x86-64 with async writes, and on
> aarch64 with sync writes.

Only a guess but I think ARM like POWER has weak memory consistency so 
maybe some sync ops are needed between writes somewhere whereas it would 
work on X86_64 that has strong guarantees so no such explicit sync is 
needed? I may completely wrong though, it's just what this reminded me of.

Regards,
BALATON Zoltan

Re: [PATCH 16/26] hw/display/apple-gfx: Asynchronous MMIO writes on x86-64

[Phil Dennis-Jordan]

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

On Tue, 16 Jul 2024 at 16:48, BALATON Zoltan <balaton@eik.bme.hu> wrote:

> Only a guess but I think ARM like POWER has weak memory consistency so
> maybe some sync ops are needed between writes somewhere whereas it would
> work on X86_64 that has strong guarantees so no such explicit sync is
> needed? I may completely wrong though, it's just what this reminded me of.
>

I didn't think this should matter for MMIO, which causes a VM exit instead
of a memory write.

The x86-64 and aarch64 binaries of Apple's PVG framework clearly diverge in
a number of ways. (For one, the x86-64 binary completely lacks the
IOSurface mapper sub-device used by Virtualization.framework and the
vmapple variant of the code here.) So I think the reason is more likely
down to implementation details in Apple's framework.

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

Re: hw/display/apple-gfx

[Phil Dennis-Jordan]

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

On Tue, 16 Jul 2024 at 08:48, Akihiko Odaki <akihiko.odaki@daynix.com>
wrote:

> The cover letter says:
>
>  > 04-13: These patches address issues identified during code review in
>  > the original e-mail threads as well as my own review.
>
> So these patches are certainly to be squashed. There are other patches
> titled "fixes" or "refactoring", which should also be squashed. I expect
> squashing them will reduce the number of patches (and code to review)
> drastically.
>

I've just resubmitted the patch set after squashing most of the commits
into the original one. (and rebasing on latest upstream) Squashing is easy,
pulling the individual changes back out is hard, so I figured I'd rather
send it as too many patches first time around in case anyone wanted to see
the individual changes I made.

Thanks for taking a look!

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

Re:

[Phil Dennis-Jordan]

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

On Tue, 16 Jul 2024 at 08:07, Akihiko Odaki <akihiko.odaki@daynix.com>
wrote:

> Hi,
>
> Thanks for continuing his effort.
>
> Please submit a patch series that includes his patches. Please also
> merge fixes for his patches into them. This saves the effort to review
> the obsolete code and keeps git bisect working.
>
>
Sorry about that - it looks like (a) my edits to the cover letter messed
something up and (b) patch 1 got email-filtered somewhere along the way for
having the "wrong" From: address. I've submitted v2 with most patches
squashed into patch 1, whose authorship I've also changed to myself (with
Co-authored-by tag for the original code) so hopefully this time around it
shows up OK.

Thanks,
Phil

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