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

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

Hi Shantanu,

On 9/24/26 5:37 PM, Shantanu Sinha wrote:
>> +static enum fw_upload_err sev_fw_upload_handle_err(struct sev_device *sev,
>> +						   int rc, int psp_ret)
>> +{
>> +	enum fw_upload_err ret = FW_UPLOAD_ERR_FW_INVALID;
>> +
>> +	if (!rc)
>> +		return FW_UPLOAD_ERR_NONE;
>> +
>> +	switch (psp_ret) {
> [...]
>> +	case SEV_RET_UPDATE_FAILED:
>> +		ret = FW_UPLOAD_ERR_HW_ERROR;
>> +		dev_err(sev->dev, "DLFW_EX: Upgrade failed, automatically reverted\n");
>> +		break;
>> +	case SEV_RET_RESTORE_REQUIRED:
>> +		dev_err(sev->dev, "DLFW_EX: live upgrade failed, please roll back\n");
>> +		/*
>> +		 * 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_HW_ERROR;
>> +		break;
>> +	case SEV_RET_HWSEV_RET_UNSAFE:
>> +		dev_err(sev->dev, "DLFW_EX: SEV firmware no longer safe. Reboot recommended\n");
>> +		/*
>> +		 * 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.
>> +		 */
>> +		psp_dead = true;
>> +		ret = FW_UPLOAD_ERR_HW_ERROR;
>> +		break;
>> +	case SEV_RET_NO_FW_CALL:
>> +		/* The command never reached the firmware. */
>> +		dev_err(sev->dev, "DLFW_EX: driver error %d\n", rc);
>> +		ret = FW_UPLOAD_ERR_HW_ERROR;
>> +		break;
>> +	default:
>> +		dev_err(sev->dev, "Unknown SEV firmware err 0x%x\n", psp_ret);
>> +		ret = FW_UPLOAD_ERR_HW_ERROR;
>> +		break;
>> +	}
>> +
>> +	return ret;
>> +}
> 
> I've been trying to work this patch into our current workflow and
> running into a slight issue. My goal is to cleanly distinguish 
> between the following failure outcomes and their associated actions:
> 
> 1. Rollback to the old (committed) firmware: SEV_RET_RESTORE_REQUIRED
>    (0x25), where the PSP did not auto-revert (psp_dead == true &&
>    fwl_rollback_required == true) and will only accept a
>    DOWNLOAD_FIRMWARE_EX upload of the CommittedVersion binary to recover
>    the host without a reboot.
> 2. Retry the update: device busy / legacy SEV guest active
>    (SEV_STATE_WORKING), driver-side -ENOMEM (SEV_RET_NO_FW_CALL), or
>    SEV_RET_UPDATE_FAILED (0x24), where the PSP either auto-reverted or
>    was never modified (psp_dead == false) and remains healthy on the
>    previous firmware.
> 3. Abandon the update due to a fundamental image or precondition error:
>    bad signature, malformed image, version/SVN check failure, or
>    SHUTDOWN_REQUIRED, where the PSP rejected the command up front and
>    neither retrying nor rebooting will help.
> 4. Reboot the host: SEV_RET_HWSEV_RET_UNSAFE (0x14) where the PSP is
>    permanently disabled until a system reset.
> 
> Right now, the same user-facing error in /sys/class/firmware/sev/error
> (FW_UPLOAD_ERR_HW_ERROR / "transferring:hw-error") is returned across
> states 1, 2, and 4 above (SEV_RET_RESTORE_REQUIRED,
> SEV_RET_UPDATE_FAILED, -ENOMEM, and SEV_RET_HWSEV_RET_UNSAFE). Because
> psp_ret is only logged via dev_dbg() in __sev_do_cmd_locked(), userspace
> tooling only sees "transferring:hw-error" in sysfs and has no clean way
> to tell whether to roll back to the committed image, retry, or reboot
> the machine without scraping English strings out of dmesg, which has
> been finicky.
> 
> Could we do a closer review of the error reporting here and potentially
> include (rc, psp_ret) (matching the "failed %d, error %#x" format
> already used in sev_fw_upload_shutdown_platform() and
> sev_fw_upload_reinit_platform() below) in every dev_err() output so the
> exact driver return code and PSP status code are always visible in
> dmesg?

Agreed. Having better error logging by adding rc and psp_ret should help.

> 
> Alternatively (or alongside that), we could also separate the enum
> fw_upload_err buckets so those four outcomes map to distinct sysfs
> errors:
> 
> - Keep FW_UPLOAD_ERR_HW_ERROR for fatal states where the PSP is dead
>   until a host reboot (SEV_RET_HWSEV_RET_UNSAFE, or when psp_dead is
>   already latched).
> - Use FW_UPLOAD_ERR_RW_ERROR for SEV_RET_RESTORE_REQUIRED (where writing
>   the committed firmware image recovers the PSP without a reboot).

This error return seems a bit of a loose fit to me. It isn't an error reading /
writing, rather it is the failure of the platform having partial success
applying this. IMO, it should be in the same bucket as FW_UPLOAD_ERR_HW_ERROR.

> - Use FW_UPLOAD_ERR_BUSY for retryable states where the PSP is still
>   running the previous firmware (SEV_RET_UPDATE_FAILED and -ENOMEM,
>   alongside SEV_STATE_WORKING).
> - Keep FW_UPLOAD_ERR_FW_INVALID (and FW_UPLOAD_ERR_INVALID_SIZE) for
>   permanent image/version rejections where the update should be
>   abandoned, and add case SEV_RET_BAD_SVN: alongside
>   SEV_RET_INVALID_CONFIG so an SVN check failure returns
>   FW_UPLOAD_ERR_FW_INVALID instead of falling into default
>   (FW_UPLOAD_ERR_HW_ERROR).

Agreed with the rest.

> 
> As a side note, I think -ETIMEDOUT is not handled properly. When a
> command times out in __sev_do_cmd_locked(), it zeroes *psp_ret = 0, sets
> psp_dead = true, and returns -ETIMEDOUT. Because
> sev_fw_upload_handle_err() only checks !rc before switch (psp_ret), a
> timeout falls into the default case, prints "Unknown SEV firmware err
> 0x0", and returns FW_UPLOAD_ERR_HW_ERROR instead of
> FW_UPLOAD_ERR_TIMEOUT.


Ack. Thanks for catching this.

I'll work in all these for the next iteration.

--Pratik