[PATCH] swiotlb: Add swiotlb=off to disable SWIOTLB

[PATCH] swiotlb: Add swiotlb=off to disable SWIOTLB

[Florian Fainelli]

It may be useful to disable the SWIOTLB completely for testing or when a
platform is known not to have any DRAM addressing limitations what so
ever.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 Documentation/admin-guide/kernel-parameters.txt | 1 +
 include/linux/swiotlb.h                         | 1 +
 kernel/dma/swiotlb.c                            | 9 +++++++++
 3 files changed, 11 insertions(+)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 04545725f187..b0223e48921e 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -5278,6 +5278,7 @@
 			force -- force using of bounce buffers even if they
 			         wouldn't be automatically used by the kernel
 			noforce -- Never use bounce buffers (for debugging)
+			off -- Completely disable SWIOTLB
 
 	switches=	[HW,M68k]
 
diff --git a/include/linux/swiotlb.h b/include/linux/swiotlb.h
index 5857a937c637..23f86243defe 100644
--- a/include/linux/swiotlb.h
+++ b/include/linux/swiotlb.h
@@ -15,6 +15,7 @@ enum swiotlb_force {
 	SWIOTLB_NORMAL,		/* Default - depending on HW DMA mask etc. */
 	SWIOTLB_FORCE,		/* swiotlb=force */
 	SWIOTLB_NO_FORCE,	/* swiotlb=noforce */
+	SWIOTLB_OFF,		/* swiotlb=off */
 };
 
 /*
diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
index c10e855a03bc..d7a4a789c7d3 100644
--- a/kernel/dma/swiotlb.c
+++ b/kernel/dma/swiotlb.c
@@ -126,6 +126,8 @@ setup_io_tlb_npages(char *str)
 	} else if (!strcmp(str, "noforce")) {
 		swiotlb_force = SWIOTLB_NO_FORCE;
 		io_tlb_nslabs = 1;
+	} else if (!strcmp(str, "off")) {
+		swiotlb_force = SWIOTLB_OFF;
 	}
 
 	return 0;
@@ -229,6 +231,9 @@ int __init swiotlb_init_with_tbl(char *tlb, unsigned long nslabs, int verbose)
 	unsigned long i, bytes;
 	size_t alloc_size;
 
+	if (swiotlb_force == SWIOTLB_OFF)
+		return 0;
+
 	bytes = nslabs << IO_TLB_SHIFT;
 
 	io_tlb_nslabs = nslabs;
@@ -284,6 +289,9 @@ swiotlb_init(int verbose)
 	unsigned char *vstart;
 	unsigned long bytes;
 
+	if (swiotlb_force == SWIOTLB_OFF)
+		goto out;
+
 	if (!io_tlb_nslabs) {
 		io_tlb_nslabs = (default_size >> IO_TLB_SHIFT);
 		io_tlb_nslabs = ALIGN(io_tlb_nslabs, IO_TLB_SEGSIZE);
@@ -302,6 +310,7 @@ swiotlb_init(int verbose)
 		io_tlb_start = 0;
 	}
 	pr_warn("Cannot allocate buffer");
+out:
 	no_iotlb_memory = true;
 }
 
-- 
2.25.1

Re: [PATCH] swiotlb: Add swiotlb=off to disable SWIOTLB

[Florian Fainelli]

On 3/18/2021 12:18 PM, Florian Fainelli wrote:
> It may be useful to disable the SWIOTLB completely for testing or when a
> platform is known not to have any DRAM addressing limitations what so
> ever.
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>

Christoph, in addition to this change, how would you feel if we
qualified the swiotlb_init() in arch/arm/mm/init.c with a:


if (memblock_end_of_DRAM() >= SZ_4G)
	swiotlb_init(1)

right now this is made unconditional whenever ARM_LPAE is enabled which
is the case for the platforms I maintain (ARCH_BRCMSTB) however we do
not really need a SWIOTLB so long as the largest DRAM physical address
does not exceed 4GB AFAICT.

Thanks!

> ---
>  Documentation/admin-guide/kernel-parameters.txt | 1 +
>  include/linux/swiotlb.h                         | 1 +
>  kernel/dma/swiotlb.c                            | 9 +++++++++
>  3 files changed, 11 insertions(+)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 04545725f187..b0223e48921e 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -5278,6 +5278,7 @@
>  			force -- force using of bounce buffers even if they
>  			         wouldn't be automatically used by the kernel
>  			noforce -- Never use bounce buffers (for debugging)
> +			off -- Completely disable SWIOTLB
>  
>  	switches=	[HW,M68k]
>  
> diff --git a/include/linux/swiotlb.h b/include/linux/swiotlb.h
> index 5857a937c637..23f86243defe 100644
> --- a/include/linux/swiotlb.h
> +++ b/include/linux/swiotlb.h
> @@ -15,6 +15,7 @@ enum swiotlb_force {
>  	SWIOTLB_NORMAL,		/* Default - depending on HW DMA mask etc. */
>  	SWIOTLB_FORCE,		/* swiotlb=force */
>  	SWIOTLB_NO_FORCE,	/* swiotlb=noforce */
> +	SWIOTLB_OFF,		/* swiotlb=off */
>  };
>  
>  /*
> diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
> index c10e855a03bc..d7a4a789c7d3 100644
> --- a/kernel/dma/swiotlb.c
> +++ b/kernel/dma/swiotlb.c
> @@ -126,6 +126,8 @@ setup_io_tlb_npages(char *str)
>  	} else if (!strcmp(str, "noforce")) {
>  		swiotlb_force = SWIOTLB_NO_FORCE;
>  		io_tlb_nslabs = 1;
> +	} else if (!strcmp(str, "off")) {
> +		swiotlb_force = SWIOTLB_OFF;
>  	}
>  
>  	return 0;
> @@ -229,6 +231,9 @@ int __init swiotlb_init_with_tbl(char *tlb, unsigned long nslabs, int verbose)
>  	unsigned long i, bytes;
>  	size_t alloc_size;
>  
> +	if (swiotlb_force == SWIOTLB_OFF)
> +		return 0;
> +
>  	bytes = nslabs << IO_TLB_SHIFT;
>  
>  	io_tlb_nslabs = nslabs;
> @@ -284,6 +289,9 @@ swiotlb_init(int verbose)
>  	unsigned char *vstart;
>  	unsigned long bytes;
>  
> +	if (swiotlb_force == SWIOTLB_OFF)
> +		goto out;
> +
>  	if (!io_tlb_nslabs) {
>  		io_tlb_nslabs = (default_size >> IO_TLB_SHIFT);
>  		io_tlb_nslabs = ALIGN(io_tlb_nslabs, IO_TLB_SEGSIZE);
> @@ -302,6 +310,7 @@ swiotlb_init(int verbose)
>  		io_tlb_start = 0;
>  	}
>  	pr_warn("Cannot allocate buffer");
> +out:
>  	no_iotlb_memory = true;
>  }
>  
> 

-- 
Florian

Re: [PATCH] swiotlb: Add swiotlb=off to disable SWIOTLB

[Robin Murphy]

On 2021-03-18 19:22, Florian Fainelli wrote:
> 
> 
> On 3/18/2021 12:18 PM, Florian Fainelli wrote:
>> It may be useful to disable the SWIOTLB completely for testing or when a
>> platform is known not to have any DRAM addressing limitations what so
>> ever.

Isn't that what "swiotlb=noforce" is for? If you're confident that we've 
really ironed out *all* the awkward corners that used to blow up if 
various internal bits were left uninitialised, then it would make sense 
to just tweak the implementation of what we already have.

I wouldn't necessarily disagree with adding "off" as an additional alias 
for "noforce", though, since it does come across as a bit wacky for 
general use.

>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> 
> Christoph, in addition to this change, how would you feel if we
> qualified the swiotlb_init() in arch/arm/mm/init.c with a:
> 
> 
> if (memblock_end_of_DRAM() >= SZ_4G)
> 	swiotlb_init(1)

Modulo "swiotlb=force", of course ;)

Robin.

> right now this is made unconditional whenever ARM_LPAE is enabled which
> is the case for the platforms I maintain (ARCH_BRCMSTB) however we do
> not really need a SWIOTLB so long as the largest DRAM physical address
> does not exceed 4GB AFAICT.
> 
> Thanks!
> 
>> ---
>>   Documentation/admin-guide/kernel-parameters.txt | 1 +
>>   include/linux/swiotlb.h                         | 1 +
>>   kernel/dma/swiotlb.c                            | 9 +++++++++
>>   3 files changed, 11 insertions(+)
>>
>> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
>> index 04545725f187..b0223e48921e 100644
>> --- a/Documentation/admin-guide/kernel-parameters.txt
>> +++ b/Documentation/admin-guide/kernel-parameters.txt
>> @@ -5278,6 +5278,7 @@
>>   			force -- force using of bounce buffers even if they
>>   			         wouldn't be automatically used by the kernel
>>   			noforce -- Never use bounce buffers (for debugging)
>> +			off -- Completely disable SWIOTLB
>>   
>>   	switches=	[HW,M68k]
>>   
>> diff --git a/include/linux/swiotlb.h b/include/linux/swiotlb.h
>> index 5857a937c637..23f86243defe 100644
>> --- a/include/linux/swiotlb.h
>> +++ b/include/linux/swiotlb.h
>> @@ -15,6 +15,7 @@ enum swiotlb_force {
>>   	SWIOTLB_NORMAL,		/* Default - depending on HW DMA mask etc. */
>>   	SWIOTLB_FORCE,		/* swiotlb=force */
>>   	SWIOTLB_NO_FORCE,	/* swiotlb=noforce */
>> +	SWIOTLB_OFF,		/* swiotlb=off */
>>   };
>>   
>>   /*
>> diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
>> index c10e855a03bc..d7a4a789c7d3 100644
>> --- a/kernel/dma/swiotlb.c
>> +++ b/kernel/dma/swiotlb.c
>> @@ -126,6 +126,8 @@ setup_io_tlb_npages(char *str)
>>   	} else if (!strcmp(str, "noforce")) {
>>   		swiotlb_force = SWIOTLB_NO_FORCE;
>>   		io_tlb_nslabs = 1;
>> +	} else if (!strcmp(str, "off")) {
>> +		swiotlb_force = SWIOTLB_OFF;
>>   	}
>>   
>>   	return 0;
>> @@ -229,6 +231,9 @@ int __init swiotlb_init_with_tbl(char *tlb, unsigned long nslabs, int verbose)
>>   	unsigned long i, bytes;
>>   	size_t alloc_size;
>>   
>> +	if (swiotlb_force == SWIOTLB_OFF)
>> +		return 0;
>> +
>>   	bytes = nslabs << IO_TLB_SHIFT;
>>   
>>   	io_tlb_nslabs = nslabs;
>> @@ -284,6 +289,9 @@ swiotlb_init(int verbose)
>>   	unsigned char *vstart;
>>   	unsigned long bytes;
>>   
>> +	if (swiotlb_force == SWIOTLB_OFF)
>> +		goto out;
>> +
>>   	if (!io_tlb_nslabs) {
>>   		io_tlb_nslabs = (default_size >> IO_TLB_SHIFT);
>>   		io_tlb_nslabs = ALIGN(io_tlb_nslabs, IO_TLB_SEGSIZE);
>> @@ -302,6 +310,7 @@ swiotlb_init(int verbose)
>>   		io_tlb_start = 0;
>>   	}
>>   	pr_warn("Cannot allocate buffer");
>> +out:
>>   	no_iotlb_memory = true;
>>   }
>>   
>>
> 

Re: [PATCH] swiotlb: Add swiotlb=off to disable SWIOTLB

[Florian Fainelli]

On 3/18/2021 12:34 PM, Robin Murphy wrote:
> On 2021-03-18 19:22, Florian Fainelli wrote:
>>
>>
>> On 3/18/2021 12:18 PM, Florian Fainelli wrote:
>>> It may be useful to disable the SWIOTLB completely for testing or when a
>>> platform is known not to have any DRAM addressing limitations what so
>>> ever.
> 
> Isn't that what "swiotlb=noforce" is for? If you're confident that we've
> really ironed out *all* the awkward corners that used to blow up if
> various internal bits were left uninitialised, then it would make sense
> to just tweak the implementation of what we already have.

swiotlb=noforce does prevent dma_direct_map_page() from resorting to the
swiotlb, however what I am also after is reclaiming these 64MB of
default SWIOTLB bounce buffering memory because my systems run with
large amounts of reserved memory into ZONE_MOVABLE and everything in
ZONE_NORMAL is precious at that point.

> 
> I wouldn't necessarily disagree with adding "off" as an additional alias
> for "noforce", though, since it does come across as a bit wacky for
> general use.
> 
>>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>>
>> Christoph, in addition to this change, how would you feel if we
>> qualified the swiotlb_init() in arch/arm/mm/init.c with a:
>>
>>
>> if (memblock_end_of_DRAM() >= SZ_4G)
>>     swiotlb_init(1)
> 
> Modulo "swiotlb=force", of course ;)

Indeed, we would need to handle that case as well. Does it sound
reasonable to do that to you as well?
-- 
Florian

Re: [PATCH] swiotlb: Add swiotlb=off to disable SWIOTLB

[Robin Murphy]

On 2021-03-18 19:43, Florian Fainelli wrote:
> 
> 
> On 3/18/2021 12:34 PM, Robin Murphy wrote:
>> On 2021-03-18 19:22, Florian Fainelli wrote:
>>>
>>>
>>> On 3/18/2021 12:18 PM, Florian Fainelli wrote:
>>>> It may be useful to disable the SWIOTLB completely for testing or when a
>>>> platform is known not to have any DRAM addressing limitations what so
>>>> ever.
>>
>> Isn't that what "swiotlb=noforce" is for? If you're confident that we've
>> really ironed out *all* the awkward corners that used to blow up if
>> various internal bits were left uninitialised, then it would make sense
>> to just tweak the implementation of what we already have.
> 
> swiotlb=noforce does prevent dma_direct_map_page() from resorting to the
> swiotlb, however what I am also after is reclaiming these 64MB of
> default SWIOTLB bounce buffering memory because my systems run with
> large amounts of reserved memory into ZONE_MOVABLE and everything in
> ZONE_NORMAL is precious at that point.

It also forces io_tlb_nslabs to the minimum, so it should be claiming 
considerably less than 64MB. IIRC the original proposal *did* skip 
initialisation completely, but that turned up the aforementioned issues.

>> I wouldn't necessarily disagree with adding "off" as an additional alias
>> for "noforce", though, since it does come across as a bit wacky for
>> general use.
>>
>>>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>>>
>>> Christoph, in addition to this change, how would you feel if we
>>> qualified the swiotlb_init() in arch/arm/mm/init.c with a:
>>>
>>>
>>> if (memblock_end_of_DRAM() >= SZ_4G)
>>>      swiotlb_init(1)
>>
>> Modulo "swiotlb=force", of course ;)
> 
> Indeed, we would need to handle that case as well. Does it sound
> reasonable to do that to you as well?

I wouldn't like it done to me personally, but for arm64, observe what 
mem_init() in arch/arm64/mm/init.c already does.

Robin.

Re: [PATCH] swiotlb: Add swiotlb=off to disable SWIOTLB

[Florian Fainelli]

On 3/18/2021 12:53 PM, Robin Murphy wrote:
> On 2021-03-18 19:43, Florian Fainelli wrote:
>>
>>
>> On 3/18/2021 12:34 PM, Robin Murphy wrote:
>>> On 2021-03-18 19:22, Florian Fainelli wrote:
>>>>
>>>>
>>>> On 3/18/2021 12:18 PM, Florian Fainelli wrote:
>>>>> It may be useful to disable the SWIOTLB completely for testing or
>>>>> when a
>>>>> platform is known not to have any DRAM addressing limitations what so
>>>>> ever.
>>>
>>> Isn't that what "swiotlb=noforce" is for? If you're confident that we've
>>> really ironed out *all* the awkward corners that used to blow up if
>>> various internal bits were left uninitialised, then it would make sense
>>> to just tweak the implementation of what we already have.
>>
>> swiotlb=noforce does prevent dma_direct_map_page() from resorting to the
>> swiotlb, however what I am also after is reclaiming these 64MB of
>> default SWIOTLB bounce buffering memory because my systems run with
>> large amounts of reserved memory into ZONE_MOVABLE and everything in
>> ZONE_NORMAL is precious at that point.
> 
> It also forces io_tlb_nslabs to the minimum, so it should be claiming
> considerably less than 64MB. IIRC the original proposal *did* skip
> initialisation completely, but that turned up the aforementioned issues.

AFAICT in that case we will have iotlb_n_slabs will set to 1, which will
still make us allocate io_tlb_n_slabs << IO_TLB_SHIFT bytes in
swiotlb_init(), which still gives us 64MB.

> 
>>> I wouldn't necessarily disagree with adding "off" as an additional alias
>>> for "noforce", though, since it does come across as a bit wacky for
>>> general use.
>>>
>>>>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>>>>
>>>> Christoph, in addition to this change, how would you feel if we
>>>> qualified the swiotlb_init() in arch/arm/mm/init.c with a:
>>>>
>>>>
>>>> if (memblock_end_of_DRAM() >= SZ_4G)
>>>>      swiotlb_init(1)
>>>
>>> Modulo "swiotlb=force", of course ;)
>>
>> Indeed, we would need to handle that case as well. Does it sound
>> reasonable to do that to you as well?
> 
> I wouldn't like it done to me personally, but for arm64, observe what
> mem_init() in arch/arm64/mm/init.c already does.
> 
> Robin.

-- 
Florian

Re: [PATCH] swiotlb: Add swiotlb=off to disable SWIOTLB

[Robin Murphy]

On 2021-03-18 21:31, Florian Fainelli wrote:
> 
> 
> On 3/18/2021 12:53 PM, Robin Murphy wrote:
>> On 2021-03-18 19:43, Florian Fainelli wrote:
>>>
>>>
>>> On 3/18/2021 12:34 PM, Robin Murphy wrote:
>>>> On 2021-03-18 19:22, Florian Fainelli wrote:
>>>>>
>>>>>
>>>>> On 3/18/2021 12:18 PM, Florian Fainelli wrote:
>>>>>> It may be useful to disable the SWIOTLB completely for testing or
>>>>>> when a
>>>>>> platform is known not to have any DRAM addressing limitations what so
>>>>>> ever.
>>>>
>>>> Isn't that what "swiotlb=noforce" is for? If you're confident that we've
>>>> really ironed out *all* the awkward corners that used to blow up if
>>>> various internal bits were left uninitialised, then it would make sense
>>>> to just tweak the implementation of what we already have.
>>>
>>> swiotlb=noforce does prevent dma_direct_map_page() from resorting to the
>>> swiotlb, however what I am also after is reclaiming these 64MB of
>>> default SWIOTLB bounce buffering memory because my systems run with
>>> large amounts of reserved memory into ZONE_MOVABLE and everything in
>>> ZONE_NORMAL is precious at that point.
>>
>> It also forces io_tlb_nslabs to the minimum, so it should be claiming
>> considerably less than 64MB. IIRC the original proposal *did* skip
>> initialisation completely, but that turned up the aforementioned issues.
> 
> AFAICT in that case we will have iotlb_n_slabs will set to 1, which will
> still make us allocate io_tlb_n_slabs << IO_TLB_SHIFT bytes in
> swiotlb_init(), which still gives us 64MB.

Eh? When did 2KB become 64MB? IO_TLB_SHIFT is 11, so that's at most one 
page in anyone's money...

>>>> I wouldn't necessarily disagree with adding "off" as an additional alias
>>>> for "noforce", though, since it does come across as a bit wacky for
>>>> general use.
>>>>
>>>>>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>>>>>
>>>>> Christoph, in addition to this change, how would you feel if we
>>>>> qualified the swiotlb_init() in arch/arm/mm/init.c with a:
>>>>>
>>>>>
>>>>> if (memblock_end_of_DRAM() >= SZ_4G)
>>>>>       swiotlb_init(1)
>>>>
>>>> Modulo "swiotlb=force", of course ;)
>>>
>>> Indeed, we would need to handle that case as well. Does it sound
>>> reasonable to do that to you as well?
>>
>> I wouldn't like it done to me personally, but for arm64, observe what
>> mem_init() in arch/arm64/mm/init.c already does.

In fact I should have looked more closely at that myself - checking 
debugfs on my 4GB arm64 board actually shows io_tlb_nslabs = 0, and 
indeed we are bypassing initialisation completely and (ab)using 
SWIOTLB_NO_FORCE to cover it up, so I guess it probably *is* safe now 
for the noforce option to do the same for itself and save even that one 
page.

Robin.

Re: [PATCH] swiotlb: Add swiotlb=off to disable SWIOTLB

[Florian Fainelli]

On 3/18/2021 4:35 PM, Robin Murphy wrote:
> On 2021-03-18 21:31, Florian Fainelli wrote:
>>
>>
>> On 3/18/2021 12:53 PM, Robin Murphy wrote:
>>> On 2021-03-18 19:43, Florian Fainelli wrote:
>>>>
>>>>
>>>> On 3/18/2021 12:34 PM, Robin Murphy wrote:
>>>>> On 2021-03-18 19:22, Florian Fainelli wrote:
>>>>>>
>>>>>>
>>>>>> On 3/18/2021 12:18 PM, Florian Fainelli wrote:
>>>>>>> It may be useful to disable the SWIOTLB completely for testing or
>>>>>>> when a
>>>>>>> platform is known not to have any DRAM addressing limitations
>>>>>>> what so
>>>>>>> ever.
>>>>>
>>>>> Isn't that what "swiotlb=noforce" is for? If you're confident that
>>>>> we've
>>>>> really ironed out *all* the awkward corners that used to blow up if
>>>>> various internal bits were left uninitialised, then it would make
>>>>> sense
>>>>> to just tweak the implementation of what we already have.
>>>>
>>>> swiotlb=noforce does prevent dma_direct_map_page() from resorting to
>>>> the
>>>> swiotlb, however what I am also after is reclaiming these 64MB of
>>>> default SWIOTLB bounce buffering memory because my systems run with
>>>> large amounts of reserved memory into ZONE_MOVABLE and everything in
>>>> ZONE_NORMAL is precious at that point.
>>>
>>> It also forces io_tlb_nslabs to the minimum, so it should be claiming
>>> considerably less than 64MB. IIRC the original proposal *did* skip
>>> initialisation completely, but that turned up the aforementioned issues.
>>
>> AFAICT in that case we will have iotlb_n_slabs will set to 1, which will
>> still make us allocate io_tlb_n_slabs << IO_TLB_SHIFT bytes in
>> swiotlb_init(), which still gives us 64MB.
> 
> Eh? When did 2KB become 64MB? IO_TLB_SHIFT is 11, so that's at most one
> page in anyone's money...

Yes, sorry incorrect shift applied here. Still, and I believe this is
what you mean below, architecture code setting swiotlb_force =
SWIOTLB_NO_FORCE does not result in not allocating the SWIOTLB, because
io_tlb_nslabs is still left set to 0 so swiotlb_init() will proceed with
allocating the default size.

> 
>>>>> I wouldn't necessarily disagree with adding "off" as an additional
>>>>> alias
>>>>> for "noforce", though, since it does come across as a bit wacky for
>>>>> general use.
>>>>>
>>>>>>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>>>>>>
>>>>>> Christoph, in addition to this change, how would you feel if we
>>>>>> qualified the swiotlb_init() in arch/arm/mm/init.c with a:
>>>>>>
>>>>>>
>>>>>> if (memblock_end_of_DRAM() >= SZ_4G)
>>>>>>       swiotlb_init(1)
>>>>>
>>>>> Modulo "swiotlb=force", of course ;)
>>>>
>>>> Indeed, we would need to handle that case as well. Does it sound
>>>> reasonable to do that to you as well?
>>>
>>> I wouldn't like it done to me personally, but for arm64, observe what
>>> mem_init() in arch/arm64/mm/init.c already does.
> 
> In fact I should have looked more closely at that myself - checking
> debugfs on my 4GB arm64 board actually shows io_tlb_nslabs = 0, and
> indeed we are bypassing initialisation completely and (ab)using
> SWIOTLB_NO_FORCE to cover it up, so I guess it probably *is* safe now
> for the noforce option to do the same for itself and save even that one
> page.

OK, I can submit a patch that does that. 5.12-rc3 works correctly for me
here as well and only allocates SWIOTLB when needed which in our case is
either:

- we have DRAM at PA >= 4GB
- we have limited peripherals (Raspberry Pi 4 derivative) that can only
address the lower 1GB

Now let's see if we can get ARM 32-bit to match :)
-- 
Florian

Re: [PATCH] swiotlb: Add swiotlb=off to disable SWIOTLB

[Konrad Rzeszutek Wilk]

> > 
> > In fact I should have looked more closely at that myself - checking
> > debugfs on my 4GB arm64 board actually shows io_tlb_nslabs = 0, and
> > indeed we are bypassing initialisation completely and (ab)using
> > SWIOTLB_NO_FORCE to cover it up, so I guess it probably *is* safe now
> > for the noforce option to do the same for itself and save even that one
> > page.
> 
> OK, I can submit a patch that does that. 5.12-rc3 works correctly for me
> here as well and only allocates SWIOTLB when needed which in our case is
> either:
> 
> - we have DRAM at PA >= 4GB
> - we have limited peripherals (Raspberry Pi 4 derivative) that can only
> address the lower 1GB
> 
> Now let's see if we can get ARM 32-bit to match :)

Whatever patch you come up with, if it is against SWIOTLB please base it on top of
devel/for-linus-5.12 in https://git.kernel.org/pub/scm/linux/kernel/git/konrad/swiotlb.git/

Thx
> -- 
> Florian

[PATCH] swiotlb: Make SWIOTLB_NO_FORCE perform no allocation

[Florian Fainelli]

When SWIOTLB_NO_FORCE is used, there should really be no allocations of
io_tlb_nslabs to occur since we are not going to use those slabs. If a
platform was somehow setting swiotlb_no_force and a later call to
swiotlb_init() was to be made we would still be proceeding with
allocating the default SWIOTLB size (64MB), whereas if swiotlb=noforce
was set on the kernel command line we would have only allocated 2KB.

This would be inconsistent and the point of initializing io_tlb_nslabs
to 1, was to avoid hitting the test for io_tlb_nslabs being 0/not
initialized.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 kernel/dma/swiotlb.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
index c10e855a03bc..526c8321b76f 100644
--- a/kernel/dma/swiotlb.c
+++ b/kernel/dma/swiotlb.c
@@ -121,12 +121,10 @@ setup_io_tlb_npages(char *str)
 	}
 	if (*str == ',')
 		++str;
-	if (!strcmp(str, "force")) {
+	if (!strcmp(str, "force"))
 		swiotlb_force = SWIOTLB_FORCE;
-	} else if (!strcmp(str, "noforce")) {
+	else if (!strcmp(str, "noforce"))
 		swiotlb_force = SWIOTLB_NO_FORCE;
-		io_tlb_nslabs = 1;
-	}
 
 	return 0;
 }
@@ -284,6 +282,9 @@ swiotlb_init(int verbose)
 	unsigned char *vstart;
 	unsigned long bytes;
 
+	if (swiotlb_force == SWIOTLB_NO_FORCE)
+		goto out;
+
 	if (!io_tlb_nslabs) {
 		io_tlb_nslabs = (default_size >> IO_TLB_SHIFT);
 		io_tlb_nslabs = ALIGN(io_tlb_nslabs, IO_TLB_SEGSIZE);
@@ -302,6 +303,7 @@ swiotlb_init(int verbose)
 		io_tlb_start = 0;
 	}
 	pr_warn("Cannot allocate buffer");
+out:
 	no_iotlb_memory = true;
 }
 
-- 
2.25.1

Re: [PATCH] swiotlb: Make SWIOTLB_NO_FORCE perform no allocation

[Konrad Rzeszutek Wilk]

On Thu, Mar 18, 2021 at 09:00:54PM -0700, Florian Fainelli wrote:
> When SWIOTLB_NO_FORCE is used, there should really be no allocations of
> io_tlb_nslabs to occur since we are not going to use those slabs. If a
> platform was somehow setting swiotlb_no_force and a later call to
> swiotlb_init() was to be made we would still be proceeding with
> allocating the default SWIOTLB size (64MB), whereas if swiotlb=noforce
> was set on the kernel command line we would have only allocated 2KB.
> 
> This would be inconsistent and the point of initializing io_tlb_nslabs
> to 1, was to avoid hitting the test for io_tlb_nslabs being 0/not
> initialized.

Could you rebase this on devel/for-linus-5.13 in

git://git.kernel.org/pub/scm/linux/kernel/git/konrad/swiotlb.git

please?
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  kernel/dma/swiotlb.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
> index c10e855a03bc..526c8321b76f 100644
> --- a/kernel/dma/swiotlb.c
> +++ b/kernel/dma/swiotlb.c
> @@ -121,12 +121,10 @@ setup_io_tlb_npages(char *str)
>  	}
>  	if (*str == ',')
>  		++str;
> -	if (!strcmp(str, "force")) {
> +	if (!strcmp(str, "force"))
>  		swiotlb_force = SWIOTLB_FORCE;
> -	} else if (!strcmp(str, "noforce")) {
> +	else if (!strcmp(str, "noforce"))
>  		swiotlb_force = SWIOTLB_NO_FORCE;
> -		io_tlb_nslabs = 1;
> -	}
>  
>  	return 0;
>  }
> @@ -284,6 +282,9 @@ swiotlb_init(int verbose)
>  	unsigned char *vstart;
>  	unsigned long bytes;
>  
> +	if (swiotlb_force == SWIOTLB_NO_FORCE)
> +		goto out;
> +
>  	if (!io_tlb_nslabs) {
>  		io_tlb_nslabs = (default_size >> IO_TLB_SHIFT);
>  		io_tlb_nslabs = ALIGN(io_tlb_nslabs, IO_TLB_SEGSIZE);
> @@ -302,6 +303,7 @@ swiotlb_init(int verbose)
>  		io_tlb_start = 0;
>  	}
>  	pr_warn("Cannot allocate buffer");
> +out:
>  	no_iotlb_memory = true;
>  }
>  
> -- 
> 2.25.1
> 

[PATCH v2] swiotlb: Make SWIOTLB_NO_FORCE perform no allocation

[Florian Fainelli]

When SWIOTLB_NO_FORCE is used, there should really be no allocations of
default_nslabs to occur since we are not going to use those slabs. If a
platform was somehow setting swiotlb_no_force and a later call to
swiotlb_init() was to be made we would still be proceeding with
allocating the default SWIOTLB size (64MB), whereas if swiotlb=noforce
was set on the kernel command line we would have only allocated 2KB.

This would be inconsistent and the point of initializing default_nslabs
to 1, was intended to allocate the minimum amount of memory possible, so
simply remove that minimal allocation period.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
Changes in v2:

- rebased against devel/for-linus-5.13
- updated commit message to reflect variable names

 kernel/dma/swiotlb.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
index 539c76beb52e..d20002a61546 100644
--- a/kernel/dma/swiotlb.c
+++ b/kernel/dma/swiotlb.c
@@ -83,12 +83,10 @@ setup_io_tlb_npages(char *str)
 	}
 	if (*str == ',')
 		++str;
-	if (!strcmp(str, "force")) {
+	if (!strcmp(str, "force"))
 		swiotlb_force = SWIOTLB_FORCE;
-	} else if (!strcmp(str, "noforce")) {
+	else if (!strcmp(str, "noforce"))
 		swiotlb_force = SWIOTLB_NO_FORCE;
-		default_nslabs = 1;
-	}
 
 	return 0;
 }
@@ -211,6 +209,9 @@ swiotlb_init(int verbose)
 	size_t bytes = PAGE_ALIGN(default_nslabs << IO_TLB_SHIFT);
 	void *tlb;
 
+	if (swiotlb_force == SWIOTLB_NO_FORCE)
+		return;
+
 	/* Get IO TLB memory from the low pages */
 	tlb = memblock_alloc_low(bytes, PAGE_SIZE);
 	if (!tlb)
