[PATCH v2 0/4] KVM: kvm_vm_ioctl_get_dirty_log() fix and cleanup

[PATCH v2 0/4] KVM: kvm_vm_ioctl_get_dirty_log() fix and cleanup

[Takuya Yoshikawa]

This patch series is for making dirty logging development, and of course
maintenance, easier. Please see individual patches for details.

Changelog
  v1 -> v2:
    - rebased
    - booke and s390, kvm_vm_ioctl_get_dirty_log() to kvm_arch_vm_ioctl_get_dirty_log()

  Takuya

---
 arch/ia64/kvm/kvm-ia64.c  |   54 ++++++++++++++++++--------------------------
 arch/powerpc/kvm/book3s.c |   29 ++++++++++++++----------
 arch/powerpc/kvm/booke.c  |    2 +-
 arch/s390/kvm/kvm-s390.c  |    4 +-
 arch/x86/kvm/x86.c        |   14 +----------
 include/linux/kvm_host.h  |    6 +---
 virt/kvm/kvm_main.c       |   24 +++++--------------
 7 files changed, 53 insertions(+), 80 deletions(-)

-- 
1.7.0.4

[PATCH v2 1/4] KVM: ia64: fix dirty_log_lock spin_lock section not to include get_dirty_log()

[Takuya Yoshikawa]

kvm_get_dirty_log() calls copy_to_user(). So we need to narrow the
dirty_log_lock spin_lock section not to include this.

Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya-gVGce1chcLdL9jVzuh4AOg@public.gmane.org>
---
 arch/ia64/kvm/kvm-ia64.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/ia64/kvm/kvm-ia64.c b/arch/ia64/kvm/kvm-ia64.c
index bd510be..d85b5d2 100644
--- a/arch/ia64/kvm/kvm-ia64.c
+++ b/arch/ia64/kvm/kvm-ia64.c
@@ -1817,10 +1817,12 @@ static int kvm_ia64_sync_dirty_log(struct kvm *kvm,
 	n = kvm_dirty_bitmap_bytes(memslot);
 	base = memslot->base_gfn / BITS_PER_LONG;
 
+	spin_lock(&kvm->arch.dirty_log_lock);
 	for (i = 0; i < n/sizeof(long); ++i) {
 		memslot->dirty_bitmap[i] = dirty_bitmap[base + i];
 		dirty_bitmap[base + i] = 0;
 	}
+	spin_unlock(&kvm->arch.dirty_log_lock);
 	r = 0;
 out:
 	return r;
@@ -1835,7 +1837,6 @@ int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
 	int is_dirty = 0;
 
 	mutex_lock(&kvm->slots_lock);
-	spin_lock(&kvm->arch.dirty_log_lock);
 
 	r = kvm_ia64_sync_dirty_log(kvm, log);
 	if (r)
@@ -1855,7 +1856,6 @@ int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
 	r = 0;
 out:
 	mutex_unlock(&kvm->slots_lock);
-	spin_unlock(&kvm->arch.dirty_log_lock);
 	return r;
 }
 
-- 
1.7.0.4

[PATCH v2 2/4] KVM: ia64: cleanup kvm_ia64_sync_dirty_log()

[Takuya Yoshikawa]

kvm_ia64_sync_dirty_log() is a helper function for kvm_vm_ioctl_get_dirty_log()
which copies ia64's arch specific dirty bitmap to general one in memslot.
So doing sanity checks in this function is unnatural. We move these checks
outside of this and change the prototype appropriately.

Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
---
 arch/ia64/kvm/kvm-ia64.c |   30 +++++++++++-------------------
 1 files changed, 11 insertions(+), 19 deletions(-)

diff --git a/arch/ia64/kvm/kvm-ia64.c b/arch/ia64/kvm/kvm-ia64.c
index d85b5d2..5cb5865 100644
--- a/arch/ia64/kvm/kvm-ia64.c
+++ b/arch/ia64/kvm/kvm-ia64.c
@@ -1795,25 +1795,15 @@ void kvm_arch_exit(void)
 	kvm_vmm_info = NULL;
 }
 
