[RFC,v2,10/49] rtl8139: adding new fields to vmstate

This patch adds virtual clock-dependent timers to VMState to allow correct
saving and restoring the state of RTL8139 network controller.

Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
---
 hw/net/rtl8139.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

Il 17/07/2014 13:02, Pavel Dovgalyuk ha scritto:
> This patch adds virtual clock-dependent timers to VMState to allow correct
> saving and restoring the state of RTL8139 network controller.
> 
> Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
> ---
>  hw/net/rtl8139.c |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
> 
> diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
> index 90bc5ec..992caf0 100644
> --- a/hw/net/rtl8139.c
> +++ b/hw/net/rtl8139.c
> @@ -3289,7 +3289,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,
> @@ -3363,6 +3363,9 @@ static const VMStateDescription vmstate_rtl8139 = {
>          VMSTATE_STRUCT(tally_counters, RTL8139State, 0,
>                         vmstate_tally_counters, RTL8139TallyCounters),
>  
> +        VMSTATE_TIMER_V(timer, RTL8139State, 5),

timer need not be migrated, because it is reinstated by rtl8139_post_load.

> +        VMSTATE_INT64_V(TimerExpire, RTL8139State, 5),

This can be in a subsection, migrated only if non-zero.

Paolo

>          VMSTATE_UINT32_V(cplus_enabled, RTL8139State, 4),
>          VMSTATE_END_OF_LIST()
>      },
> 
> 
>

> From: Paolo Bonzini [mailto:paolo.bonzini@gmail.com] On Behalf Of Paolo Bonzini
> Il 17/07/2014 13:02, Pavel Dovgalyuk ha scritto:
> > This patch adds virtual clock-dependent timers to VMState to allow correct
> > saving and restoring the state of RTL8139 network controller.
> >
> > Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
> > ---
> >  hw/net/rtl8139.c |    5 ++++-
> >  1 files changed, 4 insertions(+), 1 deletions(-)
> >
> > diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
> > index 90bc5ec..992caf0 100644
> > --- a/hw/net/rtl8139.c
> > +++ b/hw/net/rtl8139.c
> > @@ -3289,7 +3289,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,
> > @@ -3363,6 +3363,9 @@ static const VMStateDescription vmstate_rtl8139 = {
> >          VMSTATE_STRUCT(tally_counters, RTL8139State, 0,
> >                         vmstate_tally_counters, RTL8139TallyCounters),
> >
> > +        VMSTATE_TIMER_V(timer, RTL8139State, 5),
> 
> timer need not be migrated, because it is reinstated by rtl8139_post_load.
> 

  That's true for normal execution.
  In replay execution mode post_load can be called before cached virtual clock
values are loaded. This may cause invalid setting of the timer and raising
an IRQ which didn't happen in record mode.
  I will update this patch to fix post_load function and avoid this 
non-deterministic behavior.

Pavel Dovgalyuk

Il 28/07/2014 11:54, Pavel Dovgaluk ha scritto:
>>> > > +        VMSTATE_TIMER_V(timer, RTL8139State, 5),
>> > 
>> > timer need not be migrated, because it is reinstated by rtl8139_post_load.
>> > 
>   That's true for normal execution.
>   In replay execution mode post_load can be called before cached virtual clock
> values are loaded. This may cause invalid setting of the timer and raising
> an IRQ which didn't happen in record mode.
>   I will update this patch to fix post_load function and avoid this 
> non-deterministic behavior.

This is what worries me of this series.  These invariants are not
documented anywhere, and people will break them unless you add
assertions that also hold in normal mode.

Paolo

> From: Paolo Bonzini [mailto:paolo.bonzini@gmail.com] On Behalf Of Paolo Bonzini
> Il 28/07/2014 11:54, Pavel Dovgaluk ha scritto:
> >>> > > +        VMSTATE_TIMER_V(timer, RTL8139State, 5),
> >> >
> >> > timer need not be migrated, because it is reinstated by rtl8139_post_load.
> >> >
> >   That's true for normal execution.
> >   In replay execution mode post_load can be called before cached virtual clock
> > values are loaded. This may cause invalid setting of the timer and raising
> > an IRQ which didn't happen in record mode.
> >   I will update this patch to fix post_load function and avoid this
> > non-deterministic behavior.
> 
> This is what worries me of this series.  These invariants are not
> documented anywhere, and people will break them unless you add
> assertions that also hold in normal mode.

Assertions is a good idea, we added such warning message to qemu_get_timedate function
to be sure, that it is used correctly with replay.

Another thing, that could help for making snapshots - find a way to load replay structures
before all other ones. Are there any priorities in migration states list?
Priorities could also solve some other issues, because sometimes post_load function
of one device uses other devices' functions. And the second ones could be not loaded yet.

Pavel Dovgalyuk

Il 30/07/2014 10:24, Pavel Dovgaluk ha scritto:
> Assertions is a good idea, we added such warning message to qemu_get_timedate function
> to be sure, that it is used correctly with replay.
> 
> Another thing, that could help for making snapshots - find a way to load replay structures
> before all other ones. Are there any priorities in migration states list?

No, it is just about vmstate registration order.  In general bus parents
come before bus children for obvious reasons.

Paolo

> Priorities could also solve some other issues, because sometimes post_load function
> of one device uses other devices' functions. And the second ones could be not loaded yet.