[PATCH][1/3] Move kvm_vcpu_iotcl_get_dirty_log to arch

[PATCH][1/3] Move kvm_vcpu_iotcl_get_dirty_log to arch

[Zhang, Xiantao]

[-- Attachment #1: Type: text/plain, Size: 85 bytes --]

Move kvm_vcpu_ioctl_get_dirty_log to arch, and still keep the interface
in common.

[-- Attachment #2: 0001-KVM-portability-Move-kvm_vcpu_ioctl_get_dirty_log-t.patch --]
[-- Type: application/octet-stream, Size: 3334 bytes --]

From e6e73fc14b1f76c1522f1c5fb320d56e6c5f2ec3 Mon Sep 17 00:00:00 2001
From: Zhang Xiantao <xiantao.zhang@intel.com>
Date: Sun, 18 Nov 2007 20:29:43 +0800
Subject: [PATCH] KVM portability: Move kvm_vcpu_ioctl_get_dirty_log to arch file, meanwhile keep the interface in common, and leave as more logic in common as possible.
Signed-off-by: Zhang Xiantao <xiantao.zhang@intel.com>
---
 drivers/kvm/kvm.h      |    5 +++++
 drivers/kvm/kvm_main.c |   19 ++++---------------
 drivers/kvm/x86.c      |   31 +++++++++++++++++++++++++++++++
 3 files changed, 40 insertions(+), 15 deletions(-)

diff --git a/drivers/kvm/kvm.h b/drivers/kvm/kvm.h
index a7be073..c058eb8 100644
--- a/drivers/kvm/kvm.h
+++ b/drivers/kvm/kvm.h
@@ -631,6 +631,11 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu);
 
 int kvm_dev_ioctl_check_extension(long ext);
 
+int kvm_get_dirty_log(struct kvm *kvm,
+			struct kvm_dirty_log *log, int *is_dirty);
+int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
+				struct kvm_dirty_log *log);
+
 int kvm_vm_ioctl_set_memory_region(struct kvm *kvm,
 				   struct
 				   kvm_userspace_memory_region *mem,
diff --git a/drivers/kvm/kvm_main.c b/drivers/kvm/kvm_main.c
index 45b18e1..3400265 100644
--- a/drivers/kvm/kvm_main.c
+++ b/drivers/kvm/kvm_main.c
@@ -419,19 +419,14 @@ int kvm_vm_ioctl_set_memory_region(struct kvm *kvm,
 	return kvm_set_memory_region(kvm, mem, user_alloc);
 }
 
-/*
- * Get (and clear) the dirty memory log for a memory slot.
- */
-static int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
-				      struct kvm_dirty_log *log)
+int kvm_get_dirty_log(struct kvm *kvm,
+			struct kvm_dirty_log *log, int *is_dirty)
 {
 	struct kvm_memory_slot *memslot;
 	int r, i;
 	int n;
 	unsigned long any = 0;
 
-	mutex_lock(&kvm->lock);
-
 	r = -EINVAL;
 	if (log->slot >= KVM_MEMORY_SLOTS)
 		goto out;
@@ -450,17 +445,11 @@ static int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
 	if (copy_to_user(log->dirty_bitmap, memslot->dirty_bitmap, n))
 		goto out;
 
-	/* If nothing is dirty, don't bother messing with page tables. */
-	if (any) {
-		kvm_mmu_slot_remove_write_access(kvm, log->slot);
-		kvm_flush_remote_tlbs(kvm);
-		memset(memslot->dirty_bitmap, 0, n);
-	}
+	if (any)
+		*is_dirty = 1;
 
 	r = 0;
-
 out:
-	mutex_unlock(&kvm->lock);
 	return r;
 }
 
diff --git a/drivers/kvm/x86.c b/drivers/kvm/x86.c
index 6d7f384..8eebd40 100644
--- a/drivers/kvm/x86.c
+++ b/drivers/kvm/x86.c
@@ -926,6 +926,37 @@ static int kvm_vm_ioctl_set_irqchip(struct kvm *kvm, struct kvm_irqchip *chip)
 	return r;
 }
 
