diff mbox series

[v4,05/16] iommu: Move bus setup to IOMMU device registration

Message ID d342b6f27efb5ef3e93aacaa3012d25386d74866.1660572783.git.robin.murphy@arm.com (mailing list archive)
State New, archived
Headers show
Series iommu: retire bus_set_iommu() | expand

Commit Message

Robin Murphy Aug. 15, 2022, 4:20 p.m. UTC
Move 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.

Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
Reviewed-By: Krishna Reddy <vdumpa@nvidia.com>
Reviewed-by: Kevin Tian <kevin.tian@intel.com>
Tested-by: Matthew Rosato <mjrosato@linux.ibm.com> # s390
Tested-by: Niklas Schnelle <schnelle@linux.ibm.com> # s390
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---

v4: Factor out the ops check in iommu_device_register() to keep the loop
even simpler, and comment the nominal change in behaviour

 drivers/iommu/iommu.c | 55 +++++++++++++++++++++++--------------------
 1 file changed, 30 insertions(+), 25 deletions(-)

Comments

Jon Hunter Oct. 6, 2022, 2:01 p.m. UTC | #1
Hi Robin,

On 15/08/2022 17:20, Robin Murphy wrote:
> Move 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.


Since this change, I have noticed that the DRM driver on Tegra20 is 
failing to probe and I am seeing ...

  tegra-gr2d 54140000.gr2d: failed to attach to domain: -19
  drm drm: failed to initialize 54140000.gr2d: -19

Bisect points to this change and reverting it fixes it. Let me know if 
you have any thoughts.

Cheers
Jon
Robin Murphy Oct. 6, 2022, 3:27 p.m. UTC | #2
On 2022-10-06 15:01, Jon Hunter wrote:
> Hi Robin,
> 
> On 15/08/2022 17:20, Robin Murphy wrote:
>> Move 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.
> 
> 
> Since this change, I have noticed that the DRM driver on Tegra20 is 
> failing to probe and I am seeing ...
> 
>   tegra-gr2d 54140000.gr2d: failed to attach to domain: -19
>   drm drm: failed to initialize 54140000.gr2d: -19
> 
> Bisect points to this change and reverting it fixes it. Let me know if 
> you have any thoughts.

