Re: [RFC PATCH] iommu: alloc iommu group for pasid supported devices

[Robin Murphy]

On 2021-12-16 12:16, Yao Hongbo wrote:
> Fix a pci hotlug problem for PCI pasid enabled devices.
>
> We can probe PCI pasid enabled devices on boot normally,
> but the following error is seen while we poweroff and poweron
> this pci slot:
> [  312.407485] nvme 0000:9c:00.0: cannot attach to incompatible domain
> (0 SSID bits != 20)
> [  312.415618] nvme 0000:9c:00.0: Failed to add to iommu group 11: -22
>
> Each device would alloc an iommu group when the os starts.
> But when we hot-plug the pcie device, the device may be added to the
> parent iommu group. If the device supports pasid, master->ssid_bits
> would be changed in arm_smmu_enable_pasid(), but smmu->domain is from
> the parent iommu, which will result in the upon error in
> arm_smmu_attach_dev().
>
> Realloc a new iommu group if the device supports to enable pasid.

I'm not sure I fully understand the circumstances of the issue, but the 
code in the patch doesn't look right to me - it seems to be saying that 
if we don't have ACS enabled, then the the mere fact that end-to-end 
PASID support exists somehow guarantees peer-to-peer isolation anyway. 
Surely that's not true?

Is there something in the hotplug path which causes ACS and/or PASID 
enablement in a different order from boot-time probing?

Looking at the symptom, I also wonder whether the SMMU driver really 
needs to be that strict (if the device can support more PASID bits than 
the domain is configured for, couldn't we just... not use all of them?), 
but we should definitely make sense of the fundamental group lookup 
issue here before considering any other changes that might happen to 
mask it.

Robin.

> Signed-off-by: Yao Hongbo <yaohongbo@xxxxxxxxxxxxxxxxx>
> ---
>   drivers/iommu/iommu.c   |  4 ++++
>   drivers/pci/ats.c       | 12 ++++++++++++
>   include/linux/pci-ats.h |  3 +++
>   3 files changed, 19 insertions(+)
>
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index dd7863e..61b5360 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -21,6 +21,7 @@
>   #include <linux/notifier.h>
>   #include <linux/err.h>
>   #include <linux/pci.h>
> +#include <linux/pci-ats.h>
>   #include <linux/bitops.h>
>   #include <linux/property.h>
>   #include <linux/fsl/mc.h>
> @@ -1475,6 +1476,9 @@ struct iommu_group *pci_device_group(struct device *dev)
>   		if (pci_acs_path_enabled(bus->self, NULL, REQ_ACS_FLAGS))
>   			break;
>   
> +		if (pci_pasid_supported(pdev))
> +			break;
> +
>   		pdev = bus->self;
>   
>   		group = iommu_group_get(&pdev->dev);
> diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
> index c967ad6..8fcca4f 100644
> --- a/drivers/pci/ats.c
> +++ b/drivers/pci/ats.c
> @@ -349,6 +349,18 @@ void pci_pasid_init(struct pci_dev *pdev)
>   	pdev->pasid_cap = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_PASID);
>   }
>   
> +bool pci_pasid_supported(struct pci_dev *pdev)
> +{
> +	if (!pdev->pasid_cap)
> +		return false;
> +
> +	if (!pdev->eetlp_prefix_path)
> +		return false;
> +
> +	return true;
> +}
> +EXPORT_SYMBOL_GPL(pci_pasid_supported);
> +
>   /**
>    * pci_enable_pasid - Enable the PASID capability
>    * @pdev: PCI device structure
> diff --git a/include/linux/pci-ats.h b/include/linux/pci-ats.h
> index df54cd5b..623725b 100644
> --- a/include/linux/pci-ats.h
> +++ b/include/linux/pci-ats.h
> @@ -35,11 +35,14 @@ static inline bool pci_pri_supported(struct pci_dev *pdev)
>   #endif /* CONFIG_PCI_PRI */
>   
>   #ifdef CONFIG_PCI_PASID
> +bool pci_pasid_supported(struct pci_dev *dev);
>   int pci_enable_pasid(struct pci_dev *pdev, int features);
>   void pci_disable_pasid(struct pci_dev *pdev);
>   int pci_pasid_features(struct pci_dev *pdev);
>   int pci_max_pasids(struct pci_dev *pdev);
>   #else /* CONFIG_PCI_PASID */
> +static inline bool pci_pasid_supported(struct pci_dev *d)
> +{ return false; }
>   static inline int pci_enable_pasid(struct pci_dev *pdev, int features)
>   { return -EINVAL; }
>   static inline void pci_disable_pasid(struct pci_dev *pdev) { }