Re: [Qemu-devel] [PULL RESEND 12/19] vmxnet3: Convert ring values to uint32_t's

[Laurent Vivier]

On 14/03/2017 12:38, Dr. David Alan Gilbert wrote:
> * Laurent Vivier (address@hidden) wrote:
>> On 14/03/2017 12:16, Dr. David Alan Gilbert wrote:
>>> * Laurent Vivier (address@hidden) wrote:
>>>> On 06/03/2017 06:25, Jason Wang wrote:
>>>>> From: "Dr. David Alan Gilbert" <address@hidden>
>>>>>
>>>>> 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 <address@hidden>
>>>>> Acked-by: Dmitry Fleytman <address@hidden>
>>>>> Signed-off-by: Jason Wang <address@hidden>
>>>>> ---
>>>>>  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