Skip to contentSkip to conversation

KVM: MMU: Add shadow_accessed_shift

PATCH In review Yang, Sheng 3 participants 3 messages Aug 29, 2008

Subsystems (3)
x86 KVM KVM x86
lore
Share & tools
lore

Yang, Sheng

author
#0 lore raw

PATCH 1/1 KVM: MMU: Add shadow_accessed_shift

From: Sheng Yang <[email protected]>
Date: Fri, 29 Aug 2008 14:02:29 +0800
Subject: [PATCH] KVM: MMU: Add shadow_accessed_shift

Signed-off-by Sheng Yang <[email protected]>
---
 arch/x86/kvm/mmu.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)
arch/x86/kvm/mmu.c +1 -0
index 3008279..0997d82 100644
@@ -151,6 +151,7 @@ static u64 __read_mostly shadow_nx_mask;
 static u64 __read_mostly shadow_x_mask;	/* mutual exclusive with nx_mask */
 static u64 __read_mostly shadow_user_mask;
 static u64 __read_mostly shadow_accessed_mask;
+static u16 __read_mostly shadow_accessed_shift;
 static u64 __read_mostly shadow_dirty_mask;
 
 void kvm_mmu_set_nonpresent_ptes(u64 trap_pte, u64 notrap_pte)

@@ -171,6 +172,8 @@ void kvm_mmu_set_mask_ptes(u64 user_mask, u64

accessed_mask,
{
shadow_user_mask = user_mask;
shadow_accessed_mask = accessed_mask;
+ shadow_accessed_shift = find_first_bit((unsigned long *)&accessed_mask,
+ sizeof(accessed_mask));
shadow_dirty_mask = dirty_mask;
shadow_nx_mask = nx_mask;
shadow_x_mask = x_mask;
@@ -709,10 +712,10 @@ static int kvm_age_rmapp(struct kvm *kvm, unsigned long *rmapp)
int _young;
u64 _spte = *spte;
BUG_ON(!(_spte & PT_PRESENT_MASK));
- _young = _spte & PT_ACCESSED_MASK;
+ _young = _spte & shadow_accessed_mask;
if (_young) {
young = 1;
- clear_bit(PT_ACCESSED_SHIFT, (unsigned long *)spte);
+ clear_bit(shadow_accessed_shift, (unsigned long *)spte);
}
spte = rmap_next(kvm, rmapp, spte);
}
@@ -1785,7 +1788,7 @@ static void kvm_mmu_access_page(struct kvm_vcpu *vcpu, gfn_t gfn)
&& shadow_accessed_mask
&& !(*spte & shadow_accessed_mask)
&& is_shadow_present_pte(*spte))
- set_bit(PT_ACCESSED_SHIFT, (unsigned long *)spte);
+ set_bit(shadow_accessed_shift, (unsigned long *)spte);
}

void kvm_mmu_pte_write(struct kvm_vcpu *vcpu, gpa_t gpa,
--
1.5.4.5

From 3a2cc947a656a6eb4e815e64a44cb3e77a162a89 Mon Sep 17 00:00:00 2001
From: Sheng Yang <[email protected]>
Date: Fri, 29 Aug 2008 14:02:29 +0800
Subject: [PATCH] KVM: MMU: Add shadow_accessed_shift

Signed-off-by Sheng Yang <[email protected]>
---
 arch/x86/kvm/mmu.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)
arch/x86/kvm/mmu.c +6 -3
index 3008279..0997d82 100644
@@ -151,6 +151,7 @@ static u64 __read_mostly shadow_nx_mask;
 static u64 __read_mostly shadow_x_mask;	/* mutual exclusive with nx_mask */
 static u64 __read_mostly shadow_user_mask;
 static u64 __read_mostly shadow_accessed_mask;
+static u16 __read_mostly shadow_accessed_shift;
 static u64 __read_mostly shadow_dirty_mask;
 
 void kvm_mmu_set_nonpresent_ptes(u64 trap_pte, u64 notrap_pte)

