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