Move initializing SEV/SNP functionality to KVM

From: Ashish Kalra <[email protected]>

Remove dev_info and dev_err messages related to SEV/SNP initialization

Quoted 4 lines · Ashish Kalra

From: Ashish Kalra <[email protected]>

Remove dev_info and dev_err messages related to SEV/SNP initialization

I don't see any "remove" code in this patch.

From: Ashish Kalra <[email protected]>

Remove dev_info and dev_err messages related to SEV/SNP initialization from callers and instead move those inside __sev_platform_init_locked() and __sev_snp_init_locked().

This allows both _sev_platform_init_locked() and various SEV/SNP ioctls to call __sev_platform_init_locked() and __sev_snp_init_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 | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

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

index af018afd9cd7..1c1c33d3ed9a 100644
@@ -1177,19 +1177,27 @@ static int __sev_snp_init_locked(int *error)
 
 	rc = __sev_do_cmd_locked(cmd, arg, error);
 	if (rc)
-		return rc;
+		goto err;
 
 	/* 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)
-		return rc;
+		goto err;
 
 	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 0;
+
+err:
+	dev_err(sev->dev, "SEV-SNP: failed to INIT rc %d, error %#x\n",
+		rc, *error);
 	return rc;
 }
 

@@ -1268,7 +1276,7 @@ static int __sev_platform_init_locked(int *error)
 
 	rc = __sev_platform_init_handle_init_ex_path(sev);
 	if (rc)
-		return rc;
+		goto err;
 
 	rc = __sev_do_init_locked(&psp_ret);
 	if (rc && psp_ret == SEV_RET_SECURE_DATA_INVALID) {

@@ -1288,7 +1296,7 @@ static int __sev_platform_init_locked(int *error)
 		*error = psp_ret;
 
 	if (rc)
-		return rc;
+		goto err;
 
 	sev->state = SEV_STATE_INIT;
 

@@ -1296,7 +1304,7 @@ static int __sev_platform_init_locked(int *error)
 	wbinvd_on_all_cpus();
 	rc = __sev_do_cmd_locked(SEV_CMD_DF_FLUSH, NULL, error);
 	if (rc)
-		return rc;
+		goto err;
 
 	dev_dbg(sev->dev, "SEV firmware initialized\n");
 

@@ -1304,6 +1312,11 @@ static int __sev_platform_init_locked(int *error)
 		 sev->api_minor, sev->build);
 
 	return 0;
+
+err:
+	dev_err(sev->dev, "SEV: failed to INIT error %#x, rc %d\n",
+		psp_ret, rc);
+	return rc;
 }
 
 static int _sev_platform_init_locked(struct sev_platform_init_args *args)

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 adds 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.

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

---
 drivers/crypto/ccp/sev-dev.c | 149 +++++++++++++++++++++++++++++------
 1 file changed, 125 insertions(+), 24 deletions(-)

drivers/crypto/ccp/sev-dev.c +125 -25

index 1c1c33d3ed9a..0ec2e8191583 100644
@@ -1454,7 +1454,8 @@ 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, ret, error;
 
        if (!writable)
                return -EPERM;

@@ -1463,19 +1464,30 @@ static int sev_ioctl_do_pek_pdh_gen(int cmd, struct sev_issue_cmd *argp, bool wr
                rc = __sev_platform_init_locked(&argp->error);
                if (rc)
                        return rc;
+               shutdown_required = true;
+       }
+
+       rc = __sev_do_cmd_locked(cmd, NULL, &argp->error);
+
+       if (shutdown_required) {
+               ret = __sev_platform_shutdown_locked(&error);
+               if (ret)
+                       dev_err(sev->dev, "SEV: failed to SHUTDOWN error %#x, rc %d\n",
+                               error, ret);
        }
 
-       return __sev_do_cmd_locked(cmd, NULL, &argp->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;
+       int ret, rc, error;
        void *blob = NULL;
-       int ret;
 
        if (!writable)
                return -EPERM;

@@ -1506,6 +1518,7 @@ static int sev_ioctl_do_pek_csr(struct sev_issue_cmd *argp, bool writable)
                ret = __sev_platform_init_locked(&argp->error);
                if (ret)
                        goto e_free_blob;
+               shutdown_required = true;
        }
 
        ret = __sev_do_cmd_locked(SEV_CMD_PEK_CSR, &data, &argp->error);

@@ -1524,6 +1537,13 @@ static int sev_ioctl_do_pek_csr(struct sev_issue_cmd *argp, bool writable)
        }
 
 e_free_blob:
+       if (shutdown_required) {
+               rc = __sev_platform_shutdown_locked(&error);
+               if (rc)
+                       dev_err(sev->dev, "SEV: failed to SHUTDOWN error %#x, rc %d\n",
+                               error, rc);
+       }
+
        kfree(blob);
        return ret;
 }

@@ -1739,8 +1759,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, rc, error;
 
        if (!writable)
                return -EPERM;

@@ -1772,11 +1793,19 @@ static int sev_ioctl_do_pek_import(struct sev_issue_cmd *argp, bool writable)
                ret = __sev_platform_init_locked(&argp->error);
                if (ret)
                        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) {
+               rc = __sev_platform_shutdown_locked(&error);
+               if (rc)
+                       dev_err(sev->dev, "SEV: failed to SHUTDOWN error %#x, rc %d\n",
+                               error, rc);
+       }
+
        kfree(oca_blob);
 e_free_pek:
        kfree(pek_blob);

@@ -1893,17 +1922,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, rc, error;
 
        if (copy_from_user(&input, (void __user *)argp->data, sizeof(input)))
                return -EFAULT;

@@ -1944,6 +1964,16 @@ 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)
+                       return -EPERM;
+               ret = __sev_platform_init_locked(&argp->error);
+               if (ret)
+                       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. */

@@ -1970,6 +2000,13 @@ static int sev_ioctl_do_pdh_export(struct sev_issue_cmd *argp, bool writable)
        }
 
 e_free_cert:
+       if (shutdown_required) {
+               rc = __sev_platform_shutdown_locked(&error);
+               if (rc)
+                       dev_err(sev->dev, "SEV: failed to SHUTDOWN error %#x, rc %d\n",
+                               error, rc);
+       }
+
        kfree(cert_blob);
 e_free_pdh:
        kfree(pdh_blob);

@@ -1979,12 +2016,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, rc, error;
        void *data;
-       int ret;
 
-       if (!sev->snp_initialized || !argp->data)
+       if (!argp->data)
                return -EINVAL;
 
        status_page = alloc_page(GFP_KERNEL_ACCOUNT);

@@ -1993,6 +2031,13 @@ 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(&argp->error);
+               if (ret)
+                       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).

@@ -2021,6 +2066,13 @@ static int sev_ioctl_do_snp_platform_status(struct sev_issue_cmd *argp)
                ret = -EFAULT;
 
 cleanup:
+       if (shutdown_required) {
+               rc = __sev_snp_shutdown_locked(&error, false);
+               if (rc)
+                       dev_err(sev->dev, "SEV-SNP: failed to SHUTDOWN error %#x, rc %d\n",
+                               error, rc);
+       }
+
        __free_pages(status_page, 0);
        return ret;
 }

@@ -2029,21 +2081,38 @@ 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, rc, error;
 
-       if (!sev->snp_initialized)
-               return -EINVAL;
+       if (!sev->snp_initialized) {
+               ret = __sev_snp_init_locked(&argp->error);
+               if (ret)
+                       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) {
+               rc = __sev_snp_shutdown_locked(&error, false);
+               if (rc)
+                       dev_err(sev->dev, "SEV-SNP: failed to SHUTDOWN error %#x, rc %d\n",
+                               error, rc);
+       }
+
+       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, rc, error;
 
-       if (!sev->snp_initialized || !argp->data)
+       if (!argp->data)
                return -EINVAL;
 
        if (!writable)

@@ -2052,17 +2121,34 @@ 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(&argp->error);
+               if (ret)
+                       return ret;
+               shutdown_required = true;
+       }
+
+       ret = __sev_do_cmd_locked(SEV_CMD_SNP_CONFIG, &config, &argp->error);
+
+       if (shutdown_required) {
+               rc = __sev_snp_shutdown_locked(&error, false);
+               if (rc)
+                       dev_err(sev->dev, "SEV-SNP: failed to SHUTDOWN error %#x, rc %d\n",
+                               error, rc);
+       }
+
+       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, rc, error;
        void *blob;
-       int ret;
 
-       if (!sev->snp_initialized || !argp->data)
+       if (!argp->data)
                return -EINVAL;
 
        if (!writable)

@@ -2081,8 +2167,23 @@ 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(&argp->error);
+               if (ret)
+                       goto cleanup;
+               shutdown_required = true;
+       }
+
        ret = __sev_do_cmd_locked(SEV_CMD_SNP_VLEK_LOAD, &input, &argp->error);
 
+       if (shutdown_required) {
+               rc = __sev_snp_shutdown_locked(&error, false);
+               if (rc)
+                       dev_err(sev->dev, "SEV-SNP: failed to SHUTDOWN error %#x, rc %d\n",
+                               error, rc);
+       }
+
+cleanup:
        kfree(blob);
 
        return ret;
--

2.34.1

Quoted 3 lines · Ashish Kalra

From: Ashish Kalra <[email protected]>

The subject includes "Fix" but has no "Fixes" tag in the commit message.

Quoted 7 lines

Modify the behavior of implicit SEV initialization in some of the SEV ioctls to do both SEV initialization and shutdown and adds 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.

It would be helpful to update the description with the state machine you're trying to maintain implicitly.
I think that this changes the uapi contract as well, so I think you need to update the documentation.

You have SEV shutdown on error for platform maintenance ioctls here, which already have implicit init.
pdh_export gets an init if not in the init state, which wasn't already implicit because there's a wrinkle WRT the writability permission.

