iommu.lists.linux-foundation.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v4 0/5] iommu aux-domain APIs extensions
@ 2020-09-01  3:34 Lu Baolu
  2020-09-01  3:34 ` [PATCH v4 1/5] iommu: Add optional subdev in aux_at(de)tach ops Lu Baolu
                   ` (4 more replies)
  0 siblings, 5 replies; 12+ messages in thread
From: Lu Baolu @ 2020-09-01  3:34 UTC (permalink / raw)
  To: Joerg Roedel, Alex Williamson
  Cc: Jean-Philippe Brucker, Kevin Tian, Dave Jiang, Ashok Raj, kvm,
	Cornelia Huck, linux-kernel, iommu, Robin Murphy

This series aims to extend the IOMMU aux-domain API set so that it
could be more friendly to vfio/mdev usage. The interactions between
vfio/mdev and iommu during mdev creation and passthr are:

1. Create a group for mdev with iommu_group_alloc();
2. Add the device to the group with

       group = iommu_group_alloc();
       if (IS_ERR(group))
               return PTR_ERR(group);

       ret = iommu_group_add_device(group, &mdev->dev);
       if (!ret)
               dev_info(&mdev->dev, "MDEV: group_id = %d\n",
                        iommu_group_id(group));

3. Allocate an aux-domain with iommu_domain_alloc();
4. Attach the aux-domain to the iommu_group.

       iommu_group_for_each_dev {
               if (iommu_dev_feature_enabled(iommu_device, IOMMU_DEV_FEAT_AUX))
                       return iommu_aux_attach_device(domain, iommu_device);
               else
                       return iommu_attach_device(domain, iommu_device);
        }

   where, iommu_device is the aux-domain-capable device. The mdev's in
   the group are all derived from it.

In the whole process, an iommu group was allocated for the mdev and an
iommu domain was attached to the group, but the group->domain leaves
NULL. As the result, iommu_get_domain_for_dev() (or other similar
interfaces) doesn't work anymore.

The iommu_get_domain_for_dev() is a necessary interface for device
drivers that want to support vfio/mdev based aux-domain. For example,

        unsigned long pasid;
        struct iommu_domain *domain;
        struct device *dev = mdev_dev(mdev);
        struct device *iommu_device = vfio_mdev_get_iommu_device(dev);

        domain = iommu_aux_get_domain_for_dev(dev);
        if (!domain)
                return -ENODEV;

        pasid = iommu_aux_get_pasid(domain, iommu_device);
        if (pasid <= 0)
                return -EINVAL;

         /* Program the device context */
         ....

We tried to address this by extending iommu_aux_at(de)tach_device() so that
the users could pass in an optional device pointer (for example vfio/mdev).
(v2 of this series)

https://lore.kernel.org/linux-iommu/20200707013957.23672-1-baolu.lu@linux.intel.com/

But that will cause a lock issue as group->mutex has been applied in
iommu_group_for_each_dev(), but has to be reapplied again in the
iommu_aux_attach_device().

We also tried to implement an equivalent iommu_attch_group() for groups
which includes subdevices derived from a single physical device. (v3 of
this series)

https://lore.kernel.org/linux-iommu/20200714055703.5510-1-baolu.lu@linux.intel.com/

But that's too harsh (requires that all subdevices in an iommu_group
must be derived from a same physical device) and breaks some generic
concept of iommmu_group.

This version continues to address this by introducing some new APIs into
the aux-domain API set according to comments during v3 reviewing period.

