Re: [PATCH v3 01/14] hw/display/apple-gfx: Introduce ParavirtualizedGraphics.Framework support

[Akihiko Odaki <akihiko.odaki@daynix.com>]

On 2024/10/02 22:33, Phil Dennis-Jordan wrote:
> Hi,
> 
> Thanks for taking a close look at this.
> 
> There are some further comments, explanations, and also a few questions 
> inline below. Where I've not commented, I'll just go ahead and make the 
> suggested change for v4.
> 
> On Tue, 1 Oct 2024 at 11:40, Akihiko Odaki <akihiko.odaki@daynix.com 
> <mailto:akihiko.odaki@daynix.com>> wrote:
> 
> 
>      > This patch implements a QEMU device that drives PVG for the VMApple
>      > variant of it.
> 
>     I think it is better to name it MMIO variant instead of VMApple. There
>     is nothing specific to VMApple in: hw/display/apple-gfx-vmapple.m
> 
> 
> I mean, I don't see it being useful for anything whatsoever outside the 
> vmapple machine type… But I guess there's little harm in renaming it.

I don't know but perhaps we may get vmapple2 or something in the future.

> 
> 
>      > +#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__, "");
> 
>     I don't think this assertion is worthwhile. This assertion will trigger
>     if you accidentally remove depends on AARCH64 from Kconfig, but I don't
>     think such code change happens by accident, and there is no reason to
>     believe that this assertion won't be removed in such a case.
> 
> 
> As far as I'm aware the Kconfig AARCH64 dependency is for the /target/ 
> architecture, not the /host/ architecture? The static assert checks for 
> the latter. The PGIOSurfaceHostDeviceDescriptor type isn't available at 
> all on non-aarch64 macOS hosts. I've not had any luck with using this 
> variant of the device on x86-64 hosts simply by disabling any surface 
> mapper code.
> 
> Incidentally, if you know of a way to depend on a specific /host/ 
> architecture in the Kconfig, that would be even better. I couldn't spot 
> a way of doing that though.

I got your intention now. The correct way to do that is to check for cpu 
== 'aarch64'. Having assertion will break qemu-system-aarch64 on Intel Macs.

