Re: [PATCH] KVM: x86/tsc: Update guest tsc_offset again before vcpu first runs

[Oliver Upton]

On Thu, Jun 29, 2023 at 10:19:38AM -0700, Sean Christopherson wrote:
> +Oliver

It has been a while since I've looked at any x86 code, so forgive any
ignorance :)

> On Fri, Jun 30, 2023, Like Xu wrote:
> > From: Like Xu <likexu@xxxxxxxxxxx>
> > 
> > When a new vcpu is created and subsequently restored by vcpu snapshot,
> > apply kvm_vcpu_write_tsc_offset() before vcpu runs for the first time.
> > 
> > Before a vcpu runs for the first time, the user space (VMM) sets the guest
> > tsc as it wants, which may triggers the time synchronization mechanism with
> > other vcpus (if any). In a scenario where a vcpu snapshot is used to
> > restore, like the bugzilla report [*], the newly target guest tsc (e.g.
> > at the time of vcpu restoration) is synchronized with its the most
> > primitive guest timestamp initialized at the time of vcpu creation.
> > 
> > Furthermore, the VMM can actually update the target guest tsc multiple
> > times before the vcpu actually gets running, which requires the tsc_offset
> > to be updated every time it is set. In this scenario, it can be considered
> > as unstable tsc (even this vcpu has not yet even started ticking to follow
> > the intended logic of KVM timer emulation).
> > 
> > It is only necessary to delay this step until kvm_arch_vcpu_load() to
> > catch up with guest expectation with the help of kvm_vcpu_has_run(),
> > and the change is expected to not break any of the cumbersome existing
> > virt timer features.

The bug description is a bit difficult to grok, IMO. My understanding is
something like the following:

 1) Create VM_0 and save state within 1 second of creation

 2) Create VM_1 and restore state from VM_0

 3) Guest TSCs synchronize with the TSC value resulting from the vCPU
 creation in VM_1 instead of the expected value in the snapshot.

Generalizing -- restoring a vCPU that was saved within a second of its
creation leads to KVM ignoring the user-written TSC value.

Or am I entirely lost?

