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

[Aneesh Kumar K.V <aneesh.kumar@kernel.org>]

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?

-aneesh