On 9/26/23 4:06 PM, Dongli Zhang wrote: > This is to minimize the kvmclock drift during CPU hotplug (or when the > master clock and pvclock_vcpu_time_info are updated). The drift is > because kvmclock and raw monotonic (tsc) use different > equation/mult/shift to calculate that how many nanoseconds (given the tsc > as input) has passed. > > The calculation of the kvmclock is based on the pvclock_vcpu_time_info > provided by the host side. > > struct pvclock_vcpu_time_info { > u32 version; > u32 pad0; > u64 tsc_timestamp; --> by host raw monotonic > u64 system_time; --> by host raw monotonic > u32 tsc_to_system_mul; --> by host KVM > s8 tsc_shift; --> by host KVM > u8 flags; > u8 pad[2]; > } __attribute__((__packed__)); > > To calculate the current guest kvmclock: > > 1. Obtain the tsc = rdtsc() of guest. > > 2. If shift < 0: > tmp = tsc >> tsc_shift > if shift > 0: > tmp = tsc << tsc_shift > > 3. The kvmclock value will be: (tmp * tsc_to_system_mul) >> 32 > > Therefore, the current kvmclock will be either: > > (rdtsc() >> tsc_shift) * tsc_to_system_mul >> 32 > > ... or ... > > (rdtsc() << tsc_shift) * tsc_to_system_mul >> 32 > > The 'tsc_to_system_mul' and 'tsc_shift' are calculated by the host KVM. > > When the master clock is actively used, the 'tsc_timestamp' and > 'system_time' are derived from the host raw monotonic time, which is > calculated based on the 'mult' and 'shift' of clocksource_tsc: > > elapsed_time = (tsc * mult) >> shift > > Since kvmclock and raw monotonic (clocksource_tsc) use different > equation/mult/shift to convert the tsc to nanosecond, there may be clock > drift issue during CPU hotplug (when the master clock is updated). > > 1. The guest boots and all vcpus have the same 'pvclock_vcpu_time_info' > (suppose the master clock is used). > > 2. Since the master clock is never updated, the periodic kvmclock_sync_work > does not update the values in 'pvclock_vcpu_time_info'. > > 3. Suppose a very long period has passed (e.g., 30-day). > > 4. The user adds another vcpu. Both master clock and > 'pvclock_vcpu_time_info' are updated, based on the raw monotonic. > > (Ideally, we expect the update is based on 'tsc_to_system_mul' and > 'tsc_shift' from kvmclock). > > > Because kvmclock and raw monotonic (clocksource_tsc) use different > equation/mult/shift to convert the tsc to nanosecond, there will be drift > between: > > (1) kvmclock based on current rdtsc and old 'pvclock_vcpu_time_info' > (2) kvmclock based on current rdtsc and new 'pvclock_vcpu_time_info' > > According to the test, there is a drift of 4502145ns between (1) and (2) > after about 40 hours. The longer the time, the large the drift. > > This is to add a module param to allow the user to configure for how often > to refresh the master clock, in order to reduce the kvmclock drift based on > user requirement (e.g., every 5-min to every day). The more often that the > master clock is refreshed, the smaller the kvmclock drift during the vcpu > hotplug. > > Cc: Joe Jin <joe.jin@xxxxxxxxxx> > Signed-off-by: Dongli Zhang <dongli.zhang@xxxxxxxxxx> > --- > Other options are to update the masterclock in: > - kvmclock_sync_work, or > - pvclock_gtod_notify() > > arch/x86/include/asm/kvm_host.h | 1 + > arch/x86/kvm/x86.c | 34 +++++++++++++++++++++++++++++++++ > 2 files changed, 35 insertions(+) > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > index 17715cb8731d..57409dce5d73 100644 > --- a/arch/x86/include/asm/kvm_host.h > +++ b/arch/x86/include/asm/kvm_host.h > @@ -1331,6 +1331,7 @@ struct kvm_arch { > u64 master_cycle_now; > struct delayed_work kvmclock_update_work; > struct delayed_work kvmclock_sync_work; > + struct delayed_work masterclock_sync_work; > > struct kvm_xen_hvm_config xen_hvm_config; > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 9f18b06bbda6..0b71dc3785eb 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -157,6 +157,9 @@ module_param(min_timer_period_us, uint, S_IRUGO | S_IWUSR); > static bool __read_mostly kvmclock_periodic_sync = true; > module_param(kvmclock_periodic_sync, bool, S_IRUGO); > > +unsigned int __read_mostly masterclock_sync_period; > +module_param(masterclock_sync_period, uint, 0444); Can the mode be 0644 and allow it be changed at runtime? Thanks, Joe > + > /* tsc tolerance in parts per million - default to 1/2 of the NTP threshold */ > static u32 __read_mostly tsc_tolerance_ppm = 250; > module_param(tsc_tolerance_ppm, uint, S_IRUGO | S_IWUSR); > @@ -3298,6 +3301,31 @@ static void kvmclock_sync_fn(struct work_struct *work) > KVMCLOCK_SYNC_PERIOD); > } > > +static void masterclock_sync_fn(struct work_struct *work) > +{ > + unsigned long i; > + struct delayed_work *dwork = to_delayed_work(work); > + struct kvm_arch *ka = container_of(dwork, struct kvm_arch, > + masterclock_sync_work); > + struct kvm *kvm = container_of(ka, struct kvm, arch); > + struct kvm_vcpu *vcpu; > + > + if (!masterclock_sync_period) > + return; > + > + kvm_for_each_vcpu(i, vcpu, kvm) { > + /* > + * It is not required to kick the vcpu because it is not > + * expected to update the master clock immediately. > + */ > + kvm_make_request(KVM_REQ_MASTERCLOCK_UPDATE, vcpu); > + } > + > + schedule_delayed_work(&ka->masterclock_sync_work, > + masterclock_sync_period * HZ); > +} > + > + > /* These helpers are safe iff @msr is known to be an MCx bank MSR. */ > static bool is_mci_control_msr(u32 msr) > { > @@ -11970,6 +11998,10 @@ void kvm_arch_vcpu_postcreate(struct kvm_vcpu *vcpu) > if (kvmclock_periodic_sync && vcpu->vcpu_idx == 0) > schedule_delayed_work(&kvm->arch.kvmclock_sync_work, > KVMCLOCK_SYNC_PERIOD); > + > + if (masterclock_sync_period && vcpu->vcpu_idx == 0) > + schedule_delayed_work(&kvm->arch.masterclock_sync_work, > + masterclock_sync_period * HZ); > } > > void kvm_arch_vcpu_destroy(struct kvm_vcpu *vcpu) > @@ -12344,6 +12376,7 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type) > > INIT_DELAYED_WORK(&kvm->arch.kvmclock_update_work, kvmclock_update_fn); > INIT_DELAYED_WORK(&kvm->arch.kvmclock_sync_work, kvmclock_sync_fn); > + INIT_DELAYED_WORK(&kvm->arch.masterclock_sync_work, masterclock_sync_fn); > > kvm_apicv_init(kvm); > kvm_hv_init_vm(kvm); > @@ -12383,6 +12416,7 @@ static void kvm_unload_vcpu_mmus(struct kvm *kvm) > > void kvm_arch_sync_events(struct kvm *kvm) > { > + cancel_delayed_work_sync(&kvm->arch.masterclock_sync_work); > cancel_delayed_work_sync(&kvm->arch.kvmclock_sync_work); > cancel_delayed_work_sync(&kvm->arch.kvmclock_update_work); > kvm_free_pit(kvm);