[Qemu-devel] [PATCHv2] rtl8139: save/load RxMulOk counter (again)

[Qemu-devel] [PATCHv2] rtl8139: save/load RxMulOk counter (again)

[David Vrabel]

Commit 9d29cdeaaca3a0383af764000b71492c4fc67c6e (rtl8139: port
TallyCounters to vmstate) introduced in incompatibility in the v4
format as it omitted the RxOkMul counter.

There are presumably no users that were impacted by the v4 to v4'
breakage, so increase the save version to 5 and re-add the field,
keeping backward compatibility with v4'.

We can't have a field conditional on the section version in
vmstate_tally_counters since this version checked would not be the
section version (but the version defined in this structure).  So, move
all the fields into the main state structure.

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
Cc: Jason Wang <jasowang@redhat.com>
---
 hw/net/rtl8139.c | 40 ++++++++++++++--------------------------
 1 file changed, 14 insertions(+), 26 deletions(-)

diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
index 562c1fd..8ccd1d3 100644
--- a/hw/net/rtl8139.c
+++ b/hw/net/rtl8139.c
@@ -1352,29 +1352,6 @@ static void RTL8139TallyCounters_dma_write(RTL8139State *s, dma_addr_t tc_addr)
     pci_dma_write(d, tc_addr + 62,    (uint8_t *)&val16, 2);
 }
 