/**
 * 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)

/**
 * 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)

struct iommu_domain *iommu_aux_get_domain_for_dev(struct device *subdev)

This version is evolved according to feedbacks from Robin, Alex and Kevin.
I'm very appreciated to their contributions.

Best regards,
baolu

---
Change log:
 - v1->v2:
   - https://lore.kernel.org/linux-iommu/20200627031532.28046-1-baolu.lu@linux.intel.com/
   - Suggested by Robin.

 - v2->v3:
   - https://lore.kernel.org/linux-iommu/20200707013957.23672-1-baolu.lu@linux.intel.com/
   - Suggested by Alex, Kevin.

 - v3->v4:
   - https://lore.kernel.org/linux-iommu/20200714055703.5510-1-baolu.lu@linux.intel.com/
   - Evolve the aux_attach_group APIs to take an iommu_device lookup
     callback.
   - Add interface to check whether a domain is aux-domain for a device.
   - Return domain only if the domain is aux-domain in
     iommu_aux_get_domain_for_dev().

Lu Baolu (5):
  iommu: Add optional subdev in aux_at(de)tach ops
  iommu: Add iommu_at(de)tach_subdev_group()
  iommu: Add iommu_aux_get_domain_for_dev()
  vfio/type1: Use iommu_aux_at(de)tach_group() APIs
  iommu/vt-d: Add is_aux_domain support

 drivers/iommu/intel/iommu.c     | 135 +++++++++++++++++++--------
 drivers/iommu/iommu.c           | 158 +++++++++++++++++++++++++++++++-
 drivers/vfio/vfio_iommu_type1.c |  43 ++-------
 include/linux/intel-iommu.h     |  17 ++--
 include/linux/iommu.h           |  46 +++++++++-
 5 files changed, 315 insertions(+), 84 deletions(-)

-- 
2.17.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v4 1/5] iommu: Add optional subdev in aux_at(de)tach ops
  2020-09-01  3:34 [PATCH v4 0/5] iommu aux-domain APIs extensions Lu Baolu
@ 2020-09-01  3:34 ` Lu Baolu
  2020-09-10 22:05   ` Alex Williamson
  2020-09-01  3:34 ` [PATCH v4 2/5] iommu: Add iommu_at(de)tach_subdev_group() Lu Baolu
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 12+ messages in thread
From: Lu Baolu @ 2020-09-01  3:34 UTC (permalink / raw)
  To: Joerg Roedel, Alex Williamson
  Cc: Jean-Philippe Brucker, Kevin Tian, Dave Jiang, Ashok Raj, kvm,
	Cornelia Huck, linux-kernel, iommu, Robin Murphy

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 <baolu.lu@linux.intel.com>
---
 drivers/iommu/intel/iommu.c | 10 ++++++----
 drivers/iommu/iommu.c       |  4 ++--
 include/linux/iommu.h       |  6 ++++--
 3 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index bce158468abf..3c12fd06856c 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -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);
 }
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 609bd25bf154..38cdfeb887e1 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -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);
 	}
 }
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index fee209efb756..871267104915 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -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);
 
 	struct iommu_sva *(*sva_bind)(struct device *dev, struct mm_struct *mm,
-- 
2.17.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v4 2/5] iommu: Add iommu_at(de)tach_subdev_group()
  2020-09-01  3:34 [PATCH v4 0/5] iommu aux-domain APIs extensions Lu Baolu
  2020-09-01  3:34 ` [PATCH v4 1/5] iommu: Add optional subdev in aux_at(de)tach ops Lu Baolu
@ 2020-09-01  3:34 ` Lu Baolu
  2020-09-10 22:05   ` Alex Williamson
  2020-09-01  3:34 ` [PATCH v4 3/5] iommu: Add iommu_aux_get_domain_for_dev() Lu Baolu
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 12+ messages in thread
From: Lu Baolu @ 2020-09-01  3:34 UTC (permalink / raw)
  To: Joerg Roedel, Alex Williamson
  Cc: Jean-Philippe Brucker, Kevin Tian, Dave Jiang, Ashok Raj, kvm,
	Cornelia Huck, linux-kernel, iommu, Robin Murphy

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@x1.home/
Link: https://lore.kernel.org/linux-iommu/20200730151703.5daf8ad4@x1.home/
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 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;
+
+		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)
 {
-- 
2.17.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v4 3/5] iommu: Add iommu_aux_get_domain_for_dev()
  2020-09-01  3:34 [PATCH v4 0/5] iommu aux-domain APIs extensions Lu Baolu
  2020-09-01  3:34 ` [PATCH v4 1/5] iommu: Add optional subdev in aux_at(de)tach ops Lu Baolu
  2020-09-01  3:34 ` [PATCH v4 2/5] iommu: Add iommu_at(de)tach_subdev_group() Lu Baolu
@ 2020-09-01  3:34 ` Lu Baolu
  2020-09-01  3:34 ` [PATCH v4 4/5] vfio/type1: Use iommu_aux_at(de)tach_group() APIs Lu Baolu
  2020-09-01  3:34 ` [PATCH v4 5/5] iommu/vt-d: Add is_aux_domain support Lu Baolu
  4 siblings, 0 replies; 12+ messages in thread
From: Lu Baolu @ 2020-09-01  3:34 UTC (permalink / raw)
  To: Joerg Roedel, Alex Williamson
  Cc: Jean-Philippe Brucker, Kevin Tian, Dave Jiang, Ashok Raj, kvm,
	Cornelia Huck, linux-kernel, iommu, Robin Murphy

The device driver needs an API to get its aux-domain. A typical usage
scenario is:

        unsigned long pasid;
        struct iommu_domain *domain;
        struct device *dev = mdev_dev(mdev);
        struct device *iommu_device = vfio_mdev_get_iommu_device(dev);

        domain = iommu_aux_get_domain_for_dev(dev);
        if (!domain)
                return -ENODEV;

        pasid = iommu_aux_get_pasid(domain, iommu_device);
        if (pasid <= 0)
                return -EINVAL;

         /* Program the device context */
         ....

