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

["Pratik R. Sampat" <prsampat@amd.com>]

Hi Tycho

On 10/15/26 3:38 PM, Tycho Andersen wrote:
> 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()...

Yeah doing the latter seems like a better idea for quite a few calls that use this.
And I think maybe better so in a separate cleanup patch.

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

My understanding from that discussion was that userspace errors should
fall into clean buckets. read-write-error would mean "roll back",
whether it came from the first failure or a later rollback attempt,
since a failed rollback still leaves the PSP dead and needing a
rollback. dmesg would still log the exact reason for the failure.

That said, I see how it can be misleading, and I'm fine with dropping
the override or putting a comment and re-spinning this.

Thanks
--Pratik

> Thanks,
> 
> Tycho