[RFC][PATCH RFC 10/12] KVM: move dirty bitmaps to user space

[RFC][PATCH RFC 10/12] KVM: move dirty bitmaps to user space

[Takuya Yoshikawa]

We move dirty bitmaps to user space.

 - Allocation and destruction: we use do_mmap() and do_munmap().
   The new bitmap space is twice longer than the original one and we
   use the additional space for double buffering: this makes it
   possible to update the active bitmap while letting the user space
   read the other one safely. For x86, we can also remove the vmalloc()
   in kvm_vm_ioctl_get_dirty_log().

 - Bitmap manipulations: we replace all functions which access dirty
   bitmaps with *_user() functions.

 - For ia64: moving the dirty bitmaps of memory slots does not affect
   ia64 much because it's using a different place to store dirty logs
   rather than the dirty bitmaps of memory slots: all we have to change
   are sync and get of dirty log, so we don't need set_bit_user like
   functions for ia64.

Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
Signed-off-by: Fernando Luis Vazquez Cao <fernando@oss.ntt.co.jp>
CC: Avi Kivity <avi@redhat.com>
CC: Alexander Graf <agraf@suse.de>
---
 arch/ia64/kvm/kvm-ia64.c  |   15 +++++++++-
 arch/powerpc/kvm/book3s.c |    5 +++-
 arch/x86/kvm/x86.c        |   25 ++++++++----------
 include/linux/kvm_host.h  |    3 +-
 virt/kvm/kvm_main.c       |   62 +++++++++++++++++++++++++++++++++++++-------
 5 files changed, 82 insertions(+), 28 deletions(-)

