Received-SPF: pass (google.com: domain of linux-kernel+bounces-55265-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Tue, 6 Feb 2024 23:39:04 +0800
From: Xu Yilun <yilun.xu@linux.intel.com>
To: Sean Christopherson <seanjc@google.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>, kvm@vger.kernel.org,
	linux-kernel@vger.kernel.org, David Matlack <dmatlack@google.com>,
	Pattara Teerapong <pteerapong@google.com>
Subject: Re: [PATCH 7/8] KVM: x86/mmu: Alloc TDP MMU roots while holding
 mmu_lock for read
Message-ID: <ZcJSmNRLbKacPfoq@yilunxu-OptiPlex-7050>
References: <20240111020048.844847-1-seanjc@google.com>
 <20240111020048.844847-8-seanjc@google.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240111020048.844847-8-seanjc@google.com>

On Wed, Jan 10, 2024 at 06:00:47PM -0800, Sean Christopherson wrote:
> Allocate TDP MMU roots while holding mmu_lock for read, and instead use
> tdp_mmu_pages_lock to guard against duplicate roots.  This allows KVM to
> create new roots without forcing kvm_tdp_mmu_zap_invalidated_roots() to
> yield, e.g. allows vCPUs to load new roots after memslot deletion without
> forcing the zap thread to detect contention and yield (or complete if the
> kernel isn't preemptible).
> 
> Note, creating a new TDP MMU root as an mmu_lock reader is safe for two
> reasons: (1) paths that must guarantee all roots/SPTEs are *visited* take
> mmu_lock for write and so are still mutually exclusive, e.g. mmu_notifier
> invalidations, and (2) paths that require all roots/SPTEs to *observe*
> some given state without holding mmu_lock for write must ensure freshness
> through some other means, e.g. toggling dirty logging must first wait for
> SRCU readers to recognize the memslot flags change before processing
> existing roots/SPTEs.
> 
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/kvm/mmu/tdp_mmu.c | 55 +++++++++++++++-----------------------
>  1 file changed, 22 insertions(+), 33 deletions(-)
> 
> diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
> index 9a8250a14fc1..d078157e62aa 100644
> --- a/arch/x86/kvm/mmu/tdp_mmu.c
> +++ b/arch/x86/kvm/mmu/tdp_mmu.c
> @@ -223,51 +223,42 @@ static void tdp_mmu_init_child_sp(struct kvm_mmu_page *child_sp,
>  	tdp_mmu_init_sp(child_sp, iter->sptep, iter->gfn, role);
>  }
>  
> -static struct kvm_mmu_page *kvm_tdp_mmu_try_get_root(struct kvm_vcpu *vcpu)
> -{
> -	union kvm_mmu_page_role role = vcpu->arch.mmu->root_role;
> -	int as_id = kvm_mmu_role_as_id(role);
> -	struct kvm *kvm = vcpu->kvm;
> -	struct kvm_mmu_page *root;
> -
> -	for_each_valid_tdp_mmu_root_yield_safe(kvm, root, as_id) {
> -		if (root->role.word == role.word)
> -			return root;
> -	}
> -
> -	return NULL;
> -}
> -
>  int kvm_tdp_mmu_alloc_root(struct kvm_vcpu *vcpu)
>  {
>  	struct kvm_mmu *mmu = vcpu->arch.mmu;
>  	union kvm_mmu_page_role role = mmu->root_role;
> +	int as_id = kvm_mmu_role_as_id(role);
>  	struct kvm *kvm = vcpu->kvm;
>  	struct kvm_mmu_page *root;
>  
>  	/*
> -	 * Check for an existing root while holding mmu_lock for read to avoid
> +	 * Check for an existing root before acquiring the pages lock to avoid
>  	 * unnecessary serialization if multiple vCPUs are loading a new root.
>  	 * E.g. when bringing up secondary vCPUs, KVM will already have created
>  	 * a valid root on behalf of the primary vCPU.
>  	 */
>  	read_lock(&kvm->mmu_lock);
> -	root = kvm_tdp_mmu_try_get_root(vcpu);
> -	read_unlock(&kvm->mmu_lock);
>  
> -	if (root)
> -		goto out;
> +	for_each_valid_tdp_mmu_root_yield_safe(kvm, root, as_id) {
> +		if (root->role.word == role.word)
> +			goto out_read_unlock;
> +	}
>  
> -	write_lock(&kvm->mmu_lock);

It seems really complex to me...

I failed to understand why the following KVM_BUG_ON() could be avoided
without the mmu_lock for write. I thought a valid root could be added
during zapping.

  void kvm_tdp_mmu_zap_invalidated_roots(struct kvm *kvm)
  {
	struct kvm_mmu_page *root;

	read_lock(&kvm->mmu_lock);

	for_each_tdp_mmu_root_yield_safe(kvm, root) {
		if (!root->tdp_mmu_scheduled_root_to_zap)
			continue;

		root->tdp_mmu_scheduled_root_to_zap = false;
		KVM_BUG_ON(!root->role.invalid, kvm);

Thanks,
Yilun

> +	spin_lock(&kvm->arch.tdp_mmu_pages_lock);
>  
>  	/*
> -	 * Recheck for an existing root after acquiring mmu_lock for write.  It
> -	 * is possible a new usable root was created between dropping mmu_lock
> -	 * (for read) and acquiring it for write.
> +	 * Recheck for an existing root after acquiring the pages lock, another
> +	 * vCPU may have raced ahead and created a new usable root.  Manually
> +	 * walk the list of roots as the standard macros assume that the pages
> +	 * lock is *not* held.  WARN if grabbing a reference to a usable root
> +	 * fails, as the last reference to a root can only be put *after* the
> +	 * root has been invalidated, which requires holding mmu_lock for write.
>  	 */
> -	root = kvm_tdp_mmu_try_get_root(vcpu);
> -	if (root)
> -		goto out_unlock;
> +	list_for_each_entry(root, &kvm->arch.tdp_mmu_roots, link) {
> +		if (root->role.word == role.word &&
> +		    !WARN_ON_ONCE(!kvm_tdp_mmu_get_root(root)))
> +			goto out_spin_unlock;
> +	}
>  
>  	root = tdp_mmu_alloc_sp(vcpu);
>  	tdp_mmu_init_sp(root, NULL, 0, role);
> @@ -280,14 +271,12 @@ int kvm_tdp_mmu_alloc_root(struct kvm_vcpu *vcpu)
>  	 * is ultimately put by kvm_tdp_mmu_zap_invalidated_roots().
>  	 */
>  	refcount_set(&root->tdp_mmu_root_count, 2);
> -
> -	spin_lock(&kvm->arch.tdp_mmu_pages_lock);
>  	list_add_rcu(&root->link, &kvm->arch.tdp_mmu_roots);
> -	spin_unlock(&kvm->arch.tdp_mmu_pages_lock);
>  
> -out_unlock:
> -	write_unlock(&kvm->mmu_lock);
> -out:
> +out_spin_unlock:
> +	spin_unlock(&kvm->arch.tdp_mmu_pages_lock);
> +out_read_unlock:
> +	read_unlock(&kvm->mmu_lock);
>  	/*
>  	 * Note, KVM_REQ_MMU_FREE_OBSOLETE_ROOTS will prevent entering the guest
>  	 * and actually consuming the root if it's invalidated after dropping
> -- 
> 2.43.0.275.g3460e3d667-goog
> 
>