Re: [Qemu-devel] [PATCH RFC v5 07/19] virtio: allow virtio-1 queue layout

[Cornelia Huck]

On Wed, 3 Dec 2014 12:52:51 +0200
"Michael S. Tsirkin" <address@hidden> wrote:

> On Wed, Dec 03, 2014 at 10:50:04AM +0100, Cornelia Huck wrote:

> > diff --git a/hw/virtio/virtio-mmio.c b/hw/virtio/virtio-mmio.c
> > index 43b7e02..1e2a720 100644
> > --- a/hw/virtio/virtio-mmio.c
> > +++ b/hw/virtio/virtio-mmio.c
> > @@ -244,9 +244,13 @@ 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 */
> > +        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);
> > +        virtio_queue_update_rings(vdev, vdev->queue_sel);
> >          break;
> >      case VIRTIO_MMIO_QUEUEPFN:
> >          if (value == 0) {
> 
> Let's just call virtio_queue_update_rings from virtio_queue_set_align?

You're right, set_align is legacy only so we can always call
update_rings.


> > @@ -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)
> 
> Do we have to touch this now?
> It's only used by MMIO, right?

I don't think it hurts to put a guard in here.
> 
> 
> > @@ -755,7 +768,6 @@ 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]);
> 
> Don't we need to update rings?

See above, I'll call update_rings in there.

> 
> >  }
> >  
> >  void virtio_queue_notify_vq(VirtQueue *vq)
> > @@ -949,7 +961,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 +1057,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 */
> 
> What does XXX mean here?

That I have not cared about migration of virtio-1 devices yet :)

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