KVM: MMU: introduce gfn_to_pfn_atomic() function

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

Note: this speculative will mark page become dirty but it not really accessed, the same issue is in other speculative paths like invlpg, pte write, fortunately, it just affect host memory management. After Avi's patchset named "[PATCH v2 1/4] KVM: MMU: Introduce drop_spte()" merged, we will easily fix it. Will do it in the future.

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

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

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

+	if (pte_prefetch_topup_memory_cache(vcpu))
+		return;
+
+	for (j = 0; i < max; i++, j++) {
+		pt_element_t gpte;
+		unsigned pte_access;
+		u64 *spte = sp->spt + i;
+		gfn_t gfn;
+		pfn_t pfn;
+

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

Quoted 3 lines

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

+
+		if (is_rsvd_bits_set(vcpu, gpte, PT_PAGE_TABLE_LEVEL))
+			break;

BTW, doesnt sync_page also lack reserved bit checking? (unrelated to this patch).

Quoted 7 lines · Marcelo Tosatti

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

Quoted 3 lines

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

+
+		if (is_rsvd_bits_set(vcpu, gpte, PT_PAGE_TABLE_LEVEL))
+			break;

BTW, doesnt sync_page also lack reserved bit checking? (unrelated to this patch).

I think it's not since if EPT is enabled, no unsync page exist, the sync page path can't be trigged. :-)

Quoted 9 lines · Xiao Guangrong

Quoted 6 lines · Marcelo Tosatti

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

Quoted 3 lines

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

+
+		if (is_rsvd_bits_set(vcpu, gpte, PT_PAGE_TABLE_LEVEL))
+			break;

BTW, doesnt sync_page also lack reserved bit checking? (unrelated to this patch).

Quoted 2 lines

I think it's not since if EPT is enabled, no unsync page exist, the sync page path can't be trigged. :-)

What i mean is, why the sync_page path does not require reserved bit checking? (EPT disabled).

Combine guest pte read between guest pte walk and pte prefetch

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

---
 arch/x86/kvm/paging_tmpl.h |   48 ++++++++++++++++++++++++++++++-------------
 1 files changed, 33 insertions(+), 15 deletions(-)

Quoted 6 lines · Xiao Guangrong

Combine guest pte read between guest pte walk and pte prefetch

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

---
 arch/x86/kvm/paging_tmpl.h |   48 ++++++++++++++++++++++++++++++-------------
 1 files changed, 33 insertions(+), 15 deletions(-)

Can't do this, it can miss invlpg:

vcpu0 vcpu1
read guest ptes
modify guest pte
invlpg
instantiate stale
guest pte

See how the pte is reread inside fetch with mmu_lock held.

Quoted 4 lines

See how the pte is reread inside fetch with mmu_lock held.

It looks like something is broken in 'fetch' functions, this patch will fix it.

Subject: [PATCH] KVM: MMU: fix last level broken in FNAME(fetch)

We read the guest level out of 'mmu_lock', sometimes, the host mapping is confusion. Consider this case:

VCPU0: VCPU1

Read guest mapping, assume the mapping is:
GLV3 -> GLV2 -> GLV1 -> GFNA,
And in the host, the corresponding mapping is
HLV3 -> HLV2 -> HLV1(P=0)

Write GLV1 and cause the mapping point to GFNB (May occur in pte_write or
invlpg path)

Mapping GLV1 to GFNA

This issue only occurs in the last indirect mapping, since if the middle mapping is changed, the mapping will be zapped, then it will be detected in the FNAME(fetch) path, but when it map the last level, it not checked.

Fixed by also check the last level.

