From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53459) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bx4GP-0003mq-KM for qemu-devel@nongnu.org; Wed, 19 Oct 2016 23:43:30 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bx4GO-0006Xv-9T for qemu-devel@nongnu.org; Wed, 19 Oct 2016 23:43:29 -0400 Date: Thu, 20 Oct 2016 14:34:22 +1100 From: David Gibson Message-ID: <20161020033422.GO11140@umbus.fritz.box> References: <1476879941-14360-1-git-send-email-david@gibson.dropbear.id.au> <1476879941-14360-12-git-send-email-david@gibson.dropbear.id.au> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="MdEjg5WkSuUg8x46" Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] [PATCHv2 11/11] libqos: Change PCI accessors to take opaque BAR handle List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Laurent Vivier Cc: pbonzini@redhat.com, qemu-devel@nongnu.org, qemu-ppc@nongnu.org, agraf@suse.de, stefanha@redhat.com, mst@redhat.com, aik@ozlabs.ru, mdroth@linux.vnet.ibm.com, groug@kaod.org, thuth@redhat.com --MdEjg5WkSuUg8x46 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Oct 19, 2016 at 04:35:24PM +0200, Laurent Vivier wrote: >=20 >=20 > On 19/10/2016 14:25, David Gibson wrote: > > 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 | 46 +++++++----- > > 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, 305 insertions(+), 311 deletions(-) > >=20 > ... > > diff --git a/tests/libqos/pci.c b/tests/libqos/pci.c > > index 3021651..30ddf19 100644 > > --- a/tests/libqos/pci.c > > +++ b/tests/libqos/pci.c > > @@ -104,7 +104,6 @@ void qpci_msix_enable(QPCIDevice *dev) > > uint32_t table; > > uint8_t bir_table; > > uint8_t bir_pba; > > - void *offset; > > =20 > > addr =3D qpci_find_capability(dev, PCI_CAP_ID_MSIX); > > g_assert_cmphex(addr, !=3D, 0); > > @@ -114,18 +113,16 @@ void qpci_msix_enable(QPCIDevice *dev) > > =20 > > table =3D qpci_config_readl(dev, addr + PCI_MSIX_TABLE); > > bir_table =3D table & PCI_MSIX_FLAGS_BIRMASK; > > - offset =3D qpci_iomap(dev, bir_table, NULL); > > - dev->msix_table =3D offset + (table & ~PCI_MSIX_FLAGS_BIRMASK); > > + dev->msix_table_bar =3D qpci_iomap(dev, bir_table, NULL); > > + dev->msix_table_off =3D table & ~PCI_MSIX_FLAGS_BIRMASK; > > =20 > > table =3D qpci_config_readl(dev, addr + PCI_MSIX_PBA); > > bir_pba =3D table & PCI_MSIX_FLAGS_BIRMASK; > > if (bir_pba !=3D bir_table) { > > - offset =3D qpci_iomap(dev, bir_pba, NULL); > > + dev->msix_pba_bar =3D qpci_iomap(dev, bir_pba, NULL); > > } > > - dev->msix_pba =3D offset + (table & ~PCI_MSIX_FLAGS_BIRMASK); > > + dev->msix_pba_off =3D table & ~PCI_MSIX_FLAGS_BIRMASK; > > =20 > > - g_assert(dev->msix_table !=3D NULL); > > - g_assert(dev->msix_pba !=3D NULL); > > dev->msix_enabled =3D true; > > } > > =20 > > @@ -141,22 +138,25 @@ void qpci_msix_disable(QPCIDevice *dev) > > qpci_config_writew(dev, addr + PCI_MSIX_FLAGS, > > val & ~PCI_MSIX_FLAGS_= ENABLE); > > =20 > > - qpci_iounmap(dev, dev->msix_table); > > - qpci_iounmap(dev, dev->msix_pba); > > + qpci_iounmap(dev, dev->msix_table_bar); > > + qpci_iounmap(dev, dev->msix_pba_bar); > > dev->msix_enabled =3D 0; > > - dev->msix_table =3D NULL; > > - dev->msix_pba =3D NULL; > > + memset(&dev->msix_table_bar, 0, sizeof(dev->msix_table_bar)); > > + memset(&dev->msix_pba_bar, 0, sizeof(dev->msix_pba_bar)); >=20 >=20 > If it's opaque, you should not know what is the value to unset it, > perhaps you could define a "QPCIBAR_INVALID" and > set "bar =3D QPCIBAR_INVALID"? Ah, good idea. --=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 --MdEjg5WkSuUg8x46 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJYCDs8AAoJEGw4ysog2bOSdgQQAI13rTkCCqwQ5r1eRftLtCNg AeqWDH/A7uDgtsOG3zqisryZZgv4oOCYL3xrp+IZNhIY01MaG2sUsfw2iu/U78nf enU/qs2DYt/AT2reYWGMYUJIviVgt2DMWU7SNtEG+nLAOOQW+uubv4TGHL0/mWPx nekCDV+tv59vRUpO0IMQavkb5x5N9VfRcLarcALsIX4HS8cgLsLRwrXEb6Bvrcm5 hpT5xaIkBtH/ba3jTNqgbWKaujcC6Txn5TSCZyq3YRZj/TXzGrnC9bZ97pN5vJI6 qdc13kW5IFPB4oEVVPi8AtbAJij8fPKIPCcTxF9Kt5StyP4YOwWHm71w8W1kuZF+ Vo8qRXq+ayz+D1w3vva4P2M5T47/hdwZWk8Dqq8uVx20XxCEtYPT/eqGRPzsrBQE RWyykoEHL8GSS4yZgvhEEt3eVCj89ZdVYQOUCbzGoum+3L/vKAS0d8BHz9EWskTZ dws7r5fkN/hptNliox7oX5tIO5EDfA/3hwUNIvJBxwPClXziVx6Ad/66lLTnlBDX keqUlererJihvYquJn8FEUvqyrdc6V5MsMo/HLyz5OZbbQJZBORT1SDAfS0TG/PX C1gRtsfkifyO6lrwAjmPwo6Qa1GSWfu8ybzO3pqLdkcGVYnhaTLfFS5fnN828yvN 5k8hUodqBmU3xMQkOhrk =r/GR -----END PGP SIGNATURE----- --MdEjg5WkSuUg8x46--