This function is duplicated 3 times, with more potential future users.
Factor it into libqos, using qtest_memset instead of qtest_writel to
clear the message just because that looks nicer with the qtest_memread
used to read it.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 tests/qtest/libqos/pci.h               |  2 ++
 tests/qtest/libqos/pci.c               | 48 ++++++++++++++++++++++++++
 tests/qtest/libqos/virtio-pci-modern.c | 31 +++--------------
 tests/qtest/libqos/virtio-pci.c        | 40 ++++-----------------
 4 files changed, 62 insertions(+), 59 deletions(-)

diff --git a/tests/qtest/libqos/pci.h b/tests/qtest/libqos/pci.h
index 83896145235..9f8f154c301 100644
--- a/tests/qtest/libqos/pci.h
+++ b/tests/qtest/libqos/pci.h
@@ -92,6 +92,8 @@ void qpci_msix_enable(QPCIDevice *dev);
 void qpci_msix_disable(QPCIDevice *dev);
 bool qpci_msix_pending(QPCIDevice *dev, uint16_t entry);
 bool qpci_msix_masked(QPCIDevice *dev, uint16_t entry);
+bool qpci_msix_test_interrupt(QPCIDevice *dev, uint32_t msix_entry,
+                              uint64_t msix_addr, uint32_t msix_data);
 uint16_t qpci_msix_table_size(QPCIDevice *dev);
 
 uint8_t qpci_config_readb(QPCIDevice *dev, uint8_t offset);
diff --git a/tests/qtest/libqos/pci.c b/tests/qtest/libqos/pci.c
index a59197b9922..773fd1fb6cf 100644
--- a/tests/qtest/libqos/pci.c
+++ b/tests/qtest/libqos/pci.c
@@ -351,6 +351,54 @@ bool qpci_msix_masked(QPCIDevice *dev, uint16_t entry)
     }
 }
 
+/**
+ * qpci_msix_test_interrupt - test whether msix interrupt has been raised
+ * @dev: PCI device
+ * @msix_entry: msix entry to test
+ * @msix_addr: address of msix message
+ * @msix_data: expected msix message payload
+ *
+ * This tests whether the msix source has raised an interrupt. If the msix
+ * entry is masked, it tests the pending bit array for a pending message
+ * and @msix_addr and @msix_data need not be supplied. If the entry is not
+ * masked, it tests the address for corresponding data to see if the interrupt
+ * fired.
+ *
+ * Note that this does not lower the interrupt, however it does clear the
+ * msix message address to 0 if it is found set. This must be called with
+ * the msix address memory containing either 0 or the value of data, otherwise
+ * it will assert on incorrect message.
+ */
+bool qpci_msix_test_interrupt(QPCIDevice *dev, uint32_t msix_entry,
+                              uint64_t msix_addr, uint32_t msix_data)
+{
+    uint32_t data;
+
+    g_assert(dev->msix_enabled);
+    g_assert_cmpint(msix_entry, !=, -1);
+
+    if (qpci_msix_masked(dev, msix_entry)) {
+        /* No ISR checking should be done if masked, but read anyway */
+        return qpci_msix_pending(dev, msix_entry);
+    }
+
+    g_assert_cmpint(msix_addr, !=, 0);
+    g_assert_cmpint(msix_data, !=, 0);
+
+    /* msix payload is written in little-endian format */
+    qtest_memread(dev->bus->qts, msix_addr, &data, 4);
+    data = le32_to_cpu(data);
+    if (data == 0) {
+        return false;
+    }
+
+    /* got a message, ensure it matches expected value then clear it. */
+    g_assert_cmphex(data, ==, msix_data);
+    qtest_memset(dev->bus->qts, msix_addr, 0, 4);
+
+    return true;
+}
+
 uint16_t qpci_msix_table_size(QPCIDevice *dev)
 {
     uint8_t addr;
diff --git a/tests/qtest/libqos/virtio-pci-modern.c b/tests/qtest/libqos/virtio-pci-modern.c
index 5dae41e6d74..0d7d89bbcb1 100644
--- a/tests/qtest/libqos/virtio-pci-modern.c
+++ b/tests/qtest/libqos/virtio-pci-modern.c
@@ -126,28 +126,6 @@ static void set_status(QVirtioDevice *d, uint8_t status)
                           status);
 }
 
-static bool get_msix_status(QVirtioPCIDevice *dev, uint32_t msix_entry,
-                            uint32_t msix_addr, uint32_t msix_data)
-{
-    uint32_t data;
-
-    g_assert_cmpint(msix_entry, !=, -1);
-    if (qpci_msix_masked(dev->pdev, msix_entry)) {
-        /* No ISR checking should be done if masked, but read anyway */
-        return qpci_msix_pending(dev->pdev, msix_entry);
-    }
-
-    qtest_memread(dev->pdev->bus->qts, msix_addr, &data, 4);
-    data = le32_to_cpu(data);
-    if (data == 0) {
-        return false;
-    }
-    /* got a message, ensure it matches expected value then clear it. */
-    g_assert_cmphex(data, ==, msix_data);
-    qtest_writel(dev->pdev->bus->qts, msix_addr, 0);
-    return true;
-}
-
 static bool get_queue_isr_status(QVirtioDevice *d, QVirtQueue *vq)
 {
     QVirtioPCIDevice *dev = container_of(d, QVirtioPCIDevice, vdev);
@@ -155,8 +133,8 @@ static bool get_queue_isr_status(QVirtioDevice *d, QVirtQueue *vq)
     if (dev->pdev->msix_enabled) {
         QVirtQueuePCI *vqpci = container_of(vq, QVirtQueuePCI, vq);
 
-        return get_msix_status(dev, vqpci->msix_entry, vqpci->msix_addr,
-                               vqpci->msix_data);
+        return qpci_msix_test_interrupt(dev->pdev, vqpci->msix_entry,
+                                        vqpci->msix_addr, vqpci->msix_data);
     }
 
     return qpci_io_readb(dev->pdev, dev->bar, dev->isr_cfg_offset) & 1;
@@ -167,8 +145,9 @@ static bool get_config_isr_status(QVirtioDevice *d)
     QVirtioPCIDevice *dev = container_of(d, QVirtioPCIDevice, vdev);
 
     if (dev->pdev->msix_enabled) {
-        return get_msix_status(dev, dev->config_msix_entry,
-                               dev->config_msix_addr, dev->config_msix_data);
+        return qpci_msix_test_interrupt(dev->pdev, dev->config_msix_entry,
+                                        dev->config_msix_addr,
+                                        dev->config_msix_data);
     }
 
     return qpci_io_readb(dev->pdev, dev->bar, dev->isr_cfg_offset) & 2;
diff --git a/tests/qtest/libqos/virtio-pci.c b/tests/qtest/libqos/virtio-pci.c
index 76ea1f45ba9..ea8114e2438 100644
--- a/tests/qtest/libqos/virtio-pci.c
+++ b/tests/qtest/libqos/virtio-pci.c
@@ -122,25 +122,12 @@ static void qvirtio_pci_set_status(QVirtioDevice *d, uint8_t status)
 static bool qvirtio_pci_get_queue_isr_status(QVirtioDevice *d, QVirtQueue *vq)
 {
     QVirtioPCIDevice *dev = container_of(d, QVirtioPCIDevice, vdev);
-    QVirtQueuePCI *vqpci = (QVirtQueuePCI *)vq;
-    uint32_t data;
 
     if (dev->pdev->msix_enabled) {
-        g_assert_cmpint(vqpci->msix_entry, !=, -1);
-        if (qpci_msix_masked(dev->pdev, vqpci->msix_entry)) {
-            /* No ISR checking should be done if masked, but read anyway */
-            return qpci_msix_pending(dev->pdev, vqpci->msix_entry);
-        } else {
-            qtest_memread(dev->pdev->bus->qts, vqpci->msix_addr, &data, 4);
-            data = le32_to_cpu(data);
-            if (data == 0) {
-                return false;
-            }
-            /* got a message, ensure it matches expected value then clear it. */
-            g_assert_cmphex(data, ==, vqpci->msix_data);
-            qtest_writel(dev->pdev->bus->qts, vqpci->msix_addr, 0);
-            return true;
-        }
+        QVirtQueuePCI *vqpci = (QVirtQueuePCI *)vq;
+
+        return qpci_msix_test_interrupt(dev->pdev, vqpci->msix_entry,
+                                        vqpci->msix_addr, vqpci->msix_data);
     } else {
         return qpci_io_readb(dev->pdev, dev->bar, VIRTIO_PCI_ISR) & 1;
     }
@@ -149,24 +136,11 @@ static bool qvirtio_pci_get_queue_isr_status(QVirtioDevice *d, QVirtQueue *vq)
 static bool qvirtio_pci_get_config_isr_status(QVirtioDevice *d)
 {
     QVirtioPCIDevice *dev = container_of(d, QVirtioPCIDevice, vdev);
-    uint32_t data;
 
     if (dev->pdev->msix_enabled) {
-        g_assert_cmpint(dev->config_msix_entry, !=, -1);
-        if (qpci_msix_masked(dev->pdev, dev->config_msix_entry)) {
-            /* No ISR checking should be done if masked, but read anyway */
-            return qpci_msix_pending(dev->pdev, dev->config_msix_entry);
-        } else {
-            qtest_memread(dev->pdev->bus->qts, dev->config_msix_addr, &data, 4);
-            data = le32_to_cpu(data);
-            if (data == 0) {
-                return false;
-            }
-            /* got a message, ensure it matches expected value then clear it. */
-            g_assert_cmphex(data, ==, dev->config_msix_data);
-            qtest_writel(dev->pdev->bus->qts, dev->config_msix_addr, 0);
-            return true;
-        }
+        return qpci_msix_test_interrupt(dev->pdev, dev->config_msix_entry,
+                                        dev->config_msix_addr,
+                                        dev->config_msix_data);
     } else {
         return qpci_io_readb(dev->pdev, dev->bar, VIRTIO_PCI_ISR) & 2;
     }
-- 
2.47.1

On 2025/05/02 12:04, Nicholas Piggin wrote:
> This function is duplicated 3 times, with more potential future users.
> Factor it into libqos, using qtest_memset instead of qtest_writel to
> clear the message just because that looks nicer with the qtest_memread
> used to read it.
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>   tests/qtest/libqos/pci.h               |  2 ++
>   tests/qtest/libqos/pci.c               | 48 ++++++++++++++++++++++++++
>   tests/qtest/libqos/virtio-pci-modern.c | 31 +++--------------
>   tests/qtest/libqos/virtio-pci.c        | 40 ++++-----------------
>   4 files changed, 62 insertions(+), 59 deletions(-)
> 
> diff --git a/tests/qtest/libqos/pci.h b/tests/qtest/libqos/pci.h
> index 83896145235..9f8f154c301 100644
> --- a/tests/qtest/libqos/pci.h
> +++ b/tests/qtest/libqos/pci.h
> @@ -92,6 +92,8 @@ void qpci_msix_enable(QPCIDevice *dev);
>   void qpci_msix_disable(QPCIDevice *dev);
>   bool qpci_msix_pending(QPCIDevice *dev, uint16_t entry);
>   bool qpci_msix_masked(QPCIDevice *dev, uint16_t entry);
> +bool qpci_msix_test_interrupt(QPCIDevice *dev, uint32_t msix_entry,
> +                              uint64_t msix_addr, uint32_t msix_data);
>   uint16_t qpci_msix_table_size(QPCIDevice *dev);
>   
>   uint8_t qpci_config_readb(QPCIDevice *dev, uint8_t offset);
> diff --git a/tests/qtest/libqos/pci.c b/tests/qtest/libqos/pci.c
> index a59197b9922..773fd1fb6cf 100644
> --- a/tests/qtest/libqos/pci.c
> +++ b/tests/qtest/libqos/pci.c
> @@ -351,6 +351,54 @@ bool qpci_msix_masked(QPCIDevice *dev, uint16_t entry)
>       }
>   }
>   
> +/**
> + * qpci_msix_test_interrupt - test whether msix interrupt has been raised

Nitpick: Let's write as "MSI-X" instead of msix in documentation.

> + * @dev: PCI device
> + * @msix_entry: msix entry to test
> + * @msix_addr: address of msix message

Perhaps deriving the address in this function may make things simpler by 
removing the documentation and assertion code and not requiring callers 
to pass it.

> + * @msix_data: expected msix message payload
> + *
> + * This tests whether the msix source has raised an interrupt. If the msix

Another nitpick: "whether the device has raised an MSI-X interrupt" - 
"msix source" is not a pharsed used elsewhere and it can raise other 
kind of interrupts too so let's make the kind of interrupt specific.

> + * entry is masked, it tests the pending bit array for a pending message
> + * and @msix_addr and @msix_data need not be supplied. If the entry is not
> + * masked, it tests the address for corresponding data to see if the interrupt
> + * fired.
> + *
> + * Note that this does not lower the interrupt, however it does clear the
> + * msix message address to 0 if it is found set. This must be called with
> + * the msix address memory containing either 0 or the value of data, otherwise
> + * it will assert on incorrect message.
> + */
> +bool qpci_msix_test_interrupt(QPCIDevice *dev, uint32_t msix_entry,
> +                              uint64_t msix_addr, uint32_t msix_data)
> +{
> +    uint32_t data;
> +
> +    g_assert(dev->msix_enabled);
> +    g_assert_cmpint(msix_entry, !=, -1);
> +
> +    if (qpci_msix_masked(dev, msix_entry)) {
> +        /* No ISR checking should be done if masked, but read anyway */
> +        return qpci_msix_pending(dev, msix_entry);
> +    }
> +
> +    g_assert_cmpint(msix_addr, !=, 0);
> +    g_assert_cmpint(msix_data, !=, 0);
> +
> +    /* msix payload is written in little-endian format */
> +    qtest_memread(dev->bus->qts, msix_addr, &data, 4);
> +    data = le32_to_cpu(data);
> +    if (data == 0) {
> +        return false;
> +    }
> +
> +    /* got a message, ensure it matches expected value then clear it. */
> +    g_assert_cmphex(data, ==, msix_data);
> +    qtest_memset(dev->bus->qts, msix_addr, 0, 4);
> +
> +    return true;
> +}
> +
>   uint16_t qpci_msix_table_size(QPCIDevice *dev)
>   {
>       uint8_t addr;
> diff --git a/tests/qtest/libqos/virtio-pci-modern.c b/tests/qtest/libqos/virtio-pci-modern.c
> index 5dae41e6d74..0d7d89bbcb1 100644
> --- a/tests/qtest/libqos/virtio-pci-modern.c
> +++ b/tests/qtest/libqos/virtio-pci-modern.c
> @@ -126,28 +126,6 @@ static void set_status(QVirtioDevice *d, uint8_t status)
>                             status);
>   }
>   
> -static bool get_msix_status(QVirtioPCIDevice *dev, uint32_t msix_entry,
> -                            uint32_t msix_addr, uint32_t msix_data)
> -{
> -    uint32_t data;
> -
> -    g_assert_cmpint(msix_entry, !=, -1);
> -    if (qpci_msix_masked(dev->pdev, msix_entry)) {
> -        /* No ISR checking should be done if masked, but read anyway */
> -        return qpci_msix_pending(dev->pdev, msix_entry);
> -    }
> -
> -    qtest_memread(dev->pdev->bus->qts, msix_addr, &data, 4);
> -    data = le32_to_cpu(data);
> -    if (data == 0) {
> -        return false;
> -    }
> -    /* got a message, ensure it matches expected value then clear it. */
> -    g_assert_cmphex(data, ==, msix_data);
> -    qtest_writel(dev->pdev->bus->qts, msix_addr, 0);
> -    return true;
> -}
> -
>   static bool get_queue_isr_status(QVirtioDevice *d, QVirtQueue *vq)
>   {
>       QVirtioPCIDevice *dev = container_of(d, QVirtioPCIDevice, vdev);
> @@ -155,8 +133,8 @@ static bool get_queue_isr_status(QVirtioDevice *d, QVirtQueue *vq)
>       if (dev->pdev->msix_enabled) {
>           QVirtQueuePCI *vqpci = container_of(vq, QVirtQueuePCI, vq);
>   
> -        return get_msix_status(dev, vqpci->msix_entry, vqpci->msix_addr,
> -                               vqpci->msix_data);
> +        return qpci_msix_test_interrupt(dev->pdev, vqpci->msix_entry,
> +                                        vqpci->msix_addr, vqpci->msix_data);
>       }
>   
>       return qpci_io_readb(dev->pdev, dev->bar, dev->isr_cfg_offset) & 1;
> @@ -167,8 +145,9 @@ static bool get_config_isr_status(QVirtioDevice *d)
>       QVirtioPCIDevice *dev = container_of(d, QVirtioPCIDevice, vdev);
>   
>       if (dev->pdev->msix_enabled) {
> -        return get_msix_status(dev, dev->config_msix_entry,
> -                               dev->config_msix_addr, dev->config_msix_data);
> +        return qpci_msix_test_interrupt(dev->pdev, dev->config_msix_entry,
> +                                        dev->config_msix_addr,
> +                                        dev->config_msix_data);
>       }
>   
>       return qpci_io_readb(dev->pdev, dev->bar, dev->isr_cfg_offset) & 2;
> diff --git a/tests/qtest/libqos/virtio-pci.c b/tests/qtest/libqos/virtio-pci.c
> index 76ea1f45ba9..ea8114e2438 100644
> --- a/tests/qtest/libqos/virtio-pci.c
> +++ b/tests/qtest/libqos/virtio-pci.c
> @@ -122,25 +122,12 @@ static void qvirtio_pci_set_status(QVirtioDevice *d, uint8_t status)
>   static bool qvirtio_pci_get_queue_isr_status(QVirtioDevice *d, QVirtQueue *vq)
>   {
>       QVirtioPCIDevice *dev = container_of(d, QVirtioPCIDevice, vdev);
> -    QVirtQueuePCI *vqpci = (QVirtQueuePCI *)vq;
> -    uint32_t data;
>   
>       if (dev->pdev->msix_enabled) {
> -        g_assert_cmpint(vqpci->msix_entry, !=, -1);
> -        if (qpci_msix_masked(dev->pdev, vqpci->msix_entry)) {
> -            /* No ISR checking should be done if masked, but read anyway */
> -            return qpci_msix_pending(dev->pdev, vqpci->msix_entry);
> -        } else {
> -            qtest_memread(dev->pdev->bus->qts, vqpci->msix_addr, &data, 4);
> -            data = le32_to_cpu(data);
> -            if (data == 0) {
> -                return false;
> -            }
> -            /* got a message, ensure it matches expected value then clear it. */
> -            g_assert_cmphex(data, ==, vqpci->msix_data);
> -            qtest_writel(dev->pdev->bus->qts, vqpci->msix_addr, 0);
> -            return true;
> -        }
> +        QVirtQueuePCI *vqpci = (QVirtQueuePCI *)vq;
> +
> +        return qpci_msix_test_interrupt(dev->pdev, vqpci->msix_entry,
> +                                        vqpci->msix_addr, vqpci->msix_data);
>       } else {
>           return qpci_io_readb(dev->pdev, dev->bar, VIRTIO_PCI_ISR) & 1;
>       }
> @@ -149,24 +136,11 @@ static bool qvirtio_pci_get_queue_isr_status(QVirtioDevice *d, QVirtQueue *vq)
>   static bool qvirtio_pci_get_config_isr_status(QVirtioDevice *d)
>   {
>       QVirtioPCIDevice *dev = container_of(d, QVirtioPCIDevice, vdev);
> -    uint32_t data;
>   
>       if (dev->pdev->msix_enabled) {
> -        g_assert_cmpint(dev->config_msix_entry, !=, -1);
> -        if (qpci_msix_masked(dev->pdev, dev->config_msix_entry)) {
> -            /* No ISR checking should be done if masked, but read anyway */
> -            return qpci_msix_pending(dev->pdev, dev->config_msix_entry);
> -        } else {
> -            qtest_memread(dev->pdev->bus->qts, dev->config_msix_addr, &data, 4);
> -            data = le32_to_cpu(data);
> -            if (data == 0) {
> -                return false;
> -            }
> -            /* got a message, ensure it matches expected value then clear it. */
> -            g_assert_cmphex(data, ==, dev->config_msix_data);
> -            qtest_writel(dev->pdev->bus->qts, dev->config_msix_addr, 0);
> -            return true;
> -        }
> +        return qpci_msix_test_interrupt(dev->pdev, dev->config_msix_entry,
> +                                        dev->config_msix_addr,
> +                                        dev->config_msix_data);
>       } else {
>           return qpci_io_readb(dev->pdev, dev->bar, VIRTIO_PCI_ISR) & 2;
>       }

On Mon May 5, 2025 at 3:37 PM AEST, Akihiko Odaki wrote:
> On 2025/05/02 12:04, Nicholas Piggin wrote:
>> This function is duplicated 3 times, with more potential future users.
>> Factor it into libqos, using qtest_memset instead of qtest_writel to
>> clear the message just because that looks nicer with the qtest_memread
>> used to read it.
>> 
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>>   tests/qtest/libqos/pci.h               |  2 ++
>>   tests/qtest/libqos/pci.c               | 48 ++++++++++++++++++++++++++
>>   tests/qtest/libqos/virtio-pci-modern.c | 31 +++--------------
>>   tests/qtest/libqos/virtio-pci.c        | 40 ++++-----------------
>>   4 files changed, 62 insertions(+), 59 deletions(-)
>> 
>> diff --git a/tests/qtest/libqos/pci.h b/tests/qtest/libqos/pci.h
>> index 83896145235..9f8f154c301 100644
>> --- a/tests/qtest/libqos/pci.h
>> +++ b/tests/qtest/libqos/pci.h
>> @@ -92,6 +92,8 @@ void qpci_msix_enable(QPCIDevice *dev);
>>   void qpci_msix_disable(QPCIDevice *dev);
>>   bool qpci_msix_pending(QPCIDevice *dev, uint16_t entry);
>>   bool qpci_msix_masked(QPCIDevice *dev, uint16_t entry);
>> +bool qpci_msix_test_interrupt(QPCIDevice *dev, uint32_t msix_entry,
>> +                              uint64_t msix_addr, uint32_t msix_data);
>>   uint16_t qpci_msix_table_size(QPCIDevice *dev);
>>   
>>   uint8_t qpci_config_readb(QPCIDevice *dev, uint8_t offset);
>> diff --git a/tests/qtest/libqos/pci.c b/tests/qtest/libqos/pci.c
>> index a59197b9922..773fd1fb6cf 100644
>> --- a/tests/qtest/libqos/pci.c
>> +++ b/tests/qtest/libqos/pci.c
>> @@ -351,6 +351,54 @@ bool qpci_msix_masked(QPCIDevice *dev, uint16_t entry)
>>       }
>>   }
>>   
>> +/**
>> + * qpci_msix_test_interrupt - test whether msix interrupt has been raised
>
> Nitpick: Let's write as "MSI-X" instead of msix in documentation.

Okay.

>> + * @dev: PCI device
>> + * @msix_entry: msix entry to test
>> + * @msix_addr: address of msix message
>
> Perhaps deriving the address in this function may make things simpler by 
> removing the documentation and assertion code and not requiring callers 
> to pass it.

addr and data could both be derived from the MSI-X table, but passing
them in here is how some of the existing helpers are structured, so I
will leave it like this. I think I have slight preference for this way
but if there is strong preference for deriving them implicitly then it
could be a follow up patch.

>
>> + * @msix_data: expected msix message payload
>> + *
>> + * This tests whether the msix source has raised an interrupt. If the msix
>
> Another nitpick: "whether the device has raised an MSI-X interrupt" - 
> "msix source" is not a pharsed used elsewhere and it can raise other 
> kind of interrupts too so let's make the kind of interrupt specific.

Sure.

Thanks,
Nick