Re: [PATCH] iommu: Allow ATS to work on VFs when the PF uses IDENTITY

[Yi Liu <yi.l.liu@intel.com>]

On 2024/8/8 02:19, Jason Gunthorpe wrote:
> PCI ATS has a global Smallest Translation Unit field that is located in
> the PF but shared by all of the VFs.
> 
> The expectation is that the STU will be set to the root port's global STU
> capability which is driven by the IO page table configuration of the iommu
> HW. Today it becomes set when the iommu driver first enables ATS.
> 
> Thus, to enable ATS on the VF, the PF must have already had the correct
> STU programmed, even if ATS is off on the PF.
> 
> Unfortunately the PF only programs the STU when the PF enables ATS. The
> iommu drivers tend to leave ATS disabled when IDENTITY translation is
> being used.
> 
> Thus we can get into a state where the PF is setup to use IDENTITY with
> the DMA API while the VF would like to use VFIO with a PAGING domain and
> have ATS turned on. This fails because the PF never loaded a PAGING domain
> and so it never setup the STU, and the VF can't do it.
> 
> The simplest solution is to have the iommu driver set the ATS STU when it
> probes the device. This way the ATS STU is loaded immediately at boot time
> to all PFs and there is no issue when a VF comes to use it.

This only sets STU without setting the ATS_CTRL.E bit. Is it possible that
VF considers the PF's STU field as valid only if PF's ATS_CTRL.E bit is
set?

> 
> Add a new call pci_prepare_ats() which should be called by iommu drivers
> in their probe_device() op for every PCI device if the iommu driver
> supports ATS. This will setup the STU based on whatever page size
> capability the iommu HW has.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>   drivers/iommu/amd/iommu.c                   |  3 ++
>   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c |  6 ++++
>   drivers/iommu/intel/iommu.c                 |  1 +
>   drivers/pci/ats.c                           | 33 +++++++++++++++++++++
>   include/linux/pci-ats.h                     |  1 +
>   5 files changed, 44 insertions(+)
> 
> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> index b19e8c0f48fa25..98054497d343bc 100644
> --- a/drivers/iommu/amd/iommu.c
> +++ b/drivers/iommu/amd/iommu.c
> @@ -2203,6 +2203,9 @@ static struct iommu_device *amd_iommu_probe_device(struct device *dev)
>   
>   	iommu_completion_wait(iommu);
>   
> +	if (dev_is_pci(dev))
> +		pci_prepare_ats(to_pci_dev(dev), PAGE_SHIFT);
> +
>   	return iommu_dev;
>   }
>   
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> index a31460f9f3d421..9bc50bded5af72 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -3295,6 +3295,12 @@ static struct iommu_device *arm_smmu_probe_device(struct device *dev)
>   	    smmu->features & ARM_SMMU_FEAT_STALL_FORCE)
>   		master->stall_enabled = true;
>   
> +	if (dev_is_pci(dev)) {
> +		unsigned int stu = __ffs(smmu->pgsize_bitmap);
> +
> +		pci_prepare_ats(to_pci_dev(dev), stu);
> +	}
> +
>   	return &smmu->iommu;
>   
>   err_free_master:
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index 9ff8b83c19a3e2..ad81db026ab236 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -4091,6 +4091,7 @@ static struct iommu_device *intel_iommu_probe_device(struct device *dev)
>   
>   	dev_iommu_priv_set(dev, info);
>   	if (pdev && pci_ats_supported(pdev)) {
> +		pci_prepare_ats(pdev, VTD_PAGE_SHIFT);

perhaps just do it for PFs? :)

>   		ret = device_rbtree_insert(iommu, info);
>   		if (ret)
>   			goto free;
> diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
> index c570892b209095..87fa03540b8a21 100644
> --- a/drivers/pci/ats.c
> +++ b/drivers/pci/ats.c
> @@ -47,6 +47,39 @@ bool pci_ats_supported(struct pci_dev *dev)
>   }
>   EXPORT_SYMBOL_GPL(pci_ats_supported);
>   
> +/**
> + * pci_prepare_ats - Setup the PS for ATS
> + * @dev: the PCI device
> + * @ps: the IOMMU page shift
> + *
> + * This must be done by the IOMMU driver on the PF before any VFs are created to
> + * ensure that the VF can have ATS enabled.
> + *
> + * Returns 0 on success, or negative on failure.
> + */
> +int pci_prepare_ats(struct pci_dev *dev, int ps)
> +{
> +	u16 ctrl;
> +
> +	if (!pci_ats_supported(dev))
> +		return -EINVAL;
> +
> +	if (WARN_ON(dev->ats_enabled))
> +		return -EBUSY;
> +
> +	if (ps < PCI_ATS_MIN_STU)
> +		return -EINVAL;
> +
> +	if (dev->is_virtfn)
> +		return 0;
> +
> +	dev->ats_stu = ps;
> +	ctrl = PCI_ATS_CTRL_STU(dev->ats_stu - PCI_ATS_MIN_STU);
> +	pci_write_config_word(dev, dev->ats_cap + PCI_ATS_CTRL, ctrl);

Is it valuable to have a flag to mark if STU is set or not? Such way can
avoid setting STU multiple times.

> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(pci_prepare_ats);
> +
>   /**
>    * pci_enable_ats - enable the ATS capability
>    * @dev: the PCI device
> diff --git a/include/linux/pci-ats.h b/include/linux/pci-ats.h
> index df54cd5b15db09..d98929c86991be 100644
> --- a/include/linux/pci-ats.h
> +++ b/include/linux/pci-ats.h
> @@ -8,6 +8,7 @@
>   /* Address Translation Service */
>   bool pci_ats_supported(struct pci_dev *dev);
>   int pci_enable_ats(struct pci_dev *dev, int ps);
> +int pci_prepare_ats(struct pci_dev *dev, int ps);
>   void pci_disable_ats(struct pci_dev *dev);
>   int pci_ats_queue_depth(struct pci_dev *dev);
>   int pci_ats_page_aligned(struct pci_dev *dev);
> 
> base-commit: e7153d9c8cee2f17fdcd011509860717bfa91423

-- 
Regards,
Yi Liu