On Wed, Jun 26, 2024 at 04:56:33AM +0800, Edgecombe, Rick P wrote: > On Tue, 2024-06-25 at 14:14 +0800, Yan Zhao wrote: > > > Explain why not to zap invalid mirrored roots? > > No. Explain why not to zap invalid direct roots. > > Just passing KVM_DIRECT_ROOTS will zap only valid direct roots, right? > > The original kvm_tdp_mmu_zap_all() "Zap all roots, including invalid roots". > > It might be better to explain why not to zap invalid direct roots here. > > Hmm, right. We don't actually have enum value to iterate all direct roots (valid > and invalid). We could change tdp_mmu_root_match() to: > diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c > index 8320b093fef9..bcd771a8835f 100644 > --- a/arch/x86/kvm/mmu/tdp_mmu.c > +++ b/arch/x86/kvm/mmu/tdp_mmu.c > @@ -98,8 +98,8 @@ static bool tdp_mmu_root_match(struct kvm_mmu_page *root, > if (WARN_ON_ONCE(!(types & KVM_VALID_ROOTS))) > return false; > > - if (root->role.invalid) > - return types & KVM_INVALID_ROOTS; > + if (root->role.invalid && !(types & KVM_INVALID_ROOTS)) > + return false; > if (likely(!is_mirror_sp(root))) > return types & KVM_DIRECT_ROOTS; > > Maybe better than a comment...? Need to put the whole thing together and test it > still. Hmm, I think passing KVM_DIRECT_ROOTS only in kvm_tdp_mmu_zap_all() is ok, because kvm_mmu_uninit_tdp_mmu() will zap all invalidated roots eventually, though KVM_DIRECT_ROOTS | KVM_INVALID_ROOTS matches more to the function name zap_all. I'm not convinced that the change in tdp_mmu_root_match() above is needed. Once a root is marked invalid, it won't be restored later. Distinguishing between invalid direct roots and invalid mirror roots will only result in more unused roots lingering unnecessarily.
