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

2016-10-18 Thread 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  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 
> > ---
> >  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 
> 
> > +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,

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

2016-10-18 Thread Greg Kurz 
On Tue, 18 Oct 2016 21:52:06 +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 
> ---
>  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 ?

Either way works for me since it is a definite improvement over what we 
currently have:

Reviewed-by: Greg Kurz 

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

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

2016-10-18 Thread Laurent Vivier 


On 18/10/2016 12:52, 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 

Reviewed-by: Laurent Vivier 

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