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

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

On 2024/3/13 11:13, Baolu Lu wrote:
> 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?

yes. how about your opinion? @Jason. I noticed the set_dev_pasid callback 
and pasid_array update is under the group->lock, so update it should be
fine to adjust the order to update pasid_array after set_dev_pasid returns.

-- 
Regards,
Yi Liu