[Qemu-devel] [PATCH] virtio-net: unbreak self announcement and guest offloads after migration

[Qemu-devel] [PATCH] virtio-net: unbreak self announcement and guest offloads after migration

[Jason Wang]

After commit 019a3edbb25f1571e876f8af1ce4c55412939e5d ("virtio: make
features 64bit wide"). Device's guest_features was actually set after
vdc->load(). This breaks the assumption that device specific load()
function can check guest_features. For virtio-net, self announcement
and guest offloads won't work after migration.

Fixing this by defer them to virtio_net_load() where guest_features
were guaranteed to be set. Other virtio devices looks fine.

Fixes: 019a3edbb25f1571e876f8af1ce4c55412939e5d
       ("virtio: make features 64bit wide")
Cc: qemu-stable@nongnu.org
Cc: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 hw/net/virtio-net.c | 40 +++++++++++++++++++++++-----------------
 1 file changed, 23 insertions(+), 17 deletions(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index f72eebf..2775e6a 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -1458,11 +1458,33 @@ static int virtio_net_load(QEMUFile *f, void *opaque, int version_id)
 {
     VirtIONet *n = opaque;
     VirtIODevice *vdev = VIRTIO_DEVICE(n);
+    int ret;
 
     if (version_id < 2 || version_id > VIRTIO_NET_VM_VERSION)
         return -EINVAL;
 
-    return virtio_load(vdev, f, version_id);
+    ret = virtio_load(vdev, f, version_id);
+    if (ret) {
+        return ret;
+    }
+
+    if (virtio_vdev_has_feature(vdev, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS)) {
+        n->curr_guest_offloads = qemu_get_be64(f);
+    } else {
+        n->curr_guest_offloads = virtio_net_supported_guest_offloads(n);
+    }
+
+    if (peer_has_vnet_hdr(n)) {
+        virtio_net_apply_guest_offloads(n);
+    }
+
+    if (virtio_vdev_has_feature(vdev, VIRTIO_NET_F_GUEST_ANNOUNCE) &&
+        virtio_vdev_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ)) {
+        n->announce_counter = SELF_ANNOUNCE_ROUNDS;
+        timer_mod(n->announce_timer, qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL));
+    }
+
+    return 0;
 }
 
 static int virtio_net_load_device(VirtIODevice *vdev, QEMUFile *f,
@@ -1559,16 +1581,6 @@ static int virtio_net_load_device(VirtIODevice *vdev, QEMUFile *f,
         }
     }
 
-    if (virtio_vdev_has_feature(vdev, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS)) {
-        n->curr_guest_offloads = qemu_get_be64(f);
-    } else {
-        n->curr_guest_offloads = virtio_net_supported_guest_offloads(n);
-    }
-
-    if (peer_has_vnet_hdr(n)) {
-        virtio_net_apply_guest_offloads(n);
-    }
-
     virtio_net_set_queues(n);
 
     /* Find the first multicast entry in the saved MAC filter */
@@ -1586,12 +1598,6 @@ static int virtio_net_load_device(VirtIODevice *vdev, QEMUFile *f,
         qemu_get_subqueue(n->nic, i)->link_down = link_down;
     }
 
-    if (virtio_vdev_has_feature(vdev, VIRTIO_NET_F_GUEST_ANNOUNCE) &&
-        virtio_vdev_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ)) {
-        n->announce_counter = SELF_ANNOUNCE_ROUNDS;
-        timer_mod(n->announce_timer, qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL));
-    }
-
     return 0;
 }
 
-- 
2.1.4

Re: [Qemu-devel] [PATCH] virtio-net: unbreak self announcement and guest offloads after migration

[Cornelia Huck]

On Fri, 11 Sep 2015 16:01:56 +0800
Jason Wang <jasowang@redhat.com> wrote:

