Re: [PATCH v3 1/7] iommu: Introduce a replace API for device pasid

[Yi Liu <yi.l.liu@intel.com>]

On 2024/7/18 16:27, Tian, Kevin wrote:
>> From: Liu, Yi L <yi.l.liu@intel.com>
>> Sent: Friday, June 28, 2024 5:06 PM
>>
>> @@ -3289,7 +3290,20 @@ static int __iommu_set_group_pasid(struct
>> iommu_domain *domain,
>>
>>   		if (device == last_gdev)
>>   			break;
>> -		ops->remove_dev_pasid(device->dev, pasid, domain);
>> +		/* If no old domain, undo the succeeded devices/pasid */
>> +		if (!old) {
>> +			ops->remove_dev_pasid(device->dev, pasid, domain);
>> +			continue;
>> +		}
>> +
>> +		/*
>> +		 * Rollback the succeeded devices/pasid to the old domain.
>> +		 * And it is a driver bug to fail attaching with a previously
>> +		 * good domain.
>> +		 */
>> +		if (WARN_ON(old->ops->set_dev_pasid(old, device->dev,
>> +						    pasid, domain)))
>> +			ops->remove_dev_pasid(device->dev, pasid, domain);
> 
> I wonder whether @remove_dev_pasid() can be replaced by having
> blocking_domain support @set_dev_pasid?

how about your thought, @Jason?

>> +int iommu_replace_device_pasid(struct iommu_domain *domain,
>> +			       struct device *dev, ioasid_t pasid)
>> +{
>> +	/* Caller must be a probed driver on dev */
>> +	struct iommu_group *group = dev->iommu_group;
>> +	void *curr;
>> +	int ret;
>> +
>> +	if (!domain->ops->set_dev_pasid)
>> +		return -EOPNOTSUPP;
>> +
>> +	if (!group)
>> +		return -ENODEV;
>> +
>> +	if (!dev_has_iommu(dev) || dev_iommu_ops(dev) != domain-
>>> owner ||
>> +	    pasid == IOMMU_NO_PASID)
>> +		return -EINVAL;
>> +
>> +	mutex_lock(&group->mutex);
>> +	/*
>> +	 * The recorded domain is inconsistent with the domain pasid is
>> +	 * actually attached until pasid is attached to the new domain.
>> +	 * This has race condition with the paths that do not hold
>> +	 * group->mutex. E.g. the Page Request forwarding.
>> +	 */
> 
> so?

This is added per the below comment. Maybe I should have made it clearer.
Due to the order of this xa operations, the domain in the xarray does not
match the actual translation structure, but it will become consistent in
the end.

https://lore.kernel.org/linux-iommu/20240429135512.GC941030@nvidia.com/

>> +	curr = xa_store(&group->pasid_array, pasid, domain, GFP_KERNEL);
>> +	if (!curr) {
>> +		xa_erase(&group->pasid_array, pasid);
>> +		ret = -EINVAL;
>> +		goto out_unlock;
>> +	}
>> +
>> +	ret = xa_err(curr);
>> +	if (ret)
>> +		goto out_unlock;
>> +
>> +	if (curr == domain)
>> +		goto out_unlock;
>> +
>> +	ret = __iommu_set_group_pasid(domain, group, pasid, curr);
>> +	if (ret)
>> +		WARN_ON(domain != xa_store(&group->pasid_array, pasid,
>> +					   curr, GFP_KERNEL));
> 
> above can follow Jason's suggestion to iommu_group_replace_domain ()
> in Baolu's series, i.e. doing a xa_reserve() first.

yeah, I noticed it. But there is a minor difference. In Baolu's series
no need to retrieve the old domain, but this path needs to get it and
pass it to set_dev_pasid().

-- 
Regards,
Yi Liu