Re: [PATCH v4 2/5] iommu: Add iommu_at(de)tach_subdev_group()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Alex,

On 9/11/20 6:05 AM, Alex Williamson wrote:
On Tue,  1 Sep 2020 11:34:19 +0800
Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx> 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/20200730134658.44c57a67@xxxxxxx/
Link: https://lore.kernel.org/linux-iommu/20200730151703.5daf8ad4@xxxxxxx/
Signed-off-by: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
---
  drivers/iommu/iommu.c | 136 ++++++++++++++++++++++++++++++++++++++++++
  include/linux/iommu.h |  20 +++++++
  2 files changed, 156 insertions(+)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 38cdfeb887e1..fb21c2ff4861 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -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,
+					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,

Yes, agreed.

Best regards,
baolu


Alex

+
+		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
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 871267104915..b9df8b510d4f 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -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)
  {




[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux