[PATCH v3 02/14] hw/display/apple-gfx: Adds PCI implementation

Phil Dennis-Jordan posted 14 patches 1 month, 3 weeks ago
There is a newer version of this series
[PATCH v3 02/14] hw/display/apple-gfx: Adds PCI implementation
Posted by Phil Dennis-Jordan 1 month, 3 weeks ago
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 code
shared with the vmapple/mmio variant of the PVG device.

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

diff --git a/hw/display/Kconfig b/hw/display/Kconfig
index 179a479d220..c2ec268f8e9 100644
--- a/hw/display/Kconfig
+++ b/hw/display/Kconfig
@@ -152,3 +152,8 @@ config MAC_PVG_VMAPPLE
     bool
     depends on MAC_PVG
     depends on ARM
+
+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 00000000000..9370258ee46
--- /dev/null
+++ b/hw/display/apple-gfx-pci.m
@@ -0,0 +1,138 @@
+/*
+ * QEMU Apple ParavirtualizedGraphics.framework device, PCI variant
+ *
+ * Copyright © 2023-2024 Phil Dennis-Jordan
+ *
+ * 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 as a PCI device
+ * aimed primarily at x86-64 macOS VMs.
+ */
+
+#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(Object *obj, ResetType type)
+{
+    AppleGFXPCIState *s = APPLE_GFX_PCI(obj);
+    [s->common.pgdev reset];
+}
+
+static void apple_gfx_pci_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    PCIDeviceClass *pci = PCI_DEVICE_CLASS(klass);
+    ResettableClass *rc = RESETTABLE_CLASS(klass);
+
+    assert(rc->phases.hold == NULL);
+    rc->phases.hold = 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 70d855749c0..ceb7bb07612 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-145)


Re: [PATCH v3 02/14] hw/display/apple-gfx: Adds PCI implementation
Posted by Akihiko Odaki 1 month, 3 weeks ago
On 2024/09/28 17:57, Phil Dennis-Jordan wrote:
> 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 code
> shared with the vmapple/mmio variant of the PVG device.
> 
> Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
> ---
>   hw/display/Kconfig         |   5 ++
>   hw/display/apple-gfx-pci.m | 138 +++++++++++++++++++++++++++++++++++++
>   hw/display/meson.build     |   1 +
>   3 files changed, 144 insertions(+)
>   create mode 100644 hw/display/apple-gfx-pci.m
> 
> diff --git a/hw/display/Kconfig b/hw/display/Kconfig
> index 179a479d220..c2ec268f8e9 100644
> --- a/hw/display/Kconfig
> +++ b/hw/display/Kconfig
> @@ -152,3 +152,8 @@ config MAC_PVG_VMAPPLE
>       bool
>       depends on MAC_PVG
>       depends on ARM
> +
> +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 00000000000..9370258ee46
> --- /dev/null
> +++ b/hw/display/apple-gfx-pci.m
> @@ -0,0 +1,138 @@
> +/*
> + * QEMU Apple ParavirtualizedGraphics.framework device, PCI variant
> + *
> + * Copyright © 2023-2024 Phil Dennis-Jordan
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.

Please use SPDX-License-Identifier instead.

> + *
> + * 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 as a PCI device
> + * aimed primarily at x86-64 macOS VMs.
> + */
> +
> +#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>

Please add #include "qemu/osdep.h" at top and reorder according to 
"Include directives" section in: docs/devel/style.rst

> +
> +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) {

option_rom_path != NULL is unnecessary; NSURL.h has 
NS_HEADER_AUDIT_BEGIN(nullability, sendability), which means any 
non-annotated member is non-nullable.

> +        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 */

It is unclear what "-daemonize ObjC fork crash" means. Please add more 
details.

> +        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,

s is unused.

> +                                    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);

You can just pass errp to msi_init().

> +        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(Object *obj, ResetType type)
> +{
> +    AppleGFXPCIState *s = APPLE_GFX_PCI(obj);
> +    [s->common.pgdev reset];
> +}
> +
> +static void apple_gfx_pci_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    PCIDeviceClass *pci = PCI_DEVICE_CLASS(klass);
> +    ResettableClass *rc = RESETTABLE_CLASS(klass);
> +
> +    assert(rc->phases.hold == NULL);
> +    rc->phases.hold = 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 70d855749c0..ceb7bb07612 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()


Re: [PATCH v3 02/14] hw/display/apple-gfx: Adds PCI implementation
Posted by Phil Dennis-Jordan 1 month, 3 weeks ago
On Wed, 2 Oct 2024 at 09:14, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:

