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