iommu aux-domain APIs extensions

In the vfio/mdev use case of aux-domain, the subdevices are created from the physical devices with IOMMU_DEV_FEAT_AUX enabled and the aux-domains are attached to the subdevices through the iommu_ops.aux_attach_dev() interface.

Current iommu_ops.aux_at(de)tach_dev() design only takes the aux-domain and the physical device as the parameters, this is insufficient if we want the vendor iommu drivers to learn the knowledge about relationships between the aux-domains and the subdevices. Add a @subdev parameter to iommu_ops.aux_at(de)tach_dev() interfaces so that a subdevice could be opt-in.

Signed-off-by Lu Baolu <[email protected]>

---
 drivers/iommu/intel/iommu.c | 10 ++++++----
 drivers/iommu/iommu.c       |  4 ++--
 include/linux/iommu.h       |  6 ++++--
 3 files changed, 12 insertions(+), 8 deletions(-)

drivers/iommu/intel/iommu.c +6 -4

index bce158468abf..3c12fd06856c 100644
@@ -5338,8 +5338,9 @@ static int intel_iommu_attach_device(struct iommu_domain *domain,
 	return domain_add_dev_info(to_dmar_domain(domain), dev);
 }
 
-static int intel_iommu_aux_attach_device(struct iommu_domain *domain,
-					 struct device *dev)
+static int
+intel_iommu_aux_attach_device(struct iommu_domain *domain,
+			      struct device *dev, struct device *subdev)
 {
 	int ret;
 

@@ -5359,8 +5360,9 @@ static void intel_iommu_detach_device(struct iommu_domain *domain,
 	dmar_remove_one_dev_info(dev);
 }
 
-static void intel_iommu_aux_detach_device(struct iommu_domain *domain,
-					  struct device *dev)
+static void
+intel_iommu_aux_detach_device(struct iommu_domain *domain, struct device *dev,
+			      struct device *subdev)
 {
 	aux_domain_remove_dev(to_dmar_domain(domain), dev);
 }

drivers/iommu/iommu.c +2 -2

index 609bd25bf154..38cdfeb887e1 100644
@@ -2728,7 +2728,7 @@ int iommu_aux_attach_device(struct iommu_domain *domain, struct device *dev)
 	int ret = -ENODEV;
 
 	if (domain->ops->aux_attach_dev)
-		ret = domain->ops->aux_attach_dev(domain, dev);
+		ret = domain->ops->aux_attach_dev(domain, dev, NULL);
 
 	if (!ret)
 		trace_attach_device_to_domain(dev);

@@ -2740,7 +2740,7 @@ EXPORT_SYMBOL_GPL(iommu_aux_attach_device);
 void iommu_aux_detach_device(struct iommu_domain *domain, struct device *dev)
 {
 	if (domain->ops->aux_detach_dev) {
-		domain->ops->aux_detach_dev(domain, dev);
+		domain->ops->aux_detach_dev(domain, dev, NULL);
 		trace_detach_device_from_domain(dev);
 	}
 }

include/linux/iommu.h +0 -0

index fee209efb756..871267104915 100644
@@ -279,8 +279,10 @@ struct iommu_ops {
 	int (*dev_disable_feat)(struct device *dev, enum iommu_dev_features f);
 

Quoted 85 lines · On Tue, 1 Sep 2020 11:34:18 +0800 Lu Baolu <[email protected]> wrote:

In the vfio/mdev use case of aux-domain, the subdevices are created from the physical devices with IOMMU_DEV_FEAT_AUX enabled and the aux-domains are attached to the subdevices through the iommu_ops.aux_attach_dev() interface.

Current iommu_ops.aux_at(de)tach_dev() design only takes the aux-domain and the physical device as the parameters, this is insufficient if we want the vendor iommu drivers to learn the knowledge about relationships between the aux-domains and the subdevices. Add a @subdev parameter to iommu_ops.aux_at(de)tach_dev() interfaces so that a subdevice could be opt-in.

Signed-off-by Lu Baolu <[email protected]>

---
  drivers/iommu/intel/iommu.c | 10 ++++++----
  drivers/iommu/iommu.c       |  4 ++--
  include/linux/iommu.h       |  6 ++++--
  3 files changed, 12 insertions(+), 8 deletions(-)

drivers/iommu/intel/iommu.c +6 -4

index bce158468abf..3c12fd06856c 100644
@@ -5338,8 +5338,9 @@ static int intel_iommu_attach_device(struct iommu_domain *domain,
  	return domain_add_dev_info(to_dmar_domain(domain), dev);
  }
 
-static int intel_iommu_aux_attach_device(struct iommu_domain *domain,
-					 struct device *dev)
+static int
+intel_iommu_aux_attach_device(struct iommu_domain *domain,
+			      struct device *dev, struct device *subdev)
  {
  	int ret;
 

@@ -5359,8 +5360,9 @@ static void intel_iommu_detach_device(struct iommu_domain *domain,
  	dmar_remove_one_dev_info(dev);
  }
 
-static void intel_iommu_aux_detach_device(struct iommu_domain *domain,
-					  struct device *dev)
+static void
+intel_iommu_aux_detach_device(struct iommu_domain *domain, struct device *dev,
+			      struct device *subdev)
  {
  	aux_domain_remove_dev(to_dmar_domain(domain), dev);
  }

drivers/iommu/iommu.c +2 -2

index 609bd25bf154..38cdfeb887e1 100644
@@ -2728,7 +2728,7 @@ int iommu_aux_attach_device(struct iommu_domain *domain, struct device *dev)
  	int ret = -ENODEV;
 
  	if (domain->ops->aux_attach_dev)
-		ret = domain->ops->aux_attach_dev(domain, dev);
+		ret = domain->ops->aux_attach_dev(domain, dev, NULL);
 
  	if (!ret)
  		trace_attach_device_to_domain(dev);

@@ -2740,7 +2740,7 @@ EXPORT_SYMBOL_GPL(iommu_aux_attach_device);
  void iommu_aux_detach_device(struct iommu_domain *domain, struct device *dev)
  {
  	if (domain->ops->aux_detach_dev) {
-		domain->ops->aux_detach_dev(domain, dev);
+		domain->ops->aux_detach_dev(domain, dev, NULL);
  		trace_detach_device_from_domain(dev);
  	}
  }

include/linux/iommu.h +4 -2

index fee209efb756..871267104915 100644
@@ -279,8 +279,10 @@ struct iommu_ops {
  	int (*dev_disable_feat)(struct device *dev, enum iommu_dev_features f);
 
  	/* Aux-domain specific attach/detach entries */
-	int (*aux_attach_dev)(struct iommu_domain *domain, struct device *dev);
-	void (*aux_detach_dev)(struct iommu_domain *domain, struct device *dev);
+	int (*aux_attach_dev)(struct iommu_domain *domain, struct device *dev,
+			      struct device *subdev);
+	void (*aux_detach_dev)(struct iommu_domain *domain, struct device *dev,
+			       struct device *subdev);
  	int (*aux_get_pasid)(struct iommu_domain *domain, struct device *dev);
 

include/linux/iommu.h struct iommu_ops { · line 288

Quoted 1 line

include/linux/iommu.h +0 -0

  	struct iommu_sva *(*sva_bind)(struct device *dev, struct mm_struct *mm,

Would this be a good spot in the code to provide comments more formally defining this subdevice concept? For example, what exactly is the relationship between the device and the subdevice and which device do we use for the remaining aux domain functions, ex. aux_get_pasid().

Thanks,

Alex

This adds two new APIs for the use cases like vfio/mdev where subdevices derived from physical devices are created and put in an iommu_group. The new IOMMU API interfaces mimic the vfio_mdev_at(de)tach_domain() directly, testing whether the resulting device supports IOMMU_DEV_FEAT_AUX and using an aux vs non-aux at(de)tach.

By doing this we could

- Set the iommu_group.domain. The iommu_group.domain is private to iommu core (therefore vfio code cannot set it), but we need it set in order for iommu_get_domain_for_dev() to work with a group attached to an aux domain.

- Prefer to use the _attach_group() interfaces while the _attach_device() interfaces are relegated to special cases.

Link https://lore.kernel.org/linux-iommu/[email protected]/ open here

Link https://lore.kernel.org/linux-iommu/[email protected]/ open here

Signed-off-by Lu Baolu <[email protected]>

---
 drivers/iommu/iommu.c | 136 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/iommu.h |  20 +++++++
 2 files changed, 156 insertions(+)

drivers/iommu/iommu.c +56 -0

index 38cdfeb887e1..fb21c2ff4861 100644
@@ -2757,6 +2757,142 @@ int iommu_aux_get_pasid(struct iommu_domain *domain, struct device *dev)
 }
 EXPORT_SYMBOL_GPL(iommu_aux_get_pasid);
 
+static int __iommu_aux_attach_device(struct iommu_domain *domain,
+				     struct device *phys_dev,
+				     struct device *sub_dev)
+{
+	int ret;
+
+	if (unlikely(!domain->ops->aux_attach_dev))
+		return -ENODEV;
+
+	ret = domain->ops->aux_attach_dev(domain, phys_dev, sub_dev);
+	if (!ret)
+		trace_attach_device_to_domain(sub_dev);
+
+	return ret;
+}
+
+static void __iommu_aux_detach_device(struct iommu_domain *domain,
+				      struct device *phys_dev,
+				      struct device *sub_dev)
+{
+	if (unlikely(!domain->ops->aux_detach_dev))
+		return;
+
+	domain->ops->aux_detach_dev(domain, phys_dev, sub_dev);
+	trace_detach_device_from_domain(sub_dev);
+}
+
+static int __iommu_attach_subdev_group(struct iommu_domain *domain,
+				       struct iommu_group *group,
+				       iommu_device_lookup_t fn)
+{
+	struct group_device *device;
+	struct device *phys_dev;
+	int ret = -ENODEV;
+
+	list_for_each_entry(device, &group->devices, list) {
+		phys_dev = fn(device->dev);
+		if (!phys_dev) {
+			ret = -ENODEV;
+			break;
+		}
+
+		if (iommu_dev_feature_enabled(phys_dev, IOMMU_DEV_FEAT_AUX))
+			ret = __iommu_aux_attach_device(domain, phys_dev,
+							device->dev);
+		else
+			ret = __iommu_attach_device(domain, phys_dev);
+
+		if (ret)
+			break;
+	}
+
+	return ret;
+}
+
+static void __iommu_detach_subdev_group(struct iommu_domain *domain,

drivers/iommu/iommu.c +70 -0

+
+		if (iommu_dev_feature_enabled(phys_dev, IOMMU_DEV_FEAT_AUX))
+			__iommu_aux_detach_device(domain, phys_dev, device->dev);
+		else
+			__iommu_detach_device(domain, phys_dev);
+	}
+}
+
+/**
+ * iommu_attach_subdev_group - attach domain to an iommu_group which
+ *			       contains subdevices.
+ *
+ * @domain: domain
+ * @group:  iommu_group which contains subdevices
+ * @fn:     callback for each subdevice in the @iommu_group to retrieve the
+ *          physical device where the subdevice was created from.
+ *
+ * Returns 0 on success, or an error value.
+ */
+int iommu_attach_subdev_group(struct iommu_domain *domain,
+			      struct iommu_group *group,
+			      iommu_device_lookup_t fn)
+{
+	int ret = -ENODEV;
+
+	mutex_lock(&group->mutex);
+	if (group->domain) {
+		ret = -EBUSY;
+		goto unlock_out;
+	}
+
+	ret = __iommu_attach_subdev_group(domain, group, fn);
+	if (ret)
+		__iommu_detach_subdev_group(domain, group, fn);
+	else
+		group->domain = domain;
+
+unlock_out:
+	mutex_unlock(&group->mutex);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(iommu_attach_subdev_group);
+
+/**
+ * iommu_detach_subdev_group - detach domain from an iommu_group which
+ *			       contains subdevices
+ *
+ * @domain: domain
+ * @group:  iommu_group which contains subdevices
+ * @fn:     callback for each subdevice in the @iommu_group to retrieve the
+ *          physical device where the subdevice was created from.
+ *
+ * The domain must have been attached to @group via iommu_attach_subdev_group().
+ */
+void iommu_detach_subdev_group(struct iommu_domain *domain,
+			       struct iommu_group *group,
+			       iommu_device_lookup_t fn)
+{
+	mutex_lock(&group->mutex);
+	if (!group->domain)
+		goto unlock_out;
+
+	__iommu_detach_subdev_group(domain, group, fn);
+	group->domain = NULL;
+
+unlock_out:
+	mutex_unlock(&group->mutex);
+}
+EXPORT_SYMBOL_GPL(iommu_detach_subdev_group);
+
 /**
  * iommu_sva_bind_device() - Bind a process address space to a device
  * @dev: the device

include/linux/iommu.h +13 -0

index 871267104915..b9df8b510d4f 100644
@@ -48,6 +48,7 @@ struct iommu_fault_event;
 typedef int (*iommu_fault_handler_t)(struct iommu_domain *,
 			struct device *, unsigned long, int, void *);
 typedef int (*iommu_dev_fault_handler_t)(struct iommu_fault *, void *);
+typedef struct device *(*iommu_device_lookup_t)(struct device *);
 
 struct iommu_domain_geometry {
 	dma_addr_t aperture_start; /* First address that can be mapped    */

@@ -631,6 +632,12 @@ bool iommu_dev_feature_enabled(struct device *dev, enum iommu_dev_features f);
 int iommu_aux_attach_device(struct iommu_domain *domain, struct device *dev);
 void iommu_aux_detach_device(struct iommu_domain *domain, struct device *dev);
 int iommu_aux_get_pasid(struct iommu_domain *domain, struct device *dev);
+int iommu_attach_subdev_group(struct iommu_domain *domain,
+			      struct iommu_group *group,
+			      iommu_device_lookup_t fn);
+void iommu_detach_subdev_group(struct iommu_domain *domain,
+			       struct iommu_group *group,
+			       iommu_device_lookup_t fn);
 
 struct iommu_sva *iommu_sva_bind_device(struct device *dev,
 					struct mm_struct *mm,

@@ -1019,6 +1026,19 @@ iommu_aux_get_pasid(struct iommu_domain *domain, struct device *dev)
 	return -ENODEV;
 }
 
+static inline int
+iommu_attach_subdev_group(struct iommu_domain *domain, struct iommu_group *group,
+			  iommu_device_lookup_t fn)
+{
+	return -ENODEV;
+}

Quoted 88 lines · On Tue, 1 Sep 2020 11:34:19 +0800 Lu Baolu <[email protected]> wrote:

This adds two new APIs for the use cases like vfio/mdev where subdevices derived from physical devices are created and put in an iommu_group. The new IOMMU API interfaces mimic the vfio_mdev_at(de)tach_domain() directly, testing whether the resulting device supports IOMMU_DEV_FEAT_AUX and using an aux vs non-aux at(de)tach.

By doing this we could

- Set the iommu_group.domain. The iommu_group.domain is private to iommu core (therefore vfio code cannot set it), but we need it set in order for iommu_get_domain_for_dev() to work with a group attached to an aux domain.

- Prefer to use the _attach_group() interfaces while the _attach_device() interfaces are relegated to special cases.

Link https://lore.kernel.org/linux-iommu/[email protected]/ open here

Link https://lore.kernel.org/linux-iommu/[email protected]/ open here

Signed-off-by Lu Baolu <[email protected]>

---
  drivers/iommu/iommu.c | 136 ++++++++++++++++++++++++++++++++++++++++++
  include/linux/iommu.h |  20 +++++++
  2 files changed, 156 insertions(+)

drivers/iommu/iommu.c +56 -0

index 38cdfeb887e1..fb21c2ff4861 100644
@@ -2757,6 +2757,142 @@ int iommu_aux_get_pasid(struct iommu_domain *domain, struct device *dev)
  }
  EXPORT_SYMBOL_GPL(iommu_aux_get_pasid);
 
+static int __iommu_aux_attach_device(struct iommu_domain *domain,
+				     struct device *phys_dev,
+				     struct device *sub_dev)
+{
+	int ret;
+
+	if (unlikely(!domain->ops->aux_attach_dev))
+		return -ENODEV;
+
+	ret = domain->ops->aux_attach_dev(domain, phys_dev, sub_dev);
+	if (!ret)
+		trace_attach_device_to_domain(sub_dev);
+
+	return ret;
+}
+
+static void __iommu_aux_detach_device(struct iommu_domain *domain,
+				      struct device *phys_dev,
+				      struct device *sub_dev)
+{
+	if (unlikely(!domain->ops->aux_detach_dev))
+		return;
+
+	domain->ops->aux_detach_dev(domain, phys_dev, sub_dev);
+	trace_detach_device_from_domain(sub_dev);
+}
+
+static int __iommu_attach_subdev_group(struct iommu_domain *domain,
+				       struct iommu_group *group,
+				       iommu_device_lookup_t fn)
+{
+	struct group_device *device;
+	struct device *phys_dev;
+	int ret = -ENODEV;
+
+	list_for_each_entry(device, &group->devices, list) {
+		phys_dev = fn(device->dev);
+		if (!phys_dev) {
+			ret = -ENODEV;
+			break;
+		}
+
+		if (iommu_dev_feature_enabled(phys_dev, IOMMU_DEV_FEAT_AUX))
+			ret = __iommu_aux_attach_device(domain, phys_dev,
+							device->dev);
+		else
+			ret = __iommu_attach_device(domain, phys_dev);
+
+		if (ret)
+			break;
+	}
+
+	return ret;
+}
+
+static void __iommu_detach_subdev_group(struct iommu_domain *domain,

drivers/iommu/iommu.c int iommu_aux_get_pasid(struct iommu_domain *domain, struct device *dev) · line 2825

Quoted 10 lines

drivers/iommu/iommu.c +10 -0

+					struct iommu_group *group,
+					iommu_device_lookup_t fn)
+{
+	struct group_device *device;
+	struct device *phys_dev;
+
+	list_for_each_entry(device, &group->devices, list) {
+		phys_dev = fn(device->dev);
+		if (!phys_dev)
+			break;

Seems like this should be a continue rather than a break. On the unwind path maybe we're relying on holding the group mutex and deterministic behavior from the fn() callback to unwind to the same point, but we still have an entirely separate detach interface and I'm not sure we couldn't end up with an inconsistent state if we don't iterate each group device here. Thanks,

Alex

Quoted 118 lines

drivers/iommu/iommu.c +70 -0

+
+		if (iommu_dev_feature_enabled(phys_dev, IOMMU_DEV_FEAT_AUX))
+			__iommu_aux_detach_device(domain, phys_dev, device->dev);
+		else
+			__iommu_detach_device(domain, phys_dev);
+	}
+}
+
+/**
+ * iommu_attach_subdev_group - attach domain to an iommu_group which
+ *			       contains subdevices.
+ *
+ * @domain: domain
+ * @group:  iommu_group which contains subdevices
+ * @fn:     callback for each subdevice in the @iommu_group to retrieve the
+ *          physical device where the subdevice was created from.
+ *
+ * Returns 0 on success, or an error value.
+ */
+int iommu_attach_subdev_group(struct iommu_domain *domain,
+			      struct iommu_group *group,
+			      iommu_device_lookup_t fn)
+{
+	int ret = -ENODEV;
+
+	mutex_lock(&group->mutex);
+	if (group->domain) {
+		ret = -EBUSY;
+		goto unlock_out;
+	}
+
+	ret = __iommu_attach_subdev_group(domain, group, fn);
+	if (ret)
+		__iommu_detach_subdev_group(domain, group, fn);
+	else
+		group->domain = domain;
+
+unlock_out:
+	mutex_unlock(&group->mutex);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(iommu_attach_subdev_group);
+
+/**
+ * iommu_detach_subdev_group - detach domain from an iommu_group which
+ *			       contains subdevices
+ *
+ * @domain: domain
+ * @group:  iommu_group which contains subdevices
+ * @fn:     callback for each subdevice in the @iommu_group to retrieve the
+ *          physical device where the subdevice was created from.
+ *
+ * The domain must have been attached to @group via iommu_attach_subdev_group().
+ */
+void iommu_detach_subdev_group(struct iommu_domain *domain,
+			       struct iommu_group *group,
+			       iommu_device_lookup_t fn)
+{
+	mutex_lock(&group->mutex);
+	if (!group->domain)
+		goto unlock_out;
+
+	__iommu_detach_subdev_group(domain, group, fn);
+	group->domain = NULL;
+
+unlock_out:
+	mutex_unlock(&group->mutex);
+}
+EXPORT_SYMBOL_GPL(iommu_detach_subdev_group);
+
  /**
   * iommu_sva_bind_device() - Bind a process address space to a device
   * @dev: the device

include/linux/iommu.h +20 -0

index 871267104915..b9df8b510d4f 100644
@@ -48,6 +48,7 @@ struct iommu_fault_event;
  typedef int (*iommu_fault_handler_t)(struct iommu_domain *,
  			struct device *, unsigned long, int, void *);
  typedef int (*iommu_dev_fault_handler_t)(struct iommu_fault *, void *);
+typedef struct device *(*iommu_device_lookup_t)(struct device *);
 
  struct iommu_domain_geometry {
  	dma_addr_t aperture_start; /* First address that can be mapped    */

@@ -631,6 +632,12 @@ bool iommu_dev_feature_enabled(struct device *dev, enum iommu_dev_features f);
  int iommu_aux_attach_device(struct iommu_domain *domain, struct device *dev);
  void iommu_aux_detach_device(struct iommu_domain *domain, struct device *dev);
  int iommu_aux_get_pasid(struct iommu_domain *domain, struct device *dev);
+int iommu_attach_subdev_group(struct iommu_domain *domain,
+			      struct iommu_group *group,
+			      iommu_device_lookup_t fn);
+void iommu_detach_subdev_group(struct iommu_domain *domain,
+			       struct iommu_group *group,
+			       iommu_device_lookup_t fn);
 
  struct iommu_sva *iommu_sva_bind_device(struct device *dev,
  					struct mm_struct *mm,

@@ -1019,6 +1026,19 @@ iommu_aux_get_pasid(struct iommu_domain *domain, struct device *dev)
  	return -ENODEV;
  }
 
+static inline int
+iommu_attach_subdev_group(struct iommu_domain *domain, struct iommu_group *group,
+			  iommu_device_lookup_t fn)
+{
+	return -ENODEV;
+}
+
+static inline void
+iommu_detach_subdev_group(struct iommu_domain *domain, struct iommu_group *group,
+			  iommu_device_lookup_t fn)
+{
+}
+
  static inline struct iommu_sva *
  iommu_sva_bind_device(struct device *dev, struct mm_struct *mm, void *drvdata)
  {

With subdevice information opt-in through iommu_ops.aux_at(de)tach_dev() interfaces, the vendor iommu driver is able to learn the knowledge about the relationships between the subdevices and the aux-domains. Implement is_aux_domain() support based on the relationship knowledges.

Signed-off-by Lu Baolu <[email protected]>

---
 drivers/iommu/intel/iommu.c | 125 ++++++++++++++++++++++++++----------
 include/linux/intel-iommu.h |  17 +++--
 2 files changed, 103 insertions(+), 39 deletions(-)

drivers/iommu/intel/iommu.c +40 -22

index 3c12fd06856c..50431c7b2e71 100644
@@ -334,6 +334,8 @@ static int intel_iommu_attach_device(struct iommu_domain *domain,
 				     struct device *dev);
 static phys_addr_t intel_iommu_iova_to_phys(struct iommu_domain *domain,
 					    dma_addr_t iova);
+static bool intel_iommu_dev_feat_enabled(struct device *dev,
+					 enum iommu_dev_features feat);
 
 #ifdef CONFIG_INTEL_IOMMU_DEFAULT_ON
 int dmar_disabled = 0;

@@ -1832,6 +1834,7 @@ static struct dmar_domain *alloc_domain(int flags)
 		domain->flags |= DOMAIN_FLAG_USE_FIRST_LEVEL;
 	domain->has_iotlb_device = false;
 	INIT_LIST_HEAD(&domain->devices);
+	INIT_LIST_HEAD(&domain->subdevices);
 
 	return domain;
 }

@@ -2580,7 +2583,7 @@ static struct dmar_domain *dmar_insert_one_dev_info(struct intel_iommu *iommu,
 	info->iommu = iommu;
 	info->pasid_table = NULL;
 	info->auxd_enabled = 0;
-	INIT_LIST_HEAD(&info->auxiliary_domains);
+	INIT_LIST_HEAD(&info->subdevices);
 
 	if (dev && dev_is_pci(dev)) {
 		struct pci_dev *pdev = to_pci_dev(info->dev);

@@ -5137,21 +5140,28 @@ static void intel_iommu_domain_free(struct iommu_domain *domain)
 		domain_exit(to_dmar_domain(domain));
 }
 
-/*
- * Check whether a @domain could be attached to the @dev through the
- * aux-domain attach/detach APIs.
- */
-static inline bool
-is_aux_domain(struct device *dev, struct iommu_domain *domain)
+/* Lookup subdev_info in the domain's subdevice siblings. */
+static struct subdev_info *
+subdev_lookup_domain(struct dmar_domain *domain, struct device *dev,
+		     struct device *subdev)
 {
-	struct device_domain_info *info = get_domain_info(dev);
+	struct subdev_info *sinfo = NULL, *tmp;
 
-	return info && info->auxd_enabled &&
-			domain->type == IOMMU_DOMAIN_UNMANAGED;
+	assert_spin_locked(&device_domain_lock);
+
+	list_for_each_entry(tmp, &domain->subdevices, link_domain) {
+		if ((!dev || tmp->pdev == dev) && tmp->dev == subdev) {
+			sinfo = tmp;
+			break;
+		}
+	}
+
+	return sinfo;
 }
 
-static void auxiliary_link_device(struct dmar_domain *domain,
-				  struct device *dev)
+static void
+subdev_link_device(struct dmar_domain *domain, struct device *dev,
+		   struct subdev_info *sinfo)
 {
 	struct device_domain_info *info = get_domain_info(dev);
 

@@ -5159,12 +5169,13 @@ static void auxiliary_link_device(struct dmar_domain *domain,
 	if (WARN_ON(!info))
 		return;
 
-	domain->auxd_refcnt++;
-	list_add(&domain->auxd, &info->auxiliary_domains);
+	list_add(&info->subdevices, &sinfo->link_phys);
+	list_add(&domain->subdevices, &sinfo->link_domain);
 }
 
-static void auxiliary_unlink_device(struct dmar_domain *domain,
-				    struct device *dev)
+static void
+subdev_unlink_device(struct dmar_domain *domain, struct device *dev,
+		     struct subdev_info *sinfo)
 {
 	struct device_domain_info *info = get_domain_info(dev);
 

@@ -5172,24 +5183,30 @@ static void auxiliary_unlink_device(struct dmar_domain *domain,
 	if (WARN_ON(!info))
 		return;
 
-	list_del(&domain->auxd);
-	domain->auxd_refcnt--;
+	list_del(&sinfo->link_phys);
+	list_del(&sinfo->link_domain);
+	kfree(sinfo);
 
-	if (!domain->auxd_refcnt && domain->default_pasid > 0)
+	if (list_empty(&domain->subdevices) && domain->default_pasid > 0)
 		ioasid_free(domain->default_pasid);
 }
 
-static int aux_domain_add_dev(struct dmar_domain *domain,
-			      struct device *dev)
+static int aux_domain_add_dev(struct dmar_domain *domain, struct device *dev,
+			      struct device *subdev)
 {
 	int ret;
 	unsigned long flags;
 	struct intel_iommu *iommu;
+	struct subdev_info *sinfo;
 
 	iommu = device_to_iommu(dev, NULL, NULL);
 	if (!iommu)
 		return -ENODEV;
 
+	sinfo = kzalloc(sizeof(*sinfo), GFP_KERNEL);
+	if (!sinfo)
+		return -ENOMEM;
+
 	if (domain->default_pasid <= 0) {
 		int pasid;
 

@@ -5199,7 +5216,8 @@ static int aux_domain_add_dev(struct dmar_domain *domain,
 				     NULL);
 		if (pasid == INVALID_IOASID) {
 			pr_err("Can't allocate default pasid\n");
-			return -ENODEV;
+			ret = -ENODEV;
+			goto pasid_failed;
 		}
 		domain->default_pasid = pasid;
 	}

drivers/iommu/intel/iommu.c +48 -10

 	spin_unlock_irqrestore(&device_domain_lock, flags);
 

@@ -5236,27 +5257,36 @@ static int aux_domain_add_dev(struct dmar_domain *domain,
 attach_failed:
 	spin_unlock(&iommu->lock);
 	spin_unlock_irqrestore(&device_domain_lock, flags);
-	if (!domain->auxd_refcnt && domain->default_pasid > 0)
+	if (list_empty(&domain->subdevices) && domain->default_pasid > 0)
 		ioasid_free(domain->default_pasid);
+pasid_failed:
+	kfree(sinfo);
 
 	return ret;
 }
 
-static void aux_domain_remove_dev(struct dmar_domain *domain,
-				  struct device *dev)
+static void
+aux_domain_remove_dev(struct dmar_domain *domain, struct device *dev,
+		      struct device *subdev)
 {
 	struct device_domain_info *info;
 	struct intel_iommu *iommu;
+	struct subdev_info *sinfo;
 	unsigned long flags;
 
-	if (!is_aux_domain(dev, &domain->domain))
+	if (!intel_iommu_dev_feat_enabled(dev, IOMMU_DEV_FEAT_AUX) ||
+	    domain->domain.type != IOMMU_DOMAIN_UNMANAGED)
 		return;
 
 	spin_lock_irqsave(&device_domain_lock, flags);
 	info = get_domain_info(dev);
 	iommu = info->iommu;
-
-	auxiliary_unlink_device(domain, dev);
+	sinfo = subdev_lookup_domain(domain, dev, subdev);
+	if (!sinfo) {
+		spin_unlock_irqrestore(&device_domain_lock, flags);
+		return;
+	}
+	subdev_unlink_device(domain, dev, sinfo);
 
 	spin_lock(&iommu->lock);
 	intel_pasid_tear_down_entry(iommu, dev, domain->default_pasid, false);

@@ -5319,7 +5349,8 @@ static int intel_iommu_attach_device(struct iommu_domain *domain,
 		return -EPERM;
 	}
 
-	if (is_aux_domain(dev, domain))
+	if (intel_iommu_dev_feat_enabled(dev, IOMMU_DEV_FEAT_AUX) &&
+	    domain->type == IOMMU_DOMAIN_UNMANAGED)
 		return -EPERM;
 
 	/* normally dev is not mapped */

@@ -5344,14 +5375,15 @@ intel_iommu_aux_attach_device(struct iommu_domain *domain,
 {
 	int ret;
 
-	if (!is_aux_domain(dev, domain))
+	if (!intel_iommu_dev_feat_enabled(dev, IOMMU_DEV_FEAT_AUX) ||
+	    domain->type != IOMMU_DOMAIN_UNMANAGED)
 		return -EPERM;
 
 	ret = prepare_domain_attach_device(domain, dev);
 	if (ret)
 		return ret;
 
-	return aux_domain_add_dev(to_dmar_domain(domain), dev);
+	return aux_domain_add_dev(to_dmar_domain(domain), dev, subdev);
 }
 
 static void intel_iommu_detach_device(struct iommu_domain *domain,

@@ -5364,7 +5396,7 @@ static void
 intel_iommu_aux_detach_device(struct iommu_domain *domain, struct device *dev,
 			      struct device *subdev)
 {
-	aux_domain_remove_dev(to_dmar_domain(domain), dev);
+	aux_domain_remove_dev(to_dmar_domain(domain), dev, subdev);
 }
 
 /*

@@ -6020,6 +6052,32 @@ static bool intel_iommu_is_attach_deferred(struct iommu_domain *domain,
 	return attach_deferred(dev);
 }
 
+static int
+intel_iommu_domain_get_attr(struct iommu_domain *domain,
+			    enum iommu_attr attr, void *data)
+{
+	struct dmar_domain *dmar_domain = to_dmar_domain(domain);
+	unsigned long flags;
+	int ret;
+
+	if (domain->type != IOMMU_DOMAIN_UNMANAGED)
+		return -EINVAL;
+
+	switch (attr) {
+	case DOMAIN_ATTR_IS_AUX:
+		spin_lock_irqsave(&device_domain_lock, flags);
+		ret = !IS_ERR_OR_NULL(subdev_lookup_domain(dmar_domain,
+							   NULL, data));
+		spin_unlock_irqrestore(&device_domain_lock, flags);
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
+	return ret;
+}
+
 static int
 intel_iommu_domain_set_attr(struct iommu_domain *domain,
 			    enum iommu_attr attr, void *data)

@@ -6073,6 +6131,7 @@ const struct iommu_ops intel_iommu_ops = {
 	.domain_alloc		= intel_iommu_domain_alloc,
 	.domain_free		= intel_iommu_domain_free,
 	.domain_set_attr	= intel_iommu_domain_set_attr,
+	.domain_get_attr	= intel_iommu_domain_get_attr,
 	.attach_dev		= intel_iommu_attach_device,
 	.detach_dev		= intel_iommu_detach_device,
 	.aux_attach_dev		= intel_iommu_aux_attach_device,

include/linux/intel-iommu.h +10 -3

index b1ed2f25f7c0..47ba1904c691 100644
@@ -526,11 +526,9 @@ struct dmar_domain {
 					/* Domain ids per IOMMU. Use u16 since
 					 * domain ids are 16 bit wide according
 					 * to VT-d spec, section 9.3 */
-	unsigned int	auxd_refcnt;	/* Refcount of auxiliary attaching */
-
 	bool has_iotlb_device;
 	struct list_head devices;	/* all devices' list */
-	struct list_head auxd;		/* link to device's auxiliary list */
+	struct list_head subdevices;	/* all subdevices' list */
 	struct iova_domain iovad;	/* iova's that belong to this domain */
 
 	struct dma_pte	*pgd;		/* virtual address */

@@ -603,14 +601,21 @@ struct intel_iommu {
 	struct dmar_drhd_unit *drhd;
 };
 
+/* Per subdevice private data */
+struct subdev_info {
+	struct list_head link_phys;	/* link to phys device siblings */
+	struct list_head link_domain;	/* link to domain siblings */
+	struct device *pdev;		/* physical device derived from */
+	struct device *dev;		/* subdevice node */
+	struct dmar_domain *domain;	/* aux-domain */
+};
+
 /* PCI domain-device relationship */
 struct device_domain_info {

Quoted 149 lines · On Tue, 1 Sep 2020 11:34:22 +0800 Lu Baolu <[email protected]> wrote:

With subdevice information opt-in through iommu_ops.aux_at(de)tach_dev() interfaces, the vendor iommu driver is able to learn the knowledge about the relationships between the subdevices and the aux-domains. Implement is_aux_domain() support based on the relationship knowledges.

Signed-off-by Lu Baolu <[email protected]>

---
  drivers/iommu/intel/iommu.c | 125 ++++++++++++++++++++++++++----------
  include/linux/intel-iommu.h |  17 +++--
  2 files changed, 103 insertions(+), 39 deletions(-)

drivers/iommu/intel/iommu.c +40 -22

index 3c12fd06856c..50431c7b2e71 100644
@@ -334,6 +334,8 @@ static int intel_iommu_attach_device(struct iommu_domain *domain,
  				     struct device *dev);
  static phys_addr_t intel_iommu_iova_to_phys(struct iommu_domain *domain,
  					    dma_addr_t iova);
+static bool intel_iommu_dev_feat_enabled(struct device *dev,
+					 enum iommu_dev_features feat);
 
  #ifdef CONFIG_INTEL_IOMMU_DEFAULT_ON
  int dmar_disabled = 0;

@@ -1832,6 +1834,7 @@ static struct dmar_domain *alloc_domain(int flags)
  		domain->flags |= DOMAIN_FLAG_USE_FIRST_LEVEL;
  	domain->has_iotlb_device = false;
  	INIT_LIST_HEAD(&domain->devices);
+	INIT_LIST_HEAD(&domain->subdevices);
 
  	return domain;
  }

@@ -2580,7 +2583,7 @@ static struct dmar_domain *dmar_insert_one_dev_info(struct intel_iommu *iommu,
  	info->iommu = iommu;
  	info->pasid_table = NULL;
  	info->auxd_enabled = 0;
-	INIT_LIST_HEAD(&info->auxiliary_domains);
+	INIT_LIST_HEAD(&info->subdevices);
 
  	if (dev && dev_is_pci(dev)) {
  		struct pci_dev *pdev = to_pci_dev(info->dev);

@@ -5137,21 +5140,28 @@ static void intel_iommu_domain_free(struct iommu_domain *domain)
  		domain_exit(to_dmar_domain(domain));
  }
 
-/*
- * Check whether a @domain could be attached to the @dev through the
- * aux-domain attach/detach APIs.
- */
-static inline bool
-is_aux_domain(struct device *dev, struct iommu_domain *domain)
+/* Lookup subdev_info in the domain's subdevice siblings. */
+static struct subdev_info *
+subdev_lookup_domain(struct dmar_domain *domain, struct device *dev,
+		     struct device *subdev)
  {
-	struct device_domain_info *info = get_domain_info(dev);
+	struct subdev_info *sinfo = NULL, *tmp;
 
-	return info && info->auxd_enabled &&
-			domain->type == IOMMU_DOMAIN_UNMANAGED;
+	assert_spin_locked(&device_domain_lock);
+
+	list_for_each_entry(tmp, &domain->subdevices, link_domain) {
+		if ((!dev || tmp->pdev == dev) && tmp->dev == subdev) {
+			sinfo = tmp;
+			break;
+		}
+	}
+
+	return sinfo;
  }
 
-static void auxiliary_link_device(struct dmar_domain *domain,
-				  struct device *dev)
+static void
+subdev_link_device(struct dmar_domain *domain, struct device *dev,
+		   struct subdev_info *sinfo)
  {
  	struct device_domain_info *info = get_domain_info(dev);
 

@@ -5159,12 +5169,13 @@ static void auxiliary_link_device(struct dmar_domain *domain,
  	if (WARN_ON(!info))
  		return;
 
-	domain->auxd_refcnt++;
-	list_add(&domain->auxd, &info->auxiliary_domains);
+	list_add(&info->subdevices, &sinfo->link_phys);
+	list_add(&domain->subdevices, &sinfo->link_domain);
  }
 
-static void auxiliary_unlink_device(struct dmar_domain *domain,
-				    struct device *dev)
+static void
+subdev_unlink_device(struct dmar_domain *domain, struct device *dev,
+		     struct subdev_info *sinfo)
  {
  	struct device_domain_info *info = get_domain_info(dev);
 

@@ -5172,24 +5183,30 @@ static void auxiliary_unlink_device(struct dmar_domain *domain,
  	if (WARN_ON(!info))
  		return;
 
-	list_del(&domain->auxd);
-	domain->auxd_refcnt--;
+	list_del(&sinfo->link_phys);
+	list_del(&sinfo->link_domain);
+	kfree(sinfo);
 
-	if (!domain->auxd_refcnt && domain->default_pasid > 0)
+	if (list_empty(&domain->subdevices) && domain->default_pasid > 0)
  		ioasid_free(domain->default_pasid);
  }
 
-static int aux_domain_add_dev(struct dmar_domain *domain,
-			      struct device *dev)
+static int aux_domain_add_dev(struct dmar_domain *domain, struct device *dev,
+			      struct device *subdev)
  {
  	int ret;
  	unsigned long flags;
  	struct intel_iommu *iommu;
+	struct subdev_info *sinfo;
 
  	iommu = device_to_iommu(dev, NULL, NULL);
  	if (!iommu)
  		return -ENODEV;
 
+	sinfo = kzalloc(sizeof(*sinfo), GFP_KERNEL);
+	if (!sinfo)
+		return -ENOMEM;
+
  	if (domain->default_pasid <= 0) {
  		int pasid;
 

@@ -5199,7 +5216,8 @@ static int aux_domain_add_dev(struct dmar_domain *domain,
  				     NULL);
  		if (pasid == INVALID_IOASID) {
  			pr_err("Can't allocate default pasid\n");
-			return -ENODEV;
+			ret = -ENODEV;
+			goto pasid_failed;
  		}
  		domain->default_pasid = pasid;
  	}

@@ -5225,7 +5243,10 @@ static int aux_domain_add_dev(struct dmar_domain *domain,
  		goto table_failed;
  	spin_unlock(&iommu->lock);
 

drivers/iommu/intel/iommu.c static int aux_domain_add_dev(struct dmar_domain *domain, · line 5249

Quoted 5 lines

drivers/iommu/intel/iommu.c +4 -1

-	auxiliary_link_device(domain, dev);
+	sinfo->dev = subdev;
+	sinfo->domain = domain;
+	sinfo->pdev = dev;
+	subdev_link_device(domain, dev, sinfo);

The unlink path frees sinfo, would it make more sense to pass subdev, domain, and dev to the link function and let it allocate the sinfo struct and return it on success? I'm not sure if you're pre-allocating it to avoid something hard to unwind, but it feels asymmetric between the link and unlink semantics. Thanks,

Alex

Quoted 158 lines

drivers/iommu/intel/iommu.c +48 -10

  	spin_unlock_irqrestore(&device_domain_lock, flags);
 

@@ -5236,27 +5257,36 @@ static int aux_domain_add_dev(struct dmar_domain *domain,
  attach_failed:
  	spin_unlock(&iommu->lock);
  	spin_unlock_irqrestore(&device_domain_lock, flags);
-	if (!domain->auxd_refcnt && domain->default_pasid > 0)
+	if (list_empty(&domain->subdevices) && domain->default_pasid > 0)
  		ioasid_free(domain->default_pasid);
+pasid_failed:
+	kfree(sinfo);
 
  	return ret;
  }
 
-static void aux_domain_remove_dev(struct dmar_domain *domain,
-				  struct device *dev)
+static void
+aux_domain_remove_dev(struct dmar_domain *domain, struct device *dev,
+		      struct device *subdev)
  {
  	struct device_domain_info *info;
  	struct intel_iommu *iommu;
+	struct subdev_info *sinfo;
  	unsigned long flags;
 
-	if (!is_aux_domain(dev, &domain->domain))
+	if (!intel_iommu_dev_feat_enabled(dev, IOMMU_DEV_FEAT_AUX) ||
+	    domain->domain.type != IOMMU_DOMAIN_UNMANAGED)
  		return;
 
  	spin_lock_irqsave(&device_domain_lock, flags);
  	info = get_domain_info(dev);
  	iommu = info->iommu;
-
-	auxiliary_unlink_device(domain, dev);
+	sinfo = subdev_lookup_domain(domain, dev, subdev);
+	if (!sinfo) {
+		spin_unlock_irqrestore(&device_domain_lock, flags);
+		return;
+	}
+	subdev_unlink_device(domain, dev, sinfo);
 
  	spin_lock(&iommu->lock);
  	intel_pasid_tear_down_entry(iommu, dev, domain->default_pasid, false);

@@ -5319,7 +5349,8 @@ static int intel_iommu_attach_device(struct iommu_domain *domain,
  		return -EPERM;
  	}
 
-	if (is_aux_domain(dev, domain))
+	if (intel_iommu_dev_feat_enabled(dev, IOMMU_DEV_FEAT_AUX) &&
+	    domain->type == IOMMU_DOMAIN_UNMANAGED)
  		return -EPERM;
 
  	/* normally dev is not mapped */

@@ -5344,14 +5375,15 @@ intel_iommu_aux_attach_device(struct iommu_domain *domain,
  {
  	int ret;
 
-	if (!is_aux_domain(dev, domain))
+	if (!intel_iommu_dev_feat_enabled(dev, IOMMU_DEV_FEAT_AUX) ||
+	    domain->type != IOMMU_DOMAIN_UNMANAGED)
  		return -EPERM;
 
  	ret = prepare_domain_attach_device(domain, dev);
  	if (ret)
  		return ret;
 
-	return aux_domain_add_dev(to_dmar_domain(domain), dev);
+	return aux_domain_add_dev(to_dmar_domain(domain), dev, subdev);
  }
 
  static void intel_iommu_detach_device(struct iommu_domain *domain,

@@ -5364,7 +5396,7 @@ static void
  intel_iommu_aux_detach_device(struct iommu_domain *domain, struct device *dev,
  			      struct device *subdev)
  {
-	aux_domain_remove_dev(to_dmar_domain(domain), dev);
+	aux_domain_remove_dev(to_dmar_domain(domain), dev, subdev);
  }
 
  /*

@@ -6020,6 +6052,32 @@ static bool intel_iommu_is_attach_deferred(struct iommu_domain *domain,
  	return attach_deferred(dev);
  }
 
+static int
+intel_iommu_domain_get_attr(struct iommu_domain *domain,
+			    enum iommu_attr attr, void *data)
+{
+	struct dmar_domain *dmar_domain = to_dmar_domain(domain);
+	unsigned long flags;
+	int ret;
+
+	if (domain->type != IOMMU_DOMAIN_UNMANAGED)
+		return -EINVAL;
+
+	switch (attr) {
+	case DOMAIN_ATTR_IS_AUX:
+		spin_lock_irqsave(&device_domain_lock, flags);
+		ret = !IS_ERR_OR_NULL(subdev_lookup_domain(dmar_domain,
+							   NULL, data));
+		spin_unlock_irqrestore(&device_domain_lock, flags);
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
+	return ret;
+}
+
  static int
  intel_iommu_domain_set_attr(struct iommu_domain *domain,
  			    enum iommu_attr attr, void *data)

@@ -6073,6 +6131,7 @@ const struct iommu_ops intel_iommu_ops = {
  	.domain_alloc		= intel_iommu_domain_alloc,
  	.domain_free		= intel_iommu_domain_free,
  	.domain_set_attr	= intel_iommu_domain_set_attr,
+	.domain_get_attr	= intel_iommu_domain_get_attr,
  	.attach_dev		= intel_iommu_attach_device,
  	.detach_dev		= intel_iommu_detach_device,
  	.aux_attach_dev		= intel_iommu_aux_attach_device,

include/linux/intel-iommu.h +10 -3

index b1ed2f25f7c0..47ba1904c691 100644
@@ -526,11 +526,9 @@ struct dmar_domain {
  					/* Domain ids per IOMMU. Use u16 since
  					 * domain ids are 16 bit wide according
  					 * to VT-d spec, section 9.3 */
-	unsigned int	auxd_refcnt;	/* Refcount of auxiliary attaching */
-
  	bool has_iotlb_device;
  	struct list_head devices;	/* all devices' list */
-	struct list_head auxd;		/* link to device's auxiliary list */
+	struct list_head subdevices;	/* all subdevices' list */
  	struct iova_domain iovad;	/* iova's that belong to this domain */
 
  	struct dma_pte	*pgd;		/* virtual address */

@@ -603,14 +601,21 @@ struct intel_iommu {
  	struct dmar_drhd_unit *drhd;
  };
 
+/* Per subdevice private data */
+struct subdev_info {
+	struct list_head link_phys;	/* link to phys device siblings */
+	struct list_head link_domain;	/* link to domain siblings */
+	struct device *pdev;		/* physical device derived from */
+	struct device *dev;		/* subdevice node */
+	struct dmar_domain *domain;	/* aux-domain */
+};
+
  /* PCI domain-device relationship */
  struct device_domain_info {
  	struct list_head link;	/* link to domain siblings */
  	struct list_head global; /* link to global list */

include/linux/intel-iommu.h struct intel_iommu { · line 621

Quoted 8 lines

include/linux/intel-iommu.h +1 -3

  	struct list_head table;	/* link to pasid table */
-	struct list_head auxiliary_domains; /* auxiliary domains
-					     * attached to this device
-					     */
+	struct list_head subdevices; /* subdevices sibling */
  	u32 segment;		/* PCI segment number */
  	u8 bus;			/* PCI bus number */
  	u8 devfn;		/* PCI devfn number */