Received-SPF: pass (google.com: domain of linux-kernel+bounces-56703-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Wed, 7 Feb 2024 23:13:41 +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: <ZcOeJXHsiE5XUrBv@yilunxu-OptiPlex-7050>
References: <20240111020048.844847-1-seanjc@google.com>
 <20240111020048.844847-8-seanjc@google.com>
 <ZcJSmNRLbKacPfoq@yilunxu-OptiPlex-7050>
 <ZcJ2JG54O0g07e-P@google.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZcJ2JG54O0g07e-P@google.com>

On Tue, Feb 06, 2024 at 10:10:44AM -0800, Sean Christopherson wrote:
> On Tue, Feb 06, 2024, Xu Yilun wrote:
> > On Wed, Jan 10, 2024 at 06:00:47PM -0800, Sean Christopherson wrote:
> > > ---
> > >  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);
> 
> tdp_mmu_scheduled_root_to_zap is set only when mmu_lock is held for write, i.e.
> it's mutually exclusive with allocating a new root.
> 
> And tdp_mmu_scheduled_root_to_zap is cleared if and only if kvm_tdp_mmu_zap_invalidated_roots
> is already set, and is only processed by kvm_tdp_mmu_zap_invalidated_roots(),
> which runs under slots_lock (a mutex).
> 
> So a new, valid root can be added, but it won't have tdp_mmu_scheduled_root_to_zap
> set, at least not until the current "fast zap" completes and a new one beings,
> which as above requires taking mmu_lock for write.

It's clear to me.

Thanks for the detailed explanation.
>