From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54451) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bycGt-00029F-14 for qemu-devel@nongnu.org; Mon, 24 Oct 2016 06:14:24 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bycGp-0003jm-S0 for qemu-devel@nongnu.org; Mon, 24 Oct 2016 06:14:23 -0400 Received: from 17.mo6.mail-out.ovh.net ([46.105.36.150]:34334) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1bycGp-0003jJ-M4 for qemu-devel@nongnu.org; Mon, 24 Oct 2016 06:14:19 -0400 Received: from player786.ha.ovh.net (b7.ovh.net [213.186.33.57]) by mo6.mail-out.ovh.net (Postfix) with ESMTP id 720C75549C for ; Mon, 24 Oct 2016 12:14:18 +0200 (CEST) Date: Mon, 24 Oct 2016 12:14:05 +0200 From: Greg Kurz Message-ID: <20161024121405.3ba91cd1@bahia> In-Reply-To: <20161024043112.GJ19629@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> <20161024043112.GJ19629@umbus.fritz.box> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; boundary="Sig_/QsMq6=4t2b68zeuo7ef5lhm"; protocol="application/pgp-signature" 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: David Gibson 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 --Sig_/QsMq6=4t2b68zeuo7ef5lhm Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: quoted-printable On Mon, 24 Oct 2016 15:31:12 +1100 David Gibson 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: =20 > > > 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 > > > > 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 >=20 > 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. >=20 Indeed, I now realize that test_migrate_sanity() does migrate a not yet enabled device. --Sig_/QsMq6=4t2b68zeuo7ef5lhm Content-Type: application/pgp-signature Content-Description: OpenPGP digital signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iEYEARECAAYFAlgN3u0ACgkQAvw66wEB28LT5ACfdV/XySeVSxZBHf+osRCo/60f MZ8AoKRCbNaUbgH4GcrajWITs278uzVG =GhJ5 -----END PGP SIGNATURE----- --Sig_/QsMq6=4t2b68zeuo7ef5lhm--