Linux Virtualization - Jan 2015 - [Qemu-devel] [PATCH RFC v6 07/20] virtio: allow virtio-1 queue layout

On Thu, Dec 11, 2014 at 02:25:09PM +0100, Cornelia Huck
wrote:
> For virtio-1 devices, we allow a more complex queue layout that doesn't
> require descriptor table and rings on a physically-contigous memory area:
> add virtio_queue_set_rings() to allow transports to set this up.
> 
> Signed-off-by: Cornelia Huck <cornelia.huck at de.ibm.com>
> ---
>  hw/virtio/virtio-mmio.c    |    3 +++
>  hw/virtio/virtio.c         |   53
++++++++++++++++++++++++++++----------------
>  include/hw/virtio/virtio.h |    3 +++
>  3 files changed, 40 insertions(+), 19 deletions(-)
> 
> diff --git a/hw/virtio/virtio-mmio.c b/hw/virtio/virtio-mmio.c
> index 43b7e02..0c9b63b 100644
> --- a/hw/virtio/virtio-mmio.c
> +++ b/hw/virtio/virtio-mmio.c
> @@ -244,8 +244,11 @@ static void virtio_mmio_write(void *opaque, hwaddr
offset, uint64_t value,
>      case VIRTIO_MMIO_QUEUENUM:
>          DPRINTF("mmio_queue write %d max %d\n", (int)value,
VIRTQUEUE_MAX_SIZE);
>          virtio_queue_set_num(vdev, vdev->queue_sel, value);
> +        /* Note: only call this function for legacy devices */
It's not clear to me if this is an assertion that this *does* only
call the function for legacy devices or a fixme, that it *should* only
call the function for legacy devices.
> +        virtio_queue_update_rings(vdev, vdev->queue_sel);
>          break;
>      case VIRTIO_MMIO_QUEUEALIGN:
> +        /* Note: this is only valid for legacy devices */
>          virtio_queue_set_align(vdev, vdev->queue_sel, value);
>          break;
>      case VIRTIO_MMIO_QUEUEPFN:
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 8f69ffa..57190ba 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -69,7 +69,6 @@ typedef struct VRing
>  struct VirtQueue
>  {
>      VRing vring;
> -    hwaddr pa;
>      uint16_t last_avail_idx;
>      /* Last used index value we have signalled on */
>      uint16_t signalled_used;
> @@ -92,15 +91,18 @@ struct VirtQueue
>  };
>  
>  /* virt queue functions */
> -static void virtqueue_init(VirtQueue *vq)
> +void virtio_queue_update_rings(VirtIODevice *vdev, int n)
Perhaps something in the name to emphasise that this is only for <v1.0
devices?
>  {
> -    hwaddr pa = vq->pa;
> +    VRing *vring = &vdev->vq[n].vring;
>  
> -    vq->vring.desc = pa;
> -    vq->vring.avail = pa + vq->vring.num * sizeof(VRingDesc);
> -    vq->vring.used = vring_align(vq->vring.avail +
> -                                 offsetof(VRingAvail,
ring[vq->vring.num]),
> -                                 vq->vring.align);
> +    if (!vring->desc) {
> +        /* not yet setup -> nothing to do */
> +        return;
> +    }
> +    vring->avail = vring->desc + vring->num * sizeof(VRingDesc);
> +    vring->used = vring_align(vring->avail +
> +                              offsetof(VRingAvail, ring[vring->num]),
> +                              vring->align);
Would it make sense to implement this in terms of
virtio_queue_set_rings()?
>  }
>  
>  static inline uint64_t vring_desc_addr(VirtIODevice *vdev, hwaddr desc_pa,
> @@ -605,7 +607,6 @@ void virtio_reset(void *opaque)
>          vdev->vq[i].vring.avail = 0;
>          vdev->vq[i].vring.used = 0;
>          vdev->vq[i].last_avail_idx = 0;
> -        vdev->vq[i].pa = 0;
>          vdev->vq[i].vector = VIRTIO_NO_VECTOR;
>          vdev->vq[i].signalled_used = 0;
>          vdev->vq[i].signalled_used_valid = false;
> @@ -708,13 +709,21 @@ void virtio_config_writel(VirtIODevice *vdev,
uint32_t addr, uint32_t data)
>  
>  void virtio_queue_set_addr(VirtIODevice *vdev, int n, hwaddr addr)
>  {
> -    vdev->vq[n].pa = addr;
> -    virtqueue_init(&vdev->vq[n]);
> +    vdev->vq[n].vring.desc = addr;
> +    virtio_queue_update_rings(vdev, n);
>  }
>  
>  hwaddr virtio_queue_get_addr(VirtIODevice *vdev, int n)
>  {
> -    return vdev->vq[n].pa;
> +    return vdev->vq[n].vring.desc;
> +}
> +
> +void virtio_queue_set_rings(VirtIODevice *vdev, int n, hwaddr desc,
> +                            hwaddr avail, hwaddr used)
> +{
> +    vdev->vq[n].vring.desc = desc;
> +    vdev->vq[n].vring.avail = avail;
> +    vdev->vq[n].vring.used = used;
>  }
>  
>  void virtio_queue_set_num(VirtIODevice *vdev, int n, int num)
> @@ -728,7 +737,6 @@ void virtio_queue_set_num(VirtIODevice *vdev, int n,
int num)
>          return;
>      }
>      vdev->vq[n].vring.num = num;
> -    virtqueue_init(&vdev->vq[n]);
>  }
>  
>  int virtio_queue_get_num(VirtIODevice *vdev, int n)
> @@ -748,6 +756,11 @@ void virtio_queue_set_align(VirtIODevice *vdev, int n,
int align)
>      BusState *qbus = qdev_get_parent_bus(DEVICE(vdev));
>      VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
>  
> +    /* virtio-1 compliant devices cannot change the aligment */
> +    if (virtio_has_feature(vdev, VIRTIO_F_VERSION_1)) {
> +        error_report("tried to modify queue alignment for virtio-1
device");
> +        return;
> +    }
>      /* Check that the transport told us it was going to do this
>       * (so a buggy transport will immediately assert rather than
>       * silently failing to migrate this state)
> @@ -755,7 +768,7 @@ void virtio_queue_set_align(VirtIODevice *vdev, int n,
int align)
>      assert(k->has_variable_vring_alignment);
>  
>      vdev->vq[n].vring.align = align;
> -    virtqueue_init(&vdev->vq[n]);
> +    virtio_queue_update_rings(vdev, n);
>  }
>  
>  void virtio_queue_notify_vq(VirtQueue *vq)
> @@ -949,7 +962,8 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f)
>          if (k->has_variable_vring_alignment) {
>              qemu_put_be32(f, vdev->vq[i].vring.align);
>          }
> -        qemu_put_be64(f, vdev->vq[i].pa);
> +        /* XXX virtio-1 devices */
> +        qemu_put_be64(f, vdev->vq[i].vring.desc);
>          qemu_put_be16s(f, &vdev->vq[i].last_avail_idx);
>          if (k->save_queue) {
>              k->save_queue(qbus->parent, i, f);
> @@ -1044,13 +1058,14 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f,
int version_id)
>          if (k->has_variable_vring_alignment) {
>              vdev->vq[i].vring.align = qemu_get_be32(f);
>          }
> -        vdev->vq[i].pa = qemu_get_be64(f);
> +        vdev->vq[i].vring.desc = qemu_get_be64(f);
>          qemu_get_be16s(f, &vdev->vq[i].last_avail_idx);
>          vdev->vq[i].signalled_used_valid = false;
>          vdev->vq[i].notification = true;
>  
> -        if (vdev->vq[i].pa) {
> -            virtqueue_init(&vdev->vq[i]);
> +        if (vdev->vq[i].vring.desc) {
> +            /* XXX virtio-1 devices */
> +            virtio_queue_update_rings(vdev, i);
>          } else if (vdev->vq[i].last_avail_idx) {
>              error_report("VQ %d address 0x0 "
>                           "inconsistent with Host index 0x%x",
> @@ -1084,7 +1099,7 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int
version_id)
>      }
>  
>      for (i = 0; i < num; i++) {
> -        if (vdev->vq[i].pa) {
> +        if (vdev->vq[i].vring.desc) {
>              uint16_t nheads;
>              nheads = vring_avail_idx(&vdev->vq[i]) -
vdev->vq[i].last_avail_idx;
>              /* Check it isn't doing strange things with descriptor
numbers. */
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index 68c40db..b63ced3 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -224,6 +224,9 @@ void virtio_queue_set_addr(VirtIODevice *vdev, int n,
hwaddr addr);
>  hwaddr virtio_queue_get_addr(VirtIODevice *vdev, int n);
>  void virtio_queue_set_num(VirtIODevice *vdev, int n, int num);
>  int virtio_queue_get_num(VirtIODevice *vdev, int n);
> +void virtio_queue_set_rings(VirtIODevice *vdev, int n, hwaddr desc,
> +                            hwaddr avail, hwaddr used);
> +void virtio_queue_update_rings(VirtIODevice *vdev, int n);
>  void virtio_queue_set_align(VirtIODevice *vdev, int n, int align);
>  void virtio_queue_notify(VirtIODevice *vdev, int n);
>  uint16_t virtio_queue_vector(VirtIODevice *vdev, int n);
-- 
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL:
<http://lists.linuxfoundation.org/pipermail/virtualization/attachments/20150122/231dcc8e/attachment-0001.sig>

On Thu, 22 Jan 2015 13:06:09 +1100
David Gibson <david at gibson.dropbear.id.au> wrote:
> On Thu, Dec 11, 2014 at 02:25:09PM +0100, Cornelia Huck wrote:
> > For virtio-1 devices, we allow a more complex queue layout that
doesn't
> > require descriptor table and rings on a physically-contigous memory
area:
> > add virtio_queue_set_rings() to allow transports to set this up.
> > 
> > Signed-off-by: Cornelia Huck <cornelia.huck at de.ibm.com>
> > ---
> >  hw/virtio/virtio-mmio.c    |    3 +++
> >  hw/virtio/virtio.c         |   53
++++++++++++++++++++++++++++----------------
> >  include/hw/virtio/virtio.h |    3 +++
> >  3 files changed, 40 insertions(+), 19 deletions(-)
> > 
> > diff --git a/hw/virtio/virtio-mmio.c b/hw/virtio/virtio-mmio.c
> > index 43b7e02..0c9b63b 100644
> > --- a/hw/virtio/virtio-mmio.c
> > +++ b/hw/virtio/virtio-mmio.c
> > @@ -244,8 +244,11 @@ static void virtio_mmio_write(void *opaque,
hwaddr offset, uint64_t value,
> >      case VIRTIO_MMIO_QUEUENUM:
> >          DPRINTF("mmio_queue write %d max %d\n", (int)value,
VIRTQUEUE_MAX_SIZE);
> >          virtio_queue_set_num(vdev, vdev->queue_sel, value);
> > +        /* Note: only call this function for legacy devices */
> 
> It's not clear to me if this is an assertion that this *does* only
> call the function for legacy devices or a fixme, that it *should* only
> call the function for legacy devices.
It's more like a note to whoever takes the virtio-mmio legacy device
code and writes a virtio-1 virtio-mmio device.

Does
/* Note: this function must only be called for legacy devices */
make that intention clearer?
> 
> > +        virtio_queue_update_rings(vdev, vdev->queue_sel);
> >          break;
> >      case VIRTIO_MMIO_QUEUEALIGN:
> > +        /* Note: this is only valid for legacy devices */
> >          virtio_queue_set_align(vdev, vdev->queue_sel, value);
> >          break;
> >      case VIRTIO_MMIO_QUEUEPFN:
(...)
> >  /* virt queue functions */
> > -static void virtqueue_init(VirtQueue *vq)
> > +void virtio_queue_update_rings(VirtIODevice *vdev, int n)
> 
> Perhaps something in the name to emphasise that this is only for <v1.0
> devices?
virtio_queue_legacy_update_rings()? Maybe a bit long...
> 
> >  {
> > -    hwaddr pa = vq->pa;
> > +    VRing *vring = &vdev->vq[n].vring;
> >  
> > -    vq->vring.desc = pa;
> > -    vq->vring.avail = pa + vq->vring.num * sizeof(VRingDesc);
> > -    vq->vring.used = vring_align(vq->vring.avail +
> > -                                 offsetof(VRingAvail,
ring[vq->vring.num]),
> > -                                 vq->vring.align);
> > +    if (!vring->desc) {
> > +        /* not yet setup -> nothing to do */
> > +        return;
> > +    }
> > +    vring->avail = vring->desc + vring->num *
sizeof(VRingDesc);
> > +    vring->used = vring_align(vring->avail +
> > +                              offsetof(VRingAvail,
ring[vring->num]),
> > +                              vring->align);
> 
> Would it make sense to implement this in terms of
> virtio_queue_set_rings()?
Perhaps a bit confusing, since that would re-write desc.
> 
> >  }

On Wed, Jan 28, 2015 at 05:07:01PM +0100, Cornelia Huck
wrote:
> On Thu, 22 Jan 2015 13:06:09 +1100
> David Gibson <david at gibson.dropbear.id.au> wrote:
> 
> > On Thu, Dec 11, 2014 at 02:25:09PM +0100, Cornelia Huck wrote:
> > > For virtio-1 devices, we allow a more complex queue layout that
doesn't
> > > require descriptor table and rings on a physically-contigous
memory area:
> > > add virtio_queue_set_rings() to allow transports to set this up.
> > > 
> > > Signed-off-by: Cornelia Huck <cornelia.huck at de.ibm.com>
> > > ---
> > >  hw/virtio/virtio-mmio.c    |    3 +++
> > >  hw/virtio/virtio.c         |   53
++++++++++++++++++++++++++++----------------
> > >  include/hw/virtio/virtio.h |    3 +++
> > >  3 files changed, 40 insertions(+), 19 deletions(-)
> > > 
> > > diff --git a/hw/virtio/virtio-mmio.c b/hw/virtio/virtio-mmio.c
> > > index 43b7e02..0c9b63b 100644
> > > --- a/hw/virtio/virtio-mmio.c
> > > +++ b/hw/virtio/virtio-mmio.c
> > > @@ -244,8 +244,11 @@ static void virtio_mmio_write(void *opaque,
hwaddr offset, uint64_t value,
> > >      case VIRTIO_MMIO_QUEUENUM:
> > >          DPRINTF("mmio_queue write %d max %d\n",
(int)value, VIRTQUEUE_MAX_SIZE);
> > >          virtio_queue_set_num(vdev, vdev->queue_sel, value);
> > > +        /* Note: only call this function for legacy devices */
> > 
> > It's not clear to me if this is an assertion that this *does* only
> > call the function for legacy devices or a fixme, that it *should* only
> > call the function for legacy devices.
> 
> It's more like a note to whoever takes the virtio-mmio legacy device
> code and writes a virtio-1 virtio-mmio device.
> 
> Does
> /* Note: this function must only be called for legacy devices */
> make that intention clearer?
Yes, I think that's better.
> 
> > 
> > > +        virtio_queue_update_rings(vdev, vdev->queue_sel);
> > >          break;
> > >      case VIRTIO_MMIO_QUEUEALIGN:
> > > +        /* Note: this is only valid for legacy devices */
> > >          virtio_queue_set_align(vdev, vdev->queue_sel, value);
> > >          break;
> > >      case VIRTIO_MMIO_QUEUEPFN:
> 
> (...)
> 
> > >  /* virt queue functions */
> > > -static void virtqueue_init(VirtQueue *vq)
> > > +void virtio_queue_update_rings(VirtIODevice *vdev, int n)
> > 
> > Perhaps something in the name to emphasise that this is only for
<v1.0
> > devices?
> 
> virtio_queue_legacy_update_rings()? Maybe a bit long...
There aren't many callers, so I think long is ok in this case.

-- 
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL:
<http://lists.linuxfoundation.org/pipermail/virtualization/attachments/20150129/be072140/attachment.sig>