Re: [Qemu-ppc] [PATCH] spapr: clock should count only if vm is running

[Paolo Bonzini]

> This is a port to ppc of the i386 commit:
>     00f4d64 kvmclock: clock should count only if vm is running
> 
> We remove timebase_/pre_save/post_load/ functions,
> and use the VM state change handler to save and restore
> the guest_timebase (on stop and continue).
> 
> Time base offset has originally been introduced by commit
>     98a8b52 spapr: Add support for time base offset migration
> 
> So while VM is paused, the time is stopped. This allows to have
> the same result with date (based on Time Base Register) and
> hwclock (based on "get-time-of-day" RTAS call).
> 
> Moreover in TCG mode, the Time Base is always paused, so this
> patch also adjust the behavior between TCG and KVM.
> 
> VM state field "time_of_the_day_ns" is now useless but we keep
> it to be able to migrate to older version of the machine.
> 
> As vmstate_ppc_timebase structure (with timebase_pre_save() and
> timebase_post_load() functions) was only used by vmstate_spapr,
> we register the VM state change handler only in ppc_spapr_init().
> 
> Signed-off-by: Laurent Vivier <address@hidden>

I think you should keep the pre_save handler, otherwise after
migration the timebase register will be off by as long as the
time needed to do the final RAM transfer.  See commit 6053a86
("kvmclock: reduce kvmclock difference on migration", 2016-12-22).

Paolo


