Re: [PATCH 6/6] crypto/ccp: Implement SNP firmware live update

[Shantanu Sinha <shansinha@google.com>]

On Thu, Apr 30, 2026 at 10:07:16AM -0600, Tycho Andersen wrote:
> +static int sev_firmware_shutdown_if_sev_initialized(struct sev_device *sev)
> +{
> +	int rc, error;
> +	int sev_plat_state;
> +
> +	rc = sev_get_platform_state(&sev_plat_state, &error);
> +	if (rc) {
> +		if (error)
> +			rc = error;
> +		dev_dbg(sev->dev, "SEV get platform state failed %d\n", rc);
> +		return rc;
> +	}
> +
> +	switch (sev_plat_state) {
> +	case SEV_STATE_UNINIT:
> +		return 0;
> +	case SEV_STATE_INIT:
> +		error = 0;
> +		rc = __sev_platform_shutdown_locked(&error);
> +		if (rc) {
> +			if (error)
> +				rc = error;
> +			dev_err(sev->dev, "SEV platform shutdown failed %d\n", rc);
> +			return rc;
> +		}
> +
> +		sev_firmware_needs_reinit = true;
> +		return 0;

Tested this on Milan and hit a failure during SEV re-init after firmware update.

__sev_platform_shutdown_locked() sets the FW platform state to UNINIT, but
sev_es_tmr and sev_init_ex_buffer remain firmware-owned in the RMP. When the
new firmware comes up, SEV_CMD_INIT_EX rejects the stale page state and fails
with SEV_RET_INVALID_PAGE_STATE (0x1A).

Tearing down the buffers on shutdown fixes it for us:

--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -1905,6 +1905,20 @@ static int sev_firmware_shutdown_if_sev_initialized(struct sev_device *sev)
	}
 
+	if (sev_es_tmr) {
+		wbinvd_on_all_cpus();
+		__snp_free_firmware_pages(virt_to_page(sev_es_tmr),
+					  get_order(sev_es_tmr_size),
+					  true);
+		sev_es_tmr = NULL;
+	}
+
+	if (sev_init_ex_buffer) {
+		__snp_free_firmware_pages(virt_to_page(sev_init_ex_buffer),
+					  get_order(NV_LENGTH),
+					  true);
+		sev_init_ex_buffer = NULL;
+	}
+
	sev_firmware_needs_reinit = true;
	return 0;

(Could also pull this and the cleanup in __sev_firmware_shutdown() into a shared
helper. This logic is duplicated there.)

>	if (ret == FW_UPLOAD_ERR_NONE) {
>		error = 0;
> 
>		rc = sev_get_api_version();
>		if (rc) {
>			if (error)
>				rc = error;
>			dev_err(sev->dev, "SEV query api version failed %d\n", rc);
>		}
>	}
> 
> +	if (!dlfwex_wants_rollback)
> +		sev_firmware_reinit_if_shutdown(sev);

If rc == SEV_RET_HWSEV_RET_UNSAFE, psp_dead is true but dlfwex_wants_rollback
is false, so it still falls through to re-init. Even though it is
recommended that the host should be rebooted after HARDWARE_UNSAFE,
reboot tooling can lag and we may want some buffer. Plus the kernel shouldn't
be touching a dead PSP anyway. Checking !psp_dead is a simple safeguard against
extra churn while awaiting restart:

+	if (!dlfwex_wants_rollback && !psp_dead)
		sev_firmware_reinit_if_shutdown(sev);

I also moved the call to sev_firmware_reinit_if_shutdown() ahead of
sev_get_api_version() so sev->state is already restored to INIT before sev
status is queried. This is just to avoid unnecessary transient states.
Curious if that logic makes sense to you. Am I missing something that made
the original ordering strictly necessary?

Best,
Shantanu