-- 
2.25.1

Re: [PATCH v2] swiotlb: Make SWIOTLB_NO_FORCE perform no allocation

[Christoph Hellwig]

On Sat, Mar 20, 2021 at 08:37:40PM -0700, Florian Fainelli wrote:
> -	if (!strcmp(str, "force")) {
> +	if (!strcmp(str, "force"))
>  		swiotlb_force = SWIOTLB_FORCE;
> -	} else if (!strcmp(str, "noforce")) {
> +	else if (!strcmp(str, "noforce"))
>  		swiotlb_force = SWIOTLB_NO_FORCE;
> -		default_nslabs = 1;
> -	}
>  
>  	return 0;
>  }
> @@ -211,6 +209,9 @@ swiotlb_init(int verbose)
>  	size_t bytes = PAGE_ALIGN(default_nslabs << IO_TLB_SHIFT);
>  	void *tlb;
>  
> +	if (swiotlb_force == SWIOTLB_NO_FORCE)
> +		return;

We'll also need this in the other callers of swiotlb_init_with_tbl
and swiotlb_late_init_with_tbl.

I actually had a plan to mostly kill them, but that can better
way until more support for multiple io_tlb structures is merged.

[PATCH v3] swiotlb: Make SWIOTLB_NO_FORCE perform no allocation

