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

[Jishnu Warrier <jishnuvw@linux.ibm.com>]

From: Nicholas Piggin <npiggin@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 <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.c | 75 ++++++++++++++++++++++++++++++++--------
 tests/qtest/libqos/pci.h | 10 ++++++
 2 files changed, 70 insertions(+), 15 deletions(-)

diff --git a/tests/qtest/libqos/pci.c b/tests/qtest/libqos/pci.c
index cda3c56c..3d953d23 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)
@@ -338,21 +343,21 @@ bool qpci_msix_masked(QPCIDevice *dev, uint16_t entry)
 }
 
 /**
- * qpci_msix_test_interrupt - test whether msix interrupt has been raised
+ * qpci_msix_test_interrupt - test whether MSI-X 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
+ * @msix_entry: MSI-X entry to test
+ * @msix_addr: address of MSI-X message
+ * @msix_data: expected MSI-X message payload
  *
- * This tests whether the msix source has raised an interrupt. If the msix
+ * This tests whether the MSI-X source has raised an interrupt. If the MSI-X
  * 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
+ * MSI-X message address to 0 if it is found set. This must be called with
+ * the MSI-X 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,
@@ -376,8 +381,8 @@ bool qpci_msix_test_interrupt(QPCIDevice *dev, uint32_t 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);
+    /* MSI-X payload is written in little-endian format */
+    qtest_memread(dev->bus->qts, msix_addr, &data, sizeof(data));
     data = le32_to_cpu(data);
     if (data == 0) {
         return false;
@@ -385,7 +390,7 @@ bool qpci_msix_test_interrupt(QPCIDevice *dev, uint32_t msix_entry,
 
     /* 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);
+    qtest_memset(dev->bus->qts, msix_addr, 0, sizeof(data));
 
     return true;
 }
@@ -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,38 @@ 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);
+            /* 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 +668,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 19f1dd13..73739afe 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;
-- 
2.55.0