Re: [RFC PATCH v2 08/22] pci/tsm: Add PCI driver for TSM

[Bjorn Helgaas <helgaas@kernel.org>]

Match subject capitalization style of history.

Drop second "PCI", mostly redundant.

On Tue, Feb 18, 2025 at 10:09:55PM +1100, Alexey Kardashevskiy wrote:
> The PCI TSM module scans the PCI bus to initialize a TSM context for
> physical ("TDEV") and virtual ("TDI") functions. It also implements
> bus operations which at the moment is just an SPDM bouncer which talks
> to the PF's DOE mailboxes.

Expand "TSM" once here and maybe in the subject.

> + * Copyright(c) 2024 Intel Corporation. All rights reserved.

2025 now.

> +static int tsm_pci_dev_init(struct tsm_bus_subsys *tsm_bus,
> +			    struct pci_dev *pdev,
> +			    struct tsm_dev **ptdev)
> +{
> +	struct tsm_pci_dev_data *tdata;
> +	int ret = tsm_dev_init(tsm_bus, &pdev->dev, sizeof(*tdata), ptdev);

Move the tsm_dev_init() out of the automatic variable list.  Doing it
in the list is OK for trivial things, but this is kind of the meat of
the function.

> +	if (ret)
> +		return ret;
> +
> +	tdata = tsm_dev_to_bdata(*ptdev);
> +
> +	tdata->doe_mb = pci_find_doe_mailbox(pdev,
> +					     PCI_VENDOR_ID_PCI_SIG,
> +					     PCI_DOE_PROTOCOL_CMA_SPDM);
> +	tdata->doe_mb_sec = pci_find_doe_mailbox(pdev,
> +						 PCI_VENDOR_ID_PCI_SIG,
> +						 PCI_DOE_PROTOCOL_SECURED_CMA_SPDM);
> +
> +	if (tdata->doe_mb || tdata->doe_mb_sec)
> +		pci_notice(pdev, "DOE SPDM=%s SecuredSPDM=%s\n",
> +			   tdata->doe_mb ? "yes":"no", tdata->doe_mb_sec ? "yes":"no");
> +
> +	return ret;
> +}

> +
> +static int tsm_pci_alloc_device(struct tsm_bus_subsys *tsm_bus,
> +				struct pci_dev *pdev)
> +{
> +	int ret = 0;

Unnecessary initialization.

> +	/* Set up TDIs for HV (physical functions) and VM (all functions) */
> +	if ((pdev->devcap & PCI_EXP_DEVCAP_TEE) &&
> +	    (((pdev->is_physfn && (PCI_FUNC(pdev->devfn) == 0)) ||
> +	      (!pdev->is_physfn && !pdev->is_virtfn)))) {
> +
> +		struct tsm_dev *tdev = NULL;
> +
> +		if (!is_physical_endpoint(pdev))
> +			return 0;
> +
> +		ret = tsm_pci_dev_init(tsm_bus, pdev, &tdev);
> +		if (ret)
> +			return ret;
> +
> +		ret = tsm_tdi_init(tdev, &pdev->dev);
> +		tsm_dev_put(tdev);
> +		return ret;
> +	}
> +
> +	/* Set up TDIs for HV (virtual functions), should do nothing in VMs */
> +	if (pdev->is_virtfn) {
> +		struct pci_dev *pf0 = pci_get_slot(pdev->physfn->bus,
> +						   pdev->physfn->devfn & ~7);
> +
> +		if (pf0 && (pf0->devcap & PCI_EXP_DEVCAP_TEE)) {
> +			struct tsm_dev *tdev = tsm_dev_get(&pf0->dev);
> +
> +			if (!is_endpoint(pdev))
> +				return 0;
> +
> +			ret = tsm_tdi_init(tdev, &pdev->dev);
> +			tsm_dev_put(tdev);
> +			return ret;
> +		}
> +	}
> +
> +	return 0;
> +}

> +
> +static void tsm_pci_dev_free(struct pci_dev *pdev)
> +{
> +	struct tsm_tdi *tdi = tsm_tdi_get(&pdev->dev);
> +
> +	if (tdi) {
> +		tsm_tdi_put(tdi);
> +		tsm_tdi_free(tdi);
> +	}
> +
> +	struct tsm_dev *tdev = tsm_dev_get(&pdev->dev);

Move at least the declaration to automatic list at entry.

> +	if (tdev) {
> +		tsm_dev_put(tdev);
> +		tsm_dev_free(tdev);
> +	}
> +
> +	WARN_ON(!tdi && tdev);
> +}
> +
> +static int tsm_pci_bus_notifier(struct notifier_block *nb, unsigned long action, void *data)

Wrap to fit in 80 columns like the rest of drivers/pci/

> +{
> +	struct tsm_bus_subsys *tsm_bus = container_of(nb, struct tsm_bus_subsys, notifier);
> +
> +	switch (action) {
> +	case BUS_NOTIFY_ADD_DEVICE:
> +		tsm_pci_alloc_device(tsm_bus, to_pci_dev(data));
> +		break;
> +	case BUS_NOTIFY_DEL_DEVICE:
> +		tsm_pci_dev_free(to_pci_dev(data));
> +		break;
> +	}
> +
> +	return NOTIFY_OK;
> +}
> +
> +struct tsm_bus_subsys *pci_tsm_register(struct tsm_subsys *tsm)
> +{
> +	struct tsm_bus_subsys *tsm_bus = kzalloc(sizeof(*tsm_bus), GFP_KERNEL);
> +	struct pci_dev *pdev = NULL;
> +
> +	pr_info("Scan TSM PCI\n");
> +	tsm_bus->ops = &tsm_pci_ops;
> +	tsm_bus->tsm = tsm;
> +	tsm_bus->notifier.notifier_call = tsm_pci_bus_notifier;
> +	for_each_pci_dev(pdev)
> +		tsm_pci_alloc_device(tsm_bus, pdev);
> +	bus_register_notifier(&pci_bus_type, &tsm_bus->notifier);

Looks racy that we iterate through PCI devs before registering the
notifier.

> +	return tsm_bus;
> +}

> +static int __init tsm_pci_init(void)
> +{
> +	pr_info(DRIVER_DESC " version: " DRIVER_VERSION "\n");
> +	return 0;
> +}
> +
> +static void __exit tsm_pci_cleanup(void)
> +{
> +	pr_info(DRIVER_DESC " version: " DRIVER_VERSION " unload\n");

Both init and cleanup messages are OK for debug, but probably not for
upstream.

> +config PCI_TSM
> +	tristate "TEE Security Manager for PCI Device Security"
> +	select PCI_IDE
> +	depends on TSM
> +	default m
> +	help
> +	  The TEE (Trusted Execution Environment) Device Interface
> +	  Security Protocol (TDISP) defines a "TSM" as a platform agent

Expand "TSM" here.  From menu line above, I guess it's "TEE Security
Manager"?

> +	  that manages device authentication, link encryption, link
> +	  integrity protection, and assignment of PCI device functions
> +	  (virtual or physical) to confidential computing VMs that can
> +	  access (DMA) guest private memory.
> +
> +	  Enable a platform TSM driver to use this capability.
> +
>  config PCI_DOE
>  	bool
>  
> -- 
> 2.47.1
>