qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Akihiko Odaki
Subject: Re: [PATCH 4/6] hw/usb/hcd-xhci-pci: Adds property for disabling mapping in IRQ mode
Date: Mon, 9 Dec 2024 15:19:26 +0900
User-agent: Mozilla Thunderbird

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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]