Implement SNP live firmware update support

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

Once SNP is initialized the firmware requires the output page of both commands to be firmware-owned. sev->snp_plat_status cannot satisfy that as it is embedded in struct sev_device, so use __sev_do_snp_platform_status(), which stages the output through a dedicated page. Allocate the SNP_FEATURE_INFO output page with __snp_alloc_firmware_pages(), which transitions the page to firmware-owned when SNP is initialized, and release it with __snp_free_firmware_pages(), which reclaims it before freeing.

Typically this was only ever exercised during initialization. However DOWNLOAD_FIRMWARE_EX can change that. Live firmware update can race with IOCTLs such as SEV_FACTORY_RESET which could cause operations performed on incorrect cached platform states. Avoid this race by wrapping the function under the sev_cmd_mutex.

Co-developed-by Tycho Andersen (AMD) <[email protected]>

Signed-off-by Tycho Andersen (AMD) <[email protected]>

Signed-off-by Pratik R. Sampat <[email protected]>

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

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

index 1ed9e61a95cc..c40401c861d3 100644
@@ -131,6 +131,8 @@ 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,

@@ -1261,19 +1263,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;
+	lockdep_assert_held(&sev_cmd_mutex);
 
-	buf.address = __psp_pa(&sev->snp_plat_status);
-	rc = sev_do_cmd(SEV_CMD_SNP_PLATFORM_STATUS, &buf, error);
+	rc = __sev_do_snp_platform_status(&sev->snp_plat_status, error);
 	if (rc) {
 		dev_err(sev->dev, "SNP PLATFORM_STATUS command failed, ret = %d, error = %#x\n",
 			rc, *error);

@@ -1297,23 +1292,23 @@ static int snp_get_platform_data(struct sev_device *sev, int *error)
 	 * command to ensure structure is 8-byte aligned, and does not

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

 	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);
+	rc = __sev_do_cmd_locked(SEV_CMD_SNP_FEATURE_INFO, &snp_feat_info, error);
 	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(page);
+	__snp_free_firmware_pages(page, 0, true);
 
 	return rc;
 }

@@ -2097,6 +2092,8 @@ static int sev_get_api_version(void)
 	struct sev_user_data_status status;
 	int error = 0, ret;
 
+	guard(mutex)(&sev_cmd_mutex);
+
 	/*
 	 * Cache SNP platform status and SNP feature information
 	 * if SNP is available.

@@ -2107,7 +2104,7 @@ static int sev_get_api_version(void)
 			return 1;
 	}
 
-	ret = sev_platform_status(&status, &error);
+	ret = __sev_do_cmd_locked(SEV_CMD_PLATFORM_STATUS, &status, &error);
 	if (ret) {
 		dev_err(sev->dev,
 			"SEV: failed to get status. Error: %#x\n", error);

Quoted 20 lines · Pratik R. Sampat

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

Once SNP is initialized the firmware requires the output page of both commands to be firmware-owned. sev->snp_plat_status cannot satisfy that as it is embedded in struct sev_device, so use __sev_do_snp_platform_status(), which stages the output through a dedicated page. Allocate the SNP_FEATURE_INFO output page with __snp_alloc_firmware_pages(), which transitions the page to firmware-owned when SNP is initialized, and release it with __snp_free_firmware_pages(), which reclaims it before freeing.

Typically this was only ever exercised during initialization. However DOWNLOAD_FIRMWARE_EX can change that. Live firmware update can race with IOCTLs such as SEV_FACTORY_RESET which could cause operations performed on incorrect cached platform states. Avoid this race by wrapping the function under the sev_cmd_mutex.

Quoted 3 lines

Co-developed-by Tycho Andersen (AMD) <[email protected]>

Signed-off-by Tycho Andersen (AMD) <[email protected]>

Signed-off-by Pratik R. Sampat <[email protected]>

Just a minor comment below.

Reviewed-by Tom Lendacky <[email protected]>

Quoted 50 lines

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

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

index 1ed9e61a95cc..c40401c861d3 100644
@@ -131,6 +131,8 @@ 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,

@@ -1261,19 +1263,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;
+	lockdep_assert_held(&sev_cmd_mutex);
 
-	buf.address = __psp_pa(&sev->snp_plat_status);
-	rc = sev_do_cmd(SEV_CMD_SNP_PLATFORM_STATUS, &buf, error);
+	rc = __sev_do_snp_platform_status(&sev->snp_plat_status, error);
 	if (rc) {
 		dev_err(sev->dev, "SNP PLATFORM_STATUS command failed, ret = %d, error = %#x\n",
 			rc, *error);

@@ -1297,23 +1292,23 @@ static int snp_get_platform_data(struct sev_device *sev, int *error)
 	 * command to ensure structure is 8-byte aligned, and does not
 	 * cross a page boundary.
 	 */
-	page = alloc_page(GFP_KERNEL);
-	if (!page)
+	page = __snp_alloc_firmware_pages(GFP_KERNEL, 0, true);
+	feat_info =  page ? page_address(page) : NULL;
+	if (!feat_info)
 		return -ENOMEM;
 

drivers/crypto/ccp/sev-dev.c static int snp_get_platform_data(struct sev_device *sev, int *error) · line 1304

Quoted 1 line

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

-	feat_info = page_address(page);

This is a bit of churn when all you have to do is replace alloc_page() with __snp_alloc_firmware_pages(), right?

Thanks,
Tom

Quoted 35 lines

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

 	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);
+	rc = __sev_do_cmd_locked(SEV_CMD_SNP_FEATURE_INFO, &snp_feat_info, error);
 	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(page);