[Florian Fainelli]

When SWIOTLB_NO_FORCE is used, there should really be no allocations of
default_nslabs to occur since we are not going to use those slabs. If a
platform was somehow setting swiotlb_no_force and a later call to
swiotlb_init() was to be made we would still be proceeding with
allocating the default SWIOTLB size (64MB), whereas if swiotlb=noforce
was set on the kernel command line we would have only allocated 2KB.

This would be inconsistent and the point of initializing default_nslabs
to 1, was intended to allocate the minimum amount of memory possible, so
simply remove that minimal allocation period.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
Changes in v3:
- patch all call sites that can allocate SWIOTLB memory

Changes in v2:

- rebased against devel/for-linus-5.13
- updated commit message to reflect variable names

 kernel/dma/swiotlb.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
index 539c76beb52e..0a5b6f7e75bc 100644
--- a/kernel/dma/swiotlb.c
+++ b/kernel/dma/swiotlb.c
@@ -83,12 +83,10 @@ setup_io_tlb_npages(char *str)
 	}
 	if (*str == ',')
 		++str;
-	if (!strcmp(str, "force")) {
+	if (!strcmp(str, "force"))
 		swiotlb_force = SWIOTLB_FORCE;
-	} else if (!strcmp(str, "noforce")) {
+	else if (!strcmp(str, "noforce"))
 		swiotlb_force = SWIOTLB_NO_FORCE;
-		default_nslabs = 1;
-	}
 
 	return 0;
 }
