> 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.
> @@ -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@xxxxxxxxx>