Oh, apparently what's happened is that I've inadvertently enabled the 
tegra-gart driver, since it seems that *wasn't* calling bus_set_iommu() 
before. Looking at the history, it appears to have been that way since 
c7e3ca515e78 ("iommu/tegra: gart: Do not register with bus"), so 
essentially that driver has been broken and useless for close to 8 years 
now :(

Given that, I'd be inclined to "fix" it as below, or just give up and 
delete the whole thing.

Thanks,
Robin.

----->8-----
diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index 5c5cb5bee8b6..7b3f7fd6e527 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig
@@ -230,6 +230,7 @@ config TEGRA_IOMMU_GART
  	bool "Tegra GART IOMMU Support"
  	depends on ARCH_TEGRA_2x_SOC
  	depends on TEGRA_MC
+	depends on BROKEN
  	select IOMMU_API
  	help
  	  Enables support for remapping discontiguous physical memory
Thierry Reding Oct. 6, 2022, 5:12 p.m. UTC | #3
On Thu, Oct 06, 2022 at 04:27:39PM +0100, Robin Murphy wrote:
> On 2022-10-06 15:01, Jon Hunter wrote:
> > Hi Robin,
> > 
> > On 15/08/2022 17:20, Robin Murphy wrote:
> > > Move 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.
> > 
> > 
> > Since this change, I have noticed that the DRM driver on Tegra20 is
> > failing to probe and I am seeing ...
> > 
> >   tegra-gr2d 54140000.gr2d: failed to attach to domain: -19
> >   drm drm: failed to initialize 54140000.gr2d: -19
> > 
> > Bisect points to this change and reverting it fixes it. Let me know if
> > you have any thoughts.
> 
> Oh, apparently what's happened is that I've inadvertently enabled the
> tegra-gart driver, since it seems that *wasn't* calling bus_set_iommu()
> before. Looking at the history, it appears to have been that way since
> c7e3ca515e78 ("iommu/tegra: gart: Do not register with bus"), so essentially
> that driver has been broken and useless for close to 8 years now :(
> 
> Given that, I'd be inclined to "fix" it as below, or just give up and delete
> the whole thing.

I'm inclined to agree. GART is severely limited: it provides a single
IOMMU domain with an aperture of 32 MiB. It's close to useless for
anything we would want to do and my understanding is that people have
been falling back to CMA for any graphics/display stuff that the GART
would've been useful for.

Given that nobody's felt the urge to fix this for the past 8 years, I
don't think there's enough interest in this to keep it going.

Dmitry, any thoughts?

Thierry

> 
> Thanks,
> Robin.
> 
> ----->8-----
> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> index 5c5cb5bee8b6..7b3f7fd6e527 100644
> --- a/drivers/iommu/Kconfig
> +++ b/drivers/iommu/Kconfig
> @@ -230,6 +230,7 @@ config TEGRA_IOMMU_GART
>  	bool "Tegra GART IOMMU Support"
>  	depends on ARCH_TEGRA_2x_SOC
>  	depends on TEGRA_MC
> +	depends on BROKEN
>  	select IOMMU_API
>  	help
>  	  Enables support for remapping discontiguous physical memory
Dmitry Osipenko Oct. 6, 2022, 6:43 p.m. UTC | #4
On 10/6/22 20:12, Thierry Reding wrote:
> On Thu, Oct 06, 2022 at 04:27:39PM +0100, Robin Murphy wrote:
>> On 2022-10-06 15:01, Jon Hunter wrote:
>>> Hi Robin,
>>>
>>> On 15/08/2022 17:20, Robin Murphy wrote:
>>>> Move 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.
>>>
>>>
>>> Since this change, I have noticed that the DRM driver on Tegra20 is
>>> failing to probe and I am seeing ...
>>>
>>>   tegra-gr2d 54140000.gr2d: failed to attach to domain: -19
>>>   drm drm: failed to initialize 54140000.gr2d: -19

The upstream Tegra20 device-tree doesn't have IOMMU phandle for
54140000.gr2d. In this case IOMMU domain shouldn't be available for the
DRM driver [1]. Sounds like IOMMU core has a bug.

[1]
https://elixir.bootlin.com/linux/latest/source/drivers/iommu/tegra-gart.c#L243

>>> Bisect points to this change and reverting it fixes it. Let me know if
>>> you have any thoughts.
>>
>> Oh, apparently what's happened is that I've inadvertently enabled the
>> tegra-gart driver, since it seems that *wasn't* calling bus_set_iommu()
>> before. Looking at the history, it appears to have been that way since
>> c7e3ca515e78 ("iommu/tegra: gart: Do not register with bus"), so essentially
>> that driver has been broken and useless for close to 8 years now :(
>>
>> Given that, I'd be inclined to "fix" it as below, or just give up and delete
>> the whole thing.
> 
> I'm inclined to agree. GART is severely limited: it provides a single
> IOMMU domain with an aperture of 32 MiB. It's close to useless for
> anything we would want to do and my understanding is that people have
> been falling back to CMA for any graphics/display stuff that the GART
> would've been useful for.
> 
> Given that nobody's felt the urge to fix this for the past 8 years, I
> don't think there's enough interest in this to keep it going.
> 
> Dmitry, any thoughts?

This GART driver is used by a community kernel fork that has alternative
DRM driver supporting IOMMU/GART on Tegra20. The fork is periodically
synced with the latest upstream, it's used by postmarketOS. Hence it
wasn't a completely dead driver.

The 32M aperture works well for 2d/3d engines because it fits multiple
textures at once. Tegra DRM driver needs to remap buffers dynamically,
but this is easy to implement because DRM core has nice helpers for
that. We haven't got to the point where upstream DRM driver is ready to
support this feature.

CMA is hard to use for anything other than display framebuffers. It's
slow and fails to allocate memory if CMA area is "shared" due to
fragmentation and pinned pages. Reserved CMA isn't an option for GPU
because then there is no memory for the rest of system.

I don't see any problems with removing GART driver. It's not going to be
used soon in upstream and only adds maintenance burden. We can always
re-add it in the future.
Alex Williamson Oct. 12, 2022, 4:28 p.m. UTC | #5
On Mon, 15 Aug 2022 17:20:06 +0100
Robin Murphy <robin.murphy@arm.com> wrote:

> Move 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.
> 
> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Reviewed-By: Krishna Reddy <vdumpa@nvidia.com>
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> Tested-by: Matthew Rosato <mjrosato@linux.ibm.com> # s390
> Tested-by: Niklas Schnelle <schnelle@linux.ibm.com> # s390
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
> 
> v4: Factor out the ops check in iommu_device_register() to keep the loop
> even simpler, and comment the nominal change in behaviour
> 
>  drivers/iommu/iommu.c | 55 +++++++++++++++++++++++--------------------
>  1 file changed, 30 insertions(+), 25 deletions(-)

This introduces the below lockdep spat regression, bisected to commit:

57365a04c921 ("iommu: Move bus setup to IOMMU device registration")

This can be reproduced with simple vfio-pci device assignment to a VM
on x86_64 with VT-d.  Thanks,

Alex

======================================================
WARNING: possible circular locking dependency detected
6.0.0-rc4+ #127 Tainted: G            E     
------------------------------------------------------
qemu-system-x86/1726 is trying to acquire lock:
ffffffffacf8a7d0 (dmar_global_lock){++++}-{3:3}, at: intel_iommu_get_resv_regions+0x21/0x2a0

but task is already holding lock:
ffff981240efb0c0 (&group->mutex){+.+.}-{3:3}, at: iommu_get_group_resv_regions+0x2c/0x3b0

which lock already depends on the new lock.


the existing dependency chain (in reverse order) is:

-> #1 (&group->mutex){+.+.}-{3:3}:
       __mutex_lock+0x6d/0x8c0
       iommu_group_add_device+0xfb/0x330
       __iommu_probe_device+0x150/0x270
       probe_iommu_group+0x31/0x50
       bus_for_each_dev+0x67/0xa0
       bus_iommu_probe+0x38/0x2a0
       iommu_device_register+0xc1/0x130
       intel_iommu_init+0xfd9/0x120d
       pci_iommu_init+0xe/0x36
       do_one_initcall+0x5b/0x310
       kernel_init_freeable+0x275/0x2c1
       kernel_init+0x16/0x140
       ret_from_fork+0x22/0x30

-> #0 (dmar_global_lock){++++}-{3:3}:
       __lock_acquire+0x10dc/0x1da0
       lock_acquire+0xc2/0x2d0
       down_read+0x2d/0x40
       intel_iommu_get_resv_regions+0x21/0x2a0
       iommu_get_group_resv_regions+0x88/0x3b0
       vfio_iommu_type1_attach_group+0x19d/0xce1 [vfio_iommu_type1]
       vfio_fops_unl_ioctl+0x19d/0x270 [vfio]
       __x64_sys_ioctl+0x8b/0xc0
       do_syscall_64+0x3b/0x90
       entry_SYSCALL_64_after_hwframe+0x63/0xcd

other info that might help us debug this:

 Possible unsafe locking scenario:

       CPU0                    CPU1
       ----                    ----
  lock(&group->mutex);
                               lock(dmar_global_lock);
                               lock(&group->mutex);
  lock(dmar_global_lock);

 *** DEADLOCK ***

4 locks held by qemu-system-x86/1726:
 #0: ffff9811b5546c88 (&container->group_lock){++++}-{3:3}, at: vfio_fops_unl_ioctl+0xbb/0x270 [vfio]
 #1: ffffffffc058c720 (&vfio.iommu_drivers_lock){+.+.}-{3:3}, at: vfio_fops_unl_ioctl+0xeb/0x270 [vfio]
 #2: ffff9811d865ba88 (&iommu->lock#2){+.+.}-{3:3}, at: vfio_iommu_type1_attach_group+0x51/0xce1 [vfio_iommu_type1]
 #3: ffff981240efb0c0 (&group->mutex){+.+.}-{3:3}, at: iommu_get_group_resv_regions+0x2c/0x3b0

stack backtrace:
CPU: 0 PID: 1726 Comm: qemu-system-x86 Tainted: G            E      6.0.0-rc4+ #127
Hardware name: System manufacturer System Product Name/P8H67-M PRO, BIOS 3904 04/27/2013
Call Trace:
 <TASK>
 dump_stack_lvl+0x56/0x73
 check_noncircular+0xd6/0x100
 ? __lock_acquire+0x374/0x1da0
 __lock_acquire+0x10dc/0x1da0
 lock_acquire+0xc2/0x2d0
 ? intel_iommu_get_resv_regions+0x21/0x2a0
 ? trace_contention_end+0x2d/0xd0
 ? __mutex_lock+0xdf/0x8c0
 ? iommu_get_group_resv_regions+0x2c/0x3b0
 ? lock_is_held_type+0xe2/0x140
 down_read+0x2d/0x40
 ? intel_iommu_get_resv_regions+0x21/0x2a0
 intel_iommu_get_resv_regions+0x21/0x2a0
 iommu_get_group_resv_regions+0x88/0x3b0
 ? iommu_attach_group+0x76/0xa0
 vfio_iommu_type1_attach_group+0x19d/0xce1 [vfio_iommu_type1]
 ? rcu_read_lock_sched_held+0x43/0x70
 ? __module_address.part.0+0x2b/0xa0
 ? is_module_address+0x43/0x70
 ? __init_waitqueue_head+0x4a/0x60
 vfio_fops_unl_ioctl+0x19d/0x270 [vfio]
 __x64_sys_ioctl+0x8b/0xc0
 do_syscall_64+0x3b/0x90
 entry_SYSCALL_64_after_hwframe+0x63/0xcd
RIP: 0033:0x7f803853a17b
Code: 0f 1e fa 48 8b 05 1d ad 0c 00 64 c7 00 26 00 00 00 48 c7 c0 ff ff ff ff c3 66 0f 1f 44 00 00 f3 0f 1e fa b8 10 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d ed ac 0c 00 f7 d8 64 89 01 48
RSP: 002b:00007ffd8128c2e8 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
RAX: ffffffffffffffda RBX: 0000000000000020 RCX: 00007f803853a17b
RDX: 0000000000000003 RSI: 0000000000003b66 RDI: 000000000000001c
RBP: 00007ffd8128c320 R08: 000055f59d8ff8d0 R09: 00007f8038605a40
R10: 0000000000000008 R11: 0000000000000246 R12: 000055f599aed1d0
R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
 </TASK>
Baolu Lu Oct. 13, 2022, 1:08 a.m. UTC | #6
Hi Alex,

On 2022/10/13 0:28, Alex Williamson wrote:
> On Mon, 15 Aug 2022 17:20:06 +0100
> Robin Murphy<robin.murphy@arm.com>  wrote:
> 
>> Move 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.
>>
>> Tested-by: Marek Szyprowski<m.szyprowski@samsung.com>
>> Reviewed-By: Krishna Reddy<vdumpa@nvidia.com>
>> Reviewed-by: Kevin Tian<kevin.tian@intel.com>
>> Tested-by: Matthew Rosato<mjrosato@linux.ibm.com>  # s390
>> Tested-by: Niklas Schnelle<schnelle@linux.ibm.com>  # s390
>> Signed-off-by: Robin Murphy<robin.murphy@arm.com>
>> ---
>>
>> v4: Factor out the ops check in iommu_device_register() to keep the loop
>> even simpler, and comment the nominal change in behaviour
>>
>>   drivers/iommu/iommu.c | 55 +++++++++++++++++++++++--------------------
>>   1 file changed, 30 insertions(+), 25 deletions(-)
> This introduces the below lockdep spat regression, bisected to commit:
> 
> 57365a04c921 ("iommu: Move bus setup to IOMMU device registration")
> 
> This can be reproduced with simple vfio-pci device assignment to a VM
> on x86_64 with VT-d.  Thanks,

Thank you for reporting this. I have proposed below fix:

https://lore.kernel.org/all/20220927053109.4053662-1-baolu.lu@linux.intel.com/

Does it work for you?

Best regards,
baolu
Jon Hunter Oct. 18, 2022, 6:13 a.m. UTC | #7
On 06/10/2022 16:27, Robin Murphy wrote:
> On 2022-10-06 15:01, Jon Hunter wrote:
>> Hi Robin,
>>
>> On 15/08/2022 17:20, Robin Murphy wrote:
>>> Move 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.
>>
>>
>> Since this change, I have noticed that the DRM driver on Tegra20 is 
>> failing to probe and I am seeing ...
>>
>>   tegra-gr2d 54140000.gr2d: failed to attach to domain: -19
>>   drm drm: failed to initialize 54140000.gr2d: -19
>>
>> Bisect points to this change and reverting it fixes it. Let me know if 
>> you have any thoughts.
> 
> Oh, apparently what's happened is that I've inadvertently enabled the 
> tegra-gart driver, since it seems that *wasn't* calling bus_set_iommu() 
> before. Looking at the history, it appears to have been that way since 
> c7e3ca515e78 ("iommu/tegra: gart: Do not register with bus"), so 
> essentially that driver has been broken and useless for close to 8 years 
> now :(
> 
> Given that, I'd be inclined to "fix" it as below, or just give up and 
> delete the whole thing.
> 
> Thanks,
> Robin.
> 
> ----->8-----
> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> index 5c5cb5bee8b6..7b3f7fd6e527 100644
> --- a/drivers/iommu/Kconfig
> +++ b/drivers/iommu/Kconfig
> @@ -230,6 +230,7 @@ config TEGRA_IOMMU_GART
>       bool "Tegra GART IOMMU Support"
>       depends on ARCH_TEGRA_2x_SOC
>       depends on TEGRA_MC
> +    depends on BROKEN
>       select IOMMU_API
>       help
>         Enables support for remapping discontiguous physical memory


Thanks Robin. This works for me.

Thierry, Dmitry, we need a fix for v6.1 and so OK with the above?

Jon
Dmitry Osipenko Oct. 18, 2022, 9:12 p.m. UTC | #8
18.10.2022 09:13, Jon Hunter пишет:
> 
> On 06/10/2022 16:27, Robin Murphy wrote:
>> On 2022-10-06 15:01, Jon Hunter wrote:
>>> Hi Robin,
>>>
>>> On 15/08/2022 17:20, Robin Murphy wrote:
>>>> Move 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.
>>>
>>>
>>> Since this change, I have noticed that the DRM driver on Tegra20 is
>>> failing to probe and I am seeing ...
>>>
>>>   tegra-gr2d 54140000.gr2d: failed to attach to domain: -19
>>>   drm drm: failed to initialize 54140000.gr2d: -19
>>>
>>> Bisect points to this change and reverting it fixes it. Let me know
>>> if you have any thoughts.
>>
>> Oh, apparently what's happened is that I've inadvertently enabled the
>> tegra-gart driver, since it seems that *wasn't* calling
>> bus_set_iommu() before. Looking at the history, it appears to have
>> been that way since c7e3ca515e78 ("iommu/tegra: gart: Do not register
>> with bus"), so essentially that driver has been broken and useless for
>> close to 8 years now :(
>>
>> Given that, I'd be inclined to "fix" it as below, or just give up and
>> delete the whole thing.
>>
>> Thanks,
>> Robin.
>>
>> ----->8-----
>> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
>> index 5c5cb5bee8b6..7b3f7fd6e527 100644
>> --- a/drivers/iommu/Kconfig
>> +++ b/drivers/iommu/Kconfig
>> @@ -230,6 +230,7 @@ config TEGRA_IOMMU_GART
>>       bool "Tegra GART IOMMU Support"
>>       depends on ARCH_TEGRA_2x_SOC
>>       depends on TEGRA_MC
>> +    depends on BROKEN
>>       select IOMMU_API
>>       help
>>         Enables support for remapping discontiguous physical memory
> 
> 
> Thanks Robin. This works for me.
> 
> Thierry, Dmitry, we need a fix for v6.1 and so OK with the above?

To me it is more a problem of the DRM driver that it doesn't support
GART. GART will require a special handling from the DRM driver anyways [1].

[1]
https://github.com/grate-driver/linux/blob/master/drivers/gpu/drm/grate/drm.c#L460

The GART driver itself isn't broken, it's working perfectly fine. It's
the DRM driver that should start caring about the GART presence, IMO.
diff mbox series

Patch

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index a8d14f2a1035..4db0874a5ed6 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -188,6 +188,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
@@ -199,9 +207,18 @@  subsys_initcall(iommu_subsys_init);
 int iommu_device_register(struct iommu_device *iommu,
 			  const struct iommu_ops *ops, struct device *hwdev)
 {
+	int err = 0;
+
 	/* We need to be able to take module references appropriately */
 	if (WARN_ON(is_module_address((unsigned long)ops) && !ops->owner))
 		return -EINVAL;
+	/*
+	 * Temporarily enforce global restriction to a single driver. This was
+	 * already the de-facto behaviour, since any possible combination of
+	 * existing drivers would compete for at least the PCI or platform bus.
+	 */
+	if (iommu_buses[0]->iommu_ops && iommu_buses[0]->iommu_ops != ops)
+		return -EBUSY;
 
 	iommu->ops = ops;
 	if (hwdev)
@@ -210,12 +227,22 @@  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);
-	return 0;
+
+	for (int i = 0; i < ARRAY_SIZE(iommu_buses) && !err; i++) {
+		iommu_buses[i]->iommu_ops = ops;
+		err = bus_iommu_probe(iommu_buses[i]);
+	}
+	if (err)
+		iommu_device_unregister(iommu);
+	return err;
 }
 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);
@@ -1644,13 +1671,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)
 {
@@ -1822,27 +1842,12 @@  int bus_iommu_probe(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);