Move initializing SEV/SNP functionality to KVM

From: Ashish Kalra <[email protected]>

Move dev_info and dev_err messages related to SEV/SNP initialization and shutdown into __sev_platform_init_locked(), __sev_snp_init_locked() and __sev_platform_shutdown_locked(), __sev_snp_shutdown_locked() so that they don't need to be issued from callers.

This allows both _sev_platform_init_locked() and various SEV/SNP ioctls to call __sev_platform_init_locked(), __sev_snp_init_locked() and __sev_platform_shutdown_locked(), __sev_snp_shutdown_locked() for implicit SEV/SNP initialization and shutdown without additionally printing any errors/success messages.

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

---
 drivers/crypto/ccp/sev-dev.c | 44 ++++++++++++++++++++++++++----------
 1 file changed, 32 insertions(+), 12 deletions(-)

drivers/crypto/ccp/sev-dev.c +26 -8

index 2e87ca0e292a..8962a0dbc66f 100644
@@ -1176,21 +1176,31 @@ static int __sev_snp_init_locked(int *error)
 	wbinvd_on_all_cpus();
 
 	rc = __sev_do_cmd_locked(cmd, arg, error);
-	if (rc)
+	if (rc) {
+		dev_err(sev->dev, "SEV-SNP: %s failed rc %d, error %#x\n",
+			cmd == SEV_CMD_SNP_INIT_EX ? "SNP_INIT_EX" : "SNP_INIT",
+			rc, *error);
 		return rc;
+	}
 
 	/* Prepare for first SNP guest launch after INIT. */
 	wbinvd_on_all_cpus();
 	rc = __sev_do_cmd_locked(SEV_CMD_SNP_DF_FLUSH, NULL, error);
-	if (rc)
+	if (rc) {
+		dev_err(sev->dev, "SEV-SNP: SNP_DF_FLUSH failed rc %d, error %#x\n",
+			rc, *error);
 		return rc;
+	}
 
 	sev->snp_initialized = true;
 	dev_dbg(sev->dev, "SEV-SNP firmware initialized\n");
 
+	dev_info(sev->dev, "SEV-SNP API:%d.%d build:%d\n", sev->api_major,
+		 sev->api_minor, sev->build);
+
 	sev_es_tmr_size = SNP_TMR_SIZE;
 
-	return rc;
+	return 0;
 }
 
 static void __sev_platform_init_handle_tmr(struct sev_device *sev)

@@ -1287,16 +1297,22 @@ static int __sev_platform_init_locked(int *error)
 	if (error)
 		*error = psp_ret;
 
-	if (rc)
+	if (rc) {
+		dev_err(sev->dev, "SEV: %s failed %#x, rc %d\n",
+			sev_init_ex_buffer ? "INIT_EX" : "INIT", psp_ret, rc);
 		return rc;
+	}
 
 	sev->state = SEV_STATE_INIT;
 
 	/* Prepare for first SEV guest launch after INIT */
 	wbinvd_on_all_cpus();
 	rc = __sev_do_cmd_locked(SEV_CMD_DF_FLUSH, NULL, error);
-	if (rc)
+	if (rc) {
+		dev_err(sev->dev, "SEV: DF_FLUSH failed %#x, rc %d\n",
+			*error, rc);
 		return rc;
+	}
 
 	dev_dbg(sev->dev, "SEV firmware initialized\n");
 

@@ -1329,8 +1345,7 @@ static int _sev_platform_init_locked(struct sev_platform_init_args *args)
 		 * 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 rc %d, error %#x\n",
-			rc, args->error);
+		dev_err(sev->dev, "SEV-SNP: failed to INIT, continue SEV INIT\n");
 	}
 
 	/* Defer legacy SEV/SEV-ES support if allowed by caller/module. */

@@ -1367,8 +1382,11 @@ static int __sev_platform_shutdown_locked(int *error)
 		return 0;
 
 	ret = __sev_do_cmd_locked(SEV_CMD_SHUTDOWN, NULL, error);
-	if (ret)
+	if (ret) {
+		dev_err(sev->dev, "SEV: failed to SHUTDOWN error %#x, rc %d\n",
+			*error, ret);
 		return ret;
+	}
 
 	sev->state = SEV_STATE_UNINIT;
 	dev_dbg(sev->dev, "SEV firmware shutdown\n");

drivers/crypto/ccp/sev-dev.c +5 -3

 	if (!psp || !psp->sev_data)
 		return 0;

@@ -1682,9 +1700,10 @@ static int __sev_snp_shutdown_locked(int *error, bool panic)
 	ret = __sev_do_cmd_locked(SEV_CMD_SNP_SHUTDOWN_EX, &data, error);
 	/* SHUTDOWN may require DF_FLUSH */
 	if (*error == SEV_RET_DFFLUSH_REQUIRED) {
-		ret = __sev_do_cmd_locked(SEV_CMD_SNP_DF_FLUSH, NULL, NULL);
+		ret = __sev_do_cmd_locked(SEV_CMD_SNP_DF_FLUSH, NULL, &psp_error);
 		if (ret) {
-			dev_err(sev->dev, "SEV-SNP DF_FLUSH failed\n");
+			dev_err(sev->dev, "SEV-SNP DF_FLUSH failed, ret = %d, error = %#x\n",
+				ret, psp_error);
 			return ret;
 		}
 		/* reissue the shutdown command */

@@ -1692,7 +1711,8 @@ static int __sev_snp_shutdown_locked(int *error, bool panic)
 					  error);
 	}
 	if (ret) {
-		dev_err(sev->dev, "SEV-SNP firmware shutdown failed\n");
+		dev_err(sev->dev, "SEV-SNP firmware shutdown failed, rc %d, error %#x\n",
+			ret, *error);
 		return ret;
 	}
 

From: Ashish Kalra <[email protected]>

Modify the behavior of implicit SEV initialization in some of the SEV ioctls to do both SEV initialization and shutdown and add implicit SNP initialization and shutdown to some of the SNP ioctls so that the change of SEV/SNP platform initialization not being done during PSP driver probe time does not break userspace tools such as sevtool, etc.

Prior to this patch, SEV has always been initialized before these ioctls as SEV initialization is done as part of PSP module probe, but now with SEV initialization being moved to KVM module load instead of PSP driver probe, the implied SEV INIT actually makes sense and gets used and additionally to maintain SEV platform state consistency before and after the ioctl SEV shutdown needs to be done after the firmware call.

