linux-kernel - Re: [PATCH v2 14/15] KVM: x86/tdp

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Fri, 7 Jun 2024 22:31:29 +0000
From: "Edgecombe, Rick P" <rick.p.edgecombe@...el.com>
To: "pbonzini@...hat.com" <pbonzini@...hat.com>
CC: "sean.j.christopherson@...el.com" <sean.j.christopherson@...el.com>,
	"seanjc@...gle.com" <seanjc@...gle.com>, "Huang, Kai" <kai.huang@...el.com>,
	"sagis@...gle.com" <sagis@...gle.com>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "Aktas, Erdem" <erdemaktas@...gle.com>,
	"Zhao, Yan Y" <yan.y.zhao@...el.com>, "dmatlack@...gle.com"
	<dmatlack@...gle.com>, "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
	"Yamahata, Isaku" <isaku.yamahata@...el.com>, "isaku.yamahata@...il.com"
	<isaku.yamahata@...il.com>
Subject: Re: [PATCH v2 14/15] KVM: x86/tdp_mmu: Invalidate correct roots

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@...el.com> wrote:
> > 
> > From: Sean Christopherson <sean.j.christopherson@...el.com>
> > 
> > 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.