[PATCH 0/2] swiotlb: Rate-limit printing and 64-bit memory debugging

[PATCH 0/2] swiotlb: Rate-limit printing and 64-bit memory debugging

[Geert Uytterhoeven]

	Hi Konrad, Jon,

This patch series contains two improvements for the SWIOTLB subsystem.

The first patch adds rate-limiting to an error message, to avoid
flooding the kernel log.
The second patch adds a kernel command line option to aid debugging when
developing support for DMA to memory outside the 32-bit address space.

Thanks for your comments!

Geert Uytterhoeven (2):
  swiotlb: Rate-limit printing when running out of SW-IOMMU space
  swiotlb: Add swiotlb=nobounce debug option

 Documentation/kernel-parameters.txt |  3 ++-
 lib/swiotlb.c                       | 23 +++++++++++++++++++----
 2 files changed, 21 insertions(+), 5 deletions(-)

-- 
1.9.1

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

[PATCH 1/2] swiotlb: Rate-limit printing when running out of SW-IOMMU space

[Geert Uytterhoeven]

If the system runs out of SW-IOMMU space, changes are high successive
requests will fail, too, flooding the kernel log.  This is true
especially for streaming DMA, which is typically used repeatedly outside
the driver's initialization routine.  Add rate-limiting to fix this.