diff --git a/arch/ia64/kvm/kvm-ia64.c b/arch/ia64/kvm/kvm-ia64.c
index 17fd65c..03503e6 100644
--- a/arch/ia64/kvm/kvm-ia64.c
+++ b/arch/ia64/kvm/kvm-ia64.c
@@ -1823,11 +1823,19 @@ static int kvm_ia64_sync_dirty_log(struct kvm *kvm,
 	n = kvm_dirty_bitmap_bytes(memslot);
 	base = memslot->base_gfn / BITS_PER_LONG;
 
+	r = -EFAULT;
+	if (!access_ok(VERIFY_WRITE, memslot->dirty_bitmap, n))
+		goto out;
+
 	for (i = 0; i < n/sizeof(long); ++i) {
 		if (dirty_bitmap[base + i])
 			memslot->is_dirty = true;
 
-		memslot->dirty_bitmap[i] = dirty_bitmap[base + i];
+		if (__put_user(dirty_bitmap[base + i],
+			       &memslot->dirty_bitmap[i])) {
+			r = -EFAULT;
+			goto out;
+		}
 		dirty_bitmap[base + i] = 0;
 	}
 	r = 0;
@@ -1858,7 +1866,10 @@ int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
 	if (memslot->is_dirty) {
 		kvm_flush_remote_tlbs(kvm);
 		n = kvm_dirty_bitmap_bytes(memslot);
-		memset(memslot->dirty_bitmap, 0, n);
+		if (clear_user(memslot->dirty_bitmap, n)) {
+			r = -EFAULT;
+			goto out;
+		}
 		memslot->is_dirty = false;
 	}
 	r = 0;
diff --git a/arch/powerpc/kvm/book3s.c b/arch/powerpc/kvm/book3s.c
index 4b074f1..2a31d2f 100644
--- a/arch/powerpc/kvm/book3s.c
+++ b/arch/powerpc/kvm/book3s.c
@@ -1210,7 +1210,10 @@ int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
 			kvmppc_mmu_pte_pflush(vcpu, ga, ga_end);
 
 		n = kvm_dirty_bitmap_bytes(memslot);
-		memset(memslot->dirty_bitmap, 0, n);
+		if (clear_user(memslot->dirty_bitmap, n)) {
+			r = -EFAULT;
+			goto out;
+		}
 		memslot->is_dirty = false;
 	}
 
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 023c7f8..32a3d94 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2760,40 +2760,37 @@ int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
 	/* If nothing is dirty, don't bother messing with page tables. */
 	if (memslot->is_dirty) {
 		struct kvm_memslots *slots, *old_slots;
-		unsigned long *dirty_bitmap;
+		unsigned long __user *dirty_bitmap;
+		unsigned long __user *dirty_bitmap_old;
 
 		spin_lock(&kvm->mmu_lock);
 		kvm_mmu_slot_remove_write_access(kvm, log->slot);
 		spin_unlock(&kvm->mmu_lock);
 
-		r = -ENOMEM;
-		dirty_bitmap = vmalloc(n);
-		if (!dirty_bitmap)
+		dirty_bitmap = memslot->dirty_bitmap;
+		dirty_bitmap_old = memslot->dirty_bitmap_old;
+		r = -EFAULT;
+		if (clear_user(dirty_bitmap_old, n))
 			goto out;
-		memset(dirty_bitmap, 0, n);
 
 		r = -ENOMEM;
 		slots = kzalloc(sizeof(struct kvm_memslots), GFP_KERNEL);
-		if (!slots) {
-			vfree(dirty_bitmap);
+		if (!slots)
 			goto out;
-		}
+
 		memcpy(slots, kvm->memslots, sizeof(struct kvm_memslots));
-		slots->memslots[log->slot].dirty_bitmap = dirty_bitmap;
+		slots->memslots[log->slot].dirty_bitmap = dirty_bitmap_old;
+		slots->memslots[log->slot].dirty_bitmap_old = dirty_bitmap;
 		slots->memslots[log->slot].is_dirty = false;
 
 		old_slots = kvm->memslots;
 		rcu_assign_pointer(kvm->memslots, slots);
 		synchronize_srcu_expedited(&kvm->srcu);
-		dirty_bitmap = old_slots->memslots[log->slot].dirty_bitmap;
 		kfree(old_slots);
 
 		r = -EFAULT;
-		if (copy_to_user(log->dirty_bitmap, dirty_bitmap, n)) {
-			vfree(dirty_bitmap);
+		if (copy_in_user(log->dirty_bitmap, dirty_bitmap, n))
 			goto out;
-		}
-		vfree(dirty_bitmap);
 	} else {
 		r = -EFAULT;
 		if (clear_user(log->dirty_bitmap, n))
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 0aa6ecb..c95e2b7 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -116,7 +116,8 @@ struct kvm_memory_slot {
 	unsigned long npages;
 	unsigned long flags;
 	unsigned long *rmap;
-	unsigned long *dirty_bitmap;
+	unsigned long __user *dirty_bitmap;
+	unsigned long __user *dirty_bitmap_old;
 	bool is_dirty;
 	struct {
 		unsigned long rmap_pde;
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 3e3acad..ddcf65a 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -437,8 +437,20 @@ out_err_nodisable:
 
 static void kvm_destroy_dirty_bitmap(struct kvm_memory_slot *memslot)
 {
-	vfree(memslot->dirty_bitmap);
+	unsigned long user_addr;
+	unsigned long n = kvm_dirty_bitmap_bytes(memslot);
+
+	if (!memslot->dirty_bitmap)
+		return;
+
+	user_addr = min((unsigned long)memslot->dirty_bitmap,
+			(unsigned long)memslot->dirty_bitmap_old);
+	down_write(&current->mm->mmap_sem);
+	do_munmap(current->mm, user_addr, 2 * n);
+	up_write(&current->mm->mmap_sem);
+
 	memslot->dirty_bitmap = NULL;
+	memslot->dirty_bitmap_old = NULL;
 }
 
 /*
@@ -472,8 +484,12 @@ void kvm_free_physmem(struct kvm *kvm)
 	int i;
 	struct kvm_memslots *slots = kvm->memslots;
 
-	for (i = 0; i < slots->nmemslots; ++i)
+	for (i = 0; i < slots->nmemslots; ++i) {
+		/* VM process will exit: we don't unmap by ourselves. */
+		slots->memslots[i].dirty_bitmap = NULL;
+		slots->memslots[i].dirty_bitmap_old = NULL;
 		kvm_free_physmem_slot(&slots->memslots[i], NULL);
+	}
 
 	kfree(kvm->memslots);
 }
@@ -527,14 +543,35 @@ static int kvm_vm_release(struct inode *inode, struct file *filp)
 
 static int kvm_create_dirty_bitmap(struct kvm_memory_slot *memslot)
 {
-	unsigned long dirty_bytes = kvm_dirty_bitmap_bytes(memslot);
+	int err;
+	unsigned long user_addr;
+	unsigned long n = kvm_dirty_bitmap_bytes(memslot);
 
-	memslot->dirty_bitmap = vmalloc(dirty_bytes);
-	if (!memslot->dirty_bitmap)
-		return -ENOMEM;
+	down_write(&current->mm->mmap_sem);
+	user_addr = do_mmap(NULL, 0, 2 * n,
+			    PROT_READ | PROT_WRITE,
+			    MAP_PRIVATE | MAP_ANONYMOUS, 0);
+	up_write(&current->mm->mmap_sem);
+
+	if (IS_ERR((void *)user_addr)) {
+		err = PTR_ERR((void *)user_addr);
+		goto out;
+	}
+
+	memslot->dirty_bitmap = (unsigned long __user *)user_addr;
+	memslot->dirty_bitmap_old = (unsigned long __user *)(user_addr + n);
+	if (clear_user(memslot->dirty_bitmap, 2 * n)) {
+		err = -EFAULT;
+		goto out_unmap;
+	}
 
-	memset(memslot->dirty_bitmap, 0, dirty_bytes);
 	return 0;
+out_unmap:
+	down_write(&current->mm->mmap_sem);
+	do_munmap(current->mm, user_addr, 2 * n);
+	up_write(&current->mm->mmap_sem);
+out:
+	return err;
 }
 
 /*
@@ -799,7 +836,7 @@ int kvm_get_dirty_log(struct kvm *kvm, struct kvm_dirty_log *log)
 	n = kvm_dirty_bitmap_bytes(memslot);
 
 	r = -EFAULT;
-	if (copy_to_user(log->dirty_bitmap, memslot->dirty_bitmap, n))
+	if (copy_in_user(log->dirty_bitmap, memslot->dirty_bitmap, n))
 		goto out;
 
 	r = 0;
@@ -1195,11 +1232,16 @@ void mark_page_dirty(struct kvm *kvm, gfn_t gfn)
 	gfn = unalias_gfn(kvm, gfn);
 	memslot = gfn_to_memslot_unaliased(kvm, gfn);
 	if (memslot && memslot->dirty_bitmap) {
-		unsigned long rel_gfn = gfn - memslot->base_gfn;
+		int nr = generic_le_bit_offset(gfn - memslot->base_gfn);
 
-		generic___set_le_bit(rel_gfn, memslot->dirty_bitmap);
+		if (kvm_set_bit_user(nr, memslot->dirty_bitmap))
+			goto out_fault;
 		memslot->is_dirty = true;
 	}
+	return;
+
+out_fault:
+	printk(KERN_WARNING "%s: kvm_set_bit_user failed.\n", __func__);
 }
 
 /*
-- 
1.7.0.4

Re: [RFC][PATCH RFC 10/12] KVM: move dirty bitmaps to user space

[Marcelo Tosatti]

On Tue, May 04, 2010 at 10:07:02PM +0900, Takuya Yoshikawa wrote:
> We move dirty bitmaps to user space.
> 
>  - Allocation and destruction: we use do_mmap() and do_munmap().
>    The new bitmap space is twice longer than the original one and we
>    use the additional space for double buffering: this makes it
>    possible to update the active bitmap while letting the user space
>    read the other one safely. For x86, we can also remove the vmalloc()
>    in kvm_vm_ioctl_get_dirty_log().
> 
>  - Bitmap manipulations: we replace all functions which access dirty
>    bitmaps with *_user() functions.
> 
>  - For ia64: moving the dirty bitmaps of memory slots does not affect
>    ia64 much because it's using a different place to store dirty logs
>    rather than the dirty bitmaps of memory slots: all we have to change
>    are sync and get of dirty log, so we don't need set_bit_user like
>    functions for ia64.
> 
> Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
> Signed-off-by: Fernando Luis Vazquez Cao <fernando@oss.ntt.co.jp>
> CC: Avi Kivity <avi@redhat.com>
> CC: Alexander Graf <agraf@suse.de>
> ---
>  arch/ia64/kvm/kvm-ia64.c  |   15 +++++++++-
>  arch/powerpc/kvm/book3s.c |    5 +++-
>  arch/x86/kvm/x86.c        |   25 ++++++++----------
>  include/linux/kvm_host.h  |    3 +-
>  virt/kvm/kvm_main.c       |   62 +++++++++++++++++++++++++++++++++++++-------
>  5 files changed, 82 insertions(+), 28 deletions(-)
> 
> diff --git a/arch/ia64/kvm/kvm-ia64.c b/arch/ia64/kvm/kvm-ia64.c
> index 17fd65c..03503e6 100644
> --- a/arch/ia64/kvm/kvm-ia64.c
> +++ b/arch/ia64/kvm/kvm-ia64.c
> @@ -1823,11 +1823,19 @@ static int kvm_ia64_sync_dirty_log(struct kvm *kvm,
>  	n = kvm_dirty_bitmap_bytes(memslot);
>  	base = memslot->base_gfn / BITS_PER_LONG;
>  
> +	r = -EFAULT;
> +	if (!access_ok(VERIFY_WRITE, memslot->dirty_bitmap, n))
> +		goto out;
> +
>  	for (i = 0; i < n/sizeof(long); ++i) {
>  		if (dirty_bitmap[base + i])
>  			memslot->is_dirty = true;
>  
> -		memslot->dirty_bitmap[i] = dirty_bitmap[base + i];
> +		if (__put_user(dirty_bitmap[base + i],
> +			       &memslot->dirty_bitmap[i])) {
> +			r = -EFAULT;
> +			goto out;
> +		}
>  		dirty_bitmap[base + i] = 0;
>  	}
>  	r = 0;
> @@ -1858,7 +1866,10 @@ int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
>  	if (memslot->is_dirty) {
>  		kvm_flush_remote_tlbs(kvm);
>  		n = kvm_dirty_bitmap_bytes(memslot);
> -		memset(memslot->dirty_bitmap, 0, n);
> +		if (clear_user(memslot->dirty_bitmap, n)) {
> +			r = -EFAULT;
> +			goto out;
> +		}
>  		memslot->is_dirty = false;
>  	}
>  	r = 0;
> diff --git a/arch/powerpc/kvm/book3s.c b/arch/powerpc/kvm/book3s.c
> index 4b074f1..2a31d2f 100644
> --- a/arch/powerpc/kvm/book3s.c
> +++ b/arch/powerpc/kvm/book3s.c
> @@ -1210,7 +1210,10 @@ int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
>  			kvmppc_mmu_pte_pflush(vcpu, ga, ga_end);
>  
>  		n = kvm_dirty_bitmap_bytes(memslot);
> -		memset(memslot->dirty_bitmap, 0, n);
> +		if (clear_user(memslot->dirty_bitmap, n)) {
> +			r = -EFAULT;
> +			goto out;
> +		}
>  		memslot->is_dirty = false;
>  	}
>  
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 023c7f8..32a3d94 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -2760,40 +2760,37 @@ int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
>  	/* If nothing is dirty, don't bother messing with page tables. */
>  	if (memslot->is_dirty) {
>  		struct kvm_memslots *slots, *old_slots;
> -		unsigned long *dirty_bitmap;
> +		unsigned long __user *dirty_bitmap;
> +		unsigned long __user *dirty_bitmap_old;
>  
>  		spin_lock(&kvm->mmu_lock);
>  		kvm_mmu_slot_remove_write_access(kvm, log->slot);
>  		spin_unlock(&kvm->mmu_lock);
>  
> -		r = -ENOMEM;
> -		dirty_bitmap = vmalloc(n);
> -		if (!dirty_bitmap)
> +		dirty_bitmap = memslot->dirty_bitmap;
> +		dirty_bitmap_old = memslot->dirty_bitmap_old;
> +		r = -EFAULT;
> +		if (clear_user(dirty_bitmap_old, n))
>  			goto out;
> -		memset(dirty_bitmap, 0, n);
>  
>  		r = -ENOMEM;
>  		slots = kzalloc(sizeof(struct kvm_memslots), GFP_KERNEL);
> -		if (!slots) {
> -			vfree(dirty_bitmap);
> +		if (!slots)
>  			goto out;
> -		}
> +
>  		memcpy(slots, kvm->memslots, sizeof(struct kvm_memslots));
> -		slots->memslots[log->slot].dirty_bitmap = dirty_bitmap;
> +		slots->memslots[log->slot].dirty_bitmap = dirty_bitmap_old;
> +		slots->memslots[log->slot].dirty_bitmap_old = dirty_bitmap;
>  		slots->memslots[log->slot].is_dirty = false;
>  
>  		old_slots = kvm->memslots;
>  		rcu_assign_pointer(kvm->memslots, slots);
>  		synchronize_srcu_expedited(&kvm->srcu);
> -		dirty_bitmap = old_slots->memslots[log->slot].dirty_bitmap;
>  		kfree(old_slots);
>  
>  		r = -EFAULT;
> -		if (copy_to_user(log->dirty_bitmap, dirty_bitmap, n)) {
> -			vfree(dirty_bitmap);
> +		if (copy_in_user(log->dirty_bitmap, dirty_bitmap, n))
>  			goto out;
> -		}
> -		vfree(dirty_bitmap);
>  	} else {
>  		r = -EFAULT;
>  		if (clear_user(log->dirty_bitmap, n))
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 0aa6ecb..c95e2b7 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -116,7 +116,8 @@ struct kvm_memory_slot {
>  	unsigned long npages;
>  	unsigned long flags;
>  	unsigned long *rmap;
> -	unsigned long *dirty_bitmap;
> +	unsigned long __user *dirty_bitmap;
> +	unsigned long __user *dirty_bitmap_old;
>  	bool is_dirty;
>  	struct {
>  		unsigned long rmap_pde;
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 3e3acad..ddcf65a 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -437,8 +437,20 @@ out_err_nodisable:
>  
>  static void kvm_destroy_dirty_bitmap(struct kvm_memory_slot *memslot)
>  {
> -	vfree(memslot->dirty_bitmap);
> +	unsigned long user_addr;
> +	unsigned long n = kvm_dirty_bitmap_bytes(memslot);
> +
> +	if (!memslot->dirty_bitmap)
> +		return;
> +
> +	user_addr = min((unsigned long)memslot->dirty_bitmap,
> +			(unsigned long)memslot->dirty_bitmap_old);
> +	down_write(&current->mm->mmap_sem);
> +	do_munmap(current->mm, user_addr, 2 * n);
> +	up_write(&current->mm->mmap_sem);
> +
>  	memslot->dirty_bitmap = NULL;
> +	memslot->dirty_bitmap_old = NULL;
>  }
>  
>  /*
> @@ -472,8 +484,12 @@ void kvm_free_physmem(struct kvm *kvm)
>  	int i;
>  	struct kvm_memslots *slots = kvm->memslots;
>  
> -	for (i = 0; i < slots->nmemslots; ++i)
> +	for (i = 0; i < slots->nmemslots; ++i) {
> +		/* VM process will exit: we don't unmap by ourselves. */
> +		slots->memslots[i].dirty_bitmap = NULL;
> +		slots->memslots[i].dirty_bitmap_old = NULL;
>  		kvm_free_physmem_slot(&slots->memslots[i], NULL);
> +	}
>  
>  	kfree(kvm->memslots);
>  }
> @@ -527,14 +543,35 @@ static int kvm_vm_release(struct inode *inode, struct file *filp)
>  
>  static int kvm_create_dirty_bitmap(struct kvm_memory_slot *memslot)
>  {
> -	unsigned long dirty_bytes = kvm_dirty_bitmap_bytes(memslot);
> +	int err;
> +	unsigned long user_addr;
> +	unsigned long n = kvm_dirty_bitmap_bytes(memslot);
>  
> -	memslot->dirty_bitmap = vmalloc(dirty_bytes);
> -	if (!memslot->dirty_bitmap)
> -		return -ENOMEM;
> +	down_write(&current->mm->mmap_sem);
> +	user_addr = do_mmap(NULL, 0, 2 * n,
> +			    PROT_READ | PROT_WRITE,
> +			    MAP_PRIVATE | MAP_ANONYMOUS, 0);
> +	up_write(&current->mm->mmap_sem);
> +
> +	if (IS_ERR((void *)user_addr)) {
> +		err = PTR_ERR((void *)user_addr);
> +		goto out;
> +	}
> +
> +	memslot->dirty_bitmap = (unsigned long __user *)user_addr;
> +	memslot->dirty_bitmap_old = (unsigned long __user *)(user_addr + n);
> +	if (clear_user(memslot->dirty_bitmap, 2 * n)) {
> +		err = -EFAULT;
> +		goto out_unmap;
> +	}
>  
> -	memset(memslot->dirty_bitmap, 0, dirty_bytes);
>  	return 0;
> +out_unmap:
> +	down_write(&current->mm->mmap_sem);
> +	do_munmap(current->mm, user_addr, 2 * n);
> +	up_write(&current->mm->mmap_sem);
> +out:
> +	return err;
>  }
>  
>  /*
> @@ -799,7 +836,7 @@ int kvm_get_dirty_log(struct kvm *kvm, struct kvm_dirty_log *log)
>  	n = kvm_dirty_bitmap_bytes(memslot);
>  
>  	r = -EFAULT;
> -	if (copy_to_user(log->dirty_bitmap, memslot->dirty_bitmap, n))
> +	if (copy_in_user(log->dirty_bitmap, memslot->dirty_bitmap, n))
>  		goto out;
>  
>  	r = 0;
> @@ -1195,11 +1232,16 @@ void mark_page_dirty(struct kvm *kvm, gfn_t gfn)
>  	gfn = unalias_gfn(kvm, gfn);
>  	memslot = gfn_to_memslot_unaliased(kvm, gfn);
>  	if (memslot && memslot->dirty_bitmap) {
> -		unsigned long rel_gfn = gfn - memslot->base_gfn;
> +		int nr = generic_le_bit_offset(gfn - memslot->base_gfn);
>  
> -		generic___set_le_bit(rel_gfn, memslot->dirty_bitmap);
> +		if (kvm_set_bit_user(nr, memslot->dirty_bitmap))
> +			goto out_fault;

mark_page_dirty is called with the mmu_lock spinlock held in set_spte.
Must find a way to move it outside of the spinlock section.

Re: [RFC][PATCH RFC 10/12] KVM: move dirty bitmaps to user space

[Takuya Yoshikawa]

>>   	r = 0;
>> @@ -1195,11 +1232,16 @@ void mark_page_dirty(struct kvm *kvm, gfn_t gfn)
>>   	gfn = unalias_gfn(kvm, gfn);
>>   	memslot = gfn_to_memslot_unaliased(kvm, gfn);
>>   	if (memslot&&  memslot->dirty_bitmap) {
>> -		unsigned long rel_gfn = gfn - memslot->base_gfn;
>> +		int nr = generic_le_bit_offset(gfn - memslot->base_gfn);
>>
>> -		generic___set_le_bit(rel_gfn, memslot->dirty_bitmap);
>> +		if (kvm_set_bit_user(nr, memslot->dirty_bitmap))
>> +			goto out_fault;
>
> mark_page_dirty is called with the mmu_lock spinlock held in set_spte.
> Must find a way to move it outside of the spinlock section.
>

Oh, it's a serious problem. I have to consider it.


Thanks,
   Takuya

Re: [RFC][PATCH RFC 10/12] KVM: move dirty bitmaps to user space

[Takuya Yoshikawa]

>>
>> mark_page_dirty is called with the mmu_lock spinlock held in set_spte.
>> Must find a way to move it outside of the spinlock section.
>>
>
> Oh, it's a serious problem. I have to consider it.

Avi, Marcelo,

Sorry but I have to say that mmu_lock spin_lock problem was completely out of
my mind. Although I looked through the code, it seems not easy to move the
set_bit_user to outside of spinlock section without breaking the semantics of
its protection.

So this may take some time to solve.

But personally, I want to do something for x86's "vmallc() every time" problem
even though moving dirty bitmaps to user space cannot be achieved soon.

In that sense, do you mind if we do double buffering without moving dirty bitmaps to
user space?

I know that the resource for vmalloc() is precious for x86 but even now, at the timing
of get_dirty_log, we use the same amount of memory as double buffering.


Thanks,
   Takuya