snp_platform_status, snp_config, vlek_load, snp_commit now should be callable any time, not just when KVM has initialized SNP? If there's a caveat to the platform status, the docs need to reflect it. I don't know how SNP_COMMIT makes sense as having an implicit init/shutdown unless you're using it as SET_CONFIG, but I suppose it doesn't hurt?

Quoted 306 lines

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

---
 drivers/crypto/ccp/sev-dev.c | 149 +++++++++++++++++++++++++++++------
 1 file changed, 125 insertions(+), 24 deletions(-)

drivers/crypto/ccp/sev-dev.c +125 -25

index 1c1c33d3ed9a..0ec2e8191583 100644
@@ -1454,7 +1454,8 @@ 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, ret, error;
 
        if (!writable)
                return -EPERM;

@@ -1463,19 +1464,30 @@ static int sev_ioctl_do_pek_pdh_gen(int cmd, struct sev_issue_cmd *argp, bool wr
                rc = __sev_platform_init_locked(&argp->error);
                if (rc)
                        return rc;
+               shutdown_required = true;
+       }
+
+       rc = __sev_do_cmd_locked(cmd, NULL, &argp->error);
+
+       if (shutdown_required) {
+               ret = __sev_platform_shutdown_locked(&error);
+               if (ret)
+                       dev_err(sev->dev, "SEV: failed to SHUTDOWN error %#x, rc %d\n",
+                               error, ret);
        }
 
-       return __sev_do_cmd_locked(cmd, NULL, &argp->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;
+       int ret, rc, error;
        void *blob = NULL;
-       int ret;
 
        if (!writable)
                return -EPERM;

@@ -1506,6 +1518,7 @@ static int sev_ioctl_do_pek_csr(struct sev_issue_cmd *argp, bool writable)
                ret = __sev_platform_init_locked(&argp->error);
                if (ret)
                        goto e_free_blob;
+               shutdown_required = true;
        }
 
        ret = __sev_do_cmd_locked(SEV_CMD_PEK_CSR, &data, &argp->error);

@@ -1524,6 +1537,13 @@ static int sev_ioctl_do_pek_csr(struct sev_issue_cmd *argp, bool writable)
        }
 
 e_free_blob:
+       if (shutdown_required) {
+               rc = __sev_platform_shutdown_locked(&error);
+               if (rc)
+                       dev_err(sev->dev, "SEV: failed to SHUTDOWN error %#x, rc %d\n",
+                               error, rc);
+       }
+
        kfree(blob);
        return ret;
 }

@@ -1739,8 +1759,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, rc, error;
 
        if (!writable)
                return -EPERM;

@@ -1772,11 +1793,19 @@ static int sev_ioctl_do_pek_import(struct sev_issue_cmd *argp, bool writable)
                ret = __sev_platform_init_locked(&argp->error);
                if (ret)
                        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) {
+               rc = __sev_platform_shutdown_locked(&error);
+               if (rc)
+                       dev_err(sev->dev, "SEV: failed to SHUTDOWN error %#x, rc %d\n",
+                               error, rc);
+       }
+
        kfree(oca_blob);
 e_free_pek:
        kfree(pek_blob);

@@ -1893,17 +1922,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, rc, error;
 
        if (copy_from_user(&input, (void __user *)argp->data, sizeof(input)))
                return -EFAULT;

@@ -1944,6 +1964,16 @@ 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)
+                       return -EPERM;
+               ret = __sev_platform_init_locked(&argp->error);
+               if (ret)
+                       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. */

@@ -1970,6 +2000,13 @@ static int sev_ioctl_do_pdh_export(struct sev_issue_cmd *argp, bool writable)
        }
 
 e_free_cert:
+       if (shutdown_required) {
+               rc = __sev_platform_shutdown_locked(&error);
+               if (rc)
+                       dev_err(sev->dev, "SEV: failed to SHUTDOWN error %#x, rc %d\n",
+                               error, rc);
+       }
+
        kfree(cert_blob);
 e_free_pdh:
        kfree(pdh_blob);

@@ -1979,12 +2016,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, rc, error;
        void *data;
-       int ret;
 
-       if (!sev->snp_initialized || !argp->data)
+       if (!argp->data)
                return -EINVAL;
 
        status_page = alloc_page(GFP_KERNEL_ACCOUNT);

@@ -1993,6 +2031,13 @@ 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(&argp->error);
+               if (ret)
+                       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).

@@ -2021,6 +2066,13 @@ static int sev_ioctl_do_snp_platform_status(struct sev_issue_cmd *argp)
                ret = -EFAULT;
 
 cleanup:
+       if (shutdown_required) {
+               rc = __sev_snp_shutdown_locked(&error, false);
+               if (rc)
+                       dev_err(sev->dev, "SEV-SNP: failed to SHUTDOWN error %#x, rc %d\n",
+                               error, rc);
+       }
+
        __free_pages(status_page, 0);
        return ret;
 }

@@ -2029,21 +2081,38 @@ 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, rc, error;
 
-       if (!sev->snp_initialized)
-               return -EINVAL;
+       if (!sev->snp_initialized) {
+               ret = __sev_snp_init_locked(&argp->error);
+               if (ret)
+                       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) {
+               rc = __sev_snp_shutdown_locked(&error, false);
+               if (rc)
+                       dev_err(sev->dev, "SEV-SNP: failed to SHUTDOWN error %#x, rc %d\n",
+                               error, rc);
+       }
+
+       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, rc, error;
 
-       if (!sev->snp_initialized || !argp->data)
+       if (!argp->data)
                return -EINVAL;
 
        if (!writable)

@@ -2052,17 +2121,34 @@ 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(&argp->error);
+               if (ret)
+                       return ret;
+               shutdown_required = true;
+       }
+
+       ret = __sev_do_cmd_locked(SEV_CMD_SNP_CONFIG, &config, &argp->error);
+
+       if (shutdown_required) {
+               rc = __sev_snp_shutdown_locked(&error, false);
+               if (rc)
+                       dev_err(sev->dev, "SEV-SNP: failed to SHUTDOWN error %#x, rc %d\n",
+                               error, rc);
+       }
+
+       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, rc, error;
        void *blob;
-       int ret;
 
-       if (!sev->snp_initialized || !argp->data)
+       if (!argp->data)
                return -EINVAL;
 
        if (!writable)

@@ -2081,8 +2167,23 @@ 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(&argp->error);
+               if (ret)
+                       goto cleanup;
+               shutdown_required = true;
+       }
+
        ret = __sev_do_cmd_locked(SEV_CMD_SNP_VLEK_LOAD, &input, &argp->error);
 
+       if (shutdown_required) {
+               rc = __sev_snp_shutdown_locked(&error, false);
+               if (rc)
+                       dev_err(sev->dev, "SEV-SNP: failed to SHUTDOWN error %#x, rc %d\n",
+                               error, rc);
+       }
+
+cleanup:
        kfree(blob);
 
        return ret;
--

Quoted 2 lines

2.34.1

--
-Dionna Glaze, PhD, CISSP, CCSP (she/her)

Quoted 7 lines · Dionna Amalie Glaze

Quoted 3 lines · Ashish Kalra

From: Ashish Kalra <[email protected]>

The subject includes "Fix" but has no "Fixes" tag in the commit message.

I will change the commit message to be more appropriate, it is not really a fix but a change to either add a SEV shutdown to some SEV ioctls and add SNP init and shutdown to some SNP ioctls.

Quoted 8 lines

Quoted 7 lines

Modify the behavior of implicit SEV initialization in some of the SEV ioctls to do both SEV initialization and shutdown and adds 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.

Quoted 5 lines

It would be helpful to update the description with the state machine you're trying to maintain implicitly.
I think that this changes the uapi contract as well, so I think you need to update the documentation.

How does this change the uapi contract, as the SEV init and shutdown is going to happen as a sequence and the platform state is going to be consistent before and after the ioctl, the next ioctl if required will reissue SEV init.

Quoted 5 lines

You have SEV shutdown on error for platform maintenance ioctls here, which already have implicit init.
pdh_export gets an init if not in the init state, which wasn't already implicit because there's a wrinkle WRT the writability permission.

This patch only adds SEV shutdown to already implied init code as part of some of these SEV ioctls.

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

Quoted 7 lines

snp_platform_status, snp_config, vlek_load, snp_commit now should be callable any time, not just when KVM has initialized SNP? If there's a caveat to the platform status, the docs need to reflect it. I don't know how SNP_COMMIT makes sense as having an implicit init/shutdown unless you're using it as SET_CONFIG, but I suppose it doesn't hurt?

Yes, and that is what this code is allowing, to call snp_platform_status, snp_config, vlek_load and snp_commit without KVM having initialized SNP.

If you see the behavior prior to this patch, SNP has always been initialized before these ioctls as SNP initialization is done as part of PSP module load, therefore, to keep a consistent behavior, SNP init is being done here implicitly as part of these ioctls and then SNP shutdown before returning from the ioctl to maintain the consistent platform state before and after the ioctl.

Additionally looking at the SNP firmware API specs, SNP_CONFIG needs SNP to be in INIT state.

Thanks,
Ashish

Quoted 312 lines

Quoted 306 lines

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

---
 drivers/crypto/ccp/sev-dev.c | 149 +++++++++++++++++++++++++++++------
 1 file changed, 125 insertions(+), 24 deletions(-)

drivers/crypto/ccp/sev-dev.c +125 -25

index 1c1c33d3ed9a..0ec2e8191583 100644
@@ -1454,7 +1454,8 @@ 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, ret, error;
 
        if (!writable)
                return -EPERM;

@@ -1463,19 +1464,30 @@ static int sev_ioctl_do_pek_pdh_gen(int cmd, struct sev_issue_cmd *argp, bool wr
                rc = __sev_platform_init_locked(&argp->error);
                if (rc)
                        return rc;
+               shutdown_required = true;
+       }
+
+       rc = __sev_do_cmd_locked(cmd, NULL, &argp->error);
+
+       if (shutdown_required) {
+               ret = __sev_platform_shutdown_locked(&error);
+               if (ret)
+                       dev_err(sev->dev, "SEV: failed to SHUTDOWN error %#x, rc %d\n",
+                               error, ret);
        }
 
