Re: [Qemu-devel] [PATCH 1/8] libqos: Give qvirtio_config_read*() consistent semantics

[David Gibson]

On Tue, Oct 18, 2016 at 03:27:09PM +0200, Greg Kurz wrote:
> On Tue, 18 Oct 2016 21:52:06 +1100
> David Gibson <address@hidden> 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>
> > ---
> >  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(-)
> > 
> > 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"
> >  
> > -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 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 = (QVirtioPCIDevice *)d;
> >  }
> >  
> > -static uint8_t qvirtio_pci_config_readb(QVirtioDevice *d, uint64_t addr)
> > +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);
> > +    void *base = dev->addr + 
> > VIRTIO_PCI_CONFIG_OFF(dev->pdev->msix_enabled);
> 
> $ git grep VIRTIO_PCI_CONFIG_OFF tests
> tests/libqos/virtio-pci.c:    void *base = dev->addr + 
> VIRTIO_PCI_CONFIG_OFF(dev->pdev->msix_enabled);
> tests/libqos/virtio-pci.c:    void *base = dev->addr + 
> VIRTIO_PCI_CONFIG_OFF(dev->pdev->msix_enabled);
> tests/libqos/virtio-pci.c:    void *base = dev->addr + 
> VIRTIO_PCI_CONFIG_OFF(dev->pdev->msix_enabled);
> tests/libqos/virtio-pci.c:    void *base = dev->addr + 
> VIRTIO_PCI_CONFIG_OFF(dev->pdev->msix_enabled);
> 
> Maybe this could be consolidated into a helper ?

Good idea, I'll add a macro.

