diff mbox series

[v6,08/25] iommu: Reorganize iommu_get_default_domain_type() to respect def_domain_type()

Message ID 8-v6-e8114faedade+425-iommu_all_defdom_jgg@nvidia.com
State New
Headers show
Series iommu: Make default_domain's mandatory | expand

Commit Message

Jason Gunthorpe Aug. 3, 2023, 12:07 a.m. UTC 
Except for dart every driver returns 0 or IDENTITY from def_domain_type().

The drivers that return IDENTITY have some kind of good reason, typically
that quirky hardware really can't support anything other than IDENTITY.

Arrange things so that if the driver says it needs IDENTITY then
iommu_get_default_domain_type() either fails or returns IDENTITY.  It will
never reject the driver's override to IDENTITY.

The only real functional difference is that the PCI untrusted flag is now
ignored for quirky HW instead of overriding the IOMMU driver.

This makes the next patch cleaner that wants to force IDENTITY always for
ARM_IOMMU because there is no support for DMA.

Tested-by: Steven Price <steven.price@arm.com>
Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
Tested-by: Nicolin Chen <nicolinc@nvidia.com>
Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/iommu.c | 66 +++++++++++++++++++++----------------------
 1 file changed, 33 insertions(+), 33 deletions(-)

Comments

Baolu Lu Aug. 12, 2023, 2:15 a.m. UTC | #1 
On 2023/8/3 8:07, Jason Gunthorpe wrote:
> Except for dart every driver returns 0 or IDENTITY from def_domain_type().
> 
> The drivers that return IDENTITY have some kind of good reason, typically
> that quirky hardware really can't support anything other than IDENTITY.
> 
> Arrange things so that if the driver says it needs IDENTITY then
> iommu_get_default_domain_type() either fails or returns IDENTITY.  It will
> never reject the driver's override to IDENTITY.
> 
> The only real functional difference is that the PCI untrusted flag is now
> ignored for quirky HW instead of overriding the IOMMU driver.
> 
> This makes the next patch cleaner that wants to force IDENTITY always for
> ARM_IOMMU because there is no support for DMA.
> 
> Tested-by: Steven Price <steven.price@arm.com>
> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Tested-by: Nicolin Chen <nicolinc@nvidia.com>
> Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>   drivers/iommu/iommu.c | 66 +++++++++++++++++++++----------------------
>   1 file changed, 33 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index c64365169b678d..53174179102d17 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -1669,19 +1669,6 @@ struct iommu_group *fsl_mc_device_group(struct device *dev)
>   }
>   EXPORT_SYMBOL_GPL(fsl_mc_device_group);
>   
> -static int iommu_get_def_domain_type(struct device *dev)
> -{
> -	const struct iommu_ops *ops = dev_iommu_ops(dev);
> -
> -	if (dev_is_pci(dev) && to_pci_dev(dev)->untrusted)
> -		return IOMMU_DOMAIN_DMA;
> -
> -	if (ops->def_domain_type)
> -		return ops->def_domain_type(dev);
> -
> -	return 0;
> -}
> -
>   static struct iommu_domain *
>   __iommu_group_alloc_default_domain(const struct bus_type *bus,
>   				   struct iommu_group *group, int req_type)
> @@ -1775,36 +1762,49 @@ static int iommu_bus_notifier(struct notifier_block *nb,
>   static int iommu_get_default_domain_type(struct iommu_group *group,
>   					 int target_type)
>   {
> +	const struct iommu_ops *ops = dev_iommu_ops(
> +		list_first_entry(&group->devices, struct group_device, list)
> +			->dev);

How about consolidating above into a single helper?

--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1787,6 +1787,21 @@ __iommu_group_alloc_default_domain(struct 
iommu_group *group, int req_type)
  	return __iommu_group_domain_alloc(group, req_type);
  }

+/*
+ * Returns the iommu_ops for the devices in an iommu group.
+ *
+ * It is assumed that all devices in an iommu group are managed by a single
+ * IOMMU unit. Therefore, this returns the dev_iommu_ops of the first 
device
+ * in the group.
+ */
+static const struct iommu_ops *group_iommu_ops(struct iommu_group *group)
+{
+	struct group_device *device;
+
+	device = list_first_entry(&group->devices, struct group_device, list);
+	return dev_iommu_ops(device->dev);
+}
+
  /*
   * req_type of 0 means "auto" which means to select a domain based on
   * iommu_def_domain_type or what the driver actually supports.
@@ -1794,10 +1809,7 @@ __iommu_group_alloc_default_domain(struct 
iommu_group *group, int req_type)
  static struct iommu_domain *
  iommu_group_alloc_default_domain(struct iommu_group *group, int req_type)
  {
-	struct device *dev =
-		list_first_entry(&group->devices, struct group_device, list)
-			->dev;
-	const struct iommu_ops *ops = dev_iommu_ops(dev);
+	const struct iommu_ops *ops = group_iommu_ops(group);
  	struct iommu_domain *dom;

  	lockdep_assert_held(&group->mutex);
@@ -1888,9 +1900,7 @@ static int iommu_bus_notifier(struct 
notifier_block *nb,
  static int iommu_get_default_domain_type(struct iommu_group *group,
  					 int target_type)
  {
-	const struct iommu_ops *ops = dev_iommu_ops(
-		list_first_entry(&group->devices, struct group_device, list)
-			->dev);
+	const struct iommu_ops *ops = group_iommu_ops(group);
  	int best_type = target_type;
  	struct group_device *gdev;
  	struct device *last_dev;
@@ -2124,13 +2134,9 @@ static struct iommu_domain 
*__iommu_domain_alloc(const struct iommu_ops *ops,
  static struct iommu_domain *
  __iommu_group_domain_alloc(struct iommu_group *group, unsigned int type)
  {
-	struct device *dev =
-		list_first_entry(&group->devices, struct group_device, list)
-			->dev;
-
  	lockdep_assert_held(&group->mutex);

-	return __iommu_domain_alloc(dev_iommu_ops(dev), dev, type);
+	return __iommu_domain_alloc(group_iommu_ops(group), dev, type);
  }

  struct iommu_domain *iommu_domain_alloc(const struct bus_type *bus)

>   	int best_type = target_type;
>   	struct group_device *gdev;
>   	struct device *last_dev;
> +	int type;
>   
>   	lockdep_assert_held(&group->mutex);
> -
>   	for_each_group_device(group, gdev) {
> -		unsigned int type = iommu_get_def_domain_type(gdev->dev);
> -
> -		if (best_type && type && best_type != type) {
> -			if (target_type) {
> -				dev_err_ratelimited(
> -					gdev->dev,
> -					"Device cannot be in %s domain\n",
> -					iommu_domain_type_str(target_type));
> -				return -1;
> -			}
> -
> -			dev_warn(
> -				gdev->dev,
> -				"Device needs domain type %s, but device %s in the same iommu group requires type %s - using default\n",
> -				iommu_domain_type_str(type), dev_name(last_dev),
> -				iommu_domain_type_str(best_type));
> -			return 0;
> +		type = best_type;
> +		if (ops->def_domain_type) {
> +			type = ops->def_domain_type(gdev->dev);
> +			if (best_type && type && best_type != type)
> +				goto err;
>   		}
> -		if (!best_type)
> -			best_type = type;
> +
> +		if (dev_is_pci(gdev->dev) && to_pci_dev(gdev->dev)->untrusted) {
> +			type = IOMMU_DOMAIN_DMA;
> +			if (best_type && type && best_type != type)
> +				goto err;
> +		}
> +		best_type = type;
>   		last_dev = gdev->dev;
>   	}
>   	return best_type;
> +
> +err:
> +	if (target_type) {
> +		dev_err_ratelimited(
> +			gdev->dev,
> +			"Device cannot be in %s domain - it is forcing %s\n",
> +			iommu_domain_type_str(target_type),
> +			iommu_domain_type_str(type));
> +		return -1;
> +	}
> +
> +	dev_warn(
> +		gdev->dev,
> +		"Device needs domain type %s, but device %s in the same iommu group requires type %s - using default\n",
> +		iommu_domain_type_str(type), dev_name(last_dev),
> +		iommu_domain_type_str(best_type));
> +	return 0;

This doesn't match the commit message, where it states:

"Arrange things so that if the driver says it needs IDENTITY then
  iommu_get_default_domain_type() either fails or returns IDENTITY.
"

I saw that this change was made in the sequential patch. It is probably
better to put that here?

>   }
>   
>   static void iommu_group_do_probe_finalize(struct device *dev)

Best regards,
baolu
Jason Gunthorpe Aug. 14, 2023, 5:25 p.m. UTC | #2 
On Sat, Aug 12, 2023 at 10:15:42AM +0800, Baolu Lu wrote:

> 
> How about consolidating above into a single helper?
> 
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -1787,6 +1787,21 @@ __iommu_group_alloc_default_domain(struct iommu_group
> *group, int req_type)
>  	return __iommu_group_domain_alloc(group, req_type);
>  }
> 
> +/*
> + * Returns the iommu_ops for the devices in an iommu group.
> + *
> + * It is assumed that all devices in an iommu group are managed by a single
> + * IOMMU unit. Therefore, this returns the dev_iommu_ops of the first
> device
> + * in the group.
> + */
> +static const struct iommu_ops *group_iommu_ops(struct iommu_group *group)
> +{
> +	struct group_device *device;
> +
> +	device = list_first_entry(&group->devices, struct group_device, list);
> +	return dev_iommu_ops(device->dev);
> +}

Okay I did this, but it doesn't help as much..

> @@ -2124,13 +2134,9 @@ static struct iommu_domain
> *__iommu_domain_alloc(const struct iommu_ops *ops,
>  static struct iommu_domain *
>  __iommu_group_domain_alloc(struct iommu_group *group, unsigned int type)
>  {
> -	struct device *dev =
> -		list_first_entry(&group->devices, struct group_device, list)
> -			->dev;
> -
>  	lockdep_assert_held(&group->mutex);
> 
> -	return __iommu_domain_alloc(dev_iommu_ops(dev), dev, type);
> +	return __iommu_domain_alloc(group_iommu_ops(group), dev, type);
>  }

Since this is all still needed to calculate dev

> > +err:
> > +	if (target_type) {
> > +		dev_err_ratelimited(
> > +			gdev->dev,
> > +			"Device cannot be in %s domain - it is forcing %s\n",
> > +			iommu_domain_type_str(target_type),
> > +			iommu_domain_type_str(type));
> > +		return -1;
> > +	}
> > +
> > +	dev_warn(
> > +		gdev->dev,
> > +		"Device needs domain type %s, but device %s in the same iommu group requires type %s - using default\n",
> > +		iommu_domain_type_str(type), dev_name(last_dev),
> > +		iommu_domain_type_str(best_type));
> > +	return 0;
> 
> This doesn't match the commit message, where it states:
> 
> "Arrange things so that if the driver says it needs IDENTITY then
>  iommu_get_default_domain_type() either fails or returns IDENTITY.
> "
> 
> I saw that this change was made in the sequential patch. It is probably
> better to put that here?

Ah, I went over all this again and decided to try again, it is too
complicated. This patch can do what the commit message says and the
following patches are even simpler:

/*
 * Combine the driver's choosen def_domain_type across all the devices in a
 * group. Drivers must give a consistent result.
 */
static int iommu_get_def_domain_type(struct iommu_group *group,
				     struct device *dev, int cur_type)
{
	const struct iommu_ops *ops = group_iommu_ops(group);
	int type;

	if (!ops->def_domain_type)
		return cur_type;

	type = ops->def_domain_type(dev);
	if (!type || cur_type == type)
		return cur_type;
	if (!cur_type)
		return type;

	dev_err_ratelimited(
		dev,
		"IOMMU driver error, requesting conflicting def_domain_type, %s and %s, for devices in group %u.\n",
		iommu_domain_type_str(cur_type), iommu_domain_type_str(type),
		group->id);

	/*
	 * Try to recover, drivers are allowed to force IDENITY or DMA, IDENTITY
	 * takes precedence.
	 */
	if (cur_type || type == IOMMU_DOMAIN_IDENTITY)
		return IOMMU_DOMAIN_IDENTITY;
	return cur_type;
}

/*
 * A target_type of 0 will select the best domain type. 0 can be returned in
 * this case meaning the global default should be used.
 */
static int iommu_get_default_domain_type(struct iommu_group *group,
					 int target_type)
{
	struct device *untrusted = NULL;
	struct group_device *gdev;
	int driver_type = 0;

	lockdep_assert_held(&group->mutex);

	/*
	 * ARM32 drivers supporting CONFIG_ARM_DMA_USE_IOMMU can declare an
	 * identity_domain and it will automatically become their default
	 * domain. Later on ARM_DMA_USE_IOMMU will install its UNMANAGED domain.
	 * Override the selection to IDENTITY.
	 */
	if (IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU)) {
		static_assert(!(IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU) &&
				IS_ENABLED(CONFIG_IOMMU_DMA)));
		driver_type = IOMMU_DOMAIN_IDENTITY;
	}

	for_each_group_device(group, gdev) {
		driver_type = iommu_get_def_domain_type(group, gdev->dev,
							driver_type);

		if (dev_is_pci(gdev->dev) && to_pci_dev(gdev->dev)->untrusted) {
			/*
			 * No ARM32 using systems will set untrusted, it cannot
			 * work.
			 */
			if (WARN_ON(IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU)))
				return -1;
			untrusted = gdev->dev;
		}
	}

	if (untrusted) {
		if (driver_type && driver_type != IOMMU_DOMAIN_DMA) {
			dev_err_ratelimited(
				untrusted,
				"Device is not trusted, but driver is overriding group %u to %s, refusing to probe.\n",
				group->id, iommu_domain_type_str(driver_type));
			return -1;
		}
		driver_type = IOMMU_DOMAIN_DMA;
	}

	if (target_type) {
		if (driver_type && target_type != driver_type)
			return -1;
		return target_type;
	}
	return driver_type;
}

