[PATCH RFC v2 0/6] KVM: moving dirty gitmaps to user space!

[PATCH RFC v2 0/6] KVM: moving dirty gitmaps to user space!

[Takuya Yoshikawa]

Hi, this is the v2 of the "moving dirty gitmaps to user space!"

By this patch, I think everything we need becomes clear.
So we want to step forward to be ready for the final version in the
near future: of course, this is dependent on x86 and ppc asm issues.

BTW, by whom I can get ACK for ppc and ia64? I want to add to the Cc
list if possible, thank you.


Patch1: introduce slot level dirty state management
  This patch is independent from other patches and seems to be
  useful without the following parts.

Patch2: introduce wrapper functions to create and destroy dirty bitmaps
  Cleanup patch.

Patch3: introduce a wrapper function to copy dirty bitmaps to user space
  This is for dealing copy_in_user() things cleanly.

Patch4: change mark_page_dirty() to handle endian issues explicitly
  Later, __set_bit() part will be replaced with *_user function.

Patch5: moving dirty bitmaps to user space
  Replace dirty bitmap manipulations with *_user functions.

Patch6: introduce a new API for getting dirty bitmaps
  This is to access dirty bitmaps from user space.


Changelog:
 - suport for all architectures
   We have achived this without pinning.
 - one possible API suggestion
 - temporary copy_in_user like function
 - temporary set_bit_user like function with __get_user() and __put_user()
   We can use this as a generic set_bit_user_non_atomic().
   Of course, we need to optimize this part with arch specific one: we are
   testing some versions for x86 now.

What we are thinking about:
 - about set_bit_user_non_atomic()
   We noticed that ia64 won't need this: see patch1 and patch5.
   So all we have to do is to complete the implementations for x86 and ppc.
   ** x86 and ppc don't include asm-generic uaccess. So we have to put these
      into them separately.

 - about the new api
   There are many possible styles to make use of this work.
   E.g. if we export the both addresses of the two bitmaps,
   we don't need to export them at the switch timing: but we cannot
   reuse the current structures in this case. Which is better?


=
Appendix:

To test the patch 6, we are using the following patch for qemu-kvm.
---
 configure  |    2 +-
 qemu-kvm.c |   22 +++++++++++++++++-----
 2 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/configure b/configure
index be8dac4..0b2d017 100755
--- a/configure
+++ b/configure
@@ -1498,7 +1498,7 @@ fi
 if test "$kvm" != "no" ; then
     cat > $TMPC <<EOF
 #include <linux/kvm.h>
-#if !defined(KVM_API_VERSION) || KVM_API_VERSION < 12 || KVM_API_VERSION > 12
+#if !defined(KVM_API_VERSION) || KVM_API_VERSION < 13 || KVM_API_VERSION > 13
 #error Invalid KVM version
 #endif
 #if !defined(KVM_CAP_USER_MEMORY)
diff --git a/qemu-kvm.c b/qemu-kvm.c
index cc5b352..087adea 100644
--- a/qemu-kvm.c
+++ b/qemu-kvm.c
@@ -44,7 +44,7 @@
 #define BUS_MCEERR_AO 5
 #endif
 
-#define EXPECTED_KVM_API_VERSION 12
+#define EXPECTED_KVM_API_VERSION 13
 
 #if EXPECTED_KVM_API_VERSION != KVM_API_VERSION
 #error libkvm: userspace and kernel version mismatch
@@ -684,6 +684,21 @@ static int kvm_get_map(kvm_context_t kvm, int ioctl_num, int slot, void *buf)
     return 0;
 }
 