-static int kvm_ia64_sync_dirty_log(struct kvm *kvm,
-		struct kvm_dirty_log *log)
+static void kvm_ia64_sync_dirty_log(struct kvm *kvm,
+				    struct kvm_memory_slot *memslot)
 {
-	struct kvm_memory_slot *memslot;
-	int r, i;
+	int i;
 	long base;
 	unsigned long n;
 	unsigned long *dirty_bitmap = (unsigned long *)(kvm->arch.vm_base +
 			offsetof(struct kvm_vm_data, kvm_mem_dirty_log));
 
-	r = -EINVAL;
-	if (log->slot >= KVM_MEMORY_SLOTS)
-		goto out;
-
-	memslot = &kvm->memslots->memslots[log->slot];
-	r = -ENOENT;
-	if (!memslot->dirty_bitmap)
-		goto out;
-
 	n = kvm_dirty_bitmap_bytes(memslot);
 	base = memslot->base_gfn / BITS_PER_LONG;
 
@@ -1823,9 +1813,6 @@ static int kvm_ia64_sync_dirty_log(struct kvm *kvm,
 		dirty_bitmap[base + i] = 0;
 	}
 	spin_unlock(&kvm->arch.dirty_log_lock);
-	r = 0;
-out:
-	return r;
 }
 
 int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
@@ -1838,10 +1825,16 @@ int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
 
 	mutex_lock(&kvm->slots_lock);
 
-	r = kvm_ia64_sync_dirty_log(kvm, log);
-	if (r)
+	r = -EINVAL;
+	if (log->slot >= KVM_MEMORY_SLOTS)
+		goto out;
+
+	memslot = &kvm->memslots->memslots[log->slot];
+	r = -ENOENT;
+	if (!memslot->dirty_bitmap)
 		goto out;
 
+	kvm_ia64_sync_dirty_log(kvm, memslot);
 	r = kvm_get_dirty_log(kvm, log, &is_dirty);
 	if (r)
 		goto out;
@@ -1849,7 +1842,6 @@ int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
 	/* If nothing is dirty, don't bother messing with page tables. */
 	if (is_dirty) {
 		kvm_flush_remote_tlbs(kvm);
-		memslot = &kvm->memslots->memslots[log->slot];
 		n = kvm_dirty_bitmap_bytes(memslot);
 		memset(memslot->dirty_bitmap, 0, n);
 	}
-- 
1.7.0.4

[PATCH v2 3/4] KVM: cleanup: remove kvm_get_dirty_log()

[Takuya Yoshikawa]

kvm_get_dirty_log() is a helper function for kvm_vm_ioctl_get_dirty_log() which
is currently used by ia64 and ppc and the following is what it is doing:

  - sanity checks
  - bitmap scan to check if the slot is dirty
  - copy_to_user()

Considering the fact that x86 is not using this anymore and sanity checks must
be done before kvm_ia64_sync_dirty_log(), we can say that this is not working
for code sharing effectively. So we just remove this.

Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya-gVGce1chcLdL9jVzuh4AOg@public.gmane.org>
---
 arch/ia64/kvm/kvm-ia64.c  |   20 ++++++++++++++------
 arch/powerpc/kvm/book3s.c |   29 ++++++++++++++++++++++-------
 include/linux/kvm_host.h  |    2 --
 virt/kvm/kvm_main.c       |   34 ----------------------------------
 4 files changed, 36 insertions(+), 49 deletions(-)

