From: James Bottomley <James.Bottomley@HansenPartnership.com>

If the SNP boot has a SVSM, probe for the vTPM device by sending a
SVSM_VTPM_QUERY call (function 8). The SVSM will return a bitmap with
the TPM_SEND_COMMAND bit set only if the vTPM is present and it is able
to handle TPM commands at runtime.

If a vTPM is found, register a platform device as "platform:tpm" so it
can be attached to the tpm_platform.c driver.

Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
[CC] Used SVSM_VTPM_QUERY to probe the TPM
Signed-off-by: Claudio Carvalho <cclaudio@linux.ibm.com>
[SG] Code adjusted with some changes introduced in 6.11
[SG] Used macro for SVSM_VTPM_CALL
Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
 arch/x86/coco/sev/core.c | 64 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 64 insertions(+)

diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c
index c5b0148b8c0a..ec0153fddc9e 100644
--- a/arch/x86/coco/sev/core.c
+++ b/arch/x86/coco/sev/core.c
@@ -21,6 +21,7 @@
 #include <linux/cpumask.h>
 #include <linux/efi.h>
 #include <linux/platform_device.h>
+#include <linux/tpm_platform.h>
 #include <linux/io.h>
 #include <linux/psp-sev.h>
 #include <linux/dmi.h>
@@ -2578,6 +2579,51 @@ static struct platform_device sev_guest_device = {
 	.id		= -1,
 };
 
+static struct platform_device tpm_device = {
+	.name		= "tpm",
+	.id		= -1,
+};
+
+static int snp_issue_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);
+}
+
+static bool is_svsm_vtpm_send_command_supported(void)
+{
+	struct svsm_call call = {};
+	u64 send_cmd_mask = 0;
+	u64 platform_cmds;
+	u64 features;
+	int ret;
+
+	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 must be zero */
+	if (features)
+		return false;
+
+	/* TPM_SEND_COMMAND - platform command 8 */
+	send_cmd_mask = 1 << 8;
+
+	return (platform_cmds & send_cmd_mask) == send_cmd_mask;
+}
+
 static int __init snp_init_platform_device(void)
 {
 	struct sev_guest_platform_data data;
@@ -2593,6 +2639,24 @@ static int __init snp_init_platform_device(void)
 		return -ENODEV;
 
 	pr_info("SNP guest platform device initialized.\n");
+
+	/*
+	 * The VTPM device is available only if we have a SVSM and
+	 * its VTPM supports the TPM_SEND_COMMAND platform command
+	 */
+	if (IS_ENABLED(CONFIG_TCG_PLATFORM) && snp_vmpl &&
+	    is_svsm_vtpm_send_command_supported()) {
+		struct tpm_platform_ops pops = {
+			.sendrcv = snp_issue_svsm_vtpm_send_command,
+		};
+
+		if (platform_device_add_data(&tpm_device, &pops, sizeof(pops)))
+			return -ENODEV;
+		if (platform_device_register(&tpm_device))
+			return -ENODEV;
+		pr_info("SNP SVSM VTPM platform device initialized\n");
+	}
+
 	return 0;
 }
 device_initcall(snp_init_platform_device);
-- 
2.47.1

On 12/10/24 08:34, Stefano Garzarella wrote:
> From: James Bottomley <James.Bottomley@HansenPartnership.com>
> 
> If the SNP boot has a SVSM, probe for the vTPM device by sending a
> SVSM_VTPM_QUERY call (function 8). The SVSM will return a bitmap with
> the TPM_SEND_COMMAND bit set only if the vTPM is present and it is able
> to handle TPM commands at runtime.
> 
> If a vTPM is found, register a platform device as "platform:tpm" so it
> can be attached to the tpm_platform.c driver.
> 
> Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
> [CC] Used SVSM_VTPM_QUERY to probe the TPM
> Signed-off-by: Claudio Carvalho <cclaudio@linux.ibm.com>
> [SG] Code adjusted with some changes introduced in 6.11
> [SG] Used macro for SVSM_VTPM_CALL
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
>  arch/x86/coco/sev/core.c | 64 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 64 insertions(+)
> 
> diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c
> index c5b0148b8c0a..ec0153fddc9e 100644
> --- a/arch/x86/coco/sev/core.c
> +++ b/arch/x86/coco/sev/core.c
> @@ -21,6 +21,7 @@
>  #include <linux/cpumask.h>
>  #include <linux/efi.h>
>  #include <linux/platform_device.h>
> +#include <linux/tpm_platform.h>
>  #include <linux/io.h>
>  #include <linux/psp-sev.h>
>  #include <linux/dmi.h>
> @@ -2578,6 +2579,51 @@ static struct platform_device sev_guest_device = {
>  	.id		= -1,
>  };
>  
> +static struct platform_device tpm_device = {
> +	.name		= "tpm",
> +	.id		= -1,
> +};
> +
> +static int snp_issue_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);
> +}
> +
> +static bool is_svsm_vtpm_send_command_supported(void)
> +{
> +	struct svsm_call call = {};
> +	u64 send_cmd_mask = 0;
> +	u64 platform_cmds;
> +	u64 features;
> +	int ret;
> +
> +	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 must be zero */
> +	if (features)
> +		return false;

I think this check should be removed. The SVSM currently returns all
zeroes for the features to allow for future support. If a new feature is
added in the future, this then allows a driver that supports that
feature to operate with a version of an SVSM that doesn't have that
feature implemented. It also allows a version of the driver that doesn't
know about that feature to work with an SVSM that has that feature.

A feature added to the vTPM shouldn't alter the behavior of something
that isn't using or understands that feature.

> +
> +	/* TPM_SEND_COMMAND - platform command 8 */
> +	send_cmd_mask = 1 << 8;
> +
> +	return (platform_cmds & send_cmd_mask) == send_cmd_mask;
> +}
> +
>  static int __init snp_init_platform_device(void)
>  {
>  	struct sev_guest_platform_data data;
> @@ -2593,6 +2639,24 @@ static int __init snp_init_platform_device(void)
>  		return -ENODEV;
>  
>  	pr_info("SNP guest platform device initialized.\n");
> +
> +	/*
> +	 * The VTPM device is available only if we have a SVSM and
> +	 * its VTPM supports the TPM_SEND_COMMAND platform command

s/VTPM/vTPM/g

Thanks,
Tom

> +	 */
> +	if (IS_ENABLED(CONFIG_TCG_PLATFORM) && snp_vmpl &&
> +	    is_svsm_vtpm_send_command_supported()) {
> +		struct tpm_platform_ops pops = {
> +			.sendrcv = snp_issue_svsm_vtpm_send_command,
> +		};
> +
> +		if (platform_device_add_data(&tpm_device, &pops, sizeof(pops)))
> +			return -ENODEV;
> +		if (platform_device_register(&tpm_device))
> +			return -ENODEV;
> +		pr_info("SNP SVSM VTPM platform device initialized\n");
> +	}
> +
>  	return 0;
>  }
>  device_initcall(snp_init_platform_device);

