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

[Alexey Kardashevskiy <aik@amd.com>]

On 14/8/25 11:40, dan.j.williams@intel.com wrote:
> 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

Right but I'd think that devices (or, more precisely, PCIe slots) are statically assigned to TSMs. A bit hard to imagine 2 TSMs in a system and ability to connect some PCI device to either of those. It is not impossible but not exactly "painfully simple".


> [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.

Hm. Those groups are per a TSM so no device's certificates/meas/report blobs there, right?

> 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.

Yeah, I realized that soon after I hit "send".


>>> +	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'?"

Nah, ignore my "imho" then. Thanks,


> ...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.

-- 
Alexey