Re: [PATCH v17 00/11] New vmapple machine type and xhci fixes

[Michael S. Tsirkin]

On Wed, Jan 15, 2025 at 04:33:45PM +0100, Phil Dennis-Jordan wrote:
> 
> 
> On Wed, 15 Jan 2025 at 16:08, Michael S. Tsirkin <mst@redhat.com> wrote:
> 
>     On Sun, Jan 12, 2025 at 10:00:45PM +0100, Phil Dennis-Jordan wrote:
>     > This patch set introduces a new ARM and macOS HVF specific machine type
>     > called "vmapple". There are also some patches for fixing XHCI spec
>     > compliance issues and adding a workaround to a quirk in the macOS 
> guest's
>     > XHCI driver.
>     >
>     > The vmapple machine type approximates the configuration in macOS's own
>     > Virtualization.framework when running arm64 macOS guests. In addition to
>     > generic components such as a GICv3 and an XHCI USB controller, it
>     > includes nonstandard extensions to the virtio block device, a special
>     > "hardware" aes engine, a configuration device, a pvpanic variant, a
>     > "backdoor" interface, and of course the apple-gfx paravirtualised 
> display
>     > adapter.
>     >
>     >
>     > The macOS guest initially did not work well with QEMU's XHCI controller,
>     > which required some investigation, bug fixing, and a work-around.
>     >
>     > Essentially, the macOS driver attempts to use XHCI event rings 1 and 2
>     > even when there is only a single pin-based interrupt available. The
>     > interrupts for rings 1 and 2 are dropped, and so events are only handled
>     > after a timeout. The driver appears to expect the device to act as if
>     > interrupter mapping was not supported - the spec only mentions that
>     > interrupter mapping should be disabled if only one interrupter is
>     > enabled, not one interrupt, although there is potential ambiguity in
>     > the spec's wording around enabling and disabling interrupters.
>     >
>     > In any case, this investigation has led to 3 changes:
>     >
>     >  * The spec requires that modulo arithmetic be used for selecting
>     >    the MSI vector to notify from the interrupter/event ring index.
>     >    (Patch 1)
>     >  * The spec requires that all events be directed at ring 0 if
>     >    interrupter mapping is not available; the condition for this
>     >    mentioned in the spec is when there is only 1 interrupter
>     >    available. (Patch 2)
>     >  * A property is added to the PCI XHCI controller classes to disable
>     >    interrupter mapping when using pin-based interrupts. This makes
>     >    the macOS guest drivers work. (Patch 9) This is enabled in the
>     >    vmapple machine type, which does not offer MSI(-X) support.
>     >
>     > There are currently a few limitations to the vmapple machine. These
>     > aren't intrinsic, just imperfect emulation of the VZF, but it's good
>     > enough to be just about usable for some purposes:
>     >
>     >  * macOS 12 guests only. Versions 13+ currently fail during early boot.
>     >  * macOS 11+ arm64 hosts only, with hvf accel. (Perhaps some differences
>     >    between Apple M series CPUs and TCG's aarch64 implementation? macOS
>     >    hosts only because ParavirtualizedGraphics.framework is a black box
>     >    implementing most of the logic behind the apple-gfx device.)
>     >  * The guest OS must first be provisioned using 
> Virtualization.framework;
>     >    the disk images can subsequently be used in Qemu. (See docs.)
>     >
>     >
>     > Previous versions of this series also included the macOS PV graphics
>     > device ("apple-gfx"); those patches have already been merged, so
>     > the title has been changed. Previous iteration:
>     > https://patchew.org/QEMU/20241223221645.29911-1-phil@philjordan.eu/
>     >
>     > Furthermore, the XHCI fixes and workaround were previously submitted
>     > as a separate patch set, of which a few patches have also been merged.
>     > "hw/usb/hcd-xhci: Fixes, improvements and macOS workaround"
>     > https://patchew.org/QEMU/20241227121336.25838-1-phil@philjordan.eu/
>     >
>     > Finally, I've included one of Philippe Mathieu-Daudé's GICv3 patches
>     > which arose out of the discovery that the software GICv3 dependency
>     > was missing when building v16 and earlier versions of this series
>     > in a HVF-only configuration.
>     > https://patchew.org/QEMU/20241227202435.48055-1-philmd@linaro.org/
> 
> 
>     just to make sure, who is merging this? Me?
> 
> 
> ICYMI, Phil M-D has already pulled patches 1 & 2, and Stefan has merged them
> into staging & master. 
> 
> As I understand it, Phil M-D was also in the process of merging the rest of 
> v16
> of this series before discovering the GICv3 dependency/Kconfig issue:
> 
> https://lore.kernel.org/qemu-devel/
> 8554330d-fd9b-4fa5-b37c-161f70b71f7d@linaro.org/
> 
> I don't really mind either way who merges this, but he might have some more
> thoughts on the matter - there might be a specific reason he's so far only
> picked up those two patches. :-)
> 

