Skip to contentSkip to conversation

Implement SNP DOWNLOAD_FIRMWARE_EX support

RFC In review Tycho Andersen 4 participants 13 messages Apr 30 – Sep 2

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

cover Implement SNP DOWNLOAD_FIRMWARE_EX support

From: "Tycho Andersen (AMD)" <[email protected]>

Here is an implementation of the SEV-SNP firmware's DOWNLOAD_FIRMWARE_EX command. The core difference between this and the previous implementation
https://lore.kernel.org/lkml/[email protected]/
is that it relies on the SEV firmware's state (WORKING) to indicate that there are legacy VMs running instead of tracking things explicitly via ASID.

There is a race condition in slide 18 of
https://pretalx.com/media/kvm-forum-2025/submissions/TAMRR8/resources/SEV_FW_Hotl_zfT5e9Y.pdf
which this series does not address, I am still trying to understand what the best way to fix that is.

Also note that patch 1 is a duplicate of
https://lore.kernel.org/all/[email protected]/
so it can be dropped when that is applied.

Thanks,

Tycho

Tycho Andersen (AMD) (6):
crypto/ccp: Hoist kernel part of SNP_PLATFORM_STATUS crypto/ccp: Allow snp_get_platform_data() after SNP init crypto/ccp: Add DOWNLOAD_FIRMWARE_EX message struct crypto/ccp: Reclaim command buffer when the PSP dies crypto/ccp: Register with fw_uploader and always fail crypto/ccp: Implement SNP firmware live update

 drivers/crypto/ccp/sev-dev.c | 416 +++++++++++++++++++++++++++++++----
 drivers/crypto/ccp/sev-dev.h |   3 +
 include/linux/psp-sev.h      |  20 ++
 3 files changed, 393 insertions(+), 46 deletions(-)

base-commit: 254f49634ee16a731174d2ae34bc50bd5f45e731

Signature · 1 line
2.54.0
#1 lore raw

RFC 1/6 crypto/ccp: Hoist kernel part of SNP_PLATFORM_STATUS

From: "Tycho Andersen (AMD)" <[email protected]>

...to its own function. This way it can be used when the kernel needs access to the platform status regardless of the INIT state of the firmware.

No functional change intended.

---
 drivers/crypto/ccp/sev-dev.c | 31 +++++++++++++++++++++++--------
 1 file changed, 23 insertions(+), 8 deletions(-)
drivers/crypto/ccp/sev-dev.c +23 -8
index d1e9e0ac63b6..22bc4ef27a63 100644
@@ -2381,7 +2381,8 @@ static int sev_ioctl_do_pdh_export(struct sev_issue_cmd *argp, bool writable)
 	return ret;
 }
 
-static int sev_ioctl_do_snp_platform_status(struct sev_issue_cmd *argp)
+static int __sev_do_snp_platform_status(struct sev_user_data_snp_status *status,
+					int *error)
 {
 	struct sev_device *sev = psp_master->sev_data;
 	struct sev_data_snp_addr buf;
@@ -2389,9 +2390,6 @@ static int sev_ioctl_do_snp_platform_status(struct sev_issue_cmd *argp)
 	void *data;
 	int ret;
 
-	if (!argp->data)
-		return -EINVAL;
-
 	status_page = alloc_page(GFP_KERNEL_ACCOUNT);
 	if (!status_page)
 		return -ENOMEM;
@@ -2414,7 +2412,7 @@ static int sev_ioctl_do_snp_platform_status(struct sev_issue_cmd *argp)
 	}
 
 	buf.address = __psp_pa(data);
