[PATCH] swiotlb: ensure io_tlb_default_mem spinlock always initialised

[PATCH] swiotlb: ensure io_tlb_default_mem spinlock always initialised

[Ben Dooks]

If the io_tlb_default_mem is used by a device that then gets used
by the swiotlb code, the spinlock warning is triggered causing a
lot of stack-trace.

Fix this by making the structure's lock initialised at build time.

Avoids the following BUG trigger:

[    3.046401] BUG: spinlock bad magic on CPU#3, kworker/u8:0/7
[    3.046689]  lock: io_tlb_default_mem+0x30/0x60, .magic: 00000000, .owner: <none>/-1, .owner_cpu: 0
[    3.047217] CPU: 3 PID: 7 Comm: kworker/u8:0 Not tainted 5.17.0-00056-g1e9bac738084-dirty #310
[    3.048363] Workqueue: events_unbound deferred_probe_work_func
[    3.048892] Call Trace:
[    3.049224] [<ffffffff800048aa>] dump_backtrace+0x1c/0x24
[    3.049576] [<ffffffff805c5f74>] show_stack+0x2c/0x38
[    3.049898] [<ffffffff805cade2>] dump_stack_lvl+0x40/0x58
[    3.050216] [<ffffffff805cae0e>] dump_stack+0x14/0x1c
[    3.050460] [<ffffffff805c69f6>] spin_dump+0x62/0x6e
[    3.050681] [<ffffffff8004e000>] do_raw_spin_lock+0xb0/0xd0
[    3.050934] [<ffffffff805d5b58>] _raw_spin_lock_irqsave+0x20/0x2c
[    3.051157] [<ffffffff80067e38>] swiotlb_tbl_map_single+0xce/0x3da
[    3.051372] [<ffffffff80068320>] swiotlb_map+0x3a/0x15c
[    3.051668] [<ffffffff80065a56>] dma_map_page_attrs+0x76/0x162
[    3.051975] [<ffffffff8031d542>] dw_pcie_host_init+0x326/0x3f2

Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
---
 kernel/dma/swiotlb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
index cb50f8d38360..a707a944c39a 100644
--- a/kernel/dma/swiotlb.c
+++ b/kernel/dma/swiotlb.c
@@ -65,7 +65,7 @@
 static bool swiotlb_force_bounce;
 static bool swiotlb_force_disable;
 
-struct io_tlb_mem io_tlb_default_mem;
+struct io_tlb_mem io_tlb_default_mem = { .lock = __SPIN_LOCK_UNLOCKED(io_tlb_default_mem.lock) } ;
 
 phys_addr_t swiotlb_unencrypted_base;
 
-- 
2.35.1

Re: [PATCH] swiotlb: ensure io_tlb_default_mem spinlock always initialised

[Robin Murphy]

On 2022-07-08 18:08, Ben Dooks wrote:
> If the io_tlb_default_mem is used by a device that then gets used
> by the swiotlb code, the spinlock warning is triggered causing a
> lot of stack-trace.

Hang on, how have we got as far as trying to allocate out of an 
uninitialised SWIOTLB at all? That seems like either something's gone 
more fundamentally wrong or we're missing a proper check somewhere. I 
don't think papering over it like this is right.

Thanks,
Robin.

