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

[Baolu Lu <baolu.lu@linux.intel.com>]

On 2024/3/12 11:07, Yi Liu wrote:
> On 2024/3/11 17:26, Tian, Kevin wrote:
>>> From: Liu, Yi L <yi.l.liu@intel.com>
>>> Sent: Sunday, March 10, 2024 9:06 PM
>>>
>>> On 2024/1/16 01:19, Jason Gunthorpe wrote:
>>>> On Sun, Nov 26, 2023 at 10:34:21PM -0800, Yi Liu wrote:
>>>>> +int iommu_replace_device_pasid(struct iommu_domain *domain,
>>>>> +                   struct device *dev, ioasid_t pasid)
>>>>> +{
>>>>> +    struct iommu_group *group = dev->iommu_group;
>>>>> +    struct iommu_domain *old_domain;
>>>>> +    int ret;
>>>>> +
>>>>> +    if (!domain)
>>>>> +        return -EINVAL;
>>>>> +
>>>>> +    if (!group)
>>>>> +        return -ENODEV;
>>>>> +
>>>>> +    mutex_lock(&group->mutex);
>>>>> +    __iommu_remove_group_pasid(group, pasid);
>>>>
>>>> It is not replace if you do remove first.
>>>>
>>>> Replace must just call set_dev_pasid and nothing much else..
>>>
>>> Seems uneasy to do it so far. The VT-d driver needs to get the old 
>>> domain
>>> first in order to do replacement. However, VT-d driver does not track 
>>> the
>>> attached domains of pasids. It gets domain of a pasid
>>> by iommu_get_domain_for_dev_pasid(). Like
>>> intel_iommu_remove_dev_pasid)
>>> in link [1]. While the iommu layer exchanges the domain in the
>>> group->pasid_array before calling into VT-d driver. So when calling into
>>> VT-d driver, the domain got by iommu_get_domain_for_dev_pasid() is
>>> already
>>> the new domain. To solve it, we may need to let VT-d driver have its
>>> own tracking on the domains. How about your thoughts? @Jason, @Kevin,
>>> @Baoplu?
>>>
>>> [1]
>>> https://github.com/torvalds/linux/blob/master/drivers/iommu/intel/iommu
>>> .c#L4621C19-L4621C20
>>>
>>
>> Jason's point was that the core helper should directly call set_dev_pasid
>> and underlying iommu driver decides whether atomic replacement
>> can be implemented inside the callback. If you first remove in the core
>> then one can never implement a replace semantics.
> 
> yeah, I got Jason's point. I'm raising an open to make the set_dev_pasid
> callback to handle domain replacement. The existing intel iommu driver
> does not track attached domains for pasid. But it's needed to know the
> attached domain of a pasid and find the dev_pasid under the domain, hence
> be able to clean up the old attachment and do the new attachment. Existing
> code cannot work as I mentioned above. The group->pasid_xarray is updated
> before calling set_dev_pasid callback. This means the underlying iommu
> driver cannot get the old domain in the set_dev_pasid callback by the
> iommu_get_domain_for_dev_pasid() helper.
> 
> As above, I'm considering the possibility to track attached domains for
> pasid by an xarray in the struct device_domain_info. Hence, intel iommu
> driver could store/retrieve attached domain for pasids. If it's
> replacement, the set_dev_pasid callback could find the attached domain and
> the dev_pasid instance in the domain. Then be able to do detach and clean
> up the tracking structures (e.g. dev_pasid).

Maintaining the same data structure in both core and individual iommu
drivers seems to be duplicate effort. It appears that what you want here
is just to get the currently used domain in the set_dev_pasid path. Is
it possible to adjust the core code so that the pasid array is updated
after the driver's set_dev_pasid callback returns success?

Best regards,
baolu