On Fri, May 21, 2021 at 8:46 PM Philippe Mathieu-Daudé
<phi...@redhat.com> wrote:
>
> On 5/21/21 4:42 AM, Bin Meng wrote:
> > From: Ruimei Yan <ruimei....@windriver.com>
> >
> > Per xHCI spec v1.2 chapter 4.17.5 page 296:
> >
> > If MSI or MSI-X interrupts are enabled, Interrupt Pending (IP)
> > shall be cleared automatically when the PCI dword write generated
> > by the interrupt assertion is complete.
> >
> > Currently QEMU does not clear the IP flag in the MSI / MSI-X mode.
> > This causes subsequent spurious interrupt to be delivered to guests.
> > To solve this, we change the xhci intr_raise() hook routine to have
> > a bool return value that is passed to its caller (the xhci core),
> > with true indicating that IP should be self-cleared.
> >
> > Fixes: 62c6ae04cf43 ("xhci: Initial xHCI implementation")
> > Fixes: 4c47f800631a ("xhci: add msix support")
> > Signed-off-by: Ruimei Yan <ruimei....@windriver.com>
> > [bmeng: move IP clear codes from xhci pci to xhci core]
> > Signed-off-by: Bin Meng <bin.m...@windriver.com>
> > ---
> >
> > hw/usb/hcd-xhci.h | 2 +-
> > hw/usb/hcd-xhci-pci.c | 8 +++++---
> > hw/usb/hcd-xhci-sysbus.c | 4 +++-
> > hw/usb/hcd-xhci.c | 8 ++++++--
> > 4 files changed, 15 insertions(+), 7 deletions(-)
> >
> > diff --git a/hw/usb/hcd-xhci.h b/hw/usb/hcd-xhci.h
> > index 7bba361f3b..98f598382a 100644
> > --- a/hw/usb/hcd-xhci.h
> > +++ b/hw/usb/hcd-xhci.h
> > @@ -194,7 +194,7 @@ typedef struct XHCIState {
> > uint32_t flags;
> > uint32_t max_pstreams_mask;
> > void (*intr_update)(XHCIState *s, int n, bool enable);
> > - void (*intr_raise)(XHCIState *s, int n, bool level);
> > + bool (*intr_raise)(XHCIState *s, int n, bool level);
> > DeviceState *hostOpaque;
> >
> > /* Operational Registers */
> > diff --git a/hw/usb/hcd-xhci-pci.c b/hw/usb/hcd-xhci-pci.c
> > index b6acd1790c..e934b1a5b1 100644
> > --- a/hw/usb/hcd-xhci-pci.c
> > +++ b/hw/usb/hcd-xhci-pci.c
> > @@ -57,7 +57,7 @@ static void xhci_pci_intr_update(XHCIState *xhci, int n,
> > bool enable)
> > }
> > }
> >
> > -static void xhci_pci_intr_raise(XHCIState *xhci, int n, bool level)
> > +static bool xhci_pci_intr_raise(XHCIState *xhci, int n, bool level)
> > {
> > XHCIPciState *s = container_of(xhci, XHCIPciState, xhci);
> > PCIDevice *pci_dev = PCI_DEVICE(s);
> > @@ -70,13 +70,15 @@ static void xhci_pci_intr_raise(XHCIState *xhci, int n,
> > bool level)
> >
> > if (msix_enabled(pci_dev) && level) {
> > msix_notify(pci_dev, n);
> > - return;
> > + return true;
> > }
> >
> > if (msi_enabled(pci_dev) && level) {
> > msi_notify(pci_dev, n);
> > - return;
> > + return true;
> > }
> > +
> > + return false;
> > }
>
> Could be simplified as:
>
> if (!level) {
> return false;
> }
> if (msix_enabled(pci_dev)) {
> msix_notify(pci_dev, n);
> }
> if (msi_enabled(pci_dev)) {
> msi_notify(pci_dev, n);
> }
> return true;
The simplified logic will deliver both interrupts if both msix and msi
are enabled. The previous logic prevents such from happening.
>
> Otherwise,
> Reviewed-by: Philippe Mathieu-Daudé <phi...@redhat.com>
>
Regards,
Bin