> Fix this by making the structure's lock initialised at build time.
> 
> Avoids the following BUG trigger:
> 
> [    3.046401] BUG: spinlock bad magic on CPU#3, kworker/u8:0/7
> [    3.046689]  lock: io_tlb_default_mem+0x30/0x60, .magic: 00000000, .owner: <none>/-1, .owner_cpu: 0
> [    3.047217] CPU: 3 PID: 7 Comm: kworker/u8:0 Not tainted 5.17.0-00056-g1e9bac738084-dirty #310
> [    3.048363] Workqueue: events_unbound deferred_probe_work_func
> [    3.048892] Call Trace:
> [    3.049224] [<ffffffff800048aa>] dump_backtrace+0x1c/0x24
> [    3.049576] [<ffffffff805c5f74>] show_stack+0x2c/0x38
> [    3.049898] [<ffffffff805cade2>] dump_stack_lvl+0x40/0x58
> [    3.050216] [<ffffffff805cae0e>] dump_stack+0x14/0x1c
> [    3.050460] [<ffffffff805c69f6>] spin_dump+0x62/0x6e
> [    3.050681] [<ffffffff8004e000>] do_raw_spin_lock+0xb0/0xd0
> [    3.050934] [<ffffffff805d5b58>] _raw_spin_lock_irqsave+0x20/0x2c
> [    3.051157] [<ffffffff80067e38>] swiotlb_tbl_map_single+0xce/0x3da
> [    3.051372] [<ffffffff80068320>] swiotlb_map+0x3a/0x15c
> [    3.051668] [<ffffffff80065a56>] dma_map_page_attrs+0x76/0x162
> [    3.051975] [<ffffffff8031d542>] dw_pcie_host_init+0x326/0x3f2
> 
> Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
> ---
>   kernel/dma/swiotlb.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
> index cb50f8d38360..a707a944c39a 100644
> --- a/kernel/dma/swiotlb.c
> +++ b/kernel/dma/swiotlb.c
> @@ -65,7 +65,7 @@
>   static bool swiotlb_force_bounce;
>   static bool swiotlb_force_disable;
>   
> -struct io_tlb_mem io_tlb_default_mem;
> +struct io_tlb_mem io_tlb_default_mem = { .lock = __SPIN_LOCK_UNLOCKED(io_tlb_default_mem.lock) } ;
>   
>   phys_addr_t swiotlb_unencrypted_base;
>   

Re: [PATCH] swiotlb: ensure io_tlb_default_mem spinlock always initialised

[Ben Dooks]