-       return __sev_do_cmd_locked(cmd, NULL, &argp->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;
+       int ret, rc, error;
        void *blob = NULL;
-       int ret;
 
        if (!writable)
                return -EPERM;

@@ -1506,6 +1518,7 @@ static int sev_ioctl_do_pek_csr(struct sev_issue_cmd *argp, bool writable)
                ret = __sev_platform_init_locked(&argp->error);
                if (ret)
                        goto e_free_blob;
+               shutdown_required = true;
        }
 
        ret = __sev_do_cmd_locked(SEV_CMD_PEK_CSR, &data, &argp->error);

@@ -1524,6 +1537,13 @@ static int sev_ioctl_do_pek_csr(struct sev_issue_cmd *argp, bool writable)
        }
 
 e_free_blob:
+       if (shutdown_required) {
+               rc = __sev_platform_shutdown_locked(&error);
+               if (rc)
+                       dev_err(sev->dev, "SEV: failed to SHUTDOWN error %#x, rc %d\n",
+                               error, rc);
+       }
+
        kfree(blob);
        return ret;
 }

@@ -1739,8 +1759,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, rc, error;
 
        if (!writable)
                return -EPERM;

@@ -1772,11 +1793,19 @@ static int sev_ioctl_do_pek_import(struct sev_issue_cmd *argp, bool writable)
                ret = __sev_platform_init_locked(&argp->error);
                if (ret)
                        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) {
+               rc = __sev_platform_shutdown_locked(&error);
+               if (rc)
+                       dev_err(sev->dev, "SEV: failed to SHUTDOWN error %#x, rc %d\n",
+                               error, rc);
+       }
+
        kfree(oca_blob);
 e_free_pek:
        kfree(pek_blob);

@@ -1893,17 +1922,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, rc, error;
 
        if (copy_from_user(&input, (void __user *)argp->data, sizeof(input)))
                return -EFAULT;

@@ -1944,6 +1964,16 @@ 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)
+                       return -EPERM;
+               ret = __sev_platform_init_locked(&argp->error);
+               if (ret)
+                       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. */

@@ -1970,6 +2000,13 @@ static int sev_ioctl_do_pdh_export(struct sev_issue_cmd *argp, bool writable)
        }
 
 e_free_cert:
+       if (shutdown_required) {
+               rc = __sev_platform_shutdown_locked(&error);
+               if (rc)
+                       dev_err(sev->dev, "SEV: failed to SHUTDOWN error %#x, rc %d\n",
+                               error, rc);
+       }
+
        kfree(cert_blob);
 e_free_pdh:
        kfree(pdh_blob);

@@ -1979,12 +2016,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, rc, error;
        void *data;
-       int ret;
 
-       if (!sev->snp_initialized || !argp->data)
+       if (!argp->data)
                return -EINVAL;
 
        status_page = alloc_page(GFP_KERNEL_ACCOUNT);

@@ -1993,6 +2031,13 @@ 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(&argp->error);
+               if (ret)
+                       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).

@@ -2021,6 +2066,13 @@ static int sev_ioctl_do_snp_platform_status(struct sev_issue_cmd *argp)
                ret = -EFAULT;
 
 cleanup:
+       if (shutdown_required) {
+               rc = __sev_snp_shutdown_locked(&error, false);
+               if (rc)
+                       dev_err(sev->dev, "SEV-SNP: failed to SHUTDOWN error %#x, rc %d\n",
+                               error, rc);
+       }
+
        __free_pages(status_page, 0);
        return ret;
 }

@@ -2029,21 +2081,38 @@ 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, rc, error;
 
-       if (!sev->snp_initialized)
-               return -EINVAL;
+       if (!sev->snp_initialized) {
+               ret = __sev_snp_init_locked(&argp->error);
+               if (ret)
+                       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) {
+               rc = __sev_snp_shutdown_locked(&error, false);
+               if (rc)
+                       dev_err(sev->dev, "SEV-SNP: failed to SHUTDOWN error %#x, rc %d\n",
+                               error, rc);
+       }
+
+       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, rc, error;
 
-       if (!sev->snp_initialized || !argp->data)
+       if (!argp->data)
                return -EINVAL;
 
        if (!writable)

@@ -2052,17 +2121,34 @@ 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(&argp->error);
+               if (ret)
+                       return ret;
+               shutdown_required = true;
+       }
+
+       ret = __sev_do_cmd_locked(SEV_CMD_SNP_CONFIG, &config, &argp->error);
+
+       if (shutdown_required) {
+               rc = __sev_snp_shutdown_locked(&error, false);
+               if (rc)
+                       dev_err(sev->dev, "SEV-SNP: failed to SHUTDOWN error %#x, rc %d\n",
+                               error, rc);
+       }
+
+       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, rc, error;
        void *blob;
-       int ret;
 
-       if (!sev->snp_initialized || !argp->data)
+       if (!argp->data)
                return -EINVAL;
 
        if (!writable)

@@ -2081,8 +2167,23 @@ 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(&argp->error);
+               if (ret)
+                       goto cleanup;
+               shutdown_required = true;
+       }
+
        ret = __sev_do_cmd_locked(SEV_CMD_SNP_VLEK_LOAD, &input, &argp->error);
 
+       if (shutdown_required) {
+               rc = __sev_snp_shutdown_locked(&error, false);
+               if (rc)
+                       dev_err(sev->dev, "SEV-SNP: failed to SHUTDOWN error %#x, rc %d\n",
+                               error, rc);
+       }
+
+cleanup:
        kfree(blob);
 
        return ret;
--

Quoted 2 lines

2.34.1

--
-Dionna Glaze, PhD, CISSP, CCSP (she/her)

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 adds 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.

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

---
  drivers/crypto/ccp/sev-dev.c | 149 +++++++++++++++++++++++++++++------
  1 file changed, 125 insertions(+), 24 deletions(-)

drivers/crypto/ccp/sev-dev.c +35 -15

index 1c1c33d3ed9a..0ec2e8191583 100644
@@ -1454,7 +1454,8 @@ 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, ret, error;
 
  	if (!writable)
  		return -EPERM;

@@ -1463,19 +1464,30 @@ static int sev_ioctl_do_pek_pdh_gen(int cmd, struct sev_issue_cmd *argp, bool wr
  		rc = __sev_platform_init_locked(&argp->error);
  		if (rc)
  			return rc;
+		shutdown_required = true;
+	}
+
+	rc = __sev_do_cmd_locked(cmd, NULL, &argp->error);
+
+	if (shutdown_required) {
+		ret = __sev_platform_shutdown_locked(&error);
+		if (ret)
+			dev_err(sev->dev, "SEV: failed to SHUTDOWN error %#x, rc %d\n",
+				error, ret);
  	}
 
-	return __sev_do_cmd_locked(cmd, NULL, &argp->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;
+	int ret, rc, error;
  	void *blob = NULL;
-	int ret;
 
  	if (!writable)
  		return -EPERM;

@@ -1506,6 +1518,7 @@ static int sev_ioctl_do_pek_csr(struct sev_issue_cmd *argp, bool writable)
  		ret = __sev_platform_init_locked(&argp->error);
  		if (ret)
  			goto e_free_blob;
+		shutdown_required = true;
  	}
 
  	ret = __sev_do_cmd_locked(SEV_CMD_PEK_CSR, &data, &argp->error);

@@ -1524,6 +1537,13 @@ static int sev_ioctl_do_pek_csr(struct sev_issue_cmd *argp, bool writable)
  	}
 
  e_free_blob:
+	if (shutdown_required) {
+		rc = __sev_platform_shutdown_locked(&error);
+		if (rc)
+			dev_err(sev->dev, "SEV: failed to SHUTDOWN error %#x, rc %d\n",
+				error, rc);
+	}
+
  	kfree(blob);
  	return ret;
  }

@@ -1739,8 +1759,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, rc, error;
 
  	if (!writable)
  		return -EPERM;

@@ -1772,11 +1793,19 @@ static int sev_ioctl_do_pek_import(struct sev_issue_cmd *argp, bool writable)
  		ret = __sev_platform_init_locked(&argp->error);
  		if (ret)
  			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) {
+		rc = __sev_platform_shutdown_locked(&error);
+		if (rc)
+			dev_err(sev->dev, "SEV: failed to SHUTDOWN error %#x, rc %d\n",
+				error, rc);
+	}
+
  	kfree(oca_blob);
  e_free_pek:
  	kfree(pek_blob);

@@ -1893,17 +1922,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, rc, error;
 
  	if (copy_from_user(&input, (void __user *)argp->data, sizeof(input)))
  		return -EFAULT;

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

+		ret = __sev_platform_init_locked(&argp->error);
+		if (ret)
+			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. */

@@ -1970,6 +2000,13 @@ static int sev_ioctl_do_pdh_export(struct sev_issue_cmd *argp, bool writable)
  	}
 
  e_free_cert:
+	if (shutdown_required) {
+		rc = __sev_platform_shutdown_locked(&error);
+		if (rc)
+			dev_err(sev->dev, "SEV: failed to SHUTDOWN error %#x, rc %d\n",
+				error, rc);
+	}
+
  	kfree(cert_blob);
  e_free_pdh:
  	kfree(pdh_blob);

@@ -1979,12 +2016,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, rc, error;
  	void *data;
-	int ret;
 
-	if (!sev->snp_initialized || !argp->data)
+	if (!argp->data)
  		return -EINVAL;
 
  	status_page = alloc_page(GFP_KERNEL_ACCOUNT);

@@ -1993,6 +2031,13 @@ 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(&argp->error);
+		if (ret)
+			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).

@@ -2021,6 +2066,13 @@ static int sev_ioctl_do_snp_platform_status(struct sev_issue_cmd *argp)
  		ret = -EFAULT;
 
  cleanup:
+	if (shutdown_required) {
+		rc = __sev_snp_shutdown_locked(&error, false);
+		if (rc)
+			dev_err(sev->dev, "SEV-SNP: failed to SHUTDOWN error %#x, rc %d\n",
+				error, rc);
+	}
+
  	__free_pages(status_page, 0);
  	return ret;
  }

@@ -2029,21 +2081,38 @@ 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, rc, error;
 
-	if (!sev->snp_initialized)
-		return -EINVAL;
+	if (!sev->snp_initialized) {
+		ret = __sev_snp_init_locked(&argp->error);
+		if (ret)
+			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) {
+		rc = __sev_snp_shutdown_locked(&error, false);
+		if (rc)
+			dev_err(sev->dev, "SEV-SNP: failed to SHUTDOWN error %#x, rc %d\n",
+				error, rc);
+	}
+
+	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, rc, error;
 
-	if (!sev->snp_initialized || !argp->data)
+	if (!argp->data)
  		return -EINVAL;
 
  	if (!writable)

@@ -2052,17 +2121,34 @@ 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(&argp->error);
+		if (ret)
+			return ret;
+		shutdown_required = true;
+	}
+
+	ret = __sev_do_cmd_locked(SEV_CMD_SNP_CONFIG, &config, &argp->error);
+
+	if (shutdown_required) {
+		rc = __sev_snp_shutdown_locked(&error, false);
+		if (rc)
+			dev_err(sev->dev, "SEV-SNP: failed to SHUTDOWN error %#x, rc %d\n",
+				error, rc);
+	}
+
+	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, rc, error;
  	void *blob;
-	int ret;
 
-	if (!sev->snp_initialized || !argp->data)
+	if (!argp->data)
  		return -EINVAL;
 
  	if (!writable)

@@ -2081,8 +2167,23 @@ 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(&argp->error);
+		if (ret)
+			goto cleanup;
+		shutdown_required = true;
+	}

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

+cleanup:
  	kfree(blob);
 
  	return ret;

Quoted 126 lines · Ashish Kalra

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 adds 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.

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

---
  drivers/crypto/ccp/sev-dev.c | 149 +++++++++++++++++++++++++++++------

  1 file changed, 125 insertions(+), 24 deletions(-)

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

index 1c1c33d3ed9a..0ec2e8191583 100644
@@ -1454,7 +1454,8 @@ 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, ret, error;
         if (!writable)
           return -EPERM;

@@ -1463,19 +1464,30 @@ static int sev_ioctl_do_pek_pdh_gen(int cmd, struct sev_issue_cmd *argp, bool wr
           rc = __sev_platform_init_locked(&argp->error);
           if (rc)
               return rc;
+        shutdown_required = true;
+    }
+
+    rc = __sev_do_cmd_locked(cmd, NULL, &argp->error);
+
+    if (shutdown_required) {
+        ret = __sev_platform_shutdown_locked(&error);
+        if (ret)
+            dev_err(sev->dev, "SEV: failed to SHUTDOWN error %#x, rc %d\n",
+                error, ret);
       }
   -    return __sev_do_cmd_locked(cmd, NULL, &argp->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;
+    int ret, rc, error;
       void *blob = NULL;
-    int ret;
         if (!writable)
           return -EPERM;

@@ -1506,6 +1518,7 @@ static int sev_ioctl_do_pek_csr(struct sev_issue_cmd *argp, bool writable)
           ret = __sev_platform_init_locked(&argp->error);
           if (ret)
               goto e_free_blob;
+        shutdown_required = true;
       }
         ret = __sev_do_cmd_locked(SEV_CMD_PEK_CSR, &data, &argp->error);

@@ -1524,6 +1537,13 @@ static int sev_ioctl_do_pek_csr(struct sev_issue_cmd *argp, bool writable)
       }

    e_free_blob:
+    if (shutdown_required) {
+        rc = __sev_platform_shutdown_locked(&error);
+        if (rc)
+            dev_err(sev->dev, "SEV: failed to SHUTDOWN error %#x, rc %d\n", +                error, rc);
+    }
+
      kfree(blob);
      return ret;
  }

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

@@ -1739,8 +1759,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, rc, error;
         if (!writable)
           return -EPERM;

@@ -1772,11 +1793,19 @@ static int sev_ioctl_do_pek_import(struct sev_issue_cmd *argp, bool writable)
           ret = __sev_platform_init_locked(&argp->error);
           if (ret)
               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) {
+        rc = __sev_platform_shutdown_locked(&error);
+        if (rc)
+            dev_err(sev->dev, "SEV: failed to SHUTDOWN error %#x, rc %d\n", +                error, rc);
+    }
+
      kfree(oca_blob);
  e_free_pek:
      kfree(pek_blob);

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

@@ -1893,17 +1922,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, rc, error;
         if (copy_from_user(&input, (void __user *)argp->data, sizeof(input)))
           return -EFAULT;

@@ -1944,6 +1964,16 @@ static int sev_ioctl_do_pdh_export(struct sev_issue_cmd *argp, bool writable)
       data.cert_chain_len = input.cert_chain_len;

Quoted 5 lines

    cmd:
+    /* If platform is not in INIT state then transition it to INIT. */ +    if (sev->state != SEV_STATE_INIT) {
+        if (!writable)
+            return -EPERM;

same comment as in v2:

goto e_free_cert, not return, otherwise leaks memory.

Quoted 138 lines

+        ret = __sev_platform_init_locked(&argp->error);
+        if (ret)
+            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. */

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

@@ -1970,6 +2000,13 @@ static int sev_ioctl_do_pdh_export(struct sev_issue_cmd *argp, bool writable)
       }

    e_free_cert:
+    if (shutdown_required) {
+        rc = __sev_platform_shutdown_locked(&error);
+        if (rc)
+            dev_err(sev->dev, "SEV: failed to SHUTDOWN error %#x, rc %d\n", +                error, rc);
+    }
+
      kfree(cert_blob);
  e_free_pdh:
      kfree(pdh_blob);

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

@@ -1979,12 +2016,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, rc, error;
       void *data;
-    int ret;
   -    if (!sev->snp_initialized || !argp->data)
+    if (!argp->data)
           return -EINVAL;
         status_page = alloc_page(GFP_KERNEL_ACCOUNT);

@@ -1993,6 +2031,13 @@ 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(&argp->error);
+        if (ret)
+            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).

@@ -2021,6 +2066,13 @@ static int sev_ioctl_do_snp_platform_status(struct sev_issue_cmd *argp)
           ret = -EFAULT;

    cleanup:
+    if (shutdown_required) {
+        rc = __sev_snp_shutdown_locked(&error, false);
+        if (rc)
+            dev_err(sev->dev, "SEV-SNP: failed to SHUTDOWN error %#x, rc %d\n", +                error, rc);
+    }
+
      __free_pages(status_page, 0);
      return ret;
  }

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

@@ -2029,21 +2081,38 @@ 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, rc, error;
   -    if (!sev->snp_initialized)
-        return -EINVAL;
+    if (!sev->snp_initialized) {
+        ret = __sev_snp_init_locked(&argp->error);
+        if (ret)
+            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) {
+        rc = __sev_snp_shutdown_locked(&error, false);
+        if (rc)
+            dev_err(sev->dev, "SEV-SNP: failed to SHUTDOWN error %#x, rc %d\n",
+                error, rc);
+    }
+
+    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, rc, error;
   -    if (!sev->snp_initialized || !argp->data)
+    if (!argp->data)
           return -EINVAL;
         if (!writable)

@@ -2052,17 +2121,34 @@ 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(&argp->error);
+        if (ret)
+            return ret;
+        shutdown_required = true;
+    }
+
+    ret = __sev_do_cmd_locked(SEV_CMD_SNP_CONFIG, &config, &argp->error);
+
+    if (shutdown_required) {
+        rc = __sev_snp_shutdown_locked(&error, false);
+        if (rc)
+            dev_err(sev->dev, "SEV-SNP: failed to SHUTDOWN error %#x, rc %d\n",
+                error, rc);
+    }
+
+    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, rc, error;
       void *blob;
-    int ret;
   -    if (!sev->snp_initialized || !argp->data)
+    if (!argp->data)
           return -EINVAL;
         if (!writable)

@@ -2081,8 +2167,23 @@ 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(&argp->error);
+        if (ret)
+            goto cleanup;
+        shutdown_required = true;

drivers/crypto/ccp/sev-dev.c static int sev_ioctl_do_snp_vlek_load(struct sev_issue_cmd *argp, bool writable) · line 2182

Quoted 10 lines

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

+    }
+
       ret = __sev_do_cmd_locked(SEV_CMD_SNP_VLEK_LOAD, &input, &argp->error);
   +    if (shutdown_required) {
+        rc = __sev_snp_shutdown_locked(&error, false);
+        if (rc)
+            dev_err(sev->dev, "SEV-SNP: failed to SHUTDOWN error %#x, rc %d\n",
+                error, rc);
+    }
+

same comment as in v2:

It is the same template 8 (?) times, I'd declare rc and error inside the "if (shutdown_required)" scope or even drop them and error messages as __sev_snp_shutdown_locked() prints dev_err() anyway.

if (shutdown_required)
    __sev_snp_shutdown_locked(&error, false);

and that's it. Thanks,

Quoted 4 lines

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

+cleanup:
       kfree(blob);
         return ret;

From: Ashish Kalra <[email protected]>

Register the SNP panic notifier if and only if SNP is actually initialized and deregistering the notifier when shutting down SNP in PSP driver when KVM module is unloaded.

Currently the SNP panic notifier is being registered irrespective of SNP being enabled/initialized and with this change the SNP panic notifier is registered only if SNP support is enabled and initialized.

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

