| Message ID | 20240912130427.10119-4-yi.l.liu@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Make set_dev_pasid op supporting domain replacement | expand |
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); > + domain_remove_dev_pasid(domain, dev, pasid); > } > > -static int intel_iommu_set_dev_pasid(struct iommu_domain *domain, > - struct device *dev, ioasid_t pasid, > - struct iommu_domain *old) > +static struct dev_pasid_info * > +domain_prepare_dev_pasid(struct iommu_domain *domain, > + struct device *dev, ioasid_t pasid) Why do you want to return a struct pointer instead of an integer? The returned pointer is not used after it is returned. Also, how about renaming this helper to domain_add_dev_pasid() to pair it with domain_remove_dev_pasid()? > { > struct device_domain_info *info = dev_iommu_priv_get(dev); > struct dmar_domain *dmar_domain = to_dmar_domain(domain); > @@ -4293,22 +4299,13 @@ static int intel_iommu_set_dev_pasid(struct iommu_domain *domain, > unsigned long flags; > int ret; > > - if (!pasid_supported(iommu) || dev_is_real_dma_subdevice(dev)) > - return -EOPNOTSUPP; > - > - if (domain->dirty_ops) > - return -EINVAL; > - > - if (context_copied(iommu, info->bus, info->devfn)) > - return -EBUSY; > - > ret = prepare_domain_attach_device(domain, dev); > if (ret) > - return ret; > + return ERR_PTR(ret); > > dev_pasid = kzalloc(sizeof(*dev_pasid), GFP_KERNEL); > if (!dev_pasid) > - return -ENOMEM; > + return ERR_PTR(-ENOMEM); > > ret = domain_attach_iommu(dmar_domain, iommu); > if (ret) Thanks, baolu
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 09:35, 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. oh, yes. so maybe for SI domain, there is no PRQ at all. > 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); > >> + domain_remove_dev_pasid(domain, dev, pasid); >> } >> -static int intel_iommu_set_dev_pasid(struct iommu_domain *domain, >> - struct device *dev, ioasid_t pasid, >> - struct iommu_domain *old) >> +static struct dev_pasid_info * >> +domain_prepare_dev_pasid(struct iommu_domain *domain, >> + struct device *dev, ioasid_t pasid) > > Why do you want to return a struct pointer instead of an integer? The > returned pointer is not used after it is returned. it's for the intel_iommu_debugfs_create_dev_pasid(). it needs the dev_pasid. > Also, how about renaming this helper to domain_add_dev_pasid() to pair > it with domain_remove_dev_pasid()? sure. >> { >> struct device_domain_info *info = dev_iommu_priv_get(dev); >> struct dmar_domain *dmar_domain = to_dmar_domain(domain); >> @@ -4293,22 +4299,13 @@ static int intel_iommu_set_dev_pasid(struct >> iommu_domain *domain, >> unsigned long flags; >> int ret; >> - if (!pasid_supported(iommu) || dev_is_real_dma_subdevice(dev)) >> - return -EOPNOTSUPP; >> - >> - if (domain->dirty_ops) >> - return -EINVAL; >> - >> - if (context_copied(iommu, info->bus, info->devfn)) >> - return -EBUSY; >> - >> ret = prepare_domain_attach_device(domain, dev); >> if (ret) >> - return ret; >> + return ERR_PTR(ret); >> dev_pasid = kzalloc(sizeof(*dev_pasid), GFP_KERNEL); >> if (!dev_pasid) >> - return -ENOMEM; >> + return ERR_PTR(-ENOMEM); >> ret = domain_attach_iommu(dmar_domain, iommu); >> if (ret) > > 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
> From: Baolu Lu <baolu.lu@linux.intel.com> > Sent: Friday, September 13, 2024 10:17 AM > > On 9/13/24 9:35 AM, Baolu Lu wrote: > > On 9/12/24 9:04 PM, Yi Liu wrote: > >> +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; > this is the right thing to do, but also suggesting a bug in existing code. intel_pasid_tear_down_entry() is not just for PRQ drain. It's also about iotlb/devtlb invalidation. From device p.o.v it has no idea about the translation mode in the IOMMU side and always caches the valid mappings in devtlb when ATS is enabled. Existing code skips all those housekeeping for identify domain by early return before intel_pasid_tear_down_entry(). We need a separate fix for it before this series?
On 2024/9/30 15:19, Tian, Kevin wrote: >> From: Baolu Lu <baolu.lu@linux.intel.com> >> Sent: Friday, September 13, 2024 10:17 AM >> >> On 9/13/24 9:35 AM, Baolu Lu wrote: >>> On 9/12/24 9:04 PM, Yi Liu wrote: >>>> +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; >> > > this is the right thing to do, but also suggesting a bug in existing > code. intel_pasid_tear_down_entry() is not just for PRQ drain. > It's also about iotlb/devtlb invalidation. From device p.o.v it > has no idea about the translation mode in the IOMMU side and > always caches the valid mappings in devtlb when ATS is enabled. Yes. You are right. intel_pasid_tear_down_entry() takes care of iotlb/devtlb invalidation. So it's fine as long as intel_pasid_tear_down_entry() is called for the IDENTITY domain path, right? > Existing code skips all those housekeeping for identify domain > by early return before intel_pasid_tear_down_entry(). We need > a separate fix for it before this series? Existing code doesn't skip intel_pasid_tear_down_entry(). if (domain->type == IOMMU_DOMAIN_IDENTITY) { intel_pasid_tear_down_entry(iommu, dev, pasid, false); return; } Or anything I overlooked? Thanks, baolu
> From: Baolu Lu <baolu.lu@linux.intel.com> > Sent: Wednesday, October 9, 2024 9:09 AM > > On 2024/9/30 15:19, Tian, Kevin wrote: > >> From: Baolu Lu <baolu.lu@linux.intel.com> > >> Sent: Friday, September 13, 2024 10:17 AM > >> > >> On 9/13/24 9:35 AM, Baolu Lu wrote: > >>> On 9/12/24 9:04 PM, Yi Liu wrote: > >>>> +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; > >> > > > > this is the right thing to do, but also suggesting a bug in existing > > code. intel_pasid_tear_down_entry() is not just for PRQ drain. > > It's also about iotlb/devtlb invalidation. From device p.o.v it > > has no idea about the translation mode in the IOMMU side and > > always caches the valid mappings in devtlb when ATS is enabled. > > Yes. You are right. > > intel_pasid_tear_down_entry() takes care of iotlb/devtlb invalidation. > So it's fine as long as intel_pasid_tear_down_entry() is called for the > IDENTITY domain path, right? > > > Existing code skips all those housekeeping for identify domain > > by early return before intel_pasid_tear_down_entry(). We need > > a separate fix for it before this series? > > Existing code doesn't skip intel_pasid_tear_down_entry(). > > if (domain->type == IOMMU_DOMAIN_IDENTITY) { > intel_pasid_tear_down_entry(iommu, dev, pasid, false); > return; > } > > Or anything I overlooked? No. Looks I was confused by this patch and misunderstood the existing code.
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; + domain_remove_dev_pasid(domain, dev, pasid); } -static int intel_iommu_set_dev_pasid(struct iommu_domain *domain, - struct device *dev, ioasid_t pasid, - struct iommu_domain *old) +static struct dev_pasid_info * +domain_prepare_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); @@ -4293,22 +4299,13 @@ static int intel_iommu_set_dev_pasid(struct iommu_domain *domain, unsigned long flags; int ret; - if (!pasid_supported(iommu) || dev_is_real_dma_subdevice(dev)) - return -EOPNOTSUPP; - - if (domain->dirty_ops) - return -EINVAL; - - if (context_copied(iommu, info->bus, info->devfn)) - return -EBUSY; - ret = prepare_domain_attach_device(domain, dev); if (ret) - return ret; + return ERR_PTR(ret); dev_pasid = kzalloc(sizeof(*dev_pasid), GFP_KERNEL); if (!dev_pasid) - return -ENOMEM; + return ERR_PTR(-ENOMEM); ret = domain_attach_iommu(dmar_domain, iommu); if (ret) @@ -4318,6 +4315,47 @@ static int intel_iommu_set_dev_pasid(struct iommu_domain *domain, 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, + struct device *dev, ioasid_t pasid, + struct iommu_domain *old) +{ + 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; + int ret; + + if (!pasid_supported(iommu) || dev_is_real_dma_subdevice(dev)) + return -EOPNOTSUPP; + + if (domain->dirty_ops) + return -EINVAL; + + if (context_copied(iommu, info->bus, info->devfn)) + return -EBUSY; + + dev_pasid = domain_prepare_dev_pasid(domain, dev, pasid); + if (IS_ERR(dev_pasid)) + return PTR_ERR(dev_pasid); + + /* Clear the old configuration if it already exists */ + intel_pasid_tear_down_entry(iommu, dev, pasid, + INTEL_PASID_TEARDOWN_DRAIN_PRQ); + if (dmar_domain->use_first_level) ret = domain_setup_first_level(iommu, dmar_domain, dev, pasid); @@ -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; }
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(-)