Re: [Qemu-devel] [PATCH V2 4/8] virtio-pci: fix 1.0 virtqueue migration

[Cornelia Huck]

On Wed,  2 Sep 2015 11:25:21 +0800
Jason Wang <address@hidden> wrote:

> We don't migrate the followings fields for virtio-pci:
> 
> uint32_t dfselect;
> uint32_t gfselect;
> uint32_t guest_features[2];
> struct {
>     uint16_t num;
>     bool enabled;
>     uint32_t desc[2];
>     uint32_t avail[2];
>     uint32_t used[2];
> } vqs[VIRTIO_QUEUE_MAX];
> 
> This will confuse driver if migrating during initialization. Solves
> this issue by:
> 
> - introduce transport specific callbacks to load and store extra
>   virtqueue states.
> - add a new subsection for virtio to migrate transport specific modern
>   device state.
> - implement pci specific callbacks.
> - add a new property for virtio-pci for whether or not to migrate
>   extra state.
> - compat the migration for 2.4 and elder machine types
> 
> Cc: Michael S. Tsirkin <address@hidden>
> Signed-off-by: Jason Wang <address@hidden>
> ---
>  hw/virtio/virtio-pci.c         | 129 
> +++++++++++++++++++++++++++++++++++++++++
>  hw/virtio/virtio-pci.h         |  20 ++++---
>  hw/virtio/virtio.c             |  57 ++++++++++++++++++
>  include/hw/compat.h            |   6 +-
>  include/hw/virtio/virtio-bus.h |   3 +
>  5 files changed, 207 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index c024161..a96890b 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -86,6 +86,129 @@ static void virtio_pci_save_config(DeviceState *d, 
> QEMUFile *f)
>          qemu_put_be16(f, vdev->config_vector);
>  }
> 
> +static void virtio_pci_load_modern_queue_state(VirtIOPCIQueue *vq,
> +                                               QEMUFile *f)
> +{
> +    vq->num = qemu_get_be16(f);
> +    vq->enabled = qemu_get_be16(f);
> +    vq->desc[0] = qemu_get_be32(f);
> +    vq->desc[1] = qemu_get_be32(f);
> +    vq->avail[0] = qemu_get_be32(f);
> +    vq->avail[1] = qemu_get_be32(f);
> +    vq->used[0] = qemu_get_be32(f);
> +    vq->used[1] = qemu_get_be32(f);
> +}
> +
> +static bool virtio_pci_has_extra_state(DeviceState *d)
> +{
> +    VirtIOPCIProxy *proxy = to_virtio_pci_proxy(d);
> +
> +    return proxy->flags & VIRTIO_PCI_FLAG_MIGRATE_EXTRA;

Hm, couldn't you already check whether this is a modern device here?
You don't need to save state for a legacy device, do you?

> +}
> +

(...)

> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 788b556..dcaf022 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -1056,6 +1056,16 @@ static bool virtio_virtqueue_needed(void *opaque)
>      return virtio_host_has_feature(vdev, VIRTIO_F_VERSION_1);
>  }
> 
> +static bool virtio_extra_state_needed(void *opaque)
> +{
> +    VirtIODevice *vdev = opaque;
> +    BusState *qbus = qdev_get_parent_bus(DEVICE(vdev));
> +    VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
> +
> +    return k->has_extra_state &&
> +        k->has_extra_state(qbus->parent);
> +}
> +
>  static void put_virtqueue_state(QEMUFile *f, void *pv, size_t size)
>  {
>      VirtIODevice *vdev = pv;
> @@ -1104,6 +1114,52 @@ static const VMStateDescription 
> vmstate_virtio_virtqueues = {
>      }
>  };
> 
> +static int get_extra_state(QEMUFile *f, void *pv, size_t size)
> +{
> +    VirtIODevice *vdev = pv;
> +    BusState *qbus = qdev_get_parent_bus(DEVICE(vdev));
> +    VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
> +    int ret = 0;
> +
> +    ret = k->load_extra_state(qbus->parent, f);

Should we check for ->load_extra_state() and return failure if it does
not exist?

> +
> +    return ret;
> +}
> +
> +static void put_extra_state(QEMUFile *f, void *pv, size_t size)
> +{
> +    VirtIODevice *vdev = pv;
> +    BusState *qbus = qdev_get_parent_bus(DEVICE(vdev));
> +    VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
> +
> +    k->save_extra_state(qbus->parent, f);

Maybe add an assert(k->save_extra_state) here, as this means the
transport code is buggy?

> +}
> +
> +static VMStateInfo vmstate_info_extra_state = {
> +    .name = "virtqueue_extra_state",
> +    .get = get_extra_state,
> +    .put = put_extra_state,
> +};
> +
> +static const VMStateDescription vmstate_virtio_modern_state = {

Should probably be named "extra_state".

> +    .name = "virtio/extra_state",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .needed = &virtio_extra_state_needed,
> +    .fields = (VMStateField[]) {
> +        {
> +            .name         = "modern_state",

Also here.

> +            .version_id   = 0,
> +            .field_exists = NULL,
> +            .size         = 0,
> +            .info         = &vmstate_info_extra_state,
> +            .flags        = VMS_SINGLE,
> +            .offset       = 0,
> +        },
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  static const VMStateDescription vmstate_virtio_device_endian = {
>      .name = "virtio/device_endian",
>      .version_id = 1,