Re: [Qemu-ppc] [PATCHv2 11/11] libqos: Change PCI accessors to take opaque BAR handle

[Laurent Vivier]

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 <address@hidden>
> ---
>  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