[PATCH RFC 1/8] KVM: SVM: fix: calculate end instead of passing size

[PATCH RFC 1/8] KVM: SVM: fix: calculate end instead of passing size

[Jarkko Sakkinen]

From: Tom Dohrmann <erbse.13@gmx.de>

The third parameter of `kvm_vm_do_hva_range_op` doesn't take the size
of the range, but the end of the range.

Signed-off-by: Tom Dohrmann <erbse.13@gmx.de>
Link: https://lore.kernel.org/lkml/Y6Sgwp%2FBofzCUrQe@notebook/
Signed-off-by: Jarkko Sakkinen <jarkko@profian.com>
---
 arch/x86/kvm/svm/sev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 855f5e702240..d3468d1533bd 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -500,7 +500,7 @@ static int sev_get_memfile_pfn(struct kvm *kvm, unsigned long addr,
 			       unsigned long size, unsigned long npages,
 			       struct page **pages)
 {
-	return kvm_vm_do_hva_range_op(kvm, addr, size,
+	return kvm_vm_do_hva_range_op(kvm, addr, addr + size,
 				      sev_get_memfile_pfn_handler, pages);
 }
 
-- 
2.38.1

[PATCH RFC 2/8] KVM: SVM: fix: initialize `npinned`

[Jarkko Sakkinen]

From: Tom Dohrmann <erbse.13@gmx.de>

If UPM is enabled and getting the PFN fails, `npinned` is never set, but is
read for the call to `unpin_user_pages`.

Link: https://lore.kernel.org/lkml/Y6Sgwp%2FBofzCUrQe@notebook/
Signed-off-by: Tom Dohrmann <erbse.13@gmx.de>
Signed-off-by: Jarkko Sakkinen <jarkko@profian.com>
---
 arch/x86/kvm/svm/sev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index d3468d1533bd..6d3162853c33 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -510,7 +510,7 @@ static struct page **sev_pin_memory(struct kvm *kvm, unsigned long uaddr,
 {
 	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
 	unsigned long npages, size;
-	int npinned;
+	int npinned = 0;
 	unsigned long locked, lock_limit;
 	struct page **pages;
 	unsigned long first, last;
-- 
2.38.1

[PATCH RFC 3/8] KVM: SVM: write back corrected CPUID page

[Jarkko Sakkinen]

From: Tom Dohrmann <erbse.13@gmx.de>

When doing a launch update for a CPUID page the firmware checks that the values
conform to the policy laid out in the processor programming manual. If the
values don't conform, the firmware will return SEV_RET_INVALID_PARAM.
In addition to returning an error the firmware will choose some acceptable
values and write them back to the page that was used for the launch update, so
that the VMM can inspect the changes and try again with the corrected values.
This is specified in section 8.17.2.6 in the SEV-SNP Firmware ABI spec.
Because launch updates are always done on the private UPM mappings, the pages
are first copied from the shared mappings to the private mappings. When the
firmware corrects the values, the corrected values are in the private mappings,
inaccessible to userspace. In order to make the corrected values accessible to
userspace, the page containing them must be copied from the private mappings
back to the shared mappings.

[jarkko@profian.com: fixed checkpatch.pl errors]
Link: https://lore.kernel.org/lkml/Y76%2FI6Nrh7xEAAwv@notebook/
Signed-off-by: Tom Dohrmann <erbse.13@gmx.de>
Signed-off-by: Jarkko Sakkinen <jarkko@profian.com>
---
 arch/x86/kvm/svm/sev.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 6d3162853c33..4a8e552d8cfe 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -2230,6 +2230,23 @@ static int snp_launch_update_gfn_handler(struct kvm *kvm,
 			pr_err("SEV-SNP launch update failed, ret: 0x%x, fw_error: 0x%x\n",
 			       ret, *error);
 			snp_page_reclaim(pfns[i]);
+
+			/*
+			 * When invalid CPUID function entries are detected, the firmware
+			 * corrects these entries.  In that case write the page back to
+			 * userspace.
+			 */
+			if (params.page_type == SNP_PAGE_TYPE_CPUID &&
+			    *error == SEV_RET_INVALID_PARAM) {
+				int ret;
+
+				host_rmp_make_shared(pfns[i], PG_LEVEL_4K, true);
+
+				ret = kvm_write_guest_page(kvm, gfn, kvaddr, 0, PAGE_SIZE);
+				if (ret)
+					pr_err("Guest write failed, ret: 0x%x\n", ret);
+			}
+
 			goto e_release;
 		}
 	}
-- 
2.38.1

Re: [PATCH RFC 3/8] KVM: SVM: write back corrected CPUID page

[Tom Lendacky]

On 1/26/23 20:52, Jarkko Sakkinen wrote:
> From: Tom Dohrmann <erbse.13@gmx.de>
> 
> When doing a launch update for a CPUID page the firmware checks that the values
> conform to the policy laid out in the processor programming manual. If the
> values don't conform, the firmware will return SEV_RET_INVALID_PARAM.
> In addition to returning an error the firmware will choose some acceptable
> values and write them back to the page that was used for the launch update, so
> that the VMM can inspect the changes and try again with the corrected values.
> This is specified in section 8.17.2.6 in the SEV-SNP Firmware ABI spec.
> Because launch updates are always done on the private UPM mappings, the pages
> are first copied from the shared mappings to the private mappings. When the
> firmware corrects the values, the corrected values are in the private mappings,
> inaccessible to userspace. In order to make the corrected values accessible to
> userspace, the page containing them must be copied from the private mappings
> back to the shared mappings.
> 
> [jarkko@profian.com: fixed checkpatch.pl errors]
> Link: https://lore.kernel.org/lkml/Y76%2FI6Nrh7xEAAwv@notebook/
> Signed-off-by: Tom Dohrmann <erbse.13@gmx.de>
> Signed-off-by: Jarkko Sakkinen <jarkko@profian.com>
> ---
>   arch/x86/kvm/svm/sev.c | 17 +++++++++++++++++
>   1 file changed, 17 insertions(+)
> 
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 6d3162853c33..4a8e552d8cfe 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -2230,6 +2230,23 @@ static int snp_launch_update_gfn_handler(struct kvm *kvm,
>   			pr_err("SEV-SNP launch update failed, ret: 0x%x, fw_error: 0x%x\n",
>   			       ret, *error);
>   			snp_page_reclaim(pfns[i]);
> +
> +			/*
> +			 * When invalid CPUID function entries are detected, the firmware
> +			 * corrects these entries.  In that case write the page back to
> +			 * userspace.

I would additionally add that the firmware does not encrypt the page, 
which allows the hypervisor to copy the page back to userspace.

Thanks,
Tom

> +			 */
> +			if (params.page_type == SNP_PAGE_TYPE_CPUID &&
> +			    *error == SEV_RET_INVALID_PARAM) {
> +				int ret;
> +
> +				host_rmp_make_shared(pfns[i], PG_LEVEL_4K, true);
> +
> +				ret = kvm_write_guest_page(kvm, gfn, kvaddr, 0, PAGE_SIZE);
> +				if (ret)
> +					pr_err("Guest write failed, ret: 0x%x\n", ret);
> +			}
> +
>   			goto e_release;
>   		}
>   	}

[PATCH RFC 4/8] KVM: SVM: fix: add separate error for missing slot

[Jarkko Sakkinen]

From: Tom Dohrmann <erbse.13@gmx.de>

The next error message uses slot assuming that it's not NULL, but that's not
always true. A separate error message is used when slot is NULL.

This can be triggered by a malicious guest that sends an
`SVM_VMGEXIT_AP_CREATE` ap creation event that points to an invalid AP VMSA
address (one that isn't mapped). Currently the kernel just copies the values
provided by the guest into `snp_vmsa_gpa` (see arch/x86/kvm/svm/sev.c:3930).
This value is directly passed into `gfn_to_pfn_restricted` in
`__sev_snp_update_protected_guest_state` (see arch/x86/kvm/svm/sev.c:3792).

Signed-off-by: Tom Dohrmann <erbse.13@gmx.de>
Signed-off-by: Jarkko Sakkinen <jarkko@profian.com>
---
 arch/x86/kvm/svm/sev.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 4a8e552d8cfe..d76127f1499a 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -3745,6 +3745,11 @@ static kvm_pfn_t gfn_to_pfn_restricted(struct kvm *kvm, gfn_t gfn)
 	int order = 0;
 
 	slot = gfn_to_memslot(kvm, gfn);
+	if (!slot) {
+		pr_err("SEV: Failure retrieving memslot for GFN 0x%llx\n", gfn);
+		return INVALID_PAGE;
+	}
+
 	if (!kvm_slot_can_be_private(slot)) {
 		pr_err("SEV: Failure retrieving restricted memslot for GFN 0x%llx, flags 0x%x, userspace_addr: 0x%lx\n",
 		       gfn, slot->flags, slot->userspace_addr);
-- 
2.38.1

[PATCH RFC 5/8] KVM: SVM: fix: Don't return an error for `GHCB_MSR_PSC_REQ`

[Jarkko Sakkinen]

From: Tom Dohrmann <erbse.13@gmx.de>

There's no reason to return an error when encountering an page state change
request (PSC request) because that's normal behaviour on the guest's part.
Instead 0 should be returned to cause a VM exit so that userspace can handle
the page state change request.

Link: https://lore.kernel.org/lkml/Y77J7C2E9Xd1QcmZ@notebook/
Signed-off-by: Tom Dohrmann <erbse.13@gmx.de>
Signed-off-by: Jarkko Sakkinen <jarkko@profian.com>
---
 arch/x86/kvm/svm/sev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index d76127f1499a..899c78d03c35 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -4068,7 +4068,7 @@ static int sev_handle_vmgexit_msr_protocol(struct vcpu_svm *svm)
 		vcpu->run->vmgexit.ghcb_msr = control->ghcb_gpa;
 		vcpu->arch.complete_userspace_io = snp_complete_psc_msr_protocol;
 
-		ret = -1;
+		ret = 0;
 		break;
 	case GHCB_MSR_TERM_REQ: {
 		u64 reason_set, reason_code;
-- 
2.38.1

[PATCH RFC 6/8] KVM: SVM: KVM_SEV_SNP_LAUNCH_RESET_VECTOR

[Jarkko Sakkinen]

The attestation of the workload includes the CPU state information.

When the workload that was running in the VM exits, the system stores
the state in the special area (VMSA).  When the workload is started again,
it loads the state back.  This is a well-defined process and works.

However, the *initial state* needs to be better defined.  Currently, it is
defined by kernel without userspace knowledge or ability to influence.  As
a result, Enarx does not know the initial state and has to make a guess
whenever there is need to offline digest calculation.  After all, the core
idea of confidential computing that everything can be validated and
verified in order to reach trust.

The variation could come mainly from either sev_features and vintr_ctrl.

Allow to user space to define them when the new KVM_SEV_SNP_RESET_VECTOR
init flag is set but at the same time verify that they are set only to
those values that kernel is aware of.

Link: https://enarx.dev/
Signed-off-by: Jarkko Sakkinen <jarkko@profian.com>
---
 arch/x86/include/asm/svm.h | 15 ++++++----
 arch/x86/kvm/svm/sev.c     | 57 +++++++++++++++++++++++++++++++-------
 arch/x86/kvm/svm/svm.h     |  1 +
 include/uapi/linux/kvm.h   |  5 ++++
 4 files changed, 63 insertions(+), 15 deletions(-)

diff --git a/arch/x86/include/asm/svm.h b/arch/x86/include/asm/svm.h
index e76ad26ba64f..b3b7131a1ce7 100644
--- a/arch/x86/include/asm/svm.h
+++ b/arch/x86/include/asm/svm.h
@@ -278,12 +278,17 @@ enum avic_ipi_failure_cause {
 #define AVIC_HPA_MASK	~((0xFFFULL << 52) | 0xFFF)
 #define VMCB_AVIC_APIC_BAR_MASK		0xFFFFFFFFFF000ULL
 
-#define SVM_SEV_FEAT_SNP_ACTIVE			BIT(0)
-#define SVM_SEV_FEAT_RESTRICTED_INJECTION	BIT(3)
-#define SVM_SEV_FEAT_ALTERNATE_INJECTION	BIT(4)
-#define SVM_SEV_FEAT_INT_INJ_MODES		\
-	(SVM_SEV_FEAT_RESTRICTED_INJECTION |	\
+
+#define SVM_SEV_FEAT_SNP_ACTIVE			BIT_ULL(0)
+#define SVM_SEV_FEAT_RESTRICTED_INJECTION	BIT_ULL(3)
+#define SVM_SEV_FEAT_ALTERNATE_INJECTION	BIT_ULL(4)
+#define SVM_SEV_FEAT_INT_INJ_MODES \
+	(SVM_SEV_FEAT_RESTRICTED_INJECTION |\
 	 SVM_SEV_FEAT_ALTERNATE_INJECTION)
+#define SVM_SEV_FEAT_UNSUPPORTED_MASK \
+	~(SVM_SEV_FEAT_SNP_ACTIVE |\
+	  SVM_SEV_FEAT_RESTRICTED_INJECTION |\
+	  SVM_SEV_FEAT_ALTERNATE_INJECTION)
 
 struct vmcb_seg {
 	u16 selector;
diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 899c78d03c35..5e4666b79689 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -303,6 +303,11 @@ static int verify_snp_init_flags(struct kvm *kvm, struct kvm_sev_cmd *argp)
 	/* Save the supplied flags value */
 	sev->snp_init_flags = params.flags;
 
+	if (params.flags & KVM_SEV_SNP_RESET_VECTOR) {
+		sev->sev_features = params.sev_features;
+		sev->vintr_ctrl = params.vintr_ctrl;
+	}
+
 	/* Return the supported flags value */
 	params.flags = SEV_SNP_SUPPORTED_FLAGS;
 
@@ -785,6 +790,33 @@ static int sev_es_sync_vmsa(struct vcpu_svm *svm)
 	if (svm->vcpu.guest_debug || (svm->vmcb->save.dr7 & ~DR7_FIXED_1))
 		return -EINVAL;
 
+	/* Validate that the user defined reset vector meets the expectations: */
+	if (sev->snp_init_flags & KVM_SEV_SNP_RESET_VECTOR) {
+		u64 unsupported = sev->sev_features & SVM_SEV_FEAT_UNSUPPORTED_MASK;
+
+		if (sev->sev_features & unsupported) {
+			pr_debug("sev_features: unsupported flags: 0x%016llx\n", unsupported);
+			return -EINVAL;
+		}
+
+		if (!(sev_snp_guest(svm->vcpu.kvm) &&
+		    (sev->sev_features & SVM_SEV_FEAT_SNP_ACTIVE))) {
+			pr_debug("sev_features: SNP_ACTIVE is not set\n");
+			return -EINVAL;
+		}
+
+		if (!((sev->snp_init_flags & KVM_SEV_SNP_RESTRICTED_INJET) &&
+		    (sev->sev_features & SVM_SEV_FEAT_RESTRICTED_INJECTION))) {
+			pr_debug("sev_features: SNP_SEV_FEAT_RESTRICTED_INJECTION is not set\n");
+			return -EINVAL;
+		}
+
+		if (sev->vintr_ctrl) {
+			pr_debug("vintr_ctrl: unsupported flags: 0x%016llx\n", sev->vintr_ctrl);
+			return -EINVAL;
+		}
+	}
+
 	/*
 	 * SEV-ES will use a VMSA that is pointed to by the VMCB, not
 	 * the traditional VMSA that is part of the VMCB. Copy the
@@ -820,18 +852,23 @@ static int sev_es_sync_vmsa(struct vcpu_svm *svm)
 	save->xss  = svm->vcpu.arch.ia32_xss;
 	save->dr6  = svm->vcpu.arch.dr6;
 
-	/* Enable the SEV-SNP feature */
-	if (sev_snp_guest(svm->vcpu.kvm))
-		save->sev_features |= SVM_SEV_FEAT_SNP_ACTIVE;
+	if (sev->snp_init_flags & KVM_SEV_SNP_RESET_VECTOR) {
+		save->sev_features = sev->sev_features;
+		save->vintr_ctrl = sev->vintr_ctrl;
+	} else {
+		/* Enable the SEV-SNP feature */
+		if (sev_snp_guest(svm->vcpu.kvm))
+			save->sev_features |= SVM_SEV_FEAT_SNP_ACTIVE;
 
-	if (sev->snp_init_flags & KVM_SEV_SNP_RESTRICTED_INJET)
-		save->sev_features |= SVM_SEV_FEAT_RESTRICTED_INJECTION;
+		if (sev->snp_init_flags & KVM_SEV_SNP_RESTRICTED_INJET)
+			save->sev_features |= SVM_SEV_FEAT_RESTRICTED_INJECTION;
 
-	/*
-	 * Save the VMSA synced SEV features. For now, they are the same for
-	 * all vCPUs, so just save each time.
-	 */
-	sev->sev_features = save->sev_features;
+		/*
+		 * Save the VMSA synced SEV features. For now, they are the same for
+		 * all vCPUs, so just save each time.
+		 */
+		sev->sev_features = save->sev_features;
+	}
 
 	pr_debug("Virtual Machine Save Area (VMSA):\n");
 	print_hex_dump_debug("", DUMP_PREFIX_NONE, 16, 1, save, sizeof(*save), false);
diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index 4dab13084363..5dce17eefd5d 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -106,6 +106,7 @@ struct kvm_sev_info {
 	struct mutex guest_req_lock;
 
 	u64 sev_features;	/* Features set at VMSA creation */
+	u64 vintr_ctrl;
 };
 
 struct kvm_svm {
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index 48bcc59cf86b..e176d0ec6c54 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -2054,8 +2054,13 @@ struct kvm_sev_receive_update_data {
 /* enable the restricted injection timer */
 #define KVM_SEV_SNP_RESTRICTED_TIMER_INJET   (1 << 1)
 
+/* use the given reset vector for sev_features and vintr_ctrl */
+#define KVM_SEV_SNP_RESET_VECTOR   (1 << 2)
+
 struct kvm_snp_init {
 	__u64 flags;
+	__u64 sev_features;
+	__u64 vintr_ctrl;
 };
 
 struct kvm_sev_snp_launch_start {
-- 
2.38.1

[PATCH RFC 8/8] crypto: ccp: Move __sev_snp_init_locked() call inside __sev_platform_init_locked()

[Jarkko Sakkinen]

The following functions end up calling sev_platform_init() or
__sev_platform_init_locked():

* sev_guest_init()
* sev_ioctl_do_pek_csr
* sev_ioctl_do_pdh_export()
* sev_ioctl_do_pek_import()
* sev_ioctl_do_pek_pdh_gen()
* sev_pci_init()

Only sev_guest_init() and sev_pci_init() also call sev_snp_init().
Address this by calling __sev_snp_init_locked() inside
__sev_platform_init_locked() before any other initialization.

Signed-off-by: Jarkko Sakkinen <jarkko@profian.com>
---
 arch/x86/kvm/svm/sev.c       |  4 +--
 drivers/crypto/ccp/sev-dev.c | 51 +++++++++++++-----------------------
 include/linux/psp-sev.h      | 15 -----------
 3 files changed, 19 insertions(+), 51 deletions(-)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 5e4666b79689..2dd56f59fc50 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -343,11 +343,9 @@ static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp)
 			goto e_free;
 
 		mutex_init(&sev->guest_req_lock);
-		ret = sev_snp_init(&argp->error, false);
-	} else {
-		ret = sev_platform_init(&argp->error);
 	}
 
+	ret = sev_platform_init(&argp->error);
 	if (ret)
 		goto e_free;
 
diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index 50e73df966ec..be040926f66a 100644
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -102,6 +102,7 @@ struct sev_data_range_list *snp_range_list;
 static size_t sev_es_tmr_size = SEV_ES_TMR_SIZE;
 
 static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret);
+static int __sev_snp_init_locked(int *error);
 
 static inline bool sev_version_greater_or_equal(u8 maj, u8 min)
 {
@@ -965,7 +966,8 @@ static int __sev_platform_init_locked(int *error)
 {
 	struct psp_device *psp = psp_master;
 	struct sev_device *sev;
-	int rc = 0, psp_ret = -1;
+	int psp_ret = -1;
+	int rc;
 	int (*init_function)(int *error);
 
 	if (!psp || !psp->sev_data)
@@ -976,6 +978,18 @@ static int __sev_platform_init_locked(int *error)
 	if (sev->state == SEV_STATE_INIT)
 		return 0;
 
+	rc = __sev_snp_init_locked(error);
+	if (rc < 0 && rc != -ENODEV)
+		return rc;
+
+	if (!sev_es_tmr) {
+		/* Obtain the TMR memory area for SEV-ES use */
+		sev_es_tmr = sev_fw_alloc(sev_es_tmr_size);
+		if (!sev_es_tmr)
+			dev_warn(sev->dev,
+				 "SEV: TMR allocation failed, SEV-ES support unavailable\n");
+	}
+
 	if (sev_init_ex_buffer) {
 		init_function = __sev_init_ex_locked;
 		rc = sev_read_init_ex_file();
@@ -1373,6 +1387,9 @@ static int __sev_snp_init_locked(int *error)
 	struct sev_device *sev;
 	int rc = 0;
 
+	if (!cpu_feature_enabled(X86_FEATURE_SEV_SNP))
+		return -ENODEV;
+
 	if (!psp || !psp->sev_data)
 		return -ENODEV;
 
@@ -1457,24 +1474,6 @@ static int __sev_snp_init_locked(int *error)
 	return rc;
 }
 
-int sev_snp_init(int *error, bool init_on_probe)
-{
-	int rc;
-
-	if (!cpu_feature_enabled(X86_FEATURE_SEV_SNP))
-		return -ENODEV;
-
-	if (init_on_probe && !psp_init_on_probe)
-		return 0;
-
-	mutex_lock(&sev_cmd_mutex);
-	rc = __sev_snp_init_locked(error);
-	mutex_unlock(&sev_cmd_mutex);
-
-	return rc;
-}
-EXPORT_SYMBOL_GPL(sev_snp_init);
-
 static int __sev_snp_shutdown_locked(int *error)
 {
 	struct sev_device *sev = psp_master->sev_data;
@@ -2319,14 +2318,6 @@ void sev_pci_init(void)
 		}
 	}
 
-	rc = sev_snp_init(&error, true);
-	if (rc)
-		/*
-		 * Don't abort the probe if SNP INIT failed,
-		 * continue to initialize the legacy SEV firmware.
-		 */
-		dev_err(sev->dev, "SEV-SNP: failed to INIT error %#x\n", error);
-
 	/*
 	 * If boot CPU supports SNP, then first attempt to initialize
 	 * the SNP firmware.
@@ -2341,12 +2332,6 @@ void sev_pci_init(void)
 		}
 	}
 
-	/* Obtain the TMR memory area for SEV-ES use */
-	sev_es_tmr = sev_fw_alloc(sev_es_tmr_size);
-	if (!sev_es_tmr)
-		dev_warn(sev->dev,
-			 "SEV: TMR allocation failed, SEV-ES support unavailable\n");
-
 	if (!psp_init_on_probe)
 		return;
 
diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h
index 970a9de0ed20..ef0c6941a8f4 100644
--- a/include/linux/psp-sev.h
+++ b/include/linux/psp-sev.h
@@ -794,21 +794,6 @@ struct sev_data_snp_shutdown_ex {
  */
 int sev_platform_init(int *error);
 
-/**
- * sev_snp_init - perform SEV SNP_INIT command
- *
- * @error: SEV command return code
- * @init_on_probe: indicates if called during module probe/init
- *
- * Returns:
- * 0 if the SEV successfully processed the command
- * -%ENODEV    if the SEV device is not available
- * -%ENOTSUPP  if the SEV does not support SEV
- * -%ETIMEDOUT if the SEV command timed out
- * -%EIO       if the SEV returned a non-zero return code
- */
-int sev_snp_init(int *error, bool init_on_probe);
-
 /**
  * sev_platform_status - perform SEV PLATFORM_STATUS command
  *
-- 
2.38.1

Re: [PATCH RFC 8/8] crypto: ccp: Move __sev_snp_init_locked() call inside __sev_platform_init_locked()

[Jarkko Sakkinen]

On Fri, Jan 27, 2023 at 02:52:37AM +0000, Jarkko Sakkinen wrote:
> The following functions end up calling sev_platform_init() or
> __sev_platform_init_locked():
> 
> * sev_guest_init()
> * sev_ioctl_do_pek_csr
> * sev_ioctl_do_pdh_export()
> * sev_ioctl_do_pek_import()
> * sev_ioctl_do_pek_pdh_gen()
> * sev_pci_init()
> 
> Only sev_guest_init() and sev_pci_init() also call sev_snp_init().
> Address this by calling __sev_snp_init_locked() inside
> __sev_platform_init_locked() before any other initialization.
> 
> Signed-off-by: Jarkko Sakkinen <jarkko@profian.com>
> ---
>  arch/x86/kvm/svm/sev.c       |  4 +--
>  drivers/crypto/ccp/sev-dev.c | 51 +++++++++++++-----------------------
>  include/linux/psp-sev.h      | 15 -----------
>  3 files changed, 19 insertions(+), 51 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 5e4666b79689..2dd56f59fc50 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -343,11 +343,9 @@ static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp)
>  			goto e_free;
>  
>  		mutex_init(&sev->guest_req_lock);
> -		ret = sev_snp_init(&argp->error, false);
> -	} else {
> -		ret = sev_platform_init(&argp->error);
>  	}
>  
> +	ret = sev_platform_init(&argp->error);
>  	if (ret)
>  		goto e_free;
>  
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index 50e73df966ec..be040926f66a 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -102,6 +102,7 @@ struct sev_data_range_list *snp_range_list;
>  static size_t sev_es_tmr_size = SEV_ES_TMR_SIZE;
>  
>  static int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret);
> +static int __sev_snp_init_locked(int *error);
>  
>  static inline bool sev_version_greater_or_equal(u8 maj, u8 min)
>  {
> @@ -965,7 +966,8 @@ static int __sev_platform_init_locked(int *error)
>  {
>  	struct psp_device *psp = psp_master;
>  	struct sev_device *sev;
> -	int rc = 0, psp_ret = -1;
> +	int psp_ret = -1;
> +	int rc;
>  	int (*init_function)(int *error);
>  
>  	if (!psp || !psp->sev_data)
> @@ -976,6 +978,18 @@ static int __sev_platform_init_locked(int *error)
>  	if (sev->state == SEV_STATE_INIT)
>  		return 0;
>  
> +	rc = __sev_snp_init_locked(error);
> +	if (rc < 0 && rc != -ENODEV)
> +		return rc;
> +
> +	if (!sev_es_tmr) {
> +		/* Obtain the TMR memory area for SEV-ES use */
> +		sev_es_tmr = sev_fw_alloc(sev_es_tmr_size);
> +		if (!sev_es_tmr)
> +			dev_warn(sev->dev,
> +				 "SEV: TMR allocation failed, SEV-ES support unavailable\n");
> +	}
> +
>  	if (sev_init_ex_buffer) {
>  		init_function = __sev_init_ex_locked;
>  		rc = sev_read_init_ex_file();
> @@ -1373,6 +1387,9 @@ static int __sev_snp_init_locked(int *error)
>  	struct sev_device *sev;
>  	int rc = 0;
>  
> +	if (!cpu_feature_enabled(X86_FEATURE_SEV_SNP))
> +		return -ENODEV;
> +
>  	if (!psp || !psp->sev_data)
>  		return -ENODEV;
>  
> @@ -1457,24 +1474,6 @@ static int __sev_snp_init_locked(int *error)
>  	return rc;
>  }
>  
> -int sev_snp_init(int *error, bool init_on_probe)
> -{
> -	int rc;
> -
> -	if (!cpu_feature_enabled(X86_FEATURE_SEV_SNP))
> -		return -ENODEV;
> -
> -	if (init_on_probe && !psp_init_on_probe)
> -		return 0;
> -
> -	mutex_lock(&sev_cmd_mutex);
> -	rc = __sev_snp_init_locked(error);
> -	mutex_unlock(&sev_cmd_mutex);
> -
> -	return rc;
> -}
> -EXPORT_SYMBOL_GPL(sev_snp_init);
> -
>  static int __sev_snp_shutdown_locked(int *error)
>  {
>  	struct sev_device *sev = psp_master->sev_data;
> @@ -2319,14 +2318,6 @@ void sev_pci_init(void)
>  		}
>  	}
>  
> -	rc = sev_snp_init(&error, true);
> -	if (rc)
> -		/*
> -		 * Don't abort the probe if SNP INIT failed,
> -		 * continue to initialize the legacy SEV firmware.
> -		 */
> -		dev_err(sev->dev, "SEV-SNP: failed to INIT error %#x\n", error);
> -
>  	/*
>  	 * If boot CPU supports SNP, then first attempt to initialize
>  	 * the SNP firmware.
> @@ -2341,12 +2332,6 @@ void sev_pci_init(void)
>  		}
>  	}
>  
> -	/* Obtain the TMR memory area for SEV-ES use */
> -	sev_es_tmr = sev_fw_alloc(sev_es_tmr_size);
> -	if (!sev_es_tmr)
> -		dev_warn(sev->dev,
> -			 "SEV: TMR allocation failed, SEV-ES support unavailable\n");
> -
>  	if (!psp_init_on_probe)
>  		return;
>  
> diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h
> index 970a9de0ed20..ef0c6941a8f4 100644
> --- a/include/linux/psp-sev.h
> +++ b/include/linux/psp-sev.h
> @@ -794,21 +794,6 @@ struct sev_data_snp_shutdown_ex {
>   */
>  int sev_platform_init(int *error);
>  
> -/**
> - * sev_snp_init - perform SEV SNP_INIT command
> - *
> - * @error: SEV command return code
> - * @init_on_probe: indicates if called during module probe/init
> - *
> - * Returns:
> - * 0 if the SEV successfully processed the command
> - * -%ENODEV    if the SEV device is not available
> - * -%ENOTSUPP  if the SEV does not support SEV
> - * -%ETIMEDOUT if the SEV command timed out
> - * -%EIO       if the SEV returned a non-zero return code
> - */
> -int sev_snp_init(int *error, bool init_on_probe);
> -
>  /**
>   * sev_platform_status - perform SEV PLATFORM_STATUS command
>   *
> -- 
> 2.38.1
> 

I tested this with both values for psp_init_on_probe.

BR, Jarkko