On 08/07/2022 21:32, Robin Murphy wrote:
> On 2022-07-08 18:08, Ben Dooks wrote:
>> If the io_tlb_default_mem is used by a device that then gets used
>> by the swiotlb code, the spinlock warning is triggered causing a
>> lot of stack-trace.
> 
> Hang on, how have we got as far as trying to allocate out of an 
> uninitialised SWIOTLB at all? That seems like either something's gone 
> more fundamentally wrong or we're missing a proper check somewhere. I 
> don't think papering over it like this is right.
> 
> Thanks,
> Robin
We have a system where we have no DMA capable memory in the 32bit
window, which means even if we initialise swiotlb we don't have
any dma capable memory. The code says go use an IOMMU but we don't
have one of those either (and all peripherals are capable of DMA
from any of the memory, so there shouldn't be the need for one)

Is there any other way of fixing this DMA allocation issue?

I added this fix as swiotlb prints an error but also causes
an annoying stack backtrace.

> 
>> Fix this by making the structure's lock initialised at build time.
>>
>> Avoids the following BUG trigger:
>>
>> [    3.046401] BUG: spinlock bad magic on CPU#3, kworker/u8:0/7
>> [    3.046689]  lock: io_tlb_default_mem+0x30/0x60, .magic: 00000000, 
>> .owner: <none>/-1, .owner_cpu: 0
>> [    3.047217] CPU: 3 PID: 7 Comm: kworker/u8:0 Not tainted 
>> 5.17.0-00056-g1e9bac738084-dirty #310
>> [    3.048363] Workqueue: events_unbound deferred_probe_work_func
>> [    3.048892] Call Trace:
>> [    3.049224] [<ffffffff800048aa>] dump_backtrace+0x1c/0x24
>> [    3.049576] [<ffffffff805c5f74>] show_stack+0x2c/0x38
>> [    3.049898] [<ffffffff805cade2>] dump_stack_lvl+0x40/0x58
>> [    3.050216] [<ffffffff805cae0e>] dump_stack+0x14/0x1c
>> [    3.050460] [<ffffffff805c69f6>] spin_dump+0x62/0x6e
>> [    3.050681] [<ffffffff8004e000>] do_raw_spin_lock+0xb0/0xd0
>> [    3.050934] [<ffffffff805d5b58>] _raw_spin_lock_irqsave+0x20/0x2c
>> [    3.051157] [<ffffffff80067e38>] swiotlb_tbl_map_single+0xce/0x3da
>> [    3.051372] [<ffffffff80068320>] swiotlb_map+0x3a/0x15c
>> [    3.051668] [<ffffffff80065a56>] dma_map_page_attrs+0x76/0x162
>> [    3.051975] [<ffffffff8031d542>] dw_pcie_host_init+0x326/0x3f2
>>
>> Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
>> ---
>>   kernel/dma/swiotlb.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
>> index cb50f8d38360..a707a944c39a 100644
>> --- a/kernel/dma/swiotlb.c
>> +++ b/kernel/dma/swiotlb.c
>> @@ -65,7 +65,7 @@
>>   static bool swiotlb_force_bounce;
>>   static bool swiotlb_force_disable;
>> -struct io_tlb_mem io_tlb_default_mem;
>> +struct io_tlb_mem io_tlb_default_mem = { .lock = 
>> __SPIN_LOCK_UNLOCKED(io_tlb_default_mem.lock) } ;
>>   phys_addr_t swiotlb_unencrypted_base;

Re: [PATCH] swiotlb: ensure io_tlb_default_mem spinlock always initialised

[Robin Murphy]

On 2022-07-11 08:26, Ben Dooks wrote:
> On 08/07/2022 21:32, Robin Murphy wrote:
>> On 2022-07-08 18:08, Ben Dooks wrote:
>>> If the io_tlb_default_mem is used by a device that then gets used
>>> by the swiotlb code, the spinlock warning is triggered causing a
>>> lot of stack-trace.
>>
>> Hang on, how have we got as far as trying to allocate out of an 
>> uninitialised SWIOTLB at all? That seems like either something's gone 
>> more fundamentally wrong or we're missing a proper check somewhere. I 
>> don't think papering over it like this is right.
>>
>> Thanks,
>> Robin
> We have a system where we have no DMA capable memory in the 32bit
> window, which means even if we initialise swiotlb we don't have
> any dma capable memory. The code says go use an IOMMU but we don't
> have one of those either (and all peripherals are capable of DMA
> from any of the memory, so there shouldn't be the need for one)
> 
> Is there any other way of fixing this DMA allocation issue?

If none of your peripherals should need SWIOTLB, then the fact that
you're ending up in swiotlb_map() at all is a clear sign that
something's wrong. Most likely someone's forgotten to set their DMA
masks correctly.

However, by inspection it seems we do have a bug here as well, for which
the correct fix should be as below. The fireworks you're *supposed* to
get in that situation are considerably louder and more obvious than a
DEBUG_SPINLOCK complaint ;)

Thanks,
Robin.

----->8-----
Subject: [PATCH] swiotlb: Fail map correctly with failed io_tlb_default_mem

In the failure case of trying to use a buffer which we'd previously
failed to allocate, the "!mem" condition is no longer sufficient since
io_tlb_default_mem became static and assigned by default. Update the
condition to work as intended per the rest of that conversion.

Fixes: 463e862ac63e ("swiotlb: Convert io_default_tlb_mem to static allocation")
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
  kernel/dma/swiotlb.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
index cb50f8d38360..5830dce6081b 100644
--- a/kernel/dma/swiotlb.c
+++ b/kernel/dma/swiotlb.c
@@ -580,7 +580,7 @@ phys_addr_t swiotlb_tbl_map_single(struct device *dev, phys_addr_t orig_addr,
  	int index;
  	phys_addr_t tlb_addr;
  
-	if (!mem)
+	if (!mem || !mem->nslabs)
  		panic("Can not allocate SWIOTLB buffer earlier and can't now provide you with the DMA bounce buffer");
  
  	if (cc_platform_has(CC_ATTR_MEM_ENCRYPT))
-- 
2.36.1.dirty

Re: [PATCH] swiotlb: ensure io_tlb_default_mem spinlock always initialised

[Christoph Hellwig]

On Mon, Jul 11, 2022 at 11:07:17AM +0100, Robin Murphy wrote:
> If none of your peripherals should need SWIOTLB, then the fact that
> you're ending up in swiotlb_map() at all is a clear sign that
> something's wrong. Most likely someone's forgotten to set their DMA
> masks correctly.

Yes.

>
> However, by inspection it seems we do have a bug here as well, for which
> the correct fix should be as below. The fireworks you're *supposed* to
> get in that situation are considerably louder and more obvious than a
> DEBUG_SPINLOCK complaint ;)

This looks sensible, I'll pick it up.

Re: [PATCH] swiotlb: ensure io_tlb_default_mem spinlock always initialised

[Ben Dooks]

On 11/07/2022 11:21, Christoph Hellwig wrote:
> On Mon, Jul 11, 2022 at 11:07:17AM +0100, Robin Murphy wrote:
>> If none of your peripherals should need SWIOTLB, then the fact that
>> you're ending up in swiotlb_map() at all is a clear sign that
>> something's wrong. Most likely someone's forgotten to set their DMA
>> masks correctly.
> 
> Yes.

Possibly, we had at least one driver which attempted to set a 32 bit
DMA mask which had to be removed as the DMA layer accepts this but
since there is no DMA32 memory the allocator then just fails.

I expect the above may need to be a separate discussion(s) of how to
default the DMA mask and how to stop the implicit acceptance of setting
a 32-bit DMA mask.
>>
>> However, by inspection it seems we do have a bug here as well, for which
>> the correct fix should be as below. The fireworks you're *supposed* to
>> get in that situation are considerably louder and more obvious than a
>> DEBUG_SPINLOCK complaint ;)
> 
> This looks sensible, I'll pick it up.