From: Ashish Kalra <[email protected]>

Add new API interface to do SEV/SNP platform shutdown when KVM module is unloaded.

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

From: Ashish Kalra <[email protected]>

Remove platform initialization of SEV/SNP from PSP driver probe time and

move it to KVM module load time so that KVM can do SEV/SNP platform initialization explicitly if it actually wants to use SEV/SNP functionality.

With this patch, KVM will explicitly call into the PSP driver at load time to initialize SEV/SNP by default but this behavior can be altered with KVM module parameters to not do SEV/SNP platform initialization at module load time if required. Additionally SEV/SNP platform shutdown is invoked during KVM module unload time.

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

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

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

index 943bd074a5d3..0dc8294582c6 100644
@@ -444,7 +444,6 @@ static int __sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp,
 	if (ret)
 		goto e_no_asid;
 
-	init_args.probe = false;
 	ret = sev_platform_init(&init_args);
 	if (ret)
 		goto e_free;

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

 	bool sev_snp_supported = false;
 	bool sev_es_supported = false;
 	bool sev_supported = false;

@@ -3069,6 +3069,16 @@ void __init sev_hardware_setup(void)
 	sev_supported_vmsa_features = 0;
 	if (sev_es_debug_swap_enabled)
 		sev_supported_vmsa_features |= SVM_SEV_FEAT_DEBUG_SWAP;
+
+	if (!sev_enabled)
+		return;
+
+	/*
+	 * NOTE: Always do SNP INIT regardless of sev_snp_supported
+	 * as SNP INIT has to be done to launch legacy SEV/SEV-ES
+	 * VMs in case SNP is enabled system-wide.
+	 */
+	sev_platform_init(&init_args);
 }
 
 void sev_hardware_unsetup(void)

@@ -3084,6 +3094,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 */
+	sev_platform_shutdown();
 }
 
 int sev_cpu_init(struct svm_cpu_data *sd)

Quoted 3 lines · Ashish Kalra

From: Ashish Kalra <[email protected]>

Remove platform initialization of SEV/SNP from PSP driver probe time and

Actually, you're not removing it, yet...

Quoted 23 lines

move it to KVM module load time so that KVM can do SEV/SNP platform initialization explicitly if it actually wants to use SEV/SNP functionality.

With this patch, KVM will explicitly call into the PSP driver at load time to initialize SEV/SNP by default but this behavior can be altered with KVM module parameters to not do SEV/SNP platform initialization at module load time if required. Additionally SEV/SNP platform shutdown is invoked during KVM module unload time.

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

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

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

index 943bd074a5d3..0dc8294582c6 100644
@@ -444,7 +444,6 @@ static int __sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp,
 	if (ret)
 		goto e_no_asid;
 

arch/x86/kvm/svm/sev.c void __init sev_set_cpu_caps(void) · line 2955

Quoted 9 lines

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

-	init_args.probe = false;
 	ret = sev_platform_init(&init_args);
 	if (ret)
 		goto e_free;

@@ -2953,6 +2952,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};

Will this cause issues if KVM is built-in and INIT_EX is being used (init_ex_path ccp parameter)? The probe parameter is used for initialization done before the filesystem is available.

Thanks,
Tom

Quoted 30 lines

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

 	bool sev_snp_supported = false;
 	bool sev_es_supported = false;
 	bool sev_supported = false;

@@ -3069,6 +3069,16 @@ void __init sev_hardware_setup(void)
 	sev_supported_vmsa_features = 0;
 	if (sev_es_debug_swap_enabled)
 		sev_supported_vmsa_features |= SVM_SEV_FEAT_DEBUG_SWAP;
+
+	if (!sev_enabled)
+		return;
+
+	/*
+	 * NOTE: Always do SNP INIT regardless of sev_snp_supported
+	 * as SNP INIT has to be done to launch legacy SEV/SEV-ES
+	 * VMs in case SNP is enabled system-wide.
+	 */
+	sev_platform_init(&init_args);
 }
 
 void sev_hardware_unsetup(void)

@@ -3084,6 +3094,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 */
+	sev_platform_shutdown();
 }
 
 int sev_cpu_init(struct svm_cpu_data *sd)

Quoted 30 lines · Tom Lendacky

Quoted 3 lines · Ashish Kalra

From: Ashish Kalra <[email protected]>

Remove platform initialization of SEV/SNP from PSP driver probe time and

Actually, you're not removing it, yet...

Quoted 23 lines

move it to KVM module load time so that KVM can do SEV/SNP platform initialization explicitly if it actually wants to use SEV/SNP functionality.

With this patch, KVM will explicitly call into the PSP driver at load time to initialize SEV/SNP by default but this behavior can be altered with KVM module parameters to not do SEV/SNP platform initialization at module load time if required. Additionally SEV/SNP platform shutdown is invoked during KVM module unload time.

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

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

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

index 943bd074a5d3..0dc8294582c6 100644
@@ -444,7 +444,6 @@ static int __sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp,
 	if (ret)
 		goto e_no_asid;
 

Quoted 14 lines

arch/x86/kvm/svm/sev.c void __init sev_set_cpu_caps(void) · line 2955

Quoted 9 lines

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

-	init_args.probe = false;
 	ret = sev_platform_init(&init_args);
 	if (ret)
 		goto e_free;

@@ -2953,6 +2952,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};

Will this cause issues if KVM is built-in and INIT_EX is being used (init_ex_path ccp parameter)? The probe parameter is used for initialization done before the filesystem is available.

Yes, this will cause issues if KVM is builtin and INIT_EX is being used, but my question is how will INIT_EX be used when we move SEV INIT to KVM ?

If we continue to use the probe field here and also continue to support psp_init_on_probe module parameter for CCP, how will SEV INIT_EX be invoked ?

How is SEV INIT_EX invoked in PSP driver currently if psp_init_on_probe parameter is set to false ?

The KVM path to invoke sev_platform_init() when a SEV VM is being launched cannot be used because QEMU checks for SEV to be initialized before invoking this code path to launch the guest.

Quoted 34 lines

Thanks,
Tom

Quoted 30 lines

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

 	bool sev_snp_supported = false;
 	bool sev_es_supported = false;
 	bool sev_supported = false;

@@ -3069,6 +3069,16 @@ void __init sev_hardware_setup(void)
 	sev_supported_vmsa_features = 0;
 	if (sev_es_debug_swap_enabled)
 		sev_supported_vmsa_features |= SVM_SEV_FEAT_DEBUG_SWAP;
+
+	if (!sev_enabled)
+		return;
+
+	/*
+	 * NOTE: Always do SNP INIT regardless of sev_snp_supported
+	 * as SNP INIT has to be done to launch legacy SEV/SEV-ES
+	 * VMs in case SNP is enabled system-wide.
+	 */
+	sev_platform_init(&init_args);
 }
 
 void sev_hardware_unsetup(void)

@@ -3084,6 +3094,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 */
+	sev_platform_shutdown();
 }
 
 int sev_cpu_init(struct svm_cpu_data *sd)

Quoted 30 lines · Tom Lendacky

Quoted 3 lines · Ashish Kalra

From: Ashish Kalra <[email protected]>

Remove platform initialization of SEV/SNP from PSP driver probe time and

Actually, you're not removing it, yet...

Quoted 23 lines

move it to KVM module load time so that KVM can do SEV/SNP platform initialization explicitly if it actually wants to use SEV/SNP functionality.

With this patch, KVM will explicitly call into the PSP driver at load time to initialize SEV/SNP by default but this behavior can be altered with KVM module parameters to not do SEV/SNP platform initialization at module load time if required. Additionally SEV/SNP platform shutdown is invoked during KVM module unload time.

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

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

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

index 943bd074a5d3..0dc8294582c6 100644
@@ -444,7 +444,6 @@ static int __sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp,
 	if (ret)
 		goto e_no_asid;
 

Quoted 14 lines

arch/x86/kvm/svm/sev.c void __init sev_set_cpu_caps(void) · line 2955

Quoted 9 lines

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

-	init_args.probe = false;
 	ret = sev_platform_init(&init_args);
 	if (ret)
 		goto e_free;

@@ -2953,6 +2952,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};

Will this cause issues if KVM is built-in and INIT_EX is being used (init_ex_path ccp parameter)? The probe parameter is used for initialization done before the filesystem is available.

Yes, this will cause issues if KVM is builtin and INIT_EX is being used, but my question is how will INIT_EX be used when we move SEV INIT to KVM ?

If we continue to use the probe field here and also continue to support psp_init_on_probe module parameter for CCP, how will SEV INIT_EX be invoked ?

How is SEV INIT_EX invoked in PSP driver currently if psp_init_on_probe parameter is set to false ?

The KVM path to invoke sev_platform_init() when a SEV VM is being launched cannot be used because QEMU checks for SEV to be initialized before invoking this code path to launch the guest.

Thanks,
Ashish

Quoted 33 lines

Thanks,
Tom

Quoted 30 lines

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

 	bool sev_snp_supported = false;
 	bool sev_es_supported = false;
 	bool sev_supported = false;

@@ -3069,6 +3069,16 @@ void __init sev_hardware_setup(void)
 	sev_supported_vmsa_features = 0;
 	if (sev_es_debug_swap_enabled)
 		sev_supported_vmsa_features |= SVM_SEV_FEAT_DEBUG_SWAP;
+
+	if (!sev_enabled)
+		return;
+
+	/*
+	 * NOTE: Always do SNP INIT regardless of sev_snp_supported
+	 * as SNP INIT has to be done to launch legacy SEV/SEV-ES
+	 * VMs in case SNP is enabled system-wide.
+	 */
+	sev_platform_init(&init_args);
 }
 
 void sev_hardware_unsetup(void)

@@ -3084,6 +3094,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 */
+	sev_platform_shutdown();
 }
 
 int sev_cpu_init(struct svm_cpu_data *sd)

Quoted 57 lines · Ashish

