hw/e1000e|igb: interrupts and qtests fixes

[PATCH 3/9] qtest/e1000e|igb: assert irqs are clear before triggering an irq

Posted by Nicholas Piggin 2 weeks, 4 days ago

Assert there is no existing irq raised that would lead to a false
positive interrupt test.

e1000e has to disable interrupt throttling for this test, because
it can cause delayed superfluous interrupts which trip the assertions.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 tests/qtest/libqos/e1000e.h |  1 +
 tests/qtest/e1000e-test.c   | 10 ++++++++++
 tests/qtest/igb-test.c      |  6 ++++++
 tests/qtest/libqos/e1000e.c |  9 ++++++++-
 4 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/tests/qtest/libqos/e1000e.h b/tests/qtest/libqos/e1000e.h
index 30643c80949..7154aec0339 100644
--- a/tests/qtest/libqos/e1000e.h
+++ b/tests/qtest/libqos/e1000e.h
@@ -54,6 +54,7 @@ static inline uint32_t e1000e_macreg_read(QE1000E *d, uint32_t reg)
     return qpci_io_readl(&d_pci->pci_dev, d_pci->mac_regs, reg);
 }
 
+bool e1000e_seen_isr(QE1000E *d, uint16_t msg_id);
 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);
diff --git a/tests/qtest/e1000e-test.c b/tests/qtest/e1000e-test.c
index 746d26cfb67..9ab81ecff5b 100644
--- a/tests/qtest/e1000e-test.c
+++ b/tests/qtest/e1000e-test.c
@@ -61,6 +61,9 @@ static void e1000e_send_verify(QE1000E *d, int *test_sockets, QGuestAllocator *a
                                    E1000_TXD_DTYP_D   |
                                    sizeof(buffer));
 
+    /* Ensure the interrupt has not been taken already */
+    g_assert(!e1000e_seen_isr(d, E1000E_TX0_MSG_ID));
+
     /* Put descriptor to the ring */
     e1000e_tx_ring_push(d, &descr);
 
