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

[Tom Lendacky <thomas.lendacky@amd.com>]

On 10/6/26 11:41, Pratik R. Sampat wrote:
> Hi Tom,
> 
> Thanks for the review!
> 
> On 10/6/26 11:10 AM, Tom Lendacky wrote:
>> On 10/5/26 11:15, Pratik R. Sampat wrote:
>>> Implement SNP live firmware update using the DOWNLOAD_FIRMWARE_EX
>>> command.
>>>
>>> DOWNLOAD_FIRMWARE_EX requires the legacy SEV platform to be UNINIT. If
>>> it is WORKING then legacy guests are running and the update is refused
>>> as busy. If it is INIT, shut it down, release the buffers the firmware
>>> owns across that shutdown, run the update, and bring the platform back
>>> up afterwards. SNP is never taken down, so SNP guests are unaffected.
>>>
>>> To test run the following with your sbin file in FW:
>>>
>>> echo 1 > /sys/class/firmware/sev/loading
>>> cat <firmware.sbin> > /sys/class/firmware/sev/data
>>> echo 0 > /sys/class/firmware/sev/loading
>>>
>>> The COMMIT bit is left clear, so the image is only loaded provisionally
>>> and the admin decides when to make it permanent with ioctl(/dev/sev,
>>> SNP_COMMIT). To roll back, do not commit and upload the previous image
>>> the same way.
>>>
>>> Co-developed-by: Tycho Andersen (AMD) <tycho@kernel.org>
>>> Signed-off-by: Tycho Andersen (AMD) <tycho@kernel.org>
>>> Signed-off-by: Pratik R. Sampat <prsampat@amd.com>

>>>  #ifdef CONFIG_FW_UPLOAD
>>> +/* Largest image the firmware accepts, anything above is rejected */
>>
>> I may have missed it, but I don't see anything in the SNP ABI spec that
>> says the limit is 512K. If that doesn't have a limit how did we arrive
>> at 512K?
>>
> 
> The ABI spec doesn't mention it, but the firmware had this limit hard-coded.
> This can potentially change without notice to the ABI. However, I did want a
> sanity check in the OS and that's why had it in.
> 
> I can drop it and let the firmware fail if the image is too large.

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.
> 
>>> +#define SEV_FW_IMAGE_MAX_SIZE	SZ_512K
>>> +

>>> +
>>> +	__sev_release_firmware_buffers(false);
>>
>> Do the buffers have to be released? If so, why? I think you can keep the
>> allocations. During platform initialization the buffers will be
>> detected. Is there a shutdown path where they might not get freed?
>>
> 
> Shantanu hit this on Milan with an earlier version of the series that kept the
> buffers across the update [1]. After DOWNLOAD_FIRMWARE_EX the new firmware
> rejected INIT_EX with SEV_RET_INVALID_PAGE_STATE (0x1A), because sev_es_tmr and
> sev_init_ex_buffer were still in the firmware state left over from the previous
> image's INIT. Freeing them, and letting re-init allocate fresh ones, fixed it,
> which is where this call came from.
> 
> 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.
> 
> Freeing seemed like the easiest option, but if you prefer, just cycling them
> back should be able to achieve the same effect, I believe.
> 
> [1] https://lore.kernel.org/all/20260831204757.436751-1-shansinha@google.com/

Sounds like some good info to have as a comment above the call then.

Thanks,
Tom