Re: [PATCH v16 00/14] macOS PV Graphics and new vmapple machine type

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 27/12/24 13:19, Phil Dennis-Jordan wrote:
> 
> 
> On Mon, 23 Dec 2024 at 23:58, Philippe Mathieu-Daudé <philmd@linaro.org 
> <mailto:philmd@linaro.org>> wrote:


>      > Alexander Graf (8):
>      >    hw: Add vmapple subdir
>      >    hw/misc/pvpanic: Add MMIO interface
>      >    gpex: Allow more than 4 legacy IRQs
>      >    hw/vmapple/aes: Introduce aes engine
>      >    hw/vmapple/bdif: Introduce vmapple backdoor interface
>      >    hw/vmapple/cfg: Introduce vmapple cfg region
>      >    hw/vmapple/virtio-blk: Add support for apple virtio-blk
>      >    hw/vmapple/vmapple: Add vmapple machine type
>      >
>      > Phil Dennis-Jordan (6):
>      >    ui & main loop: Redesign of system-specific main thread event
>     handling
>      >    hw/display/apple-gfx: Introduce ParavirtualizedGraphics.Framework
>      >      support
>      >    hw/display/apple-gfx: Adds PCI implementation
>      >    hw/display/apple-gfx: Adds configurable mode list
>      >    MAINTAINERS: Add myself as maintainer for apple-gfx, reviewer
>     for HVF
>      >    hw/block/virtio-blk: Replaces request free function with g_free
> 
>     If there are no objection or further comments, I'm taking this
>     series and will post a PR after xmas & testing.
> 
> 
> Thanks Phil, much appreciated! Enjoy your time off.
> 
> I've just posted an updated -v3 of my xhci patch series as v2 suffered 
> from the same breakage as this patch set. I recommend applying that one 
> on top for testing vmapple. It helps when you can use keyboard and mouse 
> properly and don't need to mess around with VNC.

I'm first taking patches 1-5 & 8 [*] squashing mostly style fixups,
TypeInfo rebase and header reduction ("qemu/osdep.h" is only for .c):

-- >8 --
diff --git a/hw/display/apple-gfx.h b/hw/display/apple-gfx.h
index a1160bf6619..3900cdbabbb 100644
--- a/hw/display/apple-gfx.h
+++ b/hw/display/apple-gfx.h
@@ -8,17 +8,14 @@
  #ifndef QEMU_APPLE_GFX_H
  #define QEMU_APPLE_GFX_H

-#define TYPE_APPLE_GFX_MMIO         "apple-gfx-mmio"
-#define TYPE_APPLE_GFX_PCI          "apple-gfx-pci"
-
-#include "qemu/osdep.h"
-#include <dispatch/dispatch.h>
-#import <ParavirtualizedGraphics/ParavirtualizedGraphics.h>
-#include "qemu/typedefs.h"
+#include "qemu/queue.h"
  #include "exec/memory.h"
  #include "hw/qdev-properties.h"
  #include "ui/surface.h"

+#define TYPE_APPLE_GFX_MMIO         "apple-gfx-mmio"
+#define TYPE_APPLE_GFX_PCI          "apple-gfx-pci"
+
  @class PGDeviceDescriptor;
  @protocol PGDevice;
  @protocol PGDisplay;
@@ -71,7 +68,7 @@ void *apple_gfx_host_ptr_for_gpa_range(uint64_t 
guest_physical,
                                         uint64_t length, bool read_only,
                                         MemoryRegion **mapping_in_region);

-extern const PropertyInfo qdev_prop_display_mode;
+extern const PropertyInfo qdev_prop_apple_gfx_display_mode;

  #endif

diff --git a/hw/display/apple-gfx-mmio.m b/hw/display/apple-gfx-mmio.m
index 60580a373d8..b2e0e7a30fa 100644
--- a/hw/display/apple-gfx-mmio.m
+++ b/hw/display/apple-gfx-mmio.m
@@ -3,9 +3,6 @@
   *
   * 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.
- *
   * SPDX-License-Identifier: GPL-2.0-or-later
   *
   * ParavirtualizedGraphics.framework is a set of libraries that macOS 
provides
@@ -16,13 +13,14 @@
   */

  #include "qemu/osdep.h"
-#import <ParavirtualizedGraphics/ParavirtualizedGraphics.h>
-#include "apple-gfx.h"
-#include "monitor/monitor.h"
+#include "qemu/log.h"
+#include "block/aio-wait.h"
  #include "hw/sysbus.h"
  #include "hw/irq.h"
+#include "apple-gfx.h"
  #include "trace.h"
-#include "qemu/log.h"
+
+#import <ParavirtualizedGraphics/ParavirtualizedGraphics.h>

  OBJECT_DECLARE_SIMPLE_TYPE(AppleGFXMMIOState, APPLE_GFX_MMIO)

@@ -32,10 +30,9 @@
   * their definitions here so that we can also work with the ARM version.
   */
  typedef bool(^IOSFCRaiseInterrupt)(uint32_t vector);