diff --git a/arch/ia64/kvm/kvm-ia64.c b/arch/ia64/kvm/kvm-ia64.c
index 5cb5865..da0c133 100644
--- a/arch/ia64/kvm/kvm-ia64.c
+++ b/arch/ia64/kvm/kvm-ia64.c
@@ -1818,10 +1818,10 @@ static void kvm_ia64_sync_dirty_log(struct kvm *kvm,
 int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
 		struct kvm_dirty_log *log)
 {
-	int r;
+	int r, i;
 	unsigned long n;
 	struct kvm_memory_slot *memslot;
-	int is_dirty = 0;
+	unsigned long is_dirty = 0;
 
 	mutex_lock(&kvm->slots_lock);
 
@@ -1835,15 +1835,23 @@ int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
 		goto out;
 
 	kvm_ia64_sync_dirty_log(kvm, memslot);
-	r = kvm_get_dirty_log(kvm, log, &is_dirty);
-	if (r)
-		goto out;
+
+	n = kvm_dirty_bitmap_bytes(memslot);
+
+	for (i = 0; !is_dirty && i < n/sizeof(long); ++i)
+		is_dirty = memslot->dirty_bitmap[i];
 
 	/* If nothing is dirty, don't bother messing with page tables. */
 	if (is_dirty) {
 		kvm_flush_remote_tlbs(kvm);
-		n = kvm_dirty_bitmap_bytes(memslot);
+		r = -EFAULT;
+		if (copy_to_user(log->dirty_bitmap, memslot->dirty_bitmap, n))
+			goto out;
 		memset(memslot->dirty_bitmap, 0, n);
+	} else {
+		r = -EFAULT;
+		if (clear_user(log->dirty_bitmap, n))
+			goto out;
 	}
 	r = 0;
 out:
diff --git a/arch/powerpc/kvm/book3s.c b/arch/powerpc/kvm/book3s.c
index 801d9f3..bea6f7c 100644
--- a/arch/powerpc/kvm/book3s.c
+++ b/arch/powerpc/kvm/book3s.c
@@ -1185,28 +1185,43 @@ int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
 	struct kvm_memory_slot *memslot;
 	struct kvm_vcpu *vcpu;
 	ulong ga, ga_end;
-	int is_dirty = 0;
-	int r;
+	unsigned long is_dirty = 0;
+	int r, i;
 	unsigned long n;
 
 	mutex_lock(&kvm->slots_lock);
 
-	r = kvm_get_dirty_log(kvm, log, &is_dirty);
-	if (r)
+	r = -EINVAL;
+	if (log->slot >= KVM_MEMORY_SLOTS)
+		goto out;
+
+	memslot = &kvm->memslots->memslots[log->slot];
+	r = -ENOENT;
+	if (!memslot->dirty_bitmap)
 		goto out;
 
+	n = kvm_dirty_bitmap_bytes(memslot);
+
+	for (i = 0; !is_dirty && i < n/sizeof(long); ++i)
+		is_dirty = memslot->dirty_bitmap[i];
+
 	/* If nothing is dirty, don't bother messing with page tables. */
 	if (is_dirty) {
-		memslot = &kvm->memslots->memslots[log->slot];
-
 		ga = memslot->base_gfn << PAGE_SHIFT;
 		ga_end = ga + (memslot->npages << PAGE_SHIFT);
 
 		kvm_for_each_vcpu(n, vcpu, kvm)
 			kvmppc_mmu_pte_pflush(vcpu, ga, ga_end);
 
-		n = kvm_dirty_bitmap_bytes(memslot);
+		r = -EFAULT;
+		if (copy_to_user(log->dirty_bitmap, memslot->dirty_bitmap, n))
+			goto out;
+
 		memset(memslot->dirty_bitmap, 0, n);
+	} else {
+		r = -EFAULT;
+		if (clear_user(log->dirty_bitmap, n))
+			goto out;
 	}
 
 	r = 0;
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index e796326..b51e6b2 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -336,8 +336,6 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
 
 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);
 
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 74f7319..8859049 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -773,40 +773,6 @@ int kvm_vm_ioctl_set_memory_region(struct kvm *kvm,
 	return kvm_set_memory_region(kvm, mem, user_alloc);
 }
 