+	__snp_free_firmware_pages(page, 0, true);
 
 	return rc;
 }

@@ -2097,6 +2092,8 @@ static int sev_get_api_version(void)
 	struct sev_user_data_status status;
 	int error = 0, ret;
 
+	guard(mutex)(&sev_cmd_mutex);
+
 	/*
 	 * Cache SNP platform status and SNP feature information
 	 * if SNP is available.

@@ -2107,7 +2104,7 @@ static int sev_get_api_version(void)
 			return 1;
 	}
 
-	ret = sev_platform_status(&status, &error);
+	ret = __sev_do_cmd_locked(SEV_CMD_PLATFORM_STATUS, &status, &error);
 	if (ret) {
 		dev_err(sev->dev,
 			"SEV: failed to get status. Error: %#x\n", error);

+#ifdef CONFIG_FW_UPLOAD

Quoted 1 line · Pratik R. Sampat

+#ifdef CONFIG_FW_UPLOAD

[...]

drivers/crypto/ccp/sev-dev.c static int sev_update_firmware(struct device *dev) · line 2287

Quoted 4 lines

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

+#else	/* CONFIG_FW_UPLOAD */
+static void register_sev_fw_uploader(struct sev_device *sev) { }
+static void unregister_sev_fw_uploader(struct sev_device *sev) { }
+#endif	/* CONFIG_FW_UPLOAD */

There's no Kconfig change in this patch. Should CRYPTO_DEV_SP_PSP select FW_UPLOAD? It defaults to N, and the other fw_upload users select it.

In preparation for SEV-SNP DOWNLOAD_FIRMWARE_EX live firmware update support, add an 'sev' firmware loader that always fails with EBUSY.

Co-developed-by Tycho Andersen (AMD) <[email protected]>

Signed-off-by Tycho Andersen (AMD) <[email protected]>

Signed-off-by Pratik R. Sampat <[email protected]>

---
 drivers/crypto/ccp/sev-dev.c | 80 ++++++++++++++++++++++++++++++++++++
 drivers/crypto/ccp/sev-dev.h |  2 +
 2 files changed, 82 insertions(+)

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

index c40401c861d3..88cf60a9640e 100644
@@ -2212,6 +2212,80 @@ static int sev_update_firmware(struct device *dev)
 	return ret;
 }
 
+#ifdef CONFIG_FW_UPLOAD
+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,
+};
+
+/* Serializes registration and unregistration of sev->fwl */
+static DEFINE_MUTEX(sev_fwl_lock);
+
+static void register_sev_fw_uploader(struct sev_device *sev)
+{
+	struct fw_upload *fwl;
+
+	guard(mutex)(&sev_fwl_lock);
+

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

+		return;
+	}
+
+	sev->fwl = fwl;
+}
+
+static void unregister_sev_fw_uploader(struct sev_device *sev)
+{
+	guard(mutex)(&sev_fwl_lock);
+
+	if (!sev->fwl)
+		return;
+
+	firmware_upload_unregister(sev->fwl);
+	sev->fwl = NULL;
+}
+#else	/* CONFIG_FW_UPLOAD */
+static void register_sev_fw_uploader(struct sev_device *sev) { }
+static void unregister_sev_fw_uploader(struct sev_device *sev) { }
+#endif	/* CONFIG_FW_UPLOAD */
+
 static int __sev_snp_shutdown_locked(int *error, bool panic)
 {
 	struct psp_device *psp = psp_master;

@@ -3018,6 +3092,8 @@ void sev_dev_destroy(struct psp_device *psp)
 	if (!sev)
 		return;
 
+	unregister_sev_fw_uploader(sev);
+
 	sev_firmware_shutdown(sev);
 
 	if (sev->misc)

@@ -3081,6 +3157,8 @@ 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:

@@ -3096,6 +3174,8 @@ void sev_pci_exit(void)
 	if (!sev)
 		return;
 
+	unregister_sev_fw_uploader(sev);
+
 	sev_firmware_shutdown(sev);
 }
 

drivers/crypto/ccp/sev-dev.h +2 -0

index d5e596606def..7ec692e2147e 100644
@@ -69,6 +69,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);

Implement SNP live firmware update using the DOWNLOAD_FIRMWARE_EX command.

DOWNLOAD_FIRMWARE_EX requires the legacy SEV platform to be UNINIT. If it is WORKING then legacy guests are running and the update is refused as busy. If it is INIT, shut it down, release the buffers the firmware owns across that shutdown, run the update, and bring the platform back up afterwards. SNP is never taken down, so SNP guests are unaffected.

To test run the following with your sbin file in FW:

echo 1 > /sys/class/firmware/sev/loading cat <firmware.sbin> > /sys/class/firmware/sev/data echo 0 > /sys/class/firmware/sev/loading

The COMMIT bit is left clear, so the image is only loaded provisionally and the admin decides when to make it permanent with ioctl(/dev/sev, SNP_COMMIT). To roll back, do not commit and upload the previous image the same way.

Co-developed-by Tycho Andersen (AMD) <[email protected]>

Signed-off-by Tycho Andersen (AMD) <[email protected]>

Signed-off-by Pratik R. Sampat <[email protected]>

---
 drivers/crypto/ccp/sev-dev.c | 284 ++++++++++++++++++++++++++++++++++-
 drivers/crypto/ccp/sev-dev.h |   2 +
 include/linux/psp-sev.h      |  19 +++
 3 files changed, 304 insertions(+), 1 deletion(-)

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

