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

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

Alexey Kardashevskiy wrote:
> On 18/7/25 04:33, Dan Williams wrote:
[..]
> > diff --git a/drivers/pci/tsm.c b/drivers/pci/tsm.c
> > new file mode 100644
> > index 000000000000..0784cc436dd3
> > --- /dev/null
> > +++ b/drivers/pci/tsm.c
[..]
> > +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);
> > +	const struct pci_tsm_ops *ops;
> > +	struct tsm_dev *tsm_dev;
> > +	int rc, id;
> > +
> > +	rc = sscanf(buf, "tsm%d\n", &id);
> 
> Why is id needed here? Are there going to be multiple DSMs per a PCI
> device?

The implementation allows for multiple TSMs per platform [1], and you
acknowledged this earlier [2] (at least the "no globals" bit).

[1]: http://lore.kernel.org/683f9e141f1b1_1626e1009@dwillia2-xfh.jf.intel.com.notmuch

[2]: http://lore.kernel.org/b281b714-5097-4b3a-9809-7bdcb9e004dc@amd.com

One of the nice properties of multiple tsm_devs is the ability to unit test
host and guest side TSM flows in the same kernel image.

> I am missing the point of tsm_dev. It does not have sysfs nodes (the
> pci_dev parent does)

The resource accounting symlinks for each each IDE stream point to the
tsm_dev, see tsm_ide_stream_register().

> tsm_register() takes attribute_group but what would posibbly go there?

Any vendor specific implementation of commonly named attributes.
Contrast that with vendor specific attributes with vendor specific names
that the vendor specific device publishes.

> certificates/meas/report blobs?

Perhaps. For now, I want to just focus on the mechanics of the getting a
TDI into the run state. The attestation flow is a separate design debate
one there is consensus on getting the TDI up and running.

> The pci_dev struct itself has *tsm now so this child device is not
> that. Hm.

This tsm_dev is not a child device it is the common class representation
of a platform capability that can establish SPDM and optionally IDE.

> > +	if (rc != 1)
> > +		return -EINVAL;
> > +
> > +	ACQUIRE(rwsem_read_intr, lock)(&pci_tsm_rwsem);
> > +	if ((rc = ACQUIRE_ERR(rwsem_read_intr, &lock)))
> > +		return rc;
> > +
> > +	if (pdev->tsm)
> > +		return -EBUSY;
> > +
> > +	tsm_dev = find_tsm_dev(id);
> 
> When PCI TSM loads, all it does is add "connect" nodes. And when write
> to "connect" happens, this find_tsm_dev() is expected to find a
> tsm_dev but what is going to add those in the real PCI?

sev_tsm_init() calls tsm_register(). Userspace catches the tsm_dev
KOBJECT_ADD event to run:

echo $TSM_DEV > /sys/bus/pci/devices/$PDEV/tsm/connect

[..]
> imho "echo 0 > connect" is more descriptive than "echo 1 > disconnect", and one less sysfs node.

That makes it a bit too ambiguous for my taste as connect is "connect to
a tsm of the following identifier", so, for example, "is '0' a shorthand
for 'tsm0'?"

...and as I say that I realize disconnect as the same problem.  I will
update disconnect to take the tsm device name just like connect for
symmetry, this ambiguity concern, and in case multiple TSM connections
per device might ever happen way down the road.

[..]
> > +/**
> > + * pci_tsm_constructor() - base 'struct pci_tsm' initialization
> > + * @pdev: The PCI device
> > + * @tsm: context to initialize
> > + * @ops: PCI operations provided by the TSM
> > + */
> > +int pci_tsm_constructor(struct pci_dev *pdev, struct pci_tsm *tsm,
> > +			const struct pci_tsm_ops *ops)
> > +{
> > +	tsm->pdev = pdev;
> > +	tsm->ops = ops;
> 
> These should go down, right before "return 0". Thanks,

Sure, makes sense.

In practice @tsm will be unwound, but might as well not make it a
valid object while it is awaiting to be freed.