| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <377d9706-cc10-dfb8-5326-96c83c47338d@oracle.com>
Date: Tue, 26 Sep 2023 17:29:44 -0700
From: Joe Jin <joe.jin@...cle.com>
To: Dongli Zhang <dongli.zhang@...cle.com>, kvm@...r.kernel.org,
x86@...nel.org
Cc: linux-kernel@...r.kernel.org, seanjc@...gle.com,
pbonzini@...hat.com, tglx@...utronix.de, mingo@...hat.com,
bp@...en8.de, dave.hansen@...ux.intel.com
Subject: Re: [PATCH RFC 1/1] KVM: x86: add param to update master clock
periodically
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@...cle.com>
> Signed-off-by: Dongli Zhang <dongli.zhang@...cle.com>
> ---
> 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);
Powered by blists - more mailing lists