[PATCH 0/5] qtest: pci and e1000e/igb msix fixes

[PATCH 0/5] qtest: pci and e1000e/igb msix fixes

[Nicholas Piggin]

Hi,

This series is split out from a larger one that added some more
XHCI functionality and tests here. Just wanted to get more focus
on the PCI issues first.

https://lore.kernel.org/qemu-devel/20241212083502.1439033-1-npiggin@gmail.com/T/#t

It is quite reworked after feedback about the incorrect PBA write
implementation. This one solves the e1000e/igb multiple-intrrupt problem
by enabling the msix vector which delivers the interrupt and lowers the
PBA pending bit.

I'm still curious about PCI spec note that masked pending interrupt
must have PBA bit cleared if the interrupt condition in the function
clears, which no device seems to implement... but at least for now,
unmasking and delivering seems to be the reliable way to clear the
interrupt.

Thanks,
Nick


Nicholas Piggin (5):
  qtest/pci: Enforce balanced iomap/unmap
  qtest/libqos/pci: Fix qpci_msix_enable sharing bar0
  qtest/libqos/pci: Do not write to PBA memory
  qtest/e1000e|igb: Clear interrupt-cause bits after irq
  qtest/e1000e|igb: Fix msix to re-trigger interrupts

 tests/qtest/libqos/ahci.h       |   1 +
 tests/qtest/libqos/e1000e.h     |   8 +++
 tests/qtest/libqos/pci.h        |   3 +
 tests/qtest/libqos/virtio-pci.h |   1 +
 tests/qtest/ahci-test.c         |   2 +
 tests/qtest/e1000e-test.c       |  10 ++-
 tests/qtest/igb-test.c          |  10 ++-
 tests/qtest/libqos/ahci.c       |   6 ++
 tests/qtest/libqos/e1000e.c     | 113 +++++++++++++++++++++++++++++++-
 tests/qtest/libqos/pci.c        |  74 ++++++++++++++++++---
 tests/qtest/libqos/virtio-pci.c |   6 +-
 11 files changed, 217 insertions(+), 17 deletions(-)

-- 
2.45.2

[PATCH 1/5] qtest/pci: Enforce balanced iomap/unmap

[Nicholas Piggin]

Add assertions to ensure a BAR is not mapped twice, and only
previously mapped BARs are unmapped. This can help catch some
bugs.

Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
Reviewed-by: Fabiano Rosas <farosas@suse.de>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 tests/qtest/libqos/ahci.h       |  1 +
 tests/qtest/libqos/pci.h        |  2 ++
 tests/qtest/libqos/virtio-pci.h |  1 +
 tests/qtest/ahci-test.c         |  2 ++
 tests/qtest/libqos/ahci.c       |  6 ++++++
 tests/qtest/libqos/pci.c        | 32 +++++++++++++++++++++++++++++++-
 tests/qtest/libqos/virtio-pci.c |  6 +++++-
 7 files changed, 48 insertions(+), 2 deletions(-)

diff --git a/tests/qtest/libqos/ahci.h b/tests/qtest/libqos/ahci.h
index a0487a1557d..5d7e26aee2a 100644
--- a/tests/qtest/libqos/ahci.h
+++ b/tests/qtest/libqos/ahci.h
@@ -575,6 +575,7 @@ QPCIDevice *get_ahci_device(QTestState *qts, uint32_t *fingerprint);
 void free_ahci_device(QPCIDevice *dev);
 void ahci_pci_enable(AHCIQState *ahci);
 void start_ahci_device(AHCIQState *ahci);
+void stop_ahci_device(AHCIQState *ahci);
 void ahci_hba_enable(AHCIQState *ahci);
 
 /* Port Management */
diff --git a/tests/qtest/libqos/pci.h b/tests/qtest/libqos/pci.h
index 83896145235..9dc82ea723a 100644
--- a/tests/qtest/libqos/pci.h
+++ b/tests/qtest/libqos/pci.h
@@ -65,6 +65,8 @@ struct QPCIDevice
 {
     QPCIBus *bus;
     int devfn;
+    bool bars_mapped[6];
+    QPCIBar bars[6];
     bool msix_enabled;
     QPCIBar msix_table_bar, msix_pba_bar;
     uint64_t msix_table_off, msix_pba_off;
diff --git a/tests/qtest/libqos/virtio-pci.h b/tests/qtest/libqos/virtio-pci.h
index f5115cacba2..efdf904b254 100644
--- a/tests/qtest/libqos/virtio-pci.h
+++ b/tests/qtest/libqos/virtio-pci.h
@@ -26,6 +26,7 @@ typedef struct QVirtioPCIDevice {
     uint64_t config_msix_addr;
     uint32_t config_msix_data;
 
+    bool enabled;
     int bar_idx;
 
     /* VIRTIO 1.0 */
diff --git a/tests/qtest/ahci-test.c b/tests/qtest/ahci-test.c
index 5a1923f721b..b3dae7a8ce4 100644
--- a/tests/qtest/ahci-test.c
+++ b/tests/qtest/ahci-test.c
@@ -1483,6 +1483,8 @@ static void test_reset_pending_callback(void)
     /* Wait for throttled write to finish. */
     sleep(1);
 
+    stop_ahci_device(ahci);
+
     /* Start again. */
     ahci_clean_mem(ahci);
     ahci_pci_enable(ahci);
diff --git a/tests/qtest/libqos/ahci.c b/tests/qtest/libqos/ahci.c
index 34a75b7f43b..cfc435b6663 100644
--- a/tests/qtest/libqos/ahci.c
+++ b/tests/qtest/libqos/ahci.c
@@ -217,6 +217,12 @@ void start_ahci_device(AHCIQState *ahci)
     qpci_device_enable(ahci->dev);
 }
 
+void stop_ahci_device(AHCIQState *ahci)
+{
+    /* Map AHCI's ABAR (BAR5) */
+    qpci_iounmap(ahci->dev, ahci->hba_bar);
+}
+
 /**
  * Test and initialize the AHCI's HBA memory areas.
  * Initialize and start any ports with devices attached.
diff --git a/tests/qtest/libqos/pci.c b/tests/qtest/libqos/pci.c
index b23d72346b6..a42ca08261d 100644
--- a/tests/qtest/libqos/pci.c
+++ b/tests/qtest/libqos/pci.c
@@ -93,12 +93,17 @@ QPCIDevice *qpci_device_find(QPCIBus *bus, int devfn)
 void qpci_device_init(QPCIDevice *dev, QPCIBus *bus, QPCIAddress *addr)
 {
     uint16_t vendor_id, device_id;
+    int i;
 
     qpci_device_set(dev, bus, addr->devfn);
     vendor_id = qpci_config_readw(dev, PCI_VENDOR_ID);
     device_id = qpci_config_readw(dev, PCI_DEVICE_ID);
     g_assert(!addr->vendor_id || vendor_id == addr->vendor_id);
     g_assert(!addr->device_id || device_id == addr->device_id);
+
+    for (i = 0; i < 6; i++) {
+        g_assert(!dev->bars_mapped[i]);
+    }
 }
 
 static uint8_t qpci_find_resource_reserve_capability(QPCIDevice *dev)
@@ -531,6 +536,8 @@ QPCIBar qpci_iomap(QPCIDevice *dev, int barno, uint64_t *sizeptr)
     uint64_t loc;
 
     g_assert(barno >= 0 && barno <= 5);
+    g_assert(!dev->bars_mapped[barno]);
+
     bar_reg = bar_reg_map[barno];
 
     qpci_config_writel(dev, bar_reg, 0xFFFFFFFF);
@@ -574,12 +581,35 @@ QPCIBar qpci_iomap(QPCIDevice *dev, int barno, uint64_t *sizeptr)
     }
 
     bar.addr = loc;
+
+    dev->bars_mapped[barno] = true;
+    dev->bars[barno] = bar;
+
     return bar;
 }
 
 void qpci_iounmap(QPCIDevice *dev, QPCIBar bar)
 {
-    /* FIXME */
+    static const int bar_reg_map[] = {
+        PCI_BASE_ADDRESS_0, PCI_BASE_ADDRESS_1, PCI_BASE_ADDRESS_2,
+        PCI_BASE_ADDRESS_3, PCI_BASE_ADDRESS_4, PCI_BASE_ADDRESS_5,
+    };
+    int bar_reg;
+    int i;
+
+    for (i = 0; i < 6; i++) {
+        if (!dev->bars_mapped[i]) {
+            continue;
+        }
+        if (dev->bars[i].addr == bar.addr) {
+            dev->bars_mapped[i] = false;
+            bar_reg = bar_reg_map[i];
+            qpci_config_writel(dev, bar_reg, 0xFFFFFFFF);
+            /* FIXME: the address space is leaked */
+            return;
+        }
+    }
+    g_assert_not_reached();
 }
 
 QPCIBar qpci_legacy_iomap(QPCIDevice *dev, uint16_t addr)
diff --git a/tests/qtest/libqos/virtio-pci.c b/tests/qtest/libqos/virtio-pci.c
index 485b8f6b7e0..2b59fb181c9 100644
--- a/tests/qtest/libqos/virtio-pci.c
+++ b/tests/qtest/libqos/virtio-pci.c
@@ -304,11 +304,15 @@ void qvirtio_pci_device_enable(QVirtioPCIDevice *d)
 {
     qpci_device_enable(d->pdev);
     d->bar = qpci_iomap(d->pdev, d->bar_idx, NULL);
+    d->enabled = true;
 }
 
 void qvirtio_pci_device_disable(QVirtioPCIDevice *d)
 {
-    qpci_iounmap(d->pdev, d->bar);
+    if (d->enabled) {
+        qpci_iounmap(d->pdev, d->bar);
+        d->enabled = false;
+    }
 }
 
 void qvirtqueue_pci_msix_setup(QVirtioPCIDevice *d, QVirtQueuePCI *vqpci,
-- 
2.45.2

[PATCH 2/5] qtest/libqos/pci: Fix qpci_msix_enable sharing bar0

[Nicholas Piggin]

Devices where the MSI-X addresses are shared with other MMIO on BAR0
can not use msi_enable because it unmaps and remaps BAR0, which
interferes with device MMIO mappings. xhci-nec is one such device we
would like to test msix with.

Use the BAR iomap tracking structure introduced in the previous change
to have qpci_misx_enable() use existing iomaps if msix bars are
already mapped.

Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
Reviewed-by: Fabiano Rosas <farosas@suse.de>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 tests/qtest/libqos/pci.h |  1 +
 tests/qtest/libqos/pci.c | 40 ++++++++++++++++++++++++++++++++++------
 2 files changed, 35 insertions(+), 6 deletions(-)

diff --git a/tests/qtest/libqos/pci.h b/tests/qtest/libqos/pci.h
index 9dc82ea723a..5a7b2454ad5 100644
--- a/tests/qtest/libqos/pci.h
+++ b/tests/qtest/libqos/pci.h
@@ -68,6 +68,7 @@ struct QPCIDevice
     bool bars_mapped[6];
     QPCIBar bars[6];
     bool msix_enabled;
+    bool msix_table_bar_iomap, msix_pba_bar_iomap;
     QPCIBar msix_table_bar, msix_pba_bar;
     uint64_t msix_table_off, msix_pba_off;
 };
