* Laurent Vivier (lviv...@redhat.com) wrote:
> On 14/03/2017 12:16, Dr. David Alan Gilbert wrote:
> > * Laurent Vivier (lviv...@redhat.com) wrote:
> >> On 06/03/2017 06:25, Jason Wang wrote:
> >>> From: "Dr. David Alan Gilbert" <dgilb...@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 <dgilb...@redhat.com>
> >>> Acked-by: Dmitry Fleytman <dmi...@daynix.com>
> >>> Signed-off-by: Jason Wang <jasow...@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 / dgilb...@redhat.com / Manchester, UK
