Re: [RFC PATCH 01/27] coco/tdx-host: Introduce a "tdx_host" device

[<dan.j.williams@intel.com>]

Jonathan Cameron wrote:
> On Fri, 19 Sep 2025 07:22:10 -0700
> Dan Williams <dan.j.williams@intel.com> wrote:
> 
> > From: Chao Gao <chao.gao@intel.com>
> > 
> > TDX depends on a platform firmware module that is invoked via instructions
> > similar to vmenter (i.e. enter into a new privileged "root-mode" context to
> > manage private memory and private device mechanisms). It is a software
> > construct that depends on the CPU vmxon state to enable invocation of
> > TDX-module ABIs. Unlike other Trusted Execution Environment (TEE) platform
> > implementations that employ a firmware module running on a PCI device with
> > an MMIO mailbox for communication, TDX has no hardware device to point to
> > as the TEE Secure Manager (TSM).
> > 
> > Create a virtual device not only to align with other implementations but
> > also to make it easier to
> > 
> >  - expose metadata (e.g., TDX module version, seamldr version etc) to
> >    the userspace as device attributes
> > 
> >  - implement firmware uploader APIs which are tied to a device. This is
> >    needed to support TDX module runtime updates
> > 
> >  - enable TDX Connect which will share a common infrastructure with other
> >    platform implementations. In the TDX Connect context, every
> >    architecture has a TSM, represented by a PCIe or virtual device. The
> >    new "tdx_host" device will serve the TSM role.
> > 
> > A faux device is used as for TDX because the TDX module is singular within
> > the system and lacks associated platform resources. Using a faux device
> > eliminates the need to create a stub bus.
> > 
> > The call to tdx_enable() makes the new module independent of kvm_intel.ko.
> > For example, TDX Connect may be used to established to PCIe link encryption
> > even if a TVM is never launched.  For now, just create the common loading
> > infrastructure.
> > 
> > Co-developed-by: Xu Yilun <yilun.xu@linux.intel.com>
> > Signed-off-by: Xu Yilun <yilun.xu@linux.intel.com>
> > Signed-off-by: Chao Gao <chao.gao@intel.com>
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> 
> I'm only taking a look to see a second example of how the core
> code is used as I care mostly about the ARM one.  Anyhow, a
> few passing comments inline.

Thanks for taking a look at a cross-arch RFC.

> 
> > diff --git a/drivers/virt/coco/tdx-host/tdx-host.c b/drivers/virt/coco/tdx-host/tdx-host.c
> > new file mode 100644
> > index 000000000000..49c205913ef6
> > --- /dev/null
> > +++ b/drivers/virt/coco/tdx-host/tdx-host.c
> > @@ -0,0 +1,52 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * TDX host user interface driver
> > + *
> > + * Copyright (C) 2025 Intel Corporation
> > + */
> > +
> > +#include <linux/kernel.h>
> 
> There is general rework ongoing to stop including kernel.h except
> where strictly necessary.  Please check exactly what you need and
> see if one or more more specific headers is appropriate.

Sure, easy enough to clean up.

> > +#include <linux/module.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/sysfs.h>
> 
> Bring headers in with the patch that first uses them. I'm not immediately
> spotting anything from this one in this patch.  Doing that just makes
> it easier to see if there are excess headers included at the end of
> building up the driver across a series.
> 
> > +#include <linux/device/faux.h>
> > +#include <asm/cpu_device_id.h>
> > +#include <asm/tdx.h>
> > +#include <asm/tdx_global_metadata.h>
> > +
> > +static const struct x86_cpu_id tdx_host_ids[] = {
> > +	X86_MATCH_FEATURE(X86_FEATURE_TDX_HOST_PLATFORM, NULL),
> > +	{}
> > +};
> > +MODULE_DEVICE_TABLE(x86cpu, tdx_host_ids);
> > +
> > +static struct faux_device *fdev;
> > +
> > +static int __init tdx_host_init(void)
> > +{
> > +	int r;
> > +
> > +	if (!x86_match_cpu(tdx_host_ids))
> > +		return -ENODEV;
> > +
> > +	/* Enable the usage of SEAMCALLs */
> 
> What's the logic behind doing that here rather than in probe
> for the faux device?  Perhaps add something to this comment.

For this specific case of tdx_enable() it will be obviated by the fact
that the new direction is to always enable TDX early [1].

[1]: http://lore.kernel.org/20251010220403.987927-1-seanjc@google.com

Otherwise there are cases where we might create the device without a
driver. E.g. sysfs ABI only for updates, vs also enabling PCI/TSM
services via a driver attached to this device.