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