On 5/21/21 3:25 PM, Bin Meng wrote:
> 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.
Oops you are right :)
>> Otherwise,
>> Reviewed-by: Philippe Mathieu-Daudé <phi...@redhat.com>
This stands.
