Message-ID: <CAGxU2F7fdAi148rB-4c==-qCOW1SJjwf4AzC2=TUhfPXMhR5pQ@mail.gmail.com>
Date: Tue, 18 Mar 2025 11:38:54 +0100
From: Stefano Garzarella <sgarzare@...hat.com>
To: Jarkko Sakkinen <jarkko@...nel.org>, Tom Lendacky <thomas.lendacky@....com>
Cc: Peter Huewe <peterhuewe@....de>, Jason Gunthorpe <jgg@...pe.ca>, x86@...nel.org, 
	linux-kernel@...r.kernel.org, Borislav Petkov <bp@...en8.de>, 
	linux-integrity@...r.kernel.org, Dov Murik <dovmurik@...ux.ibm.com>, 
	Dionna Glaze <dionnaglaze@...gle.com>, linux-coco@...ts.linux.dev, 
	James Bottomley <James.Bottomley@...senpartnership.com>, 
	Claudio Carvalho <cclaudio@...ux.ibm.com>, Ingo Molnar <mingo@...hat.com>, 
	"H. Peter Anvin" <hpa@...or.com>, Thomas Gleixner <tglx@...utronix.de>, 
	Dave Hansen <dave.hansen@...ux.intel.com>, Joerg Roedel <jroedel@...e.de>
Subject: Re: [PATCH v3 3/4] tpm: add SNP SVSM vTPM driver

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

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

Thanks,
Stefano

Powered by blists - more mailing lists