[PATCH v6 08/25] iommu: Reorganize iommu_get_default_domain_type() to respect def_domain_type()
Baolu Lu
baolu.lu at linux.intel.com
Fri Aug 11 19:15:42 PDT 2023
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 at arm.com>
> Tested-by: Marek Szyprowski <m.szyprowski at samsung.com>
> Tested-by: Nicolin Chen <nicolinc at nvidia.com>
> Reviewed-by: Lu Baolu <baolu.lu at linux.intel.com>
> Signed-off-by: Jason Gunthorpe <jgg at 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
More information about the Linux-mediatek
mailing list