-typedef bool(^IOSFCUnmapMemory)(
-    void *, void *, void *, void *, void *, void *);
-typedef bool(^IOSFCMapMemory)(
-    uint64_t phys, uint64_t len, bool ro, void **va, void *, void *);
+typedef bool(^IOSFCUnmapMemory)(void *, void *, void *, void *, void *, 
void *);
+typedef bool(^IOSFCMapMemory)(uint64_t phys, uint64_t len, bool ro, 
void **va,
+                              void *, void *);

  @interface PGDeviceDescriptor (IOSurfaceMapper)
  @property (readwrite, nonatomic) bool usingIOSurfaceMapper;
@@ -168,8 +165,8 @@ static bool apple_gfx_mmio_unmap_surface_memory(void 
*ptr)
      RCU_READ_LOCK_GUARD();
      region = memory_region_from_host(ptr, &offset);
      if (!region) {
-        qemu_log_mask(LOG_GUEST_ERROR, "%s: memory at %p to be unmapped 
not "
-                      "found.\n",
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: memory at %p to be unmapped not found.\n",
                        __func__, ptr);
          return false;
      }
@@ -261,7 +258,7 @@ static void apple_gfx_mmio_reset(Object *obj, 
ResetType type)
  static const Property apple_gfx_mmio_properties[] = {
      DEFINE_PROP_ARRAY("display-modes", AppleGFXMMIOState,
                        common.num_display_modes, common.display_modes,
-                      qdev_prop_display_mode, AppleGFXDisplayMode),
+                      qdev_prop_apple_gfx_display_mode, 
AppleGFXDisplayMode),
  };

  static void apple_gfx_mmio_class_init(ObjectClass *klass, void *data)
@@ -276,7 +273,7 @@ static void apple_gfx_mmio_class_init(ObjectClass 
*klass, void *data)
      device_class_set_props(dc, apple_gfx_mmio_properties);
  }

