qtest/libqos/pci: pci and msix fixes

[PATCH v3 2/4] qtest/libqos/pci: Enforce balanced iomap/unmap

Posted by Nicholas Piggin 2 weeks, 4 days ago

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

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 a59197b9922..05089a5f24f 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)
@@ -529,6 +534,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);
@@ -572,12 +579,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 v3 2/4] qtest/libqos/pci: Enforce balanced iomap/unmap

Posted by Philippe Mathieu-Daudé 2 weeks, 2 days ago

Hi Nick,

Only nitpicking comments...

On 17/1/25 18:22, 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.
> 
> 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(-)

Maybe put the AHCI fix in a preliminary patch?

> 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

Consider using a definition rather than a magic value:

   #define PCI_BAR_COUNT 6

> @@ -65,6 +65,8 @@ struct QPCIDevice
>   {
>       QPCIBus *bus;
>       int devfn;
> +    bool bars_mapped[6];
> +    QPCIBar bars[6];


> diff --git a/tests/qtest/libqos/pci.c b/tests/qtest/libqos/pci.c
> index a59197b9922..05089a5f24f 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]);
> +    }
>   }


> @@ -572,12 +579,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();
>   }
Regards,

Phil.

Re: [PATCH v3 2/4] qtest/libqos/pci: Enforce balanced iomap/unmap

Posted by Nicholas Piggin 2 weeks, 1 day ago

On Mon Jan 20, 2025 at 3:29 PM AEST, Philippe Mathieu-Daudé wrote:
> Hi Nick,
>
> Only nitpicking comments...

Hey, no they're good comments actually.

>
> On 17/1/25 18:22, 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.
>> 
>> 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(-)
>
> Maybe put the AHCI fix in a preliminary patch?

Yeah, this was just laziness. I will fix.

>
>> 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
>
> Consider using a definition rather than a magic value:
>
>    #define PCI_BAR_COUNT 6

Now I look again at PCI code and it has PCI_NUM_REGIONS 7
where ROM slot is the last entry. qtests doesn't use it
AFAIKS but maybe it could(?) so should I just use that
existing define?

Thanks,
Nick

Re: [PATCH v3 2/4] qtest/libqos/pci: Enforce balanced iomap/unmap

Posted by Philippe Mathieu-Daudé 2 weeks, 1 day ago

On 21/1/25 05:39, Nicholas Piggin wrote:
> On Mon Jan 20, 2025 at 3:29 PM AEST, Philippe Mathieu-Daudé wrote:
>> Hi Nick,
>>
>> Only nitpicking comments...
> 
> Hey, no they're good comments actually.
> 
>>
>> On 17/1/25 18:22, 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.
>>>
>>> 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(-)
>>
>> Maybe put the AHCI fix in a preliminary patch?
> 
> Yeah, this was just laziness. I will fix.
> 
>>
>>> 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
>>
>> Consider using a definition rather than a magic value:
>>
>>     #define PCI_BAR_COUNT 6
> 
> Now I look again at PCI code and it has PCI_NUM_REGIONS 7
> where ROM slot is the last entry. qtests doesn't use it
> AFAIKS but maybe it could(?) so should I just use that
> existing define?

Even if qtests don't use it, using it makes the code clearer IMHO.

Thanks!

Phil.

[PATCH v3 1/4] qtest/libqos/pci: Do not write to PBA memory
[PATCH v3 2/4] qtest/libqos/pci: Enforce balanced iomap/unmap
[PATCH v3 3/4] qtest/libqos/pci: Fix qpci_msix_enable sharing bar0
[PATCH v3 4/4] qtest/libqos/pci: Factor msix entry helpers into pci common code