Enable Sub-Page Write Protection Support

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

 #define MSR_BITMAP_MODE_X2APIC		1
 #define MSR_BITMAP_MODE_X2APIC_APICV	2

@@ -2391,6 +2393,7 @@ static __init int setup_vmcs_config(struct vmcs_config *vmcs_conf,
 			SECONDARY_EXEC_RDSEED_EXITING |
 			SECONDARY_EXEC_RDRAND_EXITING |
 			SECONDARY_EXEC_ENABLE_PML |
+			SECONDARY_EXEC_ENABLE_SPP |
 			SECONDARY_EXEC_TSC_SCALING |
 			SECONDARY_EXEC_ENABLE_USR_WAIT_PAUSE |
 			SECONDARY_EXEC_PT_USE_GPA |

@@ -4039,6 +4042,9 @@ static void vmx_compute_secondary_exec_control(struct vcpu_vmx *vmx)
 	if (!enable_pml)
 		exec_control &= ~SECONDARY_EXEC_ENABLE_PML;
 
+	if (!spp_supported)
+		exec_control &= ~SECONDARY_EXEC_ENABLE_SPP;
+
 	if (vmx_xsaves_supported()) {
 		/* Exposing XSAVES only when XSAVE is exposed */
 		bool xsaves_enabled =

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

+
 	if (!cpu_has_virtual_nmis())
 		enable_vnmi = 0;
 

Signature · 1 line

2.17.2

arch/x86/kvm/vmx/vmx.c line 63

Quoted 9 lines · Yang Weijiang

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

index e3394c839dea..5713e8a6224c 100644
@@ -60,6 +60,7 @@
 #include "vmcs12.h"
 #include "vmx.h"
 #include "x86.h"
+#include "../mmu/spp.h"

The ".." should be unnecessary, e.g. x86.h is obviously a level up.

Quoted 4 lines

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

 MODULE_AUTHOR("Qumranet");
 MODULE_LICENSE("GPL");

@@ -111,6 +112,7 @@ module_param_named(pml, enable_pml, bool, S_IRUGO);
 

arch/x86/kvm/vmx/vmx.c module_param_named(pml, enable_pml, bool, S_IRUGO); · line 115

Quoted 3 lines

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

 static bool __read_mostly dump_invalid_vmcs = 0;
 module_param(dump_invalid_vmcs, bool, 0644);
+static bool __read_mostly spp_supported = 0;

s/spp_supported/enable_spp to be consistent with all the other booleans.

Is there a reason this isn't exposed as a module param?

And if this is to be on by default, then the flag itself should be initialized to '1' so that it's clear to readers that the feature is enabled by default (if it's supported). Looking at only this code, I would think that SPP is forced off and can't be enabled.

That being said, turning on the enable_spp control flag should be the last patch in the series, i.e. it shouldn't be turned on until all the underlying support code is in place. So, I would keep this as is, but invert the code in hardware_setup() below. That way the flag exists and is checked, but can't be turned on without modifying the code. Then when all is said and done, you can add a patch to introduce the module param and turn on the flag by default (if that's indeed what we want).

Quoted 24 lines

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

 #define MSR_BITMAP_MODE_X2APIC		1
 #define MSR_BITMAP_MODE_X2APIC_APICV	2

@@ -2391,6 +2393,7 @@ static __init int setup_vmcs_config(struct vmcs_config *vmcs_conf,
 			SECONDARY_EXEC_RDSEED_EXITING |
 			SECONDARY_EXEC_RDRAND_EXITING |
 			SECONDARY_EXEC_ENABLE_PML |
+			SECONDARY_EXEC_ENABLE_SPP |
 			SECONDARY_EXEC_TSC_SCALING |
 			SECONDARY_EXEC_ENABLE_USR_WAIT_PAUSE |
 			SECONDARY_EXEC_PT_USE_GPA |

@@ -4039,6 +4042,9 @@ static void vmx_compute_secondary_exec_control(struct vcpu_vmx *vmx)
 	if (!enable_pml)
 		exec_control &= ~SECONDARY_EXEC_ENABLE_PML;
 
+	if (!spp_supported)
+		exec_control &= ~SECONDARY_EXEC_ENABLE_SPP;
+
 	if (vmx_xsaves_supported()) {
 		/* Exposing XSAVES only when XSAVE is exposed */
 		bool xsaves_enabled =

@@ -7630,6 +7636,9 @@ static __init int hardware_setup(void)
 	if (!cpu_has_vmx_flexpriority())
 		flexpriority_enabled = 0;
 

arch/x86/kvm/vmx/vmx.c static __init int hardware_setup(void) · line 7640

Quoted 2 lines

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

+	if (cpu_has_vmx_ept_spp() && enable_ept)
+		spp_supported = 1;

As above, invert this to disable spp when it's not supported, or when EPT is disabled (or not supported).

Quoted 7 lines

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

+
 	if (!cpu_has_virtual_nmis())
 		enable_vnmi = 0;
 

Signature · 1 line

2.17.2

SPPT is a 4-level paging structure similar to EPT, when SPP is

armed for target physical page, bit 61 of the corresponding EPT entry is flaged, then SPPT is traversed with the gfn, the leaf entry of SPPT contains the access bitmap of subpages inside the target 4KB physical page, one bit per 128-byte subpage.

Co-developed-by He Chen <[email protected]>

Signed-off-by He Chen <[email protected]>

Co-developed-by Zhang Yi <[email protected]>

Signed-off-by Zhang Yi <[email protected]>

Signed-off-by Yang Weijiang <[email protected]>

---

arch/x86/kvm/mmu/spp.c new file +68 -0

+
+	return new_spte;
+}
+
+static void spp_spte_set(u64 *sptep, u64 new_spte)
+{
+	__set_spte(sptep, new_spte);
+}
+
+bool is_spp_spte(struct kvm_mmu_page *sp)
+{
+	return sp->role.spp;
+}
+
+#define SPPT_ENTRY_PHA_MASK (0xFFFFFFFFFF << 12)
+
+int kvm_spp_setup_structure(struct kvm_vcpu *vcpu,
+			    u32 access_map, gfn_t gfn)
+{
+	struct kvm_shadow_walk_iterator iter;
+	struct kvm_mmu_page *sp;
+	gfn_t pseudo_gfn;
+	u64 old_spte, spp_spte;
+	int ret = -EFAULT;
+
+	/* direct_map spp start */
+	if (!VALID_PAGE(vcpu->kvm->arch.sppt_root))
+		return -EFAULT;
+
+	for_each_shadow_spp_entry(vcpu, (u64)gfn << PAGE_SHIFT, iter) {
+		if (iter.level == PT_PAGE_TABLE_LEVEL) {
+			spp_spte = format_spp_spte(access_map);
+			old_spte = mmu_spte_get_lockless(iter.sptep);
+			if (old_spte != spp_spte)
+				spp_spte_set(iter.sptep, spp_spte);
+			ret = 0;
+			break;
+		}
+
+		if (!is_shadow_present_pte(*iter.sptep)) {
+			u64 base_addr = iter.addr;
+
+			base_addr &= PT64_LVL_ADDR_MASK(iter.level);
+			pseudo_gfn = base_addr >> PAGE_SHIFT;
+			spp_spte = *iter.sptep;
+			sp = kvm_spp_get_page(vcpu, pseudo_gfn,
+					      iter.level - 1);
+			link_spp_shadow_page(vcpu, iter.sptep, sp);
+		} else if (iter.level == PT_DIRECTORY_LEVEL  &&
+			   !(spp_spte & PT_PRESENT_MASK) &&
+			   (spp_spte & SPPT_ENTRY_PHA_MASK)) {
+			spp_spte = *iter.sptep;
+			spp_spte |= PT_PRESENT_MASK;
+			spp_spte_set(iter.sptep, spp_spte);
+		}
+	}
+
+	kvm_flush_remote_tlbs(vcpu->kvm);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(kvm_spp_setup_structure);
+
+inline u64 construct_spptp(unsigned long root_hpa)
+{
+	return root_hpa & PAGE_MASK;
+}
+EXPORT_SYMBOL_GPL(construct_spptp);
+

arch/x86/kvm/mmu/spp.h new file +10 -0

new file mode 100644
index 000000000000..8ef94b7a2057
@@ -0,0 +1,10 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __KVM_X86_VMX_SPP_H
+#define __KVM_X86_VMX_SPP_H
+
+bool is_spp_spte(struct kvm_mmu_page *sp);
+u64 construct_spptp(unsigned long root_hpa);
+int kvm_spp_setup_structure(struct kvm_vcpu *vcpu,
+			    u32 access_map, gfn_t gfn);
+
+#endif /* __KVM_X86_VMX_SPP_H */

Signature · 1 line

2.17.2

Quoted 1 line · Yang Weijiang

SPPT is a 4-level paging structure similar to EPT, when SPP is

How does SPP interact with 5-level EPT?

Quoted 11 lines

armed for target physical page, bit 61 of the corresponding EPT entry is flaged, then SPPT is traversed with the gfn, the leaf entry of SPPT contains the access bitmap of subpages inside the target 4KB physical page, one bit per 128-byte subpage.

Co-developed-by He Chen <[email protected]>

Signed-off-by He Chen <[email protected]>

Co-developed-by Zhang Yi <[email protected]>

Signed-off-by Zhang Yi <[email protected]>

Signed-off-by Yang Weijiang <[email protected]>

---

...

arch/x86/kvm/mmu/spp.c line 152

Quoted 7 lines

arch/x86/kvm/mmu/spp.c new file +7 -0

+static u64 format_spp_spte(u32 spp_wp_bitmap)
+{
+	u64 new_spte = 0;
+	int i = 0;
+
+	/*
+	 * One 4K page contains 32 sub-pages, in SPP table L4E, old bits

Is this

One 4k page constains 32 sub-pages in SPP table L4E. Old bits are...

or
One 4k page contains 32 sub-pages. In SPP table L4E, old bits are...

or

	???

arch/x86/kvm/mmu/spp.c line 153

Quoted 1 line

arch/x86/kvm/mmu/spp.c new file +1 -0

+	 * are reserved, so we need to transfer u32 subpage write

Wrap comments at 80 columns to save lines.

arch/x86/kvm/mmu/spp.c line 154

Quoted 1 line

arch/x86/kvm/mmu/spp.c new file +1 -0

+	 * protect bitmap to u64 SPP L4E format.

What is a "page" in "one 4k page"? What old bits? Why not just track a 64-bit value? I understand *what* the code below does, but I have no clue why or whether it's correct.

arch/x86/kvm/mmu/spp.c line 160

Quoted 7 lines

arch/x86/kvm/mmu/spp.c new file +6 -0

+	 */
+	while (i < 32) {
+		if (spp_wp_bitmap & (1ULL << i))
+			new_spte |= 1ULL << (i * 2);
+		i++;
+	}

	for (i = 0; i < 32; i++)
		new_spte |= (spp_wp_bitmap & BIT_ULL(i)) << i;

At the very least, use a for loop.

Quoted 88 lines

arch/x86/kvm/mmu/spp.c new file +68 -0

+
+	return new_spte;
+}
+
+static void spp_spte_set(u64 *sptep, u64 new_spte)
+{
+	__set_spte(sptep, new_spte);
+}
+
+bool is_spp_spte(struct kvm_mmu_page *sp)
+{
+	return sp->role.spp;
+}
+
+#define SPPT_ENTRY_PHA_MASK (0xFFFFFFFFFF << 12)
+
+int kvm_spp_setup_structure(struct kvm_vcpu *vcpu,
+			    u32 access_map, gfn_t gfn)
+{
+	struct kvm_shadow_walk_iterator iter;
+	struct kvm_mmu_page *sp;
+	gfn_t pseudo_gfn;
+	u64 old_spte, spp_spte;
+	int ret = -EFAULT;
+
+	/* direct_map spp start */
+	if (!VALID_PAGE(vcpu->kvm->arch.sppt_root))
+		return -EFAULT;
+
+	for_each_shadow_spp_entry(vcpu, (u64)gfn << PAGE_SHIFT, iter) {
+		if (iter.level == PT_PAGE_TABLE_LEVEL) {
+			spp_spte = format_spp_spte(access_map);
+			old_spte = mmu_spte_get_lockless(iter.sptep);
+			if (old_spte != spp_spte)
+				spp_spte_set(iter.sptep, spp_spte);
+			ret = 0;
+			break;
+		}
+
+		if (!is_shadow_present_pte(*iter.sptep)) {
+			u64 base_addr = iter.addr;
+
+			base_addr &= PT64_LVL_ADDR_MASK(iter.level);
+			pseudo_gfn = base_addr >> PAGE_SHIFT;
+			spp_spte = *iter.sptep;
+			sp = kvm_spp_get_page(vcpu, pseudo_gfn,
+					      iter.level - 1);
+			link_spp_shadow_page(vcpu, iter.sptep, sp);
+		} else if (iter.level == PT_DIRECTORY_LEVEL  &&
+			   !(spp_spte & PT_PRESENT_MASK) &&
+			   (spp_spte & SPPT_ENTRY_PHA_MASK)) {
+			spp_spte = *iter.sptep;
+			spp_spte |= PT_PRESENT_MASK;
+			spp_spte_set(iter.sptep, spp_spte);
+		}
+	}
+
+	kvm_flush_remote_tlbs(vcpu->kvm);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(kvm_spp_setup_structure);
+
+inline u64 construct_spptp(unsigned long root_hpa)
+{
+	return root_hpa & PAGE_MASK;
+}
+EXPORT_SYMBOL_GPL(construct_spptp);
+

arch/x86/kvm/mmu/spp.h new file +10 -0

new file mode 100644
index 000000000000..8ef94b7a2057
@@ -0,0 +1,10 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __KVM_X86_VMX_SPP_H
+#define __KVM_X86_VMX_SPP_H
+
+bool is_spp_spte(struct kvm_mmu_page *sp);
+u64 construct_spptp(unsigned long root_hpa);
+int kvm_spp_setup_structure(struct kvm_vcpu *vcpu,
+			    u32 access_map, gfn_t gfn);
+
+#endif /* __KVM_X86_VMX_SPP_H */

Signature · 1 line

2.17.2

SPPT is a 4-level paging structure similar to EPT, when SPP is armed for target physical page, bit 61 of the corresponding EPT entry is flaged, then SPPT is traversed with the gfn, the leaf entry of SPPT contains the access bitmap of subpages inside the target 4KB physical page, one bit per 128-byte subpage.

Co-developed-by He Chen <[email protected]>

Signed-off-by He Chen <[email protected]>

Co-developed-by Zhang Yi <[email protected]>

Signed-off-by Zhang Yi <[email protected]>

Signed-off-by Yang Weijiang <[email protected]>

---
 arch/x86/include/asm/kvm_host.h |   5 +-
 arch/x86/kvm/mmu/spp.c          | 228 ++++++++++++++++++++++++++++++++
 arch/x86/kvm/mmu/spp.h          |  10 ++

Quoted 11 lines · Yang Weijiang

SPPT is a 4-level paging structure similar to EPT, when SPP is armed for target physical page, bit 61 of the corresponding EPT entry is flaged, then SPPT is traversed with the gfn, the leaf entry of SPPT contains the access bitmap of subpages inside the target 4KB physical page, one bit per 128-byte subpage.

Co-developed-by He Chen <[email protected]>

Signed-off-by He Chen <[email protected]>

Co-developed-by Zhang Yi <[email protected]>

Signed-off-by Zhang Yi <[email protected]>

Signed-off-by Yang Weijiang <[email protected]>

Quoted 4 lines

---
 arch/x86/include/asm/kvm_host.h |   5 +-
 arch/x86/kvm/mmu/spp.c          | 228 ++++++++++++++++++++++++++++++++
 arch/x86/kvm/mmu/spp.h          |  10 ++

This patch is completely untesteable. It adds spp.c but doesn't compile it. Actually, everything up until patch 06 is untestable.

Create access bitmap for SPP subpages, the bitmap can be accessed with a gfn. The initial access bitmap for each physical page is 0xFFFFFFFF, meaning SPP is not enabled for the subpages.

Co-developed-by He Chen <[email protected]>

Signed-off-by He Chen <[email protected]>

Co-developed-by Zhang Yi <[email protected]>

Signed-off-by Zhang Yi <[email protected]>

Signed-off-by Yang Weijiang <[email protected]>

---
 arch/x86/include/asm/kvm_host.h |   2 +
 arch/x86/kvm/mmu/spp.c          | 332 ++++++++++++++++++++++++++++++++
 arch/x86/kvm/mmu/spp.h          |  12 ++
 include/uapi/linux/kvm.h        |   8 +
 4 files changed, 354 insertions(+)

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

index 9506c9d40895..f5145b86d620 100644
@@ -812,6 +812,7 @@ struct kvm_lpage_info {
 
 struct kvm_arch_memory_slot {
 	struct kvm_rmap_head *rmap[KVM_NR_PAGE_SIZES];
+	u32 *subpage_wp_info;
 	struct kvm_lpage_info *lpage_info[KVM_NR_PAGE_SIZES - 1];
 	unsigned short *gfn_track[KVM_PAGE_TRACK_MAX];
 };

@@ -959,6 +960,7 @@ struct kvm_arch {
 	struct task_struct *nx_lpage_recovery_thread;
 
 	hpa_t sppt_root;
+	bool spp_active;
 };
 
 struct kvm_vm_stat {

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

index 5fca08af705c..edab5ec83ef3 100644
@@ -17,6 +17,21 @@ static void shadow_spp_walk_init(struct kvm_shadow_walk_iterator *iterator,
 	iterator->level = PT64_ROOT_4LEVEL;
 }
 
+u32 *gfn_to_subpage_wp_info(struct kvm_memory_slot *slot, gfn_t gfn)
+{
+	unsigned long idx;
+
+	if (!slot->arch.subpage_wp_info)
+		return NULL;
+
+	idx = gfn_to_index(gfn, slot->base_gfn, PT_PAGE_TABLE_LEVEL);
+	if (idx > slot->npages - 1)
+		return NULL;
+
+	return &slot->arch.subpage_wp_info[idx];
+}
+EXPORT_SYMBOL_GPL(gfn_to_subpage_wp_info);
+
 static bool __rmap_open_subpage_bit(struct kvm *kvm,
 				    struct kvm_rmap_head *rmap_head)
 {

@@ -172,6 +187,20 @@ bool is_spp_spte(struct kvm_mmu_page *sp)
 	return sp->role.spp;
 }
 
+static int kvm_spp_level_pages(gfn_t gfn_lower, gfn_t gfn_upper, int level)
+{
+	int page_num = KVM_PAGES_PER_HPAGE(level);
+	gfn_t gfn_max = (gfn_lower & ~(page_num - 1)) + page_num - 1;
+	int ret;

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

+	u64 spde;
+	int count;
+	bool flush = false;
+
+	vcpu = kvm_get_vcpu(kvm, 0);
+	if (!VALID_PAGE(vcpu->kvm->arch.sppt_root))
+		return -EFAULT;
+
+	for (; gfn <= gfn_max; gfn++) {
+		for_each_shadow_spp_entry(vcpu, (u64)gfn << PAGE_SHIFT, iter) {
+			if (!is_shadow_present_pte(*iter.sptep))
+				break;
+
+			if (iter.level == PT_DIRECTORY_LEVEL) {
+				spde = *iter.sptep;
+				spde &= ~PT_PRESENT_MASK;
+				spp_spte_set(iter.sptep, spde);
+				count = kvm_spp_level_pages(gfn,
+							    gfn_max,
+							    PT_DIRECTORY_LEVEL);
+				flush = true;
+				if (count >= npages)
+					goto out;
+				gfn += count;
+				break;
+			}
+		}
+	}
+out:
+	if (flush)
+		kvm_flush_remote_tlbs(kvm);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(vmx_spp_flush_sppt);
+
+static int kvm_spp_create_bitmaps(struct kvm *kvm)
+{
+	struct kvm_memslots *slots;
+	struct kvm_memory_slot *memslot;
+	int i, j, ret;
+	u32 *buff;
+
+	for (i = 0; i < KVM_ADDRESS_SPACE_NUM; i++) {
+		slots = __kvm_memslots(kvm, i);
+		kvm_for_each_memslot(memslot, slots) {
+			buff = kvzalloc(memslot->npages *
+				sizeof(*memslot->arch.subpage_wp_info),
+				GFP_KERNEL);
+
+			if (!buff) {
+				ret = -ENOMEM;
+				goto out_free;
+			}
+			memslot->arch.subpage_wp_info = buff;
+
+			for (j = 0; j < memslot->npages; j++)
+				buff[j] = FULL_SPP_ACCESS;
+		}
+	}
+
+	return 0;
+out_free:
+	for (i = 0; i < KVM_ADDRESS_SPACE_NUM; i++) {
+		slots = __kvm_memslots(kvm, i);
+		kvm_for_each_memslot(memslot, slots) {
+			if (memslot->arch.subpage_wp_info) {
+				kvfree(memslot->arch.subpage_wp_info);
+				memslot->arch.subpage_wp_info = NULL;
+			}
+		}
+	}
+
+	return ret;
+}
+
+int vmx_spp_init(struct kvm *kvm)
+{
+	int i, ret;
+	struct kvm_vcpu *vcpu;
+	int root_level;
+	struct kvm_mmu_page *ssp_sp;
+	bool first_root = true;
+
+	/* SPP feature is exclusive with nested VM.*/
+	if (kvm_x86_ops->get_nested_state)
+		return -EPERM;
+
+	if (kvm->arch.spp_active)
+		return 0;
+
+	ret = kvm_spp_create_bitmaps(kvm);
+
+	if (ret)
+		return ret;
+
+	kvm_for_each_vcpu(i, vcpu, kvm) {
+		if (first_root) {
+			/* prepare caches for SPP setup.*/
+			mmu_topup_memory_caches(vcpu);
+			root_level = vcpu->arch.mmu->shadow_root_level;
+			ssp_sp = kvm_spp_get_page(vcpu, 0, root_level);
+			first_root = false;
+			vcpu->kvm->arch.sppt_root = __pa(ssp_sp->spt);
+		}
+		++ssp_sp->root_count;
+		kvm_make_request(KVM_REQ_LOAD_CR3, vcpu);
+	}
+
+	kvm->arch.spp_active = true;
+	return 0;
+}
+EXPORT_SYMBOL_GPL(vmx_spp_init);
+
+int kvm_spp_get_permission(struct kvm *kvm, u64 gfn, u32 npages,
+			   u32 *access_map)
+{
+	u32 *access;
+	struct kvm_memory_slot *slot;
+	int i;
+
+	if (!kvm->arch.spp_active)
+		return -ENODEV;
+
+	for (i = 0; i < npages; i++, gfn++) {
+		slot = gfn_to_memslot(kvm, gfn);
+		if (!slot)
+			return -EFAULT;
+		access = gfn_to_subpage_wp_info(slot, gfn);
+		if (!access)
+			return -EFAULT;
+		access_map[i] = *access;
+	}
+
+	return i;
+}
+EXPORT_SYMBOL_GPL(kvm_spp_get_permission);
+
+static void kvm_spp_zap_pte(struct kvm *kvm, u64 *spte, int level)
+{
+	u64 pte;
+
+	pte = *spte;
+	if (is_shadow_present_pte(pte) && is_last_spte(pte, level)) {
+		drop_spte(kvm, spte);
+		if (is_large_pte(pte))
+			--kvm->stat.lpages;
+	}
+}
+
+static bool kvm_spp_flush_rmap(struct kvm *kvm, u64 gfn_min, u64 gfn_max)
+{
+	u64 *sptep;
+	struct rmap_iterator iter;
+	struct kvm_rmap_head *rmap_head;
+	int level;
+	struct kvm_memory_slot *slot;
+	bool flush = false;
+
+	slot = gfn_to_memslot(kvm, gfn_min);
+	if (!slot)
+		return false;
+
+	for (; gfn_min <= gfn_max; gfn_min++) {
+		for (level = PT_PAGE_TABLE_LEVEL;
+		     level <= PT_DIRECTORY_LEVEL; level++) {
+			rmap_head = __gfn_to_rmap(gfn_min, level, slot);
+			for_each_rmap_spte(rmap_head, &iter, sptep) {
+				pte_list_remove(rmap_head, sptep);
+				flush = true;
+			}
+		}
+	}
+
+	return flush;
+}
+
+int kvm_spp_set_permission(struct kvm *kvm, u64 gfn, u32 npages,
+			   u32 *access_map)
+{
+	gfn_t old_gfn = gfn;
+	u32 *access;
+	struct kvm_memory_slot *slot;
+	struct kvm_shadow_walk_iterator iterator;
+	struct kvm_vcpu *vcpu;
+	gfn_t gfn_max;
+	int i, count, level;
+	bool flush = false;
+
+	if (!kvm->arch.spp_active)
+		return -ENODEV;
+
+	vcpu = kvm_get_vcpu(kvm, 0);
+	if (!VALID_PAGE(vcpu->kvm->arch.sppt_root))
+		return -EFAULT;
+
+	for (i = 0; i < npages; i++, gfn++) {
+		slot = gfn_to_memslot(kvm, gfn);
+		if (!slot)
+			return -EFAULT;
+
+		access = gfn_to_subpage_wp_info(slot, gfn);
+		if (!access)
+			return -EFAULT;
+		*access = access_map[i];
+	}
+
+	gfn = old_gfn;
+	gfn_max = gfn + npages - 1;
+	vcpu = kvm_get_vcpu(kvm, 0);
+
+	if (!vcpu || (vcpu && !VALID_PAGE(vcpu->arch.mmu->root_hpa)))
+		goto out;
+
+	flush = kvm_spp_flush_rmap(kvm, gfn, gfn_max);
+
+	for (i = 0; gfn <= gfn_max; i++, gfn++) {
+		for_each_shadow_entry(vcpu, (u64)gfn << PAGE_SHIFT, iterator) {
+			if (!is_shadow_present_pte(*iterator.sptep))
+				break;
+
+			if (iterator.level == PT_PAGE_TABLE_LEVEL) {
+				if (kvm_spp_mark_protection(kvm,
+							    gfn,
+							    access_map[i]) < 0)
+					return -EFAULT;
+				break;
+			} else if (is_large_pte(*iterator.sptep)) {
+				level = iterator.level;
+				if (access_map[i] == FULL_SPP_ACCESS)
+					break;
+				count = kvm_spp_level_pages(gfn,
+							    gfn_max,
+							    level);
+				kvm_spp_zap_pte(kvm, iterator.sptep, level);
+				flush = true;
+				if (count >= npages)
+					goto out;
+				gfn += count - 1;
+			}
+		}
+	}
+out:
+	if (flush)
+		kvm_flush_remote_tlbs(kvm);
+	return npages;
+}
+
+int kvm_spp_mark_protection(struct kvm *kvm, u64 gfn, u32 access)
+{
+	struct kvm_memory_slot *slot;
+	struct kvm_rmap_head *rmap_head;
+	int ret;
+
+	if (!kvm->arch.spp_active)
+		return -ENODEV;
+
+	slot = gfn_to_memslot(kvm, gfn);
+	if (!slot)
+		return -EFAULT;
+
+	/*
+	 * check whether the target 4KB page exists in EPT leaf
+	 * entry.If it's there, just flag SPP bit of the entry,
+	 * defer the setup to SPPT miss induced vm-exit  handler.
+	 */
+	rmap_head = __gfn_to_rmap(gfn, PT_PAGE_TABLE_LEVEL, slot);
+
+	if (rmap_head->val) {
+		/*
+		 * if all subpages are not writable, open SPP bit in
+		 * EPT leaf entry to enable SPP protection for
+		 * corresponding page.
+		 */
+		if (access != FULL_SPP_ACCESS) {
+			ret = kvm_spp_open_write_protect(kvm, slot, gfn);
+			if (ret)
+				return ret;
+		} else {
+			ret = kvm_spp_clear_write_protect(kvm, slot, gfn);
+			if (ret)
+				return ret;
+		}
+	}
+
+	return 0;
+}
+
+void kvm_spp_free_memslot(struct kvm_memory_slot *free,
+			  struct kvm_memory_slot *dont)
+{
+	if (!dont || free->arch.subpage_wp_info !=
+	    dont->arch.subpage_wp_info) {
+		kvfree(free->arch.subpage_wp_info);
+		free->arch.subpage_wp_info = NULL;
+	}
+}
+
 inline u64 construct_spptp(unsigned long root_hpa)
 {
 	return root_hpa & PAGE_MASK;

arch/x86/kvm/mmu/spp.h +12 -0

index 8ef94b7a2057..99d93fdc74b5 100644
@@ -2,9 +2,21 @@
 #ifndef __KVM_X86_VMX_SPP_H
 #define __KVM_X86_VMX_SPP_H
 
+#define FULL_SPP_ACCESS		((u32)((1ULL << 32) - 1))
+
+int kvm_spp_get_permission(struct kvm *kvm, u64 gfn, u32 npages,
+			   u32 *access_map);
+int kvm_spp_set_permission(struct kvm *kvm, u64 gfn, u32 npages,
+			   u32 *access_map);
+int kvm_spp_mark_protection(struct kvm *kvm, u64 gfn, u32 access);
 bool is_spp_spte(struct kvm_mmu_page *sp);
 u64 construct_spptp(unsigned long root_hpa);
 int kvm_spp_setup_structure(struct kvm_vcpu *vcpu,
 			    u32 access_map, gfn_t gfn);
+int vmx_spp_flush_sppt(struct kvm *kvm, u64 gfn_base, u32 npages);
+void kvm_spp_free_memslot(struct kvm_memory_slot *free,
+			  struct kvm_memory_slot *dont);
+int vmx_spp_init(struct kvm *kvm);
+u32 *gfn_to_subpage_wp_info(struct kvm_memory_slot *slot, gfn_t gfn);
 
 #endif /* __KVM_X86_VMX_SPP_H */

include/uapi/linux/kvm.h +8 -0

index f0a16b4adbbd..eabd55ec5af7 100644
@@ -102,6 +102,14 @@ struct kvm_userspace_memory_region {
 	__u64 userspace_addr; /* start of the userspace allocated memory */
 };
 
+/* for KVM_SUBPAGES_GET_ACCESS and KVM_SUBPAGES_SET_ACCESS */
+struct kvm_subpage {
+	__u64 gfn_base; /* the first page gfn of the contiguous pages */
+	__u32 npages;   /* number of 4K pages */
+	__u32 flags;    /* reserved to 0 now */
+	__u32 access_map[0]; /* start place of bitmap array */
+};
+
 /*
  * The bit 0 ~ bit 15 of kvm_memory_region::flags are visible for userspace,
  * other bits are reserved for kvm internal use which are defined in

Signature · 1 line

2.17.2

Quoted 4 lines · Yang Weijiang

Create access bitmap for SPP subpages, the bitmap can be accessed with a gfn. The initial access bitmap for each physical page is 0xFFFFFFFF, meaning SPP is not enabled for the subpages.

Wrap changelogs at ~75 chars.

Create access bitmap for SPP subpages, the bitmap can be accessed with a gfn. The initial access bitmap for each physical page is 0xFFFFFFFF, meaning SPP is not enabled for the subpages.

There needs to be a *lot* more information provided in all of the changelogs for this series. I understand the basic concepts of SPP, but nothing in the documentation or changelogs explains how KVM generates the SPP tables based on userspace input. Essentially, explain the design in decent detail, with a focus on *why* KVM does what it does.

Quoted 9 lines

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

+static int kvm_spp_level_pages(gfn_t gfn_lower, gfn_t gfn_upper, int level)
+{
+	int page_num = KVM_PAGES_PER_HPAGE(level);
+	gfn_t gfn_max = (gfn_lower & ~(page_num - 1)) + page_num - 1;
+	int ret;
+
+	if (gfn_upper <= gfn_max)
+		ret = gfn_upper - gfn_lower + 1;
+	else

arch/x86/kvm/mmu/spp.c bool is_spp_spte(struct kvm_mmu_page *sp) · line 204

Quoted 6 lines

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

+		ret = gfn_max - gfn_lower + 1;
+
+	return ret;
+}
+
 #define SPPT_ENTRY_PHA_MASK (0xFFFFFFFFFF << 12)

There's almost certainly an existing macro for this.

Quoted 6 lines

int kvm_spp_setup_structure(struct kvm_vcpu *vcpu,

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

@@ -220,6 +249,309 @@ int kvm_spp_setup_structure(struct kvm_vcpu *vcpu,
 }
 EXPORT_SYMBOL_GPL(kvm_spp_setup_structure);
 

arch/x86/kvm/mmu/spp.c int kvm_spp_setup_structure(struct kvm_vcpu *vcpu, · line 257

Quoted 6 lines

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

+int vmx_spp_flush_sppt(struct kvm *kvm, u64 gfn_base, u32 npages)
+{
+	struct kvm_shadow_walk_iterator iter;
+	struct kvm_vcpu *vcpu;
+	gfn_t gfn = gfn_base;
+	gfn_t gfn_max = gfn_base + npages - 1;

s/gfn_max/gfn_end. "max" makes me think this is literally walking every possible gfn.

Quoted 10 lines

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

+	u64 spde;
+	int count;
+	bool flush = false;
+
 /*
  * The bit 0 ~ bit 15 of kvm_memory_region::flags are visible for userspace,
  * other bits are reserved for kvm internal use which are defined in

Signature · 1 line

2.17.2

User application, e.g., QEMU or VMI, must initialize SPP before gets/sets SPP subpages, the dynamic initialization is to reduce the extra storage cost if the SPP feature is not not used.

Co-developed-by He Chen <[email protected]>

Signed-off-by He Chen <[email protected]>

Co-developed-by Zhang Yi <[email protected]>

Signed-off-by Zhang Yi <[email protected]>

Signed-off-by Yang Weijiang <[email protected]>

---
 arch/x86/include/asm/kvm_host.h |  4 ++
 arch/x86/kvm/mmu/spp.c          | 44 +++++++++++++++
 arch/x86/kvm/mmu/spp.h          |  9 ++++
 arch/x86/kvm/vmx/vmx.c          | 15 ++++++
 arch/x86/kvm/x86.c              | 95 ++++++++++++++++++++++++++++++++-
 include/uapi/linux/kvm.h        |  3 ++
 6 files changed, 169 insertions(+), 1 deletion(-)

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

index f5145b86d620..c7a9f03f39a7 100644

Quoted 24 lines · Yang Weijiang

User application, e.g., QEMU or VMI, must initialize SPP before gets/sets SPP subpages, the dynamic initialization is to reduce the extra storage cost if the SPP feature is not not used.

Co-developed-by He Chen <[email protected]>

Signed-off-by He Chen <[email protected]>

Co-developed-by Zhang Yi <[email protected]>

Signed-off-by Zhang Yi <[email protected]>

Signed-off-by Yang Weijiang <[email protected]>

---
 arch/x86/include/asm/kvm_host.h |  4 ++
 arch/x86/kvm/mmu/spp.c          | 44 +++++++++++++++
 arch/x86/kvm/mmu/spp.h          |  9 ++++
 arch/x86/kvm/vmx/vmx.c          | 15 ++++++
 arch/x86/kvm/x86.c              | 95 ++++++++++++++++++++++++++++++++-
 include/uapi/linux/kvm.h        |  3 ++
 6 files changed, 169 insertions(+), 1 deletion(-)

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

index f5145b86d620..c7a9f03f39a7 100644
@@ -1238,6 +1238,10 @@ struct kvm_x86_ops {
 

arch/x86/include/asm/kvm_host.h struct kvm_x86_ops { · line 1244

Quoted 6 lines

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

 	bool (*apic_init_signal_blocked)(struct kvm_vcpu *vcpu);
 	int (*enable_direct_tlbflush)(struct kvm_vcpu *vcpu);
+
+	int (*init_spp)(struct kvm *kvm);
+	int (*flush_subpages)(struct kvm *kvm, u64 gfn, u32 npages);
+	int (*get_inst_len)(struct kvm_vcpu *vcpu);

If this is necessary, which hopefully it isn't, then get_insn_len() to be consistent with other KVM nomenclature.

A comment for the series overall, it needs a lot of work to properly order code between patches. E.g. this patch introduces get_inst_len() without any justification in the changelog and without a user. At best it's confusing, at worst this series will be impossible to bisect.

If write to subpage is not allowed, EPT violation generates and it's handled in fast_page_fault().

In current implementation, SPPT setup is only handled in handle_spp() vmexit handler, it's triggered when SPP bit is set in EPT leaf entry while SPPT entries are not ready.

A SPP specific bit(11) is added to exit_qualification and a new exit reason(66) is introduced for SPP.

Quoted 9 lines · Yang Weijiang

If write to subpage is not allowed, EPT violation generates and it's handled in fast_page_fault().

In current implementation, SPPT setup is only handled in handle_spp() vmexit handler, it's triggered when SPP bit is set in EPT leaf entry while SPPT entries are not ready.

A SPP specific bit(11) is added to exit_qualification and a new exit reason(66) is introduced for SPP.

...

Quoted 8 lines

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

index 6f92b40d798c..c41791ebee65 100644
@@ -6372,6 +6427,8 @@ unsigned long kvm_mmu_calculate_default_mmu_pages(struct kvm *kvm)
 	return nr_mmu_pages;
 }
 

arch/x86/kvm/mmu/mmu.c unsigned long kvm_mmu_calculate_default_mmu_pages(struct kvm *kvm) · line 6431

Quoted 2 lines

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

+#include "spp.c"
+

Unless there is a *very* good reason for these shenanigans, spp.c needs to built via the Makefile like any other source. If this is justified for whatever reason, then that justification needs to be very clearly stated in the changelog.

In general, the code organization of this entire series likely needs to be overhauled. There are gobs exports which are either completely unnecessary or completely backswards.

E.g. exporting VMX-only functions from spp.c, which presumably are only callbed by VMX.

	EXPORT_SYMBOL_GPL(vmx_spp_flush_sppt);
	EXPORT_SYMBOL_GPL(vmx_spp_init);

Exporting ioctl helpers from the same file, which are presumably called only from x86.c.

	EXPORT_SYMBOL_GPL(kvm_vm_ioctl_get_subpages);
	EXPORT_SYMBOL_GPL(kvm_vm_ioctl_set_subpages);

Unless there is a *very* good reason for these shenanigans, spp.c needs to built via the Makefile like any other source. If this is justified for whatever reason, then that justification needs to be very clearly stated in the changelog.

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

+				trace_kvm_spp_induced_page_fault(vcpu,
+								 gva,
+								 len);
+				break;
+			}
+
+			if (was_spp_armed(new_spte)) {
+				restore_spp_bit(&new_spte);
+				spp_protected = true;
+			} else {
+				new_spte |= PT_WRITABLE_MASK;
+			}
 
 			/*
 			 * Do not fix write-permission on the large spte.  Since

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

 #define L1D_CACHE_ORDER 4
 static void *vmx_l1d_flush_pages;
 

@@ -2999,6 +2998,7 @@ void vmx_set_cr3(struct kvm_vcpu *vcpu, unsigned long cr3)
 	bool update_guest_cr3 = true;
 	unsigned long guest_cr3;
 	u64 eptp;
+	u64 spptp;
 
 	guest_cr3 = cr3;
 	if (enable_ept) {

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

+{
+	unsigned long exit_qualification;
+	struct kvm_memory_slot *slot;
+	gpa_t gpa;
+	gfn_t gfn;
+
+	exit_qualification = vmcs_readl(EXIT_QUALIFICATION);
+
+	/*
+	 * SPP VM exit happened while executing iret from NMI,

arch/x86/kvm/vmx/vmx.c +24 -0

+			slot = gfn_to_memslot(vcpu->kvm, gfn);
+			if (!slot)
+				continue;
+			access = gfn_to_subpage_wp_info(slot, gfn);
+			if (access && *access != FULL_SPP_ACCESS)
+				kvm_spp_setup_structure(vcpu,
+							*access,
+							gfn);
+		}
+		spin_unlock(&vcpu->kvm->mmu_lock);
+		return 1;
+	}
+	/*
+	 * SPPT Misconfig
+	 * This is probably caused by some mis-configuration in SPPT
+	 * entries, cannot handle it here, escalate the fault to
+	 * emulator.
+	 */
+	WARN_ON(1);
+	vcpu->run->exit_reason = KVM_EXIT_UNKNOWN;
+	vcpu->run->hw.hardware_exit_reason = EXIT_REASON_SPP;
+	return 0;
+}
+

static int handle_monitor(struct kvm_vcpu *vcpu)
{

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

 	printk_once(KERN_WARNING "kvm: MONITOR instruction emulated as NOP!\n");

@@ -5575,6 +5649,7 @@ static int (*kvm_vmx_exit_handlers[])(struct kvm_vcpu *vcpu) = {
 	[EXIT_REASON_INVVPID]                 = handle_vmx_instruction,
 	[EXIT_REASON_RDRAND]                  = handle_invalid_op,
 	[EXIT_REASON_RDSEED]                  = handle_invalid_op,
+	[EXIT_REASON_SPP]                     = handle_spp,
 	[EXIT_REASON_PML_FULL]		      = handle_pml_full,
 	[EXIT_REASON_INVPCID]                 = handle_invpcid,
 	[EXIT_REASON_VMFUNC]		      = handle_vmx_instruction,

@@ -5807,6 +5882,9 @@ void dump_vmcs(void)
 		pr_err("PostedIntrVec = 0x%02x\n", vmcs_read16(POSTED_INTR_NV));
 	if ((secondary_exec_control & SECONDARY_EXEC_ENABLE_EPT))
 		pr_err("EPT pointer = 0x%016llx\n", vmcs_read64(EPT_POINTER));
+	if ((secondary_exec_control & SECONDARY_EXEC_ENABLE_SPP))
+		pr_err("SPPT pointer = 0x%016llx\n", vmcs_read64(SPPT_POINTER));
+
 	n = vmcs_read32(CR3_TARGET_COUNT);
 	for (i = 0; i + 1 < n; i += 4)
 		pr_err("CR3 target%u=%016lx target%u=%016lx\n",

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

index fb7da000ceaf..a9d7fc21dad6 100644
@@ -9782,6 +9782,7 @@ void kvm_arch_free_memslot(struct kvm *kvm, struct kvm_memory_slot *free,
 	}
 
 	kvm_page_track_free_memslot(free, dont);
+	kvm_spp_free_memslot(free, dont);
 }
 
 int kvm_arch_create_memslot(struct kvm *kvm, struct kvm_memory_slot *slot,

@@ -10406,3 +10407,6 @@ EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_pml_full);
 EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_pi_irte_update);
 EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_avic_unaccelerated_access);
 EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_avic_incomplete_ipi);
+EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_spp_set_subpages);
+EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_spp_induced_exit);
+EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_spp_induced_page_fault);

include/uapi/linux/kvm.h +6 -0

index 09e5e8e6e6dd..c0f3162ee46a 100644
@@ -244,6 +244,7 @@ struct kvm_hyperv_exit {
 #define KVM_EXIT_IOAPIC_EOI       26
 #define KVM_EXIT_HYPERV           27
 #define KVM_EXIT_ARM_NISV         28
+#define KVM_EXIT_SPP              29
 
 /* For KVM_EXIT_INTERNAL_ERROR */
 /* Emulate instruction failed. */

@@ -401,6 +402,11 @@ struct kvm_run {
 		struct {
 			__u8 vector;
 		} eoi;
+		/* KVM_EXIT_SPP */
+		struct {
+			__u64 addr;
+			__u8 ins_len;
+		} spp;
 		/* KVM_EXIT_HYPERV */
 		struct kvm_hyperv_exit hyperv;
 		/* KVM_EXIT_ARM_NISV */

Signature · 1 line

2.17.2

arch/x86/kvm/mmu/mmu.c static bool fast_page_fault(struct kvm_vcpu *vcpu, gva_t gva, int level, · line 3611

Quoted 12 lines · Yang Weijiang

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

@@ -3585,7 +3602,30 @@ static bool fast_page_fault(struct kvm_vcpu *vcpu, gva_t gva, int level,
 		if ((error_code & PFERR_WRITE_MASK) &&
 		    spte_can_locklessly_be_made_writable(spte))
 		{
-			new_spte |= PT_WRITABLE_MASK;
+			/*
+			 * Record write protect fault caused by
+			 * Sub-page Protection, let VMI decide
+			 * the next step.
+			 */
+			if (spte & PT_SPP_MASK) {
+				int len = kvm_x86_ops->get_inst_len(vcpu);

There's got to be a better way to handle SPP exits than adding a helper to retrieve the instruction length.

arch/x86/kvm/mmu/mmu.c static bool fast_page_fault(struct kvm_vcpu *vcpu, gva_t gva, int level, · line 3616

Quoted 5 lines

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

+
+				fault_handled = true;
+				vcpu->run->exit_reason = KVM_EXIT_SPP;
+				vcpu->run->spp.addr = gva;
+				vcpu->run->spp.ins_len = len;

s/ins_len/insn_len to be consistent with other KVM nomenclature.

Quoted 15 lines

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

+				trace_kvm_spp_induced_page_fault(vcpu,
+								 gva,
+								 len);
+				break;
+			}
+
+			if (was_spp_armed(new_spte)) {
+				restore_spp_bit(&new_spte);
+				spp_protected = true;
+			} else {
+				new_spte |= PT_WRITABLE_MASK;
+			}
 
 			/*
 			 * Do not fix write-permission on the large spte.  Since

...

arch/x86/kvm/vmx/vmx.c static const struct { · line 203

Quoted 9 lines

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

index 24e4e1c47f42..97d862c79124 100644
@@ -200,7 +200,6 @@ static const struct {
 	[VMENTER_L1D_FLUSH_EPT_DISABLED] = {"EPT disabled", false},
 	[VMENTER_L1D_FLUSH_NOT_REQUIRED] = {"not required", false},
 };
-

Spurious whitepsace.

Quoted 13 lines

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

 #define L1D_CACHE_ORDER 4
 static void *vmx_l1d_flush_pages;
 

@@ -2999,6 +2998,7 @@ void vmx_set_cr3(struct kvm_vcpu *vcpu, unsigned long cr3)
 	bool update_guest_cr3 = true;
 	unsigned long guest_cr3;
 	u64 eptp;
+	u64 spptp;
 
 	guest_cr3 = cr3;
 	if (enable_ept) {

@@ -3027,6 +3027,12 @@ void vmx_set_cr3(struct kvm_vcpu *vcpu, unsigned long cr3)
 

arch/x86/kvm/vmx/vmx.c void vmx_set_cr3(struct kvm_vcpu *vcpu, unsigned long cr3) · line 3034

Quoted 7 lines

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

 	if (update_guest_cr3)
 		vmcs_writel(GUEST_CR3, guest_cr3);
+
+	if (kvm->arch.spp_active && VALID_PAGE(vcpu->kvm->arch.sppt_root)) {
+		spptp = construct_spptp(vcpu->kvm->arch.sppt_root);
+		vmcs_write64(SPPT_POINTER, spptp);
+		vmx_flush_tlb(vcpu, true);

Why is SPP so special that it gets to force TLB flushes?

arch/x86/kvm/vmx/vmx.c static int handle_monitor_trap(struct kvm_vcpu *vcpu) · line 5370

Quoted 9 lines

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

+	}
 }
 
 int vmx_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4)

@@ -5361,6 +5367,74 @@ static int handle_monitor_trap(struct kvm_vcpu *vcpu)
 	return 1;
 }
 
+int handle_spp(struct kvm_vcpu *vcpu)

Can be static.

Quoted 10 lines

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

+{
+	unsigned long exit_qualification;
+	struct kvm_memory_slot *slot;
+	gpa_t gpa;
+	gfn_t gfn;
+
+	exit_qualification = vmcs_readl(EXIT_QUALIFICATION);
+
+	/*
+	 * SPP VM exit happened while executing iret from NMI,

arch/x86/kvm/vmx/vmx.c static int handle_monitor_trap(struct kvm_vcpu *vcpu) · line 5390

Quoted 10 lines

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

+	 * "blocked by NMI" bit has to be set before next VM entry.
+	 * There are errata that may cause this bit to not be set:
+	 * AAK134, BY25.
+	 */
+	if (!(to_vmx(vcpu)->idt_vectoring_info & VECTORING_INFO_VALID_MASK) &&
+	    (exit_qualification & SPPT_INTR_INFO_UNBLOCK_NMI))
+		vmcs_set_bits(GUEST_INTERRUPTIBILITY_INFO,
+			      GUEST_INTR_STATE_NMI);
+
+	vcpu->arch.exit_qualification = exit_qualification;

	if (WARN_ON(!(exit_qualification & SPPT_INDUCED_EXIT_TYPE)))
		goto out_err;

	<handle spp exit>

	return 1;

out_err:

	vcpu->run->exit_reason = KVM_EXIT_UNKNOWN;
	vcpu->run->hw.hardware_exit_reason = EXIT_REASON_SPP;
	return 0;

arch/x86/kvm/vmx/vmx.c static int handle_monitor_trap(struct kvm_vcpu *vcpu) · line 5392

Quoted 2 lines

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

+	if (exit_qualification & SPPT_INDUCED_EXIT_TYPE) {
+		int page_num = KVM_PAGES_PER_HPAGE(PT_DIRECTORY_LEVEL);

The compiler is probably clever enough to make these constants, but if this logic is a fundamental property of SPP then it should be defined as a macro somewhere.

Quoted 5 lines

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

+		u32 *access;
+		gfn_t gfn_max;
+
+		/*
+		 * SPPT missing

arch/x86/kvm/vmx/vmx.c static int handle_monitor_trap(struct kvm_vcpu *vcpu) · line 5403

Quoted 6 lines

arch/x86/kvm/vmx/vmx.c +6 -0

+		 * We don't set SPP write access for the corresponding
+		 * GPA, if we haven't setup, we need to construct
+		 * SPP table here.
+		 */
+		gpa = vmcs_read64(GUEST_PHYSICAL_ADDRESS);
+		gfn = gpa >> PAGE_SHIFT;

gpa_to_gfn()

Quoted 8 lines

arch/x86/kvm/vmx/vmx.c +8 -0

+		trace_kvm_spp_induced_exit(vcpu, gpa, exit_qualification);
+		/*
+		 * In level 1 of SPPT, there's no PRESENT bit, all data is
+		 * regarded as permission vector, so need to check from
+		 * level 2 to set up the vector if target page is protected.
+		 */
+		spin_lock(&vcpu->kvm->mmu_lock);
+		gfn &= ~(page_num - 1);

arch/x86/kvm/vmx/vmx.c static int handle_monitor_trap(struct kvm_vcpu *vcpu) · line 5412

Quoted 1 line

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

+		gfn_max = gfn + page_num - 1;

s/gfn_max/gfn_end

arch/x86/kvm/vmx/vmx.c static int handle_monitor_trap(struct kvm_vcpu *vcpu) · line 5413

Quoted 2 lines

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

+		for (; gfn <= gfn_max; gfn++) {

My preference would be to do:

		gfn_end = gfn + page_num;

		for ( ; gfn < gfn_end; gfn++)

Quoted 1 line

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

+			slot = gfn_to_memslot(vcpu->kvm, gfn);

Quoted 17 lines · Sean Christopherson

arch/x86/kvm/mmu/mmu.c static bool fast_page_fault(struct kvm_vcpu *vcpu, gva_t gva, int level, · line 3611

Quoted 12 lines · Yang Weijiang

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

@@ -3585,7 +3602,30 @@ static bool fast_page_fault(struct kvm_vcpu *vcpu, gva_t gva, int level,
 		if ((error_code & PFERR_WRITE_MASK) &&
 		    spte_can_locklessly_be_made_writable(spte))
 		{
-			new_spte |= PT_WRITABLE_MASK;
+			/*
+			 * Record write protect fault caused by
+			 * Sub-page Protection, let VMI decide
+			 * the next step.
+			 */
+			if (spte & PT_SPP_MASK) {
+				int len = kvm_x86_ops->get_inst_len(vcpu);

There's got to be a better way to handle SPP exits than adding a helper to retrieve the instruction length.

The fault instruction was skipped by kvm_skip_emulated_instruction() before, but Paolo suggested leave the re-do or skip option to user-space to make it flexible for write protection or write tracking, so return length to user-space.

Quoted 18 lines · Yang Weijiang

Quoted 17 lines · Sean Christopherson

arch/x86/kvm/mmu/mmu.c static bool fast_page_fault(struct kvm_vcpu *vcpu, gva_t gva, int level, · line 3611

Quoted 12 lines · Yang Weijiang

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

@@ -3585,7 +3602,30 @@ static bool fast_page_fault(struct kvm_vcpu *vcpu, gva_t gva, int level,
 		if ((error_code & PFERR_WRITE_MASK) &&
 		    spte_can_locklessly_be_made_writable(spte))
 		{
-			new_spte |= PT_WRITABLE_MASK;
+			/*
+			 * Record write protect fault caused by
+			 * Sub-page Protection, let VMI decide
+			 * the next step.
+			 */
+			if (spte & PT_SPP_MASK) {
+				int len = kvm_x86_ops->get_inst_len(vcpu);

There's got to be a better way to handle SPP exits than adding a helper to retrieve the instruction length.

Quoted 4 lines

The fault instruction was skipped by kvm_skip_emulated_instruction() before, but Paolo suggested leave the re-do or skip option to user-space to make it flexible for write protection or write tracking, so return length to user-space.

Sorry, my comment was unclear. I have no objection to punting the fault to userspace, it's the mechanics of how it's done that I dislike.

Specifically, (a) using run->exit_reason to propagate the SPP exit up the stack, e.g. instead of modifying affected call stacks to play nice with any exit to userspace, (b) assuming ->get_insn_len() will always be accurate, e.g. see the various caveats in skip_emulated_instruction() for both VMX and SVM, and (c) duplicating the state capture code in every location that can encounter a SPP fault.

What I'm hoping is that it's possible to modify the call stacks to explicitly propagate an exit to userspace and/or SPP fault, and shove all the state capture into a common location, e.g. handle_ept_violation().

Side topic, assuming the userspace VMI is going to be instrospecting the faulting instruction, won't it decode the instruction? I.e. calculate the instruction length anyways?

Quoted 34 lines · Sean Christopherson

Quoted 18 lines · Yang Weijiang

Quoted 17 lines · Sean Christopherson

arch/x86/kvm/mmu/mmu.c static bool fast_page_fault(struct kvm_vcpu *vcpu, gva_t gva, int level, · line 3611

Quoted 12 lines · Yang Weijiang

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

@@ -3585,7 +3602,30 @@ static bool fast_page_fault(struct kvm_vcpu *vcpu, gva_t gva, int level,
 		if ((error_code & PFERR_WRITE_MASK) &&
 		    spte_can_locklessly_be_made_writable(spte))
 		{
-			new_spte |= PT_WRITABLE_MASK;
+			/*
+			 * Record write protect fault caused by
+			 * Sub-page Protection, let VMI decide
+			 * the next step.
+			 */
+			if (spte & PT_SPP_MASK) {
+				int len = kvm_x86_ops->get_inst_len(vcpu);

There's got to be a better way to handle SPP exits than adding a helper to retrieve the instruction length.

Quoted 4 lines

The fault instruction was skipped by kvm_skip_emulated_instruction() before, but Paolo suggested leave the re-do or skip option to user-space to make it flexible for write protection or write tracking, so return length to user-space.

Sorry, my comment was unclear. I have no objection to punting the fault to userspace, it's the mechanics of how it's done that I dislike.

Specifically, (a) using run->exit_reason to propagate the SPP exit up the stack, e.g. instead of modifying affected call stacks to play nice with any exit to userspace, (b) assuming ->get_insn_len() will always be accurate, e.g. see the various caveats in skip_emulated_instruction() for both VMX and SVM, and (c) duplicating the state capture code in every location that can encounter a SPP fault.

Quoted 7 lines

What I'm hoping is that it's possible to modify the call stacks to explicitly propagate an exit to userspace and/or SPP fault, and shove all the state capture into a common location, e.g. handle_ept_violation().

Side topic, assuming the userspace VMI is going to be instrospecting the faulting instruction, won't it decode the instruction? I.e. calculate the instruction length anyways?

Indeed, we decode the instruction from userspace. I don't know if the instruction length helps other projects. Added Tamas and Mathieu.

In our last VMI API proposal, the breakpoint event had the instruction length sent to userspace, but I can't remember why.

https://lore.kernel.org/kvm/[email protected]/

Quoted 18 lines · Yang Weijiang

Quoted 17 lines · Sean Christopherson

arch/x86/kvm/mmu/mmu.c static bool fast_page_fault(struct kvm_vcpu *vcpu, gva_t gva, int level, · line 3611

Quoted 12 lines · Yang Weijiang

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

@@ -3585,7 +3602,30 @@ static bool fast_page_fault(struct kvm_vcpu *vcpu, gva_t gva, int level,
 		if ((error_code & PFERR_WRITE_MASK) &&
 		    spte_can_locklessly_be_made_writable(spte))
 		{
-			new_spte |= PT_WRITABLE_MASK;
+			/*
+			 * Record write protect fault caused by
+			 * Sub-page Protection, let VMI decide
+			 * the next step.
+			 */
+			if (spte & PT_SPP_MASK) {
+				int len = kvm_x86_ops->get_inst_len(vcpu);

There's got to be a better way to handle SPP exits than adding a helper to retrieve the instruction length.

Quoted 4 lines

The fault instruction was skipped by kvm_skip_emulated_instruction() before, but Paolo suggested leave the re-do or skip option to user-space to make it flexible for write protection or write tracking, so return length to user-space.

Sorry, my comment was unclear. I have no objection to punting the fault to userspace, it's the mechanics of how it's done that I dislike.

Specifically, (a) using run->exit_reason to propagate the SPP exit up the stack, e.g. instead of modifying affected call stacks to play nice with any exit to userspace, (b) assuming ->get_insn_len() will always be accurate, e.g. see the various caveats in skip_emulated_instruction() for both VMX and SVM, and (c) duplicating the state capture code in every location that can encounter a SPP fault.

What I'm hoping is that it's possible to modify the call stacks to explicitly propagate an exit to userspace and/or SPP fault, and shove all the state capture into a common location, e.g. handle_ept_violation().

Side topic, assuming the userspace VMI is going to be instrospecting the faulting instruction, won't it decode the instruction? I.e. calculate the instruction length anyways?

Quoted 27 lines · Sean Christopherson

Quoted 18 lines · Yang Weijiang

Quoted 17 lines · Sean Christopherson

arch/x86/kvm/mmu/mmu.c static bool fast_page_fault(struct kvm_vcpu *vcpu, gva_t gva, int level, · line 3611

Quoted 12 lines · Yang Weijiang

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

@@ -3585,7 +3602,30 @@ static bool fast_page_fault(struct kvm_vcpu *vcpu, gva_t gva, int level,
 		if ((error_code & PFERR_WRITE_MASK) &&
 		    spte_can_locklessly_be_made_writable(spte))
 		{
-			new_spte |= PT_WRITABLE_MASK;
+			/*
+			 * Record write protect fault caused by
+			 * Sub-page Protection, let VMI decide
+			 * the next step.
+			 */
+			if (spte & PT_SPP_MASK) {
+				int len = kvm_x86_ops->get_inst_len(vcpu);

There's got to be a better way to handle SPP exits than adding a helper to retrieve the instruction length.

Quoted 4 lines

The fault instruction was skipped by kvm_skip_emulated_instruction() before, but Paolo suggested leave the re-do or skip option to user-space to make it flexible for write protection or write tracking, so return length to user-space.

Sorry, my comment was unclear. I have no objection to punting the fault to userspace, it's the mechanics of how it's done that I dislike.

Quoted 6 lines

Specifically, (a) using run->exit_reason to propagate the SPP exit up the stack, e.g. instead of modifying affected call stacks to play nice with any exit to userspace, (b) assuming ->get_insn_len() will always be accurate, e.g. see the various caveats in skip_emulated_instruction() for both VMX and SVM, and (c) duplicating the state capture code in every location that can encounter a SPP fault.

How about calling skip_emulated_instruction() in KVM before exit to

userspace, but still return the skipped instruction length, if userspace would like to re-execute the instruction, it can unwind RIP or simply rely on KVM?

Quoted 4 lines

What I'm hoping is that it's possible to modify the call stacks to explicitly propagate an exit to userspace and/or SPP fault, and shove all the state capture into a common location, e.g. handle_ept_violation().

The problem is, the state capture code in fast_page_fault() and emulation case share different causes, the former is generic occurence of SPP induced EPT violation, the latter is atually a "faked" one while detecting emulation instruction is writing some SPP protected area, so I seperated them.

Quoted 4 lines

Side topic, assuming the userspace VMI is going to be instrospecting the faulting instruction, won't it decode the instruction? I.e. calculate the instruction length anyways?

Quoted 28 lines · Yang Weijiang

Quoted 27 lines · Sean Christopherson

Quoted 18 lines · Yang Weijiang

Quoted 17 lines · Sean Christopherson

arch/x86/kvm/mmu/mmu.c static bool fast_page_fault(struct kvm_vcpu *vcpu, gva_t gva, int level, · line 3611

Quoted 12 lines · Yang Weijiang

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

@@ -3585,7 +3602,30 @@ static bool fast_page_fault(struct kvm_vcpu *vcpu, gva_t gva, int level,
 		if ((error_code & PFERR_WRITE_MASK) &&
 		    spte_can_locklessly_be_made_writable(spte))
 		{
-			new_spte |= PT_WRITABLE_MASK;
+			/*
+			 * Record write protect fault caused by
+			 * Sub-page Protection, let VMI decide
+			 * the next step.
+			 */
+			if (spte & PT_SPP_MASK) {
+				int len = kvm_x86_ops->get_inst_len(vcpu);

There's got to be a better way to handle SPP exits than adding a helper to retrieve the instruction length.

Quoted 4 lines

The fault instruction was skipped by kvm_skip_emulated_instruction() before, but Paolo suggested leave the re-do or skip option to user-space to make it flexible for write protection or write tracking, so return length to user-space.

Sorry, my comment was unclear. I have no objection to punting the fault to userspace, it's the mechanics of how it's done that I dislike.

Quoted 8 lines

Quoted 6 lines

Specifically, (a) using run->exit_reason to propagate the SPP exit up the stack, e.g. instead of modifying affected call stacks to play nice with any exit to userspace, (b) assuming ->get_insn_len() will always be accurate, e.g. see the various caveats in skip_emulated_instruction() for both VMX and SVM, and (c) duplicating the state capture code in every location that can encounter a SPP fault.

How about calling skip_emulated_instruction() in KVM before exit to

I'm confused. It sounds like KVM_EXIT_SPP provides the instruction length because it skips an instruction before exiting to userspace. But if KVM is is emulating an instruction, it shouldn't be doing {kvm_}skip_emulated_instruction(), e.g. if emulation fails due to a SPP violation (returns KVM_EXIT_SPP) then GUEST_RIP should still point at the exiting instruction. Ditto for the fast_page_fault() case, RIP shouldn't be advanced.

What am I missing?

Quoted 3 lines

userspace, but still return the skipped instruction length, if userspace would like to re-execute the instruction, it can unwind RIP or simply rely on KVM?

I'm not convinced the instruction length needs to be provided to userspace for this case. Obviously it's not difficult to provide the info, I just don't understand the value added by doing so. As above, RIP shouldn't need to be unwound, and blindly skipping an instruction seems like an odd thing for a VMI engine to do.

Quoted 10 lines

Quoted 5 lines

What I'm hoping is that it's possible to modify the call stacks to explicitly propagate an exit to userspace and/or SPP fault, and shove all the state capture into a common location, e.g. handle_ept_violation().

The problem is, the state capture code in fast_page_fault() and emulation case share different causes, the former is generic occurence of SPP induced EPT violation, the latter is atually a "faked" one while detecting emulation instruction is writing some SPP protected area, so I seperated them.

Can we make SPP dependent on unrestricted guest so that the only entry point to the emulator is through handle_ept_violation()? And thus the only path to triggering KVM_EXIT_SPP would also be through handle_ept_violation(); (I think, might be forgetting a different emulation path).

Quoted 4 lines

Quoted 4 lines

Side topic, assuming the userspace VMI is going to be instrospecting the faulting instruction, won't it decode the instruction? I.e. calculate the instruction length anyways?

I'm not convinced the instruction length needs to be provided to userspace for this case. Obviously it's not difficult to provide the info, I just don't understand the value added by doing so. As above, RIP shouldn't need to be unwound, and blindly skipping an instruction seems like an odd thing for a VMI engine to do.