> 
> Either way works for me since it is a definite improvement over what we 
> currently have:
> 
> Reviewed-by: Greg Kurz <address@hidden>
> 
> > +    return qpci_io_readb(dev->pdev, base + off);
> >  }
> >  
> > -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;
> > -    return qpci_io_readw(dev->pdev, (void *)(uintptr_t)addr);
> > +    void *base = dev->addr + 
> > VIRTIO_PCI_CONFIG_OFF(dev->pdev->msix_enabled);
> > +    return qpci_io_readw(dev->pdev, base + off);
> >  }
> >  
> > -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;
> > -    return qpci_io_readl(dev->pdev, (void *)(uintptr_t)addr);
> > +    void *base = dev->addr + 
> > VIRTIO_PCI_CONFIG_OFF(dev->pdev->msix_enabled);
> > +    return qpci_io_readl(dev->pdev, base + off);
> >  }
> >  
> > -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;
> >      uint64_t u64 = 0;
> > +    void *base = dev->addr + 
> > VIRTIO_PCI_CONFIG_OFF(dev->pdev->msix_enabled);
> >  
> >      if (target_big_endian()) {
> >          for (i = 0; i < 8; ++i) {
> >              u64 |= (uint64_t)qpci_io_readb(dev->pdev,
> > -                                (void *)(uintptr_t)addr + i) << (7 - i) * 
> > 8;
> > +                                           base + 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;
> > +                                           base + off + i) << i * 8;
> >          }
> >      }
> >  
> > 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 = qvirtio_9p_pci_init();
> >  
> > -    addr = ((QVirtioPCIDevice *) v9p->dev)->addr + 
> > VIRTIO_PCI_CONFIG_OFF(false);
> > -    tag_len = qvirtio_config_readw(&qvirtio_pci, v9p->dev,
> > -                                   (uint64_t)(uintptr_t)addr);
> > +    tag_len = qvirtio_config_readw(&qvirtio_pci, 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(&qvirtio_pci, 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 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,
> >  }
> >  
> >  static void test_basic(const QVirtioBus *bus, QVirtioDevice *dev,
> > -            QGuestAllocator *alloc, QVirtQueue *vq, uint64_t 
> > device_specific)
> > +                       QGuestAllocator *alloc, QVirtQueue *vq)
> >  {
> >      QVirtioBlkReq req;
> >      uint64_t req_addr;
> > @@ -161,7 +161,7 @@ static void test_basic(const QVirtioBus *bus, 
> > QVirtioDevice *dev,
> >      uint8_t status;
> >      char *data;
> >  
> > -    capacity = qvirtio_config_readq(bus, dev, device_specific);
> > +    capacity = qvirtio_config_readq(bus, dev, 0);
> >  
> >      g_assert_cmpint(capacity, ==, TEST_IMAGE_SIZE / 512);
> >  
> > @@ -282,7 +282,6 @@ static void pci_basic(void)
> >      QPCIBus *bus;
> >      QVirtQueuePCI *vqpci;
> >      QGuestAllocator *alloc;
> > -    void *addr;
> >  
> >      bus = pci_test_start();
> >      dev = virtio_blk_pci_init(bus, PCI_SLOT);
> > @@ -291,11 +290,7 @@ static void pci_basic(void)
> >      vqpci = (QVirtQueuePCI *)qvirtqueue_setup(&qvirtio_pci, &dev->vdev,
> >                                                                      alloc, 
> > 0);
> >  
> > -    /* MSI-X is not enabled */
> > -    addr = 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);
> >  
> >      /* 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)
> >  
> >      dev = virtio_blk_pci_init(bus, PCI_SLOT);
> >  
> > -    /* MSI-X is not enabled */
> > -    addr = dev->addr + VIRTIO_PCI_CONFIG_OFF(false);
> > -
> > -    capacity = qvirtio_config_readq(&qvirtio_pci, &dev->vdev,
> > -                                                    
> > (uint64_t)(uintptr_t)addr);
> > +    capacity = qvirtio_config_readq(&qvirtio_pci, &dev->vdev, 0);
> >      g_assert_cmpint(capacity, ==, TEST_IMAGE_SIZE / 512);
> >  
> >      features = qvirtio_get_features(&qvirtio_pci, &dev->vdev);
> > @@ -414,18 +404,13 @@ static void pci_config(void)
> >      QVirtioPCIDevice *dev;
> >      QPCIBus *bus;
> >      int n_size = TEST_IMAGE_SIZE / 2;
> > -    void *addr;
> >      uint64_t capacity;
> >  
> >      bus = pci_test_start();
> >  
> >      dev = virtio_blk_pci_init(bus, PCI_SLOT);
> >  
> > -    /* MSI-X is not enabled */
> > -    addr = dev->addr + VIRTIO_PCI_CONFIG_OFF(false);
> > -
> > -    capacity = qvirtio_config_readq(&qvirtio_pci, &dev->vdev,
> > -                                                    
> > (uint64_t)(uintptr_t)addr);
> > +    capacity = qvirtio_config_readq(&qvirtio_pci, &dev->vdev, 0);
> >      g_assert_cmpint(capacity, ==, TEST_IMAGE_SIZE / 512);
> >  
> >      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_TIMEOUT_US);
> >  
> > -    capacity = qvirtio_config_readq(&qvirtio_pci, &dev->vdev,
> > -                                                    
> > (uint64_t)(uintptr_t)addr);
> > +    capacity = qvirtio_config_readq(&qvirtio_pci, &dev->vdev, 0);
> >      g_assert_cmpint(capacity, ==, n_size / 512);
> >  
> >      qvirtio_pci_device_disable(dev);
> > @@ -452,7 +436,6 @@ static void pci_msix(void)
> >      QGuestAllocator *alloc;
> >      QVirtioBlkReq req;
> >      int n_size = 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)
> >  
> >      qvirtio_pci_set_msix_configuration_vector(dev, alloc, 0);
> >  
> > -    /* MSI-X is enabled */
> > -    addr = dev->addr + VIRTIO_PCI_CONFIG_OFF(true);
> > -
> > -    capacity = qvirtio_config_readq(&qvirtio_pci, &dev->vdev,
> > -                                                    
> > (uint64_t)(uintptr_t)addr);
> > +    capacity = qvirtio_config_readq(&qvirtio_pci, &dev->vdev, 0);
> >      g_assert_cmpint(capacity, ==, TEST_IMAGE_SIZE / 512);
> >  
> >      features = qvirtio_get_features(&qvirtio_pci, &dev->vdev);
> > @@ -493,8 +472,7 @@ static void pci_msix(void)
> >  
> >      qvirtio_wait_config_isr(&qvirtio_pci, &dev->vdev, 
> > QVIRTIO_BLK_TIMEOUT_US);
> >  
> > -    capacity = qvirtio_config_readq(&qvirtio_pci, &dev->vdev,
> > -                                                    
> > (uint64_t)(uintptr_t)addr);
> > +    capacity = qvirtio_config_readq(&qvirtio_pci, &dev->vdev, 0);
> >      g_assert_cmpint(capacity, ==, n_size / 512);
> >  
> >      /* 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)
> >  
> >      qvirtio_pci_set_msix_configuration_vector(dev, alloc, 0);
> >  
> > -    /* MSI-X is enabled */
> > -    addr = dev->addr + VIRTIO_PCI_CONFIG_OFF(true);
> > -
> > -    capacity = qvirtio_config_readq(&qvirtio_pci, &dev->vdev,
> > -                                                    
> > (uint64_t)(uintptr_t)addr);
> > +    capacity = qvirtio_config_readq(&qvirtio_pci, &dev->vdev, 0);
> >      g_assert_cmpint(capacity, ==, TEST_IMAGE_SIZE / 512);
> >  
> >      features = qvirtio_get_features(&qvirtio_pci, &dev->vdev);
> > @@ -731,8 +704,7 @@ static void mmio_basic(void)
> >      alloc = generic_alloc_init(MMIO_RAM_ADDR, MMIO_RAM_SIZE, 
> > MMIO_PAGE_SIZE);
> >      vq = qvirtqueue_setup(&qvirtio_mmio, &dev->vdev, alloc, 0);
> >  
> > -    test_basic(&qvirtio_mmio, &dev->vdev, alloc, vq,
> > -                            QVIRTIO_MMIO_DEVICE_SPECIFIC);
> > +    test_basic(&qvirtio_mmio, &dev->vdev, alloc, vq);
> >  
> >      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);
> >  
> > -    capacity = qvirtio_config_readq(&qvirtio_mmio, &dev->vdev,
> > -                                                
> > QVIRTIO_MMIO_DEVICE_SPECIFIC);
> > +    capacity = qvirtio_config_readq(&qvirtio_mmio, &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 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;
> >  
> >      vs = 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);
> >  
> > -    addr = dev->addr + VIRTIO_PCI_CONFIG_OFF(false);
> > -    vs->num_queues = qvirtio_config_readl(&qvirtio_pci, vs->dev,
> > -                                          (uint64_t)(uintptr_t)addr);
> > +    vs->num_queues = qvirtio_config_readl(&qvirtio_pci, vs->dev, 0);
> >  
> >      g_assert_cmpint(vs->num_queues, <, MAX_NUM_QUEUES);
> >  
> 

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

signature.asc
Description: PGP signature