Re: [PATCH 06/11] samples/devsec: PCI device-security bus / endpoint sample

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

On Thu, 05 Dec 2024 14:23:51 -0800
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>
Hi Dan,

A few minor comments as I was reading this. Mostly just trying
to get my head around it hence they are all fairly superficial things.

Jonathan

> diff --git a/samples/devsec/bus.c b/samples/devsec/bus.c
> new file mode 100644
> index 000000000000..47dbe4e1b648
> --- /dev/null
> +++ b/samples/devsec/bus.c


> +static void destroy_iomem_pool(void *data)

There is a devm_gen_pool_create you can probably use.

> +{
> +	struct devsec *devsec = data;
> +
> +	gen_pool_destroy(devsec->iomem_pool);
> +}
> +
> +static void destroy_bus(void *data)
> +{
> +	struct devsec *devsec = data;
> +
> +	pci_stop_root_bus(devsec->bus);
> +	pci_remove_root_bus(devsec->bus);
> +}
> +
> +static void destroy_devs(void *data)
> +{
> +	struct devsec *devsec = data;
> +	int i;
> +
> +	for (i = ARRAY_SIZE(devsec->devsec_devs) - 1; i >= 0; i--) {
> +		struct devsec_dev *devsec_dev = devsec->devsec_devs[i];
> +
> +		if (!devsec_dev)
> +			continue;
> +		gen_pool_free(devsec->iomem_pool, devsec_dev->mmio_range.start,
> +			      range_len(&devsec_dev->mmio_range));
> +		kfree(devsec_dev);
> +		devsec->devsec_devs[i] = NULL;
> +	}
> +}
> +

> +#define MMIO_SIZE SZ_2M
> +
> +static int alloc_devs(struct devsec *devsec)
> +{
> +	struct device *dev = devsec->dev;
> +	int i, rc;
> +
> +	rc = devm_add_action_or_reset(dev, destroy_devs, devsec);
> +	if (rc)
> +		return rc;
> +
> +	for (i = 0; i < ARRAY_SIZE(devsec->devsec_devs); i++) {
> +		struct devsec_dev *devsec_dev __free(kfree) =
> +			kzalloc(sizeof(*devsec_dev), GFP_KERNEL);
> +		struct genpool_data_align data = {
> +			.align = MMIO_SIZE,
> +		};
> +		u64 phys;
> +
> +		if (!devsec_dev)
> +			return -ENOMEM;
> +
> +		phys = gen_pool_alloc_algo(devsec->iomem_pool, MMIO_SIZE,
> +					   gen_pool_first_fit_align, &data);
> +		if (!phys)
> +			return -ENOMEM;
> +
> +		devsec_dev->mmio_range = (struct range) {
> +			.start = phys,
> +			.end = phys + MMIO_SIZE - 1,
> +		};
> +		init_dev_cfg(devsec_dev);
> +		devsec->devsec_devs[i] = no_free_ptr(devsec_dev);

Similar to the case below. I'd rather see a per dev devm_ cleanup
than relying on unified cleanup and that array having null entrees.
Should end up easier to follow.  Might require devsec dev to have
a reference back to the pool though.


> +	}
> +
> +	return 0;
> +}
> +
> +static void destroy_ports(void *data)
> +{
> +	struct devsec *devsec = data;
> +	int i;
> +
> +	for (i = ARRAY_SIZE(devsec->devsec_ports) - 1; i >= 0; i--) {
> +		struct devsec_port *devsec_port = devsec->devsec_ports[i];
> +
> +		if (!devsec_port)
> +			continue;
> +		pci_bridge_emul_cleanup(&devsec_port->bridge);
> +		kfree(devsec_port);
> +		devsec->devsec_ports[i] = NULL;

Is this necessary? If so it wrecks suggestion to do per port devres cleanup.
I don't think it is necessary though as we really should be touching that
array after this function is done.


> +	}
> +}


> +static int init_port(struct devsec_port *devsec_port)
> +{
> +	struct pci_bridge_emul *bridge = &devsec_port->bridge;
> +	int rc;
> +
> +	bridge->conf.vendor = cpu_to_le16(0x8086);
> +	bridge->conf.device = cpu_to_le16(0x7075);
> +	bridge->subsystem_vendor_id = cpu_to_le16(0x8086);
> +	bridge->conf.class_revision = cpu_to_le32(0x1);
> +
> +	bridge->conf.pref_mem_base = cpu_to_le16(PCI_PREF_RANGE_TYPE_64);
> +	bridge->conf.pref_mem_limit = cpu_to_le16(PCI_PREF_RANGE_TYPE_64);
> +
> +	bridge->has_pcie = true;
> +	bridge->pcie_conf.devcap = cpu_to_le16(PCI_EXP_DEVCAP_FLR);
> +	bridge->pcie_conf.lnksta = cpu_to_le16(PCI_EXP_LNKSTA_CLS_2_5GB);
> +
> +	bridge->data = devsec_port;
> +	bridge->ops = &devsec_bridge_ops;
Maybe 
	*bridge = (struct pci_bridge_emul) {
	};
appropriate here. 	
> +
> +	init_ide(&devsec_port->ide);
> +
> +	rc = pci_bridge_emul_init(bridge, 0);

return pci_bridge_emul_init() unless a later patch is going to add more here.

> +	if (rc)
> +		return rc;
> +
> +	return 0;
> +}
> +
> +static int alloc_ports(struct devsec *devsec)
> +{
> +	struct device *dev = devsec->dev;
> +	int i, rc;
> +
> +	rc = devm_add_action_or_reset(dev, destroy_ports, devsec);
> +	if (rc)
> +		return rc;
> +
> +	for (i = 0; i < ARRAY_SIZE(devsec->devsec_ports); i++) {
> +		struct devsec_port *devsec_port __free(kfree) =
> +			kzalloc(sizeof(*devsec_port), GFP_KERNEL);
> +
> +		if (!devsec_port)
> +			return -ENOMEM;
> +
> +		rc = init_port(devsec_port);
> +		if (rc)
> +			return rc;

I'd prefer to see a per port devm cleanup registered so that you don't
have to register that before anything has happened leaving it only
loosely associated with what it is doing.


> +		devsec->devsec_ports[i] = no_free_ptr(devsec_port);
> +	}
> +
> +	return 0;
> +}