KVM: SVM: Flush Hyper-V TLB when required

---
 arch/x86/kvm/kvm_onhyperv.c | 23 +++++++++++++++++++++++
 arch/x86/kvm/kvm_onhyperv.h |  5 +++++
 arch/x86/kvm/svm/svm.c      | 18 +++++++++++++++---
 3 files changed, 43 insertions(+), 3 deletions(-)

arch/x86/kvm/kvm_onhyperv.c +0 -0

index 482d6639ef88..036e04c0a161 100644
@@ -94,6 +94,29 @@ int hv_remote_flush_tlb(struct kvm *kvm)
 }
 EXPORT_SYMBOL_GPL(hv_remote_flush_tlb);
 

Quoted 14 lines · Jeremi Piotrowski

---
 arch/x86/kvm/kvm_onhyperv.c | 23 +++++++++++++++++++++++
 arch/x86/kvm/kvm_onhyperv.h |  5 +++++
 arch/x86/kvm/svm/svm.c      | 18 +++++++++++++++---
 3 files changed, 43 insertions(+), 3 deletions(-)

arch/x86/kvm/kvm_onhyperv.c +0 -0

index 482d6639ef88..036e04c0a161 100644
@@ -94,6 +94,29 @@ int hv_remote_flush_tlb(struct kvm *kvm)
 }
 EXPORT_SYMBOL_GPL(hv_remote_flush_tlb);
 

arch/x86/kvm/kvm_onhyperv.c int hv_remote_flush_tlb(struct kvm *kvm) · line 106

Quoted 10 lines

arch/x86/kvm/kvm_onhyperv.c +10 -0

+void hv_flush_tlb_current(struct kvm_vcpu *vcpu)
+{
+	struct kvm_arch *kvm_arch = &vcpu->kvm->arch;
+	hpa_t root_tdp = vcpu->arch.mmu->root.hpa;
+
+	if (kvm_x86_ops.tlb_remote_flush == hv_remote_flush_tlb && VALID_PAGE(root_tdp)) {
+		spin_lock(&kvm_arch->hv_root_tdp_lock);
+		if (kvm_arch->hv_root_tdp != root_tdp) {
+			hyperv_flush_guest_mapping(root_tdp);
+			kvm_arch->hv_root_tdp = root_tdp;

In a vacuum, accessing kvm_arch->hv_root_tdp in the flush path is wrong. This likely fixes the issues you are seeing because the KVM bug only affects the case when KVM is loading a new root (that used to be valid), in which case hv_root_tdp is guaranteed to be different. But KVM should not rely on that behavior, i.e. if KVM says flush, then we flush. There might be scenarios where the flush is unnecessary, but those flushes should be elided by the code that knows the flush is unnecessary, not in this common code just because the target root is the globally shared root.

Somewhat of a moot point, but setting hv_root_tdp to root_tdp is also wrong. KVM's behavior is that hv_root_tdp points at a valid root if and only if all vCPUs share said root. E.g. invoking this when vCPUs have different roots will "corrupt" hv_root_tdp and possibly cause a remote flush to do the wrong thing.

arch/x86/kvm/kvm_onhyperv.c int hv_remote_flush_tlb(struct kvm *kvm) · line 115

Quoted 9 lines

arch/x86/kvm/kvm_onhyperv.c +9 -0

+		}
+		spin_unlock(&kvm_arch->hv_root_tdp_lock);
+	}
+}
+EXPORT_SYMBOL_GPL(hv_flush_tlb_current);
+
+void hv_flush_tlb_all(struct kvm_vcpu *vcpu)
+{
+	if (WARN_ON_ONCE(kvm_x86_ops.tlb_remote_flush == hv_remote_flush_tlb))

Hmm, looking at the KVM code, AFAICT KVM only enables enlightened_npt_tlb for L1 (L1 from KVM's perspective) as svm_hv_init_vmcb() is only ever called with vmcb01, never with vmcb02. I don't know if that's intentional, but I do think it means KVM can skip the Hyper-V flush for vmcb02 and instead rely on the ASID flush, i.e. KVM can do the Hyper-V iff enlightened_npt_tlb is set in the current VMCB. And that should continue to work if KVM does ever enabled enlightened_npt_tlb for L2.

arch/x86/kvm/kvm_onhyperv.c int hv_remote_flush_tlb(struct kvm *kvm) · line 118

Quoted 3 lines

arch/x86/kvm/kvm_onhyperv.c +3 -0

+		hv_remote_flush_tlb(vcpu->kvm);
+}
+EXPORT_SYMBOL_GPL(hv_flush_tlb_all);

I'd rather not add helpers to the common KVM code. I do like minimizing the amount of #ifdeffery, but defining these as common helpers makes it seem like VMX-on-HyperV is broken, i.e. raises the question of why VMX doesn't use these helpers when running on Hyper-V.

I'm thinking this?

---
 arch/x86/kvm/svm/svm.c          | 39 ++++++++++++++++++++++++++++++---
 arch/x86/kvm/svm/svm_onhyperv.h |  7 ++++++
 2 files changed, 43 insertions(+), 3 deletions(-)

arch/x86/kvm/svm/svm.c +36 -3

index 70183d2271b5..ab97fe8f1d81 100644
@@ -3746,7 +3746,7 @@ static void svm_enable_nmi_window(struct kvm_vcpu *vcpu)
 	svm->vmcb->save.rflags |= (X86_EFLAGS_TF | X86_EFLAGS_RF);
 }
 
