[1/4] virtio: ring sizes vs. reset

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

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

On Thu, 10 Sep 2015 12:02:44 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

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

I dunno. If we try to figure this out while doing migration, we'll need
to go over the rings and check whether any of them has changed, then go
over the rings again to save the values - while here, we just set this
bool once when the driver changes the value, check once to find out
whether we need to migrate the values and reset once.

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

On Thu, Sep 10, 2015 at 12:22:54PM +0200, Cornelia Huck wrote:
> On Thu, 10 Sep 2015 12:02:44 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > 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/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.
> 
> I dunno. If we try to figure this out while doing migration, we'll need
> to go over the rings and check whether any of them has changed, then go
> over the rings again to save the values - while here, we just set this
> bool once when the driver changes the value, check once to find out
> whether we need to migrate the values and reset once.

Then restore it on load ...
State is worse than stateless, without a state you are
never out of sync.

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