It is important to do SEV Shutdown here with the SEV/SNP initialization moving to KVM, an implicit SEV INIT here as part of the SEV ioctls not followed with SEV Shutdown will cause SEV to remain in INIT state and then a future SNP INIT in KVM module load will fail.

Similarly, prior to this patch, SNP has always been initialized before these ioctls as SNP initialization is done as part of PSP module probe, therefore, to keep a consistent behavior, SNP init needs to be done here implicitly as part of these ioctls followed with SNP shutdown before returning from the ioctl to maintain the consistent platform state before and after the ioctl.

Suggested-by Tom Lendacky <[email protected]>

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

---
 drivers/crypto/ccp/sev-dev.c | 145 +++++++++++++++++++++++++++--------
 1 file changed, 115 insertions(+), 30 deletions(-)

drivers/crypto/ccp/sev-dev.c +2 -1

index 8962a0dbc66f..14847f1c05fc 100644
@@ -1459,28 +1459,38 @@ static int sev_ioctl_do_platform_status(struct sev_issue_cmd *argp)
 static int sev_ioctl_do_pek_pdh_gen(int cmd, struct sev_issue_cmd *argp, bool writable)
 {
 	struct sev_device *sev = psp_master->sev_data;
-	int rc;
+	bool shutdown_required = false;
+	int rc, error;
 
 	if (!writable)
 		return -EPERM;
 

drivers/crypto/ccp/sev-dev.c +108 -27

-	return __sev_do_cmd_locked(cmd, NULL, &argp->error);
+	rc = __sev_do_cmd_locked(cmd, NULL, &argp->error);
+
+	if (shutdown_required)
+		__sev_platform_shutdown_locked(&error);
+
+	return rc;
 }
 
 static int sev_ioctl_do_pek_csr(struct sev_issue_cmd *argp, bool writable)
 {
 	struct sev_device *sev = psp_master->sev_data;
 	struct sev_user_data_pek_csr input;
+	bool shutdown_required = false;
 	struct sev_data_pek_csr data;
 	void __user *input_address;
 	void *blob = NULL;
-	int ret;
+	int ret, error;
 
 	if (!writable)
 		return -EPERM;

@@ -1508,9 +1518,12 @@ static int sev_ioctl_do_pek_csr(struct sev_issue_cmd *argp, bool writable)
 
 cmd:
 	if (sev->state == SEV_STATE_UNINIT) {
-		ret = __sev_platform_init_locked(&argp->error);
-		if (ret)
+		ret = __sev_platform_init_locked(&error);
+		if (ret) {
+			argp->error = SEV_RET_INVALID_PLATFORM_STATE;
 			goto e_free_blob;
+		}
+		shutdown_required = true;
 	}
 
 	ret = __sev_do_cmd_locked(SEV_CMD_PEK_CSR, &data, &argp->error);

@@ -1529,6 +1542,9 @@ static int sev_ioctl_do_pek_csr(struct sev_issue_cmd *argp, bool writable)
 	}
 
 e_free_blob:
+	if (shutdown_required)
+		__sev_platform_shutdown_locked(&error);
+
 	kfree(blob);
 	return ret;
 }

@@ -1746,8 +1762,9 @@ static int sev_ioctl_do_pek_import(struct sev_issue_cmd *argp, bool writable)
 	struct sev_device *sev = psp_master->sev_data;
 	struct sev_user_data_pek_cert_import input;
 	struct sev_data_pek_cert_import data;
+	bool shutdown_required = false;
 	void *pek_blob, *oca_blob;
-	int ret;
+	int ret, error;
 
 	if (!writable)
 		return -EPERM;

@@ -1776,14 +1793,20 @@ static int sev_ioctl_do_pek_import(struct sev_issue_cmd *argp, bool writable)
 
 	/* If platform is not in INIT state then transition it to INIT */
 	if (sev->state != SEV_STATE_INIT) {
-		ret = __sev_platform_init_locked(&argp->error);
-		if (ret)
+		ret = __sev_platform_init_locked(&error);
+		if (ret) {
+			argp->error = SEV_RET_INVALID_PLATFORM_STATE;
 			goto e_free_oca;
+		}
+		shutdown_required = true;
 	}
 
 	ret = __sev_do_cmd_locked(SEV_CMD_PEK_CERT_IMPORT, &data, &argp->error);
 
 e_free_oca:
+	if (shutdown_required)
+		__sev_platform_shutdown_locked(&error);
+
 	kfree(oca_blob);
 e_free_pek:
 	kfree(pek_blob);

@@ -1900,17 +1923,8 @@ static int sev_ioctl_do_pdh_export(struct sev_issue_cmd *argp, bool writable)
 	struct sev_data_pdh_cert_export data;
 	void __user *input_cert_chain_address;
 	void __user *input_pdh_cert_address;
-	int ret;
-
-	/* If platform is not in INIT state then transition it to INIT. */
-	if (sev->state != SEV_STATE_INIT) {
-		if (!writable)
-			return -EPERM;
-
-		ret = __sev_platform_init_locked(&argp->error);
-		if (ret)
-			return ret;
-	}
+	bool shutdown_required = false;
+	int ret, error;
 
 	if (copy_from_user(&input, (void __user *)argp->data, sizeof(input)))
 		return -EFAULT;

@@ -1951,6 +1965,18 @@ static int sev_ioctl_do_pdh_export(struct sev_issue_cmd *argp, bool writable)
 	data.cert_chain_len = input.cert_chain_len;
 
 cmd:
+	/* If platform is not in INIT state then transition it to INIT. */
+	if (sev->state != SEV_STATE_INIT) {
+		if (!writable)
+			goto e_free_cert;
+		ret = __sev_platform_init_locked(&error);
+		if (ret) {
+			argp->error = SEV_RET_INVALID_PLATFORM_STATE;
+			goto e_free_cert;
+		}
+		shutdown_required = true;
+	}
+
 	ret = __sev_do_cmd_locked(SEV_CMD_PDH_CERT_EXPORT, &data, &argp->error);
 
 	/* If we query the length, FW responded with expected data. */

@@ -1977,6 +2003,9 @@ static int sev_ioctl_do_pdh_export(struct sev_issue_cmd *argp, bool writable)
 	}
 
 e_free_cert:
