[PATCH v5 00/11] tests/qtest: pci and msix fixes

[PATCH v5 00/11] tests/qtest: pci and msix fixes

[Nicholas Piggin]

Since v4:
https://lore.kernel.org/qemu-devel/20250411044130.201724-1-npiggin@gmail.com/

I merged in the "spapr" series that touches similar code:

https://lore.kernel.org/qemu-devel/20250416145918.415674-1-npiggin@gmail.com/

- Simplified the ahci shutdown logic to keep one unified shutdown
  function.

Thanks,
Nick

Nicholas Piggin (11):
  tests/qtest: Enforce zero for the "un-fired" msix message value
  tests/qtest: Fix virtio msix message endianness
  tests/qtest: Add libqos function for testing msix interrupt status
  tests/qtest: Enable spapr dma with linear iommu map
  tests/qtest/ahci: unmap pci bar before reusing device
  tests/qtest/ahci: don't unmap pci bar if it wasn't mapped
  tests/qtest/libquos/virtio: unmap pci bar when disabling device
  tests/qtest/libquos/pci: Add migration fixup helper for pci devices
  qtest/libqos/pci: Enforce balanced iomap/unmap
  qtest/libqos/pci: Fix qpci_msix_enable sharing bar0
  qtest/libqos/pci: Factor msix entry helpers into pci common code

 tests/qtest/libqos/ahci.h              |   3 +
 tests/qtest/libqos/pci.h               |  20 ++-
 tests/qtest/libqos/virtio-pci.h        |   1 +
 hw/ppc/spapr_iommu.c                   |  10 +-
 tests/qtest/ahci-test.c                |  10 ++
 tests/qtest/e1000e-test.c              |  21 ---
 tests/qtest/igb-test.c                 |  21 ---
 tests/qtest/libqos/ahci.c              |  20 +++
 tests/qtest/libqos/generic-pcihost.c   |   1 -
 tests/qtest/libqos/pci-pc.c            |   3 -
 tests/qtest/libqos/pci-spapr.c         |   7 +-
 tests/qtest/libqos/pci.c               | 210 ++++++++++++++++++++++---
 tests/qtest/libqos/virtio-pci-modern.c |  30 +---
 tests/qtest/libqos/virtio-pci.c        |  97 +++---------
 tests/qtest/vhost-user-blk-test.c      |   6 -
 tests/qtest/virtio-blk-test.c          |  12 --
 16 files changed, 278 insertions(+), 194 deletions(-)

-- 
2.47.1

[PATCH v5 01/11] tests/qtest: Enforce zero for the "un-fired" msix message value

[Nicholas Piggin]

virtio-pci detects an unmasked msix interrupt has fired by looking
for the data payload value at the target address. If a value of zero
is enforced for the memory value when an interrupt has not fired,
then an assertion can be added to catch the case where something
changed the memory to an unexpected value.

This catches an endian conversion bug in the message value when
running these tests on a big endian target. Previously the test
just times out waiting for interrupt, after this it fails nicely.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Fabiano Rosas <farosas@suse.de>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 tests/qtest/libqos/virtio-pci-modern.c |  9 +++++----
 tests/qtest/libqos/virtio-pci.c        | 20 ++++++++++++--------
 2 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/tests/qtest/libqos/virtio-pci-modern.c b/tests/qtest/libqos/virtio-pci-modern.c
index 4e67fcbd5d3..f31b3be656d 100644
--- a/tests/qtest/libqos/virtio-pci-modern.c
+++ b/tests/qtest/libqos/virtio-pci-modern.c
@@ -137,12 +137,13 @@ static bool get_msix_status(QVirtioPCIDevice *dev, uint32_t msix_entry,
     }
 
     data = qtest_readl(dev->pdev->bus->qts, msix_addr);
