Re: [RFC PATCH 12/24] KVM: x86: hyper-v: Pass is_guest_mode to kvm_hv_vcpu_purge_flush_tlb()

[Sean Christopherson]

On Thu, Apr 03, 2025, Maxim Levitsky wrote:
> On Wed, 2025-03-26 at 19:36 +0000, Yosry Ahmed wrote:
> > Instead of calling is_guest_mode() inside kvm_hv_vcpu_purge_flush_tlb()
> > pass the value from the caller. Future changes will pass different
> > values than is_guest_mode(vcpu).
> > 
> > No functional change intended.
> > 
> > Signed-off-by: Yosry Ahmed <yosry.ahmed@xxxxxxxxx>
> > ---
> >  arch/x86/kvm/hyperv.h  | 8 +++++---
> >  arch/x86/kvm/svm/svm.c | 2 +-
> >  arch/x86/kvm/x86.c     | 2 +-
> >  3 files changed, 7 insertions(+), 5 deletions(-)
> > 
> > diff --git a/arch/x86/kvm/hyperv.h b/arch/x86/kvm/hyperv.h
> > index 913bfc96959cb..be715deaeb003 100644
> > --- a/arch/x86/kvm/hyperv.h
> > +++ b/arch/x86/kvm/hyperv.h
> > @@ -203,14 +203,15 @@ static inline struct kvm_vcpu_hv_tlb_flush_fifo *kvm_hv_get_tlb_flush_fifo(struc
> >  	return &hv_vcpu->tlb_flush_fifo[i];
> >  }
> >  
> > -static inline void kvm_hv_vcpu_purge_flush_tlb(struct kvm_vcpu *vcpu)
> > +static inline void kvm_hv_vcpu_purge_flush_tlb(struct kvm_vcpu *vcpu,
> > +					       bool is_guest_mode)

NAK, passing around is_guest_mode is going to cause problems.  All it takes is
one snippet of code that operates on the current vCPU state for KVM to end up
with bugs.  It's unfortunate that kvm_hv_get_tlb_flush_fifo() takes in an
@is_guest_mode param, but that's "necessary" due to the cross-vCPU nature of
the usage.  For this case, there is no such requirement/restriction.

I also think that being super explicit isn't a bad thing, even if it means we
might end up with duplicate code.  I.e. having this

	vmcb_set_flush_asid(svm->vmcb01.ptr);
	if (svm->nested.vmcb02.ptr)
		vmcb_set_flush_asid(svm->nested.vmcb02.ptr);

in svm_flush_tlb_all() is a net positive IMO, because it explicitly reads "flush
vmcb01's ASID, and vmcb02's ASID if vmcb02 is valid".  Whereas this

        svm_flush_tlb_asid(vcpu, false);
        svm_flush_tlb_asid(vcpu, true);

isn't anywhere near as explicit.  I can make a good guess as to what true/false
are specifying, but many readers will need to go at least a layer or two deeper
to understand what's going on.  More importantly, it's not at all clear in
svm_flush_tlb_asid() that the vmcb can/should only be NULL in the is_guest_mode=true
case.

        if (vmcb)
                vmcb_set_flush_asid(vmcb);

And it's even actively dangerous, in that a bug where a vmcb is unexpectedly NULL
could lead to a missed TLB flush.  I.e. we *want* a NULL pointer #GP in a case
like this, so that the host yells loudly (even if it means panicking), versus
silently doing nothing and potentially corrupting guest data.  In practice, I can't
imagine such a bug ever being truly silent, e.g. KVM is all but guaranteed to
consume the NULL vmcb sooner than later.  But I still don't like creating such a
possibility.

