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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 4444 bytes --]

On Wed, Oct 19, 2016 at 04:35:24PM +0200, Laurent Vivier wrote:
> 
> 
> 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 <david@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"?

Ah, good idea.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]