> 
>      > +
>      > +/*
>      > + * ParavirtualizedGraphics.Framework only ships header files for
>     the PCI
>      > + * 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);
> 
>     Omit dummy parameter names.
> 
> 
>      > +@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();
> 
>     It is dangerous to unlock BQL at an arbitrary place. Instead of
>     unlocking, I suggest:
>     - running [s->pgiosfc mmioReadAtOffset:offset] on another thread
>     - using a bottom half to request operations that require BQL from the
>     thread running [s->pgiosfc mmioReadAtOffset:offset]
>     - calling AIO_WAIT_WHILE() to process the bottom half and to wait for
>     the completion of [s->pgiosfc mmioReadAtOffset:offset]
> 
> 
> OK, I think I see what you mean, I'll try to rework things around that 
> pattern. Any preference on how I kick off the job on the other thread? 
> As we necessarily need to use libdispatch in a bunch of places in this 
> code anyway, I guess dispatch_async() would probably be the simplest?

Perhaps so. The QEMU way is to use a bottom half with AioContext, but 
you can't simultaneously run a dispatch queue and AioContext in one 
thread so you have to use the dispatch queue if you need one.

> 
>      > +    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);
> 
>     Use: dma_memory_map()
> 
> 
> That doesn't seem to be a precisely equivalent operation. It also says 
> in its headerdoc,
> 
>     Use only for reads OR writes - not for read-modify-write operations.
> 
> which I don't think we can guarantee here at all.
> 
> I guess I can call it twice, once for writing and once for reading, but 
> given that the dma_memory_unmap operation marks the area dirty, I'm not 
> it's intended for what I understand the use case here to be: As far as I 
> can tell, the PV graphics device uses (some) of this memory to exchange 
> data in a cache-coherent way between host and guest, e.g. as a lock-free 
> ring buffer, using atomic operations as necessary. (This works because 
> it's a PV device: it "knows" the other end just another CPU core (or 
> even the same one) executing in a different hypervisor context.) This 
> doesn't really match "traditional" DMA patterns where there's either a 
> read or a write happening.

I think the story is a bit different for this VMApple variant. Probably 
the CPU and GPU in Apple Silicon is cache-coherent so you can map normal 
memory for GPU without any kind of cache maintenance.

Cache conherency of CPU and GPU in Apple Silicon is implied with Apple's 
documentation; it says you don't need to synchronize resources for 
MTLStorageModeShared, which is the default for Apple Silicon.
https://developer.apple.com/documentation/metal/resource_fundamentals/synchronizing_a_managed_resource_in_macos

The name "IOSurface" also implies it is used not only for e.g., ring 
buffer but also for real data.

> 
> Hunting around some more for alternative APIs, there's also 
> memory_region_get_ram_ptr(), but I'm not sure its restrictions apply 
> here either.

I think you can call memory_access_is_direct() to check if the 
requirement is satisfied.

It will still break dirty page tracking implemented by 
dma_memory_unmap() and others, but it's broken for hvf, which does not 
implement dirty page tracking either.

> 
>      > +            return (bool)true;
> 
>     Why cast?
> 
> 
> Good question. Not originally my code, so I've fixed all the instances I 
> could find now.
> 
>      > +        };
>      > +
>      > +    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 {
> 
>     This autoreleasepool is not used.
> 
> 
> It is definitely used inside the apple_gfx_common_realize() call. It's 
> also impossible to say whether [PGDeviceDescriptor new] uses autorelease 
> semantics internally, so it seemed safer to wrap the whole thing in an 
> outer pool.

Theoretically, It should be safe to assume the callee creates 
autoreleasepool by themselves as needed in general. We have bunch of 
code to call Objective-C APIs without creating autoreleasepool in the 
caller. Practically, [PGDeviceDescriptor new] is likely to be 
implemented with ARC, which wraps methods in autoreleasepool as necessary.

Functions that uses a method that returns autorelease resources should 
be wrapped with autoreleasepool instead of assuming the caller creates 
autoreleasepool for them.

> 
>      > +        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;
>      > +    }
>      > +}
>      > +
> 
>      > +++ b/hw/display/apple-gfx.h
>      > @@ -0,0 +1,57 @@
>      > +#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__
> 
>     This ifdef is unnecessary.
> 
> Ah indeed; at one point, vmapple.c was #including this file, but that's 
> no longer necessary.
> 
> 
>      > diff --git a/hw/display/apple-gfx.m b/hw/display/apple-gfx.m
>      > new file mode 100644
>      > index 00000000000..837300f9cd4
>      > --- /dev/null
>      > +++ b/hw/display/apple-gfx.m
>      > @@ -0,0 +1,536 @@
>      > +/*
>      > + * 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 "apple-gfx.h"
>      > +#include "trace.h"
>      > +#include "qemu/main-loop.h"
>      > +#include "ui/console.h"
>      > +#include "monitor/monitor.h"
>      > +#include "qapi/error.h"
>      > +#include "migration/blocker.h"
>      > +#include <mach/mach_vm.h>
>      > +#import <ParavirtualizedGraphics/ParavirtualizedGraphics.h>
>      > +
>      > +static const PGDisplayCoord_t apple_gfx_modes[] = {
>      > +    { .x = 1440, .y = 1080 },
>      > +    { .x = 1280, .y = 1024 },
>      > +};
>      > +
>      > +typedef struct PGTask_s { // Name matches forward declaration in
>     PG header
> 
>     Let's name it AppleGFXTask. It is a common practice to have the same
>     tag
>     name and typedef in QEMU.
> 
> 
> This is defining a forward-declared type from framework headers which is 
> opaque from the framework's point of view. We do not get to choose its 
> struct name. The alternative is having casts wherever these objects are 
> being passed between our code and the framework. (See the original v1/v2 
> vmapple patch series for how messy this is.)

I got the idea. Let's not avoid the typedef then to clarify the naming 
is not under our control.

> 
>      > +static void apple_gfx_render_new_frame(AppleGFXState *s)
>      > +{
>      > +    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];
>      > +    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];
> 
>     I don't think this call of retain is necessary. The completion handler
>     gets the command buffer via parameter, which implies the command buffer
>     is automatically retained until the completion handler finishes.
> 
> 
> I couldn't find any hard guarantees for this documented anywhere, hence 
> erring on the side of caution. I think the harm of an additional retain/ 
> release pair here is quite minor.

Apple's sample code doesn't retain for such a case so we can certainly 
omit it:
https://developer.apple.com/documentation/metal/indirect_command_encoding/encoding_indirect_command_buffers_on_the_cpu

It's better to avoid additional resource management when the underlying 
API automatically does things correct. There is a hazard of forgetting 
release anytime adding one call of retain.

> 
> 
>      > +static void apple_gfx_render_frame_completed(AppleGFXState *s,
>     void *vram,
>      > +                                             id<MTLTexture> texture)
>      > +{
>      > +    --s->pending_frames;
>      > +    assert(s->pending_frames >= 0);
>      > +
>      > +    if (vram != s->vram) {
>      > +        /* Display mode has changed, drop this old frame. */
>      > +        assert(texture != s->texture);
>      > +        g_free(vram);
> 
>     This management of buffers looks a bit convoluted. I suggest
>     remembering
>     the width and height instead of pointers and comparing them. This way
>     you can free resources in set_mode().
> 
> 
> Yeah, I suppose that works, I can change that around.
> 
>      > +    } else {
>      > +        copy_mtl_texture_to_surface_mem(texture, vram);
> 
>     Writing vram outside BQL may result in tearing.
> 
> 
> As far as I can tell(*), QXL does the same. I couldn't find any examples 
> of double-buffering in any of the existing display devices, which would 
> be the only way to do async updates efficiently and without tearing. In 
> any case, this solution is still vastly better than a regular VGA 
> device, which suffers from very visible tearing with macOS on the guest 
> side anyway. And in an ideal world, we'd pass through the rendered 
> texture directly to the Cocoa UI code rather than copying out only for 
> the CPU to draw it back into a window surface which is then passed to 
> the GPU for host side rendering. But I felt this patch is already very, 
> very large, and if anyone cares, we can fix imperfections in subsequent 
> updates.
> 
> (*)The rendering code in that device is also fairly complex, so I may be 
> misreading it.

QXL always modifies the surface with BQL. The surface is modified with 
qxl_blit(), which is called by qxl_render_update_area_unlocked(). 
qxl_render_update_area_unlocked() is called by either of 
qxl_render_update() and qxl_render_update_area_bh(). Both of them are 
called with BQL. The name includes "unlocked", but it means it is called 
without holding QXL-internal lock.

Most devices works entirely with BQL so they don't perform double 
buffering. apple-gfx can do the same.

> 
>      > +        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;
> 
>     This assignment is unnecessary as s->new_frame_ready is always false
>     when s->gfx_update_requested. If you want to make sure
>     s->gfx_update_requested and s->new_frame_ready are mutually exclusive,
>     use one enum value instead of having two bools.
> 
> 
> I'll need to refresh my memory and get back to you on this one, it's 
> been so many months since I actively worked on this code.
> 
>      > +        } 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) {
> 
>     It should check for s->new_frame_ready as
>     apple_gfx_render_frame_completed() doesn't check if
>     s->pending_frames > 0 before calling graphic_hw_update_done(), which is
>     inconsistent.
> 
> 
> pending_frames is about guest-side frames that are queued to be rendered 
> by the host GPU.
> new_frame_ready being true indicates that the contents of the Qemu 
> console surface has been updated with new frame data since the last 
> gfx_update.
> gfx_update_requested indicates that gfx_update is currently awaiting an 
> async completion (graphic_hw_update_done) but the surface has not 
> received a new frame content, but the GPU is stily busy drawing one.
> 
> apple_gfx_render_frame_completed is scheduled exactly once per pending 
> frame, so pending_frames > 0 is an invariant there. (Hence the assert.)> > I don't think there is any inconsistency here, but I'll double check.
> It's possible that there's an easier way to express the state machine, 
> and I'll take a look at that.

I meant that apple_gfx_render_frame_completed() does not check if the 
frame is the last one currently pending. apple_gfx_fb_update_display() 
ignores a new ready frame when there is a more pending frame, but 
apple_gfx_render_frame_completed() unconditionally fires 
graphic_hw_update_done() even if there is a more pending frame. And I 
think apple_gfx_render_frame_completed() is right and 
apple_gfx_fb_update_display() is wrong in such a situation.

> 
>     Checking if s->pending_frames > 0 both in apple_gfx_fb_update_display()
>     and apple_gfx_render_frame_completed() is also problematic as that can
>     defer graphic_hw_update_done() indefinitely if we are getting new
>     frames
>     too fast.
> 
> 
> I see what you mean about this part. I'll have to test it, but I guess 
> we should reverse the priority, like this:
> 
>          if (s->new_frame_ready) {
>              dpy_gfx_update_full(s->con);
>              s->new_frame_ready = false;
>              graphic_hw_update_done(s->con);
>          } else if (s->pending_frames > 0) {
>              s->gfx_update_requested = true;
>          } else {
>              graphic_hw_update_done(s->con);
>          }
> 
> 1. If we already have a frame, ready to be displayed, return it immediately.
> 2. If the guest has reported that it's completed a frame and the GPU is 
> currently busy rendering it, defer graphic_hw_update_done until it's done.
> 3. If the guest reports no changes to its display, indicate this back to 
> Qemu as a no-op display update graphic_hw_update_done() with no 
> dpy_gfx_update* call.

Yes, that looks correct.

> 
>      > +            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)
>      > +{
>      > +    dpy_mouse_set(s->con, s->pgdisp.cursorPosition.x,
>      > +                  s->pgdisp.cursorPosition.y, s->cursor_show);
>      > +}
>      > +
>      > +static void set_mode(AppleGFXState *s, uint32_t width, uint32_t
>     height)
>      > +{
>      > +    void *vram = NULL;
>      > +    DisplaySurface *surface;
>      > +    MTLTextureDescriptor *textureDescriptor;
>      > +    id<MTLTexture> texture = nil;
>      > +    __block bool no_change = false;
>      > +
>      > +    dispatch_sync(s->render_queue,
> 
>     Calling dispatch_sync() while holding BQL may result in deadlock.
> 
> Only if any code executed on the same dispatch queue acquires the lock 
> either directly or transitively. I believe I have ensure this is not 
> done on the reqnder_queue, have you found anywhere this is the case?

The documentation is not clear what threads a dispatch queue runs on. We 
can have a deadlock if they lock the BQL.

> 
>      > +        ^{
>      > +            if (s->surface &&
>      > +                width == surface_width(s->surface) &&
>      > +                height == surface_height(s->surface)) {
>      > +                no_change = true;
>      > +            }
>      > +        });
>      > +
>      > +    if (no_change) {
>      > +        return;
>      > +    }
>      > +
>      > +    vram = g_malloc0(width * height * 4);
>      > +    surface = qemu_create_displaysurface_from(width, height,
>     PIXMAN_LE_a8r8g8b8,
>      > +                                              width * 4, vram);
>      > +
>      > +    @autoreleasepool {
>      > +        textureDescriptor =
>      > +            [MTLTextureDescriptor
>      > +               
>     texture2DDescriptorWithPixelFormat:MTLPixelFormatBGRA8Unorm
>      > +                                             width:width
>      > +                                            height:height
>      > +                                         mipmapped:NO];
>      > +        textureDescriptor.usage = s->pgdisp.minimumTextureUsage;
>      > +        texture = [s->mtl
>     newTextureWithDescriptor:textureDescriptor];
>      > +    }
>      > +
>      > +    s->using_managed_texture_storage =
>      > +        (texture.storageMode == MTLStorageModeManaged);
>      > +
>      > +    dispatch_sync(s->render_queue,
>      > +        ^{
>      > +            id<MTLTexture> old_texture = nil;
>      > +            void *old_vram = s->vram;
>      > +            s->vram = vram;
>      > +            s->surface = surface;
>      > +
>      > +            dpy_gfx_replace_surface(s->con, surface);
>      > +
>      > +            old_texture = s->texture;
>      > +            s->texture = texture;
>      > +            [old_texture release];
> 
>     You can just do:
>     [s->texture release];
>     s->texture = texture;
> 
>     This will make s->texture dangling between the two statements, but that
>     don't matter since s->texture is not an atomic variable that can be
>     safely observed from another thread anyway.
> 
>      > +
>      > +            if (s->pending_frames == 0) {
>      > +                g_free(old_vram);
>      > +            }
>      > +        });
>      > +}
>      > +
>      > +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 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)
> 
>     This function can be merged into apple_gfx_common_realize().
> 
> 
> Probably. I'll try it.
> 
>      > +{
>      > +    Error *local_err = NULL;
>      > +    int r;
>      > +    size_t mmio_range_size =
>     apple_gfx_get_default_mmio_range_size();
>      > +
>      > +    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);
>      > +    s->iomem_gfx.disable_reentrancy_guard = true;
> 
>     Why do you disable reentrancy guard?
> 
> 
> Perhaps with the proposed AIO_WAIT_WHILE based I/O scheme, this won't be 
> an issue anymore, but the guard would otherwise keep dropping MMIOs 
> which immediately caused the PV graphics device to stop making progress. 
> The MMIO APIs in the PVG framework are thread- and reentrancy-safe, so 
> we certainly don't need to serialise them on our side.

