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