From a4fdd976227240b06ced89b5df88a1a1f388f092 Mon Sep 17 00:00:00 2001 From: Robin Murphy Date: Thu, 4 May 2023 22:10:56 +0100 Subject: iommu: Use flush queue capability It remains really handy to have distinct DMA domain types within core code for the sake of default domain policy selection, but we can now hide that detail from drivers by using the new capability instead. Signed-off-by: Robin Murphy Tested-by: Jerry Snitselaar # amd, intel, smmu-v3 Reviewed-by: Jerry Snitselaar Reviewed-by: Jason Gunthorpe Link: https://lore.kernel.org/r/1c552d99e8ba452bdac48209fa74c0bdd52fd9d9.1683233867.git.robin.murphy@arm.com Signed-off-by: Joerg Roedel --- drivers/iommu/iommu.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'drivers/iommu/iommu.c') diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index f1dcfa3f1a1b..7078bf4a8ec8 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -1980,11 +1980,12 @@ static struct iommu_domain *__iommu_domain_alloc(const struct bus_type *bus, unsigned type) { struct iommu_domain *domain; + unsigned int alloc_type = type & IOMMU_DOMAIN_ALLOC_FLAGS; if (bus == NULL || bus->iommu_ops == NULL) return NULL; - domain = bus->iommu_ops->domain_alloc(type); + domain = bus->iommu_ops->domain_alloc(alloc_type); if (!domain) return NULL; -- cgit v1.2.3 From 32261d10943b7fffa864f9a532e2b40a813df79b Mon Sep 17 00:00:00 2001 From: Florian Fainelli Date: Tue, 9 May 2023 12:10:48 -0700 Subject: iommu: Suppress empty whitespaces in prints If IOMMU_CMD_LINE_DMA_API or IOMMU_CMD_LINE_STRICT are not set in iommu_cmd_line, we will be emitting a whitespace before the newline. Signed-off-by: Florian Fainelli Reviewed-by: Jason Gunthorpe Link: https://lore.kernel.org/r/20230509191049.1752259-1-f.fainelli@gmail.com Signed-off-by: Joerg Roedel --- drivers/iommu/iommu.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) (limited to 'drivers/iommu/iommu.c') diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index 7078bf4a8ec8..10eb24d2e55a 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -176,16 +176,16 @@ static int __init iommu_subsys_init(void) if (!iommu_default_passthrough() && !iommu_dma_strict) iommu_def_domain_type = IOMMU_DOMAIN_DMA_FQ; - pr_info("Default domain type: %s %s\n", + pr_info("Default domain type: %s%s\n", iommu_domain_type_str(iommu_def_domain_type), (iommu_cmd_line & IOMMU_CMD_LINE_DMA_API) ? - "(set via kernel command line)" : ""); + " (set via kernel command line)" : ""); if (!iommu_default_passthrough()) - pr_info("DMA domain TLB invalidation policy: %s mode %s\n", + pr_info("DMA domain TLB invalidation policy: %s mode%s\n", iommu_dma_strict ? "strict" : "lazy", (iommu_cmd_line & IOMMU_CMD_LINE_STRICT) ? - "(set via kernel command line)" : ""); + " (set via kernel command line)" : ""); nb = kcalloc(ARRAY_SIZE(iommu_buses), sizeof(*nb), GFP_KERNEL); if (!nb) -- cgit v1.2.3 From 4db0e5f8875ef12be6e946770ed7ef0b9c2b80ff Mon Sep 17 00:00:00 2001 From: Jason Gunthorpe Date: Thu, 11 May 2023 01:41:59 -0300 Subject: iommu: Replace iommu_group_device_count() with list_count_nodes() No reason to wrapper a standard function, just call the library directly. Reviewed-by: Lu Baolu Reviewed-by: Kevin Tian Tested-by: Heiko Stuebner Tested-by: Niklas Schnelle Signed-off-by: Jason Gunthorpe Link: https://lore.kernel.org/r/1-v5-1b99ae392328+44574-iommu_err_unwind_jgg@nvidia.com Signed-off-by: Joerg Roedel --- drivers/iommu/iommu.c | 15 ++------------- 1 file changed, 2 insertions(+), 13 deletions(-) (limited to 'drivers/iommu/iommu.c') diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index 10eb24d2e55a..aab956f1c3ab 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -1125,17 +1125,6 @@ void iommu_group_remove_device(struct device *dev) } EXPORT_SYMBOL_GPL(iommu_group_remove_device); -static int iommu_group_device_count(struct iommu_group *group) -{ - struct group_device *entry; - int ret = 0; - - list_for_each_entry(entry, &group->devices, list) - ret++; - - return ret; -} - static int __iommu_group_for_each_dev(struct iommu_group *group, void *data, int (*fn)(struct device *, void *)) { @@ -2083,7 +2072,7 @@ int iommu_attach_device(struct iommu_domain *domain, struct device *dev) */ mutex_lock(&group->mutex); ret = -EINVAL; - if (iommu_group_device_count(group) != 1) + if (list_count_nodes(&group->devices) != 1) goto out_unlock; ret = __iommu_attach_group(domain, group); @@ -2114,7 +2103,7 @@ void iommu_detach_device(struct iommu_domain *domain, struct device *dev) mutex_lock(&group->mutex); if (WARN_ON(domain != group->domain) || - WARN_ON(iommu_group_device_count(group) != 1)) + WARN_ON(list_count_nodes(&group->devices) != 1)) goto out_unlock; __iommu_group_set_core_domain(group); -- cgit v1.2.3 From 3006b15b364a34a2a19b45bb2948dd6a83c5e1fe Mon Sep 17 00:00:00 2001 From: Jason Gunthorpe Date: Thu, 11 May 2023 01:42:00 -0300 Subject: iommu: Add for_each_group_device() Convenience macro to iterate over every struct group_device in the group. Replace all open coded list_for_each_entry's with this macro. Reviewed-by: Lu Baolu Reviewed-by: Kevin Tian Tested-by: Heiko Stuebner Tested-by: Niklas Schnelle Signed-off-by: Jason Gunthorpe Link: https://lore.kernel.org/r/2-v5-1b99ae392328+44574-iommu_err_unwind_jgg@nvidia.com Signed-off-by: Joerg Roedel --- .clang-format | 1 + drivers/iommu/iommu.c | 16 ++++++++++------ 2 files changed, 11 insertions(+), 6 deletions(-) (limited to 'drivers/iommu/iommu.c') diff --git a/.clang-format b/.clang-format index 0d1ed8776733..0bbb1991defe 100644 --- a/.clang-format +++ b/.clang-format @@ -254,6 +254,7 @@ ForEachMacros: - 'for_each_free_mem_range' - 'for_each_free_mem_range_reverse' - 'for_each_func_rsrc' + - 'for_each_group_device' - 'for_each_group_evsel' - 'for_each_group_member' - 'for_each_hstate' diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index aab956f1c3ab..e806f4c781df 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -68,6 +68,10 @@ struct group_device { char *name; }; +/* Iterate over each struct group_device in a struct iommu_group */ +#define for_each_group_device(group, pos) \ + list_for_each_entry(pos, &(group)->devices, list) + struct iommu_group_attribute { struct attribute attr; ssize_t (*show)(struct iommu_group *group, char *buf); @@ -468,7 +472,7 @@ __iommu_group_remove_device(struct iommu_group *group, struct device *dev) struct group_device *device; lockdep_assert_held(&group->mutex); - list_for_each_entry(device, &group->devices, list) { + for_each_group_device(group, device) { if (device->dev == dev) { list_del(&device->list); return device; @@ -707,7 +711,7 @@ int iommu_get_group_resv_regions(struct iommu_group *group, int ret = 0; mutex_lock(&group->mutex); - list_for_each_entry(device, &group->devices, list) { + for_each_group_device(group, device) { struct list_head dev_resv_regions; /* @@ -1131,7 +1135,7 @@ static int __iommu_group_for_each_dev(struct iommu_group *group, void *data, struct group_device *device; int ret = 0; - list_for_each_entry(device, &group->devices, list) { + for_each_group_device(group, device) { ret = fn(device->dev, data); if (ret) break; @@ -1935,7 +1939,7 @@ bool iommu_group_has_isolated_msi(struct iommu_group *group) bool ret = true; mutex_lock(&group->mutex); - list_for_each_entry(group_dev, &group->devices, list) + for_each_group_device(group, group_dev) ret &= msi_device_has_isolated_msi(group_dev->dev); mutex_unlock(&group->mutex); return ret; @@ -3243,7 +3247,7 @@ static int __iommu_set_group_pasid(struct iommu_domain *domain, struct group_device *device; int ret = 0; - list_for_each_entry(device, &group->devices, list) { + for_each_group_device(group, device) { ret = domain->ops->set_dev_pasid(domain, device->dev, pasid); if (ret) break; @@ -3258,7 +3262,7 @@ static void __iommu_remove_group_pasid(struct iommu_group *group, struct group_device *device; const struct iommu_ops *ops; - list_for_each_entry(device, &group->devices, list) { + for_each_group_device(group, device) { ops = dev_iommu_ops(device->dev); ops->remove_dev_pasid(device->dev, pasid); } -- cgit v1.2.3 From dcf40ed3a20d727be054c4a20db47b32cb5036d4 Mon Sep 17 00:00:00 2001 From: Jason Gunthorpe Date: Thu, 11 May 2023 01:42:01 -0300 Subject: iommu: Make __iommu_group_set_domain() handle error unwind Let's try to have a consistent and clear strategy for error handling during domain attach failures. There are two broad categories, the first is callers doing destruction and trying to set the domain back to a previously good domain. These cases cannot handle failure during destruction flows and must succeed, or at least avoid a UAF on the current group->domain which is likely about to be freed. Many of the drivers are well behaved here and will not hit the WARN_ON's or a UAF, but some are doing hypercalls/etc that can fail unpredictably and don't meet the expectations. The second case is attaching a domain for the first time in a failable context, failure should restore the attachment back to group->domain using the above unfailable operation. Have __iommu_group_set_domain_internal() execute a common algorithm that tries to achieve this, and in the worst case, would leave a device "detached" or assigned to a global blocking domain. This relies on some existing common driver behaviors where attach failure will also do detatch and true IOMMU_DOMAIN_BLOCK implementations that are not allowed to ever fail. Name the first case with __iommu_group_set_domain_nofail() to make it clear. Pull all the error handling and WARN_ON generation into __iommu_group_set_domain_internal(). Avoid the obfuscating use of __iommu_group_for_each_dev() and be more careful about what should happen during failures by only touching devices we've already touched. Reviewed-by: Lu Baolu Reviewed-by: Kevin Tian Tested-by: Heiko Stuebner Tested-by: Niklas Schnelle Signed-off-by: Jason Gunthorpe Link: https://lore.kernel.org/r/3-v5-1b99ae392328+44574-iommu_err_unwind_jgg@nvidia.com Signed-off-by: Joerg Roedel --- drivers/iommu/iommu.c | 137 +++++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 112 insertions(+), 25 deletions(-) (limited to 'drivers/iommu/iommu.c') diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index e806f4c781df..74cb162daac3 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -101,8 +101,26 @@ static int __iommu_attach_device(struct iommu_domain *domain, struct device *dev); static int __iommu_attach_group(struct iommu_domain *domain, struct iommu_group *group); + +enum { + IOMMU_SET_DOMAIN_MUST_SUCCEED = 1 << 0, +}; + +static int __iommu_group_set_domain_internal(struct iommu_group *group, + struct iommu_domain *new_domain, + unsigned int flags); static int __iommu_group_set_domain(struct iommu_group *group, - struct iommu_domain *new_domain); + struct iommu_domain *new_domain) +{ + return __iommu_group_set_domain_internal(group, new_domain, 0); +} +static void __iommu_group_set_domain_nofail(struct iommu_group *group, + struct iommu_domain *new_domain) +{ + WARN_ON(__iommu_group_set_domain_internal( + group, new_domain, IOMMU_SET_DOMAIN_MUST_SUCCEED)); +} + static int iommu_create_device_direct_mappings(struct iommu_group *group, struct device *dev); static struct iommu_group *iommu_group_get_for_dev(struct device *dev); @@ -2022,15 +2040,13 @@ EXPORT_SYMBOL_GPL(iommu_domain_free); static void __iommu_group_set_core_domain(struct iommu_group *group) { struct iommu_domain *new_domain; - int ret; if (group->owner) new_domain = group->blocking_domain; else new_domain = group->default_domain; - ret = __iommu_group_set_domain(group, new_domain); - WARN(ret, "iommu driver failed to attach the default/blocking domain"); + __iommu_group_set_domain_nofail(group, new_domain); } static int __iommu_attach_device(struct iommu_domain *domain, @@ -2215,21 +2231,55 @@ int iommu_attach_group(struct iommu_domain *domain, struct iommu_group *group) } EXPORT_SYMBOL_GPL(iommu_attach_group); -static int iommu_group_do_set_platform_dma(struct device *dev, void *data) +static int __iommu_device_set_domain(struct iommu_group *group, + struct device *dev, + struct iommu_domain *new_domain, + unsigned int flags) { - const struct iommu_ops *ops = dev_iommu_ops(dev); - - if (!WARN_ON(!ops->set_platform_dma_ops)) - ops->set_platform_dma_ops(dev); + int ret; + ret = __iommu_attach_device(new_domain, dev); + if (ret) { + /* + * If we have a blocking domain then try to attach that in hopes + * of avoiding a UAF. Modern drivers should implement blocking + * domains as global statics that cannot fail. + */ + if ((flags & IOMMU_SET_DOMAIN_MUST_SUCCEED) && + group->blocking_domain && + group->blocking_domain != new_domain) + __iommu_attach_device(group->blocking_domain, dev); + return ret; + } return 0; } -static int __iommu_group_set_domain(struct iommu_group *group, - struct iommu_domain *new_domain) +/* + * If 0 is returned the group's domain is new_domain. If an error is returned + * then the group's domain will be set back to the existing domain unless + * IOMMU_SET_DOMAIN_MUST_SUCCEED, otherwise an error is returned and the group's + * domains is left inconsistent. This is a driver bug to fail attach with a + * previously good domain. We try to avoid a kernel UAF because of this. + * + * IOMMU groups are really the natural working unit of the IOMMU, but the IOMMU + * API works on domains and devices. Bridge that gap by iterating over the + * devices in a group. Ideally we'd have a single device which represents the + * requestor ID of the group, but we also allow IOMMU drivers to create policy + * defined minimum sets, where the physical hardware may be able to distiguish + * members, but we wish to group them at a higher level (ex. untrusted + * multi-function PCI devices). Thus we attach each device. + */ +static int __iommu_group_set_domain_internal(struct iommu_group *group, + struct iommu_domain *new_domain, + unsigned int flags) { + struct group_device *last_gdev; + struct group_device *gdev; + int result; int ret; + lockdep_assert_held(&group->mutex); + if (group->domain == new_domain) return 0; @@ -2239,8 +2289,12 @@ static int __iommu_group_set_domain(struct iommu_group *group, * platform specific behavior. */ if (!new_domain) { - __iommu_group_for_each_dev(group, NULL, - iommu_group_do_set_platform_dma); + for_each_group_device(group, gdev) { + const struct iommu_ops *ops = dev_iommu_ops(gdev->dev); + + if (!WARN_ON(!ops->set_platform_dma_ops)) + ops->set_platform_dma_ops(gdev->dev); + } group->domain = NULL; return 0; } @@ -2250,16 +2304,52 @@ static int __iommu_group_set_domain(struct iommu_group *group, * domain. This switch does not have to be atomic and DMA can be * discarded during the transition. DMA must only be able to access * either new_domain or group->domain, never something else. - * - * Note that this is called in error unwind paths, attaching to a - * domain that has already been attached cannot fail. */ - ret = __iommu_group_for_each_dev(group, new_domain, - iommu_group_do_attach_device); - if (ret) - return ret; + result = 0; + for_each_group_device(group, gdev) { + ret = __iommu_device_set_domain(group, gdev->dev, new_domain, + flags); + if (ret) { + result = ret; + /* + * Keep trying the other devices in the group. If a + * driver fails attach to an otherwise good domain, and + * does not support blocking domains, it should at least + * drop its reference on the current domain so we don't + * UAF. + */ + if (flags & IOMMU_SET_DOMAIN_MUST_SUCCEED) + continue; + goto err_revert; + } + } group->domain = new_domain; - return 0; + return result; + +err_revert: + /* + * This is called in error unwind paths. A well behaved driver should + * always allow us to attach to a domain that was already attached. + */ + last_gdev = gdev; + for_each_group_device(group, gdev) { + const struct iommu_ops *ops = dev_iommu_ops(gdev->dev); + + /* + * If set_platform_dma_ops is not present a NULL domain can + * happen only for first probe, in which case we leave + * group->domain as NULL and let release clean everything up. + */ + if (group->domain) + WARN_ON(__iommu_device_set_domain( + group, gdev->dev, group->domain, + IOMMU_SET_DOMAIN_MUST_SUCCEED)); + else if (ops->set_platform_dma_ops) + ops->set_platform_dma_ops(gdev->dev); + if (gdev == last_gdev) + break; + } + return ret; } void iommu_detach_group(struct iommu_domain *domain, struct iommu_group *group) @@ -3176,16 +3266,13 @@ EXPORT_SYMBOL_GPL(iommu_device_claim_dma_owner); static void __iommu_release_dma_ownership(struct iommu_group *group) { - int ret; - if (WARN_ON(!group->owner_cnt || !group->owner || !xa_empty(&group->pasid_array))) return; group->owner_cnt = 0; group->owner = NULL; - ret = __iommu_group_set_domain(group, group->default_domain); - WARN(ret, "iommu driver failed to attach the default domain"); + __iommu_group_set_domain_nofail(group, group->default_domain); } /** -- cgit v1.2.3 From ecd60dc5d22b2ac2a68d9bf84bed0cf96b654cde Mon Sep 17 00:00:00 2001 From: Jason Gunthorpe Date: Thu, 11 May 2023 01:42:02 -0300 Subject: iommu: Use __iommu_group_set_domain() for __iommu_attach_group() The error recovery here matches the recovery inside __iommu_group_set_domain(), so just use it directly. Reviewed-by: Kevin Tian Reviewed-by: Lu Baolu Tested-by: Heiko Stuebner Tested-by: Niklas Schnelle Signed-off-by: Jason Gunthorpe Link: https://lore.kernel.org/r/4-v5-1b99ae392328+44574-iommu_err_unwind_jgg@nvidia.com Signed-off-by: Joerg Roedel --- drivers/iommu/iommu.c | 40 +--------------------------------------- 1 file changed, 1 insertion(+), 39 deletions(-) (limited to 'drivers/iommu/iommu.c') diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index 74cb162daac3..f31ba66ccb2f 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -2159,52 +2159,14 @@ struct iommu_domain *iommu_get_dma_domain(struct device *dev) return dev->iommu_group->default_domain; } -/* - * IOMMU groups are really the natural working unit of the IOMMU, but - * the IOMMU API works on domains and devices. Bridge that gap by - * iterating over the devices in a group. Ideally we'd have a single - * device which represents the requestor ID of the group, but we also - * allow IOMMU drivers to create policy defined minimum sets, where - * the physical hardware may be able to distiguish members, but we - * wish to group them at a higher level (ex. untrusted multi-function - * PCI devices). Thus we attach each device. - */ -static int iommu_group_do_attach_device(struct device *dev, void *data) -{ - struct iommu_domain *domain = data; - - return __iommu_attach_device(domain, dev); -} - static int __iommu_attach_group(struct iommu_domain *domain, struct iommu_group *group) { - int ret; - if (group->domain && group->domain != group->default_domain && group->domain != group->blocking_domain) return -EBUSY; - ret = __iommu_group_for_each_dev(group, domain, - iommu_group_do_attach_device); - if (ret == 0) { - group->domain = domain; - } else { - /* - * To recover from the case when certain device within the - * group fails to attach to the new domain, we need force - * attaching all devices back to the old domain. The old - * domain is compatible for all devices in the group, - * hence the iommu driver should always return success. - */ - struct iommu_domain *old_domain = group->domain; - - group->domain = NULL; - WARN(__iommu_group_set_domain(group, old_domain), - "iommu driver failed to attach a compatible domain"); - } - - return ret; + return __iommu_group_set_domain(group, domain); } /** -- cgit v1.2.3 From 4c8ad9da05662141928fe4ed001d3775fd95221c Mon Sep 17 00:00:00 2001 From: Jason Gunthorpe Date: Thu, 11 May 2023 01:42:03 -0300 Subject: iommu: Use __iommu_group_set_domain() in iommu_change_dev_def_domain() This is missing re-attach error handling if the attach fails, use the common code. The ugly "group->domain = prev_domain" will be cleaned in a later patch. Reviewed-by: Lu Baolu Reviewed-by: Kevin Tian Tested-by: Heiko Stuebner Tested-by: Niklas Schnelle Signed-off-by: Jason Gunthorpe Link: https://lore.kernel.org/r/5-v5-1b99ae392328+44574-iommu_err_unwind_jgg@nvidia.com Signed-off-by: Joerg Roedel --- drivers/iommu/iommu.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'drivers/iommu/iommu.c') diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index f31ba66ccb2f..e0bfb114d08d 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -2946,11 +2946,12 @@ static int iommu_change_dev_def_domain(struct iommu_group *group, if (ret) goto restore_old_domain; - ret = iommu_group_create_direct_mappings(group); + group->domain = prev_dom; + ret = iommu_create_device_direct_mappings(group, dev); if (ret) goto free_new_domain; - ret = __iommu_attach_group(group->default_domain, group); + ret = __iommu_group_set_domain(group, group->default_domain); if (ret) goto free_new_domain; @@ -2962,7 +2963,6 @@ free_new_domain: iommu_domain_free(group->default_domain); restore_old_domain: group->default_domain = prev_dom; - group->domain = prev_dom; return ret; } -- cgit v1.2.3 From d257344c661950986e6129407f7169f54e0bb4cf Mon Sep 17 00:00:00 2001 From: Jason Gunthorpe Date: Thu, 11 May 2023 01:42:04 -0300 Subject: iommu: Replace __iommu_group_dma_first_attach() with set_domain Reorganize the attach_deferred logic to set dev->iommu->attach_deferred immediately during probe and then have __iommu_device_set_domain() check it and not attach the default_domain. This is to prepare for removing the group->domain set from iommu_group_alloc_default_domain() by calling __iommu_group_set_domain() to set the group->domain. Reviewed-by: Lu Baolu Tested-by: Heiko Stuebner Tested-by: Niklas Schnelle Signed-off-by: Jason Gunthorpe Link: https://lore.kernel.org/r/6-v5-1b99ae392328+44574-iommu_err_unwind_jgg@nvidia.com Signed-off-by: Joerg Roedel --- drivers/iommu/iommu.c | 32 +++++++++++--------------------- 1 file changed, 11 insertions(+), 21 deletions(-) (limited to 'drivers/iommu/iommu.c') diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index e0bfb114d08d..eaa63fe887f9 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -365,6 +365,8 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_list dev->iommu->iommu_dev = iommu_dev; dev->iommu->max_pasids = dev_iommu_get_max_pasids(dev); + if (ops->is_attach_deferred) + dev->iommu->attach_deferred = ops->is_attach_deferred(dev); group = iommu_group_get_for_dev(dev); if (IS_ERR(group)) { @@ -399,27 +401,14 @@ err_unlock: return ret; } -static bool iommu_is_attach_deferred(struct device *dev) -{ - const struct iommu_ops *ops = dev_iommu_ops(dev); - - if (ops->is_attach_deferred) - return ops->is_attach_deferred(dev); - - return false; -} - static int iommu_group_do_dma_first_attach(struct device *dev, void *data) { struct iommu_domain *domain = data; lockdep_assert_held(&dev->iommu_group->mutex); - if (iommu_is_attach_deferred(dev)) { - dev->iommu->attach_deferred = 1; + if (dev->iommu->attach_deferred) return 0; - } - return __iommu_attach_device(domain, dev); } @@ -1831,12 +1820,6 @@ static void probe_alloc_default_domain(const struct bus_type *bus, } -static int __iommu_group_dma_first_attach(struct iommu_group *group) -{ - return __iommu_group_for_each_dev(group, group->default_domain, - iommu_group_do_dma_first_attach); -} - static int iommu_group_do_probe_finalize(struct device *dev, void *data) { const struct iommu_ops *ops = dev_iommu_ops(dev); @@ -1899,7 +1882,8 @@ int bus_iommu_probe(const struct bus_type *bus) iommu_group_create_direct_mappings(group); - ret = __iommu_group_dma_first_attach(group); + group->domain = NULL; + ret = __iommu_group_set_domain(group, group->default_domain); mutex_unlock(&group->mutex); @@ -2200,6 +2184,12 @@ static int __iommu_device_set_domain(struct iommu_group *group, { int ret; + if (dev->iommu->attach_deferred) { + if (new_domain == group->default_domain) + return 0; + dev->iommu->attach_deferred = 0; + } + ret = __iommu_attach_device(new_domain, dev); if (ret) { /* -- cgit v1.2.3 From 0046a4337eae148510173680f82b483f7c3b7ded Mon Sep 17 00:00:00 2001 From: Jason Gunthorpe Date: Thu, 11 May 2023 01:42:05 -0300 Subject: iommu: Remove iommu_group_do_dma_first_attach() from iommu_group_add_device() This function is only used to construct the groups, it should not be operating the iommu driver. External callers in VFIO and POWER do not have any iommu drivers on the devices so group->domain will be NULL. The only internal caller is from iommu_probe_device() which already calls iommu_group_do_dma_first_attach(), meaning we are calling it twice in the only case it matters. Since iommu_probe_device() is the logical place to sort out the group's domain, remove the call from iommu_group_add_device(). Reviewed-by: Kevin Tian Tested-by: Heiko Stuebner Tested-by: Niklas Schnelle Signed-off-by: Jason Gunthorpe Link: https://lore.kernel.org/r/7-v5-1b99ae392328+44574-iommu_err_unwind_jgg@nvidia.com Signed-off-by: Joerg Roedel --- drivers/iommu/iommu.c | 12 ------------ 1 file changed, 12 deletions(-) (limited to 'drivers/iommu/iommu.c') diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index eaa63fe887f9..fa2b669ecf4b 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -1080,25 +1080,13 @@ rename: mutex_lock(&group->mutex); list_add_tail(&device->list, &group->devices); - if (group->domain) - ret = iommu_group_do_dma_first_attach(dev, group->domain); mutex_unlock(&group->mutex); - if (ret) - goto err_put_group; - trace_add_device_to_group(group->id, dev); dev_info(dev, "Adding to iommu group %d\n", group->id); return 0; -err_put_group: - mutex_lock(&group->mutex); - list_del(&device->list); - mutex_unlock(&group->mutex); - dev->iommu_group = NULL; - kobject_put(group->devices_kobj); - sysfs_remove_link(group->devices_kobj, device->name); err_free_name: kfree(device->name); err_remove_link: -- cgit v1.2.3 From 2f74198ae006c50a4188ae02c10e2c7b0b8142da Mon Sep 17 00:00:00 2001 From: Jason Gunthorpe Date: Thu, 11 May 2023 01:42:06 -0300 Subject: iommu: Replace iommu_group_do_dma_first_attach with __iommu_device_set_domain Since __iommu_device_set_domain() now knows how to handle deferred attach we can just call it directly from the only call site. Reviewed-by: Kevin Tian Reviewed-by: Lu Baolu Tested-by: Heiko Stuebner Tested-by: Niklas Schnelle Signed-off-by: Jason Gunthorpe Link: https://lore.kernel.org/r/8-v5-1b99ae392328+44574-iommu_err_unwind_jgg@nvidia.com Signed-off-by: Joerg Roedel --- drivers/iommu/iommu.c | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-) (limited to 'drivers/iommu/iommu.c') diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index fa2b669ecf4b..ea61a81c0006 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -106,6 +106,10 @@ enum { IOMMU_SET_DOMAIN_MUST_SUCCEED = 1 << 0, }; +static int __iommu_device_set_domain(struct iommu_group *group, + struct device *dev, + struct iommu_domain *new_domain, + unsigned int flags); static int __iommu_group_set_domain_internal(struct iommu_group *group, struct iommu_domain *new_domain, unsigned int flags); @@ -401,17 +405,6 @@ err_unlock: return ret; } -static int iommu_group_do_dma_first_attach(struct device *dev, void *data) -{ - struct iommu_domain *domain = data; - - lockdep_assert_held(&dev->iommu_group->mutex); - - if (dev->iommu->attach_deferred) - return 0; - return __iommu_attach_device(domain, dev); -} - int iommu_probe_device(struct device *dev) { const struct iommu_ops *ops; @@ -442,7 +435,7 @@ int iommu_probe_device(struct device *dev) * attach the default domain. */ if (group->default_domain && !group->owner) { - ret = iommu_group_do_dma_first_attach(dev, group->default_domain); + ret = __iommu_device_set_domain(group, dev, group->domain, 0); if (ret) { mutex_unlock(&group->mutex); iommu_group_put(group); -- cgit v1.2.3 From e7f85dfbbc9cf8660174c45c213571aaa518df85 Mon Sep 17 00:00:00 2001 From: Jason Gunthorpe Date: Thu, 11 May 2023 01:42:07 -0300 Subject: iommu: Fix iommu_probe_device() to attach the right domain The general invariant is that all devices in an iommu_group are attached to group->domain. We missed some cases here where an owned group would not get the device attached. Rework this logic so it follows the default domain flow of the bus_iommu_probe() - call iommu_alloc_default_domain(), then use __iommu_group_set_domain_internal() to set up all the devices. Finally always attach the device to the current domain if it is already set. This is an unlikely functional issue as iommufd uses iommu_attach_group(). It is possible to hot plug in a new group member, add a vfio driver to it and then hot add it to an existing iommufd. In this case it is required that the core code set the iommu_domain properly since iommufd won't call iommu_attach_group() again. Reviewed-by: Lu Baolu Reviewed-by: Kevin Tian Tested-by: Heiko Stuebner Tested-by: Niklas Schnelle Signed-off-by: Jason Gunthorpe Link: https://lore.kernel.org/r/9-v5-1b99ae392328+44574-iommu_err_unwind_jgg@nvidia.com Signed-off-by: Joerg Roedel --- drivers/iommu/iommu.c | 44 ++++++++++++++++++++++++-------------------- 1 file changed, 24 insertions(+), 20 deletions(-) (limited to 'drivers/iommu/iommu.c') diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index ea61a81c0006..29ab5d990ef6 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -421,27 +421,31 @@ int iommu_probe_device(struct device *dev) goto err_release; } - /* - * Try to allocate a default domain - needs support from the - * IOMMU driver. There are still some drivers which don't - * support default domains, so the return value is not yet - * checked. - */ mutex_lock(&group->mutex); - iommu_alloc_default_domain(group, dev); - /* - * If device joined an existing group which has been claimed, don't - * attach the default domain. - */ - if (group->default_domain && !group->owner) { + if (group->domain) { ret = __iommu_device_set_domain(group, dev, group->domain, 0); - if (ret) { - mutex_unlock(&group->mutex); - iommu_group_put(group); - goto err_release; - } + } else if (!group->default_domain) { + /* + * Try to allocate a default domain - needs support from the + * IOMMU driver. There are still some drivers which don't + * support default domains, so the return value is not yet + * checked. + */ + iommu_alloc_default_domain(group, dev); + group->domain = NULL; + if (group->default_domain) + ret = __iommu_group_set_domain(group, + group->default_domain); + + /* + * We assume that the iommu driver starts up the device in + * 'set_platform_dma_ops' mode if it does not support default + * domains. + */ } + if (ret) + goto err_unlock; iommu_create_device_direct_mappings(group, dev); @@ -454,6 +458,9 @@ int iommu_probe_device(struct device *dev) return 0; +err_unlock: + mutex_unlock(&group->mutex); + iommu_group_put(group); err_release: iommu_release_device(dev); @@ -1665,9 +1672,6 @@ static int iommu_alloc_default_domain(struct iommu_group *group, { unsigned int type; - if (group->default_domain) - return 0; - type = iommu_get_def_domain_type(dev) ? : iommu_def_domain_type; return iommu_group_alloc_default_domain(dev->bus, group, type); -- cgit v1.2.3 From 152431e4fe7f1aac8aa6cc57bfe58d2d2596be4d Mon Sep 17 00:00:00 2001 From: Jason Gunthorpe Date: Thu, 11 May 2023 01:42:08 -0300 Subject: iommu: Do iommu_group_create_direct_mappings() before attach The iommu_probe_device() path calls iommu_create_device_direct_mappings() after attaching the device. IOMMU_RESV_DIRECT maps need to be continually in place, so if a hotplugged device has new ranges the should have been mapped into the default domain before it is attached. Move the iommu_create_device_direct_mappings() call up. Reviewed-by: Lu Baolu Reviewed-by: Kevin Tian Tested-by: Heiko Stuebner Tested-by: Niklas Schnelle Signed-off-by: Jason Gunthorpe Link: https://lore.kernel.org/r/10-v5-1b99ae392328+44574-iommu_err_unwind_jgg@nvidia.com Signed-off-by: Joerg Roedel --- drivers/iommu/iommu.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) (limited to 'drivers/iommu/iommu.c') diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index 29ab5d990ef6..6b39f756c020 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -423,6 +423,8 @@ int iommu_probe_device(struct device *dev) mutex_lock(&group->mutex); + iommu_create_device_direct_mappings(group, dev); + if (group->domain) { ret = __iommu_device_set_domain(group, dev, group->domain, 0); } else if (!group->default_domain) { @@ -434,9 +436,11 @@ int iommu_probe_device(struct device *dev) */ iommu_alloc_default_domain(group, dev); group->domain = NULL; - if (group->default_domain) + if (group->default_domain) { + iommu_create_device_direct_mappings(group, dev); ret = __iommu_group_set_domain(group, group->default_domain); + } /* * We assume that the iommu driver starts up the device in @@ -447,8 +451,6 @@ int iommu_probe_device(struct device *dev) if (ret) goto err_unlock; - iommu_create_device_direct_mappings(group, dev); - mutex_unlock(&group->mutex); iommu_group_put(group); -- cgit v1.2.3 From dfddd54dc77c4519ee3c94e7462b1c035c69a031 Mon Sep 17 00:00:00 2001 From: Jason Gunthorpe Date: Thu, 11 May 2023 01:42:09 -0300 Subject: iommu: Remove the assignment of group->domain during default domain alloc group->domain should only be set once all the device's drivers have had their ops->attach_dev() called. iommu_group_alloc_default_domain() doesn't do this, so it shouldn't set the value. The previous patches organized things so that each caller of iommu_group_alloc_default_domain() follows up with calling __iommu_group_set_domain_internal() that does set the group->domain. Reviewed-by: Kevin Tian Reviewed-by: Lu Baolu Tested-by: Heiko Stuebner Tested-by: Niklas Schnelle Signed-off-by: Jason Gunthorpe Link: https://lore.kernel.org/r/11-v5-1b99ae392328+44574-iommu_err_unwind_jgg@nvidia.com Signed-off-by: Joerg Roedel --- drivers/iommu/iommu.c | 4 ---- 1 file changed, 4 deletions(-) (limited to 'drivers/iommu/iommu.c') diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index 6b39f756c020..2041e3e028de 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -435,7 +435,6 @@ int iommu_probe_device(struct device *dev) * checked. */ iommu_alloc_default_domain(group, dev); - group->domain = NULL; if (group->default_domain) { iommu_create_device_direct_mappings(group, dev); ret = __iommu_group_set_domain(group, @@ -1664,8 +1663,6 @@ static int iommu_group_alloc_default_domain(const struct bus_type *bus, return -ENOMEM; group->default_domain = dom; - if (!group->domain) - group->domain = dom; return 0; } @@ -1869,7 +1866,6 @@ int bus_iommu_probe(const struct bus_type *bus) iommu_group_create_direct_mappings(group); - group->domain = NULL; ret = __iommu_group_set_domain(group, group->default_domain); mutex_unlock(&group->mutex); -- cgit v1.2.3 From 8b4eb75ee50e6f4606f88debf44aeb47937057d4 Mon Sep 17 00:00:00 2001 From: Jason Gunthorpe Date: Thu, 11 May 2023 01:42:10 -0300 Subject: iommu: Consolidate the code to calculate the target default domain type Put all the code to calculate the default domain type into one function. Make the function able to handle the iommu_change_dev_def_domain() by taking in the target domain type and erroring out if the target type isn't reachable. This makes it really clear that specifying a 0 type during iommu_change_dev_def_domain() will have the same outcome as the normal probe path. Remove the obfuscating use of __iommu_group_for_each_dev() and related struct __group_domain_type. Reviewed-by: Lu Baolu Reviewed-by: Kevin Tian Tested-by: Heiko Stuebner Tested-by: Niklas Schnelle Signed-off-by: Jason Gunthorpe Link: https://lore.kernel.org/r/12-v5-1b99ae392328+44574-iommu_err_unwind_jgg@nvidia.com Signed-off-by: Joerg Roedel --- drivers/iommu/iommu.c | 88 ++++++++++++++++++++------------------------------- 1 file changed, 35 insertions(+), 53 deletions(-) (limited to 'drivers/iommu/iommu.c') diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index 2041e3e028de..9e661cbd3d42 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -1758,50 +1758,43 @@ static int iommu_bus_notifier(struct notifier_block *nb, return 0; } -struct __group_domain_type { - struct device *dev; - unsigned int type; -}; - -static int probe_get_default_domain_type(struct device *dev, void *data) +/* A target_type of 0 will select the best domain type and cannot fail */ +static int iommu_get_default_domain_type(struct iommu_group *group, + int target_type) { - struct __group_domain_type *gtype = data; - unsigned int type = iommu_get_def_domain_type(dev); + int best_type = target_type; + struct group_device *gdev; + struct device *last_dev; - if (type) { - if (gtype->type && gtype->type != type) { - dev_warn(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(gtype->dev), - iommu_domain_type_str(gtype->type)); - gtype->type = 0; - } + lockdep_assert_held(&group->mutex); - if (!gtype->dev) { - gtype->dev = dev; - gtype->type = type; + 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 iommu_def_domain_type; } + if (!best_type) + best_type = type; + last_dev = gdev->dev; } - return 0; -} - -static void probe_alloc_default_domain(const struct bus_type *bus, - struct iommu_group *group) -{ - struct __group_domain_type gtype; - - memset(>ype, 0, sizeof(gtype)); - - /* Ask for default domain requirements of all devices in the group */ - __iommu_group_for_each_dev(group, >ype, - probe_get_default_domain_type); - - if (!gtype.type) - gtype.type = iommu_def_domain_type; - - iommu_group_alloc_default_domain(bus, group, gtype.type); - + if (!best_type) + return iommu_def_domain_type; + return best_type; } static int iommu_group_do_probe_finalize(struct device *dev, void *data) @@ -1857,7 +1850,8 @@ int bus_iommu_probe(const struct bus_type *bus) list_del_init(&group->entry); /* Try to allocate default domain */ - probe_alloc_default_domain(bus, group); + iommu_group_alloc_default_domain( + bus, group, iommu_get_default_domain_type(group, 0)); if (!group->default_domain) { mutex_unlock(&group->mutex); @@ -2882,27 +2876,15 @@ EXPORT_SYMBOL_GPL(iommu_dev_disable_feature); static int iommu_change_dev_def_domain(struct iommu_group *group, struct device *dev, int type) { - struct __group_domain_type gtype = {NULL, 0}; struct iommu_domain *prev_dom; int ret; lockdep_assert_held(&group->mutex); prev_dom = group->default_domain; - __iommu_group_for_each_dev(group, >ype, - probe_get_default_domain_type); - if (!type) { - /* - * If the user hasn't requested any specific type of domain and - * if the device supports both the domains, then default to the - * domain the device was booted with - */ - type = gtype.type ? : iommu_def_domain_type; - } else if (gtype.type && type != gtype.type) { - dev_err_ratelimited(dev, "Device cannot be in %s domain\n", - iommu_domain_type_str(type)); + type = iommu_get_default_domain_type(group, type); + if (type < 0) return -EINVAL; - } /* * Switch to a new domain only if the requested domain type is different -- cgit v1.2.3 From fcbb0a4d738ce3ccc06d2c73ba227cce5094d885 Mon Sep 17 00:00:00 2001 From: Jason Gunthorpe Date: Thu, 11 May 2023 01:42:11 -0300 Subject: iommu: Revise iommu_group_alloc_default_domain() Robin points out that the fallback to guessing what domains the driver supports should only happen if the driver doesn't return a preference from its ops->def_domain_type(). Re-organize iommu_group_alloc_default_domain() so it internally uses iommu_def_domain_type only during the fallback and makes it clearer how the fallback sequence works. Make iommu_group_alloc_default_domain() return the domain so the return based logic is cleaner and to prepare for the next patch. Remove the iommu_alloc_default_domain() function as it is now trivially just calling iommu_group_alloc_default_domain(). Reviewed-by: Lu Baolu Reviewed-by: Kevin Tian Tested-by: Heiko Stuebner Tested-by: Niklas Schnelle Signed-off-by: Jason Gunthorpe Link: https://lore.kernel.org/r/13-v5-1b99ae392328+44574-iommu_err_unwind_jgg@nvidia.com Signed-off-by: Joerg Roedel --- drivers/iommu/iommu.c | 71 +++++++++++++++++++++++++++------------------------ 1 file changed, 37 insertions(+), 34 deletions(-) (limited to 'drivers/iommu/iommu.c') diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index 9e661cbd3d42..7120f57c8028 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -93,8 +93,9 @@ static const char * const iommu_group_resv_type_string[] = { static int iommu_bus_notifier(struct notifier_block *nb, unsigned long action, void *data); static void iommu_release_device(struct device *dev); -static int iommu_alloc_default_domain(struct iommu_group *group, - struct device *dev); +static struct iommu_domain * +iommu_group_alloc_default_domain(struct iommu_group *group, int req_type); +static int iommu_get_def_domain_type(struct device *dev); static struct iommu_domain *__iommu_domain_alloc(const struct bus_type *bus, unsigned type); static int __iommu_attach_device(struct iommu_domain *domain, @@ -434,7 +435,8 @@ int iommu_probe_device(struct device *dev) * support default domains, so the return value is not yet * checked. */ - iommu_alloc_default_domain(group, dev); + group->default_domain = iommu_group_alloc_default_domain( + group, iommu_get_def_domain_type(dev)); if (group->default_domain) { iommu_create_device_direct_mappings(group, dev); ret = __iommu_group_set_domain(group, @@ -1645,35 +1647,38 @@ static int iommu_get_def_domain_type(struct device *dev) return 0; } -static int iommu_group_alloc_default_domain(const struct bus_type *bus, - struct iommu_group *group, - unsigned int type) +/* + * req_type of 0 means "auto" which means to select a domain based on + * iommu_def_domain_type or what the driver actually supports. + */ +static struct iommu_domain * +iommu_group_alloc_default_domain(struct iommu_group *group, int req_type) { + const struct bus_type *bus = + list_first_entry(&group->devices, struct group_device, list) + ->dev->bus; struct iommu_domain *dom; - dom = __iommu_domain_alloc(bus, type); - if (!dom && type != IOMMU_DOMAIN_DMA) { - dom = __iommu_domain_alloc(bus, IOMMU_DOMAIN_DMA); - if (dom) - pr_warn("Failed to allocate default IOMMU domain of type %u for group %s - Falling back to IOMMU_DOMAIN_DMA", - type, group->name); - } - - if (!dom) - return -ENOMEM; + lockdep_assert_held(&group->mutex); - group->default_domain = dom; - return 0; -} + if (req_type) + return __iommu_domain_alloc(bus, req_type); -static int iommu_alloc_default_domain(struct iommu_group *group, - struct device *dev) -{ - unsigned int type; + /* The driver gave no guidance on what type to use, try the default */ + dom = __iommu_domain_alloc(bus, iommu_def_domain_type); + if (dom) + return dom; - type = iommu_get_def_domain_type(dev) ? : iommu_def_domain_type; + /* Otherwise IDENTITY and DMA_FQ defaults will try DMA */ + if (iommu_def_domain_type == IOMMU_DOMAIN_DMA) + return NULL; + dom = __iommu_domain_alloc(bus, IOMMU_DOMAIN_DMA); + if (!dom) + return NULL; - return iommu_group_alloc_default_domain(dev->bus, group, type); + pr_warn("Failed to allocate default IOMMU domain of type %u for group %s - Falling back to IOMMU_DOMAIN_DMA", + iommu_def_domain_type, group->name); + return dom; } /** @@ -1785,15 +1790,12 @@ static int iommu_get_default_domain_type(struct iommu_group *group, "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 iommu_def_domain_type; + return 0; } if (!best_type) best_type = type; last_dev = gdev->dev; } - - if (!best_type) - return iommu_def_domain_type; return best_type; } @@ -1850,9 +1852,8 @@ int bus_iommu_probe(const struct bus_type *bus) list_del_init(&group->entry); /* Try to allocate default domain */ - iommu_group_alloc_default_domain( - bus, group, iommu_get_default_domain_type(group, 0)); - + group->default_domain = iommu_group_alloc_default_domain( + group, iommu_get_default_domain_type(group, 0)); if (!group->default_domain) { mutex_unlock(&group->mutex); continue; @@ -2897,9 +2898,11 @@ static int iommu_change_dev_def_domain(struct iommu_group *group, group->domain = NULL; /* Sets group->default_domain to the newly allocated domain */ - ret = iommu_group_alloc_default_domain(dev->bus, group, type); - if (ret) + group->default_domain = iommu_group_alloc_default_domain(group, type); + if (!group->default_domain) { + ret = -EINVAL; goto restore_old_domain; + } group->domain = prev_dom; ret = iommu_create_device_direct_mappings(group, dev); -- cgit v1.2.3 From d99be00f42eac9fc35a164f3f6c8c7a56b295aa9 Mon Sep 17 00:00:00 2001 From: Jason Gunthorpe Date: Thu, 11 May 2023 01:42:12 -0300 Subject: iommu: Consolidate the default_domain setup to one function Make iommu_change_dev_def_domain() general enough to setup the initial default_domain or replace it with a new default_domain. Call the new function iommu_setup_default_domain() and make it the only place in the code that stores to group->default_domain. Consolidate the three copies of the default_domain setup sequence. The flow flow requires: - Determining the domain type to use - Checking if the current default domain is the same type - Allocating a domain - Doing iommu_create_device_direct_mappings() - Attaching it to devices - Store group->default_domain This adjusts the domain allocation from the prior patch to be able to detect if each of the allocation steps is already the domain we already have, which is a more robust version of what change default domain was already doing. Reviewed-by: Lu Baolu Reviewed-by: Kevin Tian Tested-by: Heiko Stuebner Tested-by: Niklas Schnelle Signed-off-by: Jason Gunthorpe Link: https://lore.kernel.org/r/14-v5-1b99ae392328+44574-iommu_err_unwind_jgg@nvidia.com Signed-off-by: Joerg Roedel --- drivers/iommu/iommu.c | 202 ++++++++++++++++++++++---------------------------- 1 file changed, 89 insertions(+), 113 deletions(-) (limited to 'drivers/iommu/iommu.c') diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index 7120f57c8028..34f721434b28 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -93,9 +93,6 @@ static const char * const iommu_group_resv_type_string[] = { static int iommu_bus_notifier(struct notifier_block *nb, unsigned long action, void *data); static void iommu_release_device(struct device *dev); -static struct iommu_domain * -iommu_group_alloc_default_domain(struct iommu_group *group, int req_type); -static int iommu_get_def_domain_type(struct device *dev); static struct iommu_domain *__iommu_domain_alloc(const struct bus_type *bus, unsigned type); static int __iommu_attach_device(struct iommu_domain *domain, @@ -126,7 +123,9 @@ static void __iommu_group_set_domain_nofail(struct iommu_group *group, group, new_domain, IOMMU_SET_DOMAIN_MUST_SUCCEED)); } -static int iommu_create_device_direct_mappings(struct iommu_group *group, +static int iommu_setup_default_domain(struct iommu_group *group, + int target_type); +static int iommu_create_device_direct_mappings(struct iommu_domain *domain, struct device *dev); static struct iommu_group *iommu_group_get_for_dev(struct device *dev); static ssize_t iommu_group_store_type(struct iommu_group *group, @@ -424,33 +423,18 @@ int iommu_probe_device(struct device *dev) mutex_lock(&group->mutex); - iommu_create_device_direct_mappings(group, dev); + if (group->default_domain) + iommu_create_device_direct_mappings(group->default_domain, dev); if (group->domain) { ret = __iommu_device_set_domain(group, dev, group->domain, 0); + if (ret) + goto err_unlock; } else if (!group->default_domain) { - /* - * Try to allocate a default domain - needs support from the - * IOMMU driver. There are still some drivers which don't - * support default domains, so the return value is not yet - * checked. - */ - group->default_domain = iommu_group_alloc_default_domain( - group, iommu_get_def_domain_type(dev)); - if (group->default_domain) { - iommu_create_device_direct_mappings(group, dev); - ret = __iommu_group_set_domain(group, - group->default_domain); - } - - /* - * We assume that the iommu driver starts up the device in - * 'set_platform_dma_ops' mode if it does not support default - * domains. - */ + ret = iommu_setup_default_domain(group, 0); + if (ret) + goto err_unlock; } - if (ret) - goto err_unlock; mutex_unlock(&group->mutex); iommu_group_put(group); @@ -967,16 +951,15 @@ int iommu_group_set_name(struct iommu_group *group, const char *name) } EXPORT_SYMBOL_GPL(iommu_group_set_name); -static int iommu_create_device_direct_mappings(struct iommu_group *group, +static int iommu_create_device_direct_mappings(struct iommu_domain *domain, struct device *dev) { - struct iommu_domain *domain = group->default_domain; struct iommu_resv_region *entry; struct list_head mappings; unsigned long pg_size; int ret = 0; - if (!domain || !iommu_is_dma_domain(domain)) + if (!iommu_is_dma_domain(domain)) return 0; BUG_ON(!domain->pgsize_bitmap); @@ -1647,6 +1630,15 @@ static int iommu_get_def_domain_type(struct device *dev) return 0; } +static struct iommu_domain * +__iommu_group_alloc_default_domain(const struct bus_type *bus, + struct iommu_group *group, int req_type) +{ + if (group->default_domain && group->default_domain->type == req_type) + return group->default_domain; + return __iommu_domain_alloc(bus, req_type); +} + /* * req_type of 0 means "auto" which means to select a domain based on * iommu_def_domain_type or what the driver actually supports. @@ -1662,17 +1654,17 @@ iommu_group_alloc_default_domain(struct iommu_group *group, int req_type) lockdep_assert_held(&group->mutex); if (req_type) - return __iommu_domain_alloc(bus, req_type); + return __iommu_group_alloc_default_domain(bus, group, req_type); /* The driver gave no guidance on what type to use, try the default */ - dom = __iommu_domain_alloc(bus, iommu_def_domain_type); + dom = __iommu_group_alloc_default_domain(bus, group, iommu_def_domain_type); if (dom) return dom; /* Otherwise IDENTITY and DMA_FQ defaults will try DMA */ if (iommu_def_domain_type == IOMMU_DOMAIN_DMA) return NULL; - dom = __iommu_domain_alloc(bus, IOMMU_DOMAIN_DMA); + dom = __iommu_group_alloc_default_domain(bus, group, IOMMU_DOMAIN_DMA); if (!dom) return NULL; @@ -1815,21 +1807,6 @@ static void __iommu_group_dma_finalize(struct iommu_group *group) iommu_group_do_probe_finalize); } -static int iommu_do_create_direct_mappings(struct device *dev, void *data) -{ - struct iommu_group *group = data; - - iommu_create_device_direct_mappings(group, dev); - - return 0; -} - -static int iommu_group_create_direct_mappings(struct iommu_group *group) -{ - return __iommu_group_for_each_dev(group, group, - iommu_do_create_direct_mappings); -} - int bus_iommu_probe(const struct bus_type *bus) { struct iommu_group *group, *next; @@ -1851,27 +1828,16 @@ int bus_iommu_probe(const struct bus_type *bus) /* Remove item from the list */ list_del_init(&group->entry); - /* Try to allocate default domain */ - group->default_domain = iommu_group_alloc_default_domain( - group, iommu_get_default_domain_type(group, 0)); - if (!group->default_domain) { + ret = iommu_setup_default_domain(group, 0); + if (ret) { mutex_unlock(&group->mutex); - continue; + return ret; } - - iommu_group_create_direct_mappings(group); - - ret = __iommu_group_set_domain(group, group->default_domain); - mutex_unlock(&group->mutex); - - if (ret) - break; - __iommu_group_dma_finalize(group); } - return ret; + return 0; } bool iommu_present(const struct bus_type *bus) @@ -2860,68 +2826,83 @@ int iommu_dev_disable_feature(struct device *dev, enum iommu_dev_features feat) } EXPORT_SYMBOL_GPL(iommu_dev_disable_feature); -/* - * Changes the default domain of an iommu group - * - * @group: The group for which the default domain should be changed - * @dev: The first device in the group - * @type: The type of the new default domain that gets associated with the group - * - * Returns 0 on success and error code on failure +/** + * iommu_setup_default_domain - Set the default_domain for the group + * @group: Group to change + * @target_type: Domain type to set as the default_domain * - * Note: - * 1. Presently, this function is called only when user requests to change the - * group's default domain type through /sys/kernel/iommu_groups//type - * Please take a closer look if intended to use for other purposes. + * Allocate a default domain and set it as the current domain on the group. If + * the group already has a default domain it will be changed to the target_type. + * When target_type is 0 the default domain is selected based on driver and + * system preferences. */ -static int iommu_change_dev_def_domain(struct iommu_group *group, - struct device *dev, int type) +static int iommu_setup_default_domain(struct iommu_group *group, + int target_type) { - struct iommu_domain *prev_dom; + struct iommu_domain *old_dom = group->default_domain; + struct group_device *gdev; + struct iommu_domain *dom; + int req_type; int ret; lockdep_assert_held(&group->mutex); - prev_dom = group->default_domain; - type = iommu_get_default_domain_type(group, type); - if (type < 0) + req_type = iommu_get_default_domain_type(group, target_type); + if (req_type < 0) return -EINVAL; /* - * Switch to a new domain only if the requested domain type is different - * from the existing default domain type + * There are still some drivers which don't support default domains, so + * we ignore the failure and leave group->default_domain NULL. + * + * We assume that the iommu driver starts up the device in + * 'set_platform_dma_ops' mode if it does not support default domains. */ - if (prev_dom->type == type) + dom = iommu_group_alloc_default_domain(group, req_type); + if (!dom) { + /* Once in default_domain mode we never leave */ + if (group->default_domain) + return -ENODEV; + group->default_domain = NULL; return 0; - - group->default_domain = NULL; - group->domain = NULL; - - /* Sets group->default_domain to the newly allocated domain */ - group->default_domain = iommu_group_alloc_default_domain(group, type); - if (!group->default_domain) { - ret = -EINVAL; - goto restore_old_domain; } - group->domain = prev_dom; - ret = iommu_create_device_direct_mappings(group, dev); - if (ret) - goto free_new_domain; - - ret = __iommu_group_set_domain(group, group->default_domain); - if (ret) - goto free_new_domain; - - iommu_domain_free(prev_dom); + if (group->default_domain == dom) + return 0; - return 0; + /* + * IOMMU_RESV_DIRECT and IOMMU_RESV_DIRECT_RELAXABLE regions must be + * mapped before their device is attached, in order to guarantee + * continuity with any FW activity + */ + for_each_group_device(group, gdev) + iommu_create_device_direct_mappings(dom, gdev->dev); -free_new_domain: - iommu_domain_free(group->default_domain); -restore_old_domain: - group->default_domain = prev_dom; + /* We must set default_domain early for __iommu_device_set_domain */ + group->default_domain = dom; + if (!group->domain) { + /* + * Drivers are not allowed to fail the first domain attach. + * The only way to recover from this is to fail attaching the + * iommu driver and call ops->release_device. Put the domain + * in group->default_domain so it is freed after. + */ + ret = __iommu_group_set_domain_internal( + group, dom, IOMMU_SET_DOMAIN_MUST_SUCCEED); + if (WARN_ON(ret)) + goto out_free; + } else { + ret = __iommu_group_set_domain(group, dom); + if (ret) { + iommu_domain_free(dom); + group->default_domain = old_dom; + return ret; + } + } +out_free: + if (old_dom) + iommu_domain_free(old_dom); return ret; } @@ -2937,8 +2918,6 @@ restore_old_domain: static ssize_t iommu_group_store_type(struct iommu_group *group, const char *buf, size_t count) { - struct group_device *grp_dev; - struct device *dev; int ret, req_type; if (!capable(CAP_SYS_ADMIN) || !capable(CAP_SYS_RAWIO)) @@ -2976,10 +2955,7 @@ static ssize_t iommu_group_store_type(struct iommu_group *group, return -EPERM; } - grp_dev = list_first_entry(&group->devices, struct group_device, list); - dev = grp_dev->dev; - - ret = iommu_change_dev_def_domain(group, dev, req_type); + ret = iommu_setup_default_domain(group, req_type); /* * Release the mutex here because ops->probe_finalize() call-back of -- cgit v1.2.3 From 1000dccd5d134951d5fd37a7ad85ad7b19b825fc Mon Sep 17 00:00:00 2001 From: Jason Gunthorpe Date: Thu, 11 May 2023 01:42:13 -0300 Subject: iommu: Allow IOMMU_RESV_DIRECT to work on ARM For now several ARM drivers do not allow mappings to be created until a domain is attached. This means they do not technically support IOMMU_RESV_DIRECT as it requires the 1:1 maps to work continuously. Currently if the platform requests these maps on ARM systems they are silently ignored. Work around this by trying again to establish the direct mappings after the domain is attached if the pre-attach attempt failed. In the long run the drivers will be fixed to fully setup domains when they are created without waiting for attachment. Reviewed-by: Lu Baolu Reviewed-by: Kevin Tian Tested-by: Heiko Stuebner Tested-by: Niklas Schnelle Signed-off-by: Jason Gunthorpe Link: https://lore.kernel.org/r/15-v5-1b99ae392328+44574-iommu_err_unwind_jgg@nvidia.com Signed-off-by: Joerg Roedel --- drivers/iommu/iommu.c | 33 +++++++++++++++++++++++++++++++-- 1 file changed, 31 insertions(+), 2 deletions(-) (limited to 'drivers/iommu/iommu.c') diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index 34f721434b28..197d46a1d068 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -2842,6 +2842,7 @@ static int iommu_setup_default_domain(struct iommu_group *group, struct iommu_domain *old_dom = group->default_domain; struct group_device *gdev; struct iommu_domain *dom; + bool direct_failed; int req_type; int ret; @@ -2875,8 +2876,15 @@ static int iommu_setup_default_domain(struct iommu_group *group, * mapped before their device is attached, in order to guarantee * continuity with any FW activity */ - for_each_group_device(group, gdev) - iommu_create_device_direct_mappings(dom, gdev->dev); + direct_failed = false; + for_each_group_device(group, gdev) { + if (iommu_create_device_direct_mappings(dom, gdev->dev)) { + direct_failed = true; + dev_warn_once( + gdev->dev->iommu->iommu_dev->dev, + "IOMMU driver was not able to establish FW requested direct mapping."); + } + } /* We must set default_domain early for __iommu_device_set_domain */ group->default_domain = dom; @@ -2900,6 +2908,27 @@ static int iommu_setup_default_domain(struct iommu_group *group, } } + /* + * Drivers are supposed to allow mappings to be installed in a domain + * before device attachment, but some don't. Hack around this defect by + * trying again after attaching. If this happens it means the device + * will not continuously have the IOMMU_RESV_DIRECT map. + */ + if (direct_failed) { + for_each_group_device(group, gdev) { + ret = iommu_create_device_direct_mappings(dom, gdev->dev); + if (ret) + goto err_restore; + } + } + +err_restore: + if (old_dom) { + __iommu_group_set_domain_internal( + group, old_dom, IOMMU_SET_DOMAIN_MUST_SUCCEED); + iommu_domain_free(dom); + old_dom = NULL; + } out_free: if (old_dom) iommu_domain_free(old_dom); -- cgit v1.2.3 From e996c12d76d0b1aa8d5f082c6074e82398061943 Mon Sep 17 00:00:00 2001 From: Jason Gunthorpe Date: Thu, 11 May 2023 01:42:14 -0300 Subject: iommu: Remove __iommu_group_for_each_dev() The last two users of it are quite trivial, just open code the one line loop. Reviewed-by: Lu Baolu Tested-by: Heiko Stuebner Tested-by: Niklas Schnelle Signed-off-by: Jason Gunthorpe Link: https://lore.kernel.org/r/16-v5-1b99ae392328+44574-iommu_err_unwind_jgg@nvidia.com Signed-off-by: Joerg Roedel --- drivers/iommu/iommu.c | 53 ++++++++++++++++++++++++--------------------------- 1 file changed, 25 insertions(+), 28 deletions(-) (limited to 'drivers/iommu/iommu.c') diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index 197d46a1d068..1aaf3eb6fcca 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -1110,20 +1110,6 @@ void iommu_group_remove_device(struct device *dev) } EXPORT_SYMBOL_GPL(iommu_group_remove_device); -static int __iommu_group_for_each_dev(struct iommu_group *group, void *data, - int (*fn)(struct device *, void *)) -{ - struct group_device *device; - int ret = 0; - - for_each_group_device(group, device) { - ret = fn(device->dev, data); - if (ret) - break; - } - return ret; -} - /** * iommu_group_for_each_dev - iterate over each device in the group * @group: the group @@ -1138,10 +1124,15 @@ static int __iommu_group_for_each_dev(struct iommu_group *group, void *data, int iommu_group_for_each_dev(struct iommu_group *group, void *data, int (*fn)(struct device *, void *)) { - int ret; + struct group_device *device; + int ret = 0; mutex_lock(&group->mutex); - ret = __iommu_group_for_each_dev(group, data, fn); + for_each_group_device(group, device) { + ret = fn(device->dev, data); + if (ret) + break; + } mutex_unlock(&group->mutex); return ret; @@ -1791,20 +1782,12 @@ static int iommu_get_default_domain_type(struct iommu_group *group, return best_type; } -static int iommu_group_do_probe_finalize(struct device *dev, void *data) +static void iommu_group_do_probe_finalize(struct device *dev) { const struct iommu_ops *ops = dev_iommu_ops(dev); if (ops->probe_finalize) ops->probe_finalize(dev); - - return 0; -} - -static void __iommu_group_dma_finalize(struct iommu_group *group) -{ - __iommu_group_for_each_dev(group, group->default_domain, - iommu_group_do_probe_finalize); } int bus_iommu_probe(const struct bus_type *bus) @@ -1823,6 +1806,8 @@ int bus_iommu_probe(const struct bus_type *bus) return ret; list_for_each_entry_safe(group, next, &group_list, entry) { + struct group_device *gdev; + mutex_lock(&group->mutex); /* Remove item from the list */ @@ -1834,7 +1819,15 @@ int bus_iommu_probe(const struct bus_type *bus) return ret; } mutex_unlock(&group->mutex); - __iommu_group_dma_finalize(group); + + /* + * FIXME: Mis-locked because the ops->probe_finalize() call-back + * of some IOMMU drivers calls arm_iommu_attach_device() which + * in-turn might call back into IOMMU core code, where it tries + * to take group->mutex, resulting in a deadlock. + */ + for_each_group_device(group, gdev) + iommu_group_do_probe_finalize(gdev->dev); } return 0; @@ -2995,8 +2988,12 @@ static ssize_t iommu_group_store_type(struct iommu_group *group, mutex_unlock(&group->mutex); /* Make sure dma_ops is appropriatley set */ - if (!ret) - __iommu_group_dma_finalize(group); + if (!ret) { + struct group_device *gdev; + + for_each_group_device(group, gdev) + iommu_group_do_probe_finalize(gdev->dev); + } return ret ?: count; } -- cgit v1.2.3 From 5957c19305b10c73090b1390653ddf7e5e21aa35 Mon Sep 17 00:00:00 2001 From: Jason Gunthorpe Date: Thu, 11 May 2023 01:42:15 -0300 Subject: iommu: Tidy the control flow in iommu_group_store_type() Use a normal "goto unwind" instead of trying to be clever with checking !ret and manually managing the unlock. Reviewed-by: Lu Baolu Reviewed-by: Kevin Tian Tested-by: Heiko Stuebner Tested-by: Niklas Schnelle Signed-off-by: Jason Gunthorpe Link: https://lore.kernel.org/r/17-v5-1b99ae392328+44574-iommu_err_unwind_jgg@nvidia.com Signed-off-by: Joerg Roedel --- drivers/iommu/iommu.c | 27 +++++++++++++++------------ 1 file changed, 15 insertions(+), 12 deletions(-) (limited to 'drivers/iommu/iommu.c') diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index 1aaf3eb6fcca..9e0228ef612b 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -2940,6 +2940,7 @@ out_free: static ssize_t iommu_group_store_type(struct iommu_group *group, const char *buf, size_t count) { + struct group_device *gdev; int ret, req_type; if (!capable(CAP_SYS_ADMIN) || !capable(CAP_SYS_RAWIO)) @@ -2964,20 +2965,23 @@ static ssize_t iommu_group_store_type(struct iommu_group *group, if (req_type == IOMMU_DOMAIN_DMA_FQ && group->default_domain->type == IOMMU_DOMAIN_DMA) { ret = iommu_dma_init_fq(group->default_domain); - if (!ret) - group->default_domain->type = IOMMU_DOMAIN_DMA_FQ; - mutex_unlock(&group->mutex); + if (ret) + goto out_unlock; - return ret ?: count; + group->default_domain->type = IOMMU_DOMAIN_DMA_FQ; + ret = count; + goto out_unlock; } /* Otherwise, ensure that device exists and no driver is bound. */ if (list_empty(&group->devices) || group->owner_cnt) { - mutex_unlock(&group->mutex); - return -EPERM; + ret = -EPERM; + goto out_unlock; } ret = iommu_setup_default_domain(group, req_type); + if (ret) + goto out_unlock; /* * Release the mutex here because ops->probe_finalize() call-back of @@ -2988,13 +2992,12 @@ static ssize_t iommu_group_store_type(struct iommu_group *group, mutex_unlock(&group->mutex); /* Make sure dma_ops is appropriatley set */ - if (!ret) { - struct group_device *gdev; - - for_each_group_device(group, gdev) - iommu_group_do_probe_finalize(gdev->dev); - } + for_each_group_device(group, gdev) + iommu_group_do_probe_finalize(gdev->dev); + return count; +out_unlock: + mutex_unlock(&group->mutex); return ret ?: count; } -- cgit v1.2.3