While at it, get rid of the open-coded dev_name() handling by using the
appropriate dev_err_*() variant.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 lib/swiotlb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/swiotlb.c b/lib/swiotlb.c
index 22e13a0e19d76a2b..6ce764410ae475cc 100644
--- a/lib/swiotlb.c
+++ b/lib/swiotlb.c
@@ -714,8 +714,8 @@ void swiotlb_tbl_sync_single(struct device *hwdev, phys_addr_t tlb_addr,
 	 * When the mapping is small enough return a static buffer to limit
 	 * the damage, or panic when the transfer is too big.
 	 */
-	printk(KERN_ERR "DMA: Out of SW-IOMMU space for %zu bytes at "
-	       "device %s\n", size, dev ? dev_name(dev) : "?");
+	dev_err_ratelimited(dev, "DMA: Out of SW-IOMMU space for %zu bytes\n",
+			    size);
 
 	if (size <= io_tlb_overflow || !do_panic)
 		return;
-- 
1.9.1

Re: [PATCH 1/2] swiotlb: Rate-limit printing when running out of SW-IOMMU space

[Sergei Shtylyov]

On 10/31/2016 06:45 PM, Geert Uytterhoeven wrote:

> If the system runs out of SW-IOMMU space, changes are high successive

    s/changes/chances/?

> requests will fail, too, flooding the kernel log.  This is true
> especially for streaming DMA, which is typically used repeatedly outside
> the driver's initialization routine.  Add rate-limiting to fix this.
>
> While at it, get rid of the open-coded dev_name() handling by using the
> appropriate dev_err_*() variant.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
[...]

MBR, Sergei

Re: [PATCH 1/2] swiotlb: Rate-limit printing when running out of SW-IOMMU space

[Konrad Rzeszutek Wilk]

On Mon, Oct 31, 2016 at 04:45:03PM +0100, Geert Uytterhoeven wrote:
> If the system runs out of SW-IOMMU space, changes are high successive
> requests will fail, too, flooding the kernel log.  This is true
> especially for streaming DMA, which is typically used repeatedly outside
> the driver's initialization routine.  Add rate-limiting to fix this.
> 
> While at it, get rid of the open-coded dev_name() handling by using the
> appropriate dev_err_*() variant.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

applied.
> ---
>  lib/swiotlb.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/swiotlb.c b/lib/swiotlb.c
> index 22e13a0e19d76a2b..6ce764410ae475cc 100644
> --- a/lib/swiotlb.c
> +++ b/lib/swiotlb.c
> @@ -714,8 +714,8 @@ void swiotlb_tbl_sync_single(struct device *hwdev, phys_addr_t tlb_addr,
>  	 * When the mapping is small enough return a static buffer to limit
>  	 * the damage, or panic when the transfer is too big.
>  	 */
> -	printk(KERN_ERR "DMA: Out of SW-IOMMU space for %zu bytes at "
> -	       "device %s\n", size, dev ? dev_name(dev) : "?");
> +	dev_err_ratelimited(dev, "DMA: Out of SW-IOMMU space for %zu bytes\n",
> +			    size);
>  
>  	if (size <= io_tlb_overflow || !do_panic)
>  		return;
> -- 
> 1.9.1
> 

[PATCH 2/2] swiotlb: Add swiotlb=nobounce debug option

[Geert Uytterhoeven]

On architectures like arm64, swiotlb is tied intimately to the core
architecture DMA support. In addition, ZONE_DMA cannot be disabled.

To aid debugging and catch devices not supporting DMA to memory outside
the 32-bit address space, add a kernel command line option
"swiotlb=nobounce", which disables the use of bounce buffers.
If specified, trying to map memory that cannot be used with DMA will
fail, and a warning will be printed (rate-limited).

Note that io_tlb_nslabs is set to 1, which is the minimal supported
value.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 Documentation/kernel-parameters.txt |  3 ++-
 lib/swiotlb.c                       | 19 +++++++++++++++++--
 2 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 37babf91f2cb6de2..38556cdceabaf087 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -3998,10 +3998,11 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 			it if 0 is given (See Documentation/cgroup-v1/memory.txt)
 
 	swiotlb=	[ARM,IA-64,PPC,MIPS,X86]
-			Format: { <int> | force }
+			Format: { <int> | force | nobounce }
 			<int> -- Number of I/O TLB slabs
 			force -- force using of bounce buffers even if they
 			         wouldn't be automatically used by the kernel
+			nobounce -- Never use bounce buffers (for debugging)
 
 	switches=	[HW,M68k]
 
diff --git a/lib/swiotlb.c b/lib/swiotlb.c
index 6ce764410ae475cc..4550e6b516c2a4c0 100644
--- a/lib/swiotlb.c
+++ b/lib/swiotlb.c
@@ -54,6 +54,7 @@
 #define IO_TLB_MIN_SLABS ((1<<20) >> IO_TLB_SHIFT)
 
 int swiotlb_force;
+static int swiotlb_nobounce;
 
 /*
  * Used to do a quick range check in swiotlb_tbl_unmap_single and
@@ -106,8 +107,12 @@
 	}
 	if (*str == ',')
 		++str;
-	if (!strcmp(str, "force"))
+	if (!strcmp(str, "force")) {
 		swiotlb_force = 1;
+	} else if (!strcmp(str, "nobounce")) {
+		swiotlb_nobounce = 1;
+		io_tlb_nslabs = 1;
+	}
 
 	return 0;
 }
@@ -541,8 +546,15 @@ phys_addr_t swiotlb_tbl_map_single(struct device *hwdev,
 map_single(struct device *hwdev, phys_addr_t phys, size_t size,
 	   enum dma_data_direction dir)
 {
-	dma_addr_t start_dma_addr = phys_to_dma(hwdev, io_tlb_start);
+	dma_addr_t start_dma_addr;
+
+	if (swiotlb_nobounce) {
+		dev_warn_ratelimited(hwdev, "Cannot do DMA to address %pa\n",
+				     &phys);
+		return SWIOTLB_MAP_ERROR;
+	}
 
+	start_dma_addr = phys_to_dma(hwdev, io_tlb_start);
 	return swiotlb_tbl_map_single(hwdev, start_dma_addr, phys, size, dir);
 }
 
@@ -707,6 +719,9 @@ void swiotlb_tbl_sync_single(struct device *hwdev, phys_addr_t tlb_addr,
 swiotlb_full(struct device *dev, size_t size, enum dma_data_direction dir,
 	     int do_panic)
 {
+	if (swiotlb_nobounce)
+		return;
+
 	/*
 	 * Ran out of IOMMU space for this operation. This is very bad.
 	 * Unfortunately the drivers cannot handle this operation properly.
-- 
1.9.1

Re: [PATCH 2/2] swiotlb: Add swiotlb=nobounce debug option

[Robin Murphy]

Hi Geert,

On 31/10/16 15:45, Geert Uytterhoeven wrote:
> On architectures like arm64, swiotlb is tied intimately to the core
> architecture DMA support. In addition, ZONE_DMA cannot be disabled.

To be fair, that only takes a single-character change in
arch/arm64/Kconfig - in fact, I'm amused to see my stupid patch to fix
the build if you do just that (86a5906e4d1d) has just had its birthday ;)

> To aid debugging and catch devices not supporting DMA to memory outside
> the 32-bit address space, add a kernel command line option
> "swiotlb=nobounce", which disables the use of bounce buffers.
> If specified, trying to map memory that cannot be used with DMA will
> fail, and a warning will be printed (rate-limited).

This rationale seems questionable - how useful is non-deterministic
behaviour for debugging really? What you end up with is DMA sometimes
working or sometimes not depending on whether allocations happen to
naturally fall below 4GB or not. In my experience, that in itself can be
a pain in the arse to debug.

Most of the things you might then do to make things more deterministic
again (like making the default DMA mask tiny or hacking out all the
system's 32-bit addressable RAM) are also generally sufficient to make
DMA fail earlier and make this option moot anyway. What's the specific
use case motivating this?

Robin.

> Note that io_tlb_nslabs is set to 1, which is the minimal supported
> value.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
>  Documentation/kernel-parameters.txt |  3 ++-
>  lib/swiotlb.c                       | 19 +++++++++++++++++--
>  2 files changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
> index 37babf91f2cb6de2..38556cdceabaf087 100644
> --- a/Documentation/kernel-parameters.txt
> +++ b/Documentation/kernel-parameters.txt
> @@ -3998,10 +3998,11 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
>  			it if 0 is given (See Documentation/cgroup-v1/memory.txt)
>  
>  	swiotlb=	[ARM,IA-64,PPC,MIPS,X86]
> -			Format: { <int> | force }
> +			Format: { <int> | force | nobounce }
>  			<int> -- Number of I/O TLB slabs
>  			force -- force using of bounce buffers even if they
>  			         wouldn't be automatically used by the kernel
> +			nobounce -- Never use bounce buffers (for debugging)
>  
>  	switches=	[HW,M68k]
>  
> diff --git a/lib/swiotlb.c b/lib/swiotlb.c
> index 6ce764410ae475cc..4550e6b516c2a4c0 100644
> --- a/lib/swiotlb.c
> +++ b/lib/swiotlb.c
> @@ -54,6 +54,7 @@
>  #define IO_TLB_MIN_SLABS ((1<<20) >> IO_TLB_SHIFT)
>  
>  int swiotlb_force;
> +static int swiotlb_nobounce;
>  
>  /*
>   * Used to do a quick range check in swiotlb_tbl_unmap_single and
> @@ -106,8 +107,12 @@
>  	}
>  	if (*str == ',')
>  		++str;
> -	if (!strcmp(str, "force"))
> +	if (!strcmp(str, "force")) {
>  		swiotlb_force = 1;
> +	} else if (!strcmp(str, "nobounce")) {
> +		swiotlb_nobounce = 1;
> +		io_tlb_nslabs = 1;
> +	}
>  
>  	return 0;
>  }
> @@ -541,8 +546,15 @@ phys_addr_t swiotlb_tbl_map_single(struct device *hwdev,
>  map_single(struct device *hwdev, phys_addr_t phys, size_t size,
>  	   enum dma_data_direction dir)
>  {
> -	dma_addr_t start_dma_addr = phys_to_dma(hwdev, io_tlb_start);
> +	dma_addr_t start_dma_addr;
> +
> +	if (swiotlb_nobounce) {
> +		dev_warn_ratelimited(hwdev, "Cannot do DMA to address %pa\n",
> +				     &phys);
> +		return SWIOTLB_MAP_ERROR;
> +	}
>  
> +	start_dma_addr = phys_to_dma(hwdev, io_tlb_start);
>  	return swiotlb_tbl_map_single(hwdev, start_dma_addr, phys, size, dir);
>  }
>  
> @@ -707,6 +719,9 @@ void swiotlb_tbl_sync_single(struct device *hwdev, phys_addr_t tlb_addr,
>  swiotlb_full(struct device *dev, size_t size, enum dma_data_direction dir,
>  	     int do_panic)
>  {
> +	if (swiotlb_nobounce)
> +		return;
> +
>  	/*
>  	 * Ran out of IOMMU space for this operation. This is very bad.
>  	 * Unfortunately the drivers cannot handle this operation properly.
> 

Re: [PATCH 2/2] swiotlb: Add swiotlb=nobounce debug option

[Geert Uytterhoeven]

Hi Robin,

On Mon, Oct 31, 2016 at 6:41 PM, Robin Murphy <robin.murphy@arm.com> wrote:
> On 31/10/16 15:45, Geert Uytterhoeven wrote:
>> On architectures like arm64, swiotlb is tied intimately to the core
>> architecture DMA support. In addition, ZONE_DMA cannot be disabled.
>
> To be fair, that only takes a single-character change in
> arch/arm64/Kconfig - in fact, I'm amused to see my stupid patch to fix
> the build if you do just that (86a5906e4d1d) has just had its birthday ;)

Unfortunately it's not that simple. Using a small patch (based on Mark Salter's
"arm64: make CONFIG_ZONE_DMA user settable"), it appears to work. However:
  - With CONFIG_ZONE_DMA=n and memory present over 4G, swiotlb_init() is
    not called.
    This will lead to a NULL pointer dereference later, when
    dma_map_single() calls into an unitialized SWIOTLB subsystem through
    swiotlb_tbl_map_single().
  - With CONFIG_ZONE_DMA=n and no memory present over 4G, swiotlb_init()
    is also not called, but RAVB works fine.
Disabling CONFIG_SWIOTLB is non-trivial, as the arm64 DMA core always
uses swiotlb_dma_ops, and its operations depend a lot on SWIOTLB
helpers.

So that's why I went for this option.

>> To aid debugging and catch devices not supporting DMA to memory outside
>> the 32-bit address space, add a kernel command line option
>> "swiotlb=nobounce", which disables the use of bounce buffers.
>> If specified, trying to map memory that cannot be used with DMA will
>> fail, and a warning will be printed (rate-limited).
>
> This rationale seems questionable - how useful is non-deterministic
> behaviour for debugging really? What you end up with is DMA sometimes
> working or sometimes not depending on whether allocations happen to
> naturally fall below 4GB or not. In my experience, that in itself can be
> a pain in the arse to debug.

It immediately triggered for me, though:

    rcar-dmac e7300000.dma-controller: Cannot do DMA to address
0x000000067a9b7000
    ravb e6800000.ethernet: Cannot do DMA to address 0x000000067aa07780

> Most of the things you might then do to make things more deterministic
> again (like making the default DMA mask tiny or hacking out all the
> system's 32-bit addressable RAM) are also generally sufficient to make
> DMA fail earlier and make this option moot anyway. What's the specific
> use case motivating this?

My use case is finding which drivers and DMA engines do not support 64-bit
memory. There's more info in my series "[PATCH/RFC 0/5] arm64: r8a7796: 64-bit
Memory and Ethernet Prototype"
(https://www.mail-archive.com/linux-renesas-soc@vger.kernel.org/msg08393.html)

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 2/2] swiotlb: Add swiotlb=nobounce debug option

[Robin Murphy]

On 31/10/16 18:20, Geert Uytterhoeven wrote:
> Hi Robin,
> 
> On Mon, Oct 31, 2016 at 6:41 PM, Robin Murphy <robin.murphy@arm.com> wrote:
>> On 31/10/16 15:45, Geert Uytterhoeven wrote:
>>> On architectures like arm64, swiotlb is tied intimately to the core
>>> architecture DMA support. In addition, ZONE_DMA cannot be disabled.
>>
>> To be fair, that only takes a single-character change in
>> arch/arm64/Kconfig - in fact, I'm amused to see my stupid patch to fix
>> the build if you do just that (86a5906e4d1d) has just had its birthday ;)
> 
> Unfortunately it's not that simple. Using a small patch (based on Mark Salter's
> "arm64: make CONFIG_ZONE_DMA user settable"), it appears to work. However:
>   - With CONFIG_ZONE_DMA=n and memory present over 4G, swiotlb_init() is
>     not called.
>     This will lead to a NULL pointer dereference later, when
>     dma_map_single() calls into an unitialized SWIOTLB subsystem through
>     swiotlb_tbl_map_single().
>   - With CONFIG_ZONE_DMA=n and no memory present over 4G, swiotlb_init()
>     is also not called, but RAVB works fine.
> Disabling CONFIG_SWIOTLB is non-trivial, as the arm64 DMA core always
> uses swiotlb_dma_ops, and its operations depend a lot on SWIOTLB
> helpers.
> 
> So that's why I went for this option.

OK, that's new to me - I guess this behaviour was introduced by
b67a8b29df7e ("arm64: mm: only initialize swiotlb when necessary").
Regardless of this patch, that check probably wants fixing to still do
the appropriate thing if arm64_dma_phys_limit is above 4GB (or just
depend on ZONE_DMA). Disabling ZONE_DMA for development doesn't seem
that unreasonable a thing to do, especially if there are ready-made
patches floating around already, so having it crash the kernel in ways
it didn't before isn't ideal.

>>> To aid debugging and catch devices not supporting DMA to memory outside
>>> the 32-bit address space, add a kernel command line option
>>> "swiotlb=nobounce", which disables the use of bounce buffers.
>>> If specified, trying to map memory that cannot be used with DMA will
>>> fail, and a warning will be printed (rate-limited).
>>
>> This rationale seems questionable - how useful is non-deterministic
>> behaviour for debugging really? What you end up with is DMA sometimes
>> working or sometimes not depending on whether allocations happen to
>> naturally fall below 4GB or not. In my experience, that in itself can be
>> a pain in the arse to debug.
> 
> It immediately triggered for me, though:
> 
>     rcar-dmac e7300000.dma-controller: Cannot do DMA to address
> 0x000000067a9b7000
>     ravb e6800000.ethernet: Cannot do DMA to address 0x000000067aa07780
> 
>> Most of the things you might then do to make things more deterministic
>> again (like making the default DMA mask tiny or hacking out all the
>> system's 32-bit addressable RAM) are also generally sufficient to make
>> DMA fail earlier and make this option moot anyway. What's the specific
>> use case motivating this?
> 
> My use case is finding which drivers and DMA engines do not support 64-bit
> memory. There's more info in my series "[PATCH/RFC 0/5] arm64: r8a7796: 64-bit
> Memory and Ethernet Prototype"
> (https://www.mail-archive.com/linux-renesas-soc@vger.kernel.org/msg08393.html)

Thanks for the context. I've done very similar things in the past, and
my first instinct would be to change the default DMA mask in
of_dma_configure() to something which can't reach RAM (e.g. <30 bits),
then instrument dma_set_mask() to catch cleverer drivers. That's a
straightforward way to get 100% coverage - the problem with simply
disabling bounce buffering is that whilst statistically it almost
certainly will catch >95% of cases, there will always be some that it
won't; if some driver only ever does a single dma_alloc_coherent() early
enough that allocations are still fairly deterministic, and always
happens to get a 32-bit address on that platform, it's likely to slip
through the net.

I'm not against the idea of SWIOTLB growing a runtime-disable option,
I'm just not sure what situation it's actually the best solution for.

Robin.

> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds
> 

Re: [PATCH 2/2] swiotlb: Add swiotlb=nobounce debug option

[Geert Uytterhoeven]

Hi Robin,

On Tue, Nov 1, 2016 at 12:46 PM, Robin Murphy <robin.murphy@arm.com> wrote:
>>>> To aid debugging and catch devices not supporting DMA to memory outside
>>>> the 32-bit address space, add a kernel command line option
>>>> "swiotlb=nobounce", which disables the use of bounce buffers.
>>>> If specified, trying to map memory that cannot be used with DMA will
>>>> fail, and a warning will be printed (rate-limited).
>>>
>>> This rationale seems questionable - how useful is non-deterministic
>>> behaviour for debugging really? What you end up with is DMA sometimes
>>> working or sometimes not depending on whether allocations happen to
>>> naturally fall below 4GB or not. In my experience, that in itself can be
>>> a pain in the arse to debug.
>>
>> It immediately triggered for me, though:
>>
>>     rcar-dmac e7300000.dma-controller: Cannot do DMA to address
>> 0x000000067a9b7000
>>     ravb e6800000.ethernet: Cannot do DMA to address 0x000000067aa07780
>>
>>> Most of the things you might then do to make things more deterministic
>>> again (like making the default DMA mask tiny or hacking out all the
>>> system's 32-bit addressable RAM) are also generally sufficient to make
>>> DMA fail earlier and make this option moot anyway. What's the specific
>>> use case motivating this?
>>
>> My use case is finding which drivers and DMA engines do not support 64-bit
>> memory. There's more info in my series "[PATCH/RFC 0/5] arm64: r8a7796: 64-bit
>> Memory and Ethernet Prototype"
>> (https://www.mail-archive.com/linux-renesas-soc@vger.kernel.org/msg08393.html)
>
> Thanks for the context. I've done very similar things in the past, and
> my first instinct would be to change the default DMA mask in
> of_dma_configure() to something which can't reach RAM (e.g. <30 bits),
> then instrument dma_set_mask() to catch cleverer drivers. That's a
> straightforward way to get 100% coverage - the problem with simply
> disabling bounce buffering is that whilst statistically it almost
> certainly will catch >95% of cases, there will always be some that it
> won't; if some driver only ever does a single dma_alloc_coherent() early
> enough that allocations are still fairly deterministic, and always
> happens to get a 32-bit address on that platform, it's likely to slip
> through the net.
>
> I'm not against the idea of SWIOTLB growing a runtime-disable option,
> I'm just not sure what situation it's actually the best solution for.

If I set the DMA mask to a small value, DMA is never used, and SWIOTLB
always falls back to bounce buffers (and DMAing from the small pool)?

That's the inverse of what I want to achieve: I want to avoid using the
bounce feature, to make sure the DMA engine is always used with whatever
kind of memory.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 2/2] swiotlb: Add swiotlb=nobounce debug option

[Robin Murphy]

Hi Geert,

On 07/11/16 15:41, Geert Uytterhoeven wrote:
> Hi Robin,
> 
> On Tue, Nov 1, 2016 at 12:46 PM, Robin Murphy <robin.murphy@arm.com> wrote:
>>>>> To aid debugging and catch devices not supporting DMA to memory outside
>>>>> the 32-bit address space, add a kernel command line option
>>>>> "swiotlb=nobounce", which disables the use of bounce buffers.
>>>>> If specified, trying to map memory that cannot be used with DMA will
>>>>> fail, and a warning will be printed (rate-limited).
>>>>
>>>> This rationale seems questionable - how useful is non-deterministic
>>>> behaviour for debugging really? What you end up with is DMA sometimes
>>>> working or sometimes not depending on whether allocations happen to
>>>> naturally fall below 4GB or not. In my experience, that in itself can be
>>>> a pain in the arse to debug.
>>>
>>> It immediately triggered for me, though:
>>>
>>>     rcar-dmac e7300000.dma-controller: Cannot do DMA to address
>>> 0x000000067a9b7000
>>>     ravb e6800000.ethernet: Cannot do DMA to address 0x000000067aa07780
>>>
>>>> Most of the things you might then do to make things more deterministic
>>>> again (like making the default DMA mask tiny or hacking out all the
>>>> system's 32-bit addressable RAM) are also generally sufficient to make
>>>> DMA fail earlier and make this option moot anyway. What's the specific
>>>> use case motivating this?
>>>
>>> My use case is finding which drivers and DMA engines do not support 64-bit
>>> memory. There's more info in my series "[PATCH/RFC 0/5] arm64: r8a7796: 64-bit
>>> Memory and Ethernet Prototype"
>>> (https://www.mail-archive.com/linux-renesas-soc@vger.kernel.org/msg08393.html)
>>
>> Thanks for the context. I've done very similar things in the past, and
>> my first instinct would be to change the default DMA mask in
>> of_dma_configure() to something which can't reach RAM (e.g. <30 bits),
>> then instrument dma_set_mask() to catch cleverer drivers. That's a
>> straightforward way to get 100% coverage - the problem with simply
>> disabling bounce buffering is that whilst statistically it almost
>> certainly will catch >95% of cases, there will always be some that it
>> won't; if some driver only ever does a single dma_alloc_coherent() early
>> enough that allocations are still fairly deterministic, and always
>> happens to get a 32-bit address on that platform, it's likely to slip
>> through the net.
>>
>> I'm not against the idea of SWIOTLB growing a runtime-disable option,
>> I'm just not sure what situation it's actually the best solution for.
> 
> If I set the DMA mask to a small value, DMA is never used, and SWIOTLB
> always falls back to bounce buffers (and DMAing from the small pool)?

Not quite - I meant setting the default mask to a value small enough
that any attempt to allocate or map within it (whether bounced or
otherwise) cannot possibly succeed. Of course, if you have RAM right
down to address 0 this becomes trickier - I'm not entirely certain that
going to extremes (DMA_BIT_MASK(1), say) wouldn't end up going wrong
somewhere for misleadingly unrelated reasons - but I got the impression
from the mainline DT that RAM on your platform starts at 0x40000000,
hence the 30-bit suggestion.

At that point, you can instantly tell for certain that any driver for
which DMA starts failing is relying on the default mask and definitely
needs looking at for 64-bit support, and anything that doesn't just
needs a simple check of what it's passing to dma_set_*mask*().

Alternatively, the really bulletproof option is to get the firmware to
load the kernel into the >4GB area(s) of RAM (or hack dram_base in
handle_kernel_image() in arch/arm64/kernel/efi-stub.c) and simply remove
any 32-bit-addressable areas from the DT entirely (assuming the EFI
memory map isn't hard-coded). Then SWIOTLB becomes moot as there's
nowhere it can even allocate a usable bounce buffer.

> That's the inverse of what I want to achieve: I want to avoid using the
> bounce feature, to make sure the DMA engine is always used with whatever
> kind of memory.

As I said, it's that "always" that's the problem - there is already a
no-op path through the SWIOTLB code if the buffer happens to lie within
the device's DMA mask to start with, so just making SWIOTLB a no-op does
nothing to reveal anything sufficiently lucky to always hit that path on
current platforms.

Robin.

> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds
> 

Re: [PATCH 2/2] swiotlb: Add swiotlb=nobounce debug option

[Konrad Rzeszutek Wilk]

On Mon, Oct 31, 2016 at 04:45:04PM +0100, Geert Uytterhoeven wrote:
> On architectures like arm64, swiotlb is tied intimately to the core
> architecture DMA support. In addition, ZONE_DMA cannot be disabled.
> 
> To aid debugging and catch devices not supporting DMA to memory outside
> the 32-bit address space, add a kernel command line option
> "swiotlb=nobounce", which disables the use of bounce buffers.
> If specified, trying to map memory that cannot be used with DMA will
> fail, and a warning will be printed (rate-limited).

I would make the 'swiotlb_force' an enum. And then instead of this
being 'nobounce' just do the inverse of 'force', that is the
'noforce' would trigger this no bounce effect.

So:

enum {
	NORMAL,		/* Default - depending on the hardware DMA mask and such. */
	FORCE,		/* swiotlb=force */
	NO_FORCE,	/* swiotlb=noforce */
}
> 
> Note that io_tlb_nslabs is set to 1, which is the minimal supported
> value.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
>  Documentation/kernel-parameters.txt |  3 ++-
>  lib/swiotlb.c                       | 19 +++++++++++++++++--
>  2 files changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
> index 37babf91f2cb6de2..38556cdceabaf087 100644
> --- a/Documentation/kernel-parameters.txt
> +++ b/Documentation/kernel-parameters.txt
> @@ -3998,10 +3998,11 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
>  			it if 0 is given (See Documentation/cgroup-v1/memory.txt)
>  
>  	swiotlb=	[ARM,IA-64,PPC,MIPS,X86]
> -			Format: { <int> | force }
> +			Format: { <int> | force | nobounce }
>  			<int> -- Number of I/O TLB slabs
>  			force -- force using of bounce buffers even if they
>  			         wouldn't be automatically used by the kernel
> +			nobounce -- Never use bounce buffers (for debugging)
>  
>  	switches=	[HW,M68k]
>  
> diff --git a/lib/swiotlb.c b/lib/swiotlb.c
> index 6ce764410ae475cc..4550e6b516c2a4c0 100644
> --- a/lib/swiotlb.c
> +++ b/lib/swiotlb.c
> @@ -54,6 +54,7 @@
>  #define IO_TLB_MIN_SLABS ((1<<20) >> IO_TLB_SHIFT)
>  
>  int swiotlb_force;
> +static int swiotlb_nobounce;
>  
>  /*
>   * Used to do a quick range check in swiotlb_tbl_unmap_single and
> @@ -106,8 +107,12 @@
>  	}
>  	if (*str == ',')
>  		++str;
> -	if (!strcmp(str, "force"))
> +	if (!strcmp(str, "force")) {
>  		swiotlb_force = 1;
> +	} else if (!strcmp(str, "nobounce")) {
> +		swiotlb_nobounce = 1;
> +		io_tlb_nslabs = 1;
> +	}
>  
>  	return 0;
>  }
> @@ -541,8 +546,15 @@ phys_addr_t swiotlb_tbl_map_single(struct device *hwdev,
>  map_single(struct device *hwdev, phys_addr_t phys, size_t size,
>  	   enum dma_data_direction dir)
>  {
> -	dma_addr_t start_dma_addr = phys_to_dma(hwdev, io_tlb_start);
> +	dma_addr_t start_dma_addr;
> +
> +	if (swiotlb_nobounce) {
> +		dev_warn_ratelimited(hwdev, "Cannot do DMA to address %pa\n",
> +				     &phys);
> +		return SWIOTLB_MAP_ERROR;
> +	}
>  
> +	start_dma_addr = phys_to_dma(hwdev, io_tlb_start);
>  	return swiotlb_tbl_map_single(hwdev, start_dma_addr, phys, size, dir);
>  }
>  
> @@ -707,6 +719,9 @@ void swiotlb_tbl_sync_single(struct device *hwdev, phys_addr_t tlb_addr,
>  swiotlb_full(struct device *dev, size_t size, enum dma_data_direction dir,
>  	     int do_panic)
>  {
> +	if (swiotlb_nobounce)
> +		return;
> +
>  	/*
>  	 * Ran out of IOMMU space for this operation. This is very bad.
>  	 * Unfortunately the drivers cannot handle this operation properly.
> -- 
> 1.9.1
> 

Re: [PATCH 2/2] swiotlb: Add swiotlb=nobounce debug option

[Geert Uytterhoeven]

Hi Konrad,

On Mon, Oct 31, 2016 at 6:52 PM, Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com> wrote:
> On Mon, Oct 31, 2016 at 04:45:04PM +0100, Geert Uytterhoeven wrote:
>> On architectures like arm64, swiotlb is tied intimately to the core
>> architecture DMA support. In addition, ZONE_DMA cannot be disabled.
>>
>> To aid debugging and catch devices not supporting DMA to memory outside
>> the 32-bit address space, add a kernel command line option
>> "swiotlb=nobounce", which disables the use of bounce buffers.
>> If specified, trying to map memory that cannot be used with DMA will
>> fail, and a warning will be printed (rate-limited).
>
> I would make the 'swiotlb_force' an enum. And then instead of this
> being 'nobounce' just do the inverse of 'force', that is the
> 'noforce' would trigger this no bounce effect.
>
> So:
>
> enum {
>         NORMAL,         /* Default - depending on the hardware DMA mask and such. */
>         FORCE,          /* swiotlb=force */
>         NO_FORCE,       /* swiotlb=noforce */

Fine for me, but swiotlb_force is exported to platform code. Hence all users
should be updated?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 2/2] swiotlb: Add swiotlb=nobounce debug option

