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

[Baolu Lu <baolu.lu@linux.intel.com>]

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