Add AMD Secure Nested Paging (SEV-SNP) Hypervisor Support

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.

Signed-off-by Michael Roth <[email protected]>

---
 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               | 48 ++++++++++++++++++++++++++++--
 5 files changed, 57 insertions(+), 2 deletions(-)

ld: arch/x86/kvm/../../../virt/kvm/eventfd.o: in function `kvm_arch_gmem_invalidate':
/home/boris/kernel/2nd/linux/./include/linux/kvm_host.h:2356: multiple definition of `kvm_arch_gmem_invalidate'; arch/x86/kvm/../../../virt/kvm/kvm_main.o:/home/boris/kernel/2nd/linux/./include/linux/kvm_host.h:2356: first defined here
ld: arch/x86/kvm/../../../virt/kvm/binary_stats.o: in function `kvm_arch_gmem_invalidate':

The upper bits will be needed in some cases to distinguish between nested page faults for private/shared pages, so pass along the full 64-bit value.

Signed-off-by Michael Roth <[email protected]>

---
 arch/x86/kvm/mmu/mmu.c          | 3 +--
 arch/x86/kvm/mmu/mmu_internal.h | 4 ++--
 2 files changed, 3 insertions(+), 4 deletions(-)

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

index c54672ad6cbc..0d3983b9aa7e 100644
@@ -5829,8 +5829,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;

arch/x86/kvm/mmu/mmu_internal.h +2 -2

index f1786698ae00..780b91e1da9f 100644
@@ -283,11 +283,11 @@ 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,
-		.error_code = err,
+		.error_code = lower_32_bits(err),
 		.exec = err & PFERR_FETCH_MASK,
 		.write = err & PFERR_WRITE_MASK,
 		.present = err & PFERR_PRESENT_MASK,

Signature · 1 line

2.25.1

This will be used to determine whether or not an #NPF should be serviced using a normal page vs. a guarded/gmem one.

Signed-off-by Michael Roth <[email protected]>

---
 arch/x86/include/asm/kvm_host.h |  7 +++++++
 arch/x86/kvm/mmu/mmu_internal.h | 35 ++++++++++++++++++++++++++++++++-
 2 files changed, 41 insertions(+), 1 deletion(-)

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

index b3bd24f2a390..c26f76641121 100644
@@ -1445,6 +1445,13 @@ struct kvm_arch {
 	 */
 #define SPLIT_DESC_CACHE_MIN_NR_OBJECTS (SPTE_ENT_PER_PAGE + 1)
 	struct kvm_mmu_memory_cache split_desc_cache;
+
+	/*
+	 * When set, used to determine whether a fault should be treated as
+	 * private in the context of protected VMs which use a separate gmem
+	 * pool to back private guest pages.
+	 */
+	u64 mmu_private_fault_mask;
 };
 
 struct kvm_vm_stat {

arch/x86/kvm/mmu/mmu_internal.h +11 -0

index 780b91e1da9f..9b9e75aa43f4 100644
@@ -252,6 +252,39 @@ struct kvm_page_fault {
 
 int kvm_tdp_page_fault(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault);
 
+static bool kvm_mmu_fault_is_private(struct kvm *kvm, gpa_t gpa, u64 err)
+{
+	struct kvm_memory_slot *slot;
+	bool private_fault = false;
+	gfn_t gfn = gpa_to_gfn(gpa);
+
+	slot = gfn_to_memslot(kvm, gfn);
+	if (!slot) {
+		pr_debug("%s: no slot, GFN: 0x%llx\n", __func__, gfn);
+		goto out;
+	}

arch/x86/kvm/mmu/mmu_internal.h +7 -1

+
+out:
+	pr_debug("%s: GFN: 0x%llx, private: %d\n", __func__, gfn, private_fault);
+	return private_fault;
+}
+
 /*
  * Return values of handle_mmio_page_fault(), mmu.page_fault(), fast_page_fault(),
  * and of course kvm_mmu_do_page_fault().

@@ -301,7 +334,7 @@ static inline int kvm_mmu_do_page_fault(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa,
 		.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),
+		.is_private = kvm_mmu_fault_is_private(vcpu->kvm, cr2_or_gpa, err),
 	};
 	int r;
 

Signature · 1 line

2.25.1

Quoted 46 lines · Michael Roth

This will be used to determine whether or not an #NPF should be serviced using a normal page vs. a guarded/gmem one.

Signed-off-by Michael Roth <[email protected]>

---
 arch/x86/include/asm/kvm_host.h |  7 +++++++
 arch/x86/kvm/mmu/mmu_internal.h | 35 ++++++++++++++++++++++++++++++++-
 2 files changed, 41 insertions(+), 1 deletion(-)

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

index b3bd24f2a390..c26f76641121 100644
@@ -1445,6 +1445,13 @@ struct kvm_arch {
 	 */
 #define SPLIT_DESC_CACHE_MIN_NR_OBJECTS (SPTE_ENT_PER_PAGE + 1)
 	struct kvm_mmu_memory_cache split_desc_cache;
+
+	/*
+	 * When set, used to determine whether a fault should be treated as
+	 * private in the context of protected VMs which use a separate gmem
+	 * pool to back private guest pages.
+	 */
+	u64 mmu_private_fault_mask;
 };
 
 struct kvm_vm_stat {

arch/x86/kvm/mmu/mmu_internal.h +11 -0

index 780b91e1da9f..9b9e75aa43f4 100644
@@ -252,6 +252,39 @@ struct kvm_page_fault {
 
 int kvm_tdp_page_fault(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault);
 
+static bool kvm_mmu_fault_is_private(struct kvm *kvm, gpa_t gpa, u64 err)
+{
+	struct kvm_memory_slot *slot;
+	bool private_fault = false;
+	gfn_t gfn = gpa_to_gfn(gpa);
+
+	slot = gfn_to_memslot(kvm, gfn);
+	if (!slot) {
+		pr_debug("%s: no slot, GFN: 0x%llx\n", __func__, gfn);
+		goto out;
+	}

arch/x86/kvm/mmu/mmu_internal.h struct kvm_page_fault { · line 275

Quoted 10 lines

arch/x86/kvm/mmu/mmu_internal.h +10 -0

+
+	if (!kvm_slot_can_be_private(slot)) {
+		pr_debug("%s: slot is not private, GFN: 0x%llx\n", __func__, gfn);
+		goto out;
+	}
+
+	if (kvm->arch.mmu_private_fault_mask) {
+		private_fault = !!(err & kvm->arch.mmu_private_fault_mask);
+		goto out;
+	}

What's the convention of err? Can we abstract it by introducing a new bit PFERR_PRIVATE_MASK? The caller sets it based on arch specific value. the logic will be

        .is_private = err & PFERR_PRIVATE_MASK;

arch/x86/kvm/mmu/mmu_internal.h struct kvm_page_fault { · line 281

Quoted 8 lines

arch/x86/kvm/mmu/mmu_internal.h +6 -0

+
+	/*
+	 * Handling below is for UPM self-tests and guests that treat userspace
+	 * as the authority on whether a fault should be private or not.
+	 */
+	private_fault = kvm_mem_is_private(kvm, gpa >> PAGE_SHIFT);

This code path is sad. One extra slot lookup and xarray look up. Without mmu lock, the result can change by other vcpu. Let's find a better way.

Quoted 22 lines

arch/x86/kvm/mmu/mmu_internal.h +7 -1

+
+out:
+	pr_debug("%s: GFN: 0x%llx, private: %d\n", __func__, gfn, private_fault);
+	return private_fault;
+}
+
 /*
  * Return values of handle_mmio_page_fault(), mmu.page_fault(), fast_page_fault(),
  * and of course kvm_mmu_do_page_fault().

@@ -301,7 +334,7 @@ static inline int kvm_mmu_do_page_fault(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa,
 		.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),
+		.is_private = kvm_mmu_fault_is_private(vcpu->kvm, cr2_or_gpa, err),
 	};
 	int r;
 

Signature · 1 line

2.25.1

Signature · 1 line

Isaku Yamahata <[email protected]>

Quoted 63 lines · Isaku Yamahata

Quoted 46 lines · Michael Roth

This will be used to determine whether or not an #NPF should be serviced using a normal page vs. a guarded/gmem one.

Signed-off-by Michael Roth <[email protected]>

---
 arch/x86/include/asm/kvm_host.h |  7 +++++++
 arch/x86/kvm/mmu/mmu_internal.h | 35 ++++++++++++++++++++++++++++++++-
 2 files changed, 41 insertions(+), 1 deletion(-)

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

index b3bd24f2a390..c26f76641121 100644
@@ -1445,6 +1445,13 @@ struct kvm_arch {
 	 */
 #define SPLIT_DESC_CACHE_MIN_NR_OBJECTS (SPTE_ENT_PER_PAGE + 1)
 	struct kvm_mmu_memory_cache split_desc_cache;
+
+	/*
+	 * When set, used to determine whether a fault should be treated as
+	 * private in the context of protected VMs which use a separate gmem
+	 * pool to back private guest pages.
+	 */
+	u64 mmu_private_fault_mask;
 };
 
 struct kvm_vm_stat {

arch/x86/kvm/mmu/mmu_internal.h +11 -0

index 780b91e1da9f..9b9e75aa43f4 100644
@@ -252,6 +252,39 @@ struct kvm_page_fault {
 
 int kvm_tdp_page_fault(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault);
 
+static bool kvm_mmu_fault_is_private(struct kvm *kvm, gpa_t gpa, u64 err)
+{
+	struct kvm_memory_slot *slot;
+	bool private_fault = false;
+	gfn_t gfn = gpa_to_gfn(gpa);
+
+	slot = gfn_to_memslot(kvm, gfn);
+	if (!slot) {
+		pr_debug("%s: no slot, GFN: 0x%llx\n", __func__, gfn);
+		goto out;
+	}

arch/x86/kvm/mmu/mmu_internal.h struct kvm_page_fault { · line 275

Quoted 10 lines

arch/x86/kvm/mmu/mmu_internal.h +10 -0

+
+	if (!kvm_slot_can_be_private(slot)) {
+		pr_debug("%s: slot is not private, GFN: 0x%llx\n", __func__, gfn);
+		goto out;
+	}
+
+	if (kvm->arch.mmu_private_fault_mask) {
+		private_fault = !!(err & kvm->arch.mmu_private_fault_mask);
+		goto out;
+	}

What's the convention of err? Can we abstract it by introducing a new bit PFERR_PRIVATE_MASK? The caller sets it based on arch specific value. the logic will be

Quoted 1 line

        .is_private = err & PFERR_PRIVATE_MASK;

I'm not sure I understand the question. 'err' is just the page fault flags, and arch.mmu_private_fault_mask is something that can be set on a per-platform basis when running in a mode where shared/private access is recorded in the page fault flags during a #NPF.

I'm not sure how we'd keep the handling cross-platform by moving to a macro, since TDX uses a different bit, and we'd want to be able to build a SNP+TDX kernel that could run on either type of hardware.

Are you suggesting to reverse that and have err be set in a platform-specific way and then use a common PFERR_PRIVATE_MASK that's software-defined and consistent across platforms? That could work, but existing handling seems to use page fault flags as-is, keeping the hardware-set values, rather than modifying them to pass additional metadata, so it seems like it might make things more confusing to make an exception to that here. Or are there other cases where it's done that way?

Quoted 10 lines

arch/x86/kvm/mmu/mmu_internal.h struct kvm_page_fault { · line 281

Quoted 6 lines

arch/x86/kvm/mmu/mmu_internal.h +6 -0

+
+	/*
+	 * Handling below is for UPM self-tests and guests that treat userspace
+	 * as the authority on whether a fault should be private or not.
+	 */
+	private_fault = kvm_mem_is_private(kvm, gpa >> PAGE_SHIFT);

This code path is sad. One extra slot lookup and xarray look up. Without mmu lock, the result can change by other vcpu. Let's find a better way.

The intention was to rely on fault->mmu_seq to determine if a KVM_SET_MEMORY_ATTRIBUTES update came in after .private_fault was set so that fault handling could be retried, but that doesn't happen until kvm_faultin_pfn() which is *after* this is logged. So yes, I think there is a race here, and the approach you took in your Misc. series of keeping the kvm_mem_is_private() check inside kvm_faultin_pfn() is more efficient/correct.

If we can figure out a way to handle checking the fault flags in a way that works for both TDX/SNP (and KVM self-test use-case) we can consolidate around that.

Quoted 75 lines · Michael Roth

Quoted 63 lines · Isaku Yamahata

Quoted 46 lines · Michael Roth

This will be used to determine whether or not an #NPF should be serviced using a normal page vs. a guarded/gmem one.

Signed-off-by Michael Roth <[email protected]>

---
 arch/x86/include/asm/kvm_host.h |  7 +++++++
 arch/x86/kvm/mmu/mmu_internal.h | 35 ++++++++++++++++++++++++++++++++-
 2 files changed, 41 insertions(+), 1 deletion(-)

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

index b3bd24f2a390..c26f76641121 100644
@@ -1445,6 +1445,13 @@ struct kvm_arch {
 	 */
 #define SPLIT_DESC_CACHE_MIN_NR_OBJECTS (SPTE_ENT_PER_PAGE + 1)
 	struct kvm_mmu_memory_cache split_desc_cache;
+
+	/*
+	 * When set, used to determine whether a fault should be treated as
+	 * private in the context of protected VMs which use a separate gmem
+	 * pool to back private guest pages.
+	 */
+	u64 mmu_private_fault_mask;
 };
 
 struct kvm_vm_stat {

arch/x86/kvm/mmu/mmu_internal.h +11 -0

index 780b91e1da9f..9b9e75aa43f4 100644
@@ -252,6 +252,39 @@ struct kvm_page_fault {
 
 int kvm_tdp_page_fault(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault);
 
+static bool kvm_mmu_fault_is_private(struct kvm *kvm, gpa_t gpa, u64 err)
+{
+	struct kvm_memory_slot *slot;
+	bool private_fault = false;
+	gfn_t gfn = gpa_to_gfn(gpa);
+
+	slot = gfn_to_memslot(kvm, gfn);
+	if (!slot) {
+		pr_debug("%s: no slot, GFN: 0x%llx\n", __func__, gfn);
+		goto out;
+	}

arch/x86/kvm/mmu/mmu_internal.h struct kvm_page_fault { · line 275

Quoted 10 lines

arch/x86/kvm/mmu/mmu_internal.h +10 -0

+
+	if (!kvm_slot_can_be_private(slot)) {
+		pr_debug("%s: slot is not private, GFN: 0x%llx\n", __func__, gfn);
+		goto out;
+	}
+
+	if (kvm->arch.mmu_private_fault_mask) {
+		private_fault = !!(err & kvm->arch.mmu_private_fault_mask);
+		goto out;
+	}

What's the convention of err? Can we abstract it by introducing a new bit PFERR_PRIVATE_MASK? The caller sets it based on arch specific value. the logic will be

Quoted 1 line

        .is_private = err & PFERR_PRIVATE_MASK;

I'm not sure I understand the question. 'err' is just the page fault flags, and arch.mmu_private_fault_mask is something that can be set on a per-platform basis when running in a mode where shared/private access is recorded in the page fault flags during a #NPF.

I'm not sure how we'd keep the handling cross-platform by moving to a macro, since TDX uses a different bit, and we'd want to be able to build a SNP+TDX kernel that could run on either type of hardware.

Quoted 7 lines

Are you suggesting to reverse that and have err be set in a platform-specific way and then use a common PFERR_PRIVATE_MASK that's software-defined and consistent across platforms? That could work, but existing handling seems to use page fault flags as-is, keeping the hardware-set values, rather than modifying them to pass additional metadata, so it seems like it might make things more confusing to make an exception to that here. Or are there other cases where it's done that way?

I meant the latter, making PFERR_PRIVATE_MASK common software-defined.

I think the SVM fault handler can use hardware value directly by carefully defining those PFERR values.

TDX doesn't have architectural bit in error code to indicate the private fault. It's coded in faulted address as shared bit. GPA bit 51 or 47. PFERR_{USER, WRITE, FETCH, PRESENT} are already software-defined value for VMX (and TDX). The fault handler for VMX, handle_ept_violation(), converts encoding. For TDX, PFERR_PRIVATE_MASK is just one more software defined bit.

I'm fine with either way, variable or macro. Which do you prefer?

- Define variable mmu_private_fault_mask (global or per struct kvm) The module initialization code, hardware_setup(), sets mmu_private_fault_mask.
- Define the software defined value, PFERR_PRIVATE_MASK.
The caller of kvm_mmu_page_fault() parses the hardware value and construct software defined error_code.
- any other?

Quoted 19 lines

Quoted 10 lines

arch/x86/kvm/mmu/mmu_internal.h struct kvm_page_fault { · line 281

Quoted 6 lines

arch/x86/kvm/mmu/mmu_internal.h +6 -0

+
+	/*
+	 * Handling below is for UPM self-tests and guests that treat userspace
+	 * as the authority on whether a fault should be private or not.
+	 */
+	private_fault = kvm_mem_is_private(kvm, gpa >> PAGE_SHIFT);

This code path is sad. One extra slot lookup and xarray look up. Without mmu lock, the result can change by other vcpu. Let's find a better way.

The intention was to rely on fault->mmu_seq to determine if a KVM_SET_MEMORY_ATTRIBUTES update came in after .private_fault was set so that fault handling could be retried, but that doesn't happen until kvm_faultin_pfn() which is *after* this is logged. So yes, I think there is a race here, and the approach you took in your Misc. series of keeping the kvm_mem_is_private() check inside kvm_faultin_pfn() is more efficient/correct.

Quoted 3 lines

If we can figure out a way to handle checking the fault flags in a way that works for both TDX/SNP (and KVM self-test use-case) we can consolidate around that.

I can think of the following ways. I think the second option is better because we don't need exit bit for error code.

- Introduce software defined error code
- Add a flags to struct kvm_arch for self-test use-case VM_TYPE_PROTECTED_VM. Set it to true for VM_TYPE_PROTECTED_VM case.
- any other?

Thanks,

Signature · 1 line

Isaku Yamahata <[email protected]>

Quoted 130 lines · Isaku Yamahata

Quoted 75 lines · Michael Roth

Quoted 63 lines · Isaku Yamahata

Quoted 46 lines · Michael Roth

This will be used to determine whether or not an #NPF should be serviced using a normal page vs. a guarded/gmem one.

Signed-off-by Michael Roth <[email protected]>

---
 arch/x86/include/asm/kvm_host.h |  7 +++++++
 arch/x86/kvm/mmu/mmu_internal.h | 35 ++++++++++++++++++++++++++++++++-
 2 files changed, 41 insertions(+), 1 deletion(-)

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

index b3bd24f2a390..c26f76641121 100644
@@ -1445,6 +1445,13 @@ struct kvm_arch {
 	 */
 #define SPLIT_DESC_CACHE_MIN_NR_OBJECTS (SPTE_ENT_PER_PAGE + 1)
 	struct kvm_mmu_memory_cache split_desc_cache;
+
+	/*
+	 * When set, used to determine whether a fault should be treated as
+	 * private in the context of protected VMs which use a separate gmem
+	 * pool to back private guest pages.
+	 */
+	u64 mmu_private_fault_mask;
 };
 
 struct kvm_vm_stat {

arch/x86/kvm/mmu/mmu_internal.h +11 -0

index 780b91e1da9f..9b9e75aa43f4 100644
@@ -252,6 +252,39 @@ struct kvm_page_fault {
 
 int kvm_tdp_page_fault(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault);
 
+static bool kvm_mmu_fault_is_private(struct kvm *kvm, gpa_t gpa, u64 err)
+{
+	struct kvm_memory_slot *slot;
+	bool private_fault = false;
+	gfn_t gfn = gpa_to_gfn(gpa);
+
+	slot = gfn_to_memslot(kvm, gfn);
+	if (!slot) {
+		pr_debug("%s: no slot, GFN: 0x%llx\n", __func__, gfn);
+		goto out;
+	}

arch/x86/kvm/mmu/mmu_internal.h struct kvm_page_fault { · line 275

Quoted 10 lines

arch/x86/kvm/mmu/mmu_internal.h +10 -0

+
+	if (!kvm_slot_can_be_private(slot)) {
+		pr_debug("%s: slot is not private, GFN: 0x%llx\n", __func__, gfn);
+		goto out;
+	}
+
+	if (kvm->arch.mmu_private_fault_mask) {
+		private_fault = !!(err & kvm->arch.mmu_private_fault_mask);
+		goto out;
+	}

What's the convention of err? Can we abstract it by introducing a new bit PFERR_PRIVATE_MASK? The caller sets it based on arch specific value. the logic will be

Quoted 1 line

        .is_private = err & PFERR_PRIVATE_MASK;

I'm not sure I understand the question. 'err' is just the page fault flags, and arch.mmu_private_fault_mask is something that can be set on a per-platform basis when running in a mode where shared/private access is recorded in the page fault flags during a #NPF.

I'm not sure how we'd keep the handling cross-platform by moving to a macro, since TDX uses a different bit, and we'd want to be able to build a SNP+TDX kernel that could run on either type of hardware.

Quoted 7 lines

Are you suggesting to reverse that and have err be set in a platform-specific way and then use a common PFERR_PRIVATE_MASK that's software-defined and consistent across platforms? That could work, but existing handling seems to use page fault flags as-is, keeping the hardware-set values, rather than modifying them to pass additional metadata, so it seems like it might make things more confusing to make an exception to that here. Or are there other cases where it's done that way?

I meant the latter, making PFERR_PRIVATE_MASK common software-defined.

I think the SVM fault handler can use hardware value directly by carefully defining those PFERR values.

TDX doesn't have architectural bit in error code to indicate the private fault. It's coded in faulted address as shared bit. GPA bit 51 or 47. PFERR_{USER, WRITE, FETCH, PRESENT} are already software-defined value for VMX (and TDX). The fault handler for VMX, handle_ept_violation(), converts encoding. For TDX, PFERR_PRIVATE_MASK is just one more software defined bit.

I'm fine with either way, variable or macro. Which do you prefer?

- Define variable mmu_private_fault_mask (global or per struct kvm) The module initialization code, hardware_setup(), sets mmu_private_fault_mask.
- Define the software defined value, PFERR_PRIVATE_MASK.
The caller of kvm_mmu_page_fault() parses the hardware value and construct software defined error_code.
- any other?

Quoted 19 lines

Quoted 10 lines

arch/x86/kvm/mmu/mmu_internal.h struct kvm_page_fault { · line 281

Quoted 6 lines

arch/x86/kvm/mmu/mmu_internal.h +6 -0

+
+	/*
+	 * Handling below is for UPM self-tests and guests that treat userspace
+	 * as the authority on whether a fault should be private or not.
+	 */
+	private_fault = kvm_mem_is_private(kvm, gpa >> PAGE_SHIFT);

This code path is sad. One extra slot lookup and xarray look up. Without mmu lock, the result can change by other vcpu. Let's find a better way.

The intention was to rely on fault->mmu_seq to determine if a KVM_SET_MEMORY_ATTRIBUTES update came in after .private_fault was set so that fault handling could be retried, but that doesn't happen until kvm_faultin_pfn() which is *after* this is logged. So yes, I think there is a race here, and the approach you took in your Misc. series of keeping the kvm_mem_is_private() check inside kvm_faultin_pfn() is more efficient/correct.

Quoted 3 lines

If we can figure out a way to handle checking the fault flags in a way that works for both TDX/SNP (and KVM self-test use-case) we can consolidate around that.

Quoted 7 lines

I can think of the following ways. I think the second option is better because we don't need exit bit for error code.

- Introduce software defined error code
- Add a flags to struct kvm_arch for self-test use-case VM_TYPE_PROTECTED_VM. Set it to true for VM_TYPE_PROTECTED_VM case.
- any other?

Vishal: hoping to get your thoughts here as well from the perspective of the KVM self-test use-case.

I was thinking that once we set fault->is_private, that sort of becomes our "software-defined" bit, and what KVM would use from that point forward to determine whether or not the access should be treated as a private one or not, and that whatever handler sets fault->is_private would encapsulate away all the platform-specific bit-checking needed to do that.

So if we were to straight-forwardly implement that based on how TDX currently handles checking for the shared bit in GPA, paired with how SEV-SNP handles checking for private bit in fault flags, it would look something like:

  bool kvm_fault_is_private(kvm, gpa, err)
  {
    /* SEV-SNP handling */
    if (kvm->arch.mmu_private_fault_mask)
      return !!(err & arch.mmu_private_fault_mask);

    /* TDX handling */
    if (kvm->arch.gfn_shared_mask)
      return !!(gpa & arch.gfn_shared_mask);

    return false;

}

  kvm_mmu_do_page_fault(vcpu, gpa, err, ...)
  {
    struct kvm_page_fault fault = {
      ...
      .is_private = kvm_fault_is_private(vcpu->kvm, gpa, err)
    };

    ...

}

And then arch.mmu_private_fault_mask and arch.gfn_shared_mask would be set per-KVM-instance, just like they are now with current SNP and TDX patchsets, since stuff like KVM self-test wouldn't be setting those masks, so it makes sense to do it per-instance in that regard.

But that still gets a little awkward for the KVM self-test use-case where .is_private should sort of be ignored in favor of whatever the xarray reports via kvm_mem_is_private(). In your Misc. series I believe you handled this by introducing a PFERR_HASATTR_MASK bit so we can determine whether existing value of fault->is_private should be ignored/overwritten or not.

So maybe kvm_fault_is_private() needs to return an integer value instead, like:

  enum {
    KVM_FAULT_VMM_DEFINED,
    KVM_FAULT_SHARED,
    KVM_FAULT_PRIVATE,
  }

  bool kvm_fault_is_private(kvm, gpa, err)
  {
    /* SEV-SNP handling */
    if (kvm->arch.mmu_private_fault_mask)
      (err & arch.mmu_private_fault_mask) ? KVM_FAULT_PRIVATE : KVM_FAULT_SHARED

    /* TDX handling */
    if (kvm->arch.gfn_shared_mask)
      (gpa & arch.gfn_shared_mask) ? KVM_FAULT_SHARED : KVM_FAULT_PRIVATE

    return KVM_FAULT_VMM_DEFINED;

}

And then down in __kvm_faultin_pfn() we do:

  if (fault->is_private == KVM_FAULT_VMM_DEFINED)
    fault->is_private = kvm_mem_is_private(vcpu->kvm, fault->gfn);
  else if (fault->is_private != kvm_mem_is_private(vcpu->kvm, fault->gfn))
    return kvm_do_memory_fault_exit(vcpu, fault);

  if (fault->is_private)
    return kvm_faultin_pfn_private(vcpu, fault);

Maybe kvm_fault_is_private() can be simplified based on what direction we end up taking WRT ongoing discussions like whether we decide to define KVM_X86_{SNP,TDX}_VM vm_types in addition to the KVM_X86_PROTECTED_VM type that the selftests uses, but hoping that for this path, any changes along that line can be encapsulated away in kvm_fault_is_private() without any/much further churn at the various call-sites like __kvm_faultin_pfn().

We could even push all the above logic down into the KVM self-tests, but have:

  bool kvm_fault_is_private(kvm, gpa, err) {
    return KVM_FAULT_VMM_DEFINED;
  }

And that would be enough to run self-tests as standalone series, with TDX/SNP should filling in kvm_fault_is_private() with their platform-specific handling.

Does that seem reasonable to you? At least as a starting point.

So if we were to straight-forwardly implement that based on how TDX currently handles checking for the shared bit in GPA, paired with how SEV-SNP handles checking for private bit in fault flags, it would look something like:

  bool kvm_fault_is_private(kvm, gpa, err)
  {
    /* SEV-SNP handling */
    if (kvm->arch.mmu_private_fault_mask)
      return !!(err & arch.mmu_private_fault_mask);

    /* TDX handling */
    if (kvm->arch.gfn_shared_mask)
      return !!(gpa & arch.gfn_shared_mask);

    return false;

}

  kvm_mmu_do_page_fault(vcpu, gpa, err, ...)
  {
    struct kvm_page_fault fault = {
      ...
      .is_private = kvm_fault_is_private(vcpu->kvm, gpa, err)

    };

    ...

}

And then arch.mmu_private_fault_mask and arch.gfn_shared_mask would be set per-KVM-instance, just like they are now with current SNP and TDX patchsets, since stuff like KVM self-test wouldn't be setting those masks, so it makes sense to do it per-instance in that regard.

But that still gets a little awkward for the KVM self-test use-case where .is_private should sort of be ignored in favor of whatever the xarray reports via kvm_mem_is_private().

In your Misc. series I believe you
handled this by introducing a PFERR_HASATTR_MASK bit so we can determine whether existing value of fault->is_private should be ignored/overwritten or not.

So maybe kvm_fault_is_private() needs to return an integer value instead, like:

  enum {
    KVM_FAULT_VMM_DEFINED,
    KVM_FAULT_SHARED,
    KVM_FAULT_PRIVATE,
  }

  bool kvm_fault_is_private(kvm, gpa, err)
  {
    /* SEV-SNP handling */
    if (kvm->arch.mmu_private_fault_mask)
      (err & arch.mmu_private_fault_mask) ? KVM_FAULT_PRIVATE : KVM_FAULT_SHARED

    /* TDX handling */
    if (kvm->arch.gfn_shared_mask)
      (gpa & arch.gfn_shared_mask) ? KVM_FAULT_SHARED : KVM_FAULT_PRIVATE

    return KVM_FAULT_VMM_DEFINED;

}

And then down in __kvm_faultin_pfn() we do:

  if (fault->is_private == KVM_FAULT_VMM_DEFINED)
    fault->is_private = kvm_mem_is_private(vcpu->kvm, fault->gfn);
  else if (fault->is_private != kvm_mem_is_private(vcpu->kvm, fault->gfn))
    return kvm_do_memory_fault_exit(vcpu, fault);

  if (fault->is_private)
    return kvm_faultin_pfn_private(vcpu, fault);

Quoted 13 lines

So if we were to straight-forwardly implement that based on how TDX currently handles checking for the shared bit in GPA, paired with how SEV-SNP handles checking for private bit in fault flags, it would look something like:

  bool kvm_fault_is_private(kvm, gpa, err)
  {
    /* SEV-SNP handling */
    if (kvm->arch.mmu_private_fault_mask)
      return !!(err & arch.mmu_private_fault_mask);

Quoted 3 lines

    /* TDX handling */
    if (kvm->arch.gfn_shared_mask)
      return !!(gpa & arch.gfn_shared_mask);

The logic of the two are identical. I think they need to be converged.

Either SEV-SNP should convert the error code private bit to the gfn_shared_mask, or TDX's shared bit should be converted to some private error bit.

Perhaps converting SEV-SNP makes more sense because if I recall correctly SEV guest also has a C-bit, correct?

Or, ...

Quoted 9 lines

    return false;

}

  kvm_mmu_do_page_fault(vcpu, gpa, err, ...)
  {
    struct kvm_page_fault fault = {
      ...
      .is_private = kvm_fault_is_private(vcpu->kvm, gpa, err)

... should we do something like:

	.is_private = static_call(kvm_x86_fault_is_private)(vcpu->kvm, gpa, 
							    err);

?

Quoted 10 lines

    };

    ...

}

And then arch.mmu_private_fault_mask and arch.gfn_shared_mask would be set per-KVM-instance, just like they are now with current SNP and TDX patchsets, since stuff like KVM self-test wouldn't be setting those masks, so it makes sense to do it per-instance in that regard.

Quoted 4 lines

But that still gets a little awkward for the KVM self-test use-case where .is_private should sort of be ignored in favor of whatever the xarray reports via kvm_mem_is_private().

I must have missed something. Why does KVM self-test have impact to how does KVM handles private fault?

Quoted 35 lines

In your Misc. series I believe you
handled this by introducing a PFERR_HASATTR_MASK bit so we can determine whether existing value of fault->is_private should be ignored/overwritten or not.

So maybe kvm_fault_is_private() needs to return an integer value instead, like:

  enum {
    KVM_FAULT_VMM_DEFINED,
    KVM_FAULT_SHARED,
    KVM_FAULT_PRIVATE,
  }

  bool kvm_fault_is_private(kvm, gpa, err)
  {
    /* SEV-SNP handling */
    if (kvm->arch.mmu_private_fault_mask)
      (err & arch.mmu_private_fault_mask) ? KVM_FAULT_PRIVATE : KVM_FAULT_SHARED

    /* TDX handling */
    if (kvm->arch.gfn_shared_mask)
      (gpa & arch.gfn_shared_mask) ? KVM_FAULT_SHARED : KVM_FAULT_PRIVATE

    return KVM_FAULT_VMM_DEFINED;

}

And then down in __kvm_faultin_pfn() we do:

  if (fault->is_private == KVM_FAULT_VMM_DEFINED)
    fault->is_private = kvm_mem_is_private(vcpu->kvm, fault->gfn);
  else if (fault->is_private != kvm_mem_is_private(vcpu->kvm, fault->gfn))
    return kvm_do_memory_fault_exit(vcpu, fault);

Quoted 2 lines

  if (fault->is_private)
    return kvm_faultin_pfn_private(vcpu, fault);

What does KVM_FAULT_VMM_DEFINED mean, exactly?

Shouldn't the fault type come from _hardware_?

Quoted 13 lines · Kai

Quoted 13 lines

So if we were to straight-forwardly implement that based on how TDX currently handles checking for the shared bit in GPA, paired with how SEV-SNP handles checking for private bit in fault flags, it would look something like:

  bool kvm_fault_is_private(kvm, gpa, err)
  {
    /* SEV-SNP handling */
    if (kvm->arch.mmu_private_fault_mask)
      return !!(err & arch.mmu_private_fault_mask);

Quoted 5 lines

Quoted 3 lines

    /* TDX handling */
    if (kvm->arch.gfn_shared_mask)
      return !!(gpa & arch.gfn_shared_mask);

The logic of the two are identical. I think they need to be converged.

I think they're just different enough that trying too hard to converge them might obfuscate things. If the determination didn't come from 2 completely different fields (gpa vs. fault flags) maybe it could be simplified a bit more, but have well-defined open-coded handler that gets called once to set fault->is_private during initial fault time so that that ugliness never needs to be looked at again by KVM MMU seems like a good way to keep things simple through the rest of the handling.

Quoted 4 lines

Either SEV-SNP should convert the error code private bit to the gfn_shared_mask, or TDX's shared bit should be converted to some private error bit.

struct kvm_page_fault seems to be the preferred way to pass additional params/metadata around, and .is_private field was introduced to track this private/shared state as part of UPM base series:

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

So it seems like unecessary complexity to track/encode that state into other additional places rather than just encapsulating it all in fault->is_private (or some similar field), and synthesizing all this platform-specific handling into a well-defined value that's conveyed by something like fault->is_private in a way where KVM MMU doesn't need to worry as much about platform-specific stuff seems like a good thing, and in line with what the UPM base series was trying to do by adding the fault->is_private field.

So all I'm really proposing is that whatever SNP and TDX end up doing should center around setting that fault->is_private field and keeping everything contained there.

If there are better ways to handle *how*
that's done I don't have any complaints there, but moving/adding bits to GPA/error_flags after fault time just seems unecessary to me when fault->is_private field can serve that purpose just as well.

Quoted 4 lines

Perhaps converting SEV-SNP makes more sense because if I recall correctly SEV guest also has a C-bit, correct?

That's correct, but the C-bit doesn't show in the GPA that gets passed up to KVM during an #NPF, and instead gets encoded into the fault's error_flags.

Quoted 13 lines

Or, ...

Quoted 9 lines

    return false;

}

  kvm_mmu_do_page_fault(vcpu, gpa, err, ...)
  {
    struct kvm_page_fault fault = {
      ...
      .is_private = kvm_fault_is_private(vcpu->kvm, gpa, err)

Quoted 4 lines

... should we do something like:

	.is_private = static_call(kvm_x86_fault_is_private)(vcpu->kvm, gpa, 
							    err);

We actually had exactly this in v7 of SNP hypervisor patches:

https://lore.kernel.org/linux-coco/[email protected]/T/#m17841f5bfdfb8350d69d78c6831dd8f3a4748638

but Sean was hoping to avoid a callback, which is why we ended up using a bitmask in this version since it basically all that callback would need to do. It's unfortunately that we don't have a common scheme between SNP/TDX, but maybe that's still possible, I just think that whatever that ends up being, it should live and be contained inside whatever helper ends up setting fault->is_private.

There's some other awkwardness with a callback approach. It sort of ties into your question about selftests so I'll address it below...

Quoted 13 lines

?

Quoted 10 lines

    };

    ...

}

And then arch.mmu_private_fault_mask and arch.gfn_shared_mask would be set per-KVM-instance, just like they are now with current SNP and TDX patchsets, since stuff like KVM self-test wouldn't be setting those masks, so it makes sense to do it per-instance in that regard.

Quoted 7 lines

Quoted 4 lines

But that still gets a little awkward for the KVM self-test use-case where .is_private should sort of be ignored in favor of whatever the xarray reports via kvm_mem_is_private().

I must have missed something. Why does KVM self-test have impact to how does KVM handles private fault?

The self-tests I'm referring to here are the ones from Vishal that shipped with v10 of Chao's UPM/fd-based private memory series, and also as part of Sean's gmem tree:

https://github.com/sean-jc/linux/commit/a0f5f8c911804f55935094ad3a277301704330a6

These exercise gmem/UPM handling without the need for any SNP/TDX hardware support. They do so by "trusting" the shared/private state that the VMM sets via KVM_SET_MEMORY_ATTRIBUTES. So if VMM says it should be private, KVM MMU will treat it as private, so we'd never get a mismatch, so KVM_EXIT_MEMORY_FAULT will never be generated.

Quoted 4 lines

If there are better ways to handle *how*
that's done I don't have any complaints there, but moving/adding bits to GPA/error_flags after fault time just seems unecessary to me when fault->is_private field can serve that purpose just as well.

Perhaps you missed my point. My point is arch.mmu_private_fault_mask and arch.gfn_shared_mask seem redundant because the logic around them are exactly the same. I do believe we should have fault->is_private passing to the common MMU code.

In fact, now I am wondering why we need to have "mmu_private_fault_mask" and "gfn_shared_mask" in _common_ KVM MMU code. We already have enough mechanism in KVM common code:

1) fault->is_private
2) kvm_mmu_page_role.private
3) an Xarray to tell whether a GFN is private or shared

I am not convinced that we need to have "mmu_private_fault_mask" and "gfn_shared_mask" in common KVM MMU code. Instead, they can be in AMD and Intel's vendor code.

Maybe it makes sense to have "gfn_shared_mask" in the KVM common code so that the fault handler can just strip away the "shared bit" at the very beginning (at least for TDX), but for the rest of the time I think we should already have enough infrastructure to handle private/shared mapping.

Btw, one minor issue is, if I recall correctly, for TDX the shared bit must be applied to the GFN for shared mapping in normal EPT. I guess AMD doesn't need that for shared mapping. So "gfn_shared_mask" maybe useful in this case, but w/o it I believe we can also achieve in another way via vendor callback.

Quoted 27 lines · Kai

Quoted 4 lines

If there are better ways to handle *how*
that's done I don't have any complaints there, but moving/adding bits to GPA/error_flags after fault time just seems unecessary to me when fault->is_private field can serve that purpose just as well.

Perhaps you missed my point. My point is arch.mmu_private_fault_mask and arch.gfn_shared_mask seem redundant because the logic around them are exactly the same. I do believe we should have fault->is_private passing to the common MMU code.

In fact, now I am wondering why we need to have "mmu_private_fault_mask" and "gfn_shared_mask" in _common_ KVM MMU code. We already have enough mechanism in KVM common code:

1) fault->is_private
2) kvm_mmu_page_role.private
3) an Xarray to tell whether a GFN is private or shared

I am not convinced that we need to have "mmu_private_fault_mask" and "gfn_shared_mask" in common KVM MMU code. Instead, they can be in AMD and Intel's vendor code.

Maybe it makes sense to have "gfn_shared_mask" in the KVM common code so that the fault handler can just strip away the "shared bit" at the very beginning (at least for TDX), but for the rest of the time I think we should already have enough infrastructure to handle private/shared mapping.

Quoted 4 lines

Btw, one minor issue is, if I recall correctly, for TDX the shared bit must be applied to the GFN for shared mapping in normal EPT. I guess AMD doesn't need that for shared mapping. So "gfn_shared_mask" maybe useful in this case, but w/o it I believe we can also achieve in another way via vendor callback.

"2) kvm_mmu_page_role.private" above has different meaning.

a). The fault is private or not.
b). page table the fault handler is walking is private or conventional.

a.) is common for SNP, TDX and PROTECTED_VM. It makes sense in kvm_mmu_do_page_fault() and __kvm_faultin_pfn(). After kvm_faultin_pfn(), the fault handler can mostly forget it for SNP and PROTECTED_VM. (large page adjustment needs it, though.) This is what we're discussing in this thread.

b.) is specific to TDX. TDX KVM MMU introduces one more page table.

Quoted 46 lines · Isaku Yamahata

Quoted 27 lines · Kai

Quoted 4 lines

If there are better ways to handle *how*
that's done I don't have any complaints there, but moving/adding bits to GPA/error_flags after fault time just seems unecessary to me when fault->is_private field can serve that purpose just as well.

Perhaps you missed my point. My point is arch.mmu_private_fault_mask and arch.gfn_shared_mask seem redundant because the logic around them are exactly the same. I do believe we should have fault->is_private passing to the common MMU code.

In fact, now I am wondering why we need to have "mmu_private_fault_mask" and "gfn_shared_mask" in _common_ KVM MMU code. We already have enough mechanism in KVM common code:

1) fault->is_private
2) kvm_mmu_page_role.private
3) an Xarray to tell whether a GFN is private or shared

I am not convinced that we need to have "mmu_private_fault_mask" and "gfn_shared_mask" in common KVM MMU code. Instead, they can be in AMD and Intel's vendor code.

Maybe it makes sense to have "gfn_shared_mask" in the KVM common code so that the fault handler can just strip away the "shared bit" at the very beginning (at least for TDX), but for the rest of the time I think we should already have enough infrastructure to handle private/shared mapping.

Quoted 4 lines

Btw, one minor issue is, if I recall correctly, for TDX the shared bit must be applied to the GFN for shared mapping in normal EPT. I guess AMD doesn't need that for shared mapping. So "gfn_shared_mask" maybe useful in this case, but w/o it I believe we can also achieve in another way via vendor callback.

"2) kvm_mmu_page_role.private" above has different meaning.

a). The fault is private or not.
b). page table the fault handler is walking is private or conventional.

a.) is common for SNP, TDX and PROTECTED_VM. It makes sense in kvm_mmu_do_page_fault() and __kvm_faultin_pfn(). After kvm_faultin_pfn(), the fault handler can mostly forget it for SNP and PROTECTED_VM. (large page adjustment needs it, though.) This is what we're discussing in this thread.

Quoted 3 lines

b.) is specific to TDX. TDX KVM MMU introduces one more page table.

I don't buy the last sentence. Even it's not necessarily for AMD from hardware's perspective, but the concept remains true for AMD too. So why cannot we use it for AMD?

This will be used to determine whether or not an #NPF should be serviced using a normal page vs. a guarded/gmem one.

Signed-off-by Michael Roth <[email protected]>

---
 arch/x86/include/asm/kvm_host.h |  7 +++++++
 arch/x86/kvm/mmu/mmu_internal.h | 35 ++++++++++++++++++++++++++++++++-
 2 files changed, 41 insertions(+), 1 deletion(-)

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

index b3bd24f2a390..c26f76641121 100644

Quoted 14 lines · Michael Roth

This will be used to determine whether or not an #NPF should be serviced using a normal page vs. a guarded/gmem one.

Signed-off-by Michael Roth <[email protected]>

---
 arch/x86/include/asm/kvm_host.h |  7 +++++++
 arch/x86/kvm/mmu/mmu_internal.h | 35 ++++++++++++++++++++++++++++++++-
 2 files changed, 41 insertions(+), 1 deletion(-)

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

index b3bd24f2a390..c26f76641121 100644

arch/x86/include/asm/kvm_host.h struct kvm_arch { · line 1450

Quoted 6 lines

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

@@ -1445,6 +1445,13 @@ struct kvm_arch {
 	 */
 #define SPLIT_DESC_CACHE_MIN_NR_OBJECTS (SPTE_ENT_PER_PAGE + 1)
 	struct kvm_mmu_memory_cache split_desc_cache;
+
+	/*
+	 * When set, used to determine whether a fault should be treated as

	   ^^^^^^^^

And when not set? Invalid?

I guess so, judging by the code below.

Signature · 5 lines

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Currently CONFIG_HAS_CC_PLATFORM is a prereq for building anything in arch/x86/coco, but that is generally only applicable for guest support.

For SEV-SNP, helpers related purely to host support will also live in arch/x86/coco. To allow for CoCo-related host support code in arch/x86/coco, move that check down into the Makefile and check for it specifically when needed.

Currently CONFIG_HAS_CC_PLATFORM is a prereq for building anything in

arch/x86/coco, but that is generally only applicable for guest support.

For SEV-SNP, helpers related purely to host support will also live in arch/x86/coco. To allow for CoCo-related host support code in arch/x86/coco, move that check down into the Makefile and check for it specifically when needed.

Quoted 1 line · Michael Roth

Currently CONFIG_HAS_CC_PLATFORM is a prereq for building anything in

					^^^^^^

Use proper english words pls.

Quoted 6 lines

arch/x86/coco, but that is generally only applicable for guest support.

For SEV-SNP, helpers related purely to host support will also live in arch/x86/coco. To allow for CoCo-related host support code in arch/x86/coco, move that check down into the Makefile and check for it specifically when needed.

I have no clue what that means. Example?

The last time we talked about paths, we ended up agreeing on:

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

So your "helpers related purely to host support" should go to

arch/x86/virt/svm/sev*.c

And just to keep it simple, that should be

arch/x86/virt/svm/sev.c

and if there's real need to split that, we can do that later.

Thx.

Signature · 4 lines

Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Basically, arch/x86/coco/Makefile is never processed if arch/x86/Kbuild indicates that CONFIG_HAS_CC_PLATFORM is not set. So if we want to have stuff in arch/x86/coco/Makefile that build for !CONFIG_HAS_CC_PLATFORM, like SNP host support, which does not rely on CONFIG_HAS_CC_PLATFORM being set, that check needs to be moved down into arch/x86/coco/Makefile.

Quoted 5 lines · Michael Roth

Basically, arch/x86/coco/Makefile is never processed if arch/x86/Kbuild indicates that CONFIG_HAS_CC_PLATFORM is not set. So if we want to have stuff in arch/x86/coco/Makefile that build for !CONFIG_HAS_CC_PLATFORM, like SNP host support, which does not rely on CONFIG_HAS_CC_PLATFORM being set, that check needs to be moved down into arch/x86/coco/Makefile.

Ok, so if you put SNP host support into arch/x86/virt/svm/sev.c, that should work too and won't have any relation to CONFIG_HAS_CC_PLATFORM, right?

The CC_PLATFORM thing is a way to check for confidential computing guest features by abstracting the capabilities so that you don't have to check *each* and *every* conf guest type in the conditionals and thus go nuts.

Thx.

Signature · 4 lines

Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Currently CONFIG_HAS_CC_PLATFORM is a prereq for building anything in arch/x86/coco, but that is generally only applicable for guest support.> For SEV-SNP, helpers related purely to host support will also live in arch/x86/coco. To allow for CoCo-related host support code in arch/x86/coco, move that check down into the Makefile and check for it specifically when needed.

Cc Kirill A. Shutemov <[email protected]>

Suggested-by Tom Lendacky <[email protected]>

Signed-off-by Michael Roth <[email protected]>

---
 arch/x86/Kbuild        | 2 +-
 arch/x86/coco/Makefile | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

arch/x86/Kbuild +1 -1

index 5a83da703e87..1889cef48b58 100644
@@ -1,5 +1,5 @@
 # SPDX-License-Identifier: GPL-2.0
-obj-$(CONFIG_ARCH_HAS_CC_PLATFORM) += coco/
+obj-y += coco/
 
 obj-y += entry/
 

arch/x86/coco/Makefile +1 -1

index c816acf78b6a..6aa52e719bf5 100644
@@ -3,6 +3,6 @@ CFLAGS_REMOVE_core.o	= -pg
 KASAN_SANITIZE_core.o	:= n
 CFLAGS_core.o		+= -fno-stack-protector
 
-obj-y += core.o
+obj-$(CONFIG_ARCH_HAS_CC_PLATFORM) += core.o
 
 obj-$(CONFIG_INTEL_TDX_GUEST)	+= tdx/

Hi,

Quoted 6 lines · Michael Roth

Currently CONFIG_HAS_CC_PLATFORM is a prereq for building anything in arch/x86/coco, but that is generally only applicable for guest support.> For SEV-SNP, helpers related purely to host support will also live in arch/x86/coco. To allow for CoCo-related host support code in arch/x86/coco, move that check down into the Makefile and check for it specifically when needed.

I think CONFIG_HAS_CC_PLATFORM is not meant to be guest specific (otherwise,

we could have named it CONFIG_HAS_CC_GUEST). Will it create any issue if we enable it in host?

Quoted 32 lines

Cc Kirill A. Shutemov <[email protected]>

Suggested-by Tom Lendacky <[email protected]>

Signed-off-by Michael Roth <[email protected]>

---
  arch/x86/Kbuild        | 2 +-
  arch/x86/coco/Makefile | 2 +-
  2 files changed, 2 insertions(+), 2 deletions(-)

arch/x86/Kbuild +1 -1

index 5a83da703e87..1889cef48b58 100644
@@ -1,5 +1,5 @@
  # SPDX-License-Identifier: GPL-2.0
-obj-$(CONFIG_ARCH_HAS_CC_PLATFORM) += coco/
+obj-y += coco/
 
  obj-y += entry/
 

arch/x86/coco/Makefile +1 -1

index c816acf78b6a..6aa52e719bf5 100644
@@ -3,6 +3,6 @@ CFLAGS_REMOVE_core.o	= -pg
  KASAN_SANITIZE_core.o	:= n
  CFLAGS_core.o		+= -fno-stack-protector
 
-obj-y += core.o
+obj-$(CONFIG_ARCH_HAS_CC_PLATFORM) += core.o
 
  obj-$(CONFIG_INTEL_TDX_GUEST)	+= tdx/

arch/x86/coco/sev/host.c line 109

Quoted 8 lines · Michael Roth

arch/x86/coco/sev/host.c new file +8 -0

+	/*
+	 * Calculate the amount the memory that must be reserved by the BIOS to
+	 * address the whole RAM, including the bookkeeping area. The RMP itself
+	 * must also be covered.
+	 */
+	max_rmp_pfn = max_pfn;
+	if (PHYS_PFN(rmp_end) > max_pfn)
+		max_rmp_pfn = PHYS_PFN(rmp_end);

Could you say a little here about how this deals with memory hotplug?

Quoted 10 lines · Dave Hansen

arch/x86/coco/sev/host.c line 109

Quoted 8 lines · Michael Roth

arch/x86/coco/sev/host.c new file +8 -0

+	/*
+	 * Calculate the amount the memory that must be reserved by the BIOS to
+	 * address the whole RAM, including the bookkeeping area. The RMP itself
+	 * must also be covered.
+	 */
+	max_rmp_pfn = max_pfn;
+	if (PHYS_PFN(rmp_end) > max_pfn)
+		max_rmp_pfn = PHYS_PFN(rmp_end);

Could you say a little here about how this deals with memory hotplug?

Does SNP hw even support memory hotplug?

I think in order to support that, you'd need some special dance because of the RMP table etc...

Yep, there's the hardware side and then there are fun nuggets like using mem= and then doing a software-only hot-add later after boot.

Also, if the hardware doesn't support any kind of hotplug, it would be great to point to the place in the spec where it says that.

From: Brijesh Singh <[email protected]>

The memory integrity guarantees of SEV-SNP are enforced through a new structure called the Reverse Map Table (RMP). The RMP is a single data structure shared across the system that contains one entry for every 4K page of DRAM that may be used by SEV-SNP VMs. APM2 section 15.36 details

a number of steps needed to detect/enable SEV-SNP and RMP table support on the host:

- Detect SEV-SNP support based on CPUID bit
- Initialize the RMP table memory reported by the RMP base/end MSR registers and configure IOMMU to be compatible with RMP access restrictions
- Set the MtrrFixDramModEn bit in SYSCFG MSR
- Set the SecureNestedPagingEn and VMPLEn bits in the SYSCFG MSR
- Configure IOMMU

RMP table entry format is non-architectural and it can vary by processor. It is defined by the PPR. Restrict SNP support to CPU models/families which are compatible with the current RMP table entry format to guard against any undefined behavior when running on other system types. Future models/support will handle this through an architectural mechanism to allow for broader compatibility.

SNP host code depends on CONFIG_KVM_AMD_SEV config flag, which may be enabled even when CONFIG_AMD_MEM_ENCRYPT isn't set, so update the SNP-specific IOMMU helpers used here to rely on CONFIG_KVM_AMD_SEV instead of CONFIG_AMD_MEM_ENCRYPT.

Co-developed-by Ashish Kalra <[email protected]>

Signed-off-by Ashish Kalra <[email protected]>

Co-developed-by Tom Lendacky <[email protected]>

Signed-off-by Tom Lendacky <[email protected]>

Signed-off-by Brijesh Singh <[email protected]>

[mdr: rework commit message to be clearer about what patch does, squash
in early_rmptable_check() handling from Tom]

Signed-off-by Michael Roth <[email protected]>

---
 arch/x86/coco/Makefile                   |   1 +
 arch/x86/coco/sev/Makefile               |   3 +
 arch/x86/coco/sev/host.c                 | 212 +++++++++++++++++++++++
 arch/x86/include/asm/disabled-features.h |   8 +-
 arch/x86/include/asm/msr-index.h         |  11 +-
 arch/x86/include/asm/sev.h               |   2 +
 arch/x86/kernel/cpu/amd.c                |  19 ++
 drivers/iommu/amd/init.c                 |   2 +-
 include/linux/amd-iommu.h                |   2 +-
 9 files changed, 256 insertions(+), 4 deletions(-)
 create mode 100644 arch/x86/coco/sev/Makefile
 create mode 100644 arch/x86/coco/sev/host.c

Quoted 6 lines · Michael Roth

From: Brijesh Singh <[email protected]>

The memory integrity guarantees of SEV-SNP are enforced through a new structure called the Reverse Map Table (RMP). The RMP is a single data structure shared across the system that contains one entry for every 4K page of DRAM that may be used by SEV-SNP VMs. APM2 section 15.36 details

Rather say 'APM v2, section "Secure Nested Paging (SEV-SNP)"' because the numbering is more likely to change than the name in the future. With the name, people can find it faster.

Quoted 11 lines

a number of steps needed to detect/enable SEV-SNP and RMP table support on the host:

- Detect SEV-SNP support based on CPUID bit
- Initialize the RMP table memory reported by the RMP base/end MSR registers and configure IOMMU to be compatible with RMP access restrictions
- Set the MtrrFixDramModEn bit in SYSCFG MSR
- Set the SecureNestedPagingEn and VMPLEn bits in the SYSCFG MSR
- Configure IOMMU

Quoted 6 lines

RMP table entry format is non-architectural and it can vary by processor. It is defined by the PPR. Restrict SNP support to CPU models/families which are compatible with the current RMP table entry format to guard against any undefined behavior when running on other system types. Future models/support will handle this through an architectural mechanism to allow for broader compatibility.

I'm guessing this is all for live migration between SNP hosts. If so, then there will have to be a guest API to handle the differences.

Quoted 4 lines

SNP host code depends on CONFIG_KVM_AMD_SEV config flag, which may be enabled even when CONFIG_AMD_MEM_ENCRYPT isn't set, so update the SNP-specific IOMMU helpers used here to rely on CONFIG_KVM_AMD_SEV instead of CONFIG_AMD_MEM_ENCRYPT.

Does that mean that even on CONFIG_AMD_MEM_ENCRYPT=n kernels, host SNP can function?

Do we even want that?

I'd expect that a host SNP kernel should have SME enabled too even though it is not absolutely necessary.

Quoted 20 lines

Co-developed-by Ashish Kalra <[email protected]>

Signed-off-by Ashish Kalra <[email protected]>

Co-developed-by Tom Lendacky <[email protected]>

Signed-off-by Tom Lendacky <[email protected]>

Signed-off-by Brijesh Singh <[email protected]>

[mdr: rework commit message to be clearer about what patch does, squash
in early_rmptable_check() handling from Tom]

Signed-off-by Michael Roth <[email protected]>

---
  arch/x86/coco/Makefile                   |   1 +
  arch/x86/coco/sev/Makefile               |   3 +
  arch/x86/coco/sev/host.c                 | 212 +++++++++++++++++++++++
  arch/x86/include/asm/disabled-features.h |   8 +-
  arch/x86/include/asm/msr-index.h         |  11 +-
  arch/x86/include/asm/sev.h               |   2 +
  arch/x86/kernel/cpu/amd.c                |  19 ++
  drivers/iommu/amd/init.c                 |   2 +-
  include/linux/amd-iommu.h                |   2 +-
  9 files changed, 256 insertions(+), 4 deletions(-)

Quoted 2 lines

create mode 100644 arch/x86/coco/sev/Makefile
create mode 100644 arch/x86/coco/sev/host.c

Ignored review comments here:

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

Ignoring this one for now too.

Quoted 6 lines · Michael Roth

From: Brijesh Singh <[email protected]>

The memory integrity guarantees of SEV-SNP are enforced through a new structure called the Reverse Map Table (RMP). The RMP is a single data structure shared across the system that contains one entry for every 4K page of DRAM that may be used by SEV-SNP VMs. APM2 section 15.36 details

Rather say 'APM v2, section "Secure Nested Paging (SEV-SNP)"' because the numbering is more likely to change than the name in the future. With the name, people can find it faster.

Quoted 11 lines

a number of steps needed to detect/enable SEV-SNP and RMP table support on the host:

- Detect SEV-SNP support based on CPUID bit
- Initialize the RMP table memory reported by the RMP base/end MSR registers and configure IOMMU to be compatible with RMP access restrictions
- Set the MtrrFixDramModEn bit in SYSCFG MSR
- Set the SecureNestedPagingEn and VMPLEn bits in the SYSCFG MSR
- Configure IOMMU

Quoted 6 lines

RMP table entry format is non-architectural and it can vary by processor. It is defined by the PPR. Restrict SNP support to CPU models/families which are compatible with the current RMP table entry format to guard against any undefined behavior when running on other system types. Future models/support will handle this through an architectural mechanism to allow for broader compatibility.

I'm guessing this is all for live migration between SNP hosts. If so, then there will have to be a guest API to handle the differences.

Quoted 5 lines

SNP host code depends on CONFIG_KVM_AMD_SEV config flag, which may be enabled even when CONFIG_AMD_MEM_ENCRYPT isn't set, so update the SNP-specific IOMMU helpers used here to rely on CONFIG_KVM_AMD_SEV instead of CONFIG_AMD_MEM_ENCRYPT.

Does that mean that even on CONFIG_AMD_MEM_ENCRYPT=n kernels, host SNP can function?

Do we even want that?

I'd expect that a host SNP kernel should have SME enabled too even though it is not absolutely necessary.

Quoted 20 lines

Co-developed-by Ashish Kalra <[email protected]>

Signed-off-by Ashish Kalra <[email protected]>

Co-developed-by Tom Lendacky <[email protected]>

Signed-off-by Tom Lendacky <[email protected]>

Signed-off-by Brijesh Singh <[email protected]>

[mdr: rework commit message to be clearer about what patch does, squash
in early_rmptable_check() handling from Tom]

Signed-off-by Michael Roth <[email protected]>

---
  arch/x86/coco/Makefile                   |   1 +
  arch/x86/coco/sev/Makefile               |   3 +
  arch/x86/coco/sev/host.c                 | 212 +++++++++++++++++++++++
  arch/x86/include/asm/disabled-features.h |   8 +-
  arch/x86/include/asm/msr-index.h         |  11 +-
  arch/x86/include/asm/sev.h               |   2 +
  arch/x86/kernel/cpu/amd.c                |  19 ++
  drivers/iommu/amd/init.c                 |   2 +-
  include/linux/amd-iommu.h                |   2 +-
  9 files changed, 256 insertions(+), 4 deletions(-)

Quoted 2 lines

create mode 100644 arch/x86/coco/sev/Makefile
create mode 100644 arch/x86/coco/sev/host.c

Ignored review comments here:

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

Ignoring this one for now too.

From: Brijesh Singh <[email protected]>

The memory integrity guarantees of SEV-SNP are enforced through a new structure called the Reverse Map Table (RMP). The RMP is a single data structure shared across the system that contains one entry for every 4K page of DRAM that may be used by SEV-SNP VMs. APM2 section 15.36 details a number of steps needed to detect/enable SEV-SNP and RMP table support on the host:

- Detect SEV-SNP support based on CPUID bit
- Initialize the RMP table memory reported by the RMP base/end MSR registers and configure IOMMU to be compatible with RMP access restrictions
- Set the MtrrFixDramModEn bit in SYSCFG MSR
- Set the SecureNestedPagingEn and VMPLEn bits in the SYSCFG MSR
- Configure IOMMU

RMP table entry format is non-architectural and it can vary by processor. It is defined by the PPR. Restrict SNP support to CPU models/families which are compatible with the current RMP table entry format to guard against any undefined behavior when running on other system types. Future models/support will handle this through an architectural mechanism to allow for broader compatibility.

SNP host code depends on CONFIG_KVM_AMD_SEV config flag, which may be enabled even when CONFIG_AMD_MEM_ENCRYPT isn't set, so update the SNP-specific IOMMU helpers used here to rely on CONFIG_KVM_AMD_SEV instead of CONFIG_AMD_MEM_ENCRYPT.

Co-developed-by Ashish Kalra <[email protected]>

Signed-off-by Ashish Kalra <[email protected]>

Co-developed-by Tom Lendacky <[email protected]>

Signed-off-by Tom Lendacky <[email protected]>

Signed-off-by Brijesh Singh <[email protected]>

[mdr: rework commit message to be clearer about what patch does, squash
in early_rmptable_check() handling from Tom]

Signed-off-by Michael Roth <[email protected]>

---
 arch/x86/coco/Makefile                   |   1 +
 arch/x86/coco/sev/Makefile               |   3 +
 arch/x86/coco/sev/host.c                 | 212 +++++++++++++++++++++++
 arch/x86/include/asm/disabled-features.h |   8 +-
 arch/x86/include/asm/msr-index.h         |  11 +-
 arch/x86/include/asm/sev.h               |   2 +
 arch/x86/kernel/cpu/amd.c                |  19 ++
 drivers/iommu/amd/init.c                 |   2 +-
 include/linux/amd-iommu.h                |   2 +-
 9 files changed, 256 insertions(+), 4 deletions(-)
 create mode 100644 arch/x86/coco/sev/Makefile
 create mode 100644 arch/x86/coco/sev/host.c

arch/x86/coco/Makefile +1 -0

index 6aa52e719bf5..6a7d876130e2 100644
@@ -6,3 +6,4 @@ CFLAGS_core.o		+= -fno-stack-protector
 obj-$(CONFIG_ARCH_HAS_CC_PLATFORM) += core.o
 
 obj-$(CONFIG_INTEL_TDX_GUEST)	+= tdx/
+obj-$(CONFIG_KVM_AMD_SEV)	+= sev/

arch/x86/coco/sev/Makefile new file +3 -0

new file mode 100644
index 000000000000..27c0500d75c8
@@ -0,0 +1,3 @@
+# SPDX-License-Identifier: GPL-2.0
+
+obj-y += host.o

arch/x86/coco/sev/host.c new file +212 -0

new file mode 100644
index 000000000000..6907ce887b23
@@ -0,0 +1,212 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * AMD SVM-SEV Host Support.
+ *
+ * Copyright (C) 2023 Advanced Micro Devices, Inc.
+ *
+ * Author: Ashish Kalra <[email protected]>
+ *
+ */
+
+#include <linux/cc_platform.h>
+#include <linux/printk.h>
+#include <linux/mm_types.h>
+#include <linux/set_memory.h>
+#include <linux/memblock.h>
+#include <linux/kernel.h>
+#include <linux/mm.h>
+#include <linux/cpumask.h>
+#include <linux/iommu.h>
+#include <linux/amd-iommu.h>
+
+#include <asm/sev.h>
+#include <asm/processor.h>
+#include <asm/setup.h>
+#include <asm/svm.h>
+#include <asm/smp.h>
+#include <asm/cpu.h>
+#include <asm/apic.h>
+#include <asm/cpuid.h>
+#include <asm/cmdline.h>
+#include <asm/iommu.h>
+
+/*
+ * The first 16KB from the RMP_BASE is used by the processor for the
+ * bookkeeping, the range needs to be added during the RMP entry lookup.
+ */
+#define RMPTABLE_CPU_BOOKKEEPING_SZ	0x4000
+
+static unsigned long rmptable_start __ro_after_init;
+static unsigned long rmptable_end __ro_after_init;
+
+#undef pr_fmt
+#define pr_fmt(fmt)	"SEV-SNP: " fmt
+
+static int __mfd_enable(unsigned int cpu)
+{
+	u64 val;
+
+	if (!cpu_feature_enabled(X86_FEATURE_SEV_SNP))
+		return 0;
+
+	rdmsrl(MSR_AMD64_SYSCFG, val);
+
+	val |= MSR_AMD64_SYSCFG_MFDM;
+
+	wrmsrl(MSR_AMD64_SYSCFG, val);
+
+	return 0;
+}
+
+static __init void mfd_enable(void *arg)
+{
+	__mfd_enable(smp_processor_id());
+}
+
+static int __snp_enable(unsigned int cpu)
+{
+	u64 val;
+
+	if (!cpu_feature_enabled(X86_FEATURE_SEV_SNP))
+		return 0;
+
+	rdmsrl(MSR_AMD64_SYSCFG, val);
+
+	val |= MSR_AMD64_SYSCFG_SNP_EN;
+	val |= MSR_AMD64_SYSCFG_SNP_VMPL_EN;
+
+	wrmsrl(MSR_AMD64_SYSCFG, val);
+
+	return 0;
+}
+
+static __init void snp_enable(void *arg)
+{
+	__snp_enable(smp_processor_id());
+}
+
+bool snp_get_rmptable_info(u64 *start, u64 *len)
+{
+	u64 max_rmp_pfn, calc_rmp_sz, rmp_sz, rmp_base, rmp_end;
+
+	rdmsrl(MSR_AMD64_RMP_BASE, rmp_base);
+	rdmsrl(MSR_AMD64_RMP_END, rmp_end);
+
+	if (!rmp_base || !rmp_end) {
+		pr_err("Memory for the RMP table has not been reserved by BIOS\n");
+		return false;
+	}
+
+	rmp_sz = rmp_end - rmp_base + 1;
+
+	/*
+	 * Calculate the amount the memory that must be reserved by the BIOS to
+	 * address the whole RAM, including the bookkeeping area. The RMP itself
+	 * must also be covered.
+	 */
+	max_rmp_pfn = max_pfn;
+	if (PHYS_PFN(rmp_end) > max_pfn)
+		max_rmp_pfn = PHYS_PFN(rmp_end);
+
+	calc_rmp_sz = (max_rmp_pfn << 4) + RMPTABLE_CPU_BOOKKEEPING_SZ;
+
+	if (calc_rmp_sz > rmp_sz) {
+		pr_err("Memory reserved for the RMP table does not cover full system RAM (expected 0x%llx got 0x%llx)\n",
+		       calc_rmp_sz, rmp_sz);
+		return false;
+	}
+
+	*start = rmp_base;
+	*len = rmp_sz;
+
+	return true;
+}
+
+static __init int __snp_rmptable_init(void)
+{
+	u64 rmp_base, sz;
+	void *start;
+	u64 val;
+
+	if (!snp_get_rmptable_info(&rmp_base, &sz))
+		return 1;
+
+	pr_info("RMP table physical address [0x%016llx - 0x%016llx]\n",
+		rmp_base, rmp_base + sz - 1);
+
+	start = memremap(rmp_base, sz, MEMREMAP_WB);
+	if (!start) {
+		pr_err("Failed to map RMP table addr 0x%llx size 0x%llx\n", rmp_base, sz);
+		return 1;
+	}
+
+	/*
+	 * Check if SEV-SNP is already enabled, this can happen in case of
+	 * kexec boot.
+	 */
+	rdmsrl(MSR_AMD64_SYSCFG, val);
+	if (val & MSR_AMD64_SYSCFG_SNP_EN)
+		goto skip_enable;
+
+	/* Initialize the RMP table to zero */
+	memset(start, 0, sz);
+
+	/* Flush the caches to ensure that data is written before SNP is enabled. */
+	wbinvd_on_all_cpus();
+
+	/* MFDM must be enabled on all the CPUs prior to enabling SNP. */
+	on_each_cpu(mfd_enable, NULL, 1);
+
+	/* Enable SNP on all CPUs. */
+	on_each_cpu(snp_enable, NULL, 1);
+
+skip_enable:
+	rmptable_start = (unsigned long)start;
+	rmptable_end = rmptable_start + sz - 1;
+
+	return 0;
+}
+
+static int __init snp_rmptable_init(void)
+{
+	int family, model;
+
+	if (!cpu_feature_enabled(X86_FEATURE_SEV_SNP))
+		return 0;
+
+	family = boot_cpu_data.x86;
+	model  = boot_cpu_data.x86_model;
+
+	/*
+	 * RMP table entry format is not architectural and it can vary by processor and
+	 * is defined by the per-processor PPR. Restrict SNP support on the known CPU
+	 * model and family for which the RMP table entry format is currently defined for.
+	 */
+	if (!(family == 0x19 && model <= 0xaf) && !(family == 0x1a && model <= 0xf))
+		goto nosnp;
+
+	if (amd_iommu_snp_enable())
+		goto nosnp;
+
+	if (__snp_rmptable_init())
+		goto nosnp;
+
+	cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "x86/rmptable_init:online", __snp_enable, NULL);
+
+	return 0;
+
+nosnp:
+	setup_clear_cpu_cap(X86_FEATURE_SEV_SNP);
+	return -ENOSYS;
+}
+
+/*
+ * This must be called after the PCI subsystem. This is because amd_iommu_snp_enable()
+ * is called to ensure the IOMMU supports the SEV-SNP feature, which can only be
+ * called after subsys_initcall().
+ *
+ * NOTE: IOMMU is enforced by SNP to ensure that hypervisor cannot program DMA
+ * directly into guest private memory. In case of SNP, the IOMMU ensures that
+ * the page(s) used for DMA are hypervisor owned.
+ */
+fs_initcall(snp_rmptable_init);

arch/x86/include/asm/disabled-features.h +7 -1

index 5dfa4fb76f4b..0a9938aea305 100644
@@ -99,6 +99,12 @@
 # define DISABLE_TDX_GUEST	(1 << (X86_FEATURE_TDX_GUEST & 31))
 #endif
 
+#ifdef CONFIG_KVM_AMD_SEV
+# define DISABLE_SEV_SNP	0
+#else
+# define DISABLE_SEV_SNP	(1 << (X86_FEATURE_SEV_SNP & 31))
+#endif
+
 /*
  * Make sure to add features to the correct mask
  */

@@ -123,7 +129,7 @@
 			 DISABLE_ENQCMD)
 #define DISABLED_MASK17	0
 #define DISABLED_MASK18	0
-#define DISABLED_MASK19	0
+#define DISABLED_MASK19	(DISABLE_SEV_SNP)
 #define DISABLED_MASK20	0
 #define DISABLED_MASK_CHECK BUILD_BUG_ON_ZERO(NCAPINTS != 21)
 

arch/x86/include/asm/msr-index.h +10 -1

index ad35355ee43e..db0f3a041930 100644
@@ -574,6 +574,8 @@
 #define MSR_AMD64_SEV_ENABLED		BIT_ULL(MSR_AMD64_SEV_ENABLED_BIT)
 #define MSR_AMD64_SEV_ES_ENABLED	BIT_ULL(MSR_AMD64_SEV_ES_ENABLED_BIT)
 #define MSR_AMD64_SEV_SNP_ENABLED	BIT_ULL(MSR_AMD64_SEV_SNP_ENABLED_BIT)
+#define MSR_AMD64_RMP_BASE		0xc0010132
+#define MSR_AMD64_RMP_END		0xc0010133
 
 /* SNP feature bits enabled by the hypervisor */
 #define MSR_AMD64_SNP_VTOM			BIT_ULL(3)

@@ -675,7 +677,14 @@
 #define MSR_K8_TOP_MEM2			0xc001001d
 #define MSR_AMD64_SYSCFG		0xc0010010
 #define MSR_AMD64_SYSCFG_MEM_ENCRYPT_BIT	23
-#define MSR_AMD64_SYSCFG_MEM_ENCRYPT	BIT_ULL(MSR_AMD64_SYSCFG_MEM_ENCRYPT_BIT)
+#define MSR_AMD64_SYSCFG_MEM_ENCRYPT		BIT_ULL(MSR_AMD64_SYSCFG_MEM_ENCRYPT_BIT)
+#define MSR_AMD64_SYSCFG_SNP_EN_BIT		24
+#define MSR_AMD64_SYSCFG_SNP_EN		BIT_ULL(MSR_AMD64_SYSCFG_SNP_EN_BIT)
+#define MSR_AMD64_SYSCFG_SNP_VMPL_EN_BIT	25
+#define MSR_AMD64_SYSCFG_SNP_VMPL_EN		BIT_ULL(MSR_AMD64_SYSCFG_SNP_VMPL_EN_BIT)
+#define MSR_AMD64_SYSCFG_MFDM_BIT		19
+#define MSR_AMD64_SYSCFG_MFDM			BIT_ULL(MSR_AMD64_SYSCFG_MFDM_BIT)
+
 #define MSR_K8_INT_PENDING_MSG		0xc0010055
 /* C1E active bits in int pending message */
 #define K8_INTP_C1E_ACTIVE_MASK		0x18000000

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

index ebc271bb6d8e..d34c46db7dd1 100644
@@ -197,6 +197,7 @@ void snp_set_wakeup_secondary_cpu(void);
 bool snp_init(struct boot_params *bp);
 void __init __noreturn snp_abort(void);
 int snp_issue_guest_request(u64 exit_code, struct snp_req_data *input, unsigned long *fw_err);
+bool snp_get_rmptable_info(u64 *start, u64 *len);
 #else
 static inline void sev_es_ist_enter(struct pt_regs *regs) { }
 static inline void sev_es_ist_exit(void) { }

@@ -221,6 +222,7 @@ static inline int snp_issue_guest_request(u64 exit_code, struct snp_req_data *in
 {
 	return -ENOTTY;
 }
+static inline bool snp_get_rmptable_info(u64 *start, u64 *len) { return false; }
 #endif
 
 #endif

arch/x86/kernel/cpu/amd.c +5 -0

index a79774181f22..1493ddf89fdf 100644
@@ -20,6 +20,7 @@
 #include <asm/delay.h>
 #include <asm/debugreg.h>
 #include <asm/resctrl.h>
+#include <asm/sev.h>
 
 #ifdef CONFIG_X86_64
 # include <asm/mmconfig.h>

@@ -546,6 +547,20 @@ static void bsp_init_amd(struct cpuinfo_x86 *c)
 	resctrl_cpu_detect(c);
 }
 
+static bool early_rmptable_check(void)
+{
+	u64 rmp_base, rmp_size;
+

A hardware limitation prevents the host from enabling Automatic IBRS when SNP is enabled. Instead, fall back to retpolines.

arch/x86/kernel/cpu/bugs.c +0 -0

index f9d060e71c3e..3fba3623ff64 100644

Quoted 2 lines · Michael Roth

A hardware limitation prevents the host from enabling Automatic IBRS when SNP is enabled. Instead, fall back to retpolines.

"Hardware limitation"? As in, it is a documented, architectural restriction? Or, it's a CPU bug?

Quoted 5 lines

arch/x86/kernel/cpu/bugs.c +0 -0

index f9d060e71c3e..3fba3623ff64 100644

arch/x86/kernel/cpu/bugs.c static void __init spectre_v2_select_mitigation(void) · line 1515

Quoted 10 lines

arch/x86/kernel/cpu/bugs.c +6 -1

@@ -1507,7 +1507,12 @@ static void __init spectre_v2_select_mitigation(void)
 
  	if (spectre_v2_in_ibrs_mode(mode)) {
  		if (boot_cpu_has(X86_FEATURE_AUTOIBRS)) {
-			msr_set_bit(MSR_EFER, _EFER_AUTOIBRS);
+			if (!cpu_feature_enabled(X86_FEATURE_SEV_SNP)) {
+				msr_set_bit(MSR_EFER, _EFER_AUTOIBRS);
+			} else {
+				pr_err("SNP feature available, not enabling AutoIBRS on the host.\n");
+				mode = spectre_v2_select_retpoline();
+			}

I think this would be nicer if you did something like:

	if (cpu_feature_enabled(X86_FEATURE_SEV_SNP))
		setup_clear_cpu_cap(X86_FEATURE_AUTOIBRS);

somewhere _else_ in the code instead of smack-dab in the middle of the mitigation selection.

Automatic IBRS provides protection to [1]:

 - Processes running at CPL=0
 - Processes running as host when Secure Nested Paging (SEV-SNP) is enabled

i.e.,

    (CPL < 3) || ((ASID == 0) && SNP)

Because of this limitation, do not enable Automatic IBRS when SNP is enabled.

Instead, fall back to retpolines.

Note that the AutoIBRS feature may continue to be used within the guest.

arch/x86/kernel/cpu/common.c +2 -1

index 8cd4126d8253..311c0a6422b5 100644
@@ -1348,7 +1348,8 @@ static void __init cpu_set_bug_bits(struct
 cpuinfo_x86 *c)
       * AMD's AutoIBRS is equivalent to Intel's eIBRS - use the Intel feature
       * flag and protect from vendor-specific bugs via the whitelist.
       */
-    if ((ia32_cap & ARCH_CAP_IBRS_ALL) || cpu_has(c, X86_FEATURE_AUTOIBRS)) {
+    if ((ia32_cap & ARCH_CAP_IBRS_ALL) || (cpu_has(c, X86_FEATURE_AUTOIBRS) &&
+        !cpu_feature_enabled(X86_FEATURE_SEV_SNP))) {
          setup_force_cpu_cap(X86_FEATURE_IBRS_ENHANCED);

         if (!cpu_matches(cpu_vuln_whitelist, NO_EIBRS_PBRSB) && !(ia32_cap & ARCH_CAP_PBRSB_NO))

On 7/18/23 15:34, Kim Phillips wrote:
...

Quoted 11 lines

Automatic IBRS provides protection to [1]:

 - Processes running at CPL=0
 - Processes running as host when Secure Nested Paging (SEV-SNP) is enabled

i.e.,

    (CPL < 3) || ((ASID == 0) && SNP)

Because of this limitation, do not enable Automatic IBRS when SNP is enabled.

Gah, I found that hard to parse. I think it's because you're talking about an SEV-SNP host in one part and "SNP" in the other but _meaning_ SNP host and SNP guest.

Could I maybe suggest that you folks follow the TDX convention and actually add _GUEST and _HOST to the feature name be explicit about which side is which?

Quoted 1 line

Instead, fall back to retpolines.

Now I'm totally lost.

This is talking about falling back to retpolines ... in the kernel. But "Automatic IBRS provides protection to ... CPL < 3", aka. the kernel.

Quoted 2 lines

Note that the AutoIBRS feature may continue to be used within the guest.

What is this trying to say?

"AutoIBRS can still be used in a guest since it protects CPL < 3"

or

"The AutoIBRS bits can still be twiddled within the guest even though it doesn't do any good"

?

Hopefully the commit text in this version will help answer all your questions?:

Quoted 2 lines · Kim Phillips

Hopefully the commit text in this version will help answer all your questions?:

To be honest, it didn't really. I kinda feel like I was having the APM contents tossed casually in my direction rather than being provided a fully considered explanation.

Here's what I came up with instead:

Host-side Automatic IBRS has different behavior based on whether SEV-SNP is enabled.

Without SEV-SNP, Automatic IBRS protects only the kernel. But when SEV-SNP is enabled, the Automatic IBRS protection umbrella widens to all host-side code, including userspace. This protection comes at a cost:
reduced userspace indirect branch performance.

To avoid this performance loss, nix using Automatic IBRS on SEV-SNP hosts. Fall back to retpolines instead.

=====

Is that about right?

I don't think any chit-chat about the guest side is even relevant.

This also absolutely needs a comment. Perhaps just pull the code up to the top level of the function and do this:

	/*
	 * Automatic IBRS imposes unacceptable overhead on host
	 * userspace for SEV-SNP systems.  Zap it instead.
	 */
	if (cpu_feature_enabled(X86_FEATURE_SEV_SNP))
		setup_clear_cpu_cap(X86_FEATURE_AUTOIBRS);

BTW, I assume you've grumbled to folks about this. It's an awful shame the hardware (or ucode) was built this was. It's just throwing Automatic IBRS out the window because it's not architected in a nice way.

Is there any plan to improve this?

arch/x86/coco/sev/host.c +11 -0

+/*
+ * The RMP entry format is not architectural. The format is defined in PPR
+ * Family 19h Model 01h, Rev B1 processor.
+ */
+struct rmpentry {
+	union {
+		struct {
+			u64	assigned	: 1,
+				pagesize	: 1,
+				immutable	: 1,
+				rsvd1		: 9,

static unsigned long rmptable_start __ro_after_init; static unsigned long rmptable_end __ro_after_init;

arch/x86/coco/sev/host.c +0 -0

@@ -210,3 +235,63 @@ static int __init snp_rmptable_init(void)
  * the page(s) used for DMA are hypervisor owned.
  */
 fs_initcall(snp_rmptable_init);

arch/x86/coco/sev/host.c +13 -0

+int snp_lookup_rmpentry(u64 pfn, bool *assigned, int *level)
+{
+	struct rmpentry e;
+	int ret;
+
+	ret = __snp_lookup_rmpentry(pfn, &e, level);
+	if (ret)
+		return ret;
+
+	*assigned = !!rmpentry_assigned(&e);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(snp_lookup_rmpentry);

arch/x86/include/asm/sev-common.h +4 -0

index b8357d6ecd47..bf0378136289 100644
@@ -171,4 +171,8 @@ struct snp_psc_desc {
 #define GHCB_ERR_INVALID_INPUT		5
 #define GHCB_ERR_INVALID_EVENT		6
 
+/* RMP page size */
+#define RMP_PG_SIZE_4K			0
+#define RMP_TO_X86_PG_LEVEL(level)	(((level) == RMP_PG_SIZE_4K) ? PG_LEVEL_4K : PG_LEVEL_2M)
+
 #endif

arch/x86/include/asm/sev-host.h new file +10 -0

new file mode 100644
index 000000000000..30d47e20081d
@@ -0,0 +1,22 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * AMD SVM-SEV Host Support.
+ *
+ * Copyright (C) 2023 Advanced Micro Devices, Inc.
+ *
+ * Author: Ashish Kalra <[email protected]>
+ *
+ */
+

arch/x86/include/asm/sev-host.h new file +1 -0

+#endif

arch/x86/include/asm/sev.h +0 -3

index d34c46db7dd1..446fc7a9f7b0 100644
@@ -81,9 +81,6 @@ extern bool handle_vc_boot_ghcb(struct pt_regs *regs);
 /* Software defined (when rFlags.CF = 1) */
 #define PVALIDATE_FAIL_NOUPDATE		255
 
-/* RMP page size */
-#define RMP_PG_SIZE_4K			0
-
 #define RMPADJUST_VMSA_PAGE_BIT		BIT(16)
 
 /* SNP Guest message request */

Quoted 11 lines · Michael Roth

arch/x86/coco/sev/host.c +11 -0

+/*
+ * The RMP entry format is not architectural. The format is defined in PPR
+ * Family 19h Model 01h, Rev B1 processor.
+ */
+struct rmpentry {
+	union {
+		struct {
+			u64	assigned	: 1,
+				pagesize	: 1,
+				immutable	: 1,
+				rsvd1		: 9,

arch/x86/coco/sev/host.c line 53

Quoted 10 lines

arch/x86/coco/sev/host.c +10 -0

+				gpa		: 39,
+				asid		: 10,
+				vmsa		: 1,
+				validated	: 1,
+				rsvd2		: 1;
+		} info;
+		u64 low;
+	};
+	u64 high;
+} __packed;

What's 'high' used for? The PPR says it's reserved. Why not call it reserved?

It _looks_ like it's only used for a debugging pr_info(). It makes the struct look kinda goofy. I'd much rather limit the goofiness to the "dumping" code, like:

     u64 *__e = (void *)e;
     ....
     pr_info("RMPEntry paddr 0x%llx: [high=0x%016llx low=0x%016llx]\n",
                               pfn << PAGE_SHIFT, __e[0], __e[1]);

BTW, why does it do any good to dump all these reserved fields?

arch/x86/coco/sev/host.c line 62

Quoted 8 lines

/*
* The first 16KB from the RMP_BASE is used by the processor for the
* bookkeeping, the range needs to be added during the RMP entry lookup. */
#define RMPTABLE_CPU_BOOKKEEPING_SZ 0x4000

arch/x86/coco/sev/host.c +3 -0

+#define RMPENTRY_SHIFT			8
+#define rmptable_page_offset(x)	(RMPTABLE_CPU_BOOKKEEPING_SZ +		\
+				 (((unsigned long)x) >> RMPENTRY_SHIFT))

Is RMPENTRY_SHIFT just log2(sizeof(struct rmpentry))? If so, wouldn't this be a *LOT* more obvious to be written:

unsigned long rmptable_offset(unsigned long paddr)

{
	return 	RMPTABLE_CPU_BOOKKEEPING_SZ +
		paddr / sizeof(struct rmpentry);
}

??

It removes a cast, gives 'x' a real name, removes a random magic number #define and is clearer to boot.

Quoted 7 lines

static unsigned long rmptable_start __ro_after_init; static unsigned long rmptable_end __ro_after_init;

arch/x86/coco/sev/host.c +0 -0

@@ -210,3 +235,63 @@ static int __init snp_rmptable_init(void)
  * the page(s) used for DMA are hypervisor owned.
  */
 fs_initcall(snp_rmptable_init);

arch/x86/coco/sev/host.c static int __init snp_rmptable_init(void) · line 247

Quoted 10 lines

arch/x86/coco/sev/host.c +10 -0

+
+static inline unsigned int rmpentry_assigned(const struct rmpentry *e)
+{
+	return e->info.assigned;
+}
+
+static inline unsigned int rmpentry_pagesize(const struct rmpentry *e)
+{
+	return e->info.pagesize;
+}

I think these are a little silly. If you're going to go to the trouble of using bitfields, you don't need helpers for every bitfield. I say either use bitfields without helpers or just regular old:

#define RMPENTRY_ASSIGNED_MASK BIT(1)

and then *maybe* you can make helpers for them.

arch/x86/coco/sev/host.c static int __init snp_rmptable_init(void) · line 255

Quoted 8 lines

arch/x86/coco/sev/host.c +7 -0

+static int rmptable_entry(unsigned long paddr, struct rmpentry *entry)
+{
+	unsigned long vaddr;
+
+	vaddr = rmptable_start + rmptable_page_offset(paddr);
+	if (unlikely(vaddr > rmptable_end))
+		return -EFAULT;

This seems like more of a WARN_ON_ONCE() kind of thing than just an error return. Isn't it a big deal if an invalid (non-RMP-covered) address makes it in here?

arch/x86/coco/sev/host.c static int __init snp_rmptable_init(void) · line 263

Quoted 8 lines

arch/x86/coco/sev/host.c +7 -0

+	*entry = *(struct rmpentry *)vaddr;
+
+	return 0;
+}
+
+static int __snp_lookup_rmpentry(u64 pfn, struct rmpentry *entry, int *level)
+{

I've been bitten by pfn vs. paddr quite a few times. I'd really encourage you to add it to the names if you're going to pass them around, like __snp_lookup_rmpentry_pfn().

arch/x86/coco/sev/host.c static int __init snp_rmptable_init(void) · line 269

Quoted 6 lines

arch/x86/coco/sev/host.c +6 -0

+	unsigned long paddr = pfn << PAGE_SHIFT;
+	struct rmpentry large_entry;
+	int ret;
+
+	if (!cpu_feature_enabled(X86_FEATURE_SEV_SNP))
+		return -ENXIO;

OK, so if you're running on non-SNP hardware, you return -ENXIO here. Remember this please.

arch/x86/coco/sev/host.c static int __init snp_rmptable_init(void) · line 283

Quoted 13 lines

arch/x86/coco/sev/host.c +13 -0

+	ret = rmptable_entry(paddr, entry);
+	if (ret)
+		return ret;
+
+	/* Read a large RMP entry to get the correct page level used in RMP entry. */
+	ret = rmptable_entry(paddr & PMD_MASK, &large_entry);
+	if (ret)
+		return ret;
+
+	*level = RMP_TO_X86_PG_LEVEL(rmpentry_pagesize(&large_entry));
+
+	return 0;
+}

This is a bit weird. Should it say something like this?

To do an 4k RMP lookup the hardware looks at two places in the RMP:

1. The actual 4k RMP entry
2. The 2M entry that "covers" the 4k entry

The page size is not indicated in the 4k entries at all. It is solely indicated by the 2M entry.

Quoted 42 lines

arch/x86/coco/sev/host.c +13 -0

+int snp_lookup_rmpentry(u64 pfn, bool *assigned, int *level)
+{
+	struct rmpentry e;
+	int ret;
+
+	ret = __snp_lookup_rmpentry(pfn, &e, level);
+	if (ret)
+		return ret;
+
+	*assigned = !!rmpentry_assigned(&e);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(snp_lookup_rmpentry);

arch/x86/include/asm/sev-common.h +4 -0

index b8357d6ecd47..bf0378136289 100644
@@ -171,4 +171,8 @@ struct snp_psc_desc {
 #define GHCB_ERR_INVALID_INPUT		5
 #define GHCB_ERR_INVALID_EVENT		6
 
+/* RMP page size */
+#define RMP_PG_SIZE_4K			0
+#define RMP_TO_X86_PG_LEVEL(level)	(((level) == RMP_PG_SIZE_4K) ? PG_LEVEL_4K : PG_LEVEL_2M)
+
 #endif

arch/x86/include/asm/sev-host.h new file +10 -0

new file mode 100644
index 000000000000..30d47e20081d
@@ -0,0 +1,22 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * AMD SVM-SEV Host Support.
+ *
+ * Copyright (C) 2023 Advanced Micro Devices, Inc.
+ *
+ * Author: Ashish Kalra <[email protected]>
+ *
+ */
+

arch/x86/include/asm/sev-host.h line 20

Quoted 10 lines

arch/x86/include/asm/sev-host.h new file +10 -0

+#ifndef __ASM_X86_SEV_HOST_H
+#define __ASM_X86_SEV_HOST_H
+
+#include <asm/sev-common.h>
+
+#ifdef CONFIG_KVM_AMD_SEV
+int snp_lookup_rmpentry(u64 pfn, bool *assigned, int *level);
+#else
+static inline int snp_lookup_rmpentry(u64 pfn, bool *assigned, int *level) { return 0; }
+#endif

Above, -ENXIO was returned when SEV-SNP was not supported. Here, 0 is returned when it is compiled out. That inconsistent.

Is snp_lookup_rmpentry() acceptable when SEV-SNP is in play? I'd like to see consistency between when it is compiled out and when it is compiled in but unsupported on the CPU.

Quoted 16 lines

arch/x86/include/asm/sev-host.h new file +1 -0

+#endif

arch/x86/include/asm/sev.h +0 -3

index d34c46db7dd1..446fc7a9f7b0 100644
@@ -81,9 +81,6 @@ extern bool handle_vc_boot_ghcb(struct pt_regs *regs);
 /* Software defined (when rFlags.CF = 1) */
 #define PVALIDATE_FAIL_NOUPDATE		255
 
-/* RMP page size */
-#define RMP_PG_SIZE_4K			0
-
 #define RMPADJUST_VMSA_PAGE_BIT		BIT(16)
 
 /* SNP Guest message request */