On Fri, Feb 21, 2025 at 05:15:24PM -0800, Dan Williams wrote:

Xu Yilun wrote:

quoted

quoted

+static int pci_tsm_disconnect(struct pci_dev *pdev)
+{
+	struct pci_tsm *pci_tsm = pdev->tsm;
+
+	lockdep_assert_held(&pci_tsm_rwsem);
+	if_not_guard(mutex_intr, &pci_tsm->lock)
+		return -EINTR;
+
+	if (pci_tsm->state < PCI_TSM_CONNECT)
+		return 0;
+	if (pci_tsm->state < PCI_TSM_INIT)
+		return -ENXIO;

Check PCI_TSM_INIT first, or this condition will never hit.

  if (pci_tsm->state < PCI_TSM_INIT)
	return -ENXIO;
  if (pci_tsm->state < PCI_TSM_CONNECT)
	return 0;

I suggest the same sequence for pci_tsm_connect().

Good catch, fixed.

[..]

quoted

quoted

+
+static void __pci_tsm_init(struct pci_dev *pdev)
+{
+	bool tee_cap;
+
+	if (!is_physical_endpoint(pdev))
+		return;

This Filters out virtual functions, just because not ready for support,
is it?

Do you see a need for PCI core to notify the TSM driver about the
arrival of VF devices?

I think yes.

My expectation is that a VF TDI communicates with the TSM driver
relative to its PF.

It is possible, but the PF TSM still need to manage the TDI context for
all it's VFs, like:

struct pci_tdi;

struct pci_tsm {
	...
	struct pci_dsm *dsm;
	struct xarray tdi_xa; // struct pci_tdi array
};


An alternative is we allow VFs has their own pci_tsm, and store their
own tdi contexts in it.

struct pci_tsm {
	...
	struct pci_dsm *dsm; // point to PF's dsm.
	struct pci_tdi *tdi;
};

I perfer the later cause we don't have to seach for TDI context
everytime we have a pdev for VF and do tsm operations on it.

quoted

quoted

+
+	tee_cap = pdev->devcap & PCI_EXP_DEVCAP_TEE;
+
+	if (!(pdev->ide_cap || tee_cap))
+		return;
+
+	lockdep_assert_held_write(&pci_tsm_rwsem);
+	if (!tsm_ops)
+		return;
+
+	struct pci_tsm *pci_tsm __free(kfree) = kzalloc(sizeof(*pci_tsm), GFP_KERNEL);
+	if (!pci_tsm)
+		return;
+
+	/*
+	 * If a physical device has any security capabilities it may be
+	 * a candidate to connect with the platform TSM
+	 */
+	struct pci_dsm *dsm __free(dsm_remove) = tsm_ops->probe(pdev);

IIUC, pdev->tsm should be for every pci function (physical or virtual),
pdev->tsm->dsm should be only for physical functions, is it?

Per above I was only expecting physical function, but the bind flow
might introduce the need for per function (phyiscal or virtual) TDI
context. I expect that is separate from the PF pdev->tsm context.

Could we embed TDI context in PF's pdev->tsm AND VF's pdev->tsm? From
TDISP spec, TSM covers TDI management so I think it is proper
struct pci_tsm contains TDI context.

Thanks,
Yilun