Re: [PATCH] swiotlb: ensure io_tlb_default_mem spinlock always initialised

[Christoph Hellwig]

On Mon, Jul 11, 2022 at 11:24:51AM +0100, Ben Dooks wrote:
> On 11/07/2022 11:21, Christoph Hellwig wrote:
>> On Mon, Jul 11, 2022 at 11:07:17AM +0100, Robin Murphy wrote:
>>> If none of your peripherals should need SWIOTLB, then the fact that
>>> you're ending up in swiotlb_map() at all is a clear sign that
>>> something's wrong. Most likely someone's forgotten to set their DMA
>>> masks correctly.
>>
>> Yes.
>
> Possibly, we had at least one driver which attempted to set a 32 bit
> DMA mask which had to be removed as the DMA layer accepts this but
> since there is no DMA32 memory the allocator then just fails.
>
> I expect the above may need to be a separate discussion(s) of how to
> default the DMA mask and how to stop the implicit acceptance of setting
> a 32-bit DMA mask.

No.  Linux simply assumes you can do 32-bit DMA and this won't
change.  So we'll need to fix your platform to support swiotlb
eventually.

Re: [PATCH] swiotlb: ensure io_tlb_default_mem spinlock always initialised

[Ben Dooks]

On 11/07/2022 11:39, Christoph Hellwig wrote:
> On Mon, Jul 11, 2022 at 11:24:51AM +0100, Ben Dooks wrote:
>> On 11/07/2022 11:21, Christoph Hellwig wrote:
>>> On Mon, Jul 11, 2022 at 11:07:17AM +0100, Robin Murphy wrote:
>>>> If none of your peripherals should need SWIOTLB, then the fact that
>>>> you're ending up in swiotlb_map() at all is a clear sign that
>>>> something's wrong. Most likely someone's forgotten to set their DMA
>>>> masks correctly.
>>>
>>> Yes.
>>
>> Possibly, we had at least one driver which attempted to set a 32 bit
>> DMA mask which had to be removed as the DMA layer accepts this but
>> since there is no DMA32 memory the allocator then just fails.
>>
>> I expect the above may need to be a separate discussion(s) of how to
>> default the DMA mask and how to stop the implicit acceptance of setting
>> a 32-bit DMA mask.
> 
> No.  Linux simply assumes you can do 32-bit DMA and this won't
> change.  So we'll need to fix your platform to support swiotlb
> eventually.

Ok, is there any examples currently in the kernel that have no memory
in the DMA32 zone that do use swiotlb?

Re: [PATCH] swiotlb: ensure io_tlb_default_mem spinlock always initialised

[Robin Murphy]

On 2022-07-11 11:42, Ben Dooks wrote:
> On 11/07/2022 11:39, Christoph Hellwig wrote:
>> On Mon, Jul 11, 2022 at 11:24:51AM +0100, Ben Dooks wrote:
>>> On 11/07/2022 11:21, Christoph Hellwig wrote:
>>>> On Mon, Jul 11, 2022 at 11:07:17AM +0100, Robin Murphy wrote:
>>>>> If none of your peripherals should need SWIOTLB, then the fact that
>>>>> you're ending up in swiotlb_map() at all is a clear sign that
>>>>> something's wrong. Most likely someone's forgotten to set their DMA
>>>>> masks correctly.
>>>>
>>>> Yes.
>>>
>>> Possibly, we had at least one driver which attempted to set a 32 bit
>>> DMA mask which had to be removed as the DMA layer accepts this but
>>> since there is no DMA32 memory the allocator then just fails.
>>>
>>> I expect the above may need to be a separate discussion(s) of how to
>>> default the DMA mask and how to stop the implicit acceptance of setting
>>> a 32-bit DMA mask.
>>
>> No.  Linux simply assumes you can do 32-bit DMA and this won't
>> change.  So we'll need to fix your platform to support swiotlb
>> eventually.
> 
> Ok, is there any examples currently in the kernel that have no memory
> in the DMA32 zone that do use swiotlb?

