On Thu, 3 May 2018 23:20:44 -0500
Michael Roth <mdr...@linux.vnet.ibm.com> wrote:
> In some cases (e.g. spapr) we record guest timebase after qmp_stop()
> via a runstate hook so we can restore it on qmp_cont(). If a migration
> occurs in between those events we end up saving it again, this time
> based on the current timebase the guest would be seeing had it been
> running. This has the effect of advancing the guest timebase while
> it is stopped, which is not what the code intends.
>
Hi Mike,
The current behavior was introduced by:
commit 42043e4f1241eeb77f87f5816b5cf0b6e9583ed7
Author: Laurent Vivier <lviv...@redhat.com>
Date: Fri Jan 27 13:24:58 2017 +0100
spapr: clock should count only if vm is running
and we have this in the changelog:
We keep timebase_pre_save to reduce the clock difference on
migration like in:
6053a86 kvmclock: reduce kvmclock difference on migration
So your patch totally negates ^^ ? Also, I can't see a case where
timebase_save() could be called from vmstate_save_state() while the
VM is running, ie, you could drop timebase_pre_save()... or am I
*probably* missing something ?
> Other than simple jumps in time, this has been seen to trigger what
> appear to be RCU-related crashes in recent kernels when the advance
> exceeds rcu_cpu_stall_timeout, and it can be triggered by fairly
> common operations such as `virsh migrate ... --timeout 60`.
>
> Cc: Alexey Kardashevskiy <a...@ozlabs.ru>
> Cc: David Gibson <da...@gibson.dropbear.id.au>
> Cc: Laurent Vivier <lviv...@redhat.com>
> Cc: qemu-...@nongnu.org
> Cc: qemu-sta...@nongnu.org
> Signed-off-by: Michael Roth <mdr...@linux.vnet.ibm.com>
> ---
> hw/ppc/ppc.c | 12 ++++++++++++
> target/ppc/cpu-qom.h | 1 +
> 2 files changed, 13 insertions(+)
>
> diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
> index ec4be25f49..ff0a107864 100644
> --- a/hw/ppc/ppc.c
> +++ b/hw/ppc/ppc.c
> @@ -865,6 +865,15 @@ static void timebase_save(PPCTimebase *tb)
> uint64_t ticks = cpu_get_host_ticks();
> PowerPCCPU *first_ppc_cpu = POWERPC_CPU(first_cpu);
>
> + /* since we generally save timebase just after the guest
> + * has stopped, avoid trying to save it again since we will
> + * end up advancing it by the amount of ticks that have
> + * elapsed in the host since the initial save
> + */
> + if (tb->saved) {
> + return;
> + }
> +
> if (!first_ppc_cpu->env.tb_env) {
> error_report("No timebase object");
> return;
> @@ -877,6 +886,7 @@ static void timebase_save(PPCTimebase *tb)
> * there is no need to update it from KVM here
> */
> tb->guest_timebase = ticks + first_ppc_cpu->env.tb_env->tb_offset;
> + tb->saved = true;
> }
>
> static void timebase_load(PPCTimebase *tb)
> @@ -908,6 +918,8 @@ static void timebase_load(PPCTimebase *tb)
> &pcpu->env.tb_env->tb_offset);
> #endif
> }
> +
> + tb->saved = false;
> }
>
> void cpu_ppc_clock_vm_state_change(void *opaque, int running,
> diff --git a/target/ppc/cpu-qom.h b/target/ppc/cpu-qom.h
> index deaa46a14b..ec2dbcdcae 100644
> --- a/target/ppc/cpu-qom.h
> +++ b/target/ppc/cpu-qom.h
> @@ -210,6 +210,7 @@ typedef struct PowerPCCPUClass {
> typedef struct PPCTimebase {
> uint64_t guest_timebase;
> int64_t time_of_the_day_ns;
> + bool saved;
> } PPCTimebase;
>
> extern const struct VMStateDescription vmstate_ppc_timebase;