> After commit 019a3edbb25f1571e876f8af1ce4c55412939e5d ("virtio: make
> features 64bit wide"). Device's guest_features was actually set after
> vdc->load(). This breaks the assumption that device specific load()
> function can check guest_features. For virtio-net, self announcement
> and guest offloads won't work after migration.
> 
> Fixing this by defer them to virtio_net_load() where guest_features
> were guaranteed to be set. Other virtio devices looks fine.
> 
> Fixes: 019a3edbb25f1571e876f8af1ce4c55412939e5d
>        ("virtio: make features 64bit wide")
> Cc: qemu-stable@nongnu.org
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  hw/net/virtio-net.c | 40 +++++++++++++++++++++++-----------------
>  1 file changed, 23 insertions(+), 17 deletions(-)

Migration support for virtio is really a twisty maze, it's easy to make
mistakes like that :(

Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>

Re: [Qemu-devel] [PATCH] virtio-net: unbreak self announcement and guest offloads after migration

[Greg Kurz]

On Fri, 11 Sep 2015 10:30:21 +0200
Cornelia Huck <cornelia.huck@de.ibm.com> wrote:

> On Fri, 11 Sep 2015 16:01:56 +0800
> Jason Wang <jasowang@redhat.com> wrote:
> 
> > After commit 019a3edbb25f1571e876f8af1ce4c55412939e5d ("virtio: make
> > features 64bit wide"). Device's guest_features was actually set after
> > vdc->load(). This breaks the assumption that device specific load()
> > function can check guest_features. For virtio-net, self announcement
> > and guest offloads won't work after migration.
> > 
> > Fixing this by defer them to virtio_net_load() where guest_features
> > were guaranteed to be set. Other virtio devices looks fine.
> > 
> > Fixes: 019a3edbb25f1571e876f8af1ce4c55412939e5d
> >        ("virtio: make features 64bit wide")
> > Cc: qemu-stable@nongnu.org
> > Cc: Gerd Hoffmann <kraxel@redhat.com>
> > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > ---
> >  hw/net/virtio-net.c | 40 +++++++++++++++++++++++-----------------
> >  1 file changed, 23 insertions(+), 17 deletions(-)
> 
> Migration support for virtio is really a twisty maze, it's easy to make
> mistakes like that :(
> 

We have the very same problem with @device_endian which is also streamed in
a subsection. To prevent early usage on the load path, we set @device_endian
to a poisoned value that triggers assert() in the virtio_is_big_endian() helper.

Should this logic be generalized ?

[Qemu-devel] [PATCH] virtio: add some migration doc

[Cornelia Huck]

Try to cover the basics of virtio migration.

Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
---
It might help if we add some documentation; at the very least, it will
prevent myself getting a headache everytime I look at that code :)

Feedback welcome.
---
 docs/virtio-migration.txt | 101 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 101 insertions(+)
 create mode 100644 docs/virtio-migration.txt

diff --git a/docs/virtio-migration.txt b/docs/virtio-migration.txt
new file mode 100644
index 0000000..9c575e6
--- /dev/null
+++ b/docs/virtio-migration.txt
@@ -0,0 +1,101 @@
+Virtio devices and migration
+============================
+
+Saving and restoring the state of virtio devices is a bit of a twisty maze,
+for several reasons:
+- state is distributed between several parts:
+  - virtio core, for common fields like features, number of queues, ...
+  - virtio transport (pci, ccw, ...), for the different proxy devices and
+    transport specific state (msix vectors, indicators, ...)
+  - virtio device (net, blk, ...), for the different device types and their
+    state (mac address, request queue, ...)
+- most fields are saved via the stream interface; subsequently, subsections
+  have been added to make cross-version migration possible
+
+This file attempts to document the current procedure and point out some
+caveats.
+
+
+Save state procedure
+====================
+
+virtio core               virtio transport          virtio device
+-----------               ----------------          -------------
+
+                                                    save() function registered
+                                                    via register_savevm()
+virtio_save()                                       <----------
+             ------>      save_config()
+                          - save proxy device
+                          - save transport-specific
+                            device fields
+- save common device
+  fields
+- save common virtqueue
+  fields
+             ------>      save_queue()
+                          - save transport-specific
+                            virtqueue fields
+             ------>                               save_device()
+                                                   - save device-specific
+                                                     fields
+- save subsections
+  - device endianness,
+    if changed from
+    default endianness
+  - 64 bit features, if
+    any high feature bit
+    is set
+  - virtio-1 virtqueue
+    fields, if VERSION_1
+    is set
+
+
+Load state procedure
+====================
+
+virtio core               virtio transport          virtio device
+-----------               ----------------          -------------
+
+                                                    load() function registered
+                                                    via register_savevm()
+virtio_load()                                       <----------
+             ------>      load_config()
+                          - load proxy device
+                          - load transport-specific
+                            device fields
+- load common device
+  fields
+- load common virtqueue
+  fields
+             ------>      load_queue()
+                          - load transport-specific
+                            virtqueue fields
+- notify guest
+             ------>                               load_device()
+                                                   - load device-specific
+                                                     fields
+- load subsections
+  - device endianness
+  - 64 bit features
+  - virtio-1 virtqueue
+    fields
+- sanitize endianness
+- sanitize features
+- virtqueue index sanity
+  check
+                                                   - feature-dependent setup
+
+
+Implications of this setup
+==========================
+
+Devices need to be careful in their state processing during load: The
+load_device() procedure is invoked by the core before subsections have
+been loaded. Any code that depends on information transmitted in subsections
+therefore has to be invoked in the device's load() function _after_
+virtio_load() returned (like e.g. code depending on features).
+
+Any extension of the state being migrated should be done in subsections
+added to the core for compatibility reasons. If transport or device specific
+state is added, core needs to invoke a callback from the new subsection.
-- 
2.3.8

Re: [Qemu-devel] [PATCH] virtio: add some migration doc

[Greg Kurz]

On Thu, 17 Sep 2015 11:06:01 +0200
Cornelia Huck <cornelia.huck@de.ibm.com> wrote:

> Try to cover the basics of virtio migration.
> 
> Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> ---
> It might help if we add some documentation; at the very least, it will
> prevent myself getting a headache everytime I look at that code :)
> 
> Feedback welcome.

Excellent ! This is a very good to start with.

Reviewed-by: Greg Kurz <gkurz@linux.vnet.ibm.com>

Just a thought below...

> ---
>  docs/virtio-migration.txt | 101 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 101 insertions(+)
>  create mode 100644 docs/virtio-migration.txt
> 
> diff --git a/docs/virtio-migration.txt b/docs/virtio-migration.txt
> new file mode 100644
> index 0000000..9c575e6
> --- /dev/null
> +++ b/docs/virtio-migration.txt
> @@ -0,0 +1,101 @@
> +Virtio devices and migration
> +============================
> +
> +Saving and restoring the state of virtio devices is a bit of a twisty maze,
> +for several reasons:
> +- state is distributed between several parts:
> +  - virtio core, for common fields like features, number of queues, ...
> +  - virtio transport (pci, ccw, ...), for the different proxy devices and
> +    transport specific state (msix vectors, indicators, ...)
> +  - virtio device (net, blk, ...), for the different device types and their
> +    state (mac address, request queue, ...)
> +- most fields are saved via the stream interface; subsequently, subsections
> +  have been added to make cross-version migration possible
> +
> +This file attempts to document the current procedure and point out some
> +caveats.
> +
> +
> +Save state procedure
> +====================
> +
> +virtio core               virtio transport          virtio device
> +-----------               ----------------          -------------
> +
> +                                                    save() function registered
> +                                                    via register_savevm()
> +virtio_save()                                       <----------
> +             ------>      save_config()
> +                          - save proxy device
> +                          - save transport-specific
> +                            device fields
> +- save common device
> +  fields
> +- save common virtqueue
> +  fields
> +             ------>      save_queue()
> +                          - save transport-specific
> +                            virtqueue fields
> +             ------>                               save_device()
> +                                                   - save device-specific
> +                                                     fields
> +- save subsections
> +  - device endianness,
> +    if changed from
> +    default endianness
> +  - 64 bit features, if
> +    any high feature bit
> +    is set
> +  - virtio-1 virtqueue
> +    fields, if VERSION_1
> +    is set
> +
> +
> +Load state procedure
> +====================
> +
> +virtio core               virtio transport          virtio device
> +-----------               ----------------          -------------
> +
> +                                                    load() function registered
> +                                                    via register_savevm()
> +virtio_load()                                       <----------
> +             ------>      load_config()
> +                          - load proxy device
> +                          - load transport-specific
> +                            device fields
> +- load common device
> +  fields
> +- load common virtqueue
> +  fields
> +             ------>      load_queue()
> +                          - load transport-specific
> +                            virtqueue fields
> +- notify guest
> +             ------>                               load_device()
> +                                                   - load device-specific
> +                                                     fields
> +- load subsections
> +  - device endianness
> +  - 64 bit features
> +  - virtio-1 virtqueue
> +    fields
> +- sanitize endianness
> +- sanitize features
> +- virtqueue index sanity
> +  check
> +                                                   - feature-dependent setup
> +
> +
> +Implications of this setup
> +==========================
> +
> +Devices need to be careful in their state processing during load: The
> +load_device() procedure is invoked by the core before subsections have
> +been loaded. Any code that depends on information transmitted in subsections
> +therefore has to be invoked in the device's load() function _after_
> +virtio_load() returned (like e.g. code depending on features).
> +

From a VMState standpoint, such code would land in a post-load callback most of
the time. Would that help comprehension if we introduce a virtio_post_load()
function ?

> +Any extension of the state being migrated should be done in subsections
> +added to the core for compatibility reasons. If transport or device specific
> +state is added, core needs to invoke a callback from the new subsection.

Re: [Qemu-devel] [PATCH] virtio: add some migration doc

[Jason Wang]

On 09/17/2015 06:39 PM, Greg Kurz wrote:
> On Thu, 17 Sep 2015 11:06:01 +0200
> Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
>
>> Try to cover the basics of virtio migration.
>>
>> Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
>> ---
>> It might help if we add some documentation; at the very least, it will
>> prevent myself getting a headache everytime I look at that code :)
>>
>> Feedback welcome.
> Excellent ! This is a very good to start with.
>
> Reviewed-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
>
> Just a thought below...
>
>> ---
>>  docs/virtio-migration.txt | 101 ++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 101 insertions(+)
>>  create mode 100644 docs/virtio-migration.txt
>>
>> diff --git a/docs/virtio-migration.txt b/docs/virtio-migration.txt
>> new file mode 100644
>> index 0000000..9c575e6
>> --- /dev/null
>> +++ b/docs/virtio-migration.txt
>> @@ -0,0 +1,101 @@
>> +Virtio devices and migration
>> +============================
>> +
>> +Saving and restoring the state of virtio devices is a bit of a twisty maze,
>> +for several reasons:
>> +- state is distributed between several parts:
>> +  - virtio core, for common fields like features, number of queues, ...
>> +  - virtio transport (pci, ccw, ...), for the different proxy devices and
>> +    transport specific state (msix vectors, indicators, ...)
>> +  - virtio device (net, blk, ...), for the different device types and their
>> +    state (mac address, request queue, ...)
>> +- most fields are saved via the stream interface; subsequently, subsections
>> +  have been added to make cross-version migration possible
>> +
>> +This file attempts to document the current procedure and point out some
>> +caveats.
>> +
>> +
>> +Save state procedure
>> +====================
>> +
>> +virtio core               virtio transport          virtio device
>> +-----------               ----------------          -------------
>> +
>> +                                                    save() function registered
>> +                                                    via register_savevm()
>> +virtio_save()                                       <----------
>> +             ------>      save_config()
>> +                          - save proxy device
>> +                          - save transport-specific
>> +                            device fields
>> +- save common device
>> +  fields
>> +- save common virtqueue
>> +  fields
>> +             ------>      save_queue()
>> +                          - save transport-specific
>> +                            virtqueue fields
>> +             ------>                               save_device()
>> +                                                   - save device-specific
>> +                                                     fields
>> +- save subsections
>> +  - device endianness,
>> +    if changed from
>> +    default endianness
>> +  - 64 bit features, if
>> +    any high feature bit
>> +    is set
>> +  - virtio-1 virtqueue
>> +    fields, if VERSION_1
>> +    is set
>> +
>> +
>> +Load state procedure
>> +====================
>> +
>> +virtio core               virtio transport          virtio device
>> +-----------               ----------------          -------------
>> +
>> +                                                    load() function registered
>> +                                                    via register_savevm()
>> +virtio_load()                                       <----------
>> +             ------>      load_config()
>> +                          - load proxy device
>> +                          - load transport-specific
>> +                            device fields
>> +- load common device
>> +  fields
>> +- load common virtqueue
>> +  fields
>> +             ------>      load_queue()
>> +                          - load transport-specific
>> +                            virtqueue fields
>> +- notify guest
>> +             ------>                               load_device()
>> +                                                   - load device-specific
>> +                                                     fields
>> +- load subsections
>> +  - device endianness
>> +  - 64 bit features
>> +  - virtio-1 virtqueue
>> +    fields
>> +- sanitize endianness
>> +- sanitize features
>> +- virtqueue index sanity
>> +  check
>> +                                                   - feature-dependent setup
>> +
>> +
>> +Implications of this setup
>> +==========================
>> +
>> +Devices need to be careful in their state processing during load: The
>> +load_device() procedure is invoked by the core before subsections have
>> +been loaded. Any code that depends on information transmitted in subsections
>> +therefore has to be invoked in the device's load() function _after_
>> +virtio_load() returned (like e.g. code depending on features).
>> +
> From a VMState standpoint, such code would land in a post-load callback most of
> the time. Would that help comprehension if we introduce a virtio_post_load()
> function ?

I think we could. (With calling a device specific method in
virtio_post_load()).

>> +Any extension of the state being migrated should be done in subsections
>> +added to the core for compatibility reasons. If transport or device specific
>> +state is added, core needs to invoke a callback from the new subsection.

Re: [Qemu-devel] [PATCH] virtio: add some migration doc

[Cornelia Huck]

On Thu, 17 Sep 2015 12:39:31 +0200
Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:
> On Thu, 17 Sep 2015 11:06:01 +0200
> Cornelia Huck <cornelia.huck@de.ibm.com> wrote:

> > +Devices need to be careful in their state processing during load: The
> > +load_device() procedure is invoked by the core before subsections have
> > +been loaded. Any code that depends on information transmitted in subsections
> > +therefore has to be invoked in the device's load() function _after_
> > +virtio_load() returned (like e.g. code depending on features).
> > +
> 
> From a VMState standpoint, such code would land in a post-load callback most of
> the time. Would that help comprehension if we introduce a virtio_post_load()
> function ?

It might. I think the main problem is that we need to do some stuff
post-load, and it's not really obvious what it is.

Re: [Qemu-devel] [PATCH] virtio: add some migration doc

[Jason Wang]

On 09/17/2015 05:06 PM, Cornelia Huck wrote:
> Try to cover the basics of virtio migration.
>
> Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> ---
> It might help if we add some documentation; at the very least, it will
> prevent myself getting a headache everytime I look at that code :)
>
> Feedback welcome.
> ---
>  docs/virtio-migration.txt | 101 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 101 insertions(+)
>  create mode 100644 docs/virtio-migration.txt

