Re: [PATCH 1/2] KVM: x86: add KVM clock time reference arg to kvm_write_tsc()

[Sean Christopherson <seanjc@google.com>]

On Thu, Jul 07, 2022, Simon Veith wrote:
> The Time Stamp Counter (TSC) value register can be set to an absolute
> value using the KVM_SET_MSRS ioctl, which calls kvm_synchronize_tsc()
> internally.
> 
> Since this is a per-vCPU register, and vCPUs are often managed by
> separate threads, setting a uniform TSC value across all vCPUs is
> challenging: After live migration, for example, the TSC value may need
> to be adjusted to account for the migration downtime. Ideally, we would
> want each vCPU to be adjusted by the same offset; but if we compute the
> offset centrally, the TSC value may become out of date due to scheduling
> delays by the time that each vCPU thread gets around to issuing
> KVM_SET_MSRS.
> 
> In preparation for the next patch, this change adds an optional, KVM
> clock based time reference argument to kvm_synchronize_tsc(). This
> argument, if present, is understood to mean "the TSC value being written
> was valid at this corresponding KVM clock time point".


Given that commit 828ca89628bf ("KVM: x86: Expose TSC offset controls to userspace")
was merged less than a year ago, it would be helpful to explicitly call out why
KVM_VCPU_TSC_CTRL doesn't work, and why that sub-ioctl can't be extended/modified to
make it work.

> kvm_synchronize_tsc() will then use this clock reference to adjust the
> TSC value being written for any delays that have been incurred since the
> provided TSC value was valid.
> 
> Co-developed-by: David Woodhouse <dwmw@amazon.co.uk>

Needs David's SOB.  And this patch should be squashed with the next patch.  The
kvm_ns != NULL path is dead code here, e.g. even if the logic is wildly broken,
bisection will blame the patch that actually passes a non-null reference.  Neither
patch is big enough to warrant splitting in this way.

And more importantly, the next patch's changelog provides a thorough description
of what it's doing, but there's very little in there that describes _why_ KVM
wants to provide this functionality.

> Signed-off-by: Simon Veith <sveith@amazon.de>
> ---
>  arch/x86/kvm/x86.c | 22 +++++++++++++++++-----
>  1 file changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 1910e1e78b15..a44d083f1bf9 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -2629,7 +2629,7 @@ static void __kvm_synchronize_tsc(struct kvm_vcpu *vcpu, u64 offset, u64 tsc,
>  	kvm_track_tsc_matching(vcpu);
>  }
>  
> -static void kvm_synchronize_tsc(struct kvm_vcpu *vcpu, u64 data)
> +static void kvm_synchronize_tsc(struct kvm_vcpu *vcpu, u64 data, u64 *kvm_ns)
>  {
>  	struct kvm *kvm = vcpu->kvm;
>  	u64 offset, ns, elapsed;
> @@ -2638,12 +2638,24 @@ static void kvm_synchronize_tsc(struct kvm_vcpu *vcpu, u64 data)
>  	bool synchronizing = false;
>  
>  	raw_spin_lock_irqsave(&kvm->arch.tsc_write_lock, flags);
> -	offset = kvm_compute_l1_tsc_offset(vcpu, data);
>  	ns = get_kvmclock_base_ns();
> +
> +	if (kvm_ns) {
> +		/*
> +		 * We have been provided a KVM clock reference time point at

Avoid pronouns, e.g. there's no need to say "We have been provided", just state
what kvm_ns is and how it's used.

> +		 * which this TSC value was correct.
> +		 * Use this time point to compensate for any delays that were
> +		 * incurred since that TSC value was valid.
> +		 */
> +		s64 delta_ns = ns + vcpu->kvm->arch.kvmclock_offset - *kvm_ns;
> +		data += nsec_to_cycles(vcpu, (u64)delta_ns);
> +	}