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

[Tom Lendacky <thomas.lendacky@amd.com>]

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
>