diff mbox

[RFC,20/30] iommu/arm-smmu-v3: Enable PCI PASID in masters

Message ID 20170227195441.5170-21-jean-philippe.brucker@arm.com (mailing list archive)
State New, archived
Headers show

Commit Message

Jean-Philippe Brucker Feb. 27, 2017, 7:54 p.m. UTC
Enable PASID for PCI devices that support it.

Signed-off-by: Jean-Philippe Brucker <jean-philippe.brucker@arm.com>
---
 drivers/iommu/arm-smmu-v3.c | 66 ++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 63 insertions(+), 3 deletions(-)

Comments

Sinan Kaya May 31, 2017, 2:10 p.m. UTC | #1
Hi Jean-Philippe,

On 2/27/2017 2:54 PM, Jean-Philippe Brucker wrote:
> Enable PASID for PCI devices that support it.
> 
> Signed-off-by: Jean-Philippe Brucker <jean-philippe.brucker@arm.com>
> ---
>  drivers/iommu/arm-smmu-v3.c | 66 ++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 63 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> index 499dc1cd07eb..37fd061405e9 100644
> --- a/drivers/iommu/arm-smmu-v3.c
> +++ b/drivers/iommu/arm-smmu-v3.c
> @@ -730,6 +730,8 @@ struct arm_smmu_master_data {
>  
>  	struct arm_smmu_stream		*streams;
>  	struct rb_root			contexts;
> +
> +	u32				avail_contexts;
>  };
>  

I know that you are doing some code restructuring. As I was looking at the amdkfd
driver, I realized that there is quite a bit of PASID, ATS and PRI work here that
can be plumbed into the framework you are building. 

https://github.com/torvalds/linux/tree/master/drivers/gpu/drm/amd/amdkfd

I wanted to share this with if you were aware of this or not. Functions of interest
are:

amd_iommu_init_device
amd_iommu_free_device
amd_iommu_bind_pasid
amd_iommu_set_invalid_ppr_cb
amd_iommu_unbind_pasid
amd_iommu_device_info

Sinan
Jean-Philippe Brucker June 1, 2017, 12:30 p.m. UTC | #2
Hi Sinan,

On 31/05/17 15:10, Sinan Kaya wrote:
> Hi Jean-Philippe,
> 
> On 2/27/2017 2:54 PM, Jean-Philippe Brucker wrote:
>> Enable PASID for PCI devices that support it.
>>
>> Signed-off-by: Jean-Philippe Brucker <jean-philippe.brucker@arm.com>
>> ---
>>  drivers/iommu/arm-smmu-v3.c | 66 ++++++++++++++++++++++++++++++++++++++++++---
>>  1 file changed, 63 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
>> index 499dc1cd07eb..37fd061405e9 100644
>> --- a/drivers/iommu/arm-smmu-v3.c
>> +++ b/drivers/iommu/arm-smmu-v3.c
>> @@ -730,6 +730,8 @@ struct arm_smmu_master_data {
>>  
>>  	struct arm_smmu_stream		*streams;
>>  	struct rb_root			contexts;
>> +
>> +	u32				avail_contexts;
>>  };
>>  
> 
> I know that you are doing some code restructuring. As I was looking at the amdkfd
> driver, I realized that there is quite a bit of PASID, ATS and PRI work here that
> can be plumbed into the framework you are building. 
>
> https://github.com/torvalds/linux/tree/master/drivers/gpu/drm/amd/amdkfd

Yes, the current plan is to create a PASID (SSID) allocator that could be
used by AMD, Intel, ARM, and other IOMMUs. Currently the kfd driver
allocates PASIDs, but this will be done by the IOMMU subsystem in the
future. Writing the generic allocator isn't on my schedule for the next
few months, though.

I'm trying to implement the next version of SMMU SVM with the following
principles in mind, and Intel are reworking their PASID allocator similarly.

* One PASID per task. Therefore bind(devA, task1) bind(devB, task1) will
return the same PASID. The PASID space is system-wide, just like ASIDs in ARM.

For ARM:

* PASID != ASID, because the PASID range depends on device capabilities,
while ASID range depends on the CPU. So PASID table might be smaller than
ASID range.

