| 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.
On Fri, 27 Dec 2024 at 21:27, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > 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? > How about this for the new commit message (I've also slightly clarified the final paragraph about macOS guests): hw/usb/hcd-xhci-pci: Use modulo to select MSI vector as per spec QEMU would crash with a failed assertion if the XHCI controller attempted to raise the interrupt on an interrupter corresponding to a MSI vector with a higher index than the highest configured for the device by the guest driver. This behaviour is correct on the MSI/PCI side: per PCI 3.0 spec, devices must ensure they do not send MSI notifications for vectors beyond the range of those allocated by the system/driver software. Unlike MSI-X, there is no generic way for handling aliasing in the case of fewer allocated vectors than requested, so the specifics are up to device implementors. (Section 6.8.3.4. "Sending Messages") 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. This makes the device work correctly in MSI mode with macOS's XHCI driver, which only allocates a single vector. 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. > msi_prepare_message() implements it by masking off the bits and bitwise or'ing the vector number, which must clearly be the correct method of doing it or guest OSes would not be happy. I was just struck by the lack of clarity in the spec. :-) Much like the handwaving in the XHCI spec on the point of pin-based interrupts. [Note to self: if I ever co-author a standard specification, make sure someone with no prior involvement implements it before ratification.]
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(+)