index 88cf60a9640e..e54f23ba1b9b 100644
@@ -29,6 +29,7 @@
 #include <linux/psp.h>
 #include <linux/amd-iommu.h>
 #include <linux/crash_dump.h>
+#include <linux/sizes.h>
 
 #include <asm/smp.h>
 #include <asm/cacheflush.h>

@@ -252,6 +253,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);

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

+#define SEV_FW_IMAGE_MAX_SIZE	SZ_512K
+
 static enum fw_upload_err sev_fw_upload_prepare(struct fw_upload *fw_upload,
 						const u8 *data, u32 size)
 {
+	struct sev_device *sev = fw_upload->dd_handle;
+
+	if (size > SEV_FW_IMAGE_MAX_SIZE) {
+		dev_err(sev->dev, "DLFW_EX: image of %u bytes exceeds the %u byte maximum\n",
+			size, SEV_FW_IMAGE_MAX_SIZE);
+		return FW_UPLOAD_ERR_INVALID_SIZE;
+	}
+
 	return FW_UPLOAD_ERR_NONE;
 }
 
+static int sev_download_firmware_ex(const u8 *data, u32 size, int *psp_ret)
+{
+	struct sev_data_download_firmware_ex sev_data = {0};
+	int ret, order;
+	struct page *p;
+	void *fw_blob;
+
+	order = get_order(size);
+	p = alloc_pages(GFP_KERNEL | __GFP_ZERO, order);
+	if (!p)
+		return -ENOMEM;

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

+	if (psp_dead) {
+		dev_err(sev->dev, "DLFW_EX: PSP not responding (failed %d, error %#x)\n",
+			rc, psp_ret);
+		sev->fwl_rollback_required = false;
+
+		return FW_UPLOAD_ERR_TIMEOUT;
+	}
+
+	switch (psp_ret) {
+	case SEV_RET_INVALID_PARAM:
+		msg = "Provided image is not well formed";
+		break;
+	case SEV_RET_INVALID_LEN:
+		ret = FW_UPLOAD_ERR_INVALID_SIZE;
+		msg = "Provided image has an unusable length";
+		break;
+	case SEV_RET_SHUTDOWN_REQUIRED:
+		msg = "Provided image cannot be live-updated, shutdown required";
+		break;
+	case SEV_RET_BAD_VERSION:
+		msg = "Provided image < committed version";
+		break;
+	case SEV_RET_INVALID_PLATFORM_STATE:
+		msg = "Platform not in UNINIT state";
+		break;
+	case SEV_RET_INVALID_ADDRESS:
+		msg = "Unaligned address provided";
+		break;
+	case SEV_RET_UNSUPPORTED:
+		msg = "Feature not supported";
+		break;
+	case SEV_RET_INVALID_CONFIG:
+		msg = "Image rejected, unsupported configuration";
+		break;
+	case SEV_RET_BAD_SVN:
+		msg = "Image rejected, SVN < committed SVN";
+		break;
+	case SEV_RET_BAD_SIGNATURE:
+		msg = "Bad firmware signature";
+		break;
+	case SEV_RET_UPDATE_FAILED:
+		/* The previous firmware is still running, a retry is safe. */
+		ret = FW_UPLOAD_ERR_BUSY;
+		msg = "Upgrade failed, automatically reverted";
+		break;
+	case SEV_RET_RESTORE_REQUIRED:
+		/*
+		 * Firmware requested a roll-back. Declare the PSP dead so
+		 * nothing else tries to use it, and let the next upload through
+		 * so the admin can restore the previous image.
+		 */
+		sev->fwl_rollback_required = true;
+		psp_dead = true;
+		ret = FW_UPLOAD_ERR_RW_ERROR;
+		msg = "Live upgrade failed, please roll back";
+		break;
+	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. Declare the PSP dead so the driver stops
+		 * issuing commands to it while the reboot is pending.
+		 */
+		sev->fwl_rollback_required = false;
+		psp_dead = true;
+		ret = FW_UPLOAD_ERR_HW_ERROR;
+		msg = "SEV firmware no longer safe. Reboot recommended";
+		break;
+	case SEV_RET_NO_FW_CALL:
+		/* The command never reached the firmware. */

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

+
+	return ret;
+}
+
+static int sev_fw_upload_shutdown_platform(struct sev_device *sev)
+{
+	int rc, error = SEV_RET_NO_FW_CALL, sev_plat_state;
+
+	lockdep_assert_held(&sev_cmd_mutex);
+
+	rc = sev_get_platform_state(&sev_plat_state, &error);
+	if (rc) {
+		dev_err(sev->dev, "SEV get platform state failed %d, error %#x\n",
+			rc, error);
+		return rc;
+	}
+
+	switch (sev_plat_state) {
+	case SEV_STATE_UNINIT:
+		return 0;
+	case SEV_STATE_WORKING:
+		/* Legacy guests are running, the update cannot proceed. */
+		return -EBUSY;
+	case SEV_STATE_INIT:
+		break;
+	default:
+		dev_err(sev->dev, "Unknown SEV firmware state %d\n", sev_plat_state);
+		return -EINVAL;

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

+
+	sev->fwl_reinit_required = true;
+
+	return 0;
+}
+
+static void sev_fw_upload_reinit_platform(struct sev_device *sev)
+{
+	int rc, error = SEV_RET_NO_FW_CALL;

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

+		return;
+	}
+
+	sev->fwl_reinit_required = false;
+}
+
+static enum fw_upload_err sev_fw_upload_update(struct sev_device *sev,
+					       const u8 *data, u32 size,
+					       u32 *written)
+{
+	int rc, error = SEV_RET_NO_FW_CALL;

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

+	 * been shut down, so no need to do it again.
+	 */
+	if (sev->fwl_rollback_required) {
+		psp_dead = false;

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

+	if (ret == FW_UPLOAD_ERR_NONE) {
+		*written = size;
+		sev->fwl_rollback_required = false;
+	}
+
+	/* A rollback retry failed. PSP now stays dead */
+	if (sev->fwl_rollback_required) {
+		psp_dead = true;
+		if (ret != FW_UPLOAD_ERR_HW_ERROR)
+			ret = FW_UPLOAD_ERR_RW_ERROR;
+	}
+
+	if (!sev->fwl_rollback_required && !psp_dead)
+		sev_fw_upload_reinit_platform(sev);
+
+	return ret;
+}
+
 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;
+	enum fw_upload_err ret;
+
+	old_major = sev->api_major;
+	old_minor = sev->api_minor;
+	old_build = sev->build;
+
+	ret = sev_fw_upload_update(sev, data, size, written);

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

+			 sev->api_major, sev->api_minor, sev->build);
+	}
+
+	return ret;
 }
 
 static enum fw_upload_err sev_fw_upload_poll_complete(struct fw_upload *fw_upload)

