Re: [PATCH v6 02/12] iommu: Add pasid_bits field in struct dev_iommu

From: Jason Gunthorpe via iommu <iommu@lists.linux-foundation.org>
To: Lu Baolu <baolu.lu@linux.intel.com>
Cc: Jean-Philippe Brucker <jean-philippe@linaro.org>,
	Kevin Tian <kevin.tian@intel.com>,
	Dave Jiang <dave.jiang@intel.com>,
	Ashok Raj <ashok.raj@intel.com>, Will Deacon <will@kernel.org>,
	iommu@lists.linux-foundation.org, linux-kernel@vger.kernel.org,
	Christoph Hellwig <hch@infradead.org>,
	Jean-Philippe Brucker <jean-philippe@linaro.com>,
	Vinod Koul <vkoul@kernel.org>,
	Jacob jun Pan <jacob.jun.pan@intel.com>,
	Robin Murphy <robin.murphy@arm.com>
Subject: Re: [PATCH v6 02/12] iommu: Add pasid_bits field in struct dev_iommu
Date: Tue, 10 May 2022 11:34:05 -0300	[thread overview]
Message-ID: <20220510143405.GE49344@nvidia.com> (raw)
In-Reply-To: <20220510061738.2761430-3-baolu.lu@linux.intel.com>

On Tue, May 10, 2022 at 02:17:28PM +0800, Lu Baolu wrote:

>  int iommu_device_register(struct iommu_device *iommu,
> 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 627a3ed5ee8f..afc63fce6107 100644
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -2681,6 +2681,8 @@ static struct iommu_device *arm_smmu_probe_device(struct device *dev)
>  	    smmu->features & ARM_SMMU_FEAT_STALL_FORCE)
>  		master->stall_enabled = true;
>  
> +	dev->iommu->pasid_bits = master->ssid_bits;
>  	return &smmu->iommu;
>  
>  err_free_master:
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index 2990f80c5e08..99643f897f26 100644
> +++ b/drivers/iommu/intel/iommu.c
> @@ -4624,8 +4624,11 @@ static struct iommu_device *intel_iommu_probe_device(struct device *dev)
>  			if (pasid_supported(iommu)) {
>  				int features = pci_pasid_features(pdev);
>  
> -				if (features >= 0)
> +				if (features >= 0) {
>  					info->pasid_supported = features | 1;
> +					dev->iommu->pasid_bits =
> +						fls(pci_max_pasids(pdev)) - 1;
> +				}

It is not very nice that both the iommu drivers have to duplicate the
code to read the pasid capability out of the PCI device.

IMHO it would make more sense for the iommu layer to report the
capability of its own HW block only, and for the core code to figure
out the master's limitation using a bus-specific approach.

It is also unfortunate that the enable/disable pasid is inside the
iommu driver as well - ideally the PCI driver itself would do this
when it knows it wants to use PASIDs.

The ordering interaction with ATS makes this look quite annoying
though. :(

I'm also not convinced individual IOMMU drivers should be forcing ATS
on, there are performance and functional implications here. Using ATS
or not is possibly best left as an administrator policy controlled by
the core code. Again we seem to have some mess.

Jason
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu