KVM: kvm_vm_ioctl_get_dirty_log() fix and cleanup

PATCH Applied Takuya Yoshikawa 8 participants 18 messages Jun 23, 2010 – Jun 28, 2010 6 messages not in the archive

Subsystems (7)

x86 KVM PowerPC s390 +3 more subsystems

lore

Share & tools

lore

Takuya Yoshikawa

author

2010-06-23 05:57 UTC #0 lore raw

v2 cover KVM: kvm_vm_ioctl_get_dirty_log() fix and cleanup

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(-)

Signature · 1 line

1.7.0.4

A message that is not in the archive (sent off-list or not yet received): 20100623145807.07a07c4c.yoshikawa.takuya-gVGce1chcLdL9jVzuh4AOg@public.gmane.org

Takuya Yoshikawa

2010-06-23 05:59 UTC #2 lore raw

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

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 <[email protected]>

---
 arch/ia64/kvm/kvm-ia64.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

arch/ia64/kvm/kvm-ia64.c +2 -2

index bd510be..d85b5d2 100644
@@ -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;
 }

Signature · 1 line

1.7.0.4

Takuya Yoshikawa

2010-06-23 06:01 UTC #3 lore raw

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

In reply to a message not in the archive

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 <[email protected]>

---
 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(-)

arch/ia64/kvm/kvm-ia64.c +14 -6

index 5cb5865..da0c133 100644
@@ -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:

arch/powerpc/kvm/book3s.c +22 -7

index 801d9f3..bea6f7c 100644
@@ -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;

include/linux/kvm_host.h +0 -2

index e796326..b51e6b2 100644
@@ -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);

virt/kvm/kvm_main.c +0 ~~-34~~

index 74f7319..8859049 100644
@@ -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;

Signature · 1 line

1.7.0.4

A message that is not in the archive (sent off-list or not yet received): 20100623150134.9fb5915b.yoshikawa.takuya-gVGce1chcLdL9jVzuh4AOg@public.gmane.org

Avi Kivity

2010-06-23 08:48 UTC #5 lore raw

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

Quoted 26 lines · 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.
arch/powerpc/kvm/book3s.c +2 -2
index 801d9f3..bea6f7c 100644
@@ -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;
 

arch/powerpc/kvm/book3s.c int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm, · line 1199

Quoted 10 lines

arch/powerpc/kvm/book3s.c +5 -2

  	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()?

Signature · 1 line

error compiling committee.c: too many arguments to function

A message that is not in the archive (sent off-list or not yet received): [email protected]

Takuya Yoshikawa

2010-06-23 08:56 UTC #7 lore raw

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

Quoted 16 lines · Avi Kivity

Quoted 16 lines

arch/powerpc/kvm/book3s.c +2 -2

index 801d9f3..bea6f7c 100644
@@ -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);

Quoted 11 lines

arch/powerpc/kvm/book3s.c int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
Quoted 7 lines
arch/powerpc/kvm/book3s.c +5 -2
- 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

Avi Kivity

2010-06-23 09:00 UTC #8 lore raw

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

Quoted 36 lines · Takuya Yoshikawa

Quoted 16 lines · Avi Kivity
Quoted 16 lines
arch/powerpc/kvm/book3s.c +2 -2
index 801d9f3..bea6f7c 100644
@@ -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);
 
Quoted 11 lines
arch/powerpc/kvm/book3s.c int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
Quoted 7 lines
arch/powerpc/kvm/book3s.c +5 -2
- 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. */

Quoted 8 lines

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?)

Signature · 1 line

error compiling committee.c: too many arguments to function

Marcelo Tosatti

2010-06-24 00:58 UTC #9 lore raw

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

Quoted 44 lines · Avi Kivity

Quoted 43 lines · Takuya Yoshikawa
Quoted 16 lines · Avi Kivity
Quoted 16 lines
arch/powerpc/kvm/book3s.c +2 -2
index 801d9f3..bea6f7c 100644
@@ -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);
 
Quoted 11 lines
arch/powerpc/kvm/book3s.c int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
Quoted 7 lines
arch/powerpc/kvm/book3s.c +5 -2
- 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.

