From: Stefano Garzarella <sgarzare@redhat.com>
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
Register the device unconditionally. The support check (e.g. SVSM, cmd)
is in snp_svsm_vtpm_probe(), keeping all logic in one place.
This function is called during the driver's probe along with other
setup tasks like memory allocation.
Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
v6:
- added Tom's R-b
v4:
- explained better why we register it anyway in the commit message
---
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 efb43c9d3d30..acbd9bc526b1 100644
--- a/arch/x86/coco/sev/core.c
+++ b/arch/x86/coco/sev/core.c
@@ -2689,6 +2689,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))
@@ -2697,6 +2702,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.49.0On Thu, Apr 03, 2025 at 12:09:42PM +0200, Stefano Garzarella wrote:
> @@ -2697,6 +2702,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;
So I don't understand the design here:
You've exported the probe function - snp_svsm_vtpm_probe() - and you're
calling it in tpm_svsm_probe().
So why aren't you registering the platform device there too but are doing this
unconditional strange thing here?
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Tue, Apr 08, 2025 at 01:00:12PM +0200, Borislav Petkov wrote: >On Thu, Apr 03, 2025 at 12:09:42PM +0200, Stefano Garzarella wrote: >> @@ -2697,6 +2702,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; > >So I don't understand the design here: > >You've exported the probe function - snp_svsm_vtpm_probe() - and you're >calling it in tpm_svsm_probe(). > >So why aren't you registering the platform device there too but are doing this >unconditional strange thing here? We discussed a bit on v3, but I'm open to change it: https://lore.kernel.org/linux-integrity/nrn4ur66lz2ocbkkjl2bgiex3xbp552szerfhalsaefunqxf7p@ki7xf66zrf6u/ I tried to keep the logic of whether or not the driver is needed all in the tpm_svsm_probe()/snp_svsm_vtpm_probe() (where I check for SVSM). If you prefer to move some pieces here, though, I'm open. Thanks, Stefano
On Tue, Apr 08, 2025 at 01:08:36PM +0200, Stefano Garzarella wrote:
> We discussed a bit on v3, but I'm open to change it:
> https://lore.kernel.org/linux-integrity/nrn4ur66lz2ocbkkjl2bgiex3xbp552szerfhalsaefunqxf7p@ki7xf66zrf6u/
>
> I tried to keep the logic of whether or not the driver is needed all in
> the tpm_svsm_probe()/snp_svsm_vtpm_probe() (where I check for SVSM).
> If you prefer to move some pieces here, though, I'm open.
Yes please.
It doesn't make a whole lotta sense right now to register a TPM platform
driver at one place without even knowing you're running with an SVSM inside
the guest blob or not.
The usual approach is to register upon a successful detection.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Tue, Apr 08, 2025 at 01:28:20PM +0200, Borislav Petkov wrote:
>On Tue, Apr 08, 2025 at 01:08:36PM +0200, Stefano Garzarella wrote:
>> We discussed a bit on v3, but I'm open to change it:
>> https://lore.kernel.org/linux-integrity/nrn4ur66lz2ocbkkjl2bgiex3xbp552szerfhalsaefunqxf7p@ki7xf66zrf6u/
>>
>> I tried to keep the logic of whether or not the driver is needed all in
>> the tpm_svsm_probe()/snp_svsm_vtpm_probe() (where I check for SVSM).
>> If you prefer to move some pieces here, though, I'm open.
>
>Yes please.
>
>It doesn't make a whole lotta sense right now to register a TPM platform
>driver at one place without even knowing you're running with an SVSM inside
>the guest blob or not.
>
>The usual approach is to register upon a successful detection.
I see, so IIUC I can just apply the following change to this patch and
avoid to export snp_svsm_vtpm_probe() at all, right?
diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c
index acbd9bc526b1..fa83e6c7f990 100644
--- a/arch/x86/coco/sev/core.c
+++ b/arch/x86/coco/sev/core.c
@@ -2702,8 +2702,10 @@ 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;
+ if (snp_svsm_vtpm_probe()) {
+ if (platform_device_register(&tpm_svsm_device))
+ return -ENODEV;
+ }
pr_info("SNP guest platform device initialized.\n");
return 0;
Thanks,
Stefano
On Tue, Apr 08, 2025 at 01:54:07PM +0200, Stefano Garzarella wrote:
> I see, so IIUC I can just apply the following change to this patch and avoid
> to export snp_svsm_vtpm_probe() at all, right?
>
> diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c
> index acbd9bc526b1..fa83e6c7f990 100644
> --- a/arch/x86/coco/sev/core.c
> +++ b/arch/x86/coco/sev/core.c
> @@ -2702,8 +2702,10 @@ 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;
> + if (snp_svsm_vtpm_probe()) {
> + if (platform_device_register(&tpm_svsm_device))
> + return -ENODEV;
> + }
> pr_info("SNP guest platform device initialized.\n");
> return 0;
No, this should go in tpm_svsm_probe().
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Wed, 9 Apr 2025 at 12:21, Borislav Petkov <bp@alien8.de> wrote:
>
> On Tue, Apr 08, 2025 at 01:54:07PM +0200, Stefano Garzarella wrote:
> > I see, so IIUC I can just apply the following change to this patch and avoid
> > to export snp_svsm_vtpm_probe() at all, right?
> >
> > diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c
> > index acbd9bc526b1..fa83e6c7f990 100644
> > --- a/arch/x86/coco/sev/core.c
> > +++ b/arch/x86/coco/sev/core.c
> > @@ -2702,8 +2702,10 @@ 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;
> > + if (snp_svsm_vtpm_probe()) {
> > + if (platform_device_register(&tpm_svsm_device))
> > + return -ENODEV;
> > + }
> > pr_info("SNP guest platform device initialized.\n");
> > return 0;
>
> No, this should go in tpm_svsm_probe().
Sorry, maybe I missed something.
tpm_svsm.c registers the driver with module_platform_driver_probe().
Someone (the platform I guess) has to register the device by calling
platform_device_register(), as we already do for example for
sev_guest.
If we move platform_device_register() to tpm_svsm_probe() how will the
probe be invoked?
Thanks,
Stefano
On Wed, Apr 09, 2025 at 12:43:01PM +0200, Stefano Garzarella wrote:
> Sorry, maybe I missed something.
>
> tpm_svsm.c registers the driver with module_platform_driver_probe().
>
> Someone (the platform I guess) has to register the device by calling
> platform_device_register(), as we already do for example for
> sev_guest.
Maybe that platform device thing is the wrong approach. Why does the core code
need to register some dummy platform device in the first place? Why can't
drivers/char/tpm/tpm_svsm.c probe and init without it?
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On 4/9/25 06:31, Borislav Petkov wrote: > On Wed, Apr 09, 2025 at 12:43:01PM +0200, Stefano Garzarella wrote: >> Sorry, maybe I missed something. >> >> tpm_svsm.c registers the driver with module_platform_driver_probe(). >> >> Someone (the platform I guess) has to register the device by calling >> platform_device_register(), as we already do for example for >> sev_guest. > > Maybe that platform device thing is the wrong approach. Why does the core code > need to register some dummy platform device in the first place? Why can't > drivers/char/tpm/tpm_svsm.c probe and init without it? I think the platform device is the right approach (just like we do for the sev-guest driver), but I think we should only register the device if an SVSM is present. Then let the vTPM driver probe routine check if the SVSM vTPM support is present. So the vTPM driver wouldn't change, just snp_init_platform_device(): if (snp_vmpl && platform_device_register(&tpm_svsm_device)) Looking at the message that is issued after, maybe it should read "devices" now. Thanks, Tom >
On Wed, Apr 09, 2025 at 11:07:49AM -0500, Tom Lendacky wrote:
> So the vTPM driver wouldn't change, just snp_init_platform_device():
>
> if (snp_vmpl && platform_device_register(&tpm_svsm_device))
So this basically says that the SVSM is always sporting a vTPM emulation. But
you can build the cocont-svsm thing without it AFAICT.
So I'm guessing Stefano's suggestion here might make more sense:
https://lore.kernel.org/r/o2u7p3wb64lcc4sziunr274hyubkgmspzdjcvihbpzkw6mkvpo@sjq3vi4y2qfl
considering it all...
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On 4/9/25 13:45, Borislav Petkov wrote: > On Wed, Apr 09, 2025 at 11:07:49AM -0500, Tom Lendacky wrote: >> So the vTPM driver wouldn't change, just snp_init_platform_device(): >> >> if (snp_vmpl && platform_device_register(&tpm_svsm_device)) > > So this basically says that the SVSM is always sporting a vTPM emulation. But > you can build the cocont-svsm thing without it AFAICT. > > So I'm guessing Stefano's suggestion here might make more sense: > > https://lore.kernel.org/r/o2u7p3wb64lcc4sziunr274hyubkgmspzdjcvihbpzkw6mkvpo@sjq3vi4y2qfl > > considering it all... That way works for me, too. Thanks, Tom >
On Wed, Apr 09, 2025 at 02:16:40PM -0500, Tom Lendacky wrote: >On 4/9/25 13:45, Borislav Petkov wrote: >> On Wed, Apr 09, 2025 at 11:07:49AM -0500, Tom Lendacky wrote: >>> So the vTPM driver wouldn't change, just snp_init_platform_device(): >>> >>> if (snp_vmpl && platform_device_register(&tpm_svsm_device)) >> >> So this basically says that the SVSM is always sporting a vTPM emulation. But >> you can build the cocont-svsm thing without it AFAICT. >> >> So I'm guessing Stefano's suggestion here might make more sense: >> >> https://lore.kernel.org/r/o2u7p3wb64lcc4sziunr274hyubkgmspzdjcvihbpzkw6mkvpo@sjq3vi4y2qfl >> >> considering it all... > >That way works for me, too. Okay, it looks like we have an agreement! I'll apply that and send v7. Thanks, Stefano
On Wed, 9 Apr 2025 at 18:08, Tom Lendacky <thomas.lendacky@amd.com> wrote: > > On 4/9/25 06:31, Borislav Petkov wrote: > > On Wed, Apr 09, 2025 at 12:43:01PM +0200, Stefano Garzarella wrote: > >> Sorry, maybe I missed something. > >> > >> tpm_svsm.c registers the driver with module_platform_driver_probe(). > >> > >> Someone (the platform I guess) has to register the device by calling > >> platform_device_register(), as we already do for example for > >> sev_guest. > > > > Maybe that platform device thing is the wrong approach. Why does the core code > > need to register some dummy platform device in the first place? Why can't > > drivers/char/tpm/tpm_svsm.c probe and init without it? > > I think the platform device is the right approach (just like we do for the > sev-guest driver), but I think we should only register the device if an > SVSM is present. Then let the vTPM driver probe routine check if the SVSM > vTPM support is present. I see, agree. > > So the vTPM driver wouldn't change, just snp_init_platform_device(): > > if (snp_vmpl && platform_device_register(&tpm_svsm_device)) I can do if we agree on that. > > Looking at the message that is issued after, maybe it should read > "devices" now. Good catch, I'll update the pr_info() message! Thanks, Stefano
On Wed, 2025-04-09 at 13:31 +0200, Borislav Petkov wrote: > On Wed, Apr 09, 2025 at 12:43:01PM +0200, Stefano Garzarella wrote: > > Sorry, maybe I missed something. > > > > tpm_svsm.c registers the driver with > > module_platform_driver_probe(). > > > > Someone (the platform I guess) has to register the device by > > calling platform_device_register(), as we already do for example > > for sev_guest. > > Maybe that platform device thing is the wrong approach. Why does the > core code need to register some dummy platform device in the first > place? Why can't drivers/char/tpm/tpm_svsm.c probe and init without > it? Because of the way driver and device matching works in Linux. We have to have a struct device because that sits at the he heart of the TPM driver binding. If we have a struct device, it has to sit on a bus (because that's the Linux design) and if we don't have a bus then we have to use a platform device (or, now, we could use a struct device on the faux bus). Busses can be either physical (PCI, GSC, ...) and abstract (virtio, xen, scsi, ...), so it's not impossible, if the SVSM has more than one device, that it should have it's own SVSM bus which we could then act a bit like the virtio bus and the SVSM vTPM struct device could sit on this (the TPM subsystem, like most driver subsystems, doesn't care about busses, it only cares that the abstract bus device id matching works). Regards, James
On Wed, Apr 09, 2025 at 08:22:37AM -0400, James Bottomley wrote:
> Because of the way driver and device matching works in Linux. We have
> to have a struct device because that sits at the he heart of the TPM
> driver binding. If we have a struct device, it has to sit on a bus
> (because that's the Linux design) and if we don't have a bus then we
> have to use a platform device
Thanks for elaborating!
> (or, now, we could use a struct device on the faux bus). Busses can be
> either physical (PCI, GSC, ...) and abstract (virtio, xen, scsi, ...), so
> it's not impossible, if the SVSM has more than one device, that it should
> have it's own SVSM bus which we could then act a bit like the virtio bus and
> the SVSM vTPM struct device could sit on this
I guess we should keep this in mind. Depending on what else needs to talk to
the SVSM in the future...
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Wed, 9 Apr 2025 at 14:22, James Bottomley <James.Bottomley@hansenpartnership.com> wrote: > > On Wed, 2025-04-09 at 13:31 +0200, Borislav Petkov wrote: > > On Wed, Apr 09, 2025 at 12:43:01PM +0200, Stefano Garzarella wrote: > > > Sorry, maybe I missed something. > > > > > > tpm_svsm.c registers the driver with > > > module_platform_driver_probe(). > > > > > > Someone (the platform I guess) has to register the device by > > > calling platform_device_register(), as we already do for example > > > for sev_guest. > > > > Maybe that platform device thing is the wrong approach. Why does the > > core code need to register some dummy platform device in the first > > place? Why can't drivers/char/tpm/tpm_svsm.c probe and init without > > it? > > Because of the way driver and device matching works in Linux. We have > to have a struct device because that sits at the he heart of the TPM > driver binding. If we have a struct device, it has to sit on a bus > (because that's the Linux design) and if we don't have a bus then we > have to use a platform device (or, now, we could use a struct device on > the faux bus). I tried to look at faux bus, but IIUC, it doesn't fit. I mean, we could use it if we had a driver here in sev/core.c, but using a separate module for the tpm-svsm driver, how do we get the module to load when we find out that the device is there? In short, my question: how do we load the module automatically when we discover the device? faux seems more useful to me when there's no need to discover the device, but loading the module itself starts everything. If, on the other hand, we want to have it load when we discover it, we have to either have a bus or have core code that registers a platform_device that will then be recognized by the driver in a separate module. > Busses can be either physical (PCI, GSC, ...) and > abstract (virtio, xen, scsi, ...), so it's not impossible, if the SVSM > has more than one device, that it should have it's own SVSM bus which > we could then act a bit like the virtio bus and the SVSM vTPM struct > device could sit on this (the TPM subsystem, like most driver > subsystems, doesn't care about busses, it only cares that the abstract > bus device id matching works). Yes, I'm also looking at introducing a svsm bus as we've already discussed, but that's going to take some time. In the end though, platform_device is not that bad IMO. The tpm-svsm is really a device provided by the (virtual) platform, so doing some sort of discovery of the bus in sev/core.c and registering the device if discovered might be a compromise for now until we develop the bus. If you agree, I'll move all the discovery here in sev/core.c as I suggested earlier, so that when we get the bus we'll move this code somehow into the bus. @Borsilav @James WDYT? Thanks, Stefano
On Thu, Apr 03, 2025 at 12:09:42PM +0200, Stefano Garzarella wrote:
> From: Stefano Garzarella <sgarzare@redhat.com>
>
> 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
>
> Register the device unconditionally. The support check (e.g. SVSM, cmd)
> is in snp_svsm_vtpm_probe(), keeping all logic in one place.
> This function is called during the driver's probe along with other
> setup tasks like memory allocation.
>
> Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
> v6:
> - added Tom's R-b
> v4:
> - explained better why we register it anyway in the commit message
> ---
> 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 efb43c9d3d30..acbd9bc526b1 100644
> --- a/arch/x86/coco/sev/core.c
> +++ b/arch/x86/coco/sev/core.c
> @@ -2689,6 +2689,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))
> @@ -2697,6 +2702,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.49.0
>
>
Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
BR, Jarkko
© 2016 - 2026 Red Hat, Inc.