linux-kernel - Re: [PATCH 17/18] KVM: x86/tdp_mmu: Don't zap valid mirror roots in kvm_tdp_mmu_zap

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] [day] [month] [year] [list]

Message-ID: <Z2I4Q7qml4K0zpwZ@yzhao56-desk.sh.intel.com>
Date: Wed, 18 Dec 2024 10:49:39 +0800
From: Yan Zhao <yan.y.zhao@...el.com>
To: Paolo Bonzini <pbonzini@...hat.com>
CC: <linux-kernel@...r.kernel.org>, <kvm@...r.kernel.org>,
	<isaku.yamahata@...el.com>, <binbin.wu@...ux.intel.com>,
	<rick.p.edgecombe@...el.com>
Subject: Re: [PATCH 17/18] KVM: x86/tdp_mmu: Don't zap valid mirror roots in
 kvm_tdp_mmu_zap_all()

On Fri, Dec 13, 2024 at 02:57:10PM -0500, Paolo Bonzini wrote:
> From: Rick Edgecombe <rick.p.edgecombe@...el.com>
> 
> Don't zap valid mirror roots in kvm_tdp_mmu_zap_all(), which in effect
> is only direct roots (invalid and valid).
> 
> For TDX, kvm_tdp_mmu_zap_all() is only called during MMU notifier
> release. Since, mirrored EPT comes from guest mem, it will never be
> mapped to userspace, and won't apply. But in addition to be unnecessary,
> mirrored EPT is cleaned up in a special way during VM destruction.
> 
> Pass the KVM_INVALID_ROOTS bit into __for_each_tdp_mmu_root_yield_safe()
> as well, to clean up invalid direct roots, as is the current behavior.
> 
> Co-developed-by: Yan Zhao <yan.y.zhao@...el.com>
> Signed-off-by: Yan Zhao <yan.y.zhao@...el.com>
> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@...el.com>
> Message-ID: <20240718211230.1492011-18-rick.p.edgecombe@...el.com>
> Signed-off-by: Paolo Bonzini <pbonzini@...hat.com>
> ---
>  arch/x86/kvm/mmu/tdp_mmu.c | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
> index 94c464ce1d12..e08c60834d0c 100644
> --- a/arch/x86/kvm/mmu/tdp_mmu.c
> +++ b/arch/x86/kvm/mmu/tdp_mmu.c
> @@ -999,19 +999,23 @@ void kvm_tdp_mmu_zap_all(struct kvm *kvm)
>  	struct kvm_mmu_page *root;
>  
>  	/*
> -	 * Zap all roots, including invalid roots, as all SPTEs must be dropped
> -	 * before returning to the caller.  Zap directly even if the root is
> -	 * also being zapped by a worker.  Walking zapped top-level SPTEs isn't
> -	 * all that expensive and mmu_lock is already held, which means the
> -	 * worker has yielded, i.e. flushing the work instead of zapping here
> -	 * isn't guaranteed to be any faster.
> +	 * Zap all roots, except valid mirror roots, as all direct SPTEs must
As now specifying "KVM_DIRECT_ROOTS | KVM_INVALID_ROOTS" only zaps valid direct
roots + invalid direct roots, do we need to update the comment? e.g. to

	* Zap all direct roots, including invalid direct roots, as all direct
	* SPTEs must be dropped before returning to the caller.	For TDX, mirror
	* roots don't need handling in response to the mmu notifier (the caller).



> +	 * be dropped before returning to the caller. For TDX, mirror roots
> +	 * don't need handling in response to the mmu notifier (the caller) and
> +	 * they also won't be invalid until the VM is being torn down.
> +	 *
> +	 * Zap directly even if the root is also being zapped by a worker.
> +	 * Walking zapped top-level SPTEs isn't all that expensive and mmu_lock
> +	 * is already held, which means the worker has yielded, i.e. flushing
> +	 * the work instead of zapping here isn't guaranteed to be any faster.
>  	 *
>  	 * A TLB flush is unnecessary, KVM zaps everything if and only the VM
>  	 * is being destroyed or the userspace VMM has exited.  In both cases,
>  	 * KVM_RUN is unreachable, i.e. no vCPUs will ever service the request.
>  	 */
>  	lockdep_assert_held_write(&kvm->mmu_lock);
> -	for_each_tdp_mmu_root_yield_safe(kvm, root)
> +	__for_each_tdp_mmu_root_yield_safe(kvm, root, -1,
> +					   KVM_DIRECT_ROOTS | KVM_INVALID_ROOTS)
>  		tdp_mmu_zap_root(kvm, root, false);
>  }
>  
> -- 
> 2.43.5
> 
>