* PASID range and other SVM capabilities are capped by the weakest device
in an iommu_domain. This allows the SMMU driver to have a single context
table per domain. The downside is that if you attach the odd device that
doesn't support SVM to a domain before the first bind, then bind fails
(for any device in the domain). If you attach it after the first bind,
then the attach fails.

> I wanted to share this with if you were aware of this or not. Functions of interest
> are:
> 
> amd_iommu_init_device
> amd_iommu_free_device
> amd_iommu_bind_pasid
> amd_iommu_set_invalid_ppr_cb
> amd_iommu_unbind_pasid
> amd_iommu_device_info
> 

* amd_iommu_bind/unbind_pasid would be replaced by iommu_bind/unbind
* amd_iommu_set_invalid_ppr_cb/set_invalidate_ctx_cb would be replaced by
iommu_set_svm_ops.
* amd_iommu_init_device/amd_iommu_free_device would be performed
internally. init_device could be done by iommu_attach_dev, or by
iommu_bind lazily. free_device would be done by detach_dev.
* and_iommu_device_info may not be needed. Drivers can use iommu_bind to
check SVM capability (maybe with a dry-run flag like intel does).

Thanks,
Jean
David Woodhouse June 1, 2017, 12:30 p.m. UTC | #3
On Thu, 2017-06-01 at 13:30 +0100, Jean-Philippe Brucker wrote:
> 
> Yes, the current plan is to create a PASID (SSID) allocator that could be
> used by AMD, Intel, ARM, and other IOMMUs. Currently the kfd driver
> allocates PASIDs, but this will be done by the IOMMU subsystem in the
> future. Writing the generic allocator isn't on my schedule for the next
> few months, though.
> 
> I'm trying to implement the next version of SMMU SVM with the following
> principles in mind, and Intel are reworking their PASID allocator similarly.

Note that the Intel allocator is designed to work that way and to be
easily made generic (that is, easily used from elsewhere).
Sinan Kaya June 23, 2017, 2:39 p.m. UTC | #4
Hi Jean-Philippe,
 
> On 2/27/2017 2:54 PM, Jean-Philippe Brucker wrote:
> Enable PASID for PCI devices that support it.
>
> Signed-off-by: Jean-Philippe Brucker <jean-philippe.brucker@arm.com>
> ---
>  drivers/iommu/arm-smmu-v3.c | 66 ++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 63 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> index 499dc1cd07eb..37fd061405e9 100644
> --- a/drivers/iommu/arm-smmu-v3.c
> +++ b/drivers/iommu/arm-smmu-v3.c
> @@ -730,6 +730,8 @@ struct arm_smmu_master_data {
>  
>  	struct arm_smmu_stream		*streams;
>  	struct rb_root			contexts;
> +
> +	u32				avail_contexts;
>  };
>  

