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

[Maxwell Doose]

On Sat May 2, 2026 at 10:18 PM CDT, Maxwell Doose wrote:
> On Thu Apr 30, 2026 at 11:07 AM CDT, Tycho Andersen wrote:
>> From: "Tycho Andersen (AMD)" <tycho@xxxxxxxxxx>
>>
>> Put all the previous primitives together to implement SNP firmware
>> live update via DOWNLOAD_FIRMWARE_EX.
>>
>
> [snip]
>
>>
>> [1]: https://lore.kernel.org/lkml/20241112232253.3379178-7-dionnaglaze@xxxxxxxxxx/
>> Signed-off-by: Tycho Andersen (AMD) <tycho@xxxxxxxxxx>
>> ---
>>  drivers/crypto/ccp/sev-dev.c | 244 ++++++++++++++++++++++++++++++++++-
>>  1 file changed, 243 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
>> index b4711bf823e8..e7fe6dbf69c2 100644
>> --- a/drivers/crypto/ccp/sev-dev.c
>> +++ b/drivers/crypto/ccp/sev-dev.c
>> +static int sev_download_firmware_ex(struct sev_device *sev, const u8 *data,
>> +				    u32 size)
>> +{
>> +	struct sev_data_download_firmware_ex sev_data = {0};
>> +	int ret, error = 0, order;
>>
>
> Why not assign across multiple lines? How about something like:
>
> int ret, order;
> int error = 0;
>
> or:
>
> int ret;
> int order;
> int error = 0;
>
> Would be better for readability and git blame.
>
>>  static enum fw_upload_err sev_fw_upload_write(struct fw_upload *fw_upload,
>>  					      const u8 *data, u32 offset,
>>  					      u32 size, u32 *written)
>> {
>>
>
> [snip]
>
>>
>> +	old_major = sev->api_major;
>> +	old_minor = sev->api_minor;
>> +	old_build = sev->build;
>> +
>> +	mutex_lock(&sev_cmd_mutex);
>>
>
> Why not guard(mutex)()? You used it earlier in
> sev_firmware_reinit_if_shutdown().
>
>>
>> +	*written = size;
>> +	ret = FW_UPLOAD_ERR_NONE;
>> +
>> +unlock:
>> +	mutex_unlock(&sev_cmd_mutex);
>>
>
> See above comment.
>
> best regards,
> maxwell

Also, forgot to mention:
>> static enum fw_upload_err sev_fw_upload_write(struct fw_upload *fw_upload,
 					      const u8 *data, u32 offset,
 					      u32 size, u32 *written)
>> {
>> -	return FW_UPLOAD_ERR_BUSY;
>> +	struct sev_device *sev = fw_upload->dd_handle;
>> +	u8 old_major, old_minor, old_build;
>> +	int rc, error = 0;
>>

Forgot to mention this, but probably should make it:

int rc;
int error = 0;

best regards,
maxwell