This adds an API for such use case.

Suggested-by: Alex Williamson <alex.williamson@redhat.com>
Link: https://lore.kernel.org/linux-iommu/20200708130749.1b1e1421@x1.home/
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/iommu/iommu.c | 18 ++++++++++++++++++
 include/linux/iommu.h | 20 ++++++++++++++++++++
 2 files changed, 38 insertions(+)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index fb21c2ff4861..8fd93a5b8764 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -2893,6 +2893,24 @@ void iommu_detach_subdev_group(struct iommu_domain *domain,
 }
 EXPORT_SYMBOL_GPL(iommu_detach_subdev_group);
 
+struct iommu_domain *iommu_aux_get_domain_for_dev(struct device *subdev)
+{
+	struct iommu_domain *domain = NULL;
+	struct iommu_group *group;
+
+	group = iommu_group_get(subdev);
+	if (!group || !group->domain)
+		return NULL;
+
+	if (domain_is_aux(group->domain, subdev))
+		domain = group->domain;
+
+	iommu_group_put(group);
+
+	return domain;
+}
+EXPORT_SYMBOL_GPL(iommu_aux_get_domain_for_dev);
+
 /**
  * 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 b9df8b510d4f..ea660a887dbf 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -120,6 +120,7 @@ enum iommu_attr {
 	DOMAIN_ATTR_NESTING,	/* two stages of translation */
 	DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE,
 	DOMAIN_ATTR_MAX,
+	DOMAIN_ATTR_IS_AUX,
 };
 
 /* These are the possible reserved region types */
@@ -622,6 +623,12 @@ static inline void dev_iommu_priv_set(struct device *dev, void *priv)
 	dev->iommu->priv = priv;
 }
 
+static inline bool
+domain_is_aux(struct iommu_domain *domain, struct device *subdev)
+{
+	return iommu_domain_get_attr(domain, DOMAIN_ATTR_IS_AUX, subdev);
+}
+
 int iommu_probe_device(struct device *dev);
 void iommu_release_device(struct device *dev);
 
