[PATCH 0/4] iommu: Add support to change default domain of a group

[PATCH 0/4] iommu: Add support to change default domain of a group

[Sai Praneeth Prakhya]

Presently, the default domain of a group is allocated during boot time and it
cannot be changed later. So, the device would typically be either in identity
(pass_through) mode or the device would be in DMA mode as long as the
system is up and running. There is no way to change the default domain type
dynamically i.e. after booting, a device cannot switch between identity mode and
DMA mode.

Assume a use case where-in the priviliged user would want to use the device in
pass-through mode when the device is used for host so that it would be high
performing. Presently, this is not supported and hence add support to change the
default domain of a group dynamically.

Support this through a sysfs file, namely "/sys/kernel/iommu_groups/<grp_id>/type".

Changes from RFC:
-----------------
1. Added support for "auto" type, so that kernel selects one among identity or
   dma mode.
2. Use "system_state" in device_def_domain_type() instead of an argument.

Testing:
--------
Tested by dynamically changing network device from
1. identity mode to dma and making sure ping works
2. dma mode to identity mode and making sure ping works
Tested only for intel_iommu/vt-d. Haven't tested on other architectures.

Sai Praneeth Prakhya (4):
  iommu/vt-d: Modify device_def_domain_type() to use at runtime
  iommu: Add device_def_domain_type() call back function to iommu_ops
  iommu: Add support to change default domain of an iommu_group
  iommu: Document usage of "/sys/kernel/iommu_groups/<grp_id>/type" file

 .../ABI/testing/sysfs-kernel-iommu_groups     |  35 +++
 drivers/iommu/intel-iommu.c                   |  44 +++-
 drivers/iommu/iommu.c                         | 232 +++++++++++++++++-
 include/linux/iommu.h                         |   4 +
 4 files changed, 301 insertions(+), 14 deletions(-)

Cc: Christoph Hellwig <hch@lst.de>
Cc: Joerg Roedel <joro@8bytes.org>
Cc: Ashok Raj <ashok.raj@intel.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Lu Baolu <baolu.lu@linux.intel.com>
Cc: Sohil Mehta <sohil.mehta@intel.com>
Cc: Robin Murphy <robin.murphy@arm.com>
Cc: Jacob Pan <jacob.jun.pan@linux.intel.com>
Signed-off-by: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>

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

[PATCH 1/4] iommu/vt-d: Modify device_def_domain_type() to use at runtime

[Sai Praneeth Prakhya]

device_def_domain_type() determines the domain type a device could have and
currently it's called only during boot time. Since the function is called
only during boot time, it *always* considers command line arguments like
"intel_iommu=igfx_off" and "iommu=pt" to determine the domain type of a
device. To support changing the domain of an iommu_group through sysfs,
this function has to be called at runtime as well. Hence, modify this
function such that command line arguments are considered to determine the
domain type of a device *only* when the system is booting and ignored if
the system is already up and running.

Cc: Christoph Hellwig <hch@lst.de>
Cc: Joerg Roedel <joro@8bytes.org>
Cc: Ashok Raj <ashok.raj@intel.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Lu Baolu <baolu.lu@linux.intel.com>
Cc: Sohil Mehta <sohil.mehta@intel.com>
Cc: Robin Murphy <robin.murphy@arm.com>
Cc: Jacob Pan <jacob.jun.pan@linux.intel.com>
Signed-off-by: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>
---
 drivers/iommu/intel-iommu.c | 35 +++++++++++++++++++++++++----------
 1 file changed, 25 insertions(+), 10 deletions(-)

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index b7454ca4a87c..27c3c893530a 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -2859,15 +2859,14 @@ static bool device_is_rmrr_locked(struct device *dev)
 }
 
 /*
- * Return the required default domain type for a specific device.
+ * Returns the possible default domain types a device could have.
  *
- * @dev: the device in query
- * @startup: true if this is during early boot
+ * @dev: The device in query
  *
  * Returns:
- *  - IOMMU_DOMAIN_DMA: device requires a dynamic mapping domain
- *  - IOMMU_DOMAIN_IDENTITY: device requires an identical mapping domain
- *  - 0: both identity and dynamic domains work for this device
+ *  - IOMMU_DOMAIN_DMA: Device should always be in dynamic mapping domain
+ *  - IOMMU_DOMAIN_IDENTITY: Device should always be in identity mapping domain
+ *  - 0: Device could be in any domain (i.e. identity or dynamic)
  */
 static int device_def_domain_type(struct device *dev)
 {
@@ -2884,11 +2883,22 @@ static int device_def_domain_type(struct device *dev)
 		if (pdev->untrusted)
 			return IOMMU_DOMAIN_DMA;
 
+		/*
+		 * Azalia device should always be in identity domain if
+		 * check_tylersburg_isoch() decides so.
+		 */
 		if ((iommu_identity_mapping & IDENTMAP_AZALIA) && IS_AZALIA(pdev))
 			return IOMMU_DOMAIN_IDENTITY;
 
-		if ((iommu_identity_mapping & IDENTMAP_GFX) && IS_GFX_DEVICE(pdev))
-			return IOMMU_DOMAIN_IDENTITY;
+		/*
+		 * intel_iommu=igfx_off should have it's effect only during
+		 * boot.
+		 */
+		if (system_state < SYSTEM_RUNNING) {
+			if ((iommu_identity_mapping & IDENTMAP_GFX) &&
+			    IS_GFX_DEVICE(pdev))
+				return IOMMU_DOMAIN_IDENTITY;
+		}
 
 		/*
 		 * We want to start off with all devices in the 1:1 domain, and
@@ -2919,8 +2929,13 @@ static int device_def_domain_type(struct device *dev)
 			return IOMMU_DOMAIN_DMA;
 	}
 
-	return (iommu_identity_mapping & IDENTMAP_ALL) ?
-			IOMMU_DOMAIN_IDENTITY : 0;
+	/* iommu=pt should have it's effect only during boot */
+	if (system_state < SYSTEM_RUNNING) {
+		if (iommu_identity_mapping & IDENTMAP_ALL)
+			return IOMMU_DOMAIN_IDENTITY;
+	}
+
+	return 0;
 }
 
 static void intel_iommu_init_qi(struct intel_iommu *iommu)
