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

[Jarkko Sakkinen <jarkko@kernel.org>]

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.
> 
> > +#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.
> 
> 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.

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

kmalloc() does do the "right thing here but it is still extra
unnecessary layer of random stuff on top...

> 
> Thanks,

BR, Jarkko