From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33843) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1byVwv-0000R0-ID for qemu-devel@nongnu.org; Sun, 23 Oct 2016 23:29:23 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1byVwu-0005fI-BM for qemu-devel@nongnu.org; Sun, 23 Oct 2016 23:29:21 -0400 Date: Mon, 24 Oct 2016 14:19:47 +1100 From: David Gibson Message-ID: <20161024031947.GH19629@umbus.fritz.box> References: <1477012792-3326-1-git-send-email-david@gibson.dropbear.id.au> <1477012792-3326-12-git-send-email-david@gibson.dropbear.id.au> <20161021152351.6b205cea@bahia> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="gTtJ75FAzB1T2CN6" Content-Disposition: inline In-Reply-To: <20161021152351.6b205cea@bahia> Subject: Re: [Qemu-devel] [PATCHv4 11/11] libqos: Change PCI accessors to take opaque BAR handle List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Greg Kurz Cc: pbonzini@redhat.com, qemu-devel@nongnu.org, qemu-ppc@nongnu.org, lvivier@redhat.com, agraf@suse.de, stefanha@redhat.com, mst@redhat.com, aik@ozlabs.ru, mdroth@linux.vnet.ibm.com, thuth@redhat.com --gTtJ75FAzB1T2CN6 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Oct 21, 2016 at 03:23:51PM +0200, Greg Kurz wrote: > On Fri, 21 Oct 2016 12:19:52 +1100 > David Gibson wrote: >=20 > > The usual use model for the libqos PCI functions is to map a specific P= CI > > 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. > >=20 > > ..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. > >=20 > > To clarify this situation change iomap() and the IO accessors to take > > a definitely opaque BAR handle (enforced with a wrapper struct) along w= ith > > an offset within the BAR. This changes both the functions and all the > > callers. > >=20 > > Asserts that iomap() returns non-NULL are removed in some places; iomap= () > > already asserts if it can't map the BAR > >=20 > > Signed-off-by: David Gibson > > --- > > 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(-) > >=20 > > 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, =3D=3D, ahci->fingerprint); > > =20 > > /* If we haven't initialized, this is as much as can be validated.= */ > > - if (!ahci->hba_base) { > > + if (!ahci->hba_bar.addr) { >=20 > 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; > > } >=20 > 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 =3D (uint64_t)qpci_config_readl(ahci->dev, PCI_BASE_ADDRE= SS_5); > > - hba_stored =3D (uint64_t)(uintptr_t)ahci->hba_base; > > + hba_stored =3D ahci->hba_bar.addr; > > g_assert_cmphex(hba_base, =3D=3D, hba_stored); >=20 > 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 =3D { > > + .addr =3D (uint64_t)-1ULL, > > +}; > > + >=20 > In v2, you had: >=20 > 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)); =20 > [...] > } >=20 > 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. --=20 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 --gTtJ75FAzB1T2CN6 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJYDX3RAAoJEGw4ysog2bOSZ0cQALhgtZe1pUO6kZY6bR0Mz8zx OJbjERnFZNCiqMVh3CvgXlxs9/YHq/tKZkI1idHt7RUF5B4PRngUWUje+3P/lDgI uBnMCCbFip7fq9g5RJuUe/CaSISCoz8PeNgqwKekKNVzYEMzyn4e3suqD2OWntnu mpihtbSmtwsWNLQEY9gXljRVEsccq7OAIx6PYqOtbR6d3/9GUxi50FffdWo+5llh TY3nMgeBx763vgd4bLMj6ZITkHedO/+z9ZHcWwltRdrOMdoEjVZ0vYgc8Aj/Q07u 7qoAir+Zdbwp3bUAsQ/WNgxPfrG86UdcI1mnxbHXcDwAGfRr7tnXdgW7/i2ZeyRF l9xtyvjn/Wl9sZeeHMnVgb+i9hl2zRaeosPrln12IoXYyGOIQxy5GpOnrwyAv4p1 zut/PxLsgJfcLl6/NmNv3satdOKghiBdgWyzxoNC6pOWIC0lPeuJ+T7TmK4hitkW e3nDq8w12xWde97snUgI+lbscM1lqmkDpB3BugL6zKivC+fAcVUNo4a+RNrDhg/q fnR4QFYp0GQHiur0id1XXZmFU//riRgJdJq3MDMeZjPuZ1hMt0a5A3wChM2Ro5ZM /ie0zx7BJPwkF48vRWMzzRbVw0HMQjmFCGC4RtyJfQAjpFhERo9pbzuerFKogZgz X9rD+N66v838tZkcB9VF =eUhe -----END PGP SIGNATURE----- --gTtJ75FAzB1T2CN6--