-	ret = __sev_do_cmd_locked(SEV_CMD_SNP_PLATFORM_STATUS, &buf, &argp->error);
+	ret = __sev_do_cmd_locked(SEV_CMD_SNP_PLATFORM_STATUS, &buf, error);
 
 	if (sev->snp_initialized) {
 		/*
@@ -2429,15 +2427,32 @@ static int sev_ioctl_do_snp_platform_status(struct sev_issue_cmd *argp)
 	if (ret)
 		goto cleanup;
 
-	if (copy_to_user((void __user *)argp->data, data,
-			 sizeof(struct sev_user_data_snp_status)))
-		ret = -EFAULT;
+	memcpy(status, data, sizeof(*status));
 
 cleanup:
 	__free_pages(status_page, 0);
 	return ret;
 }
 
+static int sev_ioctl_do_snp_platform_status(struct sev_issue_cmd *argp)
+{
+	struct sev_user_data_snp_status status;
+	int ret;
+
+	if (!argp->data)
+		return -EINVAL;
+
+	ret = __sev_do_snp_platform_status(&status, &argp->error);
+	if (ret < 0)
+		return ret;
+
+	if (copy_to_user((void __user *)argp->data, &status,
+			 sizeof(struct sev_user_data_snp_status)))
+		ret = -EFAULT;
+
+	return ret;
+}
+
 static int sev_ioctl_do_snp_commit(struct sev_issue_cmd *argp)
 {
 	struct sev_device *sev = psp_master->sev_data;
Signature · 1 line
2.54.0
#2 lore raw

RFC 2/6 crypto/ccp: Allow snp_get_platform_data() after SNP init

In reply to Tycho Andersen

From: "Tycho Andersen (AMD)" <[email protected]>

In preparation for refreshing the cached SNP platform status and feature information after a successful firmware live update, allow snp_get_platform_data() to be called when the SNP firmware is in the INIT state.

When SNP is initialized the firmware additionally requires status pages to be in the firmware-owned RMP state. __sev_do_snp_platform_status() already handles this for SNP_PLATFORM_STATUS, so switch to that helper for that command. Add the same mark/reclaim dance around the SNP_FEATURE_INFO page.

---
 drivers/crypto/ccp/sev-dev.c | 31 +++++++++++++++++++++----------
 1 file changed, 21 insertions(+), 10 deletions(-)
drivers/crypto/ccp/sev-dev.c +21 -10
index 22bc4ef27a63..7ca29ccda0e7 100644
@@ -132,6 +132,9 @@ static void __sev_firmware_shutdown(struct sev_device *sev, bool panic);
 static int snp_shutdown_on_panic(struct notifier_block *nb,
 				 unsigned long reason, void *arg);
 
+static int __sev_do_snp_platform_status(struct sev_user_data_snp_status *status,
+					int *error);
+
 static struct notifier_block snp_panic_notifier = {
 	.notifier_call = snp_shutdown_on_panic,
 };
@@ -1264,19 +1267,12 @@ static int snp_get_platform_data(struct sev_device *sev, int *error)
 {
 	struct sev_data_snp_feature_info snp_feat_info;
 	struct snp_feature_info *feat_info;
-	struct sev_data_snp_addr buf;
 	struct page *page;
 	int rc;
 
-	/*
-	 * This function is expected to be called before SNP is
-	 * initialized.
-	 */
-	if (sev->snp_initialized)
-		return -EINVAL;
-
-	buf.address = __psp_pa(&sev->snp_plat_status);
-	rc = sev_do_cmd(SEV_CMD_SNP_PLATFORM_STATUS, &buf, error);
+	mutex_lock(&sev_cmd_mutex);
+	rc = __sev_do_snp_platform_status(&sev->snp_plat_status, error);
+	mutex_unlock(&sev_cmd_mutex);
 	if (rc) {
 		dev_err(sev->dev, "SNP PLATFORM_STATUS command failed, ret = %d, error = %#x\n",
 			rc, *error);
@@ -1305,17 +1301,32 @@ static int snp_get_platform_data(struct sev_device *sev, int *error)
 		return -ENOMEM;
 
 	feat_info = page_address(page);
+
+	if (sev->snp_initialized) {
+		if (rmp_mark_pages_firmware(__pa(feat_info), 1, false)) {
+			rc = -EFAULT;
+			goto free_page;
+		}
+	}
+
 	snp_feat_info.length = sizeof(snp_feat_info);
 	snp_feat_info.ecx_in = 0;
 	snp_feat_info.feature_info_paddr = __psp_pa(feat_info);
 
 	rc = sev_do_cmd(SEV_CMD_SNP_FEATURE_INFO, &snp_feat_info, error);
+
+	if (sev->snp_initialized) {
+		if (snp_reclaim_pages(__pa(feat_info), 1, false))
+			return -EFAULT;
+	}
+
 	if (!rc)
 		sev->snp_feat_info_0 = *feat_info;
 	else
 		dev_err(sev->dev, "SNP FEATURE_INFO command failed, ret = %d, error = %#x\n",
 			rc, *error);
 
+free_page:
 	__free_page(page);
 
 	return rc;
Signature · 1 line
2.54.0
#3 lore raw

RFC 3/6 crypto/ccp: Add DOWNLOAD_FIRMWARE_EX message struct

In reply to Tycho Andersen

From: "Tycho Andersen (AMD)" <[email protected]>

...and appropriate sizeof() in sev_cmd_buffer_len() for use in do_sev_cmd(). The message is documented in SEV-SNP firmware document id 56860.

---
 drivers/crypto/ccp/sev-dev.c |  1 +
 include/linux/psp-sev.h      | 20 ++++++++++++++++++++
 2 files changed, 21 insertions(+)
drivers/crypto/ccp/sev-dev.c +1 -0
index 7ca29ccda0e7..defdc1bc226e 100644
@@ -252,6 +252,7 @@ static int sev_cmd_buffer_len(int cmd)
 	case SEV_CMD_SNP_PLATFORM_STATUS:	return sizeof(struct sev_data_snp_addr);
 	case SEV_CMD_SNP_GUEST_REQUEST:		return sizeof(struct sev_data_snp_guest_request);
 	case SEV_CMD_SNP_CONFIG:		return sizeof(struct sev_user_data_snp_config);
+	case SEV_CMD_SNP_DOWNLOAD_FIRMWARE_EX:	return sizeof(struct sev_data_download_firmware_ex);
 	case SEV_CMD_SNP_COMMIT:		return sizeof(struct sev_data_snp_commit);
 	case SEV_CMD_SNP_FEATURE_INFO:		return sizeof(struct sev_data_snp_feature_info);
 	case SEV_CMD_SNP_VLEK_LOAD:		return sizeof(struct sev_user_data_snp_vlek_load);
include/linux/psp-sev.h +20 -0
index d5099a2baca5..5227e901bff2 100644
@@ -855,6 +855,26 @@ struct sev_platform_init_args {
 	unsigned int max_snp_asid;
 };
 
+/**
+ * struct sev_data_download_firmware_ex
+ *
+ * @len: length of the command buffer read by the PSP
+ * @rsvd0: reserved
+ * @fw_paddr: physical address of the start of the firmware blob
+ * @fw_len: length of the firmware blob
+ * @commit: whether to immediately commit the firmware update. If set, this
+ *  operation behaves like DOWNLOAD_FIRMWARE.
+ * @rsvd1: reserved
+ */
+struct sev_data_download_firmware_ex {
+	u32 len;		/* In */
+	u32 rsvd0;
+	u64 fw_paddr;		/* In */
+	u32 fw_len;		/* In */
+	u32 commit:1;		/* In */
+	u32 rsvd1:31;
+} __packed;
+
 /**
  * struct sev_data_snp_commit - SNP_COMMIT structure
  *
Signature · 1 line
2.54.0
#4 lore raw

RFC 4/6 crypto/ccp: Reclaim command buffer when the PSP dies

In reply to Tycho Andersen

From: "Tycho Andersen (AMD)" <[email protected]>

When the PSP dies due to timeout the psp_dead flag is set, but the buffer-in-use flag was not unset, and the pages were not reclaimed for legacy commands.

In preparation for a firmware quirk where updates time out but the situation is recoverable, move the reclamation before the error checking and handling. Be sure to only copy the output buffer when the command has not timed out, i.e. when there is sensible output in the buffer.

---
 drivers/crypto/ccp/sev-dev.c | 60 +++++++++++++++++++-----------------
 1 file changed, 32 insertions(+), 28 deletions(-)
drivers/crypto/ccp/sev-dev.c +32 -28
index defdc1bc226e..2df621b9f6e2 100644
@@ -948,6 +948,38 @@ int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
 
 	/* wait for command completion */
 	ret = sev_wait_cmd_ioc(sev, &reg, psp_timeout);
+
+	/*
+	 * Copy potential output from the PSP back to data.  Do this even on
+	 * failure in case the caller wants to glean something from the error,
+	 * unless the operation timed out, in which case there is nothing to
+	 * copy back.
+	 */
+	if (data) {
+		int ret_reclaim;
+		/*
+		 * Restore the page state after the command completes.
+		 */
+		ret_reclaim = snp_reclaim_cmd_buf(cmd, cmd_buf);
+		if (ret_reclaim) {
+			dev_err(sev->dev,
+				"SEV: failed to reclaim buffer for legacy command %#x. Error: %d\n",
+				cmd, ret_reclaim);
+			return ret_reclaim;
+		}
+
+		if (ret != -ETIMEDOUT)
+			memcpy(data, cmd_buf, buf_len);
+
+		if (sev->cmd_buf_backup_active)
+			sev->cmd_buf_backup_active = false;
+		else
+			sev->cmd_buf_active = false;
+
+		if (snp_unmap_cmd_buf_desc_list(desc_list))
+			return -EFAULT;
+	}
+
 	if (ret) {
 		if (psp_ret)
 			*psp_ret = 0;
@@ -984,34 +1016,6 @@ int __sev_do_cmd_locked(int cmd, void *data, int *psp_ret)
 		ret = sev_write_init_ex_file_if_required(cmd);
 	}
 
-	/*
-	 * Copy potential output from the PSP back to data.  Do this even on
-	 * failure in case the caller wants to glean something from the error.
-	 */
-	if (data) {
-		int ret_reclaim;
-		/*
-		 * Restore the page state after the command completes.
-		 */
-		ret_reclaim = snp_reclaim_cmd_buf(cmd, cmd_buf);
-		if (ret_reclaim) {
-			dev_err(sev->dev,
-				"SEV: failed to reclaim buffer for legacy command %#x. Error: %d\n",
-				cmd, ret_reclaim);
-			return ret_reclaim;
-		}
-
-		memcpy(data, cmd_buf, buf_len);
-
-		if (sev->cmd_buf_backup_active)
-			sev->cmd_buf_backup_active = false;
-		else
-			sev->cmd_buf_active = false;
-
-		if (snp_unmap_cmd_buf_desc_list(desc_list))
-			return -EFAULT;
-	}
-
 	print_hex_dump_debug("(out): ", DUMP_PREFIX_OFFSET, 16, 2, data,
 			     buf_len, false);
 
Signature · 1 line
2.54.0
#5 lore raw

RFC 5/6 crypto/ccp: Register with fw_uploader and always fail

In reply to Tycho Andersen

From: "Tycho Andersen (AMD)" <[email protected]>

In preparation for SNP live firmware downloading support, add an 'sev' firmware loader that always fails with EBUSY.

---
 drivers/crypto/ccp/sev-dev.c | 51 ++++++++++++++++++++++++++++++++++++
 drivers/crypto/ccp/sev-dev.h |  3 +++
 2 files changed, 54 insertions(+)
drivers/crypto/ccp/sev-dev.c +51 -0
index 2df621b9f6e2..b4711bf823e8 100644
@@ -2040,6 +2040,53 @@ static int sev_update_firmware(struct device *dev)
 	return ret;
 }
 
+static enum fw_upload_err sev_fw_upload_prepare(struct fw_upload *fw_upload,
+						const u8 *data, u32 size)
+{
+	return FW_UPLOAD_ERR_NONE;
+}
+
+static enum fw_upload_err sev_fw_upload_write(struct fw_upload *fw_upload,
+					      const u8 *data, u32 offset,
+					      u32 size, u32 *written)
+{
+	return FW_UPLOAD_ERR_BUSY;
+}
+
+static enum fw_upload_err sev_fw_upload_poll_complete(struct fw_upload *fw_upload)
+{
+	return FW_UPLOAD_ERR_NONE;
+}
+
+static void sev_fw_upload_cancel(struct fw_upload *fw_upload)
+{
+	/* intentional no-op */
+}
+
+static const struct fw_upload_ops sev_fw_upload_ops = {
+	.prepare = sev_fw_upload_prepare,
+	.write = sev_fw_upload_write,
+	.poll_complete = sev_fw_upload_poll_complete,
+	.cancel = sev_fw_upload_cancel,
+};
+
+static void register_sev_fw_uploader(struct sev_device *sev)
+{
+	struct fw_upload *fwl;
+
+	if (!IS_ENABLED(CONFIG_FW_UPLOAD))
+		return;
+
+	fwl = firmware_upload_register(THIS_MODULE, sev->dev, "sev",
+				       &sev_fw_upload_ops, sev);
+	if (IS_ERR(fwl)) {
+		dev_err(sev->dev, "SEV firmware upload registration failure: %ld\n", PTR_ERR(fwl));
+		return;
+	}
+
+	sev->fwl = fwl;
+}
+
 static int __sev_snp_shutdown_locked(int *error, bool panic)
 {
 	struct psp_device *psp = psp_master;
@@ -2953,6 +3000,7 @@ void sev_pci_init(void)
 			 api_major, api_minor, build,
 			 sev->api_major, sev->api_minor, sev->build);
 
+	register_sev_fw_uploader(sev);
 	return;
 
 err:
@@ -2969,4 +3017,7 @@ void sev_pci_exit(void)
 		return;
 
 	sev_firmware_shutdown(sev);
+
+	if (sev->fwl)
+		firmware_upload_unregister(sev->fwl);
 }
