Message ID | 20210521024224.2277634-2-bmeng.cn@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [1/2] hw/usb: hcd-xhci-pci: Raise MSI/MSI-X interrupts only when told to | expand |
On 5/21/21 4:42 AM, Bin Meng wrote: > From: Ruimei Yan <ruimei.yan@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.yan@windriver.com> > [bmeng: move IP clear codes from xhci pci to xhci core] > Signed-off-by: Bin Meng <bin.meng@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; Otherwise, Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
On Fri, May 21, 2021 at 8:46 PM Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > > On 5/21/21 4:42 AM, Bin Meng wrote: > > From: Ruimei Yan <ruimei.yan@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.yan@windriver.com> > > [bmeng: move IP clear codes from xhci pci to xhci core] > > Signed-off-by: Bin Meng <bin.meng@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é <philmd@redhat.com> > Regards, Bin
On 5/21/21 3:25 PM, Bin Meng wrote: > On Fri, May 21, 2021 at 8:46 PM Philippe Mathieu-Daudé > <philmd@redhat.com> wrote: >> >> On 5/21/21 4:42 AM, Bin Meng wrote: >>> From: Ruimei Yan <ruimei.yan@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.yan@windriver.com> >>> [bmeng: move IP clear codes from xhci pci to xhci core] >>> Signed-off-by: Bin Meng <bin.meng@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. Oops you are right :) >> Otherwise, >> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> This stands.
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; } static void xhci_pci_reset(DeviceState *dev) diff --git a/hw/usb/hcd-xhci-sysbus.c b/hw/usb/hcd-xhci-sysbus.c index 42e2574c82..a14e438196 100644 --- a/hw/usb/hcd-xhci-sysbus.c +++ b/hw/usb/hcd-xhci-sysbus.c @@ -16,11 +16,13 @@ #include "hw/acpi/aml-build.h" #include "hw/irq.h" -static void xhci_sysbus_intr_raise(XHCIState *xhci, int n, bool level) +static bool xhci_sysbus_intr_raise(XHCIState *xhci, int n, bool level) { XHCISysbusState *s = container_of(xhci, XHCISysbusState, xhci); qemu_set_irq(s->irq[n], level); + + return false; } void xhci_sysbus_reset(DeviceState *dev) diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c index 46212b1e69..e01700039b 100644 --- a/hw/usb/hcd-xhci.c +++ b/hw/usb/hcd-xhci.c @@ -551,7 +551,9 @@ static void xhci_intr_update(XHCIState *xhci, int v) level = 1; } if (xhci->intr_raise) { - xhci->intr_raise(xhci, 0, level); + if (xhci->intr_raise(xhci, 0, level)) { + xhci->intr[0].iman &= ~IMAN_IP; + } } } if (xhci->intr_update) { @@ -579,7 +581,9 @@ static void xhci_intr_raise(XHCIState *xhci, int v) return; } if (xhci->intr_raise) { - xhci->intr_raise(xhci, v, true); + if (xhci->intr_raise(xhci, v, true)) { + xhci->intr[v].iman &= ~IMAN_IP; + } } }