It's better to understand why such reentrancy happens. Reentrancy itself 
is often a sign of bug.

> 
>      > +
>      > +    /* 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);
> 
>     Please report the error to the caller of apple_gfx_common_realize()
>     instead.
> 
>      > +        }
>      > +    }
>       > +}> +
>      > +static void
>     apple_gfx_register_task_mapping_handlers(AppleGFXState *s,
>      > +                                                   
>       PGDeviceDescriptor *desc)
>      > +{
>      > +    desc.createTask = ^(uint64_t vmSize, void * _Nullable *
>     _Nonnull baseAddress) {
>      > +        AppleGFXTask *task = apple_gfx_new_task(s, vmSize);
>      > +        *baseAddress = (void*)task->address;
> 
>     nit: please write as (void *) instead of (void*).
> 
>      > +        trace_apple_gfx_create_task(vmSize, *baseAddress);
>      > +        return task;
>      > +    };
>      > +
> 
>      > +{
>      > +    PGDisplayDescriptor *disp_desc = [PGDisplayDescriptor new];
>      > +
>      > + disp_desc.name <http://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();
>      > +        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) {
>      > +        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;
>      > +        size_t width = glyph.pixelsWide;
>      > +        size_t height = glyph.pixelsHigh;
>      > +        size_t padding_bytes_per_row = glyph.bytesPerRow - width
>     * 4;
>      > +        const uint8_t* px_data = glyph.bitmapData;
>      > +
>      > +        trace_apple_gfx_cursor_set(bpp, width, height);
>      > +
>      > +        if (s->cursor) {
>      > +            cursor_unref(s->cursor);
>      > +            s->cursor = NULL;
>      > +        }
>      > +
>      > +        if (bpp == 32) { /* Shouldn't be anything else, but just
>     to be safe...*/
>      > +            s->cursor = cursor_alloc(width, height);
>      > +            s->cursor->hot_x = hotSpot.x;
>      > +            s->cursor->hot_y = hotSpot.y;
>      > +
>      > +            uint32_t *dest_px = s->cursor->data;
>      > +
>      > +            for (size_t y = 0; y < height; ++y) {
>      > +                for (size_t x = 0; x < width; ++x) {
>      > +                    /* NSBitmapImageRep's red & blue channels
>     are swapped
>      > +                     * compared to QEMUCursor's. */
>      > +                    *dest_px =
>      > +                        (px_data[0] << 16u) |
>      > +                        (px_data[1] <<  8u) |
>      > +                        (px_data[2] <<  0u) |
>      > +                        (px_data[3] << 24u);
>      > +                    ++dest_px;
>      > +                    px_data += 4;
>      > +                }
>      > +                px_data += padding_bytes_per_row;
>      > +            }
>      > +            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);
>      > +    };
>      > +
>      > +    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 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;
>      > +
>      > +    QTAILQ_INIT(&s->tasks);
>      > +    s->render_queue = dispatch_queue_create("apple-gfx.render",
>      > +                                            DISPATCH_QUEUE_SERIAL);
>      > +    s->mtl = copy_suitable_metal_device();
>      > +    s->mtl_queue = [s->mtl newCommandQueue];
>      > +
>      > +    desc.device = s->mtl;
>      > +
>      > +    apple_gfx_register_task_mapping_handlers(s, desc);
>      > +
>      > +    s->pgdev = PGNewDeviceWithDescriptor(desc);
>      > +
>      > +    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();
>      > +
>      > +    create_fb(s);
>      > +}
>      > diff --git a/hw/display/meson.build b/hw/display/meson.build
>      > index 7db05eace97..70d855749c0 100644
>      > --- a/hw/display/meson.build
>      > +++ b/hw/display/meson.build
>      > @@ -65,6 +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_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 781f8a33203..1809a358e36 100644
>      > --- a/hw/display/trace-events
>      > +++ b/hw/display/trace-events
>      > @@ -191,3 +191,29 @@ 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) "[%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"
>      > +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_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
>      > +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 10464466ff3..f09df3f09d5 100644
>      > --- a/meson.build
>      > +++ b/meson.build
>      > @@ -741,6 +741,8 @@ socket = []
>      >   version_res = []
>      >   coref = []
>      >   iokit = []
>      > +pvg = []
>      > +metal = []
>      >   emulator_link_args = []
>      >   midl = not_found
>      >   widl = not_found
>      > @@ -762,6 +764,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'),
>