[Patch V8 0/3] iommu: Add support to change default domain of an iommu group

[Patch V8 0/3] iommu: Add support to change default domain of an iommu group

[Ashok Raj]

Presently, the default domain of an iommu 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 wherein the privileged 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. Hence add support to change the
default domain of an iommu group dynamically.

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

Testing:
--------
Tested by dynamically changing storage device (nvme) from
1. identity mode to DMA and making sure file transfer works
2. DMA mode to identity mode and making sure file transfer works
Tested only for intel_iommu/vt-d. Would appreciate if someone could test on AMD
and ARM based machines.

Based on iommu maintainer's 'next' branch.

Changes from V6,v7:
----------------
1. None except for version bump.
https://lore.kernel.org/linux-iommu/20200925073423.GT27174@8bytes.org/

Changes from V5:
----------------
1. None except for version bump because Joerg had asked to resend the patches
   after the merge window closes.

Changes from V4:
----------------
1. Created device direct mappings before attaching the device to the domain
2. Used list_first_entry() instead of list_for_each_entry() to get the first
   element of a linked list.
3. Used get_device() and put_device() before and after device_lock()
4. Passed device as an argument to iommu_change_dev_def_domain() to check that
   the device hasn't changed between calls.
5. Changed error message from "Group assigned to user level for direct access"
   to "Group not assigned to default domain".
6. Changed error message from "Cannot change default domain of a group with two
   or more devices" to "Cannot change default domain: Group has more than one
   device".
7. Removed printing error message "'def_domain_type' call back isn't registered"

Changes from V3:
----------------
1. Made changes to commit message as suggested by Baolu.
2. Don't pass "prev_dom" and "dev" as parameters to
   iommu_change_dev_def_domain(). Instead get them from group.
3. Sanitize the logic to validate user default domain type request. The logic
   remains same but is implmented differently.
4. Push lot of error checking into iommu_change_dev_def_domain() from
   iommu_group_store_type().
5. iommu_change_dev_def_domain() takes/releases group mutex as needed. So, it
   shouldn't be called holding a group mutex.
6. Use pr_err_ratelimited() instead of pr_err() to avoid DOS attack.

Changes from V2:
----------------
1. Change the logic of updating default domain from V2 because
   ops->probe_finalize() could be used to update dma_ops.
2. Drop 1st and 2nd patch of V2 series because they are no longer needed on
   iommu maintainer's 'next' branch.
3. Limit this feature to iommu groups with only one device.
4. Hold device_lock and group mutex until the default domain is changed.

Changes from V1:
----------------
1. V1 patch set wasn't updating dma_ops for some vendors (Eg: AMD), hence,
   change the logic of updating default domain as below (because adding a device
   to iommu_group automatically updates dma_ops)
   a. Allocate a new domain
   b. For every device in the group
	i. Remove the device from the group
	ii. Add the device back to the group
   c. Free previous domain
2. Drop 1st patch of V1 (iommu/vt-d: Modify device_def_domain_type() to use at
   runtime) because "iommu=pt" has no effect on this function anymore.
3. Added a patch to take/release lock while reading iommu_group->default_domain->type
   because it can be changed any time by user.
4. Before changing default domain type of a group, check if the group is
   directly assigned for user level access. If so, abort.
5. Sanitize return path (using ternary operator) in iommu_group_store_type()
6. Split 2nd patch of V1 (iommu: Add device_def_domain_type() call back function
   to iommu_ops) into two patches such that iommu generic changes are now in 1st
   patch of V2 and vt-d specific changes are in 2nd patch of V2.
7. Rename device_def_domain_type() to dev_def_domain_type()
8. Remove example from documentation
9. Change the value written to file "/sys/kernel/iommu_groups/<grp_id>/type"
   from "dma" to "DMA".

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.

Sai Praneeth Prakhya (3):
  iommu: Add support to change default domain of an iommu group
  iommu: Take lock before reading iommu group default domain type
  iommu: Document usage of "/sys/kernel/iommu_groups/<grp_id>/type" file


Sai Praneeth Prakhya (3):
  iommu: Add support to change default domain of an iommu group
  iommu: Take lock before reading iommu group default domain type
  iommu: Document usage of "/sys/kernel/iommu_groups/<grp_id>/type" file

 .../ABI/testing/sysfs-kernel-iommu_groups          |  30 +++
 drivers/iommu/iommu.c                              | 227 ++++++++++++++++++++-
 2 files changed, 256 insertions(+), 1 deletion(-)

-- 
2.7.4

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

[Patch V8 1/3] iommu: Add support to change default domain of an iommu group

[Ashok Raj]

From: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>

Presently, the default domain of an iommu group is allocated during boot
time and it cannot be changed later. So, the device would typically be
either in identity (also known as pass_through) mode 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 wherein the user trusts the device and believes that
the 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 use case
is not supported. It will be helpful if there is some way to change the
default domain of an iommu group dynamically. Hence, add such support.

A privileged user could request the kernel to change the default domain
type of a iommu 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 device in this group are
             *not* translated by the iommu
2. DMA: all the DMA transactions from the device in this group are
        translated by the iommu
3. auto: change to the type the device was booted with

Note:
1. Default domain of an iommu group with two or more devices cannot be
   changed.
