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

[Shantanu Sinha <shansinha@google.com>]

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

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

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.