Thanks,
Jason
Baolu Lu Aug. 15, 2023, 1:18 a.m. UTC | #3 
On 2023/8/15 1:25, Jason Gunthorpe wrote:
> Ah, I went over all this again and decided to try again, it is too
> complicated. This patch can do what the commit message says and the
> following patches are even simpler:

Yes. This method is more concise. Some nits below.

> 
> /*
>   * Combine the driver's choosen def_domain_type across all the devices in a
>   * group. Drivers must give a consistent result.
>   */
> static int iommu_get_def_domain_type(struct iommu_group *group,
> 				     struct device *dev, int cur_type)
> {
> 	const struct iommu_ops *ops = group_iommu_ops(group);
> 	int type;
> 
> 	if (!ops->def_domain_type)
> 		return cur_type;
> 
> 	type = ops->def_domain_type(dev);
> 	if (!type || cur_type == type)
> 		return cur_type;
> 	if (!cur_type)
> 		return type;
> 
> 	dev_err_ratelimited(
> 		dev,
> 		"IOMMU driver error, requesting conflicting def_domain_type, %s and %s, for devices in group %u.\n",
> 		iommu_domain_type_str(cur_type), iommu_domain_type_str(type),
> 		group->id);
> 
> 	/*
> 	 * Try to recover, drivers are allowed to force IDENITY or DMA, IDENTITY
> 	 * takes precedence.
> 	 */
> 	if (cur_type || type == IOMMU_DOMAIN_IDENTITY)
> 		return IOMMU_DOMAIN_IDENTITY;

No need to check cur_type. It already returned if cur_type is 0.

> 	return cur_type;
> }
> 
> /*
>   * A target_type of 0 will select the best domain type. 0 can be returned in
>   * this case meaning the global default should be used.
>   */
> static int iommu_get_default_domain_type(struct iommu_group *group,
> 					 int target_type)
> {
> 	struct device *untrusted = NULL;
> 	struct group_device *gdev;
> 	int driver_type = 0;
> 
> 	lockdep_assert_held(&group->mutex);
> 
> 	/*
> 	 * ARM32 drivers supporting CONFIG_ARM_DMA_USE_IOMMU can declare an
> 	 * identity_domain and it will automatically become their default
> 	 * domain. Later on ARM_DMA_USE_IOMMU will install its UNMANAGED domain.
> 	 * Override the selection to IDENTITY.
> 	 */
> 	if (IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU)) {
> 		static_assert(!(IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU) &&
> 				IS_ENABLED(CONFIG_IOMMU_DMA)));

IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU) is duplicate with the condition in 
the if statement. So only
		static_assert(!IS_ENABLED(CONFIG_IOMMU_DMA));
?

> 		driver_type = IOMMU_DOMAIN_IDENTITY;
> 	}
> 
> 	for_each_group_device(group, gdev) {
> 		driver_type = iommu_get_def_domain_type(group, gdev->dev,
> 							driver_type);

No need to call this in the loop body?

> 
> 		if (dev_is_pci(gdev->dev) && to_pci_dev(gdev->dev)->untrusted) {
> 			/*
> 			 * No ARM32 using systems will set untrusted, it cannot
> 			 * work.
> 			 */
> 			if (WARN_ON(IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU)))
> 				return -1;
> 			untrusted = gdev->dev;
> 		}
> 	}
> 
> 	if (untrusted) {
> 		if (driver_type && driver_type != IOMMU_DOMAIN_DMA) {
> 			dev_err_ratelimited(
> 				untrusted,
> 				"Device is not trusted, but driver is overriding group %u to %s, refusing to probe.\n",
> 				group->id, iommu_domain_type_str(driver_type));
> 			return -1;
> 		}
> 		driver_type = IOMMU_DOMAIN_DMA;
> 	}
> 
> 	if (target_type) {
> 		if (driver_type && target_type != driver_type)
> 			return -1;
> 		return target_type;
> 	}
> 	return driver_type;
> }

