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

[Jason Wang <jasowang@redhat.com>]

On 09/02/2015 07:06 PM, Cornelia Huck wrote:
> On Wed,  2 Sep 2015 11:25:21 +0800
> Jason Wang <jasowang@redhat.com> 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 <mst@redhat.com>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> ---
>>  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?

It was prepared for future. If we have something more that needs to be
migrated specifically for virtio-pci. We could just add a new subsection
for vmstate_virtio_pci without touching the core again. So we check the
modern in its subsection instead of here.
 
>
>> +}
>> +
> (...)
>
>> 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?

I think checking the existence of has_extra_state() in
virtio_extra_state_needed() is enough for this? Or is there anything I miss?

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

Right.

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

Will fix this.

Thanks.

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