> > Reported-by: Yong He <alexyonghe@xxxxxxxxxxx>
> > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217423 [*] 
> > Tested-by: Jinrong Liang <cloudliang@xxxxxxxxxxx>
> > Signed-off-by: Like Xu <likexu@xxxxxxxxxxx>
> > ---
> >  arch/x86/kvm/x86.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index 439312e04384..616940fc3791 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -4818,7 +4818,7 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
> >  		if (tsc_delta < 0)
> >  			mark_tsc_unstable("KVM discovered backwards TSC");
> >  
> > -		if (kvm_check_tsc_unstable()) {
> > +		if (kvm_check_tsc_unstable() || !kvm_vcpu_has_run(vcpu)) {
> >  			u64 offset = kvm_compute_l1_tsc_offset(vcpu,
> >  						vcpu->arch.last_guest_tsc);
> >  			kvm_vcpu_write_tsc_offset(vcpu, offset);
> 
> Doing this on every vCPU load feels all kinds of wrong, e.g. it will override the
> value set by userspace via KVM_VCPU_TSC_OFFSET.  One could argue the KVM is "helping"
> userspace by providing a more up-to-date offset for the guest, but "helping"
> userspace by silently overriding userspace rarely ends well.
> 
> Can't we instead just fix the heuristic that tries to detect synchronization?
> 
> ---
>  arch/x86/kvm/x86.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index c30364152fe6..43d40f058a41 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -2721,14 +2721,14 @@ static void kvm_synchronize_tsc(struct kvm_vcpu *vcpu, u64 data)
>  			 * kvm_clock stable after CPU hotplug
>  			 */
>  			synchronizing = true;
> -		} else {
> +		} else if (kvm_vcpu_has_run(vcpu)) {
>  			u64 tsc_exp = kvm->arch.last_tsc_write +
>  						nsec_to_cycles(vcpu, elapsed);
>  			u64 tsc_hz = vcpu->arch.virtual_tsc_khz * 1000LL;
>  			/*
>  			 * Special case: TSC write with a small delta (1 second)
> -			 * of virtual cycle time against real time is
> -			 * interpreted as an attempt to synchronize the CPU.
> +			 * of virtual cycle time against real time on a running
> +			 * vCPU is interpreted as an attempt to synchronize.
>  			 */
>  			synchronizing = data < tsc_exp + tsc_hz &&
>  					data + tsc_hz > tsc_exp;

This would break existing save/restore patterns for the TSC. QEMU relies
on KVM synchronizing the TSCs when restoring a VM, since it cannot
snapshot the TSC values of all the vCPUs in a single instant. It instead
tries to save the TSCs at roughly the same time [*], which KVM detects
on the target and gets everything back in sync. Can't wait to see when
this heuristic actually breaks :)

It's gonna be a hack no matter how we go about fixing this, but the root
of the problem is that KVM-initiated TSC changes are synchronizing with
userpsace-initiated TSC changes. Why not force a new TSC sync generation
(i.e. set @synchronizing to false) for the first user-initiated write to
the TSC MSR?

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 808c292ad3f4..8bb27ad0af53 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1321,6 +1321,7 @@ struct kvm_arch {
 	u64 cur_tsc_offset;
 	u64 cur_tsc_generation;
 	int nr_vcpus_matched_tsc;
+	bool user_changed_tsc;
 
 	u32 default_tsc_khz;
 
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 7713420abab0..1fe24bbc28f4 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2668,7 +2668,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, bool user_initiated)
 {
 	struct kvm *kvm = vcpu->kvm;
 	u64 offset, ns, elapsed;
@@ -2689,20 +2689,29 @@ static void kvm_synchronize_tsc(struct kvm_vcpu *vcpu, u64 data)
 			 * kvm_clock stable after CPU hotplug
 			 */
 			synchronizing = true;
-		} else {
+		} else if (kvm->arch.user_changed_tsc) {
 			u64 tsc_exp = kvm->arch.last_tsc_write +
 						nsec_to_cycles(vcpu, elapsed);
 			u64 tsc_hz = vcpu->arch.virtual_tsc_khz * 1000LL;
 			/*
-			 * Special case: TSC write with a small delta (1 second)
-			 * of virtual cycle time against real time is
-			 * interpreted as an attempt to synchronize the CPU.
+			 * Here lies UAPI baggage: user-initiated TSC write with
+			 * a small delta (1 second) of virtual cycle time
+			 * against real time is interpreted as an attempt to
+			 * synchronize the CPU.
+			 *
+			 * Don't synchronize user changes to the TSC with the
+			 * KVM-initiated change in kvm_arch_vcpu_postcreate()
+			 * by conditioning this mess on userspace having
+			 * written the TSC at least once already.
 			 */
 			synchronizing = data < tsc_exp + tsc_hz &&
 					data + tsc_hz > tsc_exp;
 		}
 	}
 
+	if (user_initiated)
+		kvm->arch.user_changed_tsc = true;
+
 	/*
 	 * For a reliable TSC, we can match TSC offsets, and for an unstable
 	 * TSC, we add elapsed time in this computation.  We could let the
@@ -3695,7 +3704,7 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		break;
 	case MSR_IA32_TSC:
 		if (msr_info->host_initiated) {
-			kvm_synchronize_tsc(vcpu, data);
+			kvm_synchronize_tsc(vcpu, data, true);
 		} else {
 			u64 adj = kvm_compute_l1_tsc_offset(vcpu, data) - vcpu->arch.l1_tsc_offset;
 			adjust_tsc_offset_guest(vcpu, adj);
@@ -11832,7 +11841,7 @@ void kvm_arch_vcpu_postcreate(struct kvm_vcpu *vcpu)
 	if (mutex_lock_killable(&vcpu->mutex))
 		return;
 	vcpu_load(vcpu);
-	kvm_synchronize_tsc(vcpu, 0);
+	kvm_synchronize_tsc(vcpu, 0, false);
 	vcpu_put(vcpu);
 
 	/* poll control enabled by default */