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

In speculative path, the page is not real write-access, no need mark it dirty, so clear dirty bit in this path and later examine this bit when we release the page

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

---
 arch/x86/kvm/mmu.c |   24 +++++++++++-------------
 1 files changed, 11 insertions(+), 13 deletions(-)

Quoted 4 lines · Xiao Guangrong

In speculative path, the page is not real write-access, no need mark it dirty, so clear dirty bit in this path and later examine this bit when we release the page

Quoted 4 lines

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

---
 arch/x86/kvm/mmu.c |   24 +++++++++++-------------
 1 files changed, 11 insertions(+), 13 deletions(-)

Unfortunately all pages that kvm creates translations for are marked dirty due to get_user_pages(w=1), except KSM which makes them read-only later.

Quoted 10 lines · Marcelo Tosatti

Quoted 8 lines · Xiao Guangrong

In speculative path, the page is not real write-access, no need mark it dirty, so clear dirty bit in this path and later examine this bit when we release the page

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

---
 arch/x86/kvm/mmu.c |   24 +++++++++++-------------
 1 files changed, 11 insertions(+), 13 deletions(-)

Quoted 3 lines

Unfortunately all pages that kvm creates translations for are marked dirty due to get_user_pages(w=1), except KSM which makes them read-only later.

Marcelo, i have looked into get_user_pages() function, but not catch where to make page dirty, could you point it out for me? :-)

Quoted 11 lines · Xiao Guangrong

Quoted 9 lines · Marcelo Tosatti

Quoted 8 lines · Xiao Guangrong

In speculative path, the page is not real write-access, no need mark it dirty, so clear dirty bit in this path and later examine this bit when we release the page

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

---
 arch/x86/kvm/mmu.c |   24 +++++++++++-------------
 1 files changed, 11 insertions(+), 13 deletions(-)

Quoted 5 lines

Quoted 3 lines

Unfortunately all pages that kvm creates translations for are marked dirty due to get_user_pages(w=1), except KSM which makes them read-only later.

Marcelo, i have looked into get_user_pages() function, but not catch where to make page dirty, could you point it out for me? :-)

See set_page_dirty call in mm/memory.c::follow_page.

Quoted 18 lines · Marcelo Tosatti

Quoted 16 lines · Xiao Guangrong

Quoted 5 lines · Marcelo Tosatti

Quoted 4 lines · Xiao Guangrong

In speculative path, the page is not real write-access, no need mark it dirty, so clear dirty bit in this path and later examine this bit when we release the page

Quoted 7 lines

Quoted 4 lines

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

---
 arch/x86/kvm/mmu.c |   24 +++++++++++-------------
 1 files changed, 11 insertions(+), 13 deletions(-)

Unfortunately all pages that kvm creates translations for are marked dirty due to get_user_pages(w=1), except KSM which makes them read-only later.

Marcelo, i have looked into get_user_pages() function, but not catch where to make page dirty, could you point it out for me? :-)

Quoted 1 line

See set_page_dirty call in mm/memory.c::follow_page.

Yeah, you are right, and i want to use another way to do it since track dirty bit is too heavy, also it's dangerous if we miss to set page dirty.

