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

[Akihiko Odaki <akihiko.odaki@daynix.com>]

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