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

[PATCH 1/5] qtest/pci: Enforce balanced iomap/unmap

Posted by Nicholas Piggin 1 year, 1 month ago

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

Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
Reviewed-by: Fabiano Rosas <farosas@suse.de>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 tests/qtest/libqos/ahci.h       |  1 +
 tests/qtest/libqos/pci.h        |  2 ++
 tests/qtest/libqos/virtio-pci.h |  1 +
 tests/qtest/ahci-test.c         |  2 ++
 tests/qtest/libqos/ahci.c       |  6 ++++++
 tests/qtest/libqos/pci.c        | 32 +++++++++++++++++++++++++++++++-
 tests/qtest/libqos/virtio-pci.c |  6 +++++-
 7 files changed, 48 insertions(+), 2 deletions(-)

diff --git a/tests/qtest/libqos/ahci.h b/tests/qtest/libqos/ahci.h
index a0487a1557d..5d7e26aee2a 100644
--- a/tests/qtest/libqos/ahci.h
+++ b/tests/qtest/libqos/ahci.h
@@ -575,6 +575,7 @@ QPCIDevice *get_ahci_device(QTestState *qts, uint32_t *fingerprint);
 void free_ahci_device(QPCIDevice *dev);
 void ahci_pci_enable(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/libqos/pci.h b/tests/qtest/libqos/pci.h
index 83896145235..9dc82ea723a 100644
--- a/tests/qtest/libqos/pci.h
+++ b/tests/qtest/libqos/pci.h
@@ -65,6 +65,8 @@ struct QPCIDevice
 {
     QPCIBus *bus;
     int devfn;
+    bool bars_mapped[6];
+    QPCIBar bars[6];
     bool msix_enabled;
     QPCIBar msix_table_bar, msix_pba_bar;
     uint64_t msix_table_off, msix_pba_off;
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/ahci-test.c b/tests/qtest/ahci-test.c
index 5a1923f721b..b3dae7a8ce4 100644
--- a/tests/qtest/ahci-test.c
+++ b/tests/qtest/ahci-test.c
@@ -1483,6 +1483,8 @@ static void test_reset_pending_callback(void)
     /* Wait for throttled write to finish. */
     sleep(1);
 
+    stop_ahci_device(ahci);
+
     /* Start again. */
     ahci_clean_mem(ahci);
     ahci_pci_enable(ahci);
diff --git a/tests/qtest/libqos/ahci.c b/tests/qtest/libqos/ahci.c
index 34a75b7f43b..cfc435b6663 100644
--- a/tests/qtest/libqos/ahci.c
+++ b/tests/qtest/libqos/ahci.c
@@ -217,6 +217,12 @@ void start_ahci_device(AHCIQState *ahci)
     qpci_device_enable(ahci->dev);
 }
 
+void stop_ahci_device(AHCIQState *ahci)
+{
+    /* Map AHCI's ABAR (BAR5) */
+    qpci_iounmap(ahci->dev, ahci->hba_bar);
+}
+
 /**
  * Test and initialize the AHCI's HBA memory areas.
  * Initialize and start any ports with devices attached.
diff --git a/tests/qtest/libqos/pci.c b/tests/qtest/libqos/pci.c
index b23d72346b6..a42ca08261d 100644
--- a/tests/qtest/libqos/pci.c
+++ b/tests/qtest/libqos/pci.c
@@ -93,12 +93,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 < 6; i++) {
+        g_assert(!dev->bars_mapped[i]);
+    }
 }
 
 static uint8_t qpci_find_resource_reserve_capability(QPCIDevice *dev)
@@ -531,6 +536,8 @@ QPCIBar qpci_iomap(QPCIDevice *dev, int barno, uint64_t *sizeptr)
     uint64_t loc;
 
     g_assert(barno >= 0 && barno <= 5);
+    g_assert(!dev->bars_mapped[barno]);
+
     bar_reg = bar_reg_map[barno];
 
     qpci_config_writel(dev, bar_reg, 0xFFFFFFFF);
@@ -574,12 +581,35 @@ 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 */
+    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,
+    };
+    int bar_reg;
+    int i;
+
+    for (i = 0; i < 6; i++) {
+        if (!dev->bars_mapped[i]) {
+            continue;
+        }
+        if (dev->bars[i].addr == bar.addr) {
+            dev->bars_mapped[i] = false;
+            bar_reg = bar_reg_map[i];
+            qpci_config_writel(dev, bar_reg, 0xFFFFFFFF);
+            /* FIXME: the address space is leaked */
+            return;
+        }
+    }
+    g_assert_not_reached();
 }
 
 QPCIBar qpci_legacy_iomap(QPCIDevice *dev, uint16_t addr)