@@ -174,6 +172,9 @@ int __init swiotlb_init_with_tbl(char *tlb, unsigned long nslabs, int verbose)
 	struct io_tlb_mem *mem;
 	size_t alloc_size;
 
+	if (swiotlb_force == SWIOTLB_NO_FORCE)
+		return 0;
+
 	/* protect against double initialization */
 	if (WARN_ON_ONCE(io_tlb_default_mem))
 		return -ENOMEM;
@@ -211,6 +212,9 @@ swiotlb_init(int verbose)
 	size_t bytes = PAGE_ALIGN(default_nslabs << IO_TLB_SHIFT);
 	void *tlb;
 
+	if (swiotlb_force == SWIOTLB_NO_FORCE)
+		return;
+
 	/* Get IO TLB memory from the low pages */
 	tlb = memblock_alloc_low(bytes, PAGE_SIZE);
 	if (!tlb)
@@ -240,6 +244,9 @@ swiotlb_late_init_with_default_size(size_t default_size)
 	unsigned int order;
 	int rc = 0;
 
+	if (swiotlb_force == SWIOTLB_NO_FORCE)
+		return 0;
+
 	/*
 	 * Get IO TLB memory from the low pages
 	 */
@@ -276,6 +283,9 @@ swiotlb_late_init_with_tbl(char *tlb, unsigned long nslabs)
 	unsigned long bytes = nslabs << IO_TLB_SHIFT, i;
 	struct io_tlb_mem *mem;
 
