Re: [PATCH RESEND v5 1/5] iommu: Refactor iommu_group_store_type()
On Wed, Jul 06, 2022 at 01:03:44PM +0100, John Garry wrote: > On 06/07/2022 13:00, Will Deacon wrote: > > On Mon, Apr 04, 2022 at 07:27:10PM +0800, John Garry wrote: > > > Function iommu_group_store_type() supports changing the default domain > > > of an IOMMU group. > > > > > > Many conditions need to be satisfied and steps taken for this action to be > > > successful. > > > > > > Satisfying these conditions and steps will be required for setting other > > > IOMMU group attributes, so factor into a common part and a part specific > > > to update the IOMMU group attribute. > > > > > > No functional change intended. > > > > > > Some code comments are tidied up also. > > > > > > Signed-off-by: John Garry > > > --- > > > drivers/iommu/iommu.c | 96 --- > > > 1 file changed, 62 insertions(+), 34 deletions(-) > > Acked-by: Will Deacon > > > > Thanks, but currently I have no plans to progress this series, in favour of > this > https://lore.kernel.org/linux-iommu/1656590892-42307-1-git-send-email-john.ga...@huawei.com/T/#me0e806913050c95f6e6ba2c7f7d96d51ce191204 heh, then I'll stop reviewing it then :) Shame, I quite liked it so far! Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH RESEND v5 1/5] iommu: Refactor iommu_group_store_type()
On 06/07/2022 13:00, Will Deacon wrote: On Mon, Apr 04, 2022 at 07:27:10PM +0800, John Garry wrote: Function iommu_group_store_type() supports changing the default domain of an IOMMU group. Many conditions need to be satisfied and steps taken for this action to be successful. Satisfying these conditions and steps will be required for setting other IOMMU group attributes, so factor into a common part and a part specific to update the IOMMU group attribute. No functional change intended. Some code comments are tidied up also. Signed-off-by: John Garry --- drivers/iommu/iommu.c | 96 --- 1 file changed, 62 insertions(+), 34 deletions(-) Acked-by: Will Deacon Thanks, but currently I have no plans to progress this series, in favour of this https://lore.kernel.org/linux-iommu/1656590892-42307-1-git-send-email-john.ga...@huawei.com/T/#me0e806913050c95f6e6ba2c7f7d96d51ce191204 cheers ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH RESEND v5 1/5] iommu: Refactor iommu_group_store_type()
On Mon, Apr 04, 2022 at 07:27:10PM +0800, John Garry wrote: > Function iommu_group_store_type() supports changing the default domain > of an IOMMU group. > > Many conditions need to be satisfied and steps taken for this action to be > successful. > > Satisfying these conditions and steps will be required for setting other > IOMMU group attributes, so factor into a common part and a part specific > to update the IOMMU group attribute. > > No functional change intended. > > Some code comments are tidied up also. > > Signed-off-by: John Garry > --- > drivers/iommu/iommu.c | 96 --- > 1 file changed, 62 insertions(+), 34 deletions(-) Acked-by: Will Deacon Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH RESEND v5 1/5] iommu: Refactor iommu_group_store_type()
Reviewed-by: Zhen Lei
On 2022/4/4 19:27, John Garry wrote:
> Function iommu_group_store_type() supports changing the default domain
> of an IOMMU group.
>
> Many conditions need to be satisfied and steps taken for this action to be
> successful.
>
> Satisfying these conditions and steps will be required for setting other
> IOMMU group attributes, so factor into a common part and a part specific
> to update the IOMMU group attribute.
>
> No functional change intended.
>
> Some code comments are tidied up also.
>
> Signed-off-by: John Garry
> ---
> drivers/iommu/iommu.c | 96 ---
> 1 file changed, 62 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index f2c45b85b9fc..0dd766030baf 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -3000,21 +3000,57 @@ static int iommu_change_dev_def_domain(struct
> iommu_group *group,
> return ret;
> }
>
> +enum iommu_group_op {
> + CHANGE_GROUP_TYPE,
> +};
> +
> +static int __iommu_group_store_type(const char *buf, struct iommu_group
> *group,
> + struct device *dev)
> +{
> + int type;
> +
> + if (sysfs_streq(buf, "identity"))
> + type = IOMMU_DOMAIN_IDENTITY;
> + else if (sysfs_streq(buf, "DMA"))
> + type = IOMMU_DOMAIN_DMA;
> + else if (sysfs_streq(buf, "DMA-FQ"))
> + type = IOMMU_DOMAIN_DMA_FQ;
> + else if (sysfs_streq(buf, "auto"))
> + type = 0;
> + else
> + return -EINVAL;
> +
> + /*
> + * Check if the only device in the group still has a driver bound or
> + * we're transistioning from DMA -> DMA-FQ
> + */
> + if (device_is_bound(dev) && !(type == IOMMU_DOMAIN_DMA_FQ &&
> + group->default_domain->type == IOMMU_DOMAIN_DMA)) {
> + pr_err_ratelimited("Device is still bound to driver\n");
> + return -EINVAL;
> + }
> +
> + return iommu_change_dev_def_domain(group, dev, type);
> +}
> +
> /*
> * Changing the default domain through sysfs requires the users to unbind the
> * drivers from the devices in the iommu group, except for a DMA -> DMA-FQ
> - * transition. Return failure if this isn't met.
> + * transition. Changing or any other IOMMU group attribute still requires the
> + * user to unbind the drivers from the devices in the iommu group. Return
> + * failure if these conditions are not met.
> *
> * We need to consider the race between this and the device release path.
> * device_lock(dev) is used here to guarantee that the device release path
> * will not be entered at the same time.
> */
> -static ssize_t iommu_group_store_type(struct iommu_group *group,
> - const char *buf, size_t count)
> +static ssize_t iommu_group_store_common(struct iommu_group *group,
> + enum iommu_group_op op,
> + const char *buf, size_t count)
> {
> struct group_device *grp_dev;
> struct device *dev;
> - int ret, req_type;
> + int ret;
>
> if (!capable(CAP_SYS_ADMIN) || !capable(CAP_SYS_RAWIO))
> return -EACCES;
> @@ -3022,27 +3058,16 @@ static ssize_t iommu_group_store_type(struct
> iommu_group *group,
> 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, "DMA-FQ"))
> - req_type = IOMMU_DOMAIN_DMA_FQ;
> - 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
> + * 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(>mutex);
> if (iommu_group_device_count(group) != 1) {
> mutex_unlock(>mutex);
> - pr_err_ratelimited("Cannot change default domain: Group has
> more than one device\n");
> + pr_err_ratelimited("Cannot change IOMMU group default domain
> attribute: Group has more than one device\n");
> return -EINVAL;
> }
>
> @@ -3054,16 +3079,16 @@ static ssize_t iommu_group_store_type(struct
> iommu_group *group,
> /*
>* 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
[PATCH RESEND v5 1/5] iommu: Refactor iommu_group_store_type()
Function iommu_group_store_type() supports changing the default domain
of an IOMMU group.
Many conditions need to be satisfied and steps taken for this action to be
successful.
Satisfying these conditions and steps will be required for setting other
IOMMU group attributes, so factor into a common part and a part specific
to update the IOMMU group attribute.
No functional change intended.
Some code comments are tidied up also.
Signed-off-by: John Garry
---
drivers/iommu/iommu.c | 96 ---
1 file changed, 62 insertions(+), 34 deletions(-)
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index f2c45b85b9fc..0dd766030baf 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -3000,21 +3000,57 @@ static int iommu_change_dev_def_domain(struct
iommu_group *group,
return ret;
}
+enum iommu_group_op {
+ CHANGE_GROUP_TYPE,
+};
+
+static int __iommu_group_store_type(const char *buf, struct iommu_group *group,
+ struct device *dev)
+{
+ int type;
+
+ if (sysfs_streq(buf, "identity"))
+ type = IOMMU_DOMAIN_IDENTITY;
+ else if (sysfs_streq(buf, "DMA"))
+ type = IOMMU_DOMAIN_DMA;
+ else if (sysfs_streq(buf, "DMA-FQ"))
+ type = IOMMU_DOMAIN_DMA_FQ;
+ else if (sysfs_streq(buf, "auto"))
+ type = 0;
+ else
+ return -EINVAL;
+
+ /*
+* Check if the only device in the group still has a driver bound or
+* we're transistioning from DMA -> DMA-FQ
+*/
+ if (device_is_bound(dev) && !(type == IOMMU_DOMAIN_DMA_FQ &&
+ group->default_domain->type == IOMMU_DOMAIN_DMA)) {
+ pr_err_ratelimited("Device is still bound to driver\n");
+ return -EINVAL;
+ }
+
+ return iommu_change_dev_def_domain(group, dev, type);
+}
+
/*
* Changing the default domain through sysfs requires the users to unbind the
* drivers from the devices in the iommu group, except for a DMA -> DMA-FQ
- * transition. Return failure if this isn't met.
+ * transition. Changing or any other IOMMU group attribute still requires the
+ * user to unbind the drivers from the devices in the iommu group. Return
+ * failure if these conditions are not met.
*
* We need to consider the race between this and the device release path.
* device_lock(dev) is used here to guarantee that the device release path
* will not be entered at the same time.
*/
-static ssize_t iommu_group_store_type(struct iommu_group *group,
- const char *buf, size_t count)
+static ssize_t iommu_group_store_common(struct iommu_group *group,
+ enum iommu_group_op op,
+ const char *buf, size_t count)
{
struct group_device *grp_dev;
struct device *dev;
- int ret, req_type;
+ int ret;
if (!capable(CAP_SYS_ADMIN) || !capable(CAP_SYS_RAWIO))
return -EACCES;
@@ -3022,27 +3058,16 @@ static ssize_t iommu_group_store_type(struct
iommu_group *group,
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, "DMA-FQ"))
- req_type = IOMMU_DOMAIN_DMA_FQ;
- 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
+* 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(>mutex);
if (iommu_group_device_count(group) != 1) {
mutex_unlock(>mutex);
- pr_err_ratelimited("Cannot change default domain: Group has
more than one device\n");
+ pr_err_ratelimited("Cannot change IOMMU group default domain
attribute: Group has more than one device\n");
return -EINVAL;
}
@@ -3054,16 +3079,16 @@ static ssize_t iommu_group_store_type(struct
iommu_group *group,
/*
* 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