According to the PASID ECN here (https://pcisig.com/sites/default/files/specification_documents/ECN-PASID-ATS-2011-03-31.pdf),
PASID should be enabled only if all switches between the root port and
a device support TLP prefix. 
 
I'm only seeing a call to pci_enable_pasid() in this patch but I don't
see anybody checking for TLP prefix support on the hierarchy.

This could potentially be an addition to the PCI core code.

Sinan
Jean-Philippe Brucker June 23, 2017, 3:15 p.m. UTC | #5
On 06/23/2017 03:39 PM, Sinan Kaya wrote:
> Hi Jean-Philippe,
>  
>> On 2/27/2017 2:54 PM, Jean-Philippe Brucker wrote:
>> Enable PASID for PCI devices that support it.
>>
>> Signed-off-by: Jean-Philippe Brucker <jean-philippe.brucker@arm.com>
>> ---
>>  drivers/iommu/arm-smmu-v3.c | 66 ++++++++++++++++++++++++++++++++++++++++++---
>>  1 file changed, 63 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
>> index 499dc1cd07eb..37fd061405e9 100644
>> --- a/drivers/iommu/arm-smmu-v3.c
>> +++ b/drivers/iommu/arm-smmu-v3.c
>> @@ -730,6 +730,8 @@ struct arm_smmu_master_data {
>>  
>>  	struct arm_smmu_stream		*streams;
>>  	struct rb_root			contexts;
>> +
>> +	u32				avail_contexts;
>>  };
>>  
> 
> According to the PASID ECN here (https://pcisig.com/sites/default/files/specification_documents/ECN-PASID-ATS-2011-03-31.pdf),
> PASID should be enabled only if all switches between the root port and
> a device support TLP prefix. 
>  
> I'm only seeing a call to pci_enable_pasid() in this patch but I don't
> see anybody checking for TLP prefix support on the hierarchy.
> 
> This could potentially be an addition to the PCI core code.

Good point, I think we should inspect bit "End-End TLP Prefix Supported"
in the PCI_EXP_DEVCAP2 capability of all bridges between the endpoint
and the IOMMU. And it should probably be done inside pci_enable_pasid().

Thanks,
Jean
diff mbox

Patch

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index 499dc1cd07eb..37fd061405e9 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -730,6 +730,8 @@  struct arm_smmu_master_data {
 
 	struct arm_smmu_stream		*streams;
 	struct rb_root			contexts;
+
+	u32				avail_contexts;
 };
 
 /* SMMU private data for an IOMMU domain */
@@ -2954,6 +2956,47 @@  static void arm_smmu_disable_ats(struct arm_smmu_master_data *master)
 	pci_disable_ats(pdev);
 }
 
+static int arm_smmu_enable_ssid(struct arm_smmu_master_data *master)
+{
+	int ret;
+	int features;
+	int nr_ssids;
+	struct pci_dev *pdev;
+
+	if (!dev_is_pci(master->dev))
+		return -ENOSYS;
+
+	pdev = to_pci_dev(master->dev);
+
+	features = pci_pasid_features(pdev);
+	if (features < 0)
+		return -ENOSYS;
+
+	nr_ssids = pci_max_pasids(pdev);
+
+	dev_dbg(&pdev->dev, "device supports %#x SSIDs [%s%s]\n", nr_ssids,
+		(features & PCI_PASID_CAP_EXEC) ? "x" : "",
+		(features & PCI_PASID_CAP_PRIV) ? "p" : "");
+
+	ret = pci_enable_pasid(pdev, features);
+	return ret ? ret : nr_ssids;
+}
+
+static void arm_smmu_disable_ssid(struct arm_smmu_master_data *master)
+{
+	struct pci_dev *pdev;
+
+	if (!dev_is_pci(master->dev))
+		return;
+
+	pdev = to_pci_dev(master->dev);
+
+	if (!pdev->pasid_enabled)
+		return;
+
+	pci_disable_pasid(pdev);
+}
+
 static int arm_smmu_insert_master(struct arm_smmu_device *smmu,
 				  struct arm_smmu_master_data *master)
 {
@@ -3007,6 +3050,7 @@  static struct iommu_ops arm_smmu_ops;
 static int arm_smmu_add_device(struct device *dev)
 {
 	int i, ret;
+	int nr_ssids;
 	bool ats_enabled;
 	unsigned long flags;
 	struct arm_smmu_device *smmu;
@@ -3055,9 +3099,19 @@  static int arm_smmu_add_device(struct device *dev)
 		}
 	}
 
-	ret = arm_smmu_alloc_cd_tables(master, 1);
-	if (ret < 0)
-		return ret;
+	/* PCIe PASID must be enabled before ATS */
+	nr_ssids = arm_smmu_enable_ssid(master);
+	if (nr_ssids <= 0)
+		nr_ssids = 1;
+
+	nr_ssids = arm_smmu_alloc_cd_tables(master, nr_ssids);
+	if (nr_ssids < 0) {
+		ret = nr_ssids;
+		goto err_disable_ssid;
+	}
+
+	/* SSID0 is reserved */
+	master->avail_contexts = nr_ssids - 1;
 
 	ats_enabled = !arm_smmu_enable_ats(master);
 
@@ -3088,6 +3142,9 @@  static int arm_smmu_add_device(struct device *dev)
 
 	arm_smmu_free_cd_tables(master);
 
+err_disable_ssid:
+	arm_smmu_disable_ssid(master);
+
 	return ret;
 }
 
@@ -3129,7 +3186,10 @@  static void arm_smmu_remove_device(struct device *dev)
 
 		iommu_group_put(group);
 
+		/* PCIe PASID must be disabled after ATS */
 		arm_smmu_disable_ats(master);
+		arm_smmu_disable_ssid(master);
+
 		arm_smmu_free_cd_tables(master);
 	}