drivers/crypto/ccp/sev-dev.h +3 -0
index b1cd556bbbf6..2f5781cb7bb1 100644
@@ -24,6 +24,7 @@
 #include <linux/psp-sev.h>
 #include <linux/miscdevice.h>
 #include <linux/capability.h>
+#include <linux/firmware.h>
 
 #define SEV_CMDRESP_CMD			GENMASK(26, 16)
 #define SEV_CMD_COMPLETE		BIT(1)
@@ -66,6 +67,8 @@ struct sev_device {
 
 	struct tsm_dev *tsmdev;
 	struct sev_tio_status *tio_status;
+
+	struct fw_upload *fwl;
 };
 
 int sev_dev_init(struct psp_device *psp);
Signature · 1 line
2.54.0
#6 lore raw

RFC 6/6 crypto/ccp: Implement SNP firmware live update

In reply to Tycho Andersen

From: "Tycho Andersen (AMD)" <[email protected]>

Put all the previous primitives together to implement SNP firmware live update via DOWNLOAD_FIRMWARE_EX.

DOWNLOAD_FIRMWARE_EX can only be run while the legacy SEV firmware is the UNINIT state. If the legacy firmware is in the WORKING state running legacy guests, refuse to update. If the legacy firmware is in the INIT state, de-initialize it so that the update can be run.

