KVM: MMU: fix conflict access permissions in direct sp

Introduce gfn_to_page_atomic() and gfn_to_pfn_atomic(), those functions is fast path and can used in atomic context, the later patch will use those

virt/kvm/kvm_main.c +11 -0

@@ -942,6 +942,41 @@ unsigned long gfn_to_hva(struct kvm *kvm, gfn_t gfn)
  }
  EXPORT_SYMBOL_GPL(gfn_to_hva);
 
+static pfn_t hva_to_pfn_atomic(struct kvm *kvm, unsigned long addr)
+{
+	struct page *page[1];
+	int npages;
+	pfn_t pfn;
+
+	npages = __get_user_pages_fast(addr, 1, 1, page);
+
+	if (unlikely(npages != 1)) {
+		if (is_hwpoison_address(addr)) {
+			get_page(hwpoison_page);

Too much duplication. How about putting the tail end of the function in a common helper (with an inatomic flag)?

btw, is_hwpoison_address() is racy. While it looks up the address, some other task can unmap the page tables under us.

Quoted 6 lines · Avi Kivity

Too much duplication. How about putting the tail end of the function in a common helper (with an inatomic flag)?

btw, is_hwpoison_address() is racy. While it looks up the address, some other task can unmap the page tables under us.

Where is is_hwpoison_address() coming from? I can't find it anywhere.

Anyways hwpoison will not remove the page as long as there is a reference to it, so as long as you get the reference race free against another task you're ok. Of course there might be always an error in between and the hardware may poison the data, but we don't try to handle all kernel errors.

The page is fine, the page tables are not. Another task can munmap() the thing while is_hwpoison_address() is running.

Quoted 3 lines

The page is fine, the page tables are not. Another task can munmap() the thing while is_hwpoison_address() is running.

Ok that boils down to me not seeing that source.

If it accesses the page tables yes then it's racy. But whoever looked up the page tables in the first place should have returned an -EFAULT. There's no useful address attached to poison.

btw, is_hwpoison_address() is racy.  While it looks up the address, some other task can unmap the page tables under us.

Andi/Huang?

One way of fixing it is get_user_pages_ptes_fast(), which also returns the pte, also atomically.  I want it for other reasons as well (respond to a read fault by gupping the page for read, but allowing write access if the pte indicates it is writeable).

Quoted 5 lines · Avi Kivity

btw, is_hwpoison_address() is racy. While it looks up the address, some other task can unmap the page tables under us.

Andi/Huang?

Quoted 5 lines

One way of fixing it is get_user_pages_ptes_fast(), which also returns the pte, also atomically. I want it for other reasons as well (respond to a read fault by gupping the page for read, but allowing write access if the pte indicates it is writeable).

Yes. is_hwpoison_address() is racy. But I think it is not absolutely necessary to call is_hwpoison_address() in hva_to_pfn_atomic(), is it?

For is_hwpoison_address() in hva_to_pfn(), we can protect it with mmap_sem.

Quoted 25 lines · Xiao Guangrong

Introduce gfn_to_page_atomic() and gfn_to_pfn_atomic(), those functions is fast path and can used in atomic context, the later patch will use those

virt/kvm/kvm_main.c +16 -0

@@ -942,6 +942,41 @@ unsigned long gfn_to_hva(struct kvm *kvm, gfn_t gfn)
  }
  EXPORT_SYMBOL_GPL(gfn_to_hva);
 
+static pfn_t hva_to_pfn_atomic(struct kvm *kvm, unsigned long addr)
+{
+    struct page *page[1];
+    int npages;
+    pfn_t pfn;
+
+    npages = __get_user_pages_fast(addr, 1, 1, page);
+
+    if (unlikely(npages != 1)) {
+        if (is_hwpoison_address(addr)) {
+            get_page(hwpoison_page);
+            return page_to_pfn(hwpoison_page);
+        }
+        get_page(bad_page);
+        return page_to_pfn(bad_page);
+    } else

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

Quoted 5 lines

virt/kvm/kvm_main.c +4 -0

+        pfn = page_to_pfn(page[0]);
+
+    return pfn;
+}
 

Too much duplication. How about putting the tail end of the function in a common helper (with an inatomic flag)?

Trace pte prefetch to see what trouble we meet, if can help us to improve the prefetch

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

---
  arch/x86/kvm/mmu.c         |   12 +++++++++++-
  arch/x86/kvm/mmutrace.h    |   26 ++++++++++++++++++++++++++
  arch/x86/kvm/paging_tmpl.h |    9 ++++++++-
  3 files changed, 45 insertions(+), 2 deletions(-)

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

index 941c86b..0aaa18d 100644
@@ -91,6 +91,11 @@ module_param(oos_shadow, bool, 0644);
 
  #define PTE_PREFETCH_NUM	16
 
+#define PREFETCH_SUCCESS	0
+#define PREFETCH_ERR_GFN2PFN	1
+#define PREFETCH_ERR_ALLOC_MEM	2
+#define PREFETCH_ERR_READ_GPTE	3
+
  #define PT_FIRST_AVAIL_BITS_SHIFT 9
  #define PT64_SECOND_AVAIL_BITS_SHIFT 52
 

arch/x86/kvm/mmu.c static void direct_pte_prefetch(struct · line 2078

Quoted 9 lines

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

@@ -2066,11 +2071,16 @@ static void direct_pte_prefetch(struct
 kvm_vcpu *vcpu, u64 *sptep)
          pfn = gfn_to_pfn_atomic(vcpu->kvm, gfn);
          if (is_error_pfn(pfn)) {
              kvm_release_pfn_clean(pfn);
+            trace_pte_prefetch(true, PREFETCH_ERR_GFN2PFN);
              break;
          }
 

What about virtual addresses and gptes? They can be helpful too.

Support prefetch ptes when intercept guest #PF, avoid to #PF by later access

If we meet any failure in the prefetch path, we will exit it and not try other ptes to avoid become heavy path

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

+#define PTE_PREFETCH_NUM	16
+
  #define PT_FIRST_AVAIL_BITS_SHIFT 9
  #define PT64_SECOND_AVAIL_BITS_SHIFT 52

@@ -2041,6 +2043,39 @@ static void nonpaging_new_cr3(struct kvm_vcpu *vcpu)
  {
  }
 
+static void direct_pte_prefetch(struct kvm_vcpu *vcpu, u64 *sptep)
+{
+	struct kvm_mmu_page *sp;
+	int index, i;
+
+	sp = page_header(__pa(sptep));
+	WARN_ON(!sp->role.direct);
+	index = sptep - sp->spt;
+
+	for (i = index + 1; i<  min(PT64_ENT_PER_PAGE,

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

+
+		pfn = gfn_to_pfn_atomic(vcpu->kvm, gfn);
+		if (is_error_pfn(pfn)) {
+			kvm_release_pfn_clean(pfn);

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

+static void FNAME(pte_prefetch)(struct kvm_vcpu *vcpu, u64 *sptep)
+{
+	struct kvm_mmu_page *sp;
+	pt_element_t *table = NULL;
+	int offset = 0, shift, index, i;
+
+	sp = page_header(__pa(sptep));
+	index = sptep - sp->spt;
+
+	if (PTTYPE == 32) {
+		shift = PAGE_SHIFT - (PT_LEVEL_BITS -
+					PT64_LEVEL_BITS) * sp->role.level;
+		offset = sp->role.quadrant<<  shift;
+	}
+
+	for (i = index + 1; i<  min(PT64_ENT_PER_PAGE,
+				      index + PTE_PREFETCH_NUM); i++) {
+		struct page *page;
+		pt_element_t gpte;
+		unsigned pte_access;
+		u64 *spte = sp->spt + i;
+		gfn_t gfn;
+		pfn_t pfn;
+		int dirty;
+

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

+			continue;
+		}
+		dirty = is_dirty_gpte(gpte);
+		gfn = (gpte&  PT64_BASE_ADDR_MASK)>>  PAGE_SHIFT;
+		pte_access = pte_access&  FNAME(gpte_access)(vcpu, gpte);
+gfn_mapping:
+		pfn = gfn_to_pfn_atomic(vcpu->kvm, gfn);
+		if (is_error_pfn(pfn)) {
+			kvm_release_pfn_clean(pfn);
+			break;
+		}

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

+
  /*
   * Fetch a shadow pte for a specific level in the paging hierarchy.
   */

@@ -322,6 +397,7 @@ static u64 *FNAME(fetch)(struct kvm_vcpu *vcpu, gva_t addr,
  				     is_dirty_gpte(gw->ptes[gw->level-1]),
  				     ptwrite, level,
  				     gw->gfn, pfn, false, true);
+			FNAME(pte_prefetch)(vcpu, sptep);
  			break;
  		}
 

arch/x86/kvm/mmu.c static void nonpaging_new_cr3(struct kvm_vcpu *vcpu)

Quoted 5 lines

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

+        if (*spte != shadow_trap_nonpresent_pte)
+            continue;
+
+        gfn = sp->gfn + (i<<  ((sp->role.level - 1) * PT64_LEVEL_BITS));

Can calculate outside the loop and use +=.

Can this in fact work for level != PT_PAGE_TABLE_LEVEL? We might start at PT_PAGE_DIRECTORY_LEVEL but get 4k pages while iterating.

Quoted 5 lines

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

+
+        pfn = gfn_to_pfn_atomic(vcpu->kvm, gfn);
+        if (is_error_pfn(pfn)) {

arch/x86/kvm/mmu.c static void nonpaging_new_cr3(struct kvm_vcpu *vcpu) · line 2077

Quoted 10 lines

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

+            kvm_release_pfn_clean(pfn);
+            break;
+        }
+        if (pte_prefetch_topup_memory_cache(vcpu))
+            break;
+
+        mmu_set_spte(vcpu, spte, ACC_ALL, ACC_ALL, 0, 0, 1, NULL,
+                 sp->role.level, gfn, pfn, true, false);
+    }
+}

Nice. Direct prefetch should usually succeed.

Can later augment to call get_users_pages_fast(..., PTE_PREFETCH_NUM, ...) to reduce gup overhead.

Quoted 32 lines

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

+static void FNAME(pte_prefetch)(struct kvm_vcpu *vcpu, u64 *sptep)
+{
+    struct kvm_mmu_page *sp;
+    pt_element_t *table = NULL;
+    int offset = 0, shift, index, i;
+
+    sp = page_header(__pa(sptep));
+    index = sptep - sp->spt;
+
+    if (PTTYPE == 32) {
+        shift = PAGE_SHIFT - (PT_LEVEL_BITS -
+                    PT64_LEVEL_BITS) * sp->role.level;
+        offset = sp->role.quadrant<<  shift;
+    }
+
+    for (i = index + 1; i<  min(PT64_ENT_PER_PAGE,
+                      index + PTE_PREFETCH_NUM); i++) {
+        struct page *page;
+        pt_element_t gpte;
+        unsigned pte_access;
+        u64 *spte = sp->spt + i;
+        gfn_t gfn;
+        pfn_t pfn;
+        int dirty;
+
+        if (*spte != shadow_trap_nonpresent_pte)
+            continue;
+
+        pte_access = sp->role.access;
+        if (sp->role.direct) {

arch/x86/kvm/paging_tmpl.h static void FNAME(update_pte)(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp, · line 328

Quoted 6 lines

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

+            dirty = 1;
+            gfn = sp->gfn + (i<<  ((sp->role.level - 1) *
+                          PT64_LEVEL_BITS));
+            goto gfn_mapping;
+        }

Should just call direct_pte_prefetch.

arch/x86/kvm/paging_tmpl.h static void FNAME(update_pte)(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp, · line 338

Quoted 11 lines

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

+
+        if (!table) {
+            page = gfn_to_page_atomic(vcpu->kvm, sp->gfn);
+            if (is_error_page(page)) {
+                kvm_release_page_clean(page);
+                break;
+            }
+            table = kmap_atomic(page, KM_USER0);
+            table = (pt_element_t *)((char *)table + offset);
+        }

Why not kvm_read_guest_atomic()? Can do it outside the loop.

arch/x86/kvm/paging_tmpl.h static void FNAME(update_pte)(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp, · line 346

Quoted 10 lines

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

+
+        gpte = table[i];
+        if (!(gpte&  PT_ACCESSED_MASK))
+            continue;
+
+        if (!is_present_gpte(gpte)) {
+            if (!sp->unsync)
+                *spte = shadow_notrap_nonpresent_pte;

Need __set_spte().

Quoted 17 lines

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

+            continue;
+        }
+        dirty = is_dirty_gpte(gpte);
+        gfn = (gpte&  PT64_BASE_ADDR_MASK)>>  PAGE_SHIFT;
+        pte_access = pte_access&  FNAME(gpte_access)(vcpu, gpte);
+gfn_mapping:
+        pfn = gfn_to_pfn_atomic(vcpu->kvm, gfn);
+        if (is_error_pfn(pfn)) {
+            kvm_release_pfn_clean(pfn);
+            break;
+        }
+
+        if (pte_prefetch_topup_memory_cache(vcpu))
+            break;
+        mmu_set_spte(vcpu, spte, sp->role.access, pte_access, 0, 0,
+                 dirty, NULL, sp->role.level, gfn, pfn,

arch/x86/kvm/paging_tmpl.h static void FNAME(update_pte)(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp, · line 367

Quoted 6 lines

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

+                 true, false);
+    }
+    if (table)
+        kunmap_atomic((char *)table - offset, KM_USER0);
+}

I think lot of code can be shared with the pte prefetch in invlpg.

Quoted 5 lines

Can this in fact work for level != PT_PAGE_TABLE_LEVEL? We might start at PT_PAGE_DIRECTORY_LEVEL but get 4k pages while iterating.

Ah, i forgot it. We can't assume that the host also support huge page for next gfn, as Marcelo's suggestion, we should "only map with level> 1 if the host page matches the size".

Um, the problem is, when we get host page size, we should hold 'mm->mmap_sem', it can't used in atomic context and it's also a slow path, we hope pte prefetch path is fast.

How about only allow prefetch for sp.leve = 1 now? i'll improve it in the future, i think it need more time :-)

Quoted 17 lines

Quoted 5 lines

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

+
+        pfn = gfn_to_pfn_atomic(vcpu->kvm, gfn);
+        if (is_error_pfn(pfn)) {

arch/x86/kvm/mmu.c static void nonpaging_new_cr3(struct kvm_vcpu *vcpu) · line 2077

Quoted 10 lines

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

+            kvm_release_pfn_clean(pfn);
+            break;
+        }
+        if (pte_prefetch_topup_memory_cache(vcpu))
+            break;
+
+        mmu_set_spte(vcpu, spte, ACC_ALL, ACC_ALL, 0, 0, 1, NULL,
+                 sp->role.level, gfn, pfn, true, false);
+    }
+}

Nice. Direct prefetch should usually succeed.

Quoted 3 lines

Can later augment to call get_users_pages_fast(..., PTE_PREFETCH_NUM, ...) to reduce gup overhead.

But we can't assume the gfn's hva is consecutive, for example, gfn and gfn+1 maybe in the different slots.

Quoted 13 lines

Quoted 13 lines

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

+
+        if (!table) {
+            page = gfn_to_page_atomic(vcpu->kvm, sp->gfn);
+            if (is_error_page(page)) {
+                kvm_release_page_clean(page);
+                break;
+            }
+            table = kmap_atomic(page, KM_USER0);
+            table = (pt_element_t *)((char *)table + offset);
+        }

Quoted 2 lines

Why not kvm_read_guest_atomic()? Can do it outside the loop.

Do you mean that read all prefetched sptes at one time?

If prefetch one spte fail, the later sptes that we read is waste, so i choose read next spte only when current spte is prefetched successful.

But i not have strong opinion on it since it's fast to read all sptes at one time, at the worst case, only 16 * 8 = 128 bytes we need to read.

Quoted 3 lines

I think lot of code can be shared with the pte prefetch in invlpg.

Yes, please allow me to cleanup those code after my future patchset:

[PATCH v4 9/9] KVM MMU: optimize sync/update unsync-page

it's the last part in the 'allow multiple shadow pages' patchset.

Quoted 15 lines · Xiao Guangrong

Quoted 5 lines

Can this in fact work for level != PT_PAGE_TABLE_LEVEL? We might start at PT_PAGE_DIRECTORY_LEVEL but get 4k pages while iterating.

Ah, i forgot it. We can't assume that the host also support huge page for next gfn, as Marcelo's suggestion, we should "only map with level> 1 if the host page matches the size".

Um, the problem is, when we get host page size, we should hold 'mm->mmap_sem',
it can't used in atomic context and it's also a slow path, we hope pte prefetch
path is fast.

Quoted 4 lines

How about only allow prefetch for sp.leve = 1 now? i'll improve it in the future,
i think it need more time :-)

I don't think prefetch for level > 1 is worthwhile. One fault per 2MB is already very good, no need to optimize it further.

Quoted 20 lines

Quoted 20 lines

Quoted 5 lines

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

+
+        pfn = gfn_to_pfn_atomic(vcpu->kvm, gfn);
+        if (is_error_pfn(pfn)) {

arch/x86/kvm/mmu.c static void nonpaging_new_cr3(struct kvm_vcpu *vcpu) · line 2077

Quoted 10 lines

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

+            kvm_release_pfn_clean(pfn);
+            break;
+        }
+        if (pte_prefetch_topup_memory_cache(vcpu))
+            break;
+
+        mmu_set_spte(vcpu, spte, ACC_ALL, ACC_ALL, 0, 0, 1, NULL,
+                 sp->role.level, gfn, pfn, true, false);
+    }
+}

Nice. Direct prefetch should usually succeed.

Can later augment to call get_users_pages_fast(..., PTE_PREFETCH_NUM, ...) to reduce gup overhead.

Quoted 4 lines

But we can't assume the gfn's hva is consecutive, for example, gfn and gfn+1
maybe in the different slots.

Right. We could limit it to one slot then for simplicity.

Quoted 14 lines

Quoted 14 lines

Quoted 14 lines

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

+
+        if (!table) {
+            page = gfn_to_page_atomic(vcpu->kvm, sp->gfn);
+            if (is_error_page(page)) {
+                kvm_release_page_clean(page);
+                break;
+            }
+            table = kmap_atomic(page, KM_USER0);
+            table = (pt_element_t *)((char *)table + offset);
+        }

Quoted 4 lines

Quoted 2 lines

Why not kvm_read_guest_atomic()? Can do it outside the loop.

Do you mean that read all prefetched sptes at one time?

Yes.

Quoted 3 lines

If prefetch one spte fail, the later sptes that we read is waste, so i choose read next spte only when current spte is prefetched successful.

Quoted 3 lines

But i not have strong opinion on it since it's fast to read all sptes at one time, at the worst case, only 16 * 8 = 128 bytes we need to read.

In general batching is worthwhile, the cost of the extra bytes is low compared to the cost of bringing in the cacheline and error checking.

btw, you could align the prefetch to 16 pte boundary. That would improve performance for memory that is scanned backwards.

So we can change the fault path to always fault 16 ptes, aligned on 16 pte boundary, with the needed pte called with specualtive=false.

Quoted 5 lines

So we can change the fault path to always fault 16 ptes, aligned on 16 pte boundary, with the needed pte called with specualtive=false.

Avi, i not understand it clearly, Could you please explain it? :-(

Quoted 5 lines · Xiao Guangrong

Quoted 5 lines

So we can change the fault path to always fault 16 ptes, aligned on 16 pte boundary, with the needed pte called with specualtive=false.

Quoted 2 lines

Avi, i not understand it clearly, Could you please explain it? :-(

Right now if the fault is in spte i, you prefetch ptes (i+1)..(i+MAX_PREFETCH-1). I'd like to prefetch ptes (i & ~(MAX_PREFETCH-1))..(i | (MAX_PREFETCH - 1)). Read all those gptes, and map them one by one with speculative = false only for spte i.

Perhaps we need to integrate it into walk_addr, there's no reason to read the gptes twice.

Support prefetch ptes when intercept guest #PF, avoid to #PF by later access

If we meet any failure in the prefetch path, we will exit it and not try other ptes to avoid become heavy path

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

---
 arch/x86/kvm/mmu.c         |   36 +++++++++++++++++++++
 arch/x86/kvm/paging_tmpl.h |   76 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 112 insertions(+), 0 deletions(-)

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

index 92ff099..941c86b 100644
@@ -89,6 +89,8 @@ module_param(oos_shadow, bool, 0644);
 	}
 #endif
 
+#define PTE_PREFETCH_NUM	16
+
 #define PT_FIRST_AVAIL_BITS_SHIFT 9
 #define PT64_SECOND_AVAIL_BITS_SHIFT 52
 

@@ -2041,6 +2043,39 @@ static void nonpaging_new_cr3(struct kvm_vcpu *vcpu)
 {
 }
 
+static void direct_pte_prefetch(struct kvm_vcpu *vcpu, u64 *sptep)
+{
+	struct kvm_mmu_page *sp;
+	int index, i;
+
+	sp = page_header(__pa(sptep));
+	WARN_ON(!sp->role.direct);
+	index = sptep - sp->spt;
+
+	for (i = index + 1; i < min(PT64_ENT_PER_PAGE,
+				      index + PTE_PREFETCH_NUM); i++) {
+		gfn_t gfn;
+		pfn_t pfn;
+		u64 *spte = sp->spt + i;
+
+		if (*spte != shadow_trap_nonpresent_pte)
+			continue;
+
+		gfn = sp->gfn + (i << ((sp->role.level - 1) * PT64_LEVEL_BITS));
+