Re: [PATCH v3 3/4] tpm: add SNP SVSM vTPM driver

[Jarkko Sakkinen <jarkko@kernel.org>]

On Tue, Mar 18, 2025 at 11:38:54AM +0100, Stefano Garzarella wrote:
> On Mon, Mar 17, 2025 at 03:43:18PM +0200, Jarkko Sakkinen wrote:
> >On Fri, Mar 14, 2025 at 11:48:11AM -0500, Tom Lendacky wrote:
> >> On 3/11/25 04:42, Stefano Garzarella wrote:
> >> > Add driver for the vTPM defined by the AMD SVSM spec [1].
> >> >
> >> > The specification defines a protocol that a SEV-SNP guest OS can use to
> >> > discover and talk to a vTPM emulated by the Secure VM Service Module (SVSM)
> >> > in the guest context, but at a more privileged level (VMPL0).
> >> >
> >> > The new tpm-svsm platform driver uses two functions exposed by x86/sev
> >> > to verify that the device is actually emulated by the platform and to
> >> > send commands and receive responses.
> >> >
> >> > The device cannot be hot-plugged/unplugged as it is emulated by the
> >> > platform, so we can use module_platform_driver_probe(). The probe
> >> > function will only check whether in the current runtime configuration,
> >> > SVSM is present and provides a vTPM.
> >> >
> >> > This device does not support interrupts and sends responses to commands
> >> > synchronously. In order to have .recv() called just after .send() in
> >> > tpm_try_transmit(), the .status() callback returns 0, and both
> >> > .req_complete_mask and .req_complete_val are set to 0.
> >> >
> >> > [1] "Secure VM Service Module for SEV-SNP Guests"
> >> >     Publication # 58019 Revision: 1.00
> >> >
> >> > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> >> > ---
> >> > v3:
> >> > - removed send_recv() ops and followed the ftpm driver implementing .status,
> >> >   .req_complete_mask, .req_complete_val, etc. [Jarkko]
> >> > - removed link to the spec because those URLs are unstable [Borislav]
> >> > ---
> >> >  drivers/char/tpm/tpm_svsm.c | 148 ++++++++++++++++++++++++++++++++++++
> >> >  drivers/char/tpm/Kconfig    |  10 +++
> >> >  drivers/char/tpm/Makefile   |   1 +
> >> >  3 files changed, 159 insertions(+)
> >> >  create mode 100644 drivers/char/tpm/tpm_svsm.c
> >> >
> >> > diff --git a/drivers/char/tpm/tpm_svsm.c b/drivers/char/tpm/tpm_svsm.c
> >> > new file mode 100644
> >> > index 000000000000..5540d0227eed
> >> > --- /dev/null
> >> > +++ b/drivers/char/tpm/tpm_svsm.c
> >> > @@ -0,0 +1,148 @@
> >> > +// SPDX-License-Identifier: GPL-2.0-only
> >> > +/*
> >> > + * Copyright (C) 2025 Red Hat, Inc. All Rights Reserved.
> >> > + *
> >> > + * Driver for the vTPM defined by the AMD SVSM spec [1].
> >> > + *
> >> > + * The specification defines a protocol that a SEV-SNP guest OS can use to
> >> > + * discover and talk to a vTPM emulated by the Secure VM Service Module (SVSM)
> >> > + * in the guest context, but at a more privileged level (usually VMPL0).
> >> > + *
> >> > + * [1] "Secure VM Service Module for SEV-SNP Guests"
> >> > + *     Publication # 58019 Revision: 1.00
> >> > + */
> >> > +
> >> > +#include <asm/sev.h>
> >>
> >> Typically the "asm" includes are after the "linux" includes and separated
> >> from each other by a blank line.
> 
> Yep, I already fixed it in v4, since I found that issue while
> backporting this patch to CentOS 9.
> 
> >>
> >> > +#include <linux/module.h>
> >> > +#include <linux/kernel.h>
> >> > +#include <linux/platform_device.h>
> >> > +#include <linux/svsm_vtpm.h>
> >> > +
> >> > +#include "tpm.h"
> >> > +
> >> > +struct tpm_svsm_priv {
> >> > +  u8 buffer[SVSM_VTPM_MAX_BUFFER];
> >> > +  u8 locality;
> >> > +};
> >>
> >> I'm wondering if the buffer shouldn't be a pointer to a page of memory
> >> that is a page allocation. This ensures it is always page-aligned in case
> >> the tpm_svsm_priv structure is ever modified.
> 
> @Tom Should that buffer really page aligned?
> 
> I couldn't find anything in the specification. IIRC edk2 also doesn't
> allocate it aligned, and the code in SVSM already handles the case when
> this is not aligned.
> 
> So if it is to be aligned to the pages, we should reinforce it in SVSM
> (spec/code) and also fix edk2.
> 
> Or was yours a suggestion for performance/optimization?
> 
> >>
> >> As it is, the kmalloc() allocation will be page-aligned because of the
> >> size, but it might be safer, dunno, your call.
> >
> >This was good catch. There's actually two issues here:
> >
> >1. SVSM_VTPM_MAX_BUFFER is same as page size.
> >2. SVSM_VTPM_MAX_BUFFER is IMHO defined in wrong patch 2/4.
> 
> I put it in patch 2 because IIUC it should be part of the SVSM
> specification (the size, not the alignment).
> 
> >
> >So this constant would be needed, it should be appeneded in this patch,
> >not in 2/4 because it has direct effect on implementation of the driver.
> >
> >I'd personally support the idea of removing this constant altogether
> >and use alloc_page() (i.e., same as you suggested).
> 
> Do you think it's necessary, even though alignment is not required?
> (I'm still not clear if it's a requirement, see above)

If the question is whether I would NAK based on using kzalloc(). Likely
not but still using page allocator would be more lean :-)

> 
> >
> >kmalloc() does do the "right thing here but it is still extra
> >unnecessary layer of random stuff on top...
> 
> Yes, if it has to be aligned I completely agree. I would like to use
> devm_ functions to keep the driver simple. Do you think
> devm_get_free_pages() might be a good alternative to alloc_page()?

Yes, I think it could be used here.

> 
> Thanks,
> Stefano
> 

BR, Jarkko