* [RFC PATCH v2 1/6] x86/sev: add SVSM call macros for the vTPM protocol
2025-02-28 17:07 [RFC PATCH v2 0/6] Enlightened vTPM support for SVSM on SEV-SNP Stefano Garzarella
@ 2025-02-28 17:07 ` Stefano Garzarella
2025-03-10 11:08 ` Borislav Petkov
2025-02-28 17:07 ` [RFC PATCH v2 2/6] x86/sev: add SVSM vTPM probe/send_command functions Stefano Garzarella
` (5 subsequent siblings)
6 siblings, 1 reply; 45+ messages in thread
From: Stefano Garzarella @ 2025-02-28 17:07 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: Thomas Gleixner, Claudio Carvalho, Peter Huewe, x86, Dov Murik,
linux-coco, Dionna Glaze, James Bottomley, Ingo Molnar,
Joerg Roedel, Jason Gunthorpe, linux-integrity, linux-kernel,
Dave Hansen, Tom Lendacky, Borislav Petkov, H. Peter Anvin,
Stefano Garzarella
Add macros for SVSM_VTPM_QUERY and SVSM_VTPM_CMD calls as defined
in the "Secure VM Service Module for SEV-SNP Guests"
Publication # 58019 Revision: 1.00
Link: https://www.amd.com/content/dam/amd/en/documents/epyc-technical-docs/specifications/58019.pdf
Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
arch/x86/include/asm/sev.h | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
index 1581246491b5..f6ebf4492606 100644
--- a/arch/x86/include/asm/sev.h
+++ b/arch/x86/include/asm/sev.h
@@ -384,6 +384,10 @@ struct svsm_call {
#define SVSM_ATTEST_SERVICES 0
#define SVSM_ATTEST_SINGLE_SERVICE 1
+#define SVSM_VTPM_CALL(x) ((2ULL << 32) | (x))
+#define SVSM_VTPM_QUERY 0
+#define SVSM_VTPM_CMD 1
+
#ifdef CONFIG_AMD_MEM_ENCRYPT
extern u8 snp_vmpl;
--
2.48.1
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [RFC PATCH v2 1/6] x86/sev: add SVSM call macros for the vTPM protocol
2025-02-28 17:07 ` [RFC PATCH v2 1/6] x86/sev: add SVSM call macros for the vTPM protocol Stefano Garzarella
@ 2025-03-10 11:08 ` Borislav Petkov
2025-03-10 12:13 ` Stefano Garzarella
0 siblings, 1 reply; 45+ messages in thread
From: Borislav Petkov @ 2025-03-10 11:08 UTC (permalink / raw)
To: Stefano Garzarella
Cc: Jarkko Sakkinen, Thomas Gleixner, Claudio Carvalho, Peter Huewe,
x86, Dov Murik, linux-coco, Dionna Glaze, James Bottomley,
Ingo Molnar, Joerg Roedel, Jason Gunthorpe, linux-integrity,
linux-kernel, Dave Hansen, Tom Lendacky, H. Peter Anvin
On Fri, Feb 28, 2025 at 06:07:15PM +0100, Stefano Garzarella wrote:
> Add macros for SVSM_VTPM_QUERY and SVSM_VTPM_CMD calls as defined
> in the "Secure VM Service Module for SEV-SNP Guests"
> Publication # 58019 Revision: 1.00
>
> Link: https://www.amd.com/content/dam/amd/en/documents/epyc-technical-docs/specifications/58019.pdf
Those URLs are unstable - simply naming the document properly in the commit
message so that a search engine can find it is enough.
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
> arch/x86/include/asm/sev.h | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
> index 1581246491b5..f6ebf4492606 100644
> --- a/arch/x86/include/asm/sev.h
> +++ b/arch/x86/include/asm/sev.h
> @@ -384,6 +384,10 @@ struct svsm_call {
> #define SVSM_ATTEST_SERVICES 0
> #define SVSM_ATTEST_SINGLE_SERVICE 1
>
> +#define SVSM_VTPM_CALL(x) ((2ULL << 32) | (x))
> +#define SVSM_VTPM_QUERY 0
> +#define SVSM_VTPM_CMD 1
> +
> #ifdef CONFIG_AMD_MEM_ENCRYPT
>
> extern u8 snp_vmpl;
> --
Merge this patch with the patch where those are used - no need for a separate
patch.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [RFC PATCH v2 1/6] x86/sev: add SVSM call macros for the vTPM protocol
2025-03-10 11:08 ` Borislav Petkov
@ 2025-03-10 12:13 ` Stefano Garzarella
0 siblings, 0 replies; 45+ messages in thread
From: Stefano Garzarella @ 2025-03-10 12:13 UTC (permalink / raw)
To: Borislav Petkov
Cc: Jarkko Sakkinen, Thomas Gleixner, Claudio Carvalho, Peter Huewe,
x86, Dov Murik, linux-coco, Dionna Glaze, James Bottomley,
Ingo Molnar, Joerg Roedel, Jason Gunthorpe, linux-integrity,
linux-kernel, Dave Hansen, Tom Lendacky, H. Peter Anvin
On Mon, Mar 10, 2025 at 12:08:34PM +0100, Borislav Petkov wrote:
>On Fri, Feb 28, 2025 at 06:07:15PM +0100, Stefano Garzarella wrote:
>> Add macros for SVSM_VTPM_QUERY and SVSM_VTPM_CMD calls as defined
>> in the "Secure VM Service Module for SEV-SNP Guests"
>> Publication # 58019 Revision: 1.00
>>
>> Link: https://www.amd.com/content/dam/amd/en/documents/epyc-technical-docs/specifications/58019.pdf
>
>Those URLs are unstable - simply naming the document properly in the commit
>message so that a search engine can find it is enough.
Ack, I'll do it all over the place in this series (commit descriptions,
code comment blocks, etc.).
>
>> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
>> ---
>> arch/x86/include/asm/sev.h | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
>> index 1581246491b5..f6ebf4492606 100644
>> --- a/arch/x86/include/asm/sev.h
>> +++ b/arch/x86/include/asm/sev.h
>> @@ -384,6 +384,10 @@ struct svsm_call {
>> #define SVSM_ATTEST_SERVICES 0
>> #define SVSM_ATTEST_SINGLE_SERVICE 1
>>
>> +#define SVSM_VTPM_CALL(x) ((2ULL << 32) | (x))
>> +#define SVSM_VTPM_QUERY 0
>> +#define SVSM_VTPM_CMD 1
>> +
>> #ifdef CONFIG_AMD_MEM_ENCRYPT
>>
>> extern u8 snp_vmpl;
>> --
>
>Merge this patch with the patch where those are used - no need for a separate
>patch.
Yeah, it is left over from v1 when I had added this patch over James'
patches, but now I agree that it no longer makes sense since I have
reworked almost every patch in this series. I'm going to incorporate
them!
Thanks,
Stefano
^ permalink raw reply [flat|nested] 45+ messages in thread
* [RFC PATCH v2 2/6] x86/sev: add SVSM vTPM probe/send_command functions
2025-02-28 17:07 [RFC PATCH v2 0/6] Enlightened vTPM support for SVSM on SEV-SNP Stefano Garzarella
2025-02-28 17:07 ` [RFC PATCH v2 1/6] x86/sev: add SVSM call macros for the vTPM protocol Stefano Garzarella
@ 2025-02-28 17:07 ` Stefano Garzarella
2025-03-10 11:30 ` Borislav Petkov
2025-02-28 17:07 ` [RFC PATCH v2 3/6] tpm: add send_recv() ops in tpm_class_ops Stefano Garzarella
` (4 subsequent siblings)
6 siblings, 1 reply; 45+ messages in thread
From: Stefano Garzarella @ 2025-02-28 17:07 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: Thomas Gleixner, Claudio Carvalho, Peter Huewe, x86, Dov Murik,
linux-coco, Dionna Glaze, James Bottomley, Ingo Molnar,
Joerg Roedel, Jason Gunthorpe, linux-integrity, linux-kernel,
Dave Hansen, Tom Lendacky, Borislav Petkov, H. Peter Anvin,
Stefano Garzarella
Add two new functions to probe and send commands to the SVSM vTPM.
They leverage the two calls defined by the AMD SVSM specification
for the vTPM protocol: SVSM_VTPM_QUERY and SVSM_VTPM_CMD.
Expose these functions to be used by other modules such as a tpm
driver.
Co-developed-by: James Bottomley <James.Bottomley@HansenPartnership.com>
Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
Co-developed-by: Claudio Carvalho <cclaudio@linux.ibm.com>
Signed-off-by: Claudio Carvalho <cclaudio@linux.ibm.com>
Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
James, Claudio are you fine with the Cdb, Sob?
The code is pretty much similar to what was in the initial RFC, but I
changed the context for that I reset the author but added C-o-b.
Please, let me know if this is okay or if I need to do anything
else (reset the author, etc.)
---
arch/x86/include/asm/sev.h | 3 +++
arch/x86/coco/sev/core.c | 47 ++++++++++++++++++++++++++++++++++++++
2 files changed, 50 insertions(+)
diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
index f6ebf4492606..e379bcdddf07 100644
--- a/arch/x86/include/asm/sev.h
+++ b/arch/x86/include/asm/sev.h
@@ -485,6 +485,9 @@ void snp_msg_free(struct snp_msg_desc *mdesc);
int snp_send_guest_request(struct snp_msg_desc *mdesc, struct snp_guest_req *req,
struct snp_guest_request_ioctl *rio);
+bool snp_svsm_vtpm_probe(void);
+int snp_svsm_vtpm_send_command(u8 *buffer);
+
void __init snp_secure_tsc_prepare(void);
void __init snp_secure_tsc_init(void);
diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c
index 82492efc5d94..4158e447d645 100644
--- a/arch/x86/coco/sev/core.c
+++ b/arch/x86/coco/sev/core.c
@@ -2628,6 +2628,53 @@ static int snp_issue_guest_request(struct snp_guest_req *req, struct snp_req_dat
return ret;
}
+bool snp_svsm_vtpm_probe(void)
+{
+ struct svsm_call call = {};
+ u64 send_cmd_mask = 0;
+ u64 platform_cmds;
+ u64 features;
+ int ret;
+
+ /* The vTPM device is available only if we have a SVSM */
+ if (!snp_vmpl)
+ return false;
+
+ call.caa = svsm_get_caa();
+ call.rax = SVSM_VTPM_CALL(SVSM_VTPM_QUERY);
+
+ ret = svsm_perform_call_protocol(&call);
+
+ if (ret != SVSM_SUCCESS)
+ return false;
+
+ features = call.rdx_out;
+ platform_cmds = call.rcx_out;
+
+ /* No feature supported, it should be zero */
+ if (features)
+ pr_warn("SNP SVSM vTPM unsupported features: 0x%llx\n",
+ features);
+
+ /* TPM_SEND_COMMAND - platform command 8 */
+ send_cmd_mask = 1 << 8;
+
+ return (platform_cmds & send_cmd_mask) == send_cmd_mask;
+}
+EXPORT_SYMBOL_GPL(snp_svsm_vtpm_probe);
+
+int snp_svsm_vtpm_send_command(u8 *buffer)
+{
+ struct svsm_call call = {};
+
+ call.caa = svsm_get_caa();
+ call.rax = SVSM_VTPM_CALL(SVSM_VTPM_CMD);
+ call.rcx = __pa(buffer);
+
+ return svsm_perform_call_protocol(&call);
+}
+EXPORT_SYMBOL_GPL(snp_svsm_vtpm_send_command);
+
static struct platform_device sev_guest_device = {
.name = "sev-guest",
.id = -1,
--
2.48.1
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [RFC PATCH v2 2/6] x86/sev: add SVSM vTPM probe/send_command functions
2025-02-28 17:07 ` [RFC PATCH v2 2/6] x86/sev: add SVSM vTPM probe/send_command functions Stefano Garzarella
@ 2025-03-10 11:30 ` Borislav Petkov
2025-03-10 12:46 ` Stefano Garzarella
0 siblings, 1 reply; 45+ messages in thread
From: Borislav Petkov @ 2025-03-10 11:30 UTC (permalink / raw)
To: Stefano Garzarella
Cc: Jarkko Sakkinen, Thomas Gleixner, Claudio Carvalho, Peter Huewe,
x86, Dov Murik, linux-coco, Dionna Glaze, James Bottomley,
Ingo Molnar, Joerg Roedel, Jason Gunthorpe, linux-integrity,
linux-kernel, Dave Hansen, Tom Lendacky, H. Peter Anvin
On Fri, Feb 28, 2025 at 06:07:16PM +0100, Stefano Garzarella wrote:
> +bool snp_svsm_vtpm_probe(void)
> +{
> + struct svsm_call call = {};
> + u64 send_cmd_mask = 0;
> + u64 platform_cmds;
> + u64 features;
> + int ret;
> +
> + /* The vTPM device is available only if we have a SVSM */
s/if we have a SVSM/if an SVSM is present/
> + if (!snp_vmpl)
> + return false;
> +
> + call.caa = svsm_get_caa();
> + call.rax = SVSM_VTPM_CALL(SVSM_VTPM_QUERY);
> +
> + ret = svsm_perform_call_protocol(&call);
> +
^ Superfluous newline.
> + if (ret != SVSM_SUCCESS)
> + return false;
> +
> + features = call.rdx_out;
> + platform_cmds = call.rcx_out;
> +
> + /* No feature supported, it should be zero */
> + if (features)
> + pr_warn("SNP SVSM vTPM unsupported features: 0x%llx\n",
> + features);
So
return false;
here?
> +
> + /* TPM_SEND_COMMAND - platform command 8 */
> + send_cmd_mask = 1 << 8;
BIT_ULL(8);
> +
> + return (platform_cmds & send_cmd_mask) == send_cmd_mask;
> +}
> +EXPORT_SYMBOL_GPL(snp_svsm_vtpm_probe);
> +
> +int snp_svsm_vtpm_send_command(u8 *buffer)
> +{
> + struct svsm_call call = {};
> +
> + call.caa = svsm_get_caa();
> + call.rax = SVSM_VTPM_CALL(SVSM_VTPM_CMD);
> + call.rcx = __pa(buffer);
> +
> + return svsm_perform_call_protocol(&call);
> +}
In any case, you can zap all those local vars, use comments instead and slim
down the function, diff ontop:
diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c
index 3902af4b1385..6d7e97c1f567 100644
--- a/arch/x86/coco/sev/core.c
+++ b/arch/x86/coco/sev/core.c
@@ -2631,12 +2631,9 @@ static int snp_issue_guest_request(struct snp_guest_req *req, struct snp_req_dat
bool snp_svsm_vtpm_probe(void)
{
struct svsm_call call = {};
- u64 send_cmd_mask = 0;
- u64 platform_cmds;
- u64 features;
int ret;
- /* The vTPM device is available only if we have a SVSM */
+ /* The vTPM device is available only if a SVSM is present */
if (!snp_vmpl)
return false;
@@ -2644,22 +2641,17 @@ bool snp_svsm_vtpm_probe(void)
call.rax = SVSM_VTPM_CALL(SVSM_VTPM_QUERY);
ret = svsm_perform_call_protocol(&call);
-
if (ret != SVSM_SUCCESS)
return false;
- features = call.rdx_out;
- platform_cmds = call.rcx_out;
-
/* No feature supported, it should be zero */
- if (features)
- pr_warn("SNP SVSM vTPM unsupported features: 0x%llx\n",
- features);
-
- /* TPM_SEND_COMMAND - platform command 8 */
- send_cmd_mask = 1 << 8;
+ if (call.rdx_out) {
+ pr_warn("SNP SVSM vTPM unsupported features: 0x%llx\n", call.rdx_out);
+ return false;
+ }
- return (platform_cmds & send_cmd_mask) == send_cmd_mask;
+ /* Check platform commands is TPM_SEND_COMMAND - platform command 8 */
+ return (call.rcx_out & BIT_ULL(8)) == BIT_ULL(8);
}
EXPORT_SYMBOL_GPL(snp_svsm_vtpm_probe);
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [RFC PATCH v2 2/6] x86/sev: add SVSM vTPM probe/send_command functions
2025-03-10 11:30 ` Borislav Petkov
@ 2025-03-10 12:46 ` Stefano Garzarella
2025-03-10 13:27 ` Tom Lendacky
0 siblings, 1 reply; 45+ messages in thread
From: Stefano Garzarella @ 2025-03-10 12:46 UTC (permalink / raw)
To: Borislav Petkov, Tom Lendacky
Cc: Jarkko Sakkinen, Thomas Gleixner, Claudio Carvalho, Peter Huewe,
x86, Dov Murik, linux-coco, Dionna Glaze, James Bottomley,
Ingo Molnar, Joerg Roedel, Jason Gunthorpe, linux-integrity,
linux-kernel, Dave Hansen, H. Peter Anvin
On Mon, Mar 10, 2025 at 12:30:06PM +0100, Borislav Petkov wrote:
>On Fri, Feb 28, 2025 at 06:07:16PM +0100, Stefano Garzarella wrote:
>> +bool snp_svsm_vtpm_probe(void)
>> +{
>> + struct svsm_call call = {};
>> + u64 send_cmd_mask = 0;
>> + u64 platform_cmds;
>> + u64 features;
>> + int ret;
>> +
>> + /* The vTPM device is available only if we have a SVSM */
>
>s/if we have a SVSM/if an SVSM is present/
>
>> + if (!snp_vmpl)
>> + return false;
>> +
>> + call.caa = svsm_get_caa();
>> + call.rax = SVSM_VTPM_CALL(SVSM_VTPM_QUERY);
>> +
>> + ret = svsm_perform_call_protocol(&call);
>> +
>
>
>^ Superfluous newline.
>
>> + if (ret != SVSM_SUCCESS)
>> + return false;
>> +
>> + features = call.rdx_out;
>> + platform_cmds = call.rcx_out;
>> +
>> + /* No feature supported, it should be zero */
>> + if (features)
>> + pr_warn("SNP SVSM vTPM unsupported features: 0x%llx\n",
>> + features);
>
>So
>
> return false;
>
>here?
In v1 we had that, but Tom Lendacky suggested to remove it:
https://lore.kernel.org/linux-integrity/4valfkw7wtx3fpdv2qbymzggcu7mp4mhkd65j5q7zncs2dzorc@jjjevuwfchgl/
IIUC the features are supposed to be additive, so Tom's point was to
avoid that in the future SVSM will supports new features and this driver
stops working, when it could, just without using the new features.
I added a warning just to be aware of new features, but I can remove it.
>
>> +
>> + /* TPM_SEND_COMMAND - platform command 8 */
>> + send_cmd_mask = 1 << 8;
>
> BIT_ULL(8);
>
>> +
>> + return (platform_cmds & send_cmd_mask) == send_cmd_mask;
>> +}
>> +EXPORT_SYMBOL_GPL(snp_svsm_vtpm_probe);
>> +
>> +int snp_svsm_vtpm_send_command(u8 *buffer)
>> +{
>> + struct svsm_call call = {};
>> +
>> + call.caa = svsm_get_caa();
>> + call.rax = SVSM_VTPM_CALL(SVSM_VTPM_CMD);
>> + call.rcx = __pa(buffer);
>> +
>> + return svsm_perform_call_protocol(&call);
>> +}
>
>In any case, you can zap all those local vars, use comments instead and slim
>down the function, diff ontop:
Thanks for the diff, I'll apply it except, for now, the return in the
feature check which is still not clear to me (I think I get Tom's point,
but I would like confirmation from both of you).
Thanks,
Stefano
>
>diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c
>index 3902af4b1385..6d7e97c1f567 100644
>--- a/arch/x86/coco/sev/core.c
>+++ b/arch/x86/coco/sev/core.c
>@@ -2631,12 +2631,9 @@ static int snp_issue_guest_request(struct snp_guest_req *req, struct snp_req_dat
> bool snp_svsm_vtpm_probe(void)
> {
> struct svsm_call call = {};
>- u64 send_cmd_mask = 0;
>- u64 platform_cmds;
>- u64 features;
> int ret;
>
>- /* The vTPM device is available only if we have a SVSM */
>+ /* The vTPM device is available only if a SVSM is present */
> if (!snp_vmpl)
> return false;
>
>@@ -2644,22 +2641,17 @@ bool snp_svsm_vtpm_probe(void)
> call.rax = SVSM_VTPM_CALL(SVSM_VTPM_QUERY);
>
> ret = svsm_perform_call_protocol(&call);
>-
> if (ret != SVSM_SUCCESS)
> return false;
>
>- features = call.rdx_out;
>- platform_cmds = call.rcx_out;
>-
> /* No feature supported, it should be zero */
>- if (features)
>- pr_warn("SNP SVSM vTPM unsupported features: 0x%llx\n",
>- features);
>-
>- /* TPM_SEND_COMMAND - platform command 8 */
>- send_cmd_mask = 1 << 8;
>+ if (call.rdx_out) {
>+ pr_warn("SNP SVSM vTPM unsupported features: 0x%llx\n", call.rdx_out);
>+ return false;
>+ }
>
>- return (platform_cmds & send_cmd_mask) == send_cmd_mask;
>+ /* Check platform commands is TPM_SEND_COMMAND - platform command 8 */
>+ return (call.rcx_out & BIT_ULL(8)) == BIT_ULL(8);
> }
> EXPORT_SYMBOL_GPL(snp_svsm_vtpm_probe);
>
>
>--
>Regards/Gruss,
> Boris.
>
>https://people.kernel.org/tglx/notes-about-netiquette
>
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [RFC PATCH v2 2/6] x86/sev: add SVSM vTPM probe/send_command functions
2025-03-10 12:46 ` Stefano Garzarella
@ 2025-03-10 13:27 ` Tom Lendacky
2025-03-10 13:51 ` Borislav Petkov
0 siblings, 1 reply; 45+ messages in thread
From: Tom Lendacky @ 2025-03-10 13:27 UTC (permalink / raw)
To: Stefano Garzarella, Borislav Petkov
Cc: Jarkko Sakkinen, Thomas Gleixner, Claudio Carvalho, Peter Huewe,
x86, Dov Murik, linux-coco, Dionna Glaze, James Bottomley,
Ingo Molnar, Joerg Roedel, Jason Gunthorpe, linux-integrity,
linux-kernel, Dave Hansen, H. Peter Anvin
On 3/10/25 07:46, Stefano Garzarella wrote:
> On Mon, Mar 10, 2025 at 12:30:06PM +0100, Borislav Petkov wrote:
>> On Fri, Feb 28, 2025 at 06:07:16PM +0100, Stefano Garzarella wrote:
>>> +bool snp_svsm_vtpm_probe(void)
>>> +{
>>> + struct svsm_call call = {};
>>> + u64 send_cmd_mask = 0;
>>> + u64 platform_cmds;
>>> + u64 features;
>>> + int ret;
>>> +
>>> + /* The vTPM device is available only if we have a SVSM */
>>
>> s/if we have a SVSM/if an SVSM is present/
>>
>>> + if (!snp_vmpl)
>>> + return false;
>>> +
>>> + call.caa = svsm_get_caa();
>>> + call.rax = SVSM_VTPM_CALL(SVSM_VTPM_QUERY);
>>> +
>>> + ret = svsm_perform_call_protocol(&call);
>>> +
>>
>>
>> ^ Superfluous newline.
>>
>>> + if (ret != SVSM_SUCCESS)
>>> + return false;
>>> +
>>> + features = call.rdx_out;
>>> + platform_cmds = call.rcx_out;
>>> +
>>> + /* No feature supported, it should be zero */
>>> + if (features)
>>> + pr_warn("SNP SVSM vTPM unsupported features: 0x%llx\n",
>>> + features);
>>
>> So
>>
>> return false;
>>
>> here?
>
> In v1 we had that, but Tom Lendacky suggested to remove it:
> https://lore.kernel.org/linux-integrity/4valfkw7wtx3fpdv2qbymzggcu7mp4mhkd65j5q7zncs2dzorc@jjjevuwfchgl/
>
> IIUC the features are supposed to be additive, so Tom's point was to
> avoid that in the future SVSM will supports new features and this driver
> stops working, when it could, just without using the new features.
>
> I added a warning just to be aware of new features, but I can remove it.
I don't think anything needs to be checked or printed. If you want to do
anything, just issue a pr_info() with the features value (and maybe the
platform_cmds value, too). Issuing a pr_warn() here would be like
issuing a pr_warn() for a new CPUID value that the current kernel
doesn't know about.
Thanks,
Tom
>
>>
>>> +
>>> + /* TPM_SEND_COMMAND - platform command 8 */
>>> + send_cmd_mask = 1 << 8;
>>
>> BIT_ULL(8);
>>
>>> +
>>> + return (platform_cmds & send_cmd_mask) == send_cmd_mask;
>>> +}
>>> +EXPORT_SYMBOL_GPL(snp_svsm_vtpm_probe);
>>> +
>>> +int snp_svsm_vtpm_send_command(u8 *buffer)
>>> +{
>>> + struct svsm_call call = {};
>>> +
>>> + call.caa = svsm_get_caa();
>>> + call.rax = SVSM_VTPM_CALL(SVSM_VTPM_CMD);
>>> + call.rcx = __pa(buffer);
>>> +
>>> + return svsm_perform_call_protocol(&call);
>>> +}
>>
>> In any case, you can zap all those local vars, use comments instead
>> and slim
>> down the function, diff ontop:
>
> Thanks for the diff, I'll apply it except, for now, the return in the
> feature check which is still not clear to me (I think I get Tom's point,
> but I would like confirmation from both of you).
>
> Thanks,
> Stefano
>
>>
>> diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c
>> index 3902af4b1385..6d7e97c1f567 100644
>> --- a/arch/x86/coco/sev/core.c
>> +++ b/arch/x86/coco/sev/core.c
>> @@ -2631,12 +2631,9 @@ static int snp_issue_guest_request(struct
>> snp_guest_req *req, struct snp_req_dat
>> bool snp_svsm_vtpm_probe(void)
>> {
>> struct svsm_call call = {};
>> - u64 send_cmd_mask = 0;
>> - u64 platform_cmds;
>> - u64 features;
>> int ret;
>>
>> - /* The vTPM device is available only if we have a SVSM */
>> + /* The vTPM device is available only if a SVSM is present */
>> if (!snp_vmpl)
>> return false;
>>
>> @@ -2644,22 +2641,17 @@ bool snp_svsm_vtpm_probe(void)
>> call.rax = SVSM_VTPM_CALL(SVSM_VTPM_QUERY);
>>
>> ret = svsm_perform_call_protocol(&call);
>> -
>> if (ret != SVSM_SUCCESS)
>> return false;
>>
>> - features = call.rdx_out;
>> - platform_cmds = call.rcx_out;
>> -
>> /* No feature supported, it should be zero */
>> - if (features)
>> - pr_warn("SNP SVSM vTPM unsupported features: 0x%llx\n",
>> - features);
>> -
>> - /* TPM_SEND_COMMAND - platform command 8 */
>> - send_cmd_mask = 1 << 8;
>> + if (call.rdx_out) {
>> + pr_warn("SNP SVSM vTPM unsupported features: 0x%llx\n",
>> call.rdx_out);
>> + return false;
>> + }
>>
>> - return (platform_cmds & send_cmd_mask) == send_cmd_mask;
>> + /* Check platform commands is TPM_SEND_COMMAND - platform command
>> 8 */
>> + return (call.rcx_out & BIT_ULL(8)) == BIT_ULL(8);
>> }
>> EXPORT_SYMBOL_GPL(snp_svsm_vtpm_probe);
>>
>>
>> --
>> Regards/Gruss,
>> Boris.
>>
>> https://people.kernel.org/tglx/notes-about-netiquette
>>
>
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [RFC PATCH v2 2/6] x86/sev: add SVSM vTPM probe/send_command functions
2025-03-10 13:27 ` Tom Lendacky
@ 2025-03-10 13:51 ` Borislav Petkov
2025-03-10 13:56 ` Tom Lendacky
2025-03-10 13:59 ` Stefano Garzarella
0 siblings, 2 replies; 45+ messages in thread
From: Borislav Petkov @ 2025-03-10 13:51 UTC (permalink / raw)
To: Tom Lendacky
Cc: Stefano Garzarella, Jarkko Sakkinen, Thomas Gleixner,
Claudio Carvalho, Peter Huewe, x86, Dov Murik, linux-coco,
Dionna Glaze, James Bottomley, Ingo Molnar, Joerg Roedel,
Jason Gunthorpe, linux-integrity, linux-kernel, Dave Hansen,
H. Peter Anvin
On Mon, Mar 10, 2025 at 08:27:37AM -0500, Tom Lendacky wrote:
> I don't think anything needs to be checked or printed.
Yes.
> If you want to do anything, just issue a pr_info() with the features value
> (and maybe the platform_cmds value, too). Issuing a pr_warn() here would be
> like issuing a pr_warn() for a new CPUID value that the current kernel
> doesn't know about.
I still don't get the need to print anything. Why?
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [RFC PATCH v2 2/6] x86/sev: add SVSM vTPM probe/send_command functions
2025-03-10 13:51 ` Borislav Petkov
@ 2025-03-10 13:56 ` Tom Lendacky
2025-03-10 14:02 ` Borislav Petkov
2025-03-10 13:59 ` Stefano Garzarella
1 sibling, 1 reply; 45+ messages in thread
From: Tom Lendacky @ 2025-03-10 13:56 UTC (permalink / raw)
To: Borislav Petkov
Cc: Stefano Garzarella, Jarkko Sakkinen, Thomas Gleixner,
Claudio Carvalho, Peter Huewe, x86, Dov Murik, linux-coco,
Dionna Glaze, James Bottomley, Ingo Molnar, Joerg Roedel,
Jason Gunthorpe, linux-integrity, linux-kernel, Dave Hansen,
H. Peter Anvin
On 3/10/25 08:51, Borislav Petkov wrote:
> On Mon, Mar 10, 2025 at 08:27:37AM -0500, Tom Lendacky wrote:
>> I don't think anything needs to be checked or printed.
>
> Yes.
>
>> If you want to do anything, just issue a pr_info() with the features value
>> (and maybe the platform_cmds value, too). Issuing a pr_warn() here would be
>> like issuing a pr_warn() for a new CPUID value that the current kernel
>> doesn't know about.
>
> I still don't get the need to print anything. Why?
It isn't needed. It's similar to "device" information/capabilities.
Maybe pr_debug() then? But I'm also fine with not printing anything.
Thanks,
Tom
>
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [RFC PATCH v2 2/6] x86/sev: add SVSM vTPM probe/send_command functions
2025-03-10 13:56 ` Tom Lendacky
@ 2025-03-10 14:02 ` Borislav Petkov
0 siblings, 0 replies; 45+ messages in thread
From: Borislav Petkov @ 2025-03-10 14:02 UTC (permalink / raw)
To: Tom Lendacky
Cc: Stefano Garzarella, Jarkko Sakkinen, Thomas Gleixner,
Claudio Carvalho, Peter Huewe, x86, Dov Murik, linux-coco,
Dionna Glaze, James Bottomley, Ingo Molnar, Joerg Roedel,
Jason Gunthorpe, linux-integrity, linux-kernel, Dave Hansen,
H. Peter Anvin
On Mon, Mar 10, 2025 at 08:56:53AM -0500, Tom Lendacky wrote:
> It isn't needed. It's similar to "device" information/capabilities.
> Maybe pr_debug() then? But I'm also fine with not printing anything.
Yap, nothing it is then.
If the need arises, then we can debate :)
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [RFC PATCH v2 2/6] x86/sev: add SVSM vTPM probe/send_command functions
2025-03-10 13:51 ` Borislav Petkov
2025-03-10 13:56 ` Tom Lendacky
@ 2025-03-10 13:59 ` Stefano Garzarella
2025-03-10 14:04 ` Borislav Petkov
1 sibling, 1 reply; 45+ messages in thread
From: Stefano Garzarella @ 2025-03-10 13:59 UTC (permalink / raw)
To: Borislav Petkov, Tom Lendacky
Cc: Jarkko Sakkinen, Thomas Gleixner, Claudio Carvalho, Peter Huewe,
x86, Dov Murik, linux-coco, Dionna Glaze, James Bottomley,
Ingo Molnar, Joerg Roedel, Jason Gunthorpe, linux-integrity,
linux-kernel, Dave Hansen, H. Peter Anvin
On Mon, Mar 10, 2025 at 02:51:33PM +0100, Borislav Petkov wrote:
>On Mon, Mar 10, 2025 at 08:27:37AM -0500, Tom Lendacky wrote:
>> I don't think anything needs to be checked or printed.
>
>Yes.
Ack, I removed the check and the print.
@Boris I also removed `ret` to continue the slimming, so the end
result should be this:
bool snp_svsm_vtpm_probe(void)
{
struct svsm_call call = {};
/* The vTPM device is available only if a SVSM is present */
if (!snp_vmpl)
return false;
call.caa = svsm_get_caa();
call.rax = SVSM_VTPM_CALL(SVSM_VTPM_QUERY);
if (svsm_perform_call_protocol(&call))
return false;
/* Check platform commands contains TPM_SEND_COMMAND - platform command 8 */
return (call.rcx_out & BIT_ULL(8)) == BIT_ULL(8);
}
Quite nice, thanks for the review!
Stefano
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [RFC PATCH v2 2/6] x86/sev: add SVSM vTPM probe/send_command functions
2025-03-10 13:59 ` Stefano Garzarella
@ 2025-03-10 14:04 ` Borislav Petkov
0 siblings, 0 replies; 45+ messages in thread
From: Borislav Petkov @ 2025-03-10 14:04 UTC (permalink / raw)
To: Stefano Garzarella
Cc: Tom Lendacky, Jarkko Sakkinen, Thomas Gleixner, Claudio Carvalho,
Peter Huewe, x86, Dov Murik, linux-coco, Dionna Glaze,
James Bottomley, Ingo Molnar, Joerg Roedel, Jason Gunthorpe,
linux-integrity, linux-kernel, Dave Hansen, H. Peter Anvin
On Mon, Mar 10, 2025 at 02:59:44PM +0100, Stefano Garzarella wrote:
> On Mon, Mar 10, 2025 at 02:51:33PM +0100, Borislav Petkov wrote:
> > On Mon, Mar 10, 2025 at 08:27:37AM -0500, Tom Lendacky wrote:
> > > I don't think anything needs to be checked or printed.
> >
> > Yes.
>
> Ack, I removed the check and the print.
>
> @Boris I also removed `ret` to continue the slimming, so the end
> result should be this:
>
> bool snp_svsm_vtpm_probe(void)
> {
> struct svsm_call call = {};
>
> /* The vTPM device is available only if a SVSM is present */
> if (!snp_vmpl)
> return false;
>
> call.caa = svsm_get_caa();
> call.rax = SVSM_VTPM_CALL(SVSM_VTPM_QUERY);
>
> if (svsm_perform_call_protocol(&call))
> return false;
>
> /* Check platform commands contains TPM_SEND_COMMAND - platform command 8 */
> return (call.rcx_out & BIT_ULL(8)) == BIT_ULL(8);
> }
>
> Quite nice, thanks for the review!
Thanks, looks clean to me. :)
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
^ permalink raw reply [flat|nested] 45+ messages in thread
* [RFC PATCH v2 3/6] tpm: add send_recv() ops in tpm_class_ops
2025-02-28 17:07 [RFC PATCH v2 0/6] Enlightened vTPM support for SVSM on SEV-SNP Stefano Garzarella
2025-02-28 17:07 ` [RFC PATCH v2 1/6] x86/sev: add SVSM call macros for the vTPM protocol Stefano Garzarella
2025-02-28 17:07 ` [RFC PATCH v2 2/6] x86/sev: add SVSM vTPM probe/send_command functions Stefano Garzarella
@ 2025-02-28 17:07 ` Stefano Garzarella
2025-03-01 1:45 ` Jarkko Sakkinen
2025-03-03 14:06 ` Tom Lendacky
2025-02-28 17:07 ` [RFC PATCH v2 4/6] tpm: add interface to interact with devices based on TCG Simulator Stefano Garzarella
` (3 subsequent siblings)
6 siblings, 2 replies; 45+ messages in thread
From: Stefano Garzarella @ 2025-02-28 17:07 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: Thomas Gleixner, Claudio Carvalho, Peter Huewe, x86, Dov Murik,
linux-coco, Dionna Glaze, James Bottomley, Ingo Molnar,
Joerg Roedel, Jason Gunthorpe, linux-integrity, linux-kernel,
Dave Hansen, Tom Lendacky, Borislav Petkov, H. Peter Anvin,
Stefano Garzarella
Some devices do not support interrupts and provide a single operation
to send the command and receive the response on the same buffer.
To support this scenario, a driver could set TPM_CHIP_FLAG_IRQ in the
chip's flags to get recv() to be called immediately after send() in
tpm_try_transmit().
Instead of abusing TPM_CHIP_FLAG_IRQ, introduce a new callback
send_recv(). If that callback is defined, it is called in
tpm_try_transmit() to send the command and receive the response on
the same buffer in a single call.
Suggested-by: Jason Gunthorpe <jgg@ziepe.ca>
Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
include/linux/tpm.h | 2 ++
drivers/char/tpm/tpm-interface.c | 8 +++++++-
2 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index 20a40ade8030..2ede8e0592d3 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -88,6 +88,8 @@ struct tpm_class_ops {
bool (*req_canceled)(struct tpm_chip *chip, u8 status);
int (*recv) (struct tpm_chip *chip, u8 *buf, size_t len);
int (*send) (struct tpm_chip *chip, u8 *buf, size_t len);
+ int (*send_recv)(struct tpm_chip *chip, u8 *buf, size_t buf_len,
+ size_t to_send);
void (*cancel) (struct tpm_chip *chip);
u8 (*status) (struct tpm_chip *chip);
void (*update_timeouts)(struct tpm_chip *chip,
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index b1daa0d7b341..4f92b0477696 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -82,6 +82,9 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip, void *buf, size_t bufsiz)
return -E2BIG;
}
+ if (chip->ops->send_recv)
+ goto out_recv;
+
rc = chip->ops->send(chip, buf, count);
if (rc < 0) {
if (rc != -EPIPE)
@@ -123,7 +126,10 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip, void *buf, size_t bufsiz)
return -ETIME;
out_recv:
- len = chip->ops->recv(chip, buf, bufsiz);
+ if (chip->ops->send_recv)
+ len = chip->ops->send_recv(chip, buf, bufsiz, count);
+ else
+ len = chip->ops->recv(chip, buf, bufsiz);
if (len < 0) {
rc = len;
dev_err(&chip->dev, "tpm_transmit: tpm_recv: error %d\n", rc);
--
2.48.1
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [RFC PATCH v2 3/6] tpm: add send_recv() ops in tpm_class_ops
2025-02-28 17:07 ` [RFC PATCH v2 3/6] tpm: add send_recv() ops in tpm_class_ops Stefano Garzarella
@ 2025-03-01 1:45 ` Jarkko Sakkinen
2025-03-03 16:21 ` Stefano Garzarella
2025-03-03 14:06 ` Tom Lendacky
1 sibling, 1 reply; 45+ messages in thread
From: Jarkko Sakkinen @ 2025-03-01 1:45 UTC (permalink / raw)
To: Stefano Garzarella
Cc: Thomas Gleixner, Claudio Carvalho, Peter Huewe, x86, Dov Murik,
linux-coco, Dionna Glaze, James Bottomley, Ingo Molnar,
Joerg Roedel, Jason Gunthorpe, linux-integrity, linux-kernel,
Dave Hansen, Tom Lendacky, Borislav Petkov, H. Peter Anvin
On Fri, Feb 28, 2025 at 06:07:17PM +0100, Stefano Garzarella wrote:
> + int (*send_recv)(struct tpm_chip *chip, u8 *buf, size_t buf_len,
> + size_t to_send);
Please describe the meaning and purpose of to_send.
BR, Jarkko
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [RFC PATCH v2 3/6] tpm: add send_recv() ops in tpm_class_ops
2025-03-01 1:45 ` Jarkko Sakkinen
@ 2025-03-03 16:21 ` Stefano Garzarella
2025-03-04 16:56 ` Jarkko Sakkinen
0 siblings, 1 reply; 45+ messages in thread
From: Stefano Garzarella @ 2025-03-03 16:21 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: Thomas Gleixner, Claudio Carvalho, Peter Huewe, x86, Dov Murik,
linux-coco, Dionna Glaze, James Bottomley, Ingo Molnar,
Joerg Roedel, Jason Gunthorpe, linux-integrity, linux-kernel,
Dave Hansen, Tom Lendacky, Borislav Petkov, H. Peter Anvin
On Sat, Mar 01, 2025 at 03:45:10AM +0200, Jarkko Sakkinen wrote:
>On Fri, Feb 28, 2025 at 06:07:17PM +0100, Stefano Garzarella wrote:
>> + int (*send_recv)(struct tpm_chip *chip, u8 *buf, size_t buf_len,
>> + size_t to_send);
>
>Please describe the meaning and purpose of to_send.
Sure, I'll add in the commit description.
Should I add documentation in the code as well?
The other callbacks don't have that, but if you think it's useful we can
start with that, I mean something like this:
/**
* send_recv() - send the command and receive the response on the same
* buffer in a single call.
*
* @chip: The TPM chip
* @buf: A buffer used to both send the command and receive the response
* @buf_len: The size of the buffer
* @to_send: Number of bytes in the buffer to send
*
* Return: number of received bytes on success, negative error code on
* failure.
*/
int (*send_recv)(struct tpm_chip *chip, u8 *buf, size_t buf_len,
size_t to_send);
Thanks,
Stefano
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [RFC PATCH v2 3/6] tpm: add send_recv() ops in tpm_class_ops
2025-03-03 16:21 ` Stefano Garzarella
@ 2025-03-04 16:56 ` Jarkko Sakkinen
2025-03-04 20:21 ` Jarkko Sakkinen
0 siblings, 1 reply; 45+ messages in thread
From: Jarkko Sakkinen @ 2025-03-04 16:56 UTC (permalink / raw)
To: Stefano Garzarella
Cc: Thomas Gleixner, Claudio Carvalho, Peter Huewe, x86, Dov Murik,
linux-coco, Dionna Glaze, James Bottomley, Ingo Molnar,
Joerg Roedel, Jason Gunthorpe, linux-integrity, linux-kernel,
Dave Hansen, Tom Lendacky, Borislav Petkov, H. Peter Anvin
On Mon, 2025-03-03 at 17:21 +0100, Stefano Garzarella wrote:
> On Sat, Mar 01, 2025 at 03:45:10AM +0200, Jarkko Sakkinen wrote:
> > On Fri, Feb 28, 2025 at 06:07:17PM +0100, Stefano Garzarella wrote:
> > > + int (*send_recv)(struct tpm_chip *chip, u8 *buf, size_t
> > > buf_len,
> > > + size_t to_send);
> >
> > Please describe the meaning and purpose of to_send.
>
> Sure, I'll add in the commit description.
It's always a command, right? So better be more concerete than
"to_send", e.g. "cmd_len".
I'd do instead:
if (!chip->send)
goto out_recv;
And change recv into:
int (*recv)(struct tpm_chip *chip, u8 *buf, size_t buf_len,
cmd_len);
Those who don't need the last parameter, can ignore it.
This also reduces meaningless possible states for the ops structure
such as "send_recv and send or recv defined", i.e. makes it overall
more mutually exclusive.
>
> Should I add documentation in the code as well?
>
> The other callbacks don't have that, but if you think it's useful we
> can
> start with that, I mean something like this:
>
> /**
> * send_recv() - send the command and receive the response
> on the same
> * buffer in a single call.
> *
> * @chip: The TPM chip
> * @buf: A buffer used to both send the command and receive
> the response
> * @buf_len: The size of the buffer
> * @to_send: Number of bytes in the buffer to send
> *
> * Return: number of received bytes on success, negative
> error code on
> * failure.
> */
> int (*send_recv)(struct tpm_chip *chip, u8 *buf, size_t
> buf_len,
> size_t to_send);
I would not document in callback level as their implementation is not global.
This is probably stance also taken by file_operations, vm_ops and many other
places with "ops" structure.
>
> Thanks,
> Stefano
>
>
BR, Jarkko
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [RFC PATCH v2 3/6] tpm: add send_recv() ops in tpm_class_ops
2025-03-04 16:56 ` Jarkko Sakkinen
@ 2025-03-04 20:21 ` Jarkko Sakkinen
2025-03-05 9:04 ` Stefano Garzarella
0 siblings, 1 reply; 45+ messages in thread
From: Jarkko Sakkinen @ 2025-03-04 20:21 UTC (permalink / raw)
To: Stefano Garzarella
Cc: Thomas Gleixner, Claudio Carvalho, Peter Huewe, x86, Dov Murik,
linux-coco, Dionna Glaze, James Bottomley, Ingo Molnar,
Joerg Roedel, Jason Gunthorpe, linux-integrity, linux-kernel,
Dave Hansen, Tom Lendacky, Borislav Petkov, H. Peter Anvin
On Tue, Mar 04, 2025 at 06:56:02PM +0200, Jarkko Sakkinen wrote:
> On Mon, 2025-03-03 at 17:21 +0100, Stefano Garzarella wrote:
> > On Sat, Mar 01, 2025 at 03:45:10AM +0200, Jarkko Sakkinen wrote:
> > > On Fri, Feb 28, 2025 at 06:07:17PM +0100, Stefano Garzarella wrote:
> > > > + int (*send_recv)(struct tpm_chip *chip, u8 *buf, size_t
> > > > buf_len,
> > > > + size_t to_send);
> > >
> > > Please describe the meaning and purpose of to_send.
> >
> > Sure, I'll add in the commit description.
>
> It's always a command, right? So better be more concerete than
> "to_send", e.g. "cmd_len".
>
> I'd do instead:
>
> if (!chip->send)
> goto out_recv;
>
> And change recv into:
>
> int (*recv)(struct tpm_chip *chip, u8 *buf, size_t buf_len,
> cmd_len);
I think I went here over the top, and *if* we need a new callback
putting send_recv would be fine. Only thing I'd take from this is to
rename to_len as cmd_len.
However, I don't think there are strong enough reasons to add complexity
to the callback interface with the basis of this single driver. You
should deal with this internally inside the driver instead.
So do something along the lines of, e.g.:
1. Create dummy send() copying the command to internal
buffer.
2. Create ->status() returning zero, and set req_complete_mask and
req_complete_val to zero.
3. Performan transaction in recv().
How you split send_recv() between send() and recv() is up to you. This
was merely showing that we don't need send_recv() desperately.
BR, Jarkko
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [RFC PATCH v2 3/6] tpm: add send_recv() ops in tpm_class_ops
2025-03-04 20:21 ` Jarkko Sakkinen
@ 2025-03-05 9:04 ` Stefano Garzarella
2025-03-05 19:02 ` Jason Gunthorpe
2025-03-06 22:15 ` Jarkko Sakkinen
0 siblings, 2 replies; 45+ messages in thread
From: Stefano Garzarella @ 2025-03-05 9:04 UTC (permalink / raw)
To: Jarkko Sakkinen, Jason Gunthorpe
Cc: Thomas Gleixner, Claudio Carvalho, Peter Huewe, x86, Dov Murik,
linux-coco, Dionna Glaze, James Bottomley, Ingo Molnar,
Joerg Roedel, Jason Gunthorpe, linux-integrity, linux-kernel,
Dave Hansen, Tom Lendacky, Borislav Petkov, H. Peter Anvin
On Tue, Mar 04, 2025 at 10:21:55PM +0200, Jarkko Sakkinen wrote:
>On Tue, Mar 04, 2025 at 06:56:02PM +0200, Jarkko Sakkinen wrote:
>> On Mon, 2025-03-03 at 17:21 +0100, Stefano Garzarella wrote:
>> > On Sat, Mar 01, 2025 at 03:45:10AM +0200, Jarkko Sakkinen wrote:
>> > > On Fri, Feb 28, 2025 at 06:07:17PM +0100, Stefano Garzarella wrote:
>> > > > + int (*send_recv)(struct tpm_chip *chip, u8 *buf, size_t
>> > > > buf_len,
>> > > > + size_t to_send);
>> > >
>> > > Please describe the meaning and purpose of to_send.
>> >
>> > Sure, I'll add in the commit description.
>>
>> It's always a command, right? So better be more concerete than
>> "to_send", e.g. "cmd_len".
Right!
>>
>> I'd do instead:
>>
>> if (!chip->send)
>> goto out_recv;
>>
>> And change recv into:
>>
>> int (*recv)(struct tpm_chip *chip, u8 *buf, size_t buf_len,
>> cmd_len);
>
>I think I went here over the top, and *if* we need a new callback
>putting send_recv would be fine. Only thing I'd take from this is to
>rename to_len as cmd_len.
Got it.
>
>However, I don't think there are strong enough reasons to add complexity
>to the callback interface with the basis of this single driver. You
>should deal with this internally inside the driver instead.
>
>So do something along the lines of, e.g.:
>
>1. Create dummy send() copying the command to internal
> buffer.
>2. Create ->status() returning zero, and set req_complete_mask and
> req_complete_val to zero.
>3. Performan transaction in recv().
>
>How you split send_recv() between send() and recv() is up to you. This
>was merely showing that we don't need send_recv() desperately.
We did something similar in v1 [1], but instead of your point 2, we just
set `chip->flags |= TPM_CHIP_FLAG_IRQ;` in the probe() after we
allocated the chip.
Jason suggested the send_recv() ops [2], which I liked, but if you
prefer to avoid that, I can restore what we did in v1 and replace the
TPM_CHIP_FLAG_IRQ hack with your point 2 (or use TPM_CHIP_FLAG_IRQ if
you think it is fine).
@Jarkko, @Jason, I don't have a strong preference about it, so your
choice :-)
Thanks,
Stefano
[1] https://lore.kernel.org/linux-integrity/20241210143423.101774-2-sgarzare@redhat.com/
[2] https://lore.kernel.org/linux-integrity/CAGxU2F51EoqDqi6By6eBa7qT+VT006DJ9+V-PANQ6GQrwVWt_Q@mail.gmail.com/
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [RFC PATCH v2 3/6] tpm: add send_recv() ops in tpm_class_ops
2025-03-05 9:04 ` Stefano Garzarella
@ 2025-03-05 19:02 ` Jason Gunthorpe
2025-03-06 21:52 ` Jarkko Sakkinen
2025-03-06 22:15 ` Jarkko Sakkinen
1 sibling, 1 reply; 45+ messages in thread
From: Jason Gunthorpe @ 2025-03-05 19:02 UTC (permalink / raw)
To: Stefano Garzarella
Cc: Jarkko Sakkinen, Thomas Gleixner, Claudio Carvalho, Peter Huewe,
x86, Dov Murik, linux-coco, Dionna Glaze, James Bottomley,
Ingo Molnar, Joerg Roedel, linux-integrity, linux-kernel,
Dave Hansen, Tom Lendacky, Borislav Petkov, H. Peter Anvin
On Wed, Mar 05, 2025 at 10:04:25AM +0100, Stefano Garzarella wrote:
> Jason suggested the send_recv() ops [2], which I liked, but if you prefer to
> avoid that, I can restore what we did in v1 and replace the
> TPM_CHIP_FLAG_IRQ hack with your point 2 (or use TPM_CHIP_FLAG_IRQ if you
> think it is fine).
I think it is a pretty notable simplification for the driver as it
does not need to implement send, status, req_canceled and more ops.
Given the small LOC on the core side I'd call that simplification a
win..
Jason
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [RFC PATCH v2 3/6] tpm: add send_recv() ops in tpm_class_ops
2025-03-05 19:02 ` Jason Gunthorpe
@ 2025-03-06 21:52 ` Jarkko Sakkinen
2025-03-07 15:37 ` Stefano Garzarella
0 siblings, 1 reply; 45+ messages in thread
From: Jarkko Sakkinen @ 2025-03-06 21:52 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Stefano Garzarella, Thomas Gleixner, Claudio Carvalho,
Peter Huewe, x86, Dov Murik, linux-coco, Dionna Glaze,
James Bottomley, Ingo Molnar, Joerg Roedel, linux-integrity,
linux-kernel, Dave Hansen, Tom Lendacky, Borislav Petkov,
H. Peter Anvin
On Wed, Mar 05, 2025 at 03:02:29PM -0400, Jason Gunthorpe wrote:
> On Wed, Mar 05, 2025 at 10:04:25AM +0100, Stefano Garzarella wrote:
> > Jason suggested the send_recv() ops [2], which I liked, but if you prefer to
> > avoid that, I can restore what we did in v1 and replace the
> > TPM_CHIP_FLAG_IRQ hack with your point 2 (or use TPM_CHIP_FLAG_IRQ if you
> > think it is fine).
>
> I think it is a pretty notable simplification for the driver as it
> does not need to implement send, status, req_canceled and more ops.
>
> Given the small LOC on the core side I'd call that simplification a
> win..
I'm sorry to disagree with you on this but adding a callback for
one leaf driver is not what I would call "a win" :-)
I mean, it's either a minor twist in
1. "the framework code" which affects in a way all other leaf drivers.
At bare minimum it adds a tiny bit of complexity to the callback
interface and a tiny bit of accumulated maintenance cost.
2. in the leaf driver
So I'd really would want to keep that tiny bit of extra complexity
localized.
>
> Jason
BR, Jarkko
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [RFC PATCH v2 3/6] tpm: add send_recv() ops in tpm_class_ops
2025-03-06 21:52 ` Jarkko Sakkinen
@ 2025-03-07 15:37 ` Stefano Garzarella
2025-03-07 16:32 ` Jarkko Sakkinen
0 siblings, 1 reply; 45+ messages in thread
From: Stefano Garzarella @ 2025-03-07 15:37 UTC (permalink / raw)
To: Jarkko Sakkinen, Jason Gunthorpe
Cc: Thomas Gleixner, Claudio Carvalho, Peter Huewe, x86, Dov Murik,
linux-coco, Dionna Glaze, James Bottomley, Ingo Molnar,
Joerg Roedel, linux-integrity, linux-kernel, Dave Hansen,
Tom Lendacky, Borislav Petkov, H. Peter Anvin
On Thu, Mar 06, 2025 at 11:52:46PM +0200, Jarkko Sakkinen wrote:
>On Wed, Mar 05, 2025 at 03:02:29PM -0400, Jason Gunthorpe wrote:
>> On Wed, Mar 05, 2025 at 10:04:25AM +0100, Stefano Garzarella wrote:
>> > Jason suggested the send_recv() ops [2], which I liked, but if you prefer to
>> > avoid that, I can restore what we did in v1 and replace the
>> > TPM_CHIP_FLAG_IRQ hack with your point 2 (or use TPM_CHIP_FLAG_IRQ if you
>> > think it is fine).
>>
>> I think it is a pretty notable simplification for the driver as it
>> does not need to implement send, status, req_canceled and more ops.
>>
>> Given the small LOC on the core side I'd call that simplification a
>> win..
>
>I'm sorry to disagree with you on this but adding a callback for
>one leaf driver is not what I would call "a win" :-)
IIUC in the ftpm driver (tpm_ftpm_tee.c) we could also use send_recv()
and save a memcpy() to a temporally buffer (pvt_data->resp_buf) and also
that 4k buffer allocated with the private data of the driver.
BTW if you agree, for now I'll do something similar of what we do in the
ftpm driver (which would be what Jarkko recommended - status() returns
0, .req_complete_mask = 0, .req_complete_val = 0) and we can discuss
send_recv() in a new series where I can include changes for the ftpm
driver too, to see whether it makes sense or not.
WDYT?
Thanks,
Stefano
>
>I mean, it's either a minor twist in
>
>1. "the framework code" which affects in a way all other leaf drivers.
> At bare minimum it adds a tiny bit of complexity to the callback
> interface and a tiny bit of accumulated maintenance cost.
>2. in the leaf driver
>
>So I'd really would want to keep that tiny bit of extra complexity
>localized.
>
>>
>> Jason
>
>BR, Jarkko
>
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [RFC PATCH v2 3/6] tpm: add send_recv() ops in tpm_class_ops
2025-03-07 15:37 ` Stefano Garzarella
@ 2025-03-07 16:32 ` Jarkko Sakkinen
0 siblings, 0 replies; 45+ messages in thread
From: Jarkko Sakkinen @ 2025-03-07 16:32 UTC (permalink / raw)
To: Stefano Garzarella
Cc: Jason Gunthorpe, Thomas Gleixner, Claudio Carvalho, Peter Huewe,
x86, Dov Murik, linux-coco, Dionna Glaze, James Bottomley,
Ingo Molnar, Joerg Roedel, linux-integrity, linux-kernel,
Dave Hansen, Tom Lendacky, Borislav Petkov, H. Peter Anvin
On Fri, Mar 07, 2025 at 04:37:12PM +0100, Stefano Garzarella wrote:
> On Thu, Mar 06, 2025 at 11:52:46PM +0200, Jarkko Sakkinen wrote:
> > On Wed, Mar 05, 2025 at 03:02:29PM -0400, Jason Gunthorpe wrote:
> > > On Wed, Mar 05, 2025 at 10:04:25AM +0100, Stefano Garzarella wrote:
> > > > Jason suggested the send_recv() ops [2], which I liked, but if you prefer to
> > > > avoid that, I can restore what we did in v1 and replace the
> > > > TPM_CHIP_FLAG_IRQ hack with your point 2 (or use TPM_CHIP_FLAG_IRQ if you
> > > > think it is fine).
> > >
> > > I think it is a pretty notable simplification for the driver as it
> > > does not need to implement send, status, req_canceled and more ops.
> > >
> > > Given the small LOC on the core side I'd call that simplification a
> > > win..
> >
> > I'm sorry to disagree with you on this but adding a callback for
> > one leaf driver is not what I would call "a win" :-)
>
> IIUC in the ftpm driver (tpm_ftpm_tee.c) we could also use send_recv() and
> save a memcpy() to a temporally buffer (pvt_data->resp_buf) and also that 4k
> buffer allocated with the private data of the driver.
>
> BTW if you agree, for now I'll do something similar of what we do in the
> ftpm driver (which would be what Jarkko recommended - status() returns 0,
> .req_complete_mask = 0, .req_complete_val = 0) and we can discuss
> send_recv() in a new series where I can include changes for the ftpm driver
> too, to see whether it makes sense or not.
>
> WDYT?
Yeah, that would work. Althought not related to this callback interface
per se, also tpm-dev-common.c is one example (in a way).
>
> Thanks,
> Stefano
BR, Jarkko
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [RFC PATCH v2 3/6] tpm: add send_recv() ops in tpm_class_ops
2025-03-05 9:04 ` Stefano Garzarella
2025-03-05 19:02 ` Jason Gunthorpe
@ 2025-03-06 22:15 ` Jarkko Sakkinen
2025-03-07 15:37 ` Stefano Garzarella
1 sibling, 1 reply; 45+ messages in thread
From: Jarkko Sakkinen @ 2025-03-06 22:15 UTC (permalink / raw)
To: Stefano Garzarella
Cc: Jason Gunthorpe, Thomas Gleixner, Claudio Carvalho, Peter Huewe,
x86, Dov Murik, linux-coco, Dionna Glaze, James Bottomley,
Ingo Molnar, Joerg Roedel, linux-integrity, linux-kernel,
Dave Hansen, Tom Lendacky, Borislav Petkov, H. Peter Anvin
On Wed, Mar 05, 2025 at 10:04:25AM +0100, Stefano Garzarella wrote:
> On Tue, Mar 04, 2025 at 10:21:55PM +0200, Jarkko Sakkinen wrote:
> > On Tue, Mar 04, 2025 at 06:56:02PM +0200, Jarkko Sakkinen wrote:
> > > On Mon, 2025-03-03 at 17:21 +0100, Stefano Garzarella wrote:
> > > > On Sat, Mar 01, 2025 at 03:45:10AM +0200, Jarkko Sakkinen wrote:
> > > > > On Fri, Feb 28, 2025 at 06:07:17PM +0100, Stefano Garzarella wrote:
> > > > > > + int (*send_recv)(struct tpm_chip *chip, u8 *buf, size_t
> > > > > > buf_len,
> > > > > > + size_t to_send);
> > > > >
> > > > > Please describe the meaning and purpose of to_send.
> > > >
> > > > Sure, I'll add in the commit description.
> > >
> > > It's always a command, right? So better be more concerete than
> > > "to_send", e.g. "cmd_len".
>
> Right!
>
> > >
> > > I'd do instead:
> > >
> > > if (!chip->send)
> > > goto out_recv;
> > >
> > > And change recv into:
> > >
> > > int (*recv)(struct tpm_chip *chip, u8 *buf, size_t buf_len,
> > > cmd_len);
> >
> > I think I went here over the top, and *if* we need a new callback
> > putting send_recv would be fine. Only thing I'd take from this is to
> > rename to_len as cmd_len.
>
> Got it.
>
> >
> > However, I don't think there are strong enough reasons to add complexity
> > to the callback interface with the basis of this single driver. You
> > should deal with this internally inside the driver instead.
> >
> > So do something along the lines of, e.g.:
> >
> > 1. Create dummy send() copying the command to internal
> > buffer.
> > 2. Create ->status() returning zero, and set req_complete_mask and
> > req_complete_val to zero.
> > 3. Performan transaction in recv().
> >
> > How you split send_recv() between send() and recv() is up to you. This
> > was merely showing that we don't need send_recv() desperately.
>
> We did something similar in v1 [1], but instead of your point 2, we just set
> `chip->flags |= TPM_CHIP_FLAG_IRQ;` in the probe() after we allocated the
> chip.
>
> Jason suggested the send_recv() ops [2], which I liked, but if you prefer to
> avoid that, I can restore what we did in v1 and replace the
> TPM_CHIP_FLAG_IRQ hack with your point 2 (or use TPM_CHIP_FLAG_IRQ if you
> think it is fine).
>
> @Jarkko, @Jason, I don't have a strong preference about it, so your choice
> :-)
I'd say, unless you have actual identified blocker, please go with
a driver where the complexity is managed within the driver.
>
> Thanks,
> Stefano
>
> [1] https://lore.kernel.org/linux-integrity/20241210143423.101774-2-sgarzare@redhat.com/
> [2] https://lore.kernel.org/linux-integrity/CAGxU2F51EoqDqi6By6eBa7qT+VT006DJ9+V-PANQ6GQrwVWt_Q@mail.gmail.com/
BR, Jarkko
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [RFC PATCH v2 3/6] tpm: add send_recv() ops in tpm_class_ops
2025-03-06 22:15 ` Jarkko Sakkinen
@ 2025-03-07 15:37 ` Stefano Garzarella
0 siblings, 0 replies; 45+ messages in thread
From: Stefano Garzarella @ 2025-03-07 15:37 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: Jason Gunthorpe, Thomas Gleixner, Claudio Carvalho, Peter Huewe,
x86, Dov Murik, linux-coco, Dionna Glaze, James Bottomley,
Ingo Molnar, Joerg Roedel, linux-integrity, linux-kernel,
Dave Hansen, Tom Lendacky, Borislav Petkov, H. Peter Anvin
On Fri, Mar 07, 2025 at 12:15:34AM +0200, Jarkko Sakkinen wrote:
>On Wed, Mar 05, 2025 at 10:04:25AM +0100, Stefano Garzarella wrote:
>> On Tue, Mar 04, 2025 at 10:21:55PM +0200, Jarkko Sakkinen wrote:
>> > On Tue, Mar 04, 2025 at 06:56:02PM +0200, Jarkko Sakkinen wrote:
>> > > On Mon, 2025-03-03 at 17:21 +0100, Stefano Garzarella wrote:
>> > > > On Sat, Mar 01, 2025 at 03:45:10AM +0200, Jarkko Sakkinen wrote:
>> > > > > On Fri, Feb 28, 2025 at 06:07:17PM +0100, Stefano Garzarella wrote:
>> > > > > > + int (*send_recv)(struct tpm_chip *chip, u8 *buf, size_t
>> > > > > > buf_len,
>> > > > > > + size_t to_send);
>> > > > >
>> > > > > Please describe the meaning and purpose of to_send.
>> > > >
>> > > > Sure, I'll add in the commit description.
>> > >
>> > > It's always a command, right? So better be more concerete than
>> > > "to_send", e.g. "cmd_len".
>>
>> Right!
>>
>> > >
>> > > I'd do instead:
>> > >
>> > > if (!chip->send)
>> > > goto out_recv;
>> > >
>> > > And change recv into:
>> > >
>> > > int (*recv)(struct tpm_chip *chip, u8 *buf, size_t buf_len,
>> > > cmd_len);
>> >
>> > I think I went here over the top, and *if* we need a new callback
>> > putting send_recv would be fine. Only thing I'd take from this is to
>> > rename to_len as cmd_len.
>>
>> Got it.
>>
>> >
>> > However, I don't think there are strong enough reasons to add complexity
>> > to the callback interface with the basis of this single driver. You
>> > should deal with this internally inside the driver instead.
>> >
>> > So do something along the lines of, e.g.:
>> >
>> > 1. Create dummy send() copying the command to internal
>> > buffer.
>> > 2. Create ->status() returning zero, and set req_complete_mask and
>> > req_complete_val to zero.
>> > 3. Performan transaction in recv().
>> >
>> > How you split send_recv() between send() and recv() is up to you. This
>> > was merely showing that we don't need send_recv() desperately.
>>
>> We did something similar in v1 [1], but instead of your point 2, we just set
>> `chip->flags |= TPM_CHIP_FLAG_IRQ;` in the probe() after we allocated the
>> chip.
>>
>> Jason suggested the send_recv() ops [2], which I liked, but if you prefer to
>> avoid that, I can restore what we did in v1 and replace the
>> TPM_CHIP_FLAG_IRQ hack with your point 2 (or use TPM_CHIP_FLAG_IRQ if you
>> think it is fine).
>>
>> @Jarkko, @Jason, I don't have a strong preference about it, so your choice
>> :-)
>
>I'd say, unless you have actual identified blocker, please go with
>a driver where the complexity is managed within the driver.
Yep, got it ;-)
Thanks,
Stefano
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [RFC PATCH v2 3/6] tpm: add send_recv() ops in tpm_class_ops
2025-02-28 17:07 ` [RFC PATCH v2 3/6] tpm: add send_recv() ops in tpm_class_ops Stefano Garzarella
2025-03-01 1:45 ` Jarkko Sakkinen
@ 2025-03-03 14:06 ` Tom Lendacky
2025-03-03 17:29 ` Stefano Garzarella
1 sibling, 1 reply; 45+ messages in thread
From: Tom Lendacky @ 2025-03-03 14:06 UTC (permalink / raw)
To: Stefano Garzarella, Jarkko Sakkinen
Cc: Thomas Gleixner, Claudio Carvalho, Peter Huewe, x86, Dov Murik,
linux-coco, Dionna Glaze, James Bottomley, Ingo Molnar,
Joerg Roedel, Jason Gunthorpe, linux-integrity, linux-kernel,
Dave Hansen, Borislav Petkov, H. Peter Anvin
On 2/28/25 11:07, Stefano Garzarella wrote:
> Some devices do not support interrupts and provide a single operation
> to send the command and receive the response on the same buffer.
>
> To support this scenario, a driver could set TPM_CHIP_FLAG_IRQ in the
> chip's flags to get recv() to be called immediately after send() in
> tpm_try_transmit().
>
> Instead of abusing TPM_CHIP_FLAG_IRQ, introduce a new callback
> send_recv(). If that callback is defined, it is called in
> tpm_try_transmit() to send the command and receive the response on
> the same buffer in a single call.
>
> Suggested-by: Jason Gunthorpe <jgg@ziepe.ca>
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
> include/linux/tpm.h | 2 ++
> drivers/char/tpm/tpm-interface.c | 8 +++++++-
> 2 files changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/tpm.h b/include/linux/tpm.h
> index 20a40ade8030..2ede8e0592d3 100644
> --- a/include/linux/tpm.h
> +++ b/include/linux/tpm.h
> @@ -88,6 +88,8 @@ struct tpm_class_ops {
> bool (*req_canceled)(struct tpm_chip *chip, u8 status);
> int (*recv) (struct tpm_chip *chip, u8 *buf, size_t len);
> int (*send) (struct tpm_chip *chip, u8 *buf, size_t len);
> + int (*send_recv)(struct tpm_chip *chip, u8 *buf, size_t buf_len,
> + size_t to_send);
> void (*cancel) (struct tpm_chip *chip);
> u8 (*status) (struct tpm_chip *chip);
> void (*update_timeouts)(struct tpm_chip *chip,
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index b1daa0d7b341..4f92b0477696 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -82,6 +82,9 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip, void *buf, size_t bufsiz)
> return -E2BIG;
> }
>
> + if (chip->ops->send_recv)
> + goto out_recv;
It might look a bit cleaner if you issue the send_recv() call here and
then jump to a new label after the recv() call just before 'len' is checked.
Thanks,
Tom
> +
> rc = chip->ops->send(chip, buf, count);
> if (rc < 0) {
> if (rc != -EPIPE)
> @@ -123,7 +126,10 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip, void *buf, size_t bufsiz)
> return -ETIME;
>
> out_recv:
> - len = chip->ops->recv(chip, buf, bufsiz);
> + if (chip->ops->send_recv)
> + len = chip->ops->send_recv(chip, buf, bufsiz, count);
> + else
> + len = chip->ops->recv(chip, buf, bufsiz);
> if (len < 0) {
> rc = len;
> dev_err(&chip->dev, "tpm_transmit: tpm_recv: error %d\n", rc);
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [RFC PATCH v2 3/6] tpm: add send_recv() ops in tpm_class_ops
2025-03-03 14:06 ` Tom Lendacky
@ 2025-03-03 17:29 ` Stefano Garzarella
0 siblings, 0 replies; 45+ messages in thread
From: Stefano Garzarella @ 2025-03-03 17:29 UTC (permalink / raw)
To: Tom Lendacky
Cc: Jarkko Sakkinen, Thomas Gleixner, Claudio Carvalho, Peter Huewe,
x86, Dov Murik, linux-coco, Dionna Glaze, James Bottomley,
Ingo Molnar, Joerg Roedel, Jason Gunthorpe, linux-integrity,
linux-kernel, Dave Hansen, Borislav Petkov, H. Peter Anvin
On Mon, Mar 03, 2025 at 08:06:43AM -0600, Tom Lendacky wrote:
>On 2/28/25 11:07, Stefano Garzarella wrote:
>> Some devices do not support interrupts and provide a single operation
>> to send the command and receive the response on the same buffer.
>>
>> To support this scenario, a driver could set TPM_CHIP_FLAG_IRQ in the
>> chip's flags to get recv() to be called immediately after send() in
>> tpm_try_transmit().
>>
>> Instead of abusing TPM_CHIP_FLAG_IRQ, introduce a new callback
>> send_recv(). If that callback is defined, it is called in
>> tpm_try_transmit() to send the command and receive the response on
>> the same buffer in a single call.
>>
>> Suggested-by: Jason Gunthorpe <jgg@ziepe.ca>
>> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
>> ---
>> include/linux/tpm.h | 2 ++
>> drivers/char/tpm/tpm-interface.c | 8 +++++++-
>> 2 files changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/linux/tpm.h b/include/linux/tpm.h
>> index 20a40ade8030..2ede8e0592d3 100644
>> --- a/include/linux/tpm.h
>> +++ b/include/linux/tpm.h
>> @@ -88,6 +88,8 @@ struct tpm_class_ops {
>> bool (*req_canceled)(struct tpm_chip *chip, u8 status);
>> int (*recv) (struct tpm_chip *chip, u8 *buf, size_t len);
>> int (*send) (struct tpm_chip *chip, u8 *buf, size_t len);
>> + int (*send_recv)(struct tpm_chip *chip, u8 *buf, size_t buf_len,
>> + size_t to_send);
>> void (*cancel) (struct tpm_chip *chip);
>> u8 (*status) (struct tpm_chip *chip);
>> void (*update_timeouts)(struct tpm_chip *chip,
>> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
>> index b1daa0d7b341..4f92b0477696 100644
>> --- a/drivers/char/tpm/tpm-interface.c
>> +++ b/drivers/char/tpm/tpm-interface.c
>> @@ -82,6 +82,9 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip, void *buf, size_t bufsiz)
>> return -E2BIG;
>> }
>>
>> + if (chip->ops->send_recv)
>> + goto out_recv;
>
>It might look a bit cleaner if you issue the send_recv() call here and
>then jump to a new label after the recv() call just before 'len' is checked.
Yep, I see, I was undecided to avoid adding a new label and just have
out_recv which in future cases always handles the send_recv() case.
But maybe I overthought, I will do as you suggest.
Thanks,
Stefano
>
>Thanks,
>Tom
>
>> +
>> rc = chip->ops->send(chip, buf, count);
>> if (rc < 0) {
>> if (rc != -EPIPE)
>> @@ -123,7 +126,10 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip, void *buf, size_t bufsiz)
>> return -ETIME;
>>
>> out_recv:
>> - len = chip->ops->recv(chip, buf, bufsiz);
>> + if (chip->ops->send_recv)
>> + len = chip->ops->send_recv(chip, buf, bufsiz, count);
>> + else
>> + len = chip->ops->recv(chip, buf, bufsiz);
>> if (len < 0) {
>> rc = len;
>> dev_err(&chip->dev, "tpm_transmit: tpm_recv: error %d\n", rc);
>
^ permalink raw reply [flat|nested] 45+ messages in thread
* [RFC PATCH v2 4/6] tpm: add interface to interact with devices based on TCG Simulator
2025-02-28 17:07 [RFC PATCH v2 0/6] Enlightened vTPM support for SVSM on SEV-SNP Stefano Garzarella
` (2 preceding siblings ...)
2025-02-28 17:07 ` [RFC PATCH v2 3/6] tpm: add send_recv() ops in tpm_class_ops Stefano Garzarella
@ 2025-02-28 17:07 ` Stefano Garzarella
2025-03-01 1:48 ` Jarkko Sakkinen
2025-03-03 14:28 ` Tom Lendacky
2025-02-28 17:07 ` [RFC PATCH v2 6/6] x86/sev: register tpm-svsm platform device Stefano Garzarella
` (2 subsequent siblings)
6 siblings, 2 replies; 45+ messages in thread
From: Stefano Garzarella @ 2025-02-28 17:07 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: Thomas Gleixner, Claudio Carvalho, Peter Huewe, x86, Dov Murik,
linux-coco, Dionna Glaze, James Bottomley, Ingo Molnar,
Joerg Roedel, Jason Gunthorpe, linux-integrity, linux-kernel,
Dave Hansen, Tom Lendacky, Borislav Petkov, H. Peter Anvin,
Stefano Garzarella
This is primarily designed to support an enlightened driver for the
AMD SVSM based vTPM, but it could be used by any TPM driver which
communicates with a TPM device implemented through the TCG TPM reference
implementation (https://github.com/TrustedComputingGroup/TPM)
Co-developed-by: James Bottomley <James.Bottomley@HansenPartnership.com>
Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
Co-developed-by: Claudio Carvalho <cclaudio@linux.ibm.com>
Signed-off-by: Claudio Carvalho <cclaudio@linux.ibm.com>
Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
James, Claudio are you fine with the Cdb, Sob?
The code is based to what was in the initial RFC, but I removed the
tpm_platform module, moved some code in the header, changed some names,
etc.
For these reasons I reset the author but added C-o-b.
Please, let me know if this is okay or if I need to do anything
else (reset the author, etc.)
---
include/linux/tpm_tcgsim.h | 136 +++++++++++++++++++++++++++++++++++++
1 file changed, 136 insertions(+)
create mode 100644 include/linux/tpm_tcgsim.h
diff --git a/include/linux/tpm_tcgsim.h b/include/linux/tpm_tcgsim.h
new file mode 100644
index 000000000000..bd5b123c393b
--- /dev/null
+++ b/include/linux/tpm_tcgsim.h
@@ -0,0 +1,136 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (C) 2023 James.Bottomley@HansenPartnership.com
+ * Copyright (C) 2025 Red Hat, Inc. All Rights Reserved.
+ *
+ * Generic interface usable by TPM drivers interacting with devices
+ * implemented through the TCG Simulator.
+ */
+#ifndef _TPM_TCGSIM_H_
+#define _TPM_TCGSIM_H_
+
+#include <linux/errno.h>
+#include <linux/string.h>
+#include <linux/types.h>
+
+/*
+ * The current TCG Simulator TPM commands we support. The complete list is
+ * in the TcpTpmProtocol header:
+ *
+ * https://github.com/TrustedComputingGroup/TPM/blob/main/TPMCmd/Simulator/include/TpmTcpProtocol.h
+ */
+
+#define TPM_SEND_COMMAND 8
+#define TPM_SIGNAL_CANCEL_ON 9
+#define TPM_SIGNAL_CANCEL_OFF 10
+/*
+ * Any platform specific commands should be placed here and should start
+ * at 0x8000 to avoid clashes with the TCG Simulator protocol. They should
+ * follow the same self describing buffer format below.
+ */
+
+#define TPM_TCGSIM_MAX_BUFFER 4096 /* max req/resp buffer size */
+
+/**
+ * struct tpm_req - generic request header for single word command
+ *
+ * @cmd: The command to send
+ */
+struct tpm_req {
+ u32 cmd;
+} __packed;
+
+/**
+ * struct tpm_resp - generic response header
+ *
+ * @size: The response size (zero if nothing follows)
+ *
+ * Note: most TCG Simulator commands simply return zero here with no indication
+ * of success or failure.
+ */
+struct tpm_resp {
+ u32 size;
+} __packed;
+
+/**
+ * struct tpm_send_cmd_req - Structure for a TPM_SEND_COMMAND request
+ *
+ * @hdr: The request header whit the command (must be TPM_SEND_COMMAND)
+ * @locality: The locality
+ * @inbuf_size: The size of the input buffer following
+ * @inbuf: A buffer of size inbuf_size
+ *
+ * Note that TCG Simulator expects @inbuf_size to be equal to the size of the
+ * specific TPM command, otherwise an TPM_RC_COMMAND_SIZE error is
+ * returned.
+ */
+struct tpm_send_cmd_req {
+ struct tpm_req hdr;
+ u8 locality;
+ u32 inbuf_size;
+ u8 inbuf[];
+} __packed;
+
+/**
+ * struct tpm_send_cmd_req - Structure for a TPM_SEND_COMMAND response
+ *
+ * @hdr: The response header whit the following size
+ * @outbuf: A buffer of size hdr.size
+ */
+struct tpm_send_cmd_resp {
+ struct tpm_resp hdr;
+ u8 outbuf[];
+} __packed;
+
+/**
+ * tpm_tcgsim_fill_send_cmd() - fill a struct tpm_send_cmd_req to be sent to the
+ * TCG Simulator.
+ * @req: The struct tpm_send_cmd_req to fill
+ * @locality: The locality
+ * @buf: The buffer from where to copy the payload of the command
+ * @len: The size of the buffer
+ *
+ * Return: 0 on success, negative error code on failure.
+ */
+static inline int
+tpm_tcgsim_fill_send_cmd(struct tpm_send_cmd_req *req, u8 locality,
+ const u8 *buf, size_t len)
+{
+ if (len > TPM_TCGSIM_MAX_BUFFER - sizeof(*req))
+ return -EINVAL;
+
+ req->hdr.cmd = TPM_SEND_COMMAND;
+ req->locality = locality;
+ req->inbuf_size = len;
+
+ memcpy(req->inbuf, buf, len);
+
+ return 0;
+}
+
+/**
+ * tpm_tcgsim_parse_send_cmd() - Parse a struct tpm_send_cmd_resp received from
+ * the TCG Simulator
+ * @resp: The struct tpm_send_cmd_resp to parse
+ * @buf: The buffer where to copy the response
+ * @len: The size of the buffer
+ *
+ * Return: buffer size filled with the response on success, negative error
+ * code on failure.
+ */
+static inline int
+tpm_tcgsim_parse_send_cmd(const struct tpm_send_cmd_resp *resp, u8 *buf,
+ size_t len)
+{
+ if (len < resp->hdr.size)
+ return -E2BIG;
+
+ if (resp->hdr.size > TPM_TCGSIM_MAX_BUFFER - sizeof(*resp))
+ return -EINVAL; // Invalid response from the platform TPM
+
+ memcpy(buf, resp->outbuf, resp->hdr.size);
+
+ return resp->hdr.size;
+}
+
+#endif /* _TPM_TCGSIM_H_ */
--
2.48.1
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [RFC PATCH v2 4/6] tpm: add interface to interact with devices based on TCG Simulator
2025-02-28 17:07 ` [RFC PATCH v2 4/6] tpm: add interface to interact with devices based on TCG Simulator Stefano Garzarella
@ 2025-03-01 1:48 ` Jarkko Sakkinen
2025-03-03 16:41 ` Stefano Garzarella
2025-03-04 15:23 ` Stefano Garzarella
2025-03-03 14:28 ` Tom Lendacky
1 sibling, 2 replies; 45+ messages in thread
From: Jarkko Sakkinen @ 2025-03-01 1:48 UTC (permalink / raw)
To: Stefano Garzarella
Cc: Thomas Gleixner, Claudio Carvalho, Peter Huewe, x86, Dov Murik,
linux-coco, Dionna Glaze, James Bottomley, Ingo Molnar,
Joerg Roedel, Jason Gunthorpe, linux-integrity, linux-kernel,
Dave Hansen, Tom Lendacky, Borislav Petkov, H. Peter Anvin
On Fri, Feb 28, 2025 at 06:07:18PM +0100, Stefano Garzarella wrote:
> This is primarily designed to support an enlightened driver for the
The commit message is half-way cut.
I.e. it lacks the explanation of "this".
> AMD SVSM based vTPM, but it could be used by any TPM driver which
> communicates with a TPM device implemented through the TCG TPM reference
> implementation (https://github.com/TrustedComputingGroup/TPM)
>
> Co-developed-by: James Bottomley <James.Bottomley@HansenPartnership.com>
> Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
> Co-developed-by: Claudio Carvalho <cclaudio@linux.ibm.com>
> Signed-off-by: Claudio Carvalho <cclaudio@linux.ibm.com>
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
> James, Claudio are you fine with the Cdb, Sob?
> The code is based to what was in the initial RFC, but I removed the
> tpm_platform module, moved some code in the header, changed some names,
> etc.
> For these reasons I reset the author but added C-o-b.
> Please, let me know if this is okay or if I need to do anything
> else (reset the author, etc.)
> ---
> include/linux/tpm_tcgsim.h | 136 +++++++++++++++++++++++++++++++++++++
> 1 file changed, 136 insertions(+)
> create mode 100644 include/linux/tpm_tcgsim.h
>
> diff --git a/include/linux/tpm_tcgsim.h b/include/linux/tpm_tcgsim.h
> new file mode 100644
> index 000000000000..bd5b123c393b
> --- /dev/null
> +++ b/include/linux/tpm_tcgsim.h
> @@ -0,0 +1,136 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (C) 2023 James.Bottomley@HansenPartnership.com
> + * Copyright (C) 2025 Red Hat, Inc. All Rights Reserved.
> + *
> + * Generic interface usable by TPM drivers interacting with devices
> + * implemented through the TCG Simulator.
> + */
> +#ifndef _TPM_TCGSIM_H_
> +#define _TPM_TCGSIM_H_
> +
> +#include <linux/errno.h>
> +#include <linux/string.h>
> +#include <linux/types.h>
> +
> +/*
> + * The current TCG Simulator TPM commands we support. The complete list is
> + * in the TcpTpmProtocol header:
> + *
> + * https://github.com/TrustedComputingGroup/TPM/blob/main/TPMCmd/Simulator/include/TpmTcpProtocol.h
We should not be dependent on any out-of-tree headers.
> + */
> +
> +#define TPM_SEND_COMMAND 8
> +#define TPM_SIGNAL_CANCEL_ON 9
> +#define TPM_SIGNAL_CANCEL_OFF 10
> +/*
> + * Any platform specific commands should be placed here and should start
> + * at 0x8000 to avoid clashes with the TCG Simulator protocol. They should
> + * follow the same self describing buffer format below.
> + */
> +
> +#define TPM_TCGSIM_MAX_BUFFER 4096 /* max req/resp buffer size */
> +
> +/**
> + * struct tpm_req - generic request header for single word command
> + *
> + * @cmd: The command to send
> + */
> +struct tpm_req {
> + u32 cmd;
> +} __packed;
> +
> +/**
> + * struct tpm_resp - generic response header
> + *
> + * @size: The response size (zero if nothing follows)
> + *
> + * Note: most TCG Simulator commands simply return zero here with no indication
> + * of success or failure.
> + */
> +struct tpm_resp {
> + u32 size;
> +} __packed;
> +
> +/**
> + * struct tpm_send_cmd_req - Structure for a TPM_SEND_COMMAND request
> + *
> + * @hdr: The request header whit the command (must be TPM_SEND_COMMAND)
> + * @locality: The locality
> + * @inbuf_size: The size of the input buffer following
> + * @inbuf: A buffer of size inbuf_size
> + *
> + * Note that TCG Simulator expects @inbuf_size to be equal to the size of the
> + * specific TPM command, otherwise an TPM_RC_COMMAND_SIZE error is
> + * returned.
> + */
> +struct tpm_send_cmd_req {
> + struct tpm_req hdr;
> + u8 locality;
> + u32 inbuf_size;
> + u8 inbuf[];
> +} __packed;
> +
> +/**
> + * struct tpm_send_cmd_req - Structure for a TPM_SEND_COMMAND response
> + *
> + * @hdr: The response header whit the following size
> + * @outbuf: A buffer of size hdr.size
> + */
> +struct tpm_send_cmd_resp {
> + struct tpm_resp hdr;
> + u8 outbuf[];
> +} __packed;
> +
> +/**
> + * tpm_tcgsim_fill_send_cmd() - fill a struct tpm_send_cmd_req to be sent to the
> + * TCG Simulator.
> + * @req: The struct tpm_send_cmd_req to fill
> + * @locality: The locality
> + * @buf: The buffer from where to copy the payload of the command
> + * @len: The size of the buffer
> + *
> + * Return: 0 on success, negative error code on failure.
> + */
> +static inline int
> +tpm_tcgsim_fill_send_cmd(struct tpm_send_cmd_req *req, u8 locality,
> + const u8 *buf, size_t len)
> +{
> + if (len > TPM_TCGSIM_MAX_BUFFER - sizeof(*req))
> + return -EINVAL;
> +
> + req->hdr.cmd = TPM_SEND_COMMAND;
> + req->locality = locality;
> + req->inbuf_size = len;
> +
> + memcpy(req->inbuf, buf, len);
> +
> + return 0;
> +}
> +
> +/**
> + * tpm_tcgsim_parse_send_cmd() - Parse a struct tpm_send_cmd_resp received from
> + * the TCG Simulator
> + * @resp: The struct tpm_send_cmd_resp to parse
> + * @buf: The buffer where to copy the response
> + * @len: The size of the buffer
> + *
> + * Return: buffer size filled with the response on success, negative error
> + * code on failure.
> + */
> +static inline int
> +tpm_tcgsim_parse_send_cmd(const struct tpm_send_cmd_resp *resp, u8 *buf,
> + size_t len)
> +{
> + if (len < resp->hdr.size)
> + return -E2BIG;
> +
> + if (resp->hdr.size > TPM_TCGSIM_MAX_BUFFER - sizeof(*resp))
> + return -EINVAL; // Invalid response from the platform TPM
> +
> + memcpy(buf, resp->outbuf, resp->hdr.size);
> +
> + return resp->hdr.size;
> +}
> +
> +#endif /* _TPM_TCGSIM_H_ */
> --
> 2.48.1
>
This commit got me lost tbh.
BR, Jarkko
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [RFC PATCH v2 4/6] tpm: add interface to interact with devices based on TCG Simulator
2025-03-01 1:48 ` Jarkko Sakkinen
@ 2025-03-03 16:41 ` Stefano Garzarella
2025-03-04 15:23 ` Stefano Garzarella
1 sibling, 0 replies; 45+ messages in thread
From: Stefano Garzarella @ 2025-03-03 16:41 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: Thomas Gleixner, Claudio Carvalho, Peter Huewe, x86, Dov Murik,
linux-coco, Dionna Glaze, James Bottomley, Ingo Molnar,
Joerg Roedel, Jason Gunthorpe, linux-integrity, linux-kernel,
Dave Hansen, Tom Lendacky, Borislav Petkov, H. Peter Anvin
On Sat, Mar 01, 2025 at 03:48:35AM +0200, Jarkko Sakkinen wrote:
>On Fri, Feb 28, 2025 at 06:07:18PM +0100, Stefano Garzarella wrote:
>> This is primarily designed to support an enlightened driver for the
>
>The commit message is half-way cut.
>
>I.e. it lacks the explanation of "this".
Yes, sorry, I rephrased James' previous commit description, but I admit
it didn't come out clear.
I meant to say that "this" new header contains useful functions for
creating commands and parsing responses in those drivers where vTPM
devices are implemented via the TCG TPM reference implementation.
>
>> AMD SVSM based vTPM, but it could be used by any TPM driver which
>> communicates with a TPM device implemented through the TCG TPM reference
>> implementation (https://github.com/TrustedComputingGroup/TPM)
>>
>> Co-developed-by: James Bottomley <James.Bottomley@HansenPartnership.com>
>> Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
>> Co-developed-by: Claudio Carvalho <cclaudio@linux.ibm.com>
>> Signed-off-by: Claudio Carvalho <cclaudio@linux.ibm.com>
>> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
>> ---
>> James, Claudio are you fine with the Cdb, Sob?
>> The code is based to what was in the initial RFC, but I removed the
>> tpm_platform module, moved some code in the header, changed some names,
>> etc.
>> For these reasons I reset the author but added C-o-b.
>> Please, let me know if this is okay or if I need to do anything
>> else (reset the author, etc.)
>> ---
>> include/linux/tpm_tcgsim.h | 136 +++++++++++++++++++++++++++++++++++++
>> 1 file changed, 136 insertions(+)
>> create mode 100644 include/linux/tpm_tcgsim.h
>>
>> diff --git a/include/linux/tpm_tcgsim.h b/include/linux/tpm_tcgsim.h
>> new file mode 100644
>> index 000000000000..bd5b123c393b
>> --- /dev/null
>> +++ b/include/linux/tpm_tcgsim.h
>> @@ -0,0 +1,136 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * Copyright (C) 2023 James.Bottomley@HansenPartnership.com
>> + * Copyright (C) 2025 Red Hat, Inc. All Rights Reserved.
>> + *
>> + * Generic interface usable by TPM drivers interacting with devices
>> + * implemented through the TCG Simulator.
>> + */
>> +#ifndef _TPM_TCGSIM_H_
>> +#define _TPM_TCGSIM_H_
>> +
>> +#include <linux/errno.h>
>> +#include <linux/string.h>
>> +#include <linux/types.h>
>> +
>> +/*
>> + * The current TCG Simulator TPM commands we support. The complete list is
>> + * in the TcpTpmProtocol header:
>> + *
>> + * https://github.com/TrustedComputingGroup/TPM/blob/main/TPMCmd/Simulator/include/TpmTcpProtocol.h
>
>We should not be dependent on any out-of-tree headers.
We might see that header as a specification of how to communicate with
the device.
What do you suggest we do in this case?
>
>> + */
>> +
>> +#define TPM_SEND_COMMAND 8
>> +#define TPM_SIGNAL_CANCEL_ON 9
>> +#define TPM_SIGNAL_CANCEL_OFF 10
>> +/*
>> + * Any platform specific commands should be placed here and should start
>> + * at 0x8000 to avoid clashes with the TCG Simulator protocol. They should
>> + * follow the same self describing buffer format below.
>> + */
>> +
>> +#define TPM_TCGSIM_MAX_BUFFER 4096 /* max req/resp buffer size */
>> +
>> +/**
>> + * struct tpm_req - generic request header for single word command
>> + *
>> + * @cmd: The command to send
>> + */
>> +struct tpm_req {
>> + u32 cmd;
>> +} __packed;
>> +
>> +/**
>> + * struct tpm_resp - generic response header
>> + *
>> + * @size: The response size (zero if nothing follows)
>> + *
>> + * Note: most TCG Simulator commands simply return zero here with no indication
>> + * of success or failure.
>> + */
>> +struct tpm_resp {
>> + u32 size;
>> +} __packed;
>> +
>> +/**
>> + * struct tpm_send_cmd_req - Structure for a TPM_SEND_COMMAND request
>> + *
>> + * @hdr: The request header whit the command (must be TPM_SEND_COMMAND)
>> + * @locality: The locality
>> + * @inbuf_size: The size of the input buffer following
>> + * @inbuf: A buffer of size inbuf_size
>> + *
>> + * Note that TCG Simulator expects @inbuf_size to be equal to the size of the
>> + * specific TPM command, otherwise an TPM_RC_COMMAND_SIZE error is
>> + * returned.
>> + */
>> +struct tpm_send_cmd_req {
>> + struct tpm_req hdr;
>> + u8 locality;
>> + u32 inbuf_size;
>> + u8 inbuf[];
>> +} __packed;
>> +
>> +/**
>> + * struct tpm_send_cmd_req - Structure for a TPM_SEND_COMMAND response
>> + *
>> + * @hdr: The response header whit the following size
>> + * @outbuf: A buffer of size hdr.size
>> + */
>> +struct tpm_send_cmd_resp {
>> + struct tpm_resp hdr;
>> + u8 outbuf[];
>> +} __packed;
>> +
>> +/**
>> + * tpm_tcgsim_fill_send_cmd() - fill a struct tpm_send_cmd_req to be sent to the
>> + * TCG Simulator.
>> + * @req: The struct tpm_send_cmd_req to fill
>> + * @locality: The locality
>> + * @buf: The buffer from where to copy the payload of the command
>> + * @len: The size of the buffer
>> + *
>> + * Return: 0 on success, negative error code on failure.
>> + */
>> +static inline int
>> +tpm_tcgsim_fill_send_cmd(struct tpm_send_cmd_req *req, u8 locality,
>> + const u8 *buf, size_t len)
>> +{
>> + if (len > TPM_TCGSIM_MAX_BUFFER - sizeof(*req))
>> + return -EINVAL;
>> +
>> + req->hdr.cmd = TPM_SEND_COMMAND;
>> + req->locality = locality;
>> + req->inbuf_size = len;
>> +
>> + memcpy(req->inbuf, buf, len);
>> +
>> + return 0;
>> +}
>> +
>> +/**
>> + * tpm_tcgsim_parse_send_cmd() - Parse a struct tpm_send_cmd_resp received from
>> + * the TCG Simulator
>> + * @resp: The struct tpm_send_cmd_resp to parse
>> + * @buf: The buffer where to copy the response
>> + * @len: The size of the buffer
>> + *
>> + * Return: buffer size filled with the response on success, negative error
>> + * code on failure.
>> + */
>> +static inline int
>> +tpm_tcgsim_parse_send_cmd(const struct tpm_send_cmd_resp *resp, u8 *buf,
>> + size_t len)
>> +{
>> + if (len < resp->hdr.size)
>> + return -E2BIG;
>> +
>> + if (resp->hdr.size > TPM_TCGSIM_MAX_BUFFER - sizeof(*resp))
>> + return -EINVAL; // Invalid response from the platform TPM
>> +
>> + memcpy(buf, resp->outbuf, resp->hdr.size);
>> +
>> + return resp->hdr.size;
>> +}
>> +
>> +#endif /* _TPM_TCGSIM_H_ */
>> --
>> 2.48.1
>>
>
>This commit got me lost tbh.
The vTPM device is emulated through the simulator of the TCG reference
implementation, so to communicate with it we have to send commands
following the specified format.
This header is intended to add code that could also be reused by other
drivers where the device follows this format. As James mentioned,
Microsoft has something similar for OpenHCL and may reuse this header in
the future.
If you think it is too much, I can include these things for now in
tpm-svsm.c and when we have another driver we will pull them out.
Thanks,
Stefano
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [RFC PATCH v2 4/6] tpm: add interface to interact with devices based on TCG Simulator
2025-03-01 1:48 ` Jarkko Sakkinen
2025-03-03 16:41 ` Stefano Garzarella
@ 2025-03-04 15:23 ` Stefano Garzarella
2025-03-04 17:14 ` Jarkko Sakkinen
1 sibling, 1 reply; 45+ messages in thread
From: Stefano Garzarella @ 2025-03-04 15:23 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: Thomas Gleixner, Claudio Carvalho, Peter Huewe, x86, Dov Murik,
linux-coco, Dionna Glaze, James Bottomley, Ingo Molnar,
Joerg Roedel, Jason Gunthorpe, linux-integrity, linux-kernel,
Dave Hansen, Tom Lendacky, Borislav Petkov, H. Peter Anvin
On Sat, Mar 01, 2025 at 03:48:35AM +0200, Jarkko Sakkinen wrote:
>On Fri, Feb 28, 2025 at 06:07:18PM +0100, Stefano Garzarella wrote:
>> This is primarily designed to support an enlightened driver for the
>
>The commit message is half-way cut.
>
>I.e. it lacks the explanation of "this".
>
>> AMD SVSM based vTPM, but it could be used by any TPM driver which
>> communicates with a TPM device implemented through the TCG TPM reference
>> implementation (https://github.com/TrustedComputingGroup/TPM)
>>
>> Co-developed-by: James Bottomley <James.Bottomley@HansenPartnership.com>
>> Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
>> Co-developed-by: Claudio Carvalho <cclaudio@linux.ibm.com>
>> Signed-off-by: Claudio Carvalho <cclaudio@linux.ibm.com>
>> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
>> ---
>> James, Claudio are you fine with the Cdb, Sob?
>> The code is based to what was in the initial RFC, but I removed the
>> tpm_platform module, moved some code in the header, changed some names,
>> etc.
>> For these reasons I reset the author but added C-o-b.
>> Please, let me know if this is okay or if I need to do anything
>> else (reset the author, etc.)
>> ---
>> include/linux/tpm_tcgsim.h | 136 +++++++++++++++++++++++++++++++++++++
>> 1 file changed, 136 insertions(+)
>> create mode 100644 include/linux/tpm_tcgsim.h
>>
>> diff --git a/include/linux/tpm_tcgsim.h b/include/linux/tpm_tcgsim.h
>> new file mode 100644
>> index 000000000000..bd5b123c393b
>> --- /dev/null
>> +++ b/include/linux/tpm_tcgsim.h
>> @@ -0,0 +1,136 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * Copyright (C) 2023 James.Bottomley@HansenPartnership.com
>> + * Copyright (C) 2025 Red Hat, Inc. All Rights Reserved.
>> + *
>> + * Generic interface usable by TPM drivers interacting with devices
>> + * implemented through the TCG Simulator.
>> + */
>> +#ifndef _TPM_TCGSIM_H_
>> +#define _TPM_TCGSIM_H_
>> +
>> +#include <linux/errno.h>
>> +#include <linux/string.h>
>> +#include <linux/types.h>
>> +
>> +/*
>> + * The current TCG Simulator TPM commands we support. The complete list is
>> + * in the TcpTpmProtocol header:
>> + *
>> + * https://github.com/TrustedComputingGroup/TPM/blob/main/TPMCmd/Simulator/include/TpmTcpProtocol.h
>
>We should not be dependent on any out-of-tree headers.
>
>> + */
>> +
>> +#define TPM_SEND_COMMAND 8
>> +#define TPM_SIGNAL_CANCEL_ON 9
>> +#define TPM_SIGNAL_CANCEL_OFF 10
>> +/*
>> + * Any platform specific commands should be placed here and should start
>> + * at 0x8000 to avoid clashes with the TCG Simulator protocol. They should
>> + * follow the same self describing buffer format below.
>> + */
>> +
>> +#define TPM_TCGSIM_MAX_BUFFER 4096 /* max req/resp buffer size */
>> +
>> +/**
>> + * struct tpm_req - generic request header for single word command
>> + *
>> + * @cmd: The command to send
>> + */
>> +struct tpm_req {
>> + u32 cmd;
>> +} __packed;
>> +
>> +/**
>> + * struct tpm_resp - generic response header
>> + *
>> + * @size: The response size (zero if nothing follows)
>> + *
>> + * Note: most TCG Simulator commands simply return zero here with no indication
>> + * of success or failure.
>> + */
>> +struct tpm_resp {
>> + u32 size;
>> +} __packed;
>> +
>> +/**
>> + * struct tpm_send_cmd_req - Structure for a TPM_SEND_COMMAND request
>> + *
>> + * @hdr: The request header whit the command (must be TPM_SEND_COMMAND)
>> + * @locality: The locality
>> + * @inbuf_size: The size of the input buffer following
>> + * @inbuf: A buffer of size inbuf_size
>> + *
>> + * Note that TCG Simulator expects @inbuf_size to be equal to the size of the
>> + * specific TPM command, otherwise an TPM_RC_COMMAND_SIZE error is
>> + * returned.
>> + */
>> +struct tpm_send_cmd_req {
>> + struct tpm_req hdr;
>> + u8 locality;
>> + u32 inbuf_size;
>> + u8 inbuf[];
>> +} __packed;
>> +
>> +/**
>> + * struct tpm_send_cmd_req - Structure for a TPM_SEND_COMMAND response
>> + *
>> + * @hdr: The response header whit the following size
>> + * @outbuf: A buffer of size hdr.size
>> + */
>> +struct tpm_send_cmd_resp {
>> + struct tpm_resp hdr;
>> + u8 outbuf[];
>> +} __packed;
>> +
>> +/**
>> + * tpm_tcgsim_fill_send_cmd() - fill a struct tpm_send_cmd_req to be sent to the
>> + * TCG Simulator.
>> + * @req: The struct tpm_send_cmd_req to fill
>> + * @locality: The locality
>> + * @buf: The buffer from where to copy the payload of the command
>> + * @len: The size of the buffer
>> + *
>> + * Return: 0 on success, negative error code on failure.
>> + */
>> +static inline int
>> +tpm_tcgsim_fill_send_cmd(struct tpm_send_cmd_req *req, u8 locality,
>> + const u8 *buf, size_t len)
>> +{
>> + if (len > TPM_TCGSIM_MAX_BUFFER - sizeof(*req))
>> + return -EINVAL;
>> +
>> + req->hdr.cmd = TPM_SEND_COMMAND;
>> + req->locality = locality;
>> + req->inbuf_size = len;
>> +
>> + memcpy(req->inbuf, buf, len);
>> +
>> + return 0;
>> +}
>> +
>> +/**
>> + * tpm_tcgsim_parse_send_cmd() - Parse a struct tpm_send_cmd_resp received from
>> + * the TCG Simulator
>> + * @resp: The struct tpm_send_cmd_resp to parse
>> + * @buf: The buffer where to copy the response
>> + * @len: The size of the buffer
>> + *
>> + * Return: buffer size filled with the response on success, negative error
>> + * code on failure.
>> + */
>> +static inline int
>> +tpm_tcgsim_parse_send_cmd(const struct tpm_send_cmd_resp *resp, u8 *buf,
>> + size_t len)
>> +{
>> + if (len < resp->hdr.size)
>> + return -E2BIG;
>> +
>> + if (resp->hdr.size > TPM_TCGSIM_MAX_BUFFER - sizeof(*resp))
>> + return -EINVAL; // Invalid response from the platform TPM
>> +
>> + memcpy(buf, resp->outbuf, resp->hdr.size);
>> +
>> + return resp->hdr.size;
>> +}
>> +
>> +#endif /* _TPM_TCGSIM_H_ */
>> --
>> 2.48.1
>>
>
>This commit got me lost tbh.
Now I understand why you got lost, my bad!
I checked further and these structures seem to be specific to the vTPM
protocol defined by AMD SVSM specification and independent of TCG TPM
(unless reusing some definitions like TPM_SEND_COMMAND).
At this point I think it is best to remove this header (or move in
x86/sev) and move this rewrap to x86/sev to avoid confusion.
I'll do in v3, sorry for the confusion.
Stefano
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [RFC PATCH v2 4/6] tpm: add interface to interact with devices based on TCG Simulator
2025-03-04 15:23 ` Stefano Garzarella
@ 2025-03-04 17:14 ` Jarkko Sakkinen
0 siblings, 0 replies; 45+ messages in thread
From: Jarkko Sakkinen @ 2025-03-04 17:14 UTC (permalink / raw)
To: Stefano Garzarella
Cc: Thomas Gleixner, Claudio Carvalho, Peter Huewe, x86, Dov Murik,
linux-coco, Dionna Glaze, James Bottomley, Ingo Molnar,
Joerg Roedel, Jason Gunthorpe, linux-integrity, linux-kernel,
Dave Hansen, Tom Lendacky, Borislav Petkov, H. Peter Anvin
On Tue, Mar 04, 2025 at 04:23:51PM +0100, Stefano Garzarella wrote:
> > This commit got me lost tbh.
>
> Now I understand why you got lost, my bad!
No need for apologies, just merely reporting what I do or do not
understand with brutal honesty ;-)
> I checked further and these structures seem to be specific to the vTPM
> protocol defined by AMD SVSM specification and independent of TCG TPM
> (unless reusing some definitions like TPM_SEND_COMMAND).
>
> At this point I think it is best to remove this header (or move in x86/sev)
> and move this rewrap to x86/sev to avoid confusion.
Yeah, I do agree. We can commit to SVSM specification because that
is the target of this driver anyhow (not Microsoft simulator) :-)
>
> I'll do in v3, sorry for the confusion.
Absolutely, np.
>
> Stefano
BR, Jarkko
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [RFC PATCH v2 4/6] tpm: add interface to interact with devices based on TCG Simulator
2025-02-28 17:07 ` [RFC PATCH v2 4/6] tpm: add interface to interact with devices based on TCG Simulator Stefano Garzarella
2025-03-01 1:48 ` Jarkko Sakkinen
@ 2025-03-03 14:28 ` Tom Lendacky
2025-03-03 17:30 ` Stefano Garzarella
1 sibling, 1 reply; 45+ messages in thread
From: Tom Lendacky @ 2025-03-03 14:28 UTC (permalink / raw)
To: Stefano Garzarella, Jarkko Sakkinen
Cc: Thomas Gleixner, Claudio Carvalho, Peter Huewe, x86, Dov Murik,
linux-coco, Dionna Glaze, James Bottomley, Ingo Molnar,
Joerg Roedel, Jason Gunthorpe, linux-integrity, linux-kernel,
Dave Hansen, Borislav Petkov, H. Peter Anvin
On 2/28/25 11:07, Stefano Garzarella wrote:
> This is primarily designed to support an enlightened driver for the
> AMD SVSM based vTPM, but it could be used by any TPM driver which
> communicates with a TPM device implemented through the TCG TPM reference
> implementation (https://github.com/TrustedComputingGroup/TPM)
>
> Co-developed-by: James Bottomley <James.Bottomley@HansenPartnership.com>
> Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
> Co-developed-by: Claudio Carvalho <cclaudio@linux.ibm.com>
> Signed-off-by: Claudio Carvalho <cclaudio@linux.ibm.com>
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
> James, Claudio are you fine with the Cdb, Sob?
> The code is based to what was in the initial RFC, but I removed the
> tpm_platform module, moved some code in the header, changed some names,
> etc.
> For these reasons I reset the author but added C-o-b.
> Please, let me know if this is okay or if I need to do anything
> else (reset the author, etc.)
> ---
> include/linux/tpm_tcgsim.h | 136 +++++++++++++++++++++++++++++++++++++
> 1 file changed, 136 insertions(+)
> create mode 100644 include/linux/tpm_tcgsim.h
>
> diff --git a/include/linux/tpm_tcgsim.h b/include/linux/tpm_tcgsim.h
> new file mode 100644
> index 000000000000..bd5b123c393b
> --- /dev/null
> +++ b/include/linux/tpm_tcgsim.h
> @@ -0,0 +1,136 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (C) 2023 James.Bottomley@HansenPartnership.com
> + * Copyright (C) 2025 Red Hat, Inc. All Rights Reserved.
> + *
> + * Generic interface usable by TPM drivers interacting with devices
> + * implemented through the TCG Simulator.
> + */
> +#ifndef _TPM_TCGSIM_H_
> +#define _TPM_TCGSIM_H_
> +
> +#include <linux/errno.h>
> +#include <linux/string.h>
> +#include <linux/types.h>
> +
> +/*
> + * The current TCG Simulator TPM commands we support. The complete list is
> + * in the TcpTpmProtocol header:
> + *
> + * https://github.com/TrustedComputingGroup/TPM/blob/main/TPMCmd/Simulator/include/TpmTcpProtocol.h
> + */
> +
> +#define TPM_SEND_COMMAND 8
> +#define TPM_SIGNAL_CANCEL_ON 9
> +#define TPM_SIGNAL_CANCEL_OFF 10
> +/*
> + * Any platform specific commands should be placed here and should start
> + * at 0x8000 to avoid clashes with the TCG Simulator protocol. They should
> + * follow the same self describing buffer format below.
> + */
> +
> +#define TPM_TCGSIM_MAX_BUFFER 4096 /* max req/resp buffer size */
> +
> +/**
> + * struct tpm_req - generic request header for single word command
> + *
> + * @cmd: The command to send
> + */
> +struct tpm_req {
> + u32 cmd;
> +} __packed;
> +
> +/**
> + * struct tpm_resp - generic response header
> + *
> + * @size: The response size (zero if nothing follows)
> + *
> + * Note: most TCG Simulator commands simply return zero here with no indication
> + * of success or failure.
> + */
> +struct tpm_resp {
> + u32 size;
> +} __packed;
> +
> +/**
> + * struct tpm_send_cmd_req - Structure for a TPM_SEND_COMMAND request
> + *
> + * @hdr: The request header whit the command (must be TPM_SEND_COMMAND)
> + * @locality: The locality
> + * @inbuf_size: The size of the input buffer following
> + * @inbuf: A buffer of size inbuf_size
> + *
> + * Note that TCG Simulator expects @inbuf_size to be equal to the size of the
> + * specific TPM command, otherwise an TPM_RC_COMMAND_SIZE error is
> + * returned.
> + */
> +struct tpm_send_cmd_req {
> + struct tpm_req hdr;
> + u8 locality;
> + u32 inbuf_size;
> + u8 inbuf[];
> +} __packed;
> +
> +/**
> + * struct tpm_send_cmd_req - Structure for a TPM_SEND_COMMAND response
> + *
> + * @hdr: The response header whit the following size
> + * @outbuf: A buffer of size hdr.size
> + */
> +struct tpm_send_cmd_resp {
> + struct tpm_resp hdr;
> + u8 outbuf[];
> +} __packed;
> +
> +/**
> + * tpm_tcgsim_fill_send_cmd() - fill a struct tpm_send_cmd_req to be sent to the
> + * TCG Simulator.
> + * @req: The struct tpm_send_cmd_req to fill
> + * @locality: The locality
> + * @buf: The buffer from where to copy the payload of the command
> + * @len: The size of the buffer
> + *
> + * Return: 0 on success, negative error code on failure.
> + */
> +static inline int
> +tpm_tcgsim_fill_send_cmd(struct tpm_send_cmd_req *req, u8 locality,
> + const u8 *buf, size_t len)
> +{
> + if (len > TPM_TCGSIM_MAX_BUFFER - sizeof(*req))
> + return -EINVAL;
> +
> + req->hdr.cmd = TPM_SEND_COMMAND;
> + req->locality = locality;
> + req->inbuf_size = len;
> +
> + memcpy(req->inbuf, buf, len);
> +
> + return 0;
> +}
> +
> +/**
> + * tpm_tcgsim_parse_send_cmd() - Parse a struct tpm_send_cmd_resp received from
> + * the TCG Simulator
> + * @resp: The struct tpm_send_cmd_resp to parse
> + * @buf: The buffer where to copy the response
> + * @len: The size of the buffer
> + *
> + * Return: buffer size filled with the response on success, negative error
> + * code on failure.
> + */
> +static inline int
> +tpm_tcgsim_parse_send_cmd(const struct tpm_send_cmd_resp *resp, u8 *buf,
> + size_t len)
This is a confusing name... would tpm_tcgsim_parse_cmd_resp() be a
better name?
Thanks,
Tom
> +{
> + if (len < resp->hdr.size)
> + return -E2BIG;
> +
> + if (resp->hdr.size > TPM_TCGSIM_MAX_BUFFER - sizeof(*resp))
> + return -EINVAL; // Invalid response from the platform TPM
> +
> + memcpy(buf, resp->outbuf, resp->hdr.size);
> +
> + return resp->hdr.size;
> +}
> +
> +#endif /* _TPM_TCGSIM_H_ */
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [RFC PATCH v2 4/6] tpm: add interface to interact with devices based on TCG Simulator
2025-03-03 14:28 ` Tom Lendacky
@ 2025-03-03 17:30 ` Stefano Garzarella
0 siblings, 0 replies; 45+ messages in thread
From: Stefano Garzarella @ 2025-03-03 17:30 UTC (permalink / raw)
To: Tom Lendacky
Cc: Jarkko Sakkinen, Thomas Gleixner, Claudio Carvalho, Peter Huewe,
x86, Dov Murik, linux-coco, Dionna Glaze, James Bottomley,
Ingo Molnar, Joerg Roedel, Jason Gunthorpe, linux-integrity,
linux-kernel, Dave Hansen, Borislav Petkov, H. Peter Anvin
On Mon, Mar 03, 2025 at 08:28:45AM -0600, Tom Lendacky wrote:
>On 2/28/25 11:07, Stefano Garzarella wrote:
>> This is primarily designed to support an enlightened driver for the
>> AMD SVSM based vTPM, but it could be used by any TPM driver which
>> communicates with a TPM device implemented through the TCG TPM reference
>> implementation (https://github.com/TrustedComputingGroup/TPM)
>>
>> Co-developed-by: James Bottomley <James.Bottomley@HansenPartnership.com>
>> Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
>> Co-developed-by: Claudio Carvalho <cclaudio@linux.ibm.com>
>> Signed-off-by: Claudio Carvalho <cclaudio@linux.ibm.com>
>> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
>> ---
>> James, Claudio are you fine with the Cdb, Sob?
>> The code is based to what was in the initial RFC, but I removed the
>> tpm_platform module, moved some code in the header, changed some names,
>> etc.
>> For these reasons I reset the author but added C-o-b.
>> Please, let me know if this is okay or if I need to do anything
>> else (reset the author, etc.)
>> ---
>> include/linux/tpm_tcgsim.h | 136 +++++++++++++++++++++++++++++++++++++
>> 1 file changed, 136 insertions(+)
>> create mode 100644 include/linux/tpm_tcgsim.h
>>
>> diff --git a/include/linux/tpm_tcgsim.h b/include/linux/tpm_tcgsim.h
>> new file mode 100644
>> index 000000000000..bd5b123c393b
>> --- /dev/null
>> +++ b/include/linux/tpm_tcgsim.h
>> @@ -0,0 +1,136 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * Copyright (C) 2023 James.Bottomley@HansenPartnership.com
>> + * Copyright (C) 2025 Red Hat, Inc. All Rights Reserved.
>> + *
>> + * Generic interface usable by TPM drivers interacting with devices
>> + * implemented through the TCG Simulator.
>> + */
>> +#ifndef _TPM_TCGSIM_H_
>> +#define _TPM_TCGSIM_H_
>> +
>> +#include <linux/errno.h>
>> +#include <linux/string.h>
>> +#include <linux/types.h>
>> +
>> +/*
>> + * The current TCG Simulator TPM commands we support. The complete list is
>> + * in the TcpTpmProtocol header:
>> + *
>> + * https://github.com/TrustedComputingGroup/TPM/blob/main/TPMCmd/Simulator/include/TpmTcpProtocol.h
>> + */
>> +
>> +#define TPM_SEND_COMMAND 8
>> +#define TPM_SIGNAL_CANCEL_ON 9
>> +#define TPM_SIGNAL_CANCEL_OFF 10
>> +/*
>> + * Any platform specific commands should be placed here and should start
>> + * at 0x8000 to avoid clashes with the TCG Simulator protocol. They should
>> + * follow the same self describing buffer format below.
>> + */
>> +
>> +#define TPM_TCGSIM_MAX_BUFFER 4096 /* max req/resp buffer size */
>> +
>> +/**
>> + * struct tpm_req - generic request header for single word command
>> + *
>> + * @cmd: The command to send
>> + */
>> +struct tpm_req {
>> + u32 cmd;
>> +} __packed;
>> +
>> +/**
>> + * struct tpm_resp - generic response header
>> + *
>> + * @size: The response size (zero if nothing follows)
>> + *
>> + * Note: most TCG Simulator commands simply return zero here with no indication
>> + * of success or failure.
>> + */
>> +struct tpm_resp {
>> + u32 size;
>> +} __packed;
>> +
>> +/**
>> + * struct tpm_send_cmd_req - Structure for a TPM_SEND_COMMAND request
>> + *
>> + * @hdr: The request header whit the command (must be TPM_SEND_COMMAND)
>> + * @locality: The locality
>> + * @inbuf_size: The size of the input buffer following
>> + * @inbuf: A buffer of size inbuf_size
>> + *
>> + * Note that TCG Simulator expects @inbuf_size to be equal to the size of the
>> + * specific TPM command, otherwise an TPM_RC_COMMAND_SIZE error is
>> + * returned.
>> + */
>> +struct tpm_send_cmd_req {
>> + struct tpm_req hdr;
>> + u8 locality;
>> + u32 inbuf_size;
>> + u8 inbuf[];
>> +} __packed;
>> +
>> +/**
>> + * struct tpm_send_cmd_req - Structure for a TPM_SEND_COMMAND response
>> + *
>> + * @hdr: The response header whit the following size
>> + * @outbuf: A buffer of size hdr.size
>> + */
>> +struct tpm_send_cmd_resp {
>> + struct tpm_resp hdr;
>> + u8 outbuf[];
>> +} __packed;
>> +
>> +/**
>> + * tpm_tcgsim_fill_send_cmd() - fill a struct tpm_send_cmd_req to be sent to the
>> + * TCG Simulator.
>> + * @req: The struct tpm_send_cmd_req to fill
>> + * @locality: The locality
>> + * @buf: The buffer from where to copy the payload of the command
>> + * @len: The size of the buffer
>> + *
>> + * Return: 0 on success, negative error code on failure.
>> + */
>> +static inline int
>> +tpm_tcgsim_fill_send_cmd(struct tpm_send_cmd_req *req, u8 locality,
>> + const u8 *buf, size_t len)
>> +{
>> + if (len > TPM_TCGSIM_MAX_BUFFER - sizeof(*req))
>> + return -EINVAL;
>> +
>> + req->hdr.cmd = TPM_SEND_COMMAND;
>> + req->locality = locality;
>> + req->inbuf_size = len;
>> +
>> + memcpy(req->inbuf, buf, len);
>> +
>> + return 0;
>> +}
>> +
>> +/**
>> + * tpm_tcgsim_parse_send_cmd() - Parse a struct tpm_send_cmd_resp received from
>> + * the TCG Simulator
>> + * @resp: The struct tpm_send_cmd_resp to parse
>> + * @buf: The buffer where to copy the response
>> + * @len: The size of the buffer
>> + *
>> + * Return: buffer size filled with the response on success, negative error
>> + * code on failure.
>> + */
>> +static inline int
>> +tpm_tcgsim_parse_send_cmd(const struct tpm_send_cmd_resp *resp, u8 *buf,
>> + size_t len)
>
>This is a confusing name... would tpm_tcgsim_parse_cmd_resp() be a
>better name?
Ack, and we can rename also the other in tpm_tcgsim_fill_cmd_req().
Thanks,
Stefano
^ permalink raw reply [flat|nested] 45+ messages in thread
* [RFC PATCH v2 6/6] x86/sev: register tpm-svsm platform device
2025-02-28 17:07 [RFC PATCH v2 0/6] Enlightened vTPM support for SVSM on SEV-SNP Stefano Garzarella
` (3 preceding siblings ...)
2025-02-28 17:07 ` [RFC PATCH v2 4/6] tpm: add interface to interact with devices based on TCG Simulator Stefano Garzarella
@ 2025-02-28 17:07 ` Stefano Garzarella
2025-03-01 0:30 ` [RFC PATCH v2 0/6] Enlightened vTPM support for SVSM on SEV-SNP Jason Gunthorpe
[not found] ` <20250228170720.144739-6-sgarzare@redhat.com>
6 siblings, 0 replies; 45+ messages in thread
From: Stefano Garzarella @ 2025-02-28 17:07 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: Thomas Gleixner, Claudio Carvalho, Peter Huewe, x86, Dov Murik,
linux-coco, Dionna Glaze, James Bottomley, Ingo Molnar,
Joerg Roedel, Jason Gunthorpe, linux-integrity, linux-kernel,
Dave Hansen, Tom Lendacky, Borislav Petkov, H. Peter Anvin,
Stefano Garzarella
SNP platform can provide a vTPM device emulated by SVSM.
The "tpm-svsm" device can be handled by the platform driver added
by the previous commit in drivers/char/tpm/tpm_svsm.c
The driver will call snp_svsm_vtpm_probe() to check if SVSM is
present and if it's support the vTPM protocol.
Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
arch/x86/coco/sev/core.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c
index 4158e447d645..7e91fae7d43a 100644
--- a/arch/x86/coco/sev/core.c
+++ b/arch/x86/coco/sev/core.c
@@ -2680,6 +2680,11 @@ static struct platform_device sev_guest_device = {
.id = -1,
};
+static struct platform_device tpm_svsm_device = {
+ .name = "tpm-svsm",
+ .id = -1,
+};
+
static int __init snp_init_platform_device(void)
{
if (!cc_platform_has(CC_ATTR_GUEST_SEV_SNP))
@@ -2688,6 +2693,9 @@ static int __init snp_init_platform_device(void)
if (platform_device_register(&sev_guest_device))
return -ENODEV;
+ if (platform_device_register(&tpm_svsm_device))
+ return -ENODEV;
+
pr_info("SNP guest platform device initialized.\n");
return 0;
}
--
2.48.1
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [RFC PATCH v2 0/6] Enlightened vTPM support for SVSM on SEV-SNP
2025-02-28 17:07 [RFC PATCH v2 0/6] Enlightened vTPM support for SVSM on SEV-SNP Stefano Garzarella
` (4 preceding siblings ...)
2025-02-28 17:07 ` [RFC PATCH v2 6/6] x86/sev: register tpm-svsm platform device Stefano Garzarella
@ 2025-03-01 0:30 ` Jason Gunthorpe
2025-03-03 16:20 ` Stefano Garzarella
[not found] ` <20250228170720.144739-6-sgarzare@redhat.com>
6 siblings, 1 reply; 45+ messages in thread
From: Jason Gunthorpe @ 2025-03-01 0:30 UTC (permalink / raw)
To: Stefano Garzarella
Cc: Jarkko Sakkinen, Thomas Gleixner, Claudio Carvalho, Peter Huewe,
x86, Dov Murik, linux-coco, Dionna Glaze, James Bottomley,
Ingo Molnar, Joerg Roedel, linux-integrity, linux-kernel,
Dave Hansen, Tom Lendacky, Borislav Petkov, H. Peter Anvin
On Fri, Feb 28, 2025 at 06:07:14PM +0100, Stefano Garzarella wrote:
> I put RFC back in because we haven't yet decided if this is the best
> approach to support SVSM vTPM, but I really like to receive feedbacks
> especially from the maintainer/reviewers of the TPM subsystem, to see if
> this approach is acceptable.
I didn't look in high detail, but the overall shape is what I was
thinking about in our previous conversations. Very little TPM code is
under arch/, we have a nice simplifying helper in the core code, and
you have a tidy platform device to tie it all together.
Jason
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [RFC PATCH v2 0/6] Enlightened vTPM support for SVSM on SEV-SNP
2025-03-01 0:30 ` [RFC PATCH v2 0/6] Enlightened vTPM support for SVSM on SEV-SNP Jason Gunthorpe
@ 2025-03-03 16:20 ` Stefano Garzarella
0 siblings, 0 replies; 45+ messages in thread
From: Stefano Garzarella @ 2025-03-03 16:20 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Jarkko Sakkinen, Thomas Gleixner, Claudio Carvalho, Peter Huewe,
x86, Dov Murik, linux-coco, Dionna Glaze, James Bottomley,
Ingo Molnar, Joerg Roedel, linux-integrity, linux-kernel,
Dave Hansen, Tom Lendacky, Borislav Petkov, H. Peter Anvin
On Fri, Feb 28, 2025 at 08:30:09PM -0400, Jason Gunthorpe wrote:
>On Fri, Feb 28, 2025 at 06:07:14PM +0100, Stefano Garzarella wrote:
>> I put RFC back in because we haven't yet decided if this is the best
>> approach to support SVSM vTPM, but I really like to receive feedbacks
>> especially from the maintainer/reviewers of the TPM subsystem, to see if
>> this approach is acceptable.
>
>I didn't look in high detail, but the overall shape is what I was
>thinking about in our previous conversations. Very little TPM code is
>under arch/, we have a nice simplifying helper in the core code, and
>you have a tidy platform device to tie it all together.
Thank you so much for taking a look and confirming that I understood
your suggestions correctly!
Stefano
^ permalink raw reply [flat|nested] 45+ messages in thread
[parent not found: <20250228170720.144739-6-sgarzare@redhat.com>]
* Re: [RFC PATCH v2 5/6] tpm: add SNP SVSM vTPM driver
[not found] ` <20250228170720.144739-6-sgarzare@redhat.com>
@ 2025-03-01 0:28 ` Jason Gunthorpe
2025-03-03 16:19 ` Stefano Garzarella
2025-03-01 1:51 ` Jarkko Sakkinen
1 sibling, 1 reply; 45+ messages in thread
From: Jason Gunthorpe @ 2025-03-01 0:28 UTC (permalink / raw)
To: Stefano Garzarella
Cc: Jarkko Sakkinen, Thomas Gleixner, Claudio Carvalho, Peter Huewe,
x86, Dov Murik, linux-coco, Dionna Glaze, James Bottomley,
Ingo Molnar, Joerg Roedel, linux-integrity, linux-kernel,
Dave Hansen, Tom Lendacky, Borislav Petkov, H. Peter Anvin
On Fri, Feb 28, 2025 at 06:07:19PM +0100, Stefano Garzarella wrote:
> +/*
> + * 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.
> + */
??? Is that really true? I didn't know that
I thought you could unbind anything using /sys/../unbind?
Jason
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [RFC PATCH v2 5/6] tpm: add SNP SVSM vTPM driver
2025-03-01 0:28 ` [RFC PATCH v2 5/6] tpm: add SNP SVSM vTPM driver Jason Gunthorpe
@ 2025-03-03 16:19 ` Stefano Garzarella
2025-03-03 18:24 ` Jason Gunthorpe
0 siblings, 1 reply; 45+ messages in thread
From: Stefano Garzarella @ 2025-03-03 16:19 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Jarkko Sakkinen, Thomas Gleixner, Claudio Carvalho, Peter Huewe,
x86, Dov Murik, linux-coco, Dionna Glaze, James Bottomley,
Ingo Molnar, Joerg Roedel, linux-integrity, linux-kernel,
Dave Hansen, Tom Lendacky, Borislav Petkov, H. Peter Anvin
On Fri, Feb 28, 2025 at 08:28:19PM -0400, Jason Gunthorpe wrote:
>On Fri, Feb 28, 2025 at 06:07:19PM +0100, Stefano Garzarella wrote:
>> +/*
>> + * 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.
>> + */
>
>??? Is that really true? I didn't know that
I initially followed drivers/virt/coco/sev-guest/sev-guest.c to figure
out how to clean a driver registered with
module_platform_driver_probe(), then I saw that pattern with the same
comment is used in several other drivers.
>
>I thought you could unbind anything using /sys/../unbind?
I can't see `unbind` for this driver:
$ ls /sys/bus/platform/drivers/tpm-svsm/
module tpm-svsm uevent
While I can see it for example for others like fw_cfg:
$ ls /sys/bus/platform/drivers/fw_cfg
bind module QEMU0002:00 uevent unbind
BTW I can unload the `tpm-svsm` module. Loading it again will cause
issues if I don't have a remove function that calls
tpm_chip_unregister().
Thanks,
Stefano
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [RFC PATCH v2 5/6] tpm: add SNP SVSM vTPM driver
2025-03-03 16:19 ` Stefano Garzarella
@ 2025-03-03 18:24 ` Jason Gunthorpe
0 siblings, 0 replies; 45+ messages in thread
From: Jason Gunthorpe @ 2025-03-03 18:24 UTC (permalink / raw)
To: Stefano Garzarella
Cc: Jarkko Sakkinen, Thomas Gleixner, Claudio Carvalho, Peter Huewe,
x86, Dov Murik, linux-coco, Dionna Glaze, James Bottomley,
Ingo Molnar, Joerg Roedel, linux-integrity, linux-kernel,
Dave Hansen, Tom Lendacky, Borislav Petkov, H. Peter Anvin
On Mon, Mar 03, 2025 at 05:19:05PM +0100, Stefano Garzarella wrote:
> On Fri, Feb 28, 2025 at 08:28:19PM -0400, Jason Gunthorpe wrote:
> > On Fri, Feb 28, 2025 at 06:07:19PM +0100, Stefano Garzarella wrote:
> > > +/*
> > > + * 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.
> > > + */
> >
> > ??? Is that really true? I didn't know that
>
> I initially followed drivers/virt/coco/sev-guest/sev-guest.c to figure out
> how to clean a driver registered with module_platform_driver_probe(), then I
> saw that pattern with the same comment is used in several other drivers.
>
> >
> > I thought you could unbind anything using /sys/../unbind?
>
> I can't see `unbind` for this driver:
>
> $ ls /sys/bus/platform/drivers/tpm-svsm/
> module tpm-svsm uevent
>
> While I can see it for example for others like fw_cfg:
Wow, I didn't know that could be done
> BTW I can unload the `tpm-svsm` module.
Unload the module and implicitly unbound the driver
But not manually unbind the driver?? Huh? That seems pretty wrong..
> Loading it again will cause issues if I don't have a remove function
> that calls tpm_chip_unregister().
You definately need the remove function call doing what you have, it
is just surprising to me that there is a case where you can statically
know it is not callable..
Jason
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [RFC PATCH v2 5/6] tpm: add SNP SVSM vTPM driver
[not found] ` <20250228170720.144739-6-sgarzare@redhat.com>
2025-03-01 0:28 ` [RFC PATCH v2 5/6] tpm: add SNP SVSM vTPM driver Jason Gunthorpe
@ 2025-03-01 1:51 ` Jarkko Sakkinen
2025-03-01 3:57 ` Dionna Amalie Glaze
2025-03-03 16:46 ` Stefano Garzarella
1 sibling, 2 replies; 45+ messages in thread
From: Jarkko Sakkinen @ 2025-03-01 1:51 UTC (permalink / raw)
To: Stefano Garzarella
Cc: Thomas Gleixner, Claudio Carvalho, Peter Huewe, x86, Dov Murik,
linux-coco, Dionna Glaze, James Bottomley, Ingo Molnar,
Joerg Roedel, Jason Gunthorpe, linux-integrity, linux-kernel,
Dave Hansen, Tom Lendacky, Borislav Petkov, H. Peter Anvin
On Fri, Feb 28, 2025 at 06:07:19PM +0100, 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 vTPM is emulated through the TCG reference implementation, so this
> driver leverages tpm_tcgsim.h to fill commands and parse responses.
Why? Please don't.
>
> 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.
>
> [1] "Secure VM Service Module for SEV-SNP Guests"
> Publication # 58019 Revision: 1.00
> https://www.amd.com/content/dam/amd/en/documents/epyc-technical-docs/specifications/58019.pdf
>
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
> drivers/char/tpm/tpm_svsm.c | 120 ++++++++++++++++++++++++++++++++++++
> drivers/char/tpm/Kconfig | 10 +++
> drivers/char/tpm/Makefile | 1 +
> 3 files changed, 131 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..1c34133990c5
> --- /dev/null
> +++ b/drivers/char/tpm/tpm_svsm.c
> @@ -0,0 +1,120 @@
> +// 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).
> + *
> + * The vTPM is emulated through the TCG reference implementation, so this
> + * driver leverages tpm_tcgsim.h to fill commands and parse responses.
> + *
> + * [1] "Secure VM Service Module for SEV-SNP Guests"
> + * Publication # 58019 Revision: 1.00
> + * https://www.amd.com/content/dam/amd/en/documents/epyc-technical-docs/specifications/58019.pdf
> + */
> +
> +#include <asm/sev.h>
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/platform_device.h>
> +#include <linux/tpm_tcgsim.h>
> +
> +#include "tpm.h"
> +
> +struct tpm_svsm_priv {
> + u8 buffer[TPM_TCGSIM_MAX_BUFFER];
> + u8 locality;
> +};
> +
> +static int tpm_svsm_send_recv(struct tpm_chip *chip, u8 *buf, size_t buf_len,
> + size_t to_send)
> +{
> + struct tpm_svsm_priv *priv = dev_get_drvdata(&chip->dev);
> + int ret;
> +
> + ret = tpm_tcgsim_fill_send_cmd((struct tpm_send_cmd_req *)priv->buffer,
> + priv->locality, buf, to_send);
> + if (ret)
> + return ret;
> +
> + ret = snp_svsm_vtpm_send_command(priv->buffer);
> + if (ret)
> + return ret;
> +
> + return tpm_tcgsim_parse_send_cmd((struct tpm_send_cmd_resp *)priv->buffer,
> + buf, buf_len);
> +}
> +
> +static struct tpm_class_ops tpm_chip_ops = {
> + .flags = TPM_OPS_AUTO_STARTUP,
> + .send_recv = tpm_svsm_send_recv,
> +};
> +
> +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
> --
> 2.48.1
>
OK, so this is like ARM's driver architecture using SMC, and I think
tpm_ftpm_tee is probably one great reflection for this overall. Is this
correct analysis, or not?
BR, Jarkko
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [RFC PATCH v2 5/6] tpm: add SNP SVSM vTPM driver
2025-03-01 1:51 ` Jarkko Sakkinen
@ 2025-03-01 3:57 ` Dionna Amalie Glaze
2025-03-03 16:46 ` Stefano Garzarella
1 sibling, 0 replies; 45+ messages in thread
From: Dionna Amalie Glaze @ 2025-03-01 3:57 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: Stefano Garzarella, Thomas Gleixner, Claudio Carvalho,
Peter Huewe, x86, Dov Murik, linux-coco, James Bottomley,
Ingo Molnar, Joerg Roedel, Jason Gunthorpe, linux-integrity,
linux-kernel, Dave Hansen, Tom Lendacky, Borislav Petkov,
H. Peter Anvin
On Fri, Feb 28, 2025 at 5:51 PM Jarkko Sakkinen <jarkko@kernel.org> wrote:
>
> On Fri, Feb 28, 2025 at 06:07:19PM +0100, 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 vTPM is emulated through the TCG reference implementation, so this
> > driver leverages tpm_tcgsim.h to fill commands and parse responses.
>
> Why? Please don't.
>
> >
> > 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.
> >
> > [1] "Secure VM Service Module for SEV-SNP Guests"
> > Publication # 58019 Revision: 1.00
> > https://www.amd.com/content/dam/amd/en/documents/epyc-technical-docs/specifications/58019.pdf
> >
> > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> > ---
> > drivers/char/tpm/tpm_svsm.c | 120 ++++++++++++++++++++++++++++++++++++
> > drivers/char/tpm/Kconfig | 10 +++
> > drivers/char/tpm/Makefile | 1 +
> > 3 files changed, 131 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..1c34133990c5
> > --- /dev/null
> > +++ b/drivers/char/tpm/tpm_svsm.c
> > @@ -0,0 +1,120 @@
> > +// 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).
> > + *
> > + * The vTPM is emulated through the TCG reference implementation, so this
> > + * driver leverages tpm_tcgsim.h to fill commands and parse responses.
> > + *
> > + * [1] "Secure VM Service Module for SEV-SNP Guests"
> > + * Publication # 58019 Revision: 1.00
> > + * https://www.amd.com/content/dam/amd/en/documents/epyc-technical-docs/specifications/58019.pdf
> > + */
> > +
> > +#include <asm/sev.h>
> > +#include <linux/module.h>
> > +#include <linux/kernel.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/tpm_tcgsim.h>
> > +
> > +#include "tpm.h"
> > +
> > +struct tpm_svsm_priv {
> > + u8 buffer[TPM_TCGSIM_MAX_BUFFER];
> > + u8 locality;
> > +};
> > +
> > +static int tpm_svsm_send_recv(struct tpm_chip *chip, u8 *buf, size_t buf_len,
> > + size_t to_send)
> > +{
> > + struct tpm_svsm_priv *priv = dev_get_drvdata(&chip->dev);
> > + int ret;
> > +
> > + ret = tpm_tcgsim_fill_send_cmd((struct tpm_send_cmd_req *)priv->buffer,
> > + priv->locality, buf, to_send);
> > + if (ret)
> > + return ret;
> > +
> > + ret = snp_svsm_vtpm_send_command(priv->buffer);
> > + if (ret)
> > + return ret;
> > +
> > + return tpm_tcgsim_parse_send_cmd((struct tpm_send_cmd_resp *)priv->buffer,
> > + buf, buf_len);
> > +}
> > +
> > +static struct tpm_class_ops tpm_chip_ops = {
> > + .flags = TPM_OPS_AUTO_STARTUP,
> > + .send_recv = tpm_svsm_send_recv,
> > +};
> > +
> > +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
> > --
> > 2.48.1
> >
>
> OK, so this is like ARM's driver architecture using SMC, and I think
> tpm_ftpm_tee is probably one great reflection for this overall. Is this
> correct analysis, or not?
Using ftpm is really obtuse, at least with my attempt
https://github.com/deeglaze/amdese-linux/tree/svsmftpm
I don't really know how to cleanly bind the platform_driver to the one device.
I don't think that this is any way better than what this patch series proposes.
>
> BR, Jarkko
--
-Dionna Glaze, PhD, CISSP, CCSP (she/her)
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [RFC PATCH v2 5/6] tpm: add SNP SVSM vTPM driver
2025-03-01 1:51 ` Jarkko Sakkinen
2025-03-01 3:57 ` Dionna Amalie Glaze
@ 2025-03-03 16:46 ` Stefano Garzarella
2025-03-04 17:27 ` Jarkko Sakkinen
1 sibling, 1 reply; 45+ messages in thread
From: Stefano Garzarella @ 2025-03-03 16:46 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: Thomas Gleixner, Claudio Carvalho, Peter Huewe, x86, Dov Murik,
linux-coco, Dionna Glaze, James Bottomley, Ingo Molnar,
Joerg Roedel, Jason Gunthorpe, linux-integrity, linux-kernel,
Dave Hansen, Tom Lendacky, Borislav Petkov, H. Peter Anvin
On Sat, Mar 01, 2025 at 03:51:46AM +0200, Jarkko Sakkinen wrote:
>On Fri, Feb 28, 2025 at 06:07:19PM +0100, 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 vTPM is emulated through the TCG reference implementation, so this
>> driver leverages tpm_tcgsim.h to fill commands and parse responses.
>
>Why? Please don't.
You mean it's better not to have the external header and have all the
functions here to prepare commands and parse responses?
As I mentioned, I did this because there may be other future drivers
that could use it to talk to emulated devices in the same way, that is,
through the TCG TPM reference implementation,
>
>>
>> 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.
>>
>> [1] "Secure VM Service Module for SEV-SNP Guests"
>> Publication # 58019 Revision: 1.00
>> https://www.amd.com/content/dam/amd/en/documents/epyc-technical-docs/specifications/58019.pdf
>>
>> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
>> ---
>> drivers/char/tpm/tpm_svsm.c | 120 ++++++++++++++++++++++++++++++++++++
>> drivers/char/tpm/Kconfig | 10 +++
>> drivers/char/tpm/Makefile | 1 +
>> 3 files changed, 131 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..1c34133990c5
>> --- /dev/null
>> +++ b/drivers/char/tpm/tpm_svsm.c
>> @@ -0,0 +1,120 @@
>> +// 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).
>> + *
>> + * The vTPM is emulated through the TCG reference implementation, so this
>> + * driver leverages tpm_tcgsim.h to fill commands and parse responses.
>> + *
>> + * [1] "Secure VM Service Module for SEV-SNP Guests"
>> + * Publication # 58019 Revision: 1.00
>> + * https://www.amd.com/content/dam/amd/en/documents/epyc-technical-docs/specifications/58019.pdf
>> + */
>> +
>> +#include <asm/sev.h>
>> +#include <linux/module.h>
>> +#include <linux/kernel.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/tpm_tcgsim.h>
>> +
>> +#include "tpm.h"
>> +
>> +struct tpm_svsm_priv {
>> + u8 buffer[TPM_TCGSIM_MAX_BUFFER];
>> + u8 locality;
>> +};
>> +
>> +static int tpm_svsm_send_recv(struct tpm_chip *chip, u8 *buf, size_t buf_len,
>> + size_t to_send)
>> +{
>> + struct tpm_svsm_priv *priv = dev_get_drvdata(&chip->dev);
>> + int ret;
>> +
>> + ret = tpm_tcgsim_fill_send_cmd((struct tpm_send_cmd_req *)priv->buffer,
>> + priv->locality, buf, to_send);
>> + if (ret)
>> + return ret;
>> +
>> + ret = snp_svsm_vtpm_send_command(priv->buffer);
>> + if (ret)
>> + return ret;
>> +
>> + return tpm_tcgsim_parse_send_cmd((struct tpm_send_cmd_resp *)priv->buffer,
>> + buf, buf_len);
>> +}
>> +
>> +static struct tpm_class_ops tpm_chip_ops = {
>> + .flags = TPM_OPS_AUTO_STARTUP,
>> + .send_recv = tpm_svsm_send_recv,
>> +};
>> +
>> +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
>> --
>> 2.48.1
>>
>
>OK, so this is like ARM's driver architecture using SMC, and I think
>tpm_ftpm_tee is probably one great reflection for this overall. Is this
>correct analysis, or not?
I just took a closer look at what ARM SMC is, and yes, it looks like a
correct analysis.
Dionna took a look at reusing ftpm and already replied with her
analysis, better to continue in that thread the discussion on eventually
reusing ftpm.
Thanks,
Stefano
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [RFC PATCH v2 5/6] tpm: add SNP SVSM vTPM driver
2025-03-03 16:46 ` Stefano Garzarella
@ 2025-03-04 17:27 ` Jarkko Sakkinen
2025-03-05 9:07 ` Stefano Garzarella
0 siblings, 1 reply; 45+ messages in thread
From: Jarkko Sakkinen @ 2025-03-04 17:27 UTC (permalink / raw)
To: Stefano Garzarella
Cc: Thomas Gleixner, Claudio Carvalho, Peter Huewe, x86, Dov Murik,
linux-coco, Dionna Glaze, James Bottomley, Ingo Molnar,
Joerg Roedel, Jason Gunthorpe, linux-integrity, linux-kernel,
Dave Hansen, Tom Lendacky, Borislav Petkov, H. Peter Anvin
On Mon, Mar 03, 2025 at 05:46:16PM +0100, Stefano Garzarella wrote:
> On Sat, Mar 01, 2025 at 03:51:46AM +0200, Jarkko Sakkinen wrote:
> > On Fri, Feb 28, 2025 at 06:07:19PM +0100, 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 vTPM is emulated through the TCG reference implementation, so this
> > > driver leverages tpm_tcgsim.h to fill commands and parse responses.
> >
> > Why? Please don't.
>
> You mean it's better not to have the external header and have all the
> functions here to prepare commands and parse responses?
>
> As I mentioned, I did this because there may be other future drivers that
> could use it to talk to emulated devices in the same way, that is, through
> the TCG TPM reference implementation,
Sorry about harsh comment. I think we discussed this (MS simulator
caused confusion). Anchor this to SVSM spec and we're fine.
BR, Jarkko
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [RFC PATCH v2 5/6] tpm: add SNP SVSM vTPM driver
2025-03-04 17:27 ` Jarkko Sakkinen
@ 2025-03-05 9:07 ` Stefano Garzarella
0 siblings, 0 replies; 45+ messages in thread
From: Stefano Garzarella @ 2025-03-05 9:07 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: Thomas Gleixner, Claudio Carvalho, Peter Huewe, x86, Dov Murik,
linux-coco, Dionna Glaze, James Bottomley, Ingo Molnar,
Joerg Roedel, Jason Gunthorpe, linux-integrity, linux-kernel,
Dave Hansen, Tom Lendacky, Borislav Petkov, H. Peter Anvin
On Tue, Mar 04, 2025 at 07:27:30PM +0200, Jarkko Sakkinen wrote:
>On Mon, Mar 03, 2025 at 05:46:16PM +0100, Stefano Garzarella wrote:
>> On Sat, Mar 01, 2025 at 03:51:46AM +0200, Jarkko Sakkinen wrote:
>> > On Fri, Feb 28, 2025 at 06:07:19PM +0100, 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 vTPM is emulated through the TCG reference implementation, so this
>> > > driver leverages tpm_tcgsim.h to fill commands and parse responses.
>> >
>> > Why? Please don't.
>>
>> You mean it's better not to have the external header and have all the
>> functions here to prepare commands and parse responses?
>>
>> As I mentioned, I did this because there may be other future drivers that
>> could use it to talk to emulated devices in the same way, that is, through
>> the TCG TPM reference implementation,
>
>Sorry about harsh comment. I think we discussed this (MS simulator
>caused confusion). Anchor this to SVSM spec and we're fine.
Yeah, I think we are now aligned, I will try to fix in the next version!
Thanks,
Stefano
^ permalink raw reply [flat|nested] 45+ messages in thread