[v1] qtest: pci and e1000e/igb msix fixes

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

Posted by Nicholas Piggin 3 months, 3 weeks ago

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

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

Posted by Akihiko Odaki 3 months, 2 weeks ago

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 4/5] qtest/e1000e|igb: Clear interrupt-cause bits after irq

Posted by Nicholas Piggin 3 months, 2 weeks ago

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

[PATCH 1/5] qtest/pci: Enforce balanced iomap/unmap
[PATCH 2/5] qtest/libqos/pci: Fix qpci_msix_enable sharing bar0
[PATCH 3/5] qtest/libqos/pci: Do not write to PBA memory
[PATCH 4/5] qtest/e1000e|igb: Clear interrupt-cause bits after irq
[PATCH 5/5] qtest/e1000e|igb: Fix msix to re-trigger interrupts