| Message ID | 20241104131842.13303-6-yi.l.liu@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Make set_dev_pasid op supporting domain replacement | expand |
On 11/4/24 21:18, Yi Liu wrote: > To handle domain replacement, the intel_iommu_set_dev_pasid() needs to > keep the old configuration and the prepare for the new setup. This requires > a bit refactoring to prepare for it. Above description is a bit hard to understand, are you saying ... the intel_iommu_set_dev_pasid() needs to roll back to the old configuration in the failure path, therefore refactor it to prepare for the subsequent patches ... ? > > domain_add_dev_pasid() and domain_remove_dev_pasid() are added to add/remove > the dev_pasid_info which represents the association of the pasid/device and > domain. Till now, it's still not ready for replacement yet. > > Signed-off-by: Yi Liu<yi.l.liu@intel.com> > --- > drivers/iommu/intel/iommu.c | 90 +++++++++++++++++++++++++------------ > 1 file changed, 61 insertions(+), 29 deletions(-) The change itself looks good to me, Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>
On 2024/11/5 10:49, Baolu Lu wrote: > On 11/4/24 21:18, Yi Liu wrote: >> To handle domain replacement, the intel_iommu_set_dev_pasid() needs to >> keep the old configuration and the prepare for the new setup. This requires >> a bit refactoring to prepare for it. > > Above description is a bit hard to understand, are you saying > > ... the intel_iommu_set_dev_pasid() needs to roll back to the old > configuration in the failure path, therefore refactor it to prepare for > the subsequent patches ... This is the partial reason, but not the most related reason of this patch. Say without this patch, the intel_iommu_set_dev_pasid() call avoid roll back to the old configuration in the failure path as long as it calls the pasid replace helpers. So I chose to describe like the above. Maybe another choice is to name this patch as consolidate the dev_pasid_info adding and removing to be a paired helpers. This can be used by other set_dev_pasid op within intel iommu driver. > ? > >> >> domain_add_dev_pasid() and domain_remove_dev_pasid() are added to add/remove >> the dev_pasid_info which represents the association of the pasid/device and >> domain. Till now, it's still not ready for replacement yet. >> >> Signed-off-by: Yi Liu<yi.l.liu@intel.com> >> --- >> drivers/iommu/intel/iommu.c | 90 +++++++++++++++++++++++++------------ >> 1 file changed, 61 insertions(+), 29 deletions(-) > > The change itself looks good to me, > > Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com> >
> From: Liu, Yi L <yi.l.liu@intel.com> > Sent: Tuesday, November 5, 2024 1:24 PM > > On 2024/11/5 10:49, Baolu Lu wrote: > > On 11/4/24 21:18, Yi Liu wrote: > >> To handle domain replacement, the intel_iommu_set_dev_pasid() needs > to > >> keep the old configuration and the prepare for the new setup. This > requires > >> a bit refactoring to prepare for it. > > > > Above description is a bit hard to understand, are you saying > > > > ... the intel_iommu_set_dev_pasid() needs to roll back to the old > > configuration in the failure path, therefore refactor it to prepare for > > the subsequent patches ... > > This is the partial reason, but not the most related reason of this patch. > Say without this patch, the intel_iommu_set_dev_pasid() call avoid roll > back to the old configuration in the failure path as long as it calls the > pasid replace helpers. So I chose to describe like the above. Maybe another > choice is to name this patch as consolidate the dev_pasid_info adding and > removing to be a paired helpers. This can be used by other set_dev_pasid op > within intel iommu driver. > paired helpers is clearer.
> From: Liu, Yi L <yi.l.liu@intel.com> > Sent: Monday, November 4, 2024 9:19 PM > > + > +static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t > pasid, > + struct iommu_domain *domain) > +{ > + struct device_domain_info *info = dev_iommu_priv_get(dev); > + struct intel_iommu *iommu = info->iommu; > + > intel_pasid_tear_down_entry(iommu, dev, pasid, false); > intel_drain_pasid_prq(dev, pasid); > + domain_remove_dev_pasid(domain, dev, pasid); this changes the order between physical teardown and software teardown. but looks harmless. > @@ -4310,17 +4357,9 @@ static int intel_iommu_set_dev_pasid(struct > iommu_domain *domain, > if (ret) > return ret; > > - dev_pasid = kzalloc(sizeof(*dev_pasid), GFP_KERNEL); > - if (!dev_pasid) > - return -ENOMEM; > - > - ret = domain_attach_iommu(dmar_domain, iommu); > - if (ret) > - goto out_free; > - > - ret = cache_tag_assign_domain(dmar_domain, dev, pasid); > - if (ret) > - goto out_detach_iommu; > + dev_pasid = domain_add_dev_pasid(domain, dev, pasid); > + if (IS_ERR(dev_pasid)) > + return PTR_ERR(dev_pasid); > > if (dmar_domain->use_first_level) > ret = domain_setup_first_level(iommu, dmar_domain, this also changes the order i.e. a dev_pasid might be valid in the list before its pasid entry is configured. so other places walking the list must not assume every node has a valid entry. what about adding a note to the structure field? > @@ -4329,24 +4368,17 @@ static int intel_iommu_set_dev_pasid(struct > iommu_domain *domain, > ret = intel_pasid_setup_second_level(iommu, dmar_domain, > dev, pasid); > if (ret) > - goto out_unassign_tag; > + goto out_remove_dev_pasid; > > - dev_pasid->dev = dev; > - dev_pasid->pasid = pasid; > - spin_lock_irqsave(&dmar_domain->lock, flags); > - list_add(&dev_pasid->link_domain, &dmar_domain->dev_pasids); > - spin_unlock_irqrestore(&dmar_domain->lock, flags); > + domain_remove_dev_pasid(old, dev, pasid); My preference is moving the check of non-NULL old out here. otherwise looks good, Reviewed-by: Kevin Tian <kevin.tian@intel.com>
On 2024/11/6 15:41, Tian, Kevin wrote: >> From: Liu, Yi L <yi.l.liu@intel.com> >> Sent: Monday, November 4, 2024 9:19 PM >> >> + >> +static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t >> pasid, >> + struct iommu_domain *domain) >> +{ >> + struct device_domain_info *info = dev_iommu_priv_get(dev); >> + struct intel_iommu *iommu = info->iommu; >> + >> intel_pasid_tear_down_entry(iommu, dev, pasid, false); >> intel_drain_pasid_prq(dev, pasid); >> + domain_remove_dev_pasid(domain, dev, pasid); > > this changes the order between physical teardown and software teardown. > > but looks harmless. yes. >> @@ -4310,17 +4357,9 @@ static int intel_iommu_set_dev_pasid(struct >> iommu_domain *domain, >> if (ret) >> return ret; >> >> - dev_pasid = kzalloc(sizeof(*dev_pasid), GFP_KERNEL); >> - if (!dev_pasid) >> - return -ENOMEM; >> - >> - ret = domain_attach_iommu(dmar_domain, iommu); >> - if (ret) >> - goto out_free; >> - >> - ret = cache_tag_assign_domain(dmar_domain, dev, pasid); >> - if (ret) >> - goto out_detach_iommu; >> + dev_pasid = domain_add_dev_pasid(domain, dev, pasid); >> + if (IS_ERR(dev_pasid)) >> + return PTR_ERR(dev_pasid); >> >> if (dmar_domain->use_first_level) >> ret = domain_setup_first_level(iommu, dmar_domain, > > this also changes the order i.e. a dev_pasid might be valid in the list > before its pasid entry is configured. so other places walking the list > must not assume every node has a valid entry. what about adding > a note to the structure field? Do you mean a mark to say the entry is valid or not? Perhaps it's not needed. Even it is treated as a valid entry in the new domain or the old domain, it looks to be fine. The major usage of this structure are the cache invalidation (already dropped, but it is an example)and svm mm release path. Either path looks to be fine as they just do more things that are harmless. >> @@ -4329,24 +4368,17 @@ static int intel_iommu_set_dev_pasid(struct >> iommu_domain *domain, >> ret = intel_pasid_setup_second_level(iommu, dmar_domain, >> dev, pasid); >> if (ret) >> - goto out_unassign_tag; >> + goto out_remove_dev_pasid; >> >> - dev_pasid->dev = dev; >> - dev_pasid->pasid = pasid; >> - spin_lock_irqsave(&dmar_domain->lock, flags); >> - list_add(&dev_pasid->link_domain, &dmar_domain->dev_pasids); >> - spin_unlock_irqrestore(&dmar_domain->lock, flags); >> + domain_remove_dev_pasid(old, dev, pasid); > > My preference is moving the check of non-NULL old out here. @Baolu, how about your thought? > otherwise looks good, > > Reviewed-by: Kevin Tian <kevin.tian@intel.com>
On 2024/11/6 16:02, Yi Liu wrote: >>> @@ -4329,24 +4368,17 @@ static int intel_iommu_set_dev_pasid(struct >>> iommu_domain *domain, >>> ret = intel_pasid_setup_second_level(iommu, dmar_domain, >>> dev, pasid); >>> if (ret) >>> - goto out_unassign_tag; >>> + goto out_remove_dev_pasid; >>> >>> - dev_pasid->dev = dev; >>> - dev_pasid->pasid = pasid; >>> - spin_lock_irqsave(&dmar_domain->lock, flags); >>> - list_add(&dev_pasid->link_domain, &dmar_domain->dev_pasids); >>> - spin_unlock_irqrestore(&dmar_domain->lock, flags); >>> + domain_remove_dev_pasid(old, dev, pasid); >> >> My preference is moving the check of non-NULL old out here. > > @Baolu, how about your thought? If we move the check out of this helper, there will be boilerplate code in multiple places. Something like, if (old) domain_remove_dev_pasid(old, dev, pasid); -- baolu
> From: Baolu Lu <baolu.lu@linux.intel.com> > Sent: Wednesday, November 6, 2024 4:39 PM > > On 2024/11/6 16:02, Yi Liu wrote: > >>> @@ -4329,24 +4368,17 @@ static int intel_iommu_set_dev_pasid(struct > >>> iommu_domain *domain, > >>> ret = intel_pasid_setup_second_level(iommu, dmar_domain, > >>> dev, pasid); > >>> if (ret) > >>> - goto out_unassign_tag; > >>> + goto out_remove_dev_pasid; > >>> > >>> - dev_pasid->dev = dev; > >>> - dev_pasid->pasid = pasid; > >>> - spin_lock_irqsave(&dmar_domain->lock, flags); > >>> - list_add(&dev_pasid->link_domain, &dmar_domain->dev_pasids); > >>> - spin_unlock_irqrestore(&dmar_domain->lock, flags); > >>> + domain_remove_dev_pasid(old, dev, pasid); > >> > >> My preference is moving the check of non-NULL old out here. > > > > @Baolu, how about your thought? > > If we move the check out of this helper, there will be boilerplate code > in multiple places. Something like, > > if (old) > domain_remove_dev_pasid(old, dev, pasid); > yes, but the logic is slightly clearer to reflect a replace operation in the caller side instead of pretending it to be a mandatory one.
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c index 86000901de46..6bc5ce03c6f5 100644 --- a/drivers/iommu/intel/iommu.c +++ b/drivers/iommu/intel/iommu.c @@ -4252,8 +4252,8 @@ static int intel_iommu_iotlb_sync_map(struct iommu_domain *domain, return 0; } -static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid, - struct iommu_domain *domain) +static void domain_remove_dev_pasid(struct iommu_domain *domain, + struct device *dev, ioasid_t pasid) { struct device_domain_info *info = dev_iommu_priv_get(dev); struct dev_pasid_info *curr, *dev_pasid = NULL; @@ -4261,10 +4261,12 @@ static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid, struct dmar_domain *dmar_domain; unsigned long flags; - if (domain->type == IOMMU_DOMAIN_IDENTITY) { - intel_pasid_tear_down_entry(iommu, dev, pasid, false); + if (!domain) + return; + + /* Identity domain has no meta data for pasid. */ + if (domain->type == IOMMU_DOMAIN_IDENTITY) return; - } dmar_domain = to_dmar_domain(domain); spin_lock_irqsave(&dmar_domain->lock, flags); @@ -4282,8 +4284,54 @@ static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid, domain_detach_iommu(dmar_domain, iommu); intel_iommu_debugfs_remove_dev_pasid(dev_pasid); kfree(dev_pasid); +} + +static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid, + struct iommu_domain *domain) +{ + struct device_domain_info *info = dev_iommu_priv_get(dev); + struct intel_iommu *iommu = info->iommu; + intel_pasid_tear_down_entry(iommu, dev, pasid, false); intel_drain_pasid_prq(dev, pasid); + domain_remove_dev_pasid(domain, dev, pasid); +} + +static struct dev_pasid_info * +domain_add_dev_pasid(struct iommu_domain *domain, + struct device *dev, ioasid_t pasid) +{ + struct device_domain_info *info = dev_iommu_priv_get(dev); + struct dmar_domain *dmar_domain = to_dmar_domain(domain); + struct intel_iommu *iommu = info->iommu; + struct dev_pasid_info *dev_pasid; + unsigned long flags; + int ret; + + dev_pasid = kzalloc(sizeof(*dev_pasid), GFP_KERNEL); + if (!dev_pasid) + return ERR_PTR(-ENOMEM); + + ret = domain_attach_iommu(dmar_domain, iommu); + if (ret) + goto out_free; + + ret = cache_tag_assign_domain(dmar_domain, dev, pasid); + if (ret) + goto out_detach_iommu; + + dev_pasid->dev = dev; + dev_pasid->pasid = pasid; + spin_lock_irqsave(&dmar_domain->lock, flags); + list_add(&dev_pasid->link_domain, &dmar_domain->dev_pasids); + spin_unlock_irqrestore(&dmar_domain->lock, flags); + + return dev_pasid; +out_detach_iommu: + domain_detach_iommu(dmar_domain, iommu); +out_free: + kfree(dev_pasid); + return ERR_PTR(ret); } static int intel_iommu_set_dev_pasid(struct iommu_domain *domain, @@ -4294,7 +4342,6 @@ static int intel_iommu_set_dev_pasid(struct iommu_domain *domain, struct dmar_domain *dmar_domain = to_dmar_domain(domain); struct intel_iommu *iommu = info->iommu; struct dev_pasid_info *dev_pasid; - unsigned long flags; int ret; if (!pasid_supported(iommu) || dev_is_real_dma_subdevice(dev)) @@ -4310,17 +4357,9 @@ static int intel_iommu_set_dev_pasid(struct iommu_domain *domain, if (ret) return ret; - dev_pasid = kzalloc(sizeof(*dev_pasid), GFP_KERNEL); - if (!dev_pasid) - return -ENOMEM; - - ret = domain_attach_iommu(dmar_domain, iommu); - if (ret) - goto out_free; - - ret = cache_tag_assign_domain(dmar_domain, dev, pasid); - if (ret) - goto out_detach_iommu; + dev_pasid = domain_add_dev_pasid(domain, dev, pasid); + if (IS_ERR(dev_pasid)) + return PTR_ERR(dev_pasid); if (dmar_domain->use_first_level) ret = domain_setup_first_level(iommu, dmar_domain, @@ -4329,24 +4368,17 @@ static int intel_iommu_set_dev_pasid(struct iommu_domain *domain, ret = intel_pasid_setup_second_level(iommu, dmar_domain, dev, pasid); if (ret) - goto out_unassign_tag; + goto out_remove_dev_pasid; - dev_pasid->dev = dev; - dev_pasid->pasid = pasid; - spin_lock_irqsave(&dmar_domain->lock, flags); - list_add(&dev_pasid->link_domain, &dmar_domain->dev_pasids); - spin_unlock_irqrestore(&dmar_domain->lock, flags); + domain_remove_dev_pasid(old, dev, pasid); if (domain->type & __IOMMU_DOMAIN_PAGING) intel_iommu_debugfs_create_dev_pasid(dev_pasid); return 0; -out_unassign_tag: - cache_tag_unassign_domain(dmar_domain, dev, pasid); -out_detach_iommu: - domain_detach_iommu(dmar_domain, iommu); -out_free: - kfree(dev_pasid); + +out_remove_dev_pasid: + domain_remove_dev_pasid(domain, dev, pasid); return ret; }
To handle domain replacement, the intel_iommu_set_dev_pasid() needs to keep the old configuration and the prepare for the new setup. This requires a bit refactoring to prepare for it. domain_add_dev_pasid() and domain_remove_dev_pasid() are added to add/remove the dev_pasid_info which represents the association of the pasid/device and domain. Till now, it's still not ready for replacement yet. Signed-off-by: Yi Liu <yi.l.liu@intel.com> --- drivers/iommu/intel/iommu.c | 90 +++++++++++++++++++++++++------------ 1 file changed, 61 insertions(+), 29 deletions(-)