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

[David Gibson]

On Mon, Oct 24, 2016 at 12:02:31PM +0200, Greg Kurz wrote:
> On Mon, 24 Oct 2016 14:19:47 +1100
> David Gibson <address@hidden> wrote:
> 
> > On Fri, Oct 21, 2016 at 03:23:51PM +0200, Greg Kurz wrote:
> > > On Fri, 21 Oct 2016 12:19:52 +1100
> > > David Gibson <address@hidden> 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        |  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.
> > 
> 
> Indeed.
> 
> > > >          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.
> > 
> 
> Shouldn't all QPCIBar structures be initialized to BAR_INVALID then ?

Yes, which is easier said than done.  In my latest spin, I've just
given up on any concept of "invalid" BARs.  It wasn't used for
anything except some not very useful sanity checks.  The one exception
was this stuff in ahci-test, which I replaced with an explicit flag
indicating we've activated the AHCI.

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