[PATCH 0/8] KVM: MMU: reduce the size of shadow page structure and some cleanup

[PATCH 0/8] KVM: MMU: reduce the size of shadow page structure and some cleanup

[Xiao Guangrong]

In this patchset, we observably reduce the size of struct kvm_mmu_page and let
the code more simple.

And this patchset have already tested by unittest and
RHEL.6.1 setup/boot/reboot/shutdown.

[PATCH 1/8] KVM: MMU: combine unsync and unsync_children

[Xiao Guangrong]

unsync is only used for the page of level == 1 and unsync_children is only
used in the upper page, so combine them to reduce the size of shadow page
structure

Signed-off-by: Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com>
---
 Documentation/virtual/kvm/mmu.txt |    5 ++-
 arch/x86/include/asm/kvm_host.h   |   14 +++++++++++-
 arch/x86/kvm/mmu.c                |   39 +++++++++++++++++++++++++-----------
 arch/x86/kvm/mmu_audit.c          |    6 ++--
 arch/x86/kvm/mmutrace.h           |    3 +-
 arch/x86/kvm/paging_tmpl.h        |    2 +-
 6 files changed, 48 insertions(+), 21 deletions(-)

diff --git a/Documentation/virtual/kvm/mmu.txt b/Documentation/virtual/kvm/mmu.txt
index 5dc972c..4a5fedd 100644
--- a/Documentation/virtual/kvm/mmu.txt
+++ b/Documentation/virtual/kvm/mmu.txt
@@ -205,14 +205,15 @@ Shadow pages contain the following information:
     this page's spt.  Otherwise, parent_ptes points at a data structure
     with a list of parent_ptes.
   unsync:
+    It is used when role.level == 1, only level 1 shadow pages can be unsync.
     If true, then the translations in this page may not match the guest's
     translation.  This is equivalent to the state of the tlb when a pte is
     changed but before the tlb entry is flushed.  Accordingly, unsync ptes
     are synchronized when the guest executes invlpg or flushes its tlb by
     other means.  Valid for leaf pages.
   unsync_children:
