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

[Pratik R. Sampat]

On 10/6/26 12:32 PM, Tom Lendacky wrote:
> 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@xxxxxxxxxx>
>>>> Signed-off-by: Tycho Andersen (AMD) <tycho@xxxxxxxxxx>
>>>> Signed-off-by: Pratik R. Sampat <prsampat@xxxxxxx>
> 
>>>>  #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.

Sure.

>>
>>>> +#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@xxxxxxxxxx/
> 
> Sounds like some good info to have as a comment above the call then.
> 

Will do!

Thanks,
--Pratik

> Thanks,
> Tom
>