The arm64 code originally made an assumption that a system with that 
kind of memory layout would use a DMA offset in the interconnect, and so 
placed ZONE_DMA32 in the first 4GB of available RAM rather than actual 
physical address space. The only relatively mainstream platform we 
subsequently saw with all RAM above 32 bits was AMD Seattle, which also 
*didn't* use a DMA offset, so it "worked" by virtue of this bodge in the 
sense that allocations didn't fail, but DMA transactions would then 
disappear off into nowhere when the device truncated the MSBs of 
whatever too-big DMA address it was given.

I think that stuff's long gone by now, and if any of handful of 
remaining Seattle users plug in a 32-bit PCIe device and try to use it 
with the IOMMU disabled, they'll probably see the fireworks as intended.

Much as we'd like to make DMA an explicit opt-in for all drivers, that's 
something which can only really be solved 30 years ago.

Robin.

Re: [PATCH] swiotlb: ensure io_tlb_default_mem spinlock always initialised

[Conor.Dooley]

On 11/07/2022 12:01, Robin Murphy wrote:
> On 2022-07-11 11:42, Ben Dooks wrote:
>> On 11/07/2022 11:39, Christoph Hellwig wrote:
>>> On Mon, Jul 11, 2022 at 11:24:51AM +0100, Ben Dooks wrote:
>>>> On 11/07/2022 11:21, Christoph Hellwig wrote:
>>>>> On Mon, Jul 11, 2022 at 11:07:17AM +0100, Robin Murphy wrote:
>>>>>> If none of your peripherals should need SWIOTLB, then the fact that
>>>>>> you're ending up in swiotlb_map() at all is a clear sign that
>>>>>> something's wrong. Most likely someone's forgotten to set their DMA
>>>>>> masks correctly.
>>>>>
>>>>> Yes.
>>>>
>>>> Possibly, we had at least one driver which attempted to set a 32 bit
>>>> DMA mask which had to be removed as the DMA layer accepts this but
>>>> since there is no DMA32 memory the allocator then just fails.
>>>>
>>>> I expect the above may need to be a separate discussion(s) of how to
>>>> default the DMA mask and how to stop the implicit acceptance of setting
>>>> a 32-bit DMA mask.
>>>
>>> No.  Linux simply assumes you can do 32-bit DMA and this won't
>>> change.  So we'll need to fix your platform to support swiotlb
>>> eventually.
>>
>> Ok, is there any examples currently in the kernel that have no memory
>> in the DMA32 zone that do use swiotlb?
> 
> The arm64 code originally made an assumption that a system with that kind of memory layout would use a DMA offset in the interconnect, and so placed ZONE_DMA32 in the first 4GB of available RAM rather than actual physical address space. The only relatively mainstream platform we subsequently saw with all RAM above 32 bits was AMD Seattle, which also *didn't* use a DMA offset, so it "worked" by virtue of this bodge in the sense that allocations didn't fail, but DMA transactions would then disappear off into nowhere when the device truncated the MSBs of whatever too-big DMA address it was given.
> 
> I think that stuff's long gone by now, and if any of handful of remaining Seattle users plug in a 32-bit PCIe device and try to use it with the IOMMU disabled, they'll probably see the fireworks as intended.
> 
> Much as we'd like to make DMA an explicit opt-in for all drivers, that's something which can only really be solved 30 years ago.


Out of curiosity Ben, can you shed any more light on the platform?
Thanks,
Conor.

Re: [PATCH] swiotlb: ensure io_tlb_default_mem spinlock always initialised

[Ben Dooks]

