Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 22 Mar 2023 09:20:21 -0700
In-Reply-To: <20230320185110.1346829-1-jpiotrowski@linux.microsoft.com>
Mime-Version: 1.0
References: <20230320185110.1346829-1-jpiotrowski@linux.microsoft.com>
Message-ID: <ZBsqxeRDh+iV8qmm@google.com>
Subject: Re: [PATCH] KVM: SVM: Flush Hyper-V TLB when required
From:   Sean Christopherson <seanjc@google.com>
To:     Jeremi Piotrowski <jpiotrowski@linux.microsoft.com>
Cc:     linux-kernel@vger.kernel.org, Paolo Bonzini <pbonzini@redhat.com>,
        kvm@vger.kernel.org, Vitaly Kuznetsov <vkuznets@redhat.com>,
        Tianyu Lan <ltykernel@gmail.com>,
        Michael Kelley <mikelley@microsoft.com>, stable@vger.kernel.org
Content-Type: text/plain; charset="us-ascii"
Precedence: bulk

On Mon, Mar 20, 2023, Jeremi Piotrowski wrote:
> ---
>  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(-)
> 
> diff --git a/arch/x86/kvm/kvm_onhyperv.c b/arch/x86/kvm/kvm_onhyperv.c
> index 482d6639ef88..036e04c0a161 100644
> --- a/arch/x86/kvm/kvm_onhyperv.c
> +++ b/arch/x86/kvm/kvm_onhyperv.c
> @@ -94,6 +94,29 @@ int hv_remote_flush_tlb(struct kvm *kvm)
>  }
>  EXPORT_SYMBOL_GPL(hv_remote_flush_tlb);
>  
> +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.

> +		}
> +		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.

> +		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(-)

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 70183d2271b5..ab97fe8f1d81 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -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,
diff --git a/arch/x86/kvm/svm/svm_onhyperv.h b/arch/x86/kvm/svm/svm_onhyperv.h
index cff838f15db5..d91e019fb7da 100644
--- a/arch/x86/kvm/svm/svm_onhyperv.h
+++ b/arch/x86/kvm/svm/svm_onhyperv.h
@@ -15,6 +15,13 @@ static struct kvm_x86_ops svm_x86_ops;
 
 int svm_hv_enable_l2_tlb_flush(struct kvm_vcpu *vcpu);
 
+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;
+}
+
 static inline void svm_hv_init_vmcb(struct vmcb *vmcb)
 {
 	struct hv_vmcb_enlightenments *hve = &vmcb->control.hv_enlightenments;

base-commit: 50f13998451effea5c5fdc70fe576f8b435d6224
--