Re: KVM: x86: switch KVMCLOCK base to monotonic raw clock

[Paolo Bonzini <pbonzini@redhat.com>]

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@redhat.com>
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> 
> 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