+	if (shutdown_required)
+		__sev_platform_shutdown_locked(&error);
+
 	kfree(cert_blob);
 e_free_pdh:
 	kfree(pdh_blob);

@@ -1986,12 +2015,13 @@ static int sev_ioctl_do_pdh_export(struct sev_issue_cmd *argp, bool writable)
 static int sev_ioctl_do_snp_platform_status(struct sev_issue_cmd *argp)
 {
 	struct sev_device *sev = psp_master->sev_data;
+	bool shutdown_required = false;
 	struct sev_data_snp_addr buf;
 	struct page *status_page;
+	int ret, error;
 	void *data;
-	int ret;
 
-	if (!sev->snp_initialized || !argp->data)
+	if (!argp->data)
 		return -EINVAL;
 
 	status_page = alloc_page(GFP_KERNEL_ACCOUNT);

@@ -2000,6 +2030,15 @@ static int sev_ioctl_do_snp_platform_status(struct sev_issue_cmd *argp)
 
 	data = page_address(status_page);
 
+	if (!sev->snp_initialized) {
+		ret = __sev_snp_init_locked(&error);
+		if (ret) {
+			argp->error = SEV_RET_INVALID_PLATFORM_STATE;
+			goto cleanup;
+		}
+		shutdown_required = true;
+	}
+
 	/*
 	 * Firmware expects status page to be in firmware-owned state, otherwise
 	 * it will report firmware error code INVALID_PAGE_STATE (0x1A).

@@ -2028,6 +2067,9 @@ static int sev_ioctl_do_snp_platform_status(struct sev_issue_cmd *argp)
 		ret = -EFAULT;
 
 cleanup:
+	if (shutdown_required)
+		__sev_snp_shutdown_locked(&error, false);
+
 	__free_pages(status_page, 0);
 	return ret;
 }

@@ -2036,21 +2078,36 @@ static int sev_ioctl_do_snp_commit(struct sev_issue_cmd *argp)
 {
 	struct sev_device *sev = psp_master->sev_data;
 	struct sev_data_snp_commit buf;
+	bool shutdown_required = false;
+	int ret, error;
 
-	if (!sev->snp_initialized)
-		return -EINVAL;
+	if (!sev->snp_initialized) {
+		ret = __sev_snp_init_locked(&error);
+		if (ret) {
+			argp->error = SEV_RET_INVALID_PLATFORM_STATE;
+			return ret;
+		}
+		shutdown_required = true;
+	}
 
 	buf.len = sizeof(buf);
 
-	return __sev_do_cmd_locked(SEV_CMD_SNP_COMMIT, &buf, &argp->error);
+	ret = __sev_do_cmd_locked(SEV_CMD_SNP_COMMIT, &buf, &argp->error);
+
+	if (shutdown_required)
+		__sev_snp_shutdown_locked(&error, false);
+
+	return ret;
 }
 
 static int sev_ioctl_do_snp_set_config(struct sev_issue_cmd *argp, bool writable)
 {
 	struct sev_device *sev = psp_master->sev_data;
 	struct sev_user_data_snp_config config;
+	bool shutdown_required = false;
+	int ret, error;
 
-	if (!sev->snp_initialized || !argp->data)
+	if (!argp->data)
 		return -EINVAL;
 
 	if (!writable)

@@ -2059,17 +2116,32 @@ static int sev_ioctl_do_snp_set_config(struct sev_issue_cmd *argp, bool writable
 	if (copy_from_user(&config, (void __user *)argp->data, sizeof(config)))
 		return -EFAULT;
 
-	return __sev_do_cmd_locked(SEV_CMD_SNP_CONFIG, &config, &argp->error);
+	if (!sev->snp_initialized) {
+		ret = __sev_snp_init_locked(&error);
+		if (ret) {
+			argp->error = SEV_RET_INVALID_PLATFORM_STATE;
+			return ret;
+		}
+		shutdown_required = true;
+	}
+
+	ret = __sev_do_cmd_locked(SEV_CMD_SNP_CONFIG, &config, &argp->error);
+
+	if (shutdown_required)
+		__sev_snp_shutdown_locked(&error, false);
+
+	return ret;
 }
 
 static int sev_ioctl_do_snp_vlek_load(struct sev_issue_cmd *argp, bool writable)
 {
 	struct sev_device *sev = psp_master->sev_data;
 	struct sev_user_data_snp_vlek_load input;
+	bool shutdown_required = false;
+	int ret, error;
 	void *blob;
-	int ret;
 
-	if (!sev->snp_initialized || !argp->data)
+	if (!argp->data)
 		return -EINVAL;
 
 	if (!writable)

@@ -2088,8 +2160,21 @@ static int sev_ioctl_do_snp_vlek_load(struct sev_issue_cmd *argp, bool writable)
 
 	input.vlek_wrapped_address = __psp_pa(blob);
 
+	if (!sev->snp_initialized) {
+		ret = __sev_snp_init_locked(&error);
+		if (ret) {
+			argp->error = SEV_RET_INVALID_PLATFORM_STATE;
+			goto cleanup;
+		}
+		shutdown_required = true;
+	}
+
 	ret = __sev_do_cmd_locked(SEV_CMD_SNP_VLEK_LOAD, &input, &argp->error);
 
+	if (shutdown_required)
+		__sev_snp_shutdown_locked(&error, false);
+
+cleanup:
 	kfree(blob);
 
 	return ret;

From: Ashish Kalra <[email protected]>

Currently, the SNP panic notifier is registered on module initialization regardless of whether SNP is being enabled or initialized.

Instead, register the SNP panic notifier only when SNP is actually initialized and unregister the notifier when SNP is shutdown.

Reviewed-by Dionna Glaze <[email protected]>

Reviewed-by Alexey Kardashevskiy <[email protected]>

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

---
 drivers/crypto/ccp/sev-dev.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

drivers/crypto/ccp/sev-dev.c +13 -9

index c784de6c77c3..b3479a2896d0 100644
@@ -109,6 +109,13 @@ static void *sev_init_ex_buffer;
  */
 static struct sev_data_range_list *snp_range_list;
 
+static int snp_shutdown_on_panic(struct notifier_block *nb,
+				 unsigned long reason, void *arg);
+
+static struct notifier_block snp_panic_notifier = {
+	.notifier_call = snp_shutdown_on_panic,
+};
+
 static inline bool sev_version_greater_or_equal(u8 maj, u8 min)
 {
 	struct sev_device *sev = psp_master->sev_data;

@@ -1198,6 +1205,9 @@ static int __sev_snp_init_locked(int *error)
 	dev_info(sev->dev, "SEV-SNP API:%d.%d build:%d\n", sev->api_major,
 		 sev->api_minor, sev->build);
 
+	atomic_notifier_chain_register(&panic_notifier_list,
+				       &snp_panic_notifier);
+
 	sev_es_tmr_size = SNP_TMR_SIZE;
 
 	return 0;

@@ -1754,6 +1764,9 @@ static int __sev_snp_shutdown_locked(int *error, bool panic)
 	sev->snp_initialized = false;
 	dev_dbg(sev->dev, "SEV-SNP firmware shutdown\n");
 
+	atomic_notifier_chain_unregister(&panic_notifier_list,
+					 &snp_panic_notifier);
+
 	/* Reset TMR size back to default */
 	sev_es_tmr_size = SEV_TMR_SIZE;
 

@@ -2481,10 +2494,6 @@ static int snp_shutdown_on_panic(struct notifier_block *nb,
 	return NOTIFY_DONE;
 }
 
-static struct notifier_block snp_panic_notifier = {
-	.notifier_call = snp_shutdown_on_panic,
-};
-
 int sev_issue_cmd_external_user(struct file *filep, unsigned int cmd,
 				void *data, int *error)
 {

@@ -2533,8 +2542,6 @@ void sev_pci_init(void)
 	dev_info(sev->dev, "SEV%s API:%d.%d build:%d\n", sev->snp_initialized ?
 		"-SNP" : "", sev->api_major, sev->api_minor, sev->build);
 
-	atomic_notifier_chain_register(&panic_notifier_list,
-				       &snp_panic_notifier);
 	return;
 
 err:

@@ -2551,7 +2558,4 @@ void sev_pci_exit(void)
 		return;
 
 	sev_firmware_shutdown(sev);
-
-	atomic_notifier_chain_unregister(&panic_notifier_list,
-					 &snp_panic_notifier);
 }

From: Ashish Kalra <[email protected]>

Move platform initialization of SEV/SNP from CCP driver probe time to KVM module load time so that KVM can do SEV/SNP platform initialization explicitly if it actually wants to use SEV/SNP functionality.

Add support for KVM to explicitly call into the CCP driver at load time to initialize SEV/SNP. If required, this behavior can be altered with KVM module parameters to not do SEV/SNP platform initialization at module load time. Additionally, a corresponding SEV/SNP platform shutdown is invoked during KVM module unload time.

Suggested-by Sean Christopherson <[email protected]>

Reviewed-by Tom Lendacky <[email protected]>

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

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

arch/x86/kvm/svm/sev.c +1 -0

index 74525651770a..0bc6c0486071 100644
@@ -2933,6 +2933,7 @@ void __init sev_set_cpu_caps(void)
 void __init sev_hardware_setup(void)
 {
 	unsigned int eax, ebx, ecx, edx, sev_asid_count, sev_es_asid_count;
+	struct sev_platform_init_args init_args = {0};
 	bool sev_snp_supported = false;
 	bool sev_es_supported = false;
 	bool sev_supported = false;

@@ -3059,6 +3060,17 @@ void __init sev_hardware_setup(void)
 	sev_supported_vmsa_features = 0;

arch/x86/kvm/svm/sev.c +1 -0

+	sev_platform_shutdown();
 }
 
 int sev_cpu_init(struct svm_cpu_data *sd)

Signature · 1 line

2.34.1

Quoted 33 lines · Ashish Kalra

From: Ashish Kalra <[email protected]>

Move platform initialization of SEV/SNP from CCP driver probe time to KVM module load time so that KVM can do SEV/SNP platform initialization explicitly if it actually wants to use SEV/SNP functionality.

Add support for KVM to explicitly call into the CCP driver at load time to initialize SEV/SNP. If required, this behavior can be altered with KVM module parameters to not do SEV/SNP platform initialization at module load time. Additionally, a corresponding SEV/SNP platform shutdown is invoked during KVM module unload time.

Suggested-by Sean Christopherson <[email protected]>

Reviewed-by Tom Lendacky <[email protected]>

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

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

arch/x86/kvm/svm/sev.c +1 -0

index 74525651770a..0bc6c0486071 100644
@@ -2933,6 +2933,7 @@ void __init sev_set_cpu_caps(void)
 void __init sev_hardware_setup(void)
 {
 	unsigned int eax, ebx, ecx, edx, sev_asid_count, sev_es_asid_count;
+	struct sev_platform_init_args init_args = {0};
 	bool sev_snp_supported = false;
 	bool sev_es_supported = false;
 	bool sev_supported = false;

@@ -3059,6 +3060,17 @@ void __init sev_hardware_setup(void)
 	sev_supported_vmsa_features = 0;

arch/x86/kvm/svm/sev.c void __init sev_hardware_setup(void) · line 3070

Quoted 10 lines

arch/x86/kvm/svm/sev.c +8 -0

 	if (sev_es_debug_swap_enabled)
 		sev_supported_vmsa_features |= SVM_SEV_FEAT_DEBUG_SWAP;
+
+	if (!sev_enabled)
+		return;
+
+	/*
+	 * Always perform SEV initialization at setup time to avoid
+	 * complications when performing SEV initialization later
+	 * (such as suspending active guests, etc.).

This is misleading and wildly incomplete. *SEV* doesn't have complications, *SNP* has complications. And looking through sev_platform_init(), all of this code is buggy.

The sev_platform_init() return code is completely disconnected from SNP setup. It can return errors even if SNP setup succeeds, and can return success even if SNP setup fails.

I also think it makes sense to require SNP to be initialized during KVM setup.

I don't see anything in __sev_snp_init_locked() that suggests SNP initialization can magically succeed at runtime if it failed at boot. To keep things sane and simple, I think KVM should reject module load if SNP is requested, setup fails, and kvm-amd.ko is a module. If kvm-amd.ko is built-in and SNP fails, just disable SNP support. I.e. when possible, let userspace decide what to do, but don't bring down all of KVM just because SNP setup failed.

The attached patches are compile-tested (mostly), can you please test them and slot them in?

arch/x86/kvm/svm/sev.c void sev_hardware_unsetup(void) · line 3090

Quoted 12 lines

arch/x86/kvm/svm/sev.c +5 -0

+	 */
+	init_args.probe = true;
+	sev_platform_init(&init_args);
 }
 
 void sev_hardware_unsetup(void)

@@ -3074,6 +3086,9 @@ void sev_hardware_unsetup(void)
 
 	misc_cg_set_capacity(MISC_CG_RES_SEV, 0);
 	misc_cg_set_capacity(MISC_CG_RES_SEV_ES, 0);
+
+	/* Do SEV and SNP Shutdown */

Meh, just omit this comment.

Quoted 7 lines

arch/x86/kvm/svm/sev.c +1 -0

+	sev_platform_shutdown();
 }
 
 int sev_cpu_init(struct svm_cpu_data *sd)

Signature · 1 line

2.34.1

Hello Sean,

Quoted 13 lines · Sean Christopherson

Review

Quoted 7 lines · Ashish Kalra

(quoted diff) +7 -0

+	if (!sev_enabled)
+		return;
+
+	/*
+	 * Always perform SEV initialization at setup time to avoid
+	 * complications when performing SEV initialization later
+	 * (such as suspending active guests, etc.).

This is misleading and wildly incomplete. *SEV* doesn't have complications, *SNP* has complications. And looking through sev_platform_init(), all of this code is buggy.

Quoted 5 lines

The sev_platform_init() return code is completely disconnected from SNP setup. It can return errors even if SNP setup succeeds, and can return success even if SNP setup fails.

I also think it makes sense to require SNP to be initialized during KVM setup.

There are a few important considerations here:

This is true that we require SNP to be initialized during KVM setup and also as mentioned earlier we need SNP to be initialized (SNP_INIT_EX should be done) for SEV INIT to succeed if SNP host support is enabled.

So we essentially have to do SNP_INIT(_EX) for launching SEV/SEV-ES VMs when SNP host support is enabled. In other words, if SNP_INIT(_EX) is not issued or fails then SEV/SEV-ES VMs can't be launched once SNP host support (SYSCFG.SNPEn) is enabled as SEV INIT will fail in such a situation.

And the other consideration is that runtime setup of especially SEV-ES VMs will not work if/when first SEV-ES VM is launched, if SEV INIT has not been issued at KVM setup time.

This is because qemu has a check for SEV INIT to have been done (via SEV platform status command) prior to launching SEV-ES VMs via KVM_SEV_INIT2 ioctl.

So effectively, __sev_guest_init() does not get invoked in case of launching SEV_ES VMs, if sev_platform_init() has not been done to issue SEV INIT in sev_hardware_setup().

In other words the deferred initialization only works for SEV VMs and not SEV-ES VMs.

Quoted 24 lines · Ashish Kalra

Hello Sean,

Quoted 13 lines · Sean Christopherson

Review

Quoted 7 lines · Ashish Kalra

(quoted diff) +7 -0

+	if (!sev_enabled)
+		return;
+
+	/*
+	 * Always perform SEV initialization at setup time to avoid
+	 * complications when performing SEV initialization later
+	 * (such as suspending active guests, etc.).

This is misleading and wildly incomplete. *SEV* doesn't have complications, *SNP* has complications. And looking through sev_platform_init(), all of this code is buggy.

Quoted 5 lines

The sev_platform_init() return code is completely disconnected from SNP setup. It can return errors even if SNP setup succeeds, and can return success even if SNP setup fails.

I also think it makes sense to require SNP to be initialized during KVM setup.

There are a few important considerations here:

Quoted 8 lines

This is true that we require SNP to be initialized during KVM setup and also as mentioned earlier we need SNP to be initialized (SNP_INIT_EX should be done) for SEV INIT to succeed if SNP host support is enabled.

So we essentially have to do SNP_INIT(_EX) for launching SEV/SEV-ES VMs when SNP host support is enabled. In other words, if SNP_INIT(_EX) is not issued or fails then SEV/SEV-ES VMs can't be launched once SNP host support (SYSCFG.SNPEn) is enabled as SEV INIT will fail in such a situation.

Doesn't that mean sev_platform_init() is broken and should error out if SNP setup fails? Because this doesn't match the above (or I'm misreading one or both).

	rc = __sev_snp_init_locked(&args->error);
	if (rc && rc != -ENODEV) {
		/*
		 * 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, continue SEV INIT\n");
	}

And doesn't the min version check completely wreck everything? I.e. if SNP *must* be initialized if SYSCFG.SNPEn is set in order to utilize SEV/SEV-ES, then shouldn't this be a fatal error too?

	if (!sev_version_greater_or_equal(SNP_MIN_API_MAJOR, SNP_MIN_API_MINOR)) {
		dev_dbg(sev->dev, "SEV-SNP support requires firmware version >= %d:%d\n",
			SNP_MIN_API_MAJOR, SNP_MIN_API_MINOR);
		return 0;
	}

And then aren't all of the bare calls to __sev_platform_init_locked() broken too? E.g. if userspace calls sev_ioctl_do_pek_csr() without loading KVM, then SNP won't be initialized and __sev_platform_init_locked() will fail, no?

Quoted 12 lines

And the other consideration is that runtime setup of especially SEV-ES VMs will not work if/when first SEV-ES VM is launched, if SEV INIT has not been issued at KVM setup time.

This is because qemu has a check for SEV INIT to have been done (via SEV platform status command) prior to launching SEV-ES VMs via KVM_SEV_INIT2 ioctl.

So effectively, __sev_guest_init() does not get invoked in case of launching SEV_ES VMs, if sev_platform_init() has not been done to issue SEV INIT in sev_hardware_setup().

Quoted 1 line

In other words the deferred initialization only works for SEV VMs and not SEV-ES VMs.

In that case, I vote to kill off deferred initialization entirely, and commit to enabling all of SEV+ when KVM loads (which we should have done from day one). Assuming we can do that in a way that's compatible with the /dev/sev ioctls.

Quoted 12 lines · Sean Christopherson

Quoted 11 lines · Ashish Kalra

And the other consideration is that runtime setup of especially SEV-ES VMs will not work if/when first SEV-ES VM is launched, if SEV INIT has not been issued at KVM setup time.

This is because qemu has a check for SEV INIT to have been done (via SEV platform status command) prior to launching SEV-ES VMs via KVM_SEV_INIT2 ioctl.

So effectively, __sev_guest_init() does not get invoked in case of launching SEV_ES VMs, if sev_platform_init() has not been done to issue SEV INIT in sev_hardware_setup().

Quoted 5 lines

Quoted 1 line

In other words the deferred initialization only works for SEV VMs and not SEV-ES VMs.

In that case, I vote to kill off deferred initialization entirely, and commit to enabling all of SEV+ when KVM loads (which we should have done from day one). Assuming we can do that in a way that's compatible with the /dev/sev ioctls.

Yes, that's what seems to be the right approach to enabling all SEV+ when KVM loads.

For SEV firmware hotloading we will do implicit SEV Shutdown prior to DLFW_EX and SEV (re)INIT after that to ensure that SEV is in UNINIT state before DLFW_EX.

We still probably want to keep the deferred initialization for SEV in __sev_guest_init() by calling sev_platform_init() to support the SEV INIT_EX case.

Quoted 25 lines · Ashish Kalra

Quoted 12 lines · Sean Christopherson

Quoted 11 lines · Ashish Kalra

And the other consideration is that runtime setup of especially SEV-ES VMs will not work if/when first SEV-ES VM is launched, if SEV INIT has not been issued at KVM setup time.

This is because qemu has a check for SEV INIT to have been done (via SEV platform status command) prior to launching SEV-ES VMs via KVM_SEV_INIT2 ioctl.

So effectively, __sev_guest_init() does not get invoked in case of launching SEV_ES VMs, if sev_platform_init() has not been done to issue SEV INIT in sev_hardware_setup().

Quoted 5 lines

Quoted 1 line

In other words the deferred initialization only works for SEV VMs and not SEV-ES VMs.

In that case, I vote to kill off deferred initialization entirely, and commit to enabling all of SEV+ when KVM loads (which we should have done from day one). Assuming we can do that in a way that's compatible with the /dev/sev ioctls.

Yes, that's what seems to be the right approach to enabling all SEV+ when KVM loads.

For SEV firmware hotloading we will do implicit SEV Shutdown prior to DLFW_EX and SEV (re)INIT after that to ensure that SEV is in UNINIT state before DLFW_EX.

Quoted 3 lines

We still probably want to keep the deferred initialization for SEV in __sev_guest_init() by calling sev_platform_init() to support the SEV INIT_EX case.

Refresh me, how does INIT_EX fit into all of this? I.e. why does it need special casing?

Quoted 26 lines · Sean Christopherson

Quoted 25 lines · Ashish Kalra

Quoted 12 lines · Sean Christopherson

Quoted 11 lines · Ashish Kalra

And the other consideration is that runtime setup of especially SEV-ES VMs will not work if/when first SEV-ES VM is launched, if SEV INIT has not been issued at KVM setup time.

This is because qemu has a check for SEV INIT to have been done (via SEV platform status command) prior to launching SEV-ES VMs via KVM_SEV_INIT2 ioctl.

So effectively, __sev_guest_init() does not get invoked in case of launching SEV_ES VMs, if sev_platform_init() has not been done to issue SEV INIT in sev_hardware_setup().

Quoted 5 lines

Quoted 1 line

In other words the deferred initialization only works for SEV VMs and not SEV-ES VMs.

In that case, I vote to kill off deferred initialization entirely, and commit to enabling all of SEV+ when KVM loads (which we should have done from day one). Assuming we can do that in a way that's compatible with the /dev/sev ioctls.

Yes, that's what seems to be the right approach to enabling all SEV+ when KVM loads.

For SEV firmware hotloading we will do implicit SEV Shutdown prior to DLFW_EX and SEV (re)INIT after that to ensure that SEV is in UNINIT state before DLFW_EX.

Quoted 6 lines

Quoted 3 lines

We still probably want to keep the deferred initialization for SEV in __sev_guest_init() by calling sev_platform_init() to support the SEV INIT_EX case.

Refresh me, how does INIT_EX fit into all of this? I.e. why does it need special casing?

For SEV INIT_EX, we need the filesystem to be up and running as the user-supplied SEV related persistent data is read from a regular file and provided to the INIT_EX command.

Now, with the modified SEV/SNP init flow, when SEV/SNP initialization is performed during KVM module load, then as i believe the filesystem will be mounted before KVM module loads, so SEV INIT_EX can be supported without any issues.

Therefore, we don't need deferred initialization support for SEV INIT_EX in case of KVM being loaded as a module.

But if KVM module is built-in, then filesystem will not be mounted when SEV/SNP initialization is done during KVM initialization and in that case SEV INIT_EX cannot be supported.

Therefore to support SEV INIT_EX when KVM module is built-in, the following will need to be done:

- Boot kernel with psp_init_on_probe=false command line.
- This ensures that during KVM initialization, only SNP INIT is done.
- Later at runtime, when filesystem has already been mounted, SEV VM launch will trigger deferred SEV (INIT_EX) initialization (via the __sev_guest_init() -> sev_platform_init() code path).

NOTE: psp_init_on_probe module parameter and deferred SEV initialization during SEV VM launch (__sev_guest_init()->sev_platform_init()) was added specifically to support SEV INIT_EX case.

Quoted 59 lines · Ashish Kalra

Quoted 26 lines · Sean Christopherson

Quoted 25 lines · Ashish Kalra

Quoted 12 lines · Sean Christopherson

Quoted 11 lines · Ashish Kalra

And the other consideration is that runtime setup of especially SEV-ES VMs will not work if/when first SEV-ES VM is launched, if SEV INIT has not been issued at KVM setup time.

This is because qemu has a check for SEV INIT to have been done (via SEV platform status command) prior to launching SEV-ES VMs via KVM_SEV_INIT2 ioctl.

So effectively, __sev_guest_init() does not get invoked in case of launching SEV_ES VMs, if sev_platform_init() has not been done to issue SEV INIT in sev_hardware_setup().

Quoted 5 lines

Quoted 1 line

In other words the deferred initialization only works for SEV VMs and not SEV-ES VMs.

In that case, I vote to kill off deferred initialization entirely, and commit to enabling all of SEV+ when KVM loads (which we should have done from day one). Assuming we can do that in a way that's compatible with the /dev/sev ioctls.

Yes, that's what seems to be the right approach to enabling all SEV+ when KVM loads.

For SEV firmware hotloading we will do implicit SEV Shutdown prior to DLFW_EX and SEV (re)INIT after that to ensure that SEV is in UNINIT state before DLFW_EX.

Quoted 6 lines

Quoted 3 lines

We still probably want to keep the deferred initialization for SEV in __sev_guest_init() by calling sev_platform_init() to support the SEV INIT_EX case.

Refresh me, how does INIT_EX fit into all of this? I.e. why does it need special casing?

For SEV INIT_EX, we need the filesystem to be up and running as the user-supplied SEV related persistent data is read from a regular file and provided to the INIT_EX command.

Now, with the modified SEV/SNP init flow, when SEV/SNP initialization is performed during KVM module load, then as i believe the filesystem will be mounted before KVM module loads, so SEV INIT_EX can be supported without any issues.

Therefore, we don't need deferred initialization support for SEV INIT_EX in case of KVM being loaded as a module.

But if KVM module is built-in, then filesystem will not be mounted when SEV/SNP initialization is done during KVM initialization and in that case SEV INIT_EX cannot be supported.

Therefore to support SEV INIT_EX when KVM module is built-in, the following will need to be done:

- Boot kernel with psp_init_on_probe=false command line.
- This ensures that during KVM initialization, only SNP INIT is done.
- Later at runtime, when filesystem has already been mounted, SEV VM launch will trigger deferred SEV (INIT_EX) initialization (via the __sev_guest_init() -> sev_platform_init() code path).

Quoted 3 lines

NOTE: psp_init_on_probe module parameter and deferred SEV initialization during SEV VM launch (__sev_guest_init()->sev_platform_init()) was added specifically to support SEV INIT_EX case.

Ugh. That's quite the unworkable mess. sev_hardware_setup() can't determine if SEV/SEV-ES is fully supported without initializing the platform, but userspace needs KVM to do initialization so that SEV platform status reads out correctly.

Aha!

Isn't that a Google problem? And one that resolves itself if initialization is done on kvm-amd.ko load?

A system/kernel _could_ be configured to use a path during initcalls, with the approproate initramfs magic. So there's no hard requirement that makes init_ex_path incompatible with CRYPTO_DEV_CCP_DD=y or CONFIG_KVM_AMD=y. Google's environment simply doesn't jump through those hoops.

But Google _does_ build kvm-amd.ko as a module.

So rather than carry a bunch of hard-to-follow code (and potentially impossible constraints), always do initialization at kvm-amd.ko load, and require the platform owner to ensure init_ex_path can be resolved when sev_hardware_setup() runs, i.e. when kvm-amd.ko is loaded or its initcall runs.

Quoted 64 lines · Sean Christopherson

Quoted 59 lines · Ashish Kalra

Quoted 26 lines · Sean Christopherson

Quoted 25 lines · Ashish Kalra

Quoted 12 lines · Sean Christopherson

Quoted 11 lines · Ashish Kalra

And the other consideration is that runtime setup of especially SEV-ES VMs will not work if/when first SEV-ES VM is launched, if SEV INIT has not been issued at KVM setup time.

This is because qemu has a check for SEV INIT to have been done (via SEV platform status command) prior to launching SEV-ES VMs via KVM_SEV_INIT2 ioctl.

So effectively, __sev_guest_init() does not get invoked in case of launching SEV_ES VMs, if sev_platform_init() has not been done to issue SEV INIT in sev_hardware_setup().

Quoted 5 lines

Quoted 1 line

In other words the deferred initialization only works for SEV VMs and not SEV-ES VMs.

In that case, I vote to kill off deferred initialization entirely, and commit to enabling all of SEV+ when KVM loads (which we should have done from day one). Assuming we can do that in a way that's compatible with the /dev/sev ioctls.

Yes, that's what seems to be the right approach to enabling all SEV+ when KVM loads.

For SEV firmware hotloading we will do implicit SEV Shutdown prior to DLFW_EX and SEV (re)INIT after that to ensure that SEV is in UNINIT state before DLFW_EX.

Quoted 6 lines

Quoted 3 lines

We still probably want to keep the deferred initialization for SEV in __sev_guest_init() by calling sev_platform_init() to support the SEV INIT_EX case.

Refresh me, how does INIT_EX fit into all of this? I.e. why does it need special casing?

For SEV INIT_EX, we need the filesystem to be up and running as the user-supplied SEV related persistent data is read from a regular file and provided to the INIT_EX command.

Now, with the modified SEV/SNP init flow, when SEV/SNP initialization is performed during KVM module load, then as i believe the filesystem will be mounted before KVM module loads, so SEV INIT_EX can be supported without any issues.

Therefore, we don't need deferred initialization support for SEV INIT_EX in case of KVM being loaded as a module.

But if KVM module is built-in, then filesystem will not be mounted when SEV/SNP initialization is done during KVM initialization and in that case SEV INIT_EX cannot be supported.

Therefore to support SEV INIT_EX when KVM module is built-in, the following will need to be done:

- Boot kernel with psp_init_on_probe=false command line.
- This ensures that during KVM initialization, only SNP INIT is done.
- Later at runtime, when filesystem has already been mounted, SEV VM launch will trigger deferred SEV (INIT_EX) initialization (via the __sev_guest_init() -> sev_platform_init() code path).

Quoted 3 lines

NOTE: psp_init_on_probe module parameter and deferred SEV initialization during SEV VM launch (__sev_guest_init()->sev_platform_init()) was added specifically to support SEV INIT_EX case.

Quoted 8 lines

Ugh. That's quite the unworkable mess. sev_hardware_setup() can't determine if SEV/SEV-ES is fully supported without initializing the platform, but userspace needs KVM to do initialization so that SEV platform status reads out correctly.

Aha!

Isn't that a Google problem? And one that resolves itself if initialization is done on kvm-amd.ko load?

Yes, SEV INIT_EX is mainly used/required by Google.

Quoted 6 lines

A system/kernel _could_ be configured to use a path during initcalls, with the approproate initramfs magic. So there's no hard requirement that makes init_ex_path incompatible with CRYPTO_DEV_CCP_DD=y or CONFIG_KVM_AMD=y. Google's environment simply doesn't jump through those hoops.

Quoted 6 lines

But Google _does_ build kvm-amd.ko as a module.

So rather than carry a bunch of hard-to-follow code (and potentially impossible constraints), always do initialization at kvm-amd.ko load, and require the platform owner to ensure init_ex_path can be resolved when sev_hardware_setup() runs, i.e. when kvm-amd.ko is loaded or its initcall runs.

So you are proposing that we drop all deferred initialization support for SEV, i.e, we drop the psp_init_on_probe module parameter for CCP driver, remove the probe field from sev_platform_init_args and correspondingly drop any support to skip/defer SEV INIT in _sev_platform_init_locked() and then also drop all existing support in KVM for SEV deferred initialization, i.e, remove the call to sev_platform_init() from __sev_guest_init().

Quoted 59 lines · Ashish Kalra

Quoted 26 lines · Sean Christopherson

Quoted 25 lines · Ashish Kalra

Quoted 12 lines · Sean Christopherson

Quoted 11 lines · Ashish Kalra

And the other consideration is that runtime setup of especially SEV-ES VMs will not work if/when first SEV-ES VM is launched, if SEV INIT has not been issued at KVM setup time.

This is because qemu has a check for SEV INIT to have been done (via SEV platform status command) prior to launching SEV-ES VMs via KVM_SEV_INIT2 ioctl.

So effectively, __sev_guest_init() does not get invoked in case of launching SEV_ES VMs, if sev_platform_init() has not been done to issue SEV INIT in sev_hardware_setup().

Quoted 5 lines

Quoted 1 line

In other words the deferred initialization only works for SEV VMs and not SEV-ES VMs.

In that case, I vote to kill off deferred initialization entirely, and commit to enabling all of SEV+ when KVM loads (which we should have done from day one). Assuming we can do that in a way that's compatible with the /dev/sev ioctls.

Yes, that's what seems to be the right approach to enabling all SEV+ when KVM loads.

For SEV firmware hotloading we will do implicit SEV Shutdown prior to DLFW_EX and SEV (re)INIT after that to ensure that SEV is in UNINIT state before DLFW_EX.

Quoted 6 lines

Quoted 3 lines

We still probably want to keep the deferred initialization for SEV in __sev_guest_init() by calling sev_platform_init() to support the SEV INIT_EX case.

Refresh me, how does INIT_EX fit into all of this? I.e. why does it need special casing?

For SEV INIT_EX, we need the filesystem to be up and running as the user-supplied SEV related persistent data is read from a regular file and provided to the INIT_EX command.

Now, with the modified SEV/SNP init flow, when SEV/SNP initialization is performed during KVM module load, then as i believe the filesystem will be mounted before KVM module loads, so SEV INIT_EX can be supported without any issues.

Therefore, we don't need deferred initialization support for SEV INIT_EX in case of KVM being loaded as a module.

But if KVM module is built-in, then filesystem will not be mounted when SEV/SNP initialization is done during KVM initialization and in that case SEV INIT_EX cannot be supported.

Therefore to support SEV INIT_EX when KVM module is built-in, the following will need to be done:

- Boot kernel with psp_init_on_probe=false command line.
- This ensures that during KVM initialization, only SNP INIT is done.
- Later at runtime, when filesystem has already been mounted, SEV VM launch will trigger deferred SEV (INIT_EX) initialization (via the __sev_guest_init() -> sev_platform_init() code path).

Quoted 3 lines

NOTE: psp_init_on_probe module parameter and deferred SEV initialization during SEV VM launch (__sev_guest_init()->sev_platform_init()) was added specifically to support SEV INIT_EX case.

Ugh. That's quite the unworkable mess. sev_hardware_setup() can't determine if SEV/SEV-ES is fully supported without initializing the platform, but userspace needs KVM to do initialization so that SEV platform status reads out correctly.

Aha!

Isn't that a Google problem? And one that resolves itself if initialization is done on kvm-amd.ko load?

A system/kernel _could_ be configured to use a path during initcalls, with the approproate initramfs magic. So there's no hard requirement that makes init_ex_path incompatible with CRYPTO_DEV_CCP_DD=y or CONFIG_KVM_AMD=y. Google's environment simply doesn't jump through those hoops.

But Google _does_ build kvm-amd.ko as a module.

So rather than carry a bunch of hard-to-follow code (and potentially impossible constraints), always do initialization at kvm-amd.ko load, and require the platform owner to ensure init_ex_path can be resolved when sev_hardware_setup() runs, i.e. when kvm-amd.ko is loaded or its initcall runs.

From: Ashish Kalra <[email protected]>

SNP initialization is forced during PSP driver probe purely because SNP can't be initialized if VMs are running. But the only in-tree user of SEV/SNP functionality is KVM, and KVM depends on PSP driver for the same. Forcing SEV/SNP initialization because a hypervisor could be running legacy non-confidential VMs make no sense.

This patch removes SEV/SNP initialization from the PSP driver probe time and moves the requirement to initialize SEV/SNP functionality to KVM if it wants to use SEV/SNP.

Suggested-by Sean Christopherson <[email protected]>

Reviewed-by Alexey Kardashevskiy <[email protected]>

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

---
 drivers/crypto/ccp/sev-dev.c | 16 ----------------
 1 file changed, 16 deletions(-)

drivers/crypto/ccp/sev-dev.c +0 -6

index cde6ebab589d..42988d757665 100644
@@ -1345,10 +1345,6 @@ static int _sev_platform_init_locked(struct sev_platform_init_args *args)
 	if (sev->state == SEV_STATE_INIT)
 		return 0;
 
-	/*
-	 * Legacy guests cannot be running while SNP_INIT(_EX) is executing,
-	 * so perform SEV-SNP initialization at probe time.
-	 */
 	rc = __sev_snp_init_locked(&args->error);
 	if (rc && rc != -ENODEV) {
 		/*

@@ -2516,9 +2512,7 @@ EXPORT_SYMBOL_GPL(sev_issue_cmd_external_user);
 void sev_pci_init(void)
 {
 	struct sev_device *sev = psp_master->sev_data;
-	struct sev_platform_init_args args = {0};
 	u8 api_major, api_minor, build;
-	int rc;
 
 	if (!sev)
 		return;

@@ -2541,16 +2535,6 @@ void sev_pci_init(void)
 			 api_major, api_minor, build,
 			 sev->api_major, sev->api_minor, sev->build);

drivers/crypto/ccp/sev-dev.c +0 -1

-
 	return;
 
 err: