On 2026/11/02 12:53, [email protected] wrote:
From: Nicholas Piggin <[email protected]>
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 <[email protected]>
Cc: Marcel Apfelbaum <[email protected]>
Reviewed-by: Akihiko Odaki <[email protected]>
Reviewed-by: Fabiano Rosas <[email protected]>
Signed-off-by: Nicholas Piggin <[email protected]>
Signed-off-by: Jishnu Warrier <[email protected]>
Signed-off-by: Utkarsh Verma <[email protected]>
---
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
+ /* 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;