@@ -638,6 +645,7 @@ int iommu_attach_subdev_group(struct iommu_domain *domain,
 void iommu_detach_subdev_group(struct iommu_domain *domain,
 			       struct iommu_group *group,
 			       iommu_device_lookup_t fn);
+struct iommu_domain *iommu_aux_get_domain_for_dev(struct device *subdev);
 
 struct iommu_sva *iommu_sva_bind_device(struct device *dev,
 					struct mm_struct *mm,
@@ -1039,6 +1047,18 @@ iommu_detach_subdev_group(struct iommu_domain *domain, struct iommu_group *group
 {
 }
 
+static inline bool
+domain_is_aux(struct iommu_domain *domain, struct device *subdev)
+{
+	return false;
+}
+
+static inline struct iommu_domain *
+iommu_aux_get_domain_for_dev(struct device *subdev)
+{
+	return NULL;
+}
+
 static inline struct iommu_sva *
 iommu_sva_bind_device(struct device *dev, struct mm_struct *mm, void *drvdata)
 {
-- 
2.17.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v4 4/5] vfio/type1: Use iommu_aux_at(de)tach_group() APIs
  2020-09-01  3:34 [PATCH v4 0/5] iommu aux-domain APIs extensions Lu Baolu
                   ` (2 preceding siblings ...)
  2020-09-01  3:34 ` [PATCH v4 3/5] iommu: Add iommu_aux_get_domain_for_dev() Lu Baolu
@ 2020-09-01  3:34 ` Lu Baolu
  2020-09-01  3:34 ` [PATCH v4 5/5] iommu/vt-d: Add is_aux_domain support Lu Baolu
  4 siblings, 0 replies; 12+ messages in thread
From: Lu Baolu @ 2020-09-01  3:34 UTC (permalink / raw)
  To: Joerg Roedel, Alex Williamson
  Cc: Jean-Philippe Brucker, Kevin Tian, Dave Jiang, Ashok Raj, kvm,
	Cornelia Huck, linux-kernel, iommu, Robin Murphy

Replace iommu_aux_at(de)tach_device() with iommu_at(de)tach_subdev_group().

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/vfio/vfio_iommu_type1.c | 43 +++++----------------------------
 1 file changed, 6 insertions(+), 37 deletions(-)

diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 5e556ac9102a..8d5eb7ce0986 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -1627,45 +1627,13 @@ static struct device *vfio_mdev_get_iommu_device(struct device *dev)
 	return NULL;
 }
 
-static int vfio_mdev_attach_domain(struct device *dev, void *data)
-{
-	struct iommu_domain *domain = data;
-	struct device *iommu_device;
-
-	iommu_device = vfio_mdev_get_iommu_device(dev);
-	if (iommu_device) {
-		if (iommu_dev_feature_enabled(iommu_device, IOMMU_DEV_FEAT_AUX))
-			return iommu_aux_attach_device(domain, iommu_device);
-		else
-			return iommu_attach_device(domain, iommu_device);
-	}
-
-	return -EINVAL;
-}
-
-static int vfio_mdev_detach_domain(struct device *dev, void *data)
-{
-	struct iommu_domain *domain = data;
-	struct device *iommu_device;
-
-	iommu_device = vfio_mdev_get_iommu_device(dev);
-	if (iommu_device) {
-		if (iommu_dev_feature_enabled(iommu_device, IOMMU_DEV_FEAT_AUX))
-			iommu_aux_detach_device(domain, iommu_device);
-		else
-			iommu_detach_device(domain, iommu_device);
-	}
-
-	return 0;
-}
-
 static int vfio_iommu_attach_group(struct vfio_domain *domain,
 				   struct vfio_group *group)
 {
 	if (group->mdev_group)
-		return iommu_group_for_each_dev(group->iommu_group,
-						domain->domain,
-						vfio_mdev_attach_domain);
+		return iommu_attach_subdev_group(domain->domain,
+						 group->iommu_group,
+						 vfio_mdev_get_iommu_device);
 	else
 		return iommu_attach_group(domain->domain, group->iommu_group);
 }
@@ -1674,8 +1642,9 @@ static void vfio_iommu_detach_group(struct vfio_domain *domain,
 				    struct vfio_group *group)
 {
 	if (group->mdev_group)
-		iommu_group_for_each_dev(group->iommu_group, domain->domain,
-					 vfio_mdev_detach_domain);
+		iommu_detach_subdev_group(domain->domain,
+					  group->iommu_group,
+					  vfio_mdev_get_iommu_device);
 	else
 		iommu_detach_group(domain->domain, group->iommu_group);
 }
-- 
2.17.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v4 5/5] iommu/vt-d: Add is_aux_domain support
  2020-09-01  3:34 [PATCH v4 0/5] iommu aux-domain APIs extensions Lu Baolu
                   ` (3 preceding siblings ...)
  2020-09-01  3:34 ` [PATCH v4 4/5] vfio/type1: Use iommu_aux_at(de)tach_group() APIs Lu Baolu
@ 2020-09-01  3:34 ` Lu Baolu
  2020-09-10 22:05   ` Alex Williamson
  4 siblings, 1 reply; 12+ messages in thread
From: Lu Baolu @ 2020-09-01  3:34 UTC (permalink / raw)
  To: Joerg Roedel, Alex Williamson
  Cc: Jean-Philippe Brucker, Kevin Tian, Dave Jiang, Ashok Raj, kvm,
	Cornelia Huck, linux-kernel, iommu, Robin Murphy

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 <baolu.lu@linux.intel.com>
---
 drivers/iommu/intel/iommu.c | 125 ++++++++++++++++++++++++++----------
 include/linux/intel-iommu.h |  17 +++--
 2 files changed, 103 insertions(+), 39 deletions(-)

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 3c12fd06856c..50431c7b2e71 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -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);
 
