Implement MMIO PBA writes, 1 to trigger and 0 to clear.

This functionality is used by some qtests, which keep the msix irq
masked and test irq pending via the PBA bits, for simplicity. Some
tests expect to be able to clear the irq with a store, so a side-effect
of this is that qpci_msix_pending() would actually clear the pending
bit where it previously did not. This actually causes some [possibly
buggy] tests to fail. So to avoid breakage until tests are re-examined,
prior behavior of qpci_msix_pending() is kept by changing it to avoid
clearing PBA.

A new function qpci_msix_test_clear_pending() is added for tests that
do want the PBA clearing, and it will be used by XHCI and e1000e/igb
tests in subsequent changes.

Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
Cc: Dmitry Fleytman <dmitry.fleytman@gmail.com>
Cc: Akihiko Odaki <akihiko.odaki@daynix.com>
Cc: Sriram Yagnaraman <sriram.yagnaraman@ericsson.com>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 tests/qtest/libqos/pci.h |  1 +
 hw/pci/msix.c            | 16 ++++++++++++++++
 tests/qtest/libqos/pci.c | 20 +++++++++++++++++---
 3 files changed, 34 insertions(+), 3 deletions(-)

diff --git a/tests/qtest/libqos/pci.h b/tests/qtest/libqos/pci.h
index 5a7b2454ad5..de540f7803f 100644
--- a/tests/qtest/libqos/pci.h
+++ b/tests/qtest/libqos/pci.h
@@ -94,6 +94,7 @@ uint8_t qpci_find_capability(QPCIDevice *dev, uint8_t id, uint8_t start_addr);
 void qpci_msix_enable(QPCIDevice *dev);
 void qpci_msix_disable(QPCIDevice *dev);
 bool qpci_msix_pending(QPCIDevice *dev, uint16_t entry);
+bool qpci_msix_test_clear_pending(QPCIDevice *dev, uint16_t entry);
 bool qpci_msix_masked(QPCIDevice *dev, uint16_t entry);
 uint16_t qpci_msix_table_size(QPCIDevice *dev);
 
diff --git a/hw/pci/msix.c b/hw/pci/msix.c
index 487e49834ee..b16b03b888f 100644
--- a/hw/pci/msix.c
+++ b/hw/pci/msix.c
@@ -260,6 +260,22 @@ static uint64_t msix_pba_mmio_read(void *opaque, hwaddr addr,
 static void msix_pba_mmio_write(void *opaque, hwaddr addr,
                                 uint64_t val, unsigned size)
 {
+    PCIDevice *dev = opaque;
+    unsigned vector_start = addr * 8;
+    unsigned vector_end = MIN(addr + size * 8, dev->msix_entries_nr);
+    unsigned i;
+
+    for (i = vector_start; i < vector_end; i++) {
+        if ((val >> i) & 1) {
+            if (!msix_is_pending(dev, i)) {
+                msix_notify(dev, i);
+            }
+        } else {
+            if (msix_is_pending(dev, i)) {
+                msix_clr_pending(dev, i);
+            }
+        }
+    }
 }
 
 static const MemoryRegionOps msix_pba_mmio_ops = {
diff --git a/tests/qtest/libqos/pci.c b/tests/qtest/libqos/pci.c
index 023c1617680..f8d655a0e61 100644
--- a/tests/qtest/libqos/pci.c
+++ b/tests/qtest/libqos/pci.c
@@ -361,9 +361,23 @@ bool qpci_msix_pending(QPCIDevice *dev, uint16_t entry)
 
     g_assert(dev->msix_enabled);
     pba_entry = qpci_io_readl(dev, dev->msix_pba_bar, dev->msix_pba_off + off);
-    qpci_io_writel(dev, dev->msix_pba_bar, dev->msix_pba_off + off,
-                   pba_entry & ~(1 << bit_n));
-    return (pba_entry & (1 << bit_n)) != 0;
+    return pba_entry & (1 << bit_n);
+}
+
+bool qpci_msix_test_clear_pending(QPCIDevice *dev, uint16_t entry)
+{
+    uint32_t pba_entry;
+    uint8_t bit_n = entry % 32;
+    uint64_t  off = (entry / 32) * PCI_MSIX_ENTRY_SIZE / 4;
+
+    g_assert(dev->msix_enabled);
+    pba_entry = qpci_io_readl(dev, dev->msix_pba_bar, dev->msix_pba_off + off);
+    if (pba_entry & (1 << bit_n)) {
+        qpci_io_writel(dev, dev->msix_pba_bar, dev->msix_pba_off + off,
+                       pba_entry & ~(1 << bit_n));
+        return true;
+    }
+    return false;
 }
 
 bool qpci_msix_masked(QPCIDevice *dev, uint16_t entry)
-- 
2.45.2

On 2024/12/12 17:34, Nicholas Piggin wrote:
> Implement MMIO PBA writes, 1 to trigger and 0 to clear.
> 
> This functionality is used by some qtests, which keep the msix irq
> masked and test irq pending via the PBA bits, for simplicity. Some
> tests expect to be able to clear the irq with a store, so a side-effect
> of this is that qpci_msix_pending() would actually clear the pending
> bit where it previously did not. This actually causes some [possibly
> buggy] tests to fail. So to avoid breakage until tests are re-examined,
> prior behavior of qpci_msix_pending() is kept by changing it to avoid
> clearing PBA.
> 
> A new function qpci_msix_test_clear_pending() is added for tests that
> do want the PBA clearing, and it will be used by XHCI and e1000e/igb
> tests in subsequent changes.

The specification says software should never write Pending Bits and its 
result is undefined. Tests should have an alternative method to clear 
Pending Bits.

A possible solution is to unmask the interrupt, wait until the Pending 
Bits get cleared, and mask it again.

On Fri Dec 13, 2024 at 3:14 PM AEST, Akihiko Odaki wrote:
> On 2024/12/12 17:34, Nicholas Piggin wrote:
> > Implement MMIO PBA writes, 1 to trigger and 0 to clear.
> > 
> > This functionality is used by some qtests, which keep the msix irq
> > masked and test irq pending via the PBA bits, for simplicity. Some
> > tests expect to be able to clear the irq with a store, so a side-effect
> > of this is that qpci_msix_pending() would actually clear the pending
> > bit where it previously did not. This actually causes some [possibly
> > buggy] tests to fail. So to avoid breakage until tests are re-examined,
> > prior behavior of qpci_msix_pending() is kept by changing it to avoid
> > clearing PBA.
> > 
> > A new function qpci_msix_test_clear_pending() is added for tests that
> > do want the PBA clearing, and it will be used by XHCI and e1000e/igb
> > tests in subsequent changes.
>
> The specification says software should never write Pending Bits and its 
> result is undefined. Tests should have an alternative method to clear 
> Pending Bits.

Thanks for correcting me. I guess qpci_msix_pending() should not be
trying to write to the PBA either then.

> A possible solution is to unmask the interrupt, wait until the Pending 
> Bits get cleared, and mask it again.

PCI spec says

  If a masked vector has its Pending bit set, and the associated
  underlying interrupt events are somehow satisfied (usually by software
  though the exact manner is function-specific), the function must clear
  the Pending bit, to avoid sending a spurious interrupt message later
  when software unmasks the vector. However, if a subsequent interrupt
  event occurs while the vector is still masked, the function must again
  set the Pending bit.

It looks like e1000e acutally does that with e1000e_msix_clear{_one}.
So perhaps this will work just with the e1000e ICR clearing patch. I
will test.

e1000e and igb are the only devices that call msix_clr_pending. Does
that mean many others probably do not implement this behaviour
correctly?

Thanks,
Nick