diff mbox

[3/6] virtio-pci: fix 1.0 virtqueue migration

Message ID 1440147950-1178-4-git-send-email-jasowang@redhat.com
State New
Headers show

Commit Message

Jason Wang Aug. 21, 2015, 9:05 a.m. UTC 
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 modern
  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
  modern 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         | 69 ++++++++++++++++++++++++++++++++++++++++++
 hw/virtio/virtio-pci.h         | 20 +++++++-----
 hw/virtio/virtio.c             | 58 +++++++++++++++++++++++++++++++++++
 include/hw/compat.h            |  6 +++-
 include/hw/virtio/virtio-bus.h |  3 ++
 5 files changed, 148 insertions(+), 8 deletions(-)

Comments

Cornelia Huck Aug. 21, 2015, 9:43 a.m. UTC | #1 
On Fri, 21 Aug 2015 17:05:47 +0800
Jason Wang <jasowang@redhat.com> wrote:

> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 788b556..c971ba2 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -1056,6 +1056,17 @@ static bool virtio_virtqueue_needed(void *opaque)
>      return virtio_host_has_feature(vdev, VIRTIO_F_VERSION_1);
>  }
> 
> +static bool virtio_modern_state_needed(void *opaque)
> +{
> +    VirtIODevice *vdev = opaque;
> +    BusState *qbus = qdev_get_parent_bus(DEVICE(vdev));
> +    VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
> +
> +    return virtio_virtqueue_needed(opaque) &&

Why does core want to check that? Shouldn't that be done by the class
instead (but see below)?

> +        k->has_modern_state &&
> +        k->has_modern_state(qbus->parent);
> +}

I don't really like this "modern_state" stuff (which is pci specific)
creeping into core.

How about introducing "extra_state" and/or "extra_queue_state" (or
something like that) instead?
Jason Wang Aug. 24, 2015, 5:37 a.m. UTC | #2 
On 08/21/2015 05:43 PM, Cornelia Huck wrote:
> On Fri, 21 Aug 2015 17:05:47 +0800
> Jason Wang <jasowang@redhat.com> wrote:
>
>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>> index 788b556..c971ba2 100644
>> --- a/hw/virtio/virtio.c
>> +++ b/hw/virtio/virtio.c
>> @@ -1056,6 +1056,17 @@ static bool virtio_virtqueue_needed(void *opaque)
>>      return virtio_host_has_feature(vdev, VIRTIO_F_VERSION_1);
>>  }
>>
>> +static bool virtio_modern_state_needed(void *opaque)
>> +{
>> +    VirtIODevice *vdev = opaque;
>> +    BusState *qbus = qdev_get_parent_bus(DEVICE(vdev));
>> +    VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
>> +
>> +    return virtio_virtqueue_needed(opaque) &&
> Why does core want to check that? Shouldn't that be done by the class
> instead (but see below)?

Re-think about this, it should be ok to get rid of
virtio_virtioqueue_needed() here.

>> +        k->has_modern_state &&
>> +        k->has_modern_state(qbus->parent);
>> +}
> I don't really like this "modern_state" stuff (which is pci specific)
> creeping into core.
>
> How about introducing "extra_state" and/or "extra_queue_state" (or
> something like that) instead?
>

Ok, if you don't like pci specific name, maybe something like
"virtio_1_state" is better?
Cornelia Huck Aug. 24, 2015, 2:14 p.m. UTC | #3 
On Mon, 24 Aug 2015 13:37:06 +0800
Jason Wang <jasowang@redhat.com> wrote:

> On 08/21/2015 05:43 PM, Cornelia Huck wrote:
> > On Fri, 21 Aug 2015 17:05:47 +0800
> > Jason Wang <jasowang@redhat.com> wrote:

> >> +        k->has_modern_state &&
> >> +        k->has_modern_state(qbus->parent);
> >> +}
> > I don't really like this "modern_state" stuff (which is pci specific)
> > creeping into core.
> >
> > How about introducing "extra_state" and/or "extra_queue_state" (or
> > something like that) instead?
> >
> 
> Ok, if you don't like pci specific name, maybe something like
> "virtio_1_state" is better?

I was thinking more along the lines of "transport wants to save/restore
additional state for the device" - which is not neccessarily depending
on virtio-1. It would be good if a transport can extend the state
without needlessly introducing incompatibilities.

pci can handle its modern state via this then and encapsulate it.
Jason Wang Aug. 25, 2015, 3:14 a.m. UTC | #4 
On 08/24/2015 10:14 PM, Cornelia Huck wrote:
> On Mon, 24 Aug 2015 13:37:06 +0800
> Jason Wang <jasowang@redhat.com> wrote:
>
>> On 08/21/2015 05:43 PM, Cornelia Huck wrote:
>>> On Fri, 21 Aug 2015 17:05:47 +0800
>>> Jason Wang <jasowang@redhat.com> wrote:
>>>> +        k->has_modern_state &&
>>>> +        k->has_modern_state(qbus->parent);
>>>> +}
>>> I don't really like this "modern_state" stuff (which is pci specific)
>>> creeping into core.
>>>
>>> How about introducing "extra_state" and/or "extra_queue_state" (or
>>> something like that) instead?
>>>
>> Ok, if you don't like pci specific name, maybe something like
>> "virtio_1_state" is better?
> I was thinking more along the lines of "transport wants to save/restore
> additional state for the device" - which is not neccessarily depending
> on virtio-1. It would be good if a transport can extend the state
> without needlessly introducing incompatibilities.

