On Fri, Nov 01, 2019 at 01:09:50AM +0100, Paolo Bonzini wrote:
> On 28/10/19 15:36, Marcelo Tosatti wrote:
> >
> > Commit 0bc48bea36d1 ("KVM: x86: update master clock before computing
> > kvmclock_offset")
> > switches the order of operations to avoid the conversion
> >
> > TSC (without frequency correction) ->
> > system_timestamp (with frequency correction),
> >
> > which might cause a time jump.
> >
> > However, it leaves any other masterclock update unsafe, which includes,
> > at the moment:
> >
> > * HV_X64_MSR_REFERENCE_TSC MSR write.
> > * TSC writes.
> > * Host suspend/resume.
> >
> > Avoid the time jump issue by using frequency uncorrected
> > CLOCK_MONOTONIC_RAW clock.
> >
> > Its the guests time keeping software responsability
> > to track and correct a reference clock such as UTC.
> >
> > This fixes forward time jump (which can result in
> > failure to bring up a vCPU) during vCPU hotplug:
> >
> > Oct 11 14:48:33 storage kernel: CPU2 has been hot-added
> > Oct 11 14:48:34 storage kernel: CPU3 has been hot-added
> > Oct 11 14:49:22 storage kernel: smpboot: Booting Node 0 Processor 2 APIC 0x2 <-- time jump of almost 1 minute
> > Oct 11 14:49:22 storage kernel: smpboot: do_boot_cpu failed(-1) to wakeup CPU#2
> > Oct 11 14:49:23 storage kernel: smpboot: Booting Node 0 Processor 3 APIC 0x3
> > Oct 11 14:49:23 storage kernel: kvm-clock: cpu 3, msr 0:7ff640c1, secondary cpu clock
> >
> > Which happens because:
> >
> > /*
> > * Wait 10s total for a response from AP
> > */
> > boot_error = -1;
> > timeout = jiffies + 10*HZ;
> > while (time_before(jiffies, timeout)) {
> > ...
> > }
> >
> > Analyzed-by: Igor Mammedov <imammedo@xxxxxxxxxx>
> > Signed-off-by: Marcelo Tosatti <mtosatti@xxxxxxxxxx>
> >
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index 661e2bf..ff713a1 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -1521,20 +1521,25 @@ static int do_set_msr(struct kvm_vcpu *vcpu, unsigned index, u64 *data)
> > }
> >
> > #ifdef CONFIG_X86_64
> > +struct pvclock_clock {
> > + int vclock_mode;
> > + u64 cycle_last;
> > + u64 mask;
> > + u32 mult;
> > + u32 shift;
> > +};
> > +
> > struct pvclock_gtod_data {
> > seqcount_t seq;
> >
> > - struct { /* extract of a clocksource struct */
> > - int vclock_mode;
> > - u64 cycle_last;
> > - u64 mask;
> > - u32 mult;
> > - u32 shift;
> > - } clock;
> > + struct pvclock_clock clock; /* extract of a clocksource struct */
> > + struct pvclock_clock raw_clock; /* extract of a clocksource struct */
> >
> > + u64 boot_ns_raw;
> > u64 boot_ns;
> > u64 nsec_base;
> > u64 wall_time_sec;
> > + u64 monotonic_raw_nsec;
> > };
> >
> > static struct pvclock_gtod_data pvclock_gtod_data;
> > @@ -1542,10 +1547,20 @@ struct pvclock_gtod_data {
> > static void update_pvclock_gtod(struct timekeeper *tk)
> > {
> > struct pvclock_gtod_data *vdata = &pvclock_gtod_data;
> > - u64 boot_ns;
> > + u64 boot_ns, boot_ns_raw;
> >
> > boot_ns = ktime_to_ns(ktime_add(tk->tkr_mono.base, tk->offs_boot));
> >
> > + /*
> > + * FIXME: tk->offs_boot should be converted to CLOCK_MONOTONIC_RAW
> > + * interval (that is, without frequency adjustment for that interval).
> > + *
> > + * Lack of this fix can cause system_timestamp to not be equal to
> > + * CLOCK_MONOTONIC_RAW (which happen if the host uses
> > + * suspend/resume).
> > + */
>
> This is scary. Essentially you're saying that you'd want a
> CLOCK_BOOTTIME_RAW. But is this true? CLOCK_BOOTTIME only differs by
> the suspend time, and that is computed directly in nanoseconds so the
Its read from the RTC.
> different frequency of CLOCK_MONOTONIC and CLOCK_MONOTONIC_RAW does not
> affect it.
Still different frequency from RTC and TSC, which can cause
system_timestamp to not equal CLOCK_MONOTONIC_RAW (but in fact i don't
see a fix for that, and the hosts clock also suffers from the same
issue).
Should i remove the fixme ? Or just add a note about this fact
of suspend/resume ?