> ---
>  hw/ppc/ppc.c         | 76
>  ++++++++++++++++++----------------------------------
>  hw/ppc/spapr.c       |  6 +++++
>  hw/ppc/trace-events  |  3 ---
>  target/ppc/cpu-qom.h |  3 +++
>  4 files changed, 35 insertions(+), 53 deletions(-)
> 
> diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
> index 8945869..a839e25 100644
> --- a/hw/ppc/ppc.c
> +++ b/hw/ppc/ppc.c
> @@ -847,10 +847,11 @@ static void cpu_ppc_set_tb_clk (void *opaque, uint32_t
> freq)
>      cpu_ppc_store_purr(cpu, 0x0000000000000000ULL);
>  }
>  
> -static void timebase_pre_save(void *opaque)
> +#if defined(TARGET_PPC64) && defined(CONFIG_KVM)
> +void cpu_ppc_clock_vm_state_change(void *opaque, int running,
> +                                   RunState state)
>  {
>      PPCTimebase *tb = opaque;
> -    uint64_t ticks = cpu_get_host_ticks();
>      PowerPCCPU *first_ppc_cpu = POWERPC_CPU(first_cpu);
>  
>      if (!first_ppc_cpu->env.tb_env) {
> @@ -858,64 +859,39 @@ static void timebase_pre_save(void *opaque)
>          return;
>      }
>  
> -    tb->time_of_the_day_ns = qemu_clock_get_ns(QEMU_CLOCK_HOST);
> -    /*
> -     * tb_offset is only expected to be changed by migration so
> -     * there is no need to update it from KVM here
> -     */
> -    tb->guest_timebase = ticks + first_ppc_cpu->env.tb_env->tb_offset;
> -}
> +    if (running) {
> +        uint64_t ticks = cpu_get_host_ticks();
> +        CPUState *cpu;
> +        int64_t tb_offset;
>  
> -static int timebase_post_load(void *opaque, int version_id)
> -{
> -    PPCTimebase *tb_remote = opaque;
> -    CPUState *cpu;
> -    PowerPCCPU *first_ppc_cpu = POWERPC_CPU(first_cpu);
> -    int64_t tb_off_adj, tb_off, ns_diff;
> -    int64_t migration_duration_ns, migration_duration_tb, guest_tb, host_ns;
> -    unsigned long freq;
> +        tb_offset = tb->guest_timebase - ticks;
>  
> -    if (!first_ppc_cpu->env.tb_env) {
> -        error_report("No timebase object");
> -        return -1;
> -    }
> -
> -    freq = first_ppc_cpu->env.tb_env->tb_freq;
> -    /*
> -     * Calculate timebase on the destination side of migration.
> -     * The destination timebase must be not less than the source timebase.
> -     * We try to adjust timebase by downtime if host clocks are not
> -     * too much out of sync (1 second for now).
> -     */
> -    host_ns = qemu_clock_get_ns(QEMU_CLOCK_HOST);
> -    ns_diff = MAX(0, host_ns - tb_remote->time_of_the_day_ns);
> -    migration_duration_ns = MIN(NANOSECONDS_PER_SECOND, ns_diff);
> -    migration_duration_tb = muldiv64(freq, migration_duration_ns,
> -                                     NANOSECONDS_PER_SECOND);
> -    guest_tb = tb_remote->guest_timebase + MIN(0, migration_duration_tb);
> -
> -    tb_off_adj = guest_tb - cpu_get_host_ticks();
> -
> -    tb_off = first_ppc_cpu->env.tb_env->tb_offset;
> -    trace_ppc_tb_adjust(tb_off, tb_off_adj, tb_off_adj - tb_off,
> -                        (tb_off_adj - tb_off) / freq);
> -
> -    /* Set new offset to all CPUs */
> -    CPU_FOREACH(cpu) {
> -        PowerPCCPU *pcpu = POWERPC_CPU(cpu);
> -        pcpu->env.tb_env->tb_offset = tb_off_adj;
> +        /* Set new offset to all CPUs */
> +        CPU_FOREACH(cpu) {
> +            PowerPCCPU *pcpu = POWERPC_CPU(cpu);
> +            pcpu->env.tb_env->tb_offset = tb_offset;
> +            kvm_set_one_reg(cpu, KVM_REG_PPC_TB_OFFSET,
> +                            &pcpu->env.tb_env->tb_offset);
> +        }
> +    } else {
> +        uint64_t ticks = cpu_get_host_ticks();
> +
> +        /* not used anymore, we keep it for compatibility */
> +        tb->time_of_the_day_ns = qemu_clock_get_ns(QEMU_CLOCK_HOST);
> +        /*
> +         * tb_offset is only expected to be changed by QEMU so
> +         * there is no need to update it from KVM here
> +         */
> +        tb->guest_timebase = ticks + first_ppc_cpu->env.tb_env->tb_offset;
>      }
> -
> -    return 0;
>  }
> +#endif
>  
>  const VMStateDescription vmstate_ppc_timebase = {
>      .name = "timebase",
>      .version_id = 1,
>      .minimum_version_id = 1,
>      .minimum_version_id_old = 1,
> -    .pre_save = timebase_pre_save,
> -    .post_load = timebase_post_load,
>      .fields      = (VMStateField []) {
>          VMSTATE_UINT64(guest_timebase, PPCTimebase),
>          VMSTATE_INT64(time_of_the_day_ns, PPCTimebase),
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index a642e66..ee78096 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2116,6 +2116,12 @@ static void ppc_spapr_init(MachineState *machine)
>      qemu_register_reset(spapr_ccs_reset_hook, spapr);
>  
>      qemu_register_boot_set(spapr_boot_set, spapr);
> +
> +    /* to stop and start vmclock */
> +    if (kvm_enabled()) {
> +        qemu_add_vm_change_state_handler(cpu_ppc_clock_vm_state_change,
> +                                         &spapr->tb);
> +    }
>  }
>  
>  static int spapr_kvm_type(const char *vm_type)
> diff --git a/hw/ppc/trace-events b/hw/ppc/trace-events
> index 2297ead..4360c23 100644
> --- a/hw/ppc/trace-events
> +++ b/hw/ppc/trace-events
> @@ -68,9 +68,6 @@ spapr_rtas_ibm_configure_connector_missing_fdt(uint32_t
> index) "DRC index: 0x%"P
>  spapr_vio_h_reg_crq(uint64_t reg, uint64_t queue_addr, uint64_t queue_len)
>  "CRQ for dev 0x%" PRIx64 " registered at 0x%" PRIx64 "/0x%" PRIx64
>  spapr_vio_free_crq(uint32_t reg) "CRQ for dev 0x%" PRIx32 " freed"
>  
> -# hw/ppc/ppc.c
> -ppc_tb_adjust(uint64_t offs1, uint64_t offs2, int64_t diff, int64_t seconds)
> "adjusted from 0x%"PRIx64" to 0x%"PRIx64", diff %"PRId64" (%"PRId64"s)"
> -
>  # hw/ppc/prep.c
>  prep_io_800_writeb(uint32_t addr, uint32_t val) "0x%08" PRIx32 " => 0x%02"
>  PRIx32
>  prep_io_800_readb(uint32_t addr, uint32_t retval) "0x%08" PRIx32 " <= 0x%02"
>  PRIx32
> diff --git a/target/ppc/cpu-qom.h b/target/ppc/cpu-qom.h
> index d46c31a..b7977ba 100644
> --- a/target/ppc/cpu-qom.h
> +++ b/target/ppc/cpu-qom.h
> @@ -214,6 +214,9 @@ extern const struct VMStateDescription
> vmstate_ppc_timebase;
>      .flags      = VMS_STRUCT,                                         \
>      .offset     = vmstate_offset_value(_state, _field, PPCTimebase),  \
>  }
> +
> +void cpu_ppc_clock_vm_state_change(void *opaque, int running,
> +                                   RunState state);
>  #endif
>  
>  #endif
> --
> 2.9.3
> 
>