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

[Shantanu Sinha <shansinha@google.com>]

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.

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.

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

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

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

Thanks,
Shantanu