[PATCH 0/6] hw/usb/hcd-xhci: Fixes, improvements, and macOS workaround

[PATCH 0/6] hw/usb/hcd-xhci: Fixes, improvements, and macOS workaround

[Phil Dennis-Jordan]

For a while now, I've been chasing the problem of macOS's XHCI guest driver not
working properly with QEMU's PCI XHCI controller when MSI-X is unavailable.
I've finally figured out the cause, and I think an acceptable solution. I've
explained the problem and quoted the relevant sections of the XHCI spec in more
detail in the linked GitLab issue:

https://gitlab.com/qemu-project/qemu/-/issues/2705

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 some ambiguity in the spec's wording around enabling and
disabling interrupters.

After feedback to my initial RFC submission and discovering some more unhandled
edge cases, I've now split this up into 6 different patches.

Ultimately, for macOS to be able to drive the XHCI controller with MSI-X
disabled, we need to disable interrupter mapping (in the sense of XHCI spec
section 4.17.1) when using a pin-based interrupt or MSI with only 1 vector
active.

 1. Fixes an assertion failure crash when XHCI attempts to raise an interrupt
    on a MSI vector higher than the maximum allocated. It's not a smart thing
    for a guest to do, scheduling events on interrupter 2 when it's only
    configured a single MSI vector, but we also don't want to crash in this
    case.
 2. Moves the msi/msix toggles from the NEC XHCI controller to the generic
    hci-xhci-pci superclass for consistency. This makes testing with MSI-X
    and/or MSI disabled easier when using the qemu-xhci device variant.
 3. Implements interrupter mapping disabling as per XHCI spec, when numintrs==1.
 4. Add a new boolean property to hcd-xhci-pci, "conditional-intr-mapping",
    which defaults to false, retaining existing behaviour. When set to true,
    additional constraints for enabling interrupter mapping are enabled,
    so it is disabled in pin-based mode, or with MSI and only 1 vector. This
    works around the macOS guest driver quirks.
 5. Trivial code formatting fix for dodgy indentation I stumbled across in
    hci-xhci-pci.
 6. Enables the "conditional-intr-mapping" property in the VMApple machine
    type, which does not support MSI-X or MSI and has previously suffered
    from the macOS guest driver quirk.

Of course, patch 6 can only be applied once the VMApple patch set is also
merged. Patches 2 and 5 are optional for the purposes of fixing the issue I
set out to fix, but seem sensible enough to include in this series.

You can reproduce the problems being fixed as follows:

 * Assertion failure crash fixed in patch 1: Use a x86-64 VM with macOS guest
   and machine type Q35. For USB, use: -device nec-usb-xhci,msix=off
   QEMU will crash with a failed assertion, "vector < nr_vectors" on guest boot.
 * macOS guest not driving the XHCI controller correctly with pin-based
   interrupts: Either as above but with -device nec-usb-xhci,msix=off,msi=off
   or by following the instructions to boot aarch64 macOS 12 on the VMApple
   machine type.

Review notes:

 * I am not 100% sure whether I need to add any special code to support
   backwards compatibility and migration to support the moved and newly added
   device properties.

History:

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.

Phil Dennis-Jordan (6):
  hw/usb/hcd-xhci-pci: Don't trigger MSI on higher vector than allocated
  hw/usb/hcd-xhci-pci: Moves msi/msix properties from NEC to superclass
  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
  hw/usb/hcd-xhci-pci: Indentation fix
  hw/vmapple: XHCI controller's interrupt mapping workaround for macOS

 hw/usb/hcd-xhci-nec.c |  2 --
 hw/usb/hcd-xhci-pci.c | 34 ++++++++++++++++++++++++++++++++--
 hw/usb/hcd-xhci-pci.h |  1 +
 hw/usb/hcd-xhci.c     |  5 +++++
 hw/usb/hcd-xhci.h     |  5 +++++
 hw/vmapple/vmapple.c  |  7 +++++++
 6 files changed, 50 insertions(+), 4 deletions(-)

-- 
2.39.5 (Apple Git-154)

[PATCH 1/6] hw/usb/hcd-xhci-pci: Don't trigger MSI on higher vector than allocated

[Phil Dennis-Jordan]

QEMU would crash with a failed assertion if the XHCI controller
attempted to raise the interrupt on a higher vector than the highest
configured for the device by the guest driver.

This change adds a check so the interrupt is dropped instead of crashing
the VM.

Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
---
 hw/usb/hcd-xhci-pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/usb/hcd-xhci-pci.c b/hw/usb/hcd-xhci-pci.c
index a039f5778a6..376635e889b 100644
--- a/hw/usb/hcd-xhci-pci.c
+++ b/hw/usb/hcd-xhci-pci.c
@@ -73,7 +73,7 @@ static bool xhci_pci_intr_raise(XHCIState *xhci, int n, bool level)
         return true;
     }
 