+	if (swiotlb_force == SWIOTLB_NO_FORCE)
+		return 0;
+
 	/* protect against double initialization */
 	if (WARN_ON_ONCE(io_tlb_default_mem))
 		return -ENOMEM;
-- 
2.25.1

Re: [PATCH v3] swiotlb: Make SWIOTLB_NO_FORCE perform no allocation

[Christoph Hellwig]

On Mon, Mar 22, 2021 at 06:53:49PM -0700, Florian Fainelli wrote:
> When SWIOTLB_NO_FORCE is used, there should really be no allocations of
> default_nslabs to occur since we are not going to use those slabs. If a
> platform was somehow setting swiotlb_no_force and a later call to
> swiotlb_init() was to be made we would still be proceeding with
> allocating the default SWIOTLB size (64MB), whereas if swiotlb=noforce
> was set on the kernel command line we would have only allocated 2KB.
> 
> This would be inconsistent and the point of initializing default_nslabs
> to 1, was intended to allocate the minimum amount of memory possible, so
> simply remove that minimal allocation period.
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v3] swiotlb: Make SWIOTLB_NO_FORCE perform no allocation

[Florian Fainelli]

On 3/24/2021 1:42 AM, Christoph Hellwig wrote:
> On Mon, Mar 22, 2021 at 06:53:49PM -0700, Florian Fainelli wrote:
>> When SWIOTLB_NO_FORCE is used, there should really be no allocations of
>> default_nslabs to occur since we are not going to use those slabs. If a
>> platform was somehow setting swiotlb_no_force and a later call to
>> swiotlb_init() was to be made we would still be proceeding with
>> allocating the default SWIOTLB size (64MB), whereas if swiotlb=noforce
>> was set on the kernel command line we would have only allocated 2KB.
>>
>> This would be inconsistent and the point of initializing default_nslabs
>> to 1, was intended to allocate the minimum amount of memory possible, so
>> simply remove that minimal allocation period.
>>
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> 
> Looks good,
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> 

