Re: [Qemu-devel] [PATCH 1/4] virtio: ring sizes vs. reset

["Michael S. Tsirkin" <mst@redhat.com>]

On Fri, Sep 04, 2015 at 10:54:26AM +0200, Cornelia Huck wrote:
> We allow guests to change the size of the virtqueue rings by supplying
> a number of buffers that is different from the number of buffers the
> device was initialized with. Current code has some problems, however,
> since reset does not reset the ringsizes to the default values (as this
> is not saved anywhere).
> 
> Let's extend the core code to keep track of the default ringsizes and
> migrate them once the guest changed them for any of the virtqueues
> for a device.
> 
> Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> ---
>  hw/virtio/virtio.c         | 63 ++++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/virtio/virtio.h |  1 +
>  2 files changed, 64 insertions(+)
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 788b556..687116e 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -60,6 +60,7 @@ typedef struct VRingUsed
>  typedef struct VRing
>  {
>      unsigned int num;
> +    unsigned int num_default;
>      unsigned int align;
>      hwaddr desc;
>      hwaddr avail;
> @@ -633,7 +634,9 @@ void virtio_reset(void *opaque)
>          vdev->vq[i].signalled_used = 0;
>          vdev->vq[i].signalled_used_valid = false;
>          vdev->vq[i].notification = true;
> +        vdev->vq[i].vring.num = vdev->vq[i].vring.num_default;
>      }
> +    vdev->non_default_ringsizes = false;
>  }
>  
>  uint32_t virtio_config_readb(VirtIODevice *vdev, uint32_t addr)
> @@ -855,6 +858,10 @@ void virtio_queue_set_num(VirtIODevice *vdev, int n, int num)
>          return;
>      }
>      vdev->vq[n].vring.num = num;
> +    if (num != vdev->vq[n].vring.num_default) {
> +        /* save ringsizes once one of them has been changed */
> +        vdev->non_default_ringsizes = true;
> +    }
>  }
>  
>  VirtQueue *virtio_vector_first_queue(VirtIODevice *vdev, uint16_t vector)
> @@ -964,6 +971,7 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int queue_size,
>          abort();
>  
>      vdev->vq[i].vring.num = queue_size;
> +    vdev->vq[i].vring.num_default = queue_size;
>      vdev->vq[i].vring.align = VIRTIO_PCI_VRING_ALIGN;
>      vdev->vq[i].handle_output = handle_output;
>  
> @@ -977,6 +985,7 @@ void virtio_del_queue(VirtIODevice *vdev, int n)
>      }
>  
>      vdev->vq[n].vring.num = 0;
> +    vdev->vq[n].vring.num_default = 0;
>  }
>  
>  void virtio_irq(VirtQueue *vq)
> @@ -1056,6 +1065,13 @@ static bool virtio_virtqueue_needed(void *opaque)
>      return virtio_host_has_feature(vdev, VIRTIO_F_VERSION_1);
>  }
>  
> +static bool virtio_ringsize_needed(void *opaque)
> +{
> +    VirtIODevice *vdev = opaque;
> +
> +    return vdev->non_default_ringsizes;
> +}
> +
>  static void put_virtqueue_state(QEMUFile *f, void *pv, size_t size)
>  {
>      VirtIODevice *vdev = pv;
> @@ -1104,6 +1120,52 @@ static const VMStateDescription vmstate_virtio_virtqueues = {
>      }
>  };
>  
> +static void put_ringsize_state(QEMUFile *f, void *pv, size_t size)
> +{
> +    VirtIODevice *vdev = pv;
> +    int i;
> +
> +    for (i = 0; i < VIRTIO_QUEUE_MAX; i++) {
> +        qemu_put_be32(f, vdev->vq[i].vring.num_default);
> +    }
> +}
> +
> +static int get_ringsize_state(QEMUFile *f, void *pv, size_t size)
> +{
> +    VirtIODevice *vdev = pv;
> +    int i;
> +
> +    for (i = 0; i < VIRTIO_QUEUE_MAX; i++) {
> +        vdev->vq[i].vring.num_default = qemu_get_be32(f);
> +    }
> +    return 0;
> +}
> +
> +static VMStateInfo vmstate_info_ringsize = {
> +    .name = "ringsize_state",
> +    .get = get_ringsize_state,
> +    .put = put_ringsize_state,
> +};
> +
> +static const VMStateDescription vmstate_virtio_ringsize = {
> +    .name = "virtio/ringsize",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .needed = &virtio_ringsize_needed,
> +    .fields = (VMStateField[]) {
> +        {
> +            .name         = "ringsize",
> +            .version_id   = 0,
> +            .field_exists = NULL,
> +            .size         = 0,
> +            .info         = &vmstate_info_ringsize,
> +            .flags        = VMS_SINGLE,
> +            .offset       = 0,
> +        },
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  static const VMStateDescription vmstate_virtio_device_endian = {
>      .name = "virtio/device_endian",
>      .version_id = 1,
> @@ -1138,6 +1200,7 @@ static const VMStateDescription vmstate_virtio = {
>          &vmstate_virtio_device_endian,
>          &vmstate_virtio_64bit_features,
>          &vmstate_virtio_virtqueues,
> +        &vmstate_virtio_ringsize,
>          NULL
>      }
>  };
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index cccae89..29870c8 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -90,6 +90,7 @@ struct VirtIODevice
>      VMChangeStateEntry *vmstate;
>      char *bus_name;
>      uint8_t device_endian;
> +    bool non_default_ringsizes;


Let's not try to track this separately. Just go over
rings before migration, and check whether guest changed
anything.

>      QLIST_HEAD(, VirtQueue) *vector_queues;
>  };
>  
> -- 
> 2.3.8