drivers/crypto/ccp/sev-dev.h +2 -0

index 7ec692e2147e..1e45a08c41da 100644
@@ -71,6 +71,8 @@ struct sev_device {
 	struct sev_tio_status *tio_status;
 
 	struct fw_upload *fwl;
+	bool fwl_rollback_required;
+	bool fwl_reinit_required;
 };
 
 int sev_dev_init(struct psp_device *psp);

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

index fab62228f981..b71154e9ae4b 100644
@@ -890,6 +890,25 @@ struct sev_platform_init_args {
 	unsigned int max_snp_asid;
 };
 
+/**
+ * struct sev_data_download_firmware_ex - SNP_DOWNLOAD_FIRMWARE_EX structure
+ *
+ * @len: length of the command buffer read by the PSP
+ * @rsvd0: reserved
+ * @fw_paddr: system physical address of the start of the firmware blob
+ * @fw_len: length of the firmware blob
+ * @commit: whether to immediately commit the firmware update
+ * @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
  *

Quoted 52 lines · Pratik R. Sampat

Implement SNP live firmware update using the DOWNLOAD_FIRMWARE_EX command.

DOWNLOAD_FIRMWARE_EX requires the legacy SEV platform to be UNINIT. If it is WORKING then legacy guests are running and the update is refused as busy. If it is INIT, shut it down, release the buffers the firmware owns across that shutdown, run the update, and bring the platform back up afterwards. SNP is never taken down, so SNP guests are unaffected.

To test run the following with your sbin file in FW:

echo 1 > /sys/class/firmware/sev/loading cat <firmware.sbin> > /sys/class/firmware/sev/data echo 0 > /sys/class/firmware/sev/loading

The COMMIT bit is left clear, so the image is only loaded provisionally and the admin decides when to make it permanent with ioctl(/dev/sev, SNP_COMMIT). To roll back, do not commit and upload the previous image the same way.

Co-developed-by Tycho Andersen (AMD) <[email protected]>

Signed-off-by Tycho Andersen (AMD) <[email protected]>

Signed-off-by Pratik R. Sampat <[email protected]>

---
 drivers/crypto/ccp/sev-dev.c | 284 ++++++++++++++++++++++++++++++++++-
 drivers/crypto/ccp/sev-dev.h |   2 +
 include/linux/psp-sev.h      |  19 +++
 3 files changed, 304 insertions(+), 1 deletion(-)

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

index 88cf60a9640e..e54f23ba1b9b 100644
@@ -29,6 +29,7 @@
 #include <linux/psp.h>
 #include <linux/amd-iommu.h>
 #include <linux/crash_dump.h>
+#include <linux/sizes.h>
 
 #include <asm/smp.h>
 #include <asm/cacheflush.h>

@@ -252,6 +253,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);

@@ -2213,17 +2215,297 @@ static int sev_update_firmware(struct device *dev)
 }
 

drivers/crypto/ccp/sev-dev.c static int sev_update_firmware(struct device *dev) · line 2218

Quoted 2 lines

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

 #ifdef CONFIG_FW_UPLOAD
+/* Largest image the firmware accepts, anything above is rejected */

I may have missed it, but I don't see anything in the SNP ABI spec that says the limit is 512K. If that doesn't have a limit how did we arrive at 512K?

Quoted 32 lines

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

+#define SEV_FW_IMAGE_MAX_SIZE	SZ_512K
+
 static enum fw_upload_err sev_fw_upload_prepare(struct fw_upload *fw_upload,
 						const u8 *data, u32 size)
 {
+	struct sev_device *sev = fw_upload->dd_handle;
+
+	if (size > SEV_FW_IMAGE_MAX_SIZE) {
+		dev_err(sev->dev, "DLFW_EX: image of %u bytes exceeds the %u byte maximum\n",
+			size, SEV_FW_IMAGE_MAX_SIZE);
+		return FW_UPLOAD_ERR_INVALID_SIZE;
+	}
+
 	return FW_UPLOAD_ERR_NONE;
 }
 
