Re: [PATCH v4 05/10] samples/devsec: Introduce a PCI device-security bus + endpoint sample

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

On Thu, 17 Jul 2025 11:33:53 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> Establish just enough emulated PCI infrastructure to register a sample
> TSM (platform security manager) driver and have it discover an IDE + TEE
> (link encryption + device-interface security protocol (TDISP)) capable
> device.
> 
> Use the existing a CONFIG_PCI_BRIDGE_EMUL to emulate an IDE capable root
> port, and open code the emulation of an endpoint device via simulated
> configuration cycle responses.
> 
> The devsec_tsm driver responds to the PCI core TSM operations as if it
> successfully exercised the given interface security protocol message.
> 
> The devsec_bus and devsec_tsm drivers can be loaded in either order to
> reflect cases like SEV-TIO where the TSM is PCI-device firmware, and
> cases like TDX Connect where the TSM is a software agent running on the
> host CPU.
> 
> Follow-on patches add common code for TSM managed IDE establishment. For
> now, just successfully complete setup and teardown of the DSM (device
> security manager) context as a building block for management of TDI
> (trusted device interface) instances.
> 
>  # modprobe devsec_bus
>     devsec_bus devsec_bus: PCI host bridge to bus 10000:00
>     pci_bus 10000:00: root bus resource [bus 00-01]
>     pci_bus 10000:00: root bus resource [mem 0xf000000000-0xffffffffff 64bit]
>     pci 10000:00:00.0: [8086:7075] type 01 class 0x060400 PCIe Root Port
>     pci 10000:00:00.0: PCI bridge to [bus 00]
>     pci 10000:00:00.0:   bridge window [io  0x0000-0x0fff]
>     pci 10000:00:00.0:   bridge window [mem 0x00000000-0x000fffff]
>     pci 10000:00:00.0:   bridge window [mem 0x00000000-0x000fffff 64bit pref]
>     pci 10000:00:00.0: bridge configuration invalid ([bus 00-00]), reconfiguring
>     pci 10000:01:00.0: [8086:ffff] type 00 class 0x000000 PCIe Endpoint
>     pci 10000:01:00.0: BAR 0 [mem 0xf000000000-0xf0001fffff 64bit pref]
>     pci_doe_abort: pci 10000:01:00.0: DOE: [100] Issuing Abort
>     pci_doe_cache_protocols: pci 10000:01:00.0: DOE: [100] Found protocol 0 vid: 1 prot: 1
>     pci 10000:01:00.0: disabling ASPM on pre-1.1 PCIe device.  You can enable it with 'pcie_aspm=force'
>     pci 10000:00:00.0: PCI bridge to [bus 01]
>     pci_bus 10000:01: busn_res: [bus 01] end is updated to 01
>  # modprobe devsec_tsm
>     devsec_tsm_pci_probe: pci 10000:01:00.0: devsec: tsm enabled
>     __pci_tsm_init: pci 10000:01:00.0: TSM: Device security capabilities detected ( ide tee ), TSM attach
> 
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Lukas Wunner <lukas@wunner.de>
> Cc: Samuel Ortiz <sameo@rivosinc.com>
> Cc: Alexey Kardashevskiy <aik@amd.com>
> Cc: Xu Yilun <yilun.xu@linux.intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

A fairly superficial review.  Too much staring at code today
to check the emulation was right and have any chance of spotting bugs!

> diff --git a/samples/devsec/bus.c b/samples/devsec/bus.c
> new file mode 100644
> index 000000000000..675e185fcf79
> --- /dev/null
> +++ b/samples/devsec/bus.c
> @@ -0,0 +1,708 @@

> +static int alloc_devs(struct devsec *devsec)
> +{
> +	struct device *dev = devsec->dev;

Similar to below.  Maybe use it inline.

> +
> +	for (int i = 0; i < ARRAY_SIZE(devsec->devsec_devs); i++) {
> +		struct devsec_dev *devsec_dev = devsec_dev_alloc(devsec);
> +		int rc;
> +
> +		if (IS_ERR(devsec_dev))
> +			return PTR_ERR(devsec_dev);
> +		rc = devm_add_action_or_reset(dev, destroy_devsec_dev,
> +					      devsec_dev);
> +		if (rc)
> +			return rc;
> +		devsec->devsec_devs[i] = devsec_dev;
> +	}
> +
> +	return 0;
> +}


