Message ID | 20240912130427.10119-1-yi.l.liu@intel.com |
---|---|
Headers | show |
Series | Make set_dev_pasid op supporting domain replacement | expand |
On 9/12/24 9:04 PM, Yi Liu wrote: > @@ -4325,24 +4363,18 @@ 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_undo_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); > + if (old) > + 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_undo_dev_pasid: > + domain_remove_dev_pasid(domain, dev, pasid); > return ret; > } Do you need to re-install the old domain to the pasid entry in the failure path? Thanks, baolu
On 9/13/24 9:35 AM, Baolu Lu wrote: > On 9/12/24 9:04 PM, Yi Liu wrote: >> set_dev_pasid op is going to support domain replacement and keep the old >> hardware config if it fails. Make the Intel iommu driver be prepared for >> it. >> >> Signed-off-by: Yi Liu <yi.l.liu@intel.com> >> --- >> drivers/iommu/intel/iommu.c | 98 ++++++++++++++++++++++++------------- >> 1 file changed, 65 insertions(+), 33 deletions(-) >> >> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c >> index 80b587de226d..6f5a8e549f3f 100644 >> --- a/drivers/iommu/intel/iommu.c >> +++ b/drivers/iommu/intel/iommu.c >> @@ -4248,8 +4248,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; >> @@ -4257,11 +4257,6 @@ 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, 0); >> - return; >> - } >> - >> dmar_domain = to_dmar_domain(domain); >> spin_lock_irqsave(&dmar_domain->lock, flags); >> list_for_each_entry(curr, &dmar_domain->dev_pasids, link_domain) { >> @@ -4278,13 +4273,24 @@ 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, >> INTEL_PASID_TEARDOWN_DRAIN_PRQ); >> + if (domain->type == IOMMU_DOMAIN_IDENTITY) >> + return; > > The static identity domain is not capable of handling page requests. > Therefore there is no need to drain PRQ for an identity domain removal. > > So it probably should be something like this: > > if (domain->type == IOMMU_DOMAIN_IDENTITY) { > intel_pasid_tear_down_entry(iommu, dev, pasid, 0); > return; > } > > intel_pasid_tear_down_entry(iommu, dev, pasid, > INTEL_PASID_TEARDOWN_DRAIN_PRQ); Just revisited this. It seems that we just need to drain PRQ if the attached domain is iopf-capable. Therefore, how about making it like this? unsigned int flags = 0; if (domain->iopf_handler) flags |= INTEL_PASID_TEARDOWN_DRAIN_PRQ; intel_pasid_tear_down_entry(iommu, dev, pasid, flags); /* Identity domain has no meta data for pasid. */ if (domain->type == IOMMU_DOMAIN_IDENTITY) return; Thanks, baolu
On 2024/9/13 10:17, Baolu Lu wrote: > On 9/13/24 9:35 AM, Baolu Lu wrote: >> On 9/12/24 9:04 PM, Yi Liu wrote: >>> set_dev_pasid op is going to support domain replacement and keep the old >>> hardware config if it fails. Make the Intel iommu driver be prepared for >>> it. >>> >>> Signed-off-by: Yi Liu <yi.l.liu@intel.com> >>> --- >>> drivers/iommu/intel/iommu.c | 98 ++++++++++++++++++++++++------------- >>> 1 file changed, 65 insertions(+), 33 deletions(-) >>> >>> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c >>> index 80b587de226d..6f5a8e549f3f 100644 >>> --- a/drivers/iommu/intel/iommu.c >>> +++ b/drivers/iommu/intel/iommu.c >>> @@ -4248,8 +4248,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; >>> @@ -4257,11 +4257,6 @@ 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, 0); >>> - return; >>> - } >>> - >>> dmar_domain = to_dmar_domain(domain); >>> spin_lock_irqsave(&dmar_domain->lock, flags); >>> list_for_each_entry(curr, &dmar_domain->dev_pasids, link_domain) { >>> @@ -4278,13 +4273,24 @@ 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, >>> INTEL_PASID_TEARDOWN_DRAIN_PRQ); >>> + if (domain->type == IOMMU_DOMAIN_IDENTITY) >>> + return; >> >> The static identity domain is not capable of handling page requests. >> Therefore there is no need to drain PRQ for an identity domain removal. >> >> So it probably should be something like this: >> >> if (domain->type == IOMMU_DOMAIN_IDENTITY) { >> intel_pasid_tear_down_entry(iommu, dev, pasid, 0); >> return; >> } >> >> intel_pasid_tear_down_entry(iommu, dev, pasid, >> INTEL_PASID_TEARDOWN_DRAIN_PRQ); > > Just revisited this. It seems that we just need to drain PRQ if the > attached domain is iopf-capable. Therefore, how about making it like > this? > > unsigned int flags = 0; > > if (domain->iopf_handler) > flags |= INTEL_PASID_TEARDOWN_DRAIN_PRQ; > > intel_pasid_tear_down_entry(iommu, dev, pasid, flags); > > /* Identity domain has no meta data for pasid. */ > if (domain->type == IOMMU_DOMAIN_IDENTITY) > return; > got it.
On 2024/9/13 09:42, Baolu Lu wrote: > On 9/12/24 9:04 PM, Yi Liu wrote: >> @@ -4325,24 +4363,18 @@ 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_undo_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); >> + if (old) >> + 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_undo_dev_pasid: >> + domain_remove_dev_pasid(domain, dev, pasid); >> return ret; >> } > > Do you need to re-install the old domain to the pasid entry in the > failure path? yes, but no. The old domain is still installed in the pasid entry when the failure happened. :)
On 9/13/24 8:21 PM, Yi Liu wrote: > On 2024/9/13 09:42, Baolu Lu wrote: >> On 9/12/24 9:04 PM, Yi Liu wrote: >>> @@ -4325,24 +4363,18 @@ 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_undo_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); >>> + if (old) >>> + 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_undo_dev_pasid: >>> + domain_remove_dev_pasid(domain, dev, pasid); >>> return ret; >>> } >> >> Do you need to re-install the old domain to the pasid entry in the >> failure path? > > yes, but no. The old domain is still installed in the pasid entry > when the failure happened. :) I am afraid not. The old domain has already been cleaned up by intel_pasid_tear_down_entry(). Or not? Thanks, baolu
> On Sep 14, 2024, at 09:08, Baolu Lu <baolu.lu@linux.intel.com> wrote: > > On 9/13/24 8:21 PM, Yi Liu wrote: >>> On 2024/9/13 09:42, Baolu Lu wrote: >>> On 9/12/24 9:04 PM, Yi Liu wrote: >>>> @@ -4325,24 +4363,18 @@ 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_undo_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); >>>> + if (old) >>>> + 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_undo_dev_pasid: >>>> + domain_remove_dev_pasid(domain, dev, pasid); >>>> return ret; >>>> } >>> >>> Do you need to re-install the old domain to the pasid entry in the >>> failure path? >> yes, but no. The old domain is still installed in the pasid entry >> when the failure happened. :) > > I am afraid not. The old domain has already been cleaned up by > intel_pasid_tear_down_entry(). Or not? oops, yes. The tear down was lifted to this function now instead of in the setup helpers. I may move them back to the setup helpers just like v1 does. Good catch! Regards, Yi Liu