Re: [PATCH v4 01/10] iommu: Introduce a replace API for device pasid

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

On 2024/9/30 15:38, Tian, Kevin wrote:
>> From: Liu, Yi L <yi.l.liu@intel.com>
>> Sent: Thursday, September 12, 2024 9:13 PM
>>
>> +/**
>> + * iommu_replace_device_pasid - 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. Return 0 on success, or an
>> + * error. The pasid will keep the old configuration if replacement failed.
>> + * This is supposed to be used by iommufd, and iommufd can guarantee
>> that
>> + * both iommu_attach_device_pasid() and iommu_replace_device_pasid()
>> would
>> + * pass in a valid @handle.
> 
> this function assumes handle is always valid. So above comment
> makes it clear that iommufd is the only user and it will always
> pass in a valid handle.
> 
> but the code in iommu_attach_device_pasid() allows handle to
> be NULL. Then that comment is meaningless for it.

Actually, this is why I added the above comment. iommufd can ensure
it would pass valid handle to both iommu_attach_device_pasid() and
iommu_replace_device_pasid(), and iommu_replace_device_pasid() is
only used by iommufd, so iommu_replace_device_pasid() can assume
all the pasids have a valid handle stored in the pasid_array.

> 
> Also following patches are built on iommufd always passing in
> a valid handle as it's required by pasid operations but there is
> no detail explanation why it's mandatory or any alternative
> option exists. More explanation is welcomed.

There is more detail about it in the below link, but is it necessary
to add them in the comment as well, or is it ok to add more explanation
in commit message?

https://lore.kernel.org/linux-iommu/0bf383b7-ed96-49ca-b1da-d1fff48e161a@intel.com/

>> + */
>> +int iommu_replace_device_pasid(struct iommu_domain *domain,
>> +			       struct device *dev, ioasid_t pasid,
>> +			       struct iommu_attach_handle *handle)
>> +{
>> +	/* Caller must be a probed driver on dev */
>> +	struct iommu_group *group = dev->iommu_group;
>> +	struct iommu_attach_handle *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 || !handle)
>> +		return -EINVAL;
>> +
>> +	handle->domain = domain;
>> +
>> +	mutex_lock(&group->mutex);
>> +	/*
>> +	 * The iommu_attach_handle of the pasid becomes inconsistent with
>> the
>> +	 * actual handle per the below operation. The concurrent PRI path
>> will
>> +	 * deliver the PRQs per the new handle, this does not have a function
>> +	 * impact. The PRI path would eventually become consistent when
> 
> s/function/functional/

got it.

>> the
>> +	 * replacement is done.
>> +	 */
>> +	curr = (struct iommu_attach_handle *)xa_store(&group->pasid_array,
>> +						      pasid, handle,
>> +						      GFP_KERNEL);
> 
> Could you elaborate why the PRI path will eventually becomes
> consistent with this path?

Because the handle stored in pasid_array would be consistent with the
configuration of pasid. So the PRI would be forwarded to the correct
domain.

-- 
Regards,
Yi Liu