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

[Tycho Andersen <tycho@kernel.org>]

Hi Pratik,

Two small comments if you happen to respin this one, no worries if
not.

On Thu, Oct 15, 2026 at 02:14:09AM +0000, Pratik R. Sampat wrote:
> +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 the new firmware, an explicit 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);

I guess this function could use a lockdep_assert_held(&sev_cmd_mutex),
but maybe we should not do that and instead put it in
__sev_do_cmd_locked()...

> +static enum fw_upload_err sev_fw_upload_update(struct sev_device *sev,
> +					       const u8 *data, u32 size,
> +					       u32 *written)
> +{
> +	int cmd_ret, psp_ret = SEV_RET_NO_FW_CALL;
> +	enum fw_upload_err fwl_ret;
> +
> +	guard(mutex)(&sev_cmd_mutex);
> +
> +	/*
> +	 * SEV firmware will only successfully execute the DLFW_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.
> +	 */
> +	if (psp_dead && !sev->fwl_rollback_required) {
> +		dev_err_ratelimited(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.
> +	 * Being in this state means that the legacy firmware has previously
> +	 * been shut down, so no need to do it again.
> +	 */
> +	if (sev->fwl_rollback_required) {
> +		psp_dead = false;
> +	} else {
> +		cmd_ret = sev_fw_upload_shutdown_platform(sev);
> +		if (cmd_ret)
> +			return psp_dead ? FW_UPLOAD_ERR_HW_ERROR : FW_UPLOAD_ERR_BUSY;
> +	}
> +
> +	cmd_ret = sev_download_firmware_ex(data, size, &psp_ret);
> +	fwl_ret = sev_fw_upload_handle_err(sev, cmd_ret, psp_ret);
> +	if (fwl_ret == FW_UPLOAD_ERR_NONE) {
> +		*written = size;
> +		sev->fwl_rollback_required = false;
> +	}
> +
> +	/*
> +	 * Firmware either just requested a rollback or a rollback attempt
> +	 * failed. Keep the PSP dead for all other commands, only another
> +	 * DLFW_EX attempt is allowed through.
> +	 */
> +	if (sev->fwl_rollback_required) {
> +		psp_dead = true;
> +		if (fwl_ret != FW_UPLOAD_ERR_HW_ERROR)
> +			fwl_ret = FW_UPLOAD_ERR_RW_ERROR;

I think this error switcharoo could use a comment (or maybe just be
dropped?). e.g. if the user does something to set
->fwl_rollback_required and then tries another firmware upload that is
the wrong size, ERR_INVALID_SIZE -> ERR_RW_ERROR, which seems
misleading to me.

I saw some discussion about all this:
https://lore.kernel.org/lkml/20260924213737.3833625-1-shansinha@google.com/

but it's not clear to me why you need to reset the error code here.

Thanks,

Tycho