| Message ID | 20220518182120.1136715-3-jacob.jun.pan@linux.intel.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | Enable PASID for DMA API users | expand |
On 2022/5/19 02:21, Jacob Pan wrote: > IOMMU group maintains a PASID array which stores the associated IOMMU > domains. This patch introduces a helper function to do domain to PASID > look up. It will be used by TLB flush and device-PASID attach verification. Do you really need this? The IOMMU driver has maintained a device tracking list for each domain. It has been used for cache invalidation when unmap() is called against dma domain. Best regards, baolu > > Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com> > --- > drivers/iommu/iommu.c | 22 ++++++++++++++++++++++ > include/linux/iommu.h | 6 +++++- > 2 files changed, 27 insertions(+), 1 deletion(-) > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c > index 00d0262a1fe9..22f44833db64 100644 > --- a/drivers/iommu/iommu.c > +++ b/drivers/iommu/iommu.c > @@ -3199,3 +3199,25 @@ struct iommu_domain *iommu_get_domain_for_iopf(struct device *dev, > > return domain; > } > + > +ioasid_t iommu_get_pasid_from_domain(struct device *dev, struct iommu_domain *domain) > +{ > + struct iommu_domain *tdomain; > + struct iommu_group *group; > + unsigned long index; > + ioasid_t pasid = INVALID_IOASID; > + > + group = iommu_group_get(dev); > + if (!group) > + return pasid; > + > + xa_for_each(&group->pasid_array, index, tdomain) { > + if (domain == tdomain) { > + pasid = index; > + break; > + } > + } > + iommu_group_put(group); > + > + return pasid; > +} > diff --git a/include/linux/iommu.h b/include/linux/iommu.h > index 36ad007084cc..c0440a4be699 100644 > --- a/include/linux/iommu.h > +++ b/include/linux/iommu.h > @@ -694,7 +694,7 @@ void iommu_detach_device_pasid(struct iommu_domain *domain, > struct device *dev, ioasid_t pasid); > struct iommu_domain * > iommu_get_domain_for_iopf(struct device *dev, ioasid_t pasid); > - > +ioasid_t iommu_get_pasid_from_domain(struct device *dev, struct iommu_domain *domain); > #else /* CONFIG_IOMMU_API */ > > struct iommu_ops {}; > @@ -1070,6 +1070,10 @@ iommu_get_domain_for_iopf(struct device *dev, ioasid_t pasid) > { > return NULL; > } > +static ioasid_t iommu_get_pasid_from_domain(struct device *dev, struct iommu_domain *domain) > +{ > + return INVALID_IOASID; > +} > #endif /* CONFIG_IOMMU_API */ > > #ifdef CONFIG_IOMMU_SVA
On Wed, May 18, 2022 at 11:21:16AM -0700, Jacob Pan wrote:
> +ioasid_t iommu_get_pasid_from_domain(struct device *dev, struct iommu_domain *domain)
Overly long line here.
Hi Baolu, On Thu, 19 May 2022 14:41:06 +0800, Baolu Lu <baolu.lu@linux.intel.com> wrote: > > IOMMU group maintains a PASID array which stores the associated IOMMU > > domains. This patch introduces a helper function to do domain to PASID > > look up. It will be used by TLB flush and device-PASID attach > > verification. > > Do you really need this? > > The IOMMU driver has maintained a device tracking list for each domain. > It has been used for cache invalidation when unmap() is called against > dma domain. Yes, I am aware of the device list. In v3, I stored DMA API PASID in device list of device_domain_info. Since we already have a pasid_array, Jason suggested to share the storage with the code. This helper is needed to reverse look up the DMA PASID based on the domain attached. Discussions here: https://lore.kernel.org/lkml/20220511170025.GF49344@nvidia.com/t/#mf7cb7d54d89e6e732a020dc22435260da0a49580 Thanks, Jacob
Hi Christoph, On Wed, 18 May 2022 23:48:44 -0700, Christoph Hellwig <hch@infradead.org> wrote: > On Wed, May 18, 2022 at 11:21:16AM -0700, Jacob Pan wrote: > > +ioasid_t iommu_get_pasid_from_domain(struct device *dev, struct > > iommu_domain *domain) > > Overly long line here. will fix, Thanks, Jacob
> From: Jacob Pan <jacob.jun.pan@linux.intel.com> > Sent: Thursday, May 19, 2022 2:21 AM > > IOMMU group maintains a PASID array which stores the associated IOMMU > domains. This patch introduces a helper function to do domain to PASID > look up. It will be used by TLB flush and device-PASID attach verification. > > Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com> > --- > drivers/iommu/iommu.c | 22 ++++++++++++++++++++++ > include/linux/iommu.h | 6 +++++- > 2 files changed, 27 insertions(+), 1 deletion(-) > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c > index 00d0262a1fe9..22f44833db64 100644 > --- a/drivers/iommu/iommu.c > +++ b/drivers/iommu/iommu.c > @@ -3199,3 +3199,25 @@ struct iommu_domain > *iommu_get_domain_for_iopf(struct device *dev, > > return domain; > } > + > +ioasid_t iommu_get_pasid_from_domain(struct device *dev, struct > iommu_domain *domain) > +{ > + struct iommu_domain *tdomain; > + struct iommu_group *group; > + unsigned long index; > + ioasid_t pasid = INVALID_IOASID; > + > + group = iommu_group_get(dev); > + if (!group) > + return pasid; > + > + xa_for_each(&group->pasid_array, index, tdomain) { > + if (domain == tdomain) { > + pasid = index; > + break; > + } > + } Don't we need to acquire the group lock here? Btw the intention of this function is a bit confusing. Patch01 already stores the pasid under domain hence it's redundant to get it indirectly from xarray index. You could simply introduce a flag bit (e.g. dma_pasid_enabled) in device_domain_info and then directly use domain->dma_pasid once the flag is true. > + iommu_group_put(group); > + > + return pasid; > +} > diff --git a/include/linux/iommu.h b/include/linux/iommu.h > index 36ad007084cc..c0440a4be699 100644 > --- a/include/linux/iommu.h > +++ b/include/linux/iommu.h > @@ -694,7 +694,7 @@ void iommu_detach_device_pasid(struct > iommu_domain *domain, > struct device *dev, ioasid_t pasid); > struct iommu_domain * > iommu_get_domain_for_iopf(struct device *dev, ioasid_t pasid); > - > +ioasid_t iommu_get_pasid_from_domain(struct device *dev, struct > iommu_domain *domain); > #else /* CONFIG_IOMMU_API */ > > struct iommu_ops {}; > @@ -1070,6 +1070,10 @@ iommu_get_domain_for_iopf(struct device *dev, > ioasid_t pasid) > { > return NULL; > } > +static ioasid_t iommu_get_pasid_from_domain(struct device *dev, struct > iommu_domain *domain) > +{ > + return INVALID_IOASID; > +} > #endif /* CONFIG_IOMMU_API */ > > #ifdef CONFIG_IOMMU_SVA > -- > 2.25.1
> From: Tian, Kevin > Sent: Monday, May 23, 2022 3:55 PM > > > From: Jacob Pan <jacob.jun.pan@linux.intel.com> > > +ioasid_t iommu_get_pasid_from_domain(struct device *dev, struct > > iommu_domain *domain) > > +{ > > + struct iommu_domain *tdomain; > > + struct iommu_group *group; > > + unsigned long index; > > + ioasid_t pasid = INVALID_IOASID; > > + > > + group = iommu_group_get(dev); > > + if (!group) > > + return pasid; > > + > > + xa_for_each(&group->pasid_array, index, tdomain) { > > + if (domain == tdomain) { > > + pasid = index; > > + break; > > + } > > + } > > Don't we need to acquire the group lock here? > > Btw the intention of this function is a bit confusing. Patch01 already > stores the pasid under domain hence it's redundant to get it > indirectly from xarray index. You could simply introduce a flag bit > (e.g. dma_pasid_enabled) in device_domain_info and then directly > use domain->dma_pasid once the flag is true. > Just saw your discussion with Jason about v3. While it makes sense to not specialize DMA domain in iommu driver, the use of this function should only be that when the call chain doesn't pass down a pasid value e.g. when doing cache invalidation for domain map/unmap. If the upper interface already carries a pasid e.g. in detach_dev_pasid() iommu driver can simply verify that the corresponding pasid xarray entry points to the specified domain instead of using this function to loop xarray and then verify the returned pasid (as done in patch03/04). Thanks Kevin
Hi Kevin, On Mon, 23 May 2022 09:14:04 +0000, "Tian, Kevin" <kevin.tian@intel.com> wrote: > > From: Tian, Kevin > > Sent: Monday, May 23, 2022 3:55 PM > > > > > From: Jacob Pan <jacob.jun.pan@linux.intel.com> > > > +ioasid_t iommu_get_pasid_from_domain(struct device *dev, struct > > > iommu_domain *domain) > > > +{ > > > + struct iommu_domain *tdomain; > > > + struct iommu_group *group; > > > + unsigned long index; > > > + ioasid_t pasid = INVALID_IOASID; > > > + > > > + group = iommu_group_get(dev); > > > + if (!group) > > > + return pasid; > > > + > > > + xa_for_each(&group->pasid_array, index, tdomain) { > > > + if (domain == tdomain) { > > > + pasid = index; > > > + break; > > > + } > > > + } > > > > Don't we need to acquire the group lock here? > > pasid_array is under RCU read lock so it is protected though may have stale data. It also used in atomic context for TLB flush, cannot take the group mutex. If the caller does detach_dev_pasid while doing TLB flush, it could result in extra flush but harmless. > > Btw the intention of this function is a bit confusing. Patch01 already > > stores the pasid under domain hence it's redundant to get it > > indirectly from xarray index. You could simply introduce a flag bit > > (e.g. dma_pasid_enabled) in device_domain_info and then directly > > use domain->dma_pasid once the flag is true. > > > > Just saw your discussion with Jason about v3. While it makes sense > to not specialize DMA domain in iommu driver, the use of this function > should only be that when the call chain doesn't pass down a pasid > value e.g. when doing cache invalidation for domain map/unmap. If > the upper interface already carries a pasid e.g. in detach_dev_pasid() > iommu driver can simply verify that the corresponding pasid xarray > entry points to the specified domain instead of using this function to > loop xarray and then verify the returned pasid (as done in patch03/04). Excellent point, I could just use xa_load(pasid) to compare the domain instead of loop through xa. I will add another helper. bool iommu_is_pasid_domain_attached(struct device *dev, struct iommu_domain *domain, ioasid_t pasid) { struct iommu_group *group; bool ret = false; group = iommu_group_get(dev); if (WARN_ON(!group)) return false; if (domain == xa_load(&group->pasid_array, pasid)) ret = true; iommu_group_put(group); return ret; } Thanks, Jacob
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index 00d0262a1fe9..22f44833db64 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -3199,3 +3199,25 @@ struct iommu_domain *iommu_get_domain_for_iopf(struct device *dev, return domain; } + +ioasid_t iommu_get_pasid_from_domain(struct device *dev, struct iommu_domain *domain) +{ + struct iommu_domain *tdomain; + struct iommu_group *group; + unsigned long index; + ioasid_t pasid = INVALID_IOASID; + + group = iommu_group_get(dev); + if (!group) + return pasid; + + xa_for_each(&group->pasid_array, index, tdomain) { + if (domain == tdomain) { + pasid = index; + break; + } + } + iommu_group_put(group); + + return pasid; +} diff --git a/include/linux/iommu.h b/include/linux/iommu.h index 36ad007084cc..c0440a4be699 100644 --- a/include/linux/iommu.h +++ b/include/linux/iommu.h @@ -694,7 +694,7 @@ void iommu_detach_device_pasid(struct iommu_domain *domain, struct device *dev, ioasid_t pasid); struct iommu_domain * iommu_get_domain_for_iopf(struct device *dev, ioasid_t pasid); - +ioasid_t iommu_get_pasid_from_domain(struct device *dev, struct iommu_domain *domain); #else /* CONFIG_IOMMU_API */ struct iommu_ops {}; @@ -1070,6 +1070,10 @@ iommu_get_domain_for_iopf(struct device *dev, ioasid_t pasid) { return NULL; } +static ioasid_t iommu_get_pasid_from_domain(struct device *dev, struct iommu_domain *domain) +{ + return INVALID_IOASID; +} #endif /* CONFIG_IOMMU_API */ #ifdef CONFIG_IOMMU_SVA
IOMMU group maintains a PASID array which stores the associated IOMMU domains. This patch introduces a helper function to do domain to PASID look up. It will be used by TLB flush and device-PASID attach verification. Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com> --- drivers/iommu/iommu.c | 22 ++++++++++++++++++++++ include/linux/iommu.h | 6 +++++- 2 files changed, 27 insertions(+), 1 deletion(-)