@@ -171,6 +172,8 @@ void kvm_mmu_set_mask_ptes(u64 user_mask, u64 accessed_mask,
 {
 	shadow_user_mask = user_mask;
 	shadow_accessed_mask = accessed_mask;
+	shadow_accessed_shift = find_first_bit((unsigned long *)&accessed_mask,
+					       sizeof(accessed_mask));
 	shadow_dirty_mask = dirty_mask;
 	shadow_nx_mask = nx_mask;
 	shadow_x_mask = x_mask;

@@ -709,10 +712,10 @@ static int kvm_age_rmapp(struct kvm *kvm, unsigned long *rmapp)
 		int _young;
 		u64 _spte = *spte;
 		BUG_ON(!(_spte & PT_PRESENT_MASK));
-		_young = _spte & PT_ACCESSED_MASK;
+		_young = _spte & shadow_accessed_mask;
 		if (_young) {
 			young = 1;
-			clear_bit(PT_ACCESSED_SHIFT, (unsigned long *)spte);
+			clear_bit(shadow_accessed_shift, (unsigned long *)spte);
 		}
 		spte = rmap_next(kvm, rmapp, spte);
 	}

@@ -1785,7 +1788,7 @@ static void kvm_mmu_access_page(struct kvm_vcpu *vcpu, gfn_t gfn)
 	    && shadow_accessed_mask
 	    && !(*spte & shadow_accessed_mask)
 	    && is_shadow_present_pte(*spte))