Thanks for the patch. Looks good to me. Keeping this doc to be synced
with code is important in the future. E.g I add a new subsection in core
with my pci virtio 1.0 fixes.

>
> diff --git a/docs/virtio-migration.txt b/docs/virtio-migration.txt
> new file mode 100644
> index 0000000..9c575e6
> --- /dev/null
> +++ b/docs/virtio-migration.txt
> @@ -0,0 +1,101 @@
> +Virtio devices and migration
> +============================
> +
> +Saving and restoring the state of virtio devices is a bit of a twisty maze,
> +for several reasons:
> +- state is distributed between several parts:
> +  - virtio core, for common fields like features, number of queues, ...
> +  - virtio transport (pci, ccw, ...), for the different proxy devices and
> +    transport specific state (msix vectors, indicators, ...)
> +  - virtio device (net, blk, ...), for the different device types and their
> +    state (mac address, request queue, ...)
> +- most fields are saved via the stream interface; subsequently, subsections
> +  have been added to make cross-version migration possible
> +
> +This file attempts to document the current procedure and point out some
> +caveats.
> +
> +
> +Save state procedure
> +====================
> +
> +virtio core               virtio transport          virtio device
> +-----------               ----------------          -------------
> +
> +                                                    save() function registered
> +                                                    via register_savevm()
> +virtio_save()                                       <----------
> +             ------>      save_config()
> +                          - save proxy device
> +                          - save transport-specific
> +                            device fields
> +- save common device
> +  fields
> +- save common virtqueue
> +  fields
> +             ------>      save_queue()
> +                          - save transport-specific
> +                            virtqueue fields
> +             ------>                               save_device()
> +                                                   - save device-specific
> +                                                     fields
> +- save subsections
> +  - device endianness,
> +    if changed from
> +    default endianness
> +  - 64 bit features, if
> +    any high feature bit
> +    is set
> +  - virtio-1 virtqueue
> +    fields, if VERSION_1
> +    is set
> +
> +
> +Load state procedure
> +====================
> +
> +virtio core               virtio transport          virtio device
> +-----------               ----------------          -------------
> +
> +                                                    load() function registered
> +                                                    via register_savevm()
> +virtio_load()                                       <----------
> +             ------>      load_config()
> +                          - load proxy device
> +                          - load transport-specific
> +                            device fields
> +- load common device
> +  fields
> +- load common virtqueue
> +  fields
> +             ------>      load_queue()
> +                          - load transport-specific
> +                            virtqueue fields
> +- notify guest
> +             ------>                               load_device()
> +                                                   - load device-specific
> +                                                     fields
> +- load subsections
> +  - device endianness
> +  - 64 bit features
> +  - virtio-1 virtqueue
> +    fields
> +- sanitize endianness
> +- sanitize features
> +- virtqueue index sanity
> +  check
> +                                                   - feature-dependent setup
> +
> +
> +Implications of this setup
> +==========================
> +
> +Devices need to be careful in their state processing during load: The
> +load_device() procedure is invoked by the core before subsections have
> +been loaded. Any code that depends on information transmitted in subsections
> +therefore has to be invoked in the device's load() function _after_
> +virtio_load() returned (like e.g. code depending on features).
> +
> +Any extension of the state being migrated should be done in subsections
> +added to the core for compatibility reasons. If transport or device specific
> +state is added, core needs to invoke a callback from the new subsection.