-int kvm_get_dirty_log(struct kvm *kvm,
-			struct kvm_dirty_log *log, int *is_dirty)
-{
-	struct kvm_memory_slot *memslot;
-	int r, i;
-	unsigned long n;
-	unsigned long any = 0;
-
-	r = -EINVAL;
-	if (log->slot >= KVM_MEMORY_SLOTS)
-		goto out;
-
-	memslot = &kvm->memslots->memslots[log->slot];
-	r = -ENOENT;
-	if (!memslot->dirty_bitmap)
-		goto out;
-
-	n = kvm_dirty_bitmap_bytes(memslot);
-
-	for (i = 0; !any && i < n/sizeof(long); ++i)
-		any = memslot->dirty_bitmap[i];
-
-	r = -EFAULT;
-	if (copy_to_user(log->dirty_bitmap, memslot->dirty_bitmap, n))
-		goto out;
-
-	if (any)
-		*is_dirty = 1;
-
-	r = 0;
-out:
-	return r;
-}
-
 void kvm_disable_largepages(void)
 {
 	largepages_enabled = false;
-- 
1.7.0.4

[PATCH v2 4/4] KVM: cleanup: make kvm_vm_ioctl_get_dirty_log() arch independent

[Takuya Yoshikawa]

kvm_vm_ioctl_get_dirty_log() is now implemented as arch dependent function.
But now that we know what is actually arch dependent, we can split this into
arch dependent part and arch independent part easily.

Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya-gVGce1chcLdL9jVzuh4AOg@public.gmane.org>
---
 arch/ia64/kvm/kvm-ia64.c  |   14 ++------------
 arch/powerpc/kvm/book3s.c |   14 ++------------
 arch/powerpc/kvm/booke.c  |    2 +-
 arch/s390/kvm/kvm-s390.c  |    4 ++--
 arch/x86/kvm/x86.c        |   14 ++------------
 include/linux/kvm_host.h  |    4 ++--
 virt/kvm/kvm_main.c       |   24 ++++++++++++++++++++++++
 7 files changed, 35 insertions(+), 41 deletions(-)

diff --git a/arch/ia64/kvm/kvm-ia64.c b/arch/ia64/kvm/kvm-ia64.c
index da0c133..86b6b1d 100644
--- a/arch/ia64/kvm/kvm-ia64.c
+++ b/arch/ia64/kvm/kvm-ia64.c
@@ -1815,24 +1815,15 @@ static void kvm_ia64_sync_dirty_log(struct kvm *kvm,
 	spin_unlock(&kvm->arch.dirty_log_lock);
 }
 
-int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
-		struct kvm_dirty_log *log)
+int kvm_arch_vm_ioctl_get_dirty_log(struct kvm *kvm,
+				    struct kvm_dirty_log *log)
 {
 	int r, i;
 	unsigned long n;
 	struct kvm_memory_slot *memslot;
 	unsigned long is_dirty = 0;
 
-	mutex_lock(&kvm->slots_lock);
-
-	r = -EINVAL;
-	if (log->slot >= KVM_MEMORY_SLOTS)
-		goto out;
-
 	memslot = &kvm->memslots->memslots[log->slot];
-	r = -ENOENT;
-	if (!memslot->dirty_bitmap)
-		goto out;
 
 	kvm_ia64_sync_dirty_log(kvm, memslot);
 
@@ -1855,7 +1846,6 @@ int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
 	}
 	r = 0;
 out:
-	mutex_unlock(&kvm->slots_lock);
 	return r;
 }
 
