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

[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@oss.ntt.co.jp>
---
 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

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

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

[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@suse.de> 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