+static int sev_download_firmware_ex(const u8 *data, u32 size, int *psp_ret)
+{
+	struct sev_data_download_firmware_ex sev_data = {0};
+	int ret, order;
+	struct page *p;
+	void *fw_blob;
+
+	order = get_order(size);
+	p = alloc_pages(GFP_KERNEL | __GFP_ZERO, order);
+	if (!p)
+		return -ENOMEM;
+
+	fw_blob = page_address(p);
+	memcpy(fw_blob, data, size);
+
+	sev_data.len = sizeof(sev_data);

drivers/crypto/ccp/sev-dev.c static int sev_update_firmware(struct device *dev) · line 2255

Quoted 5 lines

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

+	sev_data.fw_paddr = __psp_pa(fw_blob);
+	sev_data.fw_len = size;
+	/*
+	 * Don't commit to the new firmware immediately, perform an explicit
+	 * SNP_COMMIT after

Don't commit the new firmware, an explict SNP_COMMIT is required after update.

Quoted 4 lines

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

+	 */
+	sev_data.commit = 0;
+

drivers/crypto/ccp/sev-dev.c static int sev_update_firmware(struct device *dev) · line 2268

Quoted 10 lines

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

+	ret = __sev_do_cmd_locked(SEV_CMD_SNP_DOWNLOAD_FIRMWARE_EX, &sev_data,
+				  psp_ret);
+
+	__free_pages(p, order);
+
+	return ret;
+}
+
+static enum fw_upload_err sev_fw_upload_handle_err(struct sev_device *sev,
+						   int rc, int psp_ret)

Name rc something more specific, like cmd_ret, to better distinguish what you're checking.

drivers/crypto/ccp/sev-dev.c static int sev_update_firmware(struct device *dev) · line 2279

Quoted 12 lines

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