diff --git a/arch/powerpc/kvm/book3s.c b/arch/powerpc/kvm/book3s.c
index bea6f7c..884d4a5 100644
--- a/arch/powerpc/kvm/book3s.c
+++ b/arch/powerpc/kvm/book3s.c
@@ -1179,8 +1179,8 @@ int kvm_arch_vcpu_ioctl_translate(struct kvm_vcpu *vcpu,
 /*
  * 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 kvm_arch_vm_ioctl_get_dirty_log(struct kvm *kvm,
+				    struct kvm_dirty_log *log)
 {
 	struct kvm_memory_slot *memslot;
 	struct kvm_vcpu *vcpu;
@@ -1189,16 +1189,7 @@ int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
 	int r, i;
 	unsigned long n;
 
-	mutex_lock(&kvm->slots_lock);
-
-	r = -EINVAL;
-	if (log->slot >= KVM_MEMORY_SLOTS)
-		goto out;
-
 	memslot = &kvm->memslots->memslots[log->slot];
-	r = -ENOENT;
-	if (!memslot->dirty_bitmap)
-		goto out;
 
 	n = kvm_dirty_bitmap_bytes(memslot);
 
@@ -1226,7 +1217,6 @@ int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
 
 	r = 0;
 out:
-	mutex_unlock(&kvm->slots_lock);
 	return r;
 }
 
diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
index 8d4e35f..376b586 100644
--- a/arch/powerpc/kvm/booke.c
+++ b/arch/powerpc/kvm/booke.c
@@ -565,7 +565,7 @@ int kvm_arch_vcpu_ioctl_translate(struct kvm_vcpu *vcpu,
 	return r;
 }
 
-int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm, struct kvm_dirty_log *log)
+int kvm_arch_vm_ioctl_get_dirty_log(struct kvm *kvm, struct kvm_dirty_log *log)
 {
 	return -ENOTSUPP;
 }
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 47acdb1..159aec2 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -134,8 +134,8 @@ int kvm_dev_ioctl_check_extension(long ext)
 /*
  * 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 kvm_arch_vm_ioctl_get_dirty_log(struct kvm *kvm,
+				    struct kvm_dirty_log *log)
 {
 	return 0;
 }
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index e18e68f..ce4c603 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2861,24 +2861,15 @@ static int kvm_vm_ioctl_reinject(struct kvm *kvm,
 /*
  * 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 kvm_arch_vm_ioctl_get_dirty_log(struct kvm *kvm,
+				    struct kvm_dirty_log *log)
 {
 	int r, i;
 	struct kvm_memory_slot *memslot;
 	unsigned long n;
 	unsigned long is_dirty = 0;
 
-	mutex_lock(&kvm->slots_lock);
-
-	r = -EINVAL;
-	if (log->slot >= KVM_MEMORY_SLOTS)
-		goto out;
-
 	memslot = &kvm->memslots->memslots[log->slot];
-	r = -ENOENT;
-	if (!memslot->dirty_bitmap)
-		goto out;
 
 	n = kvm_dirty_bitmap_bytes(memslot);
 
@@ -2929,7 +2920,6 @@ int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
 
 	r = 0;
 out:
-	mutex_unlock(&kvm->slots_lock);
 	return r;
 }
 
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index b51e6b2..9289d1a 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -336,8 +336,8 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
 
 int kvm_dev_ioctl_check_extension(long ext);
 
-int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
-				struct kvm_dirty_log *log);
+int kvm_arch_vm_ioctl_get_dirty_log(struct kvm *kvm,
+				    struct kvm_dirty_log *log);
 
 int kvm_vm_ioctl_set_memory_region(struct kvm *kvm,
 				   struct
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 8859049..bf3d273 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -773,6 +773,30 @@ int kvm_vm_ioctl_set_memory_region(struct kvm *kvm,
 	return kvm_set_memory_region(kvm, mem, user_alloc);
 }
 
+static int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
+				      struct kvm_dirty_log *log)
+{
+	struct kvm_memory_slot *memslot;
+	int r;
+
+	mutex_lock(&kvm->slots_lock);
+
+	r = -EINVAL;
+	if (log->slot >= KVM_MEMORY_SLOTS)
+		goto out;
+
+	memslot = &kvm->memslots->memslots[log->slot];
+	r = -ENOENT;
+	if (!memslot->dirty_bitmap)
+		goto out;
+
+	r = kvm_arch_vm_ioctl_get_dirty_log(kvm, log);
+
+out:
+	mutex_unlock(&kvm->slots_lock);
+	return r;
+}
+
 void kvm_disable_largepages(void)
 {
 	largepages_enabled = false;
-- 
1.7.0.4

Re: [PATCH v2 3/4] KVM: cleanup: remove kvm_get_dirty_log()

[Avi Kivity]

On 06/23/2010 09:01 AM, Takuya Yoshikawa wrote:
> kvm_get_dirty_log() is a helper function for kvm_vm_ioctl_get_dirty_log() which
> is currently used by ia64 and ppc and the following is what it is doing:
>
>    - sanity checks
>    - bitmap scan to check if the slot is dirty
>    - copy_to_user()
>
> Considering the fact that x86 is not using this anymore and sanity checks must
> be done before kvm_ia64_sync_dirty_log(), we can say that this is not working
> for code sharing effectively. So we just remove this.
>
>
> diff --git a/arch/powerpc/kvm/book3s.c b/arch/powerpc/kvm/book3s.c
> index 801d9f3..bea6f7c 100644
> --- a/arch/powerpc/kvm/book3s.c
> +++ b/arch/powerpc/kvm/book3s.c
> @@ -1185,28 +1185,43 @@ int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
>   	struct kvm_memory_slot *memslot;
>   	struct kvm_vcpu *vcpu;
>   	ulong ga, ga_end;
> -	int is_dirty = 0;
> -	int r;
> +	unsigned long is_dirty = 0;
> +	int r, i;
>   	unsigned long n;
>
>   	mutex_lock(&kvm->slots_lock);
>
> -	r = kvm_get_dirty_log(kvm, log,&is_dirty);
> -	if (r)
> +	r = -EINVAL;
> +	if (log->slot>= KVM_MEMORY_SLOTS)
> +		goto out;
> +
> +	memslot =&kvm->memslots->memslots[log->slot];
>    

Not introduced by this patch, but shouldn't this use rcu_dereference()?


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

Re: [PATCH v2 0/4] KVM: kvm_vm_ioctl_get_dirty_log() fix and cleanup

[Avi Kivity]

On 06/23/2010 08:58 AM, Takuya Yoshikawa wrote:
> This patch series is for making dirty logging development, and of course
> maintenance, easier. Please see individual patches for details.
>    

Applied, thanks.

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

Re: [PATCH v2 3/4] KVM: cleanup: remove kvm_get_dirty_log()

[Avi Kivity]

On 06/23/2010 12:01 PM, Takuya Yoshikawa wrote:
> (2010/06/23 17:48), Avi Kivity wrote:
>
>>> diff --git a/arch/powerpc/kvm/book3s.c b/arch/powerpc/kvm/book3s.c
>>> index 801d9f3..bea6f7c 100644
>>> --- a/arch/powerpc/kvm/book3s.c
>>> +++ b/arch/powerpc/kvm/book3s.c
>>> @@ -1185,28 +1185,43 @@ int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
>>> struct kvm_memory_slot *memslot;
>>> struct kvm_vcpu *vcpu;
>>> ulong ga, ga_end;
>>> - int is_dirty = 0;
>>> - int r;
>>> + unsigned long is_dirty = 0;
>>> + int r, i;
>>> unsigned long n;
>>>
>>> mutex_lock(&kvm->slots_lock);
>>>
>>> - r = kvm_get_dirty_log(kvm, log,&is_dirty);
>>> - if (r)
>>> + r = -EINVAL;
>>> + if (log->slot>= KVM_MEMORY_SLOTS)
>>> + goto out;
>>> +
>>> + memslot =&kvm->memslots->memslots[log->slot];
>>
>> Not introduced by this patch, but shouldn't this use rcu_dereference()?
>>
>>
>
> I was thinking like that, but sorry I don't know well about ppc.
>
> My final goal is to make everything except
>
>   /* If nothing is dirty, don't bother messing with page tables. */
>
> part arch independent, so that we can concentrate on the truely
> dirty logging things in the future.
>
> So, making ppc code same as x86, rcu_dereference() for example , really
> helps me.
>
>
> Do you like this approach?

Well yes, I expect that not using rcu_dereference() (and 
srcu_read_lock()) is a bug here.

Marcelo?

On a related note, I'd like to consolidate rcu locking:

- all vcpu ioctls take srcu_read_lock() when they start and drop it at 
the end.
- KVM_VCPU_RUN drops the lock when sleeping and entering the guest (also 
on context switch?)

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

Re: [PATCH v2 3/4] KVM: cleanup: remove kvm_get_dirty_log()

[Takuya Yoshikawa]

(2010/06/23 17:48), Avi Kivity wrote:

>> diff --git a/arch/powerpc/kvm/book3s.c b/arch/powerpc/kvm/book3s.c
>> index 801d9f3..bea6f7c 100644
>> --- a/arch/powerpc/kvm/book3s.c
>> +++ b/arch/powerpc/kvm/book3s.c
>> @@ -1185,28 +1185,43 @@ int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
>> struct kvm_memory_slot *memslot;
>> struct kvm_vcpu *vcpu;
>> ulong ga, ga_end;
>> - int is_dirty = 0;
>> - int r;
>> + unsigned long is_dirty = 0;
>> + int r, i;
>> unsigned long n;
>>
>> mutex_lock(&kvm->slots_lock);
>>
>> - r = kvm_get_dirty_log(kvm, log,&is_dirty);
>> - if (r)
>> + r = -EINVAL;
>> + if (log->slot>= KVM_MEMORY_SLOTS)
>> + goto out;
>> +
>> + memslot =&kvm->memslots->memslots[log->slot];
>
> Not introduced by this patch, but shouldn't this use rcu_dereference()?
>
>

I was thinking like that, but sorry I don't know well about ppc.

My final goal is to make everything except

   /* If nothing is dirty, don't bother messing with page tables. */

part arch independent, so that we can concentrate on the truely
dirty logging things in the future.

So, making ppc code same as x86, rcu_dereference() for example , really
helps me.


Do you like this approach?

   Takuya

Re: [PATCH v2 3/4] KVM: cleanup: remove kvm_get_dirty_log()

[Marcelo Tosatti]

On Wed, Jun 23, 2010 at 12:00:47PM +0300, Avi Kivity wrote:
> On 06/23/2010 12:01 PM, Takuya Yoshikawa wrote:
> >(2010/06/23 17:48), Avi Kivity wrote:
> >
> >>>diff --git a/arch/powerpc/kvm/book3s.c b/arch/powerpc/kvm/book3s.c
> >>>index 801d9f3..bea6f7c 100644
> >>>--- a/arch/powerpc/kvm/book3s.c
> >>>+++ b/arch/powerpc/kvm/book3s.c
> >>>@@ -1185,28 +1185,43 @@ int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
> >>>struct kvm_memory_slot *memslot;
> >>>struct kvm_vcpu *vcpu;
> >>>ulong ga, ga_end;
> >>>- int is_dirty = 0;
> >>>- int r;
> >>>+ unsigned long is_dirty = 0;
> >>>+ int r, i;
> >>>unsigned long n;
> >>>
> >>>mutex_lock(&kvm->slots_lock);
> >>>
> >>>- r = kvm_get_dirty_log(kvm, log,&is_dirty);
> >>>- if (r)
> >>>+ r = -EINVAL;
> >>>+ if (log->slot>= KVM_MEMORY_SLOTS)
> >>>+ goto out;
> >>>+
> >>>+ memslot =&kvm->memslots->memslots[log->slot];
> >>
> >>Not introduced by this patch, but shouldn't this use rcu_dereference()?
> >>
> >>
> >
> >I was thinking like that, but sorry I don't know well about ppc.
> >
> >My final goal is to make everything except
> >
> >  /* If nothing is dirty, don't bother messing with page tables. */
> >
> >part arch independent, so that we can concentrate on the truely
> >dirty logging things in the future.
> >
> >So, making ppc code same as x86, rcu_dereference() for example , really
> >helps me.
> >
> >
> >Do you like this approach?
> 
> Well yes, I expect that not using rcu_dereference() (and
> srcu_read_lock()) is a bug here.
> 
> Marcelo?

No because slots_lock is held which serializes with memslot updates.

> On a related note, I'd like to consolidate rcu locking:
> 
> - all vcpu ioctls take srcu_read_lock() when they start and drop it
> at the end.
> - KVM_VCPU_RUN drops the lock when sleeping and entering the guest
> (also on context switch?)

Yep, makes sense.

Re: [PATCH v2 3/4] KVM: cleanup: remove kvm_get_dirty_log()

[Avi Kivity]

On 06/24/2010 03:58 AM, Marcelo Tosatti wrote:
>
>> Well yes, I expect that not using rcu_dereference() (and
>> srcu_read_lock()) is a bug here.
>>
>> Marcelo?
>>      
> No because slots_lock is held which serializes with memslot updates.
>    

Right, I need to reread Documentation/kvm/locking.txt.

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

Re: [PATCH v2 3/4] KVM: cleanup: remove kvm_get_dirty_log()

[Alexander Graf]

On 23.06.2010, at 08:01, Takuya Yoshikawa wrote:

> kvm_get_dirty_log() is a helper function for kvm_vm_ioctl_get_dirty_log() which
> is currently used by ia64 and ppc and the following is what it is doing:
> 
>  - sanity checks
>  - bitmap scan to check if the slot is dirty
>  - copy_to_user()
> 
> Considering the fact that x86 is not using this anymore and sanity checks must
> be done before kvm_ia64_sync_dirty_log(), we can say that this is not working
> for code sharing effectively. So we just remove this.

This patch plus 4/4 broke dirty bitmap updating on PPC. I didn't get around to track down why, but I figured you should now. Is there any way to get you a PPC development box? A simple G4 or G5 should be 200$ on ebay by now :).


Alex

Re: [PATCH v2 3/4] KVM: cleanup: remove kvm_get_dirty_log()

[Takuya Yoshikawa]

On Fri, 25 Jun 2010 21:25:57 +0200
Alexander Graf <agraf-l3A5Bk7waGM@public.gmane.org> wrote:

> 
> This patch plus 4/4 broke dirty bitmap updating on PPC. I didn't get around to track down why, but I figured you should now. Is there any way to get you a PPC development box? A simple G4 or G5 should be 200$ on ebay by now :).
> 

I'm sorry, I thought this change was just trivial code transformation
and test for x86 would be OK: but not actually. Probably the reason is
around the timing of copy_to_user() and newly introduced clear_user()
for clean slot.

> 
> Alex
> 

Re: [PATCH v2 3/4] KVM: cleanup: remove kvm_get_dirty_log()

[Takuya Yoshikawa]

> This patch plus 4/4 broke dirty bitmap updating on PPC. I didn't get around to track down why, but I figured you should now. Is there any way to get you a PPC development box? A simple G4 or G5 should be 200$ on ebay by now :).
> 

A simple G4 or G5, thanks for the info, I'll buy one.

I hope I can contribute a bit from there to kvm-ppc :).


