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

[Pratik R. Sampat]

Hi Shantanu,

Thanks for the review. Tycho is out for the next few weeks so picking on this
in the meantime.

On 8/31/26 4:47 PM, Shantanu Sinha wrote:
> [You don't often get email from shansinha@xxxxxxxxxx. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> 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.)

Releasing the TMR and sev_init_ex_buffer pages makes sense here.

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

Agreed. Matters more with the teardown above. init re-marks the TMR
private before failing, stranding 2MB.

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

Having an ordering isn't particularly necessary from what I can tell. It's
arguably better than what we had where early-returns on the cached state, so
re-initing first keys off the driver's own record of having shut down rather
than off whatever the new firmware reports.

The only issue I see is that it reports a stale API version. Refreshing the
platform status within __sev_platform_init_locked() should do the trick.

Thanks,
--Pratik