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)
--
2.7.4
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
© 2016 - 2025 Red Hat, Inc.