+static int kvm_switch_map(kvm_context_t kvm, int slot, void **buf)
+{
+    int r;
+    struct kvm_dirty_log log = {
+        .slot = slot,
+    };
+
+    r = kvm_vm_ioctl(kvm_state, KVM_SWITCH_DIRTY_LOG, &log);
+    if (r < 0)
+        return r;
+
+    *buf = (void *)log.addr;
+    return 0;
+}
+
 int kvm_get_dirty_pages(kvm_context_t kvm, unsigned long phys_addr, void *buf)
 {
     int slot;
@@ -706,14 +721,11 @@ int kvm_get_dirty_pages_range(kvm_context_t kvm, unsigned long phys_addr,
     for (i = 0; i < KVM_MAX_NUM_MEM_REGIONS; ++i) {
         if ((slots[i].len && (uint64_t) slots[i].phys_addr >= phys_addr)
             && ((uint64_t) slots[i].phys_addr + slots[i].len <= end_addr)) {
-            buf = qemu_malloc(BITMAP_SIZE(slots[i].len));
-            r = kvm_get_map(kvm, KVM_GET_DIRTY_LOG, i, buf);
+            r = kvm_switch_map(kvm, i, &buf);
             if (r) {
-                qemu_free(buf);
                 return r;
             }
             r = cb(slots[i].phys_addr, slots[i].len, buf, opaque);
-            qemu_free(buf);
             if (r)
                 return r;
         }
-- 
1.6.3.3

Re: [PATCH RFC v2 0/6] KVM: moving dirty gitmaps to user space!

[Alexander Graf]

On 20.04.2010, at 12:53, Takuya Yoshikawa wrote:

> Hi, this is the v2 of the "moving dirty gitmaps to user space!"
> 
> By this patch, I think everything we need becomes clear.
> So we want to step forward to be ready for the final version in the
> near future: of course, this is dependent on x86 and ppc asm issues.
> 
> BTW, by whom I can get ACK for ppc and ia64? I want to add to the Cc
> list if possible, thank you.

You'd get the ACK for ppc from me.

Do you have a PowerPC box around to test things on? By now it should work on any desktop/server PowerPC system you can find.

Also the only differences on PPC are that we are big endian and usually run 32 bit userspace on 64 bit kernels. So playing with longs is a bad idea :).

Alex

Re: [PATCH RFC v2 0/6] KVM: moving dirty gitmaps to user space!

[Takuya Yoshikawa]

(2010/04/20 19:54), Alexander Graf wrote:
>
> On 20.04.2010, at 12:53, Takuya Yoshikawa wrote:
>
>> Hi, this is the v2 of the "moving dirty gitmaps to user space!"
>>
>> By this patch, I think everything we need becomes clear.
>> So we want to step forward to be ready for the final version in the
>> near future: of course, this is dependent on x86 and ppc asm issues.
>>
>> BTW, by whom I can get ACK for ppc and ia64? I want to add to the Cc
>> list if possible, thank you.
>
> You'd get the ACK for ppc from me.

Thank you, then I will add you to Cc next time!

>
> Do you have a PowerPC box around to test things on? By now it should work on any desktop/server PowerPC system you can find.

Sorry, I have no ppc box here. So at this stage, I am sometimes using cross
compilers to check this can build or not: not enough test now.

And this patch needs arch specific(assembly level) optimization in the future.
So I want to make this development as open as possible.

>
> Also the only differences on PPC are that we are big endian and usually run 32 bit userspace on 64 bit kernels. So playing with longs is a bad idea :).

OK, I'll keep in mind!

>
> Alex
>

   Takuya

Re: [PATCH RFC v2 0/6] KVM: moving dirty gitmaps to user space!

[Takuya Yoshikawa]

Fernando, sorry I have changed some part of this series and forgot to
change your Signed-off-by to Cc for some parts.

So please give me any comments(objections) as replies to this mail thread.


Thanks,
   Takuya


(2010/04/20 19:53), Takuya Yoshikawa wrote:
> Hi, this is the v2 of the "moving dirty gitmaps to user space!"
>
> By this patch, I think everything we need becomes clear.
> So we want to step forward to be ready for the final version in the
> near future: of course, this is dependent on x86 and ppc asm issues.
>
> BTW, by whom I can get ACK for ppc and ia64? I want to add to the Cc
> list if possible, thank you.
>
>
> Patch1: introduce slot level dirty state management
>    This patch is independent from other patches and seems to be
>    useful without the following parts.
>
> Patch2: introduce wrapper functions to create and destroy dirty bitmaps
>    Cleanup patch.
>
> Patch3: introduce a wrapper function to copy dirty bitmaps to user space
>    This is for dealing copy_in_user() things cleanly.
>
> Patch4: change mark_page_dirty() to handle endian issues explicitly
>    Later, __set_bit() part will be replaced with *_user function.
>
> Patch5: moving dirty bitmaps to user space
>    Replace dirty bitmap manipulations with *_user functions.
>
> Patch6: introduce a new API for getting dirty bitmaps
>    This is to access dirty bitmaps from user space.
>
>
> Changelog:
>   - suport for all architectures
>     We have achived this without pinning.
>   - one possible API suggestion
>   - temporary copy_in_user like function
>   - temporary set_bit_user like function with __get_user() and __put_user()
>     We can use this as a generic set_bit_user_non_atomic().
>     Of course, we need to optimize this part with arch specific one: we are
>     testing some versions for x86 now.
>
> What we are thinking about:
>   - about set_bit_user_non_atomic()
>     We noticed that ia64 won't need this: see patch1 and patch5.
>     So all we have to do is to complete the implementations for x86 and ppc.
>     ** x86 and ppc don't include asm-generic uaccess. So we have to put these
>        into them separately.
>
>   - about the new api
>     There are many possible styles to make use of this work.
>     E.g. if we export the both addresses of the two bitmaps,
>     we don't need to export them at the switch timing: but we cannot
>     reuse the current structures in this case. Which is better?
>
>
> =>
> Appendix:
>
> To test the patch 6, we are using the following patch for qemu-kvm.
> ---
>   configure  |    2 +-
>   qemu-kvm.c |   22 +++++++++++++++++-----
>   2 files changed, 18 insertions(+), 6 deletions(-)
>
> diff --git a/configure b/configure
> index be8dac4..0b2d017 100755
> --- a/configure
> +++ b/configure
> @@ -1498,7 +1498,7 @@ fi
>   if test "$kvm" != "no" ; then
>       cat>  $TMPC<<EOF
>   #include<linux/kvm.h>
> -#if !defined(KVM_API_VERSION) || KVM_API_VERSION<  12 || KVM_API_VERSION>  12
> +#if !defined(KVM_API_VERSION) || KVM_API_VERSION<  13 || KVM_API_VERSION>  13
>   #error Invalid KVM version
>   #endif
>   #if !defined(KVM_CAP_USER_MEMORY)
> diff --git a/qemu-kvm.c b/qemu-kvm.c
> index cc5b352..087adea 100644
> --- a/qemu-kvm.c
> +++ b/qemu-kvm.c
> @@ -44,7 +44,7 @@
>   #define BUS_MCEERR_AO 5
>   #endif
>
> -#define EXPECTED_KVM_API_VERSION 12
> +#define EXPECTED_KVM_API_VERSION 13
>
>   #if EXPECTED_KVM_API_VERSION != KVM_API_VERSION
>   #error libkvm: userspace and kernel version mismatch
> @@ -684,6 +684,21 @@ static int kvm_get_map(kvm_context_t kvm, int ioctl_num, int slot, void *buf)
>       return 0;
>   }
>
> +static int kvm_switch_map(kvm_context_t kvm, int slot, void **buf)
> +{
> +    int r;
> +    struct kvm_dirty_log log = {
> +        .slot = slot,
> +    };
> +
> +    r = kvm_vm_ioctl(kvm_state, KVM_SWITCH_DIRTY_LOG,&log);
> +    if (r<  0)
> +        return r;
> +
> +    *buf = (void *)log.addr;
> +    return 0;
> +}
> +
>   int kvm_get_dirty_pages(kvm_context_t kvm, unsigned long phys_addr, void *buf)
>   {
>       int slot;
> @@ -706,14 +721,11 @@ int kvm_get_dirty_pages_range(kvm_context_t kvm, unsigned long phys_addr,
>       for (i = 0; i<  KVM_MAX_NUM_MEM_REGIONS; ++i) {
>           if ((slots[i].len&&  (uint64_t) slots[i].phys_addr>= phys_addr)
>               &&  ((uint64_t) slots[i].phys_addr + slots[i].len<= end_addr)) {
> -            buf = qemu_malloc(BITMAP_SIZE(slots[i].len));
> -            r = kvm_get_map(kvm, KVM_GET_DIRTY_LOG, i, buf);
> +            r = kvm_switch_map(kvm, i,&buf);
>               if (r) {
> -                qemu_free(buf);
>                   return r;
>               }
>               r = cb(slots[i].phys_addr, slots[i].len, buf, opaque);
> -            qemu_free(buf);
>               if (r)
>                   return r;
>           }