-- 
2.19.1

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

[PATCH 2/4] iommu: Add device_def_domain_type() call back function to iommu_ops

[Sai Praneeth Prakhya]

When user requests kernel to change the default domain type of a group
through sysfs, kernel has to make sure that it's ok to change the domain
type of every device in the group to the requested domain (every device may
not support both the domain types i.e. DMA and identity). Hence, add a call
back function that could be implemented per architecture that performs the
above check.

For intel_iommu, this is already done by device_def_domain_type(), but
every call back function in intel_iommu_ops starts with intel_iommu prefix,
hence modify device_def_domain_type() so that it follows the same
semantics.

Cc: Christoph Hellwig <hch@lst.de>
Cc: Joerg Roedel <joro@8bytes.org>
Cc: Ashok Raj <ashok.raj@intel.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Lu Baolu <baolu.lu@linux.intel.com>
Cc: Sohil Mehta <sohil.mehta@intel.com>
Cc: Robin Murphy <robin.murphy@arm.com>
Cc: Jacob Pan <jacob.jun.pan@linux.intel.com>
Signed-off-by: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>
---
 drivers/iommu/intel-iommu.c | 9 ++++++---
 include/linux/iommu.h       | 4 ++++
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 27c3c893530a..3e8655197aad 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -2868,7 +2868,7 @@ static bool device_is_rmrr_locked(struct device *dev)
  *  - IOMMU_DOMAIN_IDENTITY: Device should always be in identity mapping domain
  *  - 0: Device could be in any domain (i.e. identity or dynamic)
  */