-/* Loads values of tally counters from VM state file */
-
-static const VMStateDescription vmstate_tally_counters = {
-    .name = "tally_counters",
-    .version_id = 1,
-    .minimum_version_id = 1,
-    .fields = (VMStateField[]) {
-        VMSTATE_UINT64(TxOk, RTL8139TallyCounters),
-        VMSTATE_UINT64(RxOk, RTL8139TallyCounters),
-        VMSTATE_UINT64(TxERR, RTL8139TallyCounters),
-        VMSTATE_UINT32(RxERR, RTL8139TallyCounters),
-        VMSTATE_UINT16(MissPkt, RTL8139TallyCounters),
-        VMSTATE_UINT16(FAE, RTL8139TallyCounters),
-        VMSTATE_UINT32(Tx1Col, RTL8139TallyCounters),
-        VMSTATE_UINT32(TxMCol, RTL8139TallyCounters),
-        VMSTATE_UINT64(RxOkPhy, RTL8139TallyCounters),
-        VMSTATE_UINT64(RxOkBrd, RTL8139TallyCounters),
-        VMSTATE_UINT16(TxAbt, RTL8139TallyCounters),
-        VMSTATE_UINT16(TxUndrn, RTL8139TallyCounters),
-        VMSTATE_END_OF_LIST()
-    }
-};
-
 static void rtl8139_ChipCmd_write(RTL8139State *s, uint32_t val)
 {
     DeviceState *d = DEVICE(s);
@@ -3222,7 +3199,7 @@ static void rtl8139_pre_save(void *opaque)
 
 static const VMStateDescription vmstate_rtl8139 = {
     .name = "rtl8139",
-    .version_id = 4,
+    .version_id = 5,
     .minimum_version_id = 3,
     .post_load = rtl8139_post_load,
     .pre_save  = rtl8139_pre_save,
@@ -3293,8 +3270,19 @@ static const VMStateDescription vmstate_rtl8139 = {
         VMSTATE_UINT32(TimerInt, RTL8139State),
         VMSTATE_INT64(TCTR_base, RTL8139State),
 
-        VMSTATE_STRUCT(tally_counters, RTL8139State, 0,
-                       vmstate_tally_counters, RTL8139TallyCounters),
+        VMSTATE_UINT64(tally_counters.TxOk, RTL8139State),
+        VMSTATE_UINT64(tally_counters.RxOk, RTL8139State),
+        VMSTATE_UINT64(tally_counters.TxERR, RTL8139State),
+        VMSTATE_UINT32(tally_counters.RxERR, RTL8139State),
+        VMSTATE_UINT16(tally_counters.MissPkt, RTL8139State),
+        VMSTATE_UINT16(tally_counters.FAE, RTL8139State),
+        VMSTATE_UINT32(tally_counters.Tx1Col, RTL8139State),
+        VMSTATE_UINT32(tally_counters.TxMCol, RTL8139State),
+        VMSTATE_UINT64(tally_counters.RxOkPhy, RTL8139State),
+        VMSTATE_UINT64(tally_counters.RxOkBrd, RTL8139State),
+        VMSTATE_UINT32_V(tally_counters.RxOkMul, RTL8139State, 5),
+        VMSTATE_UINT16(tally_counters.TxAbt, RTL8139State),
+        VMSTATE_UINT16(tally_counters.TxUndrn, RTL8139State),
 
         VMSTATE_UINT32_V(cplus_enabled, RTL8139State, 4),
         VMSTATE_END_OF_LIST()
-- 
2.1.4

Re: [Qemu-devel] [PATCHv2] rtl8139: save/load RxMulOk counter (again)

[Jason Wang]

On 2016年06月21日 01:53, David Vrabel wrote:
> Commit 9d29cdeaaca3a0383af764000b71492c4fc67c6e (rtl8139: port
> TallyCounters to vmstate) introduced in incompatibility in the v4
> format as it omitted the RxOkMul counter.
>
> There are presumably no users that were impacted by the v4 to v4'
> breakage, so increase the save version to 5 and re-add the field,
> keeping backward compatibility with v4'.
>
> We can't have a field conditional on the section version in
> vmstate_tally_counters since this version checked would not be the
> section version (but the version defined in this structure).  So, move
> all the fields into the main state structure.
>
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>

Migration to old version is important for the user and this patch seems 
to break this. How about something like:

- introduce a subsection for RXOKMul
- only migrate it for new version (e.g >= 2.7)

Thanks

> ---
> Cc: Jason Wang <jasowang@redhat.com>
> ---
>   hw/net/rtl8139.c | 40 ++++++++++++++--------------------------
>   1 file changed, 14 insertions(+), 26 deletions(-)
>
> diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
> index 562c1fd..8ccd1d3 100644
> --- a/hw/net/rtl8139.c
> +++ b/hw/net/rtl8139.c
> @@ -1352,29 +1352,6 @@ static void RTL8139TallyCounters_dma_write(RTL8139State *s, dma_addr_t tc_addr)
>       pci_dma_write(d, tc_addr + 62,    (uint8_t *)&val16, 2);
>   }
>   
> -/* Loads values of tally counters from VM state file */
> -
> -static const VMStateDescription vmstate_tally_counters = {
> -    .name = "tally_counters",
> -    .version_id = 1,
> -    .minimum_version_id = 1,
> -    .fields = (VMStateField[]) {
> -        VMSTATE_UINT64(TxOk, RTL8139TallyCounters),
> -        VMSTATE_UINT64(RxOk, RTL8139TallyCounters),
> -        VMSTATE_UINT64(TxERR, RTL8139TallyCounters),
> -        VMSTATE_UINT32(RxERR, RTL8139TallyCounters),
> -        VMSTATE_UINT16(MissPkt, RTL8139TallyCounters),
> -        VMSTATE_UINT16(FAE, RTL8139TallyCounters),
> -        VMSTATE_UINT32(Tx1Col, RTL8139TallyCounters),
> -        VMSTATE_UINT32(TxMCol, RTL8139TallyCounters),
> -        VMSTATE_UINT64(RxOkPhy, RTL8139TallyCounters),
> -        VMSTATE_UINT64(RxOkBrd, RTL8139TallyCounters),
> -        VMSTATE_UINT16(TxAbt, RTL8139TallyCounters),
> -        VMSTATE_UINT16(TxUndrn, RTL8139TallyCounters),
> -        VMSTATE_END_OF_LIST()
> -    }
> -};
> -
>   static void rtl8139_ChipCmd_write(RTL8139State *s, uint32_t val)
>   {
>       DeviceState *d = DEVICE(s);
> @@ -3222,7 +3199,7 @@ static void rtl8139_pre_save(void *opaque)
>   
>   static const VMStateDescription vmstate_rtl8139 = {
>       .name = "rtl8139",
> -    .version_id = 4,
> +    .version_id = 5,
>       .minimum_version_id = 3,
>       .post_load = rtl8139_post_load,
>       .pre_save  = rtl8139_pre_save,
> @@ -3293,8 +3270,19 @@ static const VMStateDescription vmstate_rtl8139 = {
>           VMSTATE_UINT32(TimerInt, RTL8139State),
>           VMSTATE_INT64(TCTR_base, RTL8139State),
>   
> -        VMSTATE_STRUCT(tally_counters, RTL8139State, 0,
> -                       vmstate_tally_counters, RTL8139TallyCounters),
> +        VMSTATE_UINT64(tally_counters.TxOk, RTL8139State),
> +        VMSTATE_UINT64(tally_counters.RxOk, RTL8139State),
> +        VMSTATE_UINT64(tally_counters.TxERR, RTL8139State),
> +        VMSTATE_UINT32(tally_counters.RxERR, RTL8139State),
> +        VMSTATE_UINT16(tally_counters.MissPkt, RTL8139State),
> +        VMSTATE_UINT16(tally_counters.FAE, RTL8139State),
> +        VMSTATE_UINT32(tally_counters.Tx1Col, RTL8139State),
> +        VMSTATE_UINT32(tally_counters.TxMCol, RTL8139State),
> +        VMSTATE_UINT64(tally_counters.RxOkPhy, RTL8139State),
> +        VMSTATE_UINT64(tally_counters.RxOkBrd, RTL8139State),
> +        VMSTATE_UINT32_V(tally_counters.RxOkMul, RTL8139State, 5),
> +        VMSTATE_UINT16(tally_counters.TxAbt, RTL8139State),
> +        VMSTATE_UINT16(tally_counters.TxUndrn, RTL8139State),
>   
>           VMSTATE_UINT32_V(cplus_enabled, RTL8139State, 4),
>           VMSTATE_END_OF_LIST()

Re: [Qemu-devel] [PATCHv2] rtl8139: save/load RxMulOk counter (again)

[Paolo Bonzini]

On 21/06/2016 03:44, Jason Wang wrote:
> 
> 
> On 2016年06月21日 01:53, David Vrabel wrote:
>> Commit 9d29cdeaaca3a0383af764000b71492c4fc67c6e (rtl8139: port
>> TallyCounters to vmstate) introduced in incompatibility in the v4
>> format as it omitted the RxOkMul counter.
>>
>> There are presumably no users that were impacted by the v4 to v4'
>> breakage, so increase the save version to 5 and re-add the field,
>> keeping backward compatibility with v4'.
>>
>> We can't have a field conditional on the section version in
>> vmstate_tally_counters since this version checked would not be the
>> section version (but the version defined in this structure).  So, move
>> all the fields into the main state structure.
>>
>> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> 
> Migration to old version is important for the user and this patch seems
> to break this. How about something like:
> 
> - introduce a subsection for RXOKMul
> - only migrate it for new version (e.g >= 2.7)

Introducing a subsection is not really necessary if the value is going
to be migrated always, and upstream generally does not have "migrate it
only in some version" checks.  This is left for downstreams to implement
if they care.  We just don't have the manpower to ensure that migration
to older versions works between all releases of QEMU.

Paolo

Re: [Qemu-devel] [PATCHv2] rtl8139: save/load RxMulOk counter (again)

[David Vrabel]

On 21/06/16 08:35, Paolo Bonzini wrote:
> 
> 
> On 21/06/2016 03:44, Jason Wang wrote:
>>
>>
>> On 2016年06月21日 01:53, David Vrabel wrote:
>>> Commit 9d29cdeaaca3a0383af764000b71492c4fc67c6e (rtl8139: port
>>> TallyCounters to vmstate) introduced in incompatibility in the v4
>>> format as it omitted the RxOkMul counter.
>>>
>>> There are presumably no users that were impacted by the v4 to v4'
>>> breakage, so increase the save version to 5 and re-add the field,
>>> keeping backward compatibility with v4'.
>>>
>>> We can't have a field conditional on the section version in
>>> vmstate_tally_counters since this version checked would not be the
>>> section version (but the version defined in this structure).  So, move
>>> all the fields into the main state structure.
>>>
>>> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
>>
>> Migration to old version is important for the user and this patch seems
>> to break this. How about something like:
>>
>> - introduce a subsection for RXOKMul
>> - only migrate it for new version (e.g >= 2.7)

I don't see how this can work with snapshots where the QEMU version that
is going to restore the snapshot is not known in advance.

> Introducing a subsection is not really necessary if the value is going
> to be migrated always, and upstream generally does not have "migrate it
> only in some version" checks.  This is left for downstreams to implement
> if they care.  We just don't have the manpower to ensure that migration
> to older versions works between all releases of QEMU.

So is this patch acceptable as is?

David

Re: [Qemu-devel] [PATCHv2] rtl8139: save/load RxMulOk counter (again)

[Paolo Bonzini]

On 21/06/2016 11:36, David Vrabel wrote:
> On 21/06/16 08:35, Paolo Bonzini wrote:
>>
>>
>> On 21/06/2016 03:44, Jason Wang wrote:
>>>
>>>
>>> On 2016年06月21日 01:53, David Vrabel wrote:
>>>> Commit 9d29cdeaaca3a0383af764000b71492c4fc67c6e (rtl8139: port
>>>> TallyCounters to vmstate) introduced in incompatibility in the v4
>>>> format as it omitted the RxOkMul counter.
>>>>
>>>> There are presumably no users that were impacted by the v4 to v4'
>>>> breakage, so increase the save version to 5 and re-add the field,
>>>> keeping backward compatibility with v4'.
>>>>
>>>> We can't have a field conditional on the section version in
>>>> vmstate_tally_counters since this version checked would not be the
>>>> section version (but the version defined in this structure).  So, move
>>>> all the fields into the main state structure.
>>>>
>>>> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
>>>
>>> Migration to old version is important for the user and this patch seems
>>> to break this. How about something like:
>>>
>>> - introduce a subsection for RXOKMul
>>> - only migrate it for new version (e.g >= 2.7)
> 
> I don't see how this can work with snapshots where the QEMU version that
> is going to restore the snapshot is not known in advance.

By "new version" he meant the versioned machine types, e.g.
pc-i440fx-2.6 and older wouldn't migrate it.

>> Introducing a subsection is not really necessary if the value is going
>> to be migrated always, and upstream generally does not have "migrate it
>> only in some version" checks.  This is left for downstreams to implement
>> if they care.  We just don't have the manpower to ensure that migration
>> to older versions works between all releases of QEMU.
> 
> So is this patch acceptable as is?

I think it is.

Paolo

Re: [Qemu-devel] [PATCHv2] rtl8139: save/load RxMulOk counter (again)

[Jason Wang]

On 2016年06月21日 18:11, Paolo Bonzini wrote:
> On 21/06/2016 11:36, David Vrabel wrote:
>> >On 21/06/16 08:35, Paolo Bonzini wrote:
>>> >>
>>> >>
>>> >>On 21/06/2016 03:44, Jason Wang wrote:
>>>> >>>
>>>> >>>
>>>> >>>On 2016年06月21日 01:53, David Vrabel wrote:
>>>>> >>>>Commit 9d29cdeaaca3a0383af764000b71492c4fc67c6e (rtl8139: port
>>>>> >>>>TallyCounters to vmstate) introduced in incompatibility in the v4
>>>>> >>>>format as it omitted the RxOkMul counter.
>>>>> >>>>
>>>>> >>>>There are presumably no users that were impacted by the v4 to v4'
>>>>> >>>>breakage, so increase the save version to 5 and re-add the field,
>>>>> >>>>keeping backward compatibility with v4'.
>>>>> >>>>
>>>>> >>>>We can't have a field conditional on the section version in
>>>>> >>>>vmstate_tally_counters since this version checked would not be the
>>>>> >>>>section version (but the version defined in this structure).  So, move
>>>>> >>>>all the fields into the main state structure.
>>>>> >>>>
>>>>> >>>>Signed-off-by: David Vrabel<david.vrabel@citrix.com>
>>>> >>>
>>>> >>>Migration to old version is important for the user and this patch seems
>>>> >>>to break this. How about something like:
>>>> >>>
>>>> >>>- introduce a subsection for RXOKMul
>>>> >>>- only migrate it for new version (e.g >= 2.7)
>> >
>> >I don't see how this can work with snapshots where the QEMU version that
>> >is going to restore the snapshot is not known in advance.
> By "new version" he meant the versioned machine types, e.g.
> pc-i440fx-2.6 and older wouldn't migrate it.
>
>>> >>Introducing a subsection is not really necessary if the value is going
>>> >>to be migrated always, and upstream generally does not have "migrate it
>>> >>only in some version" checks.  This is left for downstreams to implement
>>> >>if they care.  We just don't have the manpower to ensure that migration
>>> >>to older versions works between all releases of QEMU.
>> >
>> >So is this patch acceptable as is?
> I think it is.
>
> Paolo

Applied to -net.

Thanks