From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52615) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1byc5o-0004bD-Fh for qemu-devel@nongnu.org; Mon, 24 Oct 2016 06:03:00 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1byc5k-00019n-Qh for qemu-devel@nongnu.org; Mon, 24 Oct 2016 06:02:56 -0400 Received: from 6.mo6.mail-out.ovh.net ([87.98.177.69]:55899) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1byc5k-00019W-L7 for qemu-devel@nongnu.org; Mon, 24 Oct 2016 06:02:52 -0400 Received: from player786.ha.ovh.net (b7.ovh.net [213.186.33.57]) by mo6.mail-out.ovh.net (Postfix) with ESMTP id 91645554EA for ; Mon, 24 Oct 2016 12:02:50 +0200 (CEST) Date: Mon, 24 Oct 2016 12:02:31 +0200 From: Greg Kurz Message-ID: <20161024120231.186b10ff@bahia> In-Reply-To: <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> <20161024031947.GH19629@umbus.fritz.box> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; boundary="Sig_/IJHpJ26c8gbTFZJljiNG7Ay"; 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_/IJHpJ26c8gbTFZJljiNG7Ay Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: quoted-printable On Mon, 24 Oct 2016 14:19:47 +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 wrote: > > =20 > > > 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 retu= rn > > > value from iomap is supposed to be an opaque token. > > >=20 > > > ..except that the callers expect to be able to add offsets to it. Wh= ich > > > 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= 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; iom= ap() > > > 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 validate= d. */ > > > - 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 Indeed. > > > 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 inste= ad ? =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_ADD= RESS_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 here = ? =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 Shouldn't all QPCIBar structures be initialized to BAR_INVALID then ? --Sig_/IJHpJ26c8gbTFZJljiNG7Ay Content-Type: application/pgp-signature Content-Description: OpenPGP digital signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iEYEARECAAYFAlgN3DgACgkQAvw66wEB28LXBwCeKIplksXRS9gh11chMT1qd5qz DeAAn128yOuw7bIK0OkpH6WNMpZHVoTr =ln7C -----END PGP SIGNATURE----- --Sig_/IJHpJ26c8gbTFZJljiNG7Ay--