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)

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

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