From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59036) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bwqwm-00069Y-J7 for qemu-devel@nongnu.org; Wed, 19 Oct 2016 09:30:22 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bwqwj-0000Ln-D2 for qemu-devel@nongnu.org; Wed, 19 Oct 2016 09:30:20 -0400 Received: from 3.mo179.mail-out.ovh.net ([178.33.251.175]:58357) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1bwqwj-0000Kf-3r for qemu-devel@nongnu.org; Wed, 19 Oct 2016 09:30:17 -0400 Received: from player792.ha.ovh.net (b7.ovh.net [213.186.33.57]) by mo179.mail-out.ovh.net (Postfix) with ESMTP id 9D4D63C96 for ; Wed, 19 Oct 2016 15:30:13 +0200 (CEST) Date: Wed, 19 Oct 2016 15:29:57 +0200 From: Greg Kurz Message-ID: <20161019152957.34cc56b2@bahia> In-Reply-To: <1476879941-14360-2-git-send-email-david@gibson.dropbear.id.au> References: <1476879941-14360-1-git-send-email-david@gibson.dropbear.id.au> <1476879941-14360-2-git-send-email-david@gibson.dropbear.id.au> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCHv2 01/11] libqos: Give qvirtio_config_read*() consistent semantics 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 On Wed, 19 Oct 2016 23:25:31 +1100 David Gibson wrote: > 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 address, > but for virtio-mmio, it's an offset from the device's base mmio address. > > 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. > > 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. > > Signed-off-by: David Gibson mak > --- Reviewed-by: Greg Kurz BTW, I've dropped 'David Gibson ' from th Cc: list :) > tests/libqos/virtio-mmio.c | 16 ++++++++-------- > tests/libqos/virtio-pci.c | 25 ++++++++++++++----------- > tests/virtio-9p-test.c | 8 ++------ > tests/virtio-blk-test.c | 42 +++++++++++------------------------------- > tests/virtio-scsi-test.c | 4 +--- > 5 files changed, 36 insertions(+), 59 deletions(-) > > diff --git a/tests/libqos/virtio-mmio.c b/tests/libqos/virtio-mmio.c > index bced680..7aa8383 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" > > -static uint8_t qvirtio_mmio_config_readb(QVirtioDevice *d, uint64_t addr) > +static uint8_t qvirtio_mmio_config_readb(QVirtioDevice *d, uint64_t off) > { > QVirtioMMIODevice *dev = (QVirtioMMIODevice *)d; > - return readb(dev->addr + addr); > + return readb(dev->addr + QVIRTIO_MMIO_DEVICE_SPECIFIC + off); > } > > -static uint16_t qvirtio_mmio_config_readw(QVirtioDevice *d, uint64_t addr) > +static uint16_t qvirtio_mmio_config_readw(QVirtioDevice *d, uint64_t off) > { > QVirtioMMIODevice *dev = (QVirtioMMIODevice *)d; > - return readw(dev->addr + addr); > + return readw(dev->addr + QVIRTIO_MMIO_DEVICE_SPECIFIC + off); > } > > -static uint32_t qvirtio_mmio_config_readl(QVirtioDevice *d, uint64_t addr) > +static uint32_t qvirtio_mmio_config_readl(QVirtioDevice *d, uint64_t off) > { > QVirtioMMIODevice *dev = (QVirtioMMIODevice *)d; > - return readl(dev->addr + addr); > + return readl(dev->addr + QVIRTIO_MMIO_DEVICE_SPECIFIC + off); > } > > -static uint64_t qvirtio_mmio_config_readq(QVirtioDevice *d, uint64_t addr) > +static uint64_t qvirtio_mmio_config_readq(QVirtioDevice *d, uint64_t off) > { > QVirtioMMIODevice *dev = (QVirtioMMIODevice *)d; > - return readq(dev->addr + addr); > + return readq(dev->addr + QVIRTIO_MMIO_DEVICE_SPECIFIC + off); > } > > static uint32_t qvirtio_mmio_get_features(QVirtioDevice *d) > diff --git a/tests/libqos/virtio-pci.c b/tests/libqos/virtio-pci.c > index 7e60b3a..fa82132 100644 > --- a/tests/libqos/virtio-pci.c > +++ b/tests/libqos/virtio-pci.c > @@ -62,10 +62,13 @@ static void qvirtio_pci_assign_device(QVirtioDevice *d, void *data) > *vpcidev = (QVirtioPCIDevice *)d; > } > > -static uint8_t qvirtio_pci_config_readb(QVirtioDevice *d, uint64_t addr) > +#define CONFIG_BASE(dev) \ > + ((dev)->addr + VIRTIO_PCI_CONFIG_OFF((dev)->pdev->msix_enabled)) > + > +static uint8_t qvirtio_pci_config_readb(QVirtioDevice *d, uint64_t off) > { > QVirtioPCIDevice *dev = (QVirtioPCIDevice *)d; > - return qpci_io_readb(dev->pdev, (void *)(uintptr_t)addr); > + return qpci_io_readb(dev->pdev, CONFIG_BASE(dev) + off); > } > > /* PCI is always read in little-endian order > @@ -76,31 +79,31 @@ static uint8_t qvirtio_pci_config_readb(QVirtioDevice *d, uint64_t addr) > * case will be managed inside qvirtio_is_big_endian() > */ > > -static uint16_t qvirtio_pci_config_readw(QVirtioDevice *d, uint64_t addr) > +static uint16_t qvirtio_pci_config_readw(QVirtioDevice *d, uint64_t off) > { > QVirtioPCIDevice *dev = (QVirtioPCIDevice *)d; > uint16_t value; > > - value = qpci_io_readw(dev->pdev, (void *)(uintptr_t)addr); > + value = qpci_io_readw(dev->pdev, CONFIG_BASE(dev) + off); > if (qvirtio_is_big_endian(d)) { > value = bswap16(value); > } > return value; > } > > -static uint32_t qvirtio_pci_config_readl(QVirtioDevice *d, uint64_t addr) > +static uint32_t qvirtio_pci_config_readl(QVirtioDevice *d, uint64_t off) > { > QVirtioPCIDevice *dev = (QVirtioPCIDevice *)d; > uint32_t value; > > - value = qpci_io_readl(dev->pdev, (void *)(uintptr_t)addr); > + value = qpci_io_readl(dev->pdev, CONFIG_BASE(dev) + off); > if (qvirtio_is_big_endian(d)) { > value = bswap32(value); > } > return value; > } > > -static uint64_t qvirtio_pci_config_readq(QVirtioDevice *d, uint64_t addr) > +static uint64_t qvirtio_pci_config_readq(QVirtioDevice *d, uint64_t off) > { > QVirtioPCIDevice *dev = (QVirtioPCIDevice *)d; > int i; > @@ -108,13 +111,13 @@ static uint64_t qvirtio_pci_config_readq(QVirtioDevice *d, uint64_t addr) > > if (qvirtio_is_big_endian(d)) { > for (i = 0; i < 8; ++i) { > - u64 |= (uint64_t)qpci_io_readb(dev->pdev, > - (void *)(uintptr_t)addr + i) << (7 - i) * 8; > + u64 |= (uint64_t)qpci_io_readb(dev->pdev, CONFIG_BASE(dev) > + + off + i) << (7 - i) * 8; > } > } else { > for (i = 0; i < 8; ++i) { > - u64 |= (uint64_t)qpci_io_readb(dev->pdev, > - (void *)(uintptr_t)addr + i) << i * 8; > + u64 |= (uint64_t)qpci_io_readb(dev->pdev, CONFIG_BASE(dev) > + + off + i) << i * 8; > } > } > > diff --git a/tests/virtio-9p-test.c b/tests/virtio-9p-test.c > index 693920a..d3e19f0 100644 > --- a/tests/virtio-9p-test.c > +++ b/tests/virtio-9p-test.c > @@ -95,7 +95,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; > @@ -104,15 +103,12 @@ static void pci_basic_config(void) > qs = qvirtio_9p_start(); > v9p = qvirtio_9p_pci_init(qs); > > - addr = ((QVirtioPCIDevice *) v9p->dev)->addr + VIRTIO_PCI_CONFIG_OFF(false); > - tag_len = qvirtio_config_readw(v9p->dev, > - (uint64_t)(uintptr_t)addr); > + tag_len = qvirtio_config_readw(v9p->dev, v9p->dev, 0); > g_assert_cmpint(tag_len, ==, strlen(mount_tag)); > - addr += sizeof(uint16_t); > > tag = g_malloc(tag_len); > for (i = 0; i < tag_len; i++) { > - tag[i] = qvirtio_config_readb(v9p->dev, (uint64_t)(uintptr_t)addr + i); > + tag[i] = 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 f737c40..0e32e41 100644 > --- a/tests/virtio-blk-test.c > +++ b/tests/virtio-blk-test.c > @@ -155,7 +155,7 @@ static uint64_t virtio_blk_request(QGuestAllocator *alloc, QVirtioDevice *d, > } > > static void test_basic(QVirtioDevice *dev, QGuestAllocator *alloc, > - QVirtQueue *vq, uint64_t device_specific) > + QVirtQueue *vq) > { > QVirtioBlkReq req; > uint64_t req_addr; > @@ -165,7 +165,7 @@ static void test_basic(QVirtioDevice *dev, QGuestAllocator *alloc, > uint8_t status; > char *data; > > - capacity = qvirtio_config_readq(dev, device_specific); > + capacity = qvirtio_config_readq(dev, 0); > > g_assert_cmpint(capacity, ==, TEST_IMAGE_SIZE / 512); > > @@ -285,17 +285,13 @@ static void pci_basic(void) > QVirtioPCIDevice *dev; > QOSState *qs; > QVirtQueuePCI *vqpci; > - void *addr; > > qs = pci_test_start(); > dev = virtio_blk_pci_init(qs->pcibus, PCI_SLOT); > > vqpci = (QVirtQueuePCI *)qvirtqueue_setup(&dev->vdev, qs->alloc, 0); > > - /* MSI-X is not enabled */ > - addr = dev->addr + VIRTIO_PCI_CONFIG_OFF(false); > - > - test_basic(&dev->vdev, qs->alloc, &vqpci->vq, (uint64_t)(uintptr_t)addr); > + test_basic(&dev->vdev, qs->alloc, &vqpci->vq); > > /* End test */ > qvirtqueue_cleanup(dev->vdev.bus, &vqpci->vq, qs->alloc); > @@ -311,7 +307,6 @@ static void pci_indirect(void) > QOSState *qs; > QVirtioBlkReq req; > QVRingIndirectDesc *indirect; > - void *addr; > uint64_t req_addr; > uint64_t capacity; > uint32_t features; > @@ -323,10 +318,7 @@ static void pci_indirect(void) > > dev = virtio_blk_pci_init(qs->pcibus, PCI_SLOT); > > - /* MSI-X is not enabled */ > - addr = dev->addr + VIRTIO_PCI_CONFIG_OFF(false); > - > - capacity = qvirtio_config_readq(&dev->vdev, (uint64_t)(uintptr_t)addr); > + capacity = qvirtio_config_readq(&dev->vdev, 0); > g_assert_cmpint(capacity, ==, TEST_IMAGE_SIZE / 512); > > features = qvirtio_get_features(&dev->vdev); > @@ -406,17 +398,13 @@ static void pci_config(void) > QVirtioPCIDevice *dev; > QOSState *qs; > int n_size = TEST_IMAGE_SIZE / 2; > - void *addr; > uint64_t capacity; > > qs = pci_test_start(); > > dev = virtio_blk_pci_init(qs->pcibus, PCI_SLOT); > > - /* MSI-X is not enabled */ > - addr = dev->addr + VIRTIO_PCI_CONFIG_OFF(false); > - > - capacity = qvirtio_config_readq(&dev->vdev, (uint64_t)(uintptr_t)addr); > + capacity = qvirtio_config_readq(&dev->vdev, 0); > g_assert_cmpint(capacity, ==, TEST_IMAGE_SIZE / 512); > > qvirtio_set_driver_ok(&dev->vdev); > @@ -425,7 +413,7 @@ static void pci_config(void) > " 'size': %d } }", n_size); > qvirtio_wait_config_isr(&dev->vdev, QVIRTIO_BLK_TIMEOUT_US); > > - capacity = qvirtio_config_readq(&dev->vdev, (uint64_t)(uintptr_t)addr); > + capacity = qvirtio_config_readq(&dev->vdev, 0); > g_assert_cmpint(capacity, ==, n_size / 512); > > qvirtio_pci_device_disable(dev); > @@ -441,7 +429,6 @@ static void pci_msix(void) > QVirtQueuePCI *vqpci; > QVirtioBlkReq req; > int n_size = TEST_IMAGE_SIZE / 2; > - void *addr; > uint64_t req_addr; > uint64_t capacity; > uint32_t features; > @@ -456,10 +443,7 @@ static void pci_msix(void) > > qvirtio_pci_set_msix_configuration_vector(dev, qs->alloc, 0); > > - /* MSI-X is enabled */ > - addr = dev->addr + VIRTIO_PCI_CONFIG_OFF(true); > - > - capacity = qvirtio_config_readq(&dev->vdev, (uint64_t)(uintptr_t)addr); > + capacity = qvirtio_config_readq(&dev->vdev, 0); > g_assert_cmpint(capacity, ==, TEST_IMAGE_SIZE / 512); > > features = qvirtio_get_features(&dev->vdev); > @@ -479,7 +463,7 @@ static void pci_msix(void) > > qvirtio_wait_config_isr(&dev->vdev, QVIRTIO_BLK_TIMEOUT_US); > > - capacity = qvirtio_config_readq(&dev->vdev, (uintptr_t)addr); > + capacity = qvirtio_config_readq(&dev->vdev, 0); > g_assert_cmpint(capacity, ==, n_size / 512); > > /* Write request */ > @@ -550,7 +534,6 @@ static void pci_idx(void) > QOSState *qs; > QVirtQueuePCI *vqpci; > QVirtioBlkReq req; > - void *addr; > uint64_t req_addr; > uint64_t capacity; > uint32_t features; > @@ -565,10 +548,7 @@ static void pci_idx(void) > > qvirtio_pci_set_msix_configuration_vector(dev, qs->alloc, 0); > > - /* MSI-X is enabled */ > - addr = dev->addr + VIRTIO_PCI_CONFIG_OFF(true); > - > - capacity = qvirtio_config_readq(&dev->vdev, (uint64_t)(uintptr_t)addr); > + capacity = qvirtio_config_readq(&dev->vdev, 0); > g_assert_cmpint(capacity, ==, TEST_IMAGE_SIZE / 512); > > features = qvirtio_get_features(&dev->vdev); > @@ -709,14 +689,14 @@ static void mmio_basic(void) > alloc = generic_alloc_init(MMIO_RAM_ADDR, MMIO_RAM_SIZE, MMIO_PAGE_SIZE); > vq = qvirtqueue_setup(&dev->vdev, alloc, 0); > > - test_basic(&dev->vdev, alloc, vq, QVIRTIO_MMIO_DEVICE_SPECIFIC); > + test_basic(&dev->vdev, alloc, vq); > > qmp("{ 'execute': 'block_resize', 'arguments': { 'device': 'drive0', " > " 'size': %d } }", n_size); > > qvirtio_wait_queue_isr(&dev->vdev, vq, QVIRTIO_BLK_TIMEOUT_US); > > - capacity = qvirtio_config_readq(&dev->vdev, QVIRTIO_MMIO_DEVICE_SPECIFIC); > + capacity = qvirtio_config_readq(&dev->vdev, 0); > g_assert_cmpint(capacity, ==, n_size / 512); > > /* End test */ > diff --git a/tests/virtio-scsi-test.c b/tests/virtio-scsi-test.c > index 60dc9ab..69220ef 100644 > --- a/tests/virtio-scsi-test.c > +++ b/tests/virtio-scsi-test.c > @@ -143,7 +143,6 @@ static QVirtIOSCSI *qvirtio_scsi_pci_init(int slot) > QVirtIOSCSI *vs; > QVirtioPCIDevice *dev; > struct virtio_scsi_cmd_resp resp; > - void *addr; > int i; > > vs = g_new0(QVirtIOSCSI, 1); > @@ -161,8 +160,7 @@ static QVirtIOSCSI *qvirtio_scsi_pci_init(int slot) > qvirtio_set_acknowledge(vs->dev); > qvirtio_set_driver(vs->dev); > > - addr = dev->addr + VIRTIO_PCI_CONFIG_OFF(false); > - vs->num_queues = qvirtio_config_readl(vs->dev, (uint64_t)(uintptr_t)addr); > + vs->num_queues = qvirtio_config_readl(vs->dev, 0); > > g_assert_cmpint(vs->num_queues, <, MAX_NUM_QUEUES); >