KVM: MMU: Replace role.glevels with role.cr4_pae

PATCH Applied Avi Kivity 2 participants 5 messages Apr 14, 2010 – Apr 15, 2010

Subsystems (3)

x86 KVM KVM x86

lore

Share & tools

lore

Avi Kivity

author

2010-04-14 16:20 UTC #0 lore raw

PATCH 1/1 KVM: MMU: Replace role.glevels with role.cr4_pae

There is no real distinction between glevels=3 and glevels=4; both have exactly the same format and the code is treated exactly the same way. Drop role.glevels and replace is with role.cr4_pae (which is meaningful). This simplifies the code a bit.

As a side effect, it allows sharing shadow page tables between pae and longmode guest page tables at the same guest page.

Signed-off-by Avi Kivity <[email protected]>

---
 arch/x86/include/asm/kvm_host.h |    2 +-
 arch/x86/kvm/mmu.c              |   12 ++++++------
 arch/x86/kvm/mmutrace.h         |    5 +++--
 3 files changed, 10 insertions(+), 9 deletions(-)

arch/x86/include/asm/kvm_host.h +1 -1

index 0c49c88..90530a6 100644
@@ -171,8 +171,8 @@ struct kvm_pte_chain {
 union kvm_mmu_page_role {
 	unsigned word;
 	struct {
-		unsigned glevels:4;
 		unsigned level:4;
+		unsigned cr4_pae:1;
 		unsigned quadrant:2;
 		unsigned pad_for_nice_hex_output:6;
 		unsigned direct:1;

arch/x86/kvm/mmu.c +6 -6

index b44380b..edfef80 100644
@@ -1205,7 +1205,7 @@ static int kvm_mmu_zap_page(struct kvm *kvm, struct kvm_mmu_page *sp);
 
 static int kvm_sync_page(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp)
 {
-	if (sp->role.glevels != vcpu->arch.mmu.root_level) {
+	if (sp->role.cr4_pae != !!is_pae(vcpu)) {
 		kvm_mmu_zap_page(vcpu->kvm, sp);
 		return 1;
 	}

@@ -1328,7 +1328,7 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu,
 	role.level = level;
 	role.direct = direct;
 	if (role.direct)
-		role.glevels = 0;
+		role.cr4_pae = 0;
 	role.access = access;
 	if (vcpu->arch.mmu.root_level <= PT32_ROOT_LEVEL) {
 		quadrant = gaddr >> (PAGE_SHIFT + (PT64_PT_BITS * level));

@@ -2439,7 +2439,7 @@ static int init_kvm_softmmu(struct kvm_vcpu *vcpu)
 	else
 		r = paging32_init_context(vcpu);
 
-	vcpu->arch.mmu.base_role.glevels = vcpu->arch.mmu.root_level;
+	vcpu->arch.mmu.base_role.cr4_pae = !!is_pae(vcpu);
 
 	return r;
 }

@@ -2528,7 +2528,7 @@ static void mmu_pte_write_new_pte(struct kvm_vcpu *vcpu,
         }
 
 	++vcpu->kvm->stat.mmu_pte_updated;
-	if (sp->role.glevels == PT32_ROOT_LEVEL)
+	if (!sp->role.cr4_pae)
 		paging32_update_pte(vcpu, sp, spte, new);
 	else
 		paging64_update_pte(vcpu, sp, spte, new);

@@ -2677,7 +2677,7 @@ void kvm_mmu_pte_write(struct kvm_vcpu *vcpu, gpa_t gpa,
 	hlist_for_each_entry_safe(sp, node, n, bucket, hash_link) {
 		if (sp->gfn != gfn || sp->role.direct || sp->role.invalid)
 			continue;
-		pte_size = sp->role.glevels == PT32_ROOT_LEVEL ? 4 : 8;
+		pte_size = sp->role.cr4_pae ? 8 : 4;
 		misaligned = (offset ^ (offset + bytes - 1)) & ~(pte_size - 1);
 		misaligned |= bytes < 4;
 		if (misaligned || flooded) {

@@ -2701,7 +2701,7 @@ void kvm_mmu_pte_write(struct kvm_vcpu *vcpu, gpa_t gpa,
 		page_offset = offset;
 		level = sp->role.level;
 		npte = 1;
-		if (sp->role.glevels == PT32_ROOT_LEVEL) {
+		if (!sp->role.cr4_pae) {
 			page_offset <<= 1;	/* 32->64 */
 			/*
 			 * A 32-bit pde maps 4MB while the shadow pdes map

arch/x86/kvm/mmutrace.h +3 -2

index 1fe956a..3851f1f 100644
@@ -28,9 +28,10 @@
 								        \
 	role.word = __entry->role;					\
 									\
-	trace_seq_printf(p, "sp gfn %llx %u/%u q%u%s %s%s %spge"	\
+	trace_seq_printf(p, "sp gfn %llx %u%s q%u%s %s%s %spge"		\
 			 " %snxe root %u %s%c",				\
-			 __entry->gfn, role.level, role.glevels,	\
+			 __entry->gfn, role.level,			\
+			 role.cr4_pae ? " pae" : "",			\
 			 role.quadrant,					\
 			 role.direct ? " direct" : "",			\
 			 access_str[role.access],			\

Signature · 1 line

1.7.0.4

Avi Kivity

author

2010-04-14 17:16 UTC #1 lore raw

Quoted 13 lines · Avi Kivity

There is no real distinction between glevels=3 and glevels=4; both have exactly the same format and the code is treated exactly the same way. Drop role.glevels and replace is with role.cr4_pae (which is meaningful). This simplifies the code a bit.
As a side effect, it allows sharing shadow page tables between pae and longmode guest page tables at the same guest page.
  static int kvm_sync_page(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp)
  {

arch/x86/kvm/mmu.c static int kvm_mmu_zap_page(struct kvm *kvm, struct kvm_mmu_page *sp); · line 1211

Quoted 5 lines

arch/x86/kvm/mmu.c +1 -1

-	if (sp->role.glevels != vcpu->arch.mmu.root_level) {
+	if (sp->role.cr4_pae != !!is_pae(vcpu)) {
  		kvm_mmu_zap_page(vcpu->kvm, sp);
  		return 1;
  	}

This bit confuses me a little. Why is it needed? It will never hit from mmu_sync_children(), and as for kvm_mmu_get_page(), it will simply zap unrelated pages?

Is it related to the restriction that we can only unsync if we have just one shadow page for a gfn? That's somewhat artificial (and hurts nonpae guests, and guests with linear page tables).

Signature · 1 line

error compiling committee.c: too many arguments to function

Marcelo Tosatti

2010-04-14 18:30 UTC #2 lore raw

Quoted 10 lines · Avi Kivity

Quoted 9 lines · Avi Kivity
There is no real distinction between glevels=3 and glevels=4; both have exactly the same format and the code is treated exactly the same way. Drop role.glevels and replace is with role.cr4_pae (which is meaningful). This simplifies the code a bit.
As a side effect, it allows sharing shadow page tables between pae and longmode guest page tables at the same guest page.

Quoted 11 lines

arch/x86/kvm/mmu.c static int kvm_mmu_zap_page(struct kvm *kvm, struct kvm_mmu_page *sp); · line 1211
Quoted 7 lines
static int kvm_sync_page(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp) {
arch/x86/kvm/mmu.c +1 -1
-	if (sp->role.glevels != vcpu->arch.mmu.root_level) {
+	if (sp->role.cr4_pae != !!is_pae(vcpu)) {
 		kvm_mmu_zap_page(vcpu->kvm, sp);
 		return 1;
 	}
This bit confuses me a little. Why is it needed? It will never hit from mmu_sync_children(), and as for kvm_mmu_get_page(), it will simply zap unrelated pages?

kvm_mmu_get_page is write protecting a gfn. If there's shadow for a different role, and its unsync, it needs to be synchronized.

Perhaps it could call the appropriate _sync_page version instead of zapping, similar to mmu_pte_write_new_pte.

Quoted 3 lines

Is it related to the restriction that we can only unsync if we have just one shadow page for a gfn? That's somewhat artificial (and hurts nonpae guests, and guests with linear page tables).

If gfn is shadowed at PMD or higher level, you can't unsync the PTE shadow.

Avi Kivity

author

2010-04-15 09:02 UTC #3 lore raw

Quoted 25 lines · Marcelo Tosatti

Quoted 11 lines · Avi Kivity
Quoted 8 lines · Avi Kivity
There is no real distinction between glevels=3 and glevels=4; both have exactly the same format and the code is treated exactly the same way. Drop role.glevels and replace is with role.cr4_pae (which is meaningful). This simplifies the code a bit.
As a side effect, it allows sharing shadow page tables between pae and longmode guest page tables at the same guest page.
Quoted 12 lines
arch/x86/kvm/mmu.c static int kvm_mmu_zap_page(struct kvm *kvm, struct kvm_mmu_page *sp); · line 1211
Quoted 8 lines
  static int kvm_sync_page(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp)
  {
arch/x86/kvm/mmu.c +1 -1
-	if (sp->role.glevels != vcpu->arch.mmu.root_level) {
+	if (sp->role.cr4_pae != !!is_pae(vcpu)) {
  		kvm_mmu_zap_page(vcpu->kvm, sp);
  		return 1;
  	}
This bit confuses me a little. Why is it needed? It will never hit from mmu_sync_children(), and as for kvm_mmu_get_page(), it will simply zap unrelated pages?

Quoted 1 line

kvm_mmu_get_page is write protecting a gfn.

Took me a while to figure out why.

Quoted 4 lines

If there's shadow for a
differ ent role, and its unsync, it needs to be synchronized.

We could leave it unsync and write protected, though that destroys an invariant (sync==protected, unsync==unprotected), and all the calls to rmap_write_protect() become confused.

Quoted 3 lines

Perhaps it could call the appropriate _sync_page version instead of zapping, similar to mmu_pte_write_new_pte.

Probably better for nonpae.

Quoted 7 lines

Quoted 4 lines
Is it related to the restriction that we can only unsync if we have just one shadow page for a gfn? That's somewhat artificial (and hurts nonpae guests, and guests with linear page tables).
If gfn is shadowed at PMD or higher level, you can't unsync the PTE shadow.

Yes. Even if we could, invlpg is defined to drop all PDE caches (except large page PDEs), so we would have to resync all those pages on invlpg.

Signature · 2 lines

I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Marcelo Tosatti

Applied

2010-04-15 17:23 UTC #4 lore raw

In reply to Avi Kivity

Quoted 9 lines · Avi Kivity

There is no real distinction between glevels=3 and glevels=4; both have exactly the same format and the code is treated exactly the same way. Drop role.glevels and replace is with role.cr4_pae (which is meaningful). This simplifies the code a bit.
As a side effect, it allows sharing shadow page tables between pae and longmode guest page tables at the same guest page.
Signed-off-by Avi Kivity <[email protected]>

Applied, thanks.