On 11/07/2022 12:52, Conor.Dooley@microchip.com wrote:
> 
> 
> On 11/07/2022 12:01, Robin Murphy wrote:
>> On 2022-07-11 11:42, Ben Dooks wrote:
>>> On 11/07/2022 11:39, Christoph Hellwig wrote:
>>>> On Mon, Jul 11, 2022 at 11:24:51AM +0100, Ben Dooks wrote:
>>>>> On 11/07/2022 11:21, Christoph Hellwig wrote:
>>>>>> On Mon, Jul 11, 2022 at 11:07:17AM +0100, Robin Murphy wrote:
>>>>>>> If none of your peripherals should need SWIOTLB, then the fact that
>>>>>>> you're ending up in swiotlb_map() at all is a clear sign that
>>>>>>> something's wrong. Most likely someone's forgotten to set their DMA
>>>>>>> masks correctly.
>>>>>>
>>>>>> Yes.
>>>>>
>>>>> Possibly, we had at least one driver which attempted to set a 32 bit
>>>>> DMA mask which had to be removed as the DMA layer accepts this but
>>>>> since there is no DMA32 memory the allocator then just fails.
>>>>>
>>>>> I expect the above may need to be a separate discussion(s) of how to
>>>>> default the DMA mask and how to stop the implicit acceptance of setting
>>>>> a 32-bit DMA mask.
>>>>
>>>> No.  Linux simply assumes you can do 32-bit DMA and this won't
>>>> change.  So we'll need to fix your platform to support swiotlb
>>>> eventually.
>>>
>>> Ok, is there any examples currently in the kernel that have no memory
>>> in the DMA32 zone that do use swiotlb?
>>
>> The arm64 code originally made an assumption that a system with that kind of memory layout would use a DMA offset in the interconnect, and so placed ZONE_DMA32 in the first 4GB of available RAM rather than actual physical address space. The only relatively mainstream platform we subsequently saw with all RAM above 32 bits was AMD Seattle, which also *didn't* use a DMA offset, so it "worked" by virtue of this bodge in the sense that allocations didn't fail, but DMA transactions would then disappear off into nowhere when the device truncated the MSBs of whatever too-big DMA address it was given.
>>
>> I think that stuff's long gone by now, and if any of handful of remaining Seattle users plug in a 32-bit PCIe device and try to use it with the IOMMU disabled, they'll probably see the fireworks as intended.
>>
>> Much as we'd like to make DMA an explicit opt-in for all drivers, that's something which can only really be solved 30 years ago.
> 
> 
> Out of curiosity Ben, can you shed any more light on the platform?

Not at the moment, sorry.

Re: [PATCH] swiotlb: ensure io_tlb_default_mem spinlock always initialised

[Conor.Dooley]

On 11/07/2022 13:45, Ben Dooks wrote:
> On 11/07/2022 12:52, Conor.Dooley@microchip.com wrote:
>>
>>
>> On 11/07/2022 12:01, Robin Murphy wrote:
>>> On 2022-07-11 11:42, Ben Dooks wrote:
>>>> On 11/07/2022 11:39, Christoph Hellwig wrote:
>>>>> On Mon, Jul 11, 2022 at 11:24:51AM +0100, Ben Dooks wrote:
>>>>>> On 11/07/2022 11:21, Christoph Hellwig wrote:
>>>>>>> On Mon, Jul 11, 2022 at 11:07:17AM +0100, Robin Murphy wrote:
>>>>>>>> If none of your peripherals should need SWIOTLB, then the fact that
>>>>>>>> you're ending up in swiotlb_map() at all is a clear sign that
>>>>>>>> something's wrong. Most likely someone's forgotten to set their DMA
>>>>>>>> masks correctly.
>>>>>>>
>>>>>>> Yes.
>>>>>>
>>>>>> Possibly, we had at least one driver which attempted to set a 32 bit
>>>>>> DMA mask which had to be removed as the DMA layer accepts this but
>>>>>> since there is no DMA32 memory the allocator then just fails.
>>>>>>
>>>>>> I expect the above may need to be a separate discussion(s) of how to
>>>>>> default the DMA mask and how to stop the implicit acceptance of setting
>>>>>> a 32-bit DMA mask.
>>>>>
>>>>> No.  Linux simply assumes you can do 32-bit DMA and this won't
>>>>> change.  So we'll need to fix your platform to support swiotlb
>>>>> eventually.
>>>>
>>>> Ok, is there any examples currently in the kernel that have no memory
>>>> in the DMA32 zone that do use swiotlb?
>>>
>>> The arm64 code originally made an assumption that a system with that kind of memory layout would use a DMA offset in the interconnect, and so placed ZONE_DMA32 in the first 4GB of available RAM rather than actual physical address space. The only relatively mainstream platform we subsequently saw with all RAM above 32 bits was AMD Seattle, which also *didn't* use a DMA offset, so it "worked" by virtue of this bodge in the sense that allocations didn't fail, but DMA transactions would then disappear off into nowhere when the device truncated the MSBs of whatever too-big DMA address it was given.
>>>
>>> I think that stuff's long gone by now, and if any of handful of remaining Seattle users plug in a 32-bit PCIe device and try to use it with the IOMMU disabled, they'll probably see the fireworks as intended.
>>>
>>> Much as we'd like to make DMA an explicit opt-in for all drivers, that's something which can only really be solved 30 years ago.
>>
>>
>> Out of curiosity Ben, can you shed any more light on the platform?
> 
> Not at the moment, sorry.

