Re: [PATCH v7 02/13] iommu: Introduce a replace API for device pasid

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

On 2025/2/25 17:55, Tian, Kevin wrote:
>> From: Liu, Yi L <yi.l.liu@intel.com>
>> Sent: Sunday, February 16, 2025 11:52 AM
>>
>> -struct iommu_domain *iommu_group_domain(struct iommu_group *group)
>> +static struct iommu_domain *pasid_entry_to_domain(void *pasid_entry)
>>   {
>>   	struct iommu_domain *domain;
>> -	void *pasid_entry;
>> -
>> -	lockdep_assert_held(&group->mutex);
>>
>> -	pasid_entry = xa_load(&group->pasid_array, IOMMU_NO_PASID);
>>   	if (xa_pointer_tag(pasid_entry) == IOMMU_PASID_ARRAY_HANDLE) {
>>   		struct iommu_attach_handle *handle;
>>
> 
> What about calling it iommu_group_pasid_domain(group, pasid)
> which accepts a pasid as parameter so the caller doesn't need to
> do its own xa_load?

I would consider it in the new version. However, if the caller still
has the entry, I may still need this helper to avoid duplicated
xa_load().

>> @@ -3388,7 +3392,20 @@ static int __iommu_set_group_pasid(struct
>> iommu_domain *domain,
>>   	for_each_group_device(group, device) {
>>   		if (device == last_gdev)
>>   			break;
>> -		iommu_remove_dev_pasid(device->dev, pasid, domain);
>> +		/* If no old domain, undo the succeeded devices/pasid */
>> +		if (!old) {
>> +			iommu_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)))
>> +			iommu_remove_dev_pasid(device->dev, pasid,
>> domain);
> 
> Above can be simplified as:
> 
> 	if (!old || WARN_ON(old...))
> 		iommu_remove_dev_pasid();

yes.

> 
>> +/**
>> + * iommu_replace_device_pasid_handle - Replace the domain that a pasid
>> + *                                     is attached to
>> + * @domain: the new iommu domain
>> + * @dev: the attached device.
>> + * @pasid: the pasid of the device.
>> + * @handle: the attach handle.
>> + *
>> + * This API allows the pasid to switch domains. The @pasid should have
>> been
>> + * attached via iommu_replace_device_pasid_handle(), otherwise, this
> 
> why do we care how the pasid has been attached? and in reality people
> does attach, replace, replace, etc, i.e. you cannot expect the 1st operation
> as a replace.

it's a typo. :) It should have been iommu_attach_device_pasid_handle().
However, this comment should be dropped anyhow since we are going to
support replacing domain with handle per the handle series. I'll have
it in a refreshed version of this series.

The replace path is going to have something like the below. If both domain
and handle is the same, it shall return directly.

	pasid_entry = iommu_make_pasid_entry(new_domain, handle);
	curr = xa_cmpxchg(&group->pasid_array, IOMMU_NO_PASID, NULL,
			  XA_ZERO_ENTRY, GFP_KERNEL);
	if (xa_is_err(curr))
		return xa_err(curr);
	if (curr == pasid_entry &&
	    new_domain == pasid_entry_to_domain(curr))
		return 0;


[1] https://lore.kernel.org/linux-iommu/20250218195756.GG4183890@nvidia.com/

-- 
Regards,
Yi Liu