> >  {
> >  	struct kvm_vcpu_hv_tlb_flush_fifo *tlb_flush_fifo;
> >  
> >  	if (!to_hv_vcpu(vcpu) || !kvm_check_request(KVM_REQ_HV_TLB_FLUSH, vcpu))

Case in point, kvm_check_request() is destructive (the name sucks, but it is what
it is), i.e. KVM_REQ_HV_TLB_FLUSH will be cleared, and so only the first of the
calls to svm_flush_tlb_asid() and thus kvm_hv_vcpu_purge_flush_tlb() will actually
do anything.  This particular bug is functionally benign (KVM will over-flush),
but it's still a bug.

Somewhat of a side topic, I think we should rename kvm_hv_vcpu_purge_flush_tlb()
to something like kvm_hv_purge_tlb_flush_fifo().  I initially read the first one
as "purge *and* flush TLBs", whereas the function is actually "purge the TLB
flush FIFO".

Completely untested, but I think we should shoot for something like this, over
2 or 3 patches.

---
 arch/x86/kvm/hyperv.h     | 14 +++++++++++++-
 arch/x86/kvm/svm/nested.c |  1 -
 arch/x86/kvm/svm/svm.c    | 17 ++++++++---------
 3 files changed, 21 insertions(+), 11 deletions(-)

diff --git a/arch/x86/kvm/hyperv.h b/arch/x86/kvm/hyperv.h
index 913bfc96959c..f2c17459dd8b 100644
--- a/arch/x86/kvm/hyperv.h
+++ b/arch/x86/kvm/hyperv.h
@@ -203,7 +203,7 @@ static inline struct kvm_vcpu_hv_tlb_flush_fifo *kvm_hv_get_tlb_flush_fifo(struc
 	return &hv_vcpu->tlb_flush_fifo[i];
 }
 
-static inline void kvm_hv_vcpu_purge_flush_tlb(struct kvm_vcpu *vcpu)
+static inline void kvm_hv_purge_tlb_flush_fifo(struct kvm_vcpu *vcpu)
 {
 	struct kvm_vcpu_hv_tlb_flush_fifo *tlb_flush_fifo;
 
@@ -215,6 +215,18 @@ static inline void kvm_hv_vcpu_purge_flush_tlb(struct kvm_vcpu *vcpu)
 	kfifo_reset_out(&tlb_flush_fifo->entries);
 }
 
+static inline void kvm_hv_purge_tlb_flush_fifo(struct kvm_vcpu *vcpu)
+{
+	struct kvm_vcpu_hv *hv_vcpu = to_hv_vcpu(vcpu);
+	int i;
+
+	if (!hv_vcpu || !kvm_check_request(KVM_REQ_HV_TLB_FLUSH, vcpu))
+		return;
+
+	for (i = 0; i < ARRAY_SIZE(hv_vcpu->tlb_flush_fifo); i++)
+		kfifo_reset_out(&hv_vcpu->tlb_flush_fifo[i]->entries);
+}
+
 static inline bool guest_hv_cpuid_has_l2_tlb_flush(struct kvm_vcpu *vcpu)
 {
 	struct kvm_vcpu_hv *hv_vcpu = to_hv_vcpu(vcpu);
diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index b6c27b34f8e5..7e9156f27a96 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -491,7 +491,6 @@ static void nested_svm_entry_tlb_flush(struct kvm_vcpu *vcpu)
 	 * TODO: optimize unconditional TLB flush/MMU sync.  A partial list of
 	 * things to fix before this can be conditional:
 	 *
-	 *  - Flush TLBs for both L1 and L2 remote TLB flush
 	 *  - Honor L1's request to flush an ASID on nested VMRUN
 	 *  - Sync nested NPT MMU on VMRUN that flushes L2's ASID[*]
 	 *  - Don't crush a pending TLB flush in vmcb02 on nested VMRUN
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 371593c4b629..f7be29733c9d 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -4163,15 +4163,8 @@ static void svm_flush_tlb_asid(struct kvm_vcpu *vcpu)
 	 * A TLB flush for the current ASID flushes both "host" and "guest" TLB
 	 * entries, and thus is a superset of Hyper-V's fine grained flushing.
 	 */
-	kvm_hv_vcpu_purge_flush_tlb(vcpu);
+	kvm_hv_purge_tlb_flush_fifo(vcpu);
 
-	/*
-	 * Flush only the current ASID even if the TLB flush was invoked via
-	 * kvm_flush_remote_tlbs().  Although flushing remote TLBs requires all
-	 * ASIDs to be flushed, KVM uses a single ASID for L1 and L2, and
-	 * unconditionally does a TLB flush on both nested VM-Enter and nested
-	 * VM-Exit (via kvm_mmu_reset_context()).
-	 */
 	vmcb_set_flush_asid(svm->vmcb);
 }
 
@@ -4193,6 +4186,8 @@ static void svm_flush_tlb_current(struct kvm_vcpu *vcpu)
 
 static void svm_flush_tlb_all(struct kvm_vcpu *vcpu)
 {
+	struct vcpu_svm *svm = to_svm(vcpu);
+
 	/*
 	 * When running on Hyper-V with EnlightenedNptTlb enabled, remote TLB
 	 * flushes should be routed to hv_flush_remote_tlbs() without requesting
@@ -4203,7 +4198,11 @@ static void svm_flush_tlb_all(struct kvm_vcpu *vcpu)
 	if (WARN_ON_ONCE(svm_hv_is_enlightened_tlb_enabled(vcpu)))
 		hv_flush_remote_tlbs(vcpu->kvm);
 
-	svm_flush_tlb_asid(vcpu);
+	kvm_hv_vcpu_purge_flush_tlb_all(vcpu);
+
+	vmcb_set_flush_asid(svm->vmcb01.ptr);
+	if (svm->nested.vmcb02.ptr)
+		vmcb_set_flush_asid(svm->nested.vmcb02.ptr);
 }
 
 static void svm_flush_tlb_gva(struct kvm_vcpu *vcpu, gva_t gva)

base-commit: ba550af5af66a83ad055519b2271f6a21f28cb1b
--