Re: [PATCH 2/2] hw/usb: hcd-xhci-pci: Fix spec violation of IP flag for MSI/MSI-X

[Philippe Mathieu-Daudé]

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>