| Message ID | 20230326062039.341479-1-sathyanarayanan.kuppuswamy@linux.intel.com |
|---|---|
| Headers | show |
| Series | TDX Guest Quote generation support | expand |
On Sat, Mar 25, 2023 at 11:20:38PM -0700, Kuppuswamy Sathyanarayanan wrote: > Since GetQuote support requires usage of DMA APIs, convert TDX guest > driver to a platform driver. Sorry, but that's not a valid reason to use a platform device for fake things like this: > +static struct platform_device *tdx_dev; Especially a single static one. > +static int tdx_guest_probe(struct platform_device *pdev) > +{ > + if (tdx_register_event_irq_cb(attestation_callback_handler, pdev)) > + return -EIO; > + > + return misc_register(&tdx_misc_dev); > +} > + > +static int tdx_guest_remove(struct platform_device *pdev) > +{ > + tdx_unregister_event_irq_cb(attestation_callback_handler, pdev); > + misc_deregister(&tdx_misc_dev); > + return 0; > +} > + > +static struct platform_driver tdx_guest_driver = { > + .probe = tdx_guest_probe, > + .remove = tdx_guest_remove, > + .driver.name = KBUILD_MODNAME, > +}; > + > static const struct x86_cpu_id tdx_guest_ids[] = { > X86_MATCH_FEATURE(X86_FEATURE_TDX_GUEST, NULL), > {} > @@ -84,16 +310,35 @@ MODULE_DEVICE_TABLE(x86cpu, tdx_guest_ids); > > static int __init tdx_guest_init(void) > { > + int ret; > + > if (!x86_match_cpu(tdx_guest_ids)) > return -ENODEV; > > - return misc_register(&tdx_misc_dev); > + ret = platform_driver_register(&tdx_guest_driver); > + if (ret) { > + pr_err("failed to register driver, err=%d\n", ret); > + return ret; > + } No, please do not create a fake platform driver. > + tdx_dev = platform_device_register_simple(KBUILD_MODNAME, > + PLATFORM_DEVID_NONE, > + NULL, 0); And please do not create a fake platform device. As always, do not create fake platform devices for things that are NOT platform devices. If this device needs DMA (but why?) then make it a real device and tie it to the bus it belongs to (that it is obviously doing DMA on.) But as-is, this isn't ok, sorry. thanks, greg k-h
Hi Greg, Thanks for the review comments. On 3/25/23 11:34 PM, Greg KH wrote: > On Sat, Mar 25, 2023 at 11:20:38PM -0700, Kuppuswamy Sathyanarayanan wrote: >> Since GetQuote support requires usage of DMA APIs, convert TDX guest >> driver to a platform driver. > > Sorry, but that's not a valid reason to use a platform device for fake > things like this: > >> +static struct platform_device *tdx_dev; > > Especially a single static one. > >> +static int tdx_guest_probe(struct platform_device *pdev) >> +{ >> + if (tdx_register_event_irq_cb(attestation_callback_handler, pdev)) >> + return -EIO; >> + >> + return misc_register(&tdx_misc_dev); >> +} >> + >> +static int tdx_guest_remove(struct platform_device *pdev) >> +{ >> + tdx_unregister_event_irq_cb(attestation_callback_handler, pdev); >> + misc_deregister(&tdx_misc_dev); >> + return 0; >> +} >> + >> +static struct platform_driver tdx_guest_driver = { >> + .probe = tdx_guest_probe, >> + .remove = tdx_guest_remove, >> + .driver.name = KBUILD_MODNAME, >> +}; >> + >> static const struct x86_cpu_id tdx_guest_ids[] = { >> X86_MATCH_FEATURE(X86_FEATURE_TDX_GUEST, NULL), >> {} >> @@ -84,16 +310,35 @@ MODULE_DEVICE_TABLE(x86cpu, tdx_guest_ids); >> >> static int __init tdx_guest_init(void) >> { >> + int ret; >> + >> if (!x86_match_cpu(tdx_guest_ids)) >> return -ENODEV; >> >> - return misc_register(&tdx_misc_dev); >> + ret = platform_driver_register(&tdx_guest_driver); >> + if (ret) { >> + pr_err("failed to register driver, err=%d\n", ret); >> + return ret; >> + } > > No, please do not create a fake platform driver. > >> + tdx_dev = platform_device_register_simple(KBUILD_MODNAME, >> + PLATFORM_DEVID_NONE, >> + NULL, 0); > > And please do not create a fake platform device. > > As always, do not create fake platform devices for things that are NOT > platform devices. > > If this device needs DMA (but why?) then make it a real device and tie Since the Quote generation process requires the guest and VMM to share data, we need to use shared buffers. But in TDX guest, VMM cannot directly access the guest private memory. Any memory that is required for communication with the VMM must be shared explicitly. One way is to allocate the memory on demand and share it explicitly using set_memory_decrypted() call. Although this approach is clean, since it breaks the direct mapping, it is not efficient. An alternative way is to reserve a pool of shared buffers during boot time and re-use them for guest/VMM communication. Since in TDX, DMA API is configured to tap into the SWIOTLB memory framework (which is shared by default), we try to use dma_alloc_* APIs to allocate the shared buffers from the shared pool without breaking the direct map. If usage of DMA APIs / platform device is not acceptable for this use case, an alternative approach is to allocate a fixed number of shared buffers during the TDX guest driver probe and use it for GetQuote requests. Although it would limit the amount of memory we can use for GetQuote requests at a time and also reserve a chunk of memory during the init() time, I think it is an acceptable tradeoff when compared to alternative choices. The AMD SEV guest driver also adopts the similar approach. Please let me know if this approach is acceptable. > it to the bus it belongs to (that it is obviously doing DMA on.) > > thanks, > > greg k-h
On Sun, Mar 26, 2023 at 12:06:26PM -0700, Sathyanarayanan Kuppuswamy wrote: > If usage of DMA APIs / platform device is not acceptable for this use case, It is not ok to use a platform device for this because you just do not have a platform device for it, don't make one up out of thin air please, as that really doesn't even have the correct bindings to the DMA memory that you want here. > an alternative approach is to allocate a fixed number of shared buffers during > the TDX guest driver probe and use it for GetQuote requests. Although it would > limit the amount of memory we can use for GetQuote requests at a time and also > reserve a chunk of memory during the init() time, I think it is an acceptable > tradeoff when compared to alternative choices. The AMD SEV guest driver also > adopts the similar approach. Please let me know if this approach is acceptable. This sounds like a better approach. thanks, greg k-h
On Sat, Mar 25, 2023 at 11:20 PM Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> wrote: > > Hi All, > > In TDX guest, the attestation process is used to verify the TDX guest > trustworthiness to other entities before provisioning secrets to the > guest. > > The TDX guest attestation process consists of two steps: > > 1. TDREPORT generation > 2. Quote generation. > > The First step (TDREPORT generation) involves getting the TDX guest > measurement data in the format of TDREPORT which is further used to > validate the authenticity of the TDX guest. The second step involves > sending the TDREPORT to a Quoting Enclave (QE) server to generate a > remotely verifiable Quote. TDREPORT by design can only be verified on > the local platform. To support remote verification of the TDREPORT, > TDX leverages Intel SGX Quoting Enclave to verify the TDREPORT > locally and convert it to a remotely verifiable Quote. Although > attestation software can use communication methods like TCP/IP or > vsock to send the TDREPORT to QE, not all platforms support these > communication models. So TDX GHCI specification [1] defines a method > for Quote generation via hypercalls. Please check the discussion from > Google [2] and Alibaba [3] which clarifies the need for hypercall based Thanks Sathyanarayanan for submitting patches again. I just wanted to reiterate what I said before that having a clean TDVMCALL based interface to get TDX Quote without any virtio/vsock dependency is critical for us to support many use cases.
+Chong Cai Adding a colleague per his request since he's not subscribed to the list yet. On Mon, Mar 27, 2023 at 10:36 AM Erdem Aktas <erdemaktas@google.com> wrote: > > On Sat, Mar 25, 2023 at 11:20 PM Kuppuswamy Sathyanarayanan > <sathyanarayanan.kuppuswamy@linux.intel.com> wrote: > > > > Hi All, > > > > In TDX guest, the attestation process is used to verify the TDX guest > > trustworthiness to other entities before provisioning secrets to the > > guest. > > > > The TDX guest attestation process consists of two steps: > > > > 1. TDREPORT generation > > 2. Quote generation. > > > > The First step (TDREPORT generation) involves getting the TDX guest > > measurement data in the format of TDREPORT which is further used to > > validate the authenticity of the TDX guest. The second step involves > > sending the TDREPORT to a Quoting Enclave (QE) server to generate a > > remotely verifiable Quote. TDREPORT by design can only be verified on > > the local platform. To support remote verification of the TDREPORT, > > TDX leverages Intel SGX Quoting Enclave to verify the TDREPORT > > locally and convert it to a remotely verifiable Quote. Although > > attestation software can use communication methods like TCP/IP or > > vsock to send the TDREPORT to QE, not all platforms support these > > communication models. So TDX GHCI specification [1] defines a method > > for Quote generation via hypercalls. Please check the discussion from > > Google [2] and Alibaba [3] which clarifies the need for hypercall based > Thanks Sathyanarayanan for submitting patches again. > > I just wanted to reiterate what I said before that having a clean > TDVMCALL based interface to get TDX Quote without any virtio/vsock > dependency is critical for us to support many use cases.
On Tue, Mar 28, 2023 at 12:59 PM Dionna Amalie Glaze <dionnaglaze@google.com> wrote: > > +Chong Cai > > Adding a colleague per his request since he's not subscribed to the list yet. > > On Mon, Mar 27, 2023 at 10:36 AM Erdem Aktas <erdemaktas@google.com> wrote: > > > > On Sat, Mar 25, 2023 at 11:20 PM Kuppuswamy Sathyanarayanan > > <sathyanarayanan.kuppuswamy@linux.intel.com> wrote: > > > > > > Hi All, > > > > > > In TDX guest, the attestation process is used to verify the TDX guest > > > trustworthiness to other entities before provisioning secrets to the > > > guest. > > > > > > The TDX guest attestation process consists of two steps: > > > > > > 1. TDREPORT generation > > > 2. Quote generation. > > > > > > The First step (TDREPORT generation) involves getting the TDX guest > > > measurement data in the format of TDREPORT which is further used to > > > validate the authenticity of the TDX guest. The second step involves > > > sending the TDREPORT to a Quoting Enclave (QE) server to generate a > > > remotely verifiable Quote. TDREPORT by design can only be verified on > > > the local platform. To support remote verification of the TDREPORT, > > > TDX leverages Intel SGX Quoting Enclave to verify the TDREPORT > > > locally and convert it to a remotely verifiable Quote. Although > > > attestation software can use communication methods like TCP/IP or > > > vsock to send the TDREPORT to QE, not all platforms support these > > > communication models. So TDX GHCI specification [1] defines a method > > > for Quote generation via hypercalls. Please check the discussion from > > > Google [2] and Alibaba [3] which clarifies the need for hypercall based > > Thanks Sathyanarayanan for submitting patches again. > > > > I just wanted to reiterate what I said before that having a clean > > TDVMCALL based interface to get TDX Quote without any virtio/vsock > > dependency is critical for us to support many use cases. > > +1 to Erdem's point. A simple TDVMCALL interface could make it much > easier for user cases that can not depend on virtio and vsock. > Without the TDVMCALL, it will largely limit those user cases to adopt TDX. > Thanks Sathyanarayanan for submitting this patch. > -- > -Dionna Glaze, PhD (she/her)