diff --git a/tests/qtest/libqos/virtio-pci.c b/tests/qtest/libqos/virtio-pci.c
index 485b8f6b7e0..2b59fb181c9 100644
--- a/tests/qtest/libqos/virtio-pci.c
+++ b/tests/qtest/libqos/virtio-pci.c
@@ -304,11 +304,15 @@ void qvirtio_pci_device_enable(QVirtioPCIDevice *d)
 {
     qpci_device_enable(d->pdev);
     d->bar = qpci_iomap(d->pdev, d->bar_idx, NULL);
+    d->enabled = true;
 }
 
 void qvirtio_pci_device_disable(QVirtioPCIDevice *d)
 {
-    qpci_iounmap(d->pdev, d->bar);
+    if (d->enabled) {
+        qpci_iounmap(d->pdev, d->bar);
+        d->enabled = false;
+    }
 }
 
 void qvirtqueue_pci_msix_setup(QVirtioPCIDevice *d, QVirtQueuePCI *vqpci,
-- 
2.45.2

Re: [PATCH 1/5] qtest/pci: Enforce balanced iomap/unmap

Posted by Akihiko Odaki 1 year, 1 month ago

On 2024/12/18 16:42, Nicholas Piggin wrote:
> Add assertions to ensure a BAR is not mapped twice, and only
> previously mapped BARs are unmapped. This can help catch some
> bugs.
> 
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> Reviewed-by: Fabiano Rosas <farosas@suse.de>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>   tests/qtest/libqos/ahci.h       |  1 +
>   tests/qtest/libqos/pci.h        |  2 ++
>   tests/qtest/libqos/virtio-pci.h |  1 +
>   tests/qtest/ahci-test.c         |  2 ++
>   tests/qtest/libqos/ahci.c       |  6 ++++++
>   tests/qtest/libqos/pci.c        | 32 +++++++++++++++++++++++++++++++-
>   tests/qtest/libqos/virtio-pci.c |  6 +++++-
>   7 files changed, 48 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/qtest/libqos/ahci.h b/tests/qtest/libqos/ahci.h
> index a0487a1557d..5d7e26aee2a 100644
> --- a/tests/qtest/libqos/ahci.h
> +++ b/tests/qtest/libqos/ahci.h
> @@ -575,6 +575,7 @@ QPCIDevice *get_ahci_device(QTestState *qts, uint32_t *fingerprint);
>   void free_ahci_device(QPCIDevice *dev);
>   void ahci_pci_enable(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/libqos/pci.h b/tests/qtest/libqos/pci.h
> index 83896145235..9dc82ea723a 100644
> --- a/tests/qtest/libqos/pci.h
> +++ b/tests/qtest/libqos/pci.h
> @@ -65,6 +65,8 @@ struct QPCIDevice
>   {
>       QPCIBus *bus;
>       int devfn;
> +    bool bars_mapped[6];
> +    QPCIBar bars[6];
>       bool msix_enabled;
>       QPCIBar msix_table_bar, msix_pba_bar;
>       uint64_t msix_table_off, msix_pba_off;
> 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/ahci-test.c b/tests/qtest/ahci-test.c
> index 5a1923f721b..b3dae7a8ce4 100644
> --- a/tests/qtest/ahci-test.c
> +++ b/tests/qtest/ahci-test.c
> @@ -1483,6 +1483,8 @@ static void test_reset_pending_callback(void)
>       /* Wait for throttled write to finish. */
>       sleep(1);
>   
> +    stop_ahci_device(ahci);
> +
>       /* Start again. */
>       ahci_clean_mem(ahci);
>       ahci_pci_enable(ahci);
> diff --git a/tests/qtest/libqos/ahci.c b/tests/qtest/libqos/ahci.c
> index 34a75b7f43b..cfc435b6663 100644
> --- a/tests/qtest/libqos/ahci.c
> +++ b/tests/qtest/libqos/ahci.c
> @@ -217,6 +217,12 @@ void start_ahci_device(AHCIQState *ahci)
>       qpci_device_enable(ahci->dev);
>   }
>   
> +void stop_ahci_device(AHCIQState *ahci)
> +{
> +    /* Map AHCI's ABAR (BAR5) */

I think you meant "unmap AHCI's ABAR (BAR5)".

> +    qpci_iounmap(ahci->dev, ahci->hba_bar);
> +}
> +
>   /**
>    * Test and initialize the AHCI's HBA memory areas.
>    * Initialize and start any ports with devices attached.
> diff --git a/tests/qtest/libqos/pci.c b/tests/qtest/libqos/pci.c
> index b23d72346b6..a42ca08261d 100644
> --- a/tests/qtest/libqos/pci.c
> +++ b/tests/qtest/libqos/pci.c
> @@ -93,12 +93,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 < 6; i++) {
> +        g_assert(!dev->bars_mapped[i]);
> +    }
>   }
>   
>   static uint8_t qpci_find_resource_reserve_capability(QPCIDevice *dev)
> @@ -531,6 +536,8 @@ QPCIBar qpci_iomap(QPCIDevice *dev, int barno, uint64_t *sizeptr)
>       uint64_t loc;
>   
>       g_assert(barno >= 0 && barno <= 5);
> +    g_assert(!dev->bars_mapped[barno]);
> +
>       bar_reg = bar_reg_map[barno];
>   
>       qpci_config_writel(dev, bar_reg, 0xFFFFFFFF);
> @@ -574,12 +581,35 @@ 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 */
> +    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,
> +    };
> +    int bar_reg;
> +    int i;
> +
> +    for (i = 0; i < 6; i++) {
> +        if (!dev->bars_mapped[i]) {
> +            continue;
> +        }
> +        if (dev->bars[i].addr == bar.addr) {
> +            dev->bars_mapped[i] = false;
> +            bar_reg = bar_reg_map[i];
> +            qpci_config_writel(dev, bar_reg, 0xFFFFFFFF);
> +            /* FIXME: the address space is leaked */
> +            return;
> +        }
> +    }
> +    g_assert_not_reached();
>   }
>   
>   QPCIBar qpci_legacy_iomap(QPCIDevice *dev, uint16_t addr)
> diff --git a/tests/qtest/libqos/virtio-pci.c b/tests/qtest/libqos/virtio-pci.c
> index 485b8f6b7e0..2b59fb181c9 100644
> --- a/tests/qtest/libqos/virtio-pci.c
> +++ b/tests/qtest/libqos/virtio-pci.c
> @@ -304,11 +304,15 @@ void qvirtio_pci_device_enable(QVirtioPCIDevice *d)
>   {
>       qpci_device_enable(d->pdev);
>       d->bar = qpci_iomap(d->pdev, d->bar_idx, NULL);
> +    d->enabled = true;
>   }
>   
>   void qvirtio_pci_device_disable(QVirtioPCIDevice *d)
>   {
> -    qpci_iounmap(d->pdev, d->bar);
> +    if (d->enabled) {
> +        qpci_iounmap(d->pdev, d->bar);
> +        d->enabled = false;
> +    }
>   }
>   
>   void qvirtqueue_pci_msix_setup(QVirtioPCIDevice *d, QVirtQueuePCI *vqpci,

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