| Message ID | 20231127063428.127436-9-yi.l.liu@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iommufd support pasid attach/replace | expand |
On 11/27/2023 2:34 PM, Yi Liu wrote: > From: Lu Baolu <baolu.lu@linux.intel.com> > > This allows the upper layers to set a nested type domain to a PASID of a > device if the PASID feature is supported by the IOMMU hardware. > > The set_dev_pasid callback for non-nest domain has already be there, so > this only needs to add it for nested domains. > > Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com> > Signed-off-by: Yi Liu <yi.l.liu@intel.com> > --- > drivers/iommu/intel/nested.c | 47 ++++++++++++++++++++++++++++++++++++ > 1 file changed, 47 insertions(+) > > diff --git a/drivers/iommu/intel/nested.c b/drivers/iommu/intel/nested.c > index 44ad48db7ea0..f6f687750104 100644 > --- a/drivers/iommu/intel/nested.c > +++ b/drivers/iommu/intel/nested.c > @@ -68,6 +68,52 @@ static int intel_nested_attach_dev(struct iommu_domain *domain, > return 0; > } > > +static int intel_nested_set_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 = 0; > + > + if (!pasid_supported(iommu)) > + return -EOPNOTSUPP; > + > + if (iommu->agaw < dmar_domain->s2_domain->agaw) > + return -EINVAL; > + > + ret = prepare_domain_attach_device(&dmar_domain->s2_domain->domain, dev); > + 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 err_free; > + > + ret = intel_pasid_setup_nested(iommu, dev, pasid, dmar_domain); > + if (ret) > + goto err_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); ---> list_add(&dev_pasid->link_domain, &dmar_domain->dev_pasids); dev_pasid is linked at later time, this leads to domain->has_iotlb_device is not correctly set, which finally results into a missing of device iotlb flush in iommu_flush_dev_iotlb()when it's called. Check this call path: domain_attach_iommu()->domain_update_iommu_cap()->domain_update_iotlb()->domain->has_iotlb_device = has_iotlb_device; The ugly fixup is to call domain_update_iommu_cap() or domain_update_iotlb() here again before return. The similar issue is in intel_iommu_set_dev_pasid() and intel_nested_attach_dev(). > + > + return 0; > +err_detach_iommu: > + domain_detach_iommu(dmar_domain, iommu); > +err_free: > + kfree(dev_pasid); > + return ret; > +} > + > static void intel_nested_domain_free(struct iommu_domain *domain) > { > kfree(to_dmar_domain(domain)); > @@ -128,6 +174,7 @@ static int intel_nested_cache_invalidate_user(struct iommu_domain *domain, > > static const struct iommu_domain_ops intel_nested_domain_ops = { > .attach_dev = intel_nested_attach_dev, > + .set_dev_pasid = intel_nested_set_dev_pasid, > .free = intel_nested_domain_free, > .cache_invalidate_user = intel_nested_cache_invalidate_user, > };
On 2023/12/14 10:55, Yang, Weijiang wrote: > On 11/27/2023 2:34 PM, Yi Liu wrote: >> From: Lu Baolu <baolu.lu@linux.intel.com> >> >> This allows the upper layers to set a nested type domain to a PASID of a >> device if the PASID feature is supported by the IOMMU hardware. >> >> The set_dev_pasid callback for non-nest domain has already be there, so >> this only needs to add it for nested domains. >> >> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com> >> Signed-off-by: Yi Liu <yi.l.liu@intel.com> >> --- >> drivers/iommu/intel/nested.c | 47 ++++++++++++++++++++++++++++++++++++ >> 1 file changed, 47 insertions(+) >> >> diff --git a/drivers/iommu/intel/nested.c b/drivers/iommu/intel/nested.c >> index 44ad48db7ea0..f6f687750104 100644 >> --- a/drivers/iommu/intel/nested.c >> +++ b/drivers/iommu/intel/nested.c >> @@ -68,6 +68,52 @@ static int intel_nested_attach_dev(struct >> iommu_domain *domain, >> return 0; >> } >> +static int intel_nested_set_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 = 0; >> + >> + if (!pasid_supported(iommu)) >> + return -EOPNOTSUPP; >> + >> + if (iommu->agaw < dmar_domain->s2_domain->agaw) >> + return -EINVAL; >> + >> + ret = >> prepare_domain_attach_device(&dmar_domain->s2_domain->domain, dev); >> + 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 err_free; >> + >> + ret = intel_pasid_setup_nested(iommu, dev, pasid, dmar_domain); >> + if (ret) >> + goto err_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); > > ---> list_add(&dev_pasid->link_domain, &dmar_domain->dev_pasids); > > dev_pasid is linked at later time, this leads to > domain->has_iotlb_device is not correctly set, which finally results > into a missing of device iotlb flush in iommu_flush_dev_iotlb()when it's > called. > Check this call path: > domain_attach_iommu()->domain_update_iommu_cap()->domain_update_iotlb()->domain->has_iotlb_device = has_iotlb_device; The ugly fixup is to call domain_update_iommu_cap() or domain_update_iotlb() here again before return. > The similar issue is in intel_iommu_set_dev_pasid() and > intel_nested_attach_dev(). Yes, domain->has_iotlb_device must be updated whenever a domain is attached to (or removed from) a RID or PASID. I would be grateful if you could post some patches to fix the set_device_pasid and nested_attach_dev paths. I assume Yi can fix this series in the next version. Best regards, baolu
On 12/14/2023 9:33 PM, Baolu Lu wrote: > On 2023/12/14 10:55, Yang, Weijiang wrote: >> On 11/27/2023 2:34 PM, Yi Liu wrote: >>> From: Lu Baolu <baolu.lu@linux.intel.com> >>> >>> This allows the upper layers to set a nested type domain to a PASID of a >>> device if the PASID feature is supported by the IOMMU hardware. >>> >>> The set_dev_pasid callback for non-nest domain has already be there, so >>> this only needs to add it for nested domains. >>> >>> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com> >>> Signed-off-by: Yi Liu <yi.l.liu@intel.com> >>> --- >>> drivers/iommu/intel/nested.c | 47 ++++++++++++++++++++++++++++++++++++ >>> 1 file changed, 47 insertions(+) >>> >>> diff --git a/drivers/iommu/intel/nested.c b/drivers/iommu/intel/nested.c >>> index 44ad48db7ea0..f6f687750104 100644 >>> --- a/drivers/iommu/intel/nested.c >>> +++ b/drivers/iommu/intel/nested.c >>> @@ -68,6 +68,52 @@ static int intel_nested_attach_dev(struct iommu_domain *domain, >>> return 0; >>> } >>> +static int intel_nested_set_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 = 0; >>> + >>> + if (!pasid_supported(iommu)) >>> + return -EOPNOTSUPP; >>> + >>> + if (iommu->agaw < dmar_domain->s2_domain->agaw) >>> + return -EINVAL; >>> + >>> + ret = prepare_domain_attach_device(&dmar_domain->s2_domain->domain, dev); >>> + 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 err_free; >>> + >>> + ret = intel_pasid_setup_nested(iommu, dev, pasid, dmar_domain); >>> + if (ret) >>> + goto err_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); >> >> ---> list_add(&dev_pasid->link_domain, &dmar_domain->dev_pasids); >> >> dev_pasid is linked at later time, this leads to domain->has_iotlb_device is not correctly set, which finally results into a missing of device iotlb flush in iommu_flush_dev_iotlb()when it's called. >> Check this call path: >> domain_attach_iommu()->domain_update_iommu_cap()->domain_update_iotlb()->domain->has_iotlb_device = has_iotlb_device; The ugly fixup is to call domain_update_iommu_cap() or domain_update_iotlb() here again before return. >> The similar issue is in intel_iommu_set_dev_pasid() and intel_nested_attach_dev(). > > Yes, domain->has_iotlb_device must be updated whenever a domain is > attached to (or removed from) a RID or PASID. I would be grateful if you > could post some patches to fix the set_device_pasid and > nested_attach_dev paths. Sure, I'll post fixup patches for the paths, thanks for confirmation! > > I assume Yi can fix this series in the next version. > > Best regards, > baolu
On Sun, Nov 26, 2023 at 10:34:28PM -0800, Yi Liu wrote: > +static int intel_nested_set_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 = 0; > + > + if (!pasid_supported(iommu)) > + return -EOPNOTSUPP; > + > + if (iommu->agaw < dmar_domain->s2_domain->agaw) > + return -EINVAL; > + > + ret = prepare_domain_attach_device(&dmar_domain->s2_domain->domain, dev); > + 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 err_free; > + > + ret = intel_pasid_setup_nested(iommu, dev, pasid, dmar_domain); > + if (ret) > + goto err_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 0; > +err_detach_iommu: > + domain_detach_iommu(dmar_domain, iommu); > +err_free: > + kfree(dev_pasid); > + return ret; > +} This seems alot longer than I'd think it should be, why isn't it exactly the same code as the other set_dev_pasid's? Jason
On 2024/1/16 1:22, Jason Gunthorpe wrote: > On Sun, Nov 26, 2023 at 10:34:28PM -0800, Yi Liu wrote: > >> +static int intel_nested_set_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 = 0; >> + >> + if (!pasid_supported(iommu)) >> + return -EOPNOTSUPP; >> + >> + if (iommu->agaw < dmar_domain->s2_domain->agaw) >> + return -EINVAL; >> + >> + ret = prepare_domain_attach_device(&dmar_domain->s2_domain->domain, dev); >> + 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 err_free; >> + >> + ret = intel_pasid_setup_nested(iommu, dev, pasid, dmar_domain); >> + if (ret) >> + goto err_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 0; >> +err_detach_iommu: >> + domain_detach_iommu(dmar_domain, iommu); >> +err_free: >> + kfree(dev_pasid); >> + return ret; >> +} > This seems alot longer than I'd think it should be, why isn't it > exactly the same code as the other set_dev_pasid's? Yes. It should be. The only difference is how to configure the pasid entry. Best regards, baolu
diff --git a/drivers/iommu/intel/nested.c b/drivers/iommu/intel/nested.c index 44ad48db7ea0..f6f687750104 100644 --- a/drivers/iommu/intel/nested.c +++ b/drivers/iommu/intel/nested.c @@ -68,6 +68,52 @@ static int intel_nested_attach_dev(struct iommu_domain *domain, return 0; } +static int intel_nested_set_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 = 0; + + if (!pasid_supported(iommu)) + return -EOPNOTSUPP; + + if (iommu->agaw < dmar_domain->s2_domain->agaw) + return -EINVAL; + + ret = prepare_domain_attach_device(&dmar_domain->s2_domain->domain, dev); + 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 err_free; + + ret = intel_pasid_setup_nested(iommu, dev, pasid, dmar_domain); + if (ret) + goto err_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 0; +err_detach_iommu: + domain_detach_iommu(dmar_domain, iommu); +err_free: + kfree(dev_pasid); + return ret; +} + static void intel_nested_domain_free(struct iommu_domain *domain) { kfree(to_dmar_domain(domain)); @@ -128,6 +174,7 @@ static int intel_nested_cache_invalidate_user(struct iommu_domain *domain, static const struct iommu_domain_ops intel_nested_domain_ops = { .attach_dev = intel_nested_attach_dev, + .set_dev_pasid = intel_nested_set_dev_pasid, .free = intel_nested_domain_free, .cache_invalidate_user = intel_nested_cache_invalidate_user, };