Re: [RFC PATCH 27/30] PCI/TSM: Add PCI driver callbacks to handle TSM requirements

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

On Mon, Jun 02, 2025 at 06:36:37PM +0530, Aneesh Kumar K.V wrote:
> Xu Yilun <yilun.xu@linux.intel.com> writes:
> 
> > Add optional PCI driver callbacks to notify TSM events. For now, these
> > handlers may be called during pci_tsm_unbind(). By calling these
> > handlers, TSM driver askes for external collaboration to finish entire
> > TSM unbind flow.
> >
> > If platform TSM driver could finish TSM bind/unbind all by itself, don't
> > call these handlers.
> >
> > Host may need to configure various system components according to
> > platform trusted firmware's requirements. E.g. for Intel TDX Connect,
> > host should do private MMIO mapping in S-EPT, trusted DMA setup, device
> > ownership claiming and device TDISP state transition. Some operations are
> > out of control of PCI TSM, so need collaboration by external components
> > like IOMMU driver, KVM.
> >
> > Further more, trusted firmware may enforce executing these operations
> > in a fixed sequence. E.g. Intel TDX Connect enforces the following
> > sequences for TSM unbind:
> >
> >   1. STOP TDI via TDISP message STOP_INTERFACE
> >   2. Private MMIO unmap from Secure EPT
> >   3. Trusted Device Context Table cleanup for the TDI
> >   4. TDI ownership reclaim and metadata free
> >
> > PCI TSM could do Step 1 and 4, but need KVM for Step 2 and IOMMU driver
> > for Step 3. While it is possible TSM provides finer grained APIs like
> > tdi_stop() & tdi_free(), and the caller ensures the sequence, it is
> > better these specific enforcement could be managed in platform TSM
> > driver. By introducing TSM handlers, platform TSM driver controls the
> > operation sequence and notify other components to do the real work.
> >
> > Currently add 3 callbacks for TDX Connect. disable_mmio() is for
> > VFIO to invalidate MMIO so that KVM could unmap them from S-EPT.
> > recover_mmio() is to re-validate MMIO so that KVM could map them
> > again for shared assigned device. disable_trusted_dma() is to cleanup
> > trusted IOMMU setup.
> >
> > Signed-off-by: Xu Yilun <yilun.xu@linux.intel.com>
> > ---
> >  include/linux/pci-tsm.h | 7 +++++++
> >  include/linux/pci.h     | 3 +++
> >  2 files changed, 10 insertions(+)
> >
> > diff --git a/include/linux/pci-tsm.h b/include/linux/pci-tsm.h
> > index 737767f8a9c5..ed549724eb5b 100644
> > --- a/include/linux/pci-tsm.h
> > +++ b/include/linux/pci-tsm.h
> > @@ -157,6 +157,13 @@ struct pci_tsm_ops {
> >  	int (*accept)(struct pci_dev *pdev);
> >  };
> >  
> > +/* pci drivers callbacks for TSM */
> > +struct pci_tsm_handlers {
> > +	void (*disable_mmio)(struct pci_dev *dev);
> > +	void (*recover_mmio)(struct pci_dev *dev);
> > +	void (*disable_trusted_dma)(struct pci_dev *dev);
> > +};
> > +
> >  enum pci_doe_proto {
> >  	PCI_DOE_PROTO_CMA = 1,
> >  	PCI_DOE_PROTO_SSESSION = 2,
> > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > index 5f37957da18f..4f768b4658e8 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -545,6 +545,7 @@ struct pci_dev {
> >  #endif
> >  #ifdef CONFIG_PCI_TSM
> >  	struct pci_tsm *tsm;		/* TSM operation state */
> > +	void *trusted_dma_owner;
> >  #endif
> >  	u16		acs_cap;	/* ACS Capability offset */
> >  	u8		supported_speeds; /* Supported Link Speeds Vector */
> > @@ -957,6 +958,7 @@ struct module;
> >   * @sriov_get_vf_total_msix: PF driver callback to get the total number of
> >   *              MSI-X vectors available for distribution to the VFs.
> >   * @err_handler: See Documentation/PCI/pci-error-recovery.rst
> > + * @tsm_handler: Optional driver callbacks to handle TSM requirements.
> >   * @groups:	Sysfs attribute groups.
> >   * @dev_groups: Attributes attached to the device that will be
> >   *              created once it is bound to the driver.
> > @@ -982,6 +984,7 @@ struct pci_driver {
> >  	int  (*sriov_set_msix_vec_count)(struct pci_dev *vf, int msix_vec_count); /* On PF */
> >  	u32  (*sriov_get_vf_total_msix)(struct pci_dev *pf);
> >  	const struct pci_error_handlers *err_handler;
> > +	struct pci_tsm_handlers *tsm_handler;
> >  	const struct attribute_group **groups;
> >  	const struct attribute_group **dev_groups;
> >  	struct device_driver	driver;
> > -- 
> > 2.25.1
> 
> It looks like the TSM feature is currently interacting with several
> components: struct pci_driver, VFIO, iommufd, and pci_tsm_ops.
> 
> Should we consider limiting this scattering? Would it make sense to
> encapsulate this logic within pci_tsm_ops?

I'm keeping on trying which is a better solution. Encapsulating all in
pci_tsm_ops is the most attactive one from SW POV, but only if the TSM
operations has no impact/dependency to other components. Unfortunately
it is not true, e.g. the private MMIO mapping/unmapping is actually
a writting to leaf S-EPT entry, but it requires non-leaf page-table-page
management in KVM.

Thanks,
Yilun

> 
> -aneesh