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 <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"?
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
signature.asc
Description: PGP signature