> On 2024/09/28 17:57, Phil Dennis-Jordan wrote:
> > 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 code
> > shared with the vmapple/mmio variant of the PVG device.
> >
> > Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
> > ---
> >   hw/display/Kconfig         |   5 ++
> >   hw/display/apple-gfx-pci.m | 138 +++++++++++++++++++++++++++++++++++++
> >   hw/display/meson.build     |   1 +
> >   3 files changed, 144 insertions(+)
> >   create mode 100644 hw/display/apple-gfx-pci.m
> >
> > diff --git a/hw/display/Kconfig b/hw/display/Kconfig
> > index 179a479d220..c2ec268f8e9 100644
> > --- a/hw/display/Kconfig
> > +++ b/hw/display/Kconfig
> > @@ -152,3 +152,8 @@ config MAC_PVG_VMAPPLE
> >       bool
> >       depends on MAC_PVG
> >       depends on ARM
> > +
> > +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 00000000000..9370258ee46
> > --- /dev/null
> > +++ b/hw/display/apple-gfx-pci.m
> > @@ -0,0 +1,138 @@
> > +/*
> > + * QEMU Apple ParavirtualizedGraphics.framework device, PCI variant
> > + *
> > + * Copyright © 2023-2024 Phil Dennis-Jordan
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2 or
> later.
> > + * See the COPYING file in the top-level directory.
>
> Please use SPDX-License-Identifier instead.
>
> > + *
> > + * 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 as a PCI
> device
> > + * aimed primarily at x86-64 macOS VMs.
> > + */
> > +
> > +#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>
>
> Please add #include "qemu/osdep.h" at top and reorder according to
> "Include directives" section in: docs/devel/style.rst
>
> > +
> > +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) {
>
> option_rom_path != NULL is unnecessary; NSURL.h has
> NS_HEADER_AUDIT_BEGIN(nullability, sendability), which means any
> non-annotated member is non-nullable.
>
> > +        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 */
>
> It is unclear what "-daemonize ObjC fork crash" means. Please add more
> details.
>

When libvirt starts up a VM, it runs Qemu once in a dry-run state with the
-daemonize command line argument without actually running the VM. It then
queries various information about available features. It then shuts down
Qemu and re-runs it without -daemonize and actually lets the VM run.

The -daemonize command line argument causes fork() to be called. Apple's
Objective-C runtime does not support fork()ing and will immediately crash
the process when it detects it happening. QOM class init happens before the
fork(), so you can't call any Objective-C code from the class init
functions without causing a crash with -daemonize.

I can expand the comment somewhat for clarity.


> > +        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,
>
> s is unused.
>

Hmm, I suppose I should probably have a fallback legacy PCI IRQ trigger
here when MSI is not available.


> > +                                    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);
>
> You can just pass errp to msi_init().
>
> > +        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(Object *obj, ResetType type)
> > +{
> > +    AppleGFXPCIState *s = APPLE_GFX_PCI(obj);
> > +    [s->common.pgdev reset];
> > +}
> > +
> > +static void apple_gfx_pci_class_init(ObjectClass *klass, void *data)
> > +{
> > +    DeviceClass *dc = DEVICE_CLASS(klass);
> > +    PCIDeviceClass *pci = PCI_DEVICE_CLASS(klass);
> > +    ResettableClass *rc = RESETTABLE_CLASS(klass);
> > +
> > +    assert(rc->phases.hold == NULL);
> > +    rc->phases.hold = 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 70d855749c0..ceb7bb07612 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()
>
>
Re: [PATCH v3 02/14] hw/display/apple-gfx: Adds PCI implementation
Posted by BALATON Zoltan 1 month, 3 weeks ago
On Sat, 28 Sep 2024, Phil Dennis-Jordan wrote:
> 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 code
> shared with the vmapple/mmio variant of the PVG device.
>
> Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
> ---
> hw/display/Kconfig         |   5 ++
> hw/display/apple-gfx-pci.m | 138 +++++++++++++++++++++++++++++++++++++
> hw/display/meson.build     |   1 +
> 3 files changed, 144 insertions(+)
> create mode 100644 hw/display/apple-gfx-pci.m
>
> diff --git a/hw/display/Kconfig b/hw/display/Kconfig
> index 179a479d220..c2ec268f8e9 100644
> --- a/hw/display/Kconfig
> +++ b/hw/display/Kconfig
> @@ -152,3 +152,8 @@ config MAC_PVG_VMAPPLE
>     bool
>     depends on MAC_PVG
>     depends on ARM
> +
> +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 00000000000..9370258ee46
> --- /dev/null
> +++ b/hw/display/apple-gfx-pci.m
> @@ -0,0 +1,138 @@
> +/*
> + * QEMU Apple ParavirtualizedGraphics.framework device, PCI variant
> + *
> + * Copyright © 2023-2024 Phil Dennis-Jordan
> + *
> + * 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 as a PCI device
> + * aimed primarily at x86-64 macOS VMs.
> + */
> +
> +#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;

You don't need typedef here because OBJECT_DECLARE_SIMPLE_TYPE will add 
that. You can also put the struct AppleGFXPCIState definition after the 
OBJECT_DECLARE_SIMPLE_TYPE line. (See other devices for example.)

Regards,
BALATON Zoltan

> +
> +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(Object *obj, ResetType type)
> +{
> +    AppleGFXPCIState *s = APPLE_GFX_PCI(obj);
> +    [s->common.pgdev reset];
> +}
> +
> +static void apple_gfx_pci_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    PCIDeviceClass *pci = PCI_DEVICE_CLASS(klass);
> +    ResettableClass *rc = RESETTABLE_CLASS(klass);
> +
> +    assert(rc->phases.hold == NULL);
> +    rc->phases.hold = 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 70d855749c0..ceb7bb07612 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()
>
Re: [PATCH v3 02/14] hw/display/apple-gfx: Adds PCI implementation
Posted by Phil Dennis-Jordan 1 month, 3 weeks ago
On Sat, 28 Sept 2024 at 12:39, BALATON Zoltan <balaton@eik.bme.hu> wrote:

> > +typedef struct AppleGFXPCIState {
> > +    PCIDevice parent_obj;
> > +
> > +    AppleGFXState common;
> > +} AppleGFXPCIState;
>
> You don't need typedef here because OBJECT_DECLARE_SIMPLE_TYPE will add
> that. You can also put the struct AppleGFXPCIState definition after the
> OBJECT_DECLARE_SIMPLE_TYPE line. (See other devices for example.)
>
>
Thanks for pointing that out. I've locally applied that advice to this and
the other relevant type declarations in the series, it'll be in v4.

Phil