@@ -105,6 +108,9 @@ static void e1000e_receive_verify(QE1000E *d, int *test_sockets, QGuestAllocator
     char buffer[64];
     int ret;
 
+    /* Ensure the interrupt has not been taken already */
+    g_assert(!e1000e_seen_isr(d, E1000E_RX0_MSG_ID));
+
     /* Send a dummy packet to device's socket*/
     ret = iov_send(test_sockets[0], iov, 2, 0, sizeof(len) + sizeof(packet));
     g_assert_cmpint(ret, == , sizeof(packet) + sizeof(len));
@@ -188,6 +194,10 @@ static void test_e1000e_multiple_transfers(void *obj, void *data,
         return;
     }
 
+    /* Clear EITR because buggy QEMU throttle timer causes superfluous irqs */
+    e1000e_macreg_write(d, E1000_EITR + E1000E_RX0_MSG_ID * 4, 0);
+    e1000e_macreg_write(d, E1000_EITR + E1000E_TX0_MSG_ID * 4, 0);
+
     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 cf8b4131cf2..1bbb4bea4c1 100644
--- a/tests/qtest/igb-test.c
+++ b/tests/qtest/igb-test.c
@@ -64,6 +64,9 @@ static void igb_send_verify(QE1000E *d, int *test_sockets, QGuestAllocator *allo
                                           E1000_TXD_DTYP_D   |
                                           sizeof(buffer));
 
+    /* Ensure the interrupt has not been taken already */
+    g_assert(!e1000e_seen_isr(d, E1000E_TX0_MSG_ID));
+
     /* Put descriptor to the ring */
     e1000e_tx_ring_push(d, &descr);
 
@@ -119,6 +122,9 @@ static void igb_receive_verify(QE1000E *d, int *test_sockets, QGuestAllocator *a
     memset(&descr, 0, sizeof(descr));
     descr.read.pkt_addr = cpu_to_le64(data);
 
+    /* Ensure the interrupt has not been taken already */
+    g_assert(!e1000e_seen_isr(d, E1000E_RX0_MSG_ID));
+
     /* Put descriptor to the ring */
     e1000e_rx_ring_push(d, &descr);
 
diff --git a/tests/qtest/libqos/e1000e.c b/tests/qtest/libqos/e1000e.c
index 925654c7fd4..4e4c387b0bf 100644
--- a/tests/qtest/libqos/e1000e.c
+++ b/tests/qtest/libqos/e1000e.c
@@ -77,13 +77,20 @@ static void e1000e_foreach_callback(QPCIDevice *dev, int devfn, void *data)
     g_free(dev);
 }
 
+bool e1000e_seen_isr(QE1000E *d, uint16_t msg_id)
+{
+    QE1000E_PCI *d_pci = container_of(d, QE1000E_PCI, e1000e);
+
+    return qpci_msix_pending(&d_pci->pci_dev, msg_id);
+}
+
 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;
 
     do {
-        if (qpci_msix_pending(&d_pci->pci_dev, msg_id)) {
+        if (e1000e_seen_isr(d, msg_id)) {
             return;
         }
         qtest_clock_step(d_pci->pci_dev.bus->qts, 10000);
-- 
2.45.2

Re: [PATCH 3/9] qtest/e1000e|igb: assert irqs are clear before triggering an irq

Posted by Yan Vugenfirer 2 weeks, 3 days ago

On Fri, Jan 17, 2025 at 7:05 PM Nicholas Piggin <npiggin@gmail.com> wrote:

> Assert there is no existing irq raised that would lead to a false
> positive interrupt test.
>
> e1000e has to disable interrupt throttling for this test, because
> it can cause delayed superfluous interrupts which trip the assertions.
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>  tests/qtest/libqos/e1000e.h |  1 +
>  tests/qtest/e1000e-test.c   | 10 ++++++++++
>  tests/qtest/igb-test.c      |  6 ++++++
>  tests/qtest/libqos/e1000e.c |  9 ++++++++-
>  4 files changed, 25 insertions(+), 1 deletion(-)
>
> diff --git a/tests/qtest/libqos/e1000e.h b/tests/qtest/libqos/e1000e.h
> index 30643c80949..7154aec0339 100644
> --- a/tests/qtest/libqos/e1000e.h
> +++ b/tests/qtest/libqos/e1000e.h
> @@ -54,6 +54,7 @@ static inline uint32_t e1000e_macreg_read(QE1000E *d,
> uint32_t reg)
>      return qpci_io_readl(&d_pci->pci_dev, d_pci->mac_regs, reg);
>  }
>
> +bool e1000e_seen_isr(QE1000E *d, uint16_t msg_id);
>  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);
> diff --git a/tests/qtest/e1000e-test.c b/tests/qtest/e1000e-test.c
> index 746d26cfb67..9ab81ecff5b 100644
> --- a/tests/qtest/e1000e-test.c
> +++ b/tests/qtest/e1000e-test.c
> @@ -61,6 +61,9 @@ static void e1000e_send_verify(QE1000E *d, int
> *test_sockets, QGuestAllocator *a
>                                     E1000_TXD_DTYP_D   |
>                                     sizeof(buffer));
>
> +    /* Ensure the interrupt has not been taken already */
> +    g_assert(!e1000e_seen_isr(d, E1000E_TX0_MSG_ID));
> +
>      /* Put descriptor to the ring */
>      e1000e_tx_ring_push(d, &descr);
>
> @@ -105,6 +108,9 @@ static void e1000e_receive_verify(QE1000E *d, int
> *test_sockets, QGuestAllocator
>      char buffer[64];
>      int ret;
>
> +    /* Ensure the interrupt has not been taken already */
> +    g_assert(!e1000e_seen_isr(d, E1000E_RX0_MSG_ID));
> +
>
I don't think potentially crashing the guest is the right solution.
I suggest maybe substituting with logging.


>      /* Send a dummy packet to device's socket*/
>      ret = iov_send(test_sockets[0], iov, 2, 0, sizeof(len) +
> sizeof(packet));
>      g_assert_cmpint(ret, == , sizeof(packet) + sizeof(len));
> @@ -188,6 +194,10 @@ static void test_e1000e_multiple_transfers(void *obj,
> void *data,
>          return;
>      }
>
> +    /* Clear EITR because buggy QEMU throttle timer causes superfluous
> irqs */
> +    e1000e_macreg_write(d, E1000_EITR + E1000E_RX0_MSG_ID * 4, 0);
> +    e1000e_macreg_write(d, E1000_EITR + E1000E_TX0_MSG_ID * 4, 0);
> +
>      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 cf8b4131cf2..1bbb4bea4c1 100644
> --- a/tests/qtest/igb-test.c
> +++ b/tests/qtest/igb-test.c
> @@ -64,6 +64,9 @@ static void igb_send_verify(QE1000E *d, int
> *test_sockets, QGuestAllocator *allo
>                                            E1000_TXD_DTYP_D   |
>                                            sizeof(buffer));
>
> +    /* Ensure the interrupt has not been taken already */
> +    g_assert(!e1000e_seen_isr(d, E1000E_TX0_MSG_ID));
> +
>      /* Put descriptor to the ring */
>      e1000e_tx_ring_push(d, &descr);
>
> @@ -119,6 +122,9 @@ static void igb_receive_verify(QE1000E *d, int
> *test_sockets, QGuestAllocator *a
>      memset(&descr, 0, sizeof(descr));
>      descr.read.pkt_addr = cpu_to_le64(data);
>
> +    /* Ensure the interrupt has not been taken already */
> +    g_assert(!e1000e_seen_isr(d, E1000E_RX0_MSG_ID));
> +
>      /* Put descriptor to the ring */
>      e1000e_rx_ring_push(d, &descr);
>
> diff --git a/tests/qtest/libqos/e1000e.c b/tests/qtest/libqos/e1000e.c
> index 925654c7fd4..4e4c387b0bf 100644
> --- a/tests/qtest/libqos/e1000e.c
> +++ b/tests/qtest/libqos/e1000e.c
> @@ -77,13 +77,20 @@ static void e1000e_foreach_callback(QPCIDevice *dev,
> int devfn, void *data)
>      g_free(dev);
>  }
>
> +bool e1000e_seen_isr(QE1000E *d, uint16_t msg_id)
> +{
> +    QE1000E_PCI *d_pci = container_of(d, QE1000E_PCI, e1000e);
> +
> +    return qpci_msix_pending(&d_pci->pci_dev, msg_id);
> +}
> +
>  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;
>
>      do {
> -        if (qpci_msix_pending(&d_pci->pci_dev, msg_id)) {
> +        if (e1000e_seen_isr(d, msg_id)) {
>              return;
>          }
>          qtest_clock_step(d_pci->pci_dev.bus->qts, 10000);
> --
> 2.45.2
>
>
>

Re: [PATCH 3/9] qtest/e1000e|igb: assert irqs are clear before triggering an irq

Posted by Nicholas Piggin 2 weeks, 1 day ago

On Sun Jan 19, 2025 at 7:22 PM AEST, Yan Vugenfirer wrote:
> On Fri, Jan 17, 2025 at 7:05 PM Nicholas Piggin <npiggin@gmail.com> wrote:
>
>> Assert there is no existing irq raised that would lead to a false
>> positive interrupt test.
>>
>> e1000e has to disable interrupt throttling for this test, because
>> it can cause delayed superfluous interrupts which trip the assertions.
>>
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>>  tests/qtest/libqos/e1000e.h |  1 +
>>  tests/qtest/e1000e-test.c   | 10 ++++++++++
>>  tests/qtest/igb-test.c      |  6 ++++++
>>  tests/qtest/libqos/e1000e.c |  9 ++++++++-
>>  4 files changed, 25 insertions(+), 1 deletion(-)
>>
>> diff --git a/tests/qtest/libqos/e1000e.h b/tests/qtest/libqos/e1000e.h
>> index 30643c80949..7154aec0339 100644
>> --- a/tests/qtest/libqos/e1000e.h
>> +++ b/tests/qtest/libqos/e1000e.h
>> @@ -54,6 +54,7 @@ static inline uint32_t e1000e_macreg_read(QE1000E *d,
>> uint32_t reg)
>>      return qpci_io_readl(&d_pci->pci_dev, d_pci->mac_regs, reg);
>>  }
>>
>> +bool e1000e_seen_isr(QE1000E *d, uint16_t msg_id);
>>  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);
>> diff --git a/tests/qtest/e1000e-test.c b/tests/qtest/e1000e-test.c
>> index 746d26cfb67..9ab81ecff5b 100644
>> --- a/tests/qtest/e1000e-test.c
>> +++ b/tests/qtest/e1000e-test.c
>> @@ -61,6 +61,9 @@ static void e1000e_send_verify(QE1000E *d, int
>> *test_sockets, QGuestAllocator *a
>>                                     E1000_TXD_DTYP_D   |
>>                                     sizeof(buffer));
>>
>> +    /* Ensure the interrupt has not been taken already */
>> +    g_assert(!e1000e_seen_isr(d, E1000E_TX0_MSG_ID));
>> +
>>      /* Put descriptor to the ring */
>>      e1000e_tx_ring_push(d, &descr);
>>
>> @@ -105,6 +108,9 @@ static void e1000e_receive_verify(QE1000E *d, int
>> *test_sockets, QGuestAllocator
>>      char buffer[64];
>>      int ret;
>>
>> +    /* Ensure the interrupt has not been taken already */
>> +    g_assert(!e1000e_seen_isr(d, E1000E_RX0_MSG_ID));
>> +
>>
> I don't think potentially crashing the guest is the right solution.
> I suggest maybe substituting with logging.

This is just for qtest where assertions are used to indicate
failure.

Thanks,
Nick

Re: [PATCH 3/9] qtest/e1000e|igb: assert irqs are clear before triggering an irq

Posted by Akihiko Odaki 2 weeks, 4 days ago

On 2025/01/18 2:02, Nicholas Piggin wrote:
> Assert there is no existing irq raised that would lead to a false
> positive interrupt test.
> 
> e1000e has to disable interrupt throttling for this test, because
> it can cause delayed superfluous interrupts which trip the assertions.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>   tests/qtest/libqos/e1000e.h |  1 +
>   tests/qtest/e1000e-test.c   | 10 ++++++++++
>   tests/qtest/igb-test.c      |  6 ++++++
>   tests/qtest/libqos/e1000e.c |  9 ++++++++-
>   4 files changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/qtest/libqos/e1000e.h b/tests/qtest/libqos/e1000e.h
> index 30643c80949..7154aec0339 100644
> --- a/tests/qtest/libqos/e1000e.h
> +++ b/tests/qtest/libqos/e1000e.h
> @@ -54,6 +54,7 @@ static inline uint32_t e1000e_macreg_read(QE1000E *d, uint32_t reg)
>       return qpci_io_readl(&d_pci->pci_dev, d_pci->mac_regs, reg);
>   }
>   
> +bool e1000e_seen_isr(QE1000E *d, uint16_t msg_id);

I would rather name it e1000e_pending_isr.
We may have seen an interrupt in the past but it may no longer be pending.

>   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);
> diff --git a/tests/qtest/e1000e-test.c b/tests/qtest/e1000e-test.c
> index 746d26cfb67..9ab81ecff5b 100644
> --- a/tests/qtest/e1000e-test.c
> +++ b/tests/qtest/e1000e-test.c
> @@ -61,6 +61,9 @@ static void e1000e_send_verify(QE1000E *d, int *test_sockets, QGuestAllocator *a
>                                      E1000_TXD_DTYP_D   |
>                                      sizeof(buffer));
>   
> +    /* Ensure the interrupt has not been taken already */
> +    g_assert(!e1000e_seen_isr(d, E1000E_TX0_MSG_ID));
> +
>       /* Put descriptor to the ring */
>       e1000e_tx_ring_push(d, &descr);
>   
> @@ -105,6 +108,9 @@ static void e1000e_receive_verify(QE1000E *d, int *test_sockets, QGuestAllocator
>       char buffer[64];
>       int ret;
>   
> +    /* Ensure the interrupt has not been taken already */
> +    g_assert(!e1000e_seen_isr(d, E1000E_RX0_MSG_ID));
> +
>       /* Send a dummy packet to device's socket*/
>       ret = iov_send(test_sockets[0], iov, 2, 0, sizeof(len) + sizeof(packet));
>       g_assert_cmpint(ret, == , sizeof(packet) + sizeof(len));
> @@ -188,6 +194,10 @@ static void test_e1000e_multiple_transfers(void *obj, void *data,
>           return;
>       }
>   
> +    /* Clear EITR because buggy QEMU throttle timer causes superfluous irqs */
> +    e1000e_macreg_write(d, E1000_EITR + E1000E_RX0_MSG_ID * 4, 0);
> +    e1000e_macreg_write(d, E1000_EITR + E1000E_TX0_MSG_ID * 4, 0);
> +
>       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 cf8b4131cf2..1bbb4bea4c1 100644
> --- a/tests/qtest/igb-test.c
> +++ b/tests/qtest/igb-test.c
> @@ -64,6 +64,9 @@ static void igb_send_verify(QE1000E *d, int *test_sockets, QGuestAllocator *allo
>                                             E1000_TXD_DTYP_D   |
>                                             sizeof(buffer));
>   
> +    /* Ensure the interrupt has not been taken already */
> +    g_assert(!e1000e_seen_isr(d, E1000E_TX0_MSG_ID));
> +
>       /* Put descriptor to the ring */
>       e1000e_tx_ring_push(d, &descr);
>   
> @@ -119,6 +122,9 @@ static void igb_receive_verify(QE1000E *d, int *test_sockets, QGuestAllocator *a
>       memset(&descr, 0, sizeof(descr));
>       descr.read.pkt_addr = cpu_to_le64(data);
>   
> +    /* Ensure the interrupt has not been taken already */
> +    g_assert(!e1000e_seen_isr(d, E1000E_RX0_MSG_ID));
> +
>       /* Put descriptor to the ring */
>       e1000e_rx_ring_push(d, &descr);
>   
> diff --git a/tests/qtest/libqos/e1000e.c b/tests/qtest/libqos/e1000e.c
> index 925654c7fd4..4e4c387b0bf 100644
> --- a/tests/qtest/libqos/e1000e.c
> +++ b/tests/qtest/libqos/e1000e.c
> @@ -77,13 +77,20 @@ static void e1000e_foreach_callback(QPCIDevice *dev, int devfn, void *data)
>       g_free(dev);
>   }
>   
> +bool e1000e_seen_isr(QE1000E *d, uint16_t msg_id)
> +{
> +    QE1000E_PCI *d_pci = container_of(d, QE1000E_PCI, e1000e);
> +
> +    return qpci_msix_pending(&d_pci->pci_dev, msg_id);
> +}
> +
>   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;
>   
>       do {
> -        if (qpci_msix_pending(&d_pci->pci_dev, msg_id)) {
> +        if (e1000e_seen_isr(d, msg_id)) {
>               return;
>           }
>           qtest_clock_step(d_pci->pci_dev.bus->qts, 10000);

[PATCH 1/9] qtest/e1000e|igb: Clear interrupt-cause and msix pending bits after irq
[PATCH 2/9] net/e1000e: Permit disabling interrupt throttling
[PATCH 3/9] qtest/e1000e|igb: assert irqs are clear before triggering an irq
[PATCH 4/9] net/igb: Fix interrupt throttling interval calculation
[PATCH 5/9] net/igb: Fix EITR LLI and counter fields
[PATCH 6/9] net/e1000e|igb: Fix interrupt throttling logic
[PATCH 7/9] qtest/e1000e|igb: Test interrupt throttling in multiple_transfers test
[PATCH 8/9] net/e1000e: Fix xITR minimum value
[PATCH 9/9] hw/net/e1000e|igb: Remove xitr_guest_value logic