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

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

On 10/6/26 4:09 PM, Shantanu Sinha wrote:
> On 10/6/26 12:55 PM, Pratik R. Sampat wrote:
>> On 10/6/26 12:32 PM, Tom Lendacky wrote:
>>> On 10/6/26 11:41, Pratik R. Sampat wrote:
>>>> However, a fresh allocation with SNP initialized just goes through
>>>> rmp_mark_pages_firmware(), so my understanding of what the new firmware needs
>>>> is the reclaim -> make shared -> mark firmware cycle, not necessarily new
>>>> memory.
>>>
>>> Sounds like some good info to have as a comment above the call then.
> 
> I had looked at cycling briefly, but it gets tricky. If we reclaim the
> pages in sev_fw_upload_shutdown_platform() and re-init gets skipped
> because of RESTORE_REQUIRED or a dead PSP, the buffers are still
> allocated but aren't marked as firmware pages, so all subsequent paths
> need to be aware of that.
> 
> Reclaiming after the update avoids that, but relies on the new image
> accepting a reclaim of pages the old image's INIT left behind, which I'm
> not sure works, though I haven't tested it.
> 
> In either case, failure handling also seemed tricky, and the fallback
> would just be free and re-allocate anyway.

Agreed.

> 
> The only issue I see with freeing is that the TMR is a 2M buffer that
> needs to be contiguous. If the new allocation fails, INIT carries on
> without a TMR and without any error (it's only logged in dmesg), even
> though SEV-ES is now disabled. It should be very rare (since we did
> just free the TMR and can probably get the same block back), but it
> seems worth mentioning in a comment.

Sure, I can add this to the comment too.

> 
>>> The memory holding the firmware on the call to the ASP has to be
>>> contiguous, so you're likely to fail on the alloc_pages() if the image
>>> is too large. Up to you if you want to keep it.
> 
> I think it's better to keep the explicit check and error message. If the
> size check falls through to the alloc_pages() failure, userspace sees
> device-busy, which doesn't communicate the right intent.
> 

Ack.

> One other small thing. If the platform data refresh in
> sev_fw_upload_write() fails after DLFW_EX has succeeded, it returns
> hw-error even when the PSP is still alive and the new image is running.
> I may be missing a reason for that, but would something like this make
> sense?
> 
>  	if (sev_get_api_version()) {
> -		dev_err(sev->dev, "SNP platform data refresh after firmware update failed\n");
> -		return FW_UPLOAD_ERR_HW_ERROR;
> +		dev_warn(sev->dev, "SNP platform data refresh after firmware update failed\n");
> +		return psp_dead ? FW_UPLOAD_ERR_HW_ERROR : FW_UPLOAD_ERR_NONE;
>  	}
> 

I haven't typically seen this fail for any other reason apart from potentially
failing the PSP call but I do see a few paths where this could potentially
happen. So, only returning an error when PSP is dead makes sense to me.

> Apart from these, everything else I tested in v3 works on Milan.

Thanks, for testing this out!

--Pratik

> 
> Thanks,
> Shantanu