| Message ID | 20241208191646.64857-5-phil@philjordan.eu (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | hw/usb/hcd-xhci: Fixes, improvements, and macOS workaround | expand |
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 */ > >
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 */
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(-)