-static int device_def_domain_type(struct device *dev)
+static int intel_iommu_device_def_domain_type(struct device *dev)
 {
 	if (dev_is_pci(dev)) {
 		struct pci_dev *pdev = to_pci_dev(dev);
@@ -5297,7 +5297,8 @@ static int intel_iommu_add_device(struct device *dev)
 	domain = iommu_get_domain_for_dev(dev);
 	dmar_domain = to_dmar_domain(domain);
 	if (domain->type == IOMMU_DOMAIN_DMA) {
-		if (device_def_domain_type(dev) == IOMMU_DOMAIN_IDENTITY) {
+		if (intel_iommu_device_def_domain_type(dev) ==
+		    IOMMU_DOMAIN_IDENTITY) {
 			ret = iommu_request_dm_for_dev(dev);
 			if (ret) {
 				dmar_remove_one_dev_info(dev);
@@ -5308,7 +5309,8 @@ static int intel_iommu_add_device(struct device *dev)
 			}
 		}
 	} else {
-		if (device_def_domain_type(dev) == IOMMU_DOMAIN_DMA) {
+		if (intel_iommu_device_def_domain_type(dev) ==
+		    IOMMU_DOMAIN_DMA) {
 			ret = iommu_request_dma_domain_for_dev(dev);
 			if (ret) {
 				dmar_remove_one_dev_info(dev);
@@ -5652,6 +5654,7 @@ const struct iommu_ops intel_iommu_ops = {
 	.dev_enable_feat	= intel_iommu_dev_enable_feat,
 	.dev_disable_feat	= intel_iommu_dev_disable_feat,
 	.is_attach_deferred	= intel_iommu_is_attach_deferred,
+	.device_def_domain_type	= intel_iommu_device_def_domain_type,
 	.pgsize_bitmap		= INTEL_IOMMU_PGSIZES,
 };
 
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 64ebaff33455..ae4755ad2937 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -244,6 +244,8 @@ struct iommu_iotlb_gather {
  * @sva_unbind: Unbind process address space from device
  * @sva_get_pasid: Get PASID associated to a SVA handle
  * @page_response: handle page request response
+ * @device_def_domain_type: Return the possible default domain types a device
+ * 			    could have.
  * @pgsize_bitmap: bitmap of all possible supported page sizes
  */
 struct iommu_ops {
@@ -307,6 +309,8 @@ struct iommu_ops {
 			     struct iommu_fault_event *evt,
 			     struct iommu_page_response *msg);
 
+	int (*device_def_domain_type)(struct device *dev);
+
 	unsigned long pgsize_bitmap;
 };
 
-- 
2.19.1

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

[PATCH 3/4] iommu: Add support to change default domain of an iommu_group

[Sai Praneeth Prakhya]

Presently, the default domain of an iommu_group is allocated during boot
time (i.e. when a device is being added to a group) and it cannot be
changed later. So, the device would typically be either in identity (also
known as pass_through) mode (controlled by "iommu=pt" kernel command line
argument) or the device would be in DMA mode as long as the machine is up
and running. There is no way to change the default domain type dynamically
i.e. after booting, a device cannot switch between identity mode and DMA
mode.

But, assume a use case where in the user trusts the device and also
believes that his OS is secure enough and hence wants *only* this device to
bypass IOMMU (so that it could be high performing) whereas all the other
devices to go through IOMMU (so that the system is protected). Presently,
this is not supported and hence it will be helpful if there is some way to
change the default domain of a B:D.F dynamically. Since, linux iommu
subsystem prefers to deal at iommu_group level instead of B:D.F level, it
might be helpful if there is some way to change the default domain of a
*iommu_group* dynamically. Hence, add such support.

A privileged user could request the kernel to change the default domain
type of a group by writing to "/sys/kernel/iommu_groups/<grp_id>/type"
file. Presently, only three values are supported
1. identity: all the DMA transactions from the devices in this group are
	     *not* translated by the iommu
2. dma: all the DMA transactions from the devices in this group are
	translated by the iommu
3. auto: kernel decides one among dma/identity

Also please note that a group type could be modified only when *all*
the devices in the group are not binded to any device driver. Please see
"Documentation/ABI/testing/sysfs-kernel-iommu_groups" for more information.

Cc: Christoph Hellwig <hch@lst.de>
Cc: Joerg Roedel <joro@8bytes.org>
Cc: Ashok Raj <ashok.raj@intel.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Lu Baolu <baolu.lu@linux.intel.com>
Cc: Sohil Mehta <sohil.mehta@intel.com>
Cc: Robin Murphy <robin.murphy@arm.com>
Cc: Jacob Pan <jacob.jun.pan@linux.intel.com>
Signed-off-by: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>
---
 drivers/iommu/iommu.c | 232 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 231 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 70bfbcc09248..f4a5981e570b 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -155,6 +155,8 @@ static int __iommu_attach_group(struct iommu_domain *domain,
 				struct iommu_group *group);
 static void __iommu_detach_group(struct iommu_domain *domain,
 				 struct iommu_group *group);
+static ssize_t iommu_group_store_type(struct iommu_group *group,
+				      const char *buf, size_t count);
 
 static int __init iommu_set_def_domain_type(char *str)
 {
@@ -376,7 +378,8 @@ static IOMMU_GROUP_ATTR(name, S_IRUGO, iommu_group_show_name, NULL);
 static IOMMU_GROUP_ATTR(reserved_regions, 0444,
 			iommu_group_show_resv_regions, NULL);
 
-static IOMMU_GROUP_ATTR(type, 0444, iommu_group_show_type, NULL);
+static IOMMU_GROUP_ATTR(type, 0644, iommu_group_show_type,
+			iommu_group_store_type);
 
 static void iommu_group_release(struct kobject *kobj)
 {
@@ -2470,3 +2473,230 @@ int iommu_sva_get_pasid(struct iommu_sva *handle)
 	return ops->sva_get_pasid(handle);
 }
 EXPORT_SYMBOL_GPL(iommu_sva_get_pasid);
+
+/*
+ * Changes the default domain of a group. This function is heavily inspired from
+ * request_default_domain_for_dev() and couldn't re-use the same because:
+ * 1. The domain should be changed even if there are devices under this group
+ *    because the driver is already unbinded and it's safe to do so. Also, note
+ *    that *only* default_domain is being changed and hence the devices list in
+ *    the group need not be changed.
+ * 2. Unlike request_default_domain_for_dev(), a domain is allocated only once
+ *    for the whole group, where as the former allocates a domain per device.
+ *
+ * @group: The group for which the default domain should be changed
+ * @prev_domain: The previous domain that is being switched from
+ * @type: The type of the new default domain that gets associated with the group
+ *
+ * Returns 0 on success and error code on failure
+ *
+ * Note:
+ * 1. Presently, this function is called only when user requests to change the
+ *    group's default domain type through /sys/kernel/iommu_groups/<grp_id>/type
+ *    Be aware to take a closer look if intended to use for other cases.
+ * 2. Assumes group->mutex is already taken
+ */
+static int iommu_group_change_def_domain(struct iommu_group *group,
+					 struct iommu_domain *prev_domain,
+					 int type)
+{
+	struct group_device *grp_dev;
+	struct iommu_domain *new_domain;
+	int ret = 0;
+
+	/*
+	 * iommu_domain_alloc() takes "struct bus_type" as an argument which is
+	 * a member in "struct device". Changing a group's default domain type
+	 * deals at iommu_group level rather than device level and hence there
+	 * is no straight forward way to get "bus_type" of an iommu_group that
+	 * could be passed to iommu_domain_alloc(). So, instead of directly
+	 * calling iommu_domain_alloc(), use iommu_ops from previous default
+	 * domain.
+	 */
+	if (!prev_domain || !prev_domain->ops ||
+	    !prev_domain->ops->domain_alloc || !type)
+		return -EINVAL;
+
+	/* Allocate a new domain of requested type */
+	new_domain = prev_domain->ops->domain_alloc(type);
+	if (!new_domain) {
+		pr_err("Unable to allocate memory for the new domain\n");
+		return -ENOMEM;
+	}
+
+	new_domain->type = type;
+	new_domain->ops = prev_domain->ops;
+	new_domain->pgsize_bitmap = prev_domain->pgsize_bitmap;
+
+	/* Attach all the devices in the group to the newly created domain */
+	ret = __iommu_attach_group(new_domain, group);
+	if (ret) {
+		pr_err("Unable to attach all the devices in the group to the new domain\n");
+		goto free_new_domain;
+	}
+
+	/*
+	 * Map reserved regions if the group's default domain is being changed
+	 * from identity domain to dma domain
+	 */
+	if (type == IOMMU_DOMAIN_IDENTITY)
+		goto free_prev_domain;
+
+	list_for_each_entry(grp_dev, &group->devices, list) {
+		ret = iommu_group_create_direct_mappings(group, grp_dev->dev);
+		if (ret) {
+			dev_err(grp_dev->dev, "Failed to create direct mappings for reserved regions\n");
+			goto free_new_domain;
+		}
+	}
+
+free_prev_domain:
+	/*
+	 * Free the existing default domain and replace it with the newly
+	 * created default domain. No need to set group->domain because
+	 * __iommu_attach_group() already does it on success.
+	 */
+	iommu_domain_free(prev_domain);
+	group->default_domain = new_domain;
+	return 0;
+
+free_new_domain:
+	iommu_domain_free(new_domain);
+	return ret;
+}
+
+static int is_driver_binded(struct device *dev, void *not_used)
+{
+	int ret = 0;
+
+	device_lock(dev);
+	if (device_is_bound(dev))
+		ret = 1;
+	device_unlock(dev);
+	return ret;
+}
+
+static ssize_t iommu_group_store_type(struct iommu_group *group,
+				      const char *buf, size_t count)
+{
+	int ret, req_type = 0, req_auto = 0;
+	struct iommu_domain *prev_domain;
+	struct group_device *grp_dev;
+	const struct iommu_ops *ops;
+	struct device *dev;
+
+	if (!capable(CAP_SYS_ADMIN) || !capable(CAP_SYS_RAWIO))
+		return -EACCES;
+
+	if (WARN_ON(!group))
+		return -EINVAL;
+
+	if (sysfs_streq(buf, "identity"))
+		req_type = IOMMU_DOMAIN_IDENTITY;
+	else if (sysfs_streq(buf, "dma"))
+		req_type = IOMMU_DOMAIN_DMA;
+	else if (sysfs_streq(buf, "auto"))
+		req_auto = 1;
+	else
+		return -EINVAL;
+
+	/* Check if any device in the group still has a driver binded to it */
+	if (iommu_group_for_each_dev(group, NULL, is_driver_binded)) {
+		pr_err("Active drivers exist for devices in the group\n");
+		return -EBUSY;
+	}
+
+	mutex_lock(&group->mutex);
+	prev_domain = group->default_domain;
+	if (!prev_domain || !prev_domain->ops ||
+	    !prev_domain->ops->device_def_domain_type) {
+		pr_err("'device_def_domain_type' call back isn't registered\n");
+		ret = -EINVAL;
+		goto out;
+	}
+
+	/*
+	 * If user has requested "auto", kernel has to decide the default domain
+	 * type of a group. Hence, find out individual preferences of a device.
+	 */
+	ops = prev_domain->ops;
+	if (req_auto) {
+		int dma_devs = 0, idt_devs = 0, dev_def_dom;
+
+		list_for_each_entry(grp_dev, &group->devices, list) {
+			dev = grp_dev->dev;
+			dev_def_dom = ops->device_def_domain_type(dev);
+			if (dev_def_dom == IOMMU_DOMAIN_IDENTITY)
+				idt_devs++;
+			if (dev_def_dom == IOMMU_DOMAIN_DMA)
+				dma_devs++;
+		}
+
+		/*
+		 * Default domain type of a group is undefined if some devices
+		 * in the group should be in dma domain while other devices
+		 * should be in identity domain
+		 */
+		if (idt_devs && dma_devs) {
+			pr_err("Some devices need identity domain while other need dma domain\n");
+			ret = -EINVAL;
+			goto out;
+		}
+
+		/*
+		 * Default domain type of a group is identity if
+		 * 1. All the devices in the group need to be in identity domain
+		 * 2. Some devices should be in identity domain while other
+		 *    devices could be in either of dma or identity domain
+		 */
+		if (idt_devs && !dma_devs)
+			req_type = IOMMU_DOMAIN_IDENTITY;
+
+		/*
+		 * Default domain type of a group is dma if
+		 * 1. All the devices in the group need to be in dma domain
+		 * 2. Some devices should be in dma domain while other devices
+		 *    could be in either of dma or identity domain
+		 * 3. All the devices could be in either of the domains (namely
+		 *    dma and identity)
+		 */
+		if (!idt_devs)
+			req_type = IOMMU_DOMAIN_DMA;
+	}
+
+	/*
+	 * Switch to a new domain only if the requested domain type is different
+	 * from the existing default domain type
+	 */
+	if (prev_domain->type == req_type) {
+		ret = count;
+		goto out;
+	}
+
+	/*
+	 * Every device may not support both the domain types (namely DMA and
+	 * identity), so check if it's ok to change domain type of every device
+	 * in the group to the requested domain. This check is not required if
+	 * user has requested "auto" because it's already done above implicitly.
+	 */
+	if (!req_auto) {
+		list_for_each_entry(grp_dev, &group->devices, list) {
+			int allowed_types;
+
+			dev = grp_dev->dev;
+			allowed_types = ops->device_def_domain_type(dev);
+			if (allowed_types && allowed_types != req_type) {
+				dev_err(dev, "Cannot be in %s domain\n", buf);
+				ret = -EINVAL;
+				goto out;
+			}
+		}
+	}
+
+	ret = iommu_group_change_def_domain(group, prev_domain, req_type);
+	if (!ret)
+		ret = count;
+out:
+	mutex_unlock(&group->mutex);
+	return ret;
+}
-- 
2.19.1

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

Re: [PATCH 3/4] iommu: Add support to change default domain of an iommu_group

[Joerg Roedel]

On Tue, Aug 20, 2019 at 07:42:25PM -0700, Sai Praneeth Prakhya wrote:
> +	/*
> +	 * iommu_domain_alloc() takes "struct bus_type" as an argument which is
> +	 * a member in "struct device". Changing a group's default domain type
> +	 * deals at iommu_group level rather than device level and hence there
> +	 * is no straight forward way to get "bus_type" of an iommu_group that
> +	 * could be passed to iommu_domain_alloc(). So, instead of directly
> +	 * calling iommu_domain_alloc(), use iommu_ops from previous default
> +	 * domain.
> +	 */
> +	if (!prev_domain || !prev_domain->ops ||
> +	    !prev_domain->ops->domain_alloc || !type)
> +		return -EINVAL;

Hmm, this isn't really nice and clean, but I understand why you need it.
I will think about a better way to get iommu_ops here.

> +free_prev_domain:
> +	/*
> +	 * Free the existing default domain and replace it with the newly
> +	 * created default domain. No need to set group->domain because
> +	 * __iommu_attach_group() already does it on success.
> +	 */
> +	iommu_domain_free(prev_domain);
> +	group->default_domain = new_domain;
> +	return 0;

It isn't obvious to me from this patch, how to are the dma_ops updated
when the default domain changes between identity and dma?

> +	/* Check if any device in the group still has a driver binded to it */
> +	if (iommu_group_for_each_dev(group, NULL, is_driver_binded)) {
> +		pr_err("Active drivers exist for devices in the group\n");
> +		return -EBUSY;
> +	}

This is racy with device driver probing code. Unfortunatly there is no
clean way out of that either, locking all devices in the group and then
do the re-attach will introduce a lock-inversion with group->mutex. But
please put a comment here saying that this might race with device driver
probing.


Regards,

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

RE: [PATCH 3/4] iommu: Add support to change default domain of an iommu_group

[Prakhya, Sai Praneeth]

Hi Joerg,

Thanks a lot! for the review. I highly appreciate for sparing your time to review the patch :)

> On Tue, Aug 20, 2019 at 07:42:25PM -0700, Sai Praneeth Prakhya wrote:
> > +	/*
> > +	 * iommu_domain_alloc() takes "struct bus_type" as an argument which
> is
> > +	 * a member in "struct device". Changing a group's default domain type
> > +	 * deals at iommu_group level rather than device level and hence there
> > +	 * is no straight forward way to get "bus_type" of an iommu_group that
> > +	 * could be passed to iommu_domain_alloc(). So, instead of directly
> > +	 * calling iommu_domain_alloc(), use iommu_ops from previous default
> > +	 * domain.
> > +	 */
> > +	if (!prev_domain || !prev_domain->ops ||
> > +	    !prev_domain->ops->domain_alloc || !type)
> > +		return -EINVAL;
> 
> Hmm, this isn't really nice and clean, but I understand why you need it.

I agree.. It didn't look good for me either :(
But, I didn't find any other better solution. 

> I will think about a better way to get iommu_ops here.

Sure! That will be great!

> > +free_prev_domain:
> > +	/*
> > +	 * Free the existing default domain and replace it with the newly
> > +	 * created default domain. No need to set group->domain because
> > +	 * __iommu_attach_group() already does it on success.
> > +	 */
> > +	iommu_domain_free(prev_domain);
> > +	group->default_domain = new_domain;
> > +	return 0;
> 
> It isn't obvious to me from this patch, how to are the dma_ops updated when
> the default domain changes between identity and dma?

Thanks for raising this.
For intel_iommu, dma_map_ops is defined at drivers/iommu/intel-iommu.c and
all the callbacks like alloc(), map_sg() and map_page(), check if the device needs DMA mapping or not 
by calling iommu_need_mapping(). The callbacks inherently do the right thing based on the outcome.
So, essentially the dma_ops are same for dma/identity domain.

I just realized (sorry!) that other iommu drivers (Eg: AMD) doesn't do it the same way i.e. looks like the callbacks 
aren't checking if the device needs a dma mapping or identity mapping.
I will take a look at other iommu drivers and will handle this in V2.

Please let me know if I missed something.

> > +	/* Check if any device in the group still has a driver binded to it */
> > +	if (iommu_group_for_each_dev(group, NULL, is_driver_binded)) {
> > +		pr_err("Active drivers exist for devices in the group\n");
> > +		return -EBUSY;
> > +	}
> 
> This is racy with device driver probing code. Unfortunatly there is no clean way
> out of that either, locking all devices in the group and then do the re-attach will
> introduce a lock-inversion with group->mutex. But please put a comment here
> saying that this might race with device driver probing.

Sure! Makes sense. Will add it in V2.

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

Re: [PATCH 3/4] iommu: Add support to change default domain of an iommu_group

[Lu Baolu]

Hi,

On 9/4/19 11:09 AM, Prakhya, Sai Praneeth wrote:
> Hi Joerg,
> 
> Thanks a lot! for the review. I highly appreciate for sparing your time to review the patch :)
> 
>> On Tue, Aug 20, 2019 at 07:42:25PM -0700, Sai Praneeth Prakhya wrote:
>>> +	/*
>>> +	 * iommu_domain_alloc() takes "struct bus_type" as an argument which
>> is
>>> +	 * a member in "struct device". Changing a group's default domain type
>>> +	 * deals at iommu_group level rather than device level and hence there
>>> +	 * is no straight forward way to get "bus_type" of an iommu_group that
>>> +	 * could be passed to iommu_domain_alloc(). So, instead of directly
>>> +	 * calling iommu_domain_alloc(), use iommu_ops from previous default
>>> +	 * domain.
>>> +	 */
>>> +	if (!prev_domain || !prev_domain->ops ||
>>> +	    !prev_domain->ops->domain_alloc || !type)
>>> +		return -EINVAL;
>>
>> Hmm, this isn't really nice and clean, but I understand why you need it.
> 
> I agree.. It didn't look good for me either :(
> But, I didn't find any other better solution.
> 
>> I will think about a better way to get iommu_ops here.
> 
> Sure! That will be great!
> 
>>> +free_prev_domain:
>>> +	/*
>>> +	 * Free the existing default domain and replace it with the newly
>>> +	 * created default domain. No need to set group->domain because
>>> +	 * __iommu_attach_group() already does it on success.
>>> +	 */
>>> +	iommu_domain_free(prev_domain);
>>> +	group->default_domain = new_domain;
>>> +	return 0;
>>
>> It isn't obvious to me from this patch, how to are the dma_ops updated when
>> the default domain changes between identity and dma?
> 
> Thanks for raising this.
> For intel_iommu, dma_map_ops is defined at drivers/iommu/intel-iommu.c and
> all the callbacks like alloc(), map_sg() and map_page(), check if the device needs DMA mapping or not
> by calling iommu_need_mapping(). The callbacks inherently do the right thing based on the outcome.
> So, essentially the dma_ops are same for dma/identity domain.

This isn't always true as we are considering per-device dma ops.

Best regards,
Baolu

> 
> I just realized (sorry!) that other iommu drivers (Eg: AMD) doesn't do it the same way i.e. looks like the callbacks
> aren't checking if the device needs a dma mapping or identity mapping.
> I will take a look at other iommu drivers and will handle this in V2.
> 
> Please let me know if I missed something.
> 
>>> +	/* Check if any device in the group still has a driver binded to it */
>>> +	if (iommu_group_for_each_dev(group, NULL, is_driver_binded)) {
>>> +		pr_err("Active drivers exist for devices in the group\n");
>>> +		return -EBUSY;
>>> +	}
>>
>> This is racy with device driver probing code. Unfortunatly there is no clean way
>> out of that either, locking all devices in the group and then do the re-attach will
>> introduce a lock-inversion with group->mutex. But please put a comment here
>> saying that this might race with device driver probing.
> 
> Sure! Makes sense. Will add it in V2.
> 
> Regards,
> Sai
> 
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH 3/4] iommu: Add support to change default domain of an iommu_group

[Prakhya, Sai Praneeth]

> >>> +free_prev_domain:
> >>> +	/*
> >>> +	 * Free the existing default domain and replace it with the newly
> >>> +	 * created default domain. No need to set group->domain because
> >>> +	 * __iommu_attach_group() already does it on success.
> >>> +	 */
> >>> +	iommu_domain_free(prev_domain);
> >>> +	group->default_domain = new_domain;
> >>> +	return 0;
> >>
> >> It isn't obvious to me from this patch, how to are the dma_ops
> >> updated when the default domain changes between identity and dma?
> >
> > Thanks for raising this.
> > For intel_iommu, dma_map_ops is defined at drivers/iommu/intel-iommu.c
> > and all the callbacks like alloc(), map_sg() and map_page(), check if
> > the device needs DMA mapping or not by calling iommu_need_mapping(). The
> callbacks inherently do the right thing based on the outcome.
> > So, essentially the dma_ops are same for dma/identity domain.
> 
> This isn't always true as we are considering per-device dma ops.

Ahh.. I see. I wasn't aware that per-device dma ops might change this.
Thanks for letting me know. I will take this into consideration as well for V2.

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

[PATCH 4/4] iommu: Document usage of "/sys/kernel/iommu_groups/<grp_id>/type" file

[Sai Praneeth Prakhya]

The default domain type of an iommu group can be changed using file
"/sys/kernel/iommu_groups/<grp_id>/type". Hence, document it's usage and
more importantly spell out it's limitations and an example.

Cc: Christoph Hellwig <hch@lst.de>
Cc: Joerg Roedel <joro@8bytes.org>
Cc: Ashok Raj <ashok.raj@intel.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Lu Baolu <baolu.lu@linux.intel.com>
Cc: Sohil Mehta <sohil.mehta@intel.com>
Cc: Robin Murphy <robin.murphy@arm.com>
Cc: Jacob Pan <jacob.jun.pan@linux.intel.com>
Signed-off-by: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>
---
 .../ABI/testing/sysfs-kernel-iommu_groups     | 35 +++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-kernel-iommu_groups b/Documentation/ABI/testing/sysfs-kernel-iommu_groups
index 017f5bc3920c..0a70b3a66ff3 100644
--- a/Documentation/ABI/testing/sysfs-kernel-iommu_groups
+++ b/Documentation/ABI/testing/sysfs-kernel-iommu_groups
@@ -33,3 +33,38 @@ Description:    In case an RMRR is used only by graphics or USB devices
 		it is now exposed as "direct-relaxable" instead of "direct".
 		In device assignment use case, for instance, those RMRR
 		are considered to be relaxable and safe.
+
+What:		/sys/kernel/iommu_groups/<grp_id>/type
+Date:		August 2019
+KernelVersion:	v5.4
+Contact:	Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>
+Description:	/sys/kernel/iommu_groups/<grp_id>/type lets the user know the
+		type of default domain in use by iommu for this group. A
+		privileged user could request kernel to change the group type by
+		writing to this file. Presently, only three types are supported
+		1. dma: All the DMA transactions from the devices in this group
+			are translated by the iommu.
+		2. identity: All the DMA transactions from the devices in this
+			     group are *not* translated by the iommu.
+		3. auto: Kernel decides one among dma/identity
+		Note:
+		-----
+		A group type could be modified only when *all* the devices in
+		the group are not binded to any device driver. So, the user has
+		to first unbind the appropriate drivers and then change the
+		default domain type.
+		Caution:
+		--------
+		Unbinding a device driver will take away the drivers control
+		over the device and if done on devices that host root file
+		system could lead to catastrophic effects (the user might
+		need to reboot the machine to get it to normal state). So, it's
+		expected that the user understands what he is doing.
+		Example:
+		--------
+		# Unbind USB device driver
+		1. echo "0000:00:14.0" > /sys/bus/pci/drivers/xhci_hcd/unbind
+		# Put the USB device in identity mode (a.k.a pass through)
+		2. echo "identity" > /sys/kernel/iommu_groups/<grp_id>/type
+		# Re-bind the driver
+		3. echo "0000:00:14.0" > /sys/bus/pci/drivers/xhci_hcd/bind
-- 
2.19.1

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

Re: [PATCH 4/4] iommu: Document usage of "/sys/kernel/iommu_groups/<grp_id>/type" file

[John Garry]

On 21/08/2019 03:42, Sai Praneeth Prakhya wrote:
> The default domain type of an iommu group can be changed using file
> "/sys/kernel/iommu_groups/<grp_id>/type". Hence, document it's usage and
> more importantly spell out it's limitations and an example.
>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Joerg Roedel <joro@8bytes.org>
> Cc: Ashok Raj <ashok.raj@intel.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Lu Baolu <baolu.lu@linux.intel.com>
> Cc: Sohil Mehta <sohil.mehta@intel.com>
> Cc: Robin Murphy <robin.murphy@arm.com>
> Cc: Jacob Pan <jacob.jun.pan@linux.intel.com>
> Signed-off-by: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>
> ---
>  .../ABI/testing/sysfs-kernel-iommu_groups     | 35 +++++++++++++++++++
>  1 file changed, 35 insertions(+)
>
> diff --git a/Documentation/ABI/testing/sysfs-kernel-iommu_groups b/Documentation/ABI/testing/sysfs-kernel-iommu_groups
> index 017f5bc3920c..0a70b3a66ff3 100644
> --- a/Documentation/ABI/testing/sysfs-kernel-iommu_groups
> +++ b/Documentation/ABI/testing/sysfs-kernel-iommu_groups
> @@ -33,3 +33,38 @@ Description:    In case an RMRR is used only by graphics or USB devices
>  		it is now exposed as "direct-relaxable" instead of "direct".
>  		In device assignment use case, for instance, those RMRR
>  		are considered to be relaxable and safe.
> +
> +What:		/sys/kernel/iommu_groups/<grp_id>/type
> +Date:		August 2019
> +KernelVersion:	v5.4
> +Contact:	Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>
> +Description:	/sys/kernel/iommu_groups/<grp_id>/type lets the user know the
> +		type of default domain in use by iommu for this group. A
> +		privileged user could request kernel to change the group type by
> +		writing to this file. Presently, only three types are supported

It's unclear whether the following is a list of all values the user 
could both read and write (which it isn't).

> +		1. dma: All the DMA transactions from the devices in this group
> +			are translated by the iommu.

Why "dma", and not "DMA" (which is what we would read for DMA type)?

> +		2. identity: All the DMA transactions from the devices in this
> +			     group are *not* translated by the iommu.
> +		3. auto: Kernel decides one among dma/identity

Isn't this the same as reset value, which we could read and remember?

(And the kernel could reject when we attempt to change to a disallowed 
type).

> +		Note:
> +		-----
> +		A group type could be modified only when *all* the devices in
> +		the group are not binded to any device driver. So, the user has
> +		to first unbind the appropriate drivers and then change the
> +		default domain type.
> +		Caution:
> +		--------
> +		Unbinding a device driver will take away the drivers control
> +		over the device and if done on devices that host root file
> +		system could lead to catastrophic effects (the user might
> +		need to reboot the machine to get it to normal state). So, it's
> +		expected that the user understands what he is doing.

I think that this goes without saying.

> +		Example:
> +		--------
> +		# Unbind USB device driver
> +		1. echo "0000:00:14.0" > /sys/bus/pci/drivers/xhci_hcd/unbind
> +		# Put the USB device in identity mode (a.k.a pass through)
> +		2. echo "identity" > /sys/kernel/iommu_groups/<grp_id>/type
> +		# Re-bind the driver
> +		3. echo "0000:00:14.0" > /sys/bus/pci/drivers/xhci_hcd/bind
>

Thanks,
John


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

Re: [PATCH 4/4] iommu: Document usage of "/sys/kernel/iommu_groups/<grp_id>/type" file

[Sai Praneeth Prakhya]

I don't know why people are being dropped randomly from CC'list. So, adding
them back

+ Sohil, Jacob, Joerg, Baolu

> > +What:		/sys/kernel/iommu_groups/<grp_id>/type
> > +Date:		August 2019
> > +KernelVersion:	v5.4
> > +Contact:	Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>
> > +Description:	/sys/kernel/iommu_groups/<grp_id>/type lets the user
> > know the
> > +		type of default domain in use by iommu for this group. A
> > +		privileged user could request kernel to change the group type
> > by
> > +		writing to this file. Presently, only three types are
> > supported
> 
> It's unclear whether the following is a list of all values the user 
> could both read and write (which it isn't).

Thanks for bringing this up.
True.. user would never see "auto" when he reads this file. I will make it
clear in V2.

> 
> > +		1. dma: All the DMA transactions from the devices in this
> > group
> > +			are translated by the iommu.
> 
> Why "dma", and not "DMA" (which is what we would read for DMA type)?

Makes sense.. Will change to "DMA" in V2.

> 
> > +		2. identity: All the DMA transactions from the devices in this
> > +			     group are *not* translated by the iommu.
> > +		3. auto: Kernel decides one among dma/identity
> 
> Isn't this the same as reset value, which we could read and remember?

Agreed. But, I think (assuming it's done manually and the user hasn't stored
default value in some script), remembering would be difficult if the system
had been running for weeks together and the user had already changed group
type multiple times.

> (And the kernel could reject when we attempt to change to a disallowed 
> type).

It already does and I see your point.
Since there are only two options, user might try to write "DMA" and if the
kernel disallows he would write "identity", simple enough.

I think of "auto" as an add-on feature. Since it's simple enough to implement
in kernel and reduces one extra step to user.

> 
> > +		Note:
> > +		-----
> > +		A group type could be modified only when *all* the devices in
> > +		the group are not binded to any device driver. So, the user
> > has
> > +		to first unbind the appropriate drivers and then change the
> > +		default domain type.
> > +		Caution:
> > +		--------
> > +		Unbinding a device driver will take away the drivers control
> > +		over the device and if done on devices that host root file
> > +		system could lead to catastrophic effects (the user might
> > +		need to reboot the machine to get it to normal state). So,
> > it's
> > +		expected that the user understands what he is doing.
> 
> I think that this goes without saying.

Haha.. Yes, it does. But, just wanted to be explicit so that new users are
warned well before :)

Regards,
Sai

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