When the firmware is installed, it is only provisionally loaded. It relies on userspace to issue ioctl(/dev/sev, SNP_COMMIT, ...) when it is happy with the provisional firmware.

To roll back, userspace should not do an SNP_COMMIT, and invoke the firmware loader in the same way but with the old firmware image. The firmware spec notes:

If a guest context page is updated to a provisional firmware version, then updating the context page back to the committed version after a rollback will always succeed.

There are essentially four classes of errors during an update:

1. kernel bugs that WARN_ON_ONCE()
2. invalid firmware (bad image, bad signature, downgrade too far, etc.)
3. UPDATE_FAILED, things can continue as normally
4. HARDWARE_UNSAFE, declare the PSP dead, since the behavior of the SEV firmware is undefined
5. RESTORE_REQUIRED, the firmware can only successfully execute DOWNLOAD_FIRMWARE_EX commands. The admin needs to load the old firmware image.

There is a firmware bug where upgrades across 1.58.03 time out, even though the upgrade actually succeeds. There is no documented way to determine what the input firmware version is, so there is no way to detect this case before trying a firmware update. Instead look for the timeout and try an SNP_PLATFORM_STATUS to see if the PSP is still alive.

Finally, this differs from the previous implementation [1] in a couple of ways:

1. guest context pages are no longer required to be updated as of 1.58 of the SEV-SNP Firmware spec doc 56860.
2. no WBINVD+DF_FLUSH is required after a firmware update, so it drops that code

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

---
 drivers/crypto/ccp/sev-dev.c | 244 ++++++++++++++++++++++++++++++++++-
 1 file changed, 243 insertions(+), 1 deletion(-)
drivers/crypto/ccp/sev-dev.c +243 -1
index b4711bf823e8..e7fe6dbf69c2 100644
@@ -90,6 +90,8 @@ MODULE_FIRMWARE("amd/amd_sev_fam19h_model1xh.sbin"); /* 4th gen EPYC */
 
 static bool psp_dead;
 static int psp_timeout;
