| Message ID | 20241210143423.101774-4-sgarzare@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Enlightened vTPM support for SVSM on SEV-SNP | expand |
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, 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 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, 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
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 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
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);