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

[Greg Kurz]

On Mon, 24 Oct 2016 15:31:12 +1100
David Gibson <address@hidden> wrote:

> On Mon, Oct 24, 2016 at 02:19:47PM +1100, David Gibson 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.
> >   
> > > >          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.  
> 
> Wait.. no.  There is one testcase which is called when the device has
> been located, but not enabled/initialized.  That means the BAR pointer
> isn't initialized, and the later checks in verify_state (which real IO
> registers) can't be done.  So there is a real point to this test.  I
> think I'll have to add something to allow checks for a valid BAR.
> 

Indeed, I now realize that test_migrate_sanity() does migrate a not yet
enabled device.

pgpkyM6JYe_BH.pgp
Description: OpenPGP digital signature