-    How many sptes in the page point at pages that are unsync (or have
-    unsynchronized children).
+    It is used when role.level > 1 and indicates how many sptes in the page
+    point at unsync pages or unsynchronized children.
   unsync_child_bitmap:
     A bitmap indicating which sptes in spt point (directly or indirectly) at
     pages that may be unsynchronized.  Used to quickly locate all unsychronized
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 52d6640..c0a89cd 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -233,9 +233,19 @@ struct kvm_mmu_page {
 	 * in this shadow page.
 	 */
 	DECLARE_BITMAP(slot_bitmap, KVM_MEM_SLOTS_NUM);
-	bool unsync;
 	int root_count;          /* Currently serving as active root */
-	unsigned int unsync_children;
+
+	/*
+	 * If role.level == 1, unsync indicates whether the sp is
+	 * unsync, otherwise unsync_children indicates the number
+	 * of sptes which point at unsync sp or unsychronized children.
+	 * See sp_is_unsync() / sp_unsync_children_num.
+	 */
+	union {
+		bool unsync;
+		unsigned int unsync_children;
+	};
+
 	unsigned long parent_ptes;	/* Reverse mapping for parent_pte */
 	DECLARE_BITMAP(unsync_child_bitmap, 512);

diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index 2a2a9b4..6913a16 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -151,6 +151,21 @@ module_param(dbg, bool, 0644);

 #define SHADOW_PT_INDEX(addr, level) PT64_INDEX(addr, level)

+static bool sp_is_unsync(struct kvm_mmu_page *sp)
+{
+	return sp->role.level == PT_PAGE_TABLE_LEVEL && sp->unsync;
+}
+
+static unsigned int sp_unsync_children_num(struct kvm_mmu_page *sp)
+{
+	unsigned int num = 0;
+
+	if (sp->role.level != PT_PAGE_TABLE_LEVEL)
+		num = sp->unsync_children;
+
+	return num;
+}
+
 struct pte_list_desc {
 	u64 *sptes[PTE_LIST_EXT];
 	struct pte_list_desc *more;
@@ -1401,7 +1416,7 @@ static int mmu_pages_add(struct kvm_mmu_pages *pvec, struct kvm_mmu_page *sp,
 {
 	int i;

-	if (sp->unsync)
+	if (sp_is_unsync(sp))
 		for (i=0; i < pvec->nr; i++)
 			if (pvec->page[i].sp == sp)
 				return 0;
@@ -1426,7 +1441,7 @@ static int __mmu_unsync_walk(struct kvm_mmu_page *sp,

 		child = page_header(ent & PT64_BASE_ADDR_MASK);

-		if (child->unsync_children) {
+		if (sp_unsync_children_num(child)) {
 			if (mmu_pages_add(pvec, child, i))
 				return -ENOSPC;

@@ -1437,7 +1452,7 @@ static int __mmu_unsync_walk(struct kvm_mmu_page *sp,
 				nr_unsync_leaf += ret;
 			else
 				return ret;
-		} else if (child->unsync) {
+		} else if (sp_is_unsync(child)) {
 			nr_unsync_leaf++;
 			if (mmu_pages_add(pvec, child, i))
 				return -ENOSPC;
@@ -1468,7 +1483,7 @@ static int mmu_unsync_walk(struct kvm_mmu_page *sp,

 static void kvm_unlink_unsync_page(struct kvm *kvm, struct kvm_mmu_page *sp)
 {
-	WARN_ON(!sp->unsync);
+	WARN_ON(!sp_is_unsync(sp));
 	trace_kvm_mmu_sync_page(sp);
 	sp->unsync = 0;
 	--kvm->stat.mmu_unsync;
@@ -1546,7 +1561,7 @@ static void kvm_sync_pages(struct kvm_vcpu *vcpu,  gfn_t gfn)
 	bool flush = false;

 	for_each_gfn_indirect_valid_sp(vcpu->kvm, s, gfn, node) {
-		if (!s->unsync)
+		if (!sp_is_unsync(s))
 			continue;

 		WARN_ON(s->role.level != PT_PAGE_TABLE_LEVEL);
@@ -1699,20 +1714,20 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu,
 		role.quadrant = quadrant;
 	}
 	for_each_gfn_sp(vcpu->kvm, sp, gfn, node) {
-		if (!need_sync && sp->unsync)
+		if (!need_sync && sp_is_unsync(sp))
 			need_sync = true;

 		if (sp->role.word != role.word)
 			continue;

-		if (sp->unsync && kvm_sync_page_transient(vcpu, sp))
+		if (sp_is_unsync(sp) && kvm_sync_page_transient(vcpu, sp))
 			break;

 		mmu_page_add_parent_pte(vcpu, sp, parent_pte);
-		if (sp->unsync_children) {
+		if (sp_unsync_children_num(sp)) {
 			kvm_make_request(KVM_REQ_MMU_SYNC, vcpu);
 			kvm_mmu_mark_parents_unsync(sp);
-		} else if (sp->unsync)
+		} else if (sp_is_unsync(sp))
 			kvm_mmu_mark_parents_unsync(sp);

 		__clear_sp_write_flooding_count(sp);
@@ -1914,7 +1929,7 @@ static int kvm_mmu_prepare_zap_page(struct kvm *kvm, struct kvm_mmu_page *sp,
 	kvm_mmu_unlink_parents(kvm, sp);
 	if (!sp->role.invalid && !sp->role.direct)
 		unaccount_shadowed(kvm, sp->gfn);
-	if (sp->unsync)
+	if (sp_is_unsync(sp))
 		kvm_unlink_unsync_page(kvm, sp);
 	if (!sp->root_count) {
 		/* Count self */
@@ -2163,7 +2178,7 @@ static void kvm_unsync_pages(struct kvm_vcpu *vcpu,  gfn_t gfn)
 	struct hlist_node *node;

 	for_each_gfn_indirect_valid_sp(vcpu->kvm, s, gfn, node) {
-		if (s->unsync)
+		if (sp_is_unsync(s))
 			continue;
 		WARN_ON(s->role.level != PT_PAGE_TABLE_LEVEL);
 		__kvm_unsync_page(vcpu, s);
@@ -2184,7 +2199,7 @@ static int mmu_need_write_protect(struct kvm_vcpu *vcpu, gfn_t gfn,
 		if (s->role.level != PT_PAGE_TABLE_LEVEL)
 			return 1;

-		if (!need_unsync && !s->unsync) {
+		if (!need_unsync && !sp_is_unsync(s)) {
 			need_unsync = true;
 		}
 	}
diff --git a/arch/x86/kvm/mmu_audit.c b/arch/x86/kvm/mmu_audit.c
index fe15dcc..2106910 100644
--- a/arch/x86/kvm/mmu_audit.c
+++ b/arch/x86/kvm/mmu_audit.c
@@ -102,7 +102,7 @@ static void audit_mappings(struct kvm_vcpu *vcpu, u64 *sptep, int level)

 	sp = page_header(__pa(sptep));

-	if (sp->unsync) {
+	if (sp_is_unsync(sp)) {
 		if (level != PT_PAGE_TABLE_LEVEL) {
 			audit_printk(vcpu->kvm, "unsync sp: %p "
 				     "level = %d\n", sp, level);
@@ -168,7 +168,7 @@ static void audit_spte_after_sync(struct kvm_vcpu *vcpu, u64 *sptep, int level)
 {
 	struct kvm_mmu_page *sp = page_header(__pa(sptep));

-	if (vcpu->kvm->arch.audit_point == AUDIT_POST_SYNC && sp->unsync)
+	if (vcpu->kvm->arch.audit_point == AUDIT_POST_SYNC && sp_is_unsync(sp))
 		audit_printk(vcpu->kvm, "meet unsync sp(%p) after sync "
 			     "root.\n", sp);
 }
@@ -194,7 +194,7 @@ static void audit_write_protection(struct kvm *kvm, struct kvm_mmu_page *sp)
 	unsigned long *rmapp;
 	u64 *spte;

-	if (sp->role.direct || sp->unsync || sp->role.invalid)
+	if (sp->role.direct || sp_is_unsync(sp) || sp->role.invalid)
 		return;

 	slot = gfn_to_memslot(kvm, sp->gfn);
diff --git a/arch/x86/kvm/mmutrace.h b/arch/x86/kvm/mmutrace.h
index 89fb0e8..7fe9562 100644
--- a/arch/x86/kvm/mmutrace.h
+++ b/arch/x86/kvm/mmutrace.h
@@ -38,7 +38,8 @@
 			 role.invalid ? " invalid" : "",		\
 			 role.nxe ? "" : "!",				\
 			 __entry->root_count,				\
-			 __entry->unsync ? "unsync" : "sync", 0);	\
+			 role.level == 1 && __entry->unsync ?		\
+			 "unsync" : "sync", 0);				\
 	ret;								\
 		})

diff --git a/arch/x86/kvm/paging_tmpl.h b/arch/x86/kvm/paging_tmpl.h
index 1561028..7dacc80 100644
--- a/arch/x86/kvm/paging_tmpl.h
+++ b/arch/x86/kvm/paging_tmpl.h
@@ -691,7 +691,7 @@ static void FNAME(invlpg)(struct kvm_vcpu *vcpu, gva_t gva)
 			pt_element_t gpte;
 			gpa_t pte_gpa;

-			if (!sp->unsync)
+			if (!sp_is_unsync(sp))
 				break;

 			pte_gpa = FNAME(get_level1_sp_gpa)(sp);
-- 
1.7.7.4

Re: [PATCH 1/8] KVM: MMU: combine unsync and unsync_children

[Takuya Yoshikawa]

About naming issues in the kvm mmu code.

Not restricted to your patch series, so please take as a suggestion
for the future.

(2011/12/16 19:13), Xiao Guangrong wrote:
> +static bool sp_is_unsync(struct kvm_mmu_page *sp)
> +{
> +	return sp->role.level == PT_PAGE_TABLE_LEVEL&&  sp->unsync;
> +}

is_unsync_sp() is more consistent with others?
	e.g. is_large_pte(), is_writable_pte(), is_last_spte()


	Takuya

> +
> +static unsigned int sp_unsync_children_num(struct kvm_mmu_page *sp)
> +{
> +	unsigned int num = 0;
> +
> +	if (sp->role.level != PT_PAGE_TABLE_LEVEL)
> +		num = sp->unsync_children;
> +
> +	return num;
> +}
> +

Re: [PATCH 1/8] KVM: MMU: combine unsync and unsync_children

[Marcelo Tosatti]

On Fri, Dec 16, 2011 at 06:13:42PM +0800, Xiao Guangrong wrote:
> unsync is only used for the page of level == 1 and unsync_children is only
> used in the upper page, so combine them to reduce the size of shadow page
> structure
> 
> Signed-off-by: Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com>
> ---
>  Documentation/virtual/kvm/mmu.txt |    5 ++-
>  arch/x86/include/asm/kvm_host.h   |   14 +++++++++++-
>  arch/x86/kvm/mmu.c                |   39 +++++++++++++++++++++++++-----------
>  arch/x86/kvm/mmu_audit.c          |    6 ++--
>  arch/x86/kvm/mmutrace.h           |    3 +-
>  arch/x86/kvm/paging_tmpl.h        |    2 +-
>  6 files changed, 48 insertions(+), 21 deletions(-)
> 
> diff --git a/Documentation/virtual/kvm/mmu.txt b/Documentation/virtual/kvm/mmu.txt
> index 5dc972c..4a5fedd 100644
> --- a/Documentation/virtual/kvm/mmu.txt
> +++ b/Documentation/virtual/kvm/mmu.txt
> @@ -205,14 +205,15 @@ Shadow pages contain the following information:
>      this page's spt.  Otherwise, parent_ptes points at a data structure
>      with a list of parent_ptes.
>    unsync:
> +    It is used when role.level == 1, only level 1 shadow pages can be unsync.
>      If true, then the translations in this page may not match the guest's
>      translation.  This is equivalent to the state of the tlb when a pte is
>      changed but before the tlb entry is flushed.  Accordingly, unsync ptes
>      are synchronized when the guest executes invlpg or flushes its tlb by
>      other means.  Valid for leaf pages.
>    unsync_children:
> -    How many sptes in the page point at pages that are unsync (or have
> -    unsynchronized children).
> +    It is used when role.level > 1 and indicates how many sptes in the page
> +    point at unsync pages or unsynchronized children.
>    unsync_child_bitmap:
>      A bitmap indicating which sptes in spt point (directly or indirectly) at
>      pages that may be unsynchronized.  Used to quickly locate all unsychronized
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 52d6640..c0a89cd 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -233,9 +233,19 @@ struct kvm_mmu_page {
>  	 * in this shadow page.
>  	 */
>  	DECLARE_BITMAP(slot_bitmap, KVM_MEM_SLOTS_NUM);
> -	bool unsync;
>  	int root_count;          /* Currently serving as active root */
> -	unsigned int unsync_children;
> +
> +	/*
> +	 * If role.level == 1, unsync indicates whether the sp is
> +	 * unsync, otherwise unsync_children indicates the number
> +	 * of sptes which point at unsync sp or unsychronized children.
> +	 * See sp_is_unsync() / sp_unsync_children_num.
> +	 */
> +	union {
> +		bool unsync;
> +		unsigned int unsync_children;
> +	};
> +
>  	unsigned long parent_ptes;	/* Reverse mapping for parent_pte */
>  	DECLARE_BITMAP(unsync_child_bitmap, 512);
> 
> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> index 2a2a9b4..6913a16 100644
> --- a/arch/x86/kvm/mmu.c
> +++ b/arch/x86/kvm/mmu.c
> @@ -151,6 +151,21 @@ module_param(dbg, bool, 0644);
> 
>  #define SHADOW_PT_INDEX(addr, level) PT64_INDEX(addr, level)
> 
> +static bool sp_is_unsync(struct kvm_mmu_page *sp)
> +{
> +	return sp->role.level == PT_PAGE_TABLE_LEVEL && sp->unsync;
> +}
> +
> +static unsigned int sp_unsync_children_num(struct kvm_mmu_page *sp)
> +{
> +	unsigned int num = 0;
> +
> +	if (sp->role.level != PT_PAGE_TABLE_LEVEL)
> +		num = sp->unsync_children;
> +
> +	return num;
> +}

IIRC there are windows where unsync_children is not accurate. Have you 
verified this function is not called inside one of those windows?

Re: [PATCH 1/8] KVM: MMU: combine unsync and unsync_children

[Xiao Guangrong]

On 12/23/2011 12:06 AM, Marcelo Tosatti wrote:

> On Fri, Dec 16, 2011 at 06:13:42PM +0800, Xiao Guangrong wrote:
>> unsync is only used for the page of level == 1 and unsync_children is only
>> used in the upper page, so combine them to reduce the size of shadow page
>> structure
>>
>> Signed-off-by: Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com>
>> ---
>>  Documentation/virtual/kvm/mmu.txt |    5 ++-
>>  arch/x86/include/asm/kvm_host.h   |   14 +++++++++++-
>>  arch/x86/kvm/mmu.c                |   39 +++++++++++++++++++++++++-----------
>>  arch/x86/kvm/mmu_audit.c          |    6 ++--
>>  arch/x86/kvm/mmutrace.h           |    3 +-
>>  arch/x86/kvm/paging_tmpl.h        |    2 +-
>>  6 files changed, 48 insertions(+), 21 deletions(-)
>>
>> diff --git a/Documentation/virtual/kvm/mmu.txt b/Documentation/virtual/kvm/mmu.txt
>> index 5dc972c..4a5fedd 100644
>> --- a/Documentation/virtual/kvm/mmu.txt
>> +++ b/Documentation/virtual/kvm/mmu.txt
>> @@ -205,14 +205,15 @@ Shadow pages contain the following information:
>>      this page's spt.  Otherwise, parent_ptes points at a data structure
>>      with a list of parent_ptes.
>>    unsync:
>> +    It is used when role.level == 1, only level 1 shadow pages can be unsync.
>>      If true, then the translations in this page may not match the guest's
>>      translation.  This is equivalent to the state of the tlb when a pte is
>>      changed but before the tlb entry is flushed.  Accordingly, unsync ptes
>>      are synchronized when the guest executes invlpg or flushes its tlb by
>>      other means.  Valid for leaf pages.
>>    unsync_children:
>> -    How many sptes in the page point at pages that are unsync (or have
>> -    unsynchronized children).
>> +    It is used when role.level > 1 and indicates how many sptes in the page
>> +    point at unsync pages or unsynchronized children.
>>    unsync_child_bitmap:
>>      A bitmap indicating which sptes in spt point (directly or indirectly) at
>>      pages that may be unsynchronized.  Used to quickly locate all unsychronized
>> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
>> index 52d6640..c0a89cd 100644
>> --- a/arch/x86/include/asm/kvm_host.h
>> +++ b/arch/x86/include/asm/kvm_host.h
>> @@ -233,9 +233,19 @@ struct kvm_mmu_page {
>>  	 * in this shadow page.
>>  	 */
>>  	DECLARE_BITMAP(slot_bitmap, KVM_MEM_SLOTS_NUM);
>> -	bool unsync;
>>  	int root_count;          /* Currently serving as active root */
>> -	unsigned int unsync_children;
>> +
>> +	/*
>> +	 * If role.level == 1, unsync indicates whether the sp is
>> +	 * unsync, otherwise unsync_children indicates the number
>> +	 * of sptes which point at unsync sp or unsychronized children.
>> +	 * See sp_is_unsync() / sp_unsync_children_num.
>> +	 */
>> +	union {
>> +		bool unsync;
>> +		unsigned int unsync_children;
>> +	};
>> +
>>  	unsigned long parent_ptes;	/* Reverse mapping for parent_pte */
>>  	DECLARE_BITMAP(unsync_child_bitmap, 512);
>>
>> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
>> index 2a2a9b4..6913a16 100644
>> --- a/arch/x86/kvm/mmu.c
>> +++ b/arch/x86/kvm/mmu.c
>> @@ -151,6 +151,21 @@ module_param(dbg, bool, 0644);
>>
>>  #define SHADOW_PT_INDEX(addr, level) PT64_INDEX(addr, level)
>>
>> +static bool sp_is_unsync(struct kvm_mmu_page *sp)
>> +{
>> +	return sp->role.level == PT_PAGE_TABLE_LEVEL && sp->unsync;
>> +}
>> +
>> +static unsigned int sp_unsync_children_num(struct kvm_mmu_page *sp)
>> +{
>> +	unsigned int num = 0;
>> +
>> +	if (sp->role.level != PT_PAGE_TABLE_LEVEL)
>> +		num = sp->unsync_children;
>> +
>> +	return num;
>> +}
> 
> IIRC there are windows where unsync_children is not accurate. Have you 
> verified this function is not called inside one of those windows?
> 


Actually, this function is only used to see whether the sp has unsync children
in current code.

And this patch did not change the logic, it just use sp_unsync_children_num
instead of using sp->unsync_children directly.

Re: [PATCH 1/8] KVM: MMU: combine unsync and unsync_children

[Marcelo Tosatti]

On Fri, Dec 16, 2011 at 06:13:42PM +0800, Xiao Guangrong wrote:
> unsync is only used for the page of level == 1 and unsync_children is only
> used in the upper page, so combine them to reduce the size of shadow page
> structure
> 
> Signed-off-by: Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com>
> ---
>  Documentation/virtual/kvm/mmu.txt |    5 ++-
>  arch/x86/include/asm/kvm_host.h   |   14 +++++++++++-
>  arch/x86/kvm/mmu.c                |   39 +++++++++++++++++++++++++-----------
>  arch/x86/kvm/mmu_audit.c          |    6 ++--
>  arch/x86/kvm/mmutrace.h           |    3 +-
>  arch/x86/kvm/paging_tmpl.h        |    2 +-
>  6 files changed, 48 insertions(+), 21 deletions(-)
> 
> diff --git a/Documentation/virtual/kvm/mmu.txt b/Documentation/virtual/kvm/mmu.txt
> index 5dc972c..4a5fedd 100644
> --- a/Documentation/virtual/kvm/mmu.txt
> +++ b/Documentation/virtual/kvm/mmu.txt
> @@ -205,14 +205,15 @@ Shadow pages contain the following information:
>      this page's spt.  Otherwise, parent_ptes points at a data structure
>      with a list of parent_ptes.
>    unsync:
> +    It is used when role.level == 1, only level 1 shadow pages can be unsync.
>      If true, then the translations in this page may not match the guest's
>      translation.  This is equivalent to the state of the tlb when a pte is
>      changed but before the tlb entry is flushed.  Accordingly, unsync ptes
>      are synchronized when the guest executes invlpg or flushes its tlb by
>      other means.  Valid for leaf pages.
>    unsync_children:
> -    How many sptes in the page point at pages that are unsync (or have
> -    unsynchronized children).
> +    It is used when role.level > 1 and indicates how many sptes in the page
> +    point at unsync pages or unsynchronized children.
>    unsync_child_bitmap:
>      A bitmap indicating which sptes in spt point (directly or indirectly) at
>      pages that may be unsynchronized.  Used to quickly locate all unsychronized
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 52d6640..c0a89cd 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -233,9 +233,19 @@ struct kvm_mmu_page {
>  	 * in this shadow page.
>  	 */
>  	DECLARE_BITMAP(slot_bitmap, KVM_MEM_SLOTS_NUM);
> -	bool unsync;
>  	int root_count;          /* Currently serving as active root */
> -	unsigned int unsync_children;
> +
> +	/*
> +	 * If role.level == 1, unsync indicates whether the sp is
> +	 * unsync, otherwise unsync_children indicates the number
> +	 * of sptes which point at unsync sp or unsychronized children.
> +	 * See sp_is_unsync() / sp_unsync_children_num.
> +	 */
> +	union {
> +		bool unsync;
> +		unsigned int unsync_children;
> +	};
> +
>  	unsigned long parent_ptes;	/* Reverse mapping for parent_pte */
>  	DECLARE_BITMAP(unsync_child_bitmap, 512);
> 
> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> index 2a2a9b4..6913a16 100644
> --- a/arch/x86/kvm/mmu.c
> +++ b/arch/x86/kvm/mmu.c
> @@ -151,6 +151,21 @@ module_param(dbg, bool, 0644);
> 
>  #define SHADOW_PT_INDEX(addr, level) PT64_INDEX(addr, level)
> 
> +static bool sp_is_unsync(struct kvm_mmu_page *sp)
> +{
> +	return sp->role.level == PT_PAGE_TABLE_LEVEL && sp->unsync;
> +}
> +
> +static unsigned int sp_unsync_children_num(struct kvm_mmu_page *sp)
> +{
> +	unsigned int num = 0;
> +
> +	if (sp->role.level != PT_PAGE_TABLE_LEVEL)
> +		num = sp->unsync_children;
> +
> +	return num;
> +}

This adds significant complexity/trickery to the code for little gain.
For example, in kvm_mmu_page_get, you use sp_is_unsync() which depends
on sp->role.level, but that is not set yet.

In general, given the number of optimizations made to the shadow mmu
code, and the fact that over time NPT will dominate, i'd suggest time
should be spent improving other areas.

Re: [PATCH 1/8] KVM: MMU: combine unsync and unsync_children

[Xiao Guangrong]

On 01/09/2012 07:16 PM, Marcelo Tosatti wrote:


> In general, given the number of optimizations made to the shadow mmu
> code, and the fact that over time NPT will dominate, i'd suggest time
> should be spent improving other areas.
> 

I totally agree with you, thanks Marcelo!

[PATCH 2/8] KVM: MMU: set the dirty bit for the upper shadow page

[Xiao Guangrong]

Upper page do not need to be tracked the status bit, it is safe to set the
dirty and let cpu to happily prefetch it

Signed-off-by: Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com>
---
 arch/x86/kvm/mmu.c |   13 +++++--------
 1 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index 6913a16..a2d28aa 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -150,6 +150,9 @@ module_param(dbg, bool, 0644);
 #define SPTE_HOST_WRITEABLE (1ULL << PT_FIRST_AVAIL_BITS_SHIFT)

 #define SHADOW_PT_INDEX(addr, level) PT64_INDEX(addr, level)
+#define SHADOW_PAGE_TABLE						\
+	(PT_PRESENT_MASK | PT_WRITABLE_MASK | shadow_user_mask |	\
+	 shadow_x_mask | shadow_accessed_mask | shadow_dirty_mask)

 static bool sp_is_unsync(struct kvm_mmu_page *sp)
 {
@@ -1808,9 +1811,7 @@ static void link_shadow_page(u64 *sptep, struct kvm_mmu_page *sp)
 {
 	u64 spte;

-	spte = __pa(sp->spt)
-		| PT_PRESENT_MASK | PT_ACCESSED_MASK
-		| PT_WRITABLE_MASK | PT_USER_MASK;
+	spte = __pa(sp->spt) | SHADOW_PAGE_TABLE;
 	mmu_spte_set(sptep, spte);
 }

@@ -2497,11 +2498,7 @@ static int __direct_map(struct kvm_vcpu *vcpu, gpa_t v, int write,
 				return -ENOMEM;
 			}

-			mmu_spte_set(iterator.sptep,
-				     __pa(sp->spt)
-				     | PT_PRESENT_MASK | PT_WRITABLE_MASK
-				     | shadow_user_mask | shadow_x_mask
-				     | shadow_accessed_mask);
+			link_shadow_page(iterator.sptep, sp);
 		}
 	}
 	return emulate;
-- 
1.7.7.4

Re: [PATCH 2/8] KVM: MMU: set the dirty bit for the upper shadow page

[Marcelo Tosatti]

On Fri, Dec 16, 2011 at 06:14:24PM +0800, Xiao Guangrong wrote:
> Upper page do not need to be tracked the status bit, it is safe to set the
> dirty and let cpu to happily prefetch it
> 
> Signed-off-by: Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com>
> ---
>  arch/x86/kvm/mmu.c |   13 +++++--------
>  1 files changed, 5 insertions(+), 8 deletions(-)

Dirty bit is ignored in non-leaf pagetable entries.

Re: [PATCH 2/8] KVM: MMU: set the dirty bit for the upper shadow page

[Xiao Guangrong]

On 01/09/2012 07:30 PM, Marcelo Tosatti wrote:

> On Fri, Dec 16, 2011 at 06:14:24PM +0800, Xiao Guangrong wrote:
>> Upper page do not need to be tracked the status bit, it is safe to set the
>> dirty and let cpu to happily prefetch it
>>
>> Signed-off-by: Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com>
>> ---
>>  arch/x86/kvm/mmu.c |   13 +++++--------
>>  1 files changed, 5 insertions(+), 8 deletions(-)
> 
> Dirty bit is ignored in non-leaf pagetable entries.
> 

Sorry, i forgot it!

[PATCH 3/8] KVM: MMU: do not add a nonpresent spte to rmaps of its child

[Xiao Guangrong]

Set the spte before adding it to the rmap of its child so that all parent
spte are valid when propagate unsync bit from a usnync page / children page

And this feature is needed by the later patch

Signed-off-by: Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com>
---
 arch/x86/kvm/mmu.c         |   74 +++++++++++++++----------------------------
 arch/x86/kvm/mmutrace.h    |    2 +-
 arch/x86/kvm/paging_tmpl.h |   14 +++-----
 3 files changed, 32 insertions(+), 58 deletions(-)

diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index a2d28aa..89202f4 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -1321,12 +1321,14 @@ static unsigned kvm_page_table_hashfn(gfn_t gfn)
 	return gfn & ((1 << KVM_MMU_HASH_SHIFT) - 1);
 }

-static void mmu_page_add_parent_pte(struct kvm_vcpu *vcpu,
-				    struct kvm_mmu_page *sp, u64 *parent_pte)
+static void mmu_page_add_set_parent_pte(struct kvm_vcpu *vcpu,
+					struct kvm_mmu_page *sp,
+					u64 *parent_pte)
 {
 	if (!parent_pte)
 		return;

+	mmu_spte_set(parent_pte, __pa(sp->spt) | SHADOW_PAGE_TABLE);
 	pte_list_add(vcpu, parent_pte, &sp->parent_ptes);
 }

@@ -1357,7 +1359,7 @@ static struct kvm_mmu_page *kvm_mmu_alloc_page(struct kvm_vcpu *vcpu,
 	list_add(&sp->link, &vcpu->kvm->arch.active_mmu_pages);
 	bitmap_zero(sp->slot_bitmap, KVM_MEM_SLOTS_NUM);
 	sp->parent_ptes = 0;
-	mmu_page_add_parent_pte(vcpu, sp, parent_pte);
+	mmu_page_add_set_parent_pte(vcpu, sp, parent_pte);
 	kvm_mod_used_mmu_pages(vcpu->kvm, +1);
 	return sp;
 }
@@ -1690,13 +1692,10 @@ static void clear_sp_write_flooding_count(u64 *spte)
 	__clear_sp_write_flooding_count(sp);
 }

-static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu,
-					     gfn_t gfn,
-					     gva_t gaddr,
-					     unsigned level,
-					     int direct,
-					     unsigned access,
-					     u64 *parent_pte)
+static struct kvm_mmu_page *
+kvm_mmu_get_set_page(struct kvm_vcpu *vcpu, gfn_t gfn, gva_t gaddr,
+		     unsigned level, int direct, unsigned access,
+		     u64 *parent_pte)
 {
 	union kvm_mmu_page_role role;
 	unsigned quadrant;
@@ -1726,7 +1725,7 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu,
 		if (sp_is_unsync(sp) && kvm_sync_page_transient(vcpu, sp))
 			break;

-		mmu_page_add_parent_pte(vcpu, sp, parent_pte);
+		mmu_page_add_set_parent_pte(vcpu, sp, parent_pte);
 		if (sp_unsync_children_num(sp)) {
 			kvm_make_request(KVM_REQ_MMU_SYNC, vcpu);
 			kvm_mmu_mark_parents_unsync(sp);
@@ -1734,7 +1733,7 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu,
 			kvm_mmu_mark_parents_unsync(sp);

 		__clear_sp_write_flooding_count(sp);
-		trace_kvm_mmu_get_page(sp, false);
+		trace_kvm_mmu_get_set_page(sp, false);
 		return sp;
 	}
 	++vcpu->kvm->stat.mmu_cache_miss;
@@ -1754,7 +1753,7 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu,
 		account_shadowed(vcpu->kvm, gfn);
 	}
 	init_shadow_page_table(sp);
-	trace_kvm_mmu_get_page(sp, true);
+	trace_kvm_mmu_get_set_page(sp, true);
 	return sp;
 }

@@ -1807,14 +1806,6 @@ static void shadow_walk_next(struct kvm_shadow_walk_iterator *iterator)
 	return __shadow_walk_next(iterator, *iterator->sptep);
 }

-static void link_shadow_page(u64 *sptep, struct kvm_mmu_page *sp)
-{
-	u64 spte;
-
-	spte = __pa(sp->spt) | SHADOW_PAGE_TABLE;
-	mmu_spte_set(sptep, spte);
-}
-
 static void drop_large_spte(struct kvm_vcpu *vcpu, u64 *sptep)
 {
 	if (is_large_pte(*sptep)) {
@@ -1879,11 +1870,6 @@ static void kvm_mmu_page_unlink_children(struct kvm *kvm,
 		mmu_page_zap_pte(kvm, sp, sp->spt + i);
 }

-static void kvm_mmu_put_page(struct kvm_mmu_page *sp, u64 *parent_pte)
-{
-	mmu_page_remove_parent_pte(sp, parent_pte);
-}
-
 static void kvm_mmu_unlink_parents(struct kvm *kvm, struct kvm_mmu_page *sp)
 {
 	u64 *parent_pte;
@@ -2468,7 +2454,6 @@ static int __direct_map(struct kvm_vcpu *vcpu, gpa_t v, int write,
 			bool prefault)
 {
 	struct kvm_shadow_walk_iterator iterator;
-	struct kvm_mmu_page *sp;
 	int emulate = 0;
 	gfn_t pseudo_gfn;

@@ -2489,16 +2474,9 @@ static int __direct_map(struct kvm_vcpu *vcpu, gpa_t v, int write,

 			base_addr &= PT64_LVL_ADDR_MASK(iterator.level);
 			pseudo_gfn = base_addr >> PAGE_SHIFT;
-			sp = kvm_mmu_get_page(vcpu, pseudo_gfn, iterator.addr,
-					      iterator.level - 1,
-					      1, ACC_ALL, iterator.sptep);
-			if (!sp) {
-				pgprintk("nonpaging_map: ENOMEM\n");
-				kvm_release_pfn_clean(pfn);
-				return -ENOMEM;
-			}
-
-			link_shadow_page(iterator.sptep, sp);
+			kvm_mmu_get_set_page(vcpu, pseudo_gfn, iterator.addr,
+					     iterator.level - 1,
+					     1, ACC_ALL, iterator.sptep);
 		}
 	}
 	return emulate;
@@ -2713,8 +2691,8 @@ static int mmu_alloc_direct_roots(struct kvm_vcpu *vcpu)
 	if (vcpu->arch.mmu.shadow_root_level == PT64_ROOT_LEVEL) {
 		spin_lock(&vcpu->kvm->mmu_lock);
 		kvm_mmu_free_some_pages(vcpu);
-		sp = kvm_mmu_get_page(vcpu, 0, 0, PT64_ROOT_LEVEL,
-				      1, ACC_ALL, NULL);
+		sp = kvm_mmu_get_set_page(vcpu, 0, 0, PT64_ROOT_LEVEL,
+					  1, ACC_ALL, NULL);
 		++sp->root_count;
 		spin_unlock(&vcpu->kvm->mmu_lock);
 		vcpu->arch.mmu.root_hpa = __pa(sp->spt);
@@ -2725,10 +2703,10 @@ static int mmu_alloc_direct_roots(struct kvm_vcpu *vcpu)
 			ASSERT(!VALID_PAGE(root));
 			spin_lock(&vcpu->kvm->mmu_lock);
 			kvm_mmu_free_some_pages(vcpu);
-			sp = kvm_mmu_get_page(vcpu, i << (30 - PAGE_SHIFT),
-					      i << 30,
-					      PT32_ROOT_LEVEL, 1, ACC_ALL,
-					      NULL);
+			sp = kvm_mmu_get_set_page(vcpu, i << (30 - PAGE_SHIFT),
+						  i << 30,
+						  PT32_ROOT_LEVEL, 1, ACC_ALL,
+						  NULL);
 			root = __pa(sp->spt);
 			++sp->root_count;
 			spin_unlock(&vcpu->kvm->mmu_lock);
@@ -2764,8 +2742,8 @@ static int mmu_alloc_shadow_roots(struct kvm_vcpu *vcpu)

 		spin_lock(&vcpu->kvm->mmu_lock);
 		kvm_mmu_free_some_pages(vcpu);
-		sp = kvm_mmu_get_page(vcpu, root_gfn, 0, PT64_ROOT_LEVEL,
-				      0, ACC_ALL, NULL);
+		sp = kvm_mmu_get_set_page(vcpu, root_gfn, 0, PT64_ROOT_LEVEL,
+					  0, ACC_ALL, NULL);
 		root = __pa(sp->spt);
 		++sp->root_count;
 		spin_unlock(&vcpu->kvm->mmu_lock);
@@ -2798,9 +2776,9 @@ static int mmu_alloc_shadow_roots(struct kvm_vcpu *vcpu)
 		}
 		spin_lock(&vcpu->kvm->mmu_lock);
 		kvm_mmu_free_some_pages(vcpu);
-		sp = kvm_mmu_get_page(vcpu, root_gfn, i << 30,
-				      PT32_ROOT_LEVEL, 0,
-				      ACC_ALL, NULL);
+		sp = kvm_mmu_get_set_page(vcpu, root_gfn, i << 30,
+					  PT32_ROOT_LEVEL, 0,
+					  ACC_ALL, NULL);
 		root = __pa(sp->spt);
 		++sp->root_count;
 		spin_unlock(&vcpu->kvm->mmu_lock);
diff --git a/arch/x86/kvm/mmutrace.h b/arch/x86/kvm/mmutrace.h
index 7fe9562..f100078 100644
--- a/arch/x86/kvm/mmutrace.h
+++ b/arch/x86/kvm/mmutrace.h
@@ -145,7 +145,7 @@ TRACE_EVENT(
 );

 TRACE_EVENT(
-	kvm_mmu_get_page,
+	kvm_mmu_get_set_page,
 	TP_PROTO(struct kvm_mmu_page *sp, bool created),
 	TP_ARGS(sp, created),

diff --git a/arch/x86/kvm/paging_tmpl.h b/arch/x86/kvm/paging_tmpl.h
index 7dacc80..c79c503 100644
--- a/arch/x86/kvm/paging_tmpl.h
+++ b/arch/x86/kvm/paging_tmpl.h
@@ -503,8 +503,8 @@ static u64 *FNAME(fetch)(struct kvm_vcpu *vcpu, gva_t addr,
 		sp = NULL;
 		if (!is_shadow_present_pte(*it.sptep)) {
 			table_gfn = gw->table_gfn[it.level - 2];
-			sp = kvm_mmu_get_page(vcpu, table_gfn, addr, it.level-1,
-					      false, access, it.sptep);
+			sp = kvm_mmu_get_set_page(vcpu, table_gfn, addr,
+					it.level - 1, false, access, it.sptep);
 		}

 		/*
@@ -513,9 +513,6 @@ static u64 *FNAME(fetch)(struct kvm_vcpu *vcpu, gva_t addr,
 		 */
 		if (FNAME(gpte_changed)(vcpu, gw, it.level - 1))
 			goto out_gpte_changed;
-
-		if (sp)
-			link_shadow_page(it.sptep, sp);
 	}

 	for (;
@@ -533,9 +530,8 @@ static u64 *FNAME(fetch)(struct kvm_vcpu *vcpu, gva_t addr,

 		direct_gfn = gw->gfn & ~(KVM_PAGES_PER_HPAGE(it.level) - 1);

-		sp = kvm_mmu_get_page(vcpu, direct_gfn, addr, it.level-1,
-				      true, direct_access, it.sptep);
-		link_shadow_page(it.sptep, sp);
+		kvm_mmu_get_set_page(vcpu, direct_gfn, addr, it.level - 1,
+				     true, direct_access, it.sptep);
 	}

 	clear_sp_write_flooding_count(it.sptep);
@@ -548,7 +544,7 @@ static u64 *FNAME(fetch)(struct kvm_vcpu *vcpu, gva_t addr,

 out_gpte_changed:
 	if (sp)
-		kvm_mmu_put_page(sp, it.sptep);
+		drop_parent_pte(sp, it.sptep);
 	kvm_release_pfn_clean(pfn);
 	return NULL;
 }
-- 
1.7.7.4

Re: [PATCH 3/8] KVM: MMU: do not add a nonpresent spte to rmaps of its child

[Takuya Yoshikawa]

(2011/12/16 19:15), Xiao Guangrong wrote:

> -static void mmu_page_add_parent_pte(struct kvm_vcpu *vcpu,
> -				    struct kvm_mmu_page *sp, u64 *parent_pte)
> +static void mmu_page_add_set_parent_pte(struct kvm_vcpu *vcpu,
> +					struct kvm_mmu_page *sp,
> +					u64 *parent_pte)
>   {
>   	if (!parent_pte)
>   		return;
>
> +	mmu_spte_set(parent_pte, __pa(sp->spt) | SHADOW_PAGE_TABLE);
>   	pte_list_add(vcpu, parent_pte,&sp->parent_ptes);
>   }

There are a few prefixes in the kvm mmu code.

	e.g. mmu_page_, kvm_mmu_, kvm_mmu_page_, ...

Sometimes we also use "sp".

How about deciding a consistent way from now on?

E.g.
	if the function is static and for local use only, such a prefix
	can be eliminated,

	if it is used outside of mmu.c, kvm_mmu_ is needed,

	we use sp for kvm_mmu_page,
	...

(just an example)

	Takuya

Re: [PATCH 3/8] KVM: MMU: do not add a nonpresent spte to rmaps of its child

[Avi Kivity]

On 12/19/2011 04:39 AM, Takuya Yoshikawa wrote:
> (2011/12/16 19:15), Xiao Guangrong wrote:
>
>> -static void mmu_page_add_parent_pte(struct kvm_vcpu *vcpu,
>> -                    struct kvm_mmu_page *sp, u64 *parent_pte)
>> +static void mmu_page_add_set_parent_pte(struct kvm_vcpu *vcpu,
>> +                    struct kvm_mmu_page *sp,
>> +                    u64 *parent_pte)
>>   {
>>       if (!parent_pte)
>>           return;
>>
>> +    mmu_spte_set(parent_pte, __pa(sp->spt) | SHADOW_PAGE_TABLE);
>>       pte_list_add(vcpu, parent_pte,&sp->parent_ptes);
>>   }
>
> There are a few prefixes in the kvm mmu code.
>
>     e.g. mmu_page_, kvm_mmu_, kvm_mmu_page_, ...
>
> Sometimes we also use "sp".
>
> How about deciding a consistent way from now on?
>

In general I am in favour of consistency, but I'm not inviting
mass-renaming patches.  Let's try to be consistent in new patches (like
your suggestions on this patchset).

> E.g.
>     if the function is static and for local use only, such a prefix
>     can be eliminated,
>
>     if it is used outside of mmu.c, kvm_mmu_ is needed,
>
>     we use sp for kvm_mmu_page,
>     ...

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

[PATCH 4/8] KVM: MMU: drop unsync_child_bitmap

[Xiao Guangrong]

unsync_child_bitmap is used to record which spte has unsync page or unsync
children, we can set a free bit in the spte instead of it

Signed-off-by: Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com>
---
 Documentation/virtual/kvm/mmu.txt |    4 --
 arch/x86/include/asm/kvm_host.h   |    1 -
 arch/x86/kvm/mmu.c                |   77 +++++++++++++++++++++++++------------
 3 files changed, 52 insertions(+), 30 deletions(-)

diff --git a/Documentation/virtual/kvm/mmu.txt b/Documentation/virtual/kvm/mmu.txt
index 4a5fedd..6d70a6e 100644
--- a/Documentation/virtual/kvm/mmu.txt
+++ b/Documentation/virtual/kvm/mmu.txt
@@ -214,10 +214,6 @@ Shadow pages contain the following information:
   unsync_children:
     It is used when role.level > 1 and indicates how many sptes in the page
     point at unsync pages or unsynchronized children.
-  unsync_child_bitmap:
-    A bitmap indicating which sptes in spt point (directly or indirectly) at
-    pages that may be unsynchronized.  Used to quickly locate all unsychronized
-    pages reachable from a given page.

 Reverse map
 ===========
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index c0a89cd..601c7f6 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -247,7 +247,6 @@ struct kvm_mmu_page {
 	};

 	unsigned long parent_ptes;	/* Reverse mapping for parent_pte */
-	DECLARE_BITMAP(unsync_child_bitmap, 512);

 #ifdef CONFIG_X86_32
 	int clear_spte_count;
diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index 89202f4..9bd2084 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -147,7 +147,8 @@ module_param(dbg, bool, 0644);
 #define CREATE_TRACE_POINTS
 #include "mmutrace.h"

-#define SPTE_HOST_WRITEABLE (1ULL << PT_FIRST_AVAIL_BITS_SHIFT)
+#define SPTE_HOST_WRITEABLE	(1ULL << PT_FIRST_AVAIL_BITS_SHIFT)
+#define SPTE_UNSYNC_CHILD	(1ULL << (PT_FIRST_AVAIL_BITS_SHIFT + 1))

 #define SHADOW_PT_INDEX(addr, level) PT64_INDEX(addr, level)
 #define SHADOW_PAGE_TABLE						\
@@ -561,6 +562,40 @@ static void mmu_spte_clear_no_track(u64 *sptep)
 	__update_clear_spte_fast(sptep, 0ull);
 }

+static bool mmu_spte_mark_unsync_child(struct kvm_mmu_page *sp, u64 *sptep)
+{
+	u64 new_spte = *sptep;
+	bool unsync_child = new_spte & SPTE_UNSYNC_CHILD;
+
+	if (unsync_child)
+		return true;
+
+	new_spte |= SPTE_UNSYNC_CHILD;
+	__set_spte(sptep, new_spte);
+	return sp->unsync_children++;
+}
+
+static bool mmu_spte_is_unsync_child(u64 *sptep)
+{
+	return *sptep & SPTE_UNSYNC_CHILD;
+}
+
+static void __mmu_spte_clear_unsync_child(u64 *sptep)
+{
+	page_header(__pa(sptep))->unsync_children--;
+	WARN_ON((int)page_header(__pa(sptep))->unsync_children < 0);
+}
+
+static void mmu_spte_clear_unsync_child(u64 *sptep)
+{
+	u64 new_spte = *sptep;
+
+	if (new_spte & SPTE_UNSYNC_CHILD) {
+		__set_spte(sptep, new_spte & ~SPTE_UNSYNC_CHILD);
+		__mmu_spte_clear_unsync_child(sptep);
+	}
+}
+
 static u64 mmu_spte_get_lockless(u64 *sptep)
 {
 	return __get_spte_lockless(sptep);
@@ -1342,6 +1377,10 @@ static void drop_parent_pte(struct kvm_mmu_page *sp,
 			    u64 *parent_pte)
 {
 	mmu_page_remove_parent_pte(sp, parent_pte);
+
+	if (*parent_pte & SPTE_UNSYNC_CHILD)
+		__mmu_spte_clear_unsync_child(parent_pte);
+
 	mmu_spte_clear_no_track(parent_pte);
 }

@@ -1372,16 +1411,10 @@ static void kvm_mmu_mark_parents_unsync(struct kvm_mmu_page *sp)

 static void mark_unsync(u64 *spte)
 {
-	struct kvm_mmu_page *sp;
-	unsigned int index;
+	struct kvm_mmu_page *sp = page_header(__pa(spte));

-	sp = page_header(__pa(spte));
-	index = spte - sp->spt;
-	if (__test_and_set_bit(index, sp->unsync_child_bitmap))
-		return;
-	if (sp->unsync_children++)
-		return;
-	kvm_mmu_mark_parents_unsync(sp);
+	if (!mmu_spte_mark_unsync_child(sp, spte))
+		kvm_mmu_mark_parents_unsync(sp);
 }

 static int nonpaging_sync_page(struct kvm_vcpu *vcpu,
@@ -1411,10 +1444,9 @@ 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))
+#define for_each_unsync_children(sp, sptep, idx)			\
+	for (idx = 0; idx < 512 && ((sptep) = (sp)->spt + idx); idx++)	\
+		if (!mmu_spte_is_unsync_child(sptep)) {} else

 static int mmu_pages_add(struct kvm_mmu_pages *pvec, struct kvm_mmu_page *sp,
 			 int idx)
@@ -1435,14 +1467,14 @@ static int mmu_pages_add(struct kvm_mmu_pages *pvec, struct kvm_mmu_page *sp,
 static int __mmu_unsync_walk(struct kvm_mmu_page *sp,
 			   struct kvm_mmu_pages *pvec)
 {
+	u64 *spte;
 	int i, ret, nr_unsync_leaf = 0;

-	for_each_unsync_children(sp->unsync_child_bitmap, i) {
+	for_each_unsync_children(sp, spte, i) {
 		struct kvm_mmu_page *child;
-		u64 ent = sp->spt[i];
+		u64 ent = *spte;

-		if (!is_shadow_present_pte(ent) || is_large_pte(ent))
-			goto clear_child_bitmap;
+		WARN_ON(!is_shadow_present_pte(ent) || is_large_pte(ent));

 		child = page_header(ent & PT64_BASE_ADDR_MASK);

@@ -1467,12 +1499,9 @@ static int __mmu_unsync_walk(struct kvm_mmu_page *sp,
 		continue;

 clear_child_bitmap:
-		__clear_bit(i, sp->unsync_child_bitmap);
-		sp->unsync_children--;
-		WARN_ON((int)sp->unsync_children < 0);
+		mmu_spte_clear_unsync_child(spte);
 	}

-
 	return nr_unsync_leaf;
 }

@@ -1628,9 +1657,7 @@ static void mmu_pages_clear_parents(struct mmu_page_path *parents)
 		if (!sp)
 			return;

-		--sp->unsync_children;
-		WARN_ON((int)sp->unsync_children < 0);
-		__clear_bit(idx, sp->unsync_child_bitmap);
+		mmu_spte_clear_unsync_child(sp->spt + idx);
 		level++;
 	} while (level < PT64_ROOT_LEVEL-1 && !sp->unsync_children);
 }
-- 
1.7.7.4

Re: [PATCH 4/8] KVM: MMU: drop unsync_child_bitmap

[Avi Kivity]

On 12/16/2011 12:16 PM, Xiao Guangrong wrote:
> unsync_child_bitmap is used to record which spte has unsync page or unsync
> children, we can set a free bit in the spte instead of it
>

unsync_child_bitmap takes one cacheline; the shadow page table takes
64.  This will make unsync/resync much more expensive.

I suggest to put unsync_child_bitmap at the end (together with
unsync_children) and simply not allocate it when unneeded (have two
kmem_caches for the two cases).


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

Re: [PATCH 4/8] KVM: MMU: drop unsync_child_bitmap

[Xiao Guangrong]

On 12/18/2011 04:59 PM, Avi Kivity wrote:

> On 12/16/2011 12:16 PM, Xiao Guangrong wrote:
>> unsync_child_bitmap is used to record which spte has unsync page or unsync
>> children, we can set a free bit in the spte instead of it
>>
> 
> unsync_child_bitmap takes one cacheline; the shadow page table takes
> 64.  This will make unsync/resync much more expensive.
> 


Indeed, need more thing about this patch...

> I suggest to put unsync_child_bitmap at the end (together with
> unsync_children) and simply not allocate it when unneeded (have two
> kmem_caches for the two cases).
> 
> 


Good point, thanks Avi.

[PATCH 5/8] KVM: MMU: optimize walking unsync shadow page

[Xiao Guangrong]

Exactly, unysnc_children is the number of unsync sptes, we can use to
avoid unnecessary spte fetching

Signed-off-by: Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com>
---
 arch/x86/kvm/mmu.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index 9bd2084..16e0642 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -1468,12 +1468,15 @@ static int __mmu_unsync_walk(struct kvm_mmu_page *sp,
 			   struct kvm_mmu_pages *pvec)
 {
 	u64 *spte;
-	int i, ret, nr_unsync_leaf = 0;
+	int i, ret, nr_unsync_leaf = 0, unysnc_children = sp->unsync_children;

 	for_each_unsync_children(sp, spte, i) {
 		struct kvm_mmu_page *child;
 		u64 ent = *spte;

+		if (!unysnc_children--)
+			break;
+
 		WARN_ON(!is_shadow_present_pte(ent) || is_large_pte(ent));

 		child = page_header(ent & PT64_BASE_ADDR_MASK);
-- 
1.7.7.4

[PATCH 6/8] KVM: MMU: optimize handing invlpg

[Xiao Guangrong]

Use unsync bit to see if the spte is point to unsync child

Signed-off-by: Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com>
---
 arch/x86/kvm/paging_tmpl.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kvm/paging_tmpl.h b/arch/x86/kvm/paging_tmpl.h
index c79c503..5333256 100644
--- a/arch/x86/kvm/paging_tmpl.h
+++ b/arch/x86/kvm/paging_tmpl.h
@@ -706,7 +706,7 @@ static void FNAME(invlpg)(struct kvm_vcpu *vcpu, gva_t gva)
 			FNAME(update_pte)(vcpu, sp, sptep, &gpte);
 		}

-		if (!is_shadow_present_pte(*sptep) || !sp->unsync_children)
+		if (!mmu_spte_is_unsync_child(sptep))
 			break;
 	}
 	spin_unlock(&vcpu->kvm->mmu_lock);
-- 
1.7.7.4

[PATCH 7/8] KVM: MMU: remove the redundant get_written_sptes

[Xiao Guangrong]

get_written_sptes is called twice in kvm_mmu_pte_write, one of them can be
removed

Signed-off-by: Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com>
---
 arch/x86/kvm/mmu.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index 16e0642..5d0f0e3 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -3575,7 +3575,7 @@ static u64 mmu_pte_write_fetch_gpte(struct kvm_vcpu *vcpu, gpa_t *gpa,
  * If we're seeing too many writes to a page, it may no longer be a page table,
  * or we may be forking, in which case it is better to unmap the page.
  */
-static bool detect_write_flooding(struct kvm_mmu_page *sp, u64 *spte)
+static bool detect_write_flooding(struct kvm_mmu_page *sp)
 {
 	/*
 	 * Skip write-flooding detected for the sp whose level is 1, because
@@ -3684,10 +3684,8 @@ void kvm_mmu_pte_write(struct kvm_vcpu *vcpu, gpa_t gpa,

 	mask.cr0_wp = mask.cr4_pae = mask.nxe = 1;
 	for_each_gfn_indirect_valid_sp(vcpu->kvm, sp, gfn, node) {
-		spte = get_written_sptes(sp, gpa, &npte);
-
 		if (detect_write_misaligned(sp, gpa, bytes) ||
-		      detect_write_flooding(sp, spte)) {
+		      detect_write_flooding(sp)) {
 			zap_page |= !!kvm_mmu_prepare_zap_page(vcpu->kvm, sp,
 						     &invalid_list);
 			++vcpu->kvm->stat.mmu_flooded;
-- 
1.7.7.4

Re: [PATCH 7/8] KVM: MMU: remove the redundant get_written_sptes

[Marcelo Tosatti]

On Fri, Dec 16, 2011 at 06:18:10PM +0800, Xiao Guangrong wrote:
> get_written_sptes is called twice in kvm_mmu_pte_write, one of them can be
> removed
> 
> Signed-off-by: Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com>
> ---
>  arch/x86/kvm/mmu.c |    6 ++----
>  1 files changed, 2 insertions(+), 4 deletions(-)

Applied, thanks.

[PATCH 8/8] KVM: MMU: remove PT64_SECOND_AVAIL_BITS_SHIFT

[Xiao Guangrong]

It is not used, remove it

Signed-off-by: Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com>
---
 arch/x86/kvm/mmu.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index 5d0f0e3..234a32e 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -91,7 +91,6 @@ module_param(dbg, bool, 0644);
 #define PTE_PREFETCH_NUM		8

 #define PT_FIRST_AVAIL_BITS_SHIFT 9
-#define PT64_SECOND_AVAIL_BITS_SHIFT 52

 #define PT64_LEVEL_BITS 9

-- 
1.7.7.4

Re: [PATCH 8/8] KVM: MMU: remove PT64_SECOND_AVAIL_BITS_SHIFT

[Avi Kivity]

On 12/16/2011 12:18 PM, Xiao Guangrong wrote:
> It is not used, remove it
>
> Signed-off-by: Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com>
> ---
>  arch/x86/kvm/mmu.c |    1 -
>  1 files changed, 0 insertions(+), 1 deletions(-)
>
> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> index 5d0f0e3..234a32e 100644
> --- a/arch/x86/kvm/mmu.c
> +++ b/arch/x86/kvm/mmu.c
> @@ -91,7 +91,6 @@ module_param(dbg, bool, 0644);
>  #define PTE_PREFETCH_NUM		8
>
>  #define PT_FIRST_AVAIL_BITS_SHIFT 9
> -#define PT64_SECOND_AVAIL_BITS_SHIFT 52
>
>

Don't see a reason to drop it, we may use it some day.

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

Re: [PATCH 8/8] KVM: MMU: remove PT64_SECOND_AVAIL_BITS_SHIFT

[Xiao Guangrong]

On 12/18/2011 06:42 PM, Avi Kivity wrote:

> On 12/16/2011 12:18 PM, Xiao Guangrong wrote:
>> It is not used, remove it
>>
>> Signed-off-by: Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com>
>> ---
>>  arch/x86/kvm/mmu.c |    1 -
>>  1 files changed, 0 insertions(+), 1 deletions(-)
>>
>> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
>> index 5d0f0e3..234a32e 100644
>> --- a/arch/x86/kvm/mmu.c
>> +++ b/arch/x86/kvm/mmu.c
>> @@ -91,7 +91,6 @@ module_param(dbg, bool, 0644);
>>  #define PTE_PREFETCH_NUM		8
>>
>>  #define PT_FIRST_AVAIL_BITS_SHIFT 9
>> -#define PT64_SECOND_AVAIL_BITS_SHIFT 52
>>
>>
> 
> Don't see a reason to drop it, we may use it some day.
> 


Anyway, it is not used in currently code, I do not have strong opinion on it :)
Please ignore it if you do not like.

Re: [PATCH 0/8] KVM: MMU: reduce the size of shadow page structure and some cleanup

[Xiao Guangrong]

On 12/16/2011 06:13 PM, Xiao Guangrong wrote:

> In this patchset, we observably reduce the size of struct kvm_mmu_page and let
> the code more simple.
> 
> And this patchset have already tested by unittest and
> RHEL.6.1 setup/boot/reboot/shutdown.


Hi Avi, Marcelo,

Could you please apply patch 1, 2, 7 if they are good for you? :)