| Message ID | 20241227121336.25838-2-phil@philjordan.eu (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | hw/usb/hcd-xhci: Fixes, improvements, and macOS workaround | expand |
On 27/12/24 13:13, Phil Dennis-Jordan wrote: > QEMU would crash with a failed assertion if the XHCI controller > attempted to raise the interrupt on a higher vector than the > highest configured for the device by the guest driver. > > It turns out the XHCI spec (Implementation Note in section 4.17, > "Interrupters") requires that the host controller signal the MSI > vector with the number computed by taking the interrupter number > modulo the number of enabled MSI vectors. > > This change introduces that modulo calculation, fixing the > failed assertion and making the device work correctly in MSI mode > with macOS's XHCI driver. > > Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu> > --- > > v2: > > * Switch to modulo arithmetic for MSI vector number rather than dropping, > as per spec. > > hw/usb/hcd-xhci-pci.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/hw/usb/hcd-xhci-pci.c b/hw/usb/hcd-xhci-pci.c > index e110840c7a..e5e7330387 100644 > --- a/hw/usb/hcd-xhci-pci.c > +++ b/hw/usb/hcd-xhci-pci.c > @@ -74,6 +74,7 @@ static bool xhci_pci_intr_raise(XHCIState *xhci, int n, bool level) > } > > if (msi_enabled(pci_dev) && level) { > + n %= msi_nr_vectors_allocated(pci_dev); > msi_notify(pci_dev, n); Should this be done at the MSI layer in the callee? (I haven't checked the MSI spec). (Cc'ing hw/pci/msi.c maintainers) > return true; > }
On Fri 27. Dec 2024 at 18:45, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > On 27/12/24 13:13, Phil Dennis-Jordan wrote: > > QEMU would crash with a failed assertion if the XHCI controller > > attempted to raise the interrupt on a higher vector than the > > highest configured for the device by the guest driver. > > > > It turns out the XHCI spec (Implementation Note in section 4.17, > > "Interrupters") requires that the host controller signal the MSI > > vector with the number computed by taking the interrupter number > > modulo the number of enabled MSI vectors. > > > > This change introduces that modulo calculation, fixing the > > failed assertion and making the device work correctly in MSI mode > > with macOS's XHCI driver. > > > > Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu> > > --- > > > > v2: > > > > * Switch to modulo arithmetic for MSI vector number rather than > dropping, > > as per spec. > > > > hw/usb/hcd-xhci-pci.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/hw/usb/hcd-xhci-pci.c b/hw/usb/hcd-xhci-pci.c > > index e110840c7a..e5e7330387 100644 > > --- a/hw/usb/hcd-xhci-pci.c > > +++ b/hw/usb/hcd-xhci-pci.c > > @@ -74,6 +74,7 @@ static bool xhci_pci_intr_raise(XHCIState *xhci, int > n, bool level) > > } > > > > if (msi_enabled(pci_dev) && level) { > > + n %= msi_nr_vectors_allocated(pci_dev); > > msi_notify(pci_dev, n); > > Should this be done at the MSI layer in the callee? > (I haven't checked the MSI spec). > > (Cc'ing hw/pci/msi.c maintainers) > MSI-X has specified aliasing behaviour. As far as I can tell, MSI does not - this does not seem especially ambiguous either. From the PCI base spec 3.0: 6.8.3.4. Sending Messages […] “If the Multiple Message Enable field is “000”, the function is not permitted to modify the message data.” […] “How a function uses multiple vectors (when allocated) is device dependent. A function must handle being allocated fewer vectors than requested.” I understand that to mean that MSI vector aliasing is entirely device-specific, and the assertion in msi_notify() is correct. The XHCI specification statement that the vector should be determined via the modulus of the interrupter index and the number of allocated MSI vectors does indeed seem to be XHCI-specific. (NB: reading the PCI spec, I’m struck that it seems very vague on HOW the low bits of the message data should be “modified” to encode the vector number. But perhaps I’ve just not found that section yet.) > > return true; > > } > >
On 27/12/24 20:45, Phil Dennis-Jordan wrote: > > > On Fri 27. Dec 2024 at 18:45, Philippe Mathieu-Daudé <philmd@linaro.org > <mailto:philmd@linaro.org>> wrote: > > On 27/12/24 13:13, Phil Dennis-Jordan wrote: > > QEMU would crash with a failed assertion if the XHCI controller > > attempted to raise the interrupt on a higher vector than the > > highest configured for the device by the guest driver. > > > > It turns out the XHCI spec (Implementation Note in section 4.17, > > "Interrupters") requires that the host controller signal the MSI > > vector with the number computed by taking the interrupter number > > modulo the number of enabled MSI vectors. > > > > This change introduces that modulo calculation, fixing the > > failed assertion and making the device work correctly in MSI mode > > with macOS's XHCI driver. > > > > Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu > <mailto:phil@philjordan.eu>> > > --- > > > > v2: > > > > * Switch to modulo arithmetic for MSI vector number rather than > dropping, > > as per spec. > > > > hw/usb/hcd-xhci-pci.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/hw/usb/hcd-xhci-pci.c b/hw/usb/hcd-xhci-pci.c > > index e110840c7a..e5e7330387 100644 > > --- a/hw/usb/hcd-xhci-pci.c > > +++ b/hw/usb/hcd-xhci-pci.c > > @@ -74,6 +74,7 @@ static bool xhci_pci_intr_raise(XHCIState > *xhci, int n, bool level) > > } > > > > if (msi_enabled(pci_dev) && level) { > > + n %= msi_nr_vectors_allocated(pci_dev); > > msi_notify(pci_dev, n); > > Should this be done at the MSI layer in the callee? > (I haven't checked the MSI spec). > > (Cc'ing hw/pci/msi.c maintainers) > > > MSI-X has specified aliasing behaviour. As far as I can tell, MSI does > not - this does not seem especially ambiguous either. From the PCI base > spec 3.0: > > 6.8.3.4. Sending Messages > […] > “If the Multiple Message Enable field is “000”, the function is not > permitted to modify the message data.” > […] > “How a function uses multiple vectors (when allocated) is device > dependent. A function must handle being allocated fewer vectors than > requested.” > > > I understand that to mean that MSI vector aliasing is entirely device- > specific, and the assertion in msi_notify() is correct. The XHCI > specification statement that the vector should be determined via the > modulus of the interrupter index and the number of allocated MSI vectors > does indeed seem to be XHCI-specific. OK, thanks for checking! Should we add this new information to the patch description? Otherwise, Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> > (NB: reading the PCI spec, I’m struck that it seems very vague on HOW > the low bits of the message data should be “modified” to encode the > vector number. But perhaps I’ve just not found that section yet.) Maybe MST/Marcel know.
diff --git a/hw/usb/hcd-xhci-pci.c b/hw/usb/hcd-xhci-pci.c index e110840c7a..e5e7330387 100644 --- a/hw/usb/hcd-xhci-pci.c +++ b/hw/usb/hcd-xhci-pci.c @@ -74,6 +74,7 @@ static bool xhci_pci_intr_raise(XHCIState *xhci, int n, bool level) } if (msi_enabled(pci_dev) && level) { + n %= msi_nr_vectors_allocated(pci_dev); msi_notify(pci_dev, n); return true; }
QEMU would crash with a failed assertion if the XHCI controller attempted to raise the interrupt on a higher vector than the highest configured for the device by the guest driver. It turns out the XHCI spec (Implementation Note in section 4.17, "Interrupters") requires that the host controller signal the MSI vector with the number computed by taking the interrupter number modulo the number of enabled MSI vectors. This change introduces that modulo calculation, fixing the failed assertion and making the device work correctly in MSI mode with macOS's XHCI driver. Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu> --- v2: * Switch to modulo arithmetic for MSI vector number rather than dropping, as per spec. hw/usb/hcd-xhci-pci.c | 1 + 1 file changed, 1 insertion(+)