On 19/10/2016 14:25, David Gibson wrote:
> The usual use model for the libqos PCI functions is to map a specific PCI
> BAR using qpci_iomap() then pass the returned token into IO accessor
> functions. This, and the fact that iomap() returns a (void *) which
> actually contains a PCI space address, kind of suggests that the return
> value from iomap is supposed to be an opaque token.
>
> ..except that the callers expect to be able to add offsets to it. Which
> also assumes the compiler will support pointer arithmetic on a (void *),
> and treat it as working with byte offsets.
>
> To clarify this situation change iomap() and the IO accessors to take
> a definitely opaque BAR handle (enforced with a wrapper struct) along with
> an offset within the BAR. This changes both the functions and all the
> callers.
>
> Asserts that iomap() returns non-NULL are removed in some places; iomap()
> already asserts if it can't map the BAR
>
> Signed-off-by: David Gibson <da...@gibson.dropbear.id.au>
> ---
> tests/ahci-test.c | 4 +-
> tests/e1000e-test.c | 7 +-
> tests/ide-test.c | 176
> +++++++++++++++++++++++-----------------------
> tests/ivshmem-test.c | 16 ++---
> tests/libqos/ahci.c | 3 +-
> tests/libqos/ahci.h | 6 +-
> tests/libqos/pci.c | 151 ++++++++++++++++++---------------------
> tests/libqos/pci.h | 46 +++++++-----
> tests/libqos/usb.c | 6 +-
> tests/libqos/usb.h | 2 +-
> tests/libqos/virtio-pci.c | 102 ++++++++++++++-------------
> tests/libqos/virtio-pci.h | 2 +-
> tests/rtl8139-test.c | 10 ++-
> tests/tco-test.c | 80 ++++++++++-----------
> tests/usb-hcd-ehci-test.c | 5 +-
> 15 files changed, 305 insertions(+), 311 deletions(-)
>
...
> diff --git a/tests/libqos/pci.c b/tests/libqos/pci.c
> index 3021651..30ddf19 100644
> --- a/tests/libqos/pci.c
> +++ b/tests/libqos/pci.c
> @@ -104,7 +104,6 @@ void qpci_msix_enable(QPCIDevice *dev)
> uint32_t table;
> uint8_t bir_table;
> uint8_t bir_pba;
> - void *offset;
>
> addr = qpci_find_capability(dev, PCI_CAP_ID_MSIX);
> g_assert_cmphex(addr, !=, 0);
> @@ -114,18 +113,16 @@ void qpci_msix_enable(QPCIDevice *dev)
>
> table = qpci_config_readl(dev, addr + PCI_MSIX_TABLE);
> bir_table = table & PCI_MSIX_FLAGS_BIRMASK;
> - offset = qpci_iomap(dev, bir_table, NULL);
> - dev->msix_table = offset + (table & ~PCI_MSIX_FLAGS_BIRMASK);
> + dev->msix_table_bar = qpci_iomap(dev, bir_table, NULL);
> + dev->msix_table_off = table & ~PCI_MSIX_FLAGS_BIRMASK;
>
> table = qpci_config_readl(dev, addr + PCI_MSIX_PBA);
> bir_pba = table & PCI_MSIX_FLAGS_BIRMASK;
> if (bir_pba != bir_table) {
> - offset = qpci_iomap(dev, bir_pba, NULL);
> + dev->msix_pba_bar = qpci_iomap(dev, bir_pba, NULL);
> }
> - dev->msix_pba = offset + (table & ~PCI_MSIX_FLAGS_BIRMASK);
> + dev->msix_pba_off = table & ~PCI_MSIX_FLAGS_BIRMASK;
>
> - g_assert(dev->msix_table != NULL);
> - g_assert(dev->msix_pba != NULL);
> dev->msix_enabled = true;
> }
>
> @@ -141,22 +138,25 @@ void qpci_msix_disable(QPCIDevice *dev)
> qpci_config_writew(dev, addr + PCI_MSIX_FLAGS,
> val &
> ~PCI_MSIX_FLAGS_ENABLE);
>
> - qpci_iounmap(dev, dev->msix_table);
> - qpci_iounmap(dev, dev->msix_pba);
> + qpci_iounmap(dev, dev->msix_table_bar);
> + qpci_iounmap(dev, dev->msix_pba_bar);
> dev->msix_enabled = 0;
> - dev->msix_table = NULL;
> - dev->msix_pba = NULL;
> + memset(&dev->msix_table_bar, 0, sizeof(dev->msix_table_bar));
> + memset(&dev->msix_pba_bar, 0, sizeof(dev->msix_pba_bar));
If it's opaque, you should not know what is the value to unset it,
perhaps you could define a "QPCIBAR_INVALID" and
set "bar = QPCIBAR_INVALID"?
Laurent
