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 <m...@redhat.com>
> > Cc: Marcel Apfelbaum <marcel.apfelb...@gmail.com>
> > Cc: Dmitry Fleytman <dmitry.fleyt...@gmail.com>
> > Cc: Akihiko Odaki <akihiko.od...@daynix.com>
> > Cc: Sriram Yagnaraman <sriram.yagnara...@ericsson.com>
> > Signed-off-by: Nicholas Piggin <npig...@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
