[Qemu-devel] [PATCH] scsi: esp: fix migration

[Qemu-devel] [PATCH] scsi: esp: fix migration

[Paolo Bonzini]

Commit 926cde5 ("scsi: esp: make cmdbuf big enough for maximum CDB size",
2016-06-16) changed the size of a migrated field.  Split it in two
parts, and only migrate the second part in a new vmstate version.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi/esp.c               | 5 +++--
 include/migration/vmstate.h | 5 ++++-
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index baa0a2c..1f2f2d3 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -574,7 +574,7 @@ static bool esp_mem_accepts(void *opaque, hwaddr addr,
 
 const VMStateDescription vmstate_esp = {
     .name ="esp",
-    .version_id = 3,
+    .version_id = 4,
     .minimum_version_id = 3,
     .fields = (VMStateField[]) {
         VMSTATE_BUFFER(rregs, ESPState),
@@ -585,7 +585,8 @@ const VMStateDescription vmstate_esp = {
         VMSTATE_BUFFER(ti_buf, ESPState),
         VMSTATE_UINT32(status, ESPState),
         VMSTATE_UINT32(dma, ESPState),
-        VMSTATE_BUFFER(cmdbuf, ESPState),
+        VMSTATE_PARTIAL_BUFFER(cmdbuf, ESPState, 16),
+        VMSTATE_BUFFER_START_MIDDLE_V(cmdbuf, ESPState, 16, 4),
         VMSTATE_UINT32(cmdlen, ESPState),
         VMSTATE_UINT32(do_cmd, ESPState),
         VMSTATE_UINT32(dma_left, ESPState),
diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 6c65811..187c931 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -898,8 +898,11 @@ extern const VMStateInfo vmstate_info_bitmap;
 #define VMSTATE_PARTIAL_BUFFER(_f, _s, _size)                         \
     VMSTATE_STATIC_BUFFER(_f, _s, 0, NULL, 0, _size)
 
+#define VMSTATE_BUFFER_START_MIDDLE_V(_f, _s, _start, _v) \
+    VMSTATE_STATIC_BUFFER(_f, _s, _v, NULL, _start, sizeof(typeof_field(_s, _f)))
+
 #define VMSTATE_BUFFER_START_MIDDLE(_f, _s, _start) \
-    VMSTATE_STATIC_BUFFER(_f, _s, 0, NULL, _start, sizeof(typeof_field(_s, _f)))
+    VMSTATE_BUFFER_START_MIDDLE_V(_f, _s, _start, 0)
 
 #define VMSTATE_PARTIAL_VBUFFER(_f, _s, _size)                        \
     VMSTATE_VBUFFER(_f, _s, 0, NULL, 0, _size)
-- 
2.5.5

Re: [Qemu-devel] [PATCH] scsi: esp: fix migration

[Amit Shah]

On (Mon) 20 Jun 2016 [16:33:26], Paolo Bonzini wrote:
> Commit 926cde5 ("scsi: esp: make cmdbuf big enough for maximum CDB size",
> 2016-06-16) changed the size of a migrated field.  Split it in two
> parts, and only migrate the second part in a new vmstate version.

With this patch, the static checker fails in this way:

Section "esp", Description "esp": expected field "cmdlen", got "cmdbuf"; skipping rest
Section "dc390", Description "esp": expected field "cmdlen", got "cmdbuf"; skipping rest
Section "am53c974", Description "esp": expected field "cmdlen", got "cmdbuf"; skipping rest

Note it doesn't complain about the version numbers.  That's because:

>  const VMStateDescription vmstate_esp = {
>      .name ="esp",
> -    .version_id = 3,
> +    .version_id = 4,
>      .minimum_version_id = 3,

this suggests older versions can still be accepted for incoming
migration, which isn't true.  So we'll have to bump this field up too.

I tried that, and this is how the checker fails with
minimum_version_id bumped:

Section "esp" Description "esp": minimum version error: 3 < 4
Section "esp", Description "esp": expected field "cmdlen", got "cmdbuf"; skipping rest
Section "dc390" Description "esp": minimum version error: 3 < 4
Section "dc390", Description "esp": expected field "cmdlen", got "cmdbuf"; skipping rest
Section "am53c974" Description "esp": minimum version error: 3 < 4
Section "am53c974", Description "esp": expected field "cmdlen", got "cmdbuf"; skipping rest

We're at a point where we can't accept an older migration stream for
this device, right?  At least that's what the version bump indicates.

One workaround would be to mark the current cmdbuf as unused, and push
the new one into a subsection.  That way we can keep incoming
migrations working, but given esp migrations never worked with the
partial state earlier, we'd not be any worse-off.


		Amit

Re: [Qemu-devel] [PATCH] scsi: esp: fix migration

[Paolo Bonzini]

On 27/06/2016 09:20, Amit Shah wrote:
> On (Mon) 20 Jun 2016 [16:33:26], Paolo Bonzini wrote:
>> Commit 926cde5 ("scsi: esp: make cmdbuf big enough for maximum CDB size",
>> 2016-06-16) changed the size of a migrated field.  Split it in two
>> parts, and only migrate the second part in a new vmstate version.
> 
> With this patch, the static checker fails in this way:
> 
> Section "esp", Description "esp": expected field "cmdlen", got "cmdbuf"; skipping rest
> Section "dc390", Description "esp": expected field "cmdlen", got "cmdbuf"; skipping rest
> Section "am53c974", Description "esp": expected field "cmdlen", got "cmdbuf"; skipping rest
> 
> Note it doesn't complain about the version numbers.  That's because:
> 
>>  const VMStateDescription vmstate_esp = {
>>      .name ="esp",
>> -    .version_id = 3,
>> +    .version_id = 4,
>>      .minimum_version_id = 3,
> 
> this suggests older versions can still be accepted for incoming
> migration, which isn't true.

Sure they can:

-        VMSTATE_BUFFER(cmdbuf, ESPState),
+        VMSTATE_PARTIAL_BUFFER(cmdbuf, ESPState, 16),
+        VMSTATE_BUFFER_START_MIDDLE_V(cmdbuf, ESPState, 16, 4),

2.6 is transmitting version 3 and a 16-byte buffer.

2.7 is transmitting version 4, a first 16-byte buffer, and a second
16-byte buffer that is skipped when receiving version 3.

So it seems like a static checker limitation.

Paolo

Re: [Qemu-devel] [PATCH] scsi: esp: fix migration

[Juan Quintela]

Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 27/06/2016 09:20, Amit Shah wrote:
>> On (Mon) 20 Jun 2016 [16:33:26], Paolo Bonzini wrote:
>>> Commit 926cde5 ("scsi: esp: make cmdbuf big enough for maximum CDB size",
>>> 2016-06-16) changed the size of a migrated field.  Split it in two
>>> parts, and only migrate the second part in a new vmstate version.
>> 
>> With this patch, the static checker fails in this way:
>> 
>> Section "esp", Description "esp": expected field "cmdlen", got
>> "cmdbuf"; skipping rest
>> Section "dc390", Description "esp": expected field "cmdlen", got
>> "cmdbuf"; skipping rest
>> Section "am53c974", Description "esp": expected field "cmdlen", got
>> "cmdbuf"; skipping rest
>> 
>> Note it doesn't complain about the version numbers.  That's because:
>> 
>>>  const VMStateDescription vmstate_esp = {
>>>      .name ="esp",
>>> -    .version_id = 3,
>>> +    .version_id = 4,
>>>      .minimum_version_id = 3,
>> 
>> this suggests older versions can still be accepted for incoming
>> migration, which isn't true.
>
> Sure they can:
>
> -        VMSTATE_BUFFER(cmdbuf, ESPState),
> +        VMSTATE_PARTIAL_BUFFER(cmdbuf, ESPState, 16),
> +        VMSTATE_BUFFER_START_MIDDLE_V(cmdbuf, ESPState, 16, 4),

Amit, would it help the checker if we do something like:

-        VMSTATE_BUFFER(cmdbuf, ESPState),
+        VMSTATE_PARTIAL_BUFFER_TEST(cmdbuf, ESPState, 16, v_is_3),
+        VMSTATE_BUFFER_TEST(cmdbuf, ESPState, from_4),

Yes, VMSTATE_PARTIAL_BUFFER_TEST don't exist, but it is trivial to
define.

Later, Juan.


>
> 2.6 is transmitting version 3 and a 16-byte buffer.
>
> 2.7 is transmitting version 4, a first 16-byte buffer, and a second
> 16-byte buffer that is skipped when receiving version 3.
>
> So it seems like a static checker limitation.
>
> Paolo

Re: [Qemu-devel] [PATCH] scsi: esp: fix migration

[Amit Shah]

On (Wed) 29 Jun 2016 [14:00:17], Juan Quintela wrote:
> Paolo Bonzini <pbonzini@redhat.com> wrote:
> > On 27/06/2016 09:20, Amit Shah wrote:
> >> On (Mon) 20 Jun 2016 [16:33:26], Paolo Bonzini wrote:
> >>> Commit 926cde5 ("scsi: esp: make cmdbuf big enough for maximum CDB size",
> >>> 2016-06-16) changed the size of a migrated field.  Split it in two
> >>> parts, and only migrate the second part in a new vmstate version.
> >> 
> >> With this patch, the static checker fails in this way:
> >> 
> >> Section "esp", Description "esp": expected field "cmdlen", got
> >> "cmdbuf"; skipping rest
> >> Section "dc390", Description "esp": expected field "cmdlen", got
> >> "cmdbuf"; skipping rest
> >> Section "am53c974", Description "esp": expected field "cmdlen", got
> >> "cmdbuf"; skipping rest
> >> 
> >> Note it doesn't complain about the version numbers.  That's because:
> >> 
> >>>  const VMStateDescription vmstate_esp = {
> >>>      .name ="esp",
> >>> -    .version_id = 3,
> >>> +    .version_id = 4,
> >>>      .minimum_version_id = 3,
> >> 
> >> this suggests older versions can still be accepted for incoming
> >> migration, which isn't true.
> >
> > Sure they can:
> >
> > -        VMSTATE_BUFFER(cmdbuf, ESPState),
> > +        VMSTATE_PARTIAL_BUFFER(cmdbuf, ESPState, 16),
> > +        VMSTATE_BUFFER_START_MIDDLE_V(cmdbuf, ESPState, 16, 4),
> 
> Amit, would it help the checker if we do something like:
> 
> -        VMSTATE_BUFFER(cmdbuf, ESPState),
> +        VMSTATE_PARTIAL_BUFFER_TEST(cmdbuf, ESPState, 16, v_is_3),
> +        VMSTATE_BUFFER_TEST(cmdbuf, ESPState, from_4),
> 
> Yes, VMSTATE_PARTIAL_BUFFER_TEST don't exist, but it is trivial to
> define.

Yea, I think it'll help.  But I'm also thinking what else we do in
code that we can offload to vmstate.  Or if there's something that we
just don't express in vmstate yet.

		Amit

Re: [Qemu-devel] [PATCH] scsi: esp: fix migration

[Paolo Bonzini]

> > > -        VMSTATE_BUFFER(cmdbuf, ESPState),
> > > +        VMSTATE_PARTIAL_BUFFER(cmdbuf, ESPState, 16),
> > > +        VMSTATE_BUFFER_START_MIDDLE_V(cmdbuf, ESPState, 16, 4),
> > 
> > Amit, would it help the checker if we do something like:
> > 
> > -        VMSTATE_BUFFER(cmdbuf, ESPState),
> > +        VMSTATE_PARTIAL_BUFFER_TEST(cmdbuf, ESPState, 16, v_is_3),
> > +        VMSTATE_BUFFER_TEST(cmdbuf, ESPState, from_4),
> > 
> > Yes, VMSTATE_PARTIAL_BUFFER_TEST don't exist, but it is trivial to
> > define.
> 
> Yea, I think it'll help.  But I'm also thinking what else we do in
> code that we can offload to vmstate.  Or if there's something that we
> just don't express in vmstate yet.

The checker has to compare this (vmstate version_id = 3):

              {
                "field": "cmdbuf",
                "version_id": 0,
                "field_exists": false,
                "size": 16
              },

to this (vmstate version_id = 4):

              {
                "field": "cmdbuf",
                "version_id": 0,
                "field_exists": false,
                "size": 16
              },
              {
                "field": "cmdbuf",
                "version_id": 4,
                "field_exists": false,
                "size": 16
              },

If the static checker finds a destination field whose version_id is
larger than the source vmstate's version_id, you have to print a
different warning and advance the destination only.

Thanks,

Paolo

Re: [Qemu-devel] [PATCH] scsi: esp: fix migration

[Amit Shah]

On (Mon) 27 Jun 2016 [09:54:24], Paolo Bonzini wrote:
> 
> 
> On 27/06/2016 09:20, Amit Shah wrote:
> > On (Mon) 20 Jun 2016 [16:33:26], Paolo Bonzini wrote:
> >> Commit 926cde5 ("scsi: esp: make cmdbuf big enough for maximum CDB size",
> >> 2016-06-16) changed the size of a migrated field.  Split it in two
> >> parts, and only migrate the second part in a new vmstate version.
> > 
> > With this patch, the static checker fails in this way:
> > 
> > Section "esp", Description "esp": expected field "cmdlen", got "cmdbuf"; skipping rest
> > Section "dc390", Description "esp": expected field "cmdlen", got "cmdbuf"; skipping rest
> > Section "am53c974", Description "esp": expected field "cmdlen", got "cmdbuf"; skipping rest
> > 
> > Note it doesn't complain about the version numbers.  That's because:
> > 
> >>  const VMStateDescription vmstate_esp = {
> >>      .name ="esp",
> >> -    .version_id = 3,
> >> +    .version_id = 4,
> >>      .minimum_version_id = 3,
> > 
> > this suggests older versions can still be accepted for incoming
> > migration, which isn't true.
> 
> Sure they can:
> 
> -        VMSTATE_BUFFER(cmdbuf, ESPState),
> +        VMSTATE_PARTIAL_BUFFER(cmdbuf, ESPState, 16),
> +        VMSTATE_BUFFER_START_MIDDLE_V(cmdbuf, ESPState, 16, 4),
> 
> 2.6 is transmitting version 3 and a 16-byte buffer.
> 
> 2.7 is transmitting version 4, a first 16-byte buffer, and a second
> 16-byte buffer that is skipped when receiving version 3.

Hm, yeah.

> So it seems like a static checker limitation.

Yeah, so I think this can't be checked statically -- let me dig more.


		Amit