-static TypeInfo apple_gfx_mmio_types[] = {
+static const TypeInfo apple_gfx_mmio_types[] = {
      {
          .name          = TYPE_APPLE_GFX_MMIO,
          .parent        = TYPE_SYS_BUS_DEVICE,
diff --git a/hw/display/apple-gfx-pci.m b/hw/display/apple-gfx-pci.m
index 4cfc01fc725..b939bb9b233 100644
--- a/hw/display/apple-gfx-pci.m
+++ b/hw/display/apple-gfx-pci.m
@@ -12,11 +12,12 @@
   * aimed primarily at x86-64 macOS VMs.
   */

-#include "apple-gfx.h"
+#include "qemu/osdep.h"
  #include "hw/pci/pci_device.h"
  #include "hw/pci/msi.h"
-#include "qapi/error.h"
+#include "apple-gfx.h"
  #include "trace.h"
+
  #import <ParavirtualizedGraphics/ParavirtualizedGraphics.h>

  OBJECT_DECLARE_SIMPLE_TYPE(AppleGFXPCIState, APPLE_GFX_PCI)
@@ -27,7 +28,7 @@
      AppleGFXState common;
  };

-static const char* apple_gfx_pci_option_rom_path = NULL;
+static const char *apple_gfx_pci_option_rom_path = NULL;

  static void apple_gfx_init_option_rom_path(void)
  {
@@ -117,7 +118,7 @@ static void apple_gfx_pci_reset(Object *obj, 
ResetType type)
  static const Property apple_gfx_pci_properties[] = {
      DEFINE_PROP_ARRAY("display-modes", AppleGFXPCIState,
                        common.num_display_modes, common.display_modes,
-                      qdev_prop_display_mode, AppleGFXDisplayMode),
+                      qdev_prop_apple_gfx_display_mode, 
AppleGFXDisplayMode),
  };

  static void apple_gfx_pci_class_init(ObjectClass *klass, void *data)
@@ -139,7 +140,7 @@ static void apple_gfx_pci_class_init(ObjectClass 
*klass, void *data)
      device_class_set_props(dc, apple_gfx_pci_properties);
  }

-static TypeInfo apple_gfx_pci_types[] = {
+static const TypeInfo apple_gfx_pci_types[] = {
      {
          .name          = TYPE_APPLE_GFX_PCI,
          .parent        = TYPE_PCI_DEVICE,
diff --git a/hw/display/apple-gfx.m b/hw/display/apple-gfx.m
index b00c72fc4dd..aa1455b6295 100644
--- a/hw/display/apple-gfx.m
+++ b/hw/display/apple-gfx.m
@@ -3,9 +3,6 @@
   *
   * 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.
- *
   * SPDX-License-Identifier: GPL-2.0-or-later
   *
   * ParavirtualizedGraphics.framework is a set of libraries that macOS 
provides
@@ -15,21 +12,24 @@
   */

  #include "qemu/osdep.h"
-#import <ParavirtualizedGraphics/ParavirtualizedGraphics.h>
-#include <mach/mach_vm.h>
-#include "apple-gfx.h"
-#include "trace.h"
-#include "qemu-main.h"
-#include "exec/address-spaces.h"
-#include "migration/blocker.h"
-#include "monitor/monitor.h"
-#include "qemu/main-loop.h"
+#include "qemu/lockable.h"
  #include "qemu/cutils.h"
  #include "qemu/log.h"
  #include "qapi/visitor.h"
  #include "qapi/error.h"
+#include "block/aio-wait.h"
+#include "exec/address-spaces.h"
  #include "system/dma.h"
+#include "migration/blocker.h"
  #include "ui/console.h"
+#include "apple-gfx.h"
+#include "trace.h"
+
+#include <mach/mach.h>
+#include <mach/mach_vm.h>
+#include <dispatch/dispatch.h>
+
+#import <ParavirtualizedGraphics/ParavirtualizedGraphics.h>

  static const AppleGFXDisplayMode apple_gfx_default_modes[] = {
      { 1920, 1080, 60 },
@@ -419,7 +419,7 @@ static void set_cursor_glyph(void *opaque)
          s->cursor = NULL;
      }

-    if (bpp == 32) { /* Shouldn't be anything else, but just to be 
safe...*/
+    if (bpp == 32) { /* Shouldn't be anything else, but just to be 
safe... */
          s->cursor = cursor_alloc(width, height);
          s->cursor->hot_x = job->hotspot.x;
          s->cursor->hot_y = job->hotspot.y;
@@ -467,7 +467,7 @@ static void apple_gfx_do_read_memory(void *opaque)

      r = dma_memory_read(&address_space_memory, job->physical_address,
                          job->dst, job->length, MEMTXATTRS_UNSPECIFIED);
-    job->success = r == MEMTX_OK;
+    job->success = (r == MEMTX_OK);

      qemu_sem_post(&job->sem);
  }
@@ -694,12 +694,11 @@ static void new_frame_handler_bh(void *opaque)
  static NSArray<PGDisplayMode *> *apple_gfx_create_display_mode_array(
      const AppleGFXDisplayMode display_modes[], uint32_t 
display_mode_count)
  {
-    uint32_t i;
      PGDisplayMode *mode_obj;
      NSMutableArray<PGDisplayMode *> *mode_array =
          [[NSMutableArray alloc] initWithCapacity:display_mode_count];

-    for (i = 0; i < display_mode_count; i++) {
+    for (unsigned i = 0; i < display_mode_count; i++) {
          const AppleGFXDisplayMode *mode = &display_modes[i];
          trace_apple_gfx_display_mode(i, mode->width_px, mode->height_px);
          PGDisplayCoord_t mode_size = { mode->width_px, mode->height_px };
@@ -836,8 +835,8 @@ static void apple_gfx_set_display_mode(Object *obj, 
Visitor *v,

      ret = qemu_strtoi(endptr, &endptr, 10, &val);
      if (ret || val > UINT16_MAX || val <= 0) {
-        error_setg(errp, "width in '%s' must be a decimal integer number "
-                   "of pixels in the range 1..65535", name);
+        error_setg(errp, "width in '%s' must be a decimal integer number"
+                         " of pixels in the range 1..65535", name);
          return;
      }
      mode->width_px = val;
@@ -847,8 +846,8 @@ static void apple_gfx_set_display_mode(Object *obj, 
Visitor *v,

      ret = qemu_strtoi(endptr + 1, &endptr, 10, &val);
      if (ret || val > UINT16_MAX || val <= 0) {
-        error_setg(errp, "height in '%s' must be a decimal integer number "
-                   "of pixels in the range 1..65535", name);
+        error_setg(errp, "height in '%s' must be a decimal integer number"
+                         " of pixels in the range 1..65535", name);
          return;
      }
      mode->height_px = val;
@@ -859,18 +858,18 @@ static void apple_gfx_set_display_mode(Object 
*obj, Visitor *v,
      ret = qemu_strtoi(endptr + 1, &endptr, 10, &val);
      if (ret || val > UINT16_MAX || val <= 0) {
          error_setg(errp, "refresh rate in '%s'"
-                   " must be a positive decimal integer (Hertz)", name);
+                         " must be a positive decimal integer (Hertz)", 
name);
          return;
      }
      mode->refresh_rate_hz = val;
      return;

  separator_error:
-    error_setg(errp, "Each display mode takes the format "
-               "'<width>x<height>@<rate>'");
+    error_setg(errp,
+               "Each display mode takes the format 
'<width>x<height>@<rate>'");
  }

-const PropertyInfo qdev_prop_display_mode = {
+const PropertyInfo qdev_prop_apple_gfx_display_mode = {
      .name  = "display_mode",
      .description =
          "Display mode in pixels and Hertz, as 
<width>x<height>@<refresh-rate> "
---

Regards,

Phil.

[*] I'll share the rebased branch later.