Re: [PATCH v5 04/10] PCI/TSM: Authenticate devices via platform TSM

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

Alexey Kardashevskiy wrote:
> 
> On 27/8/25 13:51, Dan Williams wrote:
> 
> [...]
> 
> > +static int pci_tsm_connect(struct pci_dev *pdev, struct tsm_dev *tsm_dev)
> > +{
> > +	int rc;
> > +	struct pci_tsm_pf0 *tsm_pf0;
> > +	const struct pci_tsm_ops *ops = tsm_pci_ops(tsm_dev);
> > +	struct pci_tsm *pci_tsm __free(tsm_remove) = ops->probe(pdev);
> > +
> > +	/* connect()  mutually exclusive with subfunction pci_tsm_init() */
> > +	lockdep_assert_held_write(&pci_tsm_rwsem);
> > +
> > +	if (!pci_tsm)
> > +		return -ENXIO;
> > +
> > +	pdev->tsm = pci_tsm;
> > +	tsm_pf0 = to_pci_tsm_pf0(pdev->tsm);
> > +
> > +	/* mutex_intr assumes connect() is always sysfs/user driven */
> > +	ACQUIRE(mutex_intr, lock)(&tsm_pf0->lock);
> > +	if ((rc = ACQUIRE_ERR(mutex_intr, &lock)))
> > +		return rc;
> > +
> > +	rc = ops->connect(pdev);
> > +	if (rc)
> > +		return rc;
> > +
> > +	pdev->tsm = no_free_ptr(pci_tsm);
> > +
> > +	/*
> > +	 * Now that the DSM is established, probe() all the potential
> > +	 * dependent functions. Failure to probe a function is not fatal
> > +	 * to connect(), it just disables subsequent security operations
> > +	 * for that function.
> > +	 */
> > +	pci_tsm_walk_fns(pdev, probe_fn, pdev);
> > +	return 0;
> > +}
> > +
> > +static ssize_t connect_show(struct device *dev, struct device_attribute *attr,
> > +			    char *buf)
> > +{
> > +	struct pci_dev *pdev = to_pci_dev(dev);
> > +	int rc;
> > +
> > +	ACQUIRE(rwsem_read_intr, lock)(&pci_tsm_rwsem);
> > +	if ((rc = ACQUIRE_ERR(rwsem_read_intr, &lock)))
> > +		return rc;
> > +
> > +	if (!pdev->tsm)
> > +		return sysfs_emit(buf, "\n");
> > +
> > +	return sysfs_emit(buf, "%s\n", tsm_name(pdev->tsm->ops->owner));
> > +}
> > +
> > +/* Is @tsm_dev managing physical link / session properties... */
> > +static bool is_link_tsm(struct tsm_dev *tsm_dev)
> > +{
> > +	const struct pci_tsm_ops *ops = tsm_pci_ops(tsm_dev);
> > +
> > +	return ops && ops->link_ops.probe;
> > +}
> > +
> > +/* ...or is @tsm_dev managing device security state ? */
> > +static bool is_devsec_tsm(struct tsm_dev *tsm_dev)
> > +{
> > +	const struct pci_tsm_ops *ops = tsm_pci_ops(tsm_dev);
> > +
> > +	return ops && ops->devsec_ops.lock;
> > +}
> > +
> > +static ssize_t connect_store(struct device *dev, struct device_attribute *attr,
> > +			     const char *buf, size_t len)
> > +{
> > +	struct pci_dev *pdev = to_pci_dev(dev);
> > +	struct tsm_dev *tsm_dev;
> > +	int rc, id;
> > +
> > +	rc = sscanf(buf, "tsm%d\n", &id);
> > +	if (rc != 1)
> > +		return -EINVAL;
> > +
> > +	ACQUIRE(rwsem_write_kill, lock)(&pci_tsm_rwsem);
> > +	if ((rc = ACQUIRE_ERR(rwsem_write_kill, &lock)))
> > +		return rc;
> > +
> > +	if (pdev->tsm)
> > +		return -EBUSY;
> 
> 
> In one of my previous RFC, I had an IDE key refresh call and it's been
> suggested [1] to ditch that and use connect() instead and the clause
> above prevents it. I am hacking something around this anyway (need to
> validate the PSP support for it) and may be this may be generalized
> now rather than later. Thanks,

When I recommended reuse "connect" I was thinking about kernel internal
helper calls ->connect() again and have the low-level TSM driver be
responsible for determining the difference. IDE Key Refresh deserves its
own follow-on patch set to layout assumptions and tradeoffs between:

* core helper that calls ->connect() again
* core helper that calls a new ->refresh()
* no core helper, TSM drivers handle locally. Local because the refresh
  policy might by dynamically negotiated per TSM-arch/DSM-device pairing
  and the core is not in a good position to drive that policy.