diff mbox series

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

Message ID 20250311094225.35129-4-sgarzare@redhat.com (mailing list archive)
State New
Headers show
Series Enlightened vTPM support for SVSM on SEV-SNP | expand

Commit Message

Stefano Garzarella March 11, 2025, 9:42 a.m. UTC 
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

Comments

Tom Lendacky March 14, 2025, 4:48 p.m. UTC | #1 
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.

Thanks,
Tom

> +
> +static int tpm_svsm_send(struct tpm_chip *chip, u8 *buf, size_t len)
> +{
> +	struct tpm_svsm_priv *priv = dev_get_drvdata(&chip->dev);
> +	int ret;
> +
> +	ret = svsm_vtpm_fill_cmd_req((struct tpm_send_cmd_req *)priv->buffer,
> +				     priv->locality, buf, len);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * The SVSM call uses the same buffer for the command and for the
> +	 * response, so after this call, the buffer will contain the response
> +	 * that can be used by .recv() op.
> +	 */
> +	return snp_svsm_vtpm_send_command(priv->buffer);
> +}
> +
> +static int tpm_svsm_recv(struct tpm_chip *chip, u8 *buf, size_t len)
> +{
> +	struct tpm_svsm_priv *priv = dev_get_drvdata(&chip->dev);
> +
> +	/*
> +	 * The internal buffer contains the response after we send the command
> +	 * to SVSM.
> +	 */
> +	return svsm_vtpm_parse_cmd_resp((struct tpm_send_cmd_resp *)priv->buffer,
> +					buf, len);
> +}
> +
> +static void tpm_svsm_cancel(struct tpm_chip *chip)
> +{
> +	/* not supported */
> +}
> +
> +static u8 tpm_svsm_status(struct tpm_chip *chip)
> +{
> +	return 0;
> +}
> +
> +static bool tpm_svsm_req_canceled(struct tpm_chip *chip, u8 status)
> +{
> +	return false;
> +}
> +
> +static struct tpm_class_ops tpm_chip_ops = {
> +	.flags = TPM_OPS_AUTO_STARTUP,
> +	.recv = tpm_svsm_recv,
> +	.send = tpm_svsm_send,
> +	.cancel = tpm_svsm_cancel,
> +	.status = tpm_svsm_status,
> +	.req_complete_mask = 0,
> +	.req_complete_val = 0,
> +	.req_canceled = tpm_svsm_req_canceled,
> +};
> +
> +static int __init tpm_svsm_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct tpm_svsm_priv *priv;
> +	struct tpm_chip *chip;
> +	int err;
> +
> +	if (!snp_svsm_vtpm_probe())
> +		return -ENODEV;
> +
> +	priv = devm_kmalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	/*
> +	 * FIXME: before implementing locality we need to agree what it means
> +	 * for the SNP SVSM vTPM
> +	 */
> +	priv->locality = 0;
> +
> +	chip = tpmm_chip_alloc(dev, &tpm_chip_ops);
> +	if (IS_ERR(chip))
> +		return PTR_ERR(chip);
> +
> +	dev_set_drvdata(&chip->dev, priv);
> +
> +	err = tpm2_probe(chip);
> +	if (err)
> +		return err;
> +
> +	err = tpm_chip_register(chip);
> +	if (err)
> +		return err;
> +
> +	dev_info(dev, "SNP SVSM vTPM %s device\n",
> +		 (chip->flags & TPM_CHIP_FLAG_TPM2) ? "2.0" : "1.2");
> +
> +	return 0;
> +}
> +
> +static void __exit tpm_svsm_remove(struct platform_device *pdev)
> +{
> +	struct tpm_chip *chip = platform_get_drvdata(pdev);
> +
> +	tpm_chip_unregister(chip);
> +}
> +
> +/*
> + * tpm_svsm_remove() lives in .exit.text. For drivers registered via
> + * module_platform_driver_probe() this is ok because they cannot get unbound
> + * at runtime. So mark the driver struct with __refdata to prevent modpost
> + * triggering a section mismatch warning.
> + */
> +static struct platform_driver tpm_svsm_driver __refdata = {
> +	.remove = __exit_p(tpm_svsm_remove),
> +	.driver = {
> +		.name = "tpm-svsm",
> +	},
> +};
> +
> +module_platform_driver_probe(tpm_svsm_driver, tpm_svsm_probe);
> +
> +MODULE_DESCRIPTION("SNP SVSM vTPM Driver");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:tpm-svsm");
> diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
> index 0fc9a510e059..fc3f1d10d31d 100644
> --- a/drivers/char/tpm/Kconfig
> +++ b/drivers/char/tpm/Kconfig
> @@ -225,5 +225,15 @@ config TCG_FTPM_TEE
>  	help
>  	  This driver proxies for firmware TPM running in TEE.
>  
> +config TCG_SVSM
> +	tristate "SNP SVSM vTPM interface"
> +	depends on AMD_MEM_ENCRYPT
> +	help
> +	  This is a driver for the AMD SVSM vTPM 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).  To compile this driver as a module, choose M
> +	  here; the module will be called tpm_svsm.
> +
>  source "drivers/char/tpm/st33zp24/Kconfig"
>  endif # TCG_TPM
> diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
> index 9bb142c75243..52d9d80a0f56 100644
> --- a/drivers/char/tpm/Makefile
> +++ b/drivers/char/tpm/Makefile
> @@ -44,3 +44,4 @@ obj-$(CONFIG_TCG_XEN) += xen-tpmfront.o
>  obj-$(CONFIG_TCG_CRB) += tpm_crb.o
>  obj-$(CONFIG_TCG_VTPM_PROXY) += tpm_vtpm_proxy.o
>  obj-$(CONFIG_TCG_FTPM_TEE) += tpm_ftpm_tee.o
> +obj-$(CONFIG_TCG_SVSM) += tpm_svsm.o
Jarkko Sakkinen March 17, 2025, 1:43 p.m. UTC | #2 
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
Stefano Garzarella March 18, 2025, 10:38 a.m. UTC | #3 
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)