Thanks! Konrad, can you apply this patch to your for-linus-5.13 branch
if you are also happy with it?
-- 
Florian

Re: [PATCH v3] swiotlb: Make SWIOTLB_NO_FORCE perform no allocation

[Konrad Rzeszutek Wilk]

On Thu, Apr 08, 2021 at 08:13:07PM -0700, Florian Fainelli wrote:
> 
> 
> On 3/24/2021 1:42 AM, Christoph Hellwig wrote:
> > On Mon, Mar 22, 2021 at 06:53:49PM -0700, Florian Fainelli wrote:
> >> When SWIOTLB_NO_FORCE is used, there should really be no allocations of
> >> default_nslabs to occur since we are not going to use those slabs. If a
> >> platform was somehow setting swiotlb_no_force and a later call to
> >> swiotlb_init() was to be made we would still be proceeding with
> >> allocating the default SWIOTLB size (64MB), whereas if swiotlb=noforce
> >> was set on the kernel command line we would have only allocated 2KB.
> >>
> >> This would be inconsistent and the point of initializing default_nslabs
> >> to 1, was intended to allocate the minimum amount of memory possible, so
> >> simply remove that minimal allocation period.
> >>
> >> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> > 
> > Looks good,
> > 
> > Reviewed-by: Christoph Hellwig <hch@lst.de>
> > 
> 
> Thanks! Konrad, can you apply this patch to your for-linus-5.13 branch
> if you are also happy with it?

