| Message ID | 1466433206-29241-1-git-send-email-pbonzini@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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
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
> > > - 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
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)
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(-)