[03/10] cpu: init vmstate for ticks and clock offset

From: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>

Ticks and clock offset used by CPU timers have to be saved in vmstate.
But vmstate for these fields registered only in icount mode.
Missing registration leads to breaking the continuity when vmstate is loaded.
This patch introduces new initialization function which fixes this.

Signed-off-by: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 cpus.c                | 8 ++++++--
 include/qemu-common.h | 2 ++
 vl.c                  | 1 +
 3 files changed, 9 insertions(+), 2 deletions(-)

Paolo Bonzini <pbonzini@redhat.com> wrote:
> From: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>
>
> Ticks and clock offset used by CPU timers have to be saved in vmstate.
> But vmstate for these fields registered only in icount mode.
> Missing registration leads to breaking the continuity when vmstate is loaded.
> This patch introduces new initialization function which fixes this.
>
> Signed-off-by: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Nack.

This break migration when using with older machine types.

Several questions before I propose an alternative:
- are we this fields always needed (then why migration has worked until
  now)
- after reding cpus.c the question is .... why it ever worked?


After reading cpus.c, it appears that we only really need this
functionality for

qemu_clock_get_ns()

when called with QEMU_CLOCK_VIRTUAL with icount disabled, right?

further grep shows that it is used for acpi_pm_tmr_get_clock, whatever
that is.  When is this used, thanks?

Later, Juan.

> ---
>  cpus.c                | 8 ++++++--
>  include/qemu-common.h | 2 ++
>  vl.c                  | 1 +
>  3 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/cpus.c b/cpus.c
> index 0f7d0ea..2a0e133 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -493,13 +493,17 @@ static const VMStateDescription vmstate_timers = {
>      }
>  };
>  
> +void cpu_ticks_init(void)
> +{
> +    seqlock_init(&timers_state.vm_clock_seqlock, NULL);
> +    vmstate_register(NULL, 0, &vmstate_timers, &timers_state);
> +}
> +
>  void configure_icount(QemuOpts *opts, Error **errp)
>  {
>      const char *option;
>      char *rem_str = NULL;
>  
> -    seqlock_init(&timers_state.vm_clock_seqlock, NULL);
> -    vmstate_register(NULL, 0, &vmstate_timers, &timers_state);
>      option = qemu_opt_get(opts, "shift");
>      if (!option) {
>          if (qemu_opt_get(opts, "align") != NULL) {
> diff --git a/include/qemu-common.h b/include/qemu-common.h
> index bcf7a6a..dcb57ab 100644
> --- a/include/qemu-common.h
> +++ b/include/qemu-common.h
> @@ -105,6 +105,8 @@ static inline char *realpath(const char *path, char *resolved_path)
>  }
>  #endif
>  
> +void cpu_ticks_init(void);
> +
>  /* icount */
>  void configure_icount(QemuOpts *opts, Error **errp);
>  extern int use_icount;
> diff --git a/vl.c b/vl.c
> index 15aea95..5db0d08 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -4334,6 +4334,7 @@ int main(int argc, char **argv, char **envp)
>      qemu_spice_init();
>  #endif
>  
> +    cpu_ticks_init();
>      if (icount_opts) {
>          if (kvm_enabled() || xen_enabled()) {
>              fprintf(stderr, "-icount is not allowed with kvm or xen\n");

Il 09/09/2014 15:25, Juan Quintela ha scritto:
> Nack.
> 
> This break migration when using with older machine types.

The other way round---this was broken recently, the patch fixes it.

Paolo

> Several questions before I propose an alternative:
> - are we this fields always needed (then why migration has worked until
>   now)
> - after reding cpus.c the question is .... why it ever worked?
> 
> 
> After reading cpus.c, it appears that we only really need this
> functionality for
> 
> qemu_clock_get_ns()
> 
> when called with QEMU_CLOCK_VIRTUAL with icount disabled, right?
> 
> further grep shows that it is used for acpi_pm_tmr_get_clock, whatever
> that is.  When is this used, thanks?