[PATCH v2 09/20] KVM: x86: Use gfn_to_pfn_cache for steal time / preempted status

[David Woodhouse]

From: Carsten Stollmaier <stollmc@xxxxxxxxxx>

This largely reverts commit 7e2175ebd695 ("KVM: x86: Fix recording of
guest steal time / preempted status"), which dropped the use of the
gfn_to_pfn_cache because it was not integrated with the MMU notifiers
at the time. That shortcoming has long since been addressed, making
the GPC work correctly for this use case.

Aside from cleaning up the last open-coded assembler access to user
addresses and associated explicit asm exception fixups, moving back
to the now-functional GPC also resolves an issue with contention on
the mmap_lock with userfaultfd. The contention issue is as follows:

On vcpu_run, before entering the guest, the update of the steal time
information causes a page-fault if the page is not present. In our
scenario, this gets handled by do_user_addr_fault() and successively
handle_userfault() because the region is registered to that.

Since handle_userfault() uses TASK_INTERRUPTIBLE, it is interruptible
by signals. But do_user_addr_fault() then busy-retries if the pending
signal is non-fatal, which leads to heavy contention of the mmap_lock.

By restoring the use of GPC for accessing the guest steal time, the
contention is avoided and refreshing the GPC happens when the vCPU is
next scheduled.

Since the gfn_to_pfn_cache gives a kernel mapping rather than a
userspace HVA, accesses are now plain C instead of unsafe_put_user()
et al. READ_ONCE()/WRITE_ONCE() and an smp_wmb() before the final
version increment replace the ordering the old inline assembly
provided implicitly.

kvm_steal_time_set_preempted(), called from the scheduler path, just
takes the atomic SRCU read lock (unconditionally safe there) and
bails if the cache is not currently valid: setting the preempted
flag is best-effort anyway — the old code used
copy_to_user_nofault(), which could equally silently fail — and
kvm_arch_vcpu_load() raises KVM_REQ_STEAL_UPDATE, so the cache is
refreshed from task context before the vCPU next enters the guest.

MSR disable (kvm_steal_time_reset()) leaves last_steal alone: it is
the baseline against the vCPU task's monotonic run_delay, and zeroing
it would dump the task's lifetime run_delay into the guest's steal
counter when the MSR is next enabled (e.g. after INIT-SIPI or kexec).

[dwmw2: Port to the SRCU-protected GPC. The original was written
        against a gpc->lock rwlock and needed a read_trylock() dance
        to be safe from the scheduler path on PREEMPT_RT; the atomic
        SRCU read lock is unconditionally safe there, so all of that
        goes away.]

Signed-off-by: Carsten Stollmaier <stollmc@xxxxxxxxxx>
Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx>
Co-developed-by: David Woodhouse <dwmw@xxxxxxxxxxxx>
Signed-off-by: David Woodhouse <dwmw@xxxxxxxxxxxx>
---
 arch/x86/include/asm/kvm_host.h |   2 +-
 arch/x86/kvm/msrs.c             |   7 +-
 arch/x86/kvm/x86.c              | 140 ++++++++++++++++----------------
 3 files changed, 76 insertions(+), 73 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 1ee4b127004d..4b0c18c77771 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -872,7 +872,7 @@ struct kvm_vcpu_arch {
 		u8 preempted;
 		u64 msr_val;
 		u64 last_steal;
-		struct gfn_to_hva_cache cache;
+		struct gfn_to_pfn_cache cache;
 	} st;
 
 	u64 l1_tsc_offset;
diff --git a/arch/x86/kvm/msrs.c b/arch/x86/kvm/msrs.c
index e91650af44c2..23407f8c441d 100644
--- a/arch/x86/kvm/msrs.c
+++ b/arch/x86/kvm/msrs.c
@@ -1728,8 +1728,13 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 
 		vcpu->arch.st.msr_val = data;
 
-		if (!(data & KVM_MSR_ENABLED))
+		if (!(data & KVM_MSR_ENABLED)) {
+			kvm_gpc_deactivate(&vcpu->arch.st.cache);
 			break;
+		}
+
+		kvm_gpc_activate(&vcpu->arch.st.cache, data & ~KVM_MSR_ENABLED,
+				 sizeof(struct kvm_steal_time));
 
 		kvm_make_request(KVM_REQ_STEAL_UPDATE, vcpu);
 
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index f5f3459ed4a5..321f29789f9f 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2058,12 +2058,11 @@ EXPORT_SYMBOL_FOR_KVM_INTERNAL(kvm_service_local_tlb_flush_requests);
 
 static void record_steal_time(struct kvm_vcpu *vcpu)
 {
-	struct gfn_to_hva_cache *ghc = &vcpu->arch.st.cache;
-	struct kvm_steal_time __user *st;
-	struct kvm_memslots *slots;
-	gpa_t gpa = vcpu->arch.st.msr_val & KVM_STEAL_VALID_BITS;
+	struct gfn_to_pfn_cache *gpc = &vcpu->arch.st.cache;
+	struct kvm_steal_time *st;
 	u64 steal;
 	u32 version;
+	int idx;
 
 	if (kvm_xen_msr_enabled(vcpu->kvm)) {
 		kvm_xen_runstate_set_running(vcpu);
@@ -2076,55 +2075,41 @@ static void record_steal_time(struct kvm_vcpu *vcpu)
 	if (WARN_ON_ONCE(current->mm != vcpu->kvm->mm))
 		return;
 
-	slots = kvm_memslots(vcpu->kvm);
+	/* We rely on the fact that it fits in a single page. */
+	BUILD_BUG_ON((sizeof(*st) - 1) & KVM_STEAL_VALID_BITS);
 
-	if (unlikely(slots->generation != ghc->generation ||
-		     gpa != ghc->gpa ||
-		     kvm_is_error_hva(ghc->hva) || !ghc->memslot)) {
-		/* We rely on the fact that it fits in a single page. */
-		BUILD_BUG_ON((sizeof(*st) - 1) & KVM_STEAL_VALID_BITS);
+	idx = srcu_read_lock_atomic(&vcpu->kvm->gpc_srcu);
+	while (!kvm_gpc_check(gpc, sizeof(*st))) {
+		srcu_read_unlock_atomic(&vcpu->kvm->gpc_srcu, idx);
 
-		if (kvm_gfn_to_hva_cache_init(vcpu->kvm, ghc, gpa, sizeof(*st)) ||
-		    kvm_is_error_hva(ghc->hva) || !ghc->memslot)
+		if (kvm_gpc_refresh(gpc, sizeof(*st)))
 			return;
+
+		idx = srcu_read_lock_atomic(&vcpu->kvm->gpc_srcu);
 	}
 
-	st = (struct kvm_steal_time __user *)ghc->hva;
+	st = gpc->khva;
+
 	/*
 	 * Doing a TLB flush here, on the guest's behalf, can avoid
 	 * expensive IPIs.
 	 */
 	if (guest_pv_has(vcpu, KVM_FEATURE_PV_TLB_FLUSH)) {
-		u8 st_preempted = 0;
-		int err = -EFAULT;
-
-		if (!user_access_begin(st, sizeof(*st)))
-			return;
-
-		asm volatile("1: xchgb %0, %2\n"
-			     "xor %1, %1\n"
-			     "2:\n"
-			     _ASM_EXTABLE_UA(1b, 2b)
-			     : "+q" (st_preempted),
-			       "+&r" (err),
-			       "+m" (st->preempted));
-		if (err)
-			goto out;
-
-		user_access_end();
+		u8 st_preempted = xchg(&st->preempted, 0);
 
 		vcpu->arch.st.preempted = 0;
 
 		trace_kvm_pv_tlb_flush(vcpu->vcpu_id,
 				       st_preempted & KVM_VCPU_FLUSH_TLB);
 		/*
-		 * Request the flush instead of performing it inline. With
-		 * shadow paging kvm_vcpu_flush_tlb_guest() can reschedule
-		 * (via mmu_sync_children()), so it could not be called while
-		 * the user access above was open. Requesting is just a bit
-		 * set on the running vCPU, and the request is serviced
-		 * before the vCPU enters the guest; nothing requires the
-		 * flush to have completed any earlier than that.
+		 * Request the flush instead of performing it inline. This
+		 * runs inside an atomic gpc_srcu read-side section, with
+		 * preemption disabled, and with shadow paging
+		 * kvm_vcpu_flush_tlb_guest() can reschedule (via
+		 * mmu_sync_children()). Requesting is just a bit set on the
+		 * running vCPU, and the request is serviced before the vCPU
+		 * enters the guest; nothing requires the flush to have
+		 * completed any earlier than that.
 		 *
 		 * Note the xchg above must remain as-is: it atomically
 		 * consumes any KVM_VCPU_FLUSH_TLB the guest set, as required
@@ -2133,39 +2118,33 @@ static void record_steal_time(struct kvm_vcpu *vcpu)
 		 */
 		if (st_preempted & KVM_VCPU_FLUSH_TLB)
 			kvm_make_request(KVM_REQ_TLB_FLUSH_GUEST, vcpu);
-
-		if (!user_access_begin(st, sizeof(*st)))
-			goto dirty;
 	} else {
-		if (!user_access_begin(st, sizeof(*st)))
-			return;
-
-		unsafe_put_user(0, &st->preempted, out);
+		WRITE_ONCE(st->preempted, 0);
 		vcpu->arch.st.preempted = 0;
 	}
 
-	unsafe_get_user(version, &st->version, out);
+	version = READ_ONCE(st->version);
 	if (version & 1)
 		version += 1;  /* first time write, random junk */
 
 	version += 1;
-	unsafe_put_user(version, &st->version, out);
+	WRITE_ONCE(st->version, version);
 
 	smp_wmb();
 
-	unsafe_get_user(steal, &st->steal, out);
+	steal = READ_ONCE(st->steal);
 	steal += current->sched_info.run_delay -
 		vcpu->arch.st.last_steal;
 	vcpu->arch.st.last_steal = current->sched_info.run_delay;
-	unsafe_put_user(steal, &st->steal, out);
+	WRITE_ONCE(st->steal, steal);
+
+	smp_wmb();
 
 	version += 1;
-	unsafe_put_user(version, &st->version, out);
+	WRITE_ONCE(st->version, version);
 
- out:
-	user_access_end();
- dirty:
-	mark_page_dirty_in_slot(vcpu->kvm, ghc->memslot, gpa_to_gfn(ghc->gpa));
+	kvm_gpc_mark_dirty_in_slot(gpc);
+	srcu_read_unlock_atomic(&vcpu->kvm->gpc_srcu, idx);
 }
 
 static inline bool kvm_can_mwait_in_guest(void)
@@ -2640,11 +2619,8 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
 
 static void kvm_steal_time_set_preempted(struct kvm_vcpu *vcpu)
 {
-	struct gfn_to_hva_cache *ghc = &vcpu->arch.st.cache;
-	struct kvm_steal_time __user *st;
-	struct kvm_memslots *slots;
-	static const u8 preempted = KVM_VCPU_PREEMPTED;
-	gpa_t gpa = vcpu->arch.st.msr_val & KVM_STEAL_VALID_BITS;
+	struct gfn_to_pfn_cache *gpc = &vcpu->arch.st.cache;
+	struct kvm_steal_time *st;
 
 	/*
 	 * The vCPU can be marked preempted if and only if the VM-Exit was on
@@ -2669,20 +2645,37 @@ static void kvm_steal_time_set_preempted(struct kvm_vcpu *vcpu)
 	if (unlikely(current->mm != vcpu->kvm->mm))
 		return;
 
-	slots = kvm_memslots(vcpu->kvm);
-
-	if (unlikely(slots->generation != ghc->generation ||
-		     gpa != ghc->gpa ||
-		     kvm_is_error_hva(ghc->hva) || !ghc->memslot))
-		return;
-
-	st = (struct kvm_steal_time __user *)ghc->hva;
-	BUILD_BUG_ON(sizeof(st->preempted) != sizeof(preempted));
-
-	if (!copy_to_user_nofault(&st->preempted, &preempted, sizeof(preempted)))
+	/*
+	 * This is called from the scheduler path (via kvm_sched_out), so
+	 * the cache cannot be refreshed here.  Setting 'preempted' is
+	 * best-effort anyway; the old HVA-based code used
+	 * copy_to_user_nofault(), which could also silently fail.  If the
+	 * cache is invalid, it will be refreshed from task context by
+	 * record_steal_time() before the vCPU next enters the guest, since
+	 * kvm_arch_vcpu_load() unconditionally raises KVM_REQ_STEAL_UPDATE.
+	 */
+	guard(srcu_atomic)(&vcpu->kvm->gpc_srcu);
+	if (kvm_gpc_check(gpc, sizeof(*st))) {
+		st = gpc->khva;
+		WRITE_ONCE(st->preempted, KVM_VCPU_PREEMPTED);
 		vcpu->arch.st.preempted = KVM_VCPU_PREEMPTED;
 
-	mark_page_dirty_in_slot(vcpu->kvm, ghc->memslot, gpa_to_gfn(ghc->gpa));
+		kvm_gpc_mark_dirty_in_slot(gpc);
+	}
+}
+
+static void kvm_steal_time_reset(struct kvm_vcpu *vcpu)
+{
+	kvm_gpc_deactivate(&vcpu->arch.st.cache);
+	vcpu->arch.st.preempted = 0;
+	vcpu->arch.st.msr_val = 0;
+	/*
+	 * Leave last_steal alone: it is the baseline against the vCPU
+	 * task's cumulative sched_info.run_delay, which is never reset.
+	 * Zeroing it here would cause the first record_steal_time() after
+	 * a vCPU reset (INIT-SIPI, kexec) to account the task's entire
+	 * lifetime run_delay as steal time in one jump.
+	 */
 }
 
 void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
@@ -9460,6 +9453,8 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
 
 	kvm_gpc_init(&vcpu->arch.pv_time, vcpu->kvm);
 
+	kvm_gpc_init(&vcpu->arch.st.cache, vcpu->kvm);
+
 	if (!irqchip_in_kernel(vcpu->kvm) || kvm_vcpu_is_reset_bsp(vcpu))
 		kvm_set_mp_state(vcpu, KVM_MP_STATE_RUNNABLE);
 	else
@@ -9569,6 +9564,8 @@ void kvm_arch_vcpu_destroy(struct kvm_vcpu *vcpu)
 	kvm_clear_async_pf_completion_queue(vcpu);
 	kvm_mmu_unload(vcpu);
 
+	kvm_steal_time_reset(vcpu);
+
 	kvmclock_reset(vcpu);
 
 	for_each_possible_cpu(cpu)
@@ -9689,7 +9686,8 @@ void kvm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
 	kvm_make_request(KVM_REQ_EVENT, vcpu);
 	vcpu->arch.apf.msr_en_val = 0;
 	vcpu->arch.apf.msr_int_val = 0;
-	vcpu->arch.st.msr_val = 0;
+
+	kvm_steal_time_reset(vcpu);
 
 	kvmclock_reset(vcpu);
 
-- 
2.43.0