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

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

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

On Fri, Oct 21, 2016 at 03:23:51PM +0200, Greg Kurz wrote:
> On Fri, 21 Oct 2016 12:19:52 +1100
> David Gibson <david@gibson.dropbear.id.au> 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        |  50 ++++++++-----
> >  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, 309 insertions(+), 311 deletions(-)
> > 
> > diff --git a/tests/ahci-test.c b/tests/ahci-test.c
> > index 9c0adce..4358631 100644
> > --- a/tests/ahci-test.c
> > +++ b/tests/ahci-test.c
> > @@ -90,12 +90,12 @@ static void verify_state(AHCIQState *ahci)
> >      g_assert_cmphex(ahci_fingerprint, ==, ahci->fingerprint);
> >  
> >      /* If we haven't initialized, this is as much as can be validated. */
> > -    if (!ahci->hba_base) {
> > +    if (!ahci->hba_bar.addr) {
> 
> Isn't ahci->hba_bar supposed to be opaque ?

Ah, good point, missed that one.  And that test isn't even right, with
the INVALID_BAR stuff.

> >          return;
> >      }
> 
> Unrelated to this patch, does it make sense to call verify_state() if
> ahci_pci_enable() hasn't been called before ? Shouldn't we assert instead ?

I'm pretty sure it is only called after PCI initialization, so I think
we should just remove the check.

> >      hba_base = (uint64_t)qpci_config_readl(ahci->dev, PCI_BASE_ADDRESS_5);
> > -    hba_stored = (uint64_t)(uintptr_t)ahci->hba_base;
> > +    hba_stored = ahci->hba_bar.addr;
> >      g_assert_cmphex(hba_base, ==, hba_stored);
> 
> Again since ahci->hba_bar is opaque, is it right to do that check here ?

Not, really no.  I was aware of that one, but decided to let it go
since it's just one pretty specific check.

But then again, if I'm fixing other things in AHCI, maybe I might as
well fix it to read the actual BAR register before the migration.

> I have another question about QPCI_BAR_INVALID far below (patch is
> long :)


[snip]
> > +struct QPCIBar {
> > +    uint64_t addr;
> > +};
> > +
> > +static const QPCIBar QPCI_BAR_INVALID = {
> > +    .addr = (uint64_t)-1ULL,
> > +};
> > +
> 
> In v2, you had:
> 
> void qpci_msix_disable(QPCIDevice *dev)
> {
> [...]
>     memset(&dev->msix_table_bar, 0, sizeof(dev->msix_table_bar));
>     memset(&dev->msix_pba_bar, 0, sizeof(dev->msix_pba_bar));  
> [...]
> }
> 
> and now they get filled with 0xff... is there a reason ?

Yes.  I realized an address of 0 is a bad way of marking an invalid
BAR, because it's actually a semi-plausible real BAR value.  For
example getting a legacy IO "BAR" at offset 0 would give you that.

-- 
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 --]