diff mbox series

[v4,04/10] iommufd: Always pass iommu_attach_handle to iommu core

Message ID 20240912131255.13305-5-yi.l.liu@intel.com
State New
Headers show
Series iommufd support pasid attach/replace | expand

Commit Message

Yi Liu Sept. 12, 2024, 1:12 p.m. UTC 
The iommu_attach_handle is optional in the RID attach/replace API and the
PASID attach APIs. But it is a mandatory argument for the PASID replace API.
Without it, the PASID replace path cannot get the old domain. Hence, the
PASID path (attach/replace) requires the attach handle. As iommufd is the
major user of the RID attach/replace with iommu_attach_handle, this also
makes the iommufd always pass the attach handle for the RID path as well.
This keeps the RID and PASID path much aligned.

Signed-off-by: Yi Liu <yi.l.liu@intel.com>
---
 drivers/iommu/iommufd/fault.c           | 21 +++++++--------------
 drivers/iommu/iommufd/iommufd_private.h | 19 ++++++++++++++++---
 2 files changed, 23 insertions(+), 17 deletions(-)

Comments

Tian, Kevin Sept. 30, 2024, 7:45 a.m. UTC | #1 
> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Thursday, September 12, 2024 9:13 PM
> 
> The iommu_attach_handle is optional in the RID attach/replace API and the
> PASID attach APIs. But it is a mandatory argument for the PASID replace API.
> Without it, the PASID replace path cannot get the old domain. Hence, the
> PASID path (attach/replace) requires the attach handle. As iommufd is the
> major user of the RID attach/replace with iommu_attach_handle, this also
> makes the iommufd always pass the attach handle for the RID path as well.
> This keeps the RID and PASID path much aligned.
> 

hmm this looks more like a design choice instead of mandatory
requirement, e.g. as Baolu commented you can also pass in the
old domain. though I'm OK with what this patch does...
Yi Liu Sept. 30, 2024, 10:43 a.m. UTC | #2 
On 2024/9/30 15:45, Tian, Kevin wrote:
>> From: Liu, Yi L <yi.l.liu@intel.com>
>> Sent: Thursday, September 12, 2024 9:13 PM
>>
>> The iommu_attach_handle is optional in the RID attach/replace API and the
>> PASID attach APIs. But it is a mandatory argument for the PASID replace API.
>> Without it, the PASID replace path cannot get the old domain. Hence, the
>> PASID path (attach/replace) requires the attach handle. As iommufd is the
>> major user of the RID attach/replace with iommu_attach_handle, this also
>> makes the iommufd always pass the attach handle for the RID path as well.
>> This keeps the RID and PASID path much aligned.
>>
> 
> hmm this looks more like a design choice instead of mandatory
> requirement, e.g. as Baolu commented you can also pass in the
> old domain. though I'm OK with what this patch does...

yes, it is a choice. I think it was agreed in the below link.

https://lore.kernel.org/linux-iommu/20240816124707.GZ2032816@nvidia.com/
diff mbox series

Patch

diff --git a/drivers/iommu/iommufd/fault.c b/drivers/iommu/iommufd/fault.c
index 50ce6c6e61be..9fa56b3c2b7d 100644
--- a/drivers/iommu/iommufd/fault.c
+++ b/drivers/iommu/iommufd/fault.c
@@ -121,7 +121,7 @@  int iommufd_fault_domain_replace_dev(struct iommufd_device *idev,
 {
 	bool iopf_off = !hwpt->fault && old->fault;
 	bool iopf_on = hwpt->fault && !old->fault;
-	struct iommufd_attach_handle *curr = NULL;
+	struct iommufd_attach_handle *curr;
 	int ret;
 
 	if (iopf_on) {
@@ -130,24 +130,17 @@  int iommufd_fault_domain_replace_dev(struct iommufd_device *idev,
 			return ret;
 	}
 
-	if (hwpt->fault) {
-		curr = iommufd_dev_replace_handle(idev, hwpt, old);
-		ret = IS_ERR(curr) ? PTR_ERR(curr) : 0;
-	} else {
-		ret = iommu_replace_group_handle(idev->igroup->group,
-						 hwpt->domain, NULL);
-	}
-
-	if (ret) {
+	curr = iommufd_dev_replace_handle(idev, hwpt, old);
+	if (IS_ERR(curr)) {
 		if (iopf_on)
 			iommufd_fault_iopf_disable(idev);
-		return ret;
+		return PTR_ERR(curr);
 	}
 
-	if (curr) {
+	if (old->fault)
 		iommufd_auto_response_faults(old, curr);
-		kfree(curr);
-	}
+
+	kfree(curr);
 
 	if (iopf_off)
 		iommufd_fault_iopf_disable(idev);
diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h
index 7039c86d9981..30696936a926 100644
--- a/drivers/iommu/iommufd/iommufd_private.h
+++ b/drivers/iommu/iommufd/iommufd_private.h
@@ -499,6 +499,8 @@  iommufd_dev_replace_handle(struct iommufd_device *idev,
 	int ret;
 
 	curr = iommufd_device_get_attach_handle(idev);
+	if (!curr)
+		return ERR_PTR(-EINVAL);
 
 	handle = kzalloc(sizeof(*handle), GFP_KERNEL);
 	if (!handle)
@@ -541,28 +543,39 @@  static inline int iommufd_hwpt_attach_device(struct iommufd_hw_pagetable *hwpt,
 	if (hwpt->fault)
 		return iommufd_fault_domain_attach_dev(hwpt, idev);
 
-	return iommu_attach_group(hwpt->domain, idev->igroup->group);
+	return iommufd_dev_attach_handle(hwpt, idev);
 }
 
 static inline void iommufd_hwpt_detach_device(struct iommufd_hw_pagetable *hwpt,
 					      struct iommufd_device *idev)
 {
+	struct iommufd_attach_handle *handle;
+
 	if (hwpt->fault) {
 		iommufd_fault_domain_detach_dev(hwpt, idev);
 		return;
 	}
 
-	iommu_detach_group(hwpt->domain, idev->igroup->group);
+	handle = iommufd_device_get_attach_handle(idev);
+	iommu_detach_group_handle(hwpt->domain, idev->igroup->group);
+	kfree(handle);
 }
 
 static inline int iommufd_hwpt_replace_device(struct iommufd_device *idev,
 					      struct iommufd_hw_pagetable *hwpt,
 					      struct iommufd_hw_pagetable *old)
 {
+	struct iommufd_attach_handle *curr;
+
 	if (old->fault || hwpt->fault)
 		return iommufd_fault_domain_replace_dev(idev, hwpt, old);
 
-	return iommu_group_replace_domain(idev->igroup->group, hwpt->domain);
+	curr = iommufd_dev_replace_handle(idev, hwpt, old);
+	if (IS_ERR(curr))
+		return PTR_ERR(curr);
+
+	kfree(curr);
+	return 0;
 }
 
 #ifdef CONFIG_IOMMUFD_TEST