diff --git a/tests/qtest/libqos/pci.c b/tests/qtest/libqos/pci.c
index a42ca08261d..023c1617680 100644
--- a/tests/qtest/libqos/pci.c
+++ b/tests/qtest/libqos/pci.c
@@ -288,15 +288,21 @@ void qpci_msix_enable(QPCIDevice *dev)
 
     table = qpci_config_readl(dev, addr + PCI_MSIX_TABLE);
     bir_table = table & PCI_MSIX_FLAGS_BIRMASK;
-    dev->msix_table_bar = qpci_iomap(dev, bir_table, NULL);
+    if (dev->bars_mapped[bir_table]) {
+        dev->msix_table_bar = dev->bars[bir_table];
+    } else {
+        dev->msix_table_bar_iomap = true;
+        dev->msix_table_bar = qpci_iomap(dev, bir_table, NULL);
+    }
     dev->msix_table_off = table & ~PCI_MSIX_FLAGS_BIRMASK;
 
     table = qpci_config_readl(dev, addr + PCI_MSIX_PBA);
     bir_pba = table & PCI_MSIX_FLAGS_BIRMASK;
-    if (bir_pba != bir_table) {
-        dev->msix_pba_bar = qpci_iomap(dev, bir_pba, NULL);
+    if (dev->bars_mapped[bir_pba]) {
+        dev->msix_pba_bar = dev->bars[bir_pba];
     } else {
-        dev->msix_pba_bar = dev->msix_table_bar;
+        dev->msix_pba_bar_iomap = true;
+        dev->msix_pba_bar = qpci_iomap(dev, bir_pba, NULL);
     }
     dev->msix_pba_off = table & ~PCI_MSIX_FLAGS_BIRMASK;
 
@@ -307,6 +313,7 @@ void qpci_msix_disable(QPCIDevice *dev)
 {
     uint8_t addr;
     uint16_t val;
+    uint32_t table;
 
     g_assert(dev->msix_enabled);
     addr = qpci_find_capability(dev, PCI_CAP_ID_MSIX, 0);
@@ -315,10 +322,31 @@ void qpci_msix_disable(QPCIDevice *dev)
     qpci_config_writew(dev, addr + PCI_MSIX_FLAGS,
                                                 val & ~PCI_MSIX_FLAGS_ENABLE);
 
-    if (dev->msix_pba_bar.addr != dev->msix_table_bar.addr) {
+    if (dev->msix_pba_bar_iomap) {
+        dev->msix_pba_bar_iomap = false;
         qpci_iounmap(dev, dev->msix_pba_bar);
+    } else {
+        /*
+         * If we had reused an existing iomap, ensure it is still mapped
+         * otherwise it would be a bug if it were unmapped before msix is
+         * disabled. A refcounting iomap implementation could avoid this
+         * issue entirely, but let's wait until that's needed.
+         */
+        uint8_t bir_pba;
+        table = qpci_config_readl(dev, addr + PCI_MSIX_PBA);
+        bir_pba = table & PCI_MSIX_FLAGS_BIRMASK;
+        g_assert(dev->bars_mapped[bir_pba]);
+    }
+
+    if (dev->msix_table_bar_iomap) {
+        dev->msix_table_bar_iomap = false;
+        qpci_iounmap(dev, dev->msix_table_bar);
+    } else {
+        uint8_t bir_table;
+        table = qpci_config_readl(dev, addr + PCI_MSIX_TABLE);
+        bir_table = table & PCI_MSIX_FLAGS_BIRMASK;
+        g_assert(dev->bars_mapped[bir_table]);
     }
-    qpci_iounmap(dev, dev->msix_table_bar);
 
     dev->msix_enabled = 0;
     dev->msix_table_off = 0;
-- 
2.45.2

[PATCH 3/5] qtest/libqos/pci: Do not write to PBA memory

[Nicholas Piggin]

The PCI Local Bus Specification says the result of writes to MSI-X
PBA memory is undefined. QEMU implements them as no-ops, so remove
the pointless write from qpci_msix_pending().

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.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/tests/qtest/libqos/pci.c b/tests/qtest/libqos/pci.c
index 023c1617680..a187349d30a 100644
--- a/tests/qtest/libqos/pci.c
+++ b/tests/qtest/libqos/pci.c
@@ -361,8 +361,6 @@ 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;
 }
 
-- 
2.45.2

[PATCH 4/5] qtest/e1000e|igb: Clear interrupt-cause bits after irq

[Nicholas Piggin]

The e1000e and igb tests do not clear the ICR/EICR cause bits (or
set auto-clear) on seeing queue interrupts, which inhibits the
triggering of a new interrupt.

Fix this by clearing the cause bits, and verify that the expected
cause bit was set.

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/e1000e-test.c | 8 ++++++--
 tests/qtest/igb-test.c    | 8 ++++++--
 2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/tests/qtest/e1000e-test.c b/tests/qtest/e1000e-test.c
index de9738fdb74..a69759da70e 100644
--- a/tests/qtest/e1000e-test.c
+++ b/tests/qtest/e1000e-test.c
@@ -64,8 +64,10 @@ static void e1000e_send_verify(QE1000E *d, int *test_sockets, QGuestAllocator *a
     /* Put descriptor to the ring */
     e1000e_tx_ring_push(d, &descr);
 
-    /* Wait for TX WB interrupt */
+    /* Wait for TX WB interrupt (this clears the MSIX PBA) */
     e1000e_wait_isr(d, E1000E_TX0_MSG_ID);
+    /* Read ICR which clears it ready for next interrupt, assert TXQ0 cause */
+    g_assert(e1000e_macreg_read(d, E1000_ICR) & E1000_ICR_TXQ0);
 
     /* Check DD bit */
     g_assert_cmphex(le32_to_cpu(descr.upper.data) & E1000_TXD_STAT_DD, ==,
@@ -115,8 +117,10 @@ static void e1000e_receive_verify(QE1000E *d, int *test_sockets, QGuestAllocator
     /* Put descriptor to the ring */
     e1000e_rx_ring_push(d, &descr);
 
-    /* Wait for TX WB interrupt */
+    /* Wait for TX WB interrupt (this clears the MSIX PBA) */
     e1000e_wait_isr(d, E1000E_RX0_MSG_ID);
+    /* Read ICR which clears it ready for next interrupt, assert RXQ0 cause */
+    g_assert(e1000e_macreg_read(d, E1000_ICR) & E1000_ICR_RXQ0);
 
     /* Check DD bit */
     g_assert_cmphex(le32_to_cpu(descr.wb.upper.status_error) &
diff --git a/tests/qtest/igb-test.c b/tests/qtest/igb-test.c
index 3d397ea6973..2f22c4fb208 100644
--- a/tests/qtest/igb-test.c
+++ b/tests/qtest/igb-test.c
@@ -67,8 +67,10 @@ static void igb_send_verify(QE1000E *d, int *test_sockets, QGuestAllocator *allo
     /* Put descriptor to the ring */
     e1000e_tx_ring_push(d, &descr);
 
-    /* Wait for TX WB interrupt */
+    /* Wait for TX WB interrupt (this clears the MSIX PBA) */
     e1000e_wait_isr(d, E1000E_TX0_MSG_ID);
+    /* Read EICR which clears it ready for next interrupt, assert TXQ0 cause */
+    g_assert(e1000e_macreg_read(d, E1000_EICR) & (1 << E1000E_TX0_MSG_ID));
 
     /* Check DD bit */
     g_assert_cmphex(le32_to_cpu(descr.wb.status) & E1000_TXD_STAT_DD, ==,
@@ -118,8 +120,10 @@ static void igb_receive_verify(QE1000E *d, int *test_sockets, QGuestAllocator *a
     /* Put descriptor to the ring */
     e1000e_rx_ring_push(d, &descr);
 
-    /* Wait for TX WB interrupt */
+    /* Wait for TX WB interrupt (this clears the MSIX PBA) */
     e1000e_wait_isr(d, E1000E_RX0_MSG_ID);
+    /* Read EICR which clears it ready for next interrupt, assert RXQ0 cause */
+    g_assert(e1000e_macreg_read(d, E1000_EICR) & (1 << E1000E_RX0_MSG_ID));
 
     /* Check DD bit */
     g_assert_cmphex(le32_to_cpu(descr.wb.upper.status_error) &
-- 
2.45.2

[PATCH 5/5] qtest/e1000e|igb: Fix msix to re-trigger interrupts

[Nicholas Piggin]

The e1000e and igb tests don't clear the msix pending bit after waiting
for it, as it is masked so the irq doesn't get sent. Failing to clear
the pending interrupt means all subsequent waits for that interrupt
after the first do not actually wait for an interrupt genreated by the
device.

This affects the multiple_transfers tests, they never actually verify
more than one interrupt is generated. So for those tests, enable the
msix vectors for the queue interrupts so they are delivered and the
pending bit is cleared.

Add assertions to ensure the masked pending tests are not used to test
an interrupt multiple times.

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/e1000e.h |   8 +++
 tests/qtest/e1000e-test.c   |   2 +
 tests/qtest/igb-test.c      |   2 +
 tests/qtest/libqos/e1000e.c | 113 +++++++++++++++++++++++++++++++++++-
 4 files changed, 122 insertions(+), 3 deletions(-)

diff --git a/tests/qtest/libqos/e1000e.h b/tests/qtest/libqos/e1000e.h
index 30643c80949..6cc1a9732b1 100644
--- a/tests/qtest/libqos/e1000e.h
+++ b/tests/qtest/libqos/e1000e.h
@@ -25,6 +25,9 @@
 #define E1000E_RX0_MSG_ID           (0)
 #define E1000E_TX0_MSG_ID           (1)
 
+#define E1000E_RX0_MSIX_DATA        (0x12345678)
+#define E1000E_TX0_MSIX_DATA        (0xabcdef00)
+
 #define E1000E_ADDRESS { 0x52, 0x54, 0x00, 0x12, 0x34, 0x56 }
 
 typedef struct QE1000E QE1000E;
@@ -40,6 +43,10 @@ struct QE1000E_PCI {
     QPCIDevice pci_dev;
     QPCIBar mac_regs;
     QE1000E e1000e;
+    uint64_t msix_rx0_addr;
+    uint64_t msix_tx0_addr;
+    bool msix_found_rx0_pending;
+    bool msix_found_tx0_pending;
 };
 
 static inline void e1000e_macreg_write(QE1000E *d, uint32_t reg, uint32_t val)
@@ -57,5 +64,6 @@ static inline uint32_t e1000e_macreg_read(QE1000E *d, uint32_t reg)
 void e1000e_wait_isr(QE1000E *d, uint16_t msg_id);
 void e1000e_tx_ring_push(QE1000E *d, void *descr);
 void e1000e_rx_ring_push(QE1000E *d, void *descr);
+void e1000e_pci_msix_enable_rxtxq_vectors(QE1000E *d, QGuestAllocator *alloc);
 
 #endif
diff --git a/tests/qtest/e1000e-test.c b/tests/qtest/e1000e-test.c
index a69759da70e..4921a141ffe 100644
--- a/tests/qtest/e1000e-test.c
+++ b/tests/qtest/e1000e-test.c
@@ -185,6 +185,8 @@ static void test_e1000e_multiple_transfers(void *obj, void *data,
         return;
     }
 
+    /* Triggering msix interrupts multiple times so must enable vectors */
+    e1000e_pci_msix_enable_rxtxq_vectors(d, alloc);
     for (i = 0; i < iterations; i++) {
         e1000e_send_verify(d, data, alloc);
         e1000e_receive_verify(d, data, alloc);
diff --git a/tests/qtest/igb-test.c b/tests/qtest/igb-test.c
index 2f22c4fb208..06082cbe7ff 100644
--- a/tests/qtest/igb-test.c
+++ b/tests/qtest/igb-test.c
@@ -188,6 +188,8 @@ static void test_igb_multiple_transfers(void *obj, void *data,
         return;
     }
 
+    /* Triggering msix interrupts multiple times so must enable vectors */
+    e1000e_pci_msix_enable_rxtxq_vectors(d, alloc);
     for (i = 0; i < iterations; i++) {
         igb_send_verify(d, data, alloc);
         igb_receive_verify(d, data, alloc);
diff --git a/tests/qtest/libqos/e1000e.c b/tests/qtest/libqos/e1000e.c
index 925654c7fd4..7b7e811bce7 100644
--- a/tests/qtest/libqos/e1000e.c
+++ b/tests/qtest/libqos/e1000e.c
@@ -19,6 +19,7 @@
 #include "qemu/osdep.h"
 #include "hw/net/e1000_regs.h"
 #include "hw/pci/pci_ids.h"
+#include "hw/pci/pci_regs.h"
 #include "../libqtest.h"
 #include "pci-pc.h"
 #include "qemu/sockets.h"
@@ -77,16 +78,77 @@ static void e1000e_foreach_callback(QPCIDevice *dev, int devfn, void *data)
     g_free(dev);
 }
 
+static bool e1000e_test_msix_irq(QE1000E *d, uint16_t msg_id,
+                                 uint64_t guest_msix_addr,
+                                 uint32_t msix_data)
+{
+    QE1000E_PCI *d_pci = container_of(d, QE1000E_PCI, e1000e);
+    QPCIDevice *pci_dev = &d_pci->pci_dev;
+
+    if (msg_id == E1000E_RX0_MSG_ID) {
+        g_assert(!d_pci->msix_found_rx0_pending);
+    } else if (msg_id == E1000E_TX0_MSG_ID) {
+        g_assert(!d_pci->msix_found_tx0_pending);
+    } else {
+        /* Must enable MSI-X vector to test multiple messages */
+        g_assert_not_reached();
+    }
+
+    if (pci_dev->msix_enabled) {
+        if (qpci_msix_masked(pci_dev, msg_id)) {
+            /* No ISR checking should be done if masked, but read anyway */
+            bool p = qpci_msix_pending(pci_dev, msg_id);
+            if (p) {
+                if (msg_id == E1000E_RX0_MSG_ID) {
+                    d_pci->msix_found_rx0_pending = true;
+                } else if (msg_id == E1000E_TX0_MSG_ID) {
+                    d_pci->msix_found_tx0_pending = true;
+                } else {
+                    g_assert_not_reached();
+                }
+            }
+            return p;
+        } else {
+            uint32_t data = qtest_readl(pci_dev->bus->qts, guest_msix_addr);
+            if (data == msix_data) {
+                qtest_writel(pci_dev->bus->qts, guest_msix_addr, 0);
+                return true;
+            } else if (data == 0) {
+                return false;
+            } else {
+                g_assert_not_reached();
+            }
+        }
+    } else {
+        g_assert_not_reached();
+    }
+}
+
 void e1000e_wait_isr(QE1000E *d, uint16_t msg_id)
 {
     QE1000E_PCI *d_pci = container_of(d, QE1000E_PCI, e1000e);
-    guint64 end_time = g_get_monotonic_time() + 5 * G_TIME_SPAN_SECOND;
+    QPCIDevice *pci_dev = &d_pci->pci_dev;
+    uint64_t end_time = g_get_monotonic_time() + 5 * G_TIME_SPAN_SECOND;
+    uint64_t guest_msix_addr;
+    uint32_t msix_data;
+
+    assert(pci_dev->msix_enabled);
+
+    if (msg_id == E1000E_RX0_MSG_ID) {
+        guest_msix_addr = d_pci->msix_rx0_addr;
+        msix_data = E1000E_RX0_MSIX_DATA;
+    } else if (msg_id == E1000E_TX0_MSG_ID) {
+        guest_msix_addr = d_pci->msix_tx0_addr;
+        msix_data = E1000E_TX0_MSIX_DATA;
+    } else {
+        g_assert_not_reached();
+    }
 
     do {
-        if (qpci_msix_pending(&d_pci->pci_dev, msg_id)) {
+        if (e1000e_test_msix_irq(d, msg_id, guest_msix_addr, msix_data)) {
             return;
         }
-        qtest_clock_step(d_pci->pci_dev.bus->qts, 10000);
+        qtest_clock_step(pci_dev->bus->qts, 10000);
     } while (g_get_monotonic_time() < end_time);
 
     g_error("Timeout expired");
@@ -99,6 +161,51 @@ static void e1000e_pci_destructor(QOSGraphObject *obj)
     qpci_msix_disable(&epci->pci_dev);
 }
 
+static void e1000e_pci_msix_enable_vector(QE1000E *d, uint16_t msg_id,
+                                          uint64_t guest_msix_addr,
+                                          uint32_t msix_data)
+{
+    QE1000E_PCI *d_pci = container_of(d, QE1000E_PCI, e1000e);
+    QPCIDevice *pci_dev = &d_pci->pci_dev;
+    uint32_t control;
+    uint64_t off;
+
+    g_assert_cmpint(msg_id , >=, 0);
+    g_assert_cmpint(msg_id , <, qpci_msix_table_size(pci_dev));
+
+    off = pci_dev->msix_table_off + (msg_id * 16);
+
+    qpci_io_writel(pci_dev, pci_dev->msix_table_bar,
+                   off + PCI_MSIX_ENTRY_LOWER_ADDR, guest_msix_addr & ~0UL);
+    qpci_io_writel(pci_dev, pci_dev->msix_table_bar,
+                   off + PCI_MSIX_ENTRY_UPPER_ADDR,
+                   (guest_msix_addr >> 32) & ~0UL);
+    qpci_io_writel(pci_dev, pci_dev->msix_table_bar,
+                   off + PCI_MSIX_ENTRY_DATA, msix_data);
+
+    control = qpci_io_readl(pci_dev, pci_dev->msix_table_bar,
+                            off + PCI_MSIX_ENTRY_VECTOR_CTRL);
+    qpci_io_writel(pci_dev, pci_dev->msix_table_bar,
+                   off + PCI_MSIX_ENTRY_VECTOR_CTRL,
+                   control & ~PCI_MSIX_ENTRY_CTRL_MASKBIT);
+}
+
+void e1000e_pci_msix_enable_rxtxq_vectors(QE1000E *d, QGuestAllocator *alloc)
+{
+    QE1000E_PCI *d_pci = container_of(d, QE1000E_PCI, e1000e);
+    QPCIDevice *pci_dev = &d_pci->pci_dev;
+
+    g_assert(pci_dev->msix_enabled);
+
+    d_pci->msix_rx0_addr = guest_alloc(alloc, 4);
+    d_pci->msix_tx0_addr = guest_alloc(alloc, 4);
+
+    e1000e_pci_msix_enable_vector(d, E1000E_RX0_MSG_ID,
+                                  d_pci->msix_rx0_addr, E1000E_RX0_MSIX_DATA);
+    e1000e_pci_msix_enable_vector(d, E1000E_TX0_MSG_ID,
+                                  d_pci->msix_tx0_addr, E1000E_TX0_MSIX_DATA);
+}
+
 static void e1000e_pci_start_hw(QOSGraphObject *obj)
 {
     QE1000E_PCI *d = (QE1000E_PCI *) obj;
-- 
2.45.2

Re: [PATCH 5/5] qtest/e1000e|igb: Fix msix to re-trigger interrupts

[Akihiko Odaki]

On 2024/12/18 16:42, Nicholas Piggin wrote:
> The e1000e and igb tests don't clear the msix pending bit after waiting
> for it, as it is masked so the irq doesn't get sent. Failing to clear
> the pending interrupt means all subsequent waits for that interrupt
> after the first do not actually wait for an interrupt genreated by the
> device.
> 
> This affects the multiple_transfers tests, they never actually verify
> more than one interrupt is generated. So for those tests, enable the
> msix vectors for the queue interrupts so they are delivered and the
> pending bit is cleared.
> 
> Add assertions to ensure the masked pending tests are not used to test
> an interrupt multiple times.
> 
> 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/e1000e.h |   8 +++
>   tests/qtest/e1000e-test.c   |   2 +
>   tests/qtest/igb-test.c      |   2 +
>   tests/qtest/libqos/e1000e.c | 113 +++++++++++++++++++++++++++++++++++-
>   4 files changed, 122 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/qtest/libqos/e1000e.h b/tests/qtest/libqos/e1000e.h
> index 30643c80949..6cc1a9732b1 100644
> --- a/tests/qtest/libqos/e1000e.h
> +++ b/tests/qtest/libqos/e1000e.h
> @@ -25,6 +25,9 @@
>   #define E1000E_RX0_MSG_ID           (0)
>   #define E1000E_TX0_MSG_ID           (1)
>   
> +#define E1000E_RX0_MSIX_DATA        (0x12345678)
> +#define E1000E_TX0_MSIX_DATA        (0xabcdef00)
> +
>   #define E1000E_ADDRESS { 0x52, 0x54, 0x00, 0x12, 0x34, 0x56 }
>   
>   typedef struct QE1000E QE1000E;
> @@ -40,6 +43,10 @@ struct QE1000E_PCI {
>       QPCIDevice pci_dev;
>       QPCIBar mac_regs;
>       QE1000E e1000e;
> +    uint64_t msix_rx0_addr;
> +    uint64_t msix_tx0_addr;
 > +    bool msix_found_rx0_pending;> +    bool msix_found_tx0_pending;

I prefer having an enum that contains E1000E_RX0_MSG_ID, 
E1000E_TX0_MSG_ID, and "E1000E_RX0_MSG_MAX" or something similar. These 
values can be used to create and index an array containing both rx and 
tx, which will save redundant comparisons with E1000E_RX0_MSG_ID and 
E1000E_RX0_MSG_ID.

>   };
>   
>   static inline void e1000e_macreg_write(QE1000E *d, uint32_t reg, uint32_t val)
> @@ -57,5 +64,6 @@ static inline uint32_t e1000e_macreg_read(QE1000E *d, uint32_t reg)
>   void e1000e_wait_isr(QE1000E *d, uint16_t msg_id);
>   void e1000e_tx_ring_push(QE1000E *d, void *descr);
>   void e1000e_rx_ring_push(QE1000E *d, void *descr);
> +void e1000e_pci_msix_enable_rxtxq_vectors(QE1000E *d, QGuestAllocator *alloc);
>   
>   #endif
> diff --git a/tests/qtest/e1000e-test.c b/tests/qtest/e1000e-test.c
> index a69759da70e..4921a141ffe 100644
> --- a/tests/qtest/e1000e-test.c
> +++ b/tests/qtest/e1000e-test.c
> @@ -185,6 +185,8 @@ static void test_e1000e_multiple_transfers(void *obj, void *data,
>           return;
>       }
>   
> +    /* Triggering msix interrupts multiple times so must enable vectors */
> +    e1000e_pci_msix_enable_rxtxq_vectors(d, alloc);
>       for (i = 0; i < iterations; i++) {
>           e1000e_send_verify(d, data, alloc);
>           e1000e_receive_verify(d, data, alloc);
> diff --git a/tests/qtest/igb-test.c b/tests/qtest/igb-test.c
> index 2f22c4fb208..06082cbe7ff 100644
> --- a/tests/qtest/igb-test.c
> +++ b/tests/qtest/igb-test.c
> @@ -188,6 +188,8 @@ static void test_igb_multiple_transfers(void *obj, void *data,
>           return;
>       }
>   
> +    /* Triggering msix interrupts multiple times so must enable vectors */
> +    e1000e_pci_msix_enable_rxtxq_vectors(d, alloc);
>       for (i = 0; i < iterations; i++) {
>           igb_send_verify(d, data, alloc);
>           igb_receive_verify(d, data, alloc);
> diff --git a/tests/qtest/libqos/e1000e.c b/tests/qtest/libqos/e1000e.c
> index 925654c7fd4..7b7e811bce7 100644
> --- a/tests/qtest/libqos/e1000e.c
> +++ b/tests/qtest/libqos/e1000e.c
> @@ -19,6 +19,7 @@
>   #include "qemu/osdep.h"
>   #include "hw/net/e1000_regs.h"
>   #include "hw/pci/pci_ids.h"
> +#include "hw/pci/pci_regs.h"
>   #include "../libqtest.h"
>   #include "pci-pc.h"
>   #include "qemu/sockets.h"
> @@ -77,16 +78,77 @@ static void e1000e_foreach_callback(QPCIDevice *dev, int devfn, void *data)
>       g_free(dev);
>   }
>   
> +static bool e1000e_test_msix_irq(QE1000E *d, uint16_t msg_id,
> +                                 uint64_t guest_msix_addr,
> +                                 uint32_t msix_data)
> +{
> +    QE1000E_PCI *d_pci = container_of(d, QE1000E_PCI, e1000e);
> +    QPCIDevice *pci_dev = &d_pci->pci_dev;
> +
> +    if (msg_id == E1000E_RX0_MSG_ID) {
> +        g_assert(!d_pci->msix_found_rx0_pending);
> +    } else if (msg_id == E1000E_TX0_MSG_ID) {
> +        g_assert(!d_pci->msix_found_tx0_pending);
> +    } else {
> +        /* Must enable MSI-X vector to test multiple messages */
> +        g_assert_not_reached();
> +    }

This assertions are somewhat tricky. If there is something that sets the 
Pending Bit and we are not aware of it, d_pci->msix_found_rx0_pending 
and d_pci->msix_found_tx0_pending will be left cleared and assertions 
will not fire.

I think asserting that the message is not masked is easier and less 
error-prone.

> +
> +    if (pci_dev->msix_enabled) {
> +        if (qpci_msix_masked(pci_dev, msg_id)) {
> +            /* No ISR checking should be done if masked, but read anyway */
> +            bool p = qpci_msix_pending(pci_dev, msg_id);
> +            if (p) {
> +                if (msg_id == E1000E_RX0_MSG_ID) {
> +                    d_pci->msix_found_rx0_pending = true;
> +                } else if (msg_id == E1000E_TX0_MSG_ID) {
> +                    d_pci->msix_found_tx0_pending = true;
> +                } else {
> +                    g_assert_not_reached();
> +                }
> +            }
> +            return p;
> +        } else {
> +            uint32_t data = qtest_readl(pci_dev->bus->qts, guest_msix_addr);
> +            if (data == msix_data) {
> +                qtest_writel(pci_dev->bus->qts, guest_msix_addr, 0);
> +                return true;
> +            } else if (data == 0) {
> +                return false;
> +            } else {
> +                g_assert_not_reached();
> +            }
> +        }
> +    } else {
> +        g_assert_not_reached();
> +    }
> +}
> +
>   void e1000e_wait_isr(QE1000E *d, uint16_t msg_id)
>   {
>       QE1000E_PCI *d_pci = container_of(d, QE1000E_PCI, e1000e);
> -    guint64 end_time = g_get_monotonic_time() + 5 * G_TIME_SPAN_SECOND;
> +    QPCIDevice *pci_dev = &d_pci->pci_dev;
> +    uint64_t end_time = g_get_monotonic_time() + 5 * G_TIME_SPAN_SECOND;
> +    uint64_t guest_msix_addr;
> +    uint32_t msix_data;
> +
> +    assert(pci_dev->msix_enabled);
> +
> +    if (msg_id == E1000E_RX0_MSG_ID) {
> +        guest_msix_addr = d_pci->msix_rx0_addr;
> +        msix_data = E1000E_RX0_MSIX_DATA;
> +    } else if (msg_id == E1000E_TX0_MSG_ID) {
> +        guest_msix_addr = d_pci->msix_tx0_addr;
> +        msix_data = E1000E_TX0_MSIX_DATA;
> +    } else {
> +        g_assert_not_reached();
> +    }
>   
>       do {
> -        if (qpci_msix_pending(&d_pci->pci_dev, msg_id)) {
> +        if (e1000e_test_msix_irq(d, msg_id, guest_msix_addr, msix_data)) {
>               return;
>           }
> -        qtest_clock_step(d_pci->pci_dev.bus->qts, 10000);
> +        qtest_clock_step(pci_dev->bus->qts, 10000);
>       } while (g_get_monotonic_time() < end_time);
>   
>       g_error("Timeout expired");
> @@ -99,6 +161,51 @@ static void e1000e_pci_destructor(QOSGraphObject *obj)
>       qpci_msix_disable(&epci->pci_dev);
>   }
>   
> +static void e1000e_pci_msix_enable_vector(QE1000E *d, uint16_t msg_id,
> +                                          uint64_t guest_msix_addr,
> +                                          uint32_t msix_data)
> +{
> +    QE1000E_PCI *d_pci = container_of(d, QE1000E_PCI, e1000e);
> +    QPCIDevice *pci_dev = &d_pci->pci_dev;
> +    uint32_t control;
> +    uint64_t off;
> +
> +    g_assert_cmpint(msg_id , >=, 0);
> +    g_assert_cmpint(msg_id , <, qpci_msix_table_size(pci_dev));
> +
> +    off = pci_dev->msix_table_off + (msg_id * 16);
> +
> +    qpci_io_writel(pci_dev, pci_dev->msix_table_bar,
> +                   off + PCI_MSIX_ENTRY_LOWER_ADDR, guest_msix_addr & ~0UL);
> +    qpci_io_writel(pci_dev, pci_dev->msix_table_bar,
> +                   off + PCI_MSIX_ENTRY_UPPER_ADDR,
> +                   (guest_msix_addr >> 32) & ~0UL);
> +    qpci_io_writel(pci_dev, pci_dev->msix_table_bar,
> +                   off + PCI_MSIX_ENTRY_DATA, msix_data);
> +
> +    control = qpci_io_readl(pci_dev, pci_dev->msix_table_bar,
> +                            off + PCI_MSIX_ENTRY_VECTOR_CTRL);
> +    qpci_io_writel(pci_dev, pci_dev->msix_table_bar,
> +                   off + PCI_MSIX_ENTRY_VECTOR_CTRL,
> +                   control & ~PCI_MSIX_ENTRY_CTRL_MASKBIT);
> +}
> +
> +void e1000e_pci_msix_enable_rxtxq_vectors(QE1000E *d, QGuestAllocator *alloc)
> +{
> +    QE1000E_PCI *d_pci = container_of(d, QE1000E_PCI, e1000e);
> +    QPCIDevice *pci_dev = &d_pci->pci_dev;
> +
> +    g_assert(pci_dev->msix_enabled);
> +
> +    d_pci->msix_rx0_addr = guest_alloc(alloc, 4);
> +    d_pci->msix_tx0_addr = guest_alloc(alloc, 4);
> +
> +    e1000e_pci_msix_enable_vector(d, E1000E_RX0_MSG_ID,
> +                                  d_pci->msix_rx0_addr, E1000E_RX0_MSIX_DATA);
> +    e1000e_pci_msix_enable_vector(d, E1000E_TX0_MSG_ID,
> +                                  d_pci->msix_tx0_addr, E1000E_TX0_MSIX_DATA);
> +}
> +
>   static void e1000e_pci_start_hw(QOSGraphObject *obj)
>   {
>       QE1000E_PCI *d = (QE1000E_PCI *) obj;

Re: [PATCH 4/5] qtest/e1000e|igb: Clear interrupt-cause bits after irq

[Akihiko Odaki]

On 2024/12/18 16:42, Nicholas Piggin wrote:
> The e1000e and igb tests do not clear the ICR/EICR cause bits (or
> set auto-clear) on seeing queue interrupts, which inhibits the
> triggering of a new interrupt.
> 
> Fix this by clearing the cause bits, and verify that the expected
> cause bit was set.
> 
> 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/e1000e-test.c | 8 ++++++--
>   tests/qtest/igb-test.c    | 8 ++++++--
>   2 files changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/qtest/e1000e-test.c b/tests/qtest/e1000e-test.c
> index de9738fdb74..a69759da70e 100644
> --- a/tests/qtest/e1000e-test.c
> +++ b/tests/qtest/e1000e-test.c
> @@ -64,8 +64,10 @@ static void e1000e_send_verify(QE1000E *d, int *test_sockets, QGuestAllocator *a
>       /* Put descriptor to the ring */
>       e1000e_tx_ring_push(d, &descr);
>   
> -    /* Wait for TX WB interrupt */
> +    /* Wait for TX WB interrupt (this clears the MSIX PBA) */

It doesn't clear the MSI-X PBA unless the next patch is applied. This 
comment change should be moved to that patch.

>       e1000e_wait_isr(d, E1000E_TX0_MSG_ID);
> +    /* Read ICR which clears it ready for next interrupt, assert TXQ0 cause */

I suggest the following to make this comment clearer:
Read ICR to make it ready for next interrupt, assert TXQ0 cause

> +    g_assert(e1000e_macreg_read(d, E1000_ICR) & E1000_ICR_TXQ0);
>   
>       /* Check DD bit */
>       g_assert_cmphex(le32_to_cpu(descr.upper.data) & E1000_TXD_STAT_DD, ==,
> @@ -115,8 +117,10 @@ static void e1000e_receive_verify(QE1000E *d, int *test_sockets, QGuestAllocator
>       /* Put descriptor to the ring */
>       e1000e_rx_ring_push(d, &descr);
>   
> -    /* Wait for TX WB interrupt */
> +    /* Wait for TX WB interrupt (this clears the MSIX PBA) */
>       e1000e_wait_isr(d, E1000E_RX0_MSG_ID);
> +    /* Read ICR which clears it ready for next interrupt, assert RXQ0 cause */
> +    g_assert(e1000e_macreg_read(d, E1000_ICR) & E1000_ICR_RXQ0);
>   
>       /* Check DD bit */
>       g_assert_cmphex(le32_to_cpu(descr.wb.upper.status_error) &
> diff --git a/tests/qtest/igb-test.c b/tests/qtest/igb-test.c
> index 3d397ea6973..2f22c4fb208 100644
> --- a/tests/qtest/igb-test.c
> +++ b/tests/qtest/igb-test.c
> @@ -67,8 +67,10 @@ static void igb_send_verify(QE1000E *d, int *test_sockets, QGuestAllocator *allo
>       /* Put descriptor to the ring */
>       e1000e_tx_ring_push(d, &descr);
>   
> -    /* Wait for TX WB interrupt */
> +    /* Wait for TX WB interrupt (this clears the MSIX PBA) */
>       e1000e_wait_isr(d, E1000E_TX0_MSG_ID);
> +    /* Read EICR which clears it ready for next interrupt, assert TXQ0 cause */
> +    g_assert(e1000e_macreg_read(d, E1000_EICR) & (1 << E1000E_TX0_MSG_ID));
>   
>       /* Check DD bit */
>       g_assert_cmphex(le32_to_cpu(descr.wb.status) & E1000_TXD_STAT_DD, ==,
> @@ -118,8 +120,10 @@ static void igb_receive_verify(QE1000E *d, int *test_sockets, QGuestAllocator *a
>       /* Put descriptor to the ring */
>       e1000e_rx_ring_push(d, &descr);
>   
> -    /* Wait for TX WB interrupt */
> +    /* Wait for TX WB interrupt (this clears the MSIX PBA) */
>       e1000e_wait_isr(d, E1000E_RX0_MSG_ID);
> +    /* Read EICR which clears it ready for next interrupt, assert RXQ0 cause */
> +    g_assert(e1000e_macreg_read(d, E1000_EICR) & (1 << E1000E_RX0_MSG_ID));
>   
>       /* Check DD bit */
>       g_assert_cmphex(le32_to_cpu(descr.wb.upper.status_error) &

Re: [PATCH 1/5] qtest/pci: Enforce balanced iomap/unmap

[Akihiko Odaki]

On 2024/12/18 16:42, Nicholas Piggin wrote:
> Add assertions to ensure a BAR is not mapped twice, and only
> previously mapped BARs are unmapped. This can help catch some
> bugs.
> 
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> Reviewed-by: Fabiano Rosas <farosas@suse.de>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>   tests/qtest/libqos/ahci.h       |  1 +
>   tests/qtest/libqos/pci.h        |  2 ++
>   tests/qtest/libqos/virtio-pci.h |  1 +
>   tests/qtest/ahci-test.c         |  2 ++
>   tests/qtest/libqos/ahci.c       |  6 ++++++
>   tests/qtest/libqos/pci.c        | 32 +++++++++++++++++++++++++++++++-
>   tests/qtest/libqos/virtio-pci.c |  6 +++++-
>   7 files changed, 48 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/qtest/libqos/ahci.h b/tests/qtest/libqos/ahci.h
> index a0487a1557d..5d7e26aee2a 100644
> --- a/tests/qtest/libqos/ahci.h
> +++ b/tests/qtest/libqos/ahci.h
> @@ -575,6 +575,7 @@ QPCIDevice *get_ahci_device(QTestState *qts, uint32_t *fingerprint);
>   void free_ahci_device(QPCIDevice *dev);
>   void ahci_pci_enable(AHCIQState *ahci);
>   void start_ahci_device(AHCIQState *ahci);
> +void stop_ahci_device(AHCIQState *ahci);
>   void ahci_hba_enable(AHCIQState *ahci);
>   
>   /* Port Management */
> diff --git a/tests/qtest/libqos/pci.h b/tests/qtest/libqos/pci.h
> index 83896145235..9dc82ea723a 100644
> --- a/tests/qtest/libqos/pci.h
> +++ b/tests/qtest/libqos/pci.h
> @@ -65,6 +65,8 @@ struct QPCIDevice
>   {
>       QPCIBus *bus;
>       int devfn;
> +    bool bars_mapped[6];
> +    QPCIBar bars[6];
>       bool msix_enabled;
>       QPCIBar msix_table_bar, msix_pba_bar;
>       uint64_t msix_table_off, msix_pba_off;
> diff --git a/tests/qtest/libqos/virtio-pci.h b/tests/qtest/libqos/virtio-pci.h
> index f5115cacba2..efdf904b254 100644
> --- a/tests/qtest/libqos/virtio-pci.h
> +++ b/tests/qtest/libqos/virtio-pci.h
> @@ -26,6 +26,7 @@ typedef struct QVirtioPCIDevice {
>       uint64_t config_msix_addr;
>       uint32_t config_msix_data;
>   
> +    bool enabled;
>       int bar_idx;
>   
>       /* VIRTIO 1.0 */
> diff --git a/tests/qtest/ahci-test.c b/tests/qtest/ahci-test.c
> index 5a1923f721b..b3dae7a8ce4 100644
> --- a/tests/qtest/ahci-test.c
> +++ b/tests/qtest/ahci-test.c
> @@ -1483,6 +1483,8 @@ static void test_reset_pending_callback(void)
>       /* Wait for throttled write to finish. */
>       sleep(1);
>   
> +    stop_ahci_device(ahci);
> +
>       /* Start again. */
>       ahci_clean_mem(ahci);
>       ahci_pci_enable(ahci);
> diff --git a/tests/qtest/libqos/ahci.c b/tests/qtest/libqos/ahci.c
> index 34a75b7f43b..cfc435b6663 100644
> --- a/tests/qtest/libqos/ahci.c
> +++ b/tests/qtest/libqos/ahci.c
> @@ -217,6 +217,12 @@ void start_ahci_device(AHCIQState *ahci)
>       qpci_device_enable(ahci->dev);
>   }
>   
> +void stop_ahci_device(AHCIQState *ahci)
> +{
> +    /* Map AHCI's ABAR (BAR5) */

I think you meant "unmap AHCI's ABAR (BAR5)".

> +    qpci_iounmap(ahci->dev, ahci->hba_bar);
> +}
> +
>   /**
>    * Test and initialize the AHCI's HBA memory areas.
>    * Initialize and start any ports with devices attached.
> diff --git a/tests/qtest/libqos/pci.c b/tests/qtest/libqos/pci.c
> index b23d72346b6..a42ca08261d 100644
> --- a/tests/qtest/libqos/pci.c
> +++ b/tests/qtest/libqos/pci.c
> @@ -93,12 +93,17 @@ QPCIDevice *qpci_device_find(QPCIBus *bus, int devfn)
>   void qpci_device_init(QPCIDevice *dev, QPCIBus *bus, QPCIAddress *addr)
>   {
>       uint16_t vendor_id, device_id;
> +    int i;
>   
>       qpci_device_set(dev, bus, addr->devfn);
>       vendor_id = qpci_config_readw(dev, PCI_VENDOR_ID);
>       device_id = qpci_config_readw(dev, PCI_DEVICE_ID);
>       g_assert(!addr->vendor_id || vendor_id == addr->vendor_id);
>       g_assert(!addr->device_id || device_id == addr->device_id);
> +
> +    for (i = 0; i < 6; i++) {
> +        g_assert(!dev->bars_mapped[i]);
> +    }
>   }
>   
>   static uint8_t qpci_find_resource_reserve_capability(QPCIDevice *dev)
> @@ -531,6 +536,8 @@ QPCIBar qpci_iomap(QPCIDevice *dev, int barno, uint64_t *sizeptr)
>       uint64_t loc;
>   
>       g_assert(barno >= 0 && barno <= 5);
> +    g_assert(!dev->bars_mapped[barno]);
> +
>       bar_reg = bar_reg_map[barno];
>   
>       qpci_config_writel(dev, bar_reg, 0xFFFFFFFF);
> @@ -574,12 +581,35 @@ QPCIBar qpci_iomap(QPCIDevice *dev, int barno, uint64_t *sizeptr)
>       }
>   
>       bar.addr = loc;
> +
> +    dev->bars_mapped[barno] = true;
> +    dev->bars[barno] = bar;
> +
>       return bar;
>   }
>   
>   void qpci_iounmap(QPCIDevice *dev, QPCIBar bar)
>   {
> -    /* FIXME */
> +    static const int bar_reg_map[] = {
> +        PCI_BASE_ADDRESS_0, PCI_BASE_ADDRESS_1, PCI_BASE_ADDRESS_2,
> +        PCI_BASE_ADDRESS_3, PCI_BASE_ADDRESS_4, PCI_BASE_ADDRESS_5,
> +    };
> +    int bar_reg;
> +    int i;
> +
> +    for (i = 0; i < 6; i++) {
> +        if (!dev->bars_mapped[i]) {
> +            continue;
> +        }
> +        if (dev->bars[i].addr == bar.addr) {
> +            dev->bars_mapped[i] = false;
> +            bar_reg = bar_reg_map[i];
> +            qpci_config_writel(dev, bar_reg, 0xFFFFFFFF);
> +            /* FIXME: the address space is leaked */
> +            return;
> +        }
> +    }
> +    g_assert_not_reached();
>   }
>   
>   QPCIBar qpci_legacy_iomap(QPCIDevice *dev, uint16_t addr)
> diff --git a/tests/qtest/libqos/virtio-pci.c b/tests/qtest/libqos/virtio-pci.c
> index 485b8f6b7e0..2b59fb181c9 100644
> --- a/tests/qtest/libqos/virtio-pci.c
> +++ b/tests/qtest/libqos/virtio-pci.c
> @@ -304,11 +304,15 @@ void qvirtio_pci_device_enable(QVirtioPCIDevice *d)
>   {
>       qpci_device_enable(d->pdev);
>       d->bar = qpci_iomap(d->pdev, d->bar_idx, NULL);
> +    d->enabled = true;
>   }
>   
>   void qvirtio_pci_device_disable(QVirtioPCIDevice *d)
>   {
> -    qpci_iounmap(d->pdev, d->bar);
> +    if (d->enabled) {
> +        qpci_iounmap(d->pdev, d->bar);
> +        d->enabled = false;
> +    }
>   }
>   
>   void qvirtqueue_pci_msix_setup(QVirtioPCIDevice *d, QVirtQueuePCI *vqpci,

Re: [PATCH 5/5] qtest/e1000e|igb: Fix msix to re-trigger interrupts

[Nicholas Piggin]

On Thu Dec 19, 2024 at 6:53 PM AEST, Akihiko Odaki wrote:
> On 2024/12/18 16:42, Nicholas Piggin wrote:
> > The e1000e and igb tests don't clear the msix pending bit after waiting
> > for it, as it is masked so the irq doesn't get sent. Failing to clear
> > the pending interrupt means all subsequent waits for that interrupt
> > after the first do not actually wait for an interrupt genreated by the
> > device.
> > 
> > This affects the multiple_transfers tests, they never actually verify
> > more than one interrupt is generated. So for those tests, enable the
> > msix vectors for the queue interrupts so they are delivered and the
> > pending bit is cleared.
> > 
> > Add assertions to ensure the masked pending tests are not used to test
> > an interrupt multiple times.
> > 
> > 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/e1000e.h |   8 +++
> >   tests/qtest/e1000e-test.c   |   2 +
> >   tests/qtest/igb-test.c      |   2 +
> >   tests/qtest/libqos/e1000e.c | 113 +++++++++++++++++++++++++++++++++++-
> >   4 files changed, 122 insertions(+), 3 deletions(-)
> > 
> > diff --git a/tests/qtest/libqos/e1000e.h b/tests/qtest/libqos/e1000e.h
> > index 30643c80949..6cc1a9732b1 100644
> > --- a/tests/qtest/libqos/e1000e.h
> > +++ b/tests/qtest/libqos/e1000e.h
> > @@ -25,6 +25,9 @@
> >   #define E1000E_RX0_MSG_ID           (0)
> >   #define E1000E_TX0_MSG_ID           (1)
> >   
> > +#define E1000E_RX0_MSIX_DATA        (0x12345678)
> > +#define E1000E_TX0_MSIX_DATA        (0xabcdef00)
> > +
> >   #define E1000E_ADDRESS { 0x52, 0x54, 0x00, 0x12, 0x34, 0x56 }
> >   
> >   typedef struct QE1000E QE1000E;
> > @@ -40,6 +43,10 @@ struct QE1000E_PCI {
> >       QPCIDevice pci_dev;
> >       QPCIBar mac_regs;
> >       QE1000E e1000e;
> > +    uint64_t msix_rx0_addr;
> > +    uint64_t msix_tx0_addr;
>  > +    bool msix_found_rx0_pending;> +    bool msix_found_tx0_pending;
>
> I prefer having an enum that contains E1000E_RX0_MSG_ID, 
> E1000E_TX0_MSG_ID, and "E1000E_RX0_MSG_MAX" or something similar. These 
> values can be used to create and index an array containing both rx and 
> tx, which will save redundant comparisons with E1000E_RX0_MSG_ID and 
> E1000E_RX0_MSG_ID.

Okay I'll see how that looks.

>
> >   };
> >   
> >   static inline void e1000e_macreg_write(QE1000E *d, uint32_t reg, uint32_t val)
> > @@ -57,5 +64,6 @@ static inline uint32_t e1000e_macreg_read(QE1000E *d, uint32_t reg)
> >   void e1000e_wait_isr(QE1000E *d, uint16_t msg_id);
> >   void e1000e_tx_ring_push(QE1000E *d, void *descr);
> >   void e1000e_rx_ring_push(QE1000E *d, void *descr);
> > +void e1000e_pci_msix_enable_rxtxq_vectors(QE1000E *d, QGuestAllocator *alloc);
> >   
> >   #endif
> > diff --git a/tests/qtest/e1000e-test.c b/tests/qtest/e1000e-test.c
> > index a69759da70e..4921a141ffe 100644
> > --- a/tests/qtest/e1000e-test.c
> > +++ b/tests/qtest/e1000e-test.c
> > @@ -185,6 +185,8 @@ static void test_e1000e_multiple_transfers(void *obj, void *data,
> >           return;
> >       }
> >   
> > +    /* Triggering msix interrupts multiple times so must enable vectors */
> > +    e1000e_pci_msix_enable_rxtxq_vectors(d, alloc);
> >       for (i = 0; i < iterations; i++) {
> >           e1000e_send_verify(d, data, alloc);
> >           e1000e_receive_verify(d, data, alloc);
> > diff --git a/tests/qtest/igb-test.c b/tests/qtest/igb-test.c
> > index 2f22c4fb208..06082cbe7ff 100644
> > --- a/tests/qtest/igb-test.c
> > +++ b/tests/qtest/igb-test.c
> > @@ -188,6 +188,8 @@ static void test_igb_multiple_transfers(void *obj, void *data,
> >           return;
> >       }
> >   
> > +    /* Triggering msix interrupts multiple times so must enable vectors */
> > +    e1000e_pci_msix_enable_rxtxq_vectors(d, alloc);
> >       for (i = 0; i < iterations; i++) {
> >           igb_send_verify(d, data, alloc);
> >           igb_receive_verify(d, data, alloc);
> > diff --git a/tests/qtest/libqos/e1000e.c b/tests/qtest/libqos/e1000e.c
> > index 925654c7fd4..7b7e811bce7 100644
> > --- a/tests/qtest/libqos/e1000e.c
> > +++ b/tests/qtest/libqos/e1000e.c
> > @@ -19,6 +19,7 @@
> >   #include "qemu/osdep.h"
> >   #include "hw/net/e1000_regs.h"
> >   #include "hw/pci/pci_ids.h"
> > +#include "hw/pci/pci_regs.h"
> >   #include "../libqtest.h"
> >   #include "pci-pc.h"
> >   #include "qemu/sockets.h"
> > @@ -77,16 +78,77 @@ static void e1000e_foreach_callback(QPCIDevice *dev, int devfn, void *data)
> >       g_free(dev);
> >   }
> >   
> > +static bool e1000e_test_msix_irq(QE1000E *d, uint16_t msg_id,
> > +                                 uint64_t guest_msix_addr,
> > +                                 uint32_t msix_data)
> > +{
> > +    QE1000E_PCI *d_pci = container_of(d, QE1000E_PCI, e1000e);
> > +    QPCIDevice *pci_dev = &d_pci->pci_dev;
> > +
> > +    if (msg_id == E1000E_RX0_MSG_ID) {
> > +        g_assert(!d_pci->msix_found_rx0_pending);
> > +    } else if (msg_id == E1000E_TX0_MSG_ID) {
> > +        g_assert(!d_pci->msix_found_tx0_pending);
> > +    } else {
> > +        /* Must enable MSI-X vector to test multiple messages */
> > +        g_assert_not_reached();
> > +    }
>
> This assertions are somewhat tricky. If there is something that sets the 
> Pending Bit and we are not aware of it, d_pci->msix_found_rx0_pending 
> and d_pci->msix_found_tx0_pending will be left cleared and assertions 
> will not fire.
>
> I think asserting that the message is not masked is easier and less 
> error-prone.

I don't understand what you mean. I allow the masked case
to be used, but only for 1 irq. It is only the multiple case
where we unmask.

If you do not expect the irq to be raised, then you should
add an assert for !e1000e_test_msix_irq().

Thanks,
Nick

Re: [PATCH 4/5] qtest/e1000e|igb: Clear interrupt-cause bits after irq

[Nicholas Piggin]

On Thu Dec 19, 2024 at 7:00 PM AEST, Akihiko Odaki wrote:
> On 2024/12/18 16:42, Nicholas Piggin wrote:
> > The e1000e and igb tests do not clear the ICR/EICR cause bits (or
> > set auto-clear) on seeing queue interrupts, which inhibits the
> > triggering of a new interrupt.
> > 
> > Fix this by clearing the cause bits, and verify that the expected
> > cause bit was set.
> > 
> > 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/e1000e-test.c | 8 ++++++--
> >   tests/qtest/igb-test.c    | 8 ++++++--
> >   2 files changed, 12 insertions(+), 4 deletions(-)
> > 
> > diff --git a/tests/qtest/e1000e-test.c b/tests/qtest/e1000e-test.c
> > index de9738fdb74..a69759da70e 100644
> > --- a/tests/qtest/e1000e-test.c
> > +++ b/tests/qtest/e1000e-test.c
> > @@ -64,8 +64,10 @@ static void e1000e_send_verify(QE1000E *d, int *test_sockets, QGuestAllocator *a
> >       /* Put descriptor to the ring */
> >       e1000e_tx_ring_push(d, &descr);
> >   
> > -    /* Wait for TX WB interrupt */
> > +    /* Wait for TX WB interrupt (this clears the MSIX PBA) */
>
> It doesn't clear the MSI-X PBA unless the next patch is applied. This 
> comment change should be moved to that patch.

Good catch. That was leftover from my improper PBA write patch.

> >       e1000e_wait_isr(d, E1000E_TX0_MSG_ID);
> > +    /* Read ICR which clears it ready for next interrupt, assert TXQ0 cause */
>
> I suggest the following to make this comment clearer:
> Read ICR to make it ready for next interrupt, assert TXQ0 cause

Sure.

Thanks,
Nick

Re: [PATCH 5/5] qtest/e1000e|igb: Fix msix to re-trigger interrupts

[Akihiko Odaki]

On 2024/12/21 13:14, Nicholas Piggin wrote:
> On Thu Dec 19, 2024 at 6:53 PM AEST, Akihiko Odaki wrote:
>> On 2024/12/18 16:42, Nicholas Piggin wrote:
>>> The e1000e and igb tests don't clear the msix pending bit after waiting
>>> for it, as it is masked so the irq doesn't get sent. Failing to clear
>>> the pending interrupt means all subsequent waits for that interrupt
>>> after the first do not actually wait for an interrupt genreated by the
>>> device.
>>>
>>> This affects the multiple_transfers tests, they never actually verify
>>> more than one interrupt is generated. So for those tests, enable the
>>> msix vectors for the queue interrupts so they are delivered and the
>>> pending bit is cleared.
>>>
>>> Add assertions to ensure the masked pending tests are not used to test
>>> an interrupt multiple times.
>>>
>>> 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/e1000e.h |   8 +++
>>>    tests/qtest/e1000e-test.c   |   2 +
>>>    tests/qtest/igb-test.c      |   2 +
>>>    tests/qtest/libqos/e1000e.c | 113 +++++++++++++++++++++++++++++++++++-
>>>    4 files changed, 122 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/tests/qtest/libqos/e1000e.h b/tests/qtest/libqos/e1000e.h
>>> index 30643c80949..6cc1a9732b1 100644
>>> --- a/tests/qtest/libqos/e1000e.h
>>> +++ b/tests/qtest/libqos/e1000e.h
>>> @@ -25,6 +25,9 @@
>>>    #define E1000E_RX0_MSG_ID           (0)
>>>    #define E1000E_TX0_MSG_ID           (1)
>>>    
>>> +#define E1000E_RX0_MSIX_DATA        (0x12345678)
>>> +#define E1000E_TX0_MSIX_DATA        (0xabcdef00)
>>> +
>>>    #define E1000E_ADDRESS { 0x52, 0x54, 0x00, 0x12, 0x34, 0x56 }
>>>    
>>>    typedef struct QE1000E QE1000E;
>>> @@ -40,6 +43,10 @@ struct QE1000E_PCI {
>>>        QPCIDevice pci_dev;
>>>        QPCIBar mac_regs;
>>>        QE1000E e1000e;
>>> +    uint64_t msix_rx0_addr;
>>> +    uint64_t msix_tx0_addr;
>>   > +    bool msix_found_rx0_pending;> +    bool msix_found_tx0_pending;
>>
>> I prefer having an enum that contains E1000E_RX0_MSG_ID,
>> E1000E_TX0_MSG_ID, and "E1000E_RX0_MSG_MAX" or something similar. These
>> values can be used to create and index an array containing both rx and
>> tx, which will save redundant comparisons with E1000E_RX0_MSG_ID and
>> E1000E_RX0_MSG_ID.
> 
> Okay I'll see how that looks.
> 
>>
>>>    };
>>>    
>>>    static inline void e1000e_macreg_write(QE1000E *d, uint32_t reg, uint32_t val)
>>> @@ -57,5 +64,6 @@ static inline uint32_t e1000e_macreg_read(QE1000E *d, uint32_t reg)
>>>    void e1000e_wait_isr(QE1000E *d, uint16_t msg_id);
>>>    void e1000e_tx_ring_push(QE1000E *d, void *descr);
>>>    void e1000e_rx_ring_push(QE1000E *d, void *descr);
>>> +void e1000e_pci_msix_enable_rxtxq_vectors(QE1000E *d, QGuestAllocator *alloc);
>>>    
>>>    #endif
>>> diff --git a/tests/qtest/e1000e-test.c b/tests/qtest/e1000e-test.c
>>> index a69759da70e..4921a141ffe 100644
>>> --- a/tests/qtest/e1000e-test.c
>>> +++ b/tests/qtest/e1000e-test.c
>>> @@ -185,6 +185,8 @@ static void test_e1000e_multiple_transfers(void *obj, void *data,
>>>            return;
>>>        }
>>>    
>>> +    /* Triggering msix interrupts multiple times so must enable vectors */
>>> +    e1000e_pci_msix_enable_rxtxq_vectors(d, alloc);
>>>        for (i = 0; i < iterations; i++) {
>>>            e1000e_send_verify(d, data, alloc);
>>>            e1000e_receive_verify(d, data, alloc);
>>> diff --git a/tests/qtest/igb-test.c b/tests/qtest/igb-test.c
>>> index 2f22c4fb208..06082cbe7ff 100644
>>> --- a/tests/qtest/igb-test.c
>>> +++ b/tests/qtest/igb-test.c
>>> @@ -188,6 +188,8 @@ static void test_igb_multiple_transfers(void *obj, void *data,
>>>            return;
>>>        }
>>>    
>>> +    /* Triggering msix interrupts multiple times so must enable vectors */
>>> +    e1000e_pci_msix_enable_rxtxq_vectors(d, alloc);
>>>        for (i = 0; i < iterations; i++) {
>>>            igb_send_verify(d, data, alloc);
>>>            igb_receive_verify(d, data, alloc);
>>> diff --git a/tests/qtest/libqos/e1000e.c b/tests/qtest/libqos/e1000e.c
>>> index 925654c7fd4..7b7e811bce7 100644
>>> --- a/tests/qtest/libqos/e1000e.c
>>> +++ b/tests/qtest/libqos/e1000e.c
>>> @@ -19,6 +19,7 @@
>>>    #include "qemu/osdep.h"
>>>    #include "hw/net/e1000_regs.h"
>>>    #include "hw/pci/pci_ids.h"
>>> +#include "hw/pci/pci_regs.h"
>>>    #include "../libqtest.h"
>>>    #include "pci-pc.h"
>>>    #include "qemu/sockets.h"
>>> @@ -77,16 +78,77 @@ static void e1000e_foreach_callback(QPCIDevice *dev, int devfn, void *data)
>>>        g_free(dev);
>>>    }
>>>    
>>> +static bool e1000e_test_msix_irq(QE1000E *d, uint16_t msg_id,
>>> +                                 uint64_t guest_msix_addr,
>>> +                                 uint32_t msix_data)
>>> +{
>>> +    QE1000E_PCI *d_pci = container_of(d, QE1000E_PCI, e1000e);
>>> +    QPCIDevice *pci_dev = &d_pci->pci_dev;
>>> +
>>> +    if (msg_id == E1000E_RX0_MSG_ID) {
>>> +        g_assert(!d_pci->msix_found_rx0_pending);
>>> +    } else if (msg_id == E1000E_TX0_MSG_ID) {
>>> +        g_assert(!d_pci->msix_found_tx0_pending);
>>> +    } else {
>>> +        /* Must enable MSI-X vector to test multiple messages */
>>> +        g_assert_not_reached();
>>> +    }
>>
>> This assertions are somewhat tricky. If there is something that sets the
>> Pending Bit and we are not aware of it, d_pci->msix_found_rx0_pending
>> and d_pci->msix_found_tx0_pending will be left cleared and assertions
>> will not fire.
>>
>> I think asserting that the message is not masked is easier and less
>> error-prone.
> 
> I don't understand what you mean. I allow the masked case
> to be used, but only for 1 irq. It is only the multiple case
> where we unmask.
> 
> If you do not expect the irq to be raised, then you should
> add an assert for !e1000e_test_msix_irq().

For example, think of the case where E1000E_RX0_MSG_ID is accidentally 
fired due to a bug in the emulation or test code. This interrupt is 
unintentional, so there is no corresponding call of 
e1000e_test_msix_irq(). This interrupt is followed by an operation that 
is intended to fire E1000E_RX0_MSG_ID and this is expected to be 
confirmed with e1000e_test_msix_irq().

In this case, e1000e_test_msix_irq() will not properly ensure the 
presence of the latter interrupt because the Pending Bit is set by the 
earlier one. g_assert(!d_pci->msix_found_rx0_pending) is intended to 
detect the Pending Bit set earlier, but it is ineffective in this case 
because e1000e_test_msix_irq() is not called for the earlier interrupt 
and d_pci->msix_found_rx0_pending is not set. In this sense, this 
assertion is incomplete.

Instead of having such assertions, we can unmask MSI-X vectors when 
testing interrupts. I also expect there will be less amount of code in 
this way because it will save the msix_found_rx0_pending and 
msix_found_tx0_pending flags and corresponding assertions.

Regards,
Akihiko Odaki

Re: [PATCH 5/5] qtest/e1000e|igb: Fix msix to re-trigger interrupts

[Nicholas Piggin]

On Sat Dec 21, 2024 at 2:26 PM AEST, Akihiko Odaki wrote:
> On 2024/12/21 13:14, Nicholas Piggin wrote:
> > On Thu Dec 19, 2024 at 6:53 PM AEST, Akihiko Odaki wrote:
> >> On 2024/12/18 16:42, Nicholas Piggin wrote:
> >>> The e1000e and igb tests don't clear the msix pending bit after waiting
> >>> for it, as it is masked so the irq doesn't get sent. Failing to clear
> >>> the pending interrupt means all subsequent waits for that interrupt
> >>> after the first do not actually wait for an interrupt genreated by the
> >>> device.
> >>>
> >>> This affects the multiple_transfers tests, they never actually verify
> >>> more than one interrupt is generated. So for those tests, enable the
> >>> msix vectors for the queue interrupts so they are delivered and the
> >>> pending bit is cleared.
> >>>
> >>> Add assertions to ensure the masked pending tests are not used to test
> >>> an interrupt multiple times.
> >>>
> >>> 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/e1000e.h |   8 +++
> >>>    tests/qtest/e1000e-test.c   |   2 +
> >>>    tests/qtest/igb-test.c      |   2 +
> >>>    tests/qtest/libqos/e1000e.c | 113 +++++++++++++++++++++++++++++++++++-
> >>>    4 files changed, 122 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/tests/qtest/libqos/e1000e.h b/tests/qtest/libqos/e1000e.h
> >>> index 30643c80949..6cc1a9732b1 100644
> >>> --- a/tests/qtest/libqos/e1000e.h
> >>> +++ b/tests/qtest/libqos/e1000e.h
> >>> @@ -25,6 +25,9 @@
> >>>    #define E1000E_RX0_MSG_ID           (0)
> >>>    #define E1000E_TX0_MSG_ID           (1)
> >>>    
> >>> +#define E1000E_RX0_MSIX_DATA        (0x12345678)
> >>> +#define E1000E_TX0_MSIX_DATA        (0xabcdef00)
> >>> +
> >>>    #define E1000E_ADDRESS { 0x52, 0x54, 0x00, 0x12, 0x34, 0x56 }
> >>>    
> >>>    typedef struct QE1000E QE1000E;
> >>> @@ -40,6 +43,10 @@ struct QE1000E_PCI {
> >>>        QPCIDevice pci_dev;
> >>>        QPCIBar mac_regs;
> >>>        QE1000E e1000e;
> >>> +    uint64_t msix_rx0_addr;
> >>> +    uint64_t msix_tx0_addr;
> >>   > +    bool msix_found_rx0_pending;> +    bool msix_found_tx0_pending;
> >>
> >> I prefer having an enum that contains E1000E_RX0_MSG_ID,
> >> E1000E_TX0_MSG_ID, and "E1000E_RX0_MSG_MAX" or something similar. These
> >> values can be used to create and index an array containing both rx and
> >> tx, which will save redundant comparisons with E1000E_RX0_MSG_ID and
> >> E1000E_RX0_MSG_ID.
> > 
> > Okay I'll see how that looks.
> > 
> >>
> >>>    };
> >>>    
> >>>    static inline void e1000e_macreg_write(QE1000E *d, uint32_t reg, uint32_t val)
> >>> @@ -57,5 +64,6 @@ static inline uint32_t e1000e_macreg_read(QE1000E *d, uint32_t reg)
> >>>    void e1000e_wait_isr(QE1000E *d, uint16_t msg_id);
> >>>    void e1000e_tx_ring_push(QE1000E *d, void *descr);
> >>>    void e1000e_rx_ring_push(QE1000E *d, void *descr);
> >>> +void e1000e_pci_msix_enable_rxtxq_vectors(QE1000E *d, QGuestAllocator *alloc);
> >>>    
> >>>    #endif
> >>> diff --git a/tests/qtest/e1000e-test.c b/tests/qtest/e1000e-test.c
> >>> index a69759da70e..4921a141ffe 100644
> >>> --- a/tests/qtest/e1000e-test.c
> >>> +++ b/tests/qtest/e1000e-test.c
> >>> @@ -185,6 +185,8 @@ static void test_e1000e_multiple_transfers(void *obj, void *data,
> >>>            return;
> >>>        }
> >>>    
> >>> +    /* Triggering msix interrupts multiple times so must enable vectors */
> >>> +    e1000e_pci_msix_enable_rxtxq_vectors(d, alloc);
> >>>        for (i = 0; i < iterations; i++) {
> >>>            e1000e_send_verify(d, data, alloc);
> >>>            e1000e_receive_verify(d, data, alloc);
> >>> diff --git a/tests/qtest/igb-test.c b/tests/qtest/igb-test.c
> >>> index 2f22c4fb208..06082cbe7ff 100644
> >>> --- a/tests/qtest/igb-test.c
> >>> +++ b/tests/qtest/igb-test.c
> >>> @@ -188,6 +188,8 @@ static void test_igb_multiple_transfers(void *obj, void *data,
> >>>            return;
> >>>        }
> >>>    
> >>> +    /* Triggering msix interrupts multiple times so must enable vectors */
> >>> +    e1000e_pci_msix_enable_rxtxq_vectors(d, alloc);
> >>>        for (i = 0; i < iterations; i++) {
> >>>            igb_send_verify(d, data, alloc);
> >>>            igb_receive_verify(d, data, alloc);
> >>> diff --git a/tests/qtest/libqos/e1000e.c b/tests/qtest/libqos/e1000e.c
> >>> index 925654c7fd4..7b7e811bce7 100644
> >>> --- a/tests/qtest/libqos/e1000e.c
> >>> +++ b/tests/qtest/libqos/e1000e.c
> >>> @@ -19,6 +19,7 @@
> >>>    #include "qemu/osdep.h"
> >>>    #include "hw/net/e1000_regs.h"
> >>>    #include "hw/pci/pci_ids.h"
> >>> +#include "hw/pci/pci_regs.h"
> >>>    #include "../libqtest.h"
> >>>    #include "pci-pc.h"
> >>>    #include "qemu/sockets.h"
> >>> @@ -77,16 +78,77 @@ static void e1000e_foreach_callback(QPCIDevice *dev, int devfn, void *data)
> >>>        g_free(dev);
> >>>    }
> >>>    
> >>> +static bool e1000e_test_msix_irq(QE1000E *d, uint16_t msg_id,
> >>> +                                 uint64_t guest_msix_addr,
> >>> +                                 uint32_t msix_data)
> >>> +{
> >>> +    QE1000E_PCI *d_pci = container_of(d, QE1000E_PCI, e1000e);
> >>> +    QPCIDevice *pci_dev = &d_pci->pci_dev;
> >>> +
> >>> +    if (msg_id == E1000E_RX0_MSG_ID) {
> >>> +        g_assert(!d_pci->msix_found_rx0_pending);
> >>> +    } else if (msg_id == E1000E_TX0_MSG_ID) {
> >>> +        g_assert(!d_pci->msix_found_tx0_pending);
> >>> +    } else {
> >>> +        /* Must enable MSI-X vector to test multiple messages */
> >>> +        g_assert_not_reached();
> >>> +    }
> >>
> >> This assertions are somewhat tricky. If there is something that sets the
> >> Pending Bit and we are not aware of it, d_pci->msix_found_rx0_pending
> >> and d_pci->msix_found_tx0_pending will be left cleared and assertions
> >> will not fire.
> >>
> >> I think asserting that the message is not masked is easier and less
> >> error-prone.
> > 
> > I don't understand what you mean. I allow the masked case
> > to be used, but only for 1 irq. It is only the multiple case
> > where we unmask.
> > 
> > If you do not expect the irq to be raised, then you should
> > add an assert for !e1000e_test_msix_irq().
>
> For example, think of the case where E1000E_RX0_MSG_ID is accidentally 
> fired due to a bug in the emulation or test code. This interrupt is 
> unintentional, so there is no corresponding call of 
> e1000e_test_msix_irq(). This interrupt is followed by an operation that 
> is intended to fire E1000E_RX0_MSG_ID and this is expected to be 
> confirmed with e1000e_test_msix_irq().
>
> In this case, e1000e_test_msix_irq() will not properly ensure the 
> presence of the latter interrupt because the Pending Bit is set by the 
> earlier one. g_assert(!d_pci->msix_found_rx0_pending) is intended to 
> detect the Pending Bit set earlier, but it is ineffective in this case 
> because e1000e_test_msix_irq() is not called for the earlier interrupt 
> and d_pci->msix_found_rx0_pending is not set. In this sense, this 
> assertion is incomplete.

I think this scenario can not be detected even if we unmask, because
we can never distinguish the unintended interrupt from the intended
one.

The test case will always have to assert(!e1000e_tests_msix_irq())
before deliberately raising an interrupt if it wanted to be sure there
are no earlier unintended ones.

> Instead of having such assertions, we can unmask MSI-X vectors when 
> testing interrupts. I also expect there will be less amount of code in 
> this way because it will save the msix_found_rx0_pending and 
> msix_found_tx0_pending flags and corresponding assertions.

We could always unmask, yes it would be less code. I just thought it
was neat to be able to test both paths. But that might be better left
to a msix specific test.

Thanks,
Nick

Re: [PATCH 5/5] qtest/e1000e|igb: Fix msix to re-trigger interrupts

[Akihiko Odaki]

On 2024/12/21 17:11, Nicholas Piggin wrote:
> On Sat Dec 21, 2024 at 2:26 PM AEST, Akihiko Odaki wrote:
>> On 2024/12/21 13:14, Nicholas Piggin wrote:
>>> On Thu Dec 19, 2024 at 6:53 PM AEST, Akihiko Odaki wrote:
>>>> On 2024/12/18 16:42, Nicholas Piggin wrote:
>>>>> The e1000e and igb tests don't clear the msix pending bit after waiting
>>>>> for it, as it is masked so the irq doesn't get sent. Failing to clear
>>>>> the pending interrupt means all subsequent waits for that interrupt
>>>>> after the first do not actually wait for an interrupt genreated by the
>>>>> device.
>>>>>
>>>>> This affects the multiple_transfers tests, they never actually verify
>>>>> more than one interrupt is generated. So for those tests, enable the
>>>>> msix vectors for the queue interrupts so they are delivered and the
>>>>> pending bit is cleared.
>>>>>
>>>>> Add assertions to ensure the masked pending tests are not used to test
>>>>> an interrupt multiple times.
>>>>>
>>>>> 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/e1000e.h |   8 +++
>>>>>     tests/qtest/e1000e-test.c   |   2 +
>>>>>     tests/qtest/igb-test.c      |   2 +
>>>>>     tests/qtest/libqos/e1000e.c | 113 +++++++++++++++++++++++++++++++++++-
>>>>>     4 files changed, 122 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/tests/qtest/libqos/e1000e.h b/tests/qtest/libqos/e1000e.h
>>>>> index 30643c80949..6cc1a9732b1 100644
>>>>> --- a/tests/qtest/libqos/e1000e.h
>>>>> +++ b/tests/qtest/libqos/e1000e.h
>>>>> @@ -25,6 +25,9 @@
>>>>>     #define E1000E_RX0_MSG_ID           (0)
>>>>>     #define E1000E_TX0_MSG_ID           (1)
>>>>>     
>>>>> +#define E1000E_RX0_MSIX_DATA        (0x12345678)
>>>>> +#define E1000E_TX0_MSIX_DATA        (0xabcdef00)
>>>>> +
>>>>>     #define E1000E_ADDRESS { 0x52, 0x54, 0x00, 0x12, 0x34, 0x56 }
>>>>>     
>>>>>     typedef struct QE1000E QE1000E;
>>>>> @@ -40,6 +43,10 @@ struct QE1000E_PCI {
>>>>>         QPCIDevice pci_dev;
>>>>>         QPCIBar mac_regs;
>>>>>         QE1000E e1000e;
>>>>> +    uint64_t msix_rx0_addr;
>>>>> +    uint64_t msix_tx0_addr;
>>>>    > +    bool msix_found_rx0_pending;> +    bool msix_found_tx0_pending;
>>>>
>>>> I prefer having an enum that contains E1000E_RX0_MSG_ID,
>>>> E1000E_TX0_MSG_ID, and "E1000E_RX0_MSG_MAX" or something similar. These
>>>> values can be used to create and index an array containing both rx and
>>>> tx, which will save redundant comparisons with E1000E_RX0_MSG_ID and
>>>> E1000E_RX0_MSG_ID.
>>>
>>> Okay I'll see how that looks.
>>>
>>>>
>>>>>     };
>>>>>     
>>>>>     static inline void e1000e_macreg_write(QE1000E *d, uint32_t reg, uint32_t val)
>>>>> @@ -57,5 +64,6 @@ static inline uint32_t e1000e_macreg_read(QE1000E *d, uint32_t reg)
>>>>>     void e1000e_wait_isr(QE1000E *d, uint16_t msg_id);
>>>>>     void e1000e_tx_ring_push(QE1000E *d, void *descr);
>>>>>     void e1000e_rx_ring_push(QE1000E *d, void *descr);
>>>>> +void e1000e_pci_msix_enable_rxtxq_vectors(QE1000E *d, QGuestAllocator *alloc);
>>>>>     
>>>>>     #endif
>>>>> diff --git a/tests/qtest/e1000e-test.c b/tests/qtest/e1000e-test.c
>>>>> index a69759da70e..4921a141ffe 100644
>>>>> --- a/tests/qtest/e1000e-test.c
>>>>> +++ b/tests/qtest/e1000e-test.c
>>>>> @@ -185,6 +185,8 @@ static void test_e1000e_multiple_transfers(void *obj, void *data,
>>>>>             return;
>>>>>         }
>>>>>     
>>>>> +    /* Triggering msix interrupts multiple times so must enable vectors */
>>>>> +    e1000e_pci_msix_enable_rxtxq_vectors(d, alloc);
>>>>>         for (i = 0; i < iterations; i++) {
>>>>>             e1000e_send_verify(d, data, alloc);
>>>>>             e1000e_receive_verify(d, data, alloc);
>>>>> diff --git a/tests/qtest/igb-test.c b/tests/qtest/igb-test.c
>>>>> index 2f22c4fb208..06082cbe7ff 100644
>>>>> --- a/tests/qtest/igb-test.c
>>>>> +++ b/tests/qtest/igb-test.c
>>>>> @@ -188,6 +188,8 @@ static void test_igb_multiple_transfers(void *obj, void *data,
>>>>>             return;
>>>>>         }
>>>>>     
>>>>> +    /* Triggering msix interrupts multiple times so must enable vectors */
>>>>> +    e1000e_pci_msix_enable_rxtxq_vectors(d, alloc);
>>>>>         for (i = 0; i < iterations; i++) {
>>>>>             igb_send_verify(d, data, alloc);
>>>>>             igb_receive_verify(d, data, alloc);
>>>>> diff --git a/tests/qtest/libqos/e1000e.c b/tests/qtest/libqos/e1000e.c
>>>>> index 925654c7fd4..7b7e811bce7 100644
>>>>> --- a/tests/qtest/libqos/e1000e.c
>>>>> +++ b/tests/qtest/libqos/e1000e.c
>>>>> @@ -19,6 +19,7 @@
>>>>>     #include "qemu/osdep.h"
>>>>>     #include "hw/net/e1000_regs.h"
>>>>>     #include "hw/pci/pci_ids.h"
>>>>> +#include "hw/pci/pci_regs.h"
>>>>>     #include "../libqtest.h"
>>>>>     #include "pci-pc.h"
>>>>>     #include "qemu/sockets.h"
>>>>> @@ -77,16 +78,77 @@ static void e1000e_foreach_callback(QPCIDevice *dev, int devfn, void *data)
>>>>>         g_free(dev);
>>>>>     }
>>>>>     
>>>>> +static bool e1000e_test_msix_irq(QE1000E *d, uint16_t msg_id,
>>>>> +                                 uint64_t guest_msix_addr,
>>>>> +                                 uint32_t msix_data)
>>>>> +{
>>>>> +    QE1000E_PCI *d_pci = container_of(d, QE1000E_PCI, e1000e);
>>>>> +    QPCIDevice *pci_dev = &d_pci->pci_dev;
>>>>> +
>>>>> +    if (msg_id == E1000E_RX0_MSG_ID) {
>>>>> +        g_assert(!d_pci->msix_found_rx0_pending);
>>>>> +    } else if (msg_id == E1000E_TX0_MSG_ID) {
>>>>> +        g_assert(!d_pci->msix_found_tx0_pending);
>>>>> +    } else {
>>>>> +        /* Must enable MSI-X vector to test multiple messages */
>>>>> +        g_assert_not_reached();
>>>>> +    }
>>>>
>>>> This assertions are somewhat tricky. If there is something that sets the
>>>> Pending Bit and we are not aware of it, d_pci->msix_found_rx0_pending
>>>> and d_pci->msix_found_tx0_pending will be left cleared and assertions
>>>> will not fire.
>>>>
>>>> I think asserting that the message is not masked is easier and less
>>>> error-prone.
>>>
>>> I don't understand what you mean. I allow the masked case
>>> to be used, but only for 1 irq. It is only the multiple case
>>> where we unmask.
>>>
>>> If you do not expect the irq to be raised, then you should
>>> add an assert for !e1000e_test_msix_irq().
>>
>> For example, think of the case where E1000E_RX0_MSG_ID is accidentally
>> fired due to a bug in the emulation or test code. This interrupt is
>> unintentional, so there is no corresponding call of
>> e1000e_test_msix_irq(). This interrupt is followed by an operation that
>> is intended to fire E1000E_RX0_MSG_ID and this is expected to be
>> confirmed with e1000e_test_msix_irq().
>>
>> In this case, e1000e_test_msix_irq() will not properly ensure the
>> presence of the latter interrupt because the Pending Bit is set by the
>> earlier one. g_assert(!d_pci->msix_found_rx0_pending) is intended to
>> detect the Pending Bit set earlier, but it is ineffective in this case
>> because e1000e_test_msix_irq() is not called for the earlier interrupt
>> and d_pci->msix_found_rx0_pending is not set. In this sense, this
>> assertion is incomplete.
> 
> I think this scenario can not be detected even if we unmask, because
> we can never distinguish the unintended interrupt from the intended
> one.
> 
> The test case will always have to assert(!e1000e_tests_msix_irq())
> before deliberately raising an interrupt if it wanted to be sure there
> are no earlier unintended ones.
> 
>> Instead of having such assertions, we can unmask MSI-X vectors when
>> testing interrupts. I also expect there will be less amount of code in
>> this way because it will save the msix_found_rx0_pending and
>> msix_found_tx0_pending flags and corresponding assertions.
> 
> We could always unmask, yes it would be less code. I just thought it
> was neat to be able to test both paths. But that might be better left
> to a msix specific test.

Indeed, I also think the code testing the masked case should be 
separated from e1000e_test_msix_irq(). Such a test will not need the 
msix_found_tx0_pending and msix_found_tx0_pending flags as it is obvious 
that MSI-X is masked.

Regards,
Akihiko Odaki