KVM: guest_memfd(), X86: Common base for SNP and TDX (was KVM: guest memory: Misc enhancement)

From: Isaku Yamahata <[email protected]>

Hello. I've updated KVM: guest memory: Misc enhancement patch series based on "[RFC PATCH v11 00/29] KVM: guest_memfd() and per-page attributes" [1]. I changed the subject to represent the patch series better.

The purpose is to get agreement on the common base patches both for SNP [2] and TDX [3]. (And hopefully for other technology to protect guest memory.) Then, SNP and TDX can make progress without stepping on each other.

The main change from the previous version is
- The rebased to v11 KVM guest_memfd()
- Introduce KVM_X86_SNP_VM and KVM_x86_TDX_VM
- Make KVM_MEM_ENC_OP uABI common for SNP and TDX

[1] https://lore.kernel.org/all/[email protected]/

[2] https://lore.kernel.org/lkml/[email protected]/
Add AMD Secure Nested Paging (SEV-SNP) Hypervisor Support

[3] https://lore.kernel.org/all/[email protected]/
KVM TDX basic feature support

Changes:
v4:
- The rebased to v11 KVM guest_memfd()
- Introduce KVM_X86_SNP_VM and KVM_x86_TDX_VM
- Newly include a patch to make KVM_MEM_ENC_OP uABI common for SNP and TDX
- include a patch to address IMPLICIT_ACCESS

v3:
https://lore.kernel.org/all/[email protected]/

v2:
https://lore.kernel.org/all/[email protected]/

v1:
https://lore.kernel.org/all/[email protected]/

Brijesh Singh (1):
KVM: x86: Export the kvm_zap_gfn_range() for the SNP use

Isaku Yamahata (6):
KVM: x86: Add is_vm_type_supported callback
KVM: x86/mmu: Pass around full 64-bit error code for the KVM page
fault
KVM: x86: Introduce PFERR_GUEST_ENC_MASK to indicate fault is private
KVM: Add new members to struct kvm_gfn_range to operate on
KVM: x86: Make struct sev_cmd common for KVM_MEM_ENC_OP
KVM: X86: KVM_MEM_ENC_OP check if unused field (flags, error) is zero

Michael Roth (2):
KVM: x86: Add gmem hook for initializing private memory
KVM: x86: Add gmem hook for invalidating private memory

Sean Christopherson (1):
KVM: x86/mmu: Guard against collision with KVM-defined
PFERR_IMPLICIT_ACCESS

 arch/x86/include/asm/kvm-x86-ops.h |  3 ++
 arch/x86/include/asm/kvm_host.h    | 10 ++++-
 arch/x86/include/uapi/asm/kvm.h    | 35 +++++++++++++++
 arch/x86/kvm/mmu.h                 |  2 -
 arch/x86/kvm/mmu/mmu.c             | 37 +++++++++++++---
 arch/x86/kvm/mmu/mmu_internal.h    | 18 ++++++--
 arch/x86/kvm/mmu/mmutrace.h        |  2 +-
 arch/x86/kvm/mmu/paging_tmpl.h     |  2 +-
 arch/x86/kvm/svm/sev.c             | 68 ++++++++++++++++--------------
 arch/x86/kvm/svm/svm.c             |  7 +++
 arch/x86/kvm/svm/svm.h             |  2 +-
 arch/x86/kvm/vmx/vmx.c             |  7 +++
 arch/x86/kvm/x86.c                 | 50 +++++++++++++++++++++-
 arch/x86/kvm/x86.h                 |  2 +
 include/linux/kvm_host.h           |  5 +++
 virt/kvm/guest_mem.c               | 44 +++++++++++++++++++
 virt/kvm/kvm_main.c                |  4 ++
 17 files changed, 249 insertions(+), 49 deletions(-)

base-commit: bfa3037d828050896ae52f6467b6ca2489ae6fb1
prerequisite-patch-id: 3bd3037b3803e2d84f0ef98bb6c678be44eddd08
prerequisite-patch-id: b474cbf4f0ea21cf945036271f5286017e0efc84
prerequisite-patch-id: bd96a89fafe51956a55fdfc08a3ea2a37a2e55e4
prerequisite-patch-id: f15d178f9000430e0089c546756ab1d8d29341a7
prerequisite-patch-id: 5b34829d7433fa81ed574d724ee476b9cc2e6a50
prerequisite-patch-id: bf75388851ee37a83b37bfa7cb0084f27301f6bc
prerequisite-patch-id: 9d77fb0e8ce8c8c21e22ff3f26bd168eb5446df0
prerequisite-patch-id: 7152514149d4b4525a0057e3460ff78861e162f5
prerequisite-patch-id: a1d688257a210564ebeb23b1eef4b9ad1f5d7be3
prerequisite-patch-id: 0b1e771c370a03e1588ed97ee77cb0493d9304f4
prerequisite-patch-id: 313219882d617e4d4cb226760d1f071f52b3f882
prerequisite-patch-id: a8ebe373e3913fd0e0a55c57f55690f432975ec0
prerequisite-patch-id: 8b06f2333214e355b145113e33c65ade85d7eac4
prerequisite-patch-id: e739dd58995d35b0f888d02a6bf4ea144476f264
prerequisite-patch-id: 0e93d19cb59f3a052a377a56ff0a4399046818aa
prerequisite-patch-id: 4e0839abbfb8885154e278b4b0071a760199ad46
prerequisite-patch-id: be193bb3393ad8a16ea376a530df20a145145259
prerequisite-patch-id: 301dbdf8448175ea609664c890a3694750ecf740
prerequisite-patch-id: ba8e6068bcef7865bb5523065e19edd49fbc02de
prerequisite-patch-id: 81b25d13169b3617c12992dce85613a2730b0e1b
prerequisite-patch-id: b4526dee5b5a95da0a13116ae0c73d4e69efa3c6
prerequisite-patch-id: 8c62bacc52a75d4a9038a3f597fe436c50e07de3
prerequisite-patch-id: 5618d2414a1ef641b4c247b5e28076f67a765b24
prerequisite-patch-id: 022b4620f6ff729eca842192259e986d126e7fa6
prerequisite-patch-id: 73ebc581a3ce9a51167785d273fe69406ccccaed
prerequisite-patch-id: 1225df90aeae430a74354bc5ad0ddf508d0707db
prerequisite-patch-id: 1e38df398ee370ad7e457f4890d6e4457e8a83fa
prerequisite-patch-id: b8812b613f5674351565ea28354e91a756efd56e
prerequisite-patch-id: e231eff2baba07c2de984dd6cf83ad1a31b792b8

2.25.1

From: Isaku Yamahata <[email protected]>

For TDX, allow the backend can override the supported vm type. Add KVM_X86_TDX_VM to reserve the bit.

---
Changes v3 -> v4:
- Added KVM_X86_SNP_VM

Changes v2 -> v3:
- no change
- didn't bother to rename KVM_X86_PROTECTED_VM to KVM_X86_SW_PROTECTED_VM

Changes v1 -> v2
- no change

---
 arch/x86/include/asm/kvm-x86-ops.h |  1 +
 arch/x86/include/asm/kvm_host.h    |  1 +
 arch/x86/include/uapi/asm/kvm.h    |  2 ++
 arch/x86/kvm/svm/svm.c             |  7 +++++++
 arch/x86/kvm/vmx/vmx.c             |  7 +++++++
 arch/x86/kvm/x86.c                 | 12 +++++++++++-
 arch/x86/kvm/x86.h                 |  2 ++
 7 files changed, 31 insertions(+), 1 deletion(-)

index 13bc212cd4bc..c0143906fe6d 100644
@@ -20,6 +20,7 @@ KVM_X86_OP(hardware_disable)
 KVM_X86_OP(hardware_unsetup)
 KVM_X86_OP(has_emulated_msr)
 KVM_X86_OP(vcpu_after_set_cpuid)
+KVM_X86_OP(is_vm_type_supported)
 KVM_X86_OP(vm_init)
 KVM_X86_OP_OPTIONAL(vm_destroy)
 KVM_X86_OP_OPTIONAL_RET0(vcpu_precreate)