Best regards,
baolu
Jason Gunthorpe Aug. 16, 2023, 12:44 p.m. UTC | #4 
On Tue, Aug 15, 2023 at 09:18:59AM +0800, Baolu Lu wrote:

> > 	/*
> > 	 * Try to recover, drivers are allowed to force IDENITY or DMA, IDENTITY
> > 	 * takes precedence.
> > 	 */
> > 	if (cur_type || type == IOMMU_DOMAIN_IDENTITY)
> > 		return IOMMU_DOMAIN_IDENTITY;
> 
> No need to check cur_type. It already returned if cur_type is 0.

Yep
 
> > 	return cur_type;
> > }
> > 
> > /*
> >   * A target_type of 0 will select the best domain type. 0 can be returned in
> >   * this case meaning the global default should be used.
> >   */
> > static int iommu_get_default_domain_type(struct iommu_group *group,
> > 					 int target_type)
> > {
> > 	struct device *untrusted = NULL;
> > 	struct group_device *gdev;
> > 	int driver_type = 0;
> > 
> > 	lockdep_assert_held(&group->mutex);
> > 
> > 	/*
> > 	 * ARM32 drivers supporting CONFIG_ARM_DMA_USE_IOMMU can declare an
> > 	 * identity_domain and it will automatically become their default
> > 	 * domain. Later on ARM_DMA_USE_IOMMU will install its UNMANAGED domain.
> > 	 * Override the selection to IDENTITY.
> > 	 */
> > 	if (IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU)) {
> > 		static_assert(!(IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU) &&
> > 				IS_ENABLED(CONFIG_IOMMU_DMA)));
> 
> IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU) is duplicate with the condition in the
> if statement. So only
> 		static_assert(!IS_ENABLED(CONFIG_IOMMU_DMA));
> ?

