KVM: support for pinning sptes (v2)

arch/x86/kvm/mmu.c +15 -0

+
+static int kvm_mmu_pin_pages(struct kvm_vcpu *vcpu)
+{
+	struct kvm_pinned_page_range *p;
+	int r = 1;
+
+	if (is_guest_mode(vcpu))
+		return r;
+
+	if (!vcpu->arch.mmu.direct_map)
+		return r;
+
+	ASSERT(VALID_PAGE(vcpu->arch.mmu.root_hpa));
+
+	list_for_each_entry(p, &vcpu->arch.pinned_mmu_pages, link) {

Small question if I may regarding kvm_mmu_pin_pages:

Quoted 22 lines · On 7/9/14, 10:12 PM, [email protected] wrote:

arch/x86/kvm/mmu.c +22 -0

+
+static int kvm_mmu_pin_pages(struct kvm_vcpu *vcpu)
+{
+	struct kvm_pinned_page_range *p;
+	int r = 1;
+
+	if (is_guest_mode(vcpu))
+		return r;
+
+	if (!vcpu->arch.mmu.direct_map)
+		return r;
+
+	ASSERT(VALID_PAGE(vcpu->arch.mmu.root_hpa));
+
+	list_for_each_entry(p, &vcpu->arch.pinned_mmu_pages, link) {
+		gfn_t gfn_offset;
+
+		for (gfn_offset = 0; gfn_offset < p->npages; gfn_offset++) {
+			gfn_t gfn = p->base_gfn + gfn_offset;
+			int r;
+			bool pinned = false;
+

arch/x86/kvm/mmu.c line 4135

Quoted 3 lines

arch/x86/kvm/mmu.c +3 -0

+			r = vcpu->arch.mmu.page_fault(vcpu, gfn << PAGE_SHIFT,
+						     PFERR_WRITE_MASK, false,
+						     true, &pinned);

I understand that the current use-case is for pinning only few pages. Yet, wouldn't it be better (for performance) to check whether the gfn uses a large page and if so to skip forward, increasing gfn_offset to point to the next large page?

Quoted 2 lines · Nadav Amit

Small question if I may regarding kvm_mmu_pin_pages:

Quoted 4 lines

I understand that the current use-case is for pinning only few pages. Yet, wouldn't it be better (for performance) to check whether the gfn uses a large page and if so to skip forward, increasing gfn_offset to point to the next large page?

Sure, that can be a lazy optimization and performed when necessary?

(feel free to do it in advance if you're interested in doing it now).

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

 	struct fpu guest_fpu;
 	u64 xcr0;
 	u64 guest_supported_xcr0;

Index: kvm.pinned-sptes/arch/x86/kvm/mmu.c
===================================================================

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

@@ -148,6 +148,9 @@
 
 #define SPTE_HOST_WRITEABLE	(1ULL << PT_FIRST_AVAIL_BITS_SHIFT)
 #define SPTE_MMU_WRITEABLE	(1ULL << (PT_FIRST_AVAIL_BITS_SHIFT + 1))
+#define SPTE_PINNED		(1ULL << (PT64_SECOND_AVAIL_BITS_SHIFT))
+
+#define SPTE_PINNED_BIT PT64_SECOND_AVAIL_BITS_SHIFT
 
 #define SHADOW_PT_INDEX(addr, level) PT64_INDEX(addr, level)
 

@@ -327,6 +330,11 @@
 	return pte & PT_PRESENT_MASK && !is_mmio_spte(pte);
 }
 
+static int is_pinned_spte(u64 spte)
+{
+	return spte & SPTE_PINNED && is_shadow_present_pte(spte);
+}
+
 static int is_large_pte(u64 pte)
 {
 	return pte & PT_PAGE_SIZE_MASK;

@@ -1176,6 +1184,16 @@
 		kvm_flush_remote_tlbs(vcpu->kvm);
 }
 
+static bool vcpu_has_pinned(struct kvm_vcpu *vcpu)
+{
+	return atomic_read(&vcpu->arch.nr_pinned_ranges);
+}
+
+static void mmu_reload_pinned_vcpus(struct kvm *kvm)
+{
+	make_all_cpus_request(kvm, KVM_REQ_MMU_RELOAD, &vcpu_has_pinned);
+}
+
 /*
 * Write-protect on the specified @sptep, @pt_protect indicates whether
 * spte write-protection is caused by protecting shadow page table.

@@ -1268,7 +1286,8 @@
 }
 
 static int kvm_unmap_rmapp(struct kvm *kvm, unsigned long *rmapp,
-			   struct kvm_memory_slot *slot, unsigned long data)
+			   struct kvm_memory_slot *slot, unsigned long data,
+			   bool age)
 {
 	u64 *sptep;
 	struct rmap_iterator iter;

@@ -1278,6 +1297,14 @@
 		BUG_ON(!(*sptep & PT_PRESENT_MASK));
 		rmap_printk("kvm_rmap_unmap_hva: spte %p %llx\n", sptep, *sptep);

arch/x86/kvm/mmu.c +25 -10

 		need_tlb_flush = 1;
 	}

@@ -1286,7 +1313,8 @@
 }
 
 static int kvm_set_pte_rmapp(struct kvm *kvm, unsigned long *rmapp,
-			     struct kvm_memory_slot *slot, unsigned long data)
+			     struct kvm_memory_slot *slot, unsigned long data,
+			     bool age)
 {
 	u64 *sptep;
 	struct rmap_iterator iter;

@@ -1304,6 +1332,9 @@
 
 		need_flush = 1;
 
+		if (is_pinned_spte(*sptep))
+			mmu_reload_pinned_vcpus(kvm);
+
 		if (pte_write(*ptep)) {
 			drop_spte(kvm, sptep);
 			sptep = rmap_get_first(*rmapp, &iter);

@@ -1334,7 +1365,8 @@
 				int (*handler)(struct kvm *kvm,
 					       unsigned long *rmapp,
 					       struct kvm_memory_slot *slot,
-					       unsigned long data))
+					       unsigned long data,
+					       bool age))
 {
 	int j;
 	int ret = 0;

@@ -1374,7 +1406,7 @@
 			rmapp = __gfn_to_rmap(gfn_start, j, memslot);
 
 			for (; idx <= idx_end; ++idx)
-				ret |= handler(kvm, rmapp++, memslot, data);
+				ret |= handler(kvm, rmapp++, memslot, data, false);
 		}
 	}
 

@@ -1385,7 +1417,8 @@
 			  unsigned long data,
 			  int (*handler)(struct kvm *kvm, unsigned long *rmapp,
 					 struct kvm_memory_slot *slot,
-					 unsigned long data))
+					 unsigned long data,
+					 bool age))
 {
 	return kvm_handle_hva_range(kvm, hva, hva + 1, data, handler);
 }

@@ -1406,7 +1439,8 @@
 }
 
 static int kvm_age_rmapp(struct kvm *kvm, unsigned long *rmapp,
-			 struct kvm_memory_slot *slot, unsigned long data)
+			 struct kvm_memory_slot *slot, unsigned long data,
+			 bool age)
 {
 	u64 *sptep;
 	struct rmap_iterator uninitialized_var(iter);

@@ -1421,7 +1455,7 @@
 	 * out actively used pages or breaking up actively used hugepages.
 	 */
 	if (!shadow_accessed_mask) {
-		young = kvm_unmap_rmapp(kvm, rmapp, slot, data);
+		young = kvm_unmap_rmapp(kvm, rmapp, slot, data, true);
 		goto out;
 	}
 

@@ -1442,7 +1476,8 @@
 }
 
 static int kvm_test_age_rmapp(struct kvm *kvm, unsigned long *rmapp,
-			      struct kvm_memory_slot *slot, unsigned long data)
+			      struct kvm_memory_slot *slot, unsigned long data,
+			      bool age)
 {
 	u64 *sptep;
 	struct rmap_iterator iter;

@@ -1480,7 +1515,7 @@
 
 	rmapp = gfn_to_rmap(vcpu->kvm, gfn, sp->role.level);
 
-	kvm_unmap_rmapp(vcpu->kvm, rmapp, NULL, 0);
+	kvm_unmap_rmapp(vcpu->kvm, rmapp, NULL, 0, false);
 	kvm_flush_remote_tlbs(vcpu->kvm);
 }
 

@@ -2753,7 +2788,8 @@
 }
 
 static bool handle_abnormal_pfn(struct kvm_vcpu *vcpu, gva_t gva, gfn_t gfn,
-				pfn_t pfn, unsigned access, int *ret_val)
+				pfn_t pfn, unsigned access, int *ret_val,
+				bool pin)
 {
 	bool ret = true;
 

@@ -2763,8 +2799,14 @@
 		goto exit;
 	}
 
-	if (unlikely(is_noslot_pfn(pfn)))
+	if (unlikely(is_noslot_pfn(pfn))) {
+		/* pinned sptes must point to RAM */
+		if (unlikely(pin)) {
+			*ret_val = -EFAULT;
+			goto exit;
+		}
 		vcpu_cache_mmio_info(vcpu, gva, gfn, access);
+	}
 
 	ret = false;

exit:

arch/x86/kvm/mmu.c +57 -2

@@ -2818,7 +2860,7 @@
 * - false: let the real page fault path to fix it.
 */
 static bool fast_page_fault(struct kvm_vcpu *vcpu, gva_t gva, int level,
-			    u32 error_code)
+			    u32 error_code, bool pin)
 {
 	struct kvm_shadow_walk_iterator iterator;
 	struct kvm_mmu_page *sp;

@@ -2828,6 +2870,9 @@
 	if (!VALID_PAGE(vcpu->arch.mmu.root_hpa))
 		return false;
 
+	if (pin)
+		return false;
+
 	if (!page_fault_can_be_fast(error_code))
 		return false;
 

@@ -2895,9 +2940,71 @@
 }
 
 static bool try_async_pf(struct kvm_vcpu *vcpu, bool prefault, gfn_t gfn,
-			 gva_t gva, pfn_t *pfn, bool write, bool *writable);
+			 gva_t gva, pfn_t *pfn, bool write, bool *writable,
+			 bool pin);
 static void make_mmu_pages_available(struct kvm_vcpu *vcpu);
 
