[v2,14/15] KVM: x86/tdp_mmu: Invalidate correct roots

Commit Message

Edgecombe, Rick P May 30, 2024, 9:07 p.m. UTC

From: Sean Christopherson <[e-mail]>

When invalidating roots, respect the root type passed.

kvm_tdp_mmu_invalidate_roots() is called with different root types. For
kvm_mmu_zap_all_fast() it only operates on shared roots. But when tearing
down a VM it needs to invalidate all roots. Check the root type in root
iterator.

Signed-off-by: Sean Christopherson <[e-mail]>
Co-developed-by: Isaku Yamahata <[e-mail]>
Signed-off-by: Isaku Yamahata <[e-mail]>
[evolved quite a bit from original author's patch]
Co-developed-by: Rick Edgecombe <[e-mail]>
Signed-off-by: Rick Edgecombe <[e-mail]>
---
TDX MMU Prep:
 - Rename from "Don't zap private pages for unsupported cases", and split
   many parts out.
 - Don't support MTRR, apic zapping (Rick)
 - Detangle private/shared alias logic in kvm_tdp_mmu_unmap_gfn_range()
   (Rick)
 - Fix TLB flushing bug debugged by (Chao Gao)
   https://lore.kernel.org/kvm/Zh8yHEiOKyvZO+QR@chao-email/
 - Split out MTRR part
 - Use enum based root iterators (Sean)
 - Reorder logic in kvm_mmu_zap_memslot_leafs().
 - Replace skip_private with enum kvm_tdp_mmu_root_type.
---
 arch/x86/kvm/mmu/tdp_mmu.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Paolo Bonzini June 7, 2024, 9:03 a.m. UTC | #1

On Thu, May 30, 2024 at 11:07 PM Rick Edgecombe
<[e-mail]> wrote:
>
> From: Sean Christopherson <[e-mail]>
>
> When invalidating roots, respect the root type passed.
>
> kvm_tdp_mmu_invalidate_roots() is called with different root types. For
> kvm_mmu_zap_all_fast() it only operates on shared roots. But when tearing
> down a VM it needs to invalidate all roots. Check the root type in root
> iterator.

This patch and patch 12 are small enough that they can be merged.

> @@ -1135,6 +1135,7 @@ void kvm_tdp_mmu_zap_invalidated_roots(struct kvm *kvm)
>  void kvm_tdp_mmu_invalidate_roots(struct kvm *kvm,
>                                   enum kvm_process process_types)
>  {
> +       enum kvm_tdp_mmu_root_types root_types = kvm_process_to_root_types(kvm, process_types);

Maybe pass directly enum kvm_tdp_mmu_root_types?

Looking at patch 12:

+ /*
+ * The private page tables doesn't support fast zapping.  The
+ * caller should handle it by other way.
+ */
+ kvm_tdp_mmu_invalidate_roots(kvm, KVM_PROCESS_SHARED);

now that we have separated private-ness and external-ness, it sounds
much better to write:

/*
 * External page tables don't support fast zapping, therefore
 * their mirrors must be invalidated separately by the caller.
 */
kvm_tdp_mmu_invalidate_roots(kvm, KVM_DIRECT_ROOTS);

while kvm_mmu_uninit_tdp_mmu() can pass KVM_ANY_ROOTS. It may also be
worth adding an

if (WARN_ON_ONCE(root_types & KVM_INVALID_ROOTS))
  root_types &= ~KVM_INVALID_ROOTS;

to document the invariants.

Paolo

Edgecombe, Rick P June 7, 2024, 10:31 p.m. UTC | #2

On Fri, 2024-06-07 at 11:03 +0200, Paolo Bonzini wrote:
> On Thu, May 30, 2024 at 11:07 PM Rick Edgecombe
> <[e-mail]> wrote:
> > 
> > From: Sean Christopherson <[e-mail]>
> > 
> > When invalidating roots, respect the root type passed.
> > 
> > kvm_tdp_mmu_invalidate_roots() is called with different root types. For
> > kvm_mmu_zap_all_fast() it only operates on shared roots. But when tearing
> > down a VM it needs to invalidate all roots. Check the root type in root
> > iterator.
> 
> This patch and patch 12 are small enough that they can be merged.

Sure.

> 
> > @@ -1135,6 +1135,7 @@ void kvm_tdp_mmu_zap_invalidated_roots(struct kvm
> > *kvm)
> >   void kvm_tdp_mmu_invalidate_roots(struct kvm *kvm,
> >                                    enum kvm_process process_types)
> >   {
> > +       enum kvm_tdp_mmu_root_types root_types =
> > kvm_process_to_root_types(kvm, process_types);
> 
> Maybe pass directly enum kvm_tdp_mmu_root_types?
> 
> Looking at patch 12:
> 
> + /*
> + * The private page tables doesn't support fast zapping.  The
> + * caller should handle it by other way.
> + */
> + kvm_tdp_mmu_invalidate_roots(kvm, KVM_PROCESS_SHARED);
> 
> now that we have separated private-ness and external-ness, it sounds
> much better to write:
> 
> /*
>  * External page tables don't support fast zapping, therefore
>  * their mirrors must be invalidated separately by the caller.
>  */
> kvm_tdp_mmu_invalidate_roots(kvm, KVM_DIRECT_ROOTS);
> 
> while kvm_mmu_uninit_tdp_mmu() can pass KVM_ANY_ROOTS. It may also be
> worth adding an
> 
> if (WARN_ON_ONCE(root_types & KVM_INVALID_ROOTS))
>   root_types &= ~KVM_INVALID_ROOTS;
> 
> to document the invariants.

Yes, I agree taking the kvm_tdp_mmu_root_types enum here makes more sense.
Especially with that comment you wrote.

Thanks.

Patch

diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
index da6024b8295f..0caa1029b6bd 100644
--- a/arch/x86/kvm/mmu/tdp_mmu.c
+++ b/arch/x86/kvm/mmu/tdp_mmu.c
@@ -1135,6 +1135,7 @@  void kvm_tdp_mmu_zap_invalidated_roots(struct kvm *kvm)
 void kvm_tdp_mmu_invalidate_roots(struct kvm *kvm,
 				  enum kvm_process process_types)
 {
+	enum kvm_tdp_mmu_root_types root_types = kvm_process_to_root_types(kvm, process_types);
 	struct kvm_mmu_page *root;
 
 	/*
@@ -1158,6 +1159,9 @@  void kvm_tdp_mmu_invalidate_roots(struct kvm *kvm,
 	 * or get/put references to roots.
 	 */
 	list_for_each_entry(root, &kvm->arch.tdp_mmu_roots, link) {
+		if (!tdp_mmu_root_match(root, root_types))
+			continue;
+
 		/*
 		 * Note, invalid roots can outlive a memslot update!  Invalid
 		 * roots must be *zapped* before the memslot update completes,