Re: [PATCH v7 08/10] qtest/libqos/pci: Enforce balanced iomap/unmap

[Akihiko Odaki]

On 2026/11/05 21:15, Utkarsh Verma wrote:
On 11/3/26 7:36 PM, Akihiko Odaki wrote:
On 2026/11/02 12:53, uve...@linux.ibm.com wrote:
From: Nicholas Piggin <npig...@gmail.com>
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 <m...@redhat.com>
Cc: Marcel Apfelbaum <marcel.apfelb...@gmail.com>
Reviewed-by: Akihiko Odaki <akihiko.od...@daynix.com>
Reviewed-by: Fabiano Rosas <faro...@suse.de>
Signed-off-by: Nicholas Piggin <npig...@gmail.com>
Signed-off-by: Jishnu Warrier <jishn...@linux.ibm.com>
Signed-off-by: Utkarsh Verma <uve...@linux.ibm.com>
---
  tests/qtest/libqos/ahci.c |  3 ++-
  tests/qtest/libqos/pci.c  | 57 +++++++++++++++++++++++++++++++++++----
  tests/qtest/libqos/pci.h  | 10 +++++++
  3 files changed, 64 insertions(+), 6 deletions(-)

diff --git a/tests/qtest/libqos/ahci.c b/tests/qtest/libqos/ahci.c
index a709558aac..53a98bd6cb 100644
--- a/tests/qtest/libqos/ahci.c
+++ b/tests/qtest/libqos/ahci.c
@@ -225,7 +225,7 @@ void start_ahci_device(AHCIQState *ahci)
  void stop_ahci_device(AHCIQState *ahci)
  {
-    /* Unmap AHCI's ABAR */
+    /* Unmap AHCI's ABAR (no-op if not mapped) */
      qpci_iounmap(ahci->dev, ahci->hba_bar);
  }
@@ -250,6 +250,7 @@ void ahci_hba_enable(AHCIQState *ahci)
      uint8_t num_cmd_slots;
      g_assert(ahci != NULL);
+    g_assert(ahci->hba_bar.mapped);
      /* Set GHC.AE to 1 */
      ahci_set(ahci, AHCI_GHC, AHCI_GHC_AE);
diff --git a/tests/qtest/libqos/pci.c b/tests/qtest/libqos/pci.c
index e78dd4d4c7..365fde1fb9 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)
@@ -554,21 +559,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);
@@ -611,12 +626,40 @@ QPCIBar qpci_iomap(QPCIDevice *dev, int barno, 
uint64_t *sizeptr)
      }
      bar.addr = loc;
+    bar.mapped = true;
+
+    dev->bars_mapped[barno] = true;
+    dev->bars[barno] = bar;
+
      return bar;
  }
  void qpci_iounmap(QPCIDevice *dev, QPCIBar bar)
  {
-    /* FIXME */
+    int bar_reg;
+    int i;
+
+    if (!bar.mapped) {
+        return; /* bar was never mapped; no-op */
+    }
+
+    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;
+            dev->bars[i].mapped = false;
+            bar_reg = qpci_bar_reg(i);
+            qpci_config_writel(dev, bar_reg, 0xFFFFFFFF);

I found that this is actually dangerous: the BAR may be 64-bit, and in 
that case, this won't unmap it. On x86, the BIOS is mapped around that 
area, and the mapped BAR will shadow it.

Regards,
Akihiko Odaki

Agree, writing all 0xFF's to a single BAR will map the PCIe device to 
4GB-size address, which can overlap with the BIOS area on x86 system.
The problem is that since qtest work directly with guest physical 
addresses, writing any address to the BAR for unmapping purpose will map 
that address to the PCIe device.
The false mapping is not the 32 bit limitation, even with full 64 bit 
BAR support, writing any address to both the BARs in qpci_iounmap() 
would result in the mapping of the address to the PCIe device. Also, 
full 64 bit BAR supoort would require implementation in the qpci_iomap() 
and other functions as well.

pci_bar_address() rejects a region whose last address is
PCI_BAR_UNMAPPED, so writing all ones to both halves of a 64-bit BAR
would unmap it. The problem here is writing only the low half. Writing 
only the low half leaves a valid address below 4 GiB when the upper half 
is zero.

Regards,
Akihiko Odaki


For now we can completely remove this BAR write, so the BAR would be 
mapped to it's original address for the rest of the qemu process (memory 
leak) but should be safe as other allocations won't use that same 
address again. Also, since these qtests launch new qemu instances so the 
memory leak only exists in that current qemu process and new tests 
create new instances, so it shouldn't be a big problem.
Do you have any other approach for this, or does the above look good?

Regards,
Utkarsh

+            /* FIXME: the address space is leaked */> + return;
+        }
+    }
+    /*
+     * bar was not iomap()ed; treat as no-op for callers that may
+     * call iounmap unconditionally during cleanup paths.
+     */
  }
  QPCIBar qpci_legacy_iomap(QPCIDevice *dev, uint16_t addr)
@@ -627,6 +670,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)
diff --git a/tests/qtest/libqos/pci.h b/tests/qtest/libqos/pci.h
index cfb7c17dd3..2f9555ae4e 100644
--- a/tests/qtest/libqos/pci.h
+++ b/tests/qtest/libqos/pci.h
@@ -58,12 +58,22 @@ struct QPCIBus {
  struct QPCIBar {
      uint64_t addr;
      bool is_io;
+    bool mapped;
  };
+/*
+ * 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;