KVM: MMU: fix forgot reserved bits check in speculative path

'walk_addr' is out of mmu_lock's protection, so while we handle 'fetch', then guest's mapping has modifited by other vcpu's write path, such as invlpg, pte_write and other fetch path

Fixed by checking all level's mapping

Quoted 8 lines · Xiao Guangrong

'walk_addr' is out of mmu_lock's protection, so while we handle 'fetch', then guest's mapping has modifited by other vcpu's write path, such as invlpg, pte_write and other fetch path

Fixed by checking all level's mapping

arch/x86/kvm/paging_tmpl.h static u64 *FNAME(fetch)(struct kvm_vcpu *vcpu, · line 324

Quoted 6 lines

arch/x86/kvm/paging_tmpl.h +2 -0

@@ -319,22 +319,23 @@ static u64 *FNAME(fetch)(struct kvm_vcpu *vcpu, 
 gva_t addr,
          direct_access&= ~ACC_WRITE_MASK;
 
      for_each_shadow_entry(vcpu, addr, iterator) {
+        bool nonpresent = false, last_mapping = false;
+

I don't like these two new variables, but no suggestion at the moment. I'll try to simplify this loop later.

One idea may be:

   while (level > walker.level) {
         handle indirect pages
   }
   while (level > hlevel) {
         handle direct pages
   }
   handle last spte

I'm worried that this change is too big for backporting, but no suggestions on how to make it smaller, so we'll have to accept it.

Introduce gfn_to_pfn_atomic(), it's the fast path and can used in atomic context, the later patch will use it

Signed-off-by Xiao Guangrong <[email protected]>

---
 include/linux/kvm_host.h |    1 +
 virt/kvm/kvm_main.c      |   32 +++++++++++++++++++++++++-------
 2 files changed, 26 insertions(+), 7 deletions(-)

include/linux/kvm_host.h +1 -0

index e796326..e0fb543 100644
@@ -295,6 +295,7 @@ void kvm_release_page_dirty(struct page *page);
 void kvm_set_page_dirty(struct page *page);
 void kvm_set_page_accessed(struct page *page);
 
+pfn_t gfn_to_pfn_atomic(struct kvm *kvm, gfn_t gfn);
 pfn_t gfn_to_pfn(struct kvm *kvm, gfn_t gfn);
 pfn_t gfn_to_pfn_memslot(struct kvm *kvm,
 			 struct kvm_memory_slot *slot, gfn_t gfn);

virt/kvm/kvm_main.c +4 -4

index a60b6b0..5467fe5 100644
@@ -934,19 +934,25 @@ unsigned long gfn_to_hva(struct kvm *kvm, gfn_t gfn)
 }
 EXPORT_SYMBOL_GPL(gfn_to_hva);
 
-static pfn_t hva_to_pfn(struct kvm *kvm, unsigned long addr)
+static pfn_t hva_to_pfn(struct kvm *kvm, unsigned long addr, bool atomic)
 {
 	struct page *page[1];
 	int npages;
 	pfn_t pfn;
 
-	might_sleep();
-
-	npages = get_user_pages_fast(addr, 1, 1, page);
+	if (atomic)
+		npages = __get_user_pages_fast(addr, 1, 1, page);
+	else {

virt/kvm/kvm_main.c +15 -3

 		down_read(&current->mm->mmap_sem);
 		if (is_hwpoison_address(addr)) {
 			up_read(&current->mm->mmap_sem);

@@ -959,6 +965,7 @@ static pfn_t hva_to_pfn(struct kvm *kvm, unsigned long addr)
 		if (vma == NULL || addr < vma->vm_start ||
 		    !(vma->vm_flags & VM_PFNMAP)) {
 			up_read(&current->mm->mmap_sem);
+return_bad_page:
 			get_page(bad_page);
 			return page_to_pfn(bad_page);
 		}

@@ -972,7 +979,7 @@ static pfn_t hva_to_pfn(struct kvm *kvm, unsigned long addr)
 	return pfn;
 }
 
-pfn_t gfn_to_pfn(struct kvm *kvm, gfn_t gfn)
+pfn_t __gfn_to_pfn(struct kvm *kvm, gfn_t gfn, bool atomic)
 {
 	unsigned long addr;
 

@@ -982,7 +989,18 @@ pfn_t gfn_to_pfn(struct kvm *kvm, gfn_t gfn)
 		return page_to_pfn(bad_page);
 	}
 
-	return hva_to_pfn(kvm, addr);
+	return hva_to_pfn(kvm, addr, atomic);
+}
+
+pfn_t gfn_to_pfn_atomic(struct kvm *kvm, gfn_t gfn)
+{
+	return __gfn_to_pfn(kvm, gfn, true);
+}
+EXPORT_SYMBOL_GPL(gfn_to_pfn_atomic);
+
+pfn_t gfn_to_pfn(struct kvm *kvm, gfn_t gfn)
+{
+	return __gfn_to_pfn(kvm, gfn, false);
 }
 EXPORT_SYMBOL_GPL(gfn_to_pfn);
 

@@ -990,7 +1008,7 @@ pfn_t gfn_to_pfn_memslot(struct kvm *kvm,
 			 struct kvm_memory_slot *slot, gfn_t gfn)
 {
 	unsigned long addr = gfn_to_hva_memslot(slot, gfn);
-	return hva_to_pfn(kvm, addr);
+	return hva_to_pfn(kvm, addr, false);
 }
 
 struct page *gfn_to_page(struct kvm *kvm, gfn_t gfn)

Signature · 8 lines

1.6.1.2


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

virt/kvm/kvm_main.c unsigned long gfn_to_hva(struct kvm *kvm, gfn_t gfn) · line 955

Quoted 8 lines

virt/kvm/kvm_main.c +4 -0

+	}
 
 	if (unlikely(npages != 1)) {
 		struct vm_area_struct *vma;
 
+		if (atomic)
+			goto return_bad_page;
+

You are skipping hwpoison test and sometimes you will return bad_page for something that returns good pfn now without caller even know. vma->vm_flags & VM_PFNMAP comes to mind. Is this safe?

(quoted diff) +4 -0

+		if (atomic)
+			goto return_bad_page;
+
 		down_read(&current->mm->mmap_sem);
 		if (is_hwpoison_address(addr)) {
 			up_read(&current->mm->mmap_sem);

@@ -959,6 +965,7 @@ static pfn_t hva_to_pfn(struct kvm *kvm, unsigned long addr)
 		if (vma == NULL || addr < vma->vm_start ||
 		    !(vma->vm_flags & VM_PFNMAP)) {
 			up_read(&current->mm->mmap_sem);
+return_bad_page:
 			get_page(bad_page);
 			return page_to_pfn(bad_page);
 		}

virt/kvm/kvm_main.c +3 -0

@@ -1011,6 +1021,23 @@ pfn_t gfn_to_pfn_memslot(struct kvm *kvm,
  	return hva_to_pfn(kvm, addr, false);
  }
 
+int gfn_to_page_many_atomic(struct kvm *kvm, gfn_t gfn,
+			    struct page **pages, int nr_pages, bool *enough)
+{

virt/kvm/kvm_main.c +5 -0

+	ret = __get_user_pages_fast(addr, entry, 1, pages);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(gfn_to_page_many_atomic);
+
  struct page *gfn_to_page(struct kvm *kvm, gfn_t gfn)
  {

pfn_t pfn;

Quoted 5 lines · Xiao Guangrong

This function not only return the gfn's page but also the page number after @gfn in the slot

virt/kvm/kvm_main.c static unsigned long gfn_to_hva_memslot(struct · line 930

Quoted 9 lines

virt/kvm/kvm_main.c +1 -1

@@ -923,15 +923,25 @@ static unsigned long gfn_to_hva_memslot(struct
 kvm_memory_slot *slot, gfn_t gfn)
      return slot->userspace_addr + (gfn - slot->base_gfn) * PAGE_SIZE;
  }
 
-unsigned long gfn_to_hva(struct kvm *kvm, gfn_t gfn)
+static unsigned long gfn_to_hva_many(struct kvm *kvm, gfn_t gfn, int
 *entry)
  {
 

'entry' better be gfn_t. We limit gfns in a slot, but that's artificial, let's use the correct type.

Also suggest nr_pages as the name.

Quoted 4 lines

virt/kvm/kvm_main.c +0 -0

@@ -1011,6 +1021,23 @@ pfn_t gfn_to_pfn_memslot(struct kvm *kvm,
      return hva_to_pfn(kvm, addr, false);
  }
 

virt/kvm/kvm_main.c pfn_t gfn_to_pfn_memslot(struct kvm *kvm,

Quoted 13 lines

virt/kvm/kvm_main.c +12 -0

+int gfn_to_page_many_atomic(struct kvm *kvm, gfn_t gfn,
+                struct page **pages, int nr_pages, bool *enough)
+{
+    unsigned long addr;
+    int entry, ret;
+
+    addr = gfn_to_hva_many(kvm, gfn,&entry);
+    if (kvm_is_error_hva(addr))
+        return -1;
+
+    entry = min(entry, nr_pages);
+    *enough = (entry == nr_pages) ? true : false;
 

s/ ? true : false//; :)

Why not return 0 if !enough?

Quoted 9 lines

virt/kvm/kvm_main.c +5 -0

+    ret = __get_user_pages_fast(addr, entry, 1, pages);
+    return ret;
+}
+EXPORT_SYMBOL_GPL(gfn_to_page_many_atomic);
+
  struct page *gfn_to_page(struct kvm *kvm, gfn_t gfn)
  {

      pfn_t pfn;

Quoted 5 lines

Quoted 5 lines

virt/kvm/kvm_main.c +0 -0

@@ -1011,6 +1021,23 @@ pfn_t gfn_to_pfn_memslot(struct kvm *kvm,
       return hva_to_pfn(kvm, addr, false);
   }
 

Quoted 18 lines

Quoted 4 lines

virt/kvm/kvm_main.c +4 -0

+int gfn_to_page_many_atomic(struct kvm *kvm, gfn_t gfn,
+                struct page **pages, int nr_pages, bool *enough)
+{
+    unsigned long addr;

virt/kvm/kvm_main.c pfn_t gfn_to_pfn_memslot(struct kvm *kvm,

Quoted 10 lines

virt/kvm/kvm_main.c +8 -0

+    int entry, ret;
+
+    addr = gfn_to_hva_many(kvm, gfn,&entry);
+    if (kvm_is_error_hva(addr))
+        return -1;
+
+    entry = min(entry, nr_pages);
+    *enough = (entry == nr_pages) ? true : false;
 
 

Why not return 0 if !enough?

I think it's better that handle the reset pages in the slot, for example, we expect 16 pages are consecutive, but only 12 pages in the slot, the better way is handle the reset 12 pages not throw those away.

Quoted 5 lines

Quoted 1 line

I think it's better that handle the reset pages in the slot, for example, we expect 16 pages are consecutive, but only 12 pages in the slot, the better
way is handle the reset 12 pages not throw those away.

It will almost never happen, let's remove edge cases.

Introduce this function to topup prefetch cache

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

index 3dcd55d..cda4587 100644

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

@@ -316,15 +318,16 @@ static void update_spte(u64 *sptep, u64 new_spte)
  	}
  }
 
-static int mmu_topup_memory_cache(struct kvm_mmu_memory_cache *cache,
-				  struct kmem_cache *base_cache, int min)
+static int __mmu_topup_memory_cache(struct kvm_mmu_memory_cache *cache,
+				    struct kmem_cache *base_cache, int min,
+				    int max, gfp_t flags)
  {
  	void *obj;
 
  	if (cache->nobjs>= min)
  		return 0;
-	while (cache->nobjs<  ARRAY_SIZE(cache->objects)) {
-		obj = kmem_cache_zalloc(base_cache, GFP_KERNEL);
+	while (cache->nobjs<  max) {
+		obj = kmem_cache_zalloc(base_cache, flags);
  		if (!obj)
  			return -ENOMEM;
  		cache->objects[cache->nobjs++] = obj;

@@ -332,6 +335,20 @@ static int mmu_topup_memory_cache(struct kvm_mmu_memory_cache *cache,
  	return 0;
  }
 
+static int mmu_topup_memory_cache(struct kvm_mmu_memory_cache *cache,
+				  struct kmem_cache *base_cache, int min)
+{
+	return __mmu_topup_memory_cache(cache, base_cache, min,
+				      ARRAY_SIZE(cache->objects), GFP_KERNEL);

Quoted 12 lines · Xiao Guangrong

Introduce this function to topup prefetch cache

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

index 3dcd55d..cda4587 100644
@@ -89,6 +89,8 @@ module_param(oos_shadow, bool, 0644);
      }
  #endif
 

arch/x86/kvm/mmu.c module_param(oos_shadow, bool, 0644); · line 96

Quoted 2 lines

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

+#define PTE_PREFETCH_NUM        16
 

Let's make it 8 to start with... It's frightening enough.

(8 = one cache line in both guest and host)

Quoted 38 lines

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

@@ -316,15 +318,16 @@ static void update_spte(u64 *sptep, u64 new_spte)
      }
  }
 
-static int mmu_topup_memory_cache(struct kvm_mmu_memory_cache *cache,
-                  struct kmem_cache *base_cache, int min)
+static int __mmu_topup_memory_cache(struct kvm_mmu_memory_cache *cache,
+                    struct kmem_cache *base_cache, int min,
+                    int max, gfp_t flags)
  {
      void *obj;
 
      if (cache->nobjs>= min)
          return 0;
-    while (cache->nobjs<  ARRAY_SIZE(cache->objects)) {
-        obj = kmem_cache_zalloc(base_cache, GFP_KERNEL);
+    while (cache->nobjs<  max) {
+        obj = kmem_cache_zalloc(base_cache, flags);
          if (!obj)
              return -ENOMEM;
          cache->objects[cache->nobjs++] = obj;

@@ -332,6 +335,20 @@ static int mmu_topup_memory_cache(struct
 kvm_mmu_memory_cache *cache,
      return 0;
  }
 
+static int mmu_topup_memory_cache(struct kvm_mmu_memory_cache *cache,
+                  struct kmem_cache *base_cache, int min)
+{
+    return __mmu_topup_memory_cache(cache, base_cache, min,
+                      ARRAY_SIZE(cache->objects), GFP_KERNEL);
+}
+
+static int pte_prefetch_topup_memory_cache(struct kvm_vcpu *vcpu)
+{
+    return __mmu_topup_memory_cache(&vcpu->arch.mmu_rmap_desc_cache,
+                    rmap_desc_cache, PTE_PREFETCH_NUM,

arch/x86/kvm/mmu.c static int mmu_topup_memory_cache(struct kvm_mmu_memory_cache *cache,

Quoted 4 lines

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

+                    PTE_PREFETCH_NUM, GFP_ATOMIC);
+}
+
 

Just make the ordinary topup sufficient for prefetch. If we allocate too much, we don't lose anything, the memory remains for the next time around.

Quoted 19 lines · Avi Kivity

Quoted 9 lines · Xiao Guangrong

Introduce this function to topup prefetch cache

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

index 3dcd55d..cda4587 100644

arch/x86/kvm/mmu.c module_param(oos_shadow, bool, 0644); · line 94

Quoted 6 lines

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

@@ -89,6 +89,8 @@ module_param(oos_shadow, bool, 0644);
       }
   #endif
 
+#define PTE_PREFETCH_NUM        16
 
 

Let's make it 8 to start with... It's frightening enough.

Quoted 2 lines

(8 = one cache line in both guest and host)

Umm, before post this patchset, i have done the draft performance test for different prefetch distance, and it shows 16 is the best distance that we can get highest performance.

Quoted 4 lines

Quoted 4 lines

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

+static int pte_prefetch_topup_memory_cache(struct kvm_vcpu *vcpu)
+{
+    return __mmu_topup_memory_cache(&vcpu->arch.mmu_rmap_desc_cache,
+                    rmap_desc_cache, PTE_PREFETCH_NUM,

Quoted 10 lines

arch/x86/kvm/mmu.c static int mmu_topup_memory_cache(struct kvm_mmu_memory_cache *cache, · line 351

Quoted 5 lines

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

+                    PTE_PREFETCH_NUM, GFP_ATOMIC);
+}
+

Just make the ordinary topup sufficient for prefetch. If we allocate too much, we don't lose anything, the memory remains for the next time around.

Umm, but at the worst case, we should allocate 40 items for rmap, it's heavy for GFP_ATOMIC allocation and holding mmu_lock.

Quoted 22 lines · Xiao Guangrong

Quoted 19 lines · Avi Kivity

Quoted 9 lines · Xiao Guangrong

Introduce this function to topup prefetch cache

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

index 3dcd55d..cda4587 100644

arch/x86/kvm/mmu.c module_param(oos_shadow, bool, 0644); · line 94

Quoted 6 lines

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

@@ -89,6 +89,8 @@ module_param(oos_shadow, bool, 0644);
       }
   #endif
 
+#define PTE_PREFETCH_NUM        16
 
 

Let's make it 8 to start with... It's frightening enough.

Quoted 8 lines

Quoted 2 lines

(8 = one cache line in both guest and host)

Umm, before post this patchset, i have done the draft performance test for
different prefetch distance, and it shows 16 is the best distance that we can
get highest performance.

What's the different between 8 and 16?

I'm worried that there are workloads that don't benefit from prefetch, and we may regress there. So I'd like to limit it, at least at first.

btw, what about dirty logging? will prefetch cause pages to be marked dirty?

We may need to instantiate prefetched pages with spte.d=0 and examine it when tearing down the spte.

Quoted 14 lines

Quoted 4 lines

Quoted 4 lines

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

+static int pte_prefetch_topup_memory_cache(struct kvm_vcpu *vcpu)
+{
+    return __mmu_topup_memory_cache(&vcpu->arch.mmu_rmap_desc_cache,
+                    rmap_desc_cache, PTE_PREFETCH_NUM,

Quoted 10 lines

arch/x86/kvm/mmu.c static int mmu_topup_memory_cache(struct kvm_mmu_memory_cache *cache, · line 351

Quoted 5 lines

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

+                    PTE_PREFETCH_NUM, GFP_ATOMIC);
+}
+

Just make the ordinary topup sufficient for prefetch. If we allocate too much, we don't lose anything, the memory remains for the next time around.

Quoted 5 lines

Umm, but at the worst case, we should allocate 40 items for rmap, it's heavy
for GFP_ATOMIC allocation and holding mmu_lock.

Why use GFP_ATOMIC at all? Make mmu_topup_memory_caches() always assume we'll be prefetching.

Why 40? I think all we need is PTE_PREFETCH_NUM rmap entries.

Quoted 5 lines

btw, what about dirty logging? will prefetch cause pages to be marked dirty?

Quoted 4 lines

We may need to instantiate prefetched pages with spte.d=0 and examine it when tearing down the spte.

Yeah, all speculative path are broken dirty bit tracking, and i guess it's need more review, so i plan to do it in the separate patch, i'll post it after this patchset merged, could you allow it?

Quoted 9 lines · Xiao Guangrong

Quoted 9 lines

btw, what about dirty logging? will prefetch cause pages to be marked dirty?

We may need to instantiate prefetched pages with spte.d=0 and examine it when tearing down the spte.

Quoted 7 lines

Yeah, all speculative path are broken dirty bit tracking, and i guess it's
need more review, so i plan to do it in the separate patch, i'll post it after
this patchset merged, could you allow it?

Regressions? no. Or do you mean the problem already exists? Where?

Quoted 3 lines

Regressions? no. Or do you mean the problem already exists? Where?

I mean this is a exist problem, likes invlpg, pte-write and sync-page, there are speculative path that it's not real access, but marked dirty if pte is writable.

Quoted 8 lines · Xiao Guangrong

Quoted 3 lines

Regressions? no. Or do you mean the problem already exists? Where?

I mean this is a exist problem, likes invlpg, pte-write and sync-page, there are
speculative path that it's not real access, but marked dirty if pte is writable.

Right. We should fix those too.

Prefetch is much more worrying though, especially with ept. If a guest is using just 1/8 of the pages, it can look to migration as if it's using 100% of the pages. The impact can be pretty large.

Quoted 7 lines

Right. We should fix those too.

Prefetch is much more worrying though, especially with ept. If a guest is using just 1/8 of the pages, it can look to migration as if it's using 100% of the pages. The impact can be pretty large.

We disabled prefetch if ept is enabled since it can break access bit tracking.

I'll fix the dirty bit tracking before post the new version of this patchset.

Quoted 10 lines · Xiao Guangrong

Quoted 7 lines

Right. We should fix those too.

Prefetch is much more worrying though, especially with ept. If a guest is using just 1/8 of the pages, it can look to migration as if it's using 100% of the pages. The impact can be pretty large.

We disabled prefetch if ept is enabled since it can break access bit tracking.

Oh yes.

Quoted 3 lines

I'll fix the dirty bit tracking before post the new version of this patchset.

Should be simple - disable prefetch for slots that have dirty tracking enabled.

What about the Linux accessed and dirty bits? Need to instantiate the speculative sptes with accessed and dirty bits clear (and later examine them when we release the page).

Quoted 6 lines

What about the Linux accessed and dirty bits? Need to instantiate the speculative sptes with accessed and dirty bits clear (and later examine them when we release the page).

I see, will do :-)

Quoted 6 lines · Xiao Guangrong

Quoted 6 lines

What about the Linux accessed and dirty bits? Need to instantiate the speculative sptes with accessed and dirty bits clear (and later examine them when we release the page).

Quoted 2 lines

I see, will do :-)

This is getting bigger and bigger...

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

What about the Linux accessed and dirty bits? Need to instantiate the speculative sptes with accessed and dirty bits clear (and later examine them when we release the page).

Quoted 6 lines

Quoted 3 lines

I see, will do :-)

This is getting bigger and bigger...

What are you worry about? or has fault in this way? :-(

Combine guest pte read between guest pte check in the fetch path and pte prefetch

Signed-off-by Xiao Guangrong <[email protected]>

---
 arch/x86/kvm/paging_tmpl.h |   69 ++++++++++++++++++++++++++-----------------
 1 files changed, 42 insertions(+), 27 deletions(-)

arch/x86/kvm/paging_tmpl.h +5 -4

index e04c1a4..a1e6d91 100644
@@ -67,6 +67,7 @@ struct guest_walker {
 	int level;
 	gfn_t table_gfn[PT_MAX_FULL_LEVELS];
 	pt_element_t ptes[PT_MAX_FULL_LEVELS];
+	pt_element_t prefetch_ptes[PTE_PREFETCH_NUM];
 	gpa_t pte_gpa[PT_MAX_FULL_LEVELS];
 	unsigned pt_access;
 	unsigned pte_access;

@@ -291,12 +292,12 @@ static void FNAME(update_pte)(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp,
 		     gpte_to_gfn(gpte), pfn, true, true);
 }
 
-static void FNAME(pte_prefetch)(struct kvm_vcpu *vcpu, u64 *sptep)
+static void FNAME(pte_prefetch)(struct kvm_vcpu *vcpu, struct guest_walker *gw,
+				u64 *sptep)
 {
 	struct kvm_mmu_page *sp;
-	pt_element_t gptep[PTE_PREFETCH_NUM];
-	gpa_t first_pte_gpa;
-	int offset = 0, index, i, j, max;
+	pt_element_t *gptep;
+	int index, i, j, max;
 
 	sp = page_header(__pa(sptep));
 	index = sptep - sp->spt;

@@ -311,15 +312,7 @@ static void FNAME(pte_prefetch)(struct kvm_vcpu *vcpu, u64 *sptep)
 	i = index & ~(PTE_PREFETCH_NUM - 1);
 	max = index | (PTE_PREFETCH_NUM - 1);
 

arch/x86/kvm/paging_tmpl.h static void FNAME(pte_prefetch)(struct kvm_vcpu *vcpu, u64 *sptep) · line 315

Quoted 8 lines

arch/x86/kvm/paging_tmpl.h +1 -7

-
-	first_pte_gpa = gfn_to_gpa(sp->gfn) +
-				(offset + i) * sizeof(pt_element_t);
-
-	if (kvm_read_guest_atomic(vcpu->kvm, first_pte_gpa, gptep,
-					sizeof(gptep)) < 0)
-		return;
+	gptep = gw->prefetch_ptes;

Where do you reread the gpte in the prefetch path?

Quoted 11 lines · Marcelo Tosatti

arch/x86/kvm/paging_tmpl.h static void FNAME(pte_prefetch)(struct kvm_vcpu *vcpu, u64 *sptep) · line 315

Quoted 8 lines

arch/x86/kvm/paging_tmpl.h +1 -7

-
-	first_pte_gpa = gfn_to_gpa(sp->gfn) +
-				(offset + i) * sizeof(pt_element_t);
-
-	if (kvm_read_guest_atomic(vcpu->kvm, first_pte_gpa, gptep,
-					sizeof(gptep)) < 0)
-		return;
+	gptep = gw->prefetch_ptes;

Where do you reread the gpte in the prefetch path?

Marcelo,

Thanks for your review.

Below codes read gptes in the prefetch path:

	index = sptep - sp->spt;
	i = index & ~(PTE_PREFETCH_NUM - 1);
	max = index | (PTE_PREFETCH_NUM - 1);

	if (PTTYPE == 32)
		offset = sp->role.quadrant << PT64_LEVEL_BITS;

	first_pte_gpa = gfn_to_gpa(sp->gfn) +
				(offset + i) * sizeof(pt_element_t);

	if (kvm_read_guest_atomic(vcpu->kvm, first_pte_gpa, gptep,
					sizeof(gptep)) < 0)
		return;

It reads the 16 aligned items around sptep's corresponding gpte and this gpte is also in this area. :-)

Quoted 13 lines

Below codes read gptes in the prefetch path:

	index = sptep - sp->spt;
	i = index & ~(PTE_PREFETCH_NUM - 1);
	max = index | (PTE_PREFETCH_NUM - 1);

	if (PTTYPE == 32)
		offset = sp->role.quadrant << PT64_LEVEL_BITS;

	first_pte_gpa = gfn_to_gpa(sp->gfn) +
				(offset + i) * sizeof(pt_element_t);

Quoted 6 lines

	if (kvm_read_guest_atomic(vcpu->kvm, first_pte_gpa, gptep,
					sizeof(gptep)) < 0)
		return;

It reads the 16 aligned items around sptep's corresponding gpte and this gpte is also in this area. :-)

But you removed that in patch 8?

Quoted 17 lines · Marcelo Tosatti

Quoted 17 lines

Below codes read gptes in the prefetch path:

	index = sptep - sp->spt;
	i = index & ~(PTE_PREFETCH_NUM - 1);
	max = index | (PTE_PREFETCH_NUM - 1);

	if (PTTYPE == 32)
		offset = sp->role.quadrant << PT64_LEVEL_BITS;

	first_pte_gpa = gfn_to_gpa(sp->gfn) +
				(offset + i) * sizeof(pt_element_t);

	if (kvm_read_guest_atomic(vcpu->kvm, first_pte_gpa, gptep,
					sizeof(gptep)) < 0)
		return;

Quoted 5 lines

Quoted 2 lines

It reads the 16 aligned items around sptep's corresponding gpte and this gpte is also in this area. :-)

But you removed that in patch 8?

Oh, i just want it good for review, you mean it's better let patch 7 and 8 in one patch?

Quoted 6 lines

Quoted 5 lines

Quoted 3 lines

It reads the 16 aligned items around sptep's corresponding gpte and this gpte is also in this area. :-)

But you removed that in patch 8?

Oh, i just want it good for review, you mean it's better let patch 7 and 8 in one patch?

I mean patch 7 is wrong because it removes rereading of gpte, with mmu_lock held, in the prefetch patch.

Quoted 10 lines · Marcelo Tosatti

Quoted 6 lines

Quoted 5 lines

Quoted 3 lines

It reads the 16 aligned items around sptep's corresponding gpte and this gpte is also in this area. :-)

But you removed that in patch 8?

Oh, i just want it good for review, you mean it's better let patch 7 and 8 in one patch?

I mean patch 7 is wrong because it removes rereading of gpte, with mmu_lock held, in the prefetch patch.

Marcelo,

Sorry, i don't understand it clearly.

In the patch 7, we read gpte like this:

hold mmu_lock
atomic read gpte during guest mapping's checking in 'fetch' path atomic read 16 aligned items chunk (include gpte) in pte_prefetch path release mmu_lock

And in the patch 8, we do it like this:

	hold mmu_lock

	atomic read 16 aligned items chunk (include gpte) in 'fetch' path, and
        saved this chunk to gw->prefetch_ptes
	
        get gptes form gw->prefetch_ptes in pte_prefetch path

	release mmu_lock

Could you please tell me where is wrong?

In the speculative path, we should check guest pte's reserved bits just as the real processor does

Reported-by Marcelo Tosatti<[email protected]>

Signed-off-by Xiao Guangrong<[email protected]>

---
  arch/x86/kvm/mmu.c         |    3 +++
  arch/x86/kvm/paging_tmpl.h |    3 ++-
  2 files changed, 5 insertions(+), 1 deletions(-)

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

index 104756b..3dcd55d 100644

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

  	mmu_guess_page_from_pte_write(vcpu, gpa, gentry);
  	spin_lock(&vcpu->kvm->mmu_lock);

if (atomic_read(&vcpu->kvm->arch.invlpg_counter) != invlpg_counter)

arch/x86/kvm/paging_tmpl.h +0 -0

index dfb2720..19f0077 100644

arch/x86/kvm/mmu.c void kvm_mmu_pte_write(struct kvm_vcpu *vcpu, gpa_t gpa, · line 2786

Quoted 4 lines

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

+    if (is_rsvd_bits_set(vcpu, gentry, PT_PAGE_TABLE_LEVEL))
+        gentry = 0;
+

That only works if the gpte is for the same mode as the current vcpu mmu mode. In some cases it is too strict (vcpu in pae mode writing a 32-bit gpte), which is not too bad, in some cases it is too permissive (vcpu in nonpae mode writing a pae gpte).

(once upon a time mixed modes were rare, only on OS setup, but with nested virt they happen all the time).

Quoted 12 lines

      mmu_guess_page_from_pte_write(vcpu, gpa, gentry);
      spin_lock(&vcpu->kvm->mmu_lock);
      if (atomic_read(&vcpu->kvm->arch.invlpg_counter) != invlpg_counter)

arch/x86/kvm/paging_tmpl.h +0 -0

index dfb2720..19f0077 100644
@@ -628,7 +628,8 @@ static int FNAME(sync_page)(struct kvm_vcpu *vcpu,
 struct kvm_mmu_page *sp,
          pte_gpa = first_pte_gpa + i * sizeof(pt_element_t);
 

arch/x86/kvm/paging_tmpl.h static int FNAME(sync_page)(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp, · line 634

Quoted 6 lines

arch/x86/kvm/paging_tmpl.h +2 -1

          if (kvm_read_guest_atomic(vcpu->kvm, pte_gpa,&gpte,
-                      sizeof(pt_element_t)))
+                      sizeof(pt_element_t)) ||
+              is_rsvd_bits_set(vcpu, gpte, PT_PAGE_TABLE_LEVEL))
              return -EINVAL;
 

This is better done a few lines down where we check for !is_present_gpte(), no?

Quoted 6 lines

arch/x86/kvm/mmu.c void kvm_mmu_pte_write(struct kvm_vcpu *vcpu, gpa_t gpa, · line 2786

Quoted 6 lines

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

+    if (is_rsvd_bits_set(vcpu, gentry, PT_PAGE_TABLE_LEVEL))
+        gentry = 0;
+

Quoted 6 lines

That only works if the gpte is for the same mode as the current vcpu mmu mode. In some cases it is too strict (vcpu in pae mode writing a 32-bit gpte), which is not too bad, in some cases it is too permissive (vcpu in nonpae mode writing a pae gpte).

Avi, thanks for your review.

Do you mean that the VM has many different mode vcpu? For example, both nonpae vcpu and pae vcpu are running in one VM? I forgot to consider this case.

Quoted 3 lines

(once upon a time mixed modes were rare, only on OS setup, but with nested virt they happen all the time).

I'm afraid it's still has problem, it will cause access corruption:
1: if nonpae vcpu write pae gpte, it will miss NX bit
2: if pae vcpu write nonpae gpte, it will add NX bit that over gpte's width

How about only update the shadow page which has the same pae set with the written vcpu? Just like this:

Quoted 5 lines

How about only update the shadow page which has the same pae set with the written
vcpu? Just like this:

Review void kvm_mmu_pte_write(struct kvm_vcpu *vcpu, · line 3008

Quoted 10 lines

(quoted diff) +4 -0

@@ -3000,6 +3000,10 @@ void kvm_mmu_pte_write(struct kvm_vcpu *vcpu,
 gpa_t gpa,
                 while (npte--) {
                         entry = *spte;
                         mmu_pte_write_zap_pte(vcpu, sp, spte);
+
+                       if (!!is_pae(vcpu) != sp->role.cr4_pae)
+                               continue;
+
 

Not enough, one vcpu can have nx set while the other has it reset, etc.