>
>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
Tom Lendacky March 18, 2025, 2:54 p.m. UTC | #4 
On 3/18/25 05:38, 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?

No reason other than the size of the buffer is the size of a page.
Allocating a page provides a page that is dedicated to the buffer for
the SVSM. To me it just makes sense to keep it separate from any driver
related data. Just a suggestion, not a requirement, and no need to
update the spec.

Thanks,
Tom

> 
>>>
>>> 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
>
Stefano Garzarella March 18, 2025, 4:18 p.m. UTC | #5 
On Tue, Mar 18, 2025 at 09:54:31AM -0500, Tom Lendacky wrote:
>On 3/18/25 05:38, 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?
>
>No reason other than the size of the buffer is the size of a page.
>Allocating a page provides a page that is dedicated to the buffer for
>the SVSM. To me it just makes sense to keep it separate from any driver
>related data. Just a suggestion, not a requirement, and no need to
>update the spec.

I see, thanks for the clarification!
I saw that with devm_get_free_pages() I can easily allocate a 
resource-managed page, so I'll do that in v4.

Thanks,
Stefano

>
>Thanks,
>Tom
>
>>
>>>>
>>>> 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
>>
>
Jason Gunthorpe March 19, 2025, 11:44 p.m. UTC | #6 
On Tue, Mar 18, 2025 at 05:18:53PM +0100, Stefano Garzarella wrote:

> I see, thanks for the clarification!
> I saw that with devm_get_free_pages() I can easily allocate a
> resource-managed page, so I'll do that in v4.

As a general note you should just use kmalloc these days, even for
PAGE_SIZE. It is efficient and OK.

Having a struct that is PAGE_SIZE+1 is not efficient and will waste
a page of memory. That should be avoided ..

Jason
Stefano Garzarella March 20, 2025, 11:18 a.m. UTC | #7 
On Wed, Mar 19, 2025 at 08:44:22PM -0300, Jason Gunthorpe wrote:
>On Tue, Mar 18, 2025 at 05:18:53PM +0100, Stefano Garzarella wrote:
>
>> I see, thanks for the clarification!
>> I saw that with devm_get_free_pages() I can easily allocate a
>> resource-managed page, so I'll do that in v4.
>
>As a general note you should just use kmalloc these days, even for
>PAGE_SIZE. It is efficient and OK.

Thanks for sharing!

I think I'll stay with devm_get_free_pages() just because if it's
page aligned (with kmalloc I'm not sure if I have a way to ensure it), 
it can be a bitter faster for SVSM to map/unmap it on every command.

>
>Having a struct that is PAGE_SIZE+1 is not efficient and will waste
>a page of memory. That should be avoided ..

Got it, I will definitely split the buffer allocation from the priv.

Thanks,
Stefano
Jarkko Sakkinen March 20, 2025, 2:56 p.m. UTC | #8 
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
Jarkko Sakkinen March 20, 2025, 3 p.m. UTC | #9 
On Wed, Mar 19, 2025 at 08:44:22PM -0300, Jason Gunthorpe wrote:
> On Tue, Mar 18, 2025 at 05:18:53PM +0100, Stefano Garzarella wrote:
> 
> > I see, thanks for the clarification!
> > I saw that with devm_get_free_pages() I can easily allocate a
> > resource-managed page, so I'll do that in v4.
> 
> As a general note you should just use kmalloc these days, even for
> PAGE_SIZE. It is efficient and OK.
> 
> Having a struct that is PAGE_SIZE+1 is not efficient and will waste
> a page of memory. That should be avoided ..

Yeah, kzalloc() takes care of this magic. As said, kzalloc() vs
alloc_page() is not an existential question for this patch set :-)

I just would personally use alloc_page(). If nothing else, it does
have some super cosmetic benefits e.g., thinner call stack (when
needing to debug deep, which sometimes happens).

> 
> Jason
> 

BR, Jarkko
diff mbox series