No worries. FWIW, if you do end up doing anything with no-DMA32
platforms keep me CCed. I've previously hit no-DMA32 related issues
on PolarFire SoC, so I'd like to test anything that you may end up
working on.

Thanks,
Conor.

Re: [PATCH] swiotlb: ensure io_tlb_default_mem spinlock always initialised

[Ben Dooks]

On 11/07/2022 12:01, Robin Murphy wrote:
> On 2022-07-11 11:42, Ben Dooks wrote:
>> On 11/07/2022 11:39, Christoph Hellwig wrote:
>>> On Mon, Jul 11, 2022 at 11:24:51AM +0100, Ben Dooks wrote:
>>>> On 11/07/2022 11:21, Christoph Hellwig wrote:
>>>>> On Mon, Jul 11, 2022 at 11:07:17AM +0100, Robin Murphy wrote:
>>>>>> If none of your peripherals should need SWIOTLB, then the fact that
>>>>>> you're ending up in swiotlb_map() at all is a clear sign that
>>>>>> something's wrong. Most likely someone's forgotten to set their DMA
>>>>>> masks correctly.
>>>>>
>>>>> Yes.
>>>>
>>>> Possibly, we had at least one driver which attempted to set a 32 bit
>>>> DMA mask which had to be removed as the DMA layer accepts this but
>>>> since there is no DMA32 memory the allocator then just fails.
>>>>
>>>> I expect the above may need to be a separate discussion(s) of how to
>>>> default the DMA mask and how to stop the implicit acceptance of setting
>>>> a 32-bit DMA mask.
>>>
>>> No.  Linux simply assumes you can do 32-bit DMA and this won't
>>> change.  So we'll need to fix your platform to support swiotlb
>>> eventually.
>>
>> Ok, is there any examples currently in the kernel that have no memory
>> in the DMA32 zone that do use swiotlb?
> 
> The arm64 code originally made an assumption that a system with that 
> kind of memory layout would use a DMA offset in the interconnect, and so 
> placed ZONE_DMA32 in the first 4GB of available RAM rather than actual 
> physical address space. The only relatively mainstream platform we 
> subsequently saw with all RAM above 32 bits was AMD Seattle, which also 
> *didn't* use a DMA offset, so it "worked" by virtue of this bodge in the 
> sense that allocations didn't fail, but DMA transactions would then 
> disappear off into nowhere when the device truncated the MSBs of 
> whatever too-big DMA address it was given.
> 
> I think that stuff's long gone by now, and if any of handful of 
> remaining Seattle users plug in a 32-bit PCIe device and try to use it 
> with the IOMMU disabled, they'll probably see the fireworks as intended.
> 
> Much as we'd like to make DMA an explicit opt-in for all drivers, that's 
> something which can only really be solved 30 years ago.

I need to go back and check through what we did to get some that worked
for us, and then come back and try and make some idea of how that is
best done with upstream kernel.

It might be possible for the PCIe controller to do some sort of very
simple IOMMU for the case where we might have somehow an PCI device
added to the system, but that is a very rare use-case I expect and
if someone does it they can do the effort of updating the PCIe code
and everything else that goes with it.

-- 
Ben

Re: [PATCH] swiotlb: ensure io_tlb_default_mem spinlock always initialised

[Christoph Hellwig]

On Mon, Jul 11, 2022 at 01:54:11PM +0100, Ben Dooks wrote:
> I need to go back and check through what we did to get some that worked
> for us, and then come back and try and make some idea of how that is
> best done with upstream kernel.
>
> It might be possible for the PCIe controller to do some sort of very
> simple IOMMU for the case where we might have somehow an PCI device
> added to the system, but that is a very rare use-case I expect and
> if someone does it they can do the effort of updating the PCIe code
> and everything else that goes with it.

Unfortunately there also are plenty of PCIe devices with addressing
limitation even if the PCIe spec explicitly forbits that.  40, 44 or
48 bit limitations seems to be more common than 32-bits, though.