-static void svm_flush_tlb_current(struct kvm_vcpu *vcpu)
+static void svm_flush_tlb_asid(struct kvm_vcpu *vcpu)
 {
 	struct vcpu_svm *svm = to_svm(vcpu);
 

@@ -3770,6 +3770,39 @@ static void svm_flush_tlb_current(struct kvm_vcpu *vcpu)
 		svm->current_vmcb->asid_generation--;
 }
 
+static void svm_flush_tlb_current(struct kvm_vcpu *vcpu)
+{
+#if IS_ENABLED(CONFIG_HYPERV)
+	hpa_t root_tdp = vcpu->arch.mmu->root.hpa;
+
+	/*
+	 * When running on Hyper-V with EnlightenedNptTlb enabled, explicitly
+	 * flush the NPT mappings via hypercall as flushing the ASID only
+	 * affects virtual to physical mappings, it does not invalidate guest
+	 * physical to host physical mappings.
+	 */
+	if (svm_hv_is_enlightened_tlb_enabled(vcpu) && VALID_PAGE(root_tdp))
+		hyperv_flush_guest_mapping(root_tdp);
+#endif
+	svm_flush_tlb_asid(vcpu);
+}
+
+static void svm_flush_tlb_all(struct kvm_vcpu *vcpu)
+{
+#if IS_ENABLED(CONFIG_HYPERV)
+	/*
+	 * When running on Hyper-V with EnlightenedNptTlb enabled, remote TLB
+	 * flushes should be routed to hv_remote_flush_tlb() without requesting
+	 * a "regular" remote flush.  Reaching this point means either there's
+	 * a KVM bug or a prior hv_remote_flush_tlb() call failed, both of
+	 * which might be fatal to the the guest.  Yell, but try to recover.
+	 */
+	if (WARN_ON_ONCE(svm_hv_is_enlightened_tlb_enabled(vcpu)))
+		hv_remote_flush_tlb(vcpu->kvm);
+#endif
+	svm_flush_tlb_asid(vcpu);
+}
+
 static void svm_flush_tlb_gva(struct kvm_vcpu *vcpu, gva_t gva)
 {
 	struct vcpu_svm *svm = to_svm(vcpu);

@@ -4762,10 +4795,10 @@ static struct kvm_x86_ops svm_x86_ops __initdata = {
 	.set_rflags = svm_set_rflags,
 	.get_if_flag = svm_get_if_flag,
 
-	.flush_tlb_all = svm_flush_tlb_current,
+	.flush_tlb_all = svm_flush_tlb_all,
 	.flush_tlb_current = svm_flush_tlb_current,
 	.flush_tlb_gva = svm_flush_tlb_gva,
-	.flush_tlb_guest = svm_flush_tlb_current,
+	.flush_tlb_guest = svm_flush_tlb_asid,
 
 	.vcpu_pre_run = svm_vcpu_pre_run,
 	.vcpu_run = svm_vcpu_run,

arch/x86/kvm/svm/svm_onhyperv.h +0 -0

index cff838f15db5..d91e019fb7da 100644

arch/x86/kvm/svm/svm_onhyperv.h +2 -0

+}
+
 static inline void svm_hv_init_vmcb(struct vmcb *vmcb)
 {
 	struct hv_vmcb_enlightenments *hve = &vmcb->control.hv_enlightenments;

base-commit: 50f13998451effea5c5fdc70fe576f8b435d6224

Quoted 8 lines · Sean Christopherson

arch/x86/kvm/svm/svm_onhyperv.h +0 -0

index cff838f15db5..d91e019fb7da 100644
@@ -15,6 +15,13 @@ static struct kvm_x86_ops svm_x86_ops;
 
 int svm_hv_enable_l2_tlb_flush(struct kvm_vcpu *vcpu);
 

arch/x86/kvm/svm/svm_onhyperv.h static struct kvm_x86_ops svm_x86_ops; · line 22

Quoted 5 lines

arch/x86/kvm/svm/svm_onhyperv.h +5 -0

+static inline bool svm_hv_is_enlightened_tlb_enabled(struct kvm_vcpu *vcpu)
+{
+	struct hv_vmcb_enlightenments *hve = &to_svm(vcpu)->vmcb->control.hv_enlightenments;
+
+	return !!hve->hv_enlightenments_control.enlightened_npt_tlb;

In theory, we should not look at Hyper-V enlightenments in VMCB control just because our kernel has CONFIG_HYPERV enabled.

I'd suggest we add a
real check that we're running on Hyper-V and we can do it the same way it is done in svm_hv_hardware_setup()/svm_hv_init_vmcb():

	return (ms_hyperv.nested_features & HV_X64_NESTED_ENLIGHTENED_TLB)
		&& !!hve->hv_enlightenments_control.enlightened_npt_tlb;

Quoted 9 lines · Vitaly Kuznetsov

Quoted 8 lines · Sean Christopherson

arch/x86/kvm/svm/svm_onhyperv.h +0 -0

index cff838f15db5..d91e019fb7da 100644
@@ -15,6 +15,13 @@ static struct kvm_x86_ops svm_x86_ops;
 
 int svm_hv_enable_l2_tlb_flush(struct kvm_vcpu *vcpu);
 

Quoted 8 lines

arch/x86/kvm/svm/svm_onhyperv.h static struct kvm_x86_ops svm_x86_ops; · line 22

Quoted 5 lines

arch/x86/kvm/svm/svm_onhyperv.h +5 -0

+static inline bool svm_hv_is_enlightened_tlb_enabled(struct kvm_vcpu *vcpu)
+{
+	struct hv_vmcb_enlightenments *hve = &to_svm(vcpu)->vmcb->control.hv_enlightenments;
+
+	return !!hve->hv_enlightenments_control.enlightened_npt_tlb;

In theory, we should not look at Hyper-V enlightenments in VMCB control just because our kernel has CONFIG_HYPERV enabled.

Oooh, right, because hv_enlightenments uses software reserved bits, and in theory KVM could be running on a different hypervisor that uses those bits for something completely different.

Quoted 4 lines

I'd suggest we add a
real check that we're running on Hyper-V and we can do it the same way it is done in svm_hv_hardware_setup()/svm_hv_init_vmcb():

Quoted 2 lines

	return (ms_hyperv.nested_features & HV_X64_NESTED_ENLIGHTENED_TLB)
		&& !!hve->hv_enlightenments_control.enlightened_npt_tlb;

Jeremi, if you grab this, can you put the && on the previous line? I.e.

	return (ms_hyperv.nested_features & HV_X64_NESTED_ENLIGHTENED_TLB) &&
	       !!hve->hv_enlightenments_control.enlightened_npt_tlb;

Quoted 14 lines · Jeremi Piotrowski

---
 arch/x86/kvm/kvm_onhyperv.c | 23 +++++++++++++++++++++++
 arch/x86/kvm/kvm_onhyperv.h |  5 +++++
 arch/x86/kvm/svm/svm.c      | 18 +++++++++++++++---
 3 files changed, 43 insertions(+), 3 deletions(-)

arch/x86/kvm/kvm_onhyperv.c +0 -0

index 482d6639ef88..036e04c0a161 100644
@@ -94,6 +94,29 @@ int hv_remote_flush_tlb(struct kvm *kvm)
 }
 EXPORT_SYMBOL_GPL(hv_remote_flush_tlb);
 

arch/x86/kvm/kvm_onhyperv.c int hv_remote_flush_tlb(struct kvm *kvm) · line 106

Quoted 10 lines

arch/x86/kvm/kvm_onhyperv.c +10 -0

+void hv_flush_tlb_current(struct kvm_vcpu *vcpu)
+{
+	struct kvm_arch *kvm_arch = &vcpu->kvm->arch;
+	hpa_t root_tdp = vcpu->arch.mmu->root.hpa;
+
+	if (kvm_x86_ops.tlb_remote_flush == hv_remote_flush_tlb && VALID_PAGE(root_tdp)) {
+		spin_lock(&kvm_arch->hv_root_tdp_lock);
+		if (kvm_arch->hv_root_tdp != root_tdp) {
+			hyperv_flush_guest_mapping(root_tdp);
+			kvm_arch->hv_root_tdp = root_tdp;

In a vacuum, accessing kvm_arch->hv_root_tdp in the flush path is wrong. This likely fixes the issues you are seeing because the KVM bug only affects the case when KVM is loading a new root (that used to be valid), in which case hv_root_tdp is guaranteed to be different. But KVM should not rely on that behavior, i.e. if KVM says flush, then we flush. There might be scenarios where the flush is unnecessary, but those flushes should be elided by the code that knows the flush is unnecessary, not in this common code just because the target root is the globally shared root>

Somewhat of a moot point, but setting hv_root_tdp to root_tdp is also wrong. KVM's behavior is that hv_root_tdp points at a valid root if and only if all vCPUs share said root. E.g. invoking this when vCPUs have different roots will "corrupt" hv_root_tdp and possibly cause a remote flush to do the wrong thing.>

arch/x86/kvm/kvm_onhyperv.c int hv_remote_flush_tlb(struct kvm *kvm) · line 115

Quoted 9 lines

arch/x86/kvm/kvm_onhyperv.c +9 -0

+		}
+		spin_unlock(&kvm_arch->hv_root_tdp_lock);
+	}
+}
+EXPORT_SYMBOL_GPL(hv_flush_tlb_current);
+
+void hv_flush_tlb_all(struct kvm_vcpu *vcpu)
+{
+	if (WARN_ON_ONCE(kvm_x86_ops.tlb_remote_flush == hv_remote_flush_tlb))

Hmm, looking at the KVM code, AFAICT KVM only enables enlightened_npt_tlb for L1 (L1 from KVM's perspective) as svm_hv_init_vmcb() is only ever called with vmcb01, never with vmcb02. I don't know if that's intentional, but I do think it means KVM can skip the Hyper-V flush for vmcb02 and instead rely on the ASID flush, i.e. KVM can do the Hyper-V iff enlightened_npt_tlb is set in the current VMCB. And that should continue to work if KVM does ever enabled enlightened_npt_tlb for L2.

arch/x86/kvm/kvm_onhyperv.c int hv_remote_flush_tlb(struct kvm *kvm) · line 118

Quoted 3 lines

arch/x86/kvm/kvm_onhyperv.c +3 -0

+		hv_remote_flush_tlb(vcpu->kvm);
+}
+EXPORT_SYMBOL_GPL(hv_flush_tlb_all);

I'd rather not add helpers to the common KVM code. I do like minimizing the amount of #ifdeffery, but defining these as common helpers makes it seem like VMX-on-HyperV is broken, i.e. raises the question of why VMX doesn't use these helpers when running on Hyper-V.

I'm thinking this?

I have the #ifdef version ready to send out, but what do you think about this:

arch/x86/kvm/svm/svm.c +1 -0

@@ -3753,6 +3753,39 @@ static void svm_flush_tlb_current(struct kvm_vcpu *vcpu)
 		svm->current_vmcb->asid_generation--;
 }
 
+static void svm_flush_tlb_current(struct kvm_vcpu *vcpu)

arch/x86/kvm/svm/svm.c +8 -0

+	    svm_hv_is_enlightened_tlb_enabled(vcpu) &&
+	    VALID_PAGE(root_tdp))
+		hyperv_flush_guest_mapping(root_tdp);
+
+	svm_flush_tlb_asid(vcpu);
+}
+
+static void svm_flush_tlb_all(struct kvm_vcpu *vcpu)

arch/x86/kvm/svm/svm.c +6 -2

+
+	svm_flush_tlb_asid(vcpu);
+}
+
 static void svm_flush_tlb_gva(struct kvm_vcpu *vcpu, gva_t gva)
 {
 	struct vcpu_svm *svm = to_svm(vcpu);

@@ -4745,10 +4778,10 @@ static struct kvm_x86_ops svm_x86_ops __initdata = {
 	.set_rflags = svm_set_rflags,
 	.get_if_flag = svm_get_if_flag,
 
-	.flush_tlb_all = svm_flush_tlb_current,
+	.flush_tlb_all = svm_flush_tlb_all,
 	.flush_tlb_current = svm_flush_tlb_current,
 	.flush_tlb_gva = svm_flush_tlb_gva,
-	.flush_tlb_guest = svm_flush_tlb_current,
+	.flush_tlb_guest = svm_flush_tlb_asid,
 
 	.vcpu_pre_run = svm_vcpu_pre_run,
 	.vcpu_run = svm_vcpu_run,

arch/x86/kvm/svm/svm_onhyperv.h +2 -0

index cff838f15db5..4c9e0d4ba3dd 100644
@@ -6,6 +6,8 @@
 #ifndef __ARCH_X86_KVM_SVM_ONHYPERV_H__
 #define __ARCH_X86_KVM_SVM_ONHYPERV_H__
 
+#include <asm/mshyperv.h>
+
 #if IS_ENABLED(CONFIG_HYPERV)
 
 #include "kvm_onhyperv.h"