Re: [PATCH v4 05/13] iommu/vt-d: Prepare intel_iommu_set_dev_pasid() handle replacement

Yi Liu
2024-11-06T07:58:09Z

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

--
Regards,
Yi Liu