[PATCH v2 13/20] KVM: x86: Post KVM_REQ_GET_NESTED_STATE_PAGES on memslot updates

[David Woodhouse]

From: David Woodhouse <dwmw@xxxxxxxxxxxx>

The physical addresses which nested page setup latches into hardware
control structures (vmcs02's APIC-access, virtual-APIC and posted
interrupt descriptor addresses) are derived from gPA→uHVA
translations which a memslot update can change. Software readers of
the underlying gfn_to_pfn_caches catch that lazily, via the memslot
generation check at their next use — but the CPU's use of a latched
address from guest mode is continuous and checks nothing. A vCPU
running L2 across a memslot move keeps using the old translation
until a nested VM-exit happens to re-run nested page setup. (A
staleness problem, not a lifetime one: freeing the page is the
mmu_notifier's business. The replaced kvm_host_map code had the same
staleness with no remedy at all.)

Post the request from the memslot update itself — the single point
where the generation changes, and a slow path by definition — rather
than checking generations in the VM-entry path forever. The request
is what survives racing with a concurrent VM-entry (a bare kick
landing before vcpu->mode is set would be lost), and a vCPU which
resolved its pages against the old memslots but has not yet entered
guest mode is invisible to any is_guest_mode() filter, so it is
posted unconditionally to every vCPU. TD vCPUs share vmx_nested_ops
but are not struct vcpu_vmx; the VMX handler bails for them up
front.

The posting site is kvm_arch_commit_memory_region(), once per
user-visible update with the final memslots in place — not
kvm_arch_memslots_updated(), which also runs when a DELETE or MOVE
installs its transient KVM_MEMSLOT_INVALID placeholder, where the
handler could only retry, and skipping FLAGS_ONLY updates, which
cannot change any translation. kvm_arch_memslots_updated() keeps a
bare kick for the steal_time cache's check-at-use sites.

The handlers distinguish the request's origin, extending VMX's
nested.resume_pending to nSVM. A resume into L2 from saved state
(KVM_SET_NESTED_STATE after migration, or RSM) re-derives PAE PDPTRs
and re-runs the MSRPM merge; a failure there is a userspace error
(or, in a narrow window, a concurrent memslot update's transient
KVM_MEMSLOT_INVALID placeholder), reported as KVM_EXIT_INTERNAL_ERROR
with the request re-posted and resume_pending kept, so a further
KVM_RUN bounces back out rather than entering L2 with the restore
work undone — and so a userspace which retries recovers once a
transient failure clears. A memslot-posted request re-latches the vmcs02 addresses on
VMX and is a no-op on SVM, which latches no L1-owned physical
address in hardware (the merged MSRPM and IOPM are kernel pages);
neither vendor re-derives PDPTRs for it, as those latch only at MOV
CR3 and VM-entry. The classification dies with the request: nested
VM-exit clears resume_pending on both vendors, so a request posted
after L1 re-enters L2 cannot inherit entry-time semantics.
Accordingly, downgrade the WARN in svm_get_nested_state_pages(): a
request with nothing to do is now expected.

The memslot-move mode of the vmx_apic_update_test selftest, added
later in this series, exercises this path.

Signed-off-by: David Woodhouse <dwmw@xxxxxxxxxxxx>
---
 arch/x86/kvm/svm/nested.c | 45 ++++++++++++++++++++++++++++++++++-----
 arch/x86/kvm/svm/svm.h    | 12 +++++++++++
 arch/x86/kvm/vmx/nested.c | 12 +++++++++--
 arch/x86/kvm/x86.c        | 44 +++++++++++++++++++++++++++++++++++++-
 4 files changed, 105 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index af0ef678433d..ab187abc11b5 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -1078,8 +1078,10 @@ int enter_svm_guest_mode(struct kvm_vcpu *vcpu, u64 vmcb12_gpa, bool from_vmrun)
 	if (ret)
 		return ret;
 
-	if (!from_vmrun)
+	if (!from_vmrun) {
+		svm->nested.resume_pending = true;
 		kvm_make_request(KVM_REQ_GET_NESTED_STATE_PAGES, vcpu);
+	}
 
 	svm_set_gif(svm, true);
 
@@ -1340,6 +1342,7 @@ void nested_svm_vmexit(struct vcpu_svm *svm)
 	kvm_warn_on_nested_run_pending(vcpu);
 
 	kvm_clear_request(KVM_REQ_GET_NESTED_STATE_PAGES, vcpu);