+
+static int get_sptep_hierarchy(struct kvm_vcpu *vcpu, u64 addr, u64 *sptes[4])
+
+{
+	struct kvm_shadow_walk_iterator iterator;
+	int nr_sptes = 0;
+
+	if (!VALID_PAGE(vcpu->arch.mmu.root_hpa))
+		return nr_sptes;
+
+	for_each_shadow_entry(vcpu, addr, iterator) {
+		sptes[iterator.level-1] = iterator.sptep;
+		nr_sptes++;
+		if (!is_shadow_present_pte(*iterator.sptep))
+			break;
+	}
+
+	return nr_sptes;
+}
+
+static bool __direct_pin_sptes(struct kvm_vcpu *vcpu, gfn_t gfn, bool pin)
+{
+	u64 *sptes[4];
+	int r, i, level;
+
+	r = get_sptep_hierarchy(vcpu, gfn << PAGE_SHIFT, sptes);
+	if (!r)
+		return false;
+
+	level = 5 - r;
+	if (!is_last_spte(*sptes[level-1], level))
+		return false;
+	if (!is_shadow_present_pte(*sptes[level-1]))
+		return false;
+
+	for (i = 0; i < r; i++) {
+		u64 *sptep = sptes[3-i];
+		struct kvm_mmu_page *sp = page_header(__pa(sptep));
+
+		if (pin) {
+			sp->pinned = true;
+			set_bit(SPTE_PINNED_BIT, (unsigned long *)sptep);
+		} else {
+			sp->pinned = false;
+			clear_bit(SPTE_PINNED_BIT, (unsigned long *)sptep);
+		}
+	}
+
+	return true;
+}
+

Quoted 4 lines

arch/x86/kvm/mmu.c +4 -0

+		if (is_pinned_spte(*sptep)) {
+			/* don't nuke pinned sptes if page aging: return
+ 			 * young=yes instead.
+ 			 */

Quoted 5 lines

arch/x86/kvm/mmu.c +4 -0

+			if (age)
+				return 1;
+			mmu_reload_pinned_vcpus(kvm);
+		}

		drop_spte(kvm, step);

This has a window between zapping spte and re-pin spte, so guest will fail at this time.