+{
+	enum fw_upload_err ret = FW_UPLOAD_ERR_FW_INVALID;
+	const char *msg;
+
+	if (!rc)
+		return FW_UPLOAD_ERR_NONE;
+
+	/*
+	 * The command timed out: psp_ret was cleared and the PSP was declared
+	 * dead, so there is no firmware status to decode.
+	 */

Move this comment into the if block as it is explaining what happened if psp_dead is set and simplify it:

"The SEV command timed out and marked the ASP dead, there is no status to decode."

Quoted 76 lines

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

+	if (psp_dead) {
+		dev_err(sev->dev, "DLFW_EX: PSP not responding (failed %d, error %#x)\n",
+			rc, psp_ret);
+		sev->fwl_rollback_required = false;
+
+		return FW_UPLOAD_ERR_TIMEOUT;
+	}
+
+	switch (psp_ret) {
+	case SEV_RET_INVALID_PARAM:
+		msg = "Provided image is not well formed";
+		break;
+	case SEV_RET_INVALID_LEN:
+		ret = FW_UPLOAD_ERR_INVALID_SIZE;
+		msg = "Provided image has an unusable length";
+		break;
+	case SEV_RET_SHUTDOWN_REQUIRED:
+		msg = "Provided image cannot be live-updated, shutdown required";
+		break;
+	case SEV_RET_BAD_VERSION:
+		msg = "Provided image < committed version";
+		break;
+	case SEV_RET_INVALID_PLATFORM_STATE:
+		msg = "Platform not in UNINIT state";
+		break;
+	case SEV_RET_INVALID_ADDRESS:
+		msg = "Unaligned address provided";
+		break;
+	case SEV_RET_UNSUPPORTED:
+		msg = "Feature not supported";
+		break;
+	case SEV_RET_INVALID_CONFIG:
+		msg = "Image rejected, unsupported configuration";
+		break;
+	case SEV_RET_BAD_SVN:
+		msg = "Image rejected, SVN < committed SVN";
+		break;
+	case SEV_RET_BAD_SIGNATURE:
+		msg = "Bad firmware signature";
+		break;
+	case SEV_RET_UPDATE_FAILED:
+		/* The previous firmware is still running, a retry is safe. */
+		ret = FW_UPLOAD_ERR_BUSY;
+		msg = "Upgrade failed, automatically reverted";
+		break;
+	case SEV_RET_RESTORE_REQUIRED:
+		/*
+		 * Firmware requested a roll-back. Declare the PSP dead so
+		 * nothing else tries to use it, and let the next upload through
+		 * so the admin can restore the previous image.
+		 */
+		sev->fwl_rollback_required = true;
+		psp_dead = true;
+		ret = FW_UPLOAD_ERR_RW_ERROR;
+		msg = "Live upgrade failed, please roll back";
+		break;
+	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. Declare the PSP dead so the driver stops
+		 * issuing commands to it while the reboot is pending.
+		 */
+		sev->fwl_rollback_required = false;
+		psp_dead = true;
+		ret = FW_UPLOAD_ERR_HW_ERROR;
+		msg = "SEV firmware no longer safe. Reboot recommended";
+		break;
+	case SEV_RET_NO_FW_CALL:
+		/* The command never reached the firmware. */
+		ret = FW_UPLOAD_ERR_BUSY;
+		msg = "Driver error";
+		break;
+	default:
+		ret = FW_UPLOAD_ERR_HW_ERROR;

drivers/crypto/ccp/sev-dev.c static int sev_update_firmware(struct device *dev) · line 2359

Quoted 5 lines

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

+		msg = "Unknown SEV firmware error";
+		break;
+	}
+
+	dev_err(sev->dev, "DLFW_EX: %s (failed %d, error %#x)\n", msg, rc, psp_ret);

This is coming from userspace interaction, so probably should use ratelimited variant (here and any place you issue a message).

Quoted 34 lines

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

+
+	return ret;
+}
+
+static int sev_fw_upload_shutdown_platform(struct sev_device *sev)
+{
+	int rc, error = SEV_RET_NO_FW_CALL, sev_plat_state;
+
+	lockdep_assert_held(&sev_cmd_mutex);
+
+	rc = sev_get_platform_state(&sev_plat_state, &error);
+	if (rc) {
+		dev_err(sev->dev, "SEV get platform state failed %d, error %#x\n",
+			rc, error);
+		return rc;
+	}
+
+	switch (sev_plat_state) {
+	case SEV_STATE_UNINIT:
+		return 0;
+	case SEV_STATE_WORKING:
+		/* Legacy guests are running, the update cannot proceed. */
+		return -EBUSY;
+	case SEV_STATE_INIT:
+		break;
+	default:
+		dev_err(sev->dev, "Unknown SEV firmware state %d\n", sev_plat_state);
+		return -EINVAL;
+	}
+
+	rc = __sev_platform_shutdown_locked(&error);
+	if (rc) {
+		dev_err(sev->dev, "SEV platform shutdown failed %d, error %#x\n",
+			rc, error);

drivers/crypto/ccp/sev-dev.c static int sev_update_firmware(struct device *dev) · line 2397

Quoted 4 lines

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

+		return rc;
+	}
+
+	__sev_release_firmware_buffers(false);

Do the buffers have to be released? If so, why? I think you can keep the allocations. During platform initialization the buffers will be detected. Is there a shutdown path where they might not get freed?

Quoted 13 lines

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

+
+	sev->fwl_reinit_required = true;
+
+	return 0;
+}
+
+static void sev_fw_upload_reinit_platform(struct sev_device *sev)
+{
+	int rc, error = SEV_RET_NO_FW_CALL;
+
+	lockdep_assert_held(&sev_cmd_mutex);
+
+	if (!sev->fwl_reinit_required)

drivers/crypto/ccp/sev-dev.c static int sev_update_firmware(struct device *dev) · line 2416

Quoted 6 lines

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

+		return;
+
+	rc = __sev_platform_init_locked(&error);
+	if (rc) {
+		dev_err(sev->dev, "SEV platform re-init failed %d, error %#x\n",
+			rc, error);

Single line.

Quoted 12 lines

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

+		return;
+	}
+
+	sev->fwl_reinit_required = false;
+}
+
+static enum fw_upload_err sev_fw_upload_update(struct sev_device *sev,
+					       const u8 *data, u32 size,
+					       u32 *written)
+{
+	int rc, error = SEV_RET_NO_FW_CALL;

drivers/crypto/ccp/sev-dev.c static int sev_update_firmware(struct device *dev) · line 2437

Quoted 10 lines

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

+	enum fw_upload_err ret;
+
+	guard(mutex)(&sev_cmd_mutex);
+
+	/*
+	 * A PSP declared dead only executes DOWNLOAD_FIRMWARE_EX if it was the
+	 * firmware update that killed it and asked for a rollback. Declared
+	 * dead for any other reason it will not answer until the platform is
+	 * rebooted.
+	 */

"SEV firmware will only successfully execute the DOWNLOAD_FIRMWARE_EX command if a firmware rollback is required. Other commands may be processed, but may not execute properly. Use the psp_dead boolean to restrict execution to this path."

Say something similar where psp_dead is being set to true in sev_fw_upload_handle_err().

drivers/crypto/ccp/sev-dev.c static int sev_update_firmware(struct device *dev) · line 2444

Quoted 8 lines

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

+	if (psp_dead && !sev->fwl_rollback_required) {
+		dev_err(sev->dev, "DLFW_EX: PSP is not responding\n");
+		return FW_UPLOAD_ERR_HW_ERROR;
+	}
+
+	/*
+	 * If the last firmware update returned RESTORE_REQUIRED, retry DLFW_EX.

I see a mix of DOWNLOAD_FIRMWARE_EX and DLFW_EX, change these to all be the same name of your choice.

drivers/crypto/ccp/sev-dev.c static int sev_update_firmware(struct device *dev) · line 2445

Quoted 2 lines

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

+	 * We being in this state means that the legacy firmware has previously

s/We being/Being/

Quoted 5 lines

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

+	 * been shut down, so no need to do it again.
+	 */
+	if (sev->fwl_rollback_required) {
+		psp_dead = false;

drivers/crypto/ccp/sev-dev.c static int sev_update_firmware(struct device *dev) · line 2459

Quoted 10 lines

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

+	} else {
+		rc = sev_fw_upload_shutdown_platform(sev);
+		if (rc) {
+			return psp_dead ? FW_UPLOAD_ERR_HW_ERROR
+					: FW_UPLOAD_ERR_BUSY;
+		}
+	}
+
+	rc = sev_download_firmware_ex(data, size, &error);
+	ret = sev_fw_upload_handle_err(sev, rc, error);

Maybe it's just me, but using generic rc and ret can make this possibly confusing in the future. How about:

s/rc/cmd_ret/
s/ret/fwl_ret/
s/error/psp_ret/

Quoted 36 lines

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

+	if (ret == FW_UPLOAD_ERR_NONE) {
+		*written = size;
+		sev->fwl_rollback_required = false;
+	}
+
+	/* A rollback retry failed. PSP now stays dead */
+	if (sev->fwl_rollback_required) {
+		psp_dead = true;
+		if (ret != FW_UPLOAD_ERR_HW_ERROR)
+			ret = FW_UPLOAD_ERR_RW_ERROR;
+	}
+
+	if (!sev->fwl_rollback_required && !psp_dead)
+		sev_fw_upload_reinit_platform(sev);
+
+	return ret;
+}
+
 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;