-    if (msi_enabled(pci_dev) && level) {
+    if (msi_enabled(pci_dev) && level && n < msi_nr_vectors_allocated(pci_dev)) {
         msi_notify(pci_dev, n);
         return true;
     }
-- 
2.39.5 (Apple Git-154)

[PATCH 2/6] hw/usb/hcd-xhci-pci: Moves msi/msix properties from NEC to superclass

[Phil Dennis-Jordan]

The NEC XHCI controller exposes the underlying PCI device's msi and
msix properties, but the superclass and thus the qemu-xhci device do
not. There does not seem to be any obvious reason for this limitation.
This change moves these properties to the superclass so they are
exposed by both PCI XHCI device variants.

Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
---
 hw/usb/hcd-xhci-nec.c | 2 --
 hw/usb/hcd-xhci-pci.c | 7 +++++++
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/hw/usb/hcd-xhci-nec.c b/hw/usb/hcd-xhci-nec.c
index 0c063b3697d..408bf065e5a 100644
--- a/hw/usb/hcd-xhci-nec.c
+++ b/hw/usb/hcd-xhci-nec.c
@@ -39,8 +39,6 @@ struct XHCINecState {
 };
 
 static Property nec_xhci_properties[] = {
-    DEFINE_PROP_ON_OFF_AUTO("msi", XHCIPciState, msi, ON_OFF_AUTO_AUTO),
-    DEFINE_PROP_ON_OFF_AUTO("msix", XHCIPciState, msix, ON_OFF_AUTO_AUTO),
     DEFINE_PROP_UINT32("intrs", XHCINecState, intrs, XHCI_MAXINTRS),
     DEFINE_PROP_UINT32("slots", XHCINecState, slots, XHCI_MAXSLOTS),
     DEFINE_PROP_END_OF_LIST(),
diff --git a/hw/usb/hcd-xhci-pci.c b/hw/usb/hcd-xhci-pci.c
index 376635e889b..0278b0fbce2 100644
--- a/hw/usb/hcd-xhci-pci.c
+++ b/hw/usb/hcd-xhci-pci.c
@@ -197,6 +197,12 @@ static void xhci_instance_init(Object *obj)
     qdev_alias_all_properties(DEVICE(&s->xhci), obj);
 }
 
+static Property xhci_pci_properties[] = {
+    DEFINE_PROP_ON_OFF_AUTO("msi", XHCIPciState, msi, ON_OFF_AUTO_AUTO),
+    DEFINE_PROP_ON_OFF_AUTO("msix", XHCIPciState, msix, ON_OFF_AUTO_AUTO),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static void xhci_class_init(ObjectClass *klass, void *data)
 {
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
@@ -208,6 +214,7 @@ static void xhci_class_init(ObjectClass *klass, void *data)
     k->realize      = usb_xhci_pci_realize;
     k->exit         = usb_xhci_pci_exit;
     k->class_id     = PCI_CLASS_SERIAL_USB;
+    device_class_set_props(dc, xhci_pci_properties);
 }
 
 static const TypeInfo xhci_pci_info = {
-- 
2.39.5 (Apple Git-154)

Re: [PATCH 2/6] hw/usb/hcd-xhci-pci: Moves msi/msix properties from NEC to superclass

[Akihiko Odaki]

Let's drop -s from the "Moves" in the subject:
hw/usb/hcd-xhci-pci: Move msi/msix properties from NEC to superclass

It is a more popular patch style and makes consistent with other patches.

On 2024/12/09 4:16, Phil Dennis-Jordan wrote:
> The NEC XHCI controller exposes the underlying PCI device's msi and
> msix properties, but the superclass and thus the qemu-xhci device do
> not. There does not seem to be any obvious reason for this limitation.
> This change moves these properties to the superclass so they are
> exposed by both PCI XHCI device variants.
> 
> Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
> ---
>   hw/usb/hcd-xhci-nec.c | 2 --
>   hw/usb/hcd-xhci-pci.c | 7 +++++++
>   2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/usb/hcd-xhci-nec.c b/hw/usb/hcd-xhci-nec.c
> index 0c063b3697d..408bf065e5a 100644
> --- a/hw/usb/hcd-xhci-nec.c
> +++ b/hw/usb/hcd-xhci-nec.c
> @@ -39,8 +39,6 @@ struct XHCINecState {
>   };
>   
>   static Property nec_xhci_properties[] = {
> -    DEFINE_PROP_ON_OFF_AUTO("msi", XHCIPciState, msi, ON_OFF_AUTO_AUTO),
> -    DEFINE_PROP_ON_OFF_AUTO("msix", XHCIPciState, msix, ON_OFF_AUTO_AUTO),
>       DEFINE_PROP_UINT32("intrs", XHCINecState, intrs, XHCI_MAXINTRS),
>       DEFINE_PROP_UINT32("slots", XHCINecState, slots, XHCI_MAXSLOTS),
>       DEFINE_PROP_END_OF_LIST(),
> diff --git a/hw/usb/hcd-xhci-pci.c b/hw/usb/hcd-xhci-pci.c
> index 376635e889b..0278b0fbce2 100644
> --- a/hw/usb/hcd-xhci-pci.c
> +++ b/hw/usb/hcd-xhci-pci.c
> @@ -197,6 +197,12 @@ static void xhci_instance_init(Object *obj)
>       qdev_alias_all_properties(DEVICE(&s->xhci), obj);
>   }
>   
> +static Property xhci_pci_properties[] = {
> +    DEFINE_PROP_ON_OFF_AUTO("msi", XHCIPciState, msi, ON_OFF_AUTO_AUTO),
> +    DEFINE_PROP_ON_OFF_AUTO("msix", XHCIPciState, msix, ON_OFF_AUTO_AUTO),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
>   static void xhci_class_init(ObjectClass *klass, void *data)
>   {
>       PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> @@ -208,6 +214,7 @@ static void xhci_class_init(ObjectClass *klass, void *data)
>       k->realize      = usb_xhci_pci_realize;
>       k->exit         = usb_xhci_pci_exit;
>       k->class_id     = PCI_CLASS_SERIAL_USB;
> +    device_class_set_props(dc, xhci_pci_properties);
>   }
>   
>   static const TypeInfo xhci_pci_info = {

[PATCH 3/6] hw/usb/hcd-xhci-pci: Use event ring 0 if mapping unsupported.

[Phil Dennis-Jordan]

The XHCI specification, section 4.17.1 specifies that "If Interrupter
Mapping is not supported, the Interrupter Target field shall be
ignored by the xHC and all Events targeted at Interrupter 0."

QEMU's XHCI device has so far not specially addressed this case,
so we add a check to xhci_event() to redirect to event ring and
interrupt 0 if mapping is disabled.

Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
---
 hw/usb/hcd-xhci.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index d85adaca0dc..5fb140c2382 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -644,6 +644,10 @@ static void xhci_event(XHCIState *xhci, XHCIEvent *event, int v)
     dma_addr_t erdp;
     unsigned int dp_idx;
 
+    if (xhci->numintrs == 1) {
+        v = 0;
+    }
+
     if (v >= xhci->numintrs) {
         DPRINTF("intr nr out of range (%d >= %d)\n", v, xhci->numintrs);
         return;
-- 
2.39.5 (Apple Git-154)

Re: [PATCH 3/6] hw/usb/hcd-xhci-pci: Use event ring 0 if mapping unsupported.

[Akihiko Odaki]

Remove the period from the subject.

On 2024/12/09 4:16, Phil Dennis-Jordan wrote:
> The XHCI specification, section 4.17.1 specifies that "If Interrupter
> Mapping is not supported, the Interrupter Target field shall be
> ignored by the xHC and all Events targeted at Interrupter 0."
> 
> QEMU's XHCI device has so far not specially addressed this case,
> so we add a check to xhci_event() to redirect to event ring and
> interrupt 0 if mapping is disabled.
> 
> Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
> ---
>   hw/usb/hcd-xhci.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
> index d85adaca0dc..5fb140c2382 100644
> --- a/hw/usb/hcd-xhci.c
> +++ b/hw/usb/hcd-xhci.c
> @@ -644,6 +644,10 @@ static void xhci_event(XHCIState *xhci, XHCIEvent *event, int v)
>       dma_addr_t erdp;
>       unsigned int dp_idx;
>   
> +    if (xhci->numintrs == 1) {
> +        v = 0;
> +    }
> +
>       if (v >= xhci->numintrs) {
>           DPRINTF("intr nr out of range (%d >= %d)\n", v, xhci->numintrs);
>           return;

[PATCH 4/6] hw/usb/hcd-xhci-pci: Adds property for disabling mapping in IRQ mode

[Phil Dennis-Jordan]

This change addresses an edge case that trips up macOS guest drivers
for PCI based XHCI controllers. The guest driver would attempt to
schedule events to XHCI event rings 1 and 2 even when only one interrupt
line is available; interrupts would therefore be dropped, and events
only handled on timeout when using pin-based interrupts. Moreover,
when MSI is available, the macOS guest drivers would only configure 1
vector and leading to the same problem.

So, in addition to disabling interrupter mapping if numintrs is 1, a
callback is added to xhci to check whether interrupter mapping should be
enabled. The PCI XHCI device type now provides an implementation of
this callback if the new "conditional-intr-mapping" property is enabled.
(default: disabled) When enabled, interrupter mapping is only enabled
when MSI-X is active, or when MSI is active with more than 1 vector.

This means that when using pin-based interrupts, or only 1 MSI vector,
events are only submitted to interrupter 0 regardless of selected
target. This allows the macOS guest drivers to work with the device in
those configurations.

Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2705
---
 hw/usb/hcd-xhci-pci.c | 23 +++++++++++++++++++++++
 hw/usb/hcd-xhci-pci.h |  1 +
 hw/usb/hcd-xhci.c     |  3 ++-
 hw/usb/hcd-xhci.h     |  5 +++++
 4 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/hw/usb/hcd-xhci-pci.c b/hw/usb/hcd-xhci-pci.c
index 0278b0fbce2..8e293cd5951 100644
--- a/hw/usb/hcd-xhci-pci.c
+++ b/hw/usb/hcd-xhci-pci.c
@@ -81,6 +81,23 @@ static bool xhci_pci_intr_raise(XHCIState *xhci, int n, bool level)
     return false;
 }
 
+static bool xhci_pci_intr_mapping_conditional(XHCIState *xhci)
+{
+    XHCIPciState *s = container_of(xhci, XHCIPciState, xhci);
+    PCIDevice *pci_dev = PCI_DEVICE(s);
+
+    /*
+     * Implementation of the "conditional-intr-mapping" property, which only
+     * enables interrupter mapping if there are actually multiple interrupt
+     * vectors available. Forces all events onto interrupter/event ring 0
+     * in pin-based IRQ mode or when only 1 MSI vector is allocated.
+     * Provides compatibility with macOS guests on machine types where MSI-X is
+     * not available.
+     */
+    return msix_enabled(pci_dev) ||
+        (msi_enabled(pci_dev) && msi_nr_vectors_allocated(pci_dev) > 1);
+}
+
 static void xhci_pci_reset(DeviceState *dev)
 {
     XHCIPciState *s = XHCI_PCI(dev);
@@ -118,6 +135,9 @@ static void usb_xhci_pci_realize(struct PCIDevice *dev, Error **errp)
     object_property_set_link(OBJECT(&s->xhci), "host", OBJECT(s), NULL);
     s->xhci.intr_update = xhci_pci_intr_update;
     s->xhci.intr_raise = xhci_pci_intr_raise;
+    if (s->conditional_intr_mapping) {
+        s->xhci.intr_mapping_supported = xhci_pci_intr_mapping_conditional;
+    }
     if (!qdev_realize(DEVICE(&s->xhci), NULL, errp)) {
         return;
     }
@@ -200,6 +220,9 @@ static void xhci_instance_init(Object *obj)
 static Property xhci_pci_properties[] = {
     DEFINE_PROP_ON_OFF_AUTO("msi", XHCIPciState, msi, ON_OFF_AUTO_AUTO),
     DEFINE_PROP_ON_OFF_AUTO("msix", XHCIPciState, msix, ON_OFF_AUTO_AUTO),
+    /* When true, disable interrupter mapping for IRQ mode or only 1 vector */
+    DEFINE_PROP_BOOL("conditional-intr-mapping", XHCIPciState,
+                     conditional_intr_mapping, false),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/usb/hcd-xhci-pci.h b/hw/usb/hcd-xhci-pci.h
index 08f70ce97cc..5b61ae84555 100644
--- a/hw/usb/hcd-xhci-pci.h
+++ b/hw/usb/hcd-xhci-pci.h
@@ -40,6 +40,7 @@ typedef struct XHCIPciState {
     XHCIState xhci;
     OnOffAuto msi;
     OnOffAuto msix;
+    bool conditional_intr_mapping;
 } XHCIPciState;
 
 #endif
diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index 5fb140c2382..b607ddd1a93 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -644,7 +644,8 @@ static void xhci_event(XHCIState *xhci, XHCIEvent *event, int v)
     dma_addr_t erdp;
     unsigned int dp_idx;
 
-    if (xhci->numintrs == 1) {
+    if (xhci->numintrs == 1 ||
+        (xhci->intr_mapping_supported && !xhci->intr_mapping_supported(xhci))) {
         v = 0;
     }
 
diff --git a/hw/usb/hcd-xhci.h b/hw/usb/hcd-xhci.h
index fe16d7ad055..fdaa21ba7f6 100644
--- a/hw/usb/hcd-xhci.h
+++ b/hw/usb/hcd-xhci.h
@@ -193,6 +193,11 @@ typedef struct XHCIState {
     uint32_t max_pstreams_mask;
     void (*intr_update)(XHCIState *s, int n, bool enable);
     bool (*intr_raise)(XHCIState *s, int n, bool level);
+    /*
+     * Callback for special-casing interrupter mapping support. NULL for most
+     * implementations, for defaulting to enabled mapping unless numintrs == 1.
+     */
+    bool (*intr_mapping_supported)(XHCIState *s);
     DeviceState *hostOpaque;
 
     /* Operational Registers */
-- 
2.39.5 (Apple Git-154)

Re: [PATCH 4/6] hw/usb/hcd-xhci-pci: Adds property for disabling mapping in IRQ mode

[Akihiko Odaki]

On 2024/12/09 4:16, Phil Dennis-Jordan wrote:
> This change addresses an edge case that trips up macOS guest drivers
> for PCI based XHCI controllers. The guest driver would attempt to
> schedule events to XHCI event rings 1 and 2 even when only one interrupt
> line is available; interrupts would therefore be dropped, and events
> only handled on timeout when using pin-based interrupts. Moreover,
> when MSI is available, the macOS guest drivers would only configure 1
> vector and leading to the same problem.
> 
> So, in addition to disabling interrupter mapping if numintrs is 1, a
> callback is added to xhci to check whether interrupter mapping should be
> enabled. The PCI XHCI device type now provides an implementation of
> this callback if the new "conditional-intr-mapping" property is enabled.
> (default: disabled) When enabled, interrupter mapping is only enabled
> when MSI-X is active, or when MSI is active with more than 1 vector.
> 
> This means that when using pin-based interrupts, or only 1 MSI vector,
> events are only submitted to interrupter 0 regardless of selected
> target. This allows the macOS guest drivers to work with the device in
> those configurations.
> 
> Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2705
> ---
>   hw/usb/hcd-xhci-pci.c | 23 +++++++++++++++++++++++
>   hw/usb/hcd-xhci-pci.h |  1 +
>   hw/usb/hcd-xhci.c     |  3 ++-
>   hw/usb/hcd-xhci.h     |  5 +++++
>   4 files changed, 31 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/usb/hcd-xhci-pci.c b/hw/usb/hcd-xhci-pci.c
> index 0278b0fbce2..8e293cd5951 100644
> --- a/hw/usb/hcd-xhci-pci.c
> +++ b/hw/usb/hcd-xhci-pci.c
> @@ -81,6 +81,23 @@ static bool xhci_pci_intr_raise(XHCIState *xhci, int n, bool level)
>       return false;
>   }
>   
> +static bool xhci_pci_intr_mapping_conditional(XHCIState *xhci)
> +{
> +    XHCIPciState *s = container_of(xhci, XHCIPciState, xhci);
> +    PCIDevice *pci_dev = PCI_DEVICE(s);
> +
> +    /*
> +     * Implementation of the "conditional-intr-mapping" property, which only
> +     * enables interrupter mapping if there are actually multiple interrupt
> +     * vectors available. Forces all events onto interrupter/event ring 0
> +     * in pin-based IRQ mode or when only 1 MSI vector is allocated.
> +     * Provides compatibility with macOS guests on machine types where MSI-X is
> +     * not available.
> +     */
> +    return msix_enabled(pci_dev) ||
> +        (msi_enabled(pci_dev) && msi_nr_vectors_allocated(pci_dev) > 1);

This will make it behave incosistently when 
msi_nr_vectors_allocated(pci_dev) is not sufficient to accomodate all 
Interrupters;  If > 1, overflowed Interrupters will be ignored, but if 
<= 1, overflowed Interrupters will be redirected to Interrupter 0. 
Remove the condition unless it is truly unnecessary.

> +}
> +
>   static void xhci_pci_reset(DeviceState *dev)
>   {
>       XHCIPciState *s = XHCI_PCI(dev);
> @@ -118,6 +135,9 @@ static void usb_xhci_pci_realize(struct PCIDevice *dev, Error **errp)
>       object_property_set_link(OBJECT(&s->xhci), "host", OBJECT(s), NULL);
>       s->xhci.intr_update = xhci_pci_intr_update;
>       s->xhci.intr_raise = xhci_pci_intr_raise;
> +    if (s->conditional_intr_mapping) {
> +        s->xhci.intr_mapping_supported = xhci_pci_intr_mapping_conditional;
> +    }
>       if (!qdev_realize(DEVICE(&s->xhci), NULL, errp)) {
>           return;
>       }
> @@ -200,6 +220,9 @@ static void xhci_instance_init(Object *obj)
>   static Property xhci_pci_properties[] = {
>       DEFINE_PROP_ON_OFF_AUTO("msi", XHCIPciState, msi, ON_OFF_AUTO_AUTO),
>       DEFINE_PROP_ON_OFF_AUTO("msix", XHCIPciState, msix, ON_OFF_AUTO_AUTO),
> +    /* When true, disable interrupter mapping for IRQ mode or only 1 vector */
> +    DEFINE_PROP_BOOL("conditional-intr-mapping", XHCIPciState,
> +                     conditional_intr_mapping, false),
>       DEFINE_PROP_END_OF_LIST(),
>   };
>   
> diff --git a/hw/usb/hcd-xhci-pci.h b/hw/usb/hcd-xhci-pci.h
> index 08f70ce97cc..5b61ae84555 100644
> --- a/hw/usb/hcd-xhci-pci.h
> +++ b/hw/usb/hcd-xhci-pci.h
> @@ -40,6 +40,7 @@ typedef struct XHCIPciState {
>       XHCIState xhci;
>       OnOffAuto msi;
>       OnOffAuto msix;
> +    bool conditional_intr_mapping;
>   } XHCIPciState;
>   
>   #endif
> diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
> index 5fb140c2382..b607ddd1a93 100644
> --- a/hw/usb/hcd-xhci.c
> +++ b/hw/usb/hcd-xhci.c
> @@ -644,7 +644,8 @@ static void xhci_event(XHCIState *xhci, XHCIEvent *event, int v)
>       dma_addr_t erdp;
>       unsigned int dp_idx;
>   
> -    if (xhci->numintrs == 1) {
> +    if (xhci->numintrs == 1 ||
> +        (xhci->intr_mapping_supported && !xhci->intr_mapping_supported(xhci))) {
>           v = 0;
>       }
>   
> diff --git a/hw/usb/hcd-xhci.h b/hw/usb/hcd-xhci.h
> index fe16d7ad055..fdaa21ba7f6 100644
> --- a/hw/usb/hcd-xhci.h
> +++ b/hw/usb/hcd-xhci.h
> @@ -193,6 +193,11 @@ typedef struct XHCIState {
>       uint32_t max_pstreams_mask;
>       void (*intr_update)(XHCIState *s, int n, bool enable);
>       bool (*intr_raise)(XHCIState *s, int n, bool level);
> +    /*
> +     * Callback for special-casing interrupter mapping support. NULL for most
> +     * implementations, for defaulting to enabled mapping unless numintrs == 1.
> +     */
> +    bool (*intr_mapping_supported)(XHCIState *s);
>       DeviceState *hostOpaque;
>   
>       /* Operational Registers */

Re: [PATCH 4/6] hw/usb/hcd-xhci-pci: Adds property for disabling mapping in IRQ mode

[Phil Dennis-Jordan]

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

On Mon, 9 Dec 2024 at 07:19, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:

> On 2024/12/09 4:16, Phil Dennis-Jordan wrote:
> > This change addresses an edge case that trips up macOS guest drivers
> > for PCI based XHCI controllers. The guest driver would attempt to
> > schedule events to XHCI event rings 1 and 2 even when only one interrupt
> > line is available; interrupts would therefore be dropped, and events
> > only handled on timeout when using pin-based interrupts. Moreover,
> > when MSI is available, the macOS guest drivers would only configure 1
> > vector and leading to the same problem.
> >
> > So, in addition to disabling interrupter mapping if numintrs is 1, a
> > callback is added to xhci to check whether interrupter mapping should be
> > enabled. The PCI XHCI device type now provides an implementation of
> > this callback if the new "conditional-intr-mapping" property is enabled.
> > (default: disabled) When enabled, interrupter mapping is only enabled
> > when MSI-X is active, or when MSI is active with more than 1 vector.
> >
> > This means that when using pin-based interrupts, or only 1 MSI vector,
> > events are only submitted to interrupter 0 regardless of selected
> > target. This allows the macOS guest drivers to work with the device in
> > those configurations.
> >
> > Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2705
> > ---
> >   hw/usb/hcd-xhci-pci.c | 23 +++++++++++++++++++++++
> >   hw/usb/hcd-xhci-pci.h |  1 +
> >   hw/usb/hcd-xhci.c     |  3 ++-
> >   hw/usb/hcd-xhci.h     |  5 +++++
> >   4 files changed, 31 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/usb/hcd-xhci-pci.c b/hw/usb/hcd-xhci-pci.c
> > index 0278b0fbce2..8e293cd5951 100644
> > --- a/hw/usb/hcd-xhci-pci.c
> > +++ b/hw/usb/hcd-xhci-pci.c
> > @@ -81,6 +81,23 @@ static bool xhci_pci_intr_raise(XHCIState *xhci, int
> n, bool level)
> >       return false;
> >   }
> >
> > +static bool xhci_pci_intr_mapping_conditional(XHCIState *xhci)
> > +{
> > +    XHCIPciState *s = container_of(xhci, XHCIPciState, xhci);
> > +    PCIDevice *pci_dev = PCI_DEVICE(s);
> > +
> > +    /*
> > +     * Implementation of the "conditional-intr-mapping" property, which
> only
> > +     * enables interrupter mapping if there are actually multiple
> interrupt
> > +     * vectors available. Forces all events onto interrupter/event ring
> 0
> > +     * in pin-based IRQ mode or when only 1 MSI vector is allocated.
> > +     * Provides compatibility with macOS guests on machine types where
> MSI-X is
> > +     * not available.
> > +     */
> > +    return msix_enabled(pci_dev) ||
> > +        (msi_enabled(pci_dev) && msi_nr_vectors_allocated(pci_dev) > 1);
>
> This will make it behave incosistently when
> msi_nr_vectors_allocated(pci_dev) is not sufficient to accomodate all
> Interrupters;  If > 1, overflowed Interrupters will be ignored, but if
> <= 1, overflowed Interrupters will be redirected to Interrupter 0.
> Remove the condition unless it is truly unnecessary.
>

After applying the existing patch 1/6 to fix the failed assertion, if you
run a VM with a macOS guest, and configure the XHCI controller so that MSI
is on and MSI-X is off:
-device nec-usb-xhci,msix=off
You'll find that it exhibits the same apparent problem as when using
pin-based interrupts: the macOS driver configures only one MSI vector, and
then schedules events to event rings 1 and 2.

You have however just prompted me to re-check the specification on the
details of MSI, and it looks like I missed something in the "Implementation
note" in section 4.17 (Interrupters):

When MSI is activated:
>
[…]
>
The Interrupt Vector associated with an Interrupter shall be defined as
> function of the value of the MSI Message Control register Multiple Message
> Enable field using the following algorithm.
>
>      Interrupt Vector = (Index of Interrupter) MODULUS (MSI Message
> Control:Multiple Message Enable)


So it seems that patch 1/6 should actually be changed to

    if (msi_enabled(pci_dev) && level) {
        n %= msi_nr_vectors_allocated(pci_dev);
        msi_notify(pci_dev, n);
        return true;
    }

To implement this modulus algorithm. (msi_nr_vectors_allocated() reads the
"Multiple Message Enable" field.) Then we can drop the vector count
condition in this patch. I have verified that the macOS guest's XHCI driver
handles this arrangement correctly.

> +}
> > +
> >   static void xhci_pci_reset(DeviceState *dev)
> >   {
> >       XHCIPciState *s = XHCI_PCI(dev);
> > @@ -118,6 +135,9 @@ static void usb_xhci_pci_realize(struct PCIDevice
> *dev, Error **errp)
> >       object_property_set_link(OBJECT(&s->xhci), "host", OBJECT(s),
> NULL);
> >       s->xhci.intr_update = xhci_pci_intr_update;
> >       s->xhci.intr_raise = xhci_pci_intr_raise;
> > +    if (s->conditional_intr_mapping) {
> > +        s->xhci.intr_mapping_supported =
> xhci_pci_intr_mapping_conditional;
> > +    }
> >       if (!qdev_realize(DEVICE(&s->xhci), NULL, errp)) {
> >           return;
> >       }
> > @@ -200,6 +220,9 @@ static void xhci_instance_init(Object *obj)
> >   static Property xhci_pci_properties[] = {
> >       DEFINE_PROP_ON_OFF_AUTO("msi", XHCIPciState, msi,
> ON_OFF_AUTO_AUTO),
> >       DEFINE_PROP_ON_OFF_AUTO("msix", XHCIPciState, msix,
> ON_OFF_AUTO_AUTO),
> > +    /* When true, disable interrupter mapping for IRQ mode or only 1
> vector */
> > +    DEFINE_PROP_BOOL("conditional-intr-mapping", XHCIPciState,
> > +                     conditional_intr_mapping, false),
> >       DEFINE_PROP_END_OF_LIST(),
> >   };
> >
> > diff --git a/hw/usb/hcd-xhci-pci.h b/hw/usb/hcd-xhci-pci.h
> > index 08f70ce97cc..5b61ae84555 100644
> > --- a/hw/usb/hcd-xhci-pci.h
> > +++ b/hw/usb/hcd-xhci-pci.h
> > @@ -40,6 +40,7 @@ typedef struct XHCIPciState {
> >       XHCIState xhci;
> >       OnOffAuto msi;
> >       OnOffAuto msix;
> > +    bool conditional_intr_mapping;
> >   } XHCIPciState;
> >
> >   #endif
> > diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
> > index 5fb140c2382..b607ddd1a93 100644
> > --- a/hw/usb/hcd-xhci.c
> > +++ b/hw/usb/hcd-xhci.c
> > @@ -644,7 +644,8 @@ static void xhci_event(XHCIState *xhci, XHCIEvent
> *event, int v)
> >       dma_addr_t erdp;
> >       unsigned int dp_idx;
> >
> > -    if (xhci->numintrs == 1) {
> > +    if (xhci->numintrs == 1 ||
> > +        (xhci->intr_mapping_supported &&
> !xhci->intr_mapping_supported(xhci))) {
> >           v = 0;
> >       }
> >
> > diff --git a/hw/usb/hcd-xhci.h b/hw/usb/hcd-xhci.h
> > index fe16d7ad055..fdaa21ba7f6 100644
> > --- a/hw/usb/hcd-xhci.h
> > +++ b/hw/usb/hcd-xhci.h
> > @@ -193,6 +193,11 @@ typedef struct XHCIState {
> >       uint32_t max_pstreams_mask;
> >       void (*intr_update)(XHCIState *s, int n, bool enable);
> >       bool (*intr_raise)(XHCIState *s, int n, bool level);
> > +    /*
> > +     * Callback for special-casing interrupter mapping support. NULL
> for most
> > +     * implementations, for defaulting to enabled mapping unless
> numintrs == 1.
> > +     */
> > +    bool (*intr_mapping_supported)(XHCIState *s);
> >       DeviceState *hostOpaque;
> >
> >       /* Operational Registers */
>
>

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

[PATCH 5/6] hw/usb/hcd-xhci-pci: Indentation fix

[Phil Dennis-Jordan]

Fixes number of spaces used for indentation on one line.

Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
---
 hw/usb/hcd-xhci-pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/usb/hcd-xhci-pci.c b/hw/usb/hcd-xhci-pci.c
index 8e293cd5951..6b6f0f91a18 100644
--- a/hw/usb/hcd-xhci-pci.c
+++ b/hw/usb/hcd-xhci-pci.c
@@ -111,7 +111,7 @@ static int xhci_pci_vmstate_post_load(void *opaque, int version_id)
     PCIDevice *pci_dev = PCI_DEVICE(s);
     int intr;
 
-   for (intr = 0; intr < s->xhci.numintrs; intr++) {
+    for (intr = 0; intr < s->xhci.numintrs; intr++) {
         if (s->xhci.intr[intr].msix_used) {
             msix_vector_use(pci_dev, intr);
         } else {
-- 
2.39.5 (Apple Git-154)

[PATCH 6/6] hw/vmapple: XHCI controller's interrupt mapping workaround for macOS

[Phil Dennis-Jordan]

This change enables the new conditional interrupt mapping support
property on the vmapple machine type's integrated XHCI controller.
The macOS guest driver attempts to use event rings 1 and 2 on the XHCI
controller, despite there being only one (PCI pin) interrupt channel
available. With conditional interrupt mapping enabled, the XHCI
controller will only schedule events on interrupter 0 in PCI pin mode
or when only a single MSI vector is active.

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

diff --git a/hw/vmapple/vmapple.c b/hw/vmapple/vmapple.c
index f607981bc40..156ea33ae79 100644
--- a/hw/vmapple/vmapple.c
+++ b/hw/vmapple/vmapple.c
@@ -453,6 +453,13 @@ static void create_pcie(VMAppleMachineState *vms)
     }
 
     usb_controller = qdev_new(TYPE_QEMU_XHCI);
+    /*
+     * macOS XHCI driver attempts to schedule events onto even rings 1 & 2
+     * even when (as here) there is no MSI-X support on this PCIe bus. Disabling
+     * interrupter mapping in the XHCI controller works around the problem.
+     */
+    object_property_set_bool(OBJECT(usb_controller),
+                             "conditional-intr-mapping", true, &error_fatal);
     qdev_realize_and_unref(usb_controller, BUS(pci->bus), &error_fatal);
 
     usb_bus = USB_BUS(object_resolve_type_unambiguous(TYPE_USB_BUS,
-- 
2.39.5 (Apple Git-154)

Re: [PATCH 6/6] hw/vmapple: XHCI controller's interrupt mapping workaround for macOS

[Akihiko Odaki]

On 2024/12/09 4:16, Phil Dennis-Jordan wrote:
> This change enables the new conditional interrupt mapping support
> property on the vmapple machine type's integrated XHCI controller.
> The macOS guest driver attempts to use event rings 1 and 2 on the XHCI
> controller, despite there being only one (PCI pin) interrupt channel
> available. With conditional interrupt mapping enabled, the XHCI
> controller will only schedule events on interrupter 0 in PCI pin mode
> or when only a single MSI vector is active.

I think docs/system/arm/vmapple.rst also needs to be updated.

> 
> Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
> ---
>   hw/vmapple/vmapple.c | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
> diff --git a/hw/vmapple/vmapple.c b/hw/vmapple/vmapple.c
> index f607981bc40..156ea33ae79 100644
> --- a/hw/vmapple/vmapple.c
> +++ b/hw/vmapple/vmapple.c
> @@ -453,6 +453,13 @@ static void create_pcie(VMAppleMachineState *vms)
>       }
>   
>       usb_controller = qdev_new(TYPE_QEMU_XHCI);
> +    /*
> +     * macOS XHCI driver attempts to schedule events onto even rings 1 & 2
> +     * even when (as here) there is no MSI-X support on this PCIe bus. Disabling
> +     * interrupter mapping in the XHCI controller works around the problem.
> +     */
> +    object_property_set_bool(OBJECT(usb_controller),
> +                             "conditional-intr-mapping", true, &error_fatal);

Use compat_props to change the global default for this machine.

By the way, this unconditionally adds xHCI and USB devices, but that 
should be avoided so that users can customize the configuration. Use 
defaults_enabled() as a condition.

Regards,
Akihiko Odaki

>       qdev_realize_and_unref(usb_controller, BUS(pci->bus), &error_fatal);
>   
>       usb_bus = USB_BUS(object_resolve_type_unambiguous(TYPE_USB_BUS,

Re: [PATCH 6/6] hw/vmapple: XHCI controller's interrupt mapping workaround for macOS

[Phil Dennis-Jordan]

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

On Mon, 9 Dec 2024 at 07:26, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:

> On 2024/12/09 4:16, Phil Dennis-Jordan wrote:
> > This change enables the new conditional interrupt mapping support
> > property on the vmapple machine type's integrated XHCI controller.
> > The macOS guest driver attempts to use event rings 1 and 2 on the XHCI
> > controller, despite there being only one (PCI pin) interrupt channel
> > available. With conditional interrupt mapping enabled, the XHCI
> > controller will only schedule events on interrupter 0 in PCI pin mode
> > or when only a single MSI vector is active.
>
> I think docs/system/arm/vmapple.rst also needs to be updated.
>

Can you be more specific about what you think I should include? That file
currently does not mention USB in any way, and if we set the new property
in the machine type, there shouldn't be any need for manual configuration
on the command line, should there?


> >
> > Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
> > ---
> >   hw/vmapple/vmapple.c | 7 +++++++
> >   1 file changed, 7 insertions(+)
> >
> > diff --git a/hw/vmapple/vmapple.c b/hw/vmapple/vmapple.c
> > index f607981bc40..156ea33ae79 100644
> > --- a/hw/vmapple/vmapple.c
> > +++ b/hw/vmapple/vmapple.c
> > @@ -453,6 +453,13 @@ static void create_pcie(VMAppleMachineState *vms)
> >       }
> >
> >       usb_controller = qdev_new(TYPE_QEMU_XHCI);
> > +    /*
> > +     * macOS XHCI driver attempts to schedule events onto even rings 1
> & 2
> > +     * even when (as here) there is no MSI-X support on this PCIe bus.
> Disabling
> > +     * interrupter mapping in the XHCI controller works around the
> problem.
> > +     */
> > +    object_property_set_bool(OBJECT(usb_controller),
> > +                             "conditional-intr-mapping", true,
> &error_fatal);
>
> Use compat_props to change the global default for this machine.
>

Thanks, that works.



> By the way, this unconditionally adds xHCI and USB devices, but that
> should be avoided so that users can customize the configuration. Use
> defaults_enabled() as a condition.
>

Makes sense, I guess I'd better add that to the VMApple patch set though.



> Regards,
> Akihiko Odaki
>
> >       qdev_realize_and_unref(usb_controller, BUS(pci->bus),
> &error_fatal);
> >
> >       usb_bus = USB_BUS(object_resolve_type_unambiguous(TYPE_USB_BUS,
>
>

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

Re: [PATCH 6/6] hw/vmapple: XHCI controller's interrupt mapping workaround for macOS

[Akihiko Odaki]

On 2024/12/09 20:14, Phil Dennis-Jordan wrote:
> 
> 
> On Mon, 9 Dec 2024 at 07:26, Akihiko Odaki <akihiko.odaki@daynix.com 
> <mailto:akihiko.odaki@daynix.com>> wrote:
> 
>     On 2024/12/09 4:16, Phil Dennis-Jordan wrote:
>      > This change enables the new conditional interrupt mapping support
>      > property on the vmapple machine type's integrated XHCI controller.
>      > The macOS guest driver attempts to use event rings 1 and 2 on the
>     XHCI
>      > controller, despite there being only one (PCI pin) interrupt channel
>      > available. With conditional interrupt mapping enabled, the XHCI
>      > controller will only schedule events on interrupter 0 in PCI pin mode
>      > or when only a single MSI vector is active.
> 
>     I think docs/system/arm/vmapple.rst also needs to be updated.
> 
> 
> Can you be more specific about what you think I should include? That 
> file currently does not mention USB in any way, and if we set the new 
> property in the machine type, there shouldn't be any need for manual 
> configuration on the command line, should there?

I mistook the patch message for the documentation. The documentation 
file is fine so there is no need for change here.