Re: [Qemu-devel] [PATCH] virtio: add some migration doc

[Cornelia Huck]

On Thu, 17 Sep 2015 18:44:00 +0800
Jason Wang <jasowang@redhat.com> wrote:
> 
> 
> On 09/17/2015 05:06 PM, Cornelia Huck wrote:
> > Try to cover the basics of virtio migration.
> >
> > Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> > ---
> > It might help if we add some documentation; at the very least, it will
> > prevent myself getting a headache everytime I look at that code :)
> >
> > Feedback welcome.
> > ---
> >  docs/virtio-migration.txt | 101 ++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 101 insertions(+)
> >  create mode 100644 docs/virtio-migration.txt
> 
> Thanks for the patch. Looks good to me. Keeping this doc to be synced
> with code is important in the future. E.g I add a new subsection in core
> with my pci virtio 1.0 fixes.

Yes, that's a drawback of the "document it" approach...

Re: [Qemu-devel] [PATCH] virtio: add some migration doc

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1106 bytes --]

On 09/17/2015 03:06 AM, Cornelia Huck wrote:
> Try to cover the basics of virtio migration.
> 
> Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> ---
> It might help if we add some documentation; at the very least, it will
> prevent myself getting a headache everytime I look at that code :)
> 
> Feedback welcome.
> ---
>  docs/virtio-migration.txt | 101 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 101 insertions(+)
>  create mode 100644 docs/virtio-migration.txt
> 
> diff --git a/docs/virtio-migration.txt b/docs/virtio-migration.txt
> new file mode 100644
> index 0000000..9c575e6
> --- /dev/null
> +++ b/docs/virtio-migration.txt
> @@ -0,0 +1,101 @@
> +Virtio devices and migration
> +============================