-	auxiliary_link_device(domain, dev);
+	sinfo->dev = subdev;
+	sinfo->domain = domain;
+	sinfo->pdev = dev;
+	subdev_link_device(domain, dev, sinfo);
 
 	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,
diff --git a/include/linux/intel-iommu.h b/include/linux/intel-iommu.h
index b1ed2f25f7c0..47ba1904c691 100644
--- a/include/linux/intel-iommu.h
+++ b/include/linux/intel-iommu.h
@@ -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 */
 	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 */
-- 
2.17.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v4 2/5] iommu: Add iommu_at(de)tach_subdev_group()
  2020-09-01  3:34 ` [PATCH v4 2/5] iommu: Add iommu_at(de)tach_subdev_group() Lu Baolu
@ 2020-09-10 22:05   ` Alex Williamson
  2020-09-14  3:02     ` Lu Baolu
  0 siblings, 1 reply; 12+ messages in thread
From: Alex Williamson @ 2020-09-10 22:05 UTC (permalink / raw)
  To: Lu Baolu
  Cc: Jean-Philippe Brucker, Kevin Tian, Dave Jiang, Ashok Raj, kvm,
	Cornelia Huck, linux-kernel, iommu, Robin Murphy

On Tue,  1 Sep 2020 11:34:19 +0800
Lu Baolu <baolu.lu@linux.intel.com> 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@x1.home/
> Link: https://lore.kernel.org/linux-iommu/20200730151703.5daf8ad4@x1.home/
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  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,

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)
>  {

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v4 1/5] iommu: Add optional subdev in aux_at(de)tach ops
  2020-09-01  3:34 ` [PATCH v4 1/5] iommu: Add optional subdev in aux_at(de)tach ops Lu Baolu
@ 2020-09-10 22:05   ` Alex Williamson
  2020-09-14  2:48     ` Lu Baolu
  0 siblings, 1 reply; 12+ messages in thread
From: Alex Williamson @ 2020-09-10 22:05 UTC (permalink / raw)
  To: Lu Baolu
  Cc: Jean-Philippe Brucker, Kevin Tian, Dave Jiang, Ashok Raj, kvm,
	Cornelia Huck, linux-kernel, iommu, Robin Murphy

