Re: [Patch v3 7/7] crypto/ccp: Implement SNP Download Firmware EX

[Tom Lendacky <thomas.lendacky@amd.com>]

On 10/5/26 11:15, Pratik R. Sampat wrote:
> 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) <tycho@kernel.org>
> Signed-off-by: Tycho Andersen (AMD) <tycho@kernel.org>
> Signed-off-by: Pratik R. Sampat <prsampat@amd.com>
> ---
>  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(-)
> 
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index 88cf60a9640e..e54f23ba1b9b 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -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)
>  }
>  
>  #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?

> +#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);
> +	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.

> +	 */
> +	sev_data.commit = 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.

> +{
> +	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."

> +	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;
> +		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).

> +
> +	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);
> +		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?

> +
> +	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)
> +		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.

> +		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;
> +	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().

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

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

s/We being/Being/

> +	 * been shut down, so no need to do it again.
> +	 */
> +	if (sev->fwl_rollback_required) {
> +		psp_dead = false;
> +	} 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/

> +	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()) {
> +		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.

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

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

> +			 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
> +			 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)
> diff --git a/drivers/crypto/ccp/sev-dev.h b/drivers/crypto/ccp/sev-dev.h
> index 7ec692e2147e..1e45a08c41da 100644
> --- a/drivers/crypto/ccp/sev-dev.h
> +++ b/drivers/crypto/ccp/sev-dev.h
> @@ -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);
> diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h
> index fab62228f981..b71154e9ae4b 100644
> --- a/include/linux/psp-sev.h
> +++ b/include/linux/psp-sev.h
> @@ -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
>   *