Re: [PATCH 5/6] iommu: init pasid array while doing domain_replace and iopf is active

[Joel Granados]

On Thu, Sep 12, 2024 at 12:21:53PM +0800, Baolu Lu wrote:
> On 9/11/24 6:56 PM, Joel Granados wrote:
> > On Thu, Sep 05, 2024 at 11:30:05AM +0800, Baolu Lu wrote:
> >> On 9/4/24 9:17 PM, Joel Granados via B4 Relay wrote:
> >>> From: Joel Granados<j.granados@xxxxxxxxxxx>
> >>>
> >>> iommu_report_device_fault expects a pasid array to have an
> >>> iommu_attach_handle when a fault is detected.
> >> The iommu_attach_handle is expected only when an iopf-capable domain is
> >> attached to the device or PASID. The iommu_report_device_fault() treats
> >> it as a fault when a fault occurs, but no iopf-capable domain is
> >> attached.
> >>
> >>> Add this handle when the
> >>> replacing hwpt has a valid iommufd fault object. Remove it when we
> >>> release ownership of the group.
> >> The iommu_attach_handle is managed by the caller (iommufd here for
> >> example). Therefore, before iommu_attach_handle tries to attach a domain
> >> to an iopf-capable device or pasid, it should allocate the handle and
> >> pass it to the domain attachment interfaces.
> > What do you mean here?
> > 1. Do you want to move the iommufd_init_pasid_array call up to
> > iommufd_hwpt_replace_device?
> > 2. Do you want to move it further up to iommufd_device_do_replace?
> 
> I'm having trouble understanding the need for iommu_init_pasid_array()
> in the core.

The iommu_init_pasid_array function is there because I needed to access
struct iommu_group->pasid_array. The struct declaration for iommu_group
is in the core (drivers/iommu/iommu.c), so I thought that the function
to modify a member of that struct would go in core also.

To move all the logic into iommufd_init_pasid_array in iommufd, we would
need to move the iommu_group struct (include/linux/iommu.h?). Here is a
proposed patch that would make that happen (and removes
iommu_init_pasid_array). What do you think?


diff --git i/drivers/iommu/iommu-priv.h w/drivers/iommu/iommu-priv.h
index 493e501badc7..de5b54eaa8bf 100644
--- i/drivers/iommu/iommu-priv.h
+++ w/drivers/iommu/iommu-priv.h
@@ -38,9 +38,6 @@ void iommu_device_unregister_bus(struct iommu_device *iommu,
 struct iommu_attach_handle *iommu_attach_handle_get(struct iommu_group *group,
 						    ioasid_t pasid,
 						    unsigned int type);
-int iommu_init_pasid_array(struct iommu_domain *domain,
-			   struct iommu_group *group,
-			   struct iommu_attach_handle *handle);
 int iommu_attach_group_handle(struct iommu_domain *domain,
 			      struct iommu_group *group,
 			      struct iommu_attach_handle *handle);
diff --git i/drivers/iommu/iommu.c w/drivers/iommu/iommu.c
index 64ae1cf571aa..83bc0bfd7bb0 100644
--- i/drivers/iommu/iommu.c
+++ w/drivers/iommu/iommu.c
@@ -44,24 +44,6 @@ static unsigned int iommu_def_domain_type __read_mostly;
 static bool iommu_dma_strict __read_mostly = IS_ENABLED(CONFIG_IOMMU_DEFAULT_DMA_STRICT);
 static u32 iommu_cmd_line __read_mostly;
 
-struct iommu_group {
-	struct kobject kobj;
-	struct kobject *devices_kobj;
-	struct list_head devices;
-	struct xarray pasid_array;
-	struct mutex mutex;
-	void *iommu_data;
-	void (*iommu_data_release)(void *iommu_data);
-	char *name;
-	int id;
-	struct iommu_domain *default_domain;
-	struct iommu_domain *blocking_domain;
-	struct iommu_domain *domain;
-	struct list_head entry;
-	unsigned int owner_cnt;
-	void *owner;
-};
-
 struct group_device {
 	struct list_head list;
 	struct device *dev;
@@ -3498,33 +3480,6 @@ iommu_attach_handle_get(struct iommu_group *group, ioasid_t pasid, unsigned int
 }
 EXPORT_SYMBOL_NS_GPL(iommu_attach_handle_get, IOMMUFD_INTERNAL);
 
-/**
- * iommu_init_pasid_array - Initialize pasid array in the domain group
- *
- * Returns 0 on success. Error code on failure
- *
- * An IOMMU_NO_PASID element is *NOT* replaced if there is one already there.
- */
-int iommu_init_pasid_array(struct iommu_domain *domain,
-			   struct iommu_group *group,
-			   struct iommu_attach_handle *handle)
-{
-	int ret;
-
-	if (handle)
-		handle->domain = domain;
-
-	mutex_lock(&group->mutex);
-	ret = xa_insert(&group->pasid_array, IOMMU_NO_PASID, handle, GFP_KERNEL);
-	mutex_unlock(&group->mutex);
-
-	if (ret == -EBUSY)
-		ret = 0;
-
-	return ret;
-}
-EXPORT_SYMBOL_NS_GPL(iommu_init_pasid_array, IOMMUFD_INTERNAL);
-
 /**
  * iommu_attach_group_handle - Attach an IOMMU domain to an IOMMU group
  * @domain: IOMMU domain to attach
diff --git i/drivers/iommu/iommufd/fault.c w/drivers/iommu/iommufd/fault.c
index ea7f1bf64892..51cb70465b87 100644
--- i/drivers/iommu/iommufd/fault.c
+++ w/drivers/iommu/iommufd/fault.c
@@ -189,8 +189,15 @@ static int iommufd_init_pasid_array(struct iommu_domain *domain,
 	if (!handle)
 		return -ENOMEM;
 	handle->idev = idev;
+	handle->handle.domain = domain;
+
+	mutex_lock(&group->mutex);
+	ret = xa_insert(&group->pasid_array, IOMMU_NO_PASID, handle, GFP_KERNEL);
+	mutex_unlock(&group->mutex);
+
+	if (ret == -EBUSY)
+		ret = 0;
 
-	ret = iommu_init_pasid_array(domain, group, &handle->handle);
 	if (ret)
 		kfree(handle);
 
diff --git i/include/linux/iommu.h w/include/linux/iommu.h
index bd722f473635..c34aa737aeb2 100644
--- i/include/linux/iommu.h
+++ w/include/linux/iommu.h
@@ -31,8 +31,25 @@
  */
 #define IOMMU_PRIV	(1 << 5)
 
+struct iommu_group {
+	struct kobject kobj;
+	struct kobject *devices_kobj;
+	struct list_head devices;
+	struct xarray pasid_array;
+	struct mutex mutex;
+	void *iommu_data;
+	void (*iommu_data_release)(void *iommu_data);
+	char *name;
+	int id;
+	struct iommu_domain *default_domain;
+	struct iommu_domain *blocking_domain;
+	struct iommu_domain *domain;
+	struct list_head entry;
+	unsigned int owner_cnt;
+	void *owner;
+};
+
 struct iommu_ops;
-struct iommu_group;
 struct bus_type;
 struct device;
 struct iommu_domain;



-- 

Joel Granados