iommu: Retire bus_set_iommu()
Subsystems (19)
Robin Murphy
reviewerv3 cover iommu: Retire bus_set_iommu()
v2: https://lore.kernel.org/linux-iommu/[email protected]/
Hi all,
Here's v3, now with working x86! Having finally made sense of how I broke Intel, I've given AMD the same fix by inspection. I'm still not 100% sure about s390, but it looks like it should probably be OK since it seems to register an IOMMU instance for each PCI device (?!) before disappearing into PCI hotplug code, wherein I assume we should never see a PCI device appear without its IOMMU already registered.
Otherwise, the only other updates are hooking up the new host1x context bus (noting that it now takes all of 4 lines to support a whole new bus, yay!), and a slight tweak to make sure we keep rejecting registration of conflicting iommu_ops rather than needlessly change that just yet.
Thanks,
Robin.
Robin Murphy (15):
iommu/vt-d: Handle race between registration and device probe iommu/amd: Handle race between registration and device probe
iommu: Always register bus notifiers
iommu: Move bus setup to IOMMU device registration iommu/amd: Clean up bus_set_iommu()
iommu/arm-smmu: Clean up bus_set_iommu() iommu/arm-smmu-v3: Clean up bus_set_iommu() iommu/dart: Clean up bus_set_iommu()
iommu/exynos: Clean up bus_set_iommu() iommu/ipmmu-vmsa: Clean up bus_set_iommu() iommu/mtk: Clean up bus_set_iommu()
iommu/omap: Clean up bus_set_iommu() iommu/tegra-smmu: Clean up bus_set_iommu() iommu/virtio: Clean up bus_set_iommu()
iommu: Clean up bus_set_iommu()
drivers/iommu/amd/amd_iommu.h | 1 - drivers/iommu/amd/init.c | 9 +- drivers/iommu/amd/iommu.c | 25 +---- drivers/iommu/apple-dart.c | 30 +---- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 53 +-------- drivers/iommu/arm/arm-smmu/arm-smmu.c | 84 +------------- drivers/iommu/arm/arm-smmu/qcom_iommu.c | 4 - drivers/iommu/exynos-iommu.c | 9 -- drivers/iommu/fsl_pamu_domain.c | 4 - drivers/iommu/intel/iommu.c | 4 +- drivers/iommu/iommu.c | 117 +++++++++----------- drivers/iommu/ipmmu-vmsa.c | 35 +----- drivers/iommu/msm_iommu.c | 2 - drivers/iommu/mtk_iommu.c | 24 +--- drivers/iommu/mtk_iommu_v1.c | 13 +-- drivers/iommu/omap-iommu.c | 6 - drivers/iommu/rockchip-iommu.c | 2 - drivers/iommu/s390-iommu.c | 6 - drivers/iommu/sprd-iommu.c | 5 - drivers/iommu/sun50i-iommu.c | 2 - drivers/iommu/tegra-smmu.c | 29 +---- drivers/iommu/virtio-iommu.c | 25 ----- include/linux/iommu.h | 1 - 23 files changed, 75 insertions(+), 415 deletions(-)
Signature · 1 line
2.36.1.dirty
Robin Murphy
reviewerPATCH v3 1/15 iommu/vt-d: Handle race between registration and device probe
Currently we rely on registering all our instances before initially allowing any .probe_device calls via bus_set_iommu(). In preparation for phasing out the latter, make sure we won't inadvertently return success for a device associated with a known but not yet registered instance, otherwise we'll run straight into iommu_group_get_for_dev() trying to use NULL ops.
That also highlights an issue with intel_iommu_get_resv_regions() taking dmar_global_lock from within a section where intel_iommu_init() already holds it, which already exists via probe_acpi_namespace_devices() when an ANDD device is probed, but gets more obvious with the upcoming change to iommu_device_register(). Since they are both read locks it manages not to deadlock in practice, so I'm leaving it here for someone with more confidence to tackle a larger rework of the locking.
---
v3: New
drivers/iommu/intel/iommu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/iommu/intel/iommu.c +1 -1
@@ -4600,7 +4600,7 @@ static struct iommu_device *intel_iommu_probe_device(struct device *dev)u8 bus, devfn; iommu =device_to_iommu (dev, &bus, &devfn); -if (!iommu) +if (!iommu || !iommu->iommu.ops)return ERR_PTR (-ENODEV ); info =kzalloc (sizeof (*info),GFP_KERNEL );
Signature · 1 line
2.36.1.dirty
Baolu Lu
maintainerRe: [PATCH v3 01/15] iommu/vt-d: Handle race between registration and device probe
Quoted 7 lines · Robin Murphy
Currently we rely on registering all our instances before initially allowing any .probe_device calls via
bus_set_iommu(). In preparation for phasing out the latter, make sure we won't inadvertently return success for a device associated with a known but not yet registered instance, otherwise we'll run straight intoiommu_group_get_for_dev()trying to use NULL ops.
Quoted 7 lines
That also highlights an issue with
intel_iommu_get_resv_regions()takingdmar_global_lockfrom within a section whereintel_iommu_init()already holds it, which already exists viaprobe_acpi_namespace_devices()when an ANDD device is probed, but gets more obvious with the upcoming change toiommu_device_register(). Since they are both read locks it manages not to deadlock in practice, so I'm leaving it here for someone with more confidence to tackle a larger rework of the locking.
Thanks for highlighting this. I will look into it later.
Best regards,
baolu
Quoted 22 lines
Signed-off-by Robin Murphy <[email protected]>---
v3: New
drivers/iommu/intel/iommu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/iommu/intel/iommu.c +1
-1@@ -4600,7 +4600,7 @@ static struct iommu_device *intel_iommu_probe_device(struct device *dev)u8 bus, devfn; iommu =device_to_iommu (dev, &bus, &devfn); -if (!iommu) +if (!iommu || !iommu->iommu.ops)return ERR_PTR (-ENODEV ); info =kzalloc (sizeof (*info),GFP_KERNEL );
RE: [PATCH v3 01/15] iommu/vt-d: Handle race between registration and device probe
In reply to Robin MurphyQuoted 18 lines
From: Robin Murphy <[email protected]>
Sent: Wednesday, July 6, 2022 1:08 AMCurrently we rely on registering all our instances before initially allowing any .probe_device calls via
bus_set_iommu(). In preparation for phasing out the latter, make sure we won't inadvertently return success for a device associated with a known but not yet registered instance, otherwise we'll run straight intoiommu_group_get_for_dev()trying to use NULL ops.That also highlights an issue with
intel_iommu_get_resv_regions()takingdmar_global_lockfrom within a section whereintel_iommu_init()already holds it, which already exists viaprobe_acpi_namespace_devices()when an ANDD device is probed, but gets more obvious with the upcoming change toiommu_device_register(). Since they are both read locks it manages not to deadlock in practice, so I'm leaving it here for someone with more confidence to tackle a larger rework of the locking.
Quoted 1 line
Signed-off-by Robin Murphy <[email protected]>
Baolu Lu
maintainerRe: [PATCH v3 01/15] iommu/vt-d: Handle race between registration and device probe
In reply to Robin MurphyQuoted 7 lines · Robin Murphy
That also highlights an issue with
intel_iommu_get_resv_regions()takingdmar_global_lockfrom within a section whereintel_iommu_init()already holds it, which already exists viaprobe_acpi_namespace_devices()when an ANDD device is probed, but gets more obvious with the upcoming change toiommu_device_register(). Since they are both read locks it manages not to deadlock in practice, so I'm leaving it here for someone with more confidence to tackle a larger rework of the locking.
I am trying to reproduce this problem. Strangely, even if I selected CONFIG_LOCKDEP=y, the kernel didn't complain anything. :-)
In fact the rmrr list in the Intel IOMMU driver is always static after parsing the ACPI/DMAR tables. There's no need to protect it with a lock. Hence we can safely remove below down/up_read().
4512 static void intel_iommu_get_resv_regions(struct device *device,
4513 struct list_head *head)
4514 {
4515 int prot = DMA_PTE_READ | DMA_PTE_WRITE; 4516 struct iommu_resv_region *reg; 4517 struct dmar_rmrr_unit *rmrr; 4518 struct device *i_dev; 4519 int i;
4520
4521 down_read(&dmar_global_lock);
4522 for_each_rmrr_units(rmrr) {
4523 for_each_active_dev_scope(rmrr->devices, rmrr->devices_cnt,
4524 i, i_dev) {
Best regards,
baolu
Robin Murphy
reviewerRe: [PATCH v3 01/15] iommu/vt-d: Handle race between registration and device probe
Quoted 11 lines · Baolu Lu
Quoted 7 lines · Robin Murphy
That also highlights an issue with
intel_iommu_get_resv_regions()takingdmar_global_lockfrom within a section whereintel_iommu_init()already holds it, which already exists viaprobe_acpi_namespace_devices()when an ANDD device is probed, but gets more obvious with the upcoming change toiommu_device_register(). Since they are both read locks it manages not to deadlock in practice, so I'm leaving it here for someone with more confidence to tackle a larger rework of the locking.I am trying to reproduce this problem. Strangely, even if I selected
CONFIG_LOCKDEP=y, the kernel didn't complain anything. :-)
FWIW, see below for the full report I get with this series applied (my machine doesn't have any ANDD entries to trigger the existing case).
Quoted 3 lines
In fact the rmrr list in the Intel IOMMU driver is always static after parsing the ACPI/DMAR tables. There's no need to protect it with a lock. Hence we can safely remove below down/up_read().
IIRC that leads to RCU warnings via for_each_dev_scope(), though. I did try replacing this down_read() with rcu_read_lock(), but then it doesn't like the GFP_KERNEL allocation in iommu_alloc_resv_region(), and that's where I gave up :)
I'm mostly left wondering whether the dmar_drhd_units list really needs to be RCU protected at all, as that seems to be the root of most of the problems here.
Cheers,
Robin.
Quoted 14 lines
4512 static void intel_iommu_get_resv_regions(struct device *device, 4513 struct
list_head*head) 4514 {
4515 int prot =DMA_PTE_READ|DMA_PTE_WRITE;
4516 structiommu_resv_region*reg;
4517 structdmar_rmrr_unit*rmrr;
4518 struct device*i_dev;
4519 int i;
4520
4521down_read(&dmar_global_lock);
4522for_each_rmrr_units(rmrr){
4523 for_each_active_dev_scope(rmrr->devices,rmrr->devices_cnt, 4524 i,i_dev) {
Quoted 2 lines
Best regards,
baolu
---->8-----
[ 11.421712] pci 0000:00:1b.0: Adding to iommu group 0
[ 11.421977]
[ 11.421978] ============================================
[ 11.421979] WARNING: possible recursive locking detected
[ 11.421981] 5.19.0-rc3-00015-gdc44a2269276 #32 Not tainted
[ 11.421984] --------------------------------------------
[ 11.421985] swapper/0/1 is trying to acquire lock:
[ 11.421986] ffffffffb987b770 (dmar_global_lock){++++}-{3:3}, at: intel_iommu_get_resv_regions+0x28/0x3a0
[ 11.422000]
[ 11.422000] but task is already holding lock:
[ 11.422001] ffffffffb987b770 (dmar_global_lock){++++}-{3:3}, at: intel_iommu_init+0x1638/0x1a08
[ 11.422011]
[ 11.422011] other info that might help us debug this:
[ 11.422013] Possible unsafe locking scenario:
[ 11.422013]
[ 11.422013] CPU0
[ 11.422014] ----
[ 11.422015] lock(dmar_global_lock);
[ 11.422018] lock(dmar_global_lock);
[ 11.422020]
[ 11.422020] *** DEADLOCK ***
[ 11.422020]
[ 11.422021] May be due to missing lock nesting notation
[ 11.422021]
[ 11.422022] 2 locks held by swapper/0/1:
[ 11.422024] #0: ffffffffb987b770 (dmar_global_lock){++++}-{3:3}, at: intel_iommu_init+0x1638/0x1a08
[ 11.422033] #1: ffff8881077a10c0 (&group->mutex){+.+.}-{3:3}, at: bus_iommu_probe+0x139/0x4c0
[ 11.422044]
[ 11.422044] stack backtrace:
[ 11.422046] CPU: 8 PID: 1 Comm: swapper/0 Not tainted 5.19.0-rc3-00015-gdc44a2269276 #32
[ 11.422050] Hardware name: LENOVO 30B6S08J03/1030, BIOS S01KT29A 06/20/2016
[ 11.422052] Call Trace:
[ 11.422054] <TASK>
[ 11.422056] dump_stack_lvl+0x45/0x59
[ 11.422064] __lock_acquire.cold+0x131/0x305
[ 11.422075] ? lockdep_hardirqs_on_prepare+0x220/0x220
[ 11.422082] ? lock_is_held_type+0xd7/0x130
[ 11.422090] ? rcu_read_lock_sched_held+0x9c/0xd0
[ 11.422098] lock_acquire+0x165/0x410
[ 11.422102] ? intel_iommu_get_resv_regions+0x28/0x3a0
[ 11.422108] ? lock_release+0x410/0x410
[ 11.422112] ? __iommu_domain_alloc+0xc5/0x130
[ 11.422116] ? iommu_group_alloc_default_domain+0x16/0x90
[ 11.422121] ? bus_iommu_probe+0x26d/0x4c0
[ 11.422126] ? iommu_device_register+0x11e/0x160
[ 11.422130] ? intel_iommu_init+0x16e0/0x1a08
[ 11.422135] ? do_one_initcall+0xb6/0x3c0
[ 11.422140] ? lock_is_held_type+0xd7/0x130
[ 11.422147] down_read+0x97/0x2f0
[ 11.422152] ? intel_iommu_get_resv_regions+0x28/0x3a0
[ 11.422156] ? rcu_read_lock_bh_held+0xb0/0xb0
[ 11.422161] ? rwsem_down_read_slowpath+0xa10/0xa10
[ 11.422166] ? find_held_lock+0x85/0xa0
[ 11.422173] intel_iommu_get_resv_regions+0x28/0x3a0
[ 11.422178] ? rcu_read_lock_sched_held+0x9c/0xd0
[ 11.422185] iommu_create_device_direct_mappings.isra.0+0x11a/0x330
[ 11.422193] ? iommu_map+0x50/0x50
[ 11.422198] ? __iommu_domain_alloc+0xc5/0x130
[ 11.422205] bus_iommu_probe+0x2bc/0x4c0
[ 11.422210] ? iommu_device_register+0xba/0x160
[ 11.422216] ? iommu_group_default_domain+0x20/0x20
[ 11.422221] ? do_raw_spin_lock+0x114/0x1d0
[ 11.422226] ? rwlock_bug.part.0+0x50/0x50
[ 11.422231] ? rwsem_down_read_slowpath+0xa10/0xa10
[ 11.422239] iommu_device_register+0x11e/0x160
[ 11.422244] intel_iommu_init+0x16e0/0x1a08
[ 11.422249] ? rcu_read_lock_bh_held+0xb0/0xb0
[ 11.422254] ? _raw_spin_unlock_irqrestore+0x28/0x50
[ 11.422261] ? lock_release+0x240/0x410
[ 11.422265] ? populate_rootfs+0x26/0x37
[ 11.422272] ? lock_downgrade+0x3a0/0x3a0
[ 11.422277] ? dmar_parse_one_rmrr+0x203/0x203
[ 11.422281] ? lock_is_held_type+0xd7/0x130
[ 11.422286] ? iommu_setup+0x282/0x282
[ 11.422291] ? rcu_read_lock_sched_held+0x9c/0xd0
[ 11.422296] ? rcu_read_lock_bh_held+0xb0/0xb0
[ 11.422301] ? up_write+0xd3/0x260
[ 11.422305] ? iommu_setup+0x282/0x282
[ 11.422309] pci_iommu_init+0x15/0x39
[ 11.422313] do_one_initcall+0xb6/0x3c0
[ 11.422317] ? initcall_blacklisted+0x120/0x120
[ 11.422322] ? rcu_read_lock_sched_held+0x9c/0xd0
[ 11.422327] ? rcu_read_lock_bh_held+0xb0/0xb0
[ 11.422331] ? kasan_unpoison+0x23/0x50
[ 11.422337] ? __kasan_slab_alloc+0x2c/0x80
[ 11.422344] kernel_init_freeable+0x330/0x389
[ 11.422349] ? rest_init+0x1b0/0x1b0
[ 11.422354] kernel_init+0x14/0x130
[ 11.422359] ret_from_fork+0x22/0x30
[ 11.422367] </TASK>Lu Baolu
maintainerRe: [PATCH v3 01/15] iommu/vt-d: Handle race between registration and device probe
Hi Robin,
Quoted 7 lines · Robin Murphy
Quoted 3 lines
In fact the rmrr list in the Intel IOMMU driver is always static after parsing the ACPI/DMAR tables. There's no need to protect it with a lock. Hence we can safely remove below down/up_read().
IIRC that leads to RCU warnings via
for_each_dev_scope(), though. I did try replacing thisdown_read()withrcu_read_lock(), but then it doesn't like theGFP_KERNELallocation iniommu_alloc_resv_region(), and that's where I gave up :)I'm mostly left wondering whether the
dmar_drhd_unitslist really needs to be RCU protected at all, as that seems to be the root of most of the problems here.
I just posted a fix patch here:
https://lore.kernel.org/linux-iommu/[email protected]/
It can remove the recursive locking and RCU warnings. Can you please take a look at it?
Best regards,
baolu
Robin Murphy
reviewerPATCH v3 2/15 iommu/amd: Handle race between registration and device probe
In reply to Robin MurphyAs for the Intel driver, make sure the AMD driver can cope with seeing .probe_device calls without having to wait for all known instances to register first.
---
v3: New
drivers/iommu/amd/iommu.c | 4 ++++ 1 file changed, 4 insertions(+)
drivers/iommu/amd/iommu.c +4 -0
@@ -1757,6 +1757,10 @@ static struct iommu_device *amd_iommu_probe_device(struct device *dev) devid =get_device_id (dev); iommu = amd_iommu_rlookup_table[devid]; +/* Not registered yet? */ +if (!iommu->iommu.ops) +return ERR_PTR (-ENODEV ); +if (dev_iommu_priv_get (dev))return &iommu->iommu;
Signature · 1 line
2.36.1.dirty
Robin Murphy
reviewerPATCH v3 3/15 iommu: Always register bus notifiers
In reply to Robin MurphyThe number of bus types that the IOMMU subsystem deals with is small and manageable, so pull that list into core code as a first step towards cleaning up all the boilerplate bus-awareness from drivers. Calling iommu_probe_device() before bus->iommu_ops is set will simply return -ENODEV and not break the notifier call chain, so there should be no harm in proactively registering all our bus notifiers at init time.
---
v3: Add host1x_context_bus
drivers/iommu/iommu.c | 53 ++++++++++++++++++++++++++----------------- 1 file changed, 32 insertions(+), 21 deletions(-)
drivers/iommu/iommu.c +32 -21
@@ -6,6 +6,7 @@#define pr_fmt (fmt) "iommu: " fmt +#include <linux/amba/bus.h> #include <linux/device.h> #include <linux/dma-iommu.h> #include <linux/kernel.h>
@@ -16,11 +17,13 @@#include <linux/export.h> #include <linux/slab.h> #include <linux/errno.h> +#include <linux/host1x_context_bus.h> #include <linux/iommu.h> #include <linux/idr.h> #include <linux/err.h> #include <linux/pci.h> #include <linux/bitops.h> +#include <linux/platform_device.h> #include <linux/property.h> #include <linux/fsl/mc.h> #include <linux/module.h>
@@ -75,6 +78,7 @@ static const char * const iommu_group_resv_type_string[] = {#define IOMMU_CMD_LINE_DMA_API BIT(0)#define IOMMU_CMD_LINE_STRICT BIT(1) +static int iommu_bus_init (struct bus_type *bus);static int iommu_alloc_default_domain (struct iommu_group *group,struct device *dev); static struct iommu_domain *__iommu_domain_alloc(struct bus_type *bus,
@@ -103,6 +107,22 @@ struct iommu_group_attribute iommu_group_attr_##_name = \static LIST_HEAD (iommu_device_list);static DEFINE_SPINLOCK (iommu_device_lock); +static struct bus_type *const iommu_buses[] = { + &platform_bus_type, +#ifdef CONFIG_PCI + &pci_bus_type, +#endif +#ifdef CONFIG_ARM_AMBA + &amba_bustype, +#endif +#ifdef CONFIG_FSL_MC_BUS + &fsl_mc_bus_type, +#endif +#ifdef CONFIG_TEGRA_HOST1X_CONTEXT_BUS + &host1x_context_device_bus_type, +#endif +}; +/* * Use a function instead of an array here because the domain-type is a * bit-field, so an array would waste memory.
@@ -152,6 +172,10 @@ static int __init iommu_subsys_init(void) (iommu_cmd_line &IOMMU_CMD_LINE_STRICT ) ? "(set via kernel command line)" : ""); +/* If the system is so broken that this fails, it will WARN anyway */ +for (int i =0 ; i <ARRAY_SIZE (iommu_buses); i++) +iommu_bus_init (iommu_buses[i]); +return 0 ; }subsys_initcall (iommu_subsys_init);
@@ -1772,35 +1796,19 @@ int bus_iommu_probe(struct bus_type *bus)return ret; } -static int iommu_bus_init (struct bus_type *bus,const struct iommu_ops *ops) +static int iommu_bus_init (struct bus_type *bus) {struct notifier_block *nb;int err; - nb =kzalloc (sizeof (struct notifier_block ),GFP_KERNEL ); + nb =kzalloc (sizeof (*nb),GFP_KERNEL );if (!nb)return -ENOMEM ; nb->notifier_call = iommu_bus_notifier; - err =bus_register_notifier (bus, nb);if (err) - goto out_free; - - err =bus_iommu_probe (bus); -if (err) - goto out_err; - - -return 0 ; - -out_err: -/* Clean up */ -bus_for_each_dev (bus,NULL ,NULL , remove_iommu_group); -bus_unregister_notifier (bus, nb); - -out_free: -kfree (nb); +kfree (nb);return err; }
@@ -1833,9 +1841,12 @@ int bus_set_iommu(struct bus_type *bus, const struct iommu_ops *ops) bus->iommu_ops = ops;/* Do IOMMU specific setup for this bus-type */ - err =iommu_bus_init (bus, ops); -if (err) + err =bus_iommu_probe (bus); +if (err) { +/* Clean up */ +bus_for_each_dev (bus,NULL ,NULL , remove_iommu_group); bus->iommu_ops =NULL ; + }return err; }
Signature · 1 line
2.36.1.dirty
Baolu Lu
maintainerRe: [PATCH v3 03/15] iommu: Always register bus notifiers
drivers/iommu/iommu.c static int __init iommu_subsys_init(void) · line 175
Quoted 8 lines · Robin Murphy
/*
* Use a function instead of an array here because the domain-type is a
* bit-field, so an array would waste memory.drivers/iommu/iommu.c +1
-0@@ -152,6 +172,10 @@ static int __init iommu_subsys_init(void) (iommu_cmd_line &IOMMU_CMD_LINE_STRICT ) ? "(set via kernel command line)" : ""); +/* If the system is so broken that this fails, it will WARN anyway */
Can you please elaborate a bit on this? iommu_bus_init() still return errors.
drivers/iommu/iommu.c static int __init iommu_subsys_init(void) · line 179
Quoted 4 lines
drivers/iommu/iommu.c +3
-0+for (int i =0 ; i <ARRAY_SIZE (iommu_buses); i++) +iommu_bus_init (iommu_buses[i]); +return 0 ;
Best regards,
baolu
Robin Murphy
reviewerRe: [PATCH v3 03/15] iommu: Always register bus notifiers
Quoted 12 lines · Baolu Lu
Quoted 8 lines · Robin Murphy
/*
* Use a function instead of an array here because the domain-type is a
* bit-field, so an array would waste memory.(quoted diff) +0
-0@@ -152,6 +172,10 @@ static int __init iommu_subsys_init(void)(
iommu_cmd_line&IOMMU_CMD_LINE_STRICT) ? "(set via kernel command line)" : ""); + /* If the system is so broken that this fails, it will WARN anyway */Can you please elaborate a bit on this?
iommu_bus_init()still return errors.
Indeed, it's commenting on the fact that we don't try to clean up or propagate an error value further even if it did ever manage to return one. I feared that if I strip the error handling out of iommu_bus_init() itself on the same reasoning, we'll just get constant patches from the static checker brigade trying to add it back, so it seemed like the neatest compromise to keep that decision where it's obviously in an early initcall, rather than in the helper function which can be viewed out of context. However, I'm happy to either expand this comment or go the whole way and make iommu_bus_init() return void if you think it's worthwhile.
Cheers,
Robin.
Quoted 8 lines
Quoted 5 lines
+ for (int i = 0; i <
ARRAY_SIZE(iommu_buses); i++) +iommu_bus_init(iommu_buses[i]);
+
return 0;Best regards,
baolu
Baolu Lu
maintainerRe: [PATCH v3 03/15] iommu: Always register bus notifiers
Quoted 14 lines · Robin Murphy
Quoted 12 lines · Baolu Lu
Quoted 8 lines · Robin Murphy
/*
* Use a function instead of an array here because the domain-type is a
* bit-field, so an array would waste memory.(quoted diff) +0
-0@@ -152,6 +172,10 @@ static int __init iommu_subsys_init(void)(
iommu_cmd_line&IOMMU_CMD_LINE_STRICT) ? "(set via kernel command line)" : ""); + /* If the system is so broken that this fails, it will WARN anyway */Can you please elaborate a bit on this?
iommu_bus_init()still return errors.
Quoted 1 line
Indeed, it's commenting on the fact that we don't try to clean up or propagate an error value further even if it did ever manage to return one. I feared that if I strip the error handling out of
iommu_bus_init()itself on the same reasoning, we'll just get constant patches from the static checker brigade trying to add it back, so it seemed like the neatest compromise to keep that decision where it's obviously in an early initcall, rather than in the helper function which can be viewed out of context. However, I'm happy to either expand this comment or go the whole way and makeiommu_bus_init()return void if you think it's worthwhile.
Thanks for the explanation. It would be helpful if the comment could be expanded. In this case, after a long time, people will not consider it an oversight. :-)
Best regards,
baolu
Quoted 13 lines
Cheers,
Robin.Quoted 8 lines
Quoted 5 lines
+ for (int i = 0; i <
ARRAY_SIZE(iommu_buses); i++) +iommu_bus_init(iommu_buses[i]);
+
return 0;Best regards,
baolu
RE: [PATCH v3 03/15] iommu: Always register bus notifiers
Quoted 30 lines
From: Baolu Lu <[email protected]>
Sent: Thursday, July 7, 2022 8:21 AMQuoted 15 lines · Robin Murphy
Quoted 13 lines · Baolu Lu
Quoted 9 lines · Robin Murphy
/*
* Use a function instead of an array here because the domain-type is a
* bit-field, so an array would waste memory.(quoted diff) +0
-0@@ -152,6 +172,10 @@ static int __init iommu_subsys_init(void)(
iommu_cmd_line&IOMMU_CMD_LINE_STRICT) ? "(set via kernel command line)" : ""); + /* If the system is so broken that this fails, it will WARN anyway */Can you please elaborate a bit on this?
iommu_bus_init()still return errors.Quoted 10 lines
Indeed, it's commenting on the fact that we don't try to clean up or propagate an error value further even if it did ever manage to return one. I feared that if I strip the error handling out of
iommu_bus_init()itself on the same reasoning, we'll just get constant patches from the static checker brigade trying to add it back, so it seemed like the neatest compromise to keep that decision where it's obviously in an early initcall, rather than in the helper function which can be viewed out of context. However, I'm happy to either expand this comment or go the whole way and makeiommu_bus_init()return void if you think it's worthwhile.
Quoted 4 lines
Thanks for the explanation. It would be helpful if the comment could be expanded. In this case, after a long time, people will not consider it an oversight. :-)
I'd prefer to making iommu_bus_init() return void plus expanding the comment otherwise the question arises that if the only caller is not interested in the return value then why bother returning it in the first place. 😊
Robin Murphy
reviewerRe: [PATCH v3 03/15] iommu: Always register bus notifiers
Quoted 35 lines · Kevin
Quoted 30 lines
From: Baolu Lu <[email protected]>
Sent: Thursday, July 7, 2022 8:21 AMQuoted 15 lines · Robin Murphy
Quoted 13 lines · Baolu Lu
Quoted 9 lines · Robin Murphy
/*
* Use a function instead of an array here because the domain-type
is a
* bit-field, so an array would waste memory.drivers/iommu/iommu.c +1
-0@@ -152,6 +172,10 @@ static int __init iommu_subsys_init(void) (iommu_cmd_line & IOMMU_CMD_LINE_STRICT) ? "(set via kernel command line)" : ""); + /* If the system is so broken that this fails, it will WARNanyway */
Can you please elaborate a bit on this?
iommu_bus_init()still return errors.Quoted 10 lines
Indeed, it's commenting on the fact that we don't try to clean up or propagate an error value further even if it did ever manage to return one. I feared that if I strip the error handling out of
iommu_bus_init()itself on the same reasoning, we'll just get constant patches from the static checker brigade trying to add it back, so it seemed like the neatest compromise to keep that decision where it's obviously in an early initcall, rather than in the helper function which can be viewed out of context. However, I'm happy to either expand this comment or go the whole way and makeiommu_bus_init()return void if you think it's worthwhile.Quoted 4 lines
Thanks for the explanation. It would be helpful if the comment could be expanded. In this case, after a long time, people will not consider it an oversight. :-)
Quoted 4 lines
I'd prefer to making
iommu_bus_init()return void plus expanding the comment otherwise the question arises that if the only caller is not interested in the return value then why bother returning it in the first place. 😊
OK, that's fair enough, will do.
Thanks,
Robin.
RE: [PATCH v3 03/15] iommu: Always register bus notifiers
In reply to Robin MurphyQuoted 10 lines
From: Robin Murphy <[email protected]>
Sent: Wednesday, July 6, 2022 1:08 AMThe number of bus types that the IOMMU subsystem deals with is small and manageable, so pull that list into core code as a first step towards cleaning up all the boilerplate bus-awareness from drivers. Calling
iommu_probe_device()beforebus->iommu_opsis set will simply return-ENODEVand not break the notifier call chain, so there should be no harm in proactively registering all our bus notifiers at init time.
Suppose we miss a check on iommu ops in iommu_release_device():
if (!dev->iommu) <<<<<<< return; iommu_device_unlink(dev->iommu->iommu_dev, dev); ops = dev_iommu_ops(dev); ops->release_device(dev);
following the rationale in patch01 a device could be removed when it's associated with a known but not registered instance.
Robin Murphy
reviewerRe: [PATCH v3 03/15] iommu: Always register bus notifiers
Quoted 21 lines · Kevin
Quoted 10 lines
From: Robin Murphy <[email protected]>
Sent: Wednesday, July 6, 2022 1:08 AMThe number of bus types that the IOMMU subsystem deals with is small and manageable, so pull that list into core code as a first step towards cleaning up all the boilerplate bus-awareness from drivers. Calling
iommu_probe_device()beforebus->iommu_opsis set will simply return-ENODEVand not break the notifier call chain, so there should be no harm in proactively registering all our bus notifiers at init time.Suppose we miss a check on iommu ops in
iommu_release_device():if (!dev->iommu) <<<<<<< return; iommu_device_unlink(dev->iommu->iommu_dev, dev); ops = dev_iommu_ops(dev); ops->release_device(dev);
Quoted 2 lines
following the rationale in patch01 a device could be removed when it's associated with a known but not registered instance.
No, because at that point the instance is only known internally to the driver. As long as it isn't erroneously returned from ->probe_device(dev), dev->iommu will remain NULL and the rest of the core code works as expected.
Thanks,
Robin.
RE: [PATCH v3 03/15] iommu: Always register bus notifiers
Quoted 26 lines
From: Robin Murphy <[email protected]>
Sent: Thursday, July 7, 2022 5:59 PMQuoted 4 lines · Kevin
Quoted 4 lines
From: Robin Murphy <[email protected]>
Sent: Wednesday, July 6, 2022 1:08 AMThe number of bus types that the IOMMU subsystem deals with is small
and
Quoted 17 lines
Quoted 6 lines
manageable, so pull that list into core code as a first step towards cleaning up all the boilerplate bus-awareness from drivers. Calling
iommu_probe_device()beforebus->iommu_opsis set will simply return-ENODEVand not break the notifier call chain, so there should be no harm in proactively registering all our bus notifiers at init time.Suppose we miss a check on iommu ops in
iommu_release_device():if (!dev->iommu) <<<<<<< return; iommu_device_unlink(dev->iommu->iommu_dev, dev); ops = dev_iommu_ops(dev); ops->release_device(dev);
Quoted 8 lines
Quoted 2 lines
following the rationale in patch01 a device could be removed when it's associated with a known but not registered instance.
No, because at that point the instance is only known internally to the driver. As long as it isn't erroneously returned from ->probe_device(dev),
dev->iommuwill remain NULL and the rest of the core code works as expected.
You are correct. I overlooked dev->iommu as device_to_iommu() in patch01. As long as the device hasn't been probed or ->probe_device doesn't do bad thing then dev->iommu should be NULL in this case.
Robin Murphy
reviewerPATCH v3 4/15 iommu: Move bus setup to IOMMU device registration
In reply to Robin MurphyMove the bus setup to iommu_device_register(). This should allow bus_iommu_probe() to be correctly replayed for multiple IOMMU instances, and leaves bus_set_iommu() as a glorified no-op to be cleaned up next.
At this point we can also handle cleanup better than just rolling back the most-recently-touched bus upon failure - which may release devices owned by other already-registered instances, and still leave devices on other buses with dangling pointers to the failed instance. Now it's easy to clean up the exact footprint of a given instance, no more, no less.
---
v3: Have iommu_device_register() return -EBUSY for conflicting ops
rather than change that behaviour just yet.
drivers/iommu/iommu.c | 54 ++++++++++++++++++++++++------------------- 1 file changed, 30 insertions(+), 24 deletions(-)
drivers/iommu/iommu.c +30 -24
@@ -180,6 +180,14 @@ static int __init iommu_subsys_init(void) }subsys_initcall (iommu_subsys_init); +static int remove_iommu_group (struct device *dev,void *data) +{ +if (dev->iommu && dev->iommu->iommu_dev == data) +iommu_release_device (dev); + +return 0 ; +} +/** * iommu_device_register() - Register an IOMMU hardware instance * @iommu: IOMMU handle for the instance
@@ -202,12 +210,32 @@ int iommu_device_register(struct iommu_device *iommu,spin_lock (&iommu_device_lock);list_add_tail (&iommu->list, &iommu_device_list);spin_unlock (&iommu_device_lock); + +for (int i =0 ; i <ARRAY_SIZE (iommu_buses); i++) { +struct bus_type *bus = iommu_buses[i]; +int err; + +if (bus->iommu_ops && bus->iommu_ops != ops) { + err = -EBUSY ; + }else { + bus->iommu_ops = ops; + err =bus_iommu_probe (bus); + } +if (err) { +iommu_device_unregister (iommu); +return err; + } + } +return 0 ; }EXPORT_SYMBOL_GPL (iommu_device_register);void iommu_device_unregister (struct iommu_device *iommu) { +for (int i =0 ; i <ARRAY_SIZE (iommu_buses); i++) +bus_for_each_dev (iommu_buses[i],NULL , iommu, remove_iommu_group); +spin_lock (&iommu_device_lock);list_del (&iommu->list);spin_unlock (&iommu_device_lock);
@@ -1633,13 +1661,6 @@ static int probe_iommu_group(struct device *dev, void *data)return ret; } -static int remove_iommu_group (struct device *dev,void *data) -{ -iommu_release_device (dev); - -return 0 ; -} -static int iommu_bus_notifier (struct notifier_block *nb,unsigned long action,void *data) {
@@ -1828,27 +1849,12 @@ static int iommu_bus_init(struct bus_type *bus)*/ int bus_set_iommu (struct bus_type *bus,const struct iommu_ops *ops) { -int err; - -if (ops ==NULL ) { - bus->iommu_ops =NULL ; -return 0 ; - } - -if (bus->iommu_ops !=NULL ) +if (bus->iommu_ops && ops && bus->iommu_ops != ops)return -EBUSY ; bus->iommu_ops = ops; -/* Do IOMMU specific setup for this bus-type */ - err =bus_iommu_probe (bus); -if (err) { -/* Clean up */ -bus_for_each_dev (bus,NULL ,NULL , remove_iommu_group); - bus->iommu_ops =NULL ; - } - -return err; +return 0 ; }EXPORT_SYMBOL_GPL (bus_set_iommu);
Signature · 1 line
2.36.1.dirty
Baolu Lu
maintainerRe: [PATCH v3 04/15] iommu: Move bus setup to IOMMU device registration
Quoted 14 lines · Robin Murphy
drivers/iommu/iommu.c +10
-0@@ -202,12 +210,32 @@ int iommu_device_register(struct iommu_device *iommu,spin_lock (&iommu_device_lock);list_add_tail (&iommu->list, &iommu_device_list);spin_unlock (&iommu_device_lock); + +for (int i =0 ; i <ARRAY_SIZE (iommu_buses); i++) { +struct bus_type *bus = iommu_buses[i]; +int err; + +if (bus->iommu_ops && bus->iommu_ops != ops) { + err = -EBUSY ; + }else { + bus->iommu_ops = ops; + err =bus_iommu_probe (bus);
drivers/iommu/iommu.c int iommu_device_register(struct iommu_device *iommu, · line 232
Quoted 10 lines
drivers/iommu/iommu.c +7
-0+ } +if (err) { +iommu_device_unregister (iommu); +return err; + } + } +return 0 ; }EXPORT_SYMBOL_GPL (iommu_device_register);
With bus_set_iommu() retired, my understanding is that now we embrace the first-come-first-serve policy for bus->iommu_ops setting. This will lead to problem in different iommu_ops for different bus case. Did I overlook anything?
Best regards,
baolu
Robin Murphy
reviewerRe: [PATCH v3 04/15] iommu: Move bus setup to IOMMU device registration
Quoted 23 lines · Baolu Lu
Quoted 22 lines · Robin Murphy
drivers/iommu/iommu.c +17
-0@@ -202,12 +210,32 @@ int iommu_device_register(struct iommu_device *iommu, spin_lock(&iommu_device_lock); list_add_tail(&iommu->list, &iommu_device_list); spin_unlock(&iommu_device_lock); + + for (int i = 0; i < ARRAY_SIZE(iommu_buses); i++) { + struct bus_type *bus = iommu_buses[i]; + int err; + + if (bus->iommu_ops && bus->iommu_ops != ops) { + err = -EBUSY; + } else { + bus->iommu_ops = ops; + err = bus_iommu_probe(bus); + } + if (err) { + iommu_device_unregister(iommu); + return err; + } + } +
Quoted 8 lines
drivers/iommu/iommu.c int iommu_device_register(struct iommu_device *iommu, · line 232
Quoted 3 lines
drivers/iommu/iommu.c +0
-0return 0; } EXPORT_SYMBOL_GPL(iommu_device_register);With
bus_set_iommu()retired, my understanding is that now we embrace the first-come-first-serve policy forbus->iommu_opssetting. This will lead to problem in differentiommu_opsfor different bus case. Did I overlook anything?
This is just formalising the de-facto situation that we don't actually have any combination of drivers that could load on the same system without already attempting to claim at least one bus in common. It's also only temporary until the bus ops are removed completely and we fully support multiple drivers coexisting, which only actually takes a handful more patches - I've realised I could even bring that change *ahead* of the big job of converting iommu_domain_alloc() (I'm not convinced that the tree-wide flag-day patch for that I currently have in the dev branch is really viable, nor that I've actually got the correct device at some of the callsites), although whether it's worth the potentially-surprising behaviour that might result I'm less sure.
If we already had systems where in-tree drivers successfully coexisted on different buses then I'd have split this up and done something a bit more involved to keep a vestigial bus_set_iommu() around until the final bus ops removal, but since we don't, it seemed neatest to do all the related work in one go.
Thanks,
Robin.
Baolu Lu
maintainerRe: [PATCH v3 04/15] iommu: Move bus setup to IOMMU device registration
Quoted 33 lines · Robin Murphy
Quoted 23 lines · Baolu Lu
Quoted 22 lines · Robin Murphy
drivers/iommu/iommu.c +17
-0@@ -202,12 +210,32 @@ int iommu_device_register(struct iommu_device *iommu, spin_lock(&iommu_device_lock); list_add_tail(&iommu->list, &iommu_device_list); spin_unlock(&iommu_device_lock); + + for (int i = 0; i < ARRAY_SIZE(iommu_buses); i++) { + struct bus_type *bus = iommu_buses[i]; + int err; + + if (bus->iommu_ops && bus->iommu_ops != ops) { + err = -EBUSY; + } else { + bus->iommu_ops = ops; + err = bus_iommu_probe(bus); + } + if (err) { + iommu_device_unregister(iommu); + return err; + } + } +Quoted 8 lines
drivers/iommu/iommu.c int iommu_device_register(struct iommu_device *iommu, · line 232
Quoted 3 lines
drivers/iommu/iommu.c +0
-0return 0; } EXPORT_SYMBOL_GPL(iommu_device_register);With
bus_set_iommu()retired, my understanding is that now we embrace the first-come-first-serve policy forbus->iommu_opssetting. This will lead to problem in differentiommu_opsfor different bus case. Did I overlook anything?
Quoted 3 lines
This is just formalising the de-facto situation that we don't actually have any combination of drivers that could load on the same system without already attempting to claim at least one bus in common. It's also only temporary until the bus ops are removed completely and we fully support multiple drivers coexisting, which only actually takes a handful more patches - I've realised I could even bring that change *ahead* of the big job of converting
iommu_domain_alloc()(I'm not convinced that the tree-wide flag-day patch for that I currently have in the dev branch is really viable, nor that I've actually got the correct device at some of the callsites), although whether it's worth the potentially-surprising behaviour that might result I'm less sure.If we already had systems where in-tree drivers successfully coexisted on different buses then I'd have split this up and done something a bit more involved to keep a vestigial
bus_set_iommu()around until the final bus ops removal, but since we don't, it seemed neatest to do all the related work in one go.
Fair enough. I've never seen a mixed system as far. It's fine for us to retire bus_set_iommu() for now and then formally support mixed IOMMU drivers later.
Best regards,
baolu
RE: [PATCH v3 04/15] iommu: Move bus setup to IOMMU device registration
In reply to Robin MurphyQuoted 15 lines
From: Robin Murphy <[email protected]>
Sent: Wednesday, July 6, 2022 1:08 AMdrivers/iommu/iommu.c +7
-0@@ -202,12 +210,32 @@ int iommu_device_register(struct iommu_device *iommu,spin_lock (&iommu_device_lock);list_add_tail (&iommu->list, &iommu_device_list);spin_unlock (&iommu_device_lock); + +for (int i =0 ; i <ARRAY_SIZE (iommu_buses); i++) { +struct bus_type *bus = iommu_buses[i]; +int err; + +if (bus->iommu_ops && bus->iommu_ops != ops) { + err = -EBUSY ;
drivers/iommu/iommu.c int iommu_device_register(struct iommu_device · line 230
Quoted 10 lines
drivers/iommu/iommu.c +10
-0+ }else { + bus->iommu_ops = ops; + err =bus_iommu_probe (bus); + } +if (err) { +iommu_device_unregister (iommu); +return err; + } + } +
Probably move above into a new function bus_iommu_probe_all():
/* probe all buses for devices associated with this iommu */
err = bus_iommu_probe_all();
if (err) {
iommu_device_unregister(iommu);
return err;
}Just my personal preference on leaving logic in iommu_device_register() more relevant to the iommu instance itself.
Apart from that:
Robin Murphy
reviewerRe: [PATCH v3 04/15] iommu: Move bus setup to IOMMU device registration
Quoted 35 lines · Kevin
Quoted 15 lines
From: Robin Murphy <[email protected]>
Sent: Wednesday, July 6, 2022 1:08 AMdrivers/iommu/iommu.c +7
-0@@ -202,12 +210,32 @@ int iommu_device_register(struct iommu_device *iommu,spin_lock (&iommu_device_lock);list_add_tail (&iommu->list, &iommu_device_list);spin_unlock (&iommu_device_lock); + +for (int i =0 ; i <ARRAY_SIZE (iommu_buses); i++) { +struct bus_type *bus = iommu_buses[i]; +int err; + +if (bus->iommu_ops && bus->iommu_ops != ops) { + err = -EBUSY ;drivers/iommu/iommu.c int iommu_device_register(struct iommu_device · line 230
Quoted 10 lines
drivers/iommu/iommu.c +10
-0+ }else { + bus->iommu_ops = ops; + err =bus_iommu_probe (bus); + } +if (err) { +iommu_device_unregister (iommu); +return err; + } + } +Probably move above into a new function
bus_iommu_probe_all():/* probe all buses for devices associated with this iommu */ err = bus_iommu_probe_all(); if (err) { iommu_device_unregister(iommu); return err; }
Quoted 2 lines
Just my personal preference on leaving logic in
iommu_device_register()more relevant to the iommu instance itself.
On reflection I think it makes sense to pull the iommu_device_unregister() out of the loop anyway - I think that's really a left-over from between v1 and v2 when that error case briefly jumped to another cleanup loop, before I realised it was actually trivial for iommu_device_unregister() to clean up for itself.
However I now see I've also missed another opportunity, and the -EBUSY case should be hoisted out of the loop as well, since checking iommu_buses[0] is sufficient. Then it's hopefully much clearer that once the bus ops go away we'll be left with just a single extra line for the loop, as in iommu_device_unregister(). Does that sound reasonable?
Quoted 3 lines
Apart from that:
Reviewed-by Kevin Tian <[email protected]>
Thanks! (and for the others as well)
Robin.
RE: [PATCH v3 04/15] iommu: Move bus setup to IOMMU device registration
Quoted 49 lines
From: Robin Murphy <[email protected]>
Sent: Thursday, July 7, 2022 6:58 PMQuoted 4 lines · Kevin
Review int iommu_device_register(struct
Quoted 4 lines
From: Robin Murphy <[email protected]>
Sent: Wednesday, July 6, 2022 1:08 AM(quoted diff) +0
-0@@ -202,12 +210,32 @@ int iommu_device_register(struct
iommu_deviceQuoted 31 lines
Quoted 11 lines
drivers/iommu/iommu.c +7
-0*iommu,spin_lock (&iommu_device_lock);list_add_tail (&iommu->list, &iommu_device_list);spin_unlock (&iommu_device_lock); + +for (int i =0 ; i <ARRAY_SIZE (iommu_buses); i++) { +struct bus_type *bus = iommu_buses[i]; +int err; + +if (bus->iommu_ops && bus->iommu_ops != ops) { + err = -EBUSY ;drivers/iommu/iommu.c int iommu_device_register(struct iommu_device *iommu, · line 229
Quoted 10 lines
drivers/iommu/iommu.c +10
-0+ }else { + bus->iommu_ops = ops; + err =bus_iommu_probe (bus); + } +if (err) { +iommu_device_unregister (iommu); +return err; + } + } +Probably move above into a new function
bus_iommu_probe_all():/* probe all buses for devices associated with this iommu */ err = bus_iommu_probe_all(); if (err) { iommu_device_unregister(iommu); return err; }Quoted 2 lines
Just my personal preference on leaving logic in
iommu_device_register()more relevant to the iommu instance itself.On reflection I think it makes sense to pull the
iommu_device_unregister()out of the loop anyway - I think that's really a left-over from between v1 and v2 when that error case briefly jumped to another cleanup loop, before I realised it was actually trivial foriommu_device_unregister()to clean up for itself.
Quoted 6 lines
However I now see I've also missed another opportunity, and the
-EBUSYcase should be hoisted out of the loop as well, since checking iommu_buses[0] is sufficient. Then it's hopefully much clearer that once the bus ops go away we'll be left with just a single extra line for the loop, as iniommu_device_unregister(). Does that sound reasonable?
Yes, sounds good to me.
Robin Murphy
reviewerPATCH v3 5/15 iommu/amd: Clean up bus_set_iommu()
In reply to Robin MurphyStop calling bus_set_iommu() since it's now unnecessary, and garbage-collect the last remnants of amd_iommu_init_api().
---
v3: No change
drivers/iommu/amd/amd_iommu.h | 1 - drivers/iommu/amd/init.c | 9 +-------- drivers/iommu/amd/iommu.c | 21 --------------------- 3 files changed, 1 insertion(+), 30 deletions(-)
drivers/iommu/amd/amd_iommu.h +0 -1
@@ -18,7 +18,6 @@ extern void amd_iommu_restart_event_logging(struct amd_iommu *iommu);extern int amd_iommu_init_devices (void );extern void amd_iommu_uninit_devices (void );extern void amd_iommu_init_notifier (void ); -extern int amd_iommu_init_api (void );#ifdef CONFIG_AMD_IOMMU_DEBUGFS void amd_iommu_debugfs_setup (struct amd_iommu *iommu);
drivers/iommu/amd/init.c +1 -8
@@ -1969,20 +1969,13 @@ static int __init amd_iommu_init_pci(void)/* * Order is important here to make sure any unity map requirements are * fulfilled. The unity mappings are created and written to the device -* table during the amd_iommu_init_api() call. +* table during the iommu_init_pci() call. * * After that we call init_device_table_dma() to make sure any * uninitialized DTE will block DMA, and in the end we flush the caches * of all IOMMUs to make sure the changes to the device table are * active. */ - ret =amd_iommu_init_api (); -if (ret) { -pr_err ("IOMMU: Failed to initialize IOMMU-API interface (error=%d)!\n" , - ret); - goto out; - } -init_device_table_dma ();for_each_iommu (iommu)
drivers/iommu/amd/iommu.c +0 -21
@@ -11,8 +11,6 @@#include <linux/ratelimit.h> #include <linux/pci.h> #include <linux/acpi.h> -#include <linux/amba/bus.h> -#include <linux/platform_device.h> #include <linux/pci-ats.h> #include <linux/bitmap.h> #include <linux/slab.h>
@@ -1842,25 +1840,6 @@ void amd_iommu_domain_update(struct protection_domain *domain)amd_iommu_domain_flush_complete (domain); } -int __init amd_iommu_init_api (void ) -{ -int err; - - err =bus_set_iommu (&pci_bus_type, &amd_iommu_ops); -if (err) -return err; -#ifdef CONFIG_ARM_AMBA - err =bus_set_iommu (&amba_bustype, &amd_iommu_ops); -if (err) -return err; -#endif - err =bus_set_iommu (&platform_bus_type, &amd_iommu_ops); -if (err) -return err; - -return 0 ; -} -/***************************************************************************** * * The following functions belong to the exported interface of AMD IOMMU
Signature · 1 line
2.36.1.dirty
Robin Murphy
reviewerPATCH v3 6/15 iommu/arm-smmu: Clean up bus_set_iommu()
In reply to Robin MurphyStop calling bus_set_iommu() since it's now unnecessary. With device probes now replayed for every IOMMU instance registration, the whole sorry ordering workaround for legacy DT bindings goes too, hooray!
---
v3: No change
drivers/iommu/arm/arm-smmu/arm-smmu.c | 84 +-------------------------- 1 file changed, 2 insertions(+), 82 deletions(-)
drivers/iommu/arm/arm-smmu/arm-smmu.c +2 -82
@@ -37,7 +37,6 @@#include <linux/ratelimit.h> #include <linux/slab.h> -#include <linux/amba/bus.h> #include <linux/fsl/mc.h> #include "arm-smmu.h"
@@ -93,8 +92,6 @@ static struct platform_driver arm_smmu_driver;static struct iommu_ops arm_smmu_ops;#ifdef CONFIG_ARM_SMMU_LEGACY_DT_BINDINGS -static int arm_smmu_bus_init (struct iommu_ops *ops); -static struct device_node *dev_get_dev_node (struct device *dev) {if (dev_is_pci (dev)) {
@@ -180,20 +177,6 @@ static int arm_smmu_register_legacy_master(struct device *dev,kfree (sids); return err; } - -/* -* With the legacy DT binding in play, we have no guarantees about -* probe order, but then we're also not doing default domains, so we can -* delay setting bus ops until we're sure every possible SMMU is ready, -* and that way ensure that no probe_device() calls get missed. -*/ -static int arm_smmu_legacy_bus_init (void ) -{ -if (using_legacy_binding) -return arm_smmu_bus_init (&arm_smmu_ops); -return 0 ; -} -device_initcall_sync (arm_smmu_legacy_bus_init);#else static int arm_smmu_register_legacy_master (struct device *dev,struct arm_smmu_device **smmu)
@@ -2025,52 +2008,6 @@ static int arm_smmu_device_dt_probe(struct arm_smmu_device *smmu,return 0 ; } -static int arm_smmu_bus_init (struct iommu_ops *ops) -{ -int err; - -/* Oh, for a proper bus abstraction */ -if (!iommu_present (&platform_bus_type)) { - err =bus_set_iommu (&platform_bus_type, ops); -if (err) -return err; - } -#ifdef CONFIG_ARM_AMBA -if (!iommu_present (&amba_bustype)) { - err =bus_set_iommu (&amba_bustype, ops); -if (err) - goto err_reset_platform_ops; - } -#endif -#ifdef CONFIG_PCI -if (!iommu_present (&pci_bus_type)) { - err =bus_set_iommu (&pci_bus_type, ops); -if (err) - goto err_reset_amba_ops; - } -#endif -#ifdef CONFIG_FSL_MC_BUS -if (!iommu_present (&fsl_mc_bus_type)) { - err =bus_set_iommu (&fsl_mc_bus_type, ops); -if (err) - goto err_reset_pci_ops; - } -#endif -return 0 ; - -err_reset_pci_ops:__maybe_unused ; -#ifdef CONFIG_PCI -bus_set_iommu (&pci_bus_type,NULL ); -#endif -err_reset_amba_ops:__maybe_unused ; -#ifdef CONFIG_ARM_AMBA -bus_set_iommu (&amba_bustype,NULL ); -#endif -err_reset_platform_ops:__maybe_unused ; -bus_set_iommu (&platform_bus_type,NULL ); -return err; -} -static int arm_smmu_device_probe (struct platform_device *pdev) {struct resource *res;
@@ -2187,7 +2124,8 @@ static int arm_smmu_device_probe(struct platform_device *pdev) err =iommu_device_register (&smmu->iommu, &arm_smmu_ops, dev);if (err) {dev_err (dev,"Failed to register iommu\n" ); - goto err_sysfs_remove; +iommu_device_sysfs_remove (&smmu->iommu); +return err; }platform_set_drvdata (pdev, smmu);
@@ -2205,24 +2143,7 @@ static int arm_smmu_device_probe(struct platform_device *pdev)pm_runtime_enable (dev); } -/* -* For ACPI and generic DT bindings, an SMMU will be probed before -* any device which might need it, so we want the bus ops in place -* ready to handle default domain setup as soon as any SMMU exists. -*/ -if (!using_legacy_binding) { - err =arm_smmu_bus_init (&arm_smmu_ops); -if (err) - goto err_unregister_device; - } -return 0 ; - -err_unregister_device: -iommu_device_unregister (&smmu->iommu); -err_sysfs_remove: -iommu_device_sysfs_remove (&smmu->iommu); -return err; }static int arm_smmu_device_remove (struct platform_device *pdev)
@@ -2235,7 +2156,6 @@ static int arm_smmu_device_remove(struct platform_device *pdev)if (!bitmap_empty (smmu->context_map,ARM_SMMU_MAX_CBS ))dev_notice (&pdev->dev,"disabling translation\n" ); -arm_smmu_bus_init (NULL );iommu_device_unregister (&smmu->iommu);iommu_device_sysfs_remove (&smmu->iommu);
Signature · 1 line
2.36.1.dirty
Robin Murphy
reviewerPATCH v3 7/15 iommu/arm-smmu-v3: Clean up bus_set_iommu()
In reply to Robin MurphyStop calling bus_set_iommu() since it's now unnecessary, and simplify the probe failure path accordingly.
---
v3: No change
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 53 +-------------------- 1 file changed, 2 insertions(+), 51 deletions(-)
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +2 -51
@@ -28,8 +28,6 @@#include <linux/pci-ats.h> #include <linux/platform_device.h> -#include <linux/amba/bus.h> -#include "arm-smmu-v3.h" #include "../../iommu-sva-lib.h"
@@ -3698,43 +3696,6 @@ static unsigned long arm_smmu_resource_size(struct arm_smmu_device *smmu)return SZ_128K ; } -static int arm_smmu_set_bus_ops (struct iommu_ops *ops) -{ -int err; - -#ifdef CONFIG_PCI -if (pci_bus_type.iommu_ops != ops) { - err =bus_set_iommu (&pci_bus_type, ops); -if (err) -return err; - } -#endif -#ifdef CONFIG_ARM_AMBA -if (amba_bustype.iommu_ops != ops) { - err =bus_set_iommu (&amba_bustype, ops); -if (err) - goto err_reset_pci_ops; - } -#endif -if (platform_bus_type.iommu_ops != ops) { - err =bus_set_iommu (&platform_bus_type, ops); -if (err) - goto err_reset_amba_ops; - } - -return 0 ; - -err_reset_amba_ops: -#ifdef CONFIG_ARM_AMBA -bus_set_iommu (&amba_bustype,NULL ); -#endif -err_reset_pci_ops:__maybe_unused ; -#ifdef CONFIG_PCI -bus_set_iommu (&pci_bus_type,NULL ); -#endif -return err; -} -static void __iomem *arm_smmu_ioremap (struct device *dev,resource_size_t start,resource_size_t size) {
@@ -3840,27 +3801,17 @@ static int arm_smmu_device_probe(struct platform_device *pdev) ret =iommu_device_register (&smmu->iommu, &arm_smmu_ops, dev);if (ret) {dev_err (dev,"Failed to register iommu\n" ); - goto err_sysfs_remove; +iommu_device_sysfs_remove (&smmu->iommu); +return ret; } - ret =arm_smmu_set_bus_ops (&arm_smmu_ops); -if (ret) - goto err_unregister_device; -return 0 ; - -err_unregister_device: -iommu_device_unregister (&smmu->iommu); -err_sysfs_remove: -iommu_device_sysfs_remove (&smmu->iommu); -return ret; }static int arm_smmu_device_remove (struct platform_device *pdev) {struct arm_smmu_device *smmu =platform_get_drvdata (pdev); -arm_smmu_set_bus_ops (NULL );iommu_device_unregister (&smmu->iommu);iommu_device_sysfs_remove (&smmu->iommu);arm_smmu_device_disable (smmu);
Signature · 1 line
2.36.1.dirty
Robin Murphy
reviewerPATCH v3 8/15 iommu/dart: Clean up bus_set_iommu()
In reply to Robin MurphyStop calling bus_set_iommu() since it's now unnecessary, and simplify the probe failure path accordingly.
---
v3: No change
drivers/iommu/apple-dart.c | 30 +----------------------------- 1 file changed, 1 insertion(+), 29 deletions(-)
drivers/iommu/apple-dart.c +1 -29
@@ -824,27 +824,6 @@ static irqreturn_t apple_dart_irq(int irq, void *dev)return IRQ_HANDLED ; } -static int apple_dart_set_bus_ops (const struct iommu_ops *ops) -{ -int ret; - -if (!iommu_present (&platform_bus_type)) { - ret =bus_set_iommu (&platform_bus_type, ops); -if (ret) -return ret; - } -#ifdef CONFIG_PCI -if (!iommu_present (&pci_bus_type)) { - ret =bus_set_iommu (&pci_bus_type, ops); -if (ret) { -bus_set_iommu (&platform_bus_type,NULL ); -return ret; - } - } -#endif -return 0 ; -} -static int apple_dart_probe (struct platform_device *pdev) {int ret;
@@ -899,14 +878,10 @@ static int apple_dart_probe(struct platform_device *pdev)platform_set_drvdata (pdev, dart); - ret =apple_dart_set_bus_ops (&apple_dart_iommu_ops); -if (ret) - goto err_free_irq; - ret =iommu_device_sysfs_add (&dart->iommu, dev,NULL ,"apple-dart.%s" ,dev_name (&pdev->dev));if (ret) - goto err_remove_bus_ops; + goto err_free_irq; ret =iommu_device_register (&dart->iommu, &apple_dart_iommu_ops, dev);if (ret)
@@ -920,8 +895,6 @@ static int apple_dart_probe(struct platform_device *pdev) err_sysfs_remove:iommu_device_sysfs_remove (&dart->iommu); -err_remove_bus_ops: -apple_dart_set_bus_ops (NULL ); err_free_irq:free_irq (dart->irq, dart); err_clk_disable:
@@ -936,7 +909,6 @@ static int apple_dart_remove(struct platform_device *pdev)apple_dart_hw_reset (dart);free_irq (dart->irq, dart); -apple_dart_set_bus_ops (NULL );iommu_device_unregister (&dart->iommu);iommu_device_sysfs_remove (&dart->iommu);
Signature · 1 line
2.36.1.dirty
Robin Murphy
reviewerPATCH v3 9/15 iommu/exynos: Clean up bus_set_iommu()
In reply to Robin MurphyStop calling bus_set_iommu() since it's now unnecessary, and simplify the init failure path accordingly.
---
v3: No change
drivers/iommu/exynos-iommu.c | 9 --------- 1 file changed, 9 deletions(-)
drivers/iommu/exynos-iommu.c +0 -9
@@ -1356,16 +1356,7 @@ static int __init exynos_iommu_init(void) goto err_zero_lv2; } - ret =bus_set_iommu (&platform_bus_type, &exynos_iommu_ops); -if (ret) { -pr_err ("%s: Failed to register exynos-iommu driver.\n" , - __func__); - goto err_set_iommu; - } -return 0 ; -err_set_iommu: -kmem_cache_free (lv2table_kmem_cache, zero_lv2_table); err_zero_lv2:platform_driver_unregister (&exynos_sysmmu_driver); err_reg_driver:
Signature · 1 line
2.36.1.dirty
Robin Murphy
reviewerPATCH v3 10/15 iommu/ipmmu-vmsa: Clean up bus_set_iommu()
In reply to Robin MurphyStop calling bus_set_iommu() since it's now unnecessary. This also leaves the custom initcall effectively doing nothing but register the driver, which no longer needs to happen early either, so convert it to builtin_platform_driver().
---
v3: No change
drivers/iommu/ipmmu-vmsa.c | 35 +---------------------------------- 1 file changed, 1 insertion(+), 34 deletions(-)
drivers/iommu/ipmmu-vmsa.c +1 -34
@@ -1090,11 +1090,6 @@ static int ipmmu_probe(struct platform_device *pdev) ret =iommu_device_register (&mmu->iommu, &ipmmu_ops, &pdev->dev);if (ret)return ret; - -#if defined(CONFIG_IOMMU_DMA ) -if (!iommu_present (&platform_bus_type)) -bus_set_iommu (&platform_bus_type, &ipmmu_ops); -#endif }/*
@@ -1168,32 +1163,4 @@ static struct platform_driver ipmmu_driver = { .probe = ipmmu_probe, .remove = ipmmu_remove, }; - -static int __init ipmmu_init (void ) -{ -struct device_node *np; -static bool setup_done; -int ret; - -if (setup_done) -return 0 ; - - np =of_find_matching_node (NULL , ipmmu_of_ids); -if (!np) -return 0 ; - -of_node_put (np); - - ret =platform_driver_register (&ipmmu_driver); -if (ret <0 ) -return ret; - -#if defined(CONFIG_ARM ) && !defined(CONFIG_IOMMU_DMA ) -if (!iommu_present (&platform_bus_type)) -bus_set_iommu (&platform_bus_type, &ipmmu_ops); -#endif - - setup_done = true; -return 0 ; -} -subsys_initcall (ipmmu_init); +builtin_platform_driver (ipmmu_driver);
Signature · 1 line
2.36.1.dirty
Robin Murphy
reviewerPATCH v3 11/15 iommu/mtk: Clean up bus_set_iommu()
In reply to Robin MurphyStop calling bus_set_iommu() since it's now unnecessary, and simplify the probe failure paths accordingly.
---
v3: Rebase around recent probe/remove changes
drivers/iommu/mtk_iommu.c | 24 +----------------------- drivers/iommu/mtk_iommu_v1.c | 13 +------------ 2 files changed, 2 insertions(+), 35 deletions(-)
drivers/iommu/mtk_iommu.c +1 -23
@@ -1239,30 +1239,13 @@ static int mtk_iommu_probe(struct platform_device *pdev) data->hw_list = &data->hw_list_head; } -if (!iommu_present (&platform_bus_type)) { - ret =bus_set_iommu (&platform_bus_type, &mtk_iommu_ops); -if (ret) - goto out_list_del; - } -if (MTK_IOMMU_IS_TYPE (data->plat_data,MTK_IOMMU_TYPE_MM )) { ret =component_master_add_with_match (dev, &mtk_iommu_com_ops, match);if (ret) - goto out_bus_set_null; - }else if (MTK_IOMMU_IS_TYPE (data->plat_data,MTK_IOMMU_TYPE_INFRA ) && -MTK_IOMMU_HAS_FLAG (data->plat_data,IFA_IOMMU_PCIE_SUPPORT )) { -#ifdef CONFIG_PCI -if (!iommu_present (&pci_bus_type)) { - ret =bus_set_iommu (&pci_bus_type, &mtk_iommu_ops); -if (ret)/* PCIe fail don't affect platform_bus. */ - goto out_list_del; - } -#endif + goto out_list_del; }return ret; -out_bus_set_null: -bus_set_iommu (&platform_bus_type,NULL ); out_list_del:list_del (&data->list);iommu_device_unregister (&data->iommu);
@@ -1290,11 +1273,6 @@ static int mtk_iommu_remove(struct platform_device *pdev)if (MTK_IOMMU_IS_TYPE (data->plat_data,MTK_IOMMU_TYPE_MM )) {device_link_remove (data->smicomm_dev, &pdev->dev);component_master_del (&pdev->dev, &mtk_iommu_com_ops); - }else if (MTK_IOMMU_IS_TYPE (data->plat_data,MTK_IOMMU_TYPE_INFRA ) && -MTK_IOMMU_HAS_FLAG (data->plat_data,IFA_IOMMU_PCIE_SUPPORT )) { -#ifdef CONFIG_PCI -bus_set_iommu (&pci_bus_type,NULL ); -#endif }pm_runtime_disable (&pdev->dev);for (i =0 ; i < data->plat_data->banks_num; i++) {
drivers/iommu/mtk_iommu_v1.c +1 -12
@@ -696,19 +696,11 @@ static int mtk_iommu_v1_probe(struct platform_device *pdev)if (ret) goto out_sysfs_remove; -if (!iommu_present (&platform_bus_type)) { - ret =bus_set_iommu (&platform_bus_type, &mtk_iommu_v1_ops); -if (ret) - goto out_dev_unreg; - } - ret =component_master_add_with_match (dev, &mtk_iommu_v1_com_ops, match);if (ret) - goto out_bus_set_null; + goto out_dev_unreg;return ret; -out_bus_set_null: -bus_set_iommu (&platform_bus_type,NULL ); out_dev_unreg:iommu_device_unregister (&data->iommu); out_sysfs_remove:
@@ -723,9 +715,6 @@ static int mtk_iommu_v1_remove(struct platform_device *pdev)iommu_device_sysfs_remove (&data->iommu);iommu_device_unregister (&data->iommu); -if (iommu_present (&platform_bus_type)) -bus_set_iommu (&platform_bus_type,NULL ); -clk_disable_unprepare (data->bclk);devm_free_irq (&pdev->dev, data->irq, data);component_master_del (&pdev->dev, &mtk_iommu_v1_com_ops);
Signature · 1 line
2.36.1.dirty
Robin Murphy
reviewerPATCH v3 12/15 iommu/omap: Clean up bus_set_iommu()
In reply to Robin MurphyStop calling bus_set_iommu() since it's now unnecessary, and simplify the init failure path accordingly.
---
v3: No change
drivers/iommu/omap-iommu.c | 6 ------ 1 file changed, 6 deletions(-)
drivers/iommu/omap-iommu.c +0 -6
@@ -1776,14 +1776,8 @@ static int __init omap_iommu_init(void) goto fail_driver; } - ret =bus_set_iommu (&platform_bus_type, &omap_iommu_ops); -if (ret) - goto fail_bus; -return 0 ; -fail_bus: -platform_driver_unregister (&omap_iommu_driver); fail_driver:kmem_cache_destroy (iopte_cachep);return ret;
Signature · 1 line
2.36.1.dirty
Robin Murphy
reviewerPATCH v3 13/15 iommu/tegra-smmu: Clean up bus_set_iommu()
In reply to Robin MurphyStop calling bus_set_iommu() since it's now unnecessary, and simplify the probe failure path accordingly.
---
v3: No change
drivers/iommu/tegra-smmu.c | 29 ++++++----------------------- 1 file changed, 6 insertions(+), 23 deletions(-)
drivers/iommu/tegra-smmu.c +6 -23
@@ -1086,8 +1086,8 @@ struct tegra_smmu *tegra_smmu_probe(struct device *dev,/* * This is a bit of a hack. Ideally we'd want to simply return this -* value. However the IOMMU registration process will attempt to add -* all devices to the IOMMU when bus_set_iommu() is called. In order +* value. However iommu_device_register() will attempt to add +* all devices to the IOMMU before we get that far. In order * not to rely on global variables to track the IOMMU instance, we * set it here so that it can be looked up from the .probe_device() * callback via the IOMMU device's .drvdata field.
@@ -1141,32 +1141,15 @@ struct tegra_smmu *tegra_smmu_probe(struct device *dev,return ERR_PTR (err); err =iommu_device_register (&smmu->iommu, &tegra_smmu_ops, dev); -if (err) - goto remove_sysfs; - - err =bus_set_iommu (&platform_bus_type, &tegra_smmu_ops); -if (err <0 ) - goto unregister; - -#ifdef CONFIG_PCI - err =bus_set_iommu (&pci_bus_type, &tegra_smmu_ops); -if (err <0 ) - goto unset_platform_bus; -#endif +if (err) { +iommu_device_sysfs_remove (&smmu->iommu); +return ERR_PTR (err); + }if (IS_ENABLED (CONFIG_DEBUG_FS ))tegra_smmu_debugfs_init (smmu);return smmu; - -unset_platform_bus:__maybe_unused ; -bus_set_iommu (&platform_bus_type,NULL ); -unregister: -iommu_device_unregister (&smmu->iommu); -remove_sysfs: -iommu_device_sysfs_remove (&smmu->iommu); - -return ERR_PTR (err); }void tegra_smmu_remove (struct tegra_smmu *smmu)
Signature · 1 line
2.36.1.dirty
Robin Murphy
reviewerPATCH v3 14/15 iommu/virtio: Clean up bus_set_iommu()
In reply to Robin MurphyStop calling bus_set_iommu() since it's now unnecessary, and simplify the probe failure path accordingly.
---
v3: No change
drivers/iommu/virtio-iommu.c | 25 ------------------------- 1 file changed, 25 deletions(-)
drivers/iommu/virtio-iommu.c +0 -25
@@ -7,7 +7,6 @@#define pr_fmt (fmt) KBUILD_MODNAME ": " fmt -#include <linux/amba/bus.h> #include <linux/delay.h> #include <linux/dma-iommu.h> #include <linux/dma-map-ops.h>
@@ -17,7 +16,6 @@#include <linux/module.h> #include <linux/of_platform.h> #include <linux/pci.h> -#include <linux/platform_device.h> #include <linux/virtio.h> #include <linux/virtio_config.h> #include <linux/virtio_ids.h>
@@ -1146,26 +1144,6 @@ static int viommu_probe(struct virtio_device *vdev)iommu_device_register (&viommu->iommu, &viommu_ops, parent_dev); -#ifdef CONFIG_PCI -if (pci_bus_type.iommu_ops != &viommu_ops) { - ret =bus_set_iommu (&pci_bus_type, &viommu_ops); -if (ret) - goto err_unregister; - } -#endif -#ifdef CONFIG_ARM_AMBA -if (amba_bustype.iommu_ops != &viommu_ops) { - ret =bus_set_iommu (&amba_bustype, &viommu_ops); -if (ret) - goto err_unregister; - } -#endif -if (platform_bus_type.iommu_ops != &viommu_ops) { - ret =bus_set_iommu (&platform_bus_type, &viommu_ops); -if (ret) - goto err_unregister; - } - vdev->priv = viommu; dev_info(dev, "input address: %u bits\n",
@@ -1174,9 +1152,6 @@ static int viommu_probe(struct virtio_device *vdev)return 0 ; -err_unregister: -iommu_device_sysfs_remove (&viommu->iommu); -iommu_device_unregister (&viommu->iommu); err_free_vqs: vdev->config->del_vqs (vdev);
Signature · 1 line
2.36.1.dirty
Robin Murphy
reviewerPATCH v3 15/15 iommu: Clean up bus_set_iommu()
In reply to Robin MurphyClean up the remaining trivial bus_set_iommu() callsites along with the implementation. Now drivers only have to know and care about iommu_device instances, phew!
---
v3: Also catch Intel's cheeky open-coded assignment
drivers/iommu/arm/arm-smmu/qcom_iommu.c | 4 ---- drivers/iommu/fsl_pamu_domain.c | 4 ---- drivers/iommu/intel/iommu.c | 2 -- drivers/iommu/iommu.c | 24 ------------------------ drivers/iommu/msm_iommu.c | 2 -- drivers/iommu/rockchip-iommu.c | 2 -- drivers/iommu/s390-iommu.c | 6 ------ drivers/iommu/sprd-iommu.c | 5 ----- drivers/iommu/sun50i-iommu.c | 2 -- include/linux/iommu.h | 1 - 10 files changed, 52 deletions(-)
drivers/iommu/arm/arm-smmu/qcom_iommu.c +0 -4
@@ -845,8 +845,6 @@ static int qcom_iommu_device_probe(struct platform_device *pdev) goto err_pm_disable; } -bus_set_iommu (&platform_bus_type, &qcom_iommu_ops); -if (qcom_iommu->local_base) {pm_runtime_get_sync (dev);writel_relaxed (0xffffffff , qcom_iommu->local_base +SMMU_INTR_SEL_NS );
@@ -864,8 +862,6 @@ static int qcom_iommu_device_remove(struct platform_device *pdev) {struct qcom_iommu_dev *qcom_iommu =platform_get_drvdata (pdev); -bus_set_iommu (&platform_bus_type,NULL ); -pm_runtime_force_suspend (&pdev->dev);platform_set_drvdata (pdev,NULL );iommu_device_sysfs_remove (&qcom_iommu->iommu);
drivers/iommu/fsl_pamu_domain.c +0 -4
@@ -481,11 +481,7 @@ int __init pamu_domain_init(void)if (ret) {iommu_device_sysfs_remove (&pamu_iommu);pr_err ("Can't register iommu device\n" ); -return ret; } -bus_set_iommu (&platform_bus_type, &fsl_pamu_ops); -bus_set_iommu (&pci_bus_type, &fsl_pamu_ops); -return ret; }
drivers/iommu/intel/iommu.c +0 -2
@@ -4031,7 +4031,6 @@ static int __init probe_acpi_namespace_devices(void)continue ; } - pn->dev->bus->iommu_ops = &intel_iommu_ops; ret =iommu_probe_device (pn->dev);if (ret)break ;
@@ -4153,7 +4152,6 @@ int __init intel_iommu_init(void) }up_read (&dmar_global_lock); -bus_set_iommu (&pci_bus_type, &intel_iommu_ops);if (si_domain && !hw_pass_through)register_memory_notifier (&intel_iommu_memory_nb);
drivers/iommu/iommu.c +0 -24
@@ -1834,30 +1834,6 @@ static int iommu_bus_init(struct bus_type *bus)return err; } -/** -* bus_set_iommu - set iommu-callbacks for the bus -* @bus: bus. -* @ops: the callbacks provided by the iommu-driver -* -* This function is called by an iommu driver to set the iommu methods -* used for a particular bus. Drivers for devices on that bus can use -* the iommu-api after these ops are registered. -* This special function is needed because IOMMUs are usually devices on -* the bus itself, so the iommu drivers are not initialized when the bus -* is set up. With this function the iommu-driver can set the iommu-ops -* afterwards. -*/ -int bus_set_iommu (struct bus_type *bus,const struct iommu_ops *ops) -{ -if (bus->iommu_ops && ops && bus->iommu_ops != ops) -return -EBUSY ; - - bus->iommu_ops = ops; - -return 0 ; -} -EXPORT_SYMBOL_GPL (bus_set_iommu); -bool iommu_present (struct bus_type *bus) {return bus->iommu_ops !=NULL ;
drivers/iommu/msm_iommu.c +0 -2
@@ -797,8 +797,6 @@ static int msm_iommu_probe(struct platform_device *pdev) goto fail; } -bus_set_iommu (&platform_bus_type, &msm_iommu_ops); -pr_info ("device mapped at %p, irq %d with %d ctx banks\n" , iommu->base, iommu->irq, iommu->ncb);
drivers/iommu/rockchip-iommu.c +0 -2
@@ -1300,8 +1300,6 @@ static int rk_iommu_probe(struct platform_device *pdev)if (!dma_dev) dma_dev = &pdev->dev; -bus_set_iommu (&platform_bus_type, &rk_iommu_ops); -pm_runtime_enable (dev);for (i =0 ; i < iommu->num_irq; i++) {
drivers/iommu/s390-iommu.c +0 -6
@@ -385,9 +385,3 @@ static const struct iommu_ops s390_iommu_ops = { .free = s390_domain_free, } }; - -static int __init s390_iommu_init (void ) -{ -return bus_set_iommu (&pci_bus_type, &s390_iommu_ops); -} -subsys_initcall (s390_iommu_init);
drivers/iommu/sprd-iommu.c +0 -5
@@ -507,9 +507,6 @@ static int sprd_iommu_probe(struct platform_device *pdev)if (ret) goto remove_sysfs; -if (!iommu_present (&platform_bus_type)) -bus_set_iommu (&platform_bus_type, &sprd_iommu_ops); - ret =sprd_iommu_clk_enable (sdev);if (ret) goto unregister_iommu;
@@ -545,8 +542,6 @@ static int sprd_iommu_remove(struct platform_device *pdev)iommu_group_put (sdev->group); sdev->group =NULL ; -bus_set_iommu (&platform_bus_type,NULL ); -platform_set_drvdata (pdev,NULL );iommu_device_sysfs_remove (&sdev->iommu);iommu_device_unregister (&sdev->iommu);
drivers/iommu/sun50i-iommu.c +0 -2
@@ -968,8 +968,6 @@ static int sun50i_iommu_probe(struct platform_device *pdev)if (ret <0 ) goto err_unregister; -bus_set_iommu (&platform_bus_type, &sun50i_iommu_ops); -return 0 ; err_unregister:
include/linux/iommu.h +0 -1
@@ -412,7 +412,6 @@ static inline const struct iommu_ops *dev_iommu_ops(struct device *dev)return dev->iommu->iommu_dev->ops; } -extern int bus_set_iommu (struct bus_type *bus,const struct iommu_ops *ops);extern int bus_iommu_probe (struct bus_type *bus);extern bool iommu_present (struct bus_type *bus);extern bool device_iommu_capable (struct device *dev,enum iommu_cap cap);
Signature · 1 line
2.36.1.dirty
RE: [PATCH v3 15/15] iommu: Clean up bus_set_iommu()
Quoted 8 lines
From: Robin Murphy <[email protected]>
Sent: Wednesday, July 6, 2022 1:09 AMClean up the remaining trivial
bus_set_iommu()callsites along with the implementation. Now drivers only have to know and care aboutiommu_deviceinstances, phew!Signed-off-by Robin Murphy <[email protected]>
Matthew Rosato
maintainerRe: [PATCH v3 15/15] iommu: Clean up bus_set_iommu()
In reply to Robin MurphyQuoted 9 lines · Robin Murphy
Clean up the remaining trivial
bus_set_iommu()callsites along with the implementation. Now drivers only have to know and care aboutiommu_deviceinstances, phew!Signed-off-by Robin Murphy <[email protected]>---
v3: Also catch Intel's cheeky open-coded assignment
...
Quoted 5 lines
drivers/iommu/s390-iommu.c +0
-0
drivers/iommu/s390-iommu.c static const struct iommu_ops s390_iommu_ops = { · line 393
Quoted 9 lines
drivers/iommu/s390-iommu.c +0
-6@@ -385,9 +385,3 @@ static const struct iommu_ops s390_iommu_ops = { .free = s390_domain_free, } }; - -static int __init s390_iommu_init (void ) -{ -return bus_set_iommu (&pci_bus_type, &s390_iommu_ops); -} -subsys_initcall (s390_iommu_init);
Previously s390_iommu_ops was only being set for pci_bus_type, but with this series it will now also be set for platform_bus_type.
To tolerate that, this series needs a change along the lines of:
From: Matthew Rosato <[email protected]>
Date: Thu, 7 Jul 2022 08:45:44 -0400
Subject: [PATCH] iommu/s390: fail probe for non-pci device
s390-iommu only supports pci_bus_type today
---
drivers/iommu/s390-iommu.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
drivers/iommu/s390-iommu.c +0 -0
index dd957145fb81..762f892b4ec3 100644
--- a/drivers/iommu/s390-iommu.c
+++ b/drivers/iommu/s390-iommu.c
@@ -185,7 +185,12 @@ static void s390_iommu_detach_device(struct iommu_domain *domain,
static struct iommu_device *s390_iommu_probe_device(struct device
*dev)
{
- struct zpci_dev *zdev = to_zpci_dev(dev);
+ struct zpci_dev *zdev;
+
+ if (!dev_is_pci(dev))
+ return ERR_PTR(-ENODEV);
+
+ zdev = to_zpci_dev(dev);
return &zdev->iommu_dev;
} return &zdev->iommu_dev;
}
Matthew Rosato
maintainerRe: [PATCH v3 15/15] iommu: Clean up bus_set_iommu()
Quoted 30 lines · Matthew Rosato
Quoted 9 lines · Robin Murphy
Clean up the remaining trivial
bus_set_iommu()callsites along with the implementation. Now drivers only have to know and care aboutiommu_deviceinstances, phew!Signed-off-by Robin Murphy <[email protected]>---
v3: Also catch Intel's cheeky open-coded assignment
...
Quoted 5 lines
drivers/iommu/s390-iommu.c +0
-0drivers/iommu/s390-iommu.c static const struct iommu_ops s390_iommu_ops = { · line 393
Quoted 9 lines
drivers/iommu/s390-iommu.c +0
-6@@ -385,9 +385,3 @@ static const struct iommu_ops s390_iommu_ops = { .free = s390_domain_free, } }; - -static int __init s390_iommu_init (void ) -{ - return bus_set_iommu(&pci_bus_type, &s390_iommu_ops); -} -subsys_initcall (s390_iommu_init);Previously
s390_iommu_opswas only being set forpci_bus_type, but with this series it will now also be set forplatform_bus_type.
Quoted 2 lines
To tolerate that, this series needs a change along the lines of:
... Sorry, let's try that again without a mangled diff:
From: Matthew Rosato <[email protected]>
Date: Thu, 7 Jul 2022 08:45:44 -0400
Subject: [PATCH] iommu/s390: fail probe for non-pci device
s390-iommu only supports pci_bus_type today
---
drivers/iommu/s390-iommu.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
drivers/iommu/s390-iommu.c +0 -0
index dd957145fb81..762f892b4ec3 100644
--- a/drivers/iommu/s390-iommu.c
+++ b/drivers/iommu/s390-iommu.c
@@ -185,7 +185,12 @@ static void s390_iommu_detach_device(struct iommu_domain *domain,
static struct iommu_device *s390_iommu_probe_device(struct device
*dev)
{
- struct zpci_dev *zdev = to_zpci_dev(dev);
+ struct zpci_dev *zdev;
+
+ if (!dev_is_pci(dev))
+ return ERR_PTR(-ENODEV);
+
+ zdev = to_zpci_dev(dev);
return &zdev->iommu_dev;
}
Robin Murphy
reviewerRe: [PATCH v3 15/15] iommu: Clean up bus_set_iommu()
Quoted 14 lines · Matthew Rosato
Quoted 13 lines · Matthew Rosato
Quoted 9 lines · Robin Murphy
Clean up the remaining trivial
bus_set_iommu()callsites along with the implementation. Now drivers only have to know and care aboutiommu_deviceinstances, phew!Signed-off-by Robin Murphy <[email protected]>---
v3: Also catch Intel's cheeky open-coded assignment
...
Quoted 16 lines
Quoted 8 lines
Quoted 8 lines
drivers/iommu/s390-iommu.c +0
-0@@ -385,9 +385,3 @@ static const struct iommu_ops s390_iommu_ops = { .free = s390_domain_free, } };Quoted 8 lines
drivers/iommu/s390-iommu.c static const struct iommu_ops s390_iommu_ops = { · line 393
Quoted 6 lines
drivers/iommu/s390-iommu.c +0
-6- -static int __init s390_iommu_init (void ) -{ - return bus_set_iommu(&pci_bus_type, &s390_iommu_ops); -} -subsys_initcall (s390_iommu_init);Previously
s390_iommu_opswas only being set forpci_bus_type, but with this series it will now also be set forplatform_bus_type.
Ah, indeed I hadn't got as far as fully appreciating that to_zpci_dev() isn't robust enough on its own. Thanks for the patch, I've pulled it in and will include it in v4. Do I take it that all else works OK with this fixed?
Cheers,
Robin.
Quoted 42 lines
Quoted 3 lines
To tolerate that, this series needs a change along the lines of:
... Sorry, let's try that again without a mangled diff:
From: Matthew Rosato <[email protected]>
Date: Thu, 7 Jul 2022 08:45:44 -0400
Subject: [PATCH] iommu/s390: fail probe for non-pci devices390-iommu only supports
pci_bus_typetodaySigned-off-by Matthew Rosato <[email protected]>--- drivers/iommu/s390-iommu.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)drivers/iommu/s390-iommu.c +6
-1@@ -185,7 +185,12 @@ static void s390_iommu_detach_device(struct iommu_domain *domain, static struct iommu_device *s390_iommu_probe_device(struct device *dev) { - struct zpci_dev *zdev = to_zpci_dev(dev); + struct zpci_dev *zdev; + + if (!dev_is_pci(dev)) + return ERR_PTR(-ENODEV); + + zdev = to_zpci_dev(dev);return
&zdev->iommu_dev;
}
Re: [PATCH v3 15/15] iommu: Clean up bus_set_iommu()
Quoted 32 lines · Robin Murphy
Quoted 14 lines · Matthew Rosato
Quoted 13 lines · Matthew Rosato
Quoted 9 lines · Robin Murphy
Clean up the remaining trivial
bus_set_iommu()callsites along with the implementation. Now drivers only have to know and care aboutiommu_deviceinstances, phew!Signed-off-by Robin Murphy <[email protected]>---
v3: Also catch Intel's cheeky open-coded assignment
...
Quoted 16 lines
Quoted 8 lines
Quoted 8 lines
drivers/iommu/s390-iommu.c +0
-0@@ -385,9 +385,3 @@ static const struct iommu_ops s390_iommu_ops = { .free = s390_domain_free, } };Quoted 8 lines
drivers/iommu/s390-iommu.c static const struct iommu_ops s390_iommu_ops = { · line 393
Quoted 6 lines
drivers/iommu/s390-iommu.c +0
-6- -static int __init s390_iommu_init (void ) -{ - return bus_set_iommu(&pci_bus_type, &s390_iommu_ops); -} -subsys_initcall (s390_iommu_init);Previously
s390_iommu_opswas only being set forpci_bus_type, but with this series it will now also be set forplatform_bus_type.
Quoted 2 lines
Ah, indeed I hadn't got as far as fully appreciating that
to_zpci_dev()isn't robust enough on its own. Thanks for the patch, I've pulled it in and will include it in v4. Do I take it that all else works OK with this fixed?
Yes, with that patch included this looks OK so for s390 you can also take a
Niklas Schnelle
maintainerPATCH 1/1 iommu/s390: fail probe for non-pci device
In reply to Matthew RosatoFrom: Matthew Rosato <[email protected]>
s390-iommu only supports pci_bus_type today
--- drivers/iommu/s390-iommu.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
drivers/iommu/s390-iommu.c +6 -1
@@ -185,7 +185,12 @@ static void s390_iommu_detach_device(struct iommu_domain *domain,static struct iommu_device *s390_iommu_probe_device (struct device *dev) { -struct zpci_dev *zdev =to_zpci_dev (dev); +struct zpci_dev *zdev; + +if (!dev_is_pci (dev)) +return ERR_PTR (-ENODEV ); + + zdev =to_zpci_dev (dev);return &zdev->iommu_dev; }
Signature · 1 line
2.34.1
Quoted 10 lines · Robin Murphy
v2: https://lore.kernel.org/linux-iommu/[email protected]/
Hi all,
Here's v3, now with working x86! Having finally made sense of how I broke Intel, I've given AMD the same fix by inspection. I'm still not 100% sure about s390, but it looks like it should probably be OK since it seems to register an IOMMU instance for each PCI device (?!) before disappearing into PCI hotplug code, wherein I assume we should never see a PCI device appear without its IOMMU already registered.
Yes, this is a bit unusual as our PCI architecture doesn't really have a notion of an IOMMU device only of I/O translation tables. These are then registered per PCI function. PCI functions may share I/O translation tables and thus DMA address spaces but this is not done at the moment. As Matt already mentioned we do need a small change for this patch series. Since that was still mangled in his mail for me I just replied with that using "git send-email". With Matt's patch applied I can confirm that this works fine for us and does look like a useful simplification. So feel free to add my
Robin Murphy
reviewerQuoted 11 lines · Robin Murphy
v2: https://lore.kernel.org/linux-iommu/[email protected]/
Hi all,
Here's v3, now with working x86! Having finally made sense of how I broke Intel, I've given AMD the same fix by inspection. I'm still not 100% sure about s390, but it looks like it should probably be OK since it seems to register an IOMMU instance for each PCI device (?!) before disappearing into PCI hotplug code, wherein I assume we should never see a PCI device appear without its IOMMU already registered.
Quoted 4 lines
Otherwise, the only other updates are hooking up the new host1x context bus (noting that it now takes all of 4 lines to support a whole new bus, yay!), and a slight tweak to make sure we keep rejecting registration of conflicting
iommu_opsrather than needlessly change that just yet.
FWIW I've prepared v4, including Matt's s390 patch and some nice extra cleanups thanks to Kevin's suggestions, but have now decided to wait to rebase and send it after the upcoming merge window. If anyone's interested in the meantime, there's a preliminary branch here:
https://gitlab.arm.com/linux-arm/linux-rm/-/commits/bus-set-iommu-v4
(temporarily including the host1x patch from -next to avoid breakage on arm64 as well)
Thanks,
Robin.
Quoted 26 lines
From: Robin Murphy <[email protected]>
Sent: Friday, July 15, 2022 9:13 PMQuoted 1 line · Robin Murphy
iommu/[email protected]/
Quoted 10 lines
Hi all,
Here's v3, now with working x86! Having finally made sense of how I broke Intel, I've given AMD the same fix by inspection. I'm still not 100% sure about s390, but it looks like it should probably be OK since it seems to register an IOMMU instance for each PCI device (?!) before disappearing into PCI hotplug code, wherein I assume we should never see a PCI device appear without its IOMMU already registered.
Quoted 4 lines
Otherwise, the only other updates are hooking up the new host1x context bus (noting that it now takes all of 4 lines to support a whole new bus, yay!), and a slight tweak to make sure we keep rejecting registration of conflicting
iommu_opsrather than needlessly change that just yet.FWIW I've prepared v4, including Matt's s390 patch and some nice extra cleanups thanks to Kevin's suggestions, but have now decided to wait to rebase and send it after the upcoming merge window. If anyone's interested in the meantime, there's a preliminary branch here:
Quoted 5 lines
https://gitlab.arm.com/linux-arm/linux-rm/-/commits/bus-set-iommu-v4
(temporarily including the host1x patch from -next to avoid breakage on arm64 as well)
This looks good to me.