It should be now visible?
> -- 
> Florian

Re: [PATCH v3] swiotlb: Make SWIOTLB_NO_FORCE perform no allocation

[Florian Fainelli]

On 4/9/2021 12:32 PM, Konrad Rzeszutek Wilk wrote:
> On Thu, Apr 08, 2021 at 08:13:07PM -0700, Florian Fainelli wrote:
>>
>>
>> On 3/24/2021 1:42 AM, Christoph Hellwig wrote:
>>> On Mon, Mar 22, 2021 at 06:53:49PM -0700, Florian Fainelli wrote:
>>>> When SWIOTLB_NO_FORCE is used, there should really be no allocations of
>>>> default_nslabs to occur since we are not going to use those slabs. If a
>>>> platform was somehow setting swiotlb_no_force and a later call to
>>>> swiotlb_init() was to be made we would still be proceeding with
>>>> allocating the default SWIOTLB size (64MB), whereas if swiotlb=noforce
>>>> was set on the kernel command line we would have only allocated 2KB.
>>>>
>>>> This would be inconsistent and the point of initializing default_nslabs
>>>> to 1, was intended to allocate the minimum amount of memory possible, so
>>>> simply remove that minimal allocation period.
>>>>
>>>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>>>
>>> Looks good,
>>>
>>> Reviewed-by: Christoph Hellwig <hch@lst.de>
>>>
>>
>> Thanks! Konrad, can you apply this patch to your for-linus-5.13 branch
>> if you are also happy with it?
> 
> It should be now visible?

Not seeing it here:

https://git.kernel.org/pub/scm/linux/kernel/git/konrad/swiotlb.git/log/?h=devel/for-linus-5.13
-- 
Florian