index bbefd79b7950..2c9350aa0da4 100644
@@ -1543,6 +1543,7 @@ struct kvm_x86_ops {
 	bool (*has_emulated_msr)(struct kvm *kvm, u32 index);
 	void (*vcpu_after_set_cpuid)(struct kvm_vcpu *vcpu);
 
+	bool (*is_vm_type_supported)(unsigned long vm_type);
 	unsigned int vm_size;
 	int (*vm_init)(struct kvm *kvm);
 	void (*vm_destroy)(struct kvm *kvm);

index a448d0964fc0..aa7a56a47564 100644
@@ -564,5 +564,7 @@ struct kvm_pmu_event_filter {
 
 #define KVM_X86_DEFAULT_VM	0
 #define KVM_X86_SW_PROTECTED_VM	1
+#define KVM_X86_TDX_VM		2
+#define KVM_X86_SNP_VM		3
 
 #endif /* _ASM_X86_KVM_H */

index d381ad424554..d681dd7ad397 100644
@@ -4768,6 +4768,12 @@ static void svm_vm_destroy(struct kvm *kvm)
 	sev_vm_destroy(kvm);
 }
 
+static bool svm_is_vm_type_supported(unsigned long type)
+{
+	/* FIXME: Check if CPU is capable of SEV-SNP. */
+	return __kvm_is_vm_type_supported(type);
+}
+
 static int svm_vm_init(struct kvm *kvm)
 {
 	if (!pause_filter_count || !pause_filter_thresh)

@@ -4796,6 +4802,7 @@ static struct kvm_x86_ops svm_x86_ops __initdata = {
 	.vcpu_free = svm_vcpu_free,
 	.vcpu_reset = svm_vcpu_reset,
 
+	.is_vm_type_supported = svm_is_vm_type_supported,
 	.vm_size = sizeof(struct kvm_svm),
 	.vm_init = svm_vm_init,
 	.vm_destroy = svm_vm_destroy,

index 946380b53cf5..693f07b80966 100644
@@ -7511,6 +7511,12 @@ static int vmx_vcpu_create(struct kvm_vcpu *vcpu)
 	return err;
 }
 
+static bool vmx_is_vm_type_supported(unsigned long type)
+{
+	/* TODO: Check if TDX is supported. */
+	return __kvm_is_vm_type_supported(type);
+}
+
 #define L1TF_MSG_SMT "L1TF CPU bug present and SMT on, data leak possible. See CVE-2018-3646 and https://www.kernel.org/doc/html/latest/admin-guide/hw-vuln/l1tf.html for details.\n"
 #define L1TF_MSG_L1D "L1TF CPU bug present and virtualization mitigation disabled, data leak possible. See CVE-2018-3646 and https://www.kernel.org/doc/html/latest/admin-guide/hw-vuln/l1tf.html for details.\n"
 

@@ -8180,6 +8186,7 @@ static struct kvm_x86_ops vmx_x86_ops __initdata = {
 	.hardware_disable = vmx_hardware_disable,
 	.has_emulated_msr = vmx_has_emulated_msr,
 
+	.is_vm_type_supported = vmx_is_vm_type_supported,
 	.vm_size = sizeof(struct kvm_vmx),
 	.vm_init = vmx_vm_init,
 	.vm_destroy = vmx_vm_destroy,

index de195ad83ec0..fd6c05d1883c 100644
@@ -4427,12 +4427,18 @@ static int kvm_ioctl_get_supported_hv_cpuid(struct kvm_vcpu *vcpu,
 	return 0;
 }
 
-static bool kvm_is_vm_type_supported(unsigned long type)
+bool __kvm_is_vm_type_supported(unsigned long type)
 {
 	return type == KVM_X86_DEFAULT_VM ||
 	       (type == KVM_X86_SW_PROTECTED_VM &&
 		IS_ENABLED(CONFIG_KVM_SW_PROTECTED_VM) && tdp_enabled);
 }
+EXPORT_SYMBOL_GPL(__kvm_is_vm_type_supported);
+
+static bool kvm_is_vm_type_supported(unsigned long type)
+{
+	return static_call(kvm_x86_is_vm_type_supported)(type);
+}
 
 int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
 {

@@ -4628,6 +4634,10 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
 		r = BIT(KVM_X86_DEFAULT_VM);
 		if (kvm_is_vm_type_supported(KVM_X86_SW_PROTECTED_VM))
 			r |= BIT(KVM_X86_SW_PROTECTED_VM);
+		if (kvm_is_vm_type_supported(KVM_X86_TDX_VM))
+			r |= BIT(KVM_X86_TDX_VM);
+		if (kvm_is_vm_type_supported(KVM_X86_SNP_VM))
+			r |= BIT(KVM_X86_SNP_VM);
 		break;
 	default:
 		break;

index 82e3dafc5453..7de3a45f655a 100644
@@ -9,6 +9,8 @@
 #include "kvm_cache_regs.h"
 #include "kvm_emulate.h"
 
+bool __kvm_is_vm_type_supported(unsigned long type);
+
 struct kvm_caps {
 	/* control of guest tsc rate supported? */
 	bool has_tsc_control;

2.25.1

From: Sean Christopherson <[email protected]>

Add an assertion in kvm_mmu_page_fault() to ensure the error code provided by hardware doesn't conflict with KVM's software-defined IMPLICIT_ACCESS flag. In the unlikely scenario that future hardware starts using bit 48 for a hardware-defined flag, preserving the bit could result in KVM incorrectly interpreting the unknown flag as KVM's IMPLICIT_ACCESS flag.

WARN so that any such conflict can be surfaced to KVM developers and resolved, but otherwise ignore the bit as KVM can't possibly rely on a flag it knows nothing about.

---
 arch/x86/kvm/mmu/mmu.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

index 05943ccb55a4..a9bbc20c7dfd 100644
@@ -5822,6 +5822,17 @@ int noinline kvm_mmu_page_fault(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa, u64 err
 	int r, emulation_type = EMULTYPE_PF;
 	bool direct = vcpu->arch.mmu->root_role.direct;
 
+	/*
+	 * IMPLICIT_ACCESS is a KVM-defined flag used to correctly perform SMAP
+	 * checks when emulating instructions that triggers implicit access.
+	 * WARN if hardware generates a fault with an error code that collides
+	 * with the KVM-defined value.  Clear the flag and continue on, i.e.
+	 * don't terminate the VM, as KVM can't possibly be relying on a flag
+	 * that KVM doesn't know about.
+	 */
+	if (WARN_ON_ONCE(error_code & PFERR_IMPLICIT_ACCESS))
+		error_code &= ~PFERR_IMPLICIT_ACCESS;
+
 	if (WARN_ON(!VALID_PAGE(vcpu->arch.mmu->root.hpa)))
 		return RET_PF_RETRY;
 

2.25.1

From: Isaku Yamahata <[email protected]>

Because the full 64-bit error code, or more info about the fault, for the KVM page fault will be needed for protected VM, TDX and SEV-SNP, update kvm_mmu_do_page_fault() to accept the 64-bit value so it can pass it to the callbacks.

The upper 32 bits of error code are discarded at kvm_mmu_page_fault() by lower_32_bits(). Now it's passed down as full 64 bits. Currently two hardware defined bits, PFERR_GUEST_FINAL_MASK and PFERR_GUEST_PAGE_MASK, and one software defined bit, PFERR_IMPLICIT_ACCESS, is defined.

PFERR_IMPLICIT_ACCESS:
commit 4f4aa80e3b88 ("KVM: X86: Handle implicit supervisor access with SMAP") introduced a software defined bit PFERR_IMPLICIT_ACCESS at bit 48 to indicate implicit access for SMAP with instruction emulator. Concretely emulator_read_std() and emulator_write_std() set the bit. permission_fault() checks the bit as smap implicit access. The vendor page fault handler shouldn't pass the bit to kvm_mmu_page_fault().

PFERR_GUEST_FINAL_MASK and PFERR_GUEST_PAGE_MASK:
commit 147277540bbc ("kvm: svm: Add support for additional SVM NPF error codes") introduced them to optimize the nested page fault handling. Other code path doesn't use the bits. Those two bits can be safely passed down without functionality change.

The accesses of fault->error_code are as follows
- FNAME(page_fault): PFERR_IMPLICIT_ACCESS shouldn't be passed down. PFERR_GUEST_FINAL_MASK and PFERR_GUEST_PAGE_MASK aren't used.
- kvm_mmu_page_fault(): explicit mask with PFERR_RSVD_MASK, and PFERR_NESTED_GUEST_PAGE is used outside of the masking upper 32 bits.
- mmutrace: change u32 -> u64
- pgprintk(): change %x -> %llx

No functional change is intended. This is a preparation to pass on more info with page fault error code.

---
Changes v2 -> v3:
- Make depends on a patch to clear PFERR_IMPLICIT_ACCESS
- drop clearing the upper 32 bit, instead just pass whole 64 bits
- update commit message to mention about PFERR_IMPLICIT_ACCESS and PFERR_NESTED_GUEST_PAGE

Changes v1 -> v2:
- no change

---
 arch/x86/kvm/mmu/mmu.c          | 5 ++---
 arch/x86/kvm/mmu/mmu_internal.h | 4 ++--
 arch/x86/kvm/mmu/mmutrace.h     | 2 +-
 arch/x86/kvm/mmu/paging_tmpl.h  | 2 +-
 4 files changed, 6 insertions(+), 7 deletions(-)

index a9bbc20c7dfd..a2fe091e327a 100644
@@ -4523,7 +4523,7 @@ static int direct_page_fault(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault
 static int nonpaging_page_fault(struct kvm_vcpu *vcpu,
 				struct kvm_page_fault *fault)
 {
-	pgprintk("%s: gva %lx error %x\n", __func__, fault->addr, fault->error_code);
+	pgprintk("%s: gva %llx error %llx\n", __func__, fault->addr, fault->error_code);
 
 	/* This path builds a PAE pagetable, we can map 2mb pages at maximum. */
 	fault->max_level = PG_LEVEL_2M;

@@ -5844,8 +5844,7 @@ int noinline kvm_mmu_page_fault(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa, u64 err
 	}
 
 	if (r == RET_PF_INVALID) {
-		r = kvm_mmu_do_page_fault(vcpu, cr2_or_gpa,
-					  lower_32_bits(error_code), false,
+		r = kvm_mmu_do_page_fault(vcpu, cr2_or_gpa, error_code, false,
 					  &emulation_type);
 		if (KVM_BUG_ON(r == RET_PF_INVALID, vcpu->kvm))
 			return -EIO;

index f1786698ae00..7f9ec1e5b136 100644
@@ -191,7 +191,7 @@ static inline bool is_nx_huge_page_enabled(struct kvm *kvm)
 struct kvm_page_fault {
 	/* arguments to kvm_mmu_do_page_fault.  */
 	const gpa_t addr;
-	const u32 error_code;
+	const u64 error_code;
 	const bool prefetch;
 
 	/* Derived from error_code.  */

@@ -283,7 +283,7 @@ enum {
 };
 
 static inline int kvm_mmu_do_page_fault(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa,
-					u32 err, bool prefetch, int *emulation_type)
+					u64 err, bool prefetch, int *emulation_type)
 {
 	struct kvm_page_fault fault = {
 		.addr = cr2_or_gpa,

index 2d7555381955..2e77883c92f6 100644
@@ -261,7 +261,7 @@ TRACE_EVENT(
 	TP_STRUCT__entry(
 		__field(int, vcpu_id)
 		__field(gpa_t, cr2_or_gpa)
-		__field(u32, error_code)
+		__field(u64, error_code)
 		__field(u64 *, sptep)
 		__field(u64, old_spte)
 		__field(u64, new_spte)

index 0662e0278e70..42d48b1ec7b3 100644
@@ -758,7 +758,7 @@ static int FNAME(page_fault)(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault
 	struct guest_walker walker;
 	int r;
 
-	pgprintk("%s: addr %lx err %x\n", __func__, fault->addr, fault->error_code);
+	pgprintk("%s: addr %llx err %llx\n", __func__, fault->addr, fault->error_code);
 	WARN_ON_ONCE(fault->is_tdp);
 
 	/*

2.25.1

From: Isaku Yamahata <[email protected]>

Add two PFERR codes to designate that the page fault is private and that it requires looking up memory attributes. The vendor kvm page fault handler should set PFERR_GUEST_ENC_MASK bit based on their fault information. It may or may not use the hardware value directly or parse the hardware value to set the bit.

For KVM_X86_PROTECTED_VM, ask memory attributes for the fault privateness.

---
Changes v3 -> v4:
- rename back struct kvm_page_fault::private => is_private
- catch up rename: KVM_X86_PROTECTED_VM => KVM_X86_SW_PROTECTED_VM

Changes v2 -> v3:
- Revive PFERR_GUEST_ENC_MASK
- rename struct kvm_page_fault::is_private => private
- Add check KVM_X86_PROTECTED_VM

Changes v1 -> v2:
- Introduced fault type and replaced is_private with fault_type.
- Add kvm_get_fault_type() to encapsulate the difference.

---
 arch/x86/include/asm/kvm_host.h |  2 ++
 arch/x86/kvm/mmu/mmu.c          |  8 ++++++--
 arch/x86/kvm/mmu/mmu_internal.h | 14 +++++++++++++-
 3 files changed, 21 insertions(+), 3 deletions(-)

index 2c9350aa0da4..ab7d080bf544 100644
@@ -255,6 +255,7 @@ enum x86_intercept_stage;
 #define PFERR_SGX_BIT 15
 #define PFERR_GUEST_FINAL_BIT 32
 #define PFERR_GUEST_PAGE_BIT 33
+#define PFERR_GUEST_ENC_BIT 34
 #define PFERR_IMPLICIT_ACCESS_BIT 48
 
 #define PFERR_PRESENT_MASK	BIT(PFERR_PRESENT_BIT)

@@ -266,6 +267,7 @@ enum x86_intercept_stage;
 #define PFERR_SGX_MASK		BIT(PFERR_SGX_BIT)
 #define PFERR_GUEST_FINAL_MASK	BIT_ULL(PFERR_GUEST_FINAL_BIT)
 #define PFERR_GUEST_PAGE_MASK	BIT_ULL(PFERR_GUEST_PAGE_BIT)
+#define PFERR_GUEST_ENC_MASK	BIT_ULL(PFERR_GUEST_ENC_BIT)
 #define PFERR_IMPLICIT_ACCESS	BIT_ULL(PFERR_IMPLICIT_ACCESS_BIT)
 
 #define PFERR_NESTED_GUEST_PAGE (PFERR_GUEST_PAGE_MASK |	\

index a2fe091e327a..d2ebe26fb822 100644
@@ -4399,8 +4399,12 @@ static int __kvm_faultin_pfn(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault
 			return RET_PF_EMULATE;
 	}
 
-	if (fault->is_private != kvm_mem_is_private(vcpu->kvm, fault->gfn))
-		return kvm_do_memory_fault_exit(vcpu, fault);
+	if (fault->is_private != kvm_mem_is_private(vcpu->kvm, fault->gfn)) {
+		if (vcpu->kvm->arch.vm_type == KVM_X86_SW_PROTECTED_VM)
+			return RET_PF_RETRY;
+		else
+			return kvm_do_memory_fault_exit(vcpu, fault);
+	}
 
 	if (fault->is_private)
 		return kvm_faultin_pfn_private(vcpu, fault);

index 7f9ec1e5b136..4f8f83546c37 100644
@@ -282,6 +282,18 @@ enum {
 	RET_PF_SPURIOUS,
 };
 
+static inline bool kvm_is_fault_private(struct kvm *kvm, gpa_t gpa, u64 error_code)
+{
+	/*
+	 * This is racy with mmu_seq.  If we hit a race, it would result in a
+	 * spurious KVM_EXIT_MEMORY_FAULT.
+	 */
+	if (kvm->arch.vm_type == KVM_X86_SW_PROTECTED_VM)
+		return kvm_mem_is_private(kvm, gpa_to_gfn(gpa));
+
+	return error_code & PFERR_GUEST_ENC_MASK;
+}
+
 static inline int kvm_mmu_do_page_fault(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa,
 					u64 err, bool prefetch, int *emulation_type)
 {

@@ -295,13 +307,13 @@ static inline int kvm_mmu_do_page_fault(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa,
 		.user = err & PFERR_USER_MASK,
 		.prefetch = prefetch,
 		.is_tdp = likely(vcpu->arch.mmu->page_fault == kvm_tdp_page_fault),
+		.is_private = kvm_is_fault_private(vcpu->kvm, cr2_or_gpa, err),
 		.nx_huge_page_workaround_enabled =
 			is_nx_huge_page_enabled(vcpu->kvm),
 
 		.max_level = KVM_MAX_HUGEPAGE_LEVEL,
 		.req_level = PG_LEVEL_4K,
 		.goal_level = PG_LEVEL_4K,
-		.is_private = kvm_mem_is_private(vcpu->kvm, cr2_or_gpa >> PAGE_SHIFT),
 	};
 	int r;
 

2.25.1

s/Introduce/Use

This doesn't "introduce" anything, in the sense that it's an AMD-defined error code flag. That matters because KVM *did* introduce/define PFERR_IMPLICIT_ACCESS.

...

index 20e289e872eb..de9e0a9c41e6 100644
@@ -5751,6 +5751,10 @@ int noinline kvm_mmu_page_fault(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa, u64 err
        if (WARN_ON(!VALID_PAGE(vcpu->arch.mmu->root.hpa)))
                return RET_PF_RETRY;
 
+       if (vcpu->kvm->arch.vm_type == KVM_X86_SW_PROTECTED_VM &&
+           kvm_mem_is_private(vcpu->kvm, gpa_to_gfn(cr2_or_gpa)))
+               error_code |= PFERR_GUEST_ENC_MASK;
+
        r = RET_PF_INVALID;
        if (unlikely(error_code & PFERR_RSVD_MASK)) {
                r = handle_mmio_page_fault(vcpu, cr2_or_gpa, direct);

Here is the updated patch.

From 30c452cd6a94b485eaa5f92dee4c222dd30ebcbe Mon Sep 17 00:00:00 2001
Message-Id: <30c452cd6a94b485eaa5f92dee4c222dd30ebcbe.1689987085.git.isaku.yamahata@intel.com>
In-Reply-To: <ab9d8654bd98ae24de05788a2ecaa4bea6c0c44b.1689987085.git.isaku.yamahata@intel.com>
References: <ab9d8654bd98ae24de05788a2ecaa4bea6c0c44b.1689987085.git.isaku.yamahata@intel.com>
From: Isaku Yamahata <[email protected]>
Date: Wed, 14 Jun 2023 12:34:00 -0700
Subject: [PATCH 4/8] KVM: x86: Use PFERR_GUEST_ENC_MASK to indicate fault is
private

SEV-SNP defines PFERR_GUEST_ENC_MASK (bit 32) in page-fault error bits to represent the guest page is encrypted. Use the bit to designate that the page fault is private and that it requires looking up memory attributes. The vendor kvm page fault handler should set PFERR_GUEST_ENC_MASK bit based on their fault information. It may or may not use the hardware value directly or parse the hardware value to set the bit.

For KVM_X86_SW_PROTECTED_VM, ask memory attributes for the fault privateness. For async page fault, carry the bit and use it for kvm page fault handler.

---
Changes v4 -> v5:
- Eliminate kvm_is_fault_private() by open code the function
- Make async page fault handler to carry is_private bit

Changes v3 -> v4:
- rename back struct kvm_page_fault::private => is_private
- catch up rename: KVM_X86_PROTECTED_VM => KVM_X86_SW_PROTECTED_VM

Changes v2 -> v3:
- Revive PFERR_GUEST_ENC_MASK
- rename struct kvm_page_fault::is_private => private
- Add check KVM_X86_PROTECTED_VM

Changes v1 -> v2:
- Introduced fault type and replaced is_private with fault_type.
- Add kvm_get_fault_type() to encapsulate the difference.

---
 arch/x86/include/asm/kvm_host.h |  3 +++
 arch/x86/kvm/mmu/mmu.c          | 32 +++++++++++++++++++++++---------
 arch/x86/kvm/mmu/mmu_internal.h |  2 +-
 3 files changed, 27 insertions(+), 10 deletions(-)

index 2c9350aa0da4..a1ae3d881063 100644
@@ -255,6 +255,7 @@ enum x86_intercept_stage;
 #define PFERR_SGX_BIT 15
 #define PFERR_GUEST_FINAL_BIT 32
 #define PFERR_GUEST_PAGE_BIT 33
+#define PFERR_GUEST_ENC_BIT 34
 #define PFERR_IMPLICIT_ACCESS_BIT 48
 
 #define PFERR_PRESENT_MASK	BIT(PFERR_PRESENT_BIT)

@@ -266,6 +267,7 @@ enum x86_intercept_stage;
 #define PFERR_SGX_MASK		BIT(PFERR_SGX_BIT)
 #define PFERR_GUEST_FINAL_MASK	BIT_ULL(PFERR_GUEST_FINAL_BIT)
 #define PFERR_GUEST_PAGE_MASK	BIT_ULL(PFERR_GUEST_PAGE_BIT)
+#define PFERR_GUEST_ENC_MASK	BIT_ULL(PFERR_GUEST_ENC_BIT)
 #define PFERR_IMPLICIT_ACCESS	BIT_ULL(PFERR_IMPLICIT_ACCESS_BIT)
 
 #define PFERR_NESTED_GUEST_PAGE (PFERR_GUEST_PAGE_MASK |	\

@@ -1770,6 +1772,7 @@ struct kvm_arch_async_pf {
 	gfn_t gfn;
 	unsigned long cr3;
 	bool direct_map;
+	u64 error_code;
 };
 
 extern u32 __read_mostly kvm_nr_uret_msrs;

index a2fe091e327a..01e74af48e4c 100644
@@ -4285,18 +4285,19 @@ static u32 alloc_apf_token(struct kvm_vcpu *vcpu)
 	return (vcpu->arch.apf.id++ << 12) | vcpu->vcpu_id;
 }
 
-static bool kvm_arch_setup_async_pf(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa,
-				    gfn_t gfn)
+static bool kvm_arch_setup_async_pf(struct kvm_vcpu *vcpu,
+				    struct kvm_page_fault *fault)
 {
 	struct kvm_arch_async_pf arch;
 
 	arch.token = alloc_apf_token(vcpu);
-	arch.gfn = gfn;
+	arch.gfn = fault->gfn;
 	arch.direct_map = vcpu->arch.mmu->root_role.direct;
 	arch.cr3 = kvm_mmu_get_guest_pgd(vcpu, vcpu->arch.mmu);
+	arch.error_code = fault->error_code & PFERR_GUEST_ENC_MASK;
 
-	return kvm_setup_async_pf(vcpu, cr2_or_gpa,
-				  kvm_vcpu_gfn_to_hva(vcpu, gfn), &arch);
+	return kvm_setup_async_pf(vcpu, fault->addr,
+				  kvm_vcpu_gfn_to_hva(vcpu, fault->gfn), &arch);
 }
 
 void kvm_arch_async_page_ready(struct kvm_vcpu *vcpu, struct kvm_async_pf *work)

@@ -4315,7 +4316,8 @@ void kvm_arch_async_page_ready(struct kvm_vcpu *vcpu, struct kvm_async_pf *work)
 	      work->arch.cr3 != kvm_mmu_get_guest_pgd(vcpu, vcpu->arch.mmu))
 		return;
 
-	kvm_mmu_do_page_fault(vcpu, work->cr2_or_gpa, 0, true, NULL);
+	kvm_mmu_do_page_fault(vcpu, work->cr2_or_gpa, work->arch.error_code,
+			      true, NULL);
 }
 
 static inline u8 kvm_max_level_for_order(int order)

@@ -4399,8 +4401,12 @@ static int __kvm_faultin_pfn(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault
 			return RET_PF_EMULATE;
 	}
 
-	if (fault->is_private != kvm_mem_is_private(vcpu->kvm, fault->gfn))
-		return kvm_do_memory_fault_exit(vcpu, fault);
+	if (fault->is_private != kvm_mem_is_private(vcpu->kvm, fault->gfn)) {
+		if (vcpu->kvm->arch.vm_type == KVM_X86_SW_PROTECTED_VM)
+			return RET_PF_RETRY;
+		else
+			return kvm_do_memory_fault_exit(vcpu, fault);
+	}
 
 	if (fault->is_private)
 		return kvm_faultin_pfn_private(vcpu, fault);

@@ -4418,7 +4424,7 @@ static int __kvm_faultin_pfn(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault
 			trace_kvm_async_pf_repeated_fault(fault->addr, fault->gfn);
 			kvm_make_request(KVM_REQ_APF_HALT, vcpu);
 			return RET_PF_RETRY;
-		} else if (kvm_arch_setup_async_pf(vcpu, fault->addr, fault->gfn)) {
+		} else if (kvm_arch_setup_async_pf(vcpu, fault)) {
 			return RET_PF_RETRY;
 		}
 	}

@@ -5836,6 +5842,14 @@ int noinline kvm_mmu_page_fault(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa, u64 err
 	if (WARN_ON(!VALID_PAGE(vcpu->arch.mmu->root.hpa)))
 		return RET_PF_RETRY;
 
+	/*
+	 * This is racy with updating memory attributes with mmu_seq.  If we
+	 * hit a race, it would result in retrying page fault.
+	 */
+	if (vcpu->kvm->arch.vm_type == KVM_X86_SW_PROTECTED_VM &&
+	    kvm_mem_is_private(vcpu->kvm, gpa_to_gfn(cr2_or_gpa)))
+		error_code |= PFERR_GUEST_ENC_MASK;
+
 	r = RET_PF_INVALID;
 	if (unlikely(error_code & PFERR_RSVD_MASK)) {
 		r = handle_mmio_page_fault(vcpu, cr2_or_gpa, direct);

index 7f9ec1e5b136..3a423403af01 100644
@@ -295,13 +295,13 @@ static inline int kvm_mmu_do_page_fault(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa,
 		.user = err & PFERR_USER_MASK,
 		.prefetch = prefetch,
 		.is_tdp = likely(vcpu->arch.mmu->page_fault == kvm_tdp_page_fault),
+		.is_private = err & PFERR_GUEST_ENC_MASK,
 		.nx_huge_page_workaround_enabled =
 			is_nx_huge_page_enabled(vcpu->kvm),
 
 		.max_level = KVM_MAX_HUGEPAGE_LEVEL,
 		.req_level = PG_LEVEL_4K,
 		.goal_level = PG_LEVEL_4K,
-		.is_private = kvm_mem_is_private(vcpu->kvm, cr2_or_gpa >> PAGE_SHIFT),
 	};
 	int r;
 

2.25.1



-- 
Isaku Yamahata <[email protected]>

...

From: Isaku Yamahata <[email protected]>

TDX needs to know which mapping to operate on. Shared-EPT vs. Secure-EPT. The following sequence to convert the GPA to private doesn't work for TDX because the page can already be private.

1) Update memory attributes to private in memory attributes xarray
2) Zap the GPA range irrespective of private-or-shared. Even if the page is already private, zap the entry.
3) EPT violation on the GPA
4) Populate the GPA as private
The page is zeroed, and the guest has to accept the page again.

In step 2, TDX wants to zap only shared pages and skip private ones.

Add new members to strut kvm_gfn_range to indicate which mapping (private-vs-shared) to operate on. only_private and only_shared. Update mmu notifier, set memory attributes ioctl or KVM gmem callback to initialize them.

- If operating on a file to back shared pages, zap shared pages only. It's the mmu notifier.
(only_private, only_shared) = (false, true)

- If operating a file to back private pages, zap private pages only. It's the KVM gmem.
(only_private, only_shared) = (true, false)

- If setting memory attributes, vendor callback checks new attributes and make decisions.
SNP would do nothing and handle it later with gmem callback TDX callback would do as follows.
When it converts pages to shared, zap private pages only. When it converts pages to private, zap shared pages only. (only_private, only_shared) = (false, false)

- If operating on both backing files, zap both private and shared pages. This is when destructing guest.
(only_private, only_shared) = (true, true)

---
Changes v3 -> v4:
- rebased v11 kvm gmem.

Changes v2 -> v3:
- Drop the KVM_GFN_RANGE flags
- Updated struct kvm_gfn_range
- Change kvm_arch_set_memory_attributes() to return bool for flush
- Added set_memory_attributes x86 op for vendor backends
- Refined commit message to describe TDX care concretely

Changes v1 -> v2:
- consolidate KVM_GFN_RANGE_FLAGS_GMEM_{PUNCH_HOLE, RELEASE} into KVM_GFN_RANGE_FLAGS_GMEM.
- Update the commit message to describe TDX more. Drop SEV_SNP.

---
 include/linux/kvm_host.h | 2 ++
 virt/kvm/guest_mem.c     | 2 ++
 virt/kvm/kvm_main.c      | 4 ++++
 3 files changed, 8 insertions(+)

index 091bc89ae805..ce4d91585368 100644
@@ -268,6 +268,8 @@ struct kvm_gfn_range {
 		u64 raw;
 	} arg;
 	bool may_block;
+	bool only_private;
+	bool only_shared;
 };
 bool kvm_unmap_gfn_range(struct kvm *kvm, struct kvm_gfn_range *range);
 bool kvm_age_gfn(struct kvm *kvm, struct kvm_gfn_range *range);

index 384671a55b41..ac185c776cda 100644
@@ -105,6 +105,8 @@ static void kvm_gmem_invalidate_begin(struct kvm_gmem *gmem, pgoff_t start,
 			.end = slot->base_gfn + min(pgoff + slot->npages, end) - pgoff,
 			.slot = slot,
 			.may_block = true,
+			.only_private = true,
+			.only_shared = false,
 		};
 
 		flush |= kvm_mmu_unmap_gfn_range(kvm, &gfn_range);

index ee331cf8ba54..4e2a2463ab19 100644
@@ -603,6 +603,8 @@ static __always_inline int __kvm_handle_hva_range(struct kvm *kvm,
 			 */
 			gfn_range.arg.raw = range->arg.raw;
 			gfn_range.may_block = range->may_block;
+			gfn_range.only_private = false;
+			gfn_range.only_shared = true;
 
 			/*
 			 * {gfn(page) | page intersects with [hva_start, hva_end)} =

@@ -2405,6 +2407,8 @@ static __always_inline void kvm_handle_gfn_range(struct kvm *kvm,
 
 	gfn_range.arg.raw = range->arg.raw;
 	gfn_range.may_block = range->may_block;
+	gfn_range.only_private = false;
+	gfn_range.only_shared = false;
 
 	for (i = 0; i < kvm_arch_nr_memslot_as_ids(kvm); i++) {
 		slots = __kvm_memslots(kvm, i);

2.25.1

From: Brijesh Singh <[email protected]>

While resolving the RMP page fault, there may be cases where the page level between the RMP entry and TDP does not match and the 2M RMP entry must be split into 4K RMP entries. Or a 2M TDP page need to be broken into multiple of 4K pages.

To keep the RMP and TDP page level in sync, zap the gfn range after splitting the pages in the RMP entry. The zap should force the TDP to gets rebuilt with the new page level.

---
Changes v3 -> v4:
- removed redandunt blank line

Changes v2 -> v3:
- Newly added

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

index ab7d080bf544..e4f2938bb1fc 100644
@@ -1842,6 +1842,7 @@ void kvm_mmu_slot_leaf_clear_dirty(struct kvm *kvm,
 void kvm_mmu_zap_all(struct kvm *kvm);
 void kvm_mmu_invalidate_mmio_sptes(struct kvm *kvm, u64 gen);
 void kvm_mmu_change_mmu_pages(struct kvm *kvm, unsigned long kvm_nr_mmu_pages);
+void kvm_zap_gfn_range(struct kvm *kvm, gfn_t gfn_start, gfn_t gfn_end);
 
 int load_pdptrs(struct kvm_vcpu *vcpu, unsigned long cr3);
 

index 92d5a1924fc1..963c734642f6 100644
@@ -235,8 +235,6 @@ static inline u8 permission_fault(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu,
 	return -(u32)fault & errcode;
 }
 
-void kvm_zap_gfn_range(struct kvm *kvm, gfn_t gfn_start, gfn_t gfn_end);
-
 int kvm_arch_write_log_dirty(struct kvm_vcpu *vcpu);
 
 int kvm_mmu_post_init_vm(struct kvm *kvm);

index d2ebe26fb822..a73ddb43a2cf 100644
@@ -6759,6 +6759,7 @@ static bool kvm_mmu_zap_collapsible_spte(struct kvm *kvm,
 
 	return need_tlb_flush;
 }
+EXPORT_SYMBOL_GPL(kvm_zap_gfn_range);
 
 static void kvm_rmap_zap_collapsible_sptes(struct kvm *kvm,
 					   const struct kvm_memory_slot *slot)

2.25.1

From: Michael Roth <[email protected]>

All gmem pages are expected to be 'private' as defined by a particular arch/platform. Platforms like SEV-SNP require additional operations to move these pages into a private state, so implement a hook that can be used to prepare this memory prior to mapping it into a guest.

In the case of SEV-SNP, whether or not a 2MB page can be mapped via a 2MB mapping in the guest's nested page table depends on whether or not any subpages within the range have already been initialized as private in the RMP table, so this hook will also be used by the KVM MMU to clamp the maximum mapping size accordingly.

---
Changes v2 -> v3:
- Newly added

---
 arch/x86/include/asm/kvm-x86-ops.h |  1 +
 arch/x86/include/asm/kvm_host.h    |  3 +++
 arch/x86/kvm/mmu/mmu.c             | 12 ++++++++++--
 3 files changed, 14 insertions(+), 2 deletions(-)

index c0143906fe6d..a4cb248519cf 100644
@@ -134,6 +134,7 @@ KVM_X86_OP(msr_filter_changed)
 KVM_X86_OP(complete_emulated_msr)
 KVM_X86_OP(vcpu_deliver_sipi_vector)
 KVM_X86_OP_OPTIONAL_RET0(vcpu_get_apicv_inhibit_reasons);
+KVM_X86_OP_OPTIONAL_RET0(gmem_prepare)
 
 #undef KVM_X86_OP
 #undef KVM_X86_OP_OPTIONAL

index e4f2938bb1fc..de7f0dffa135 100644
@@ -1735,6 +1735,9 @@ struct kvm_x86_ops {
 	 * Returns vCPU specific APICv inhibit reasons
 	 */
 	unsigned long (*vcpu_get_apicv_inhibit_reasons)(struct kvm_vcpu *vcpu);
+
+	int (*gmem_prepare)(struct kvm *kvm, struct kvm_memory_slot *slot,
+			    kvm_pfn_t pfn, gfn_t gfn, u8 *max_level);
 };
 
 struct kvm_x86_nested_ops {

index a73ddb43a2cf..35bb14363828 100644
@@ -4352,6 +4352,7 @@ static int kvm_faultin_pfn_private(struct kvm_vcpu *vcpu,
 				   struct kvm_page_fault *fault)
 {
 	int max_order, r;
+	u8 max_level;
 
 	if (!kvm_slot_can_be_private(fault->slot))
 		return kvm_do_memory_fault_exit(vcpu, fault);

@@ -4361,8 +4362,15 @@ static int kvm_faultin_pfn_private(struct kvm_vcpu *vcpu,
 	if (r)
 		return r;
 
-	fault->max_level = min(kvm_max_level_for_order(max_order),
-			       fault->max_level);
+	max_level = kvm_max_level_for_order(max_order);
+	r = static_call(kvm_x86_gmem_prepare)(vcpu->kvm, fault->slot, fault->pfn,
+					      fault->gfn, &max_level);
+	if (r) {
+		kvm_release_pfn_clean(fault->pfn);
+		return r;
+	}
+
+	fault->max_level = min(max_level, fault->max_level);
 	fault->map_writable = !(fault->slot->flags & KVM_MEM_READONLY);
 	return RET_PF_CONTINUE;
 }

2.25.1

I'd originally added an arch hook in kvm_gmem_get_pfn() to handle RMP/HKID when the folio is allocated:

https://github.com/mdroth/linux/commit/adf78d224126f31e9096b80be21619e1ba447304

Based on your response it seemed like there was a preference to either:

a) do the RMP/HKID update via KVM MMU prior to mapping into the guest
b) implement an ioctl() to let userspace pre-allocate/pre-prepare and do the RMP updates in advance.

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

So this patch basically implements suggestion a), or at least my understanding of it. Is the original patch that does it via kvm_gmem_get_pfn() more in line with what you're suggesting here, or were you still thinking of an ioctl? Or some other place in the code? Keep in mind the GPA is needed to do the RMP updates so that prevents us from hooking into the more obvious place like kvm_gmem_get_folio().

Unfortunately I don't think things aren't quite that flexible with SNP. If RMP entry is 2MB, and you map a sub-page as 4K in the NPT, you'll immediately get a PFERR_GUEST_SIZEM on the first access (presumably when the guest tries to PVALIDATE it before use). The RMP fault handler will then subsequently need to PSMASH the 2MB entry into 4K before that guest can access it. So you get an extra page fault for every 2MB page that's mapped this way. (APM Volume 2, Section 15.36.10).

That might not be a big deal for guests that are at least somewhat optimized to make use of 2MB pages, but another situation is:

- gmem allocates 2MB page
- guest issues PVALIDATE on 2MB page
- guest later converts a subpage to shared but doesn't holepunch
- SNP host code issues PSMASH to split 2MB RMP mapping to 4K
- KVM MMU splits NPT mapping to 4K
- guest converts that shared page back to private

At this point there are no mixed attributes, and KVM would normally allow for 2MB NPT mappings again, but this is actually not allowed because the RMP table mappings are validated/4K and cannot be promoted on the hypervisor, so the NPT mappings must still be limited to 4K to match this, otherwise we hit the reverse of the PFERR_GUEST_SIZEM scenario above, where the NPT mapping level is *larger* than the RMP entry level. Unfortunately that does not result in a PFERR_GUEST_SIZEM where we can fix things up in response, but instead it's a general RMP fault that would be tricky to distinguish from an RMP fault resulting from an implicit page conversion or some other guest weirdness without doing RMP table checks every time we get a general RMP fault.

So for all intents and purposes it does sort of end up being the case that the mapping size and RMP entry size are sort of intertwined and can't totally be decoupled, and if you don't take that into account when updating the RMP entry, you'll have to do some extra PSMASH's in response to PFERR_GUEST_SIZEM RMP faults later.

Yah we'd still need a hook in roughly this same place for clamping max_level. Previous versions of SNP hypervisor patches all had a separate hook for handling these cases, but since the work of updating the RMP table prior to mapping isn't too dissimilar from the work of determining max mapping size, I combined both of them into the kvm_x86_gmem_prepare() hook/implementation.

But I don't see any major issue with moving RMPUPDATE handling to an allocation-time hook. As mentioned above we'd get additional PFERR_GUEST_SIZEM faults by not taking MMU mapping level into account, but I previously had it implemented similarly via a hook in kvm_gmem_get_pfn() (because we need the GPA) and didn't notice anything major. But I'm not sure exactly where you're suggesting we do it now, so could use some clarify on that if kvm_gmem_get_pfn() isn't what you had in mind.

WRT avoiding the RMP lookup, I'd tried __filemap_get_folio() without FGP_CREAT flag as a way to check whether the folio was already allocated or not, but it seemed to result in a lockup, and didn't look like it would be any better performance-wise than just doing the RMP lookup anyway (which is just a normal memory read), so that's the route I ended up going.

-Mike

Sorry for responding so late, lost track of this and only found it against when reviewing the next version :-/

Well that's just bloody stupid. Why is that a restriction? Obviously creating an NPT mapping that's larger would be annoying to handle, e.g. would require locking multiple entries or something, so I can understand why that's disallowed. But why can't software map at a finer granularity?

Note, I'm expecting a spec change, just expressing a bit of disbelief.

Anyways, IMO, we should eat the extra #NPF in that scenario and optimize for much, much more likely scenario of the RMP and NPT both being able to use 2MiB pages. And that means not inserting into the RMP when handling #NPFs, e.g. so that userspace can do fallocate() to prep the memory before mapping, and so that if the SPTEs get zapped for whatever reason, faulting them back in doesn't trigger useless RMP updates.

I guess the other way to optimze things would be for userspace to use the ioctl() to map memory into the guest[*]. But even then, initializing when allocating seems cleaner, especially for TDX.

[*] https://lore.kernel.org/all/[email protected]

This seems like a bug in the SNP code. (a) why would KVM/SNP PSMASH in that scenario and (b) why can't it zap/split the NPT before PSMASH?

I was thinking kvm_gmem_get_folio(). If userspace doesn't preallocate, then kvm_gmem_get_pfn() will still east the cost of the conversion, but it at least gives userspace the option and handles the zap case. Tracking which folios have been assigned (HKID or RMP) should be pretty straightforward.

I'm not totally opposed to doing more work at map time, e.g. to avoid faults or
to play nice with PSMASH, but I would rather that not bleed into gmem. And I
think/hope if we hook kvm_gmem_get_folio(), then TDX and SNP can use that as a common base, i.e. whatever extra shenanigans are needed for SNP can be carried in the SNP series. For me, having such "quirks" in the vendor specific series would be quite helpful as I'm having a hell of a time keeping track of all the wrinkles of TDX and SNP.

I asked David Kaplan about this and one of the reasons is that it allows hardware to reduce the number of RMP table accesses when doing table walks. E.g. if the mapping is 4K in the NPT, then only that RMP entry needs to be checked, the hardware doesn't need to also then check the 2MB-aligned RMP entry before determining whether to generate a #NPF.

(note: the 'assigned' bit does get set on each individual 4K entry when RMPUPDATE use the 'page-size' bit to create a 2MB RMP entry, but if you look at the pseudo-code in the APM for RMPUPDATE it actually leaves ASID=0, so those accesses always generate an #NPF(RMP) and aren't actually usable for 4K NPT mappings of sub-pages for the 2MB range corresponding to that 2MB RMP entry (and not #NPF+RMP+SIZEM as I incorrectly stated above))

That seems fair, I only see this particularly case occur once or twice for boot.

Makes sense.

We wouldn't issue an RMPUPDATE if we saw that the page was already set to private in the RMP table. So what you actually save on is just accesses to the memory range containing the RMP entry to do that check, but the approach still makes sense.

a) A PSMASH will be needed at some point, since, as detailed above, the 4K NPT mapping requires the RMP entries for the pages it maps to be limited to 4K RMP entries, but...
b) What would happen normally[1] is the guest would issue PVALIDATE to *rescind* the validated status of that 4K GPA before issuing the GHCB request to convert it to shared. This would cause an #NPF(RMP+SIZE_MISMATCH) and handle_rmp_page_fault() would PSMASH the RMP entry so the PVALIDATE can succeed.

So KVM doesn't even really have the option of deferring the PSMASH, it happens before the SET_MEMORY_ATTRIBUTES is even issued to zap the 2MB NPT mapping and switch the 2M range to 'mixed'. Because of that, we also need a hook in the KVM MMU code to clamp the max mapping level based on RMP entry size. Currently the kvm_gmem_prepare() in this patch doubles for handling that clamping, so we would still need a similar hook for that if we move the RMP initialization stuff to allocation time.

[1] This handling is recommended for 'well-behaved' guests according to GHCB, but I don't see it documented as a hard requirement anywhere, so there is a possibility that that we have to deal with a guest that doesn't do this. What would happen then is the PVALIDATE wouldn't trigger the #NPF(RMP+SIZEM), and instead the SET_MEMORY_ATTRIBUTES would zap the 2MB mapping, install 4K entries on next #NPF(NOT_PRESENT), and at *that* point we would get an #NPF(RMP) without the SIZEM bit set, due to the behavior described in the beginning of this email.

handle_rmp_page_fault() can do the corresponding PSMASH to deal with that, but it is a little unfortunate since we can't differentiate that case from a spurious/unexpected RMP faults, so would need to attempt a PSMASH in all cases, sometimes failing.

gmem itself could also trigger this case if the lpage_info_slot() tracking ever became more granular than what the guest was expected (which I don't think would happen normally, but I may have hit one case where it does, but haven't had a chance to debug if that's on the lpage_info_slot() side or something else on the SNP end.

This is a little tricky. kvm_gmem_get_pfn() can handle RMP entry initialization, no problem, because it has the GPA we are fetching the PFN for.

But kvm_gmem_get_folio() doesn't. But as long as the preallocation happens after kvm_gmem_bind() and before kvm_gmem_unbind() we should be able to use the slot to figure out what GPA a particular FD offset corresponds to. I'll look into this more.

Seems reasonable. I'll work on re-working the hooks using this approach.

Thanks,

Mike

The spurious case here is when the guest is accessing a private page that's just been PSMASH'd by another thread. I thought these might still occur before the PSMASH has completed so we'd still potentially see the page-size bit set in the RMP entry, but the RMP faults only happen after the PSMASH has finished, so the spurious cases can be filtered out by just checking if the page-size bit is set before attempting a PSMASH.

Turns out that was for a case where the shared/private attributes for the 2M range really were mixed at the time of access. In this case it is when OVMF converts some shared memory to private during early boot: the end address is not 2MB-aligned, so it is in a mixed region, and the NPT mapping is 4K, but the RMP entry is initialized as 2MB. In this case the PVALIDATE and NPT agree on the 4K mapping size so the SIZEM bit isn't set, just the #NPF(RMP).

So we need to be able to deal with that even for 'well-behaved' guests. With RMP-init-during-mapping-time approach I had some checks that avoided creating the 2MB RMP entry in this mixed case which is why I didn't need handling for this previously. But it's just one extra #NPF(RMP) and can be handled cleanly since it can be distinguished from spurious cases.

-Mike

Just to make sure you're not waiting on me for something, the TL;DR is that my suggestion is viable and not too horrific?

From: Michael Roth <[email protected]>

TODO: add a CONFIG option that can be to completely skip arch invalidation loop and avoid __weak references for arch/platforms that don't need an additional invalidation hook.

In some cases, like with SEV-SNP, guest memory needs to be updated in a platform-specific manner before it can be safely freed back to the host. Add hooks to wire up handling of this sort when freeing memory in response to FALLOC_FL_PUNCH_HOLE operations.

Also issue invalidations of all allocated pages when releasing the gmem file so that the pages are not left in an unusable state when they get freed back to the host.

---
Changes v2 -> v3:
- Newly added

---
 arch/x86/include/asm/kvm-x86-ops.h |  1 +
 arch/x86/include/asm/kvm_host.h    |  1 +
 arch/x86/kvm/x86.c                 |  6 +++++
 include/linux/kvm_host.h           |  3 +++
 virt/kvm/guest_mem.c               | 42 ++++++++++++++++++++++++++++++
 5 files changed, 53 insertions(+)

index a4cb248519cf..d520c6370cd6 100644
@@ -135,6 +135,7 @@ KVM_X86_OP(complete_emulated_msr)
 KVM_X86_OP(vcpu_deliver_sipi_vector)
 KVM_X86_OP_OPTIONAL_RET0(vcpu_get_apicv_inhibit_reasons);
 KVM_X86_OP_OPTIONAL_RET0(gmem_prepare)
+KVM_X86_OP_OPTIONAL(gmem_invalidate)
 
 #undef KVM_X86_OP
 #undef KVM_X86_OP_OPTIONAL

index de7f0dffa135..440a4a13a93f 100644
@@ -1738,6 +1738,7 @@ struct kvm_x86_ops {
 
 	int (*gmem_prepare)(struct kvm *kvm, struct kvm_memory_slot *slot,
 			    kvm_pfn_t pfn, gfn_t gfn, u8 *max_level);
+	void (*gmem_invalidate)(struct kvm *kvm, kvm_pfn_t start, kvm_pfn_t end);
 };
 
 struct kvm_x86_nested_ops {

index fd6c05d1883c..2ae40fa8e178 100644
@@ -13284,6 +13284,12 @@ bool kvm_arch_no_poll(struct kvm_vcpu *vcpu)
 }
 EXPORT_SYMBOL_GPL(kvm_arch_no_poll);
 
+#ifdef CONFIG_KVM_PRIVATE_MEM
+void kvm_arch_gmem_invalidate(struct kvm *kvm, kvm_pfn_t start, kvm_pfn_t end)
+{
+	static_call_cond(kvm_x86_gmem_invalidate)(kvm, start, end);
+}
+#endif
 
 int kvm_spec_ctrl_test_value(u64 value)
 {

index ce4d91585368..6c5d39e429e9 100644
@@ -2360,6 +2360,7 @@ static inline bool kvm_mem_is_private(struct kvm *kvm, gfn_t gfn)
 #ifdef CONFIG_KVM_PRIVATE_MEM
 int kvm_gmem_get_pfn(struct kvm *kvm, struct kvm_memory_slot *slot,
 			      gfn_t gfn, kvm_pfn_t *pfn, int *max_order);
+void kvm_arch_gmem_invalidate(struct kvm *kvm, kvm_pfn_t start, kvm_pfn_t end);
 #else
 static inline int kvm_gmem_get_pfn(struct kvm *kvm,
 				   struct kvm_memory_slot *slot, gfn_t gfn,

@@ -2368,6 +2369,8 @@ static inline int kvm_gmem_get_pfn(struct kvm *kvm,
 	KVM_BUG_ON(1, kvm);
 	return -EIO;
 }
+
+void kvm_arch_gmem_invalidate(struct kvm *kvm, kvm_pfn_t start, kvm_pfn_t end) { }
 #endif /* CONFIG_KVM_PRIVATE_MEM */
 
 #endif

2.25.1

From: Isaku Yamahata <[email protected]>

TDX KVM will use KVM_MEM_ENC_OP. Make struct sev_cmd common both for vendor backends, SEV and TDX, with rename. Make the struct common uABI for KVM_MEM_ENC_OP. TDX backend wants to return 64 bit error code instead of 32 bit. To keep ABI for SEV backend, use union to accommodate 64 bit member. Opportunistically make the implicit padding after id member to explicit flags member for future use and clarity.

Some data structures for sub-commands could be common. The current candidate would be KVM_SEV{,_ES}_INIT, KVM_SEV_LAUNCH_FINISH, KVM_SEV_LAUNCH_UPDATE_VMSA, KVM_SEV_DBG_DECRYPT, and KVM_SEV_DBG_ENCRYPT.

Only compile tested for SEV code.

---
Changes v3 -> v4:
- newly added

---
 arch/x86/include/asm/kvm_host.h |  2 +-
 arch/x86/include/uapi/asm/kvm.h | 33 ++++++++++++++++
 arch/x86/kvm/svm/sev.c          | 68 ++++++++++++++++++---------------
 arch/x86/kvm/svm/svm.h          |  2 +-
 arch/x86/kvm/x86.c              | 16 +++++++-
 5 files changed, 87 insertions(+), 34 deletions(-)

index 440a4a13a93f..5ede982442a0 100644
@@ -1710,7 +1710,7 @@ struct kvm_x86_ops {
 	void (*enable_smi_window)(struct kvm_vcpu *vcpu);
 #endif
 
-	int (*mem_enc_ioctl)(struct kvm *kvm, void __user *argp);
+	int (*mem_enc_ioctl)(struct kvm *kvm, struct kvm_mem_enc_cmd *cmd);
 	int (*mem_enc_register_region)(struct kvm *kvm, struct kvm_enc_region *argp);
 	int (*mem_enc_unregister_region)(struct kvm *kvm, struct kvm_enc_region *argp);
 	int (*vm_copy_enc_context_from)(struct kvm *kvm, unsigned int source_fd);

index aa7a56a47564..32883e520b00 100644
@@ -562,6 +562,39 @@ struct kvm_pmu_event_filter {
 /* x86-specific KVM_EXIT_HYPERCALL flags. */
 #define KVM_EXIT_HYPERCALL_LONG_MODE	BIT(0)
 
+struct kvm_mem_enc_cmd {
+	/* sub-command id of KVM_MEM_ENC_OP. */
+	__u32 id;
+	/*
+	 * Auxiliary flags for sub-command.  If sub-command doesn't use it,
+	 * set zero.
+	 */
+	__u32 flags;
+	/*
+	 * Data for sub-command.  An immediate or a pointer to the actual
+	 * data in process virtual address.  If sub-command doesn't use it,
+	 * set zero.
+	 */
+	__u64 data;
+	/*
+	 * Supplemental error code in the case of error.
+	 * SEV error code from the PSP or TDX SEAMCALL status code.
+	 * The caller should set zero.
+	 */
+	union {
+		struct {
+			__u32 error;
+			/*
+			 * KVM_SEV_LAUNCH_START and KVM_SEV_RECEIVE_START
+			 * require extra data. Not included in struct
+			 * kvm_sev_launch_start or struct kvm_sev_receive_start.
+			 */
+			__u32 sev_fd;
+		};
+		__u64 error64;
+	};
+};
+
 #define KVM_X86_DEFAULT_VM	0
 #define KVM_X86_SW_PROTECTED_VM	1
 #define KVM_X86_TDX_VM		2

index 18af7e712a5a..74ecab20c24b 100644
@@ -716,7 +716,7 @@ void avic_refresh_virtual_apic_mode(struct kvm_vcpu *vcpu);
 extern unsigned int max_sev_asid;
 
 void sev_vm_destroy(struct kvm *kvm);
-int sev_mem_enc_ioctl(struct kvm *kvm, void __user *argp);
+int sev_mem_enc_ioctl(struct kvm *kvm, struct kvm_mem_enc_cmd *cmd);
 int sev_mem_enc_register_region(struct kvm *kvm,
 				struct kvm_enc_region *range);
 int sev_mem_enc_unregister_region(struct kvm *kvm,

index 2ae40fa8e178..ab36e8940f1b 100644
@@ -7040,11 +7040,25 @@ int kvm_arch_vm_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg)
 		goto out;
 	}
 	case KVM_MEMORY_ENCRYPT_OP: {
+		struct kvm_mem_enc_cmd cmd;
+
 		r = -ENOTTY;
 		if (!kvm_x86_ops.mem_enc_ioctl)
 			goto out;
 
-		r = static_call(kvm_x86_mem_enc_ioctl)(kvm, argp);
+		if (!argp) {
+			r = 0;
+			goto out;
+		}
+
+		if (copy_from_user(&cmd, argp, sizeof(cmd))) {
+			r = -EFAULT;
+			goto out;
+		}
+		r = static_call(kvm_x86_mem_enc_ioctl)(kvm, &cmd);
+		if (copy_to_user(argp, &cmd, sizeof(cmd)))
+			r = -EFAULT;
+
 		break;
 	}
 	case KVM_MEMORY_ENCRYPT_REG_REGION: {

2.25.1

Ok, let's drop this patch. Keep the ABI different for now.

Isaku Yamahata <[email protected]>

generalizing the struct of KVM_MEM_ENC_OP should be the first step. The final target should be generalizing a set of commands for confidential VMs (SEV-* VMs and TDs, maybe even for other arches), e.g., the commands to create a confidential VM and commands to live migration a confidential VM.

However, there seems not small divergence between the commands to create a SEV-* VM and TDX VMs. I'm not sure if it is worth investigating and pursuing.

It's not just the one structure though. The "data" field is a pointer to yet another layer of commands, and SEV has a rather big pile of those. Making kvm_mem_enc_cmd common is just putting lipstick on a pig since the vast majority of the structures associated with the ioctl() would still be vendor specific.

struct kvm_sev_launch_start
struct kvm_sev_launch_update_data
struct kvm_sev_launch_secret
struct kvm_sev_launch_measure
struct kvm_sev_guest_status
struct kvm_sev_dbg
struct kvm_sev_attestation_report
struct kvm_sev_send_start
struct kvm_sev_send_update_data
struct kvm_sev_receive_start
struct kvm_sev_receive_update_data

FWIW, I really dislike KVM's uAPI for KVM_MEM_ENC_OP. The above structures are all basically copied verbatim from PSP firmware structures, i.e. the commands and their payloads are tightly coupled to "hardware" and essentially have no abstraction
whatsoever. But that ship has already sailed, and practically speaking trying to
provide a layer of abstraction might not of worked very well anyways.

In other words, unless there's an obvious and easy way path to convergence, I recommend you don't spend much time/effort on trying to share code with TDX.

I think we can easily unify vcpu initialization, populating/measure initial memory, completing guest creation, and guest memory access for debug.

KVM_SEV_LAUNCH_UPDATE_VMSA <-> KVM_TDX_INIT_VCPU
KVM_SEV_LAUNCH_UPDATE_DATA and KVM_SEV_LAUNCH_MEASURE <-> KVM_INIT_MEM_REGION KVM_SEV_LAUNCH_FINISH <-> KVM_TDX_FINALIZE_VM
KVM_SEV_DBG_DECRYPT, KVM_SEV_DBG_ENCRYPT: KVM common API for access protected guest memory

Here's my assessment. For now I don't address migration.

For creating confidential guest:

- Get the capability of underlying platform
KVM_TDX_CAPABILITY: no sev correspondence.

- Initialize VM as confidential VM
struct kvm_sev_launch_start
KVM_SEV{,_ES}_INIT, and KVM_SEV_LAUNCH_START:
KVM_TDX_INIT_VM
They take vendor specific data.

- Initialize vcpu
KVM_SEV_LAUNCH_UPDATE_VMSA: no extra argument
KVM_TDX_INIT_VCPU: no extra argument

- populate initial memory + measurement KVM_SEV_LAUNCH_UPDATE_DATA and KVM_SEV_LAUNCH_MEASURE,

  struct kvm_sev_launch_update_data {
        __u64 uaddr;
        __u32 len;
  };
  struct kvm_sev_launch_measure {
        __u64 uaddr;
        __u32 len;

};
=> GPA is calculated from uaddr.

  KVM_INIT_MEM_REGION:
  struct kvm_tdx_init_mem_region {
        __u64 source_addr;      // uaddr
        __u64 gpa;
        __u64 nr_pages;
  };

  I think those can same structure. Or prefault or prepopulating
  e.g.
  struct {
        __u64 uaddr;
        __u64 gpa;
        __u64 len;
  #define FLAG_MEASURE    BIT(0)
  #define FLAG_GPA        BIT(1)  // GPA is valid or calculated from uaddr
        __u64 flags;
  };

- Complete initialization. Make the guest ready to run vcpu
KVM_SEV_LAUNCH_FINISH: no argument
KVM_TDX_FINALIZE_VM: no argument

- KVM_SEV_LAUNCH_SECRET: no TDX correspondence
struct kvm_sev_launch_secret

For guest debug

- KVM_SEV_DBG_DECRYPT, KVM_SEV_DBG_ENCRYPT: struct kvm_sev_dbg This is to read/write guest memory for debug. We can easily have a common API.

- KVM_SEV_GUEST_STATUS
struct kvm_sev_guest_status
No TDX correspondence

Thanks,

Isaku Yamahata <[email protected]>

From: Isaku Yamahata <[email protected]>

This breaks uABI as the current code doesn't check padding and sev_fd when unused.

---
Changes v3 -> v4:
- newly added

---
 arch/x86/kvm/x86.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

index ab36e8940f1b..1d6085af6a00 100644
@@ -7055,6 +7055,22 @@ int kvm_arch_vm_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg)
 			r = -EFAULT;
 			goto out;
 		}
+		/* No sub-command uses flags at the moment. */
+		if (cmd.flags) {
+			r = -EINVAL;
+			goto out;
+		}
+		if (cmd.id != KVM_SEV_LAUNCH_START &&
+		    cmd.id != KVM_SEV_RECEIVE_START && cmd.error64) {
+			r = -EINVAL;
+			goto out;
+		}
+		if ((cmd.id == KVM_SEV_LAUNCH_START ||
+		     cmd.id == KVM_SEV_RECEIVE_START) && cmd.error) {
+			r = -EINVAL;
+			goto out;
+		}
+
 		r = static_call(kvm_x86_mem_enc_ioctl)(kvm, &cmd);
 		if (copy_to_user(argp, &cmd, sizeof(cmd)))
 			r = -EFAULT;

2.25.1