Patch

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>
+#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;
+};
+
+static int tpm_svsm_send(struct tpm_chip *chip, u8 *buf, size_t len)
+{
+	struct tpm_svsm_priv *priv = dev_get_drvdata(&chip->dev);
+	int ret;
+
+	ret = svsm_vtpm_fill_cmd_req((struct tpm_send_cmd_req *)priv->buffer,
+				     priv->locality, buf, len);
+	if (ret)
+		return ret;
+
+	/*
+	 * The SVSM call uses the same buffer for the command and for the
+	 * response, so after this call, the buffer will contain the response
+	 * that can be used by .recv() op.
+	 */
+	return snp_svsm_vtpm_send_command(priv->buffer);
+}
+
+static int tpm_svsm_recv(struct tpm_chip *chip, u8 *buf, size_t len)
+{
+	struct tpm_svsm_priv *priv = dev_get_drvdata(&chip->dev);
+
+	/*
+	 * The internal buffer contains the response after we send the command
+	 * to SVSM.
+	 */
+	return svsm_vtpm_parse_cmd_resp((struct tpm_send_cmd_resp *)priv->buffer,
+					buf, len);
+}
+
+static void tpm_svsm_cancel(struct tpm_chip *chip)
+{
+	/* not supported */
+}
+
+static u8 tpm_svsm_status(struct tpm_chip *chip)
+{
+	return 0;
+}
+
+static bool tpm_svsm_req_canceled(struct tpm_chip *chip, u8 status)
+{
+	return false;
+}
+
+static struct tpm_class_ops tpm_chip_ops = {
+	.flags = TPM_OPS_AUTO_STARTUP,
+	.recv = tpm_svsm_recv,
+	.send = tpm_svsm_send,
+	.cancel = tpm_svsm_cancel,
+	.status = tpm_svsm_status,
+	.req_complete_mask = 0,
+	.req_complete_val = 0,
+	.req_canceled = tpm_svsm_req_canceled,
+};
+
+static int __init tpm_svsm_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct tpm_svsm_priv *priv;
+	struct tpm_chip *chip;
+	int err;
+
+	if (!snp_svsm_vtpm_probe())
+		return -ENODEV;
+
+	priv = devm_kmalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	/*
+	 * FIXME: before implementing locality we need to agree what it means
+	 * for the SNP SVSM vTPM
+	 */
+	priv->locality = 0;
+
+	chip = tpmm_chip_alloc(dev, &tpm_chip_ops);
+	if (IS_ERR(chip))
+		return PTR_ERR(chip);
+
+	dev_set_drvdata(&chip->dev, priv);
+
+	err = tpm2_probe(chip);
+	if (err)
+		return err;
+
+	err = tpm_chip_register(chip);
+	if (err)
+		return err;
+
+	dev_info(dev, "SNP SVSM vTPM %s device\n",
+		 (chip->flags & TPM_CHIP_FLAG_TPM2) ? "2.0" : "1.2");
+
+	return 0;
+}
+
+static void __exit tpm_svsm_remove(struct platform_device *pdev)
+{
+	struct tpm_chip *chip = platform_get_drvdata(pdev);
+
+	tpm_chip_unregister(chip);
+}
+
+/*
+ * tpm_svsm_remove() lives in .exit.text. For drivers registered via
+ * module_platform_driver_probe() this is ok because they cannot get unbound
+ * at runtime. So mark the driver struct with __refdata to prevent modpost
+ * triggering a section mismatch warning.
+ */
+static struct platform_driver tpm_svsm_driver __refdata = {
+	.remove = __exit_p(tpm_svsm_remove),
+	.driver = {
+		.name = "tpm-svsm",
+	},
+};
+
+module_platform_driver_probe(tpm_svsm_driver, tpm_svsm_probe);
+
+MODULE_DESCRIPTION("SNP SVSM vTPM Driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:tpm-svsm");
diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
index 0fc9a510e059..fc3f1d10d31d 100644
--- a/drivers/char/tpm/Kconfig
+++ b/drivers/char/tpm/Kconfig
@@ -225,5 +225,15 @@  config TCG_FTPM_TEE
 	help
 	  This driver proxies for firmware TPM running in TEE.
 
+config TCG_SVSM
+	tristate "SNP SVSM vTPM interface"
+	depends on AMD_MEM_ENCRYPT
+	help
+	  This is a driver for the AMD SVSM vTPM 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).  To compile this driver as a module, choose M
+	  here; the module will be called tpm_svsm.
+
 source "drivers/char/tpm/st33zp24/Kconfig"
 endif # TCG_TPM
diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
index 9bb142c75243..52d9d80a0f56 100644
--- a/drivers/char/tpm/Makefile
+++ b/drivers/char/tpm/Makefile
@@ -44,3 +44,4 @@  obj-$(CONFIG_TCG_XEN) += xen-tpmfront.o
 obj-$(CONFIG_TCG_CRB) += tpm_crb.o
 obj-$(CONFIG_TCG_VTPM_PROXY) += tpm_vtpm_proxy.o
 obj-$(CONFIG_TCG_FTPM_TEE) += tpm_ftpm_tee.o
+obj-$(CONFIG_TCG_SVSM) += tpm_svsm.o