Quoted 30 lines · Tom Lendacky

Quoted 3 lines · Ashish Kalra

From: Ashish Kalra <[email protected]>

Remove platform initialization of SEV/SNP from PSP driver probe time and

Actually, you're not removing it, yet...

Quoted 23 lines

move it to KVM module load time so that KVM can do SEV/SNP platform initialization explicitly if it actually wants to use SEV/SNP functionality.

With this patch, KVM will explicitly call into the PSP driver at load time to initialize SEV/SNP by default but this behavior can be altered with KVM module parameters to not do SEV/SNP platform initialization at module load time if required. Additionally SEV/SNP platform shutdown is invoked during KVM module unload time.

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

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

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

index 943bd074a5d3..0dc8294582c6 100644
@@ -444,7 +444,6 @@ static int __sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp,
 	if (ret)
 		goto e_no_asid;
 

Quoted 14 lines

arch/x86/kvm/svm/sev.c void __init sev_set_cpu_caps(void) · line 2955

Quoted 9 lines

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

-	init_args.probe = false;
 	ret = sev_platform_init(&init_args);
 	if (ret)
 		goto e_free;

@@ -2953,6 +2952,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};

Will this cause issues if KVM is built-in and INIT_EX is being used (init_ex_path ccp parameter)? The probe parameter is used for initialization done before the filesystem is available.

Yes, this will cause issues if KVM is builtin and INIT_EX is being used, but my question is how will INIT_EX be used when we move SEV INIT to KVM ?

If we continue to use the probe field here and also continue to support psp_init_on_probe module parameter for CCP, how will SEV INIT_EX be invoked ?

How is SEV INIT_EX invoked in PSP driver currently if psp_init_on_probe parameter is set to false ?

Quoted 3 lines

The KVM path to invoke sev_platform_init() when a SEV VM is being launched cannot be used because QEMU checks for SEV to be initialized before invoking this code path to launch the guest.

Qemu only requires that for an SEV-ES guest. I was able to use the init_ex_path=/root/... and psp_init_on_probe=0 to successfully delay SEV INIT_EX and launch an SEV guest.

Quoted 35 lines

Quoted 34 lines

Thanks,
Tom

Quoted 30 lines

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

 	bool sev_snp_supported = false;
 	bool sev_es_supported = false;
 	bool sev_supported = false;

@@ -3069,6 +3069,16 @@ void __init sev_hardware_setup(void)
 	sev_supported_vmsa_features = 0;
 	if (sev_es_debug_swap_enabled)
 		sev_supported_vmsa_features |= SVM_SEV_FEAT_DEBUG_SWAP;
+
+	if (!sev_enabled)
+		return;
+
+	/*
+	 * NOTE: Always do SNP INIT regardless of sev_snp_supported
+	 * as SNP INIT has to be done to launch legacy SEV/SEV-ES
+	 * VMs in case SNP is enabled system-wide.
+	 */
+	sev_platform_init(&init_args);
 }
 
 void sev_hardware_unsetup(void)

@@ -3084,6 +3094,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 */
+	sev_platform_shutdown();
 }
 
 int sev_cpu_init(struct svm_cpu_data *sd)

Quoted 62 lines · Tom Lendacky

Quoted 60 lines · Ashish

Quoted 30 lines · Tom Lendacky

Quoted 3 lines · Ashish Kalra

From: Ashish Kalra <[email protected]>

Remove platform initialization of SEV/SNP from PSP driver probe time and

Actually, you're not removing it, yet...

Quoted 23 lines

move it to KVM module load time so that KVM can do SEV/SNP platform initialization explicitly if it actually wants to use SEV/SNP functionality.

With this patch, KVM will explicitly call into the PSP driver at load time to initialize SEV/SNP by default but this behavior can be altered with KVM module parameters to not do SEV/SNP platform initialization at module load time if required. Additionally SEV/SNP platform shutdown is invoked during KVM module unload time.

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

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

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

index 943bd074a5d3..0dc8294582c6 100644
@@ -444,7 +444,6 @@ static int __sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp,
 	if (ret)
 		goto e_no_asid;
 

Quoted 14 lines

arch/x86/kvm/svm/sev.c void __init sev_set_cpu_caps(void) · line 2955

Quoted 9 lines

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

-	init_args.probe = false;
 	ret = sev_platform_init(&init_args);
 	if (ret)
 		goto e_free;

@@ -2953,6 +2952,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};

Will this cause issues if KVM is built-in and INIT_EX is being used (init_ex_path ccp parameter)? The probe parameter is used for initialization done before the filesystem is available.

Yes, this will cause issues if KVM is builtin and INIT_EX is being used, but my question is how will INIT_EX be used when we move SEV INIT to KVM ?

If we continue to use the probe field here and also continue to support psp_init_on_probe module parameter for CCP, how will SEV INIT_EX be invoked ?

How is SEV INIT_EX invoked in PSP driver currently if psp_init_on_probe parameter is set to false ?

The KVM path to invoke sev_platform_init() when a SEV VM is being launched cannot be used because QEMU checks for SEV to be initialized before invoking this code path to launch the guest.

Quoted 4 lines

Qemu only requires that for an SEV-ES guest. I was able to use the init_ex_path=/root/... and psp_init_on_probe=0 to successfully delay SEV INIT_EX and launch an SEV guest.

Thanks Tom, i will make sure that we continue to support both the probe field and psp_init_on_probe module parameter for CCP modules as part of v4.

Quoted 1 line

Quoted 1 line

Quoted 32 lines

Quoted 32 lines

Quoted 31 lines

Quoted 31 lines

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

 	bool sev_snp_supported = false;
 	bool sev_es_supported = false;
 	bool sev_supported = false;

@@ -3069,6 +3069,16 @@ void __init sev_hardware_setup(void)
 	sev_supported_vmsa_features = 0;
 	if (sev_es_debug_swap_enabled)
 		sev_supported_vmsa_features |= SVM_SEV_FEAT_DEBUG_SWAP;
+
+	if (!sev_enabled)
+		return;
+
+	/*
+	 * NOTE: Always do SNP INIT regardless of sev_snp_supported
+	 * as SNP INIT has to be done to launch legacy SEV/SEV-ES
+	 * VMs in case SNP is enabled system-wide.
+	 */
+	sev_platform_init(&init_args);
 }
 
 void sev_hardware_unsetup(void)

@@ -3084,6 +3094,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 */
+	sev_platform_shutdown();
 }
 
 int sev_cpu_init(struct svm_cpu_data *sd)

It looks like i have hit a serious blocker issue with this approach of moving SEV/SNP initialization to KVM module load time.

While testing with kvm_amd and PSP driver built-in, it looks like kvm_amd driver is being loaded/initialized before PSP driver is loaded, and that causes sev_platform_init() call from sev_hardware_setup(kvm_amd) to fail:

[   10.717898] kvm_amd: TSC scaling supported
[   10.722470] kvm_amd: Nested Virtualization enabled
[   10.727816] kvm_amd: Nested Paging enabled
[   10.732388] kvm_amd: LBR virtualization supported
[   10.737639] kvm_amd: SEV enabled (ASIDs 100 - 509)
[   10.742985] kvm_amd: SEV-ES enabled (ASIDs 1 - 99)
[   10.748333] kvm_amd: SEV-SNP enabled (ASIDs 1 - 99)
[   10.753768] PSP driver not init                        <<<---- sev_platform_init() returns failure as PSP driver is still not initialized
[   10.757563] kvm_amd: Virtual VMLOAD VMSAVE supported
[   10.763124] kvm_amd: Virtual GIF supported
...
...
[   12.514857] ccp 0000:23:00.1: enabling device (0000 -> 0002)
[   12.521691] ccp 0000:23:00.1: no command queues available
[   12.527991] ccp 0000:23:00.1: sev enabled
[   12.532592] ccp 0000:23:00.1: psp enabled
[   12.537382] ccp 0000:a2:00.1: enabling device (0000 -> 0002)
[   12.544389] ccp 0000:a2:00.1: no command queues available
[   12.550627] ccp 0000:a2:00.1: psp enabled

depmod -> modules.builtin show kernel/arch/x86/kvm/kvm_amd.ko higher on the list and before kernel/drivers/crypto/ccp/ccp.ko

modules.builtin:
kernel/arch/x86/kvm/kvm.ko
kernel/arch/x86/kvm/kvm-amd.ko
...
...
kernel/drivers/crypto/ccp/ccp.ko

I believe that the modules which are compiled first get called first and it looks like that the only way to change the order for builtin modules is by changing which makefiles get compiled first ?

Is there a way to change the load order of built-in modules and/or change dependency of built-in modules ?

Quoted 36 lines · Ashish Kalra

It looks like i have hit a serious blocker issue with this approach of moving SEV/SNP initialization to KVM module load time.

While testing with kvm_amd and PSP driver built-in, it looks like kvm_amd driver is being loaded/initialized before PSP driver is loaded, and that causes sev_platform_init() call from sev_hardware_setup(kvm_amd) to fail:

[   10.717898] kvm_amd: TSC scaling supported
[   10.722470] kvm_amd: Nested Virtualization enabled
[   10.727816] kvm_amd: Nested Paging enabled
[   10.732388] kvm_amd: LBR virtualization supported
[   10.737639] kvm_amd: SEV enabled (ASIDs 100 - 509)
[   10.742985] kvm_amd: SEV-ES enabled (ASIDs 1 - 99)
[   10.748333] kvm_amd: SEV-SNP enabled (ASIDs 1 - 99)
[   10.753768] PSP driver not init                        <<<---- sev_platform_init() returns failure as PSP driver is still not initialized
[   10.757563] kvm_amd: Virtual VMLOAD VMSAVE supported
[   10.763124] kvm_amd: Virtual GIF supported
...
...
[   12.514857] ccp 0000:23:00.1: enabling device (0000 -> 0002)
[   12.521691] ccp 0000:23:00.1: no command queues available
[   12.527991] ccp 0000:23:00.1: sev enabled
[   12.532592] ccp 0000:23:00.1: psp enabled
[   12.537382] ccp 0000:a2:00.1: enabling device (0000 -> 0002)
[   12.544389] ccp 0000:a2:00.1: no command queues available
[   12.550627] ccp 0000:a2:00.1: psp enabled