Quoted 6 lines

Quoted 1 line
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.

Quoted 6 lines

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.

A message that is not in the archive (sent off-list or not yet received): [email protected]

Avi Kivity

2010-06-24 08:10 UTC #11 lore raw

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

Quoted 8 lines · Marcelo Tosatti

Quoted 6 lines
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.

Signature · 1 line

error compiling committee.c: too many arguments to function

Alexander Graf

2010-06-25 19:25 UTC #12 lore raw

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

In reply to a message not in the archive

Quoted 10 lines · 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.

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

A message that is not in the archive (sent off-list or not yet received): E5BC6546-6255-4E25-B3DD-F6B6261B6830-l3A5Bk7waGM@public.gmane.org

Takuya Yoshikawa

2010-06-26 00:38 UTC #14 lore raw

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

Quoted 3 lines · On Fri, 25 Jun 2010 21:25:57 +0200 Alexander Graf…

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.

Quoted 3 lines

Alex

Takuya Yoshikawa

2010-06-26 00:55 UTC #15 lore raw

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

In reply to a message not in the archive

Quoted 3 lines

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 :).

Quoted 3 lines

Alex

Alexander Graf

2010-06-26 09:38 UTC #16 lore raw

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

Quoted 5 lines · Takuya Yoshikawa

Quoted 3 lines
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

Avi Kivity

2010-06-27 07:32 UTC #17 lore raw

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

In reply to a message not in the archive

Quoted 10 lines · Alexander Graf

Quoted 7 lines · 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()

Quoted 6 lines

Quoted 4 lines
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.

Signature · 1 line

error compiling committee.c: too many arguments to function

A message that is not in the archive (sent off-list or not yet received): [email protected]

Takuya Yoshikawa

2010-06-28 08:14 UTC #19 lore raw

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

Quoted 16 lines · Avi Kivity

Quoted 15 lines · Alexander Graf
Quoted 13 lines · 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.

Quoted 7 lines

Quoted 4 lines
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.

Avi Kivity

2010-06-28 09:36 UTC #20 lore raw

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

Quoted 7 lines · Takuya Yoshikawa

Quoted 2 lines
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.

Signature · 1 line

error compiling committee.c: too many arguments to function

Takuya Yoshikawa

2010-06-23 06:02 UTC #21 lore raw

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

In reply to a message not in the archive

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 <[email protected]>

---
 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(-)

arch/ia64/kvm/kvm-ia64.c +2 ~~-12~~

index da0c133..86b6b1d 100644
@@ -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;
 }

arch/powerpc/kvm/book3s.c +2 ~~-12~~

index bea6f7c..884d4a5 100644
@@ -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;
 }

arch/powerpc/kvm/booke.c +1 -1

index 8d4e35f..376b586 100644
@@ -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;
 }

arch/s390/kvm/kvm-s390.c +2 -2

index 47acdb1..159aec2 100644
@@ -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;
 }

arch/x86/kvm/x86.c +2 ~~-12~~

index e18e68f..ce4c603 100644
@@ -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;
 }

include/linux/kvm_host.h +2 -2

index b51e6b2..9289d1a 100644
@@ -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

virt/kvm/kvm_main.c +24 -0

index 8859049..bf3d273 100644
@@ -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;

Signature · 1 line

1.7.0.4

Avi Kivity

Applied

2010-06-23 08:50 UTC #22 lore raw

In reply to a message not in the archive

Quoted 3 lines · Takuya Yoshikawa

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

Applied, thanks.

Signature · 1 line

error compiling committee.c: too many arguments to function

Takuya Yoshikawa

author

2010-06-23 05:59 UTC #23 lore raw

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

In reply to 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 <[email protected]>

---
 arch/ia64/kvm/kvm-ia64.c |   30 +++++++++++-------------------
 1 files changed, 11 insertions(+), 19 deletions(-)

arch/ia64/kvm/kvm-ia64.c +11 ~~-19~~

index d85b5d2..5cb5865 100644
@@ -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);
 	}

Signature · 1 line

1.7.0.4