[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZcJ2JG54O0g07e-P@google.com>
Date: Tue, 6 Feb 2024 10:10:44 -0800
From: Sean Christopherson <seanjc@...gle.com>
To: Xu Yilun <yilun.xu@...ux.intel.com>
Cc: Paolo Bonzini <pbonzini@...hat.com>, kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
David Matlack <dmatlack@...gle.com>, Pattara Teerapong <pteerapong@...gle.com>
Subject: Re: [PATCH 7/8] KVM: x86/mmu: Alloc TDP MMU roots while holding
mmu_lock for read
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.
Powered by blists - more mailing lists