[PATCH 0/3] KVM: MMU: Trivial fix and cleanups

[PATCH 0/3] KVM: MMU: Trivial fix and cleanups

[Takuya Yoshikawa]

Made when I was reading mmu code.

	Takuya

BTW, is threre any good way to test large page functionality?

[PATCH 1/3] KVM: MMU: Remove for_each_unsync_children() macro

[Takuya Yoshikawa]

There is only one user of it and for_each_set_bit() does the same.

Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
---
 arch/x86/kvm/mmu.c |    7 +------
 1 files changed, 1 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index d737443..09da963 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -1403,11 +1403,6 @@ struct kvm_mmu_pages {
 	unsigned int nr;
 };
 
-#define for_each_unsync_children(bitmap, idx)		\
-	for (idx = find_first_bit(bitmap, 512);		\
-	     idx < 512;					\
-	     idx = find_next_bit(bitmap, 512, idx+1))
-
 static int mmu_pages_add(struct kvm_mmu_pages *pvec, struct kvm_mmu_page *sp,
 			 int idx)
 {
@@ -1429,7 +1424,7 @@ static int __mmu_unsync_walk(struct kvm_mmu_page *sp,
 {
 	int i, ret, nr_unsync_leaf = 0;
 
-	for_each_unsync_children(sp->unsync_child_bitmap, i) {
+	for_each_set_bit(i, sp->unsync_child_bitmap, 512) {
 		struct kvm_mmu_page *child;
 		u64 ent = sp->spt[i];
 
-- 
1.7.5.4

[PATCH 2/3] KVM: MMU: Add missing large page accounting to drop_large_spte()

[Takuya Yoshikawa]

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

diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index 09da963..5e761ff 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -1803,6 +1803,7 @@ static void drop_large_spte(struct kvm_vcpu *vcpu, u64 *sptep)
 {
 	if (is_large_pte(*sptep)) {
 		drop_spte(vcpu->kvm, sptep);
+		--vcpu->kvm->stat.lpages;
 		kvm_flush_remote_tlbs(vcpu->kvm);
 	}
 }
-- 
1.7.5.4

Re: [PATCH 2/3] KVM: MMU: Add missing large page accounting to drop_large_spte()

[Takuya Yoshikawa]

ping

(2011/11/29 14:03), Takuya Yoshikawa wrote:
> Signed-off-by: Takuya Yoshikawa<yoshikawa.takuya@oss.ntt.co.jp>
> ---
>   arch/x86/kvm/mmu.c |    1 +
>   1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> index 09da963..5e761ff 100644
> --- a/arch/x86/kvm/mmu.c
> +++ b/arch/x86/kvm/mmu.c
> @@ -1803,6 +1803,7 @@ static void drop_large_spte(struct kvm_vcpu *vcpu, u64 *sptep)
>   {
>   	if (is_large_pte(*sptep)) {
>   		drop_spte(vcpu->kvm, sptep);
> +		--vcpu->kvm->stat.lpages;
>   		kvm_flush_remote_tlbs(vcpu->kvm);
>   	}
>   }

Is this fix wrong?

I do not mind dropping other two cleanups in the patch set
if you do not like; of course I will update this if needed.

	Takuya

Re: [PATCH 2/3] KVM: MMU: Add missing large page accounting to drop_large_spte()

[Avi Kivity]

On 12/20/2011 06:37 AM, Takuya Yoshikawa wrote:
> ping
>
> (2011/11/29 14:03), Takuya Yoshikawa wrote:
>> Signed-off-by: Takuya Yoshikawa<yoshikawa.takuya@oss.ntt.co.jp>
>> ---
>>   arch/x86/kvm/mmu.c |    1 +
>>   1 files changed, 1 insertions(+), 0 deletions(-)
>>
>> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
>> index 09da963..5e761ff 100644
>> --- a/arch/x86/kvm/mmu.c
>> +++ b/arch/x86/kvm/mmu.c
>> @@ -1803,6 +1803,7 @@ static void drop_large_spte(struct kvm_vcpu
>> *vcpu, u64 *sptep)
>>   {
>>       if (is_large_pte(*sptep)) {
>>           drop_spte(vcpu->kvm, sptep);
>> +        --vcpu->kvm->stat.lpages;
>>           kvm_flush_remote_tlbs(vcpu->kvm);
>>       }
>>   }
>
> Is this fix wrong?
>
> I do not mind dropping other two cleanups in the patch set
> if you do not like; of course I will update this if needed.
>

Sorry, missed it.  It's not wrong, applied it and patch 1.

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

[PATCH 3/3] KVM: MMU: Make drop_large_spte() more generic

[Takuya Yoshikawa]

There are many places where we drop large spte and we are always doing
much the same as drop_large_spte().

To avoid these duplications, this patch makes drop_large_spte() more
generically usable: it now takes an argument to know if it must flush
the remote tlbs and returns true or false depending on is_large_pte()
check result.

Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
---
 arch/x86/kvm/mmu.c         |   35 +++++++++++++++++------------------
 arch/x86/kvm/paging_tmpl.h |    4 ++--
 2 files changed, 19 insertions(+), 20 deletions(-)

diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index 5e761ff..2db12b3 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -1023,6 +1023,19 @@ static void drop_spte(struct kvm *kvm, u64 *sptep)
 		rmap_remove(kvm, sptep);
 }
 
+static bool drop_large_spte(struct kvm *kvm, u64 *sptep, bool flush_now)
+{
+	if (!is_large_pte(*sptep))
+		return false;
+
+	drop_spte(kvm, sptep);
+	--kvm->stat.lpages;
+
+	if (flush_now)
+		kvm_flush_remote_tlbs(kvm);
+	return true;
+}
+
 int kvm_mmu_rmap_write_protect(struct kvm *kvm, u64 gfn,
 			       struct kvm_memory_slot *slot)
 {
@@ -1052,8 +1065,7 @@ int kvm_mmu_rmap_write_protect(struct kvm *kvm, u64 gfn,
 			BUG_ON(!is_large_pte(*spte));
 			pgprintk("rmap_write_protect(large): spte %p %llx %lld\n", spte, *spte, gfn);
 			if (is_writable_pte(*spte)) {
-				drop_spte(kvm, spte);
-				--kvm->stat.lpages;
+				drop_large_spte(kvm, spte, false);
 				spte = NULL;
 				write_protected = 1;
 			}
@@ -1799,15 +1811,6 @@ static void link_shadow_page(u64 *sptep, struct kvm_mmu_page *sp)
 	mmu_spte_set(sptep, spte);
 }
 
-static void drop_large_spte(struct kvm_vcpu *vcpu, u64 *sptep)
-{
-	if (is_large_pte(*sptep)) {
-		drop_spte(vcpu->kvm, sptep);
-		--vcpu->kvm->stat.lpages;
-		kvm_flush_remote_tlbs(vcpu->kvm);
-	}
-}
-
 static void validate_direct_spte(struct kvm_vcpu *vcpu, u64 *sptep,
 				   unsigned direct_access)
 {
@@ -1839,9 +1842,8 @@ static bool mmu_page_zap_pte(struct kvm *kvm, struct kvm_mmu_page *sp,
 	pte = *spte;
 	if (is_shadow_present_pte(pte)) {
 		if (is_last_spte(pte, sp->role.level)) {
-			drop_spte(kvm, spte);
-			if (is_large_pte(pte))
-				--kvm->stat.lpages;
+			if (!drop_large_spte(kvm, spte, false))
+				drop_spte(kvm, spte);
 		} else {
 			child = page_header(pte & PT64_BASE_ADDR_MASK);
 			drop_parent_pte(child, spte);
@@ -3859,11 +3861,8 @@ void kvm_mmu_slot_remove_write_access(struct kvm *kvm, int slot)
 			      !is_last_spte(pt[i], sp->role.level))
 				continue;
 
-			if (is_large_pte(pt[i])) {
-				drop_spte(kvm, &pt[i]);
-				--kvm->stat.lpages;
+			if (drop_large_spte(kvm, &pt[i], false))
 				continue;
-			}
 
 			/* avoid RMW */
 			if (is_writable_pte(pt[i]))
diff --git a/arch/x86/kvm/paging_tmpl.h b/arch/x86/kvm/paging_tmpl.h
index 52e9d58..c40f074 100644
--- a/arch/x86/kvm/paging_tmpl.h
+++ b/arch/x86/kvm/paging_tmpl.h
@@ -498,7 +498,7 @@ static u64 *FNAME(fetch)(struct kvm_vcpu *vcpu, gva_t addr,
 		gfn_t table_gfn;
 
 		clear_sp_write_flooding_count(it.sptep);
-		drop_large_spte(vcpu, it.sptep);
+		drop_large_spte(vcpu->kvm, it.sptep, true);
 
 		sp = NULL;
 		if (!is_shadow_present_pte(*it.sptep)) {
@@ -526,7 +526,7 @@ static u64 *FNAME(fetch)(struct kvm_vcpu *vcpu, gva_t addr,
 		clear_sp_write_flooding_count(it.sptep);
 		validate_direct_spte(vcpu, it.sptep, direct_access);
 
-		drop_large_spte(vcpu, it.sptep);
+		drop_large_spte(vcpu->kvm, it.sptep, true);
 
 		if (is_shadow_present_pte(*it.sptep))
 			continue;
-- 
1.7.5.4

Re: [PATCH 3/3] KVM: MMU: Make drop_large_spte() more generic

[Avi Kivity]

On 11/29/2011 07:04 AM, Takuya Yoshikawa wrote:
> There are many places where we drop large spte and we are always doing
> much the same as drop_large_spte().
>
> To avoid these duplications, this patch makes drop_large_spte() more
> generically usable: it now takes an argument to know if it must flush
> the remote tlbs and returns true or false depending on is_large_pte()
> check result.

> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> index 5e761ff..2db12b3 100644
> --- a/arch/x86/kvm/mmu.c
> +++ b/arch/x86/kvm/mmu.c
> @@ -1023,6 +1023,19 @@ static void drop_spte(struct kvm *kvm, u64 *sptep)
>  		rmap_remove(kvm, sptep);
>  }
>  
> +static bool drop_large_spte(struct kvm *kvm, u64 *sptep, bool flush_now)
> +{
> +	if (!is_large_pte(*sptep))
> +		return false;
> +
> +	drop_spte(kvm, sptep);
> +	--kvm->stat.lpages;
> +
> +	if (flush_now)
> +		kvm_flush_remote_tlbs(kvm);
> +	return true;
> +}

I don't really like the pattern of adding more bools and conditionals
all over the place.  It makes the code harder to read and to execute.

I prefer separate functions like drop_large_spte() and
drop_large_spte_flush().

But it may be better to just drop kvm->stat, and open-code everything,
something like

   if (is_large_pte(*sptep))
       drop_spte(sptep)

is just as clear as drop_large_spte().

> @@ -1839,9 +1842,8 @@ static bool mmu_page_zap_pte(struct kvm *kvm, struct kvm_mmu_page *sp,
>  	pte = *spte;
>  	if (is_shadow_present_pte(pte)) {
>  		if (is_last_spte(pte, sp->role.level)) {
> -			drop_spte(kvm, spte);
> -			if (is_large_pte(pte))
> -				--kvm->stat.lpages;
> +			if (!drop_large_spte(kvm, spte, false))
> +				drop_spte(kvm, spte);

Now this is just confusing.


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

Re: [PATCH 0/3] KVM: MMU: Trivial fix and cleanups

[Avi Kivity]

On 11/29/2011 07:01 AM, Takuya Yoshikawa wrote:
> Made when I was reading mmu code.
>
> 	Takuya
>
> BTW, is threre any good way to test large page functionality?
>

Just booting a guest with transparent hugepages enabled is a good test. 
The guest kernel will use hugepages itself even if applications don't. 
With [en]pt = 0, the test is even harder, since kvm will have to remove
any large mappings in a frame that contains a guest page table.

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