From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49177) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bye1K-0002Zv-00 for qemu-devel@nongnu.org; Mon, 24 Oct 2016 08:06:33 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bye1F-00011k-PB for qemu-devel@nongnu.org; Mon, 24 Oct 2016 08:06:25 -0400 Date: Mon, 24 Oct 2016 23:02:48 +1100 From: David Gibson Message-ID: <20161024120248.GG11052@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> <20161024031947.GH19629@umbus.fritz.box> <20161024120231.186b10ff@bahia> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="uc35eWnScqDcQrv5" Content-Disposition: inline In-Reply-To: <20161024120231.186b10ff@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 --uc35eWnScqDcQrv5 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Oct 24, 2016 at 12:02:31PM +0200, Greg Kurz wrote: > On Mon, 24 Oct 2016 14:19:47 +1100 > David Gibson wrote: >=20 > > 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 specif= ic 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 re= turn > > > > 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 (voi= d *), > > > > and treat it as working with byte offsets. > > > >=20 > > > > To clarify this situation change iomap() and the IO accessors to ta= ke > > > > a definitely opaque BAR handle (enforced with a wrapper struct) alo= ng with > > > > 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; i= omap() > > > > 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 valida= ted. */ > > > > - if (!ahci->hba_base) { > > > > + if (!ahci->hba_bar.addr) { =20 > > >=20 > > > Isn't ahci->hba_bar supposed to be opaque ? =20 > >=20 > > Ah, good point, missed that one. And that test isn't even right, with > > the INVALID_BAR stuff. > >=20 >=20 > Indeed. >=20 > > > > return; > > > > } =20 > > >=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 ins= tead ? =20 > >=20 > > I'm pretty sure it is only called after PCI initialization, so I think > > we should just remove the check. > >=20 > > > > hba_base =3D (uint64_t)qpci_config_readl(ahci->dev, PCI_BASE_A= DDRESS_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 > > >=20 > > > Again since ahci->hba_bar is opaque, is it right to do that check her= e ? =20 > >=20 > > Not, really no. I was aware of that one, but decided to let it go > > since it's just one pretty specific check. > >=20 > > 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. > >=20 > > > I have another question about QPCI_BAR_INVALID far below (patch is > > > long :) =20 > >=20 > >=20 > > [snip] > > > > +struct QPCIBar { > > > > + uint64_t addr; > > > > +}; > > > > + > > > > +static const QPCIBar QPCI_BAR_INVALID =3D { > > > > + .addr =3D (uint64_t)-1ULL, > > > > +}; > > > > + =20 > > >=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 ? =20 > >=20 > > 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 >=20 > 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. --=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 --uc35eWnScqDcQrv5 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJYDfhkAAoJEGw4ysog2bOSFC4P/2xPHqtSSiwxktuZRAXsR5WQ QWrIniZPXMJQciHZE9/turKEB/zckHnHMCDjHauiBE66356TcSJy4d5g7N16lqBI lxO9QVMviOhC44/gkjICNdukBRfQiETMATyr7sy2DIVYB2hHLEUHTX7alxUXgiDk 0/QCufm11bNka9E72C5EoRWpSyawqXX8GdBttS8XkxXlXZS2D25JSJAL45kH4D9j fs0+XiEOsbVDpBOyPjs3sWooWKEn4lFF3smGXvGnVf+fcK4WJJnt7rjZCf4C8KY6 SpqHlt/r9TtBxsMbCtOvSvdzWFXxDsd+ta+flauMSkLWoEKE96Cv5tuhQaT+XNNv X6soGr4CH81kQDpn04vo0IGwyW2266gnvE1ekrcvArF/qP6uBMT5aPEbLi/+kw35 sqRDkspf+DVUUMTOxpFByxohQ6rnUXno5jadToHvN2qwDO8OmWCOBOEvajeibcN5 zI05FucVOoALtWkfJgEjjmm2v4henEdCmA+9TB8Gkv1IZEpSiqZ9TNtR195fvCs1 a+DP+xwXi0U0LA90eG7Y1UPoqxy2spE5+g47JVSIPSQUT7TRMzgKK5GRZKBVNq6I UD5M0uv7d9ONIcudf5xb79+7vyCHkBnPNvGVMylGZ+ePuCGCuTMFYXMFvSiJlqFE /Y3UuT5Dnm+JLTH2PW4L =YrYd -----END PGP SIGNATURE----- --uc35eWnScqDcQrv5--