From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51293) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bwioK-0003yX-QP for qemu-devel@nongnu.org; Wed, 19 Oct 2016 00:49:11 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bwioC-0001Vt-N2 for qemu-devel@nongnu.org; Wed, 19 Oct 2016 00:49:04 -0400 Date: Wed, 19 Oct 2016 13:41:10 +1100 From: David Gibson Message-ID: <20161019024110.GE11140@umbus.fritz.box> References: <1476787933-7180-1-git-send-email-david@gibson.dropbear.id.au> <1476787933-7180-2-git-send-email-david@gibson.dropbear.id.au> <20161018152709.403fbc1b@bahia> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="X3gaHHMYHkYqP6yf" Content-Disposition: inline In-Reply-To: <20161018152709.403fbc1b@bahia> Subject: Re: [Qemu-devel] [PATCH 1/8] libqos: Give qvirtio_config_read*() consistent semantics 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 --X3gaHHMYHkYqP6yf Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Oct 18, 2016 at 03:27:09PM +0200, Greg Kurz wrote: > On Tue, 18 Oct 2016 21:52:06 +1100 > David Gibson wrote: >=20 > > The 'addr' parameter to qvirtio_config_read*() doesn't have a consistent > > meaning: when using the virtio-pci versions, it's a full PCI space addr= ess, > > but for virtio-mmio, it's an offset from the device's base mmio address. > >=20 > > This means that the callers need to do different things to calculate the > > addresses in the two cases, which rather defeats the purpose of function > > pointer backends. > >=20 > > All the current users of these functions are using them to retrieve > > variables from the device specific portion of the virtio config space. > > So, this patch alters the semantics to always be an offset into that > > device specific config area. > >=20 > > Signed-off-by: David Gibson > > --- > > tests/libqos/virtio-mmio.c | 16 +++++++-------- > > tests/libqos/virtio-pci.c | 22 ++++++++++++-------- > > tests/virtio-9p-test.c | 9 ++------ > > tests/virtio-blk-test.c | 51 ++++++++++----------------------------= -------- > > tests/virtio-scsi-test.c | 5 +---- > > 5 files changed, 35 insertions(+), 68 deletions(-) > >=20 > > diff --git a/tests/libqos/virtio-mmio.c b/tests/libqos/virtio-mmio.c > > index 0cab38f..b0b74dc 100644 > > --- a/tests/libqos/virtio-mmio.c > > +++ b/tests/libqos/virtio-mmio.c > > @@ -15,28 +15,28 @@ > > #include "libqos/malloc-generic.h" > > #include "standard-headers/linux/virtio_ring.h" > > =20 > > -static uint8_t qvirtio_mmio_config_readb(QVirtioDevice *d, uint64_t ad= dr) > > +static uint8_t qvirtio_mmio_config_readb(QVirtioDevice *d, uint64_t of= f) > > { > > QVirtioMMIODevice *dev =3D (QVirtioMMIODevice *)d; > > - return readb(dev->addr + addr); > > + return readb(dev->addr + QVIRTIO_MMIO_DEVICE_SPECIFIC + off); > > } > > =20 > > -static uint16_t qvirtio_mmio_config_readw(QVirtioDevice *d, uint64_t a= ddr) > > +static uint16_t qvirtio_mmio_config_readw(QVirtioDevice *d, uint64_t o= ff) > > { > > QVirtioMMIODevice *dev =3D (QVirtioMMIODevice *)d; > > - return readw(dev->addr + addr); > > + return readw(dev->addr + QVIRTIO_MMIO_DEVICE_SPECIFIC + off); > > } > > =20 > > -static uint32_t qvirtio_mmio_config_readl(QVirtioDevice *d, uint64_t a= ddr) > > +static uint32_t qvirtio_mmio_config_readl(QVirtioDevice *d, uint64_t o= ff) > > { > > QVirtioMMIODevice *dev =3D (QVirtioMMIODevice *)d; > > - return readl(dev->addr + addr); > > + return readl(dev->addr + QVIRTIO_MMIO_DEVICE_SPECIFIC + off); > > } > > =20 > > -static uint64_t qvirtio_mmio_config_readq(QVirtioDevice *d, uint64_t a= ddr) > > +static uint64_t qvirtio_mmio_config_readq(QVirtioDevice *d, uint64_t o= ff) > > { > > QVirtioMMIODevice *dev =3D (QVirtioMMIODevice *)d; > > - return readq(dev->addr + addr); > > + return readq(dev->addr + QVIRTIO_MMIO_DEVICE_SPECIFIC + off); > > } > > =20 > > static uint32_t qvirtio_mmio_get_features(QVirtioDevice *d) > > diff --git a/tests/libqos/virtio-pci.c b/tests/libqos/virtio-pci.c > > index 6e005c1..8037724 100644 > > --- a/tests/libqos/virtio-pci.c > > +++ b/tests/libqos/virtio-pci.c > > @@ -62,39 +62,43 @@ static void qvirtio_pci_assign_device(QVirtioDevice= *d, void *data) > > *vpcidev =3D (QVirtioPCIDevice *)d; > > } > > =20 > > -static uint8_t qvirtio_pci_config_readb(QVirtioDevice *d, uint64_t add= r) > > +static uint8_t qvirtio_pci_config_readb(QVirtioDevice *d, uint64_t off) > > { > > QVirtioPCIDevice *dev =3D (QVirtioPCIDevice *)d; > > - return qpci_io_readb(dev->pdev, (void *)(uintptr_t)addr); > > + void *base =3D dev->addr + VIRTIO_PCI_CONFIG_OFF(dev->pdev->msix_e= nabled); >=20 > $ git grep VIRTIO_PCI_CONFIG_OFF tests > tests/libqos/virtio-pci.c: void *base =3D dev->addr + VIRTIO_PCI_CONFI= G_OFF(dev->pdev->msix_enabled); > tests/libqos/virtio-pci.c: void *base =3D dev->addr + VIRTIO_PCI_CONFI= G_OFF(dev->pdev->msix_enabled); > tests/libqos/virtio-pci.c: void *base =3D dev->addr + VIRTIO_PCI_CONFI= G_OFF(dev->pdev->msix_enabled); > tests/libqos/virtio-pci.c: void *base =3D dev->addr + VIRTIO_PCI_CONFI= G_OFF(dev->pdev->msix_enabled); >=20 > Maybe this could be consolidated into a helper ? Good idea, I'll add a macro. >=20 > Either way works for me since it is a definite improvement over what we c= urrently have: >=20 > Reviewed-by: Greg Kurz >=20 > > + return qpci_io_readb(dev->pdev, base + off); > > } > > =20 > > -static uint16_t qvirtio_pci_config_readw(QVirtioDevice *d, uint64_t ad= dr) > > +static uint16_t qvirtio_pci_config_readw(QVirtioDevice *d, uint64_t of= f) > > { > > QVirtioPCIDevice *dev =3D (QVirtioPCIDevice *)d; > > - return qpci_io_readw(dev->pdev, (void *)(uintptr_t)addr); > > + void *base =3D dev->addr + VIRTIO_PCI_CONFIG_OFF(dev->pdev->msix_e= nabled); > > + return qpci_io_readw(dev->pdev, base + off); > > } > > =20 > > -static uint32_t qvirtio_pci_config_readl(QVirtioDevice *d, uint64_t ad= dr) > > +static uint32_t qvirtio_pci_config_readl(QVirtioDevice *d, uint64_t of= f) > > { > > QVirtioPCIDevice *dev =3D (QVirtioPCIDevice *)d; > > - return qpci_io_readl(dev->pdev, (void *)(uintptr_t)addr); > > + void *base =3D dev->addr + VIRTIO_PCI_CONFIG_OFF(dev->pdev->msix_e= nabled); > > + return qpci_io_readl(dev->pdev, base + off); > > } > > =20 > > -static uint64_t qvirtio_pci_config_readq(QVirtioDevice *d, uint64_t ad= dr) > > +static uint64_t qvirtio_pci_config_readq(QVirtioDevice *d, uint64_t of= f) > > { > > QVirtioPCIDevice *dev =3D (QVirtioPCIDevice *)d; > > int i; > > uint64_t u64 =3D 0; > > + void *base =3D dev->addr + VIRTIO_PCI_CONFIG_OFF(dev->pdev->msix_e= nabled); > > =20 > > if (target_big_endian()) { > > for (i =3D 0; i < 8; ++i) { > > u64 |=3D (uint64_t)qpci_io_readb(dev->pdev, > > - (void *)(uintptr_t)addr + i) << (7 - i= ) * 8; > > + base + off + i) << (7 - i) = * 8; > > } > > } else { > > for (i =3D 0; i < 8; ++i) { > > u64 |=3D (uint64_t)qpci_io_readb(dev->pdev, > > - (void *)(uintptr_t)addr + i) << i * 8; > > + base + off + i) << i * 8; > > } > > } > > =20 > > diff --git a/tests/virtio-9p-test.c b/tests/virtio-9p-test.c > > index e8b2196..620e523 100644 > > --- a/tests/virtio-9p-test.c > > +++ b/tests/virtio-9p-test.c > > @@ -92,7 +92,6 @@ static void qvirtio_9p_pci_free(QVirtIO9P *v9p) > > static void pci_basic_config(void) > > { > > QVirtIO9P *v9p; > > - void *addr; > > size_t tag_len; > > char *tag; > > int i; > > @@ -100,16 +99,12 @@ static void pci_basic_config(void) > > qvirtio_9p_start(); > > v9p =3D qvirtio_9p_pci_init(); > > =20 > > - addr =3D ((QVirtioPCIDevice *) v9p->dev)->addr + VIRTIO_PCI_CONFIG= _OFF(false); > > - tag_len =3D qvirtio_config_readw(&qvirtio_pci, v9p->dev, > > - (uint64_t)(uintptr_t)addr); > > + tag_len =3D qvirtio_config_readw(&qvirtio_pci, v9p->dev, 0); > > g_assert_cmpint(tag_len, =3D=3D, strlen(mount_tag)); > > - addr +=3D sizeof(uint16_t); > > =20 > > tag =3D g_malloc(tag_len); > > for (i =3D 0; i < tag_len; i++) { > > - tag[i] =3D qvirtio_config_readb(&qvirtio_pci, v9p->dev, > > - (uint64_t)(uintptr_t)addr + i); > > + tag[i] =3D qvirtio_config_readb(&qvirtio_pci, v9p->dev, i + 2); > > } > > g_assert_cmpmem(tag, tag_len, mount_tag, tag_len); > > g_free(tag); > > diff --git a/tests/virtio-blk-test.c b/tests/virtio-blk-test.c > > index 0506917..9162a5d 100644 > > --- a/tests/virtio-blk-test.c > > +++ b/tests/virtio-blk-test.c > > @@ -151,7 +151,7 @@ static uint64_t virtio_blk_request(QGuestAllocator = *alloc, QVirtioBlkReq *req, > > } > > =20 > > static void test_basic(const QVirtioBus *bus, QVirtioDevice *dev, > > - QGuestAllocator *alloc, QVirtQueue *vq, uint64_t device_sp= ecific) > > + QGuestAllocator *alloc, QVirtQueue *vq) > > { > > QVirtioBlkReq req; > > uint64_t req_addr; > > @@ -161,7 +161,7 @@ static void test_basic(const QVirtioBus *bus, QVirt= ioDevice *dev, > > uint8_t status; > > char *data; > > =20 > > - capacity =3D qvirtio_config_readq(bus, dev, device_specific); > > + capacity =3D qvirtio_config_readq(bus, dev, 0); > > =20 > > g_assert_cmpint(capacity, =3D=3D, TEST_IMAGE_SIZE / 512); > > =20 > > @@ -282,7 +282,6 @@ static void pci_basic(void) > > QPCIBus *bus; > > QVirtQueuePCI *vqpci; > > QGuestAllocator *alloc; > > - void *addr; > > =20 > > bus =3D pci_test_start(); > > dev =3D virtio_blk_pci_init(bus, PCI_SLOT); > > @@ -291,11 +290,7 @@ static void pci_basic(void) > > vqpci =3D (QVirtQueuePCI *)qvirtqueue_setup(&qvirtio_pci, &dev->vd= ev, > > al= loc, 0); > > =20 > > - /* MSI-X is not enabled */ > > - addr =3D dev->addr + VIRTIO_PCI_CONFIG_OFF(false); > > - > > - test_basic(&qvirtio_pci, &dev->vdev, alloc, &vqpci->vq, > > - (uint64_t)(uintptr= _t)addr); > > + test_basic(&qvirtio_pci, &dev->vdev, alloc, &vqpci->vq); > > =20 > > /* End test */ > > qvirtqueue_cleanup(&qvirtio_pci, &vqpci->vq, alloc); > > @@ -314,7 +309,6 @@ static void pci_indirect(void) > > QGuestAllocator *alloc; > > QVirtioBlkReq req; > > QVRingIndirectDesc *indirect; > > - void *addr; > > uint64_t req_addr; > > uint64_t capacity; > > uint32_t features; > > @@ -326,11 +320,7 @@ static void pci_indirect(void) > > =20 > > dev =3D virtio_blk_pci_init(bus, PCI_SLOT); > > =20 > > - /* MSI-X is not enabled */ > > - addr =3D dev->addr + VIRTIO_PCI_CONFIG_OFF(false); > > - > > - capacity =3D qvirtio_config_readq(&qvirtio_pci, &dev->vdev, > > - (uint64_t)(uintptr= _t)addr); > > + capacity =3D qvirtio_config_readq(&qvirtio_pci, &dev->vdev, 0); > > g_assert_cmpint(capacity, =3D=3D, TEST_IMAGE_SIZE / 512); > > =20 > > features =3D qvirtio_get_features(&qvirtio_pci, &dev->vdev); > > @@ -414,18 +404,13 @@ static void pci_config(void) > > QVirtioPCIDevice *dev; > > QPCIBus *bus; > > int n_size =3D TEST_IMAGE_SIZE / 2; > > - void *addr; > > uint64_t capacity; > > =20 > > bus =3D pci_test_start(); > > =20 > > dev =3D virtio_blk_pci_init(bus, PCI_SLOT); > > =20 > > - /* MSI-X is not enabled */ > > - addr =3D dev->addr + VIRTIO_PCI_CONFIG_OFF(false); > > - > > - capacity =3D qvirtio_config_readq(&qvirtio_pci, &dev->vdev, > > - (uint64_t)(uintptr= _t)addr); > > + capacity =3D qvirtio_config_readq(&qvirtio_pci, &dev->vdev, 0); > > g_assert_cmpint(capacity, =3D=3D, TEST_IMAGE_SIZE / 512); > > =20 > > qvirtio_set_driver_ok(&qvirtio_pci, &dev->vdev); > > @@ -434,8 +419,7 @@ static void pci_config(void) > > " 'size': %d } }",= n_size); > > qvirtio_wait_config_isr(&qvirtio_pci, &dev->vdev, QVIRTIO_BLK_TIME= OUT_US); > > =20 > > - capacity =3D qvirtio_config_readq(&qvirtio_pci, &dev->vdev, > > - (uint64_t)(uintptr= _t)addr); > > + capacity =3D qvirtio_config_readq(&qvirtio_pci, &dev->vdev, 0); > > g_assert_cmpint(capacity, =3D=3D, n_size / 512); > > =20 > > qvirtio_pci_device_disable(dev); > > @@ -452,7 +436,6 @@ static void pci_msix(void) > > QGuestAllocator *alloc; > > QVirtioBlkReq req; > > int n_size =3D TEST_IMAGE_SIZE / 2; > > - void *addr; > > uint64_t req_addr; > > uint64_t capacity; > > uint32_t features; > > @@ -468,11 +451,7 @@ static void pci_msix(void) > > =20 > > qvirtio_pci_set_msix_configuration_vector(dev, alloc, 0); > > =20 > > - /* MSI-X is enabled */ > > - addr =3D dev->addr + VIRTIO_PCI_CONFIG_OFF(true); > > - > > - capacity =3D qvirtio_config_readq(&qvirtio_pci, &dev->vdev, > > - (uint64_t)(uintptr= _t)addr); > > + capacity =3D qvirtio_config_readq(&qvirtio_pci, &dev->vdev, 0); > > g_assert_cmpint(capacity, =3D=3D, TEST_IMAGE_SIZE / 512); > > =20 > > features =3D qvirtio_get_features(&qvirtio_pci, &dev->vdev); > > @@ -493,8 +472,7 @@ static void pci_msix(void) > > =20 > > qvirtio_wait_config_isr(&qvirtio_pci, &dev->vdev, QVIRTIO_BLK_TIME= OUT_US); > > =20 > > - capacity =3D qvirtio_config_readq(&qvirtio_pci, &dev->vdev, > > - (uint64_t)(uintptr= _t)addr); > > + capacity =3D qvirtio_config_readq(&qvirtio_pci, &dev->vdev, 0); > > g_assert_cmpint(capacity, =3D=3D, n_size / 512); > > =20 > > /* Write request */ > > @@ -568,7 +546,6 @@ static void pci_idx(void) > > QVirtQueuePCI *vqpci; > > QGuestAllocator *alloc; > > QVirtioBlkReq req; > > - void *addr; > > uint64_t req_addr; > > uint64_t capacity; > > uint32_t features; > > @@ -584,11 +561,7 @@ static void pci_idx(void) > > =20 > > qvirtio_pci_set_msix_configuration_vector(dev, alloc, 0); > > =20 > > - /* MSI-X is enabled */ > > - addr =3D dev->addr + VIRTIO_PCI_CONFIG_OFF(true); > > - > > - capacity =3D qvirtio_config_readq(&qvirtio_pci, &dev->vdev, > > - (uint64_t)(uintptr= _t)addr); > > + capacity =3D qvirtio_config_readq(&qvirtio_pci, &dev->vdev, 0); > > g_assert_cmpint(capacity, =3D=3D, TEST_IMAGE_SIZE / 512); > > =20 > > features =3D qvirtio_get_features(&qvirtio_pci, &dev->vdev); > > @@ -731,8 +704,7 @@ static void mmio_basic(void) > > alloc =3D generic_alloc_init(MMIO_RAM_ADDR, MMIO_RAM_SIZE, MMIO_PA= GE_SIZE); > > vq =3D qvirtqueue_setup(&qvirtio_mmio, &dev->vdev, alloc, 0); > > =20 > > - test_basic(&qvirtio_mmio, &dev->vdev, alloc, vq, > > - QVIRTIO_MMIO_DEVICE_SPECIFIC); > > + test_basic(&qvirtio_mmio, &dev->vdev, alloc, vq); > > =20 > > qmp("{ 'execute': 'block_resize', 'arguments': { 'device': 'drive0= ', " > > " 'size': %d } }",= n_size); > > @@ -740,8 +712,7 @@ static void mmio_basic(void) > > qvirtio_wait_queue_isr(&qvirtio_mmio, &dev->vdev, vq, > > QVIRTIO_BLK_TIMEOUT_US); > > =20 > > - capacity =3D qvirtio_config_readq(&qvirtio_mmio, &dev->vdev, > > - QVIRTIO_MMIO_DEVICE_SP= ECIFIC); > > + capacity =3D qvirtio_config_readq(&qvirtio_mmio, &dev->vdev, 0); > > g_assert_cmpint(capacity, =3D=3D, n_size / 512); > > =20 > > /* End test */ > > diff --git a/tests/virtio-scsi-test.c b/tests/virtio-scsi-test.c > > index 79088bb..2df8f9a 100644 > > --- a/tests/virtio-scsi-test.c > > +++ b/tests/virtio-scsi-test.c > > @@ -141,7 +141,6 @@ static QVirtIOSCSI *qvirtio_scsi_pci_init(int slot) > > QVirtIOSCSI *vs; > > QVirtioPCIDevice *dev; > > struct virtio_scsi_cmd_resp resp; > > - void *addr; > > int i; > > =20 > > vs =3D g_new0(QVirtIOSCSI, 1); > > @@ -158,9 +157,7 @@ static QVirtIOSCSI *qvirtio_scsi_pci_init(int slot) > > qvirtio_set_acknowledge(&qvirtio_pci, vs->dev); > > qvirtio_set_driver(&qvirtio_pci, vs->dev); > > =20 > > - addr =3D dev->addr + VIRTIO_PCI_CONFIG_OFF(false); > > - vs->num_queues =3D qvirtio_config_readl(&qvirtio_pci, vs->dev, > > - (uint64_t)(uintptr_t)addr); > > + vs->num_queues =3D qvirtio_config_readl(&qvirtio_pci, vs->dev, 0); > > =20 > > g_assert_cmpint(vs->num_queues, <, MAX_NUM_QUEUES); > > =20 >=20 --=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 --X3gaHHMYHkYqP6yf Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJYBt1DAAoJEGw4ysog2bOS778P/iNlzrUNNF7AklcuHlh9TNNG jgfwNIYvQIioXj2VkwY9rw4rsduxZE4EENJNZzcTx0p0KTAhGomaieftuAdhrc4K 3doWpuN1NXphnjX0KMOKuTKzdQd/uSIahgI+5jS92S8njCV8kigbCC3rok6V5pHi e5tTm38mwvTdgxIF0sX1xRXjQ4ad2QwI/pgeyaDWjNkNqVC3nnq4o8O5502fze/b JNF4UtYSyy5Re3+0GATVmBS3NSTC/VZKIa4e3XGf23p9l4lsDMYnEmdVEHsrY9xT kP+24u/f0f4ckvZQ1Jp9766eHO3NlXWSfMTMM2PsF0UGXTATJ2NEa3Y1v5SIYT9i FWL6QbTYxq27BtAs3l6u/ptTcf7J7bvwZnowrph3oQsc7TXDxPiFkifh5Iz+cQ3v rpb/EQdqLRs0NJsJjjlZHFgS+INEV2FLctDj4/bZOdK4I7YJR312PwnPYZ0QUkCo 4pVh7kaxLGJEXsEnO23jJEjbgZcW3EeuiTvHunypUf0SfhAGzHbSvtKd8CJOTd6W KZGSDwdtDQ04E5DYm6DI5btZqSyD8iauRwxMI6BIw7ZO6nZZL6I8b5dM6dadReHk OOXtoRMOoo683O0QOW2OUIEBDJrx8AAN0JPMbOjTWpKm/QUnGNjKatF+gWAOg5t+ F7Zv4Z4TJKibNaHaMtSp =G90S -----END PGP SIGNATURE----- --X3gaHHMYHkYqP6yf--