+/*
+ * Get (and clear) the dirty memory log for a memory slot.
+ */
+int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
+				      struct kvm_dirty_log *log)
+{
+	int r;
+	int n;
+	struct kvm_memory_slot *memslot;
+	int is_dirty = 0;
+
+	mutex_lock(&kvm->lock);
+
+	r = kvm_get_dirty_log(kvm, log, &is_dirty);
+	if (r)
+		goto out;
+
+	/* If nothing is dirty, don't bother messing with page tables. */
+	if (is_dirty) {
+		kvm_mmu_slot_remove_write_access(kvm, log->slot);
+		kvm_flush_remote_tlbs(kvm);
+		memslot = &kvm->memslots[log->slot];
+		n = ALIGN(memslot->npages, BITS_PER_LONG) / 8;
+		memset(memslot->dirty_bitmap, 0, n);
+	}
+	r = 0;
+out:
+	mutex_unlock(&kvm->lock);
+	return r;
+}
+
 long kvm_arch_vm_ioctl(struct file *filp,
 		       unsigned int ioctl, unsigned long arg)
 {
-- 
1.5.1.2


[-- Attachment #3: Type: text/plain, Size: 228 bytes --]

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

[-- Attachment #4: Type: text/plain, Size: 186 bytes --]

_______________________________________________
kvm-devel mailing list
kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/kvm-devel

Re: [PATCH][1/3] Move kvm_vcpu_iotcl_get_dirty_log to arch

[Carsten Otte]

Zhang, Xiantao wrote:
> Move kvm_vcpu_ioctl_get_dirty_log to arch, and still keep the interface
> in common.

diff --git a/drivers/kvm/kvm_main.c b/drivers/kvm/kvm_main.c
index 45b18e1..3400265 100644
--- a/drivers/kvm/kvm_main.c
+++ b/drivers/kvm/kvm_main.c
@@ -419,19 +419,14 @@ int kvm_vm_ioctl_set_memory_region(struct kvm *kvm,
  	return kvm_set_memory_region(kvm, mem, user_alloc);
  }

-/*
- * Get (and clear) the dirty memory log for a memory slot.
- */
-static int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
-				      struct kvm_dirty_log *log)
+int kvm_get_dirty_log(struct kvm *kvm,
+			struct kvm_dirty_log *log, int *is_dirty)

Any reason to remove that comment? It looks helpful to me.


  {
  	struct kvm_memory_slot *memslot;
  	int r, i;
  	int n;
  	unsigned long any = 0;

-	mutex_lock(&kvm->lock);
-
  	r = -EINVAL;
  	if (log->slot >= KVM_MEMORY_SLOTS)
  		goto out;
@@ -450,17 +445,11 @@ static int kvm_vm_ioctl_get_dirty_log(struct kvm 
*kvm,
  	if (copy_to_user(log->dirty_bitmap, memslot->dirty_bitmap, n))
  		goto out;

-	/* If nothing is dirty, don't bother messing with page tables. */
-	if (any) {
-		kvm_mmu_slot_remove_write_access(kvm, log->slot);
-		kvm_flush_remote_tlbs(kvm);
-		memset(memslot->dirty_bitmap, 0, n);
-	}
+	if (any)
+		*is_dirty = 1;

  	r = 0;
-
  out:
-	mutex_unlock(&kvm->lock);
  	return r;
  }

This split won't work on s390. I think kvm_get_dirty_log should be 
arch dependent alltogether: we're not going to have a dirty bitmap on 
s390, we rather rely on our hardware support to update our storage 
keys accordingly without guest/host intervention required. We'd want 
to use that, and thus this implementation of kvm_get_dirty_log makes 
no sense for us. On the other hand, I'd really want to keep the ioctl 
common, so that guest migration code in userland can be common for all 
archs.

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH][1/3] Move kvm_vcpu_iotcl_get_dirty_log to arch

[Avi Kivity]

Carsten Otte wrote:
> Zhang, Xiantao wrote:
>> Move kvm_vcpu_ioctl_get_dirty_log to arch, and still keep the interface
>> in common.
>
> diff --git a/drivers/kvm/kvm_main.c b/drivers/kvm/kvm_main.c
> index 45b18e1..3400265 100644
> --- a/drivers/kvm/kvm_main.c
> +++ b/drivers/kvm/kvm_main.c
> @@ -419,19 +419,14 @@ int kvm_vm_ioctl_set_memory_region(struct kvm *kvm,
>      return kvm_set_memory_region(kvm, mem, user_alloc);
>  }
>
> -/*
> - * Get (and clear) the dirty memory log for a memory slot.
> - */
> -static int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
> -                      struct kvm_dirty_log *log)
> +int kvm_get_dirty_log(struct kvm *kvm,
> +            struct kvm_dirty_log *log, int *is_dirty)
>
> Any reason to remove that comment? It looks helpful to me.
>
>
>  {
>      struct kvm_memory_slot *memslot;
>      int r, i;
>      int n;
>      unsigned long any = 0;
>
> -    mutex_lock(&kvm->lock);
> -
>      r = -EINVAL;
>      if (log->slot >= KVM_MEMORY_SLOTS)
>          goto out;
> @@ -450,17 +445,11 @@ static int kvm_vm_ioctl_get_dirty_log(struct kvm 
> *kvm,
>      if (copy_to_user(log->dirty_bitmap, memslot->dirty_bitmap, n))
>          goto out;
>
> -    /* If nothing is dirty, don't bother messing with page tables. */
> -    if (any) {
> -        kvm_mmu_slot_remove_write_access(kvm, log->slot);
> -        kvm_flush_remote_tlbs(kvm);
> -        memset(memslot->dirty_bitmap, 0, n);
> -    }
> +    if (any)
> +        *is_dirty = 1;
>
>      r = 0;
> -
>  out:
> -    mutex_unlock(&kvm->lock);
>      return r;
>  }
>
> This split won't work on s390. I think kvm_get_dirty_log should be 
> arch dependent alltogether: we're not going to have a dirty bitmap on 
> s390, we rather rely on our hardware support to update our storage 
> keys accordingly without guest/host intervention required. We'd want 
> to use that, and thus this implementation of kvm_get_dirty_log makes 
> no sense for us. On the other hand, I'd really want to keep the ioctl 
> common, so that guest migration code in userland can be common for all 
> archs.

On the other hand, it will work for all others IIUC.  Duplicating it in 
all other archs is not a good idea.

We can special case it using #ifdef ARCH_HAS_KVM_GUEST_DIRTY_BITMAP or 
something.

(hides from the #ifdef police)

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


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH][1/3] Move kvm_vcpu_iotcl_get_dirty_log to arch

[Carsten Otte]

Avi Kivity wrote:
> Carsten Otte wrote:
>> Zhang, Xiantao wrote:
>>> Move kvm_vcpu_ioctl_get_dirty_log to arch, and still keep the interface
>>> in common.
>> diff --git a/drivers/kvm/kvm_main.c b/drivers/kvm/kvm_main.c
>> index 45b18e1..3400265 100644
>> --- a/drivers/kvm/kvm_main.c
>> +++ b/drivers/kvm/kvm_main.c
>> @@ -419,19 +419,14 @@ int kvm_vm_ioctl_set_memory_region(struct kvm *kvm,
>>      return kvm_set_memory_region(kvm, mem, user_alloc);
>>  }
>>
>> -/*
>> - * Get (and clear) the dirty memory log for a memory slot.
>> - */
>> -static int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
>> -                      struct kvm_dirty_log *log)
>> +int kvm_get_dirty_log(struct kvm *kvm,
>> +            struct kvm_dirty_log *log, int *is_dirty)
>>
>> Any reason to remove that comment? It looks helpful to me.
>>
>>
>>  {
>>      struct kvm_memory_slot *memslot;
>>      int r, i;
>>      int n;
>>      unsigned long any = 0;
>>
>> -    mutex_lock(&kvm->lock);
>> -
>>      r = -EINVAL;
>>      if (log->slot >= KVM_MEMORY_SLOTS)
>>          goto out;
>> @@ -450,17 +445,11 @@ static int kvm_vm_ioctl_get_dirty_log(struct kvm 
>> *kvm,
>>      if (copy_to_user(log->dirty_bitmap, memslot->dirty_bitmap, n))
>>          goto out;
>>
>> -    /* If nothing is dirty, don't bother messing with page tables. */
>> -    if (any) {
>> -        kvm_mmu_slot_remove_write_access(kvm, log->slot);
>> -        kvm_flush_remote_tlbs(kvm);
>> -        memset(memslot->dirty_bitmap, 0, n);
>> -    }
>> +    if (any)
>> +        *is_dirty = 1;
>>
>>      r = 0;
>> -
>>  out:
>> -    mutex_unlock(&kvm->lock);
>>      return r;
>>  }
>>
>> This split won't work on s390. I think kvm_get_dirty_log should be 
>> arch dependent alltogether: we're not going to have a dirty bitmap on 
>> s390, we rather rely on our hardware support to update our storage 
>> keys accordingly without guest/host intervention required. We'd want 
>> to use that, and thus this implementation of kvm_get_dirty_log makes 
>> no sense for us. On the other hand, I'd really want to keep the ioctl 
>> common, so that guest migration code in userland can be common for all 
>> archs.
> 
> On the other hand, it will work for all others IIUC.  Duplicating it in 
> all other archs is not a good idea.
> 
> We can special case it using #ifdef ARCH_HAS_KVM_GUEST_DIRTY_BITMAP or 
> something.
> 
> (hides from the #ifdef police)

Yea, I agree that it would make sense in case it works for power too. 
We could have an
#ifdef CONFIG_ARCH_S390
	return cool_big_iron_get_dirty_log(args);
#endif
at the beginning of that function. It would'nt hurt readability too much.

Hollis? Christian? How about ppc?

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH][1/3] Move kvm_vcpu_iotcl_get_dirty_log to arch

[Carsten Otte]

Carsten Otte wrote:
> Yea, I agree that it would make sense in case it works for power too. We 
> could have an
> #ifdef CONFIG_ARCH_S390
>     return cool_big_iron_get_dirty_log(args);
> #endif
> at the beginning of that function. It would'nt hurt readability too much.
Stupid suggestion. We could outsource the #ifdef to the caller, that 
would be much cleaner.

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/