[PATCH] KVM: MMU: Don't read pdptrs with mmu spinlock held in mmu_alloc_roots

[PATCH] KVM: MMU: Don't read pdptrs with mmu spinlock held in mmu_alloc_roots

[Avi Kivity]

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.

Signed-off-by: Avi Kivity <avi@redhat.com>
---

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)
-- 
1.7.0.4

Re: [PATCH] KVM: MMU: Don't read pdptrs with mmu spinlock held in mmu_alloc_roots

[Roedel, Joerg]

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 <joerg.roedel@amd.com>

> 
> Signed-off-by: Avi Kivity <avi@redhat.com>
> ---
> 
> 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)

Re: [PATCH] KVM: MMU: Don't read pdptrs with mmu spinlock held in mmu_alloc_roots

[Marcelo Tosatti]

On Tue, May 04, 2010 at 01:03:50PM +0300, 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.
> 
> Signed-off-by: Avi Kivity <avi@redhat.com>
> ---
> 
> Marcelo, dropping and re-acquiring the lock before mmu_sync_roots(), is fine,
> yes?

Yes, but you should call kvm_mmu_free_some_pages after reacquiring the
spin_lock, to guarantee kvm_mmu_alloc_page won't fail.

Re: [PATCH] KVM: MMU: Don't read pdptrs with mmu spinlock held in mmu_alloc_roots

[Avi Kivity]

On 05/06/2010 10:10 PM, Marcelo Tosatti wrote:
> On Tue, May 04, 2010 at 01:03:50PM +0300, 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.
>>
>> Marcelo, dropping and re-acquiring the lock before mmu_sync_roots(), is fine,
>> yes?
>>      
> Yes, but you should call kvm_mmu_free_some_pages after reacquiring the
> spin_lock, to guarantee kvm_mmu_alloc_page won't fail.
>
>    

We never fail due to lack of free pages (but I agree it's cleaner).

The patch is already in, I'll send an incremental.

-- 
error compiling committee.c: too many arguments to function