On Mon, 24 Oct 2016 14:19:47 +1100 David Gibson <da...@gibson.dropbear.id.au> 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 <da...@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 <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 | 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 ?
pgpO7QZpjHC43.pgp
Description: OpenPGP digital signature
