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
different frequency of CLOCK_MONOTONIC and CLOCK_MONOTONIC_RAW does not
affect it.
Thanks,
Paolo
Paolo
