| Message ID | 1488777954-4578-13-git-send-email-jasowang@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 06/03/2017 06:25, Jason Wang wrote: > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > The index's in the Vmxnet3Ring were migrated as 32bit ints > yet are declared as size_t's. They appear to be derived > from 32bit values loaded from guest memory, so actually > store them as that. > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > Acked-by: Dmitry Fleytman <dmitry@daynix.com> > Signed-off-by: Jason Wang <jasowang@redhat.com> > --- > hw/net/vmxnet3.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c > index e13a798..224c109 100644 > --- a/hw/net/vmxnet3.c > +++ b/hw/net/vmxnet3.c > @@ -141,17 +141,17 @@ typedef struct VMXNET3Class { > /* Cyclic ring abstraction */ > typedef struct { > hwaddr pa; > - size_t size; > - size_t cell_size; > - size_t next; > + uint32_t size; > + uint32_t cell_size; > + uint32_t next; > uint8_t gen; > } Vmxnet3Ring; > > static inline void vmxnet3_ring_init(PCIDevice *d, > Vmxnet3Ring *ring, > hwaddr pa, > - size_t size, > - size_t cell_size, > + uint32_t size, > + uint32_t cell_size, > bool zero_region) > { > ring->pa = pa; > @@ -166,7 +166,7 @@ static inline void vmxnet3_ring_init(PCIDevice *d, > } > > #define VMXNET3_RING_DUMP(macro, ring_name, ridx, r) \ > - macro("%s#%d: base %" PRIx64 " size %zu cell_size %zu gen %d next %zu", \ > + macro("%s#%d: base %" PRIx64 " size %u cell_size %u gen %d next %u", \ > (ring_name), (ridx), \ > (r)->pa, (r)->size, (r)->cell_size, (r)->gen, (r)->next) > > David, with '-dump-vmstate' with 2.8 machine type and with v2.8 and master I can see the size of "txq_descr" and "rxq_descr" changes because of "sizeof(Vmxnet3TxqDescr)" and "sizeof(Vmxnet3RxqDescr)". This changes because the size of Vmxnet3Ring has changed with the s/size_t/uint32_t/": { "field": "txq_descr", "version_id": 0, "field_exists": false, "size": 176 }, { "field": "rxq_descr", "version_id": 0, "field_exists": false, "size": 216 }, becomes: { "field": "txq_descr", "version_id": 0, "field_exists": false, "size": 144 }, { "field": "rxq_descr", "version_id": 0, "field_exists": false, "size": 168 }, And if I try a migration, I have: qemu/migration/vmstate.c:112: vmstate_load_state: Assertion `first_elem || !n_elems' failed. Laurent
* Laurent Vivier (lvivier@redhat.com) wrote: > On 06/03/2017 06:25, Jason Wang wrote: > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > > > The index's in the Vmxnet3Ring were migrated as 32bit ints > > yet are declared as size_t's. They appear to be derived > > from 32bit values loaded from guest memory, so actually > > store them as that. > > > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > Acked-by: Dmitry Fleytman <dmitry@daynix.com> > > Signed-off-by: Jason Wang <jasowang@redhat.com> > > --- > > hw/net/vmxnet3.c | 12 ++++++------ > > 1 file changed, 6 insertions(+), 6 deletions(-) > > > > diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c > > index e13a798..224c109 100644 > > --- a/hw/net/vmxnet3.c > > +++ b/hw/net/vmxnet3.c > > @@ -141,17 +141,17 @@ typedef struct VMXNET3Class { > > /* Cyclic ring abstraction */ > > typedef struct { > > hwaddr pa; > > - size_t size; > > - size_t cell_size; > > - size_t next; > > + uint32_t size; > > + uint32_t cell_size; > > + uint32_t next; > > uint8_t gen; > > } Vmxnet3Ring; > > > > static inline void vmxnet3_ring_init(PCIDevice *d, > > Vmxnet3Ring *ring, > > hwaddr pa, > > - size_t size, > > - size_t cell_size, > > + uint32_t size, > > + uint32_t cell_size, > > bool zero_region) > > { > > ring->pa = pa; > > @@ -166,7 +166,7 @@ static inline void vmxnet3_ring_init(PCIDevice *d, > > } > > > > #define VMXNET3_RING_DUMP(macro, ring_name, ridx, r) \ > > - macro("%s#%d: base %" PRIx64 " size %zu cell_size %zu gen %d next %zu", \ > > + macro("%s#%d: base %" PRIx64 " size %u cell_size %u gen %d next %u", \ > > (ring_name), (ridx), \ > > (r)->pa, (r)->size, (r)->cell_size, (r)->gen, (r)->next) > > > > > > > David, with '-dump-vmstate' with 2.8 machine type and with v2.8 and > master I can see the size of "txq_descr" and "rxq_descr" changes because > of "sizeof(Vmxnet3TxqDescr)" and "sizeof(Vmxnet3RxqDescr)". This changes > because the size of Vmxnet3Ring has changed with the s/size_t/uint32_t/": > > { > "field": "txq_descr", > "version_id": 0, > "field_exists": false, > "size": 176 > }, > { > "field": "rxq_descr", > "version_id": 0, > "field_exists": false, > "size": 216 > }, > > becomes: > > { > "field": "txq_descr", > "version_id": 0, > "field_exists": false, > "size": 144 > }, > { > "field": "rxq_descr", > "version_id": 0, > "field_exists": false, > "size": 168 > }, But the old migration code used to write these fields using qemu_put_be32 (from a11f5cb005f9854f0d78da97fc23adf5bc8c83f3) -static void vmxnet3_put_ring_to_file(QEMUFile *f, Vmxnet3Ring *r) -{ - qemu_put_be64(f, r->pa); - qemu_put_be32(f, r->size); - qemu_put_be32(f, r->cell_size); - qemu_put_be32(f, r->next); - qemu_put_byte(f, r->gen); -} +static const VMStateDescription vmstate_vmxnet3_ring = { + .name = "vmxnet3-ring", + .version_id = 0, + .fields = (VMStateField[]) { + VMSTATE_UINT64(pa, Vmxnet3Ring), + VMSTATE_UINT32(size, Vmxnet3Ring), + VMSTATE_UINT32(cell_size, Vmxnet3Ring), + VMSTATE_UINT32(next, Vmxnet3Ring), + VMSTATE_UINT8(gen, Vmxnet3Ring), + VMSTATE_END_OF_LIST() + } }; so they used to be size_t's written with be32 and now they're uint32_t's written as 32bit - so that should be the same. I'm not sure i understand where the old txq_descr sizes come from - what managed to print the 176 number before it was converted to VMState? > And if I try a migration, I have: > > qemu/migration/vmstate.c:112: vmstate_load_state: Assertion `first_elem > || !n_elems' failed. Interesting; that's Halil's new assert; I need to look. Dave > Laurent -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 14/03/2017 12:16, Dr. David Alan Gilbert wrote: > * Laurent Vivier (lvivier@redhat.com) wrote: >> On 06/03/2017 06:25, Jason Wang wrote: >>> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> >>> >>> The index's in the Vmxnet3Ring were migrated as 32bit ints >>> yet are declared as size_t's. They appear to be derived >>> from 32bit values loaded from guest memory, so actually >>> store them as that. >>> >>> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> >>> Acked-by: Dmitry Fleytman <dmitry@daynix.com> >>> Signed-off-by: Jason Wang <jasowang@redhat.com> >>> --- >>> hw/net/vmxnet3.c | 12 ++++++------ >>> 1 file changed, 6 insertions(+), 6 deletions(-) >>> >>> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c >>> index e13a798..224c109 100644 >>> --- a/hw/net/vmxnet3.c >>> +++ b/hw/net/vmxnet3.c >>> @@ -141,17 +141,17 @@ typedef struct VMXNET3Class { >>> /* Cyclic ring abstraction */ >>> typedef struct { >>> hwaddr pa; >>> - size_t size; >>> - size_t cell_size; >>> - size_t next; >>> + uint32_t size; >>> + uint32_t cell_size; >>> + uint32_t next; >>> uint8_t gen; >>> } Vmxnet3Ring; >>> >>> static inline void vmxnet3_ring_init(PCIDevice *d, >>> Vmxnet3Ring *ring, >>> hwaddr pa, >>> - size_t size, >>> - size_t cell_size, >>> + uint32_t size, >>> + uint32_t cell_size, >>> bool zero_region) >>> { >>> ring->pa = pa; >>> @@ -166,7 +166,7 @@ static inline void vmxnet3_ring_init(PCIDevice *d, >>> } >>> >>> #define VMXNET3_RING_DUMP(macro, ring_name, ridx, r) \ >>> - macro("%s#%d: base %" PRIx64 " size %zu cell_size %zu gen %d next %zu", \ >>> + macro("%s#%d: base %" PRIx64 " size %u cell_size %u gen %d next %u", \ >>> (ring_name), (ridx), \ >>> (r)->pa, (r)->size, (r)->cell_size, (r)->gen, (r)->next) >>> >>> >> >> >> David, with '-dump-vmstate' with 2.8 machine type and with v2.8 and >> master I can see the size of "txq_descr" and "rxq_descr" changes because >> of "sizeof(Vmxnet3TxqDescr)" and "sizeof(Vmxnet3RxqDescr)". This changes >> because the size of Vmxnet3Ring has changed with the s/size_t/uint32_t/": >> >> { >> "field": "txq_descr", >> "version_id": 0, >> "field_exists": false, >> "size": 176 >> }, >> { >> "field": "rxq_descr", >> "version_id": 0, >> "field_exists": false, >> "size": 216 >> }, >> >> becomes: >> >> { >> "field": "txq_descr", >> "version_id": 0, >> "field_exists": false, >> "size": 144 >> }, >> { >> "field": "rxq_descr", >> "version_id": 0, >> "field_exists": false, >> "size": 168 >> }, > > But the old migration code used to write these fields using qemu_put_be32 > (from a11f5cb005f9854f0d78da97fc23adf5bc8c83f3) > > -static void vmxnet3_put_ring_to_file(QEMUFile *f, Vmxnet3Ring *r) > -{ > - qemu_put_be64(f, r->pa); > - qemu_put_be32(f, r->size); > - qemu_put_be32(f, r->cell_size); > - qemu_put_be32(f, r->next); > - qemu_put_byte(f, r->gen); > -} > > +static const VMStateDescription vmstate_vmxnet3_ring = { > + .name = "vmxnet3-ring", > + .version_id = 0, > + .fields = (VMStateField[]) { > + VMSTATE_UINT64(pa, Vmxnet3Ring), > + VMSTATE_UINT32(size, Vmxnet3Ring), > + VMSTATE_UINT32(cell_size, Vmxnet3Ring), > + VMSTATE_UINT32(next, Vmxnet3Ring), > + VMSTATE_UINT8(gen, Vmxnet3Ring), > + VMSTATE_END_OF_LIST() > + } > }; > > so they used to be size_t's written with be32 and now they're uint32_t's > written as 32bit - so that should be the same. > I'm not sure i understand where the old txq_descr sizes come from - what > managed to print the 176 number before it was converted to VMState? I think it comes from the sizeof() in VMSTATE_STRUCT(Vmxnet3TxqDescr): static const VMStateDescription vmstate_vmxnet3_txq_descr = { .name = "vmxnet3-txq-descr", .version_id = 0, .fields = (VMStateField[]) { VMSTATE_STRUCT(tx_ring, Vmxnet3TxqDescr, 0, vmstate_vmxnet3_ring, Vmxnet3Ring), VMSTATE_STRUCT(comp_ring, Vmxnet3TxqDescr, 0, vmstate_vmxnet3_ring, Vmxnet3Ring), VMSTATE_UINT8(intr_idx, Vmxnet3TxqDescr), VMSTATE_UINT64(tx_stats_pa, Vmxnet3TxqDescr), VMSTATE_STRUCT(txq_stats, Vmxnet3TxqDescr, 0, vmstate_vmxnet3_tx_stats, struct UPT1_TxStats), VMSTATE_END_OF_LIST() } }; I will check the '-dump-vmstate' code to see if it's relevant. > >> And if I try a migration, I have: >> >> qemu/migration/vmstate.c:112: vmstate_load_state: Assertion `first_elem >> || !n_elems' failed. > > Interesting; that's Halil's new assert; I need to look. Can't it be related to the size problem? Thanks, Laurent
* Laurent Vivier (lvivier@redhat.com) wrote: > On 14/03/2017 12:16, Dr. David Alan Gilbert wrote: > > * Laurent Vivier (lvivier@redhat.com) wrote: > >> On 06/03/2017 06:25, Jason Wang wrote: > >>> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > >>> > >>> The index's in the Vmxnet3Ring were migrated as 32bit ints > >>> yet are declared as size_t's. They appear to be derived > >>> from 32bit values loaded from guest memory, so actually > >>> store them as that. > >>> > >>> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > >>> Acked-by: Dmitry Fleytman <dmitry@daynix.com> > >>> Signed-off-by: Jason Wang <jasowang@redhat.com> > >>> --- > >>> hw/net/vmxnet3.c | 12 ++++++------ > >>> 1 file changed, 6 insertions(+), 6 deletions(-) > >>> > >>> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c > >>> index e13a798..224c109 100644 > >>> --- a/hw/net/vmxnet3.c > >>> +++ b/hw/net/vmxnet3.c > >>> @@ -141,17 +141,17 @@ typedef struct VMXNET3Class { > >>> /* Cyclic ring abstraction */ > >>> typedef struct { > >>> hwaddr pa; > >>> - size_t size; > >>> - size_t cell_size; > >>> - size_t next; > >>> + uint32_t size; > >>> + uint32_t cell_size; > >>> + uint32_t next; > >>> uint8_t gen; > >>> } Vmxnet3Ring; > >>> > >>> static inline void vmxnet3_ring_init(PCIDevice *d, > >>> Vmxnet3Ring *ring, > >>> hwaddr pa, > >>> - size_t size, > >>> - size_t cell_size, > >>> + uint32_t size, > >>> + uint32_t cell_size, > >>> bool zero_region) > >>> { > >>> ring->pa = pa; > >>> @@ -166,7 +166,7 @@ static inline void vmxnet3_ring_init(PCIDevice *d, > >>> } > >>> > >>> #define VMXNET3_RING_DUMP(macro, ring_name, ridx, r) \ > >>> - macro("%s#%d: base %" PRIx64 " size %zu cell_size %zu gen %d next %zu", \ > >>> + macro("%s#%d: base %" PRIx64 " size %u cell_size %u gen %d next %u", \ > >>> (ring_name), (ridx), \ > >>> (r)->pa, (r)->size, (r)->cell_size, (r)->gen, (r)->next) > >>> > >>> > >> > >> > >> David, with '-dump-vmstate' with 2.8 machine type and with v2.8 and > >> master I can see the size of "txq_descr" and "rxq_descr" changes because > >> of "sizeof(Vmxnet3TxqDescr)" and "sizeof(Vmxnet3RxqDescr)". This changes > >> because the size of Vmxnet3Ring has changed with the s/size_t/uint32_t/": > >> > >> { > >> "field": "txq_descr", > >> "version_id": 0, > >> "field_exists": false, > >> "size": 176 > >> }, > >> { > >> "field": "rxq_descr", > >> "version_id": 0, > >> "field_exists": false, > >> "size": 216 > >> }, > >> > >> becomes: > >> > >> { > >> "field": "txq_descr", > >> "version_id": 0, > >> "field_exists": false, > >> "size": 144 > >> }, > >> { > >> "field": "rxq_descr", > >> "version_id": 0, > >> "field_exists": false, > >> "size": 168 > >> }, > > > > But the old migration code used to write these fields using qemu_put_be32 > > (from a11f5cb005f9854f0d78da97fc23adf5bc8c83f3) > > > > -static void vmxnet3_put_ring_to_file(QEMUFile *f, Vmxnet3Ring *r) > > -{ > > - qemu_put_be64(f, r->pa); > > - qemu_put_be32(f, r->size); > > - qemu_put_be32(f, r->cell_size); > > - qemu_put_be32(f, r->next); > > - qemu_put_byte(f, r->gen); > > -} > > > > +static const VMStateDescription vmstate_vmxnet3_ring = { > > + .name = "vmxnet3-ring", > > + .version_id = 0, > > + .fields = (VMStateField[]) { > > + VMSTATE_UINT64(pa, Vmxnet3Ring), > > + VMSTATE_UINT32(size, Vmxnet3Ring), > > + VMSTATE_UINT32(cell_size, Vmxnet3Ring), > > + VMSTATE_UINT32(next, Vmxnet3Ring), > > + VMSTATE_UINT8(gen, Vmxnet3Ring), > > + VMSTATE_END_OF_LIST() > > + } > > }; > > > > so they used to be size_t's written with be32 and now they're uint32_t's > > written as 32bit - so that should be the same. > > I'm not sure i understand where the old txq_descr sizes come from - what > > managed to print the 176 number before it was converted to VMState? > > I think it comes from the sizeof() in VMSTATE_STRUCT(Vmxnet3TxqDescr): > > static const VMStateDescription vmstate_vmxnet3_txq_descr = { > .name = "vmxnet3-txq-descr", > .version_id = 0, > .fields = (VMStateField[]) { > VMSTATE_STRUCT(tx_ring, Vmxnet3TxqDescr, 0, vmstate_vmxnet3_ring, > Vmxnet3Ring), > VMSTATE_STRUCT(comp_ring, Vmxnet3TxqDescr, 0, vmstate_vmxnet3_ring, > Vmxnet3Ring), > VMSTATE_UINT8(intr_idx, Vmxnet3TxqDescr), > VMSTATE_UINT64(tx_stats_pa, Vmxnet3TxqDescr), > VMSTATE_STRUCT(txq_stats, Vmxnet3TxqDescr, 0, > vmstate_vmxnet3_tx_stats, > struct UPT1_TxStats), > VMSTATE_END_OF_LIST() > } > }; > > I will check the '-dump-vmstate' code to see if it's relevant. OK, but that sizeof() should never make it onto the wire; all that teh VMSTATE_STRUCT should do is execute the vmstate_vmxnet3_ring on that data. (And again I'm curious what happened in the version before I committed that change). > > > >> And if I try a migration, I have: > >> > >> qemu/migration/vmstate.c:112: vmstate_load_state: Assertion `first_elem > >> || !n_elems' failed. > > > > Interesting; that's Halil's new assert; I need to look. > > Can't it be related to the size problem? It could, I need to see exactly how it's failing; what was the full command line you used, and with what guest? Dave > > Thanks, > Laurent > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 14/03/2017 12:38, Dr. David Alan Gilbert wrote: > * Laurent Vivier (lvivier@redhat.com) wrote: >> On 14/03/2017 12:16, Dr. David Alan Gilbert wrote: >>> * Laurent Vivier (lvivier@redhat.com) wrote: >>>> On 06/03/2017 06:25, Jason Wang wrote: >>>>> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> >>>>> >>>>> The index's in the Vmxnet3Ring were migrated as 32bit ints >>>>> yet are declared as size_t's. They appear to be derived >>>>> from 32bit values loaded from guest memory, so actually >>>>> store them as that. >>>>> >>>>> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> >>>>> Acked-by: Dmitry Fleytman <dmitry@daynix.com> >>>>> Signed-off-by: Jason Wang <jasowang@redhat.com> >>>>> --- >>>>> hw/net/vmxnet3.c | 12 ++++++------ >>>>> 1 file changed, 6 insertions(+), 6 deletions(-) >>>>> >>>>> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c >>>>> index e13a798..224c109 100644 >>>>> --- a/hw/net/vmxnet3.c >>>>> +++ b/hw/net/vmxnet3.c >>>>> @@ -141,17 +141,17 @@ typedef struct VMXNET3Class { >>>>> /* Cyclic ring abstraction */ >>>>> typedef struct { >>>>> hwaddr pa; >>>>> - size_t size; >>>>> - size_t cell_size; >>>>> - size_t next; >>>>> + uint32_t size; >>>>> + uint32_t cell_size; >>>>> + uint32_t next; >>>>> uint8_t gen; >>>>> } Vmxnet3Ring; >>>>> >>>>> static inline void vmxnet3_ring_init(PCIDevice *d, >>>>> Vmxnet3Ring *ring, >>>>> hwaddr pa, >>>>> - size_t size, >>>>> - size_t cell_size, >>>>> + uint32_t size, >>>>> + uint32_t cell_size, >>>>> bool zero_region) >>>>> { >>>>> ring->pa = pa; >>>>> @@ -166,7 +166,7 @@ static inline void vmxnet3_ring_init(PCIDevice *d, >>>>> } >>>>> >>>>> #define VMXNET3_RING_DUMP(macro, ring_name, ridx, r) \ >>>>> - macro("%s#%d: base %" PRIx64 " size %zu cell_size %zu gen %d next %zu", \ >>>>> + macro("%s#%d: base %" PRIx64 " size %u cell_size %u gen %d next %u", \ >>>>> (ring_name), (ridx), \ >>>>> (r)->pa, (r)->size, (r)->cell_size, (r)->gen, (r)->next) >>>>> >>>>> >>>> >>>> >>>> David, with '-dump-vmstate' with 2.8 machine type and with v2.8 and >>>> master I can see the size of "txq_descr" and "rxq_descr" changes because >>>> of "sizeof(Vmxnet3TxqDescr)" and "sizeof(Vmxnet3RxqDescr)". This changes >>>> because the size of Vmxnet3Ring has changed with the s/size_t/uint32_t/": >>>> >>>> { >>>> "field": "txq_descr", >>>> "version_id": 0, >>>> "field_exists": false, >>>> "size": 176 >>>> }, >>>> { >>>> "field": "rxq_descr", >>>> "version_id": 0, >>>> "field_exists": false, >>>> "size": 216 >>>> }, >>>> >>>> becomes: >>>> >>>> { >>>> "field": "txq_descr", >>>> "version_id": 0, >>>> "field_exists": false, >>>> "size": 144 >>>> }, >>>> { >>>> "field": "rxq_descr", >>>> "version_id": 0, >>>> "field_exists": false, >>>> "size": 168 >>>> }, >>> >>> But the old migration code used to write these fields using qemu_put_be32 >>> (from a11f5cb005f9854f0d78da97fc23adf5bc8c83f3) >>> >>> -static void vmxnet3_put_ring_to_file(QEMUFile *f, Vmxnet3Ring *r) >>> -{ >>> - qemu_put_be64(f, r->pa); >>> - qemu_put_be32(f, r->size); >>> - qemu_put_be32(f, r->cell_size); >>> - qemu_put_be32(f, r->next); >>> - qemu_put_byte(f, r->gen); >>> -} >>> >>> +static const VMStateDescription vmstate_vmxnet3_ring = { >>> + .name = "vmxnet3-ring", >>> + .version_id = 0, >>> + .fields = (VMStateField[]) { >>> + VMSTATE_UINT64(pa, Vmxnet3Ring), >>> + VMSTATE_UINT32(size, Vmxnet3Ring), >>> + VMSTATE_UINT32(cell_size, Vmxnet3Ring), >>> + VMSTATE_UINT32(next, Vmxnet3Ring), >>> + VMSTATE_UINT8(gen, Vmxnet3Ring), >>> + VMSTATE_END_OF_LIST() >>> + } >>> }; >>> >>> so they used to be size_t's written with be32 and now they're uint32_t's >>> written as 32bit - so that should be the same. >>> I'm not sure i understand where the old txq_descr sizes come from - what >>> managed to print the 176 number before it was converted to VMState? >> >> I think it comes from the sizeof() in VMSTATE_STRUCT(Vmxnet3TxqDescr): >> >> static const VMStateDescription vmstate_vmxnet3_txq_descr = { >> .name = "vmxnet3-txq-descr", >> .version_id = 0, >> .fields = (VMStateField[]) { >> VMSTATE_STRUCT(tx_ring, Vmxnet3TxqDescr, 0, vmstate_vmxnet3_ring, >> Vmxnet3Ring), >> VMSTATE_STRUCT(comp_ring, Vmxnet3TxqDescr, 0, vmstate_vmxnet3_ring, >> Vmxnet3Ring), >> VMSTATE_UINT8(intr_idx, Vmxnet3TxqDescr), >> VMSTATE_UINT64(tx_stats_pa, Vmxnet3TxqDescr), >> VMSTATE_STRUCT(txq_stats, Vmxnet3TxqDescr, 0, >> vmstate_vmxnet3_tx_stats, >> struct UPT1_TxStats), >> VMSTATE_END_OF_LIST() >> } >> }; >> >> I will check the '-dump-vmstate' code to see if it's relevant. > > OK, but that sizeof() should never make it onto the wire; all that > teh VMSTATE_STRUCT should do is execute the vmstate_vmxnet3_ring on > that data. > (And again I'm curious what happened in the version before I committed > that change). > >>> >>>> And if I try a migration, I have: >>>> >>>> qemu/migration/vmstate.c:112: vmstate_load_state: Assertion `first_elem >>>> || !n_elems' failed. >>> >>> Interesting; that's Halil's new assert; I need to look. >> >> Can't it be related to the size problem? > > It could, I need to see exactly how it's failing; what was the full > command line you used, and with what guest? Tested with pseries machine type on ppc64le host, no guest started. qemu-system-ppc64 -M pseries-2.8 -S -serial mon:stdio -vnc :0 -device vmxnet3 Laurent
* Laurent Vivier (lvivier@redhat.com) wrote: > On 14/03/2017 12:38, Dr. David Alan Gilbert wrote: > > * Laurent Vivier (lvivier@redhat.com) wrote: > >> On 14/03/2017 12:16, Dr. David Alan Gilbert wrote: > >>> * Laurent Vivier (lvivier@redhat.com) wrote: > >>>> On 06/03/2017 06:25, Jason Wang wrote: > >>>>> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > >>>>> > >>>>> The index's in the Vmxnet3Ring were migrated as 32bit ints > >>>>> yet are declared as size_t's. They appear to be derived > >>>>> from 32bit values loaded from guest memory, so actually > >>>>> store them as that. > >>>>> > >>>>> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > >>>>> Acked-by: Dmitry Fleytman <dmitry@daynix.com> > >>>>> Signed-off-by: Jason Wang <jasowang@redhat.com> > >>>>> --- > >>>>> hw/net/vmxnet3.c | 12 ++++++------ > >>>>> 1 file changed, 6 insertions(+), 6 deletions(-) > >>>>> > >>>>> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c > >>>>> index e13a798..224c109 100644 > >>>>> --- a/hw/net/vmxnet3.c > >>>>> +++ b/hw/net/vmxnet3.c > >>>>> @@ -141,17 +141,17 @@ typedef struct VMXNET3Class { > >>>>> /* Cyclic ring abstraction */ > >>>>> typedef struct { > >>>>> hwaddr pa; > >>>>> - size_t size; > >>>>> - size_t cell_size; > >>>>> - size_t next; > >>>>> + uint32_t size; > >>>>> + uint32_t cell_size; > >>>>> + uint32_t next; > >>>>> uint8_t gen; > >>>>> } Vmxnet3Ring; > >>>>> > >>>>> static inline void vmxnet3_ring_init(PCIDevice *d, > >>>>> Vmxnet3Ring *ring, > >>>>> hwaddr pa, > >>>>> - size_t size, > >>>>> - size_t cell_size, > >>>>> + uint32_t size, > >>>>> + uint32_t cell_size, > >>>>> bool zero_region) > >>>>> { > >>>>> ring->pa = pa; > >>>>> @@ -166,7 +166,7 @@ static inline void vmxnet3_ring_init(PCIDevice *d, > >>>>> } > >>>>> > >>>>> #define VMXNET3_RING_DUMP(macro, ring_name, ridx, r) \ > >>>>> - macro("%s#%d: base %" PRIx64 " size %zu cell_size %zu gen %d next %zu", \ > >>>>> + macro("%s#%d: base %" PRIx64 " size %u cell_size %u gen %d next %u", \ > >>>>> (ring_name), (ridx), \ > >>>>> (r)->pa, (r)->size, (r)->cell_size, (r)->gen, (r)->next) > >>>>> > >>>>> > >>>> > >>>> > >>>> David, with '-dump-vmstate' with 2.8 machine type and with v2.8 and > >>>> master I can see the size of "txq_descr" and "rxq_descr" changes because > >>>> of "sizeof(Vmxnet3TxqDescr)" and "sizeof(Vmxnet3RxqDescr)". This changes > >>>> because the size of Vmxnet3Ring has changed with the s/size_t/uint32_t/": > >>>> > >>>> { > >>>> "field": "txq_descr", > >>>> "version_id": 0, > >>>> "field_exists": false, > >>>> "size": 176 > >>>> }, > >>>> { > >>>> "field": "rxq_descr", > >>>> "version_id": 0, > >>>> "field_exists": false, > >>>> "size": 216 > >>>> }, > >>>> > >>>> becomes: > >>>> > >>>> { > >>>> "field": "txq_descr", > >>>> "version_id": 0, > >>>> "field_exists": false, > >>>> "size": 144 > >>>> }, > >>>> { > >>>> "field": "rxq_descr", > >>>> "version_id": 0, > >>>> "field_exists": false, > >>>> "size": 168 > >>>> }, > >>> > >>> But the old migration code used to write these fields using qemu_put_be32 > >>> (from a11f5cb005f9854f0d78da97fc23adf5bc8c83f3) > >>> > >>> -static void vmxnet3_put_ring_to_file(QEMUFile *f, Vmxnet3Ring *r) > >>> -{ > >>> - qemu_put_be64(f, r->pa); > >>> - qemu_put_be32(f, r->size); > >>> - qemu_put_be32(f, r->cell_size); > >>> - qemu_put_be32(f, r->next); > >>> - qemu_put_byte(f, r->gen); > >>> -} > >>> > >>> +static const VMStateDescription vmstate_vmxnet3_ring = { > >>> + .name = "vmxnet3-ring", > >>> + .version_id = 0, > >>> + .fields = (VMStateField[]) { > >>> + VMSTATE_UINT64(pa, Vmxnet3Ring), > >>> + VMSTATE_UINT32(size, Vmxnet3Ring), > >>> + VMSTATE_UINT32(cell_size, Vmxnet3Ring), > >>> + VMSTATE_UINT32(next, Vmxnet3Ring), > >>> + VMSTATE_UINT8(gen, Vmxnet3Ring), > >>> + VMSTATE_END_OF_LIST() > >>> + } > >>> }; > >>> > >>> so they used to be size_t's written with be32 and now they're uint32_t's > >>> written as 32bit - so that should be the same. > >>> I'm not sure i understand where the old txq_descr sizes come from - what > >>> managed to print the 176 number before it was converted to VMState? > >> > >> I think it comes from the sizeof() in VMSTATE_STRUCT(Vmxnet3TxqDescr): > >> > >> static const VMStateDescription vmstate_vmxnet3_txq_descr = { > >> .name = "vmxnet3-txq-descr", > >> .version_id = 0, > >> .fields = (VMStateField[]) { > >> VMSTATE_STRUCT(tx_ring, Vmxnet3TxqDescr, 0, vmstate_vmxnet3_ring, > >> Vmxnet3Ring), > >> VMSTATE_STRUCT(comp_ring, Vmxnet3TxqDescr, 0, vmstate_vmxnet3_ring, > >> Vmxnet3Ring), > >> VMSTATE_UINT8(intr_idx, Vmxnet3TxqDescr), > >> VMSTATE_UINT64(tx_stats_pa, Vmxnet3TxqDescr), > >> VMSTATE_STRUCT(txq_stats, Vmxnet3TxqDescr, 0, > >> vmstate_vmxnet3_tx_stats, > >> struct UPT1_TxStats), > >> VMSTATE_END_OF_LIST() > >> } > >> }; > >> > >> I will check the '-dump-vmstate' code to see if it's relevant. > > > > OK, but that sizeof() should never make it onto the wire; all that > > teh VMSTATE_STRUCT should do is execute the vmstate_vmxnet3_ring on > > that data. > > (And again I'm curious what happened in the version before I committed > > that change). > > > >>> > >>>> And if I try a migration, I have: > >>>> > >>>> qemu/migration/vmstate.c:112: vmstate_load_state: Assertion `first_elem > >>>> || !n_elems' failed. > >>> > >>> Interesting; that's Halil's new assert; I need to look. > >> > >> Can't it be related to the size problem? > > > > It could, I need to see exactly how it's failing; what was the full > > command line you used, and with what guest? > > Tested with pseries machine type on ppc64le host, no guest started. > > qemu-system-ppc64 -M pseries-2.8 -S -serial mon:stdio -vnc :0 -device > vmxnet3 OK, I can trigger this on x86 on outgoing migration - the same assert but in the vmstate_save_state side; for me it's in the older vmxstate_vmxnet3_mcast_list can you confirm that it's the same structure you're hitting on the load side? Dave > Laurent > > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 14/03/2017 13:32, Dr. David Alan Gilbert wrote: > * Laurent Vivier (lvivier@redhat.com) wrote: >> On 14/03/2017 12:38, Dr. David Alan Gilbert wrote: >>> * Laurent Vivier (lvivier@redhat.com) wrote: >>>> On 14/03/2017 12:16, Dr. David Alan Gilbert wrote: >>>>> * Laurent Vivier (lvivier@redhat.com) wrote: >>>>>> On 06/03/2017 06:25, Jason Wang wrote: >>>>>>> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> >>>>>>> >>>>>>> The index's in the Vmxnet3Ring were migrated as 32bit ints >>>>>>> yet are declared as size_t's. They appear to be derived >>>>>>> from 32bit values loaded from guest memory, so actually >>>>>>> store them as that. >>>>>>> >>>>>>> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> >>>>>>> Acked-by: Dmitry Fleytman <dmitry@daynix.com> >>>>>>> Signed-off-by: Jason Wang <jasowang@redhat.com> >>>>>>> --- >>>>>>> hw/net/vmxnet3.c | 12 ++++++------ >>>>>>> 1 file changed, 6 insertions(+), 6 deletions(-) >>>>>>> >>>>>>> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c >>>>>>> index e13a798..224c109 100644 >>>>>>> --- a/hw/net/vmxnet3.c >>>>>>> +++ b/hw/net/vmxnet3.c >>>>>>> @@ -141,17 +141,17 @@ typedef struct VMXNET3Class { >>>>>>> /* Cyclic ring abstraction */ >>>>>>> typedef struct { >>>>>>> hwaddr pa; >>>>>>> - size_t size; >>>>>>> - size_t cell_size; >>>>>>> - size_t next; >>>>>>> + uint32_t size; >>>>>>> + uint32_t cell_size; >>>>>>> + uint32_t next; >>>>>>> uint8_t gen; >>>>>>> } Vmxnet3Ring; >>>>>>> >>>>>>> static inline void vmxnet3_ring_init(PCIDevice *d, >>>>>>> Vmxnet3Ring *ring, >>>>>>> hwaddr pa, >>>>>>> - size_t size, >>>>>>> - size_t cell_size, >>>>>>> + uint32_t size, >>>>>>> + uint32_t cell_size, >>>>>>> bool zero_region) >>>>>>> { >>>>>>> ring->pa = pa; >>>>>>> @@ -166,7 +166,7 @@ static inline void vmxnet3_ring_init(PCIDevice *d, >>>>>>> } >>>>>>> >>>>>>> #define VMXNET3_RING_DUMP(macro, ring_name, ridx, r) \ >>>>>>> - macro("%s#%d: base %" PRIx64 " size %zu cell_size %zu gen %d next %zu", \ >>>>>>> + macro("%s#%d: base %" PRIx64 " size %u cell_size %u gen %d next %u", \ >>>>>>> (ring_name), (ridx), \ >>>>>>> (r)->pa, (r)->size, (r)->cell_size, (r)->gen, (r)->next) >>>>>>> >>>>>>> >>>>>> >>>>>> >>>>>> David, with '-dump-vmstate' with 2.8 machine type and with v2.8 and >>>>>> master I can see the size of "txq_descr" and "rxq_descr" changes because >>>>>> of "sizeof(Vmxnet3TxqDescr)" and "sizeof(Vmxnet3RxqDescr)". This changes >>>>>> because the size of Vmxnet3Ring has changed with the s/size_t/uint32_t/": >>>>>> >>>>>> { >>>>>> "field": "txq_descr", >>>>>> "version_id": 0, >>>>>> "field_exists": false, >>>>>> "size": 176 >>>>>> }, >>>>>> { >>>>>> "field": "rxq_descr", >>>>>> "version_id": 0, >>>>>> "field_exists": false, >>>>>> "size": 216 >>>>>> }, >>>>>> >>>>>> becomes: >>>>>> >>>>>> { >>>>>> "field": "txq_descr", >>>>>> "version_id": 0, >>>>>> "field_exists": false, >>>>>> "size": 144 >>>>>> }, >>>>>> { >>>>>> "field": "rxq_descr", >>>>>> "version_id": 0, >>>>>> "field_exists": false, >>>>>> "size": 168 >>>>>> }, >>>>> >>>>> But the old migration code used to write these fields using qemu_put_be32 >>>>> (from a11f5cb005f9854f0d78da97fc23adf5bc8c83f3) >>>>> >>>>> -static void vmxnet3_put_ring_to_file(QEMUFile *f, Vmxnet3Ring *r) >>>>> -{ >>>>> - qemu_put_be64(f, r->pa); >>>>> - qemu_put_be32(f, r->size); >>>>> - qemu_put_be32(f, r->cell_size); >>>>> - qemu_put_be32(f, r->next); >>>>> - qemu_put_byte(f, r->gen); >>>>> -} >>>>> >>>>> +static const VMStateDescription vmstate_vmxnet3_ring = { >>>>> + .name = "vmxnet3-ring", >>>>> + .version_id = 0, >>>>> + .fields = (VMStateField[]) { >>>>> + VMSTATE_UINT64(pa, Vmxnet3Ring), >>>>> + VMSTATE_UINT32(size, Vmxnet3Ring), >>>>> + VMSTATE_UINT32(cell_size, Vmxnet3Ring), >>>>> + VMSTATE_UINT32(next, Vmxnet3Ring), >>>>> + VMSTATE_UINT8(gen, Vmxnet3Ring), >>>>> + VMSTATE_END_OF_LIST() >>>>> + } >>>>> }; >>>>> >>>>> so they used to be size_t's written with be32 and now they're uint32_t's >>>>> written as 32bit - so that should be the same. >>>>> I'm not sure i understand where the old txq_descr sizes come from - what >>>>> managed to print the 176 number before it was converted to VMState? >>>> >>>> I think it comes from the sizeof() in VMSTATE_STRUCT(Vmxnet3TxqDescr): >>>> >>>> static const VMStateDescription vmstate_vmxnet3_txq_descr = { >>>> .name = "vmxnet3-txq-descr", >>>> .version_id = 0, >>>> .fields = (VMStateField[]) { >>>> VMSTATE_STRUCT(tx_ring, Vmxnet3TxqDescr, 0, vmstate_vmxnet3_ring, >>>> Vmxnet3Ring), >>>> VMSTATE_STRUCT(comp_ring, Vmxnet3TxqDescr, 0, vmstate_vmxnet3_ring, >>>> Vmxnet3Ring), >>>> VMSTATE_UINT8(intr_idx, Vmxnet3TxqDescr), >>>> VMSTATE_UINT64(tx_stats_pa, Vmxnet3TxqDescr), >>>> VMSTATE_STRUCT(txq_stats, Vmxnet3TxqDescr, 0, >>>> vmstate_vmxnet3_tx_stats, >>>> struct UPT1_TxStats), >>>> VMSTATE_END_OF_LIST() >>>> } >>>> }; >>>> >>>> I will check the '-dump-vmstate' code to see if it's relevant. >>> >>> OK, but that sizeof() should never make it onto the wire; all that >>> teh VMSTATE_STRUCT should do is execute the vmstate_vmxnet3_ring on >>> that data. >>> (And again I'm curious what happened in the version before I committed >>> that change). >>> >>>>> >>>>>> And if I try a migration, I have: >>>>>> >>>>>> qemu/migration/vmstate.c:112: vmstate_load_state: Assertion `first_elem >>>>>> || !n_elems' failed. >>>>> >>>>> Interesting; that's Halil's new assert; I need to look. >>>> >>>> Can't it be related to the size problem? >>> >>> It could, I need to see exactly how it's failing; what was the full >>> command line you used, and with what guest? >> >> Tested with pseries machine type on ppc64le host, no guest started. >> >> qemu-system-ppc64 -M pseries-2.8 -S -serial mon:stdio -vnc :0 -device >> vmxnet3 > > OK, I can trigger this on x86 on outgoing migration - the same assert > but in the vmstate_save_state side; for me it's in the older vmxstate_vmxnet3_mcast_list > can you confirm that it's the same structure you're hitting on the load side? It always happens on the master branch side (load or save): if master is on the destination side: #4 0x00000000277fb9e8 in vmstate_load_state (f=0x2b9a2000, vmsd=0x27d37c08 <vmxstate_vmxnet3_mcast_list>, opaque=0x28a9c000, version_id=<optimized out>) at /home/lvivier/Projects/qemu/migration/vmstate.c:112 if master is on the source side: #4 0x000000002e2bcbb8 in vmstate_save_state (f=0x32462000, vmsd=0x2e7f7c08 <vmxstate_vmxnet3_mcast_list>, opaque=0x2f55c000, vmdesc=0x32259ef0) at /home/lvivier/Projects/qemu/migration/vmstate.c:328 Laurent
* Laurent Vivier (lvivier@redhat.com) wrote: > On 14/03/2017 12:38, Dr. David Alan Gilbert wrote: > > * Laurent Vivier (lvivier@redhat.com) wrote: > >> On 14/03/2017 12:16, Dr. David Alan Gilbert wrote: > >>> * Laurent Vivier (lvivier@redhat.com) wrote: > >>>> On 06/03/2017 06:25, Jason Wang wrote: > >>>>> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > >>>>> > >>>>> The index's in the Vmxnet3Ring were migrated as 32bit ints > >>>>> yet are declared as size_t's. They appear to be derived > >>>>> from 32bit values loaded from guest memory, so actually > >>>>> store them as that. > >>>>> > >>>>> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > >>>>> Acked-by: Dmitry Fleytman <dmitry@daynix.com> > >>>>> Signed-off-by: Jason Wang <jasowang@redhat.com> > >>>>> --- > >>>>> hw/net/vmxnet3.c | 12 ++++++------ > >>>>> 1 file changed, 6 insertions(+), 6 deletions(-) > >>>>> > >>>>> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c > >>>>> index e13a798..224c109 100644 > >>>>> --- a/hw/net/vmxnet3.c > >>>>> +++ b/hw/net/vmxnet3.c > >>>>> @@ -141,17 +141,17 @@ typedef struct VMXNET3Class { > >>>>> /* Cyclic ring abstraction */ > >>>>> typedef struct { > >>>>> hwaddr pa; > >>>>> - size_t size; > >>>>> - size_t cell_size; > >>>>> - size_t next; > >>>>> + uint32_t size; > >>>>> + uint32_t cell_size; > >>>>> + uint32_t next; > >>>>> uint8_t gen; > >>>>> } Vmxnet3Ring; > >>>>> > >>>>> static inline void vmxnet3_ring_init(PCIDevice *d, > >>>>> Vmxnet3Ring *ring, > >>>>> hwaddr pa, > >>>>> - size_t size, > >>>>> - size_t cell_size, > >>>>> + uint32_t size, > >>>>> + uint32_t cell_size, > >>>>> bool zero_region) > >>>>> { > >>>>> ring->pa = pa; > >>>>> @@ -166,7 +166,7 @@ static inline void vmxnet3_ring_init(PCIDevice *d, > >>>>> } > >>>>> > >>>>> #define VMXNET3_RING_DUMP(macro, ring_name, ridx, r) \ > >>>>> - macro("%s#%d: base %" PRIx64 " size %zu cell_size %zu gen %d next %zu", \ > >>>>> + macro("%s#%d: base %" PRIx64 " size %u cell_size %u gen %d next %u", \ > >>>>> (ring_name), (ridx), \ > >>>>> (r)->pa, (r)->size, (r)->cell_size, (r)->gen, (r)->next) > >>>>> > >>>>> > >>>> > >>>> > >>>> David, with '-dump-vmstate' with 2.8 machine type and with v2.8 and > >>>> master I can see the size of "txq_descr" and "rxq_descr" changes because > >>>> of "sizeof(Vmxnet3TxqDescr)" and "sizeof(Vmxnet3RxqDescr)". This changes > >>>> because the size of Vmxnet3Ring has changed with the s/size_t/uint32_t/": > >>>> > >>>> { > >>>> "field": "txq_descr", > >>>> "version_id": 0, > >>>> "field_exists": false, > >>>> "size": 176 > >>>> }, > >>>> { > >>>> "field": "rxq_descr", > >>>> "version_id": 0, > >>>> "field_exists": false, > >>>> "size": 216 > >>>> }, > >>>> > >>>> becomes: > >>>> > >>>> { > >>>> "field": "txq_descr", > >>>> "version_id": 0, > >>>> "field_exists": false, > >>>> "size": 144 > >>>> }, > >>>> { > >>>> "field": "rxq_descr", > >>>> "version_id": 0, > >>>> "field_exists": false, > >>>> "size": 168 > >>>> }, > >>> > >>> But the old migration code used to write these fields using qemu_put_be32 > >>> (from a11f5cb005f9854f0d78da97fc23adf5bc8c83f3) > >>> > >>> -static void vmxnet3_put_ring_to_file(QEMUFile *f, Vmxnet3Ring *r) > >>> -{ > >>> - qemu_put_be64(f, r->pa); > >>> - qemu_put_be32(f, r->size); > >>> - qemu_put_be32(f, r->cell_size); > >>> - qemu_put_be32(f, r->next); > >>> - qemu_put_byte(f, r->gen); > >>> -} > >>> > >>> +static const VMStateDescription vmstate_vmxnet3_ring = { > >>> + .name = "vmxnet3-ring", > >>> + .version_id = 0, > >>> + .fields = (VMStateField[]) { > >>> + VMSTATE_UINT64(pa, Vmxnet3Ring), > >>> + VMSTATE_UINT32(size, Vmxnet3Ring), > >>> + VMSTATE_UINT32(cell_size, Vmxnet3Ring), > >>> + VMSTATE_UINT32(next, Vmxnet3Ring), > >>> + VMSTATE_UINT8(gen, Vmxnet3Ring), > >>> + VMSTATE_END_OF_LIST() > >>> + } > >>> }; > >>> > >>> so they used to be size_t's written with be32 and now they're uint32_t's > >>> written as 32bit - so that should be the same. > >>> I'm not sure i understand where the old txq_descr sizes come from - what > >>> managed to print the 176 number before it was converted to VMState? > >> > >> I think it comes from the sizeof() in VMSTATE_STRUCT(Vmxnet3TxqDescr): > >> > >> static const VMStateDescription vmstate_vmxnet3_txq_descr = { > >> .name = "vmxnet3-txq-descr", > >> .version_id = 0, > >> .fields = (VMStateField[]) { > >> VMSTATE_STRUCT(tx_ring, Vmxnet3TxqDescr, 0, vmstate_vmxnet3_ring, > >> Vmxnet3Ring), > >> VMSTATE_STRUCT(comp_ring, Vmxnet3TxqDescr, 0, vmstate_vmxnet3_ring, > >> Vmxnet3Ring), > >> VMSTATE_UINT8(intr_idx, Vmxnet3TxqDescr), > >> VMSTATE_UINT64(tx_stats_pa, Vmxnet3TxqDescr), > >> VMSTATE_STRUCT(txq_stats, Vmxnet3TxqDescr, 0, > >> vmstate_vmxnet3_tx_stats, > >> struct UPT1_TxStats), > >> VMSTATE_END_OF_LIST() > >> } > >> }; > >> > >> I will check the '-dump-vmstate' code to see if it's relevant. > > > > OK, but that sizeof() should never make it onto the wire; all that > > teh VMSTATE_STRUCT should do is execute the vmstate_vmxnet3_ring on > > that data. > > (And again I'm curious what happened in the version before I committed > > that change). > > > >>> > >>>> And if I try a migration, I have: > >>>> > >>>> qemu/migration/vmstate.c:112: vmstate_load_state: Assertion `first_elem > >>>> || !n_elems' failed. > >>> > >>> Interesting; that's Halil's new assert; I need to look. > >> > >> Can't it be related to the size problem? > > > > It could, I need to see exactly how it's failing; what was the full > > command line you used, and with what guest? > > Tested with pseries machine type on ppc64le host, no guest started. > > qemu-system-ppc64 -M pseries-2.8 -S -serial mon:stdio -vnc :0 -device > vmxnet3 > OK, I think this is just Halil's assert being overly-pessimistic, it needs a similar fix as proposed for s390x; static const VMStateDescription vmxstate_vmxnet3_mcast_list = { .name = "vmxnet3/mcast_list", .version_id = 1, .minimum_version_id = 1, .pre_load = vmxnet3_mcast_list_pre_load, .needed = vmxnet3_mc_list_needed, .fields = (VMStateField[]) { VMSTATE_VBUFFER_UINT32(mcast_list, VMXNET3State, 0, NULL, mcast_list_buff_size), VMSTATE_END_OF_LIST() } }; in the case where the mcast list is empty, we end up with mcast_list_buff_size =0 and mcast_list = NULL and I think a VMSTATE_VBUFFER_UINT32 ends up with first_elem = NULL, n_elems = 1 *size_offset = 0 so it ends up with a single zero size offset element at a NULL pointer; which is legal but hits that new assert. Dave > Laurent > > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
* Laurent Vivier (lvivier@redhat.com) wrote: > On 14/03/2017 12:38, Dr. David Alan Gilbert wrote: > > * Laurent Vivier (lvivier@redhat.com) wrote: > >> On 14/03/2017 12:16, Dr. David Alan Gilbert wrote: > >>> * Laurent Vivier (lvivier@redhat.com) wrote: > >>>> On 06/03/2017 06:25, Jason Wang wrote: > >>>>> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > >>>>> > >>>>> The index's in the Vmxnet3Ring were migrated as 32bit ints > >>>>> yet are declared as size_t's. They appear to be derived > >>>>> from 32bit values loaded from guest memory, so actually > >>>>> store them as that. > >>>>> > >>>>> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > >>>>> Acked-by: Dmitry Fleytman <dmitry@daynix.com> > >>>>> Signed-off-by: Jason Wang <jasowang@redhat.com> > >>>>> --- > >>>>> hw/net/vmxnet3.c | 12 ++++++------ > >>>>> 1 file changed, 6 insertions(+), 6 deletions(-) > >>>>> > >>>>> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c > >>>>> index e13a798..224c109 100644 > >>>>> --- a/hw/net/vmxnet3.c > >>>>> +++ b/hw/net/vmxnet3.c > >>>>> @@ -141,17 +141,17 @@ typedef struct VMXNET3Class { > >>>>> /* Cyclic ring abstraction */ > >>>>> typedef struct { > >>>>> hwaddr pa; > >>>>> - size_t size; > >>>>> - size_t cell_size; > >>>>> - size_t next; > >>>>> + uint32_t size; > >>>>> + uint32_t cell_size; > >>>>> + uint32_t next; > >>>>> uint8_t gen; > >>>>> } Vmxnet3Ring; > >>>>> > >>>>> static inline void vmxnet3_ring_init(PCIDevice *d, > >>>>> Vmxnet3Ring *ring, > >>>>> hwaddr pa, > >>>>> - size_t size, > >>>>> - size_t cell_size, > >>>>> + uint32_t size, > >>>>> + uint32_t cell_size, > >>>>> bool zero_region) > >>>>> { > >>>>> ring->pa = pa; > >>>>> @@ -166,7 +166,7 @@ static inline void vmxnet3_ring_init(PCIDevice *d, > >>>>> } > >>>>> > >>>>> #define VMXNET3_RING_DUMP(macro, ring_name, ridx, r) \ > >>>>> - macro("%s#%d: base %" PRIx64 " size %zu cell_size %zu gen %d next %zu", \ > >>>>> + macro("%s#%d: base %" PRIx64 " size %u cell_size %u gen %d next %u", \ > >>>>> (ring_name), (ridx), \ > >>>>> (r)->pa, (r)->size, (r)->cell_size, (r)->gen, (r)->next) > >>>>> > >>>>> > >>>> > >>>> > >>>> David, with '-dump-vmstate' with 2.8 machine type and with v2.8 and > >>>> master I can see the size of "txq_descr" and "rxq_descr" changes because > >>>> of "sizeof(Vmxnet3TxqDescr)" and "sizeof(Vmxnet3RxqDescr)". This changes > >>>> because the size of Vmxnet3Ring has changed with the s/size_t/uint32_t/": > >>>> > >>>> { > >>>> "field": "txq_descr", > >>>> "version_id": 0, > >>>> "field_exists": false, > >>>> "size": 176 > >>>> }, > >>>> { > >>>> "field": "rxq_descr", > >>>> "version_id": 0, > >>>> "field_exists": false, > >>>> "size": 216 > >>>> }, > >>>> > >>>> becomes: > >>>> > >>>> { > >>>> "field": "txq_descr", > >>>> "version_id": 0, > >>>> "field_exists": false, > >>>> "size": 144 > >>>> }, > >>>> { > >>>> "field": "rxq_descr", > >>>> "version_id": 0, > >>>> "field_exists": false, > >>>> "size": 168 > >>>> }, > >>> > >>> But the old migration code used to write these fields using qemu_put_be32 > >>> (from a11f5cb005f9854f0d78da97fc23adf5bc8c83f3) > >>> > >>> -static void vmxnet3_put_ring_to_file(QEMUFile *f, Vmxnet3Ring *r) > >>> -{ > >>> - qemu_put_be64(f, r->pa); > >>> - qemu_put_be32(f, r->size); > >>> - qemu_put_be32(f, r->cell_size); > >>> - qemu_put_be32(f, r->next); > >>> - qemu_put_byte(f, r->gen); > >>> -} > >>> > >>> +static const VMStateDescription vmstate_vmxnet3_ring = { > >>> + .name = "vmxnet3-ring", > >>> + .version_id = 0, > >>> + .fields = (VMStateField[]) { > >>> + VMSTATE_UINT64(pa, Vmxnet3Ring), > >>> + VMSTATE_UINT32(size, Vmxnet3Ring), > >>> + VMSTATE_UINT32(cell_size, Vmxnet3Ring), > >>> + VMSTATE_UINT32(next, Vmxnet3Ring), > >>> + VMSTATE_UINT8(gen, Vmxnet3Ring), > >>> + VMSTATE_END_OF_LIST() > >>> + } > >>> }; > >>> > >>> so they used to be size_t's written with be32 and now they're uint32_t's > >>> written as 32bit - so that should be the same. > >>> I'm not sure i understand where the old txq_descr sizes come from - what > >>> managed to print the 176 number before it was converted to VMState? > >> > >> I think it comes from the sizeof() in VMSTATE_STRUCT(Vmxnet3TxqDescr): > >> > >> static const VMStateDescription vmstate_vmxnet3_txq_descr = { > >> .name = "vmxnet3-txq-descr", > >> .version_id = 0, > >> .fields = (VMStateField[]) { > >> VMSTATE_STRUCT(tx_ring, Vmxnet3TxqDescr, 0, vmstate_vmxnet3_ring, > >> Vmxnet3Ring), > >> VMSTATE_STRUCT(comp_ring, Vmxnet3TxqDescr, 0, vmstate_vmxnet3_ring, > >> Vmxnet3Ring), > >> VMSTATE_UINT8(intr_idx, Vmxnet3TxqDescr), > >> VMSTATE_UINT64(tx_stats_pa, Vmxnet3TxqDescr), > >> VMSTATE_STRUCT(txq_stats, Vmxnet3TxqDescr, 0, > >> vmstate_vmxnet3_tx_stats, > >> struct UPT1_TxStats), > >> VMSTATE_END_OF_LIST() > >> } > >> }; > >> > >> I will check the '-dump-vmstate' code to see if it's relevant. > > > > OK, but that sizeof() should never make it onto the wire; all that > > teh VMSTATE_STRUCT should do is execute the vmstate_vmxnet3_ring on > > that data. > > (And again I'm curious what happened in the version before I committed > > that change). > > > >>> > >>>> And if I try a migration, I have: > >>>> > >>>> qemu/migration/vmstate.c:112: vmstate_load_state: Assertion `first_elem > >>>> || !n_elems' failed. > >>> > >>> Interesting; that's Halil's new assert; I need to look. > >> > >> Can't it be related to the size problem? > > > > It could, I need to see exactly how it's failing; what was the full > > command line you used, and with what guest? > > Tested with pseries machine type on ppc64le host, no guest started. > > qemu-system-ppc64 -M pseries-2.8 -S -serial mon:stdio -vnc :0 -device > vmxnet3 QingFeng Hao's <haoqf@linux.vnet.ibm.com> patch: 'vmstate: fix failed iotests case 68 and 91' seems to fix that for me. Dave > Laurent > > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 14/03/2017 14:31, Dr. David Alan Gilbert wrote: > * Laurent Vivier (lvivier@redhat.com) wrote: >> On 14/03/2017 12:38, Dr. David Alan Gilbert wrote: >>> * Laurent Vivier (lvivier@redhat.com) wrote: >>>> On 14/03/2017 12:16, Dr. David Alan Gilbert wrote: >>>>> * Laurent Vivier (lvivier@redhat.com) wrote: >>>>>> On 06/03/2017 06:25, Jason Wang wrote: >>>>>>> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> >>>>>>> >>>>>>> The index's in the Vmxnet3Ring were migrated as 32bit ints >>>>>>> yet are declared as size_t's. They appear to be derived >>>>>>> from 32bit values loaded from guest memory, so actually >>>>>>> store them as that. >>>>>>> >>>>>>> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> >>>>>>> Acked-by: Dmitry Fleytman <dmitry@daynix.com> >>>>>>> Signed-off-by: Jason Wang <jasowang@redhat.com> >>>>>>> --- >>>>>>> hw/net/vmxnet3.c | 12 ++++++------ >>>>>>> 1 file changed, 6 insertions(+), 6 deletions(-) >>>>>>> >>>>>>> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c >>>>>>> index e13a798..224c109 100644 >>>>>>> --- a/hw/net/vmxnet3.c >>>>>>> +++ b/hw/net/vmxnet3.c >>>>>>> @@ -141,17 +141,17 @@ typedef struct VMXNET3Class { >>>>>>> /* Cyclic ring abstraction */ >>>>>>> typedef struct { >>>>>>> hwaddr pa; >>>>>>> - size_t size; >>>>>>> - size_t cell_size; >>>>>>> - size_t next; >>>>>>> + uint32_t size; >>>>>>> + uint32_t cell_size; >>>>>>> + uint32_t next; >>>>>>> uint8_t gen; >>>>>>> } Vmxnet3Ring; >>>>>>> >>>>>>> static inline void vmxnet3_ring_init(PCIDevice *d, >>>>>>> Vmxnet3Ring *ring, >>>>>>> hwaddr pa, >>>>>>> - size_t size, >>>>>>> - size_t cell_size, >>>>>>> + uint32_t size, >>>>>>> + uint32_t cell_size, >>>>>>> bool zero_region) >>>>>>> { >>>>>>> ring->pa = pa; >>>>>>> @@ -166,7 +166,7 @@ static inline void vmxnet3_ring_init(PCIDevice *d, >>>>>>> } >>>>>>> >>>>>>> #define VMXNET3_RING_DUMP(macro, ring_name, ridx, r) \ >>>>>>> - macro("%s#%d: base %" PRIx64 " size %zu cell_size %zu gen %d next %zu", \ >>>>>>> + macro("%s#%d: base %" PRIx64 " size %u cell_size %u gen %d next %u", \ >>>>>>> (ring_name), (ridx), \ >>>>>>> (r)->pa, (r)->size, (r)->cell_size, (r)->gen, (r)->next) >>>>>>> >>>>>>> >>>>>> >>>>>> >>>>>> David, with '-dump-vmstate' with 2.8 machine type and with v2.8 and >>>>>> master I can see the size of "txq_descr" and "rxq_descr" changes because >>>>>> of "sizeof(Vmxnet3TxqDescr)" and "sizeof(Vmxnet3RxqDescr)". This changes >>>>>> because the size of Vmxnet3Ring has changed with the s/size_t/uint32_t/": >>>>>> >>>>>> { >>>>>> "field": "txq_descr", >>>>>> "version_id": 0, >>>>>> "field_exists": false, >>>>>> "size": 176 >>>>>> }, >>>>>> { >>>>>> "field": "rxq_descr", >>>>>> "version_id": 0, >>>>>> "field_exists": false, >>>>>> "size": 216 >>>>>> }, >>>>>> >>>>>> becomes: >>>>>> >>>>>> { >>>>>> "field": "txq_descr", >>>>>> "version_id": 0, >>>>>> "field_exists": false, >>>>>> "size": 144 >>>>>> }, >>>>>> { >>>>>> "field": "rxq_descr", >>>>>> "version_id": 0, >>>>>> "field_exists": false, >>>>>> "size": 168 >>>>>> }, >>>>> >>>>> But the old migration code used to write these fields using qemu_put_be32 >>>>> (from a11f5cb005f9854f0d78da97fc23adf5bc8c83f3) >>>>> >>>>> -static void vmxnet3_put_ring_to_file(QEMUFile *f, Vmxnet3Ring *r) >>>>> -{ >>>>> - qemu_put_be64(f, r->pa); >>>>> - qemu_put_be32(f, r->size); >>>>> - qemu_put_be32(f, r->cell_size); >>>>> - qemu_put_be32(f, r->next); >>>>> - qemu_put_byte(f, r->gen); >>>>> -} >>>>> >>>>> +static const VMStateDescription vmstate_vmxnet3_ring = { >>>>> + .name = "vmxnet3-ring", >>>>> + .version_id = 0, >>>>> + .fields = (VMStateField[]) { >>>>> + VMSTATE_UINT64(pa, Vmxnet3Ring), >>>>> + VMSTATE_UINT32(size, Vmxnet3Ring), >>>>> + VMSTATE_UINT32(cell_size, Vmxnet3Ring), >>>>> + VMSTATE_UINT32(next, Vmxnet3Ring), >>>>> + VMSTATE_UINT8(gen, Vmxnet3Ring), >>>>> + VMSTATE_END_OF_LIST() >>>>> + } >>>>> }; >>>>> >>>>> so they used to be size_t's written with be32 and now they're uint32_t's >>>>> written as 32bit - so that should be the same. >>>>> I'm not sure i understand where the old txq_descr sizes come from - what >>>>> managed to print the 176 number before it was converted to VMState? >>>> >>>> I think it comes from the sizeof() in VMSTATE_STRUCT(Vmxnet3TxqDescr): >>>> >>>> static const VMStateDescription vmstate_vmxnet3_txq_descr = { >>>> .name = "vmxnet3-txq-descr", >>>> .version_id = 0, >>>> .fields = (VMStateField[]) { >>>> VMSTATE_STRUCT(tx_ring, Vmxnet3TxqDescr, 0, vmstate_vmxnet3_ring, >>>> Vmxnet3Ring), >>>> VMSTATE_STRUCT(comp_ring, Vmxnet3TxqDescr, 0, vmstate_vmxnet3_ring, >>>> Vmxnet3Ring), >>>> VMSTATE_UINT8(intr_idx, Vmxnet3TxqDescr), >>>> VMSTATE_UINT64(tx_stats_pa, Vmxnet3TxqDescr), >>>> VMSTATE_STRUCT(txq_stats, Vmxnet3TxqDescr, 0, >>>> vmstate_vmxnet3_tx_stats, >>>> struct UPT1_TxStats), >>>> VMSTATE_END_OF_LIST() >>>> } >>>> }; >>>> >>>> I will check the '-dump-vmstate' code to see if it's relevant. >>> >>> OK, but that sizeof() should never make it onto the wire; all that >>> teh VMSTATE_STRUCT should do is execute the vmstate_vmxnet3_ring on >>> that data. >>> (And again I'm curious what happened in the version before I committed >>> that change). >>> >>>>> >>>>>> And if I try a migration, I have: >>>>>> >>>>>> qemu/migration/vmstate.c:112: vmstate_load_state: Assertion `first_elem >>>>>> || !n_elems' failed. >>>>> >>>>> Interesting; that's Halil's new assert; I need to look. >>>> >>>> Can't it be related to the size problem? >>> >>> It could, I need to see exactly how it's failing; what was the full >>> command line you used, and with what guest? >> >> Tested with pseries machine type on ppc64le host, no guest started. >> >> qemu-system-ppc64 -M pseries-2.8 -S -serial mon:stdio -vnc :0 -device >> vmxnet3 > > QingFeng Hao's <haoqf@linux.vnet.ibm.com> patch: > 'vmstate: fix failed iotests case 68 and 91' > seems to fix that for me. For me too. Thanks, Laurent
diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c index e13a798..224c109 100644 --- a/hw/net/vmxnet3.c +++ b/hw/net/vmxnet3.c @@ -141,17 +141,17 @@ typedef struct VMXNET3Class { /* Cyclic ring abstraction */ typedef struct { hwaddr pa; - size_t size; - size_t cell_size; - size_t next; + uint32_t size; + uint32_t cell_size; + uint32_t next; uint8_t gen; } Vmxnet3Ring; static inline void vmxnet3_ring_init(PCIDevice *d, Vmxnet3Ring *ring, hwaddr pa, - size_t size, - size_t cell_size, + uint32_t size, + uint32_t cell_size, bool zero_region) { ring->pa = pa; @@ -166,7 +166,7 @@ static inline void vmxnet3_ring_init(PCIDevice *d, } #define VMXNET3_RING_DUMP(macro, ring_name, ridx, r) \ - macro("%s#%d: base %" PRIx64 " size %zu cell_size %zu gen %d next %zu", \ + macro("%s#%d: base %" PRIx64 " size %u cell_size %u gen %d next %u", \ (ring_name), (ridx), \ (r)->pa, (r)->size, (r)->cell_size, (r)->gen, (r)->next)