On Fri, 2024-06-07 at 11:03 +0200, Paolo Bonzini wrote:
> On Thu, May 30, 2024 at 11:07 PM Rick Edgecombe
> <rick.p.edgecombe@xxxxxxxxx> wrote:
> >
> > From: Sean Christopherson <sean.j.christopherson@xxxxxxxxx>
> >
> > 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.
