| Message ID | 20170227195441.5170-21-jean-philippe.brucker@arm.com |
|---|---|
| State | Not Applicable |
| Headers | show |
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
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
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).
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
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 --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); }
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(-)