Excellent just making sure it's not lost.
Thanks!

>     > ---
>     >
>     > v2 -> v3:
>     >
>     >  * Merged the apple-gfx and vmapple patchsets.
>     >  * Squashed a bunch of later apple-gfx patches into the main one.
>     >    (dGPU support, queried MMIO area size, host GPU picking logic.)
>     >  * Rebased on latest upstream, fixing any breakages due to internal
>     >    Qemu API changes.
>     >  * apple-gfx: Switched to re-entrant MMIO. This is supported by the
>     >    underlying framework and simplifies the MMIO forwarding code which
>     >    was previously different on x86-64 vs aarch64.
>     >  * vmapple: Fixes for minor bugs and comments from the last round of
>     >    review.
>     >  * vmapple aes, conf, apple-gfx: Switched reset methods to implement
>     >    the ResettableClass base's interface.
>     >  * vmapple: switched from virtio-hid to an XHCI USB controller and
>     >    USB mouse and tablet devices. macOS does not provide drivers for
>     >    virtio HID devices, at least not in version 12's vmapple kernel.
>     >    So input now sort of works (interrupt issues) rather than not
>     >    at all. Use network-based remote access to the guest OS as a
>     >    work-around.
>     >
>     > v3 -> v4:
>     >
>     >  * Complete rework of the mechanism for handling runloop/libdispatch
>     >    events on the main thread. PV graphics now work with the SDL UI.
>     >  * Renamed 'apple-gfx-vmapple' device to 'apple-gfx-mmio'
>     >  * hw/display/apple-gfx: threading model overhaul to be more consistent,
>     >    safer, and more QEMU-idiomatic.
>     >  * display-modes property on the apple-gfx devices now uses the
>     >    native array property mechanism and works on both device variants.
>     >  * hw/vmapple/aes: Improvements to logging and error handling.
>     >  * hw/vmapple/cfg: Bug fixes around device property default values.
>     >  * hw/vmapple/{aes,cfg,virtio-blk/vmapple}: Most header code moved into
>     >    .c files, only a single vmapple.h now contains the #defines for the
>     >    vmapple machine model-specific device type names.
>     >  * hw/block/virtio-blk: New patch for replacing virtio_blk_free_request
>     >    with g_free. (Optional)
>     >  * Various smaller changes following comments in v3 code review in
>     >    apple-gfx, aes, cfg, bdif, virtio-blk-vmapple, and the vmapple
>     >    machine type itself. See patch-specific v4 change notes for details.
>     >
>     > v4 -> v5:
>     >
>     >  * Simplified the main thread runloop mechanism. Back to setting
>     >        qemu_main directly, but narrowing the scope of what it needs to
>     do,
>     >        and it can now be NULL. (Meaning run the QEMU main event loop on
>     >        the main thread as is traditional.)
>     >  * hw/display/apple-gfx: Further improvements to the BH based job code
>     bridging
>     >    the libdispatch & QEMU thread synchronisation impedance mismatch.
>     >  * hw/display/apple-gfx: Thread safety and object lifetime improvements.
>     >  * hw/display/apple-gfx-*: Better buffer and error handling in display
>     mode
>     >    property setters and getters.
>     >  * hw/vmapple/aes: More consistent and safer logging/tracing
>     >  * hw/vmapple/cfg: Better error reporting on overlong property strings.
>     >  * hw/vmapple/virtio-blk: Fixed theoretically-unaligned write to config
>     buffer.
>     >  * vmapple machine type: Moved ecam region into machine state, improved
>     device
>     >    property setting error handling, improved ECID/UUID extraction script
>     and
>     >    docs.
>     >  * Various smaller fixes in apple-gfx/-mmio, apple-gfx-pci, vmapple/aes,
>     >    vmapple/cfg, vmapple/virtio-blk, and vmapple machine type.
>     >  * Added SPDX license identifiers where they were missing.
>     >
>     > v5 -> v6:
>     >
>     >  * 01/15 (main/Cocoa/runloop): Combined functions, fixed whitespace
>     >  * 02/15 (apple-gfx): Further refinement of PVG threading: reduced some
>     callback
>     >    tasks from BHs to merely acquiring RCU read lock; replaced some
>     libdispatch
>     >    tasks with BHs; last remaining synchronous BH now uses emphemeral
>     >    QemuSemaphore.
>     >  * 02/15 (apple-gfx): Readability improvements and other smaller tweaks
>     >    (see patch change notes for details)
>     >  * 04/15 (display modes): Replaced use of alloca() with NSMutableArray.
>     >
>     > v6 -> v7:
>     >
>     >  * 02/15 (apple-gfx): Use g_ptr_array_find() helper function, coding
>     style tweak
>     >  * 03/15 (apple-gfx-pci): Removed an unused function parameter
>     >  * 04/15 (apple-gfx display mode property): Simplified error handling in
>     >    property parsing.
>     >  * 10/15 (vmapple/aes): Coding style tweaks.
>     >  * 12/15 (vmapple/cfg): Changed error messages for overrun of properties
>     with
>     >    fixed-length strings to be more useful to users than developers.
>     >  * 15/15 (vmapple machine type): Tiny error handling fix, un-inlined
>     function
>     >
>     > v7 -> v8:
>     >
>     >  * 02/15 (apple-gfx): Naming and type use improvements, fixes for a bug
>     and a
>     >    leak.
>     >  * 04/15 (apple-gfx display mode property): Type use improvement
>     >  * 10/15 (vmapple/aes): Guest error logging tweaks.
>     >  * 11/15 (vmapple/bdif): Replaced uses of cpu_physical_memory_read with
>     >    dma_memory_read, and a g_free call with g_autofree.
>     >  * 12/15 (vmapple/cfg): Macro hygiene fix: consistently enclosing
>     arguments in
>     >    parens.
>     >  * 15/15 (vmapple machine type): Use less verbose pattern for defining
>     uuid
>     >    property.
>     >
>     > v8 -> v9:
>     >
>     >  * 01/16 (ui & main loop): Set qemu_main to NULL for GTK UI as well.
>     >  * 02/16 (apple-gfx): Pass device pointer to graphic_console_init(),
>     various
>     >        non-functional changes.
>     >  * 03/16 (apple-gfx-pci): Fixup of changed common call, whitespace and
>     comment
>     >    formatting tweaks.
>     >  * 04/16 (apple-gfx display modes): Re-ordered type definitions so we 
> can
>     drop
>     >    a 'struct' keyword.
>     >  * 10/16 (vmapple/aes): Replaced a use of cpu_physical_memory_write with
>     >    dma_memory_write, minor style tweak.
>     >  * 11/16 (vmapple/bdif): Replaced uses of cpu_physical_memory_write with
>     >    dma_memory_write.
>     >  * 13/16 (vmapple/virtio-blk): Correctly specify class_size for
>     >    VMAppleVirtIOBlkClass.
>     >  * 15/16 (vmapple machine type): Documentation improvements, fixed
>     variable
>     >    name and struct field used during pvpanic device creation.
>     >  * 16/16 (NEW/RFC vmapple/virtio-blk): Proposed change to replace type
>     hierarchy
>     >    with a variant property. This seems cleaner and less confusing than
>     the
>     >    original approach to me, but I'm not sure if it warrants creation of 
> a
>     new
>     >    QAPI enum and property type definition.
>     >
>     > v9 -> v10:
>     >
>     >  * 01/15 (ui & main loop): Added comments to qemu_main declaration and
>     GTK.
>     >  * 02/15 (apple-gfx): Reworked the way frame rendering code is threaded
>     to use
>     >    BHs for sections requiring BQL.
>     >  * 02/15 (apple-gfx): Fixed ./configure error on non-macOS platforms.
>     >  * 10/15 (vmapple/aes): Code style and comment improvements.
>     >  * 12/15 (vmapple/cfg): Slightly tidier error reporting for overlong
>     property
>     >    values.
>     >  * 13/15 (vmapple/virtio-blk): Folded v9 patch 16/16 into this one,
>     changing
>     >    the device type design to provide a single device type with a variant
>     >        property instead of 2 different subtypes for aux and root 
> volumes.
>     >  * 15/15 (vmapple machine type): Documentation fixup for changed
>     virtio-blk
>     >    device type; small improvements to shell commands in documentation;
>     >    improved propagation of errors during cfg device instantiation.
>     >
>     > v10 -> v11:
>     >
>     >  * 01/15 (ui & main loop): Simplified main.c, better comments & commit
>     message
>     >  * 02/15 (apple-gfx): Give each PV display instance a unique serial
>     number.
>     >  * 02 & 03/15 (apple-gfx, -pci): Formatting/style tweaks
>     >  * 15/15 (vmapple machine type): Improvements to shell code in docs
>     >
>     > v11 -> v12:
>     >
>     >  * 01/15 (ui & main loop): More precise wording of code comments.
>     >  * 02/15 (apple-gfx): Fixed memory management regressions introduced in
>     v10;
>     >    improved error handling; various more conmetic code adjustments
>     >  * 09/15 (GPEX): Fixed uses of deleted GPEX_NUM_IRQS constant that have
>     been
>     >    added to QEMU since this patch was originally written.
>     >
>     > v12 -> v13:
>     >
>     >  * 15/15 (vmapple machine type): Bumped the machine type version from 
> 9.2
>     >    to 10.0.
>     >  * All patches in the series now have been positively reviewed and
>     received
>     >    corresponding reviewed-by tags.
>     >
>     > v13 -> v14:
>     >
>     >  * 6/15 (hw/vmapple directory): Changed myself from reviewer
>     >    to maintainer, as that seemed appropriate at this point.
>     >  * 15/15 (vmapple machine type): Gate creation of XHCI and
>     >    USB HID devices behind if (defaults_enabled()).
>     >
>     > v14 -> v15
>     >
>     >  * Constified property tables to match Richard Henderson's recent
>     project-
>     >    wide convention change. (patches 4/15, 7/15, 11/15, 12/15, & 13/15)
>     >
>     > v15 -> v16
>     >
>     >  * 14 patches now, as patch 8 has already been pulled. (Thanks 
> Philippe!)
>     >  * Fixed a bunch of conflicts with upstream code motion:
>     >     - DEFINE_PROP_END_OF_LIST removal (4/14 - apple-gfx mode list, 7/14 
> -
>     >       pvpanic-mmio, 10/14 - bdif, 11/14 - cfg device, and
>     >       12/14 - vmapple-virtio-blk)
>     >     - sysemu->system move/rename: (1/14 - ui/qemu-main, 2/14 - 
> apple-gfx,
>     >       9/14 - aes, 10/14 - bdif, 14/14 - vmapple machine type)
>     >  * 14/14 (vmapple machine type):
>     >     - Moved compatibility setting for removing legacy mode from
>     virtio-pci
>     >       to proper global property table rather than (ab)using sugar
>     property.
>     >     - Removed a few superfluous #includes during sysemu rename cleanup.
>     >     - Removed machine type versioning as it's not necessary (yet?)
>     >     - Made memory map array const
>     >
>     > XHCI RFC -> v1:
>     >
>     >  * Gated conditional interrupter mapping support behind a property,
>     enabled
>     >    that property in the VMApple machine type.
>     >  * Added patch to fix the MSI vector assertion failure.
>     >  * Moved msi and msix properties from NEC XHCI controller to generic
>     xhci-pci
>     >    superclass as that also seems useful.
>     >  * Broke the workaround up into 2 patches, one for mapping disabling
>     required
>     >    by the standard, and one for the conditional disabling workaround.
>     >
>     > XHCI v1 -> v2:
>     >
>     >  * 1/6: Switch to modulo arithmetic for MSI vector number, as per spec.
>     >  * 6/6: Set the "conditional-intr-mapping" property via compat_props.
>     >  * Commit message tweaks
>     >
>     > XHCI v2 -> v3:
>     >
>     >  * 2/6: In line with recent upstream changes, the property table is now
>     >    const and no longer carries an end-of-list marker.
>     >  * The indentation fix (previously 5/6) has already been merged, so is 
> no
>     >    longer included.
>     >  * Added patch fixing up logging of certain unhandled MMIO cases. (4/6)
>     >  * 6/6: Moved the compat global property table into vmapple patch set
>     -v16;
>     >    we now just add the conditional-intr-mapping property to it in this
>     >    patch. We also set the property on any device implementing the
>     abstract
>     >    TYPE_XHCI_PCI rather than only the TYPE_QEMU_XHCI device 
> specifically.
>     >
>     > v16 -> v17
>     >
>     >  * Rebased on latest upstream (with minor conflict fixes)
>     >  * apple-gfx, GPEX, and ui/cocoa patches dropped as they have been
>     merged.
>     >  * Unmerged patches from xhci series v3 combined into this series.
>     >  * vmapple machine type: Explicitly depend on software GICv3.
>     >  * vmapple machine type: Enable the new XHCI PCI 
> conditional-intr-mapping
>     >    property via the machine type's global compat property table.
>     >  * Integrated Philippe's patch on renaming the GICv3's confusing config
>     name,
>     >    and removing its TCG dependency. (It's needed with HVF too.)
>     >  * vmapple machine type: Dropped Tested-by tag because of above changes
>     >
>     >
>     > Alexander Graf (7):
>     >   hw: Add vmapple subdir
>     >   hw/misc/pvpanic: Add MMIO interface
>     >   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 (3):
>     >   hw/usb/hcd-xhci-pci: Use modulo to select MSI vector as per spec
>     >   hw/usb/hcd-xhci-pci: Use event ring 0 if mapping unsupported
>     >   hw/usb/hcd-xhci-pci: Adds property for disabling mapping in IRQ mode
>     >
>     > Philippe Mathieu-Daudé (1):
>     >   hw/intc: Remove TCG dependency on ARM_GICV3
>     >
>     >  MAINTAINERS                         |   8 +
>     >  contrib/vmapple/uuid.sh             |   9 +
>     >  docs/system/arm/vmapple.rst         |  63 +++
>     >  docs/system/target-arm.rst          |   1 +
>     >  hw/Kconfig                          |   1 +
>     >  hw/block/virtio-blk.c               |  19 +-
>     >  hw/core/qdev-properties-system.c    |   8 +
>     >  hw/intc/Kconfig                     |   6 +-
>     >  hw/intc/meson.build                 |   4 +-
>     >  hw/meson.build                      |   1 +
>     >  hw/misc/Kconfig                     |   4 +
>     >  hw/misc/meson.build                 |   1 +
>     >  hw/misc/pvpanic-mmio.c              |  60 +++
>     >  hw/usb/hcd-xhci-pci.c               |  25 ++
>     >  hw/usb/hcd-xhci-pci.h               |   1 +
>     >  hw/usb/hcd-xhci.c                   |   5 +
>     >  hw/usb/hcd-xhci.h                   |   5 +
>     >  hw/vmapple/Kconfig                  |  32 ++
>     >  hw/vmapple/aes.c                    | 581 ++++++++++++++++++++++++++
>     >  hw/vmapple/bdif.c                   | 274 ++++++++++++
>     >  hw/vmapple/cfg.c                    | 195 +++++++++
>     >  hw/vmapple/meson.build              |   5 +
>     >  hw/vmapple/trace-events             |  21 +
>     >  hw/vmapple/trace.h                  |   1 +
>     >  hw/vmapple/virtio-blk.c             | 204 +++++++++
>     >  hw/vmapple/vmapple.c                | 618 ++++++++++++++++++++++++++++
>     >  include/hw/misc/pvpanic.h           |   1 +
>     >  include/hw/pci/pci_ids.h            |   1 +
>     >  include/hw/qdev-properties-system.h |   5 +
>     >  include/hw/virtio/virtio-blk.h      |  11 +-
>     >  include/hw/vmapple/vmapple.h        |  23 ++
>     >  include/qemu/cutils.h               |  15 +
>     >  meson.build                         |   1 +
>     >  qapi/virtio.json                    |  14 +
>     >  util/hexdump.c                      |  18 +
>     >  35 files changed, 2231 insertions(+), 10 deletions(-)
>     >  create mode 100755 contrib/vmapple/uuid.sh
>     >  create mode 100644 docs/system/arm/vmapple.rst
>     >  create mode 100644 hw/misc/pvpanic-mmio.c
>     >  create mode 100644 hw/vmapple/Kconfig
>     >  create mode 100644 hw/vmapple/aes.c
>     >  create mode 100644 hw/vmapple/bdif.c
>     >  create mode 100644 hw/vmapple/cfg.c
>     >  create mode 100644 hw/vmapple/meson.build
>     >  create mode 100644 hw/vmapple/trace-events
>     >  create mode 100644 hw/vmapple/trace.h
>     >  create mode 100644 hw/vmapple/virtio-blk.c
>     >  create mode 100644 hw/vmapple/vmapple.c
>     >  create mode 100644 include/hw/vmapple/vmapple.h
>     >
>     > --
>     > 2.39.5 (Apple Git-154)
> 
>