+	enum fw_upload_err ret;
+
+	old_major = sev->api_major;
+	old_minor = sev->api_minor;
+	old_build = sev->build;
+
+	ret = sev_fw_upload_update(sev, data, size, written);
+	if (ret != FW_UPLOAD_ERR_NONE)
+		return ret;
+
+	if (sev_get_api_version()) {

drivers/crypto/ccp/sev-dev.c static int sev_update_firmware(struct device *dev) · line 2500

Quoted 6 lines

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

+		dev_err(sev->dev, "SNP platform data refresh after firmware update failed\n");
+		return FW_UPLOAD_ERR_HW_ERROR;
+	}
+
+	if (sev->api_major != old_major || sev->api_minor != old_minor ||
+	    sev->build != old_build) {

One line.

drivers/crypto/ccp/sev-dev.c static int sev_update_firmware(struct device *dev) · line 2501

Quoted 2 lines

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

+		dev_info(sev->dev, "SEV firmware updated to %d.%d build %d\n",

Should be the same as the sev_pci_init() issued message.

drivers/crypto/ccp/sev-dev.c static int sev_update_firmware(struct device *dev) · line 2504

Quoted 3 lines

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

+			 sev->api_major, sev->api_minor, sev->build);
+	} else {
+		dev_info(sev->dev, "SEV firmware version unchanged: %d.%d build %d\n",

s/ build /./

Thanks,
Tom

Quoted 50 lines

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

+			 sev->api_major, sev->api_minor, sev->build);
+	}
+
+	return ret;
 }
 
 static enum fw_upload_err sev_fw_upload_poll_complete(struct fw_upload *fw_upload)

drivers/crypto/ccp/sev-dev.h +2 -0

index 7ec692e2147e..1e45a08c41da 100644
@@ -71,6 +71,8 @@ struct sev_device {
 	struct sev_tio_status *tio_status;
 
 	struct fw_upload *fwl;
+	bool fwl_rollback_required;
+	bool fwl_reinit_required;
 };
 
 int sev_dev_init(struct psp_device *psp);

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

index fab62228f981..b71154e9ae4b 100644
@@ -890,6 +890,25 @@ struct sev_platform_init_args {
 	unsigned int max_snp_asid;
 };
 
+/**
+ * struct sev_data_download_firmware_ex - SNP_DOWNLOAD_FIRMWARE_EX structure
+ *
+ * @len: length of the command buffer read by the PSP
+ * @rsvd0: reserved
+ * @fw_paddr: system physical address of the start of the firmware blob
+ * @fw_len: length of the firmware blob
+ * @commit: whether to immediately commit the firmware update
+ * @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
  *

Hi Tom,

Thanks for the review!

Quoted 25 lines · Tom Lendacky

Quoted 23 lines · Pratik R. Sampat

Implement SNP live firmware update using the DOWNLOAD_FIRMWARE_EX command.

DOWNLOAD_FIRMWARE_EX requires the legacy SEV platform to be UNINIT. If it is WORKING then legacy guests are running and the update is refused as busy. If it is INIT, shut it down, release the buffers the firmware owns across that shutdown, run the update, and bring the platform back up afterwards. SNP is never taken down, so SNP guests are unaffected.

To test run the following with your sbin file in FW:

echo 1 > /sys/class/firmware/sev/loading cat <firmware.sbin> > /sys/class/firmware/sev/data echo 0 > /sys/class/firmware/sev/loading

The COMMIT bit is left clear, so the image is only loaded provisionally and the admin decides when to make it permanent with ioctl(/dev/sev, SNP_COMMIT). To roll back, do not commit and upload the previous image the same way.

Co-developed-by Tycho Andersen (AMD) <[email protected]>

Signed-off-by Tycho Andersen (AMD) <[email protected]>

Signed-off-by Pratik R. Sampat <[email protected]>

Quoted 7 lines

Quoted 2 lines

#ifdef CONFIG_FW_UPLOAD
+/* Largest image the firmware accepts, anything above is rejected */

I may have missed it, but I don't see anything in the SNP ABI spec that says the limit is 512K. If that doesn't have a limit how did we arrive at 512K?

The ABI spec doesn't mention it, but the firmware had this limit hard-coded. This can potentially change without notice to the ABI. However, I did want a sanity check in the OS and that's why had it in.

I can drop it and let the firmware fail if the image is too large.

Quoted 11 lines

drivers/crypto/ccp/sev-dev.c static int sev_update_firmware(struct device *dev)

Quoted 6 lines

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

+#define SEV_FW_IMAGE_MAX_SIZE	SZ_512K
+
 
+
+	__sev_release_firmware_buffers(false);

Do the buffers have to be released? If so, why? I think you can keep the allocations. During platform initialization the buffers will be detected. Is there a shutdown path where they might not get freed?

Shantanu hit this on Milan with an earlier version of the series that kept the buffers across the update [1]. After DOWNLOAD_FIRMWARE_EX the new firmware rejected INIT_EX with SEV_RET_INVALID_PAGE_STATE (0x1A), because sev_es_tmr and sev_init_ex_buffer were still in the firmware state left over from the previous image's INIT. Freeing them, and letting re-init allocate fresh ones, fixed it, which is where this call came from.

However, a fresh allocation with SNP initialized just goes through rmp_mark_pages_firmware(), so my understanding of what the new firmware needs is the reclaim -> make shared -> mark firmware cycle, not necessarily new memory.

Freeing seemed like the easiest option, but if you prefer, just cycling them back should be able to achieve the same effect, I believe.

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

