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

[Tycho Andersen]

On Sat, May 02, 2026 at 10:18:42PM -0500, 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.

Sure, I'll make the change here and in the other places you noted.

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

Because this code calls some functions, including
sev_firmware_reinit_if_shutdown(), that take the lock again. We could
use scoped_guard() I suppose, I can look at that for v2.

It may be useful to do a larger series where we re-think when the
locks are acquired here. It seems like only grabbing them at the top
level entrypoints: ioctl, platform init, firmware update, etc. and
putting lockdep asserts in all the helpers in the file might be
cleaner generally.

Tycho