2. The device in the iommu group shouldn't be bound to any driver.
3. The device shouldn't be assigned to user for direct access.
4. The vendor iommu driver is required to add def_domain_type() callback.
   The change request will fail if the request type conflicts with that
   returned from the callback.

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>
Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>
Signed-off-by: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>
---
 drivers/iommu/iommu.c | 225 +++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 224 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 6c14c88cd525..2e93c48ce248 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -93,6 +93,8 @@ static void __iommu_detach_group(struct iommu_domain *domain,
 static int iommu_create_device_direct_mappings(struct iommu_group *group,
 					       struct device *dev);
 static struct iommu_group *iommu_group_get_for_dev(struct device *dev);
+static ssize_t iommu_group_store_type(struct iommu_group *group,
+				      const char *buf, size_t count);
 
 #define IOMMU_GROUP_ATTR(_name, _mode, _show, _store)		\
 struct iommu_group_attribute iommu_group_attr_##_name =		\
@@ -525,7 +527,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)
 {
@@ -2849,3 +2852,223 @@ 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 an iommu group that has *only* one device
+ *
+ * @group: The group for which the default domain should be changed
+ * @prev_dev: The device in the group (this is used to make sure that the device
+ *	 hasn't changed after the caller has called this function)
+ * @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
+ *    Please take a closer look if intended to use for other purposes.
+ */
+static int iommu_change_dev_def_domain(struct iommu_group *group,
+				       struct device *prev_dev, int type)
+{
+	struct iommu_domain *prev_dom;
+	struct group_device *grp_dev;
+	const struct iommu_ops *ops;
+	int ret, dev_def_dom;
+	struct device *dev;
+
+	if (!group)
+		return -EINVAL;
+
+	mutex_lock(&group->mutex);
+
+	if (group->default_domain != group->domain) {
+		pr_err_ratelimited("Group not assigned to default domain\n");
+		ret = -EBUSY;
+		goto out;
+	}
+
+	/*
+	 * iommu group wasn't locked while acquiring device lock in
+	 * iommu_group_store_type(). So, make sure that the device count hasn't
+	 * changed while acquiring device lock.
+	 *
+	 * Changing default domain of an iommu group with two or more devices
+	 * isn't supported because there could be a potential deadlock. Consider
+	 * the following scenario. T1 is trying to acquire device locks of all
+	 * the devices in the group and before it could acquire all of them,
+	 * there could be another thread T2 (from different sub-system and use
+	 * case) that has already acquired some of the device locks and might be
+	 * waiting for T1 to release other device locks.
+	 */
+	if (iommu_group_device_count(group) != 1) {
+		pr_err_ratelimited("Cannot change default domain: Group has more than one device\n");
+		ret = -EINVAL;
+		goto out;
+	}
+
+	/* Since group has only one device */
+	grp_dev = list_first_entry(&group->devices, struct group_device, list);
+	dev = grp_dev->dev;
+
+	if (prev_dev != dev) {
+		pr_err_ratelimited("Cannot change default domain: Device has been changed\n");
+		ret = -EBUSY;
+		goto out;
+	}
+
+	prev_dom = group->default_domain;
+	if (!prev_dom || !prev_dom->ops || !prev_dom->ops->def_domain_type) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	ops = prev_dom->ops;
+	dev_def_dom = ops->def_domain_type(dev);
+
+	/* Check if user requested domain is supported by the device or not */
+	if (!type) {
+		/*
+		 * If the user hasn't requested any specific type of domain and
+		 * if the device supports both the domains, then default to the
+		 * domain the device was booted with
+		 */
+		type = dev_def_dom ?: iommu_def_domain_type;
+	} else if (dev_def_dom && type != dev_def_dom) {
+		pr_err_ratelimited("Device cannot be in %s domain\n",
+				   iommu_domain_type_str(type));
+		ret = -EINVAL;
+		goto out;
+	}
+
+	/*
+	 * Switch to a new domain only if the requested domain type is different
+	 * from the existing default domain type
+	 */
+	if (prev_dom->type == type) {
+		ret = 0;
+		goto out;
+	}
+
+	/* Sets group->default_domain to the newly allocated domain */
+	ret = iommu_group_alloc_default_domain(dev->bus, group, type);
+	if (ret)
+		goto out;
+
+	ret = iommu_create_device_direct_mappings(group, dev);
+	if (ret)
+		goto free_new_domain;
+
+	ret = __iommu_attach_device(group->default_domain, dev);
+	if (ret)
+		goto free_new_domain;
+
+	group->domain = group->default_domain;
+
+	/*
+	 * Release the mutex here because ops->probe_finalize() call-back of
+	 * some vendor IOMMU drivers calls arm_iommu_attach_device() which
+	 * in-turn might call back into IOMMU core code, where it tries to take
+	 * group->mutex, resulting in a deadlock.
+	 */
+	mutex_unlock(&group->mutex);
+
+	/* Make sure dma_ops is appropriatley set */
+	iommu_group_do_probe_finalize(dev, group->default_domain);
+	iommu_domain_free(prev_dom);
+	return 0;
+
+free_new_domain:
+	iommu_domain_free(group->default_domain);
+	group->default_domain = prev_dom;
+	group->domain = prev_dom;
+
+out:
+	mutex_unlock(&group->mutex);
+
+	return ret;
+}
+
+static ssize_t iommu_group_store_type(struct iommu_group *group,
+				      const char *buf, size_t count)
+{
+	struct group_device *grp_dev;
+	struct device *dev;
+	int ret, req_type;
+
+	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_type = 0;
+	else
+		return -EINVAL;
+
+	/*
+	 * Lock/Unlock the group mutex here before device lock to
+	 * 1. Make sure that the iommu group has only one device (this is a
+	 *    prerequisite for step 2)
+	 * 2. Get struct *dev which is needed to lock device
+	 */
+	mutex_lock(&group->mutex);
+	if (iommu_group_device_count(group) != 1) {
+		mutex_unlock(&group->mutex);
+		pr_err_ratelimited("Cannot change default domain: Group has more than one device\n");
+		return -EINVAL;
+	}
+
+	/* Since group has only one device */
+	grp_dev = list_first_entry(&group->devices, struct group_device, list);
+	dev = grp_dev->dev;
+	get_device(dev);
+
+	/*
+	 * Don't hold the group mutex because taking group mutex first and then
+	 * the device lock could potentially cause a deadlock as below. Assume
+	 * two threads T1 and T2. T1 is trying to change default domain of an
+	 * iommu group and T2 is trying to hot unplug a device or release [1] VF
+	 * of a PCIe device which is in the same iommu group. T1 takes group
+	 * mutex and before it could take device lock assume T2 has taken device
+	 * lock and is yet to take group mutex. Now, both the threads will be
+	 * waiting for the other thread to release lock. Below, lock order was
+	 * suggested.
+	 * device_lock(dev);
+	 *	mutex_lock(&group->mutex);
+	 *		iommu_change_dev_def_domain();
+	 *	mutex_unlock(&group->mutex);
+	 * device_unlock(dev);
+	 *
+	 * [1] Typical device release path
+	 * device_lock() from device/driver core code
+	 *  -> bus_notifier()
+	 *   -> iommu_bus_notifier()
+	 *    -> iommu_release_device()
+	 *     -> ops->release_device() vendor driver calls back iommu core code
+	 *      -> mutex_lock() from iommu core code
+	 */
+	mutex_unlock(&group->mutex);
+
+	/* Check if the device in the group still has a driver bound to it */
+	device_lock(dev);
+	if (device_is_bound(dev)) {
+		pr_err_ratelimited("Device is still bound to driver\n");
+		ret = -EBUSY;
+		goto out;
+	}
+
+	ret = iommu_change_dev_def_domain(group, dev, req_type);
+	ret = ret ?: count;
+
+out:
+	device_unlock(dev);
+	put_device(dev);
+
+	return ret;
+}
-- 
2.7.4

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

[Patch V8 2/3] iommu: Take lock before reading iommu group default domain type

[Ashok Raj]

From: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>

"/sys/kernel/iommu_groups/<grp_id>/type" file could be read to find out the
default domain type of an iommu group. The default domain of an iommu group
doesn't change after booting and hence could be read directly. But,
after addding support to dynamically change iommu group default domain, the
above assumption no longer stays valid.

iommu group default domain type could be changed at any time by writing to
"/sys/kernel/iommu_groups/<grp_id>/type". So, take group mutex before
reading iommu group default domain type so that the user wouldn't see stale
values or iommu_group_show_type() doesn't try to derefernce stale pointers.

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>
Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>
Signed-off-by: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>
---
 drivers/iommu/iommu.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 2e93c48ce248..b540ae1e679d 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -501,6 +501,7 @@ static ssize_t iommu_group_show_type(struct iommu_group *group,
 {
 	char *type = "unknown\n";
 
+	mutex_lock(&group->mutex);
 	if (group->default_domain) {
 		switch (group->default_domain->type) {
 		case IOMMU_DOMAIN_BLOCKED:
@@ -517,6 +518,7 @@ static ssize_t iommu_group_show_type(struct iommu_group *group,
 			break;
 		}
 	}
+	mutex_unlock(&group->mutex);
 	strcpy(buf, type);
 
 	return strlen(type);
-- 
2.7.4

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

[Patch V8 3/3] iommu: Document usage of "/sys/kernel/iommu_groups/<grp_id>/type" file

[Ashok Raj]

From: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>

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

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>
Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>
Signed-off-by: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>
---
 .../ABI/testing/sysfs-kernel-iommu_groups          | 30 ++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-kernel-iommu_groups b/Documentation/ABI/testing/sysfs-kernel-iommu_groups
index 017f5bc3920c..effde9d23f4f 100644
--- a/Documentation/ABI/testing/sysfs-kernel-iommu_groups
+++ b/Documentation/ABI/testing/sysfs-kernel-iommu_groups
@@ -33,3 +33,33 @@ 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:		September 2020
+KernelVersion:	v5.10
+Contact:	Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>
+Description:	Let 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 device in this group
+			are translated by the iommu.
+		2. identity: All the DMA transactions from the device in this
+			     group are *not* translated by the iommu.
+		3. auto: Change to the type the device was booted with. When the
+			 user reads the file he would never see "auto". This is
+			 just a write only value.
+		Note:
+		-----
+		A group type could be modified only when
+		1. The group has *only* one device
+		2. The device in the group is not bound to any device driver.
+		   So, the user must first unbind the appropriate driver and
+		   then change the default domain type.
+		Caution:
+		--------
+		Unbinding a device driver will take away the driver's 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.
-- 
2.7.4

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

Re: [Patch V8 0/3] iommu: Add support to change default domain of an iommu group

[Joerg Roedel]

Hi Ashok,

On Fri, Sep 25, 2020 at 12:06:17PM -0700, Ashok Raj wrote:
> Sai Praneeth Prakhya (3):
>   iommu: Add support to change default domain of an iommu group
>   iommu: Take lock before reading iommu group default domain type
>   iommu: Document usage of "/sys/kernel/iommu_groups/<grp_id>/type" file
> 
>  .../ABI/testing/sysfs-kernel-iommu_groups          |  30 +++
>  drivers/iommu/iommu.c                              | 227 ++++++++++++++++++++-
>  2 files changed, 256 insertions(+), 1 deletion(-)

Thanks for the repost, I can grab it just fine with b4. But this nees
some more testing on my side and some time in linux-next, so it is too
late now to queue it for v5.10. Can you please remind me after the next
merge window? I'll pick it up then and do the testing and it will
hopefully spend enough time in linux-next.

Thanks,

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

Re: [Patch V8 0/3] iommu: Add support to change default domain of an iommu group

[Raj, Ashok]

Hi Joerg

On Thu, Oct 01, 2020 at 02:58:41PM +0200, Joerg Roedel wrote:
> Hi Ashok,
> 
> On Fri, Sep 25, 2020 at 12:06:17PM -0700, Ashok Raj wrote:
> > Sai Praneeth Prakhya (3):
> >   iommu: Add support to change default domain of an iommu group
> >   iommu: Take lock before reading iommu group default domain type
> >   iommu: Document usage of "/sys/kernel/iommu_groups/<grp_id>/type" file
> > 
> >  .../ABI/testing/sysfs-kernel-iommu_groups          |  30 +++
> >  drivers/iommu/iommu.c                              | 227 ++++++++++++++++++++-
> >  2 files changed, 256 insertions(+), 1 deletion(-)
> 
> Thanks for the repost, I can grab it just fine with b4. But this nees
> some more testing on my side and some time in linux-next, so it is too
> late now to queue it for v5.10. Can you please remind me after the next
> merge window? I'll pick it up then and do the testing and it will
> hopefully spend enough time in linux-next.

Yes, I'll try to remind you after the next merge window.

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

Re: [Patch V8 3/3] iommu: Document usage of "/sys/kernel/iommu_groups/<grp_id>/type" file

[Will Deacon]

On Fri, Sep 25, 2020 at 12:06:20PM -0700, Ashok Raj wrote:
> From: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>
> 
> The default domain type of an iommu group can be changed by writing to
> "/sys/kernel/iommu_groups/<grp_id>/type" file. Hence, document it's usage
> and more importantly spell out its limitations.
> 
> 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>
> Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>
> Signed-off-by: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>
> ---
>  .../ABI/testing/sysfs-kernel-iommu_groups          | 30 ++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-kernel-iommu_groups b/Documentation/ABI/testing/sysfs-kernel-iommu_groups
> index 017f5bc3920c..effde9d23f4f 100644
> --- a/Documentation/ABI/testing/sysfs-kernel-iommu_groups
> +++ b/Documentation/ABI/testing/sysfs-kernel-iommu_groups
> @@ -33,3 +33,33 @@ 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:		September 2020
> +KernelVersion:	v5.10

^^ Please can you update these two lines?

> +Contact:	Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>
> +Description:	Let 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 device in this group
> +			are translated by the iommu.
> +		2. identity: All the DMA transactions from the device in this
> +			     group are *not* translated by the iommu.
> +		3. auto: Change to the type the device was booted with. When the
> +			 user reads the file he would never see "auto". This is

Can we avoid assuming gender here and just use "they" instead of "he", please?
Same thing for the "Caution" note below.

> +			 just a write only value.

I can't figure out from this description what string is returned to
userspace in the case that the group is configured as  blocked or unmanaged.

> +		Note:
> +		-----
> +		A group type could be modified only when

s/could be/may be/

> +		1. The group has *only* one device
> +		2. The device in the group is not bound to any device driver.
> +		   So, the user must first unbind the appropriate driver and
> +		   then change the default domain type.
> +		Caution:
> +		--------
> +		Unbinding a device driver will take away the driver's 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.

Thanks,

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

Re: [Patch V8 1/3] iommu: Add support to change default domain of an iommu group

[Will Deacon]

On Fri, Sep 25, 2020 at 12:06:18PM -0700, Ashok Raj wrote:
> From: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>
> 
> Presently, the default domain of an iommu group is allocated during boot
> time and it cannot be changed later. So, the device would typically be
> either in identity (also known as pass_through) mode 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 wherein the user trusts the device and believes that
> the 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 use case
> is not supported. It will be helpful if there is some way to change the
> default domain of an iommu group dynamically. Hence, add such support.
> 
> A privileged user could request the kernel to change the default domain
> type of a iommu 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 device in this group are
>              *not* translated by the iommu
> 2. DMA: all the DMA transactions from the device in this group are
>         translated by the iommu
> 3. auto: change to the type the device was booted with
> 
> Note:
> 1. Default domain of an iommu group with two or more devices cannot be
>    changed.
> 2. The device in the iommu group shouldn't be bound to any driver.
> 3. The device shouldn't be assigned to user for direct access.
> 4. The vendor iommu driver is required to add def_domain_type() callback.
>    The change request will fail if the request type conflicts with that
>    returned from the callback.
> 
> 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>
> Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>
> Signed-off-by: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>
> ---
>  drivers/iommu/iommu.c | 225 +++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 224 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 6c14c88cd525..2e93c48ce248 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -93,6 +93,8 @@ static void __iommu_detach_group(struct iommu_domain *domain,
>  static int iommu_create_device_direct_mappings(struct iommu_group *group,
>  					       struct device *dev);
>  static struct iommu_group *iommu_group_get_for_dev(struct device *dev);
> +static ssize_t iommu_group_store_type(struct iommu_group *group,
> +				      const char *buf, size_t count);
>  
>  #define IOMMU_GROUP_ATTR(_name, _mode, _show, _store)		\
>  struct iommu_group_attribute iommu_group_attr_##_name =		\
> @@ -525,7 +527,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)
>  {
> @@ -2849,3 +2852,223 @@ 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 an iommu group that has *only* one device
> + *
> + * @group: The group for which the default domain should be changed
> + * @prev_dev: The device in the group (this is used to make sure that the device
> + *	 hasn't changed after the caller has called this function)
> + * @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
> + *    Please take a closer look if intended to use for other purposes.
> + */
> +static int iommu_change_dev_def_domain(struct iommu_group *group,
> +				       struct device *prev_dev, int type)
> +{
> +	struct iommu_domain *prev_dom;
> +	struct group_device *grp_dev;
> +	const struct iommu_ops *ops;
> +	int ret, dev_def_dom;
> +	struct device *dev;
> +
> +	if (!group)
> +		return -EINVAL;
> +
> +	mutex_lock(&group->mutex);
> +
> +	if (group->default_domain != group->domain) {
> +		pr_err_ratelimited("Group not assigned to default domain\n");

This error is lacking any context. Can we use dev_err_ratelimited to make
it a bit more useful? Same for the other errors you're adding in this
function.

> +		ret = -EBUSY;
> +		goto out;
> +	}
> +
> +	/*
> +	 * iommu group wasn't locked while acquiring device lock in
> +	 * iommu_group_store_type(). So, make sure that the device count hasn't
> +	 * changed while acquiring device lock.
> +	 *
> +	 * Changing default domain of an iommu group with two or more devices
> +	 * isn't supported because there could be a potential deadlock. Consider
> +	 * the following scenario. T1 is trying to acquire device locks of all
> +	 * the devices in the group and before it could acquire all of them,
> +	 * there could be another thread T2 (from different sub-system and use
> +	 * case) that has already acquired some of the device locks and might be
> +	 * waiting for T1 to release other device locks.
> +	 */
> +	if (iommu_group_device_count(group) != 1) {
> +		pr_err_ratelimited("Cannot change default domain: Group has more than one device\n");
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	/* Since group has only one device */
> +	grp_dev = list_first_entry(&group->devices, struct group_device, list);
> +	dev = grp_dev->dev;
> +
> +	if (prev_dev != dev) {
> +		pr_err_ratelimited("Cannot change default domain: Device has been changed\n");
> +		ret = -EBUSY;
> +		goto out;
> +	}
> +
> +	prev_dom = group->default_domain;
> +	if (!prev_dom || !prev_dom->ops || !prev_dom->ops->def_domain_type) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	ops = prev_dom->ops;
> +	dev_def_dom = ops->def_domain_type(dev);
> +
> +	/* Check if user requested domain is supported by the device or not */
> +	if (!type) {
> +		/*
> +		 * If the user hasn't requested any specific type of domain and
> +		 * if the device supports both the domains, then default to the
> +		 * domain the device was booted with
> +		 */
> +		type = dev_def_dom ?: iommu_def_domain_type;
> +	} else if (dev_def_dom && type != dev_def_dom) {
> +		pr_err_ratelimited("Device cannot be in %s domain\n",
> +				   iommu_domain_type_str(type));
> +		ret = -EINVAL;
> +		goto out;
> +	}

Can we replace this with iommu_get_def_domain_type() ?

> +
> +	/*
> +	 * Switch to a new domain only if the requested domain type is different
> +	 * from the existing default domain type
> +	 */
> +	if (prev_dom->type == type) {
> +		ret = 0;
> +		goto out;
> +	}
> +
> +	/* Sets group->default_domain to the newly allocated domain */
> +	ret = iommu_group_alloc_default_domain(dev->bus, group, type);
> +	if (ret)
> +		goto out;
> +
> +	ret = iommu_create_device_direct_mappings(group, dev);
> +	if (ret)
> +		goto free_new_domain;
> +
> +	ret = __iommu_attach_device(group->default_domain, dev);
> +	if (ret)
> +		goto free_new_domain;
> +
> +	group->domain = group->default_domain;
> +
> +	/*
> +	 * Release the mutex here because ops->probe_finalize() call-back of
> +	 * some vendor IOMMU drivers calls arm_iommu_attach_device() which
> +	 * in-turn might call back into IOMMU core code, where it tries to take
> +	 * group->mutex, resulting in a deadlock.
> +	 */
> +	mutex_unlock(&group->mutex);
> +
> +	/* Make sure dma_ops is appropriatley set */
> +	iommu_group_do_probe_finalize(dev, group->default_domain);
> +	iommu_domain_free(prev_dom);
> +	return 0;
> +
> +free_new_domain:
> +	iommu_domain_free(group->default_domain);
> +	group->default_domain = prev_dom;
> +	group->domain = prev_dom;i

Hmm. This seems to rely on all users of group->default_domain holding the
group->mutex. Have you confirmed that this is the case? There's a funny
use of iommu_group_get() in the exynos IOMMU driver at least.

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

Re: [Patch V8 0/3] iommu: Add support to change default domain of an iommu group

[Will Deacon]

On Fri, Sep 25, 2020 at 12:06:17PM -0700, Ashok Raj wrote:
> Presently, the default domain of an iommu 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 wherein the privileged 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. Hence add support to change the
> default domain of an iommu group dynamically.
> 
> Support this by writing to a sysfs file, namely
> "/sys/kernel/iommu_groups/<grp_id>/type".
> 
> Testing:
> --------
> Tested by dynamically changing storage device (nvme) from
> 1. identity mode to DMA and making sure file transfer works
> 2. DMA mode to identity mode and making sure file transfer works
> Tested only for intel_iommu/vt-d. Would appreciate if someone could test on AMD
> and ARM based machines.
> 
> Based on iommu maintainer's 'next' branch.

Modulo my minor comments, I think this looks good for 5.11 if you can
please send a version 9.

Robin -- please can you give it the once-over too? I think root can break
things quite badly with this interface, but root can do that in other ways
anyway...

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

Re: [Patch V8 1/3] iommu: Add support to change default domain of an iommu group

[Lu Baolu]

Hi Will,

The original author of this patch series has left Intel. I am now the
backup.

On 11/18/20 9:51 PM, Will Deacon wrote:
> On Fri, Sep 25, 2020 at 12:06:18PM -0700, Ashok Raj wrote:
>> From: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>
>>
>> Presently, the default domain of an iommu group is allocated during boot
>> time and it cannot be changed later. So, the device would typically be
>> either in identity (also known as pass_through) mode 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 wherein the user trusts the device and believes that
>> the 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 use case
>> is not supported. It will be helpful if there is some way to change the
>> default domain of an iommu group dynamically. Hence, add such support.
>>
>> A privileged user could request the kernel to change the default domain
>> type of a iommu 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 device in this group are
>>               *not* translated by the iommu
>> 2. DMA: all the DMA transactions from the device in this group are
>>          translated by the iommu
>> 3. auto: change to the type the device was booted with
>>
>> Note:
>> 1. Default domain of an iommu group with two or more devices cannot be
>>     changed.
>> 2. The device in the iommu group shouldn't be bound to any driver.
>> 3. The device shouldn't be assigned to user for direct access.
>> 4. The vendor iommu driver is required to add def_domain_type() callback.
>>     The change request will fail if the request type conflicts with that
>>     returned from the callback.
>>
>> 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>
>> Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>
>> Signed-off-by: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>
>> ---
>>   drivers/iommu/iommu.c | 225 +++++++++++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 224 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index 6c14c88cd525..2e93c48ce248 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -93,6 +93,8 @@ static void __iommu_detach_group(struct iommu_domain *domain,
>>   static int iommu_create_device_direct_mappings(struct iommu_group *group,
>>   					       struct device *dev);
>>   static struct iommu_group *iommu_group_get_for_dev(struct device *dev);
>> +static ssize_t iommu_group_store_type(struct iommu_group *group,
>> +				      const char *buf, size_t count);
>>   
>>   #define IOMMU_GROUP_ATTR(_name, _mode, _show, _store)		\
>>   struct iommu_group_attribute iommu_group_attr_##_name =		\
>> @@ -525,7 +527,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)
>>   {
>> @@ -2849,3 +2852,223 @@ 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 an iommu group that has *only* one device
>> + *
>> + * @group: The group for which the default domain should be changed
>> + * @prev_dev: The device in the group (this is used to make sure that the device
>> + *	 hasn't changed after the caller has called this function)
>> + * @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
>> + *    Please take a closer look if intended to use for other purposes.
>> + */
>> +static int iommu_change_dev_def_domain(struct iommu_group *group,
>> +				       struct device *prev_dev, int type)
>> +{
>> +	struct iommu_domain *prev_dom;
>> +	struct group_device *grp_dev;
>> +	const struct iommu_ops *ops;
>> +	int ret, dev_def_dom;
>> +	struct device *dev;
>> +
>> +	if (!group)
>> +		return -EINVAL;
>> +
>> +	mutex_lock(&group->mutex);
>> +
>> +	if (group->default_domain != group->domain) {
>> +		pr_err_ratelimited("Group not assigned to default domain\n");
> 
> This error is lacking any context. Can we use dev_err_ratelimited to make
> it a bit more useful? Same for the other errors you're adding in this
> function.

Yes. dev_err_ratelimited() is better. Will replace all of them.

> 
>> +		ret = -EBUSY;
>> +		goto out;
>> +	}
>> +
>> +	/*
>> +	 * iommu group wasn't locked while acquiring device lock in
>> +	 * iommu_group_store_type(). So, make sure that the device count hasn't
>> +	 * changed while acquiring device lock.
>> +	 *
>> +	 * Changing default domain of an iommu group with two or more devices
>> +	 * isn't supported because there could be a potential deadlock. Consider
>> +	 * the following scenario. T1 is trying to acquire device locks of all
>> +	 * the devices in the group and before it could acquire all of them,
>> +	 * there could be another thread T2 (from different sub-system and use
>> +	 * case) that has already acquired some of the device locks and might be
>> +	 * waiting for T1 to release other device locks.
>> +	 */
>> +	if (iommu_group_device_count(group) != 1) {
>> +		pr_err_ratelimited("Cannot change default domain: Group has more than one device\n");
>> +		ret = -EINVAL;
>> +		goto out;
>> +	}
>> +
>> +	/* Since group has only one device */
>> +	grp_dev = list_first_entry(&group->devices, struct group_device, list);
>> +	dev = grp_dev->dev;
>> +
>> +	if (prev_dev != dev) {
>> +		pr_err_ratelimited("Cannot change default domain: Device has been changed\n");
>> +		ret = -EBUSY;
>> +		goto out;
>> +	}
>> +
>> +	prev_dom = group->default_domain;
>> +	if (!prev_dom || !prev_dom->ops || !prev_dom->ops->def_domain_type) {
>> +		ret = -EINVAL;
>> +		goto out;
>> +	}
>> +
>> +	ops = prev_dom->ops;
>> +	dev_def_dom = ops->def_domain_type(dev);
>> +
>> +	/* Check if user requested domain is supported by the device or not */
>> +	if (!type) {
>> +		/*
>> +		 * If the user hasn't requested any specific type of domain and
>> +		 * if the device supports both the domains, then default to the
>> +		 * domain the device was booted with
>> +		 */
>> +		type = dev_def_dom ?: iommu_def_domain_type;
>> +	} else if (dev_def_dom && type != dev_def_dom) {
>> +		pr_err_ratelimited("Device cannot be in %s domain\n",
>> +				   iommu_domain_type_str(type));
>> +		ret = -EINVAL;
>> +		goto out;
>> +	}
> 
> Can we replace this with iommu_get_def_domain_type() ?

Of cause. I will do.

> 
>> +
>> +	/*
>> +	 * Switch to a new domain only if the requested domain type is different
>> +	 * from the existing default domain type
>> +	 */
>> +	if (prev_dom->type == type) {
>> +		ret = 0;
>> +		goto out;
>> +	}
>> +
>> +	/* Sets group->default_domain to the newly allocated domain */
>> +	ret = iommu_group_alloc_default_domain(dev->bus, group, type);
>> +	if (ret)
>> +		goto out;
>> +
>> +	ret = iommu_create_device_direct_mappings(group, dev);
>> +	if (ret)
>> +		goto free_new_domain;
>> +
>> +	ret = __iommu_attach_device(group->default_domain, dev);
>> +	if (ret)
>> +		goto free_new_domain;
>> +
>> +	group->domain = group->default_domain;
>> +
>> +	/*
>> +	 * Release the mutex here because ops->probe_finalize() call-back of
>> +	 * some vendor IOMMU drivers calls arm_iommu_attach_device() which
>> +	 * in-turn might call back into IOMMU core code, where it tries to take
>> +	 * group->mutex, resulting in a deadlock.
>> +	 */
>> +	mutex_unlock(&group->mutex);
>> +
>> +	/* Make sure dma_ops is appropriatley set */
>> +	iommu_group_do_probe_finalize(dev, group->default_domain);
>> +	iommu_domain_free(prev_dom);
>> +	return 0;
>> +
>> +free_new_domain:
>> +	iommu_domain_free(group->default_domain);
>> +	group->default_domain = prev_dom;
>> +	group->domain = prev_dom;i
> 
> Hmm. This seems to rely on all users of group->default_domain holding the
> group->mutex. Have you confirmed that this is the case? There's a funny
> use of iommu_group_get() in the exynos IOMMU driver at least.

Emm. This change happens within the area with group->mutex held. Or I
am not getting your point?

+free_new_domain:
+	iommu_domain_free(group->default_domain);
+	group->default_domain = prev_dom;
+	group->domain = prev_dom;
+
+out:
+	mutex_unlock(&group->mutex);
+
+	return ret;

> 
> Will

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

Re: [Patch V8 3/3] iommu: Document usage of "/sys/kernel/iommu_groups/<grp_id>/type" file

[Lu Baolu]

Hi Will,

On 11/18/20 9:51 PM, Will Deacon wrote:
> On Fri, Sep 25, 2020 at 12:06:20PM -0700, Ashok Raj wrote:
>> From: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>
>>
>> The default domain type of an iommu group can be changed by writing to
>> "/sys/kernel/iommu_groups/<grp_id>/type" file. Hence, document it's usage
>> and more importantly spell out its limitations.
>>
>> 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>
>> Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>
>> Signed-off-by: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>
>> ---
>>   .../ABI/testing/sysfs-kernel-iommu_groups          | 30 ++++++++++++++++++++++
>>   1 file changed, 30 insertions(+)
>>
>> diff --git a/Documentation/ABI/testing/sysfs-kernel-iommu_groups b/Documentation/ABI/testing/sysfs-kernel-iommu_groups
>> index 017f5bc3920c..effde9d23f4f 100644
>> --- a/Documentation/ABI/testing/sysfs-kernel-iommu_groups
>> +++ b/Documentation/ABI/testing/sysfs-kernel-iommu_groups
>> @@ -33,3 +33,33 @@ 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:		September 2020
>> +KernelVersion:	v5.10
> 
> ^^ Please can you update these two lines?

Sure.

> 
>> +Contact:	Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>
>> +Description:	Let 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 device in this group
>> +			are translated by the iommu.
>> +		2. identity: All the DMA transactions from the device in this
>> +			     group are *not* translated by the iommu.
>> +		3. auto: Change to the type the device was booted with. When the
>> +			 user reads the file he would never see "auto". This is
> 
> Can we avoid assuming gender here and just use "they" instead of "he", please?
> Same thing for the "Caution" note below.

Yes, absolutely.

> 
>> +			 just a write only value.
> 
> I can't figure out from this description what string is returned to
> userspace in the case that the group is configured as  blocked or unmanaged.

This series only enables switching a default domain in use between DMA
and IDENTITY. Other cases will result in write failures.

> 
>> +		Note:
>> +		-----
>> +		A group type could be modified only when
> 
> s/could be/may be/
> 
>> +		1. The group has *only* one device
>> +		2. The device in the group is not bound to any device driver.
>> +		   So, the user must first unbind the appropriate driver and
>> +		   then change the default domain type.
>> +		Caution:
>> +		--------
>> +		Unbinding a device driver will take away the driver's 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.
> 
> Thanks,
> 
> Will

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

Re: [Patch V8 0/3] iommu: Add support to change default domain of an iommu group

[Lu Baolu]

On 11/18/20 9:52 PM, Will Deacon wrote:
> On Fri, Sep 25, 2020 at 12:06:17PM -0700, Ashok Raj wrote:
>> Presently, the default domain of an iommu 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 wherein the privileged 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. Hence add support to change the
>> default domain of an iommu group dynamically.
>>
>> Support this by writing to a sysfs file, namely
>> "/sys/kernel/iommu_groups/<grp_id>/type".
>>
>> Testing:
>> --------
>> Tested by dynamically changing storage device (nvme) from
>> 1. identity mode to DMA and making sure file transfer works
>> 2. DMA mode to identity mode and making sure file transfer works
>> Tested only for intel_iommu/vt-d. Would appreciate if someone could test on AMD
>> and ARM based machines.
>>
>> Based on iommu maintainer's 'next' branch.
> 
> Modulo my minor comments, I think this looks good for 5.11 if you can
> please send a version 9.
> 
> Robin -- please can you give it the once-over too? I think root can break
> things quite badly with this interface, but root can do that in other ways
> anyway...

Sure. I will send a v9 after Robin's review.

> 
> Will

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

Re: [Patch V8 1/3] iommu: Add support to change default domain of an iommu group

[Will Deacon]

On Thu, Nov 19, 2020 at 10:18:05AM +0800, Lu Baolu wrote:
> The original author of this patch series has left Intel. I am now the
> backup.

Ok, thanks for letting me know.

> On 11/18/20 9:51 PM, Will Deacon wrote:
> > On Fri, Sep 25, 2020 at 12:06:18PM -0700, Ashok Raj wrote:
> > > From: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>

[...]

> > > +free_new_domain:
> > > +	iommu_domain_free(group->default_domain);
> > > +	group->default_domain = prev_dom;
> > > +	group->domain = prev_dom;i
> > 
> > Hmm. This seems to rely on all users of group->default_domain holding the
> > group->mutex. Have you confirmed that this is the case? There's a funny
> > use of iommu_group_get() in the exynos IOMMU driver at least.
> 
> Emm. This change happens within the area with group->mutex held. Or I
> am not getting your point?

Yeah, sorry, I wasn't very clear. This code holds the group->mutex, and it
relies on _anybody_ else who wants to inspect group->default_domain also
holding that mutex, otherwise they could observe a transient domain pointer
which we free on the failure path here.

My question is whether or not there is code that inspects
group->default_domain without group->mutex held? The exynos case doesn't
obviously hold it, and I'd like to make sure that there aren't others that
we need to worry about.

Does that make more sense?

Thanks,

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

Re: [Patch V8 3/3] iommu: Document usage of "/sys/kernel/iommu_groups/<grp_id>/type" file

[Will Deacon]

Hi Lu,

On Thu, Nov 19, 2020 at 10:32:43AM +0800, Lu Baolu wrote:
> On 11/18/20 9:51 PM, Will Deacon wrote:
> > On Fri, Sep 25, 2020 at 12:06:20PM -0700, Ashok Raj wrote:
> > I can't figure out from this description what string is returned to
> > userspace in the case that the group is configured as  blocked or unmanaged.
> 
> This series only enables switching a default domain in use between DMA
> and IDENTITY. Other cases will result in write failures.

I understood that from the text, but what I couldn't figure out is what
happens if you *read* the file when the default domain is not identity
or DMA. I think that should be documented.

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

Re: [Patch V8 1/3] iommu: Add support to change default domain of an iommu group

[Lu Baolu]

Hi Will,

On 11/19/20 4:53 PM, Will Deacon wrote:
> On Thu, Nov 19, 2020 at 10:18:05AM +0800, Lu Baolu wrote:
>> The original author of this patch series has left Intel. I am now the
>> backup.
> 
> Ok, thanks for letting me know.
> 
>> On 11/18/20 9:51 PM, Will Deacon wrote:
>>> On Fri, Sep 25, 2020 at 12:06:18PM -0700, Ashok Raj wrote:
>>>> From: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>
> 
> [...]
> 
>>>> +free_new_domain:
>>>> +	iommu_domain_free(group->default_domain);
>>>> +	group->default_domain = prev_dom;
>>>> +	group->domain = prev_dom;i
>>>
>>> Hmm. This seems to rely on all users of group->default_domain holding the
>>> group->mutex. Have you confirmed that this is the case? There's a funny
>>> use of iommu_group_get() in the exynos IOMMU driver at least.
>>
>> Emm. This change happens within the area with group->mutex held. Or I
>> am not getting your point?
> 
> Yeah, sorry, I wasn't very clear. This code holds the group->mutex, and it
> relies on _anybody_ else who wants to inspect group->default_domain also
> holding that mutex, otherwise they could observe a transient domain pointer
> which we free on the failure path here.

Clear to me now. Thanks for explanation. :-)

Changing default domain through sysfs requires the users to ubind any
driver from the devices in the group. There's a check code and return
failure if this requirement doesn't meet.

So we only need to consider the device release path. device_lock(dev) is
used in this patch to guarantee that no device release happens at the
same time.

> 
> My question is whether or not there is code that inspects
> group->default_domain without group->mutex held? The exynos case doesn't
> obviously hold it, and I'd like to make sure that there aren't others that
> we need to worry about.

I searched the code. The exynos is the only case that inspects
group->default_domain without holding the mutex during run time. It's in
the device release path, so I think it's safe.

> 
> Does that make more sense?

Yes. Thanks!

> 
> Thanks,
> 
> Will
> 

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

Re: [Patch V8 3/3] iommu: Document usage of "/sys/kernel/iommu_groups/<grp_id>/type" file

[Lu Baolu]

Hi Will

On 11/19/20 4:55 PM, Will Deacon wrote:
> Hi Lu,
> 
> On Thu, Nov 19, 2020 at 10:32:43AM +0800, Lu Baolu wrote:
>> On 11/18/20 9:51 PM, Will Deacon wrote:
>>> On Fri, Sep 25, 2020 at 12:06:20PM -0700, Ashok Raj wrote:
>>> I can't figure out from this description what string is returned to
>>> userspace in the case that the group is configured as  blocked or unmanaged.
>>
>> This series only enables switching a default domain in use between DMA
>> and IDENTITY. Other cases will result in write failures.
> 
> I understood that from the text, but what I couldn't figure out is what
> happens if you *read* the file when the default domain is not identity
> or DMA. I think that should be documented.

Yes, agreed. I will add this.

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

Re: [Patch V8 1/3] iommu: Add support to change default domain of an iommu group

[Will Deacon]

On Fri, Nov 20, 2020 at 10:11:58AM +0800, Lu Baolu wrote:
> On 11/19/20 4:53 PM, Will Deacon wrote:
> > On Thu, Nov 19, 2020 at 10:18:05AM +0800, Lu Baolu wrote:
> > > On 11/18/20 9:51 PM, Will Deacon wrote:
> > > > On Fri, Sep 25, 2020 at 12:06:18PM -0700, Ashok Raj wrote:
> > > > > From: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>
> > 
> > [...]
> > 
> > > > > +free_new_domain:
> > > > > +	iommu_domain_free(group->default_domain);
> > > > > +	group->default_domain = prev_dom;
> > > > > +	group->domain = prev_dom;i
> > > > 
> > > > Hmm. This seems to rely on all users of group->default_domain holding the
> > > > group->mutex. Have you confirmed that this is the case? There's a funny
> > > > use of iommu_group_get() in the exynos IOMMU driver at least.
> > > 
> > > Emm. This change happens within the area with group->mutex held. Or I
> > > am not getting your point?
> > 
> > Yeah, sorry, I wasn't very clear. This code holds the group->mutex, and it
> > relies on _anybody_ else who wants to inspect group->default_domain also
> > holding that mutex, otherwise they could observe a transient domain pointer
> > which we free on the failure path here.
> 
> Clear to me now. Thanks for explanation. :-)
> 
> Changing default domain through sysfs requires the users to ubind any
> driver from the devices in the group. There's a check code and return
> failure if this requirement doesn't meet.
> 
> So we only need to consider the device release path. device_lock(dev) is
> used in this patch to guarantee that no device release happens at the
> same time.

Aha, thanks. Please can you add a comment for future reference?

> 
> > 
> > My question is whether or not there is code that inspects
> > group->default_domain without group->mutex held? The exynos case doesn't
> > obviously hold it, and I'd like to make sure that there aren't others that
> > we need to worry about.
> 
> I searched the code. The exynos is the only case that inspects
> group->default_domain without holding the mutex during run time. It's in
> the device release path, so I think it's safe.

Great, thanks for looking.

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

RE: [Patch V8 1/3] iommu: Add support to change default domain of an iommu group

[Shameerali Kolothum Thodi]

Hi Baolu/Ashok,

> -----Original Message-----
> From: iommu [mailto:iommu-bounces@lists.linux-foundation.org] On Behalf Of
> Ashok Raj
> Sent: 25 September 2020 20:06
> To: Joerg Roedel <joro@8bytes.org>; iommu@lists.linux-foundation.org
> Cc: Ashok Raj <ashok.raj@intel.com>; Will Deacon <will.deacon@arm.com>;
> Robin Murphy <robin.murphy@arm.com>; Christoph Hellwig <hch@lst.de>
> Subject: [Patch V8 1/3] iommu: Add support to change default domain of an
> iommu group
> 
> From: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>
> 
> Presently, the default domain of an iommu group is allocated during boot
> time and it cannot be changed later. So, the device would typically be
> either in identity (also known as pass_through) mode 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 wherein the user trusts the device and believes that
> the 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 use case
> is not supported. It will be helpful if there is some way to change the
> default domain of an iommu group dynamically. Hence, add such support.
> 
> A privileged user could request the kernel to change the default domain
> type of a iommu 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 device in this group are
>              *not* translated by the iommu
> 2. DMA: all the DMA transactions from the device in this group are
>         translated by the iommu
> 3. auto: change to the type the device was booted with
> 
> Note:
> 1. Default domain of an iommu group with two or more devices cannot be
>    changed.
> 2. The device in the iommu group shouldn't be bound to any driver.
> 3. The device shouldn't be assigned to user for direct access.
> 4. The vendor iommu driver is required to add def_domain_type() callback.
>    The change request will fail if the request type conflicts with that
>    returned from the callback.

Currently Arm SMMUv3 driver doesn't provide the def_doman_type() callback.
And I have sent a patch[1] based on this series to just add that. But Robin made
couple of suggestions there which can be incorporated into this series so that the
vendor driver no more required to provide the callback for this feature to work. 

1. Include a generic checking for 
	if (dev_is_pci(dev)) {
		if (pci_dev->untrusted)
			return IOMMU_DOMAIN_DMA;
	}

2. Also if there is no def_doman_type() callback provided by vendor driver, assume
  that the dev supports both IDENTITY and DMA domain types.

If you plan to respin this series, could you please consider the above as well?
Please let me know.

Thanks,
Shameer

1. https://lore.kernel.org/linux-iommu/a3c73596-70d9-4204-d598-5e6684868544@arm.com/

> 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>
> Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>
> Signed-off-by: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>
> ---
>  drivers/iommu/iommu.c | 225
> +++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 224 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 6c14c88cd525..2e93c48ce248 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -93,6 +93,8 @@ static void __iommu_detach_group(struct
> iommu_domain *domain,
>  static int iommu_create_device_direct_mappings(struct iommu_group
> *group,
>  					       struct device *dev);
>  static struct iommu_group *iommu_group_get_for_dev(struct device *dev);
> +static ssize_t iommu_group_store_type(struct iommu_group *group,
> +				      const char *buf, size_t count);
> 
>  #define IOMMU_GROUP_ATTR(_name, _mode, _show, _store)		\
>  struct iommu_group_attribute iommu_group_attr_##_name =		\
> @@ -525,7 +527,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)
>  {
> @@ -2849,3 +2852,223 @@ 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 an iommu group that has *only* one device
> + *
> + * @group: The group for which the default domain should be changed
> + * @prev_dev: The device in the group (this is used to make sure that the
> device
> + *	 hasn't changed after the caller has called this function)
> + * @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
> + *    Please take a closer look if intended to use for other purposes.
> + */
> +static int iommu_change_dev_def_domain(struct iommu_group *group,
> +				       struct device *prev_dev, int type)
> +{
> +	struct iommu_domain *prev_dom;
> +	struct group_device *grp_dev;
> +	const struct iommu_ops *ops;
> +	int ret, dev_def_dom;
> +	struct device *dev;
> +
> +	if (!group)
> +		return -EINVAL;
> +
> +	mutex_lock(&group->mutex);
> +
> +	if (group->default_domain != group->domain) {
> +		pr_err_ratelimited("Group not assigned to default domain\n");
> +		ret = -EBUSY;
> +		goto out;
> +	}
> +
> +	/*
> +	 * iommu group wasn't locked while acquiring device lock in
> +	 * iommu_group_store_type(). So, make sure that the device count hasn't
> +	 * changed while acquiring device lock.
> +	 *
> +	 * Changing default domain of an iommu group with two or more devices
> +	 * isn't supported because there could be a potential deadlock. Consider
> +	 * the following scenario. T1 is trying to acquire device locks of all
> +	 * the devices in the group and before it could acquire all of them,
> +	 * there could be another thread T2 (from different sub-system and use
> +	 * case) that has already acquired some of the device locks and might be
> +	 * waiting for T1 to release other device locks.
> +	 */
> +	if (iommu_group_device_count(group) != 1) {
> +		pr_err_ratelimited("Cannot change default domain: Group has more
> than one device\n");
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	/* Since group has only one device */
> +	grp_dev = list_first_entry(&group->devices, struct group_device, list);
> +	dev = grp_dev->dev;
> +
> +	if (prev_dev != dev) {
> +		pr_err_ratelimited("Cannot change default domain: Device has been
> changed\n");
> +		ret = -EBUSY;
> +		goto out;
> +	}
> +
> +	prev_dom = group->default_domain;
> +	if (!prev_dom || !prev_dom->ops || !prev_dom->ops->def_domain_type) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	ops = prev_dom->ops;
> +	dev_def_dom = ops->def_domain_type(dev);
> +
> +	/* Check if user requested domain is supported by the device or not */
> +	if (!type) {
> +		/*
> +		 * If the user hasn't requested any specific type of domain and
> +		 * if the device supports both the domains, then default to the
> +		 * domain the device was booted with
> +		 */
> +		type = dev_def_dom ?: iommu_def_domain_type;
> +	} else if (dev_def_dom && type != dev_def_dom) {
> +		pr_err_ratelimited("Device cannot be in %s domain\n",
> +				   iommu_domain_type_str(type));
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	/*
> +	 * Switch to a new domain only if the requested domain type is different
> +	 * from the existing default domain type
> +	 */
> +	if (prev_dom->type == type) {
> +		ret = 0;
> +		goto out;
> +	}
> +
> +	/* Sets group->default_domain to the newly allocated domain */
> +	ret = iommu_group_alloc_default_domain(dev->bus, group, type);
> +	if (ret)
> +		goto out;
> +
> +	ret = iommu_create_device_direct_mappings(group, dev);
> +	if (ret)
> +		goto free_new_domain;
> +
> +	ret = __iommu_attach_device(group->default_domain, dev);
> +	if (ret)
> +		goto free_new_domain;
> +
> +	group->domain = group->default_domain;
> +
> +	/*
> +	 * Release the mutex here because ops->probe_finalize() call-back of
> +	 * some vendor IOMMU drivers calls arm_iommu_attach_device() which
> +	 * in-turn might call back into IOMMU core code, where it tries to take
> +	 * group->mutex, resulting in a deadlock.
> +	 */
> +	mutex_unlock(&group->mutex);
> +
> +	/* Make sure dma_ops is appropriatley set */
> +	iommu_group_do_probe_finalize(dev, group->default_domain);
> +	iommu_domain_free(prev_dom);
> +	return 0;
> +
> +free_new_domain:
> +	iommu_domain_free(group->default_domain);
> +	group->default_domain = prev_dom;
> +	group->domain = prev_dom;
> +
> +out:
> +	mutex_unlock(&group->mutex);
> +
> +	return ret;
> +}
> +
> +static ssize_t iommu_group_store_type(struct iommu_group *group,
> +				      const char *buf, size_t count)
> +{
> +	struct group_device *grp_dev;
> +	struct device *dev;
> +	int ret, req_type;
> +
> +	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_type = 0;
> +	else
> +		return -EINVAL;
> +
> +	/*
> +	 * Lock/Unlock the group mutex here before device lock to
> +	 * 1. Make sure that the iommu group has only one device (this is a
> +	 *    prerequisite for step 2)
> +	 * 2. Get struct *dev which is needed to lock device
> +	 */
> +	mutex_lock(&group->mutex);
> +	if (iommu_group_device_count(group) != 1) {
> +		mutex_unlock(&group->mutex);
> +		pr_err_ratelimited("Cannot change default domain: Group has more
> than one device\n");
> +		return -EINVAL;
> +	}
> +
> +	/* Since group has only one device */
> +	grp_dev = list_first_entry(&group->devices, struct group_device, list);
> +	dev = grp_dev->dev;
> +	get_device(dev);
> +
> +	/*
> +	 * Don't hold the group mutex because taking group mutex first and then
> +	 * the device lock could potentially cause a deadlock as below. Assume
> +	 * two threads T1 and T2. T1 is trying to change default domain of an
> +	 * iommu group and T2 is trying to hot unplug a device or release [1] VF
> +	 * of a PCIe device which is in the same iommu group. T1 takes group
> +	 * mutex and before it could take device lock assume T2 has taken device
> +	 * lock and is yet to take group mutex. Now, both the threads will be
> +	 * waiting for the other thread to release lock. Below, lock order was
> +	 * suggested.
> +	 * device_lock(dev);
> +	 *	mutex_lock(&group->mutex);
> +	 *		iommu_change_dev_def_domain();
> +	 *	mutex_unlock(&group->mutex);
> +	 * device_unlock(dev);
> +	 *
> +	 * [1] Typical device release path
> +	 * device_lock() from device/driver core code
> +	 *  -> bus_notifier()
> +	 *   -> iommu_bus_notifier()
> +	 *    -> iommu_release_device()
> +	 *     -> ops->release_device() vendor driver calls back iommu core
> code
> +	 *      -> mutex_lock() from iommu core code
> +	 */
> +	mutex_unlock(&group->mutex);
> +
> +	/* Check if the device in the group still has a driver bound to it */
> +	device_lock(dev);
> +	if (device_is_bound(dev)) {
> +		pr_err_ratelimited("Device is still bound to driver\n");
> +		ret = -EBUSY;
> +		goto out;
> +	}
> +
> +	ret = iommu_change_dev_def_domain(group, dev, req_type);
> +	ret = ret ?: count;
> +
> +out:
> +	device_unlock(dev);
> +	put_device(dev);
> +
> +	return ret;
> +}
> --
> 2.7.4
> 
> _______________________________________________
> iommu mailing list
> iommu@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [Patch V8 1/3] iommu: Add support to change default domain of an iommu group

[Lu Baolu]

Hi Shameer,

On 2020/11/20 19:27, Shameerali Kolothum Thodi wrote:
> Hi Baolu/Ashok,
> 
>> -----Original Message-----
>> From: iommu [mailto:iommu-bounces@lists.linux-foundation.org] On Behalf Of
>> Ashok Raj
>> Sent: 25 September 2020 20:06
>> To: Joerg Roedel <joro@8bytes.org>; iommu@lists.linux-foundation.org
>> Cc: Ashok Raj <ashok.raj@intel.com>; Will Deacon <will.deacon@arm.com>;
>> Robin Murphy <robin.murphy@arm.com>; Christoph Hellwig <hch@lst.de>
>> Subject: [Patch V8 1/3] iommu: Add support to change default domain of an
>> iommu group
>>
>> From: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>
>>
>> Presently, the default domain of an iommu group is allocated during boot
>> time and it cannot be changed later. So, the device would typically be
>> either in identity (also known as pass_through) mode 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 wherein the user trusts the device and believes that
>> the 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 use case
>> is not supported. It will be helpful if there is some way to change the
>> default domain of an iommu group dynamically. Hence, add such support.
>>
>> A privileged user could request the kernel to change the default domain
>> type of a iommu 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 device in this group are
>>               *not* translated by the iommu
>> 2. DMA: all the DMA transactions from the device in this group are
>>          translated by the iommu
>> 3. auto: change to the type the device was booted with
>>
>> Note:
>> 1. Default domain of an iommu group with two or more devices cannot be
>>     changed.
>> 2. The device in the iommu group shouldn't be bound to any driver.
>> 3. The device shouldn't be assigned to user for direct access.
>> 4. The vendor iommu driver is required to add def_domain_type() callback.
>>     The change request will fail if the request type conflicts with that
>>     returned from the callback.
> 
> Currently Arm SMMUv3 driver doesn't provide the def_doman_type() callback.
> And I have sent a patch[1] based on this series to just add that. But Robin made
> couple of suggestions there which can be incorporated into this series so that the
> vendor driver no more required to provide the callback for this feature to work.
> 
> 1. Include a generic checking for
> 	if (dev_is_pci(dev)) {
> 		if (pci_dev->untrusted)
> 			return IOMMU_DOMAIN_DMA;
> 	}

To be honest, I have the same idea. Okay! I can do this in the next
version.

> 
> 2. Also if there is no def_doman_type() callback provided by vendor driver, assume
>    that the dev supports both IDENTITY and DMA domain types.

It's true for boot case. I will assume this in this series.

> 
> If you plan to respin this series, could you please consider the above as well?
> Please let me know.

Sure!

> 
> Thanks,
> Shameer

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