On Tue,  1 Sep 2020 11:34:18 +0800
Lu Baolu <baolu.lu@linux.intel.com> 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 <baolu.lu@linux.intel.com>
> ---
>  drivers/iommu/intel/iommu.c | 10 ++++++----
>  drivers/iommu/iommu.c       |  4 ++--
>  include/linux/iommu.h       |  6 ++++--
>  3 files changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index bce158468abf..3c12fd06856c 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -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);
>  }
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 609bd25bf154..38cdfeb887e1 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -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);
>  	}
>  }
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index fee209efb756..871267104915 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -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);
>  
>  	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

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v4 5/5] iommu/vt-d: Add is_aux_domain support
  2020-09-01  3:34 ` [PATCH v4 5/5] iommu/vt-d: Add is_aux_domain support Lu Baolu
@ 2020-09-10 22:05   ` Alex Williamson
  2020-09-14  5:01     ` Lu Baolu
  0 siblings, 1 reply; 12+ messages in thread
From: Alex Williamson @ 2020-09-10 22:05 UTC (permalink / raw)
  To: Lu Baolu
  Cc: Jean-Philippe Brucker, Kevin Tian, Dave Jiang, Ashok Raj, kvm,
	Cornelia Huck, linux-kernel, iommu, Robin Murphy

On Tue,  1 Sep 2020 11:34:22 +0800
Lu Baolu <baolu.lu@linux.intel.com> 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 <baolu.lu@linux.intel.com>
> ---
>  drivers/iommu/intel/iommu.c | 125 ++++++++++++++++++++++++++----------
>  include/linux/intel-iommu.h |  17 +++--
>  2 files changed, 103 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index 3c12fd06856c..50431c7b2e71 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -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);
>  
> -	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


>  
>  	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,
> diff --git a/include/linux/intel-iommu.h b/include/linux/intel-iommu.h
> index b1ed2f25f7c0..47ba1904c691 100644
> --- a/include/linux/intel-iommu.h
> +++ b/include/linux/intel-iommu.h
> @@ -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 */
>  	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 */

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v4 1/5] iommu: Add optional subdev in aux_at(de)tach ops
  2020-09-10 22:05   ` Alex Williamson
@ 2020-09-14  2:48     ` Lu Baolu
  0 siblings, 0 replies; 12+ messages in thread
From: Lu Baolu @ 2020-09-14  2:48 UTC (permalink / raw)
  To: Alex Williamson
  Cc: Jean-Philippe Brucker, Kevin Tian, Dave Jiang, Ashok Raj, kvm,
	Cornelia Huck, linux-kernel, iommu, Robin Murphy

Hi Alex,

On 9/11/20 6:05 AM, Alex Williamson wrote:
> On Tue,  1 Sep 2020 11:34:18 +0800
> Lu Baolu <baolu.lu@linux.intel.com> 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 <baolu.lu@linux.intel.com>
>> ---
>>   drivers/iommu/intel/iommu.c | 10 ++++++----
>>   drivers/iommu/iommu.c       |  4 ++--
>>   include/linux/iommu.h       |  6 ++++--
>>   3 files changed, 12 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
>> index bce158468abf..3c12fd06856c 100644
>> --- a/drivers/iommu/intel/iommu.c
>> +++ b/drivers/iommu/intel/iommu.c
>> @@ -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);
>>   }
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index 609bd25bf154..38cdfeb887e1 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -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);
>>   	}
>>   }
>> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
>> index fee209efb756..871267104915 100644
>> --- a/include/linux/iommu.h
>> +++ b/include/linux/iommu.h
>> @@ -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);
>>   
>>   	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().

Yes. Agreed. I will add below comments and adjust the function
parameters according to the naming rule.

/*
- * Aux-domain specific attach/detach.
+ * Aux-domain specific interfaces.
   *
   * Only works if iommu_dev_feature_enabled(dev, IOMMU_DEV_FEAT_AUX) 
returns
   * true. Also, as long as domains are attached to a device through this
@@ -2722,6 +2722,14 @@ EXPORT_SYMBOL_GPL(iommu_dev_feature_enabled);
   * (iommu_detach_device() can't fail, so we fail when trying to 
re-attach).
   * This should make us safe against a device being attached to a guest 
as a
   * whole while there are still pasid users on it (aux and sva).
+ *
+ * Some physical devices can be configured to generate several subdevices.
+ * The modern IOMMUs support the identification and isolation of these
+ * subdevices. Hence they could be passed through to users space. VFIO/mdev
+ * provides a generic framework for subdevice passthrough. Below interfaces
+ * are extended to support such use case. Generally, among the 
parameters of
+ * the following aux-domain specific functions, @physdev represents a 
physical
+ * device and @subdev represents a subdevice.
   */

Please help to review.

Best regards,
baolu

> Thanks,
> 
> Alex
> 
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v4 2/5] iommu: Add iommu_at(de)tach_subdev_group()
  2020-09-10 22:05   ` Alex Williamson