-    if (data == msix_data) {
-        qtest_writel(dev->pdev->bus->qts, msix_addr, 0);
-        return true;
-    } else {
+    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)
diff --git a/tests/qtest/libqos/virtio-pci.c b/tests/qtest/libqos/virtio-pci.c
index 002bf8b8c2d..102e45b5248 100644
--- a/tests/qtest/libqos/virtio-pci.c
+++ b/tests/qtest/libqos/virtio-pci.c
@@ -132,12 +132,13 @@ static bool qvirtio_pci_get_queue_isr_status(QVirtioDevice *d, QVirtQueue *vq)
             return qpci_msix_pending(dev->pdev, vqpci->msix_entry);
         } else {
             data = qtest_readl(dev->pdev->bus->qts, vqpci->msix_addr);
-            if (data == vqpci->msix_data) {
-                qtest_writel(dev->pdev->bus->qts, vqpci->msix_addr, 0);
-                return true;
-            } else {
+            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;
         }
     } else {
         return qpci_io_readb(dev->pdev, dev->bar, VIRTIO_PCI_ISR) & 1;
@@ -156,12 +157,13 @@ static bool qvirtio_pci_get_config_isr_status(QVirtioDevice *d)
             return qpci_msix_pending(dev->pdev, dev->config_msix_entry);
         } else {
             data = qtest_readl(dev->pdev->bus->qts, dev->config_msix_addr);
-            if (data == dev->config_msix_data) {
-                qtest_writel(dev->pdev->bus->qts, dev->config_msix_addr, 0);
-                return true;
-            } else {
+            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;
         }
     } else {
         return qpci_io_readb(dev->pdev, dev->bar, VIRTIO_PCI_ISR) & 2;
@@ -323,6 +325,7 @@ void qvirtqueue_pci_msix_setup(QVirtioPCIDevice *d, QVirtQueuePCI *vqpci,
     vqpci->msix_entry = entry;
 
     vqpci->msix_addr = guest_alloc(alloc, 4);
+    qtest_memset(d->pdev->bus->qts, vqpci->msix_addr, 0, 4);
     qpci_io_writel(d->pdev, d->pdev->msix_table_bar,
                    off + PCI_MSIX_ENTRY_LOWER_ADDR, vqpci->msix_addr & ~0UL);
     qpci_io_writel(d->pdev, d->pdev->msix_table_bar,
@@ -355,6 +358,7 @@ void qvirtio_pci_set_msix_configuration_vector(QVirtioPCIDevice *d,
 
     d->config_msix_data = 0x12345678;
     d->config_msix_addr = guest_alloc(alloc, 4);
+    qtest_memset(d->pdev->bus->qts, d->config_msix_addr, 0, 4);
 
     qpci_io_writel(d->pdev, d->pdev->msix_table_bar,
                    off + PCI_MSIX_ENTRY_LOWER_ADDR, d->config_msix_addr & ~0UL);
-- 
2.47.1

[PATCH v5 02/11] tests/qtest: Fix virtio msix message endianness

[Nicholas Piggin]

msix messages are written to memory in little-endian order, so they
should not be byteswapped depending on target endianness, but read
as le and converted to host endian by the qtest.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Fabiano Rosas <farosas@suse.de>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 tests/qtest/libqos/virtio-pci-modern.c | 4 +++-
 tests/qtest/libqos/virtio-pci.c        | 6 ++++--
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/tests/qtest/libqos/virtio-pci-modern.c b/tests/qtest/libqos/virtio-pci-modern.c
index f31b3be656d..5dae41e6d74 100644
--- a/tests/qtest/libqos/virtio-pci-modern.c
+++ b/tests/qtest/libqos/virtio-pci-modern.c
@@ -8,6 +8,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/bswap.h"
 #include "standard-headers/linux/pci_regs.h"
 #include "standard-headers/linux/virtio_pci.h"
 #include "standard-headers/linux/virtio_config.h"
@@ -136,7 +137,8 @@ static bool get_msix_status(QVirtioPCIDevice *dev, uint32_t msix_entry,
         return qpci_msix_pending(dev->pdev, msix_entry);
     }
 
-    data = qtest_readl(dev->pdev->bus->qts, msix_addr);
+    qtest_memread(dev->pdev->bus->qts, msix_addr, &data, 4);
+    data = le32_to_cpu(data);
     if (data == 0) {
         return false;
     }
diff --git a/tests/qtest/libqos/virtio-pci.c b/tests/qtest/libqos/virtio-pci.c
index 102e45b5248..76ea1f45ba9 100644
--- a/tests/qtest/libqos/virtio-pci.c
+++ b/tests/qtest/libqos/virtio-pci.c
@@ -131,7 +131,8 @@ static bool qvirtio_pci_get_queue_isr_status(QVirtioDevice *d, QVirtQueue *vq)
             /* No ISR checking should be done if masked, but read anyway */
             return qpci_msix_pending(dev->pdev, vqpci->msix_entry);
         } else {
-            data = qtest_readl(dev->pdev->bus->qts, vqpci->msix_addr);
+            qtest_memread(dev->pdev->bus->qts, vqpci->msix_addr, &data, 4);
+            data = le32_to_cpu(data);
             if (data == 0) {
                 return false;
             }
@@ -156,7 +157,8 @@ static bool qvirtio_pci_get_config_isr_status(QVirtioDevice *d)
             /* No ISR checking should be done if masked, but read anyway */
             return qpci_msix_pending(dev->pdev, dev->config_msix_entry);
         } else {
-            data = qtest_readl(dev->pdev->bus->qts, dev->config_msix_addr);
+            qtest_memread(dev->pdev->bus->qts, dev->config_msix_addr, &data, 4);
+            data = le32_to_cpu(data);
             if (data == 0) {
                 return false;
             }
-- 
2.47.1

Re: [PATCH v5 02/11] tests/qtest: Fix virtio msix message endianness

[Akihiko Odaki]

On 2025/05/02 12:04, Nicholas Piggin wrote:
> msix messages are written to memory in little-endian order, so they
> should not be byteswapped depending on target endianness, but read
> as le and converted to host endian by the qtest.
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Reviewed-by: Fabiano Rosas <farosas@suse.de>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>   tests/qtest/libqos/virtio-pci-modern.c | 4 +++-
>   tests/qtest/libqos/virtio-pci.c        | 6 ++++--
>   2 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/qtest/libqos/virtio-pci-modern.c b/tests/qtest/libqos/virtio-pci-modern.c
> index f31b3be656d..5dae41e6d74 100644
> --- a/tests/qtest/libqos/virtio-pci-modern.c
> +++ b/tests/qtest/libqos/virtio-pci-modern.c
> @@ -8,6 +8,7 @@
>    */
>   
>   #include "qemu/osdep.h"
> +#include "qemu/bswap.h"
>   #include "standard-headers/linux/pci_regs.h"
>   #include "standard-headers/linux/virtio_pci.h"
>   #include "standard-headers/linux/virtio_config.h"
> @@ -136,7 +137,8 @@ static bool get_msix_status(QVirtioPCIDevice *dev, uint32_t msix_entry,
>           return qpci_msix_pending(dev->pdev, msix_entry);
>       }
>   
> -    data = qtest_readl(dev->pdev->bus->qts, msix_addr);
> +    qtest_memread(dev->pdev->bus->qts, msix_addr, &data, 4);
> +    data = le32_to_cpu(data);
>       if (data == 0) {
>           return false;
>       }
> diff --git a/tests/qtest/libqos/virtio-pci.c b/tests/qtest/libqos/virtio-pci.c
> index 102e45b5248..76ea1f45ba9 100644
> --- a/tests/qtest/libqos/virtio-pci.c
> +++ b/tests/qtest/libqos/virtio-pci.c
> @@ -131,7 +131,8 @@ static bool qvirtio_pci_get_queue_isr_status(QVirtioDevice *d, QVirtQueue *vq)
>               /* No ISR checking should be done if masked, but read anyway */
>               return qpci_msix_pending(dev->pdev, vqpci->msix_entry);
>           } else {
> -            data = qtest_readl(dev->pdev->bus->qts, vqpci->msix_addr);
> +            qtest_memread(dev->pdev->bus->qts, vqpci->msix_addr, &data, 4);

It may be a bit nicer if 4 is replaced with with sizeof(data).

> +            data = le32_to_cpu(data);
>               if (data == 0) {
>                   return false;
>               }
> @@ -156,7 +157,8 @@ static bool qvirtio_pci_get_config_isr_status(QVirtioDevice *d)
>               /* No ISR checking should be done if masked, but read anyway */
>               return qpci_msix_pending(dev->pdev, dev->config_msix_entry);
>           } else {
> -            data = qtest_readl(dev->pdev->bus->qts, dev->config_msix_addr);
> +            qtest_memread(dev->pdev->bus->qts, dev->config_msix_addr, &data, 4);
> +            data = le32_to_cpu(data);
>               if (data == 0) {
>                   return false;
>               }

Re: [PATCH v5 02/11] tests/qtest: Fix virtio msix message endianness

[Nicholas Piggin]

On Mon May 5, 2025 at 3:05 PM AEST, Akihiko Odaki wrote:
> On 2025/05/02 12:04, Nicholas Piggin wrote:
>> msix messages are written to memory in little-endian order, so they
>> should not be byteswapped depending on target endianness, but read
>> as le and converted to host endian by the qtest.
>> 
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> Reviewed-by: Fabiano Rosas <farosas@suse.de>
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>>   tests/qtest/libqos/virtio-pci-modern.c | 4 +++-
>>   tests/qtest/libqos/virtio-pci.c        | 6 ++++--
>>   2 files changed, 7 insertions(+), 3 deletions(-)
>> 
>> diff --git a/tests/qtest/libqos/virtio-pci-modern.c b/tests/qtest/libqos/virtio-pci-modern.c
>> index f31b3be656d..5dae41e6d74 100644
>> --- a/tests/qtest/libqos/virtio-pci-modern.c
>> +++ b/tests/qtest/libqos/virtio-pci-modern.c
>> @@ -8,6 +8,7 @@
>>    */
>>   
>>   #include "qemu/osdep.h"
>> +#include "qemu/bswap.h"
>>   #include "standard-headers/linux/pci_regs.h"
>>   #include "standard-headers/linux/virtio_pci.h"
>>   #include "standard-headers/linux/virtio_config.h"
>> @@ -136,7 +137,8 @@ static bool get_msix_status(QVirtioPCIDevice *dev, uint32_t msix_entry,
>>           return qpci_msix_pending(dev->pdev, msix_entry);
>>       }
>>   
>> -    data = qtest_readl(dev->pdev->bus->qts, msix_addr);
>> +    qtest_memread(dev->pdev->bus->qts, msix_addr, &data, 4);
>> +    data = le32_to_cpu(data);
>>       if (data == 0) {
>>           return false;
>>       }
>> diff --git a/tests/qtest/libqos/virtio-pci.c b/tests/qtest/libqos/virtio-pci.c
>> index 102e45b5248..76ea1f45ba9 100644
>> --- a/tests/qtest/libqos/virtio-pci.c
>> +++ b/tests/qtest/libqos/virtio-pci.c
>> @@ -131,7 +131,8 @@ static bool qvirtio_pci_get_queue_isr_status(QVirtioDevice *d, QVirtQueue *vq)
>>               /* No ISR checking should be done if masked, but read anyway */
>>               return qpci_msix_pending(dev->pdev, vqpci->msix_entry);
>>           } else {
>> -            data = qtest_readl(dev->pdev->bus->qts, vqpci->msix_addr);
>> +            qtest_memread(dev->pdev->bus->qts, vqpci->msix_addr, &data, 4);
>
> It may be a bit nicer if 4 is replaced with with sizeof(data).

Sure.

Thanks,
Nick

[PATCH v5 03/11] tests/qtest: Add libqos function for testing msix interrupt status

[Nicholas Piggin]

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

Re: [PATCH v5 03/11] tests/qtest: Add libqos function for testing msix interrupt status

[Akihiko Odaki]

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;
>       }

Re: [PATCH v5 03/11] tests/qtest: Add libqos function for testing msix interrupt status

[Nicholas Piggin]

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

[PATCH v5 04/11] tests/qtest: Enable spapr dma with linear iommu map

[Nicholas Piggin]

qtests spapr dma was broken because the iommu was not set up.

spapr requires hypercalls to set up the iommu (TCE tables), but
there is no support for that or a side-channel to the iommu in
qtests at the moment, so add a quick workaround in QEMU to have
the spapr iommu provide a linear map to memory when running
qtests.

The buggy msix checks can all be removed since the tests all work
now.

Reviewed-by: Fabiano Rosas <farosas@suse.de>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 tests/qtest/libqos/pci.h             |  4 ----
 hw/ppc/spapr_iommu.c                 | 10 +++++++++-
 tests/qtest/e1000e-test.c            | 21 ---------------------
 tests/qtest/igb-test.c               | 21 ---------------------
 tests/qtest/libqos/generic-pcihost.c |  1 -
 tests/qtest/libqos/pci-pc.c          |  3 ---
 tests/qtest/libqos/pci-spapr.c       |  7 ++++---
 tests/qtest/libqos/pci.c             | 14 --------------
 tests/qtest/vhost-user-blk-test.c    |  6 ------
 tests/qtest/virtio-blk-test.c        | 12 ------------
 10 files changed, 13 insertions(+), 86 deletions(-)

diff --git a/tests/qtest/libqos/pci.h b/tests/qtest/libqos/pci.h
index 9f8f154c301..ef40a6917d3 100644
--- a/tests/qtest/libqos/pci.h
+++ b/tests/qtest/libqos/pci.h
@@ -51,7 +51,6 @@ struct QPCIBus {
     QTestState *qts;
     uint64_t pio_alloc_ptr, pio_limit;
     uint64_t mmio_alloc_ptr, mmio_limit;
-    bool has_buggy_msi; /* TRUE for spapr, FALSE for pci */
     bool not_hotpluggable; /* TRUE if devices cannot be hotplugged */
 
 };
@@ -83,9 +82,6 @@ QPCIDevice *qpci_device_find(QPCIBus *bus, int devfn);
 void qpci_device_init(QPCIDevice *dev, QPCIBus *bus, QPCIAddress *addr);
 int qpci_secondary_buses_init(QPCIBus *bus);
 
-bool qpci_has_buggy_msi(QPCIDevice *dev);
-bool qpci_check_buggy_msi(QPCIDevice *dev);
-
 void qpci_device_enable(QPCIDevice *dev);
 uint8_t qpci_find_capability(QPCIDevice *dev, uint8_t id, uint8_t start_addr);
 void qpci_msix_enable(QPCIDevice *dev);
diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c
index c2432a0c00c..c89ccc87d71 100644
--- a/hw/ppc/spapr_iommu.c
+++ b/hw/ppc/spapr_iommu.c
@@ -22,6 +22,8 @@
 #include "qemu/log.h"
 #include "qemu/module.h"
 #include "system/kvm.h"
+#include "system/qtest.h"
+#include "exec/target_page.h"
 #include "kvm_ppc.h"
 #include "migration/vmstate.h"
 #include "system/dma.h"
@@ -125,7 +127,13 @@ static IOMMUTLBEntry spapr_tce_translate_iommu(IOMMUMemoryRegion *iommu,
         .perm = IOMMU_NONE,
     };
 
-    if ((addr >> tcet->page_shift) < tcet->nb_table) {
+    if (qtest_enabled()) {
+        /* spapr qtests does not set up the IOMMU, shortcut a linear map */
+        ret.iova = addr & TARGET_PAGE_MASK;
+        ret.translated_addr = addr & TARGET_PAGE_MASK;
+        ret.addr_mask = ~TARGET_PAGE_MASK;
+        ret.perm = IOMMU_RW;
+    } else if ((addr >> tcet->page_shift) < tcet->nb_table) {
         /* Check if we are in bound */
         hwaddr page_mask = IOMMU_PAGE_MASK(tcet->page_shift);
 
diff --git a/tests/qtest/e1000e-test.c b/tests/qtest/e1000e-test.c
index de9738fdb74..8300bf5a5b3 100644
--- a/tests/qtest/e1000e-test.c
+++ b/tests/qtest/e1000e-test.c
@@ -139,13 +139,6 @@ static void test_e1000e_tx(void *obj, void *data, QGuestAllocator * alloc)
 {
     QE1000E_PCI *e1000e = obj;
     QE1000E *d = &e1000e->e1000e;
-    QOSGraphObject *e_object = obj;
-    QPCIDevice *dev = e_object->get_driver(e_object, "pci-device");
-
-    /* FIXME: add spapr support */
-    if (qpci_check_buggy_msi(dev)) {
-        return;
-    }
 
     e1000e_send_verify(d, data, alloc);
 }
@@ -154,13 +147,6 @@ static void test_e1000e_rx(void *obj, void *data, QGuestAllocator * alloc)
 {
     QE1000E_PCI *e1000e = obj;
     QE1000E *d = &e1000e->e1000e;
-    QOSGraphObject *e_object = obj;
-    QPCIDevice *dev = e_object->get_driver(e_object, "pci-device");
-
-    /* FIXME: add spapr support */
-    if (qpci_check_buggy_msi(dev)) {
-        return;
-    }
 
     e1000e_receive_verify(d, data, alloc);
 }
@@ -173,13 +159,6 @@ static void test_e1000e_multiple_transfers(void *obj, void *data,
 
     QE1000E_PCI *e1000e = obj;
     QE1000E *d = &e1000e->e1000e;
-    QOSGraphObject *e_object = obj;
-    QPCIDevice *dev = e_object->get_driver(e_object, "pci-device");
-
-    /* FIXME: add spapr support */
-    if (qpci_check_buggy_msi(dev)) {
-        return;
-    }
 
     for (i = 0; i < iterations; i++) {
         e1000e_send_verify(d, data, alloc);
diff --git a/tests/qtest/igb-test.c b/tests/qtest/igb-test.c
index 3d397ea6973..1b3b5aa6c76 100644
--- a/tests/qtest/igb-test.c
+++ b/tests/qtest/igb-test.c
@@ -142,13 +142,6 @@ static void test_igb_tx(void *obj, void *data, QGuestAllocator * alloc)
 {
     QE1000E_PCI *e1000e = obj;
     QE1000E *d = &e1000e->e1000e;
-    QOSGraphObject *e_object = obj;
-    QPCIDevice *dev = e_object->get_driver(e_object, "pci-device");
-
-    /* FIXME: add spapr support */
-    if (qpci_check_buggy_msi(dev)) {
-        return;
-    }
 
     igb_send_verify(d, data, alloc);
 }
@@ -157,13 +150,6 @@ static void test_igb_rx(void *obj, void *data, QGuestAllocator * alloc)
 {
     QE1000E_PCI *e1000e = obj;
     QE1000E *d = &e1000e->e1000e;
-    QOSGraphObject *e_object = obj;
-    QPCIDevice *dev = e_object->get_driver(e_object, "pci-device");
-
-    /* FIXME: add spapr support */
-    if (qpci_check_buggy_msi(dev)) {
-        return;
-    }
 
     igb_receive_verify(d, data, alloc);
 }
@@ -176,13 +162,6 @@ static void test_igb_multiple_transfers(void *obj, void *data,
 
     QE1000E_PCI *e1000e = obj;
     QE1000E *d = &e1000e->e1000e;
-    QOSGraphObject *e_object = obj;
-    QPCIDevice *dev = e_object->get_driver(e_object, "pci-device");
-
-    /* FIXME: add spapr support */
-    if (qpci_check_buggy_msi(dev)) {
-        return;
-    }
 
     for (i = 0; i < iterations; i++) {
         igb_send_verify(d, data, alloc);
diff --git a/tests/qtest/libqos/generic-pcihost.c b/tests/qtest/libqos/generic-pcihost.c
index 4bbeb5ff508..568897e0ecc 100644
--- a/tests/qtest/libqos/generic-pcihost.c
+++ b/tests/qtest/libqos/generic-pcihost.c
@@ -182,7 +182,6 @@ void qpci_init_generic(QGenericPCIBus *qpci, QTestState *qts,
 
     qpci->gpex_pio_base = 0x3eff0000;
     qpci->bus.not_hotpluggable = !hotpluggable;
-    qpci->bus.has_buggy_msi = false;
 
     qpci->bus.pio_readb = qpci_generic_pio_readb;
     qpci->bus.pio_readw = qpci_generic_pio_readw;
diff --git a/tests/qtest/libqos/pci-pc.c b/tests/qtest/libqos/pci-pc.c
index 147009f4f44..8b79d858bd5 100644
--- a/tests/qtest/libqos/pci-pc.c
+++ b/tests/qtest/libqos/pci-pc.c
@@ -124,9 +124,6 @@ void qpci_init_pc(QPCIBusPC *qpci, QTestState *qts, QGuestAllocator *alloc)
 {
     assert(qts);
 
-    /* tests can use pci-bus */
-    qpci->bus.has_buggy_msi = false;
-
     qpci->bus.pio_readb = qpci_pc_pio_readb;
     qpci->bus.pio_readw = qpci_pc_pio_readw;
     qpci->bus.pio_readl = qpci_pc_pio_readl;
diff --git a/tests/qtest/libqos/pci-spapr.c b/tests/qtest/libqos/pci-spapr.c
index 0f1023e4a73..dfa2087a599 100644
--- a/tests/qtest/libqos/pci-spapr.c
+++ b/tests/qtest/libqos/pci-spapr.c
@@ -20,6 +20,10 @@
  * PCI devices are always little-endian
  * SPAPR by default is big-endian
  * so PCI accessors need to swap data endianness
+ *
+ * The spapr iommu model has a qtest_enabled() check that short-cuts
+ * the TCE table and provides a linear map for DMA, since qtests does
+ * not have a way to make hcalls to set up the TCE table.
  */
 
 static uint8_t qpci_spapr_pio_readb(QPCIBus *bus, uint32_t addr)
@@ -155,9 +159,6 @@ void qpci_init_spapr(QPCIBusSPAPR *qpci, QTestState *qts,
 {
     assert(qts);
 
-    /* tests cannot use spapr, needs to be fixed first */
-    qpci->bus.has_buggy_msi = true;
-
     qpci->alloc = alloc;
 
     qpci->bus.pio_readb = qpci_spapr_pio_readb;
diff --git a/tests/qtest/libqos/pci.c b/tests/qtest/libqos/pci.c
index 773fd1fb6cf..2bae119bfca 100644
--- a/tests/qtest/libqos/pci.c
+++ b/tests/qtest/libqos/pci.c
@@ -53,20 +53,6 @@ void qpci_device_foreach(QPCIBus *bus, int vendor_id, int device_id,
     }
 }
 
-bool qpci_has_buggy_msi(QPCIDevice *dev)
-{
-    return dev->bus->has_buggy_msi;
-}
-
-bool qpci_check_buggy_msi(QPCIDevice *dev)
-{
-    if (qpci_has_buggy_msi(dev)) {
-        g_test_skip("Skipping due to incomplete support for MSI");
-        return true;
-    }
-    return false;
-}
-
 static void qpci_device_set(QPCIDevice *dev, QPCIBus *bus, int devfn)
 {
     g_assert(dev);
diff --git a/tests/qtest/vhost-user-blk-test.c b/tests/qtest/vhost-user-blk-test.c
index ea90d41232e..3e71fdb9d78 100644
--- a/tests/qtest/vhost-user-blk-test.c
+++ b/tests/qtest/vhost-user-blk-test.c
@@ -554,14 +554,8 @@ static void idx(void *obj, void *u_data, QGuestAllocator *t_alloc)
     uint32_t desc_idx;
     uint8_t status;
     char *data;
-    QOSGraphObject *blk_object = obj;
-    QPCIDevice *pci_dev = blk_object->get_driver(blk_object, "pci-device");
     QTestState *qts = global_qtest;
 
-    if (qpci_check_buggy_msi(pci_dev)) {
-        return;
-    }
-
     qpci_msix_enable(pdev->pdev);
     qvirtio_pci_set_msix_configuration_vector(pdev, t_alloc, 0);
 
diff --git a/tests/qtest/virtio-blk-test.c b/tests/qtest/virtio-blk-test.c
index 98c906ebb4a..3a005d600c1 100644
--- a/tests/qtest/virtio-blk-test.c
+++ b/tests/qtest/virtio-blk-test.c
@@ -474,14 +474,8 @@ static void msix(void *obj, void *u_data, QGuestAllocator *t_alloc)
     uint32_t free_head;
     uint8_t status;
     char *data;
-    QOSGraphObject *blk_object = obj;
-    QPCIDevice *pci_dev = blk_object->get_driver(blk_object, "pci-device");
     QTestState *qts = global_qtest;
 
-    if (qpci_check_buggy_msi(pci_dev)) {
-        return;
-    }
-
     qpci_msix_enable(pdev->pdev);
     qvirtio_pci_set_msix_configuration_vector(pdev, t_alloc, 0);
 
@@ -584,14 +578,8 @@ static void idx(void *obj, void *u_data, QGuestAllocator *t_alloc)
     uint32_t desc_idx;
     uint8_t status;
     char *data;
-    QOSGraphObject *blk_object = obj;
-    QPCIDevice *pci_dev = blk_object->get_driver(blk_object, "pci-device");
     QTestState *qts = global_qtest;
 
-    if (qpci_check_buggy_msi(pci_dev)) {
-        return;
-    }
-
     qpci_msix_enable(pdev->pdev);
     qvirtio_pci_set_msix_configuration_vector(pdev, t_alloc, 0);
 
-- 
2.47.1

Re: [PATCH v5 04/11] tests/qtest: Enable spapr dma with linear iommu map

[Alexander Mikhalitsyn]

Hi Nicholas,

I was working on a qtest for a NVMe emulated device [1] and noticed
that test fails on ppc64 with spapr bus. After small research I found out that
the issue is that pci_dma_read() fails on a perfectly valid physical address...

I wonder if you are going to continue working on this patchset?

It looks like this change is what I need to fix it (haven't yet tried, but happy to do so).

[1] https://lore.kernel.org/qemu-devel/20260526113301.91492-9-alexander@mihalicyn.com/

Kind regards,
Alex

On Fri, 2025-05-02 at 13:04 +1000, Nicholas Piggin wrote:
> qtests spapr dma was broken because the iommu was not set up.
> 
> spapr requires hypercalls to set up the iommu (TCE tables), but
> there is no support for that or a side-channel to the iommu in
> qtests at the moment, so add a quick workaround in QEMU to have
> the spapr iommu provide a linear map to memory when running
> qtests.
> 
> The buggy msix checks can all be removed since the tests all work
> now.
> 
> Reviewed-by: Fabiano Rosas <farosas@suse.de>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>  tests/qtest/libqos/pci.h             |  4 ----
>  hw/ppc/spapr_iommu.c                 | 10 +++++++++-
>  tests/qtest/e1000e-test.c            | 21 ---------------------
>  tests/qtest/igb-test.c               | 21 ---------------------
>  tests/qtest/libqos/generic-pcihost.c |  1 -
>  tests/qtest/libqos/pci-pc.c          |  3 ---
>  tests/qtest/libqos/pci-spapr.c       |  7 ++++---
>  tests/qtest/libqos/pci.c             | 14 --------------
>  tests/qtest/vhost-user-blk-test.c    |  6 ------
>  tests/qtest/virtio-blk-test.c        | 12 ------------
>  10 files changed, 13 insertions(+), 86 deletions(-)
> 
> diff --git a/tests/qtest/libqos/pci.h b/tests/qtest/libqos/pci.h
> index 9f8f154c301..ef40a6917d3 100644
> --- a/tests/qtest/libqos/pci.h
> +++ b/tests/qtest/libqos/pci.h
> @@ -51,7 +51,6 @@ struct QPCIBus {
>      QTestState *qts;
>      uint64_t pio_alloc_ptr, pio_limit;
>      uint64_t mmio_alloc_ptr, mmio_limit;
> -    bool has_buggy_msi; /* TRUE for spapr, FALSE for pci */
>      bool not_hotpluggable; /* TRUE if devices cannot be hotplugged */
>  
>  };
> @@ -83,9 +82,6 @@ QPCIDevice *qpci_device_find(QPCIBus *bus, int devfn);
>  void qpci_device_init(QPCIDevice *dev, QPCIBus *bus, QPCIAddress *addr);
>  int qpci_secondary_buses_init(QPCIBus *bus);
>  
> -bool qpci_has_buggy_msi(QPCIDevice *dev);
> -bool qpci_check_buggy_msi(QPCIDevice *dev);
> -
>  void qpci_device_enable(QPCIDevice *dev);
>  uint8_t qpci_find_capability(QPCIDevice *dev, uint8_t id, uint8_t start_addr);
>  void qpci_msix_enable(QPCIDevice *dev);
> diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c
> index c2432a0c00c..c89ccc87d71 100644
> --- a/hw/ppc/spapr_iommu.c
> +++ b/hw/ppc/spapr_iommu.c
> @@ -22,6 +22,8 @@
>  #include "qemu/log.h"
>  #include "qemu/module.h"
>  #include "system/kvm.h"
> +#include "system/qtest.h"
> +#include "exec/target_page.h"
>  #include "kvm_ppc.h"
>  #include "migration/vmstate.h"
>  #include "system/dma.h"
> @@ -125,7 +127,13 @@ static IOMMUTLBEntry spapr_tce_translate_iommu(IOMMUMemoryRegion *iommu,
>          .perm = IOMMU_NONE,
>      };
>  
> -    if ((addr >> tcet->page_shift) < tcet->nb_table) {
> +    if (qtest_enabled()) {
> +        /* spapr qtests does not set up the IOMMU, shortcut a linear map */
> +        ret.iova = addr & TARGET_PAGE_MASK;
> +        ret.translated_addr = addr & TARGET_PAGE_MASK;
> +        ret.addr_mask = ~TARGET_PAGE_MASK;
> +        ret.perm = IOMMU_RW;
> +    } else if ((addr >> tcet->page_shift) < tcet->nb_table) {
>          /* Check if we are in bound */
>          hwaddr page_mask = IOMMU_PAGE_MASK(tcet->page_shift);
>  
> diff --git a/tests/qtest/e1000e-test.c b/tests/qtest/e1000e-test.c
> index de9738fdb74..8300bf5a5b3 100644
> --- a/tests/qtest/e1000e-test.c
> +++ b/tests/qtest/e1000e-test.c
> @@ -139,13 +139,6 @@ static void test_e1000e_tx(void *obj, void *data, QGuestAllocator * alloc)
>  {
>      QE1000E_PCI *e1000e = obj;
>      QE1000E *d = &e1000e->e1000e;
> -    QOSGraphObject *e_object = obj;
> -    QPCIDevice *dev = e_object->get_driver(e_object, "pci-device");
> -
> -    /* FIXME: add spapr support */
> -    if (qpci_check_buggy_msi(dev)) {
> -        return;
> -    }
>  
>      e1000e_send_verify(d, data, alloc);
>  }
> @@ -154,13 +147,6 @@ static void test_e1000e_rx(void *obj, void *data, QGuestAllocator * alloc)
>  {
>      QE1000E_PCI *e1000e = obj;
>      QE1000E *d = &e1000e->e1000e;
> -    QOSGraphObject *e_object = obj;
> -    QPCIDevice *dev = e_object->get_driver(e_object, "pci-device");
> -
> -    /* FIXME: add spapr support */
> -    if (qpci_check_buggy_msi(dev)) {
> -        return;
> -    }
>  
>      e1000e_receive_verify(d, data, alloc);
>  }
> @@ -173,13 +159,6 @@ static void test_e1000e_multiple_transfers(void *obj, void *data,
>  
>      QE1000E_PCI *e1000e = obj;
>      QE1000E *d = &e1000e->e1000e;
> -    QOSGraphObject *e_object = obj;
> -    QPCIDevice *dev = e_object->get_driver(e_object, "pci-device");
> -
> -    /* FIXME: add spapr support */
> -    if (qpci_check_buggy_msi(dev)) {
> -        return;
> -    }
>  
>      for (i = 0; i < iterations; i++) {
>          e1000e_send_verify(d, data, alloc);
> diff --git a/tests/qtest/igb-test.c b/tests/qtest/igb-test.c
> index 3d397ea6973..1b3b5aa6c76 100644
> --- a/tests/qtest/igb-test.c
> +++ b/tests/qtest/igb-test.c
> @@ -142,13 +142,6 @@ static void test_igb_tx(void *obj, void *data, QGuestAllocator * alloc)
>  {
>      QE1000E_PCI *e1000e = obj;
>      QE1000E *d = &e1000e->e1000e;
> -    QOSGraphObject *e_object = obj;
> -    QPCIDevice *dev = e_object->get_driver(e_object, "pci-device");
> -
> -    /* FIXME: add spapr support */
> -    if (qpci_check_buggy_msi(dev)) {
> -        return;
> -    }
>  
>      igb_send_verify(d, data, alloc);
>  }
> @@ -157,13 +150,6 @@ static void test_igb_rx(void *obj, void *data, QGuestAllocator * alloc)
>  {
>      QE1000E_PCI *e1000e = obj;
>      QE1000E *d = &e1000e->e1000e;
> -    QOSGraphObject *e_object = obj;
> -    QPCIDevice *dev = e_object->get_driver(e_object, "pci-device");
> -
> -    /* FIXME: add spapr support */
> -    if (qpci_check_buggy_msi(dev)) {
> -        return;
> -    }
>  
>      igb_receive_verify(d, data, alloc);
>  }
> @@ -176,13 +162,6 @@ static void test_igb_multiple_transfers(void *obj, void *data,
>  
>      QE1000E_PCI *e1000e = obj;
>      QE1000E *d = &e1000e->e1000e;
> -    QOSGraphObject *e_object = obj;
> -    QPCIDevice *dev = e_object->get_driver(e_object, "pci-device");
> -
> -    /* FIXME: add spapr support */
> -    if (qpci_check_buggy_msi(dev)) {
> -        return;
> -    }
>  
>      for (i = 0; i < iterations; i++) {
>          igb_send_verify(d, data, alloc);
> diff --git a/tests/qtest/libqos/generic-pcihost.c b/tests/qtest/libqos/generic-pcihost.c
> index 4bbeb5ff508..568897e0ecc 100644
> --- a/tests/qtest/libqos/generic-pcihost.c
> +++ b/tests/qtest/libqos/generic-pcihost.c
> @@ -182,7 +182,6 @@ void qpci_init_generic(QGenericPCIBus *qpci, QTestState *qts,
>  
>      qpci->gpex_pio_base = 0x3eff0000;
>      qpci->bus.not_hotpluggable = !hotpluggable;
> -    qpci->bus.has_buggy_msi = false;
>  
>      qpci->bus.pio_readb = qpci_generic_pio_readb;
>      qpci->bus.pio_readw = qpci_generic_pio_readw;
> diff --git a/tests/qtest/libqos/pci-pc.c b/tests/qtest/libqos/pci-pc.c
> index 147009f4f44..8b79d858bd5 100644
> --- a/tests/qtest/libqos/pci-pc.c
> +++ b/tests/qtest/libqos/pci-pc.c
> @@ -124,9 +124,6 @@ void qpci_init_pc(QPCIBusPC *qpci, QTestState *qts, QGuestAllocator *alloc)
>  {
>      assert(qts);
>  
> -    /* tests can use pci-bus */
> -    qpci->bus.has_buggy_msi = false;
> -
>      qpci->bus.pio_readb = qpci_pc_pio_readb;
>      qpci->bus.pio_readw = qpci_pc_pio_readw;
>      qpci->bus.pio_readl = qpci_pc_pio_readl;
> diff --git a/tests/qtest/libqos/pci-spapr.c b/tests/qtest/libqos/pci-spapr.c
> index 0f1023e4a73..dfa2087a599 100644
> --- a/tests/qtest/libqos/pci-spapr.c
> +++ b/tests/qtest/libqos/pci-spapr.c
> @@ -20,6 +20,10 @@
>   * PCI devices are always little-endian
>   * SPAPR by default is big-endian
>   * so PCI accessors need to swap data endianness
> + *
> + * The spapr iommu model has a qtest_enabled() check that short-cuts
> + * the TCE table and provides a linear map for DMA, since qtests does
> + * not have a way to make hcalls to set up the TCE table.
>   */
>  
>  static uint8_t qpci_spapr_pio_readb(QPCIBus *bus, uint32_t addr)
> @@ -155,9 +159,6 @@ void qpci_init_spapr(QPCIBusSPAPR *qpci, QTestState *qts,
>  {
>      assert(qts);
>  
> -    /* tests cannot use spapr, needs to be fixed first */
> -    qpci->bus.has_buggy_msi = true;
> -
>      qpci->alloc = alloc;
>  
>      qpci->bus.pio_readb = qpci_spapr_pio_readb;
> diff --git a/tests/qtest/libqos/pci.c b/tests/qtest/libqos/pci.c
> index 773fd1fb6cf..2bae119bfca 100644
> --- a/tests/qtest/libqos/pci.c
> +++ b/tests/qtest/libqos/pci.c
> @@ -53,20 +53,6 @@ void qpci_device_foreach(QPCIBus *bus, int vendor_id, int device_id,
>      }
>  }
>  
> -bool qpci_has_buggy_msi(QPCIDevice *dev)
> -{
> -    return dev->bus->has_buggy_msi;
> -}
> -
> -bool qpci_check_buggy_msi(QPCIDevice *dev)
> -{
> -    if (qpci_has_buggy_msi(dev)) {
> -        g_test_skip("Skipping due to incomplete support for MSI");
> -        return true;
> -    }
> -    return false;
> -}
> -
>  static void qpci_device_set(QPCIDevice *dev, QPCIBus *bus, int devfn)
>  {
>      g_assert(dev);
> diff --git a/tests/qtest/vhost-user-blk-test.c b/tests/qtest/vhost-user-blk-test.c
> index ea90d41232e..3e71fdb9d78 100644
> --- a/tests/qtest/vhost-user-blk-test.c
> +++ b/tests/qtest/vhost-user-blk-test.c
> @@ -554,14 +554,8 @@ static void idx(void *obj, void *u_data, QGuestAllocator *t_alloc)
>      uint32_t desc_idx;
>      uint8_t status;
>      char *data;
> -    QOSGraphObject *blk_object = obj;
> -    QPCIDevice *pci_dev = blk_object->get_driver(blk_object, "pci-device");
>      QTestState *qts = global_qtest;
>  
> -    if (qpci_check_buggy_msi(pci_dev)) {
> -        return;
> -    }
> -
>      qpci_msix_enable(pdev->pdev);
>      qvirtio_pci_set_msix_configuration_vector(pdev, t_alloc, 0);
>  
> diff --git a/tests/qtest/virtio-blk-test.c b/tests/qtest/virtio-blk-test.c
> index 98c906ebb4a..3a005d600c1 100644
> --- a/tests/qtest/virtio-blk-test.c
> +++ b/tests/qtest/virtio-blk-test.c
> @@ -474,14 +474,8 @@ static void msix(void *obj, void *u_data, QGuestAllocator *t_alloc)
>      uint32_t free_head;
>      uint8_t status;
>      char *data;
> -    QOSGraphObject *blk_object = obj;
> -    QPCIDevice *pci_dev = blk_object->get_driver(blk_object, "pci-device");
>      QTestState *qts = global_qtest;
>  
> -    if (qpci_check_buggy_msi(pci_dev)) {
> -        return;
> -    }
> -
>      qpci_msix_enable(pdev->pdev);
>      qvirtio_pci_set_msix_configuration_vector(pdev, t_alloc, 0);
>  
> @@ -584,14 +578,8 @@ static void idx(void *obj, void *u_data, QGuestAllocator *t_alloc)
>      uint32_t desc_idx;
>      uint8_t status;
>      char *data;
> -    QOSGraphObject *blk_object = obj;
> -    QPCIDevice *pci_dev = blk_object->get_driver(blk_object, "pci-device");
>      QTestState *qts = global_qtest;
>  
> -    if (qpci_check_buggy_msi(pci_dev)) {
> -        return;
> -    }
> -
>      qpci_msix_enable(pdev->pdev);
>      qvirtio_pci_set_msix_configuration_vector(pdev, t_alloc, 0);
>  

[PATCH v5 05/11] tests/qtest/ahci: unmap pci bar before reusing device

[Nicholas Piggin]

ahci-test double-maps the hba bar in the pending_callback test.
Unmap it first, to keep iomaps balanced.

Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
Cc: Akihiko Odaki <akihiko.odaki@daynix.com>
Reviewed-by: Akihiko Odaki <akihiko.odaki@daynix.com>
Reviewed-by: Fabiano Rosas <farosas@suse.de>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 tests/qtest/libqos/ahci.h |  2 ++
 tests/qtest/ahci-test.c   |  4 ++++
 tests/qtest/libqos/ahci.c | 11 +++++++++++
 3 files changed, 17 insertions(+)

diff --git a/tests/qtest/libqos/ahci.h b/tests/qtest/libqos/ahci.h
index a0487a1557d..f610bd32a5f 100644
--- a/tests/qtest/libqos/ahci.h
+++ b/tests/qtest/libqos/ahci.h
@@ -574,7 +574,9 @@ void ahci_clean_mem(AHCIQState *ahci);
 QPCIDevice *get_ahci_device(QTestState *qts, uint32_t *fingerprint);
 void free_ahci_device(QPCIDevice *dev);
 void ahci_pci_enable(AHCIQState *ahci);
+void ahci_pci_disable(AHCIQState *ahci);
 void start_ahci_device(AHCIQState *ahci);
+void stop_ahci_device(AHCIQState *ahci);
 void ahci_hba_enable(AHCIQState *ahci);
 
 /* Port Management */
diff --git a/tests/qtest/ahci-test.c b/tests/qtest/ahci-test.c
index e8aabfc13f5..36caa7b2999 100644
--- a/tests/qtest/ahci-test.c
+++ b/tests/qtest/ahci-test.c
@@ -198,6 +198,7 @@ static void ahci_shutdown(AHCIQState *ahci)
 {
     QOSState *qs = ahci->parent;
 
+    ahci_pci_disable(ahci);
     ahci_clean_mem(ahci);
     free_ahci_device(ahci->dev);
     g_free(ahci);
@@ -1418,6 +1419,7 @@ static void test_reset(void)
                                CMD_READ_DMA_EXT,
                                CMD_WRITE_DMA_EXT);
         ahci_set(ahci, AHCI_GHC, AHCI_GHC_HR);
+        stop_ahci_device(ahci);
         ahci_clean_mem(ahci);
     }
 
@@ -1484,6 +1486,7 @@ static void test_reset_pending_callback(void)
     sleep(1);
 
     /* Start again. */
+    stop_ahci_device(ahci);
     ahci_clean_mem(ahci);
     ahci_pci_enable(ahci);
     ahci_hba_enable(ahci);
@@ -1502,6 +1505,7 @@ static void test_reset_pending_callback(void)
     ahci_free(ahci, ptr1);
     ahci_free(ahci, ptr2);
 
+    stop_ahci_device(ahci);
     ahci_clean_mem(ahci);
 
     ahci_shutdown(ahci);
diff --git a/tests/qtest/libqos/ahci.c b/tests/qtest/libqos/ahci.c
index 34a75b7f43b..bd1994a9208 100644
--- a/tests/qtest/libqos/ahci.c
+++ b/tests/qtest/libqos/ahci.c
@@ -205,6 +205,11 @@ void ahci_pci_enable(AHCIQState *ahci)
 
 }
 
+void ahci_pci_disable(AHCIQState *ahci)
+{
+    stop_ahci_device(ahci);
+}
+
 /**
  * Map BAR5/ABAR, and engage the PCI device.
  */
@@ -217,6 +222,12 @@ void start_ahci_device(AHCIQState *ahci)
     qpci_device_enable(ahci->dev);
 }
 
+void stop_ahci_device(AHCIQState *ahci)
+{
+    /* Unmap AHCI's ABAR */
+    qpci_iounmap(ahci->dev, ahci->hba_bar);
+}
+
 /**
  * Test and initialize the AHCI's HBA memory areas.
  * Initialize and start any ports with devices attached.
-- 
2.47.1

Re: [PATCH v5 05/11] tests/qtest/ahci: unmap pci bar before reusing device

[Akihiko Odaki]

On 2025/05/02 12:04, Nicholas Piggin wrote:
> ahci-test double-maps the hba bar in the pending_callback test.
> Unmap it first, to keep iomaps balanced.
> 
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> Cc: Akihiko Odaki <akihiko.odaki@daynix.com>
> Reviewed-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> Reviewed-by: Fabiano Rosas <farosas@suse.de>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>   tests/qtest/libqos/ahci.h |  2 ++
>   tests/qtest/ahci-test.c   |  4 ++++
>   tests/qtest/libqos/ahci.c | 11 +++++++++++
>   3 files changed, 17 insertions(+)
> 
> diff --git a/tests/qtest/libqos/ahci.h b/tests/qtest/libqos/ahci.h
> index a0487a1557d..f610bd32a5f 100644
> --- a/tests/qtest/libqos/ahci.h
> +++ b/tests/qtest/libqos/ahci.h
> @@ -574,7 +574,9 @@ void ahci_clean_mem(AHCIQState *ahci);
>   QPCIDevice *get_ahci_device(QTestState *qts, uint32_t *fingerprint);
>   void free_ahci_device(QPCIDevice *dev);
>   void ahci_pci_enable(AHCIQState *ahci);
> +void ahci_pci_disable(AHCIQState *ahci);
>   void start_ahci_device(AHCIQState *ahci);
> +void stop_ahci_device(AHCIQState *ahci);
>   void ahci_hba_enable(AHCIQState *ahci);
>   
>   /* Port Management */
> diff --git a/tests/qtest/ahci-test.c b/tests/qtest/ahci-test.c
> index e8aabfc13f5..36caa7b2999 100644
> --- a/tests/qtest/ahci-test.c
> +++ b/tests/qtest/ahci-test.c
> @@ -198,6 +198,7 @@ static void ahci_shutdown(AHCIQState *ahci)
>   {
>       QOSState *qs = ahci->parent;
>   
> +    ahci_pci_disable(ahci);
>       ahci_clean_mem(ahci);
>       free_ahci_device(ahci->dev);
>       g_free(ahci);
> @@ -1418,6 +1419,7 @@ static void test_reset(void)
>                                  CMD_READ_DMA_EXT,
>                                  CMD_WRITE_DMA_EXT);
>           ahci_set(ahci, AHCI_GHC, AHCI_GHC_HR);
> +        stop_ahci_device(ahci);
>           ahci_clean_mem(ahci);
>       }
>   
> @@ -1484,6 +1486,7 @@ static void test_reset_pending_callback(void)
>       sleep(1);
>   
>       /* Start again. */
> +    stop_ahci_device(ahci);
>       ahci_clean_mem(ahci);
>       ahci_pci_enable(ahci);
>       ahci_hba_enable(ahci);
> @@ -1502,6 +1505,7 @@ static void test_reset_pending_callback(void)
>       ahci_free(ahci, ptr1);
>       ahci_free(ahci, ptr2);
>   
> +    stop_ahci_device(ahci);

This stop_ahci_device() call is unnecessary since ahci_shutdown() follows.

>       ahci_clean_mem(ahci);
>   
>       ahci_shutdown(ahci);
> diff --git a/tests/qtest/libqos/ahci.c b/tests/qtest/libqos/ahci.c
> index 34a75b7f43b..bd1994a9208 100644
> --- a/tests/qtest/libqos/ahci.c
> +++ b/tests/qtest/libqos/ahci.c
> @@ -205,6 +205,11 @@ void ahci_pci_enable(AHCIQState *ahci)
>   
>   }
>   
> +void ahci_pci_disable(AHCIQState *ahci)
> +{
> +    stop_ahci_device(ahci);
> +}
> +
>   /**
>    * Map BAR5/ABAR, and engage the PCI device.
>    */
> @@ -217,6 +222,12 @@ void start_ahci_device(AHCIQState *ahci)
>       qpci_device_enable(ahci->dev);
>   }
>   
> +void stop_ahci_device(AHCIQState *ahci)
> +{
> +    /* Unmap AHCI's ABAR */
> +    qpci_iounmap(ahci->dev, ahci->hba_bar);
> +}
> +
>   /**
>    * Test and initialize the AHCI's HBA memory areas.
>    * Initialize and start any ports with devices attached.

[PATCH v5 06/11] tests/qtest/ahci: don't unmap pci bar if it wasn't mapped

[Nicholas Piggin]

ahci-test has a bunch of tests where the pci bar was not mapped. Avoid
unmapping it in these cases, to keep iomaps balanced.

Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
Cc: Akihiko Odaki <akihiko.odaki@daynix.com>
Cc: Fabiano Rosas <farosas@suse.de>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 tests/qtest/libqos/ahci.h | 1 +
 tests/qtest/ahci-test.c   | 7 ++++++-
 tests/qtest/libqos/ahci.c | 9 +++++++++
 3 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/tests/qtest/libqos/ahci.h b/tests/qtest/libqos/ahci.h
index f610bd32a5f..d639692aac4 100644
--- a/tests/qtest/libqos/ahci.h
+++ b/tests/qtest/libqos/ahci.h
@@ -342,6 +342,7 @@ typedef struct AHCIQState {
     uint32_t cap;
     uint32_t cap2;
     AHCIPortQState port[32];
+    bool pci_enabled;
     bool enabled;
 } AHCIQState;
 
diff --git a/tests/qtest/ahci-test.c b/tests/qtest/ahci-test.c
index 36caa7b2999..7d5f73ac40b 100644
--- a/tests/qtest/ahci-test.c
+++ b/tests/qtest/ahci-test.c
@@ -85,6 +85,8 @@ static void verify_state(AHCIQState *ahci, uint64_t hba_old)
     uint64_t hba_base;
     AHCICommandHeader cmd;
 
+    g_assert_cmphex(ahci->hba_bar.addr, ==, hba_old);
+
     ahci_fingerprint = qpci_config_readl(ahci->dev, PCI_VENDOR_ID);
     g_assert_cmphex(ahci_fingerprint, ==, ahci->fingerprint);
 
@@ -198,7 +200,9 @@ static void ahci_shutdown(AHCIQState *ahci)
 {
     QOSState *qs = ahci->parent;
 
-    ahci_pci_disable(ahci);
+    if (ahci->pci_enabled) {
+        ahci_pci_disable(ahci);
+    }
     ahci_clean_mem(ahci);
     free_ahci_device(ahci->dev);
     g_free(ahci);
@@ -1421,6 +1425,7 @@ static void test_reset(void)
         ahci_set(ahci, AHCI_GHC, AHCI_GHC_HR);
         stop_ahci_device(ahci);
         ahci_clean_mem(ahci);
+        start_ahci_device(ahci);
     }
 
     ahci_shutdown(ahci);
diff --git a/tests/qtest/libqos/ahci.c b/tests/qtest/libqos/ahci.c
index bd1994a9208..cc4f5b7b534 100644
--- a/tests/qtest/libqos/ahci.c
+++ b/tests/qtest/libqos/ahci.c
@@ -215,17 +215,25 @@ void ahci_pci_disable(AHCIQState *ahci)
  */
 void start_ahci_device(AHCIQState *ahci)
 {
+    g_assert(!ahci->pci_enabled);
+
     /* Map AHCI's ABAR (BAR5) */
     ahci->hba_bar = qpci_iomap(ahci->dev, 5, &ahci->barsize);
 
     /* turns on pci.cmd.iose, pci.cmd.mse and pci.cmd.bme */
     qpci_device_enable(ahci->dev);
+
+    ahci->pci_enabled = true;
 }
 
 void stop_ahci_device(AHCIQState *ahci)
 {
+    g_assert(ahci->pci_enabled);
+
     /* Unmap AHCI's ABAR */
     qpci_iounmap(ahci->dev, ahci->hba_bar);
+
+    ahci->pci_enabled = false;
 }
 
 /**
@@ -249,6 +257,7 @@ void ahci_hba_enable(AHCIQState *ahci)
     uint8_t num_cmd_slots;
 
     g_assert(ahci != NULL);
+    g_assert(ahci->pci_enabled);
 
     /* Set GHC.AE to 1 */
     ahci_set(ahci, AHCI_GHC, AHCI_GHC_AE);
-- 
2.47.1

Re: [PATCH v5 06/11] tests/qtest/ahci: don't unmap pci bar if it wasn't mapped

[Akihiko Odaki]

On 2025/05/02 12:04, Nicholas Piggin wrote:
> ahci-test has a bunch of tests where the pci bar was not mapped. Avoid
> unmapping it in these cases, to keep iomaps balanced.
> 
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> Cc: Akihiko Odaki <akihiko.odaki@daynix.com>
> Cc: Fabiano Rosas <farosas@suse.de>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>   tests/qtest/libqos/ahci.h | 1 +
>   tests/qtest/ahci-test.c   | 7 ++++++-
>   tests/qtest/libqos/ahci.c | 9 +++++++++
>   3 files changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/qtest/libqos/ahci.h b/tests/qtest/libqos/ahci.h
> index f610bd32a5f..d639692aac4 100644
> --- a/tests/qtest/libqos/ahci.h
> +++ b/tests/qtest/libqos/ahci.h
> @@ -342,6 +342,7 @@ typedef struct AHCIQState {
>       uint32_t cap;
>       uint32_t cap2;
>       AHCIPortQState port[32];
> +    bool pci_enabled;

The following patch also adds a similar variable for virtio and has a 
slightly different semantics; qvirtio_pci_device_disable() is no-op but 
ahci_pci_disable() aborts when no-op.

A bool flag can be added to QPCIBar instead so that we can enforce the 
"no-op if not mapped" semantics everywhere consistently with less code.

>       bool enabled;
>   } AHCIQState;
>   
> diff --git a/tests/qtest/ahci-test.c b/tests/qtest/ahci-test.c
> index 36caa7b2999..7d5f73ac40b 100644
> --- a/tests/qtest/ahci-test.c
> +++ b/tests/qtest/ahci-test.c
> @@ -85,6 +85,8 @@ static void verify_state(AHCIQState *ahci, uint64_t hba_old)
>       uint64_t hba_base;
>       AHCICommandHeader cmd;
>   
> +    g_assert_cmphex(ahci->hba_bar.addr, ==, hba_old);
> +
>       ahci_fingerprint = qpci_config_readl(ahci->dev, PCI_VENDOR_ID);
>       g_assert_cmphex(ahci_fingerprint, ==, ahci->fingerprint);
>   
> @@ -198,7 +200,9 @@ static void ahci_shutdown(AHCIQState *ahci)
>   {
>       QOSState *qs = ahci->parent;
>   
> -    ahci_pci_disable(ahci);
> +    if (ahci->pci_enabled) {
> +        ahci_pci_disable(ahci);
> +    }
>       ahci_clean_mem(ahci);
>       free_ahci_device(ahci->dev);
>       g_free(ahci);
> @@ -1421,6 +1425,7 @@ static void test_reset(void)
>           ahci_set(ahci, AHCI_GHC, AHCI_GHC_HR);
>           stop_ahci_device(ahci);
>           ahci_clean_mem(ahci);
> +        start_ahci_device(ahci);
>       }
>   
>       ahci_shutdown(ahci);
> diff --git a/tests/qtest/libqos/ahci.c b/tests/qtest/libqos/ahci.c
> index bd1994a9208..cc4f5b7b534 100644
> --- a/tests/qtest/libqos/ahci.c
> +++ b/tests/qtest/libqos/ahci.c
> @@ -215,17 +215,25 @@ void ahci_pci_disable(AHCIQState *ahci)
>    */
>   void start_ahci_device(AHCIQState *ahci)
>   {
> +    g_assert(!ahci->pci_enabled);
> +
>       /* Map AHCI's ABAR (BAR5) */
>       ahci->hba_bar = qpci_iomap(ahci->dev, 5, &ahci->barsize);
>   
>       /* turns on pci.cmd.iose, pci.cmd.mse and pci.cmd.bme */
>       qpci_device_enable(ahci->dev);
> +
> +    ahci->pci_enabled = true;
>   }
>   
>   void stop_ahci_device(AHCIQState *ahci)
>   {
> +    g_assert(ahci->pci_enabled);
> +
>       /* Unmap AHCI's ABAR */
>       qpci_iounmap(ahci->dev, ahci->hba_bar);
> +
> +    ahci->pci_enabled = false;
>   }
>   
>   /**
> @@ -249,6 +257,7 @@ void ahci_hba_enable(AHCIQState *ahci)
>       uint8_t num_cmd_slots;
>   
>       g_assert(ahci != NULL);
> +    g_assert(ahci->pci_enabled);
>   
>       /* Set GHC.AE to 1 */
>       ahci_set(ahci, AHCI_GHC, AHCI_GHC_AE);

Re: [PATCH v5 06/11] tests/qtest/ahci: don't unmap pci bar if it wasn't mapped

[Nicholas Piggin]

On Mon May 5, 2025 at 3:25 PM AEST, Akihiko Odaki wrote:
> On 2025/05/02 12:04, Nicholas Piggin wrote:
>> ahci-test has a bunch of tests where the pci bar was not mapped. Avoid
>> unmapping it in these cases, to keep iomaps balanced.
>> 
>> Cc: Michael S. Tsirkin <mst@redhat.com>
>> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
>> Cc: Akihiko Odaki <akihiko.odaki@daynix.com>
>> Cc: Fabiano Rosas <farosas@suse.de>
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>>   tests/qtest/libqos/ahci.h | 1 +
>>   tests/qtest/ahci-test.c   | 7 ++++++-
>>   tests/qtest/libqos/ahci.c | 9 +++++++++
>>   3 files changed, 16 insertions(+), 1 deletion(-)
>> 
>> diff --git a/tests/qtest/libqos/ahci.h b/tests/qtest/libqos/ahci.h
>> index f610bd32a5f..d639692aac4 100644
>> --- a/tests/qtest/libqos/ahci.h
>> +++ b/tests/qtest/libqos/ahci.h
>> @@ -342,6 +342,7 @@ typedef struct AHCIQState {
>>       uint32_t cap;
>>       uint32_t cap2;
>>       AHCIPortQState port[32];
>> +    bool pci_enabled;
>
> The following patch also adds a similar variable for virtio and has a 
> slightly different semantics; qvirtio_pci_device_disable() is no-op but 
> ahci_pci_disable() aborts when no-op.
>
> A bool flag can be added to QPCIBar instead so that we can enforce the 
> "no-op if not mapped" semantics everywhere consistently with less code.

Yeah that might be a good idea. I'll try out that suggestion and see
how it looks.

Thanks,
Nick

Re: [PATCH v5 06/11] tests/qtest/ahci: don't unmap pci bar if it wasn't mapped

[Nicholas Piggin]

On Mon May 5, 2025 at 3:25 PM AEST, Akihiko Odaki wrote:
> On 2025/05/02 12:04, Nicholas Piggin wrote:
>> ahci-test has a bunch of tests where the pci bar was not mapped. Avoid
>> unmapping it in these cases, to keep iomaps balanced.
>> 
>> Cc: Michael S. Tsirkin <mst@redhat.com>
>> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
>> Cc: Akihiko Odaki <akihiko.odaki@daynix.com>
>> Cc: Fabiano Rosas <farosas@suse.de>
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>>   tests/qtest/libqos/ahci.h | 1 +
>>   tests/qtest/ahci-test.c   | 7 ++++++-
>>   tests/qtest/libqos/ahci.c | 9 +++++++++
>>   3 files changed, 16 insertions(+), 1 deletion(-)
>> 
>> diff --git a/tests/qtest/libqos/ahci.h b/tests/qtest/libqos/ahci.h
>> index f610bd32a5f..d639692aac4 100644
>> --- a/tests/qtest/libqos/ahci.h
>> +++ b/tests/qtest/libqos/ahci.h
>> @@ -342,6 +342,7 @@ typedef struct AHCIQState {
>>       uint32_t cap;
>>       uint32_t cap2;
>>       AHCIPortQState port[32];
>> +    bool pci_enabled;
>
> The following patch also adds a similar variable for virtio and has a 
> slightly different semantics; qvirtio_pci_device_disable() is no-op but 
> ahci_pci_disable() aborts when no-op.
>
> A bool flag can be added to QPCIBar instead so that we can enforce the 
> "no-op if not mapped" semantics everywhere consistently with less code.

Now I think about it, the reason for the patch in the first place is
to ensure tests balance their maps and unmaps.

If we want to just keep allowing iounmap() of not-mapped bar to simplify
error handling and cleanup cases that's fine, I think I can just drop
these patches and remove assert in iounmap.

Thanks,
Nick

[PATCH v5 07/11] tests/qtest/libquos/virtio: unmap pci bar when disabling device

[Nicholas Piggin]

Unmap the virtio-pci bar in qvirtio_pci_disable_device() to keep
iomap/iounmap balanced.

Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
Reviewed-by: Akihiko Odaki <akihiko.odaki@daynix.com>
Reviewed-by: Fabiano Rosas <farosas@suse.de>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 tests/qtest/libqos/virtio-pci.h |  1 +
 tests/qtest/libqos/virtio-pci.c | 11 ++++++++++-
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/tests/qtest/libqos/virtio-pci.h b/tests/qtest/libqos/virtio-pci.h
index f5115cacba2..efdf904b254 100644
--- a/tests/qtest/libqos/virtio-pci.h
+++ b/tests/qtest/libqos/virtio-pci.h
@@ -26,6 +26,7 @@ typedef struct QVirtioPCIDevice {
     uint64_t config_msix_addr;
     uint32_t config_msix_data;
 
+    bool enabled;
     int bar_idx;
 
     /* VIRTIO 1.0 */
diff --git a/tests/qtest/libqos/virtio-pci.c b/tests/qtest/libqos/virtio-pci.c
index ea8114e2438..d6333a4d576 100644
--- a/tests/qtest/libqos/virtio-pci.c
+++ b/tests/qtest/libqos/virtio-pci.c
@@ -278,13 +278,22 @@ static const QVirtioPCIMSIXOps qvirtio_pci_msix_ops_legacy = {
 
 void qvirtio_pci_device_enable(QVirtioPCIDevice *d)
 {
+    g_assert(!d->enabled);
+    d->enabled = true;
     qpci_device_enable(d->pdev);
     d->bar = qpci_iomap(d->pdev, d->bar_idx, NULL);
 }
 
 void qvirtio_pci_device_disable(QVirtioPCIDevice *d)
 {
-    qpci_iounmap(d->pdev, d->bar);
+    /*
+     * Test for "enabled" device state because some paths can call
+     * qvirtio_pci_disable_device() on devices that have not been enabled.
+     */
+    if (d->enabled) {
+        qpci_iounmap(d->pdev, d->bar);
+        d->enabled = false;
+    }
 }
 
 void qvirtqueue_pci_msix_setup(QVirtioPCIDevice *d, QVirtQueuePCI *vqpci,
-- 
2.47.1

[PATCH v5 08/11] tests/qtest/libquos/pci: Add migration fixup helper for pci devices

[Nicholas Piggin]

Migration tests can create new QPCI devices for the destination
machine which may need to take on some state of the source machine
after destination is complete.

Add a migration fixup helper and call it from ahci migration tests.
This is currently a noop and will be used subsequently.

Reviewed-by: Akihiko Odaki <akihiko.odaki@daynix.com>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 tests/qtest/libqos/pci.h | 1 +
 tests/qtest/ahci-test.c  | 1 +
 tests/qtest/libqos/pci.c | 4 ++++
 3 files changed, 6 insertions(+)

diff --git a/tests/qtest/libqos/pci.h b/tests/qtest/libqos/pci.h
index ef40a6917d3..19f1dd13501 100644
--- a/tests/qtest/libqos/pci.h
+++ b/tests/qtest/libqos/pci.h
@@ -120,6 +120,7 @@ void qpci_memwrite(QPCIDevice *bus, QPCIBar token, uint64_t off,
                    const void *buf, size_t len);
 QPCIBar qpci_iomap(QPCIDevice *dev, int barno, uint64_t *sizeptr);
 void qpci_iounmap(QPCIDevice *dev, QPCIBar addr);
+void qpci_migrate_fixup(QPCIDevice *to, QPCIDevice *from);
 QPCIBar qpci_legacy_iomap(QPCIDevice *dev, uint16_t addr);
 
 void qpci_unplug_acpi_device_test(QTestState *qs, const char *id, uint8_t slot);
diff --git a/tests/qtest/ahci-test.c b/tests/qtest/ahci-test.c
index 7d5f73ac40b..60eef9949db 100644
--- a/tests/qtest/ahci-test.c
+++ b/tests/qtest/ahci-test.c
@@ -137,6 +137,7 @@ static void ahci_migrate(AHCIQState *from, AHCIQState *to, const char *uri)
     memcpy(to, from, sizeof(AHCIQState));
     to->parent = tmp;
     to->dev = dev;
+    qpci_migrate_fixup(to->dev, from->dev);
 
     tmp = from->parent;
     dev = from->dev;
diff --git a/tests/qtest/libqos/pci.c b/tests/qtest/libqos/pci.c
index 2bae119bfca..de95329e486 100644
--- a/tests/qtest/libqos/pci.c
+++ b/tests/qtest/libqos/pci.c
@@ -620,6 +620,10 @@ QPCIBar qpci_legacy_iomap(QPCIDevice *dev, uint16_t addr)
     return bar;
 }
 
+void qpci_migrate_fixup(QPCIDevice *to, QPCIDevice *from)
+{
+}
+
 void add_qpci_address(QOSGraphEdgeOptions *opts, QPCIAddress *addr)
 {
     g_assert(addr);
-- 
2.47.1

[PATCH v5 09/11] qtest/libqos/pci: Enforce balanced iomap/unmap

[Nicholas Piggin]

Add assertions to ensure a BAR is not mapped twice, and that only
previously mapped BARs are unmapped. This can help catch bugs and
fragile coding.

Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
Reviewed-by: Akihiko Odaki <akihiko.odaki@daynix.com>
Reviewed-by: Fabiano Rosas <farosas@suse.de>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 tests/qtest/libqos/pci.h |  9 +++++++
 tests/qtest/libqos/pci.c | 51 ++++++++++++++++++++++++++++++++++++----
 2 files changed, 55 insertions(+), 5 deletions(-)

diff --git a/tests/qtest/libqos/pci.h b/tests/qtest/libqos/pci.h
index 19f1dd13501..a51bf60620f 100644
--- a/tests/qtest/libqos/pci.h
+++ b/tests/qtest/libqos/pci.h
@@ -60,10 +60,19 @@ struct QPCIBar {
     bool is_io;
 };
 
+/*
+ * hw/pci permits 7 (PCI_NUM_REGIONS) regions, the last for PCI_ROM_SLOT.
+ * libqos does not implement PCI_ROM_SLOT at the moment, and as such it
+ * permits 6.
+ */
+#define QPCI_NUM_REGIONS 6
+
 struct QPCIDevice
 {
     QPCIBus *bus;
     int devfn;
+    bool bars_mapped[QPCI_NUM_REGIONS];
+    QPCIBar bars[QPCI_NUM_REGIONS];
     bool msix_enabled;
     QPCIBar msix_table_bar, msix_pba_bar;
     uint64_t msix_table_off, msix_pba_off;
diff --git a/tests/qtest/libqos/pci.c b/tests/qtest/libqos/pci.c
index de95329e486..694f1458f46 100644
--- a/tests/qtest/libqos/pci.c
+++ b/tests/qtest/libqos/pci.c
@@ -79,12 +79,17 @@ QPCIDevice *qpci_device_find(QPCIBus *bus, int devfn)
 void qpci_device_init(QPCIDevice *dev, QPCIBus *bus, QPCIAddress *addr)
 {
     uint16_t vendor_id, device_id;
+    int i;
 
     qpci_device_set(dev, bus, addr->devfn);
     vendor_id = qpci_config_readw(dev, PCI_VENDOR_ID);
     device_id = qpci_config_readw(dev, PCI_DEVICE_ID);
     g_assert(!addr->vendor_id || vendor_id == addr->vendor_id);
     g_assert(!addr->device_id || device_id == addr->device_id);
+
+    for (i = 0; i < QPCI_NUM_REGIONS; i++) {
+        g_assert(!dev->bars_mapped[i]);
+    }
 }
 
 static uint8_t qpci_find_resource_reserve_capability(QPCIDevice *dev)
@@ -549,21 +554,31 @@ void qpci_memwrite(QPCIDevice *dev, QPCIBar token, uint64_t off,
     dev->bus->memwrite(dev->bus, token.addr + off, buf, len);
 }
 
-QPCIBar qpci_iomap(QPCIDevice *dev, int barno, uint64_t *sizeptr)
+static uint8_t qpci_bar_reg(int barno)
 {
-    QPCIBus *bus = dev->bus;
     static const int bar_reg_map[] = {
         PCI_BASE_ADDRESS_0, PCI_BASE_ADDRESS_1, PCI_BASE_ADDRESS_2,
         PCI_BASE_ADDRESS_3, PCI_BASE_ADDRESS_4, PCI_BASE_ADDRESS_5,
     };
+
+    g_assert(barno >= 0 && barno <= QPCI_NUM_REGIONS);
+
+    return bar_reg_map[barno];
+}
+
+QPCIBar qpci_iomap(QPCIDevice *dev, int barno, uint64_t *sizeptr)
+{
+    QPCIBus *bus = dev->bus;
     QPCIBar bar;
     int bar_reg;
     uint32_t addr, size;
     uint32_t io_type;
     uint64_t loc;
 
-    g_assert(barno >= 0 && barno <= 5);
-    bar_reg = bar_reg_map[barno];
+    g_assert(barno >= 0 && barno <= QPCI_NUM_REGIONS);
+    g_assert(!dev->bars_mapped[barno]);
+
+    bar_reg = qpci_bar_reg(barno);
 
     qpci_config_writel(dev, bar_reg, 0xFFFFFFFF);
     addr = qpci_config_readl(dev, bar_reg);
@@ -606,12 +621,34 @@ QPCIBar qpci_iomap(QPCIDevice *dev, int barno, uint64_t *sizeptr)
     }
 
     bar.addr = loc;
+
+    dev->bars_mapped[barno] = true;
+    dev->bars[barno] = bar;
+
     return bar;
 }
 
 void qpci_iounmap(QPCIDevice *dev, QPCIBar bar)
 {
-    /* FIXME */
+    int bar_reg;
+    int i;
+
+    g_assert(bar.addr);
+
+    for (i = 0; i < QPCI_NUM_REGIONS; i++) {
+        if (!dev->bars_mapped[i]) {
+            continue;
+        }
+        if (dev->bars[i].addr == bar.addr) {
+            dev->bars_mapped[i] = false;
+            memset(&dev->bars_mapped[i], 0, sizeof(dev->bars_mapped[i]));
+            bar_reg = qpci_bar_reg(i);
+            qpci_config_writel(dev, bar_reg, 0xFFFFFFFF);
+            /* FIXME: the address space is leaked */
+            return;
+        }
+    }
+    g_assert_not_reached(); /* device was not iomap()ed */
 }
 
 QPCIBar qpci_legacy_iomap(QPCIDevice *dev, uint16_t addr)
@@ -622,6 +659,10 @@ QPCIBar qpci_legacy_iomap(QPCIDevice *dev, uint16_t addr)
 
 void qpci_migrate_fixup(QPCIDevice *to, QPCIDevice *from)
 {
+    memcpy(to->bars_mapped, from->bars_mapped, sizeof(from->bars_mapped));
+    memset(from->bars_mapped, 0, sizeof(from->bars_mapped));
+    memcpy(to->bars, from->bars, sizeof(from->bars));
+    memset(from->bars, 0, sizeof(from->bars));
 }
 
 void add_qpci_address(QOSGraphEdgeOptions *opts, QPCIAddress *addr)
-- 
2.47.1

[PATCH v5 10/11] qtest/libqos/pci: Fix qpci_msix_enable sharing bar0

[Nicholas Piggin]

Devices where the MSI-X addresses are shared with other MMIO on BAR0
can not use msi_enable because it unmaps and remaps BAR0, which
interferes with device MMIO mappings. xhci-nec is one such device we
would like to test with msix.

Use the BAR iomap tracking structure introduced in the previous change
to have qpci_misx_enable() use existing iomaps if msix bars are
already mapped.

Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
Reviewed-by: Akihiko Odaki <akihiko.odaki@daynix.com>
Reviewed-by: Fabiano Rosas <farosas@suse.de>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 tests/qtest/libqos/pci.h |  1 +
 tests/qtest/libqos/pci.c | 40 ++++++++++++++++++++++++++++++++++------
 2 files changed, 35 insertions(+), 6 deletions(-)

diff --git a/tests/qtest/libqos/pci.h b/tests/qtest/libqos/pci.h
index a51bf60620f..852d4a85f6f 100644
--- a/tests/qtest/libqos/pci.h
+++ b/tests/qtest/libqos/pci.h
@@ -74,6 +74,7 @@ struct QPCIDevice
     bool bars_mapped[QPCI_NUM_REGIONS];
     QPCIBar bars[QPCI_NUM_REGIONS];
     bool msix_enabled;
+    bool msix_table_bar_iomap, msix_pba_bar_iomap;
     QPCIBar msix_table_bar, msix_pba_bar;
     uint64_t msix_table_off, msix_pba_off;
 };
diff --git a/tests/qtest/libqos/pci.c b/tests/qtest/libqos/pci.c
index 694f1458f46..cc83dcc163a 100644
--- a/tests/qtest/libqos/pci.c
+++ b/tests/qtest/libqos/pci.c
@@ -274,15 +274,21 @@ void qpci_msix_enable(QPCIDevice *dev)
 
     table = qpci_config_readl(dev, addr + PCI_MSIX_TABLE);
     bir_table = table & PCI_MSIX_FLAGS_BIRMASK;
-    dev->msix_table_bar = qpci_iomap(dev, bir_table, NULL);
+    if (dev->bars_mapped[bir_table]) {
+        dev->msix_table_bar = dev->bars[bir_table];
+    } else {
+        dev->msix_table_bar_iomap = true;
+        dev->msix_table_bar = qpci_iomap(dev, bir_table, NULL);
+    }
     dev->msix_table_off = table & ~PCI_MSIX_FLAGS_BIRMASK;
 
     table = qpci_config_readl(dev, addr + PCI_MSIX_PBA);
     bir_pba = table & PCI_MSIX_FLAGS_BIRMASK;
-    if (bir_pba != bir_table) {
-        dev->msix_pba_bar = qpci_iomap(dev, bir_pba, NULL);
+    if (dev->bars_mapped[bir_pba]) {
+        dev->msix_pba_bar = dev->bars[bir_pba];
     } else {
-        dev->msix_pba_bar = dev->msix_table_bar;
+        dev->msix_pba_bar_iomap = true;
+        dev->msix_pba_bar = qpci_iomap(dev, bir_pba, NULL);
     }
     dev->msix_pba_off = table & ~PCI_MSIX_FLAGS_BIRMASK;
 
@@ -293,6 +299,7 @@ void qpci_msix_disable(QPCIDevice *dev)
 {
     uint8_t addr;
     uint16_t val;
+    uint32_t table;
 
     g_assert(dev->msix_enabled);
     addr = qpci_find_capability(dev, PCI_CAP_ID_MSIX, 0);
@@ -301,10 +308,31 @@ void qpci_msix_disable(QPCIDevice *dev)
     qpci_config_writew(dev, addr + PCI_MSIX_FLAGS,
                                                 val & ~PCI_MSIX_FLAGS_ENABLE);
 
-    if (dev->msix_pba_bar.addr != dev->msix_table_bar.addr) {
+    if (dev->msix_pba_bar_iomap) {
+        dev->msix_pba_bar_iomap = false;
         qpci_iounmap(dev, dev->msix_pba_bar);
+    } else {
+        /*
+         * If we had reused an existing iomap, ensure it is still mapped
+         * otherwise it would be a bug if it were unmapped before msix is
+         * disabled. A refcounting iomap implementation could avoid this
+         * issue entirely, but let's wait until that's needed.
+         */
+        uint8_t bir_pba;
+        table = qpci_config_readl(dev, addr + PCI_MSIX_PBA);
+        bir_pba = table & PCI_MSIX_FLAGS_BIRMASK;
+        g_assert(dev->bars_mapped[bir_pba]);
+    }
+
+    if (dev->msix_table_bar_iomap) {
+        dev->msix_table_bar_iomap = false;
+        qpci_iounmap(dev, dev->msix_table_bar);
+    } else {
+        uint8_t bir_table;
+        table = qpci_config_readl(dev, addr + PCI_MSIX_TABLE);
+        bir_table = table & PCI_MSIX_FLAGS_BIRMASK;
+        g_assert(dev->bars_mapped[bir_table]);
     }
-    qpci_iounmap(dev, dev->msix_table_bar);
 
     dev->msix_enabled = 0;
     dev->msix_table_off = 0;
-- 
2.47.1

[PATCH v5 11/11] qtest/libqos/pci: Factor msix entry helpers into pci common code

[Nicholas Piggin]

Setting msix entry address and data and masking is moved into
common code helpers from virtio tests.

For now that remains the only user, but there are changes under
development to enable msix vectors for msix, e1000e, and xhci
tests, which can make use of them.

Reviewed-by: Akihiko Odaki <akihiko.odaki@daynix.com>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 tests/qtest/libqos/pci.h        |  3 ++
 tests/qtest/libqos/pci.c        | 53 +++++++++++++++++++++++++++++++++
 tests/qtest/libqos/virtio-pci.c | 48 ++++-------------------------
 3 files changed, 61 insertions(+), 43 deletions(-)

diff --git a/tests/qtest/libqos/pci.h b/tests/qtest/libqos/pci.h
index 852d4a85f6f..1c4f5406c58 100644
--- a/tests/qtest/libqos/pci.h
+++ b/tests/qtest/libqos/pci.h
@@ -96,8 +96,11 @@ void qpci_device_enable(QPCIDevice *dev);
 uint8_t qpci_find_capability(QPCIDevice *dev, uint8_t id, uint8_t start_addr);
 void qpci_msix_enable(QPCIDevice *dev);
 void qpci_msix_disable(QPCIDevice *dev);
+void qpci_msix_set_entry(QPCIDevice *dev, uint16_t entry,
+                         uint64_t guest_addr, uint32_t data);
 bool qpci_msix_pending(QPCIDevice *dev, uint16_t entry);
 bool qpci_msix_masked(QPCIDevice *dev, uint16_t entry);
+void qpci_msix_set_masked(QPCIDevice *dev, uint16_t entry, bool masked);
 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);
diff --git a/tests/qtest/libqos/pci.c b/tests/qtest/libqos/pci.c
index cc83dcc163a..e26cb4dee13 100644
--- a/tests/qtest/libqos/pci.c
+++ b/tests/qtest/libqos/pci.c
@@ -339,6 +339,25 @@ void qpci_msix_disable(QPCIDevice *dev)
     dev->msix_pba_off = 0;
 }
 
+void qpci_msix_set_entry(QPCIDevice *dev, uint16_t entry,
+                         uint64_t guest_addr, uint32_t data)
+{
+    uint64_t vector_off = dev->msix_table_off + entry * PCI_MSIX_ENTRY_SIZE;
+
+    g_assert(dev->msix_enabled);
+    g_assert_cmpint(entry, >=, 0);
+    g_assert_cmpint(entry, <, qpci_msix_table_size(dev));
+
+    qpci_io_writel(dev, dev->msix_table_bar,
+                   vector_off + PCI_MSIX_ENTRY_LOWER_ADDR, guest_addr & ~0UL);
+    qpci_io_writel(dev, dev->msix_table_bar,
+                   vector_off + PCI_MSIX_ENTRY_UPPER_ADDR,
+                   (guest_addr >> 32) & ~0UL);
+
+    qpci_io_writel(dev, dev->msix_table_bar,
+                   vector_off + PCI_MSIX_ENTRY_DATA, data);
+}
+
 bool qpci_msix_pending(QPCIDevice *dev, uint16_t entry)
 {
     uint32_t pba_entry;
@@ -346,6 +365,9 @@ bool qpci_msix_pending(QPCIDevice *dev, uint16_t entry)
     uint64_t  off = (entry / 32) * PCI_MSIX_ENTRY_SIZE / 4;
 
     g_assert(dev->msix_enabled);
+    g_assert_cmpint(entry, >=, 0);
+    g_assert_cmpint(entry, <, qpci_msix_table_size(dev));
+
     pba_entry = qpci_io_readl(dev, dev->msix_pba_bar, dev->msix_pba_off + off);
     return (pba_entry & (1 << bit_n)) != 0;
 }
@@ -357,6 +379,9 @@ bool qpci_msix_masked(QPCIDevice *dev, uint16_t entry)
     uint64_t vector_off = dev->msix_table_off + entry * PCI_MSIX_ENTRY_SIZE;
 
     g_assert(dev->msix_enabled);
+    g_assert_cmpint(entry, >=, 0);
+    g_assert_cmpint(entry, <, qpci_msix_table_size(dev));
+
     addr = qpci_find_capability(dev, PCI_CAP_ID_MSIX, 0);
     g_assert_cmphex(addr, !=, 0);
     val = qpci_config_readw(dev, addr + PCI_MSIX_FLAGS);
@@ -370,6 +395,34 @@ bool qpci_msix_masked(QPCIDevice *dev, uint16_t entry)
     }
 }
 
+void qpci_msix_set_masked(QPCIDevice *dev, uint16_t entry, bool masked)
+{
+    uint8_t addr;
+    uint16_t val;
+    uint64_t vector_off = dev->msix_table_off + entry * PCI_MSIX_ENTRY_SIZE;
+
+    g_assert(dev->msix_enabled);
+    g_assert_cmpint(entry, >=, 0);
+    g_assert_cmpint(entry, <, qpci_msix_table_size(dev));
+
+    addr = qpci_find_capability(dev, PCI_CAP_ID_MSIX, 0);
+    g_assert_cmphex(addr, !=, 0);
+    val = qpci_config_readw(dev, addr + PCI_MSIX_FLAGS);
+    g_assert(!(val & PCI_MSIX_FLAGS_MASKALL));
+
+    val = qpci_io_readl(dev, dev->msix_table_bar,
+                         vector_off + PCI_MSIX_ENTRY_VECTOR_CTRL);
+    if (masked && !(val & PCI_MSIX_ENTRY_CTRL_MASKBIT)) {
+        qpci_io_writel(dev, dev->msix_table_bar,
+                      vector_off + PCI_MSIX_ENTRY_VECTOR_CTRL,
+                      val | PCI_MSIX_ENTRY_CTRL_MASKBIT);
+    } else if (!masked && (val & PCI_MSIX_ENTRY_CTRL_MASKBIT)) {
+        qpci_io_writel(dev, dev->msix_table_bar,
+                      vector_off + PCI_MSIX_ENTRY_VECTOR_CTRL,
+                      val & ~PCI_MSIX_ENTRY_CTRL_MASKBIT);
+    }
+}
+
 /**
  * qpci_msix_test_interrupt - test whether msix interrupt has been raised
  * @dev: PCI device
diff --git a/tests/qtest/libqos/virtio-pci.c b/tests/qtest/libqos/virtio-pci.c
index d6333a4d576..4e27b7a7931 100644
--- a/tests/qtest/libqos/virtio-pci.c
+++ b/tests/qtest/libqos/virtio-pci.c
@@ -299,66 +299,28 @@ void qvirtio_pci_device_disable(QVirtioPCIDevice *d)
 void qvirtqueue_pci_msix_setup(QVirtioPCIDevice *d, QVirtQueuePCI *vqpci,
                                         QGuestAllocator *alloc, uint16_t entry)
 {
-    uint32_t control;
-    uint64_t off;
-
     g_assert(d->pdev->msix_enabled);
-    off = d->pdev->msix_table_off + (entry * 16);
-
-    g_assert_cmpint(entry, >=, 0);
-    g_assert_cmpint(entry, <, qpci_msix_table_size(d->pdev));
     vqpci->msix_entry = entry;
-
     vqpci->msix_addr = guest_alloc(alloc, 4);
     qtest_memset(d->pdev->bus->qts, vqpci->msix_addr, 0, 4);
-    qpci_io_writel(d->pdev, d->pdev->msix_table_bar,
-                   off + PCI_MSIX_ENTRY_LOWER_ADDR, vqpci->msix_addr & ~0UL);
-    qpci_io_writel(d->pdev, d->pdev->msix_table_bar,
-                   off + PCI_MSIX_ENTRY_UPPER_ADDR,
-                   (vqpci->msix_addr >> 32) & ~0UL);
-    qpci_io_writel(d->pdev, d->pdev->msix_table_bar,
-                   off + PCI_MSIX_ENTRY_DATA, vqpci->msix_data);
-
-    control = qpci_io_readl(d->pdev, d->pdev->msix_table_bar,
-                            off + PCI_MSIX_ENTRY_VECTOR_CTRL);
-    qpci_io_writel(d->pdev, d->pdev->msix_table_bar,
-                   off + PCI_MSIX_ENTRY_VECTOR_CTRL,
-                   control & ~PCI_MSIX_ENTRY_CTRL_MASKBIT);
 
+    qpci_msix_set_entry(d->pdev, entry, vqpci->msix_addr, vqpci->msix_data);
+    qpci_msix_set_masked(d->pdev, entry, false);
     d->msix_ops->set_queue_vector(d, vqpci->vq.index, entry);
 }
 
 void qvirtio_pci_set_msix_configuration_vector(QVirtioPCIDevice *d,
                                         QGuestAllocator *alloc, uint16_t entry)
 {
-    uint32_t control;
-    uint64_t off;
-
     g_assert(d->pdev->msix_enabled);
-    off = d->pdev->msix_table_off + (entry * 16);
-
-    g_assert_cmpint(entry, >=, 0);
-    g_assert_cmpint(entry, <, qpci_msix_table_size(d->pdev));
     d->config_msix_entry = entry;
-
     d->config_msix_data = 0x12345678;
     d->config_msix_addr = guest_alloc(alloc, 4);
     qtest_memset(d->pdev->bus->qts, d->config_msix_addr, 0, 4);
 
-    qpci_io_writel(d->pdev, d->pdev->msix_table_bar,
-                   off + PCI_MSIX_ENTRY_LOWER_ADDR, d->config_msix_addr & ~0UL);
-    qpci_io_writel(d->pdev, d->pdev->msix_table_bar,
-                   off + PCI_MSIX_ENTRY_UPPER_ADDR,
-                   (d->config_msix_addr >> 32) & ~0UL);
-    qpci_io_writel(d->pdev, d->pdev->msix_table_bar,
-                   off + PCI_MSIX_ENTRY_DATA, d->config_msix_data);
-
-    control = qpci_io_readl(d->pdev, d->pdev->msix_table_bar,
-                            off + PCI_MSIX_ENTRY_VECTOR_CTRL);
-    qpci_io_writel(d->pdev, d->pdev->msix_table_bar,
-                   off + PCI_MSIX_ENTRY_VECTOR_CTRL,
-                   control & ~PCI_MSIX_ENTRY_CTRL_MASKBIT);
-
+    qpci_msix_set_entry(d->pdev, entry, d->config_msix_addr,
+                                        d->config_msix_data);
+    qpci_msix_set_masked(d->pdev, entry, false);
     d->msix_ops->set_config_vector(d, entry);
 }
 
-- 
2.47.1