I see.

>
> pci can handle its modern state via this then and encapsulate it.
>

Ok. Will have a try.
diff mbox

Patch

diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index c024161..d785623 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -86,6 +86,69 @@  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_modern_state(DeviceState *d)
+{
+    VirtIOPCIProxy *proxy = to_virtio_pci_proxy(d);
+
+    return proxy->flags & VIRTIO_PCI_FLAG_MIGRATE_MODERN;
+}
+
+static int virtio_pci_load_modern_state(DeviceState *d, QEMUFile *f)
+{
+    VirtIOPCIProxy *proxy = to_virtio_pci_proxy(d);
+    int i;
+
+    proxy->dfselect = qemu_get_be32(f);
+    proxy->gfselect = qemu_get_be32(f);
+    proxy->guest_features[0] = qemu_get_be32(f);
+    proxy->guest_features[1] = qemu_get_be32(f);
+    for (i = 0; i < VIRTIO_QUEUE_MAX; i++) {
+        virtio_pci_load_modern_queue_state(&proxy->vqs[i], f);
+    }
+
+    return 0;
+}
+
+static void virtio_pci_save_modern_queue_state(VirtIOPCIQueue *vq,
+                                               QEMUFile *f)
+{
+    qemu_put_be16(f, vq->num);
+    qemu_put_be16(f, vq->enabled);
+    qemu_put_be32(f, vq->desc[0]);
+    qemu_put_be32(f, vq->desc[1]);
+    qemu_put_be32(f, vq->avail[0]);
+    qemu_put_be32(f, vq->avail[1]);
+    qemu_put_be32(f, vq->used[0]);
+    qemu_put_be32(f, vq->used[1]);
+}
+
+static void virtio_pci_save_modern_state(DeviceState *d, QEMUFile *f)
+{
+    VirtIOPCIProxy *proxy = to_virtio_pci_proxy(d);
+    int i;
+
+    qemu_put_be32(f, proxy->dfselect);
+    qemu_put_be32(f, proxy->gfselect);
+    qemu_put_be32(f, proxy->guest_features[0]);
+    qemu_put_be32(f, proxy->guest_features[1]);
+    for (i = 0; i < VIRTIO_QUEUE_MAX; i++) {
+        virtio_pci_save_modern_queue_state(&proxy->vqs[i], f);
+    }
+}
+
 static void virtio_pci_save_queue(DeviceState *d, int n, QEMUFile *f)
 {
     VirtIOPCIProxy *proxy = to_virtio_pci_proxy(d);
@@ -133,6 +196,7 @@  static int virtio_pci_load_queue(DeviceState *d, int n, QEMUFile *f)
     if (vector != VIRTIO_NO_VECTOR) {
         return msix_vector_use(&proxy->pci_dev, vector);
     }
+
     return 0;
 }
 
@@ -1618,6 +1682,8 @@  static Property virtio_pci_properties[] = {
                     VIRTIO_PCI_FLAG_DISABLE_LEGACY_BIT, false),
     DEFINE_PROP_BIT("disable-modern", VirtIOPCIProxy, flags,
                     VIRTIO_PCI_FLAG_DISABLE_MODERN_BIT, true),
+    DEFINE_PROP_BIT("migrate-modern", VirtIOPCIProxy, flags,
+                    VIRTIO_PCI_FLAG_MIGRATE_MODERN_BIT, true),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -2211,6 +2277,9 @@  static void virtio_pci_bus_class_init(ObjectClass *klass, void *data)
     k->load_config = virtio_pci_load_config;
     k->save_queue = virtio_pci_save_queue;
     k->load_queue = virtio_pci_load_queue;
+    k->save_modern_state = virtio_pci_save_modern_state;
+    k->load_modern_state = virtio_pci_load_modern_state;
+    k->has_modern_state = virtio_pci_has_modern_state;
     k->query_guest_notifiers = virtio_pci_query_guest_notifiers;
     k->set_host_notifier = virtio_pci_set_host_notifier;
     k->set_guest_notifiers = virtio_pci_set_guest_notifiers;
diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h
index b6c442f..fd2e889 100644
--- a/hw/virtio/virtio-pci.h
+++ b/hw/virtio/virtio-pci.h
@@ -75,6 +75,10 @@  typedef struct VirtioBusClass VirtioPCIBusClass;
 #define VIRTIO_PCI_FLAG_DISABLE_LEGACY (1 << VIRTIO_PCI_FLAG_DISABLE_LEGACY_BIT)
 #define VIRTIO_PCI_FLAG_DISABLE_MODERN (1 << VIRTIO_PCI_FLAG_DISABLE_MODERN_BIT)
 
