| Message ID | 20241218074232.1784427-6-npiggin@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | qtest: pci and e1000e/igb msix fixes | expand |
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;
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
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
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
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
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;
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(-)