> +static int init_port(struct devsec_port *devsec_port)
> +{
> +	struct pci_bridge_emul *bridge = &devsec_port->bridge;
> +
> +	*bridge = (struct pci_bridge_emul) {
> +		.conf = {
> +			.vendor = cpu_to_le16(0x8086),
> +			.device = cpu_to_le16(0x7075),

Emulating something real?  If not maybe we should get an ID from another space
(or reserve this one ;)

> +			.class_revision = cpu_to_le32(0x1),
> +			.pref_mem_base = cpu_to_le16(PCI_PREF_RANGE_TYPE_64),
> +			.pref_mem_limit = cpu_to_le16(PCI_PREF_RANGE_TYPE_64),
> +		},


> +{
> +	struct device *dev = devsec->dev;

Only used once. I'd move it down there.

> +
> +	for (int i = 0; i < ARRAY_SIZE(devsec->devsec_ports); i++) {
> +		struct devsec_port *devsec_port = devsec_port_alloc();
> +		int rc;
> +
> +		if (IS_ERR(devsec_port))
> +			return PTR_ERR(devsec_port);
> +		rc = devm_add_action_or_reset(dev, destroy_port, devsec_port);
> +		if (rc)
> +			return rc;
> +		devsec->devsec_ports[i] = devsec_port;
> +	}
> +
> +	return 0;
> +}
> +
> +static int __init devsec_bus_probe(struct platform_device *pdev)
> +{
> +	int rc;
> +	struct devsec *devsec;
> +	u64 mmio_size = SZ_64G;
> +	struct devsec_sysdata *sd;
> +	struct pci_host_bridge *hb;
> +	struct device *dev = &pdev->dev;
> +	u64 mmio_start = iomem_resource.end + 1 - SZ_64G;
> +
> +	hb = devm_pci_alloc_host_bridge(
> +		dev, sizeof(*devsec) - sizeof(struct pci_host_bridge));

I'd move dev up a line.

> +	if (!hb)
> +		return -ENOMEM;



> diff --git a/samples/devsec/tsm.c b/samples/devsec/tsm.c
> new file mode 100644
> index 000000000000..a4705212a7e4
> --- /dev/null
> +++ b/samples/devsec/tsm.c
> @@ -0,0 +1,173 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* Copyright(c) 2024 - 2025 Intel Corporation. All rights reserved. */

> +
> +static const struct pci_tsm_ops *__devsec_pci_ops;
> +
> +static struct pci_tsm *devsec_tsm_pf0_probe(struct pci_dev *pdev)
> +{
> +	int rc;
> +
> +	struct devsec_tsm_pf0 *devsec_tsm __free(kfree) =
> +		kzalloc(sizeof(*devsec_tsm), GFP_KERNEL);
> +	if (!devsec_tsm)
> +		return NULL;
> +
> +	rc = pci_tsm_pf0_constructor(pdev, &devsec_tsm->pci, __devsec_pci_ops);

As below. I'm not seeing why we can't use &devsec_pci_ops directly here.

> +	if (rc)
> +		return NULL;
> +
> +	pci_dbg(pdev, "tsm enabled\n");
> +	return &no_free_ptr(devsec_tsm)->pci.tsm;
> +}
> +
> +static struct pci_tsm *devsec_tsm_fn_probe(struct pci_dev *pdev)
> +{
> +	int rc;
> +
> +	struct devsec_tsm_fn *devsec_tsm __free(kfree) =
> +		kzalloc(sizeof(*devsec_tsm), GFP_KERNEL);
> +	if (!devsec_tsm)
> +		return NULL;
> +
> +	rc = pci_tsm_constructor(pdev, &devsec_tsm->pci, __devsec_pci_ops);

here as well.

> +	if (rc)
> +		return NULL;
> +
> +	pci_dbg(pdev, "tsm (sub-function) enabled\n");
> +	return &no_free_ptr(devsec_tsm)->pci;
> +}

> +static struct pci_tsm_ops devsec_pci_ops = {
> +	.probe = devsec_tsm_pci_probe,
> +	.remove = devsec_tsm_pci_remove,
> +	.connect = devsec_tsm_connect,
> +	.disconnect = devsec_tsm_disconnect,
> +};
> +
> +static void devsec_tsm_remove(void *tsm_dev)
> +{
> +	tsm_unregister(tsm_dev);
> +}
> +
> +static int devsec_tsm_probe(struct faux_device *fdev)
> +{
> +	struct tsm_dev *tsm_dev;
> +
> +	tsm_dev = tsm_register(&fdev->dev, NULL, &devsec_pci_ops);
> +	if (IS_ERR(tsm_dev))
> +		return PTR_ERR(tsm_dev);
> +
> +	return devm_add_action_or_reset(&fdev->dev, devsec_tsm_remove,
> +					tsm_dev);
> +}
> +
> +static struct faux_device *devsec_tsm;
> +
> +static const struct faux_device_ops devsec_device_ops = {
> +	.probe = devsec_tsm_probe,
> +};
> +
> +static int __init devsec_tsm_init(void)
> +{
> +	__devsec_pci_ops = &devsec_pci_ops;

I'm not immediately grasping why this global is needed.
You never check if it's set, so why not just move definition of devsec_pci_ops
early enough that can be directly used everywhere.


> +	devsec_tsm = faux_device_create("devsec_tsm", NULL, &devsec_device_ops);
> +	if (!devsec_tsm)
> +		return -ENOMEM;
> +	return 0;
> +}
> +module_init(devsec_tsm_init);
> +
> +static void __exit devsec_tsm_exit(void)
> +{
> +	faux_device_destroy(devsec_tsm);
> +}
> +module_exit(devsec_tsm_exit);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Device Security Sample Infrastructure: Platform TSM Driver");