Quoted 42 lines · Pratik R. Sampat

Hi Tom,

Thanks for the review!

Quoted 25 lines · Tom Lendacky

Quoted 23 lines · Pratik R. Sampat

Implement SNP live firmware update using the DOWNLOAD_FIRMWARE_EX command.

DOWNLOAD_FIRMWARE_EX requires the legacy SEV platform to be UNINIT. If it is WORKING then legacy guests are running and the update is refused as busy. If it is INIT, shut it down, release the buffers the firmware owns across that shutdown, run the update, and bring the platform back up afterwards. SNP is never taken down, so SNP guests are unaffected.

To test run the following with your sbin file in FW:

echo 1 > /sys/class/firmware/sev/loading cat <firmware.sbin> > /sys/class/firmware/sev/data echo 0 > /sys/class/firmware/sev/loading

The COMMIT bit is left clear, so the image is only loaded provisionally and the admin decides when to make it permanent with ioctl(/dev/sev, SNP_COMMIT). To roll back, do not commit and upload the previous image the same way.

Co-developed-by Tycho Andersen (AMD) <[email protected]>

Signed-off-by Tycho Andersen (AMD) <[email protected]>

Signed-off-by Pratik R. Sampat <[email protected]>

Quoted 7 lines

Quoted 2 lines

#ifdef CONFIG_FW_UPLOAD
+/* Largest image the firmware accepts, anything above is rejected */

I may have missed it, but I don't see anything in the SNP ABI spec that says the limit is 512K. If that doesn't have a limit how did we arrive at 512K?

The ABI spec doesn't mention it, but the firmware had this limit hard-coded. This can potentially change without notice to the ABI. However, I did want a sanity check in the OS and that's why had it in.

Quoted 1 line

I can drop it and let the firmware fail if the image is too large.

The memory holding the firmware on the call to the ASP has to be contiguous, so you're likely to fail on the alloc_pages() if the image is too large. Up to you if you want to keep it.

Quoted 24 lines

Quoted 11 lines

drivers/crypto/ccp/sev-dev.c static int sev_update_firmware(struct device *dev)

Quoted 6 lines

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

+#define SEV_FW_IMAGE_MAX_SIZE	SZ_512K
+
 
+
+	__sev_release_firmware_buffers(false);

Do the buffers have to be released? If so, why? I think you can keep the allocations. During platform initialization the buffers will be detected. Is there a shutdown path where they might not get freed?

Shantanu hit this on Milan with an earlier version of the series that kept the buffers across the update [1]. After DOWNLOAD_FIRMWARE_EX the new firmware rejected INIT_EX with SEV_RET_INVALID_PAGE_STATE (0x1A), because sev_es_tmr and sev_init_ex_buffer were still in the firmware state left over from the previous image's INIT. Freeing them, and letting re-init allocate fresh ones, fixed it, which is where this call came from.

However, a fresh allocation with SNP initialized just goes through rmp_mark_pages_firmware(), so my understanding of what the new firmware needs is the reclaim -> make shared -> mark firmware cycle, not necessarily new memory.

Quoted 4 lines

Freeing seemed like the easiest option, but if you prefer, just cycling them back should be able to achieve the same effect, I believe.

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

Sounds like some good info to have as a comment above the call then.

Thanks,
Tom

Quoted 7 lines · Tom Lendacky

Quoted 4 lines · Pratik R. Sampat

However, a fresh allocation with SNP initialized just goes through rmp_mark_pages_firmware(), so my understanding of what the new firmware needs is the reclaim -> make shared -> mark firmware cycle, not necessarily new memory.

Sounds like some good info to have as a comment above the call then.

Quoted 3 lines

The memory holding the firmware on the call to the ASP has to be contiguous, so you're likely to fail on the alloc_pages() if the image is too large. Up to you if you want to keep it.

Quoted 8 lines · Pratik R. Sampat

Quoted 7 lines · Tom Lendacky

Quoted 4 lines · Pratik R. Sampat

However, a fresh allocation with SNP initialized just goes through rmp_mark_pages_firmware(), so my understanding of what the new firmware needs is the reclaim -> make shared -> mark firmware cycle, not necessarily new memory.

Sounds like some good info to have as a comment above the call then.

I had looked at cycling briefly, but it gets tricky. If we reclaim the pages in sev_fw_upload_shutdown_platform() and re-init gets skipped because of RESTORE_REQUIRED or a dead PSP, the buffers are still allocated but aren't marked as firmware pages, so all subsequent paths need to be aware of that.

Reclaiming after the update avoids that, but relies on the new image accepting a reclaim of pages the old image's INIT left behind, which I'm not sure works, though I haven't tested it.

In either case, failure handling also seemed tricky, and the fallback would just be free and re-allocate anyway.

The only issue I see with freeing is that the TMR is a 2M buffer that needs to be contiguous. If the new allocation fails, INIT carries on without a TMR and without any error (it's only logged in dmesg), even though SEV-ES is now disabled. It should be very rare (since we did just free the TMR and can probably get the same block back), but it seems worth mentioning in a comment.

Quoted 4 lines

Quoted 4 lines

The memory holding the firmware on the call to the ASP has to be contiguous, so you're likely to fail on the alloc_pages() if the image is too large. Up to you if you want to keep it.

I think it's better to keep the explicit check and error message. If the size check falls through to the alloc_pages() failure, userspace sees device-busy, which doesn't communicate the right intent.

Apart from these, everything else I tested in v3 works on Milan.

Thanks,
Shantanu