static_assert doesn't work that way, it ignores its calling context
and always checks during compilation, so the duplication is required

> > 
> > 	for_each_group_device(group, gdev) {
> > 		driver_type = iommu_get_def_domain_type(group, gdev->dev,
> > 							driver_type);
> 
> No need to call this in the loop body?

Do need it, this only gets the def_domain_type of a single device so
we have to iterate over all the devices in the group to 'reduce' the
type for the group.

Thanks,
Jason
diff mbox series

Patch

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index c64365169b678d..53174179102d17 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1669,19 +1669,6 @@  struct iommu_group *fsl_mc_device_group(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(fsl_mc_device_group);
 
-static int iommu_get_def_domain_type(struct device *dev)
-{
-	const struct iommu_ops *ops = dev_iommu_ops(dev);
-
-	if (dev_is_pci(dev) && to_pci_dev(dev)->untrusted)
-		return IOMMU_DOMAIN_DMA;
-
-	if (ops->def_domain_type)
-		return ops->def_domain_type(dev);
-
-	return 0;
-}
-
 static struct iommu_domain *
 __iommu_group_alloc_default_domain(const struct bus_type *bus,
 				   struct iommu_group *group, int req_type)
@@ -1775,36 +1762,49 @@  static int iommu_bus_notifier(struct notifier_block *nb,
 static int iommu_get_default_domain_type(struct iommu_group *group,
 					 int target_type)
 {
+	const struct iommu_ops *ops = dev_iommu_ops(
+		list_first_entry(&group->devices, struct group_device, list)
+			->dev);
 	int best_type = target_type;
 	struct group_device *gdev;
 	struct device *last_dev;
+	int type;
 
 	lockdep_assert_held(&group->mutex);
-
 	for_each_group_device(group, gdev) {
-		unsigned int type = iommu_get_def_domain_type(gdev->dev);
-
-		if (best_type && type && best_type != type) {
-			if (target_type) {
-				dev_err_ratelimited(
-					gdev->dev,
-					"Device cannot be in %s domain\n",
-					iommu_domain_type_str(target_type));
-				return -1;
-			}
-
-			dev_warn(
-				gdev->dev,
-				"Device needs domain type %s, but device %s in the same iommu group requires type %s - using default\n",
-				iommu_domain_type_str(type), dev_name(last_dev),
-				iommu_domain_type_str(best_type));
-			return 0;
+		type = best_type;
+		if (ops->def_domain_type) {
+			type = ops->def_domain_type(gdev->dev);
+			if (best_type && type && best_type != type)
+				goto err;
 		}
-		if (!best_type)
-			best_type = type;
+
+		if (dev_is_pci(gdev->dev) && to_pci_dev(gdev->dev)->untrusted) {
+			type = IOMMU_DOMAIN_DMA;
+			if (best_type && type && best_type != type)
+				goto err;
+		}
+		best_type = type;
 		last_dev = gdev->dev;
 	}
 	return best_type;
+
+err:
+	if (target_type) {
+		dev_err_ratelimited(
+			gdev->dev,
+			"Device cannot be in %s domain - it is forcing %s\n",
+			iommu_domain_type_str(target_type),
+			iommu_domain_type_str(type));
+		return -1;
+	}
+
+	dev_warn(
+		gdev->dev,
+		"Device needs domain type %s, but device %s in the same iommu group requires type %s - using default\n",
+		iommu_domain_type_str(type), dev_name(last_dev),
+		iommu_domain_type_str(best_type));
+	return 0;
 }
 
 static void iommu_group_do_probe_finalize(struct device *dev)