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

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

Jonathan Cameron wrote:
> 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.

ok.

> > +	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 ;)

I am happy to switch to something else, but no, I do not have time to
chase this through PCI SIG. I do not expect this id to cause conflicts,
but no guarantees.

> > +			.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.

ok.

> 
> > +
> > +	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.

clang-format disagrees and I prefer just letting a tool do my formatting.

[..]
> > +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.

Because devsec_pci_ops is used inside the ops it declares. So either
forward declare all those ops, or do this pointer assigment dance. I
opted for the latter as it is smaller.