depmod -> modules.builtin show kernel/arch/x86/kvm/kvm_amd.ko higher on the list and before kernel/drivers/crypto/ccp/ccp.ko

modules.builtin:
kernel/arch/x86/kvm/kvm.ko
kernel/arch/x86/kvm/kvm-amd.ko
...
...
kernel/drivers/crypto/ccp/ccp.ko

Quoted 6 lines

I believe that the modules which are compiled first get called first and it looks like that the only way to change the order for builtin modules is by changing which makefiles get compiled first ?

Is there a way to change the load order of built-in modules and/or change dependency of built-in modules ?

The least awful option I know of would be to have the PSP use a higher priority initcall type so that it runs before the standard initcalls. When compiled as a module, all initcall types are #defined to module_init.

E.g. this should work, /cross fingers

drivers/crypto/ccp/sp-dev.c +0 -0

index 7eb3e4668286..02c49fbf6198 100644

drivers/crypto/ccp/sp-dev.c +0 -0

 module_exit(sp_mod_exit);

Quoted 17 lines · Sean Christopherson

Quoted 2 lines

Is there a way to change the load order of built-in modules and/or change dependency of built-in modules ?

The least awful option I know of would be to have the PSP use a higher priority initcall type so that it runs before the standard initcalls. When compiled as a module, all initcall types are #defined to module_init.

E.g. this should work, /cross fingers

drivers/crypto/ccp/sp-dev.c +0 -0

index 7eb3e4668286..02c49fbf6198 100644
@@ -295,5 +295,6 @@ static void __exit sp_mod_exit(void)
 #endif
 }
 

drivers/crypto/ccp/sp-dev.c static void __exit sp_mod_exit(void) · line 299

Quoted 3 lines

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

-module_init(sp_mod_init);
+/* The PSP needs to be initialized before dependent modules, e.g. before KVM. */
+subsys_initcall(sp_mod_init);

I was 2 seconds from clicking send with this exact suggestion. There are examples in 'drivers/' that use subsys_initcall / module_exit pairs.

Quoted 36 lines · Ashish Kalra

It looks like i have hit a serious blocker issue with this approach of moving SEV/SNP initialization to KVM module load time.

While testing with kvm_amd and PSP driver built-in, it looks like kvm_amd driver is being loaded/initialized before PSP driver is loaded, and that causes sev_platform_init() call from sev_hardware_setup(kvm_amd) to fail:

[   10.717898] kvm_amd: TSC scaling supported
[   10.722470] kvm_amd: Nested Virtualization enabled
[   10.727816] kvm_amd: Nested Paging enabled
[   10.732388] kvm_amd: LBR virtualization supported
[   10.737639] kvm_amd: SEV enabled (ASIDs 100 - 509)
[   10.742985] kvm_amd: SEV-ES enabled (ASIDs 1 - 99)
[   10.748333] kvm_amd: SEV-SNP enabled (ASIDs 1 - 99)
[   10.753768] PSP driver not init                        <<<---- sev_platform_init() returns failure as PSP driver is still not initialized
[   10.757563] kvm_amd: Virtual VMLOAD VMSAVE supported
[   10.763124] kvm_amd: Virtual GIF supported
...
...
[   12.514857] ccp 0000:23:00.1: enabling device (0000 -> 0002)
[   12.521691] ccp 0000:23:00.1: no command queues available
[   12.527991] ccp 0000:23:00.1: sev enabled
[   12.532592] ccp 0000:23:00.1: psp enabled
[   12.537382] ccp 0000:a2:00.1: enabling device (0000 -> 0002)
[   12.544389] ccp 0000:a2:00.1: no command queues available
[   12.550627] ccp 0000:a2:00.1: psp enabled

depmod -> modules.builtin show kernel/arch/x86/kvm/kvm_amd.ko higher on the list and before kernel/drivers/crypto/ccp/ccp.ko

modules.builtin:
kernel/arch/x86/kvm/kvm.ko
kernel/arch/x86/kvm/kvm-amd.ko
...
...
kernel/drivers/crypto/ccp/ccp.ko

Quoted 6 lines

I believe that the modules which are compiled first get called first and it looks like that the only way to change the order for builtin modules is by changing which makefiles get compiled first ?

Is there a way to change the load order of built-in modules and/or change dependency of built-in modules ?

The least awful option I know of would be to have the PSP use a higher priority initcall type so that it runs before the standard initcalls. When compiled as a module, all initcall types are #defined to module_init.

E.g. this should work, /cross fingers

drivers/crypto/ccp/sp-dev.c +0 -0

index 7eb3e4668286..02c49fbf6198 100644

Quoted 55 lines · Sean Christopherson

Quoted 36 lines · Ashish Kalra

It looks like i have hit a serious blocker issue with this approach of moving SEV/SNP initialization to KVM module load time.

While testing with kvm_amd and PSP driver built-in, it looks like kvm_amd driver is being loaded/initialized before PSP driver is loaded, and that causes sev_platform_init() call from sev_hardware_setup(kvm_amd) to fail:

[   10.717898] kvm_amd: TSC scaling supported
[   10.722470] kvm_amd: Nested Virtualization enabled
[   10.727816] kvm_amd: Nested Paging enabled
[   10.732388] kvm_amd: LBR virtualization supported
[   10.737639] kvm_amd: SEV enabled (ASIDs 100 - 509)
[   10.742985] kvm_amd: SEV-ES enabled (ASIDs 1 - 99)
[   10.748333] kvm_amd: SEV-SNP enabled (ASIDs 1 - 99)
[   10.753768] PSP driver not init                        <<<---- sev_platform_init() returns failure as PSP driver is still not initialized
[   10.757563] kvm_amd: Virtual VMLOAD VMSAVE supported
[   10.763124] kvm_amd: Virtual GIF supported
...
...
[   12.514857] ccp 0000:23:00.1: enabling device (0000 -> 0002)
[   12.521691] ccp 0000:23:00.1: no command queues available
[   12.527991] ccp 0000:23:00.1: sev enabled
[   12.532592] ccp 0000:23:00.1: psp enabled
[   12.537382] ccp 0000:a2:00.1: enabling device (0000 -> 0002)
[   12.544389] ccp 0000:a2:00.1: no command queues available
[   12.550627] ccp 0000:a2:00.1: psp enabled

depmod -> modules.builtin show kernel/arch/x86/kvm/kvm_amd.ko higher on the list and before kernel/drivers/crypto/ccp/ccp.ko

modules.builtin:
kernel/arch/x86/kvm/kvm.ko
kernel/arch/x86/kvm/kvm-amd.ko
...
...
kernel/drivers/crypto/ccp/ccp.ko

Quoted 6 lines

I believe that the modules which are compiled first get called first and it looks like that the only way to change the order for builtin modules is by changing which makefiles get compiled first ?

Is there a way to change the load order of built-in modules and/or change dependency of built-in modules ?

The least awful option I know of would be to have the PSP use a higher priority initcall type so that it runs before the standard initcalls. When compiled as a module, all initcall types are #defined to module_init.

E.g. this should work, /cross fingers

drivers/crypto/ccp/sp-dev.c +0 -0

index 7eb3e4668286..02c49fbf6198 100644

drivers/crypto/ccp/sp-dev.c static void __exit sp_mod_exit(void) · line 300

Quoted 7 lines

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

@@ -295,5 +295,6 @@ static void __exit sp_mod_exit(void)
 #endif
 }
 
-module_init(sp_mod_init);
+/* The PSP needs to be initialized before dependent modules, e.g. before KVM. */
+subsys_initcall(sp_mod_init);
 module_exit(sp_mod_exit);

Thanks for the suggestion, but there are actually two major issues here:

With the above change, PSP driver initialization fails as following:

...

[    7.274005] pci 0000:20:08.1: bridge window [mem 0xf6200000-0xf64fffff]: not claimed; can't enable device
[    7.277945] pci 0000:20:08.1: Error enabling bridge (-22), continuing
[    7.281947] ccp 0000:23:00.1: BAR 2 [mem 0xf6300000-0xf63fffff]: not claimed; can't enable device
[    7.285945] ccp 0000:23:00.1: pcim_enable_device failed (-22)
[    7.289943] ccp 0000:23:00.1: initialization failed
[    7.293944] ccp 0000:23:00.1: probe with driver ccp failed with error -22
[    7.301981] pci 0000:a0:08.1: bridge window [mem 0xb6200000-0xb63fffff]: not claimed; can't enable device
[    7.313956] pci 0000:a0:08.1: Error enabling bridge (-22), continuing
[    7.321947] ccp 0000:a2:00.1: BAR 2 [mem 0xb6200000-0xb62fffff]: not claimed; can't enable device
[    7.329945] ccp 0000:a2:00.1: pcim_enable_device failed (-22)
[    7.337943] ccp 0000:a2:00.1: initialization failed
[    7.341946] ccp 0000:a2:00.1: probe with driver ccp failed with error -22

...

It looks as PCI bus resource allocation is still not done, hence PSP driver cannot be enabled as early as subsys_initcall, it can be initialized probably via device_initcall(), but then that will be too late as kvm_amd would have been initialized before that.

Additionally, it looks like that there is an issue with SNP host support being enabled with kvm_amd module being built-in:

SNP host support is enabled in snp_rmptable_init() in arch/x86/virt/svm/sev.c, which is invoked as a device_initcall(). Here device_initcall() is used as snp_rmptable_init() expects AMD IOMMU SNP support to be enabled prior to it and the AMD IOMMU driver is initialized after PCI bus enumeration.