Reload remote vcpus MMU from GET_DIRTY_LOG codepath, before deleting a pinned spte.

Signed-off-by Marcelo Tosatti <[email protected]>

---
 arch/x86/kvm/mmu.c |   29 +++++++++++++++++++++++------
 1 file changed, 23 insertions(+), 6 deletions(-)

Index: kvm.pinned-sptes/arch/x86/kvm/mmu.c
===================================================================

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

@@ -1208,7 +1208,8 @@
  *
  * Return true if tlb need be flushed.
  */
-static bool spte_write_protect(struct kvm *kvm, u64 *sptep, bool pt_protect)
+static bool spte_write_protect(struct kvm *kvm, u64 *sptep, bool pt_protect,
+			       bool skip_pinned)
 {
 	u64 spte = *sptep;
 

@@ -1218,6 +1219,22 @@
 
 	rmap_printk("rmap_write_protect: spte %p %llx\n", sptep, *sptep);
 
+	if (is_pinned_spte(spte)) {

arch/x86/kvm/mmu.c +7 -4

+	}
+
+
 	if (pt_protect)
 		spte &= ~SPTE_MMU_WRITEABLE;
 	spte = spte & ~PT_WRITABLE_MASK;

@@ -1226,7 +1243,7 @@
 }
 
 static bool __rmap_write_protect(struct kvm *kvm, unsigned long *rmapp,
-				 bool pt_protect)
+				 bool pt_protect, bool skip_pinned)
 {
 	u64 *sptep;
 	struct rmap_iterator iter;

@@ -1235,7 +1252,7 @@
 	for (sptep = rmap_get_first(*rmapp, &iter); sptep;) {
 		BUG_ON(!(*sptep & PT_PRESENT_MASK));
 
-		flush |= spte_write_protect(kvm, sptep, pt_protect);
+		flush |= spte_write_protect(kvm, sptep, pt_protect, skip_pinned);
 		sptep = rmap_get_next(&iter);
 	}
 

@@ -1261,7 +1278,7 @@
 	while (mask) {
 		rmapp = __gfn_to_rmap(slot->base_gfn + gfn_offset + __ffs(mask),
 				      PT_PAGE_TABLE_LEVEL, slot);
-		__rmap_write_protect(kvm, rmapp, false);
+		__rmap_write_protect(kvm, rmapp, false, true);
 
 		/* clear the first set bit */
 		mask &= mask - 1;

@@ -1280,7 +1297,7 @@
 	for (i = PT_PAGE_TABLE_LEVEL;
 	     i < PT_PAGE_TABLE_LEVEL + KVM_NR_PAGE_SIZES; ++i) {
 		rmapp = __gfn_to_rmap(gfn, i, slot);
-		write_protected |= __rmap_write_protect(kvm, rmapp, true);
+		write_protected |= __rmap_write_protect(kvm, rmapp, true, false);
 	}
 
 	return write_protected;

Quoted 27 lines · On Wed, Jul 09, 2014 at 04:12:53PM -0300, [email protected] wrote:

Reload remote vcpus MMU from GET_DIRTY_LOG codepath, before deleting a pinned spte.

Signed-off-by Marcelo Tosatti <[email protected]>

---
 arch/x86/kvm/mmu.c |   29 +++++++++++++++++++++++------
 1 file changed, 23 insertions(+), 6 deletions(-)

Index: kvm.pinned-sptes/arch/x86/kvm/mmu.c
===================================================================

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

@@ -1208,7 +1208,8 @@
  *
  * Return true if tlb need be flushed.
  */
-static bool spte_write_protect(struct kvm *kvm, u64 *sptep, bool pt_protect)
+static bool spte_write_protect(struct kvm *kvm, u64 *sptep, bool pt_protect,
+			       bool skip_pinned)
 {
 	u64 spte = *sptep;
 

@@ -1218,6 +1219,22 @@
 
 	rmap_printk("rmap_write_protect: spte %p %llx\n", sptep, *sptep);
 

arch/x86/kvm/mmu.c line 1232

Quoted 11 lines

arch/x86/kvm/mmu.c +11 -0

+	if (is_pinned_spte(spte)) {
+		/* keep pinned spte intact, mark page dirty again */
+		if (skip_pinned) {
+			struct kvm_mmu_page *sp;
+			gfn_t gfn;
+
+			sp = page_header(__pa(sptep));
+			gfn = kvm_mmu_page_get_gfn(sp, sptep - sp->spt);
+
+			mark_page_dirty(kvm, gfn);
+			return false;

Why not mark all pinned gfns as dirty in kvm_vm_ioctl_get_dirty_log() while populating dirty_bitmap_buffer?

arch/x86/kvm/mmu.c line 1234

Quoted 2 lines

arch/x86/kvm/mmu.c +2 -0

+		} else
+			mmu_reload_pinned_vcpus(kvm);

Can you explain why do you need this?

Quoted 28 lines · Gleb Natapov

Quoted 27 lines · On Wed, Jul 09, 2014 at 04:12:53PM -0300, [email protected] wrote:

Reload remote vcpus MMU from GET_DIRTY_LOG codepath, before deleting a pinned spte.

Signed-off-by Marcelo Tosatti <[email protected]>

---
 arch/x86/kvm/mmu.c |   29 +++++++++++++++++++++++------
 1 file changed, 23 insertions(+), 6 deletions(-)

Index: kvm.pinned-sptes/arch/x86/kvm/mmu.c
===================================================================

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

@@ -1208,7 +1208,8 @@
  *
  * Return true if tlb need be flushed.
  */
-static bool spte_write_protect(struct kvm *kvm, u64 *sptep, bool pt_protect)
+static bool spte_write_protect(struct kvm *kvm, u64 *sptep, bool pt_protect,
+			       bool skip_pinned)
 {
 	u64 spte = *sptep;
 

@@ -1218,6 +1219,22 @@
 
 	rmap_printk("rmap_write_protect: spte %p %llx\n", sptep, *sptep);
 

Quoted 13 lines

arch/x86/kvm/mmu.c line 1232

Quoted 11 lines

arch/x86/kvm/mmu.c +11 -0

+	if (is_pinned_spte(spte)) {
+		/* keep pinned spte intact, mark page dirty again */
+		if (skip_pinned) {
+			struct kvm_mmu_page *sp;
+			gfn_t gfn;
+
+			sp = page_header(__pa(sptep));
+			gfn = kvm_mmu_page_get_gfn(sp, sptep - sp->spt);
+
+			mark_page_dirty(kvm, gfn);
+			return false;

Why not mark all pinned gfns as dirty in kvm_vm_ioctl_get_dirty_log() while populating dirty_bitmap_buffer?

The pinned gfns are per-vcpu. Requires looping all vcpus (not scalable).

Quoted 3 lines

arch/x86/kvm/mmu.c line 1234

Quoted 2 lines

arch/x86/kvm/mmu.c +2 -0

+		} else
+			mmu_reload_pinned_vcpus(kvm);

Can you explain why do you need this?

Because if skip_pinned = false, we want vcpus to exit (called from enable dirty logging codepath).

Quoted 47 lines · Marcelo Tosatti

Quoted 28 lines · Gleb Natapov

Quoted 27 lines · On Wed, Jul 09, 2014 at 04:12:53PM -0300, [email protected] wrote:

Reload remote vcpus MMU from GET_DIRTY_LOG codepath, before deleting a pinned spte.

Signed-off-by Marcelo Tosatti <[email protected]>

---
 arch/x86/kvm/mmu.c |   29 +++++++++++++++++++++++------
 1 file changed, 23 insertions(+), 6 deletions(-)

Index: kvm.pinned-sptes/arch/x86/kvm/mmu.c
===================================================================

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

@@ -1208,7 +1208,8 @@
  *
  * Return true if tlb need be flushed.
  */
-static bool spte_write_protect(struct kvm *kvm, u64 *sptep, bool pt_protect)
+static bool spte_write_protect(struct kvm *kvm, u64 *sptep, bool pt_protect,
+			       bool skip_pinned)
 {
 	u64 spte = *sptep;
 

@@ -1218,6 +1219,22 @@
 
 	rmap_printk("rmap_write_protect: spte %p %llx\n", sptep, *sptep);
 

Quoted 13 lines

arch/x86/kvm/mmu.c line 1232

Quoted 11 lines

arch/x86/kvm/mmu.c +11 -0

+	if (is_pinned_spte(spte)) {
+		/* keep pinned spte intact, mark page dirty again */
+		if (skip_pinned) {
+			struct kvm_mmu_page *sp;
+			gfn_t gfn;
+
+			sp = page_header(__pa(sptep));
+			gfn = kvm_mmu_page_get_gfn(sp, sptep - sp->spt);
+
+			mark_page_dirty(kvm, gfn);
+			return false;

Why not mark all pinned gfns as dirty in kvm_vm_ioctl_get_dirty_log() while populating dirty_bitmap_buffer?

The pinned gfns are per-vcpu. Requires looping all vcpus (not scalable).

Quoted 6 lines

Quoted 3 lines

arch/x86/kvm/mmu.c line 1234

Quoted 2 lines

arch/x86/kvm/mmu.c +2 -0

+		} else
+			mmu_reload_pinned_vcpus(kvm);

Can you explain why do you need this?

Because if skip_pinned = false, we want vcpus to exit (called from enable dirty logging codepath).

Gleb, any further opinions on this ?

Can you ack the patch ? TIA

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

Quoted 28 lines · Gleb Natapov

Quoted 27 lines · On Wed, Jul 09, 2014 at 04:12:53PM -0300, [email protected] wrote:

Reload remote vcpus MMU from GET_DIRTY_LOG codepath, before deleting a pinned spte.

Signed-off-by Marcelo Tosatti <[email protected]>

---
 arch/x86/kvm/mmu.c |   29 +++++++++++++++++++++++------
 1 file changed, 23 insertions(+), 6 deletions(-)

Index: kvm.pinned-sptes/arch/x86/kvm/mmu.c
===================================================================

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

@@ -1208,7 +1208,8 @@
  *
  * Return true if tlb need be flushed.
  */
-static bool spte_write_protect(struct kvm *kvm, u64 *sptep, bool pt_protect)
+static bool spte_write_protect(struct kvm *kvm, u64 *sptep, bool pt_protect,
+			       bool skip_pinned)
 {
 	u64 spte = *sptep;
 

@@ -1218,6 +1219,22 @@
 
 	rmap_printk("rmap_write_protect: spte %p %llx\n", sptep, *sptep);
 

Quoted 13 lines

arch/x86/kvm/mmu.c line 1232

Quoted 11 lines

arch/x86/kvm/mmu.c +11 -0

+	if (is_pinned_spte(spte)) {
+		/* keep pinned spte intact, mark page dirty again */
+		if (skip_pinned) {
+			struct kvm_mmu_page *sp;
+			gfn_t gfn;
+
+			sp = page_header(__pa(sptep));
+			gfn = kvm_mmu_page_get_gfn(sp, sptep - sp->spt);
+
+			mark_page_dirty(kvm, gfn);
+			return false;

Why not mark all pinned gfns as dirty in kvm_vm_ioctl_get_dirty_log() while populating dirty_bitmap_buffer?

The pinned gfns are per-vcpu. Requires looping all vcpus (not scalable).

Quoted 4 lines

arch/x86/kvm/mmu.c line 1234

Quoted 3 lines

arch/x86/kvm/mmu.c +2 -0

+		} else
+			mmu_reload_pinned_vcpus(kvm);

Can you explain why do you need this?

Because if skip_pinned = false, we want vcpus to exit (called from enable dirty logging codepath).

Quoted 43 lines · Marcelo Tosatti

Quoted 41 lines · Gleb Natapov

Quoted 28 lines · On Wed, Jul 09, 2014 at 04:12:53PM -0300, [email protected] wrote:

Reload remote vcpus MMU from GET_DIRTY_LOG codepath, before deleting a pinned spte.

Signed-off-by Marcelo Tosatti <[email protected]>

---
 arch/x86/kvm/mmu.c |   29 +++++++++++++++++++++++------
 1 file changed, 23 insertions(+), 6 deletions(-)

Index: kvm.pinned-sptes/arch/x86/kvm/mmu.c
===================================================================

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

@@ -1208,7 +1208,8 @@
  *
  * Return true if tlb need be flushed.
  */
-static bool spte_write_protect(struct kvm *kvm, u64 *sptep, bool pt_protect)
+static bool spte_write_protect(struct kvm *kvm, u64 *sptep, bool pt_protect,
+			       bool skip_pinned)
 {
 	u64 spte = *sptep;
 

@@ -1218,6 +1219,22 @@
 
 	rmap_printk("rmap_write_protect: spte %p %llx\n", sptep, *sptep);
 
+	if (is_pinned_spte(spte)) {

arch/x86/kvm/mmu.c line 1232

Quoted 10 lines

arch/x86/kvm/mmu.c +10 -0

+		/* keep pinned spte intact, mark page dirty again */
+		if (skip_pinned) {
+			struct kvm_mmu_page *sp;
+			gfn_t gfn;
+
+			sp = page_header(__pa(sptep));
+			gfn = kvm_mmu_page_get_gfn(sp, sptep - sp->spt);
+
+			mark_page_dirty(kvm, gfn);
+			return false;

Why not mark all pinned gfns as dirty in kvm_vm_ioctl_get_dirty_log() while populating dirty_bitmap_buffer?

Quoted 3 lines

The pinned gfns are per-vcpu. Requires looping all vcpus (not scalable).

OK, but do they really have to be per-cpu? What's the advantage?

Quoted 8 lines

Quoted 4 lines

arch/x86/kvm/mmu.c line 1234

Quoted 3 lines

arch/x86/kvm/mmu.c +2 -0

+		} else
+			mmu_reload_pinned_vcpus(kvm);

Can you explain why do you need this?

Because if skip_pinned = false, we want vcpus to exit (called from enable dirty logging codepath).

I guess what I wanted to ask is why do we need skip_pinned? As far as I see it is set to false in two cases:
1: page is write protected for shadow MMU needs, should not happen since the feature

is not supported with shadow mmu (can happen with nested EPT, but page will be marked is accessed during next vcpu entry anyway, so how will it work)?

2: when slot is marked as read only: such slot cannot have PEBS pages and if it will guest will die
anyway during next guest entry, so why not maintain list of pinned pages per slot and kill aguest if slot with pinned pages is marked read only.

Quoted 44 lines · Gleb Natapov

Quoted 43 lines · Marcelo Tosatti

Quoted 41 lines · Gleb Natapov

Quoted 28 lines · On Wed, Jul 09, 2014 at 04:12:53PM -0300, [email protected] wrote:

Reload remote vcpus MMU from GET_DIRTY_LOG codepath, before deleting a pinned spte.

Signed-off-by Marcelo Tosatti <[email protected]>

---
 arch/x86/kvm/mmu.c |   29 +++++++++++++++++++++++------
 1 file changed, 23 insertions(+), 6 deletions(-)

Index: kvm.pinned-sptes/arch/x86/kvm/mmu.c
===================================================================

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

@@ -1208,7 +1208,8 @@
  *
  * Return true if tlb need be flushed.
  */
-static bool spte_write_protect(struct kvm *kvm, u64 *sptep, bool pt_protect)
+static bool spte_write_protect(struct kvm *kvm, u64 *sptep, bool pt_protect,
+			       bool skip_pinned)
 {
 	u64 spte = *sptep;
 

@@ -1218,6 +1219,22 @@
 
 	rmap_printk("rmap_write_protect: spte %p %llx\n", sptep, *sptep);
 
+	if (is_pinned_spte(spte)) {

arch/x86/kvm/mmu.c line 1232

Quoted 10 lines

arch/x86/kvm/mmu.c +10 -0

+		/* keep pinned spte intact, mark page dirty again */
+		if (skip_pinned) {
+			struct kvm_mmu_page *sp;
+			gfn_t gfn;
+
+			sp = page_header(__pa(sptep));
+			gfn = kvm_mmu_page_get_gfn(sp, sptep - sp->spt);
+
+			mark_page_dirty(kvm, gfn);
+			return false;

Why not mark all pinned gfns as dirty in kvm_vm_ioctl_get_dirty_log() while populating dirty_bitmap_buffer?

Quoted 4 lines

Quoted 3 lines

The pinned gfns are per-vcpu. Requires looping all vcpus (not scalable).

OK, but do they really have to be per-cpu? What's the advantage?

The guest configures pinning on a per-cpu basis (that is, enabling PEBS is done on a per-cpu basis).

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

arch/x86/kvm/mmu.c line 1234

Quoted 4 lines

arch/x86/kvm/mmu.c +2 -0

+		} else
+			mmu_reload_pinned_vcpus(kvm);

Can you explain why do you need this?

Quoted 6 lines

Quoted 3 lines

Because if skip_pinned = false, we want vcpus to exit (called from enable dirty logging codepath).

I guess what I wanted to ask is why do we need skip_pinned? As far as I see it is set to false in two cases:
1: page is write protected for shadow MMU needs, should not happen since the feature

Correct.

Quoted 2 lines

is not supported with shadow mmu (can happen with nested EPT, but page will be marked is accessed during next vcpu entry anyway, so how will it work)?

PEBS is not supported on nested EPT.

Quoted 3 lines

2: when slot is marked as read only: such slot cannot have PEBS pages and if it will guest will die
anyway during next guest entry, so why not maintain list of pinned pages per slot and kill aguest if slot with pinned pages is marked read only.

2: when slots pages have dirty logging enabled. In that case, the page is marked dirty immediately. This is the call from kvm_mmu_slot_remove_write_access.

So when enabling dirty logging, for pinned sptes:

- maintain pinned spte intact.
- mark gfn for which pinned spte represents as dirty in the dirty log.

Quoted 45 lines · Marcelo Tosatti

Quoted 44 lines · Gleb Natapov

Quoted 43 lines · Marcelo Tosatti

Quoted 41 lines · Gleb Natapov

Quoted 28 lines · On Wed, Jul 09, 2014 at 04:12:53PM -0300, [email protected] wrote:

Reload remote vcpus MMU from GET_DIRTY_LOG codepath, before deleting a pinned spte.

Signed-off-by Marcelo Tosatti <[email protected]>

---
 arch/x86/kvm/mmu.c |   29 +++++++++++++++++++++++------
 1 file changed, 23 insertions(+), 6 deletions(-)

Index: kvm.pinned-sptes/arch/x86/kvm/mmu.c
===================================================================

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

@@ -1208,7 +1208,8 @@
  *
  * Return true if tlb need be flushed.
  */
-static bool spte_write_protect(struct kvm *kvm, u64 *sptep, bool pt_protect)
+static bool spte_write_protect(struct kvm *kvm, u64 *sptep, bool pt_protect,
+			       bool skip_pinned)
 {
 	u64 spte = *sptep;
 

@@ -1218,6 +1219,22 @@
 
 	rmap_printk("rmap_write_protect: spte %p %llx\n", sptep, *sptep);
 
+	if (is_pinned_spte(spte)) {

arch/x86/kvm/mmu.c line 1232

Quoted 10 lines

arch/x86/kvm/mmu.c +10 -0

+		/* keep pinned spte intact, mark page dirty again */
+		if (skip_pinned) {
+			struct kvm_mmu_page *sp;
+			gfn_t gfn;
+
+			sp = page_header(__pa(sptep));
+			gfn = kvm_mmu_page_get_gfn(sp, sptep - sp->spt);
+
+			mark_page_dirty(kvm, gfn);
+			return false;

Why not mark all pinned gfns as dirty in kvm_vm_ioctl_get_dirty_log() while populating dirty_bitmap_buffer?

Quoted 7 lines

Quoted 4 lines

Quoted 3 lines

The pinned gfns are per-vcpu. Requires looping all vcpus (not scalable).

OK, but do they really have to be per-cpu? What's the advantage?

The guest configures pinning on a per-cpu basis (that is, enabling PEBS is done on a per-cpu basis).

Is it a problem to maintain global pinned pages list for each memslot too?

Quoted 13 lines

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

arch/x86/kvm/mmu.c line 1234

Quoted 4 lines

arch/x86/kvm/mmu.c +2 -0

+		} else
+			mmu_reload_pinned_vcpus(kvm);

Can you explain why do you need this?

Quoted 6 lines

Quoted 3 lines

Because if skip_pinned = false, we want vcpus to exit (called from enable dirty logging codepath).

I guess what I wanted to ask is why do we need skip_pinned? As far as I see it is set to false in two cases:
1: page is write protected for shadow MMU needs, should not happen since the feature

Quoted 7 lines

Correct.

Quoted 2 lines

is not supported with shadow mmu (can happen with nested EPT, but page will be marked is accessed during next vcpu entry anyway, so how will it work)?

PEBS is not supported on nested EPT.

OK, so for this case we do not need skip_pinned. Assert if it happens.

Quoted 8 lines

Quoted 3 lines

2: when slot is marked as read only: such slot cannot have PEBS pages and if it will guest will die
anyway during next guest entry, so why not maintain list of pinned pages per slot and kill aguest if slot with pinned pages is marked read only.

2: when slots pages have dirty logging enabled. In that case, the page is marked dirty immediately. This is the call from kvm_mmu_slot_remove_write_access.

Right, my 2 is incorrect.

Quoted 6 lines

So when enabling dirty logging, for pinned sptes:

- maintain pinned spte intact.
- mark gfn for which pinned spte represents as dirty in the dirty log.

But you set skip_pinned to false in kvm_mmu_slot_remove_write_access(), so this is not what is happening. Did you mean to set it to true there?

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Why write protect it at all? mmu_reload_pinned_vcpus() will unprotected
it anyway
on the next vmentry. Isn't it better to just report all pinned pages as
dirty alway.

Quoted 87 lines · Gleb Natapov

Quoted 45 lines · Marcelo Tosatti

Quoted 44 lines · Gleb Natapov

Quoted 43 lines · Marcelo Tosatti

Quoted 41 lines · Gleb Natapov

Quoted 28 lines · On Wed, Jul 09, 2014 at 04:12:53PM -0300, [email protected] wrote:

Reload remote vcpus MMU from GET_DIRTY_LOG codepath, before deleting a pinned spte.

Signed-off-by Marcelo Tosatti <[email protected]>

---
 arch/x86/kvm/mmu.c |   29 +++++++++++++++++++++++------
 1 file changed, 23 insertions(+), 6 deletions(-)

Index: kvm.pinned-sptes/arch/x86/kvm/mmu.c
===================================================================

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

@@ -1208,7 +1208,8 @@
  *
  * Return true if tlb need be flushed.
  */
-static bool spte_write_protect(struct kvm *kvm, u64 *sptep, bool pt_protect)
+static bool spte_write_protect(struct kvm *kvm, u64 *sptep, bool pt_protect,
+			       bool skip_pinned)
 {
 	u64 spte = *sptep;
 

@@ -1218,6 +1219,22 @@
 
 	rmap_printk("rmap_write_protect: spte %p %llx\n", sptep, *sptep);
 
+	if (is_pinned_spte(spte)) {

arch/x86/kvm/mmu.c line 1232

Quoted 10 lines

arch/x86/kvm/mmu.c +10 -0

+		/* keep pinned spte intact, mark page dirty again */
+		if (skip_pinned) {
+			struct kvm_mmu_page *sp;
+			gfn_t gfn;
+
+			sp = page_header(__pa(sptep));
+			gfn = kvm_mmu_page_get_gfn(sp, sptep - sp->spt);
+
+			mark_page_dirty(kvm, gfn);
+			return false;

Why not mark all pinned gfns as dirty in kvm_vm_ioctl_get_dirty_log() while populating dirty_bitmap_buffer?

Quoted 7 lines

Quoted 4 lines

Quoted 3 lines

The pinned gfns are per-vcpu. Requires looping all vcpus (not scalable).

OK, but do they really have to be per-cpu? What's the advantage?

The guest configures pinning on a per-cpu basis (that is, enabling PEBS is done on a per-cpu basis).

Is it a problem to maintain global pinned pages list for each memslot too?

Quoted 13 lines

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

arch/x86/kvm/mmu.c line 1234

Quoted 4 lines

arch/x86/kvm/mmu.c +2 -0

+		} else
+			mmu_reload_pinned_vcpus(kvm);

Can you explain why do you need this?

Quoted 6 lines

Quoted 3 lines

Because if skip_pinned = false, we want vcpus to exit (called from enable dirty logging codepath).

I guess what I wanted to ask is why do we need skip_pinned? As far as I see it is set to false in two cases:
1: page is write protected for shadow MMU needs, should not happen since the feature

Quoted 7 lines

Correct.

Quoted 2 lines

is not supported with shadow mmu (can happen with nested EPT, but page will be marked is accessed during next vcpu entry anyway, so how will it work)?

PEBS is not supported on nested EPT.

OK, so for this case we do not need skip_pinned. Assert if it happens.

Quoted 8 lines

Quoted 3 lines

2: when slot is marked as read only: such slot cannot have PEBS pages and if it will guest will die
anyway during next guest entry, so why not maintain list of pinned pages per slot and kill aguest if slot with pinned pages is marked read only.

2: when slots pages have dirty logging enabled. In that case, the page is marked dirty immediately. This is the call from kvm_mmu_slot_remove_write_access.

Right, my 2 is incorrect.

Quoted 8 lines

Quoted 6 lines

So when enabling dirty logging, for pinned sptes:

- maintain pinned spte intact.
- mark gfn for which pinned spte represents as dirty in the dirty log.

But you set skip_pinned to false in kvm_mmu_slot_remove_write_access(), so this is not what is happening. Did you mean to set it to true there?

Argh, lets try again:

skip_pinned = true
------------------

mark page dirty, keep spte intact

called from get dirty log path.

skip_pinned = false
-------------------
reload remote mmu
destroy pinned spte.

called from: dirty log enablement, rmap write protect (unused for pinned sptes)

Note this behaviour is your suggestion:

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Why write protect it at all? mmu_reload_pinned_vcpus() will unprotected
it anyway
on the next vmentry. Isn't it better to just report all pinned pages as
dirty alway.

Quoted 19 lines

Argh, lets try again:

skip_pinned = true
------------------

mark page dirty, keep spte intact

called from get dirty log path.

skip_pinned = false
-------------------
reload remote mmu
destroy pinned spte.

called from: dirty log enablement, rmap write protect (unused for pinned sptes)

Quoted 1 line

Note this behaviour is your suggestion:

Yes, I remember that and I thought we will not need this skip_pinned at all. For rmap write protect case there shouldn't be any pinned pages, but why dirty log enablement sets skip_pinned to false? Why not mark pinned pages as dirty just like you do in get dirty log path?

Quoted 21 lines

Quoted 20 lines

Argh, lets try again:

skip_pinned = true
------------------

mark page dirty, keep spte intact

called from get dirty log path.

skip_pinned = false
-------------------
reload remote mmu
destroy pinned spte.

called from: dirty log enablement, rmap write protect (unused for pinned sptes)

Note this behaviour is your suggestion:

Quoted 4 lines

Yes, I remember that and I thought we will not need this skip_pinned at all. For rmap write protect case there shouldn't be any pinned pages, but why dirty log enablement sets skip_pinned to false? Why not mark pinned pages as dirty just like you do in get dirty log path?

Because if its a large spte, it must be nuked (or marked read-only, which for pinned sptes, is not possible).

Quoted 26 lines · Marcelo Tosatti

Quoted 25 lines

Quoted 15 lines

Argh, lets try again:

skip_pinned = true
------------------

mark page dirty, keep spte intact

called from get dirty log path.

skip_pinned = false
-------------------
reload remote mmu
destroy pinned spte.

Quoted 5 lines

called from: dirty log enablement, rmap write protect (unused for pinned sptes)

Note this behaviour is your suggestion:

Yes, I remember that and I thought we will not need this skip_pinned at all. For rmap write protect case there shouldn't be any pinned pages, but why dirty log enablement sets skip_pinned to false? Why not mark pinned pages as dirty just like you do in get dirty log path?

Quoted 3 lines

Because if its a large spte, it must be nuked (or marked read-only, which for pinned sptes, is not possible).

If a large page has one small page pinned inside it its spte will be marked as pinned, correct?

We did nuke large ptes here until very
recently: c126d94f2c90ed9d, but we cannot drop a pte here anyway without kicking all vcpu from a guest mode, but do you need additional skip_pinned parameter? Why not check if spte is large instead?

So why not have per slot pinned page list (Xiao suggested the same) and do:

spte_write_protect() {

  if (is_pinned(spte) {
    if (large(spte))
       // cannot drop while vcpu are running
       mmu_reload_pinned_vcpus();
    else
       return false;
}

get_dirty_log() {
for_each(pinned pages i)

    makr_dirty(i);
}

Quoted 27 lines · Gleb Natapov

Quoted 26 lines · Marcelo Tosatti

Quoted 25 lines

Quoted 15 lines

Argh, lets try again:

skip_pinned = true
------------------

mark page dirty, keep spte intact

called from get dirty log path.

skip_pinned = false
-------------------
reload remote mmu
destroy pinned spte.

Quoted 5 lines

called from: dirty log enablement, rmap write protect (unused for pinned sptes)

Note this behaviour is your suggestion:

Yes, I remember that and I thought we will not need this skip_pinned at all. For rmap write protect case there shouldn't be any pinned pages, but why dirty log enablement sets skip_pinned to false? Why not mark pinned pages as dirty just like you do in get dirty log path?

Quoted 5 lines

Quoted 3 lines

Because if its a large spte, it must be nuked (or marked read-only, which for pinned sptes, is not possible).

If a large page has one small page pinned inside it its spte will be marked as pinned, correct?

Correct.

Quoted 4 lines

We did nuke large ptes here until very
recently: c126d94f2c90ed9d, but we cannot drop a pte here anyway without kicking all vcpu from a guest mode, but do you need additional skip_pinned parameter? Why not check if spte is large instead?

Nuke only if large spte is found? Can do that, instead.

Quoted 1 line

So why not have per slot pinned page list (Xiao suggested the same) and do:

The interface is per-vcpu (that is registration of pinned pages is performed on a per-vcpu basis).

Quoted 10 lines

spte_write_protect() {

  if (is_pinned(spte) {
    if (large(spte))
       // cannot drop while vcpu are running
       mmu_reload_pinned_vcpus();
    else
       return false;
}

Quoted 4 lines

get_dirty_log() {
for_each(pinned pages i)

    makr_dirty(i);
}

That is effectively the same this patchset does, except that the spte pinned bit is checked at spte_write_protect, instead of looping over page pinned list. Fail to see huge advantage there.

I'll drop the skip_pinned parameter and use is_large_pte check instead.

Reload remote vcpus MMU from GET_DIRTY_LOG codepath, before deleting a pinned spte.

Signed-off-by Marcelo Tosatti <[email protected]>

---
arch/x86/kvm/mmu.c | 29 +++++++++++++++++++++++------

1 file changed, 23 insertions(+), 6 deletions(-)

Index: kvm.pinned-sptes/arch/x86/kvm/mmu.c
===================================================================

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

@@ -1208,7 +1208,8 @@
 *
 * Return true if tlb need be flushed.
 */
-static bool spte_write_protect(struct kvm *kvm, u64 *sptep, bool pt_protect)
+static bool spte_write_protect(struct kvm *kvm, u64 *sptep, bool pt_protect,
+			       bool skip_pinned)
 {
 	u64 spte = *sptep;
 

@@ -1218,6 +1219,22 @@
 
 	rmap_printk("rmap_write_protect: spte %p %llx\n", sptep, *sptep);
 
+	if (is_pinned_spte(spte)) {
+		/* keep pinned spte intact, mark page dirty again */
+		if (skip_pinned) {
+			struct kvm_mmu_page *sp;
+			gfn_t gfn;
+
+			sp = page_header(__pa(sptep));
+			gfn = kvm_mmu_page_get_gfn(sp, sptep - sp->spt);
+

Quoted 41 lines · On Jul 10, 2014, at 3:12 AM, [email protected] wrote:

Reload remote vcpus MMU from GET_DIRTY_LOG codepath, before deleting a pinned spte.

Signed-off-by Marcelo Tosatti <[email protected]>

---
arch/x86/kvm/mmu.c | 29 +++++++++++++++++++++++------

1 file changed, 23 insertions(+), 6 deletions(-)

Index: kvm.pinned-sptes/arch/x86/kvm/mmu.c
===================================================================

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

@@ -1208,7 +1208,8 @@
 *
 * Return true if tlb need be flushed.
 */
-static bool spte_write_protect(struct kvm *kvm, u64 *sptep, bool pt_protect)
+static bool spte_write_protect(struct kvm *kvm, u64 *sptep, bool pt_protect,
+			       bool skip_pinned)
 {
 	u64 spte = *sptep;
 

@@ -1218,6 +1219,22 @@
 
 	rmap_printk("rmap_write_protect: spte %p %llx\n", sptep, *sptep);
 
+	if (is_pinned_spte(spte)) {
+		/* keep pinned spte intact, mark page dirty again */
+		if (skip_pinned) {
+			struct kvm_mmu_page *sp;
+			gfn_t gfn;
+
+			sp = page_header(__pa(sptep));
+			gfn = kvm_mmu_page_get_gfn(sp, sptep - sp->spt);
+
+			mark_page_dirty(kvm, gfn);
+			return false;
+		} else
+			mmu_reload_pinned_vcpus(kvm);
+	}

arch/x86/kvm/mmu.c line 1240

Quoted 5 lines

arch/x86/kvm/mmu.c +2 -0

+
+
 	if (pt_protect)
 		spte &= ~SPTE_MMU_WRITEABLE;
 	spte = spte & ~PT_WRITABLE_MASK;

This is also a window between marking spte readonly and re-ping… IIUC, I think all spte spte can not be zapped and write-protected at any time

Skip pinned shadow pages when selecting pages to zap.

Quoted 1 line · On Jul 10, 2014, at 3:12 AM, [email protected] wrote:

Skip pinned shadow pages when selecting pages to zap.

It seems there is no way to prevent changing pinned spte on zap-all path?

I am thing if we could move pinned spte to another list (eg. pinned_shadow_pages) instead of active list so that it can not be touched by any other free paths. Your idea?

Quoted 7 lines · Xiao Guangrong

Quoted 1 line · On Jul 10, 2014, at 3:12 AM, [email protected] wrote:

Skip pinned shadow pages when selecting pages to zap.

It seems there is no way to prevent changing pinned spte on zap-all path?

Xiao,

The way would be to reload remote mmus, forcing the vcpu to exit, zapping a page, then vcpu will pagefault any necessary page via kvm_mmu_pin_pages.

kvm_mmu_invalidate_zap_all_pages does:

- spin_lock(mmu_lock)
- kvm_reload_remote_mmus
...
- spin_unlock(mmu_lock)

So its OK to change pinned spte on zap all path.

Quoted 3 lines

I am thing if we could move pinned spte to another list (eg. pinned_shadow_pages) instead of active list so that it can not be touched by any other free paths. Your idea?

As mentioned it above, it is ok to zap pinned sptes as long w reload remote mmus request is performed.

That said, you still consider a separate list?

Quoted 7 lines · Xiao Guangrong

Quoted 1 line · On Jul 10, 2014, at 3:12 AM, [email protected] wrote:

Skip pinned shadow pages when selecting pages to zap.

It seems there is no way to prevent changing pinned spte on zap-all path?

Xiao,

The way would be to reload remote mmus, forcing the vcpu to exit, zapping a page, then vcpu will pagefault any necessary page via kvm_mmu_pin_pages.

kvm_mmu_invalidate_zap_all_pages does:

- spin_lock(mmu_lock)
- kvm_reload_remote_mmus
...
- spin_unlock(mmu_lock)

So its OK to change pinned spte on zap all path.

Quoted 4 lines

I am thing if we could move pinned spte to another list (eg. pinned_shadow_pages) instead of active list so that it can not be touched by any other free paths. Your idea?

As mentioned it above, it is ok to zap pinned sptes as long w reload remote mmus request is performed.

That said, you still consider a separate list?

Required by PEBS support as discussed at

Subject: [patch 0/4] [patch 0/5] Implement PEBS virtualization for Silvermont
Message-Id: <[email protected]>

Thread.

----------------------

v2:
- unify remote kick function (Gleb)
- keep sptes intact on get_dirty_log (Gleb)
- add new internal error (Gleb)
- unpin sptes/shadow pages (Avi)
- fix spte walker level (Nadav)
- do not require get_page