-		set_bit(PT_ACCESSED_SHIFT, (unsigned long *)spte);
+		set_bit(shadow_accessed_shift, (unsigned long *)spte);
 }
 
 void kvm_mmu_pte_write(struct kvm_vcpu *vcpu, gpa_t gpa,
Signature · 1 line
1.5.4.5
#1 lore raw
Quoted 9 lines · Sheng

From: Sheng Yang <[email protected]>
Date: Fri, 29 Aug 2008 14:02:29 +0800
Subject: [PATCH] KVM: MMU: Add shadow_accessed_shift

static u64 __read_mostly shadow_dirty_mask;

void kvm_mmu_set_nonpresent_ptes(u64 trap_pte, u64 notrap_pte)

arch/x86/kvm/mmu.c void kvm_mmu_set_mask_ptes(u64 user_mask, u64 · line 178

Quoted 7 lines
arch/x86/kvm/mmu.c +2 -0
@@ -171,6 +172,8 @@ void kvm_mmu_set_mask_ptes(u64 user_mask, u64 
 accessed_mask,
 {
 	shadow_user_mask = user_mask;
 	shadow_accessed_mask = accessed_mask;
+	shadow_accessed_shift = find_first_bit((unsigned long *)&accessed_mask,
+					       sizeof(accessed_mask));

Ah, I thought ept accessed_mask was zero.

arch/x86/kvm/mmu.c static int kvm_age_rmapp(struct kvm *kvm, unsigned long · line 717

Quoted 11 lines
arch/x86/kvm/mmu.c +0 -0
 	shadow_dirty_mask = dirty_mask;
arch/x86/kvm/mmu.c +1 -1
 	shadow_nx_mask = nx_mask;
 	shadow_x_mask = x_mask;

@@ -709,10 +712,10 @@ static int kvm_age_rmapp(struct kvm *kvm, unsigned long 
 *rmapp)
 		int _young;
 		u64 _spte = *spte;
 		BUG_ON(!(_spte & PT_PRESENT_MASK));
-		_young = _spte & PT_ACCESSED_MASK;
+		_young = _spte & shadow_accessed_mask;

_young is an int, so will be set to zero on ept due to truncation.

arch/x86/kvm/mmu.c static int kvm_age_rmapp(struct kvm *kvm, unsigned long *rmapp) · line 721

Quoted 8 lines
arch/x86/kvm/mmu.c +1 -1
 		if (_young) {
 			young = 1;
-			clear_bit(PT_ACCESSED_SHIFT, (unsigned long *)spte);
+			clear_bit(shadow_accessed_shift, (unsigned long *)spte);
 		}
arch/x86/kvm/mmu.c +0 -0
 		spte = rmap_next(kvm, rmapp, spte);
 	}

Even if the former is fixed, what does it mean? guest memory will never be considered young by Linux, and so will be swapped sooner.

Maybe we ought to cheat in the other direction and return young = 1 for ept.

arch/x86/kvm/mmu.c static void kvm_mmu_access_page(struct kvm_vcpu *vcpu, · line 1790

Quoted 4 lines
arch/x86/kvm/mmu.c +0 -0
@@ -1785,7 +1788,7 @@ static void kvm_mmu_access_page(struct kvm_vcpu *vcpu, 
 gfn_t gfn)
 	    && shadow_accessed_mask

Ah, this is to disable this check for ept (I thought accessed_mask was zero; why?)

arch/x86/kvm/mmu.c static void kvm_mmu_access_page(struct kvm_vcpu *vcpu, gfn_t gfn) · line 1792

Quoted 6 lines
arch/x86/kvm/mmu.c +1 -1
 	    && !(*spte & shadow_accessed_mask)
 	    && is_shadow_present_pte(*spte))
-		set_bit(PT_ACCESSED_SHIFT, (unsigned long *)spte);
+		set_bit(shadow_accessed_shift, (unsigned long *)spte);
 }

... but in any case I don't think this code path is ever hit? maybe doing a movsb from mmio to RAM.

Signature · 2 lines
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.
#2 lore raw
Quoted 8 lines · Avi Kivity
Quoted 7 lines · Sheng

From: Sheng Yang <[email protected]>
Date: Fri, 29 Aug 2008 14:02:29 +0800
Subject: [PATCH] KVM: MMU: Add shadow_accessed_shift

static u64 __read_mostly shadow_dirty_mask;

Quoted 11 lines

arch/x86/kvm/mmu.c void kvm_mmu_set_mask_ptes(u64 user_mask, u64 · line 178

Quoted 9 lines

void kvm_mmu_set_nonpresent_ptes(u64 trap_pte, u64 notrap_pte)

arch/x86/kvm/mmu.c +2 -0
@@ -171,6 +172,8 @@ void kvm_mmu_set_mask_ptes(u64 user_mask, u64  
 accessed_mask,
 {
 	shadow_user_mask = user_mask;
 	shadow_accessed_mask = accessed_mask;
+	shadow_accessed_shift = find_first_bit((unsigned long *)&accessed_mask,
+					       sizeof(accessed_mask));

Ah, I thought ept accessed_mask was zero.

Ah... Indeed I gave a FAKE_ACCESSED_MASK(1<<62) to EPT when the patch checked in. I just want to keep the behaviour consistent with EPT and shadow rather than do "if xxx_mask == 0" all the time.

I thought the FAKE one can be set when the EPTE is new (and maybe some rare condition below). But I have neglected the check of kvm_mmu_access_page() below when I add those fake bits...

Quoted 27 lines

arch/x86/kvm/mmu.c static int kvm_age_rmapp(struct kvm *kvm, · line 717

Quoted 12 lines
arch/x86/kvm/mmu.c +0 -0
 	shadow_dirty_mask = dirty_mask;
arch/x86/kvm/mmu.c +1 -1
 	shadow_nx_mask = nx_mask;
 	shadow_x_mask = x_mask;

@@ -709,10 +712,10 @@ static int kvm_age_rmapp(struct kvm *kvm, 
 unsigned long *rmapp)
 		int _young;
 		u64 _spte = *spte;
 		BUG_ON(!(_spte & PT_PRESENT_MASK));
-		_young = _spte & PT_ACCESSED_MASK;
+		_young = _spte & shadow_accessed_mask;

_young is an int, so will be set to zero on ept due to truncation.

arch/x86/kvm/mmu.c static int kvm_age_rmapp(struct kvm *kvm, unsigned long *rmapp) · line 721

Quoted 8 lines
arch/x86/kvm/mmu.c +1 -1
 		if (_young) {
 			young = 1;
-			clear_bit(PT_ACCESSED_SHIFT, (unsigned long *)spte);
+			clear_bit(shadow_accessed_shift, (unsigned long *)spte);
 		}
arch/x86/kvm/mmu.c +0 -0
 		spte = rmap_next(kvm, rmapp, spte);
 	}

Even if the former is fixed, what does it mean? guest memory will never be considered young by Linux, and so will be swapped sooner.

Quoted 1 line

Maybe we ought to cheat in the other direction and return young = 1 for ept.

If I understand correctly, now I don't have idea when we can return young=1 except ept entry was just created. For EPT, we may return 0 for the most of time, after this clear_bit action.

Quoted 8 lines

arch/x86/kvm/mmu.c static void kvm_mmu_access_page(struct kvm_vcpu · line 1790

Quoted 5 lines
arch/x86/kvm/mmu.c +0 -0
@@ -1785,7 +1788,7 @@ static void kvm_mmu_access_page(struct kvm_vcpu 
 *vcpu, gfn_t gfn)
 	    && shadow_accessed_mask

Ah, this is to disable this check for ept (I thought accessed_mask was zero; why?)

My fault. Overlook this from the beginning... The check here no longer fit for EPT, for shadow_accessed_mask of EPT is not zero now. I will remove that.

Quoted 10 lines

arch/x86/kvm/mmu.c static void kvm_mmu_access_page(struct kvm_vcpu *vcpu, gfn_t gfn) · line 1792

Quoted 7 lines
arch/x86/kvm/mmu.c +1 -1
 	    && !(*spte & shadow_accessed_mask)
 	    && is_shadow_present_pte(*spte))
-		set_bit(PT_ACCESSED_SHIFT, (unsigned long *)spte);
+		set_bit(shadow_accessed_shift, (unsigned long *)spte);
 }

... but in any case I don't think this code path is ever hit? maybe doing a movsb from mmio to RAM.

Thanks... movsb seems reasonable, maybe I just thought it's "in case"... :)

--
regards
Yang, Sheng

Quoted 9 lines
Signature · 8 lines
-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html