Re: [Qemu-devel] [PATCHv2 01/11] libqos: Give qvirtio_config_read*() consistent semantics

[Laurent Vivier]

On 19/10/2016 14:25, 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 <address@hidden>mak

Reviewed-by: Laurent Vivier <address@hidden>

> ---
>  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);
>  
>