[PATCH RFC-for-10.0] hw/usb/hcd-xhci-pci: Use event ring 0 if interrupter mapping unsupported

[Phil Dennis-Jordan]

This change addresses an edge case that trips up macOS guest drivers
for PCI based XHCI controllers.

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."

The PCI XHCI controller supports MSI(-X) and maxintrs is therefore
reasonably set to 16. The specification does not address whether
interrupter mapping should be considered "supported" if the guest
driver does not enable MSI(-X) via the PCI configuration area, possibly
because the PCI host bus does not support it.

QEMU's XHCI device has so far not specially addressed this case, and
no interrupt is generated for events delivered to event rings 1 and
above. The macOS guest driver does not get along with this
interpretation, so many events are not delivered to the guest OS when
MSI(-X) is off.

This patch changes the xhci_event() function such that event ring 0
is always used when numintrs is 1 (as per spec section 4.17.1) or
if neither MSI nor MSI-X are enabled. The latter is checked by adding
a new, optional intr_mapping_supported function pointer field supplied
by the concrete XHCI controller implementation. The PCI implementation's
supplied function calls msix_enabled and msi_enabled accordingly.

Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2705
---
I've been chasing this problem for a while, and 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

The fix provided is I think the simplest one in terms of lines of code,
but it's also a little ugly, as we're constantly checking msix_enabled
and msi_enabled via a callback function. Granted, we're already doing
that in xhci_pci_intr_raise and xhci_pci_intr_update, but this adds
a bunch more calls.

The main alternative I can see is to "push" the state of whether
interrupter mapping is supported: provide a custom config_write
implementation for the PCI device, and every time the configuration
area is updated, evaluate whether or not this means that MSI or MSI-X
are enabled and update a corresponding flag inside XHCIState. We could
even use this opportunity to switch out different .intr_raise and
.intr_update functions depending on which interrupt delivery mechanism
is active in the PCI device.


 hw/usb/hcd-xhci-pci.c | 9 +++++++++
 hw/usb/hcd-xhci.c     | 5 +++++
 hw/usb/hcd-xhci.h     | 5 +++++
 3 files changed, 19 insertions(+)

diff --git a/hw/usb/hcd-xhci-pci.c b/hw/usb/hcd-xhci-pci.c
index a039f5778a6..21802211cf7 100644
--- a/hw/usb/hcd-xhci-pci.c
+++ b/hw/usb/hcd-xhci-pci.c
@@ -81,6 +81,14 @@ static bool xhci_pci_intr_raise(XHCIState *xhci, int n, bool 
level)
     return false;
 }
 
+static bool xhci_pci_intr_mapping_supported(XHCIState *xhci)
+{
+    XHCIPciState *s = container_of(xhci, XHCIPciState, xhci);
+    PCIDevice *pci_dev = PCI_DEVICE(s);
+
+    return msix_enabled(pci_dev) || msi_enabled(pci_dev);
+}
+
 static void xhci_pci_reset(DeviceState *dev)
 {
     XHCIPciState *s = XHCI_PCI(dev);
@@ -118,6 +126,7 @@ 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;
+    s->xhci.intr_mapping_supported = xhci_pci_intr_mapping_supported;
     if (!qdev_realize(DEVICE(&s->xhci), NULL, errp)) {
         return;
     }
diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index d85adaca0dc..a2a878e4594 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -644,6 +644,11 @@ static void xhci_event(XHCIState *xhci, XHCIEvent *event, 
int v)
     dma_addr_t erdp;
     unsigned int dp_idx;
 
+    if (xhci->numintrs == 1 ||
+        (xhci->intr_mapping_supported && !xhci->intr_mapping_supported(xhci))) 
{
+        v = 0; /* Per section 4.17.1 */
+    }
+
     if (v >= xhci->numintrs) {
         DPRINTF("intr nr out of range (%d >= %d)\n", v, xhci->numintrs);
         return;
diff --git a/hw/usb/hcd-xhci.h b/hw/usb/hcd-xhci.h
index fe16d7ad055..6e901de6e6b 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);
+    /*
+     * Device supports Interrupter Mapping per section 4.17.1 of XHCI spec.
+     * If NULL, assume true if numintrs > 1.
+     */
+    bool (*intr_mapping_supported)(XHCIState *s);
     DeviceState *hostOpaque;
 
     /* Operational Registers */
-- 
2.39.5 (Apple Git-154)