How about just track access bit for speculative path, we set page both accessed and dirty(if it's writable) only if the access bit is set?

Quoted 7 lines

Yeah, you are right, and i want to use another way to do it since track dirty bit is too heavy, also it's dangerous if we miss to set page dirty.

How about just track access bit for speculative path, we set page both accessed and dirty(if it's writable) only if the access bit is set?

A useful thing to do would be to allow read-only mappings, in the fault path (Lai sent a few patches in that direction sometime ago but there was no follow up).

So in the case of a read-only fault from the guest, you'd inform get_user_pages() that read-only access is acceptable (so swapcache pages can be mapped, or qemu can mprotect(PROT_READ) guest memory).

Quoted 2 lines

How about just track access bit for speculative path, we set page both accessed and dirty(if it's writable) only if the access bit is set?

A useful thing to do would be to allow read-only mappings, in the fault path (Lai sent a few patches in that direction sometime ago but there was no follow up).

So in the case of a read-only fault from the guest, you'd inform get_user_pages() that read-only access is acceptable (so swapcache pages can be mapped, or qemu can mprotect(PROT_READ) guest memory).

Quoted 11 lines · Marcelo Tosatti

Quoted 2 lines

How about just track access bit for speculative path, we set page both accessed and dirty(if it's writable) only if the access bit is set?

A useful thing to do would be to allow read-only mappings, in the fault path (Lai sent a few patches in that direction sometime ago but there was no follow up).

So in the case of a read-only fault from the guest, you'd inform get_user_pages() that read-only access is acceptable (so swapcache pages can be mapped, or qemu can mprotect(PROT_READ) guest memory).

Yeah, it's a great work, i guess Lai will post the new version soon.

And, even we do this, i think the page dirty track is still needed, right? Then, how about my new idea to track page dirty for speculative path, just as below draft patch does:

(quoted diff) +8 -4

@@ -687,10 +687,11 @@ static void drop_spte(struct kvm *kvm, u64 *sptep, u64 new_spte)
        if (!is_rmap_spte(old_spte))
                return;
        pfn = spte_to_pfn(old_spte);
-       if (old_spte & shadow_accessed_mask)
+       if (old_spte & shadow_accessed_mask) {
                kvm_set_pfn_accessed(pfn);
-       if (is_writable_pte(old_spte))
-               kvm_set_pfn_dirty(pfn);
+               if (is_writable_pte(old_spte))
+                       kvm_set_pfn_dirty(pfn);
+       }
        rmap_remove(kvm, sptep);
 }
 

@@ -1920,8 +1921,11 @@ static int set_spte(struct kvm_vcpu *vcpu, u64 *sptep,
         * demand paging).
         */
        spte = shadow_base_present_pte | shadow_dirty_mask;
-       if (!speculative)
+       if (!speculative) {
                spte |= shadow_accessed_mask;
+               if (is_writable_pte(*sptep))
+                       kvm_set_pfn_dirty(pfn);
+       }
        if (!dirty)
                pte_access &= ~ACC_WRITE_MASK;
        if (pte_access & ACC_EXEC_MASK)

It uses access bit to track both page accessed and page dirty, and it's rather cheap...

Quoted 1 line

It uses access bit to track both page accessed and page dirty, and it's rather cheap...

Xiao,

I don't understand it. What are you trying to achieve?

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         |    8 ++++++++
 arch/x86/kvm/paging_tmpl.h |    5 +++--
 2 files changed, 11 insertions(+), 2 deletions(-)

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

index b93b94f..9fc1524 100644
@@ -2783,6 +2783,9 @@ void kvm_mmu_pte_write(struct kvm_vcpu *vcpu, gpa_t gpa,
 		break;
 	}
 
+	if (is_rsvd_bits_set(vcpu, gentry, PT_PAGE_TABLE_LEVEL))
+		gentry = 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/mmu.c void kvm_mmu_pte_write(struct kvm_vcpu *vcpu, gpa_t gpa, · line 2861

Quoted 6 lines

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

 			mmu_pte_write_zap_pte(vcpu, sp, spte);
+
+			if (!!is_pae(vcpu) != sp->role.cr4_pae ||
+			      is_nx(vcpu) != sp->role.nxe)
+				continue;
+

This breaks remote_flush assignment below.

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         |    8 ++++++++
  arch/x86/kvm/paging_tmpl.h |    5 +++--
  2 files changed, 11 insertions(+), 2 deletions(-)

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

index b93b94f..9fc1524 100644
@@ -2783,6 +2783,9 @@ void kvm_mmu_pte_write(struct kvm_vcpu *vcpu, gpa_t gpa,
  		break;
  	}
 
+	if (is_rsvd_bits_set(vcpu, gentry, PT_PAGE_TABLE_LEVEL))
+		gentry = 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/mmu.c void kvm_mmu_pte_write(struct kvm_vcpu *vcpu, gpa_t gpa, · line 2861

Quoted 6 lines

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

+
+            if (!!is_pae(vcpu) != sp->role.cr4_pae ||
+                  is_nx(vcpu) != sp->role.nxe)
+                continue;
+

Do we also need to check cr0.wp? I think so.

Quoted 4 lines

              if (gentry)
                  mmu_pte_write_new_pte(vcpu, sp, spte,&gentry);

Please move the checks to mmu_pte_write_new_pte(), it's a more logical place.

It means the reserved bits check happens multiple times, but that's ok.

Also, you can use arch.mmu.base_role to compare:

static const kvm_mmu_page_role mask = { .level = -1U, .cr4_pae = 1,
... };

    if ((sp->role.word ^ base_role.word) & mask.word)
         return;

Quoted 9 lines · Avi Kivity

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

Quoted 7 lines

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

+
+            if (!!is_pae(vcpu) != sp->role.cr4_pae ||
+                  is_nx(vcpu) != sp->role.nxe)
+                continue;
+

Do we also need to check cr0.wp? I think so.

I think it's not too bad since we just decrease the access right, for example, we mark the mapping readonly for cr0.wp=0's page, the later write-access will cause #PF, and the read-access is OK.