(quoted diff) +0 -0

  			mmu_set_spte(vcpu, sptep, access,
  				     gw->pte_access&  access,

user_fault, write_fault,

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

@@ -376,10 +394,10 @@ static u64 *FNAME(fetch)(struct kvm_vcpu *vcpu, gva_t addr,
  		sp = kvm_mmu_get_page(vcpu, table_gfn, addr, level-1,
  					       direct, access, sptep);

Quoted 32 lines · Xiao Guangrong

Quoted 4 lines

See how the pte is reread inside fetch with mmu_lock held.

It looks like something is broken in 'fetch' functions, this patch will fix it.

Subject: [PATCH] KVM: MMU: fix last level broken in FNAME(fetch)

We read the guest level out of 'mmu_lock', sometimes, the host mapping is confusion. Consider this case:

VCPU0: VCPU1

Read guest mapping, assume the mapping is:
GLV3 -> GLV2 -> GLV1 -> GFNA,
And in the host, the corresponding mapping is
HLV3 -> HLV2 -> HLV1(P=0)

Write GLV1 and
cause the
mapping point to GFNB (May occur in
pte_write or
invlpg path)

Mapping GLV1 to GFNA

This issue only occurs in the last indirect mapping, since if the middle mapping is changed, the mapping will be zapped, then it will be detected in the FNAME(fetch) path, but when it map the last level, it not checked.

Quoted 3 lines

Fixed by also check the last level.

I don't really see what is fixed. We already check the gpte. What's special about the new scenario?

Quoted 39 lines · Avi Kivity

Quoted 37 lines · Xiao Guangrong

Quoted 5 lines

See how the pte is reread inside fetch with mmu_lock held.

It looks like something is broken in 'fetch' functions, this patch will fix it.

Subject: [PATCH] KVM: MMU: fix last level broken in FNAME(fetch)

We read the guest level out of 'mmu_lock', sometimes, the host mapping is confusion. Consider this case:

VCPU0: VCPU1

Read guest mapping, assume the mapping is:
GLV3 -> GLV2 -> GLV1 -> GFNA,
And in the host, the corresponding mapping is
HLV3 -> HLV2 -> HLV1(P=0)

Write GLV1 and
cause the
mapping point to GFNB (May occur in
pte_write or
invlpg path)

Mapping GLV1 to GFNA

This issue only occurs in the last indirect mapping, since if the middle mapping is changed, the mapping will be zapped, then it will be detected in the FNAME(fetch) path, but when it map the last level, it not checked.

Fixed by also check the last level.

Quoted 4 lines

I don't really see what is fixed. We already check the gpte. What's special about the new scenario?

I mean is: while we map the last level, we will directly set to the pfn but the pfn is got by walk_addr, at this time, the guest mapping may be changed.

What is the 'We already check the gpte' mean? i think i miss something :-(

Quoted 1 line

        if (!direct) {
            r = kvm_read_guest_atomic(vcpu->kvm,
                          gw->pte_gpa[level - 2],
&curr_pte, sizeof(curr_pte));
            if (r || curr_pte != gw->ptes[level - 2]) {
                kvm_mmu_put_page(shadow_page, sptep);
                kvm_release_pfn_clean(pfn);
                sptep = NULL;
                break;
            }
        }

the code you moved... under what scenario is it not sufficient?

Quoted 14 lines · Avi Kivity

Quoted 2 lines

         if (!direct) {
             r = kvm_read_guest_atomic(vcpu->kvm,
                           gw->pte_gpa[level - 2],
&curr_pte, sizeof(curr_pte));
             if (r || curr_pte != gw->ptes[level - 2]) {
                 kvm_mmu_put_page(shadow_page, sptep);
                 kvm_release_pfn_clean(pfn);
                 sptep = NULL;
                 break;
             }
         }

Quoted 3 lines

the code you moved... under what scenario is it not sufficient?

I not move those code, just use common function instead, that it's FNAME(check_level_mapping)(), there are do the same work.

And this check is not sufficient, since it's only checked if the mapping is zapped or not exist, for other words only when broken this judgment:

	is_shadow_present_pte(*sptep)&&  !is_large_pte(*sptep)

but if the middle level is present and it's not the large mapping, this check is skipped.

Quoted 26 lines · Xiao Guangrong

Quoted 13 lines · Avi Kivity

Quoted 1 line

         if (!direct) {
             r = kvm_read_guest_atomic(vcpu->kvm,
                           gw->pte_gpa[level - 2],
&curr_pte, sizeof(curr_pte));
             if (r || curr_pte != gw->ptes[level - 2]) {
                 kvm_mmu_put_page(shadow_page, sptep);
                 kvm_release_pfn_clean(pfn);
                 sptep = NULL;
                 break;
             }
         }

Quoted 2 lines

the code you moved... under what scenario is it not sufficient?

I not move those code, just use common function instead, that it's FNAME(check_level_mapping)(), there are do the same work.

And this check is not sufficient, since it's only checked if the mapping is zapped or not exist, for other words only when broken this judgment:

    is_shadow_present_pte(*sptep)&&  !is_large_pte(*sptep)

Quoted 2 lines

but if the middle level is present and it's not the large mapping, this check is skipped.

Well, in the description, it looked like everything was using small pages (in kvm, level=1 means PTE level, we need to change this one day). Please describe again and say exactly when the guest or host uses large pages.

Quoted 8 lines

Well, in the description, it looked like everything was using small pages (in kvm, level=1 means PTE level, we need to change this one day). Please describe again and say exactly when the guest or host uses large pages.

Oh, I see what you mean.

Regarding the patch, is it possible just to move the check before, instead of adding the 'check' variable?

Quoted 10 lines · Avi Kivity

Quoted 8 lines

Well, in the description, it looked like everything was using small pages (in kvm, level=1 means PTE level, we need to change this one day). Please describe again and say exactly when the guest or host uses large pages.

Oh, I see what you mean.

Quoted 4 lines

Regarding the patch, is it possible just to move the check before, instead of adding the 'check' variable?

Umm, if we move the check before the judgment, it'll check every level, actually, only the opened mapping and the laster level need checked, so for the performance reason, maybe it's better to keep two check-point.

Quoted 4 lines

Umm, if we move the check before the judgment, it'll check every level, actually, only the opened mapping and the laster level need checked, so for the performance reason, maybe it's better to keep two check-point.

What exactly are the conditions when you want to check?

Perhaps we do need to check every level. A write to a PDE (or higher level) will clear the corresponding spte, but a fault immediately afterwards can re-establish the spte to point to the new page.

Quoted 5 lines · Avi Kivity

Quoted 5 lines

Umm, if we move the check before the judgment, it'll check every level, actually, only the opened mapping and the laster level need checked, so for the performance reason, maybe it's better to keep two check-point.

Quoted 7 lines

What exactly are the conditions when you want to check?

Perhaps we do need to check every level. A write to a PDE (or higher level) will clear the corresponding spte, but a fault immediately afterwards can re-establish the spte to point to the new page.

Looks into the code more carefully, maybe this code is wrong:

              if (!direct) {
                      r = kvm_read_guest_atomic(vcpu->kvm,

(quoted diff) +2 -2

-                                          gw->pte_gpa[level - 2],
+                                          gw->pte_gpa[level - 1],
                                       &curr_pte, sizeof(curr_pte));
-                    if (r || curr_pte != gw->ptes[level - 2]) {
+                    if (r || curr_pte != gw->ptes[level - 1]) {
                                 kvm_mmu_put_page(sp, sptep);
                                 kvm_release_pfn_clean(pfn);
                                 sptep = NULL;

It should check the 'level' mapping not 'level - 1', in the later description i'll explain it.

Since the 'walk_addr' functions is out of mmu_lock protection, we should handle the guest modify its mapping between 'walk_addr' and 'fetch'. Now, let's consider every case may be happened while handle guest #PF.

One case is host handle guest written after 'fetch', just like this order:

VCPU 0 VCPU 1
walk_addr
guest modify its mapping
fetch
host handle this written(pte_write or invlpg)

This case is not broken anything, even if 'fetch' setup the wrong mapping, the later written handler will fix it.

Another case is host handle guest written before 'fetch', like this:

CPU 0 VCPU 1
walk_addr
guest modify its mapping
host handle this written(pte_write or invlpg)
fetch

We should notice this case, since the later 'fetch' will setup the wrong mapping.

For example, the guest mapping which 'walk_addr' got is:

GPML4E -> GPDPE -> GPDE -> GPTE -> GFNA
(Just take small page for example, other mapping way is also applied)

And, for good to describe, we abstract 'fetch''s work:

for_each_shadow_entry(vcpu, addr, iterator) {
	if (iterator.level == hlevel)
		Mapping the later level

	if (is_shadow_present_pte(*sptep)&&
		!is_large_pte(*sptep))		<------ Point A
		continue;

	/* handle the non-present/wrong  middle level */

	find/create the corresponding sp<----- Point B

	if (the guest mapping is modified)<----- Point C
		break;
	setup this level's mapping<----- Point D

}

[
Note: the later level means PTE, PDE if 2M page size, PDPE if 1G page size the middle level means PDE, PDPE if not using large page size / PML4E
]

There are two cases:

1: Guest modify the middle level, for example, guest modify the GPDE.
a: the GPDE has corresponding sp entry, after VCPU1 handle this written,
the corresponding host mapping is like this:
HPML4E -> HPDPE -> HPDE
[ HPDE.P = 0 since VCPU1 written handler zapped it in pte_wirte ]

Under this case, it can broke Point A's judgment and Point C can detect the guest mapping is modified, so it exits this function without setup the mapping.

And, we should check the guest mapping at GPDE not GPTE since it's GPDE modified not GPTE, it's the explanation for the front fix.

b: the GPDE not has corresponding sp entry(the area is firstly accessed),
corresponding host mapping is like this:
HPML4E -> HPDPE
[ HPDPE.P = 0]

under this case, VCPU1 happily write GPDE without #PF since the GPDE not has shadow page, it's not write-protected.

while we handle HPDPE, we will create the shadow page for GPDE at Point B, then the host mapping is like this:
HPML4E -> HPDPE -> HPDE
[ HPDE.P = 0 ]
it's the same as 1.a, so do the same work as 1.a

Note: there is a trick: we should put 'Point C' behind of 'Point B', since after we
create sp for GPDE, the later modify GPDE will cause #PF, it can ensure later check is valid

2: Guest modify the later level.
Form 'fetch''s abstract, we can see the late level is not checked by 'Point C', if guest modified the GPTE's mapping, the wrong-mapping will be setup by 'fetch', this is just this path does

Quoted 14 lines

Looks into the code more carefully, maybe this code is wrong:

              if (!direct) {
                      r = kvm_read_guest_atomic(vcpu->kvm,

(quoted diff) +2 -2

-                                          gw->pte_gpa[level - 2],
+                                          gw->pte_gpa[level - 1],
                                       &curr_pte, sizeof(curr_pte));
-                    if (r || curr_pte != gw->ptes[level - 2]) {
+                    if (r || curr_pte != gw->ptes[level - 1]) {
                                 kvm_mmu_put_page(sp, sptep);
                                 kvm_release_pfn_clean(pfn);
                                 sptep = NULL;

Quoted 4 lines

It should check the 'level' mapping not 'level - 1', in the later description
i'll explain it.

Right, this fixes the check for the top level, but it removes a check from the bottom level.

We need to move this to the top of the loop so we check all levels. I guess this is why you needed to add a new check point. But since we loop at least glevels times, we don't need two check points.

Ok. So moving the check to before point A, and s/level - 2/level - 1/
should work, yes?

Should be slightly simpler since we don't need to kvm_mmu_put_page(sp, sptep) any more.

Thanks for the detailed explanation.

Quoted 19 lines · Avi Kivity

Quoted 18 lines

Looks into the code more carefully, maybe this code is wrong:

               if (!direct) {
                       r = kvm_read_guest_atomic(vcpu->kvm,

(quoted diff) +2 -2

-                                          gw->pte_gpa[level - 2],
+                                          gw->pte_gpa[level - 1],
                                        &curr_pte, sizeof(curr_pte));
-                    if (r || curr_pte != gw->ptes[level - 2]) {
+                    if (r || curr_pte != gw->ptes[level - 1]) {
                                  kvm_mmu_put_page(sp, sptep);
                                  kvm_release_pfn_clean(pfn);
                                  sptep = NULL;

It should check the 'level' mapping not 'level - 1', in the later description
i'll explain it.

Quoted 3 lines

Right, this fixes the check for the top level, but it removes a check from the bottom level.

We no need check the bottom level if guest not modify the bottom level, if guest modify it, the bottom level is no-present,

it also can broke
Point A's judgment and be checked by 'Point C'

Quoted 11 lines

We need to move this to the top of the loop so we check all levels. I guess this is why you needed to add a new check point. But since we loop at least glevels times, we don't need two check points.

Ok. So moving the check to before point A, and s/level - 2/level - 1/
should work, yes?

Should be slightly simpler since we don't need to kvm_mmu_put_page(sp, sptep) any more.

Yeah, it can work, but check all levels is really unnecessary, if guest not modify the level, the check can be avoid.

This is why i choose two check-point, one is behind Point A's judgment, this point checks the level which modified by guest, and another point is at mapping last level point, this check is alway need.

I'm not convinced we can bypass the checks. Consider:

VCPU0 VCPU1

#PF
walk_addr
-> gpml4e0,gpdpe0,gpde0,gpte0

replace gpdpe0 with gpdpe1
#PF
walk_addr
-> gpml4e0,gpdpe1,gpde1,gpte1

                      fetch
                      -> establish hpml4e0,hpdpte1,hpde0,hpte1
fetch
read hpdpe1
if (present(hpdpe1))
    continue;

...
write hpte0 using shadow hieratchy for hpte1

Quoted 13 lines · Avi Kivity

I'm not convinced we can bypass the checks. Consider:

VCPU0 VCPU1

#PF
walk_addr
-> gpml4e0,gpdpe0,gpde0,gpte0

replace gpdpe0 with gpdpe1
#PF
walk_addr
-> gpml4e0,gpdpe1,gpde1,gpte1

Quoted 10 lines

                       fetch
                       ->  establish hpml4e0,hpdpte1,hpde0,hpte1
fetch
read hpdpe1
if (present(hpdpe1))
     continue;

...
write hpte0 using shadow hieratchy for hpte1

Ah, i missed this case, thanks for you point it out, i'll fix it in the next version.

Note: I think we have to check _after_ kvm_mmu_get_page(), otherwise we might be checking a page that is not write-protected and can change again.

So the logic needs to be something like

for_each_shadow_entry:

    if (!last_level && !present(*spte))
        kvm_mmu_get_page
    verify gpte
    if (last_level)
        mmu_set_spte()

Quoted 8 lines

And this check is not sufficient, since it's only checked if the mapping is zapped or not exist, for other words only when broken this judgment:

    is_shadow_present_pte(*sptep)&&  !is_large_pte(*sptep)

but if the middle level is present and it's not the large mapping, this check is skipped.

Well, in the description, it looked like everything was using small pages (in kvm, level=1 means PTE level, we need to change this one day). Please describe again and say exactly when the guest or host uses large pages.

Quoted 10 lines · Avi Kivity

Quoted 9 lines

And this check is not sufficient, since it's only checked if the mapping is zapped or not exist, for other words only when broken this judgment:

     is_shadow_present_pte(*sptep)&&   !is_large_pte(*sptep)

but if the middle level is present and it's not the large mapping, this check is skipped.

Quoted 6 lines

Well, in the description, it looked like everything was using small pages (in kvm, level=1 means PTE level, we need to change this one day). Please describe again and say exactly when the guest or host uses large pages.

Avi, sorry for my poor English, i not mean "everything was using small", i don't know where cause you confused :-(

Quoted 7 lines · Xiao Guangrong

Combine guest pte read between guest pte walk and pte prefetch

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

---
  arch/x86/kvm/paging_tmpl.h |   48 ++++++++++++++++++++++++++++++-------------
  1 files changed, 33 insertions(+), 15 deletions(-)

Can't do this, it can miss invlpg:

vcpu0 vcpu1
read guest ptes
modify guest pte
invlpg
instantiate stale
guest pte

See how the pte is reread inside fetch with mmu_lock held.

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

---
 arch/x86/mm/gup.c        |    2 ++
 include/linux/kvm_host.h |    1 +
 virt/kvm/kvm_main.c      |   32 +++++++++++++++++++++++++-------
 3 files changed, 28 insertions(+), 7 deletions(-)

arch/x86/mm/gup.c +1 -0

index 738e659..0c9034b 100644
@@ -6,6 +6,7 @@
  */
 #include <linux/sched.h>
 #include <linux/mm.h>
+#include <linux/module.h>
 #include <linux/vmstat.h>
 #include <linux/highmem.h>
 

Quoted 28 lines · Xiao Guangrong

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

---
 arch/x86/mm/gup.c        |    2 ++
 include/linux/kvm_host.h |    1 +
 virt/kvm/kvm_main.c      |   32 +++++++++++++++++++++++++-------
 3 files changed, 28 insertions(+), 7 deletions(-)

arch/x86/mm/gup.c +2 -0

index 738e659..0c9034b 100644
@@ -6,6 +6,7 @@
  */
 #include <linux/sched.h>
 #include <linux/mm.h>
+#include <linux/module.h>
 #include <linux/vmstat.h>
 #include <linux/highmem.h>
 

@@ -274,6 +275,7 @@ int __get_user_pages_fast(unsigned long start, int nr_pages, int write,
 
 	return nr;
 }
+EXPORT_SYMBOL_GPL(__get_user_pages_fast);
 

arch/x86/mm/gup.c int __get_user_pages_fast(unsigned long start, int nr_pages, int write, · line 281

Quoted 2 lines

arch/x86/mm/gup.c +0 -0

 /**
  * get_user_pages_fast() - pin user pages in memory

This should be a separate patch (overlooked that before). Ingo, Nick, can this go in through the kvm tree?