On 2024/11/6 15:41, Tian, Kevin wrote:
From: Liu, Yi L <yi.l.liu@xxxxxxxxx>
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@xxxxxxxxx>
--
Regards,
Yi Liu