Now, if kvm_amd module is built-in, it gets initialized before SNP host support is enabled in snp_rmptable_init() :

[   10.131811] kvm_amd: TSC scaling supported
[   10.136384] kvm_amd: Nested Virtualization enabled
[   10.141734] kvm_amd: Nested Paging enabled
[   10.146304] kvm_amd: LBR virtualization supported
[   10.151557] kvm_amd: SEV enabled (ASIDs 100 - 509)
[   10.156905] kvm_amd: SEV-ES enabled (ASIDs 1 - 99)
[   10.162256] kvm_amd: SEV-SNP enabled (ASIDs 1 - 99)
[   10.167701] PSP driver not init
[   10.171508] kvm_amd: Virtual VMLOAD VMSAVE supported
[   10.177052] kvm_amd: Virtual GIF supported
...
...
[   10.201648] kvm_amd: in svm_enable_virtualization_cpu WRMSR VM_HSAVE_PA non-zero

And then svm_x86_ops->enable_virtualization_cpu() (svm_enable_virtualization_cpu) programs MSR_VM_HSAVE_PA as following:
wrmsrl(MSR_VM_HSAVE_PA, sd->save_area_pa);

So VM_HSAVE_PA is non-zero before SNP support is enabled on all CPUs.

snp_rmptable_init() gets invoked after svm_enable_virtualization_cpu() as following :
...

[   11.256138] kvm_amd: in svm_enable_virtualization_cpu WRMSR VM_HSAVE_PA non-zero
...
[   11.264918] SEV-SNP: in snp_rmptable_init

This triggers a #GP exception in snp_rmptable_init() when snp_enable() is invoked to set SNP_EN in SYSCFG MSR:

[   11.294289] unchecked MSR access error: WRMSR to 0xc0010010 (tried to write 0x0000000003fc0000) at rIP: 0xffffffffaf5d5c28 (native_write_msr+0x8/0x30)
...
[   11.294404] Call Trace:
[   11.294482]  <IRQ>
[   11.294513]  ? show_stack_regs+0x26/0x30
[   11.294522]  ? ex_handler_msr+0x10f/0x180
[   11.294529]  ? search_extable+0x2b/0x40
[   11.294538]  ? fixup_exception+0x2dd/0x340
[   11.294542]  ? exc_general_protection+0x14f/0x440
[   11.294550]  ? asm_exc_general_protection+0x2b/0x30
[   11.294557]  ? __pfx_snp_enable+0x10/0x10
[   11.294567]  ? native_write_msr+0x8/0x30
[   11.294570]  ? __snp_enable+0x5d/0x70
[   11.294575]  snp_enable+0x19/0x20
[   11.294578]  __flush_smp_call_function_queue+0x9c/0x3a0
[   11.294586]  generic_smp_call_function_single_interrupt+0x17/0x20
[   11.294589]  __sysvec_call_function+0x20/0x90
[   11.294596]  sysvec_call_function+0x80/0xb0
[   11.294601]  </IRQ>
[   11.294603]  <TASK>
[   11.294605]  asm_sysvec_call_function+0x1f/0x30
...
[   11.294631]  arch_cpu_idle+0xd/0x20
[   11.294633]  default_idle_call+0x34/0xd0
[   11.294636]  do_idle+0x1f1/0x230
[   11.294643]  ? complete+0x71/0x80
[   11.294649]  cpu_startup_entry+0x30/0x40
[   11.294652]  start_secondary+0x12d/0x160
[   11.294655]  common_startup_64+0x13e/0x141
[   11.294662]  </TASK>

This #GP exception is getting triggered due to the following errata for AMD family 19h Models 10h-1Fh Processors:

Processor may generate spurious #GP(0) Exception on WRMSR instruction:
Description:
The Processor will generate a spurious #GP(0) Exception on a WRMSR instruction if the following conditions are all met:
- the target of the WRMSR is a SYSCFG register.
- the write changes the value of SYSCFG.SNPEn from 0 to 1.
- One of the threads that share the physical core has a non-zero value in the VM_HSAVE_PA MSR.

The suggested workaround is when enabling SNP, program VM_HSAVE_PA to 0h on both threads that share a physical core before setting SYSCFG.SNPEn

The document being referred to above:
https://www.amd.com/content/dam/amd/en/documents/processor-tech-docs/revision-guides/57095-PUB_1_01.pdf

Therefore, with kvm_amd module being built-in, KVM/SVM initialization happens before Host SNP is enabled and this SVM initialization sets VM_HSAVE_PA to non-zero, which then triggers this #GP when SYSCFG.SNPEn is being set and this will subsequently cause SNP_INIT(_EX) to fail with INVALID_CONFIG error as SYSCFG[SnpEn] is not set on all CPUs.

So it looks like the current SNP host enabling code and effectively SNP is broken with respect to the KVM module being built-in.

Essentially SNP host enabling code should be invoked before KVM initialization, which is currently not the case when KVM is built-in.

Additionally, the PSP driver probably needs to be initialized at device_initcall level if it is built-in, but that is much later than KVM module initialization, therefore, that is blocker for moving SEV/SNP initialization to KVM module load time instead of PSP module probe time. Do note that i have verified and tested that PSP module initialization works when invoked as a device_initcall().

Quoted 5 lines · Ashish

SNP host support is enabled in snp_rmptable_init() in arch/x86/virt/svm/sev.c, which is invoked as a device_initcall(). Here device_initcall() is used as snp_rmptable_init() expects AMD IOMMU SNP support to be enabled prior to it and the AMD IOMMU driver is initialized after PCI bus enumeration.

Quoted 6 lines

Additionally, the PSP driver probably needs to be initialized at device_initcall level if it is built-in, but that is much later than KVM module initialization, therefore, that is blocker for moving SEV/SNP initialization to KVM module load time instead of PSP module probe time. Do note that i have verified and tested that PSP module initialization works when invoked as a device_initcall().

As a follow-up to the above issues, i have an important question:

Do we really need kvm_amd module to be built-in for SEV/SNP support ?

Is there any usage case/scenario where the kvm_amd module needs to be built-in for SEV/SNP support ?

Quoted 6 lines · Ashish Kalra

Quoted 5 lines · Ashish

SNP host support is enabled in snp_rmptable_init() in arch/x86/virt/svm/sev.c, which is invoked as a device_initcall(). Here device_initcall() is used as snp_rmptable_init() expects AMD IOMMU SNP support to be enabled prior to it and the AMD IOMMU driver is initialized after PCI bus enumeration.

Ugh. So. Many. Dependencies.

That's a kernel bug, full stop. RMP initialization very obviously is not device initialization.

Why isn't snp_rmptable_init() called from mem_encrypt_init()? AFAICT, arch_cpu_finalize_init() is called after IOMMU initialziation. And if that doesn't work, hack it into arch_post_acpi_subsys_init(). Using device_initcall() to initialization the RMP is insane, IMO.

Quoted 11 lines

Quoted 7 lines

Additionally, the PSP driver probably needs to be initialized at device_initcall level if it is built-in, but that is much later than KVM module initialization, therefore, that is blocker for moving SEV/SNP initialization to KVM module load time instead of PSP module probe time. Do note that i have verified and tested that PSP module initialization works when invoked as a device_initcall().

As a follow-up to the above issues, i have an important question:

Do we really need kvm_amd module to be built-in for SEV/SNP support ?

Yes.

Quoted 2 lines

Is there any usage case/scenario where the kvm_amd module needs to be built-in for SEV/SNP support ?

Don't care. I am 100% against setting a precedent of tying features to KVM being a module or not, especially since this is a solvable problem.

Ideally, the initcall infrastructure would let modules express dependencies, but I can appreciate that solving this generically would require a high amount of complexity.

Having KVM explicitly call into the PSP driver as needed isn't difficult, just gross. But for me, it's still far better giving up and requiring everything to be modules.

E.g.

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

index 943bd074a5d3..a2ee12e998f0 100644
@@ -2972,6 +2972,16 @@ void __init sev_hardware_setup(void)
            WARN_ON_ONCE(!boot_cpu_has(X86_FEATURE_FLUSHBYASID)))
                goto out;
 
+       /*
+        * The kernel's initcall infrastructure lacks the ability to express
+        * dependencies between initcalls, where as the modules infrastructure
+        * automatically handles dependencies via symbol loading.  Ensure the
+        * PSP SEV driver is initialized before proceeding if KVM is built-in,
+        * as the dependency isn't handled by the initcall infrastructure.
+        */
+       if (IS_BUILTIN(CONFIG_KVM_AMD) && sev_module_init())
+               goto out;
+
        /* Retrieve SEV CPUID information */
        cpuid(0x8000001f, &eax, &ebx, &ecx, &edx);
 

drivers/crypto/ccp/sp-dev.c +12 -0

index 7eb3e4668286..a0cdc03984cb 100644
@@ -253,8 +253,12 @@ struct sp_device *sp_get_psp_master_device(void)
 static int __init sp_mod_init(void)
 {
 #ifdef CONFIG_X86
+       static bool initialized;
        int ret;
 
+       if (initialized)
+               return 0;
+
        ret = sp_pci_init();
        if (ret)
                return ret;

@@ -263,6 +267,7 @@ static int __init sp_mod_init(void)
        psp_pci_init();
 #endif
 
+       initialized = true;
        return 0;
 #endif
 

@@ -279,6 +284,13 @@ static int __init sp_mod_init(void)
        return -ENODEV;
 }
 
+#if IS_BUILTIN(CONFIG_KVM_AMD) && IS_ENABLED(CONFIG_KVM_AMD_SEV)
+int __init sev_module_init(void)
+{
+       return sp_mod_init();
+}
+#endif
+
 static void __exit sp_mod_exit(void)
 {
 #ifdef CONFIG_X86

include/linux/psp-sev.h +0 -0

index 903ddfea8585..0138d22b46ac 100644