+static bool dlfwex_wants_rollback;
+static bool sev_firmware_needs_reinit;
 
 enum snp_hv_fixed_pages_state {
 	ALLOCATED,
@@ -2046,11 +2048,251 @@ static enum fw_upload_err sev_fw_upload_prepare(struct fw_upload *fw_upload,
 	return FW_UPLOAD_ERR_NONE;
 }
 
+static int sev_download_firmware_ex(struct sev_device *sev, const u8 *data,
+				    u32 size)
+{
+	struct sev_data_download_firmware_ex sev_data = {0};
+	int ret, error = 0, order;
+	struct page *p;
+	void *fw_blob;
+
+	order = get_order(size);
+	p = alloc_pages(GFP_KERNEL, order);
+	if (!p)
+		return -ENOMEM;
+
+	fw_blob = page_address(p);
+	memcpy(fw_blob, data, size);
+
+	sev_data.len = sizeof(sev_data);
+	sev_data.fw_paddr = __psp_pa(fw_blob);
+	sev_data.fw_len = size;
+	sev_data.commit = 0;
+
+	ret = __sev_do_cmd_locked(SEV_CMD_SNP_DOWNLOAD_FIRMWARE_EX, &sev_data, &error);
+
+	/*
+	 * Quirk: firmware upgrades across 1.58.03 give ETIMEDOUT for
+	 * DLFWEX, even though the command actually succeeds. If we're
+	 * in this case, test that we can do SNP_PLATFORM_STATUS, and
+	 * if so, continue as normal.
+	 */
+	if (ret == -ETIMEDOUT) {
+		struct sev_user_data_snp_status status;
+
+		dev_info(sev->dev, "Firmware update timed out, checking status for quirk...\n");
+		psp_dead = false;
+
+		ret = __sev_do_snp_platform_status(&status, &error);
+		if (ret) {
+			dev_err(sev->dev, "SNP STATUS failed after firmware upgrade, ret = %d, error = %#x\n",
+				ret, error);
+			psp_dead = true;
+			goto out;
+		}
+	}
+
+	if (ret < 0 && error != 0)
+		ret = error;
+
+out:
+	__free_pages(p, order);
+	return ret;
+}
+
+static int sev_firmware_shutdown_if_sev_initialized(struct sev_device *sev)
+{
+	int rc, error, sev_plat_state;
+
+	lockdep_assert_held(&sev_cmd_mutex);
+
+	error = 0;
+	rc = sev_get_platform_state(&sev_plat_state, &error);
+	if (rc < 0) {
+		if (error)
+			rc = error;
+		dev_dbg(sev->dev, "SEV get platform state failed %d\n", rc);
+		return rc;
+	}
+
+	switch (sev_plat_state) {
+	case SEV_STATE_UNINIT:
+		return 0;
+	case SEV_STATE_INIT:
+		error = 0;
+		rc = __sev_platform_shutdown_locked(&error);
+		if (rc) {
+			if (error)
+				rc = error;
+			dev_err(sev->dev, "SEV platform shutdown failed %d\n", rc);
+			return rc;
+		}
+		sev_firmware_needs_reinit = true;
+		return 0;
+	case SEV_STATE_WORKING:
+		return -EBUSY;
+	default:
+		dev_err(sev->dev, "Unknown SEV firmware state: %d\n", sev_plat_state);
+		return -EINVAL;
+	}
+}
+
+static void sev_firmware_reinit_if_shutdown(struct sev_device *sev)
+{
+	int rc, error;
+
+	guard(mutex)(&sev_cmd_mutex);
+
+	if (!sev_firmware_needs_reinit)
+		return;
+
+	sev_firmware_needs_reinit = false;
+	error = 0;
+	rc = __sev_platform_init_locked(&error);
+	if (rc) {
+		if (error)
+			rc = error;
+		dev_err(sev->dev, "SEV platform re-init failed %d\n", rc);
+	}
+}
+
 static enum fw_upload_err sev_fw_upload_write(struct fw_upload *fw_upload,
 					      const u8 *data, u32 offset,
 					      u32 size, u32 *written)
 {
-	return FW_UPLOAD_ERR_BUSY;
+	struct sev_device *sev = fw_upload->dd_handle;
+	u8 old_major, old_minor, old_build;
+	int rc, error = 0;
+	enum fw_upload_err ret;
+
+	if (offset != 0)
+		return FW_UPLOAD_ERR_INVALID_SIZE;
+
+	old_major = sev->api_major;
+	old_minor = sev->api_minor;
+	old_build = sev->build;
+
+	mutex_lock(&sev_cmd_mutex);
+
+	/*
+	 * If the last firmware update returned RESTORE_REQUIRED, allow only
+	 * this DLFWEX command so the admin can restore the previous FW
+	 * version. If we are in this state the legacy firmware has previously
+	 * been shut down, so no need to do it again.
+	 */
+	if (dlfwex_wants_rollback && psp_dead) {
+		dlfwex_wants_rollback = false;
+		psp_dead = false;
+	} else {
+		rc = sev_firmware_shutdown_if_sev_initialized(sev);
+		if (rc) {
+			ret = FW_UPLOAD_ERR_BUSY;
+			goto unlock;
+		}
+	}
+
+	rc = sev_download_firmware_ex(sev, data, size);
+	if (rc) {
+		ret = FW_UPLOAD_ERR_FW_INVALID;
+		switch (rc) {
+		case SEV_RET_INVALID_PLATFORM_STATE:
+			fallthrough;
+		case SEV_RET_INVALID_ADDRESS:
+			/* these are probably kernel bugs */
+			WARN_ON_ONCE(true);
+			ret = FW_UPLOAD_ERR_BUSY;
+			goto unlock;
+		case SEV_RET_INVALID_LEN:
+			ret = FW_UPLOAD_ERR_INVALID_SIZE;
+			goto unlock;
+		case SEV_RET_INVALID_PARAM:
+			dev_err(sev->dev, "SEV firmware image is not well formed\n");
+			goto unlock;
+		case SEV_RET_SHUTDOWN_REQUIRED:
+			dev_err(sev->dev, "SEV firmware too far, shutdown required\n");
+			goto unlock;
+		case SEV_RET_INVALID_CONFIG:
+			dev_err(sev->dev, "SEV firmware upgrade would rollback SVN\n");
+			goto unlock;
+		case SEV_RET_BAD_SIGNATURE:
+			dev_err(sev->dev, "SEV firmware upgrade bad signature\n");
+			goto unlock;
+		case SEV_RET_BAD_VERSION:
+			dev_err(sev->dev, "SEV firmware upgrade less than CommittedVersion\n");
+			goto unlock;
+		case SEV_RET_UNSUPPORTED:
+			dev_err(sev->dev, "SEV firmware required feature not supported\n");
+			goto unlock;
+		case SEV_RET_UPDATE_FAILED:
+			/*
+			 * Update failed but fw rolled back on its own,
+			 * operation can continue normally.
+			 */
+			dev_err(sev->dev, "SEV firmware update failed\n");
+			ret = FW_UPLOAD_ERR_HW_ERROR;
+			goto unlock;
+		case SEV_RET_HWSEV_RET_UNSAFE:
+			/*
+			 * "Following a return of HARDWARE_UNSAFE, operation of
+			 * the SEV firmware is indeterminate
+			 * and the recommendation is to reboot the platform."
+			 */
+			dev_err(sev->dev, "SEV firmware no longer safe to operate\n");
+			psp_dead = true;
+			ret = FW_UPLOAD_ERR_HW_ERROR;
+			goto unlock;
+		case SEV_RET_RESTORE_REQUIRED:
+			/*
+			 * FW asked us to roll back; we don't hold onto the
+			 * last FW image, so we can't. We can set a flag to
+			 * allow the admin to rollback if they happen to have
+			 * the old firmware image handy.
+			 */
+			dev_err(sev->dev, "SEV firmware update failed, please roll back\n");
+			psp_dead = true;
+			dlfwex_wants_rollback = true;
+			ret = FW_UPLOAD_ERR_HW_ERROR;
+			goto unlock;
+		default:
+			dev_err(sev->dev, "Unknown SEV firmware err %d\n", rc);
+			ret = FW_UPLOAD_ERR_HW_ERROR;
+			goto unlock;
+		}
+	}
+
+	*written = size;
+	ret = FW_UPLOAD_ERR_NONE;
+
+unlock:
+	mutex_unlock(&sev_cmd_mutex);
+
+	/*
+	 * sev_get_api_version() updates the SEV and SNP statuses, SNP feature
+	 * info if available, build numbers, etc. cached in struct sev_device.
+	 * Update these if they may have changed for new firmware.
+	 */
+	if (ret == FW_UPLOAD_ERR_NONE) {
+		error = 0;
+
+		rc = sev_get_api_version();
+		if (rc) {
+			dev_warn(sev->dev,
+				 "SNP platform data refresh after firmware update failed %d\n",
+				 rc);
+		} else if (sev->api_major != old_major ||
+			   sev->api_minor != old_minor ||
+			   sev->build != old_build) {
+			dev_info(sev->dev, "SEV firmware updated to %d.%d build %d\n",
+				 sev->api_major, sev->api_minor, sev->build);
+		} else {
+			dev_info(sev->dev, "SEV firmware not updated\n");
+		}
+	}
+
+	if (!dlfwex_wants_rollback)
+		sev_firmware_reinit_if_shutdown(sev);
+
+	return ret;
 }
 
 static enum fw_upload_err sev_fw_upload_poll_complete(struct fw_upload *fw_upload)
Signature · 1 line
2.54.0
#7 lore raw

Re: [RFC v1 6/6] crypto/ccp: Implement SNP firmware live update

Quoted 5 lines · Tycho Andersen

From: "Tycho Andersen (AMD)" <[email protected]>

Put all the previous primitives together to implement SNP firmware live update via DOWNLOAD_FIRMWARE_EX.

[snip]

Quoted 12 lines

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

---
 drivers/crypto/ccp/sev-dev.c | 244 ++++++++++++++++++++++++++++++++++-
 1 file changed, 243 insertions(+), 1 deletion(-)
drivers/crypto/ccp/sev-dev.c +0 -0
index b4711bf823e8..e7fe6dbf69c2 100644

drivers/crypto/ccp/sev-dev.c static enum fw_upload_err sev_fw_upload_prepare(struct fw_upload *fw_upload, · line 2055

Quoted 5 lines
drivers/crypto/ccp/sev-dev.c +5 -0
+static int sev_download_firmware_ex(struct sev_device *sev, const u8 *data,
+				    u32 size)
+{
+	struct sev_data_download_firmware_ex sev_data = {0};
+	int ret, error = 0, order;

Why not assign across multiple lines? How about something like:

int ret, order;
int error = 0;

or:

int ret;
int order;
int error = 0;

Would be better for readability and git blame.

Quoted 5 lines

static enum fw_upload_err sev_fw_upload_write(struct fw_upload *fw_upload,

drivers/crypto/ccp/sev-dev.c +0 -0
 					      const u8 *data, u32 offset,
 					      u32 size, u32 *written)

{

[snip]

drivers/crypto/ccp/sev-dev.c static enum fw_upload_err sev_fw_upload_prepare(struct fw_upload *fw_upload, · line 2175

Quoted 7 lines
drivers/crypto/ccp/sev-dev.c +5 -0
+	old_major = sev->api_major;
+	old_minor = sev->api_minor;
+	old_build = sev->build;
+
+	mutex_lock(&sev_cmd_mutex);

Why not guard(mutex)()? You used it earlier in
sev_firmware_reinit_if_shutdown().

drivers/crypto/ccp/sev-dev.c static enum fw_upload_err sev_fw_upload_prepare(struct fw_upload *fw_upload, · line 2267

Quoted 7 lines
drivers/crypto/ccp/sev-dev.c +5 -0
+	*written = size;
+	ret = FW_UPLOAD_ERR_NONE;
+
+unlock:
+	mutex_unlock(&sev_cmd_mutex);

See above comment.

best regards,
maxwell

#8 lore raw

Re: [RFC v1 6/6] crypto/ccp: Implement SNP firmware live update

Quoted 69 lines · Maxwell Doose
Quoted 5 lines · Tycho Andersen

From: "Tycho Andersen (AMD)" <[email protected]>

Put all the previous primitives together to implement SNP firmware live update via DOWNLOAD_FIRMWARE_EX.

[snip]

Quoted 12 lines

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

---
 drivers/crypto/ccp/sev-dev.c | 244 ++++++++++++++++++++++++++++++++++-
 1 file changed, 243 insertions(+), 1 deletion(-)
drivers/crypto/ccp/sev-dev.c +0 -0
index b4711bf823e8..e7fe6dbf69c2 100644

drivers/crypto/ccp/sev-dev.c static enum fw_upload_err sev_fw_upload_prepare(struct fw_upload *fw_upload, · line 2055

Quoted 5 lines
drivers/crypto/ccp/sev-dev.c +5 -0
+static int sev_download_firmware_ex(struct sev_device *sev, const u8 *data,
+				    u32 size)
+{
+	struct sev_data_download_firmware_ex sev_data = {0};
+	int ret, error = 0, order;

Why not assign across multiple lines? How about something like:

int ret, order;
int error = 0;

or:

int ret;
int order;
int error = 0;

Would be better for readability and git blame.

Quoted 5 lines

static enum fw_upload_err sev_fw_upload_write(struct fw_upload *fw_upload,

drivers/crypto/ccp/sev-dev.c +0 -0
 					      const u8 *data, u32 offset,
 					      u32 size, u32 *written)

{

[snip]

drivers/crypto/ccp/sev-dev.c static enum fw_upload_err sev_fw_upload_prepare(struct fw_upload *fw_upload, · line 2175

Quoted 7 lines
drivers/crypto/ccp/sev-dev.c +5 -0
+	old_major = sev->api_major;
+	old_minor = sev->api_minor;
+	old_build = sev->build;
+
+	mutex_lock(&sev_cmd_mutex);

Why not guard(mutex)()? You used it earlier in
sev_firmware_reinit_if_shutdown().

drivers/crypto/ccp/sev-dev.c static enum fw_upload_err sev_fw_upload_prepare(struct fw_upload *fw_upload, · line 2267

Quoted 7 lines
drivers/crypto/ccp/sev-dev.c +5 -0
+	*written = size;
+	ret = FW_UPLOAD_ERR_NONE;
+
+unlock:
+	mutex_unlock(&sev_cmd_mutex);

See above comment.

Quoted 2 lines

best regards,
maxwell

Also, forgot to mention:

Quoted 1 line
Quoted 1 line

static enum fw_upload_err sev_fw_upload_write(struct fw_upload *fw_upload,

const u8 *data, u32 offset, u32 size, u32 *written)

Quoted 6 lines

drivers/crypto/ccp/sev-dev.c static enum fw_upload_err sev_fw_upload_prepare(struct fw_upload *fw_upload, · line 2165

Quoted 6 lines

{

drivers/crypto/ccp/sev-dev.c +3 -1
-	return FW_UPLOAD_ERR_BUSY;
+	struct sev_device *sev = fw_upload->dd_handle;
+	u8 old_major, old_minor, old_build;
+	int rc, error = 0;

Forgot to mention this, but probably should make it:

int rc;
int error = 0;

best regards,
maxwell

#9 lore raw

Re: [RFC v1 6/6] crypto/ccp: Implement SNP firmware live update

In reply to Maxwell Doose
Quoted 32 lines · Maxwell Doose
Quoted 5 lines · Tycho Andersen

From: "Tycho Andersen (AMD)" <[email protected]>

Put all the previous primitives together to implement SNP firmware live update via DOWNLOAD_FIRMWARE_EX.

[snip]

Quoted 12 lines

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

---
 drivers/crypto/ccp/sev-dev.c | 244 ++++++++++++++++++++++++++++++++++-
 1 file changed, 243 insertions(+), 1 deletion(-)
drivers/crypto/ccp/sev-dev.c +0 -0
index b4711bf823e8..e7fe6dbf69c2 100644

drivers/crypto/ccp/sev-dev.c static enum fw_upload_err sev_fw_upload_prepare(struct fw_upload *fw_upload, · line 2055

Quoted 5 lines
drivers/crypto/ccp/sev-dev.c +5 -0
+static int sev_download_firmware_ex(struct sev_device *sev, const u8 *data,
+				    u32 size)
+{
+	struct sev_data_download_firmware_ex sev_data = {0};
+	int ret, error = 0, order;

Why not assign across multiple lines? How about something like:

int ret, order;
int error = 0;

Quoted 7 lines

or:

int ret;
int order;
int error = 0;

Would be better for readability and git blame.

Sure, I'll make the change here and in the other places you noted.

Quoted 9 lines
Quoted 5 lines

static enum fw_upload_err sev_fw_upload_write(struct fw_upload *fw_upload,

drivers/crypto/ccp/sev-dev.c +0 -0
 					      const u8 *data, u32 offset,
 					      u32 size, u32 *written)

{

[snip]

Quoted 1 line
Quoted 9 lines

drivers/crypto/ccp/sev-dev.c static enum fw_upload_err sev_fw_upload_prepare(struct fw_upload *fw_upload, · line 2175

Quoted 6 lines
drivers/crypto/ccp/sev-dev.c +5 -0
+	old_major = sev->api_major;
+	old_minor = sev->api_minor;
+	old_build = sev->build;
+
+	mutex_lock(&sev_cmd_mutex);

Why not guard(mutex)()? You used it earlier in
sev_firmware_reinit_if_shutdown().

Because this code calls some functions, including sev_firmware_reinit_if_shutdown(), that take the lock again. We could use scoped_guard() I suppose, I can look at that for v2.

It may be useful to do a larger series where we re-think when the locks are acquired here. It seems like only grabbing them at the top level entrypoints: ioctl, platform init, firmware update, etc. and putting lockdep asserts in all the helpers in the file might be cleaner generally.

Tycho

#10 lore raw

Re: [RFC v1 6/6] crypto/ccp: Implement SNP firmware live update

Quoted 2 lines · Tycho Andersen
Quoted 1 line · Maxwell Doose

On Thu Apr 30, 2026 at 11:07 AM CDT, Tycho Andersen wrote:

[snip]

Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
drivers/crypto/ccp/sev-dev.c +5 -0
+   old_major = sev->api_major;
+   old_minor = sev->api_minor;
+   old_build = sev->build;
+
+   mutex_lock(&sev_cmd_mutex);
Quoted 7 lines
Quoted 2 lines

Why not guard(mutex)()? You used it earlier in
sev_firmware_reinit_if_shutdown().

Because this code calls some functions, including sev_firmware_reinit_if_shutdown(), that take the lock again. We could use scoped_guard() I suppose, I can look at that for v2.

Sorry, I should've clarified in my email, I meant why not use the RAII patterns in cleanup.h like before. Like you said, scoped_guard() is likely the logical way forward if there's some functions that acquire the same lock.

Quoted 5 lines

It may be useful to do a larger series where we re-think when the locks are acquired here. It seems like only grabbing them at the top level entrypoints: ioctl, platform init, firmware update, etc. and putting lockdep asserts in all the helpers in the file might be cleaner generally.

That's also probably a good idea.

best regards,
maxwell

Quoted 2 lines

Tycho

#11 lore raw

Re: [PATCH 6/6] crypto/ccp: Implement SNP firmware live update

In reply to Tycho Andersen
Quoted 18 lines · Tycho Andersen
(quoted diff) +18 -0
+static int sev_firmware_shutdown_if_sev_initialized(struct sev_device *sev)
+{
+	int rc, error;
+	int sev_plat_state;
+
+	rc = sev_get_platform_state(&sev_plat_state, &error);
+	if (rc) {
+		if (error)
+			rc = error;
+		dev_dbg(sev->dev, "SEV get platform state failed %d\n", rc);
+		return rc;
+	}
+
+	switch (sev_plat_state) {
+	case SEV_STATE_UNINIT:
+		return 0;
+	case SEV_STATE_INIT:
+		error = 0;

Review

Quoted 10 lines
(quoted diff) +10 -0
+		rc = __sev_platform_shutdown_locked(&error);
+		if (rc) {
+			if (error)
+				rc = error;
+			dev_err(sev->dev, "SEV platform shutdown failed %d\n", rc);
+			return rc;
+		}
+
+		sev_firmware_needs_reinit = true;
+		return 0;

Tested this on Milan and hit a failure during SEV re-init after firmware update.

__sev_platform_shutdown_locked() sets the FW platform state to UNINIT, but sev_es_tmr and sev_init_ex_buffer remain firmware-owned in the RMP. When the new firmware comes up, SEV_CMD_INIT_EX rejects the stale page state and fails with SEV_RET_INVALID_PAGE_STATE (0x1A).

Tearing down the buffers on shutdown fixes it for us:

drivers/crypto/ccp/sev-dev.c +0 -0
@@ -1905,6 +1905,20 @@ static int sev_firmware_shutdown_if_sev_initialized(struct sev_device *sev)
	}

+ if (sev_es_tmr) {
+ wbinvd_on_all_cpus();
+ __snp_free_firmware_pages(virt_to_page(sev_es_tmr),
+ get_order(sev_es_tmr_size),
+ true);
+ sev_es_tmr = NULL;
+ }
+
+ if (sev_init_ex_buffer) {
+ __snp_free_firmware_pages(virt_to_page(sev_init_ex_buffer),
+ get_order(NV_LENGTH),
+ true);
+ sev_init_ex_buffer = NULL;
+ }
+

	sev_firmware_needs_reinit = true;
	return 0;

(Could also pull this and the cleanup in __sev_firmware_shutdown() into a shared helper. This logic is duplicated there.)

Review

Quoted 13 lines
	if (ret == FW_UPLOAD_ERR_NONE) {
		error = 0;

		rc = sev_get_api_version();
		if (rc) {
			if (error)
				rc = error;
			dev_err(sev->dev, "SEV query api version failed %d\n", rc);
		}
	}
(quoted diff) +2 -0
+	if (!dlfwex_wants_rollback)
+		sev_firmware_reinit_if_shutdown(sev);

If rc == SEV_RET_HWSEV_RET_UNSAFE, psp_dead is true but dlfwex_wants_rollback is false, so it still falls through to re-init. Even though it is recommended that the host should be rebooted after HARDWARE_UNSAFE, reboot tooling can lag and we may want some buffer. Plus the kernel shouldn't be touching a dead PSP anyway. Checking !psp_dead is a simple safeguard against extra churn while awaiting restart:

+ if (!dlfwex_wants_rollback && !psp_dead)

		sev_firmware_reinit_if_shutdown(sev);

I also moved the call to sev_firmware_reinit_if_shutdown() ahead of sev_get_api_version() so sev->state is already restored to INIT before sev status is queried. This is just to avoid unnecessary transient states. Curious if that logic makes sense to you. Am I missing something that made the original ordering strictly necessary?

Best,
Shantanu

#12 lore raw

Re: [PATCH 6/6] crypto/ccp: Implement SNP firmware live update

Hi Shantanu,

Thanks for the review. Tycho is out for the next few weeks so picking on this in the meantime.

Quoted 64 lines · Shantanu Sinha

[You don't often get email from [email protected]. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]

Quoted 18 lines · Tycho Andersen
(quoted diff) +18 -0
+static int sev_firmware_shutdown_if_sev_initialized(struct sev_device *sev)
+{
+     int rc, error;
+     int sev_plat_state;
+
+     rc = sev_get_platform_state(&sev_plat_state, &error);
+     if (rc) {
+             if (error)
+                     rc = error;
+             dev_dbg(sev->dev, "SEV get platform state failed %d\n", rc);
+             return rc;
+     }
+
+     switch (sev_plat_state) {
+     case SEV_STATE_UNINIT:
+             return 0;
+     case SEV_STATE_INIT:
+             error = 0;

Review

Quoted 10 lines
(quoted diff) +10 -0
+             rc = __sev_platform_shutdown_locked(&error);
+             if (rc) {
+                     if (error)
+                             rc = error;
+                     dev_err(sev->dev, "SEV platform shutdown failed %d\n", rc);
+                     return rc;
+             }
+
+             sev_firmware_needs_reinit = true;
+             return 0;

Tested this on Milan and hit a failure during SEV re-init after firmware update.

__sev_platform_shutdown_locked() sets the FW platform state to UNINIT, but sev_es_tmr and sev_init_ex_buffer remain firmware-owned in the RMP. When the new firmware comes up, SEV_CMD_INIT_EX rejects the stale page state and fails with SEV_RET_INVALID_PAGE_STATE (0x1A).

Tearing down the buffers on shutdown fixes it for us:

drivers/crypto/ccp/sev-dev.c +15 -0
@@ -1905,6 +1905,20 @@ static int sev_firmware_shutdown_if_sev_initialized(struct sev_device *sev)
        }
 
+       if (sev_es_tmr) {
+               wbinvd_on_all_cpus();
+               __snp_free_firmware_pages(virt_to_page(sev_es_tmr),
+                                         get_order(sev_es_tmr_size),
+                                         true);
+               sev_es_tmr = NULL;
+       }
+
+       if (sev_init_ex_buffer) {
+               __snp_free_firmware_pages(virt_to_page(sev_init_ex_buffer),
+                                         get_order(NV_LENGTH),
+                                         true);
+               sev_init_ex_buffer = NULL;
+       }
+
        sev_firmware_needs_reinit = true;
        return 0;
 
Quoted 2 lines

(Could also pull this and the cleanup in __sev_firmware_shutdown() into a shared helper. This logic is duplicated there.)

Releasing the TMR and sev_init_ex_buffer pages makes sense here.

Quoted 22 lines
Quoted 12 lines
      if (ret == FW_UPLOAD_ERR_NONE) {
              error = 0;

              rc = sev_get_api_version();
              if (rc) {
                      if (error)
                              rc = error;
                      dev_err(sev->dev, "SEV query api version failed %d\n", rc);
              }
      }

Review

Quoted 2 lines
(quoted diff) +2 -0
+     if (!dlfwex_wants_rollback)
+             sev_firmware_reinit_if_shutdown(sev);

If rc == SEV_RET_HWSEV_RET_UNSAFE, psp_dead is true but dlfwex_wants_rollback is false, so it still falls through to re-init. Even though it is recommended that the host should be rebooted after HARDWARE_UNSAFE, reboot tooling can lag and we may want some buffer. Plus the kernel shouldn't be touching a dead PSP anyway. Checking !psp_dead is a simple safeguard against extra churn while awaiting restart:

Quoted 2 lines

+ if (!dlfwex_wants_rollback && !psp_dead)

                sev_firmware_reinit_if_shutdown(sev);

Agreed. Matters more with the teardown above. init re-marks the TMR private before failing, stranding 2MB.

Quoted 7 lines

I also moved the call to sev_firmware_reinit_if_shutdown() ahead of sev_get_api_version() so sev->state is already restored to INIT before sev status is queried. This is just to avoid unnecessary transient states. Curious if that logic makes sense to you. Am I missing something that made the original ordering strictly necessary?

Having an ordering isn't particularly necessary from what I can tell. It's arguably better than what we had where early-returns on the cached state, so re-initing first keys off the driver's own record of having shut down rather than off whatever the new firmware reports.

The only issue I see is that it reports a stale API version. Refreshing the platform status within __sev_platform_init_locked() should do the trick.

Thanks,
--Pratik