Since you didn't supply an explicit Copyright/License, this document
inherits the project default of GPLv2+.  Not the end of the world, but a
lot of recently-added documentation has been explicitly listing a license.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH] virtio-net: unbreak self announcement and guest offloads after migration

[Greg Kurz]

On Fri, 11 Sep 2015 16:01:56 +0800
Jason Wang <jasowang@redhat.com> wrote:

> After commit 019a3edbb25f1571e876f8af1ce4c55412939e5d ("virtio: make
> features 64bit wide"). Device's guest_features was actually set after
> vdc->load(). This breaks the assumption that device specific load()

Yeah... subsections are loaded after the device specific state...

> function can check guest_features. For virtio-net, self announcement
> and guest offloads won't work after migration.
> 
> Fixing this by defer them to virtio_net_load() where guest_features
> were guaranteed to be set. Other virtio devices looks fine.
> 
> Fixes: 019a3edbb25f1571e876f8af1ce4c55412939e5d
>        ("virtio: make features 64bit wide")
> Cc: qemu-stable@nongnu.org
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---

Reviewed-by: Greg Kurz <gkurz@linux.vnet.ibm.com>

>  hw/net/virtio-net.c | 40 +++++++++++++++++++++++-----------------
>  1 file changed, 23 insertions(+), 17 deletions(-)
> 
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index f72eebf..2775e6a 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -1458,11 +1458,33 @@ static int virtio_net_load(QEMUFile *f, void *opaque, int version_id)
>  {
>      VirtIONet *n = opaque;
>      VirtIODevice *vdev = VIRTIO_DEVICE(n);
> +    int ret;
> 
>      if (version_id < 2 || version_id > VIRTIO_NET_VM_VERSION)
>          return -EINVAL;
> 
> -    return virtio_load(vdev, f, version_id);
> +    ret = virtio_load(vdev, f, version_id);
> +    if (ret) {
> +        return ret;
> +    }
> +
> +    if (virtio_vdev_has_feature(vdev, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS)) {
> +        n->curr_guest_offloads = qemu_get_be64(f);
> +    } else {
> +        n->curr_guest_offloads = virtio_net_supported_guest_offloads(n);
> +    }
> +
> +    if (peer_has_vnet_hdr(n)) {
> +        virtio_net_apply_guest_offloads(n);
> +    }
> +
> +    if (virtio_vdev_has_feature(vdev, VIRTIO_NET_F_GUEST_ANNOUNCE) &&
> +        virtio_vdev_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ)) {
> +        n->announce_counter = SELF_ANNOUNCE_ROUNDS;
> +        timer_mod(n->announce_timer, qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL));
> +    }
> +
> +    return 0;
>  }
> 
>  static int virtio_net_load_device(VirtIODevice *vdev, QEMUFile *f,
> @@ -1559,16 +1581,6 @@ static int virtio_net_load_device(VirtIODevice *vdev, QEMUFile *f,
>          }
>      }
> 
> -    if (virtio_vdev_has_feature(vdev, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS)) {
> -        n->curr_guest_offloads = qemu_get_be64(f);
> -    } else {
> -        n->curr_guest_offloads = virtio_net_supported_guest_offloads(n);
> -    }
> -
> -    if (peer_has_vnet_hdr(n)) {
> -        virtio_net_apply_guest_offloads(n);
> -    }
> -
>      virtio_net_set_queues(n);
> 
>      /* Find the first multicast entry in the saved MAC filter */
> @@ -1586,12 +1598,6 @@ static int virtio_net_load_device(VirtIODevice *vdev, QEMUFile *f,
>          qemu_get_subqueue(n->nic, i)->link_down = link_down;
>      }
> 
> -    if (virtio_vdev_has_feature(vdev, VIRTIO_NET_F_GUEST_ANNOUNCE) &&
> -        virtio_vdev_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ)) {
> -        n->announce_counter = SELF_ANNOUNCE_ROUNDS;
> -        timer_mod(n->announce_timer, qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL));
> -    }
> -
>      return 0;
>  }
>