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
>
