[PATCH RFC v2 3/6] KVM: introduce a wrapper function to copy dirty

[PATCH RFC v2 3/6] KVM: introduce a wrapper function to copy dirty

[Takuya Yoshikawa]

We will replace copy_to_user() to copy_in_user() when we move
the dirty bitmaps to user space.

But sadly, we have copy_in_user() only for 64 bits architectures.
So this function should work as a wrapper to hide ifdefs from outside.
Once we get copy_in_user() for 32 bits architectures, we can remove
this wrapper and use copy_in_user() directly.

Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
Signed-off-by: Fernando Luis Vazquez Cao <fernando@oss.ntt.co.jp>
---
 arch/x86/kvm/x86.c       |    4 +---
 include/linux/kvm_host.h |    3 +++
 virt/kvm/kvm_main.c      |   12 +++++++++++-
 3 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 76b23ed..6f0b706 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2771,9 +2771,7 @@ int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
 		kfree(old_slots);
 	}
 
-	r = 0;
-	if (copy_to_user(log->dirty_bitmap, dirty_bitmap, n))
-		r = -EFAULT;
+	r = kvm_copy_dirty_bitmap(log->dirty_bitmap, dirty_bitmap, n);
 out_free:
 	vfree(dirty_bitmap);
 out:
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 4446622..42b7161 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -330,6 +330,9 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
 
 int kvm_dev_ioctl_check_extension(long ext);
 
+int kvm_copy_dirty_bitmap(unsigned long __user *to,
+			  const unsigned long *from,
+			  unsigned long bytes);
 int kvm_get_dirty_log(struct kvm *kvm, struct kvm_dirty_log *log);
 int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
 				struct kvm_dirty_log *log);
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 40a6888..6908304 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -777,6 +777,16 @@ int kvm_vm_ioctl_set_memory_region(struct kvm *kvm,
 	return kvm_set_memory_region(kvm, mem, user_alloc);
 }
 
+int kvm_copy_dirty_bitmap(unsigned long __user *to,
+			  const unsigned long *from,
+			  unsigned long bytes)
+{
+	if (copy_to_user(to, from, bytes))
+		return -EFAULT;
+
+	return 0;
+}
+
 int kvm_get_dirty_log(struct kvm *kvm, struct kvm_dirty_log *log)
 {
 	struct kvm_memory_slot *memslot;
@@ -795,7 +805,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 (kvm_copy_dirty_bitmap(log->dirty_bitmap, memslot->dirty_bitmap, n))
 		goto out;
 
 	r = 0;
-- 
1.6.3.3

Re: [PATCH RFC v2 3/6] KVM: introduce a wrapper function to copy

[Avi Kivity]

On 04/20/2010 01:59 PM, Takuya Yoshikawa wrote:
> We will replace copy_to_user() to copy_in_user() when we move
> the dirty bitmaps to user space.
>
> But sadly, we have copy_in_user() only for 64 bits architectures.
> So this function should work as a wrapper to hide ifdefs from outside.
> Once we get copy_in_user() for 32 bits architectures, we can remove
> this wrapper and use copy_in_user() directly.
>    

I prefer a generic copy_in_user() instead of having multiple paths in kvm.

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

Re: [PATCH RFC v2 3/6] KVM: introduce a wrapper function to copy

[Takuya Yoshikawa]

(2010/04/21 20:12), Avi Kivity wrote:
> On 04/20/2010 01:59 PM, Takuya Yoshikawa wrote:
>> We will replace copy_to_user() to copy_in_user() when we move
>> the dirty bitmaps to user space.
>>
>> But sadly, we have copy_in_user() only for 64 bits architectures.
>> So this function should work as a wrapper to hide ifdefs from outside.
>> Once we get copy_in_user() for 32 bits architectures, we can remove
>> this wrapper and use copy_in_user() directly.
>
> I prefer a generic copy_in_user() instead of having multiple paths in kvm.
>

I do too :).

I checked ppc64's copy_in_user().
It looks like just using __copy_tofrom_user() and it is also implemented
for ppc32, IIUC.

So yes, we may not need to implement it by ourselves!

Just ask them to make it useful for 32bit too.



And about x86_32 copy_in_user().

They are using copy_user_generic() which is implemented only for 64bit.

So I'll show them current copy_from_user() and copy_to_user() version and
see their response.

If rejected, though I hope not, please give me another option, OK?

Re: [PATCH RFC v2 3/6] KVM: introduce a wrapper function to copy

[Avi Kivity]

On 04/22/2010 11:57 AM, Takuya Yoshikawa wrote:
>
> And about x86_32 copy_in_user().
>
> They are using copy_user_generic() which is implemented only for 64bit.
>
> So I'll show them current copy_from_user() and copy_to_user() version and
> see their response.
>
> If rejected, though I hope not, please give me another option, OK?
>

If we can justify it, I doubt it will be rejected, but in case it does, 
we'll work something out.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.