From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Roedel, Joerg" Subject: Re: [PATCH] KVM: MMU: Don't read pdptrs with mmu spinlock held in mmu_alloc_roots Date: Tue, 4 May 2010 12:59:23 +0200 Message-ID: <20100504105923.GG28950@amd.com> References: <1272967430-6191-1-git-send-email-avi@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Cc: "kvm@vger.kernel.org" , Marcelo Tosatti To: Avi Kivity Return-path: Received: from va3ehsobe005.messaging.microsoft.com ([216.32.180.15]:42253 "EHLO VA3EHSOBE006.bigfish.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751106Ab0EDK7f (ORCPT ); Tue, 4 May 2010 06:59:35 -0400 Content-Disposition: inline In-Reply-To: <1272967430-6191-1-git-send-email-avi@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On Tue, May 04, 2010 at 06:03:50AM -0400, Avi Kivity wrote: > On svm, kvm_read_pdptr() may require reading guest memory, which can sleep. > > Push the spinlock into mmu_alloc_roots(), and only take it after we've read > the pdptr. This fixes the lockdep issue for me. Thanks. Tested-by: Joerg Roedel > > Signed-off-by: Avi Kivity > --- > > Marcelo, dropping and re-acquiring the lock before mmu_sync_roots(), is fine, > yes? > > arch/x86/kvm/mmu.c | 7 +++++++ > 1 files changed, 7 insertions(+), 0 deletions(-) > > diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c > index 51eb6d6..de99638 100644 > --- a/arch/x86/kvm/mmu.c > +++ b/arch/x86/kvm/mmu.c > @@ -2065,11 +2065,13 @@ static int mmu_alloc_roots(struct kvm_vcpu *vcpu) > direct = 1; > root_gfn = 0; > } > + spin_lock(&vcpu->kvm->mmu_lock); > sp = kvm_mmu_get_page(vcpu, root_gfn, 0, > PT64_ROOT_LEVEL, direct, > ACC_ALL, NULL); > root = __pa(sp->spt); > ++sp->root_count; > + spin_unlock(&vcpu->kvm->mmu_lock); > vcpu->arch.mmu.root_hpa = root; > return 0; > } > @@ -2093,11 +2095,14 @@ static int mmu_alloc_roots(struct kvm_vcpu *vcpu) > direct = 1; > root_gfn = i << 30; > } > + spin_lock(&vcpu->kvm->mmu_lock); > sp = kvm_mmu_get_page(vcpu, root_gfn, i << 30, > PT32_ROOT_LEVEL, direct, > ACC_ALL, NULL); > root = __pa(sp->spt); > ++sp->root_count; > + spin_unlock(&vcpu->kvm->mmu_lock); > + > vcpu->arch.mmu.pae_root[i] = root | PT_PRESENT_MASK; > } > vcpu->arch.mmu.root_hpa = __pa(vcpu->arch.mmu.pae_root); > @@ -2466,7 +2471,9 @@ int kvm_mmu_load(struct kvm_vcpu *vcpu) > goto out; > spin_lock(&vcpu->kvm->mmu_lock); > kvm_mmu_free_some_pages(vcpu); > + spin_unlock(&vcpu->kvm->mmu_lock); > r = mmu_alloc_roots(vcpu); > + spin_lock(&vcpu->kvm->mmu_lock); > mmu_sync_roots(vcpu); > spin_unlock(&vcpu->kvm->mmu_lock); > if (r)