On 2024/12/02 1:03, Phil Dennis-Jordan wrote:
> 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.
This section says "an xHC implementation may support Interrupter
Mapping," and section 3 says "Host Controller (xHC). The host controller
is the specific hardware implementation of the host controller
architecture." So the entity that "supports" interrupt mapping is the
hardware and does not include the guest driver.
I wasn't talking about the driver in this context - I'm questioning whether there is any reasonable interpretation where hardware should be considered to support interrupter mapping when it is configured to use pin-based interrupts.
>
> 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.
I think the current behavior to drop interrupts is correct and targeting
Interrupter 0 is a violation of the specification.
As noted earlier, this xHC implementation supports Interrupter Mapping,
so it must target Interrupters 1 to MaxInstrs-1 instead of always
targeting Interrupter 0.
At the beginning of section 4.7, the specification also says "an
Interrupter shall assert an interrupt if it is enabled and its
associated Event Ring contains Event TRBs that require an interrupt".
Unfortunately, this is an ambiguous wording - "it" could refer to either the interrupt or the interrupter, and I suspect it is talking about the IE flag. The IE flag is called "Interrupt Enable," not "InterruptER Enable." (If it's off, you can still use the Event Ring in polling mode - the interruptER includes the event ring, the flag only applies to interrupt delivery.) On the other hand, there is an "InterruptER Enable" (INTE) flag in the USBCMD register - but there's only one, which covers all the interrupters available.
On the other hand, in 4.2 it says "Enable the Interrupter by writing a '1' to the Interrupt Enable (IE) field…".
So the terminology is not consistent.
It
also says "interrupters 1 to MaxIntrs-1 shall be disabled" "when the PCI
Pin Interrupt is activated". So disabled Interrupters must drop interrupts.
What does disabling the Interrupter (again, not the interrupt) mean? The Event Ring is part of the Interrupter. So if the interrupter is disabled, is the ring disabled? If the ring is disabled, should the hardware really be dispatching events to it?
While the proposed behavior violates the specification, it is still
crucial to run macOS guests. A possible solution is to add a property to
enable this behavior not conforming to the specification, and requires
users to opt-in it when running macOS.
It's certainly a possibility - but it adds user-facing complexity for no discernible gain. Users of non-macOS guests won't care which way the flag is set because it doesn't affect them, and the default setting won't work for macOS guest users, meaning they need to research what's going on if they're hit by the problem. OK, we can enable it by default for the vmapple machine type, and most users of x86-64 macOS guests will be using the Q35 machine type, where MSI-X is functional. I'm just not convinced the higher complexity(*) is worth a user setting that no user will ever change.
(* And it WILL be higher complexity, because you definitely need to insert the condition in xhci_event(). This isn't fixable with just a tweak in xhci_pci_intr_raise, say - the HCI needs to behave as if interrupter mapping was unsupported, or it won't work.)
>
> 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 fact that macOS rejects xHC with numintrs < 4 implies macOS still
expects multiple Interrupters are enabled, but all Interrupters assert
the INTx# pin. Implementation of such a behavior will be contained in
xhci_pci_intr_raise() so we can save an indirection call of
intr_mapping_supported() as a bonus.
It's not as simple as redirecting xhci_pci_intr_raise for n != 0 to still trigger the pin-based interrupt when MSI(-X) is disabled with macOS guests. For one, pin-based interrupts are level-triggered, so you have to cheat with the IMAN_IP state. Even then though, the macOS guest will only process event rings for which it received an interrupt, which makes some sense, as there's no point having multiple rings if you're going to check all of them when you receive an interrupt for only one of them.
Regards,
Akihiko Odaki
>
> 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 */