Re: [PATCH 17/18] KVM: x86/tdp_mmu: Don't zap valid mirror roots in kvm_tdp_mmu_zap_all()

[Yan Zhao <yan.y.zhao@intel.com>]

On Fri, Dec 13, 2024 at 02:57:10PM -0500, Paolo Bonzini wrote:
> From: Rick Edgecombe <rick.p.edgecombe@intel.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@intel.com>
> Signed-off-by: Yan Zhao <yan.y.zhao@intel.com>
> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
> Message-ID: <20240718211230.1492011-18-rick.p.edgecombe@intel.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.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
> 
>