> 
> Alex
> 

Re: [PATCH v2 3/4] KVM: cleanup: remove kvm_get_dirty_log()

[Alexander Graf]

On 26.06.2010, at 02:55, Takuya Yoshikawa wrote:

> 
>> This patch plus 4/4 broke dirty bitmap updating on PPC. I didn't get around to track down why, but I figured you should now. Is there any way to get you a PPC development box? A simple G4 or G5 should be 200$ on ebay by now :).
>> 
> 
> A simple G4 or G5, thanks for the info, I'll buy one.

The G4 is 32-bit while the G5 is 64-bit. But since the code paths are very similar on both, you'd be fine with a G4 too. I just checked again and apparently a 1.25Ghz G4 is ~100$ while a dual 1.8Ghz G5 is around 330$.

If it's too troublesome to acquire one yourself, I can also try to buy a notebook and bring it over for the KVM Forum.

I'm looking forward to tested patches :)


Alex

Re: [PATCH v2 3/4] KVM: cleanup: remove kvm_get_dirty_log()

[Avi Kivity]

On 06/25/2010 10:25 PM, Alexander Graf wrote:
> On 23.06.2010, at 08:01, Takuya Yoshikawa wrote:
>
>    
>> kvm_get_dirty_log() is a helper function for kvm_vm_ioctl_get_dirty_log() which
>> is currently used by ia64 and ppc and the following is what it is doing:
>>
>>   - sanity checks
>>   - bitmap scan to check if the slot is dirty
>>   - copy_to_user()
>>
>> Considering the fact that x86 is not using this anymore and sanity checks must
>> be done before kvm_ia64_sync_dirty_log(), we can say that this is not working
>> for code sharing effectively. So we just remove this.
>>      
> This patch plus 4/4 broke dirty bitmap updating on PPC. I didn't get around to track down why, but I figured you should now. Is there any way to get you a PPC development box? A simple G4 or G5 should be 200$ on ebay by now :).
>    