[Konrad Rzeszutek Wilk]

On Mon, Nov 07, 2016 at 07:57:11PM +0100, Geert Uytterhoeven wrote:
> Hi Konrad,
> 
> On Mon, Oct 31, 2016 at 6:52 PM, Konrad Rzeszutek Wilk
> <konrad.wilk@oracle.com> wrote:
> > On Mon, Oct 31, 2016 at 04:45:04PM +0100, Geert Uytterhoeven wrote:
> >> On architectures like arm64, swiotlb is tied intimately to the core
> >> architecture DMA support. In addition, ZONE_DMA cannot be disabled.
> >>
> >> To aid debugging and catch devices not supporting DMA to memory outside
> >> the 32-bit address space, add a kernel command line option
> >> "swiotlb=nobounce", which disables the use of bounce buffers.
> >> If specified, trying to map memory that cannot be used with DMA will
> >> fail, and a warning will be printed (rate-limited).
> >
> > I would make the 'swiotlb_force' an enum. And then instead of this
> > being 'nobounce' just do the inverse of 'force', that is the
> > 'noforce' would trigger this no bounce effect.
> >
> > So:
> >
> > enum {
> >         NORMAL,         /* Default - depending on the hardware DMA mask and such. */
> >         FORCE,          /* swiotlb=force */
> >         NO_FORCE,       /* swiotlb=noforce */
> 
> Fine for me, but swiotlb_force is exported to platform code. Hence all users
> should be updated?

Yeah it would have to be moved to the swiotlb.h (the enum).

Thanks!