Re: [PATCH 05/11] PCI/TSM: Authenticate devices via platform TSM

[Xu Yilun <yilun.xu@linux.intel.com>]

On Thu, Feb 27, 2025 at 04:15:26PM -0800, Dan Williams wrote:
> Xu Yilun wrote:
> > On Fri, Feb 21, 2025 at 05:15:24PM -0800, Dan Williams wrote:
> > > Xu Yilun wrote:
> > > > > +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.
> > > 
> > > [..]
> > > > > +
> > > > > +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.
> 
> I do think it makes sense to have one ->tsm pointer from a PCI device to
> represent any possible TSM context, but I do not think it makes sense
> for that context to always contain members that are only relevant to PF
> Function 0.
> So, here is an updated proposal:
> 
> /**
>  * struct pci_tsm - Core TSM context for a given PCIe endpoint
>  * @pdev: indicates the type of pci_tsm object
>  *
>  * This structure is wrapped by a low level TSM driver and returned by
>  * tsm_ops.probe(), it is freed by tsm_ops.remove(). Depending on
>  * whether @pdev is physical function 0, another physical function, or a
>  * virtual function determines the pci_tsm object type. E.g. see 'struct
>  * pci_tsm_pf0'.
>  */
> struct pci_tsm {
>         struct pci_dev *pdev;
> };
> 
> /**
>  * struct pci_tsm_pf0 - Physical Function 0 TDISP context
>  * @state: reflect device initialized, connected, or bound
>  * @lock: protect @state vs pci_tsm_ops invocation
>  * @doe_mb: PCIe Data Object Exchange mailbox
>  */
> struct pci_tsm_pf0 {
>         enum pci_tsm_state state;
>         struct mutex lock;

I think the scope of the lock should expand to pci_tsm_ops::bind(), we
need to ensure the TDI bind won't race with its PF0's (dis)connect.

  struct pci_tsm {
	struct pci_dev *pdev;
	struct pci_tdi *tdi;
  };

  struct pci_tdi {
	struct pci_tsm_pf0 *tsm_pf0;
	...
  };

  int pci_tdi_lock(struct pci_tdi *tdi)
  {
	mutex_lock(&tdi->tsm_pf0->lock);
  }

Thanks,
Yilun

>         struct pci_doe_mb *doe_mb;
>         struct pci_tsm tsm;
> };
> 
> This arrangement lets the core 'struct pci_tsm' object hold
> common-to-all device-type details like a 'struct pci_tdi' pointer. For
> physical function0 devices the core does:
> 
>    container_of(pdev->tsm, struct pci_tsm_pf0, tsm)
> 
> ...to get to those exclusive details.
> 
> > > > > +
> > > > > +	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.
> 
> Yes, makes sense. I will work on moving the physical function0 data
> out-of-line from the core 'struct pci_tsm' definition.
> 
> So, 'struct pci_tdi' is common to 'struct pci_tsm' since any PCI
> function can become a TDI. If VFs or non-function0 functions need
> addtional context we could create a 'struct pci_tsm_vf' or similar for
> that data.