IOMMU Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] iommu: Remove functions that support private domain
@ 2020-05-13 22:47 Sai Praneeth Prakhya
  2020-05-14 13:13 ` Joerg Roedel
  2020-05-15 10:01 ` Joerg Roedel
  0 siblings, 2 replies; 15+ messages in thread
From: Sai Praneeth Prakhya @ 2020-05-13 22:47 UTC (permalink / raw)
  To: iommu

After moving iommu_group setup to iommu core code [1][2] and removing
private domain support in vt-d [3], there are no users for functions such
as iommu_request_dm_for_dev(), iommu_request_dma_domain_for_dev() and
request_default_domain_for_dev(). So, remove these functions.

[1] commit dce8d6964ebd ("iommu/amd: Convert to probe/release_device()
    call-backs")
[2] commit e5d1841f18b2 ("iommu/vt-d: Convert to probe/release_device()
    call-backs")
[3] commit 327d5b2fee91 ("iommu/vt-d: Allow 32bit devices to uses DMA
    domain")

Cc: Joerg Roedel <joro@8bytes.org>
Cc: Lu Baolu <baolu.lu@linux.intel.com>
Signed-off-by: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>
---
 drivers/iommu/iommu.c | 65 -------------------------------------------
 include/linux/iommu.h | 12 --------
 2 files changed, 77 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 4050569188be..374b34fd6fac 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -2536,71 +2536,6 @@ struct iommu_resv_region *iommu_alloc_resv_region(phys_addr_t start,
 }
 EXPORT_SYMBOL_GPL(iommu_alloc_resv_region);
 
-static int
-request_default_domain_for_dev(struct device *dev, unsigned long type)
-{
-	struct iommu_domain *domain;
-	struct iommu_group *group;
-	int ret;
-
-	/* Device must already be in a group before calling this function */
-	group = iommu_group_get(dev);
-	if (!group)
-		return -EINVAL;
-
-	mutex_lock(&group->mutex);
-
-	ret = 0;
-	if (group->default_domain && group->default_domain->type == type)
-		goto out;
-
-	/* Don't change mappings of existing devices */
-	ret = -EBUSY;
-	if (iommu_group_device_count(group) != 1)
-		goto out;
-
-	ret = -ENOMEM;
-	domain = __iommu_domain_alloc(dev->bus, type);
-	if (!domain)
-		goto out;
-
-	/* Attach the device to the domain */
-	ret = __iommu_attach_group(domain, group);
-	if (ret) {
-		iommu_domain_free(domain);
-		goto out;
-	}
-
-	/* Make the domain the default for this group */
-	if (group->default_domain)
-		iommu_domain_free(group->default_domain);
-	group->default_domain = domain;
-
-	iommu_create_device_direct_mappings(group, dev);
-
-	dev_info(dev, "Using iommu %s mapping\n",
-		 type == IOMMU_DOMAIN_DMA ? "dma" : "direct");
-
-	ret = 0;
-out:
-	mutex_unlock(&group->mutex);
-	iommu_group_put(group);
-
-	return ret;
-}
-
-/* Request that a device is direct mapped by the IOMMU */
-int iommu_request_dm_for_dev(struct device *dev)
-{
-	return request_default_domain_for_dev(dev, IOMMU_DOMAIN_IDENTITY);
-}
-
-/* Request that a device can't be direct mapped by the IOMMU */
-int iommu_request_dma_domain_for_dev(struct device *dev)
-{
-	return request_default_domain_for_dev(dev, IOMMU_DOMAIN_DMA);
-}
-
 void iommu_set_default_passthrough(bool cmd_line)
 {
 	if (cmd_line)
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 7cfd2dddb49d..78a26ae5c2b6 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -482,8 +482,6 @@ extern void iommu_get_resv_regions(struct device *dev, struct list_head *list);
 extern void iommu_put_resv_regions(struct device *dev, struct list_head *list);
 extern void generic_iommu_put_resv_regions(struct device *dev,
 					   struct list_head *list);
-extern int iommu_request_dm_for_dev(struct device *dev);
-extern int iommu_request_dma_domain_for_dev(struct device *dev);
 extern void iommu_set_default_passthrough(bool cmd_line);
 extern void iommu_set_default_translated(bool cmd_line);
 extern bool iommu_default_passthrough(void);
@@ -804,16 +802,6 @@ static inline int iommu_get_group_resv_regions(struct iommu_group *group,
 	return -ENODEV;
 }
 
-static inline int iommu_request_dm_for_dev(struct device *dev)
-{
-	return -ENODEV;
-}
-
-static inline int iommu_request_dma_domain_for_dev(struct device *dev)
-{
-	return -ENODEV;
-}
-
 static inline void iommu_set_default_passthrough(bool cmd_line)
 {
 }
-- 
2.19.1

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

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

* Re: [PATCH] iommu: Remove functions that support private domain
  2020-05-13 22:47 [PATCH] iommu: Remove functions that support private domain Sai Praneeth Prakhya
@ 2020-05-14 13:13 ` Joerg Roedel
  2020-05-14 17:51   ` Prakhya, Sai Praneeth
  2020-05-15 10:01 ` Joerg Roedel
  1 sibling, 1 reply; 15+ messages in thread
From: Joerg Roedel @ 2020-05-14 13:13 UTC (permalink / raw)
  To: Sai Praneeth Prakhya; +Cc: iommu

On Wed, May 13, 2020 at 03:47:21PM -0700, Sai Praneeth Prakhya wrote:
> After moving iommu_group setup to iommu core code [1][2] and removing
> private domain support in vt-d [3], there are no users for functions such
> as iommu_request_dm_for_dev(), iommu_request_dma_domain_for_dev() and
> request_default_domain_for_dev(). So, remove these functions.

I thought these functions are needed for the per-group default-domain
patch-set? That is why I left them in for now, but I can also remove
them if not.


	Joerg

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

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

* RE: [PATCH] iommu: Remove functions that support private domain
  2020-05-14 13:13 ` Joerg Roedel
@ 2020-05-14 17:51   ` Prakhya, Sai Praneeth
  2020-05-14 18:32     ` Joerg Roedel
  0 siblings, 1 reply; 15+ messages in thread
From: Prakhya, Sai Praneeth @ 2020-05-14 17:51 UTC (permalink / raw)
  To: Joerg Roedel; +Cc: iommu

Hi Joerg,

> -----Original Message-----
> From: Joerg Roedel <joro@8bytes.org>
> Sent: Thursday, May 14, 2020 6:13 AM
> To: Prakhya, Sai Praneeth <sai.praneeth.prakhya@intel.com>
> Cc: iommu@lists.linux-foundation.org; Lu Baolu <baolu.lu@linux.intel.com>
> Subject: Re: [PATCH] iommu: Remove functions that support private domain
> 
> On Wed, May 13, 2020 at 03:47:21PM -0700, Sai Praneeth Prakhya wrote:
> > After moving iommu_group setup to iommu core code [1][2] and removing
> > private domain support in vt-d [3], there are no users for functions
> > such as iommu_request_dm_for_dev(),
> iommu_request_dma_domain_for_dev()
> > and request_default_domain_for_dev(). So, remove these functions.
> 
> I thought these functions are needed for the per-group default-domain patch-
> set? That is why I left them in for now, but I can also remove them if not.

Sorry! didn't get that quite well. When you meant "per-group default-domain patch-set", do you mean the patch set that I am working on which changes iommu group default domain dynamically by writing to sysfs file?
If so, they don't use these functions so it should be ok to remove them. If you meant some other patch set, could you please point me to them?

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

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

* Re: [PATCH] iommu: Remove functions that support private domain
  2020-05-14 17:51   ` Prakhya, Sai Praneeth
@ 2020-05-14 18:32     ` Joerg Roedel
  2020-05-14 18:44       ` Prakhya, Sai Praneeth
  0 siblings, 1 reply; 15+ messages in thread
From: Joerg Roedel @ 2020-05-14 18:32 UTC (permalink / raw)
  To: Prakhya, Sai Praneeth; +Cc: iommu

On Thu, May 14, 2020 at 05:51:39PM +0000, Prakhya, Sai Praneeth wrote:
> Sorry! didn't get that quite well. When you meant "per-group
> default-domain patch-set", do you mean the patch set that I am working
> on which changes iommu group default domain dynamically by writing to
> sysfs file?

Not only the sysfs file, but also changing it at boot already. Note that
changing the default-domain at runtime is only possible for
single-device groups.

I'll queue that patch tomorrow.

Regards,

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

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

* RE: [PATCH] iommu: Remove functions that support private domain
  2020-05-14 18:32     ` Joerg Roedel
@ 2020-05-14 18:44       ` Prakhya, Sai Praneeth
  2020-05-14 19:56         ` Joerg Roedel
  0 siblings, 1 reply; 15+ messages in thread
From: Prakhya, Sai Praneeth @ 2020-05-14 18:44 UTC (permalink / raw)
  To: Joerg Roedel; +Cc: iommu

Hi Joerg,

> -----Original Message-----
> From: Joerg Roedel <joro@8bytes.org>
> Sent: Thursday, May 14, 2020 11:33 AM
> To: Prakhya, Sai Praneeth <sai.praneeth.prakhya@intel.com>
> Cc: iommu@lists.linux-foundation.org; Lu Baolu <baolu.lu@linux.intel.com>
> Subject: Re: [PATCH] iommu: Remove functions that support private domain
> 
> On Thu, May 14, 2020 at 05:51:39PM +0000, Prakhya, Sai Praneeth wrote:
> > Sorry! didn't get that quite well. When you meant "per-group
> > default-domain patch-set", do you mean the patch set that I am working
> > on which changes iommu group default domain dynamically by writing to
> > sysfs file?
> 
> Not only the sysfs file, but also changing it at boot already. Note that changing
> the default-domain at runtime is only possible for single-device groups.

Could you please explain why we shouldn't change default-domain for an iommu group that has multiple devices?

I am asking this particularly because the patch set I am working on allows to change default-domain for an iommu group that has multiple devices. The pre-requisite being that all the devices in the group should already be unbounded from the device driver and the default-domain preferences of all the devices in the group shouldn't have conflicting types i.e. some devices cannot say they *only* need identity domain while other devices in the same group say that they *only* need to be in DMA domain. In this case, we will not be able to decide upon a default-domain for the iommu group.

> I'll queue that patch tomorrow.

Great! I will take a look at it.

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

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

* Re: [PATCH] iommu: Remove functions that support private domain
  2020-05-14 18:44       ` Prakhya, Sai Praneeth
@ 2020-05-14 19:56         ` Joerg Roedel
  2020-05-14 23:12           ` Prakhya, Sai Praneeth
  0 siblings, 1 reply; 15+ messages in thread
From: Joerg Roedel @ 2020-05-14 19:56 UTC (permalink / raw)
  To: Prakhya, Sai Praneeth; +Cc: iommu

On Thu, May 14, 2020 at 06:44:16PM +0000, Prakhya, Sai Praneeth wrote:
> Could you please explain why we shouldn't change default-domain for an
> iommu group that has multiple devices?

Because you can't be sure that a device is bound to a driver while the
default domain of the group is changed. As long as this race condition
exists we can't change the default domains of groups with multiple
devices at runtime.

> I am asking this particularly because the patch set I am working on
> allows to change default-domain for an iommu group that has multiple
> devices. The pre-requisite being that all the devices in the group
> should already be unbounded from the device driver and the
> default-domain preferences of all the devices in the group shouldn't
> have conflicting types i.e. some devices cannot say they *only* need
> identity domain while other devices in the same group say that they
> *only* need to be in DMA domain. In this case, we will not be able to
> decide upon a default-domain for the iommu group.

Yeah, but as I wrote above, this is racy and there is currently no way
to fix that. So we can't support it.


Regards,

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

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

* RE: [PATCH] iommu: Remove functions that support private domain
  2020-05-14 19:56         ` Joerg Roedel
@ 2020-05-14 23:12           ` Prakhya, Sai Praneeth
  2020-05-15  9:59             ` Joerg Roedel
  0 siblings, 1 reply; 15+ messages in thread
From: Prakhya, Sai Praneeth @ 2020-05-14 23:12 UTC (permalink / raw)
  To: Joerg Roedel; +Cc: iommu

Hi Joerg,

> -----Original Message-----
> From: Joerg Roedel <joro@8bytes.org>
> Sent: Thursday, May 14, 2020 12:56 PM
> To: Prakhya, Sai Praneeth <sai.praneeth.prakhya@intel.com>
> Cc: iommu@lists.linux-foundation.org; Lu Baolu <baolu.lu@linux.intel.com>
> Subject: Re: [PATCH] iommu: Remove functions that support private domain
> 
> On Thu, May 14, 2020 at 06:44:16PM +0000, Prakhya, Sai Praneeth wrote:
> > Could you please explain why we shouldn't change default-domain for an
> > iommu group that has multiple devices?
> 
> Because you can't be sure that a device is bound to a driver while the default
> domain of the group is changed. As long as this race condition exists we can't
> change the default domains of groups with multiple devices at runtime.
> 
> > I am asking this particularly because the patch set I am working on
> > allows to change default-domain for an iommu group that has multiple
> > devices. The pre-requisite being that all the devices in the group
> > should already be unbounded from the device driver and the
> > default-domain preferences of all the devices in the group shouldn't
> > have conflicting types i.e. some devices cannot say they *only* need
> > identity domain while other devices in the same group say that they
> > *only* need to be in DMA domain. In this case, we will not be able to
> > decide upon a default-domain for the iommu group.
> 
> Yeah, but as I wrote above, this is racy and there is currently no way to fix that.
> So we can't support it.

Ok.. below is a previous *similar* comment that you had for one of my patches that change default-domain dynamically.. could you please elaborate it more so that I could have better understanding?
Specifically this one.. "locking all devices in the group and then do the re-attach will introduce a lock-inversion with group->mutex". I didn't get which function call chain would lead us to lock inversion. Could you please shed some light here?

+static int is_driver_bound(struct device *dev, void *not_used)
+{
+	int ret = 0;
+
+	device_lock(dev);
+	if (device_is_bound(dev))
+		ret = 1;
+	device_unlock(dev);
+	return ret;
+}

[SNIP]

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

Another question I have is.. if it's racy then it should be racy even for one device iommu groups.. right? Why would it be racy only with multiple devices iommu group?

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

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

* Re: [PATCH] iommu: Remove functions that support private domain
  2020-05-14 23:12           ` Prakhya, Sai Praneeth
@ 2020-05-15  9:59             ` Joerg Roedel
  2020-05-15 12:55               ` Lu Baolu
  2020-05-15 18:35               ` Prakhya, Sai Praneeth
  0 siblings, 2 replies; 15+ messages in thread
From: Joerg Roedel @ 2020-05-15  9:59 UTC (permalink / raw)
  To: Prakhya, Sai Praneeth; +Cc: iommu

On Thu, May 14, 2020 at 11:12:52PM +0000, Prakhya, Sai Praneeth wrote:
> +static int is_driver_bound(struct device *dev, void *not_used)
> +{
> +	int ret = 0;
> +
> +	device_lock(dev);
> +	if (device_is_bound(dev))
> +		ret = 1;
> +	device_unlock(dev);
> +	return ret;
> +}

This locks only one device, so without lock-conversion there could be a
driver probe after the device_unlock(), while we are probing the other
devices of the group.

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

The lock inversion comes into the picture when this code is called from
device(-driver) core through the bus-notifiers. The notifiers are called
with the device already locked.

> Another question I have is.. if it's racy then it should be racy even
> for one device iommu groups.. right? Why would it be racy only with
> multiple devices iommu group?

Valid point. So the device needs to be locked _while_ the default domain
change happens. If triggered by sysfs there should be no locking
problems, I guess. But you better try it out.

Regards,

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

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

* Re: [PATCH] iommu: Remove functions that support private domain
  2020-05-13 22:47 [PATCH] iommu: Remove functions that support private domain Sai Praneeth Prakhya
  2020-05-14 13:13 ` Joerg Roedel
@ 2020-05-15 10:01 ` Joerg Roedel
  1 sibling, 0 replies; 15+ messages in thread
From: Joerg Roedel @ 2020-05-15 10:01 UTC (permalink / raw)
  To: Sai Praneeth Prakhya; +Cc: iommu

On Wed, May 13, 2020 at 03:47:21PM -0700, Sai Praneeth Prakhya wrote:
> After moving iommu_group setup to iommu core code [1][2] and removing
> private domain support in vt-d [3], there are no users for functions such
> as iommu_request_dm_for_dev(), iommu_request_dma_domain_for_dev() and
> request_default_domain_for_dev(). So, remove these functions.
> 
> [1] commit dce8d6964ebd ("iommu/amd: Convert to probe/release_device()
>     call-backs")
> [2] commit e5d1841f18b2 ("iommu/vt-d: Convert to probe/release_device()
>     call-backs")
> [3] commit 327d5b2fee91 ("iommu/vt-d: Allow 32bit devices to uses DMA
>     domain")
> 
> Cc: Joerg Roedel <joro@8bytes.org>
> Cc: Lu Baolu <baolu.lu@linux.intel.com>
> Signed-off-by: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>
> ---
>  drivers/iommu/iommu.c | 65 -------------------------------------------
>  include/linux/iommu.h | 12 --------
>  2 files changed, 77 deletions(-)

Applied that patch to the x86/vt-d branch.

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

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

* Re: [PATCH] iommu: Remove functions that support private domain
  2020-05-15  9:59             ` Joerg Roedel
@ 2020-05-15 12:55               ` Lu Baolu
  2020-05-15 15:46                 ` Joerg Roedel
  2020-05-15 18:35               ` Prakhya, Sai Praneeth
  1 sibling, 1 reply; 15+ messages in thread
From: Lu Baolu @ 2020-05-15 12:55 UTC (permalink / raw)
  To: Joerg Roedel, Prakhya, Sai Praneeth; +Cc: iommu

Hi,

On 2020/5/15 17:59, Joerg Roedel wrote:
> On Thu, May 14, 2020 at 11:12:52PM +0000, Prakhya, Sai Praneeth wrote:
>> +static int is_driver_bound(struct device *dev, void *not_used)
>> +{
>> +	int ret = 0;
>> +
>> +	device_lock(dev);
>> +	if (device_is_bound(dev))
>> +		ret = 1;
>> +	device_unlock(dev);
>> +	return ret;
>> +}
> 
> This locks only one device, so without lock-conversion there could be a
> driver probe after the device_unlock(), while we are probing the other
> devices of the group.
> 
>> [SNIP]
>>
>> +	/*
>> +	 * Check if any device in the group still has a driver binded to it.
>> +	 * This might race with device driver probing code and unfortunately
>> +	 * there is no clean way out of that either, locking all devices in the
>> +	 * group and then do the re-attach will introduce a lock-inversion with
>> +	 * group->mutex - Joerg.
>> +	 */
>> +	if (iommu_group_for_each_dev(group, NULL, is_driver_bound)) {
>> +		pr_err("Active drivers exist for devices in the group\n");
>> +		return -EBUSY;
>> +	}
> 
> The lock inversion comes into the picture when this code is called from
> device(-driver) core through the bus-notifiers. The notifiers are called
> with the device already locked.
> 
>> Another question I have is.. if it's racy then it should be racy even
>> for one device iommu groups.. right? Why would it be racy only with
>> multiple devices iommu group?
> 
> Valid point. So the device needs to be locked _while_ the default domain
> change happens. If triggered by sysfs there should be no locking
> problems, I guess. But you better try it out.

It seems that we can do like this:

[1] mutex_lock(&group->lock)
[2] for_each_group_device(device_lock())
[3] if (for_each_group_device(!device_is_bound()))
	change_default_domain()
[4] for_each_group_device_reverse(device_unlock())
[5] mutex_unlock(&group->lock)

A possible problem exists at step 2 when another thread is trying to
lock devices in the reverse order at the same time.

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

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

* Re: [PATCH] iommu: Remove functions that support private domain
  2020-05-15 12:55               ` Lu Baolu
@ 2020-05-15 15:46                 ` Joerg Roedel
  2020-05-17  8:29                   ` Prakhya, Sai Praneeth
  0 siblings, 1 reply; 15+ messages in thread
From: Joerg Roedel @ 2020-05-15 15:46 UTC (permalink / raw)
  To: Lu Baolu; +Cc: iommu

On Fri, May 15, 2020 at 08:55:42PM +0800, Lu Baolu wrote:
> It seems that we can do like this:
> 
> [1] mutex_lock(&group->lock)
> [2] for_each_group_device(device_lock())
> [3] if (for_each_group_device(!device_is_bound()))
> 	change_default_domain()
> [4] for_each_group_device_reverse(device_unlock())
> [5] mutex_unlock(&group->lock)

The problem here is that I am pretty sure we also have:

	device_lock() /* from device/driver core code */
	-> bus_notifier()
	  -> iommu_bus_notifier()
	    -> ...
	      -> mutex_lock(&group->lock)

Which would cause lock-inversion with the above code.


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

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

* RE: [PATCH] iommu: Remove functions that support private domain
  2020-05-15  9:59             ` Joerg Roedel
  2020-05-15 12:55               ` Lu Baolu
@ 2020-05-15 18:35               ` Prakhya, Sai Praneeth
  1 sibling, 0 replies; 15+ messages in thread
From: Prakhya, Sai Praneeth @ 2020-05-15 18:35 UTC (permalink / raw)
  To: Joerg Roedel; +Cc: iommu

Hi Joerg,

> -----Original Message-----
> From: Joerg Roedel <joro@8bytes.org>
> Sent: Friday, May 15, 2020 2:59 AM
> To: Prakhya, Sai Praneeth <sai.praneeth.prakhya@intel.com>
> Cc: iommu@lists.linux-foundation.org; Lu Baolu <baolu.lu@linux.intel.com>
> Subject: Re: [PATCH] iommu: Remove functions that support private domain
> 
> On Thu, May 14, 2020 at 11:12:52PM +0000, Prakhya, Sai Praneeth wrote:
> > +static int is_driver_bound(struct device *dev, void *not_used) {
> > +	int ret = 0;
> > +
> > +	device_lock(dev);
> > +	if (device_is_bound(dev))
> > +		ret = 1;
> > +	device_unlock(dev);
> > +	return ret;
> > +}
> 
> This locks only one device, so without lock-conversion there could be a driver
> probe after the device_unlock(), while we are probing the other devices of the
> group.
> 
> > [SNIP]
> >
> > +	/*
> > +	 * Check if any device in the group still has a driver binded to it.
> > +	 * This might race with device driver probing code and unfortunately
> > +	 * there is no clean way out of that either, locking all devices in the
> > +	 * group and then do the re-attach will introduce a lock-inversion with
> > +	 * group->mutex - Joerg.
> > +	 */
> > +	if (iommu_group_for_each_dev(group, NULL, is_driver_bound)) {
> > +		pr_err("Active drivers exist for devices in the group\n");
> > +		return -EBUSY;
> > +	}
> 
> The lock inversion comes into the picture when this code is called from
> device(-driver) core through the bus-notifiers. The notifiers are called with the
> device already locked.

Make sense. I will look through that code.

> > Another question I have is.. if it's racy then it should be racy even
> > for one device iommu groups.. right? Why would it be racy only with
> > multiple devices iommu group?
> 
> Valid point. So the device needs to be locked _while_ the default domain change
> happens. If triggered by sysfs there should be no locking problems, I guess. But
> you better try it out.

I will try this out and will update you.

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

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

* RE: [PATCH] iommu: Remove functions that support private domain
  2020-05-15 15:46                 ` Joerg Roedel
@ 2020-05-17  8:29                   ` Prakhya, Sai Praneeth
  2020-05-25 13:56                     ` Joerg Roedel
  0 siblings, 1 reply; 15+ messages in thread
From: Prakhya, Sai Praneeth @ 2020-05-17  8:29 UTC (permalink / raw)
  To: Joerg Roedel, Lu Baolu; +Cc: iommu

> -----Original Message-----
> From: Joerg Roedel <joro@8bytes.org>
> Sent: Friday, May 15, 2020 8:46 AM
> To: Lu Baolu <baolu.lu@linux.intel.com>
> Cc: Prakhya, Sai Praneeth <sai.praneeth.prakhya@intel.com>;
> iommu@lists.linux-foundation.org
> Subject: Re: [PATCH] iommu: Remove functions that support private domain
> 
> On Fri, May 15, 2020 at 08:55:42PM +0800, Lu Baolu wrote:
> > It seems that we can do like this:
> >
> > [1] mutex_lock(&group->lock)
> > [2] for_each_group_device(device_lock())
> > [3] if (for_each_group_device(!device_is_bound()))
> > 	change_default_domain()
> > [4] for_each_group_device_reverse(device_unlock())
> > [5] mutex_unlock(&group->lock)

>> A possible problem exists at step 2 when another thread is trying to lock devices in the reverse order at the same time. -> By Allen

Makes sense.. this could happen and we could prevent this if the iommu_group has only one device. So, going with Joerg's suggestion, if we support dynamic switching of default-domain of a group with only one device, I think we shouldn't hit this issue.

> The problem here is that I am pretty sure we also have:
> 
> 	device_lock() /* from device/driver core code */
> 	-> bus_notifier()
> 	  -> iommu_bus_notifier()
> 	    -> ...
> 	      -> mutex_lock(&group->lock)
> 
> Which would cause lock-inversion with the above code.

Thanks for this call chain, Joerg. It makes sense to me how lock inversion could happen

iommu_bus_notifier()
-> iommu_release_device()
 -> ops->release_device() (Eg: intel_iommu_release_device())
  -> iommu_group_remove_device()
   -> mutex_lock()

But, I added print statements to iommu_bus_notifier() and iommu_group_remove_device() and noticed that iommu_group_remove_device() wasn't being called upon modprobe -r <driver_name> and iommu_probe_device() isn't called upon modprobe <driver_name> because

	if (action == BUS_NOTIFY_ADD_DEVICE) {
		int ret;

		ret = iommu_probe_device(dev);
		return (ret) ? NOTIFY_DONE : NOTIFY_OK;
	} else if (action == BUS_NOTIFY_REMOVED_DEVICE) {
		iommu_release_device(dev);
		return NOTIFY_OK;
	}

"action" was != BUS_NOTIFY_ADD_DEVICE || BUS_NOTIFY_REMOVED_DEVICE upon modprobe. So (after booting [1]), I am wondering when would action be == to one of the above so that these iommu functions get called.

And,
	switch (action) {
	case BUS_NOTIFY_BIND_DRIVER:
		group_action = IOMMU_GROUP_NOTIFY_BIND_DRIVER;
		break;
	case BUS_NOTIFY_BOUND_DRIVER:
		group_action = IOMMU_GROUP_NOTIFY_BOUND_DRIVER;
		break;
	case BUS_NOTIFY_UNBIND_DRIVER:
		group_action = IOMMU_GROUP_NOTIFY_UNBIND_DRIVER;
		break;
	case BUS_NOTIFY_UNBOUND_DRIVER:
		group_action = IOMMU_GROUP_NOTIFY_UNBOUND_DRIVER;
		break;
	}

	if (group_action)
		blocking_notifier_call_chain(&group->notifier,
					     group_action, dev);

I also noticed that vfio is the only one that registers for group notifiers and from a first look it wasn't very obvious if vfio is trying to get group->mutex.

[1] I am interested in after booting because dynamic switching of iommu_group default-domain is supported only through sysfs.

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

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

* Re: [PATCH] iommu: Remove functions that support private domain
  2020-05-17  8:29                   ` Prakhya, Sai Praneeth
@ 2020-05-25 13:56                     ` Joerg Roedel
  2020-05-28 19:31                       ` Prakhya, Sai Praneeth
  0 siblings, 1 reply; 15+ messages in thread
From: Joerg Roedel @ 2020-05-25 13:56 UTC (permalink / raw)
  To: Prakhya, Sai Praneeth; +Cc: iommu

On Sun, May 17, 2020 at 08:29:17AM +0000, Prakhya, Sai Praneeth wrote:
> iommu_bus_notifier()
> -> iommu_release_device()
>  -> ops->release_device() (Eg: intel_iommu_release_device())
>   -> iommu_group_remove_device()
>    -> mutex_lock()
> 
> But, I added print statements to iommu_bus_notifier() and
> iommu_group_remove_device() and noticed that
> iommu_group_remove_device() wasn't being called upon modprobe -r
> <driver_name> and iommu_probe_device() isn't called upon modprobe
> <driver_name> because

Calling modprobe is the device driver binding path, the release_device
event is called when a device is going away, e.g. on hotunplug or when a
VF of a PCI device is released.

This could also happen at runtime, and the code needs to protect against
that. My suggestion is still to limit runtime-changing of default
domains to groups with only one device. The flow would be as follows:


	1. device_lock(dev);
	   // Device can't be bound to a driver or hot-removed from now
	   // on.

	2. if (device_is_bound_to_some_driver(dev))
	           goto 6;

	3. mutex_lock(&group->mutex);
	   // group of dev

	4. Change the default domain

	5. mutex_unlock(&group->mutex);

	6. device_unlock(dev);

This avoids lock inversion and should be safe with regard to hotplug and
device driver probing.

Regards,

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

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

* RE: [PATCH] iommu: Remove functions that support private domain
  2020-05-25 13:56                     ` Joerg Roedel
@ 2020-05-28 19:31                       ` Prakhya, Sai Praneeth
  0 siblings, 0 replies; 15+ messages in thread
From: Prakhya, Sai Praneeth @ 2020-05-28 19:31 UTC (permalink / raw)
  To: Joerg Roedel; +Cc: iommu

Hi Joerg,

> -----Original Message-----
> From: Joerg Roedel <joro@8bytes.org>
> Sent: Monday, May 25, 2020 6:57 AM
> To: Prakhya, Sai Praneeth <sai.praneeth.prakhya@intel.com>
> Cc: Lu Baolu <baolu.lu@linux.intel.com>; iommu@lists.linux-foundation.org
> Subject: Re: [PATCH] iommu: Remove functions that support private domain
> 
> On Sun, May 17, 2020 at 08:29:17AM +0000, Prakhya, Sai Praneeth wrote:
> > iommu_bus_notifier()
> > -> iommu_release_device()
> >  -> ops->release_device() (Eg: intel_iommu_release_device())
> >   -> iommu_group_remove_device()
> >    -> mutex_lock()
> >
> > But, I added print statements to iommu_bus_notifier() and
> > iommu_group_remove_device() and noticed that
> > iommu_group_remove_device() wasn't being called upon modprobe -r
> > <driver_name> and iommu_probe_device() isn't called upon modprobe
> > <driver_name> because
> 
> Calling modprobe is the device driver binding path, the release_device event is
> called when a device is going away, e.g. on hotunplug or when a VF of a PCI
> device is released.
> 
> This could also happen at runtime, and the code needs to protect against that.
> My suggestion is still to limit runtime-changing of default domains to groups
> with only one device. The flow would be as follows:

Thanks for explaining how lock release path could be called at run time. It makes sense to me now.

> 
> 	1. device_lock(dev);
> 	   // Device can't be bound to a driver or hot-removed from now
> 	   // on.
> 
> 	2. if (device_is_bound_to_some_driver(dev))
> 	           goto 6;
> 
> 	3. mutex_lock(&group->mutex);
> 	   // group of dev
> 
> 	4. Change the default domain
> 
> 	5. mutex_unlock(&group->mutex);
> 
> 	6. device_unlock(dev);
> 
> This avoids lock inversion and should be safe with regard to hotplug and device
> driver probing.

Thanks for the steps. I have implemented "changing default domain" following the steps you gave above. Could you please review the patch set when you have some time and let me know your comments?

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

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

end of thread, back to index

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-13 22:47 [PATCH] iommu: Remove functions that support private domain Sai Praneeth Prakhya
2020-05-14 13:13 ` Joerg Roedel
2020-05-14 17:51   ` Prakhya, Sai Praneeth
2020-05-14 18:32     ` Joerg Roedel
2020-05-14 18:44       ` Prakhya, Sai Praneeth
2020-05-14 19:56         ` Joerg Roedel
2020-05-14 23:12           ` Prakhya, Sai Praneeth
2020-05-15  9:59             ` Joerg Roedel
2020-05-15 12:55               ` Lu Baolu
2020-05-15 15:46                 ` Joerg Roedel
2020-05-17  8:29                   ` Prakhya, Sai Praneeth
2020-05-25 13:56                     ` Joerg Roedel
2020-05-28 19:31                       ` Prakhya, Sai Praneeth
2020-05-15 18:35               ` Prakhya, Sai Praneeth
2020-05-15 10:01 ` Joerg Roedel

IOMMU Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-iommu/0 linux-iommu/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-iommu linux-iommu/ https://lore.kernel.org/linux-iommu \
		iommu@lists.linux-foundation.org
	public-inbox-index linux-iommu

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.linux-foundation.lists.iommu


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git