Best to revert these patches and re-apply when fixed.

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

Re: [PATCH v2 3/4] KVM: cleanup: remove kvm_get_dirty_log()

[Takuya Yoshikawa]

(2010/06/27 16:32), Avi Kivity wrote:
> On 06/25/2010 10:25 PM, Alexander Graf wrote:
>> On 23.06.2010, at 08:01, Takuya Yoshikawa wrote:
>>
>>> kvm_get_dirty_log() is a helper function for
>>> kvm_vm_ioctl_get_dirty_log() which
>>> is currently used by ia64 and ppc and the following is what it is doing:
>>>
>>> - sanity checks
>>> - bitmap scan to check if the slot is dirty
>>> - copy_to_user()
>>>
>>> Considering the fact that x86 is not using this anymore and sanity
>>> checks must
>>> be done before kvm_ia64_sync_dirty_log(), we can say that this is not
>>> working
>>> for code sharing effectively. So we just remove this.
>> This patch plus 4/4 broke dirty bitmap updating on PPC. I didn't get
>> around to track down why, but I figured you should now. Is there any
>> way to get you a PPC development box? A simple G4 or G5 should be 200$
>> on ebay by now :).
>
> Best to revert these patches and re-apply when fixed.
>

Please revert. Sincerely sorry.

I'll fix as soon as possible and re-submit when test on PPC gets OK.

Re: [PATCH v2 3/4] KVM: cleanup: remove kvm_get_dirty_log()

[Avi Kivity]

On 06/28/2010 11:14 AM, Takuya Yoshikawa wrote:
>> Best to revert these patches and re-apply when fixed.
>>
>
>
> Please revert. Sincerely sorry.
>
> I'll fix as soon as possible and re-submit when test on PPC gets OK.

No problem, regressions happen all the time.  Lucky Alex caught it so 
quickly.

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