Re: [PATCH v3 1/6] hw/usb/hcd-xhci-pci: Use modulo to select MSI vector as per spec

[Philippe Mathieu-Daudé]

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.