On Wed, 2024-12-11 at 10:30 -0600, Tom Lendacky wrote:
> On 12/10/24 08:34, Stefano Garzarella wrote:
[...]
> > +static bool is_svsm_vtpm_send_command_supported(void)
> > +{
> > +       struct svsm_call call = {};
> > +       u64 send_cmd_mask = 0;
> > +       u64 platform_cmds;
> > +       u64 features;
> > +       int ret;
> > +
> > +       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 must be zero */
> > +       if (features)
> > +               return false;
> 
> I think this check should be removed. The SVSM currently returns all
> zeroes for the features to allow for future support. If a new feature
> is added in the future, this then allows a driver that supports that
> feature to operate with a version of an SVSM that doesn't have that
> feature implemented. It also allows a version of the driver that
> doesn't know about that feature to work with an SVSM that has that
> feature.
> 
> A feature added to the vTPM shouldn't alter the behavior of something
> that isn't using or understands that feature.

I actually don't think this matters, because I can't see any reason to
use the SVSM features flag for the vTPM.  The reason is that the TPM
itself contains a versioned feature mechanism that external programs
already use, so there's no real need to duplicate it.

That said, I'm happy with either keeping or removing this.

Regards,

James

On Wed, Dec 11, 2024 at 12:02:49PM -0500, James Bottomley wrote:
>On Wed, 2024-12-11 at 10:30 -0600, Tom Lendacky wrote:
>> On 12/10/24 08:34, Stefano Garzarella wrote:
>[...]
>> > +static bool is_svsm_vtpm_send_command_supported(void)
>> > +{
>> > +       struct svsm_call call = {};
>> > +       u64 send_cmd_mask = 0;
>> > +       u64 platform_cmds;
>> > +       u64 features;
>> > +       int ret;
>> > +
>> > +       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 must be zero */
>> > +       if (features)
>> > +               return false;
>>
>> I think this check should be removed. The SVSM currently returns all
>> zeroes for the features to allow for future support. If a new feature
>> is added in the future, this then allows a driver that supports that
>> feature to operate with a version of an SVSM that doesn't have that
>> feature implemented. It also allows a version of the driver that
>> doesn't know about that feature to work with an SVSM that has that
>> feature.
>>
>> A feature added to the vTPM shouldn't alter the behavior of something
>> that isn't using or understands that feature.
>
>I actually don't think this matters, because I can't see any reason to
>use the SVSM features flag for the vTPM.  The reason is that the TPM
>itself contains a versioned feature mechanism that external programs
>already use, so there's no real need to duplicate it.
>
>That said, I'm happy with either keeping or removing this.

If we remove the check, should we print some warning if `feature` is not 
0 or just ignore it?

Thanks,
Stefano

On Wed, Dec 11, 2024 at 5:31 PM Tom Lendacky <thomas.lendacky@amd.com> wrote:
>
> On 12/10/24 08:34, Stefano Garzarella wrote:
> > From: James Bottomley <James.Bottomley@HansenPartnership.com>
> >
> > If the SNP boot has a SVSM, probe for the vTPM device by sending a
> > SVSM_VTPM_QUERY call (function 8). The SVSM will return a bitmap with
> > the TPM_SEND_COMMAND bit set only if the vTPM is present and it is able
> > to handle TPM commands at runtime.
> >
> > If a vTPM is found, register a platform device as "platform:tpm" so it
> > can be attached to the tpm_platform.c driver.
> >
> > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
> > [CC] Used SVSM_VTPM_QUERY to probe the TPM
> > Signed-off-by: Claudio Carvalho <cclaudio@linux.ibm.com>
> > [SG] Code adjusted with some changes introduced in 6.11
> > [SG] Used macro for SVSM_VTPM_CALL
> > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> > ---
> >  arch/x86/coco/sev/core.c | 64 ++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 64 insertions(+)
> >
> > diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c
> > index c5b0148b8c0a..ec0153fddc9e 100644
> > --- a/arch/x86/coco/sev/core.c
> > +++ b/arch/x86/coco/sev/core.c
> > @@ -21,6 +21,7 @@
> >  #include <linux/cpumask.h>
> >  #include <linux/efi.h>
> >  #include <linux/platform_device.h>
> > +#include <linux/tpm_platform.h>
> >  #include <linux/io.h>
> >  #include <linux/psp-sev.h>
> >  #include <linux/dmi.h>
> > @@ -2578,6 +2579,51 @@ static struct platform_device sev_guest_device = {
> >       .id             = -1,
> >  };
> >
> > +static struct platform_device tpm_device = {
> > +     .name           = "tpm",
> > +     .id             = -1,
> > +};
> > +
> > +static int snp_issue_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);
> > +}
> > +
> > +static bool is_svsm_vtpm_send_command_supported(void)
> > +{
> > +     struct svsm_call call = {};
> > +     u64 send_cmd_mask = 0;
> > +     u64 platform_cmds;
> > +     u64 features;
> > +     int ret;
> > +
> > +     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 must be zero */
> > +     if (features)
> > +             return false;
>
> I think this check should be removed. The SVSM currently returns all
> zeroes for the features to allow for future support. If a new feature is
> added in the future, this then allows a driver that supports that
> feature to operate with a version of an SVSM that doesn't have that
> feature implemented. It also allows a version of the driver that doesn't
> know about that feature to work with an SVSM that has that feature.

I couldn't find much in the specification, but is a feature considered
additive only?

Let me explain, since there's no negotiation, the driver can't disable
features, so if these are just additive, it's perfectly fine to remove
this check, but if these can change the behavior of the device, then
it's risky.

I'll give an example, let's say a future version of TCG TPM changes
the format of requests for whatever reason, I guess in that case we
could use a feature to tell the driver to use the new format. What
happens if the driver is old and doesn't support it?

Maybe in this case we can define a new supported command, so if we are
sure that the features are just additive, we can remove this check.

>
> A feature added to the vTPM shouldn't alter the behavior of something
> that isn't using or understands that feature.

Okay, so this confirms that features are only additive.
BTW it wasn't perfectly clear from the specification, so if it can be
added it would be better IMHO.

>
> > +
> > +     /* TPM_SEND_COMMAND - platform command 8 */
> > +     send_cmd_mask = 1 << 8;
> > +
> > +     return (platform_cmds & send_cmd_mask) == send_cmd_mask;
> > +}
> > +
> >  static int __init snp_init_platform_device(void)
> >  {
> >       struct sev_guest_platform_data data;
> > @@ -2593,6 +2639,24 @@ static int __init snp_init_platform_device(void)
> >               return -ENODEV;
> >
> >       pr_info("SNP guest platform device initialized.\n");
> > +
> > +     /*
> > +      * The VTPM device is available only if we have a SVSM and
> > +      * its VTPM supports the TPM_SEND_COMMAND platform command
>
> s/VTPM/vTPM/g

I'll fix it!

Thanks for the review,
Stefano

On Tue, Dec 10, 2024 at 03:34:23PM +0100, Stefano Garzarella wrote:

> +		if (platform_device_add_data(&tpm_device, &pops, sizeof(pops)))
> +			return -ENODEV;
> +		if (platform_device_register(&tpm_device))
> +			return -ENODEV;

This seems like an old fashioned way to instantiate a device. Why do
this? Just put the TPM driver here and forget about pops? Simple tpm
drivers are not very complex.

Jason

On Tue, 2024-12-10 at 10:40 -0400, Jason Gunthorpe wrote:
> On Tue, Dec 10, 2024 at 03:34:23PM +0100, Stefano Garzarella wrote:
> 
> > +               if (platform_device_add_data(&tpm_device, &pops,
> > sizeof(pops)))
> > +                       return -ENODEV;
> > +               if (platform_device_register(&tpm_device))
> > +                       return -ENODEV;
> 
> This seems like an old fashioned way to instantiate a device. Why do
> this? Just put the TPM driver here and forget about pops? Simple tpm
> drivers are not very complex.

This driver may be for the AMD SEV SVSM vTPM module, but there are
other platforms where there's an internal vTPM which might be contacted
via a platform specific enlightenment (Intel SNP and Microsoft
OpenHCL).  This separation of the platform device from the contact
mechanism is designed to eliminate the duplication of having a platform
device within each implementation and to make any bugs in the mssim
protocol centrally fixable (every vTPM currently speaks this).

Regards,

James

On Tue, Dec 10, 2024 at 09:55:41AM -0500, James Bottomley wrote:
> On Tue, 2024-12-10 at 10:40 -0400, Jason Gunthorpe wrote:
> > On Tue, Dec 10, 2024 at 03:34:23PM +0100, Stefano Garzarella wrote:
> > 
> > > +               if (platform_device_add_data(&tpm_device, &pops,
> > > sizeof(pops)))
> > > +                       return -ENODEV;
> > > +               if (platform_device_register(&tpm_device))
> > > +                       return -ENODEV;
> > 
> > This seems like an old fashioned way to instantiate a device. Why do
> > this? Just put the TPM driver here and forget about pops? Simple tpm
> > drivers are not very complex.
> 
> This driver may be for the AMD SEV SVSM vTPM module, but there are
> other platforms where there's an internal vTPM which might be contacted
> via a platform specific enlightenment (Intel SNP and Microsoft
> OpenHCL). 

Sure, that's what TPM drivers are for, give those platforms TPM drivers
too.

Why put a mini driver hidden under an already mini driver?

> This separation of the platform device from the contact
> mechanism is designed to eliminate the duplication of having a platform
> device within each implementation and to make any bugs in the mssim
> protocol centrally fixable (every vTPM currently speaks this).

That makes sense, but that isn't really what I see in this series?

Patch one just has tpm_class_ops send() invoke pops sendrcv() after
re-arranging the arguments?

It looks to me like there would be mert in adding a new op to
tpm_class_ops for the send/recv type operating mode and have the core
code manage the buffer singleton (is a global static even *correct*??)

After that, there is no meaningful shared code here, and maybe the
TPM_CHIP_FLAG_IRQ hack can be avoided too.

Simply call tpm_chip_alloc/register from the sev code directly and
provide an op that does the send/recv. Let the tpm core code deal with
everything else. It is much cleaner than platform devices and driver
data..

Jason

On Tue, Dec 10, 2024 at 4:04 PM Jason Gunthorpe <jgg@ziepe.ca> wrote:
>
> On Tue, Dec 10, 2024 at 09:55:41AM -0500, James Bottomley wrote:
> > On Tue, 2024-12-10 at 10:40 -0400, Jason Gunthorpe wrote:
> > > On Tue, Dec 10, 2024 at 03:34:23PM +0100, Stefano Garzarella wrote:
> > >
> > > > +               if (platform_device_add_data(&tpm_device, &pops,
> > > > sizeof(pops)))
> > > > +                       return -ENODEV;
> > > > +               if (platform_device_register(&tpm_device))
> > > > +                       return -ENODEV;
> > >
> > > This seems like an old fashioned way to instantiate a device. Why do
> > > this? Just put the TPM driver here and forget about pops? Simple tpm
> > > drivers are not very complex.
> >
> > This driver may be for the AMD SEV SVSM vTPM module, but there are
> > other platforms where there's an internal vTPM which might be contacted
> > via a platform specific enlightenment (Intel SNP and Microsoft
> > OpenHCL).
>
> Sure, that's what TPM drivers are for, give those platforms TPM drivers
> too.
>
> Why put a mini driver hidden under an already mini driver?
>
> > This separation of the platform device from the contact
> > mechanism is designed to eliminate the duplication of having a platform
> > device within each implementation and to make any bugs in the mssim
> > protocol centrally fixable (every vTPM currently speaks this).
>
> That makes sense, but that isn't really what I see in this series?
>
> Patch one just has tpm_class_ops send() invoke pops sendrcv() after
> re-arranging the arguments?
>
> It looks to me like there would be mert in adding a new op to
> tpm_class_ops for the send/recv type operating mode and have the core
> code manage the buffer singleton (is a global static even *correct*??)
>
> After that, there is no meaningful shared code here, and maybe the
> TPM_CHIP_FLAG_IRQ hack can be avoided too.

IIUC you are proposing the following steps:
- extend tpm_class_ops to add a new send_recv() op and use it in
tpm_try_transmit()
- call the code in tpm_platform_probe() directly in sev

This would remove the intermediate driver, but at this point is it
worth keeping tpm_platform_send() and tpm_platform_recv() in a header
or module, since these are not related to sev, but to MSSIM?

As James mentioned, other platforms may want to reuse it.

Thanks,
Stefano

>
> Simply call tpm_chip_alloc/register from the sev code directly and
> provide an op that does the send/recv. Let the tpm core code deal with
> everything else. It is much cleaner than platform devices and driver
> data..

On Wed, Dec 11, 2024 at 09:19:04AM +0100, Stefano Garzarella wrote:

> > After that, there is no meaningful shared code here, and maybe the
> > TPM_CHIP_FLAG_IRQ hack can be avoided too.
> 
> IIUC you are proposing the following steps:
> - extend tpm_class_ops to add a new send_recv() op and use it in
> tpm_try_transmit()

Yes, that seems to be the majority of your shared code.

> - call the code in tpm_platform_probe() directly in sev

Yes

> This would remove the intermediate driver, but at this point is it
> worth keeping tpm_platform_send() and tpm_platform_recv() in a header
> or module, since these are not related to sev, but to MSSIM?

Reuse *what* exactly? These are 10 both line funtions that just call
another function pointer. Where exactly is this common MSSIM stuff?

Stated another way, by adding send_Recv() op to tpm_class_ops you have
already allowed reuse of all the code in tpm_platform_send/recv().

Jason

On Wed, 11 Dec 2024 at 16:00, Jason Gunthorpe <jgg@ziepe.ca> wrote:
>
> On Wed, Dec 11, 2024 at 09:19:04AM +0100, Stefano Garzarella wrote:
>
> > > After that, there is no meaningful shared code here, and maybe the
> > > TPM_CHIP_FLAG_IRQ hack can be avoided too.
> >
> > IIUC you are proposing the following steps:
> > - extend tpm_class_ops to add a new send_recv() op and use it in
> > tpm_try_transmit()
>
> Yes, that seems to be the majority of your shared code.
>
> > - call the code in tpm_platform_probe() directly in sev
>
> Yes

I tried this, it's not bad, but I have a problem that I'm not sure how 
to solve. Basically, the functions used in tpm_platform_probe() (e.g. 
tpmm_chip_alloc, tpm2_probe, tpm_chip_register) are all defined in 
drivers/char/tpm/tpm.h
And in fact all users are in drivers/char/tpm.

So to use them directly in sev, we would have to move these definitions 
into include/linux/tpm.h or some other file in inlcude/. Is this 
acceptable for TPM maintainers?

Otherwise we need an intermediate module in drivers/char/tpm. Here we 
have 2 options:
1. continue as James did by creating a platform_device.
2. or we could avoid this by just exposing a registration API invoked by 
sev to specify the send_recv() callback to use. I mean something like 
renaming tpm_platform_probe() in tpm_platform_register(), and call it in 
snp_init_platform_device().

WDYT?

Thanks,
Stefano

On Thu Dec 19, 2024 at 5:35 PM EET, Stefano Garzarella wrote:
> So to use them directly in sev, we would have to move these definitions 
> into include/linux/tpm.h or some other file in inlcude/. Is this 
> acceptable for TPM maintainers?

There's only me.

I don't know.

What you want to put to include/linux/tpm.h anyway? I have not followed
this discussion.

BR, Jarkko

On Thu, Dec 19, 2024 at 05:40:58PM +0200, Jarkko Sakkinen wrote:
>On Thu Dec 19, 2024 at 5:35 PM EET, Stefano Garzarella wrote:
>> So to use them directly in sev, we would have to move these definitions
>> into include/linux/tpm.h or some other file in inlcude/. Is this
>> acceptable for TPM maintainers?
>
>There's only me.
>
>I don't know.
>
>What you want to put to include/linux/tpm.h anyway?

At least tpmm_chip_alloc(), tpm2_probe(), and tpm_chip_register()

>I have not followed this discussion.

Let me try to summarize what we are doing: We are writing a small TPM
driver to support AMD SEV-SNP SVSM. Basically SVSM defines some sort of
hypercalls, which the guest OS can call to talk to the emulated vTPM.

In the current version of this series, based on James' RFC, we have an
intermediate module (tpm_platform) and then another small driver
(platform_device) in arch/x86/coco/sev/core.c that registers the
callback to use.

To avoid the intermediate driver (Jason correct me if I misunderstood),
we want to register the `tpm_chip` with its `tpm_class_ops` directly in
arch/x86/coco/sev/core.c where it's easy to use "SVSM calls" (i.e.
svsm_perform_call_protocol()).

And here I have this problem, so I was proposing to expose these APIs.
BTW, we do have an alternative though that I proposed in the previous 
email that might avoid this.

Thanks,
Stefano

Hi Jarkko,

On Thu, 19 Dec 2024 at 17:07, Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> On Thu, Dec 19, 2024 at 05:40:58PM +0200, Jarkko Sakkinen wrote:
> >On Thu Dec 19, 2024 at 5:35 PM EET, Stefano Garzarella wrote:
> >> So to use them directly in sev, we would have to move these definitions
> >> into include/linux/tpm.h or some other file in inlcude/. Is this
> >> acceptable for TPM maintainers?
> >
> >There's only me.
> >
> >I don't know.
> >
> >What you want to put to include/linux/tpm.h anyway?
>
> At least tpmm_chip_alloc(), tpm2_probe(), and tpm_chip_register()
>
> >I have not followed this discussion.
>
> Let me try to summarize what we are doing: We are writing a small TPM
> driver to support AMD SEV-SNP SVSM. Basically SVSM defines some sort of
> hypercalls, which the guest OS can call to talk to the emulated vTPM.
>
> In the current version of this series, based on James' RFC, we have an
> intermediate module (tpm_platform) and then another small driver
> (platform_device) in arch/x86/coco/sev/core.c that registers the
> callback to use.
>
> To avoid the intermediate driver (Jason correct me if I misunderstood),
> we want to register the `tpm_chip` with its `tpm_class_ops` directly in
> arch/x86/coco/sev/core.c where it's easy to use "SVSM calls" (i.e.
> svsm_perform_call_protocol()).
>
> And here I have this problem, so I was proposing to expose these APIs.
> BTW, we do have an alternative though that I proposed in the previous
> email that might avoid this.

Any thought on this?

Thanks,
Stefano

On Tue Jan 14, 2025 at 12:42 PM EET, Stefano Garzarella wrote:
> Hi Jarkko,
>
> On Thu, 19 Dec 2024 at 17:07, Stefano Garzarella <sgarzare@redhat.com> wrote:
> >
> > On Thu, Dec 19, 2024 at 05:40:58PM +0200, Jarkko Sakkinen wrote:
> > >On Thu Dec 19, 2024 at 5:35 PM EET, Stefano Garzarella wrote:
> > >> So to use them directly in sev, we would have to move these definitions
> > >> into include/linux/tpm.h or some other file in inlcude/. Is this
> > >> acceptable for TPM maintainers?
> > >
> > >There's only me.
> > >
> > >I don't know.
> > >
> > >What you want to put to include/linux/tpm.h anyway?
> >
> > At least tpmm_chip_alloc(), tpm2_probe(), and tpm_chip_register()
> >
> > >I have not followed this discussion.
> >
> > Let me try to summarize what we are doing: We are writing a small TPM
> > driver to support AMD SEV-SNP SVSM. Basically SVSM defines some sort of
> > hypercalls, which the guest OS can call to talk to the emulated vTPM.
> >
> > In the current version of this series, based on James' RFC, we have an
> > intermediate module (tpm_platform) and then another small driver
> > (platform_device) in arch/x86/coco/sev/core.c that registers the
> > callback to use.
> >
> > To avoid the intermediate driver (Jason correct me if I misunderstood),
> > we want to register the `tpm_chip` with its `tpm_class_ops` directly in
> > arch/x86/coco/sev/core.c where it's easy to use "SVSM calls" (i.e.
> > svsm_perform_call_protocol()).
> >
> > And here I have this problem, so I was proposing to expose these APIs.
> > BTW, we do have an alternative though that I proposed in the previous
> > email that might avoid this.
>
> Any thought on this?

A redundant super low-quality TPM stack driver implemtation to support
only single vendor's vTPM with speculative generalization.

It's a formula for destruction really.

I don't know if I event want to comment on this. Figure out a better
solution I guess that works together sound with existing stack.

If that helps we could make the main TPM driver only Y/N (instead of
tristate).

>
> Thanks,
> Stefano

[1] "could be used by any platform which communicates with a TPM device."

BR, Jarkko

On Wed Jan 15, 2025 at 12:46 AM EET, Jarkko Sakkinen wrote:
> On Tue Jan 14, 2025 at 12:42 PM EET, Stefano Garzarella wrote:
> > Hi Jarkko,
> >
> > On Thu, 19 Dec 2024 at 17:07, Stefano Garzarella <sgarzare@redhat.com> wrote:
> > >
> > > On Thu, Dec 19, 2024 at 05:40:58PM +0200, Jarkko Sakkinen wrote:
> > > >On Thu Dec 19, 2024 at 5:35 PM EET, Stefano Garzarella wrote:
> > > >> So to use them directly in sev, we would have to move these definitions
> > > >> into include/linux/tpm.h or some other file in inlcude/. Is this
> > > >> acceptable for TPM maintainers?
> > > >
> > > >There's only me.
> > > >
> > > >I don't know.
> > > >
> > > >What you want to put to include/linux/tpm.h anyway?
> > >
> > > At least tpmm_chip_alloc(), tpm2_probe(), and tpm_chip_register()
> > >
> > > >I have not followed this discussion.
> > >
> > > Let me try to summarize what we are doing: We are writing a small TPM
> > > driver to support AMD SEV-SNP SVSM. Basically SVSM defines some sort of
> > > hypercalls, which the guest OS can call to talk to the emulated vTPM.
> > >
> > > In the current version of this series, based on James' RFC, we have an
> > > intermediate module (tpm_platform) and then another small driver
> > > (platform_device) in arch/x86/coco/sev/core.c that registers the
> > > callback to use.
> > >
> > > To avoid the intermediate driver (Jason correct me if I misunderstood),
> > > we want to register the `tpm_chip` with its `tpm_class_ops` directly in
> > > arch/x86/coco/sev/core.c where it's easy to use "SVSM calls" (i.e.
> > > svsm_perform_call_protocol()).
> > >
> > > And here I have this problem, so I was proposing to expose these APIs.
> > > BTW, we do have an alternative though that I proposed in the previous
> > > email that might avoid this.
> >
> > Any thought on this?
>
> A redundant super low-quality TPM stack driver implemtation to support
> only single vendor's vTPM with speculative generalization.
>
> It's a formula for destruction really.
>
> I don't know if I event want to comment on this. Figure out a better
> solution I guess that works together sound with existing stack.
>
> If that helps we could make the main TPM driver only Y/N (instead of
> tristate).

Also e.g. James' hmac encryption: not a single bug fixed by the author,
which does further reduce my ability to have any possible trust on this.

I do care quality over features, sorry.

BR, Jarkko

On Wed Jan 15, 2025 at 12:48 AM EET, Jarkko Sakkinen wrote:
> On Wed Jan 15, 2025 at 12:46 AM EET, Jarkko Sakkinen wrote:
> > On Tue Jan 14, 2025 at 12:42 PM EET, Stefano Garzarella wrote:
> > > Hi Jarkko,
> > >
> > > On Thu, 19 Dec 2024 at 17:07, Stefano Garzarella <sgarzare@redhat.com> wrote:
> > > >
> > > > On Thu, Dec 19, 2024 at 05:40:58PM +0200, Jarkko Sakkinen wrote:
> > > > >On Thu Dec 19, 2024 at 5:35 PM EET, Stefano Garzarella wrote:
> > > > >> So to use them directly in sev, we would have to move these definitions
> > > > >> into include/linux/tpm.h or some other file in inlcude/. Is this
> > > > >> acceptable for TPM maintainers?
> > > > >
> > > > >There's only me.
> > > > >
> > > > >I don't know.
> > > > >
> > > > >What you want to put to include/linux/tpm.h anyway?
> > > >
> > > > At least tpmm_chip_alloc(), tpm2_probe(), and tpm_chip_register()
> > > >
> > > > >I have not followed this discussion.
> > > >
> > > > Let me try to summarize what we are doing: We are writing a small TPM
> > > > driver to support AMD SEV-SNP SVSM. Basically SVSM defines some sort of
> > > > hypercalls, which the guest OS can call to talk to the emulated vTPM.
> > > >
> > > > In the current version of this series, based on James' RFC, we have an
> > > > intermediate module (tpm_platform) and then another small driver
> > > > (platform_device) in arch/x86/coco/sev/core.c that registers the
> > > > callback to use.
> > > >
> > > > To avoid the intermediate driver (Jason correct me if I misunderstood),
> > > > we want to register the `tpm_chip` with its `tpm_class_ops` directly in
> > > > arch/x86/coco/sev/core.c where it's easy to use "SVSM calls" (i.e.
> > > > svsm_perform_call_protocol()).
> > > >
> > > > And here I have this problem, so I was proposing to expose these APIs.
> > > > BTW, we do have an alternative though that I proposed in the previous
> > > > email that might avoid this.
> > >
> > > Any thought on this?
> >
> > A redundant super low-quality TPM stack driver implemtation to support
> > only single vendor's vTPM with speculative generalization.
> >
> > It's a formula for destruction really.
> >
> > I don't know if I event want to comment on this. Figure out a better
> > solution I guess that works together sound with existing stack.
> >
> > If that helps we could make the main TPM driver only Y/N (instead of
> > tristate).
>
> Also e.g. James' hmac encryption: not a single bug fixed by the author,
> which does further reduce my ability to have any possible trust on this.
>
> I do care quality over features, sorry.

One more rant.

It's engineering problem to find **a fit** for the existing art. For
You can set the constraint here as "no two TPM stacks".

I know also almost nothing about SVSM. E.g. I don't understand why a
vTPM cannot be seen as fTPM by the guest, and why this needs user
space exported device (please do not answer here, do a better job
instead).

Even if I wanted to say how this should be changed, I could not
because it too far away to make any possible sense to begin with.
And I don't want to take the risk of those words being used as an
argument later on, when I don't even know what I'm looking.

BR, Jarkko

On Tue, Jan 14, 2025 at 3:12 PM Jarkko Sakkinen <jarkko@kernel.org> wrote:
>
> On Wed Jan 15, 2025 at 12:48 AM EET, Jarkko Sakkinen wrote:
> > On Wed Jan 15, 2025 at 12:46 AM EET, Jarkko Sakkinen wrote:
> > > On Tue Jan 14, 2025 at 12:42 PM EET, Stefano Garzarella wrote:
> > > > Hi Jarkko,
> > > >
> > > > On Thu, 19 Dec 2024 at 17:07, Stefano Garzarella <sgarzare@redhat.com> wrote:
> > > > >
> > > > > On Thu, Dec 19, 2024 at 05:40:58PM +0200, Jarkko Sakkinen wrote:
> > > > > >On Thu Dec 19, 2024 at 5:35 PM EET, Stefano Garzarella wrote:
> > > > > >> So to use them directly in sev, we would have to move these definitions
> > > > > >> into include/linux/tpm.h or some other file in inlcude/. Is this
> > > > > >> acceptable for TPM maintainers?
> > > > > >
> > > > > >There's only me.
> > > > > >
> > > > > >I don't know.
> > > > > >
> > > > > >What you want to put to include/linux/tpm.h anyway?
> > > > >
> > > > > At least tpmm_chip_alloc(), tpm2_probe(), and tpm_chip_register()
> > > > >
> > > > > >I have not followed this discussion.
> > > > >
> > > > > Let me try to summarize what we are doing: We are writing a small TPM
> > > > > driver to support AMD SEV-SNP SVSM. Basically SVSM defines some sort of
> > > > > hypercalls, which the guest OS can call to talk to the emulated vTPM.
> > > > >
> > > > > In the current version of this series, based on James' RFC, we have an
> > > > > intermediate module (tpm_platform) and then another small driver
> > > > > (platform_device) in arch/x86/coco/sev/core.c that registers the
> > > > > callback to use.
> > > > >
> > > > > To avoid the intermediate driver (Jason correct me if I misunderstood),
> > > > > we want to register the `tpm_chip` with its `tpm_class_ops` directly in
> > > > > arch/x86/coco/sev/core.c where it's easy to use "SVSM calls" (i.e.
> > > > > svsm_perform_call_protocol()).
> > > > >
> > > > > And here I have this problem, so I was proposing to expose these APIs.
> > > > > BTW, we do have an alternative though that I proposed in the previous
> > > > > email that might avoid this.
> > > >
> > > > Any thought on this?
> > >
> > > A redundant super low-quality TPM stack driver implemtation to support
> > > only single vendor's vTPM with speculative generalization.
> > >
> > > It's a formula for destruction really.
> > >
> > > I don't know if I event want to comment on this. Figure out a better
> > > solution I guess that works together sound with existing stack.
> > >
> > > If that helps we could make the main TPM driver only Y/N (instead of
> > > tristate).
> >
> > Also e.g. James' hmac encryption: not a single bug fixed by the author,
> > which does further reduce my ability to have any possible trust on this.
> >
> > I do care quality over features, sorry.
>
> One more rant.
>
> It's engineering problem to find **a fit** for the existing art. For
> You can set the constraint here as "no two TPM stacks".
>
> I know also almost nothing about SVSM. E.g. I don't understand why a
> vTPM cannot be seen as fTPM by the guest, and why this needs user
> space exported device (please do not answer here, do a better job
> instead).

I can appreciate this viewpoint. It even surfaced Microsoft's fTPM
paper to me, which solves some interesting problems we need to solve
in SVSM too. So thanks for that.

Just to clarify, you're not asking for SVSM to implement the TIS-MMIO
interface instead, but rather to use the fTPM stack, which could make
SVSM calls a TEE device operation?

>
> Even if I wanted to say how this should be changed, I could not
> because it too far away to make any possible sense to begin with.
> And I don't want to take the risk of those words being used as an
> argument later on, when I don't even know what I'm looking.
>
> BR, Jarkko
>


-- 
-Dionna Glaze, PhD, CISSP, CCSP (she/her)

On Wed Jan 22, 2025 at 11:29 PM EET, Dionna Amalie Glaze wrote:
> I can appreciate this viewpoint. It even surfaced Microsoft's fTPM
> paper to me, which solves some interesting problems we need to solve
> in SVSM too. So thanks for that.
>
> Just to clarify, you're not asking for SVSM to implement the TIS-MMIO
> interface instead, but rather to use the fTPM stack, which could make
> SVSM calls a TEE device operation?

I don't really know what I'm asking because this is barely even a
PoC, and I state it like this knowingly.

You should make the argument, and the case for the solution. Then
it is my turn to comment on that scheme.

That said, I would not give high odds for acceptance of a duplicate
TPM stack succeeding.

BR, Jarkko

On Thu, Jan 23, 2025 at 11:50:40AM +0200, Jarkko Sakkinen wrote:
>On Wed Jan 22, 2025 at 11:29 PM EET, Dionna Amalie Glaze wrote:
>> I can appreciate this viewpoint. It even surfaced Microsoft's fTPM
>> paper to me, which solves some interesting problems we need to solve
>> in SVSM too. So thanks for that.
>>
>> Just to clarify, you're not asking for SVSM to implement the TIS-MMIO
>> interface instead, but rather to use the fTPM stack, which could make
>> SVSM calls a TEE device operation?
>
>I don't really know what I'm asking because this is barely even a
>PoC, and I state it like this knowingly.
>
>You should make the argument, and the case for the solution. Then
>it is my turn to comment on that scheme.

I'll check if I can use fTPM, in the meantime I had started to simplify
this series, avoiding the double stack and exposing some APIs from SEV
to probe the vTPM and to send the commands. The final driver in
drivers/char/tpm would be quite simple.

But I'll try to see if reusing fTPM is a feasible way, I like the idea.

>
>That said, I would not give high odds for acceptance of a duplicate
>TPM stack succeeding.

Got it ;-)

Thanks to everyone for the helpful feedbacks!

I've been a bit messy these days and I'm in FOSDEM next week, so I hope
not to take too long for the v2.

Stefano

On Thu Jan 23, 2025 at 12:09 PM EET, Stefano Garzarella wrote:
> On Thu, Jan 23, 2025 at 11:50:40AM +0200, Jarkko Sakkinen wrote:
> >On Wed Jan 22, 2025 at 11:29 PM EET, Dionna Amalie Glaze wrote:
> >> I can appreciate this viewpoint. It even surfaced Microsoft's fTPM
> >> paper to me, which solves some interesting problems we need to solve
> >> in SVSM too. So thanks for that.
> >>
> >> Just to clarify, you're not asking for SVSM to implement the TIS-MMIO
> >> interface instead, but rather to use the fTPM stack, which could make
> >> SVSM calls a TEE device operation?
> >
> >I don't really know what I'm asking because this is barely even a
> >PoC, and I state it like this knowingly.
> >
> >You should make the argument, and the case for the solution. Then
> >it is my turn to comment on that scheme.
>
> I'll check if I can use fTPM, in the meantime I had started to simplify
> this series, avoiding the double stack and exposing some APIs from SEV
> to probe the vTPM and to send the commands. The final driver in
> drivers/char/tpm would be quite simple.
>
> But I'll try to see if reusing fTPM is a feasible way, I like the idea.
>
> >
> >That said, I would not give high odds for acceptance of a duplicate
> >TPM stack succeeding.
>
> Got it ;-)
>
> Thanks to everyone for the helpful feedbacks!
>
> I've been a bit messy these days and I'm in FOSDEM next week, so I hope
> not to take too long for the v2.

Yeah, OK one thing that I want to say.

Nail the story. What is it about what is the problem what is the
motivation to solve it etc. If you have all that properly written
up then it is easier to forgive not that well nailed code and
give reasonable arguments.

And don't rush, I have all the time in the world ;-)

> Stefano

BR, Jarkko

On Thu Jan 23, 2025 at 1:46 PM EET, Jarkko Sakkinen wrote:
> On Thu Jan 23, 2025 at 12:09 PM EET, Stefano Garzarella wrote:
> > On Thu, Jan 23, 2025 at 11:50:40AM +0200, Jarkko Sakkinen wrote:
> > >On Wed Jan 22, 2025 at 11:29 PM EET, Dionna Amalie Glaze wrote:
> > >> I can appreciate this viewpoint. It even surfaced Microsoft's fTPM
> > >> paper to me, which solves some interesting problems we need to solve
> > >> in SVSM too. So thanks for that.
> > >>
> > >> Just to clarify, you're not asking for SVSM to implement the TIS-MMIO
> > >> interface instead, but rather to use the fTPM stack, which could make
> > >> SVSM calls a TEE device operation?
> > >
> > >I don't really know what I'm asking because this is barely even a
> > >PoC, and I state it like this knowingly.
> > >
> > >You should make the argument, and the case for the solution. Then
> > >it is my turn to comment on that scheme.
> >
> > I'll check if I can use fTPM, in the meantime I had started to simplify
> > this series, avoiding the double stack and exposing some APIs from SEV
> > to probe the vTPM and to send the commands. The final driver in
> > drivers/char/tpm would be quite simple.
> >
> > But I'll try to see if reusing fTPM is a feasible way, I like the idea.
> >
> > >
> > >That said, I would not give high odds for acceptance of a duplicate
> > >TPM stack succeeding.
> >
> > Got it ;-)
> >
> > Thanks to everyone for the helpful feedbacks!
> >
> > I've been a bit messy these days and I'm in FOSDEM next week, so I hope
> > not to take too long for the v2.
>
> Yeah, OK one thing that I want to say.
>
> Nail the story. What is it about what is the problem what is the
> motivation to solve it etc. If you have all that properly written
> up then it is easier to forgive not that well nailed code and
> give reasonable arguments.
>
> And don't rush, I have all the time in the world ;-)

Here the point is that if I don't fully understand the context
(starting explaining the obvious like what is SVSM) I might 
give some ridiculously wrong advice.

Then people come back to me and start blaming me on saying
opposite arguments. I hope you see where I'm standing here.
I neither don't want you to do useless and unproductive
work.

BR, Jarkko

On Thu, Jan 23, 2025 at 01:49:34PM +0200, Jarkko Sakkinen wrote:
>On Thu Jan 23, 2025 at 1:46 PM EET, Jarkko Sakkinen wrote:
>> On Thu Jan 23, 2025 at 12:09 PM EET, Stefano Garzarella wrote:
>> > On Thu, Jan 23, 2025 at 11:50:40AM +0200, Jarkko Sakkinen wrote:
>> > >On Wed Jan 22, 2025 at 11:29 PM EET, Dionna Amalie Glaze wrote:
>> > >> I can appreciate this viewpoint. It even surfaced Microsoft's fTPM
>> > >> paper to me, which solves some interesting problems we need to solve
>> > >> in SVSM too. So thanks for that.
>> > >>
>> > >> Just to clarify, you're not asking for SVSM to implement the TIS-MMIO
>> > >> interface instead, but rather to use the fTPM stack, which could make
>> > >> SVSM calls a TEE device operation?
>> > >
>> > >I don't really know what I'm asking because this is barely even a
>> > >PoC, and I state it like this knowingly.
>> > >
>> > >You should make the argument, and the case for the solution. Then
>> > >it is my turn to comment on that scheme.
>> >
>> > I'll check if I can use fTPM, in the meantime I had started to simplify
>> > this series, avoiding the double stack and exposing some APIs from SEV
>> > to probe the vTPM and to send the commands. The final driver in
>> > drivers/char/tpm would be quite simple.
>> >
>> > But I'll try to see if reusing fTPM is a feasible way, I like the idea.
>> >
>> > >
>> > >That said, I would not give high odds for acceptance of a duplicate
>> > >TPM stack succeeding.
>> >
>> > Got it ;-)
>> >
>> > Thanks to everyone for the helpful feedbacks!
>> >
>> > I've been a bit messy these days and I'm in FOSDEM next week, so I hope
>> > not to take too long for the v2.
>>
>> Yeah, OK one thing that I want to say.
>>
>> Nail the story. What is it about what is the problem what is the
>> motivation to solve it etc. If you have all that properly written
>> up then it is easier to forgive not that well nailed code and
>> give reasonable arguments.

Yes, I completely understand your point and I admit that the cover
letter and the commits description were not very informative, I will
fix them putting more context.

>>
>> And don't rush, I have all the time in the world ;-)

;-)

>
>Here the point is that if I don't fully understand the context
>(starting explaining the obvious like what is SVSM) I might
>give some ridiculously wrong advice.
>
>Then people come back to me and start blaming me on saying
>opposite arguments. I hope you see where I'm standing here.

I see it completely!

>I neither don't want you to do useless and unproductive
>work.

Thanks for that,
Stefano

On Tue, Jan 14, 2025 at 11:42:34AM +0100, Stefano Garzarella wrote:
> Hi Jarkko,
> 
> On Thu, 19 Dec 2024 at 17:07, Stefano Garzarella <sgarzare@redhat.com> wrote:
> >
> > On Thu, Dec 19, 2024 at 05:40:58PM +0200, Jarkko Sakkinen wrote:
> > >On Thu Dec 19, 2024 at 5:35 PM EET, Stefano Garzarella wrote:
> > >> So to use them directly in sev, we would have to move these definitions
> > >> into include/linux/tpm.h or some other file in inlcude/. Is this
> > >> acceptable for TPM maintainers?
> > >
> > >There's only me.
> > >
> > >I don't know.
> > >
> > >What you want to put to include/linux/tpm.h anyway?
> >
> > At least tpmm_chip_alloc(), tpm2_probe(), and tpm_chip_register()

The intention was that tpm drivers would be under drivers/char/tpm/

Do you really need to put your tpm driver in arch code? Historically
drivers in arch code have not worked out so well.

Meaning that you'd export some of your arch stuff for the tpm driver
to live in its natural home

Jason

On Tue, Jan 14, 2025 at 09:07:20AM -0400, Jason Gunthorpe wrote:
>On Tue, Jan 14, 2025 at 11:42:34AM +0100, Stefano Garzarella wrote:
>> Hi Jarkko,
>>
>> On Thu, 19 Dec 2024 at 17:07, Stefano Garzarella <sgarzare@redhat.com> wrote:
>> >
>> > On Thu, Dec 19, 2024 at 05:40:58PM +0200, Jarkko Sakkinen wrote:
>> > >On Thu Dec 19, 2024 at 5:35 PM EET, Stefano Garzarella wrote:
>> > >> So to use them directly in sev, we would have to move these definitions
>> > >> into include/linux/tpm.h or some other file in inlcude/. Is this
>> > >> acceptable for TPM maintainers?
>> > >
>> > >There's only me.
>> > >
>> > >I don't know.
>> > >
>> > >What you want to put to include/linux/tpm.h anyway?
>> >
>> > At least tpmm_chip_alloc(), tpm2_probe(), and tpm_chip_register()
>
>The intention was that tpm drivers would be under drivers/char/tpm/
>
>Do you really need to put your tpm driver in arch code? Historically
>drivers in arch code have not worked out so well.

I think I misinterpreted your answer here: https://lore.kernel.org/linux-coco/20241211150048.GJ1888283@ziepe.ca/ when I asked about calling "the code in
tpm_platform_probe() directly in sev".

I totally agree that it's not a good idea, which is why I had proposed
this: https://lore.kernel.org/linux-coco/CAGxU2F7QjQTnXsqYeKc0q03SQCoW+BHbej9Q2Z8gxbgu-3O2fA@mail.gmail.com/

   Otherwise we need an intermediate module in drivers/char/tpm. Here we
   have 2 options:
   1. continue as James did by creating a platform_device.
   2. or we could avoid this by just exposing a registration API invoked by
   sev to specify the send_recv() callback to use. I mean something like
   renaming tpm_platform_probe() in tpm_platform_register(), and call it in
   snp_init_platform_device().

I'm thinking of sending an RFC implementing 2 so we can discuss there,
it should be a good compromise between your suggestions and James'
version.

>
>Meaning that you'd export some of your arch stuff for the tpm driver
>to live in its natural home

@Tom do you think we can eventually expose sev API like
svsm_perform_call_protocol(), svsm_get_caa(), etc.?

Maybe option 2 that I proposed could avoid this and have sev register a
simple callback so that we avoid exposing these internal APIs.

Thanks,
Stefano

On Tue, Jan 14, 2025 at 05:51:33PM +0100, Stefano Garzarella wrote:
>   Otherwise we need an intermediate module in drivers/char/tpm. Here we
>   have 2 options:
>   1. continue as James did by creating a platform_device.
>   2. or we could avoid this by just exposing a registration API invoked by
>   sev to specify the send_recv() callback to use. I mean something like
>   renaming tpm_platform_probe() in tpm_platform_register(), and call it in
>   snp_init_platform_device().

You should not layer things on top of things. If you have a clearly
defined driver write it in the natural logical way and export the
symbols you need.

Either export TPM stuff to arch code, or export arch code to
TPM. Don't make crazy boutique shims to avoid simple exports.

> > Meaning that you'd export some of your arch stuff for the tpm driver
> > to live in its natural home
> 
> @Tom do you think we can eventually expose sev API like
> svsm_perform_call_protocol(), svsm_get_caa(), etc.?

We have lots of ways to make restricted exports now, you can use them
and export those symbols. There shouldn't be resistance to this.

Jason

On Wed, Dec 11, 2024 at 4:00 PM Jason Gunthorpe <jgg@ziepe.ca> wrote:
>
> On Wed, Dec 11, 2024 at 09:19:04AM +0100, Stefano Garzarella wrote:
>
> > > After that, there is no meaningful shared code here, and maybe the
> > > TPM_CHIP_FLAG_IRQ hack can be avoided too.
> >
> > IIUC you are proposing the following steps:
> > - extend tpm_class_ops to add a new send_recv() op and use it in
> > tpm_try_transmit()
>
> Yes, that seems to be the majority of your shared code.
>
> > - call the code in tpm_platform_probe() directly in sev
>
> Yes

Thanks for confirming!

>
> > This would remove the intermediate driver, but at this point is it
> > worth keeping tpm_platform_send() and tpm_platform_recv() in a header
> > or module, since these are not related to sev, but to MSSIM?
>
> Reuse *what* exactly? These are 10 both line funtions that just call
> another function pointer. Where exactly is this common MSSIM stuff?

Except for the call to pops->sendrcv(buffer) the rest depends on how
the TCG TPM reference implementation [1] expects the request/response
to be formatted (we refer to this protocol with MSSIM).

This format doesn't depend on sev, and as James said, OpenHCL for
example will have to use the same format (e.g. buffer defined by
struct tpm_send_cmd_req, filled with TPM_SEND_COMMAND, etc.), so
basically rewrite a similar function, because it also emulates the
vTPM using the TCG TPM reference implementation.

Now, I understand it's only 10 lines of code, but that code is
strictly TCG TPM dependent, so it might make sense to avoid having to
rewrite it for every implementation where the device is emulated by
TCG TPM.

>
> Stated another way, by adding send_Recv() op to tpm_class_ops you have
> already allowed reuse of all the code in tpm_platform_send/recv().

Partially, I mean the buffer format will always be the same for all
platforms (e.g. sev, OpenHCL, etc.), but how we read/write will be
different.
That is why I was saying to create a header with helpers that create
the request/parse the response as TCG TPM expects.

Thanks,
Stefano

[1] https://github.com/TrustedComputingGroup/TPM

On Wed, Dec 11, 2024 at 04:38:29PM +0100, Stefano Garzarella wrote:

> Except for the call to pops->sendrcv(buffer) the rest depends on how
> the TCG TPM reference implementation [1] expects the request/response
> to be formatted (we refer to this protocol with MSSIM).

Make a small inline helper to do the reformatting? Much better than a
layered driver.

> That is why I was saying to create a header with helpers that create
> the request/parse the response as TCG TPM expects.

Yes helpers sound better

Jason

On Wed, Dec 11, 2024 at 4:54 PM Jason Gunthorpe <jgg@ziepe.ca> wrote:
>
> On Wed, Dec 11, 2024 at 04:38:29PM +0100, Stefano Garzarella wrote:
>
> > Except for the call to pops->sendrcv(buffer) the rest depends on how
> > the TCG TPM reference implementation [1] expects the request/response
> > to be formatted (we refer to this protocol with MSSIM).
>
> Make a small inline helper to do the reformatting? Much better than a
> layered driver.
>
> > That is why I was saying to create a header with helpers that create
> > the request/parse the response as TCG TPM expects.
>
> Yes helpers sound better

Ack, I'll do in v2 (together with send_recv op) if there are no
objections or other ideas.

Thanks,
Stefano