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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

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.