+	svm->nested.resume_pending = false;
 
 	/* in case we halted in L2 */
 	kvm_set_mp_state(vcpu, KVM_MP_STATE_RUNNABLE);
@@ -1568,6 +1571,7 @@ void svm_leave_nested(struct kvm_vcpu *vcpu)
 	}
 
 	kvm_clear_request(KVM_REQ_GET_NESTED_STATE_PAGES, vcpu);
+	svm->nested.resume_pending = false;
 }
 
 static int nested_svm_exit_handled_msr(struct vcpu_svm *svm)
@@ -2113,6 +2117,7 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu,
 	if (kvm_vcpu_apicv_active(vcpu))
 		kvm_make_request(KVM_REQ_APICV_UPDATE, vcpu);
 
+	svm->nested.resume_pending = true;
 	kvm_make_request(KVM_REQ_GET_NESTED_STATE_PAGES, vcpu);
 out_free:
 	kfree(save);
@@ -2123,7 +2128,21 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu,
 
 static bool svm_get_nested_state_pages(struct kvm_vcpu *vcpu)
 {
-	if (WARN_ON(!is_guest_mode(vcpu)))
+	struct vcpu_svm *svm = to_svm(vcpu);
+
+	/*
+	 * Memslot updates post this request to every vCPU (to make any
+	 * vCPU which has guest pages latched in hardware re-resolve them
+	 * against the new memslots), so it can arrive with nothing to
+	 * do: outside guest mode, or (unlike VMX) even with L2
+	 * running, because SVM latches no L1-owned physical address in
+	 * hardware (the merged MSRPM and the IOPM are kernel pages).
+	 * The restore work below (PDPTR re-derivation, MSRPM merge)
+	 * runs only when the request came from a migration restore;
+	 * running it for a memslot update would re-latch a live L2's
+	 * PDPTRs at a moment the architecture does not allow.
+	 */
+	if (!is_guest_mode(vcpu) || !svm->nested.resume_pending)
 		return true;
 
 	if (is_pae_paging(vcpu)) {
@@ -2139,16 +2158,32 @@ static bool svm_get_nested_state_pages(struct kvm_vcpu *vcpu)
 			kvm_register_mark_for_reload(vcpu, VCPU_REG_PDPTR);
 		else if (!vcpu->arch.pdptrs_from_userspace &&
 			 CC(!load_pdptrs(vcpu, vcpu->arch.cr3)))
-			return false;
+			goto internal_error;
 	}
 
 	if (!nested_svm_merge_msrpm(vcpu))
-		return false;
+		goto internal_error;
 
 	if (kvm_hv_verify_vp_assist(vcpu))
-		return false;
+		goto internal_error;
 
+	/* Success consumes the resume classification. */
+	svm->nested.resume_pending = false;
 	return true;
+
+internal_error:
+	/*
+	 * Restored state names an unusable page. (In a narrow window, a
+	 * concurrent memslot update's KVM_MEMSLOT_INVALID placeholder
+	 * lands here too; a userspace which retries KVM_RUN recovers
+	 * once the update commits.) The caller fills in the
+	 * KVM_EXIT_INTERNAL_ERROR exit and re-posts the request; what is
+	 * SVM's to keep is resume_pending, still set, so that no
+	 * subsequent KVM_RUN can enter L2 with the restore work undone:
+	 * the vCPU bounces back here (and to userspace) until the
+	 * failure is resolved or the vCPU state is replaced.
+	 */
+	return false;
 }
 
 static gpa_t svm_translate_nested_gpa(struct kvm_vcpu *vcpu, gpa_t gpa,
diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index 6d0242966dc8..1b8f3f2057d9 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -236,6 +236,18 @@ struct svm_nested_state {
 
 	bool initialized;
 
+	/*
+	 * The pending KVM_REQ_GET_NESTED_STATE_PAGES was posted by a
+	 * migration restore (KVM_SET_NESTED_STATE or RSM back into L2),
+	 * where PDPTRs must be re-derived and the MSRPM merge re-run
+	 * before re-entering L2, and a failure is a userspace error to
+	 * report. Unlike VMX, SVM latches no L1-owned physical address
+	 * in hardware (the merged MSRPM and the IOPM are kernel pages),
+	 * so a request posted by a memslot update finds nothing to
+	 * re-latch and the handler no-ops on !resume_pending.
+	 */
+	bool resume_pending;
+
 	/*
 	 * Indicates whether MSR bitmap for L2 needs to be rebuilt due to
 	 * changes in MSR bitmap for L1 or switching to a different L2. Note,
diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 23f1fff97917..6c8004009bcd 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -3846,6 +3846,14 @@ static bool __vmx_get_nested_state_pages(struct kvm_vcpu *vcpu)
 
 static bool vmx_get_nested_state_pages(struct kvm_vcpu *vcpu)
 {
+	/*
+	 * Memslot updates post this request to every vCPU, including TD
+	 * vCPUs, which are not struct vcpu_vmx and have no nested state
+	 * to re-resolve.
+	 */
+	if (is_td_vcpu(vcpu))
+		return true;
+
 	if (!__vmx_get_nested_state_pages(vcpu)) {
 		/*
 		 * The caller (vcpu_enter_guest) consumed
@@ -5501,8 +5509,8 @@ void __nested_vmx_vmexit(struct kvm_vcpu *vcpu, u32 vm_exit_reason,
 
 	/*
 	 * The resume classification dies with the request: a stale
-	 * resume_pending would give the next request (e.g. posted by an
-	 * invalidation after L1 re-enters L2) entry-time semantics,
+	 * resume_pending would give the next request (e.g. posted by a
+	 * memslot update after L1 re-enters L2) entry-time semantics,
 	 * re-deriving PDPTRs mid-L2 and treating a transiently
 	 * unbackable page as a terminal error.
 	 */
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 321f29789f9f..892c6f81d704 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -10282,7 +10282,16 @@ void kvm_arch_memslots_updated(struct kvm *kvm, u64 gen)
 	 */
 	kvm_mmu_invalidate_mmio_sptes(kvm, gen);
 
-	/* Force re-initialization of steal_time cache */
+	/*
+	 * Force re-initialization of the steal_time cache: its
+	 * check-at-use sites only see the new generation once the vCPU
+	 * goes around its run loop. This runs on both memslot swaps of a
+	 * DELETE/MOVE; the re-resolution of latched nested-state pages
+	 * is posted from kvm_arch_commit_memory_region() instead, once
+	 * per update and only when the final memslots are in place, so
+	 * that its handler never chases an in-flight update's
+	 * KVM_MEMSLOT_INVALID placeholder.
+	 */
 	kvm_for_each_vcpu(i, vcpu, kvm)
 		kvm_vcpu_kick(vcpu);
 }
@@ -10452,6 +10461,39 @@ void kvm_arch_commit_memory_region(struct kvm *kvm,
 				const struct kvm_memory_slot *new,
 				enum kvm_mr_change change)
 {
+	/*
+	 * Force re-resolution of any nested-state pages whose physical
+	 * addresses a vCPU has latched in hardware control structures
+	 * (e.g. vmcs02) from a gfn_to_pfn_cache. Software readers of
+	 * such caches catch the generation bump lazily, via
+	 * kvm_gpc_check() at their next use; the CPU's use from guest
+	 * mode is continuous and checks nothing, so the vCPU must be
+	 * told to re-resolve and re-latch before it next enters the
+	 * guest. The request is the artifact that survives racing with
+	 * a concurrent VM-entry (a bare kick landing before vcpu->mode
+	 * is set would be lost); its handler re-runs nested page setup,
+	 * whose gPA lookups then see the new generation and refresh.
+	 *
+	 * This is posted from commit, not from kvm_arch_memslots_updated(),
+	 * so it runs once per user-visible update with the final memslots
+	 * in place: a request serviced during a DELETE/MOVE's
+	 * invalidation phase would find the KVM_MEMSLOT_INVALID
+	 * placeholder and could only retry. A FLAGS_ONLY update (e.g. a
+	 * dirty-log toggle) cannot change any translation — base_gfn and
+	 * userspace_addr are immutable — so it posts nothing.
+	 *
+	 * The kick is deliberately fire-and-forget (no KVM_REQUEST_WAIT):
+	 * a vCPU still in L2 may keep using the old translation until it
+	 * services the request, which matches the staleness semantics the
+	 * replaced kvm_host_map code had, unboundedly. Lifetime is not at
+	 * stake — the underlying page is freed only via the mmu_notifier,
+	 * whose invalidation forces pinned vCPUs out of guest mode and
+	 * waits. Making the ioctl wait here would buy only a tighter
+	 * staleness bound, at a vCPU-roundtrip cost on every update.
+	 */
+	if (change != KVM_MR_FLAGS_ONLY)
+		kvm_make_all_cpus_request(kvm, KVM_REQ_GET_NESTED_STATE_PAGES);
+
 	if (change == KVM_MR_DELETE)
 		kvm_page_track_delete_slot(kvm, old);
 
-- 
2.43.0