RE: [PATCH V4 1/3] iommu: Add support to change default domain of an iommu group
Hi Joerg, Thanks for the reply. I will spin another version of the patch addressing your comments. > -Original Message- > From: Joerg Roedel > Sent: Wednesday, July 22, 2020 6:53 AM > To: Prakhya, Sai Praneeth > Cc: Raj, Ashok ; Will Deacon ; > iommu@lists.linux-foundation.org; Robin Murphy ; > Christoph Hellwig > Subject: Re: [PATCH V4 1/3] iommu: Add support to change default domain of > an iommu group > > On Tue, Jul 14, 2020 at 06:23:54PM +, Prakhya, Sai Praneeth wrote: > > Q1: > > > Presently, iommu_change_dev_def_domain() checks if the iommu group > > > still has only one device or not. Hence, checking if iommu group has > > > one device or not is done twice, once before taking device_lock() > > > and the other, after taking device_lock(). > > > > > > I agree that the code isn't checking if the iommu group still has > > > the _same_ device or not. > > > One way, I could think of doing it is by storing "dev" temporarily > > > and checking for it. > > > Do you think that's ok? Or would you rather suggest something else? > > That sounds reasonable, get the device from the group, lock it, take > group->mutex, and check whether the same device is still alone in the > group. Sounds good! I will implement this. Regards, Sai ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH V4 1/3] iommu: Add support to change default domain of an iommu group
On Tue, Jul 14, 2020 at 06:23:54PM +, Prakhya, Sai Praneeth wrote: > Q1: > > Presently, iommu_change_dev_def_domain() checks if the iommu group still has > > only one device or not. Hence, checking if iommu group has one device or > > not is > > done twice, once before taking device_lock() and the other, after taking > > device_lock(). > > > > I agree that the code isn't checking if the iommu group still has the _same_ > > device or not. > > One way, I could think of doing it is by storing "dev" temporarily and > > checking > > for it. > > Do you think that's ok? Or would you rather suggest something else? That sounds reasonable, get the device from the group, lock it, take group->mutex, and check whether the same device is still alone in the group. > Q2: > > The reason for taking iommu_group->mutex in the beginning of > > iommu_change_dev_def_domain() is that the function > > > > 1. Checks if the group is being directly used by user level drivers (i.e. > > if (group- > > >default_domain != group->domain)) > > > > 2. Uses iommu_ops > > (prev_dom = group->default_domain; > > if (!prev_dom || !prev_dom->ops || !prev_dom->ops->def_domain_type)) > > > > 3. Sets iomu_group->domain to iommu_group->default_domain > > > > I wanted to make sure that iommu_group->domain and iommu_group- > > >default_domain aren't changed by some other thread while this thread is > > working on it. So, please let me know if I misunderstood something. This looks correct as well. Regards, Joerg ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH V4 1/3] iommu: Add support to change default domain of an iommu group
Hi Joerg, Replying again because I noticed that I couldn't find this mail in the external iommu mailing list while I was able to find your comments on my patch. Also, could you please answer my two questions below? > -Original Message- > From: iommu On Behalf Of > Prakhya, Sai Praneeth > Sent: Tuesday, June 30, 2020 8:04 PM > To: Joerg Roedel > Cc: Raj, Ashok ; Will Deacon ; > iommu@lists.linux-foundation.org; Robin Murphy ; > Christoph Hellwig > Subject: RE: [PATCH V4 1/3] iommu: Add support to change default domain of > an iommu group > > Hi Joerg, > > > -Original Message- > > From: Joerg Roedel > > Sent: Tuesday, June 30, 2020 2:16 AM > > To: Prakhya, Sai Praneeth > > Cc: iommu@lists.linux-foundation.org; Christoph Hellwig ; > > Raj, Ashok ; Will Deacon ; > > Lu Baolu ; Mehta, Sohil > > ; Robin Murphy ; Jacob > > Pan > > Subject: Re: [PATCH V4 1/3] iommu: Add support to change default > > domain of an iommu group > > > > On Thu, Jun 04, 2020 at 06:32:06PM -0700, Sai Praneeth Prakhya wrote: > > > +static int iommu_change_dev_def_domain(struct iommu_group *group, > > > +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 assigned to user level for direct > > > +access\n"); > > > > Make this message: "Group not assigned to default domain\n". > > Sure! I will change it > > > > + 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 of a group > > with > > > +two or more devices\n"); > > > > "Can not change default domain: Group has more than one device\n" > > Ok.. make sense. I will change this. > > > > + ret = -EINVAL; > > > + goto out; > > > + } > > > + > > > + /* Since group has only one device */ > > > + list_for_each_entry(grp_dev, &group->devices, list) > > > + dev = grp_dev->dev; > > > + > > > + prev_dom = group->default_domain; > > > + if (!prev_dom || !prev_dom->ops || !prev_dom->ops- > > >def_domain_type) { > > > + pr_err_ratelimited("'def_domain_type' call back isn't > > > +registered\n"); > > > > This message isn't needed. > > Ok. I will remove it. > > > > + ret = __iommu_attach_device(group->default_domain, dev); > > > + if (ret) > > > + goto free_new_domain; > > > + > > > + group->domain = group->default_domain; > > > + > > > + ret = iommu_create_device_direct_mappings(group, dev); > > > + if (ret) > > > + goto free_new_domain; > > > > You need to create the direct mappings before you attach the device to > > the new domain. Otherwise there might be a short time-window where > > RMRR regions are not mapped. > > Ok.. makes sense. I will change this accordingly. > > > > +static ssize_t iommu_group_store_type(struct iommu_group *group, > > > + const char *buf, size_t count) { > > > + struct group_device *grp_dev; > > >
RE: [PATCH V4 1/3] iommu: Add support to change default domain of an iommu group
Hi Joerg, > -Original Message- > From: Joerg Roedel > Sent: Tuesday, June 30, 2020 2:16 AM > To: Prakhya, Sai Praneeth > Cc: iommu@lists.linux-foundation.org; Christoph Hellwig ; Raj, > Ashok ; Will Deacon ; Lu Baolu > ; Mehta, Sohil ; Robin > Murphy ; Jacob Pan > Subject: Re: [PATCH V4 1/3] iommu: Add support to change default domain of > an iommu group > > On Thu, Jun 04, 2020 at 06:32:06PM -0700, Sai Praneeth Prakhya wrote: > > +static int iommu_change_dev_def_domain(struct iommu_group *group, 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 assigned to user level for direct > > +access\n"); > > Make this message: "Group not assigned to default domain\n". Sure! I will change it > > + 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 of a group > with > > +two or more devices\n"); > > "Can not change default domain: Group has more than one device\n" Ok.. make sense. I will change this. > > + ret = -EINVAL; > > + goto out; > > + } > > + > > + /* Since group has only one device */ > > + list_for_each_entry(grp_dev, &group->devices, list) > > + dev = grp_dev->dev; > > + > > + prev_dom = group->default_domain; > > + if (!prev_dom || !prev_dom->ops || !prev_dom->ops- > >def_domain_type) { > > + pr_err_ratelimited("'def_domain_type' call back isn't > > +registered\n"); > > This message isn't needed. Ok. I will remove it. > > + ret = __iommu_attach_device(group->default_domain, dev); > > + if (ret) > > + goto free_new_domain; > > + > > + group->domain = group->default_domain; > > + > > + ret = iommu_create_device_direct_mappings(group, dev); > > + if (ret) > > + goto free_new_domain; > > You need to create the direct mappings before you attach the device to the new > domain. Otherwise there might be a short time-window where RMRR regions > are not mapped. Ok.. makes sense. I will change this accordingly. > > +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)
Re: [PATCH V4 1/3] iommu: Add support to change default domain of an iommu group
On Thu, Jun 04, 2020 at 06:32:06PM -0700, Sai Praneeth Prakhya wrote: > +static int iommu_change_dev_def_domain(struct iommu_group *group, 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 assigned to user level for direct > access\n"); Make this message: "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 of a group > with two or more devices\n"); "Can not change default domain: Group has more than one device\n" > + ret = -EINVAL; > + goto out; > + } > + > + /* Since group has only one device */ > + list_for_each_entry(grp_dev, &group->devices, list) > + dev = grp_dev->dev; > + > + prev_dom = group->default_domain; > + if (!prev_dom || !prev_dom->ops || !prev_dom->ops->def_domain_type) { > + pr_err_ratelimited("'def_domain_type' call back isn't > registered\n"); This message isn't needed. > + ret = __iommu_attach_device(group->default_domain, dev); > + if (ret) > + goto free_new_domain; > + > + group->domain = group->default_domain; > + > + ret = iommu_create_device_direct_mappings(group, dev); > + if (ret) > + goto free_new_domain; You need to create the direct mappings before you attach the device to the new domain. Otherwise there might be a short time-window where RMRR regions are not mapped. > +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 of a group > with two or more devices\n"); > + return -EINVAL; > + } > + > + /* Since group has only one device */ > + list_for_each_entry(grp_dev, &group->devices, list) > + dev = grp_dev->dev; Please use list_first_entry(). You also need to take a reference with get_device() and then drop the group->mutex. After device_lock() you need to verify that the device is still in the same group and that the group has still only one device in it. Then you can call down to iommu_change_dev_def_domain() which does not need to take the group-mutex by itself. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH V4 1/3] iommu: Add support to change default domain of an iommu group
On 6/5/20 9:32 AM, Sai Praneeth Prakhya 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 (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//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 Cc: Joerg Roedel Cc: Ashok Raj Cc: Will Deacon Cc: Lu Baolu This patch looks good to me now. Reviewed-by: Lu Baolu Best regards, baolu Cc: Sohil Mehta Cc: Robin Murphy Cc: Jacob Pan Signed-off-by: Sai Praneeth Prakhya --- drivers/iommu/iommu.c | 215 +- 1 file changed, 214 insertions(+), 1 deletion(-) diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index d43120eb1dc5..b023f06f12d6 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) { @@ -2838,3 +2841,213 @@ 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 + * + * @group: The group for which the default domain should be changed + * @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//type + *Please take a closer look if intended to use for other purposes. + */ +static int iommu_change_dev_def_domain(struct iommu_group *group, 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 assigned to user level for direct access\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 poten
[PATCH V4 1/3] iommu: Add support to change default domain of an iommu group
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//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 Cc: Joerg Roedel Cc: Ashok Raj Cc: Will Deacon Cc: Lu Baolu Cc: Sohil Mehta Cc: Robin Murphy Cc: Jacob Pan Signed-off-by: Sai Praneeth Prakhya --- drivers/iommu/iommu.c | 215 +- 1 file changed, 214 insertions(+), 1 deletion(-) diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index d43120eb1dc5..b023f06f12d6 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) { @@ -2838,3 +2841,213 @@ 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 + * + * @group: The group for which the default domain should be changed + * @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//type + *Please take a closer look if intended to use for other purposes. + */ +static int iommu_change_dev_def_domain(struct iommu_group *group, 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 assigned to user level for direct access\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 b