@ 2020-09-14  3:02     ` Lu Baolu
  0 siblings, 0 replies; 12+ messages in thread
From: Lu Baolu @ 2020-09-14  3:02 UTC (permalink / raw)
  To: Alex Williamson
  Cc: Jean-Philippe Brucker, Kevin Tian, Dave Jiang, Ashok Raj, kvm,
	Cornelia Huck, linux-kernel, iommu, Robin Murphy

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@linux.intel.com> 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@x1.home/
>> Link: https://lore.kernel.org/linux-iommu/20200730151703.5daf8ad4@x1.home/
>> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
>> ---
>>   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)
>>   {
> 
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v4 5/5] iommu/vt-d: Add is_aux_domain support
  2020-09-10 22:05   ` Alex Williamson
@ 2020-09-14  5:01     ` Lu Baolu
  0 siblings, 0 replies; 12+ messages in thread
From: Lu Baolu @ 2020-09-14  5:01 UTC (permalink / raw)
  To: Alex Williamson
  Cc: Jean-Philippe Brucker, Kevin Tian, Dave Jiang, Ashok Raj, kvm,
	Cornelia Huck, linux-kernel, iommu, Robin Murphy

Hi Alex,

On 9/11/20 6:05 AM, Alex Williamson wrote:
> On Tue,  1 Sep 2020 11:34:22 +0800
> Lu Baolu <baolu.lu@linux.intel.com> 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 <baolu.lu@linux.intel.com>
>> ---
>>   drivers/iommu/intel/iommu.c | 125 ++++++++++++++++++++++++++----------
>>   include/linux/intel-iommu.h |  17 +++--
>>   2 files changed, 103 insertions(+), 39 deletions(-)
>>
>> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
>> index 3c12fd06856c..50431c7b2e71 100644
>> --- a/drivers/iommu/intel/iommu.c
>> +++ b/drivers/iommu/intel/iommu.c
>> @@ -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);
>>   
>> -	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,

You are right. I should make the link and unlink helpers symmetric. I
will move free() out to the caller.

Best regards,
baolu

> 
> Alex
> 
> 
>>   
>>   	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,
>> diff --git a/include/linux/intel-iommu.h b/include/linux/intel-iommu.h
>> index b1ed2f25f7c0..47ba1904c691 100644
>> --- a/include/linux/intel-iommu.h
>> +++ b/include/linux/intel-iommu.h
>> @@ -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 */
>>   	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 */
> 
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2020-09-14  5:07 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-01  3:34 [PATCH v4 0/5] iommu aux-domain APIs extensions Lu Baolu
2020-09-01  3:34 ` [PATCH v4 1/5] iommu: Add optional subdev in aux_at(de)tach ops Lu Baolu
2020-09-10 22:05   ` Alex Williamson
2020-09-14  2:48     ` Lu Baolu
2020-09-01  3:34 ` [PATCH v4 2/5] iommu: Add iommu_at(de)tach_subdev_group() Lu Baolu
2020-09-10 22:05   ` Alex Williamson
2020-09-14  3:02     ` Lu Baolu
2020-09-01  3:34 ` [PATCH v4 3/5] iommu: Add iommu_aux_get_domain_for_dev() Lu Baolu
2020-09-01  3:34 ` [PATCH v4 4/5] vfio/type1: Use iommu_aux_at(de)tach_group() APIs Lu Baolu
2020-09-01  3:34 ` [PATCH v4 5/5] iommu/vt-d: Add is_aux_domain support Lu Baolu
2020-09-10 22:05   ` Alex Williamson
2020-09-14  5:01     ` Lu Baolu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).