Messages in this thread | | | Date | Wed, 13 Mar 2024 16:11:41 +0800 | Subject | Re: [PATCH 1/8] iommu: Introduce a replace API for device pasid | From | Yi Liu <> |
| |
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
| |