+/* migrate modern state ? */
+#define VIRTIO_PCI_FLAG_MIGRATE_MODERN_BIT 4
+#define VIRTIO_PCI_FLAG_MIGRATE_MODERN (1 << VIRTIO_PCI_FLAG_MIGRATE_MODERN_BIT)
+
 typedef struct {
     MSIMessage msg;
     int virq;
@@ -104,6 +108,14 @@  typedef struct VirtIOPCIRegion {
     uint32_t type;
 } VirtIOPCIRegion;
 
+typedef struct VirtIOPCIQueue {
+  uint16_t num;
+  bool enabled;
+  uint32_t desc[2];
+  uint32_t avail[2];
+  uint32_t used[2];
+} VirtIOPCIQueue;
+
 struct VirtIOPCIProxy {
     PCIDevice pci_dev;
     MemoryRegion bar;
@@ -124,13 +136,7 @@  struct VirtIOPCIProxy {
     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];
+    VirtIOPCIQueue vqs[VIRTIO_QUEUE_MAX];
 
     bool ioeventfd_disabled;
     bool ioeventfd_started;
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 788b556..c971ba2 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -1056,6 +1056,17 @@  static bool virtio_virtqueue_needed(void *opaque)
     return virtio_host_has_feature(vdev, VIRTIO_F_VERSION_1);
 }
 
+static bool virtio_modern_state_needed(void *opaque)
+{
+    VirtIODevice *vdev = opaque;
+    BusState *qbus = qdev_get_parent_bus(DEVICE(vdev));
+    VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
+
+    return virtio_virtqueue_needed(opaque) &&
+        k->has_modern_state &&
+        k->has_modern_state(qbus->parent);
+}
+
 static void put_virtqueue_state(QEMUFile *f, void *pv, size_t size)
 {
     VirtIODevice *vdev = pv;
@@ -1104,6 +1115,52 @@  static const VMStateDescription vmstate_virtio_virtqueues = {
     }
 };
 
+static int get_modern_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_modern_state(qbus->parent, f);
+
+    return ret;
+}
+
+static void put_modern_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_modern_state(qbus->parent, f);
+}
+
+static VMStateInfo vmstate_info_modern_state = {
+    .name = "virtqueue_state",
+    .get = get_modern_state,
+    .put = put_modern_state,
+};
+
+static const VMStateDescription vmstate_virtio_modern_state = {
+    .name = "virtio/modern_state",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = &virtio_modern_state_needed,
+    .fields = (VMStateField[]) {
+        {
+            .name         = "modern_state",
+            .version_id   = 0,
+            .field_exists = NULL,
+            .size         = 0,
+            .info         = &vmstate_info_modern_state,
+            .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 +1195,7 @@  static const VMStateDescription vmstate_virtio = {
         &vmstate_virtio_device_endian,
         &vmstate_virtio_64bit_features,
         &vmstate_virtio_virtqueues,
+        &vmstate_virtio_modern_state,
         NULL
     }
 };
diff --git a/include/hw/compat.h b/include/hw/compat.h
index 095de5d..f468880 100644
--- a/include/hw/compat.h
+++ b/include/hw/compat.h
@@ -2,7 +2,11 @@ 
 #define HW_COMPAT_H
 
 #define HW_COMPAT_2_4 \
-        /* empty */
+        {\
+            .driver   = "virtio-pci",\
+            .property = "migrate-modern",\
+            .value    = "off",\
+        },
 
 #define HW_COMPAT_2_3 \
         {\
diff --git a/include/hw/virtio/virtio-bus.h b/include/hw/virtio/virtio-bus.h
index 8811415..30e6b07 100644
--- a/include/hw/virtio/virtio-bus.h
+++ b/include/hw/virtio/virtio-bus.h
@@ -44,9 +44,12 @@  typedef struct VirtioBusClass {
     void (*notify)(DeviceState *d, uint16_t vector);
     void (*save_config)(DeviceState *d, QEMUFile *f);
     void (*save_queue)(DeviceState *d, int n, QEMUFile *f);
+    void (*save_modern_state)(DeviceState *d, QEMUFile *f);
     int (*load_config)(DeviceState *d, QEMUFile *f);
     int (*load_queue)(DeviceState *d, int n, QEMUFile *f);
     int (*load_done)(DeviceState *d, QEMUFile *f);
+    int (*load_modern_state)(DeviceState *d, QEMUFile *f);
+    bool (*has_modern_state)(DeviceState *d);
     bool (*query_guest_notifiers)(DeviceState *d);
     int (*set_guest_notifiers)(DeviceState *d, int nvqs, bool assign);
     int (*set_host_notifier)(DeviceState *d, int n, bool assigned);