[PATCH v1 0/4] swiotlb: some cleanup

[PATCH v1 0/4] swiotlb: some cleanup

[Dongli Zhang]

Hello,

The patch 1-2 are to cleanup unused variable and return type.

The patch 3 is to fix the param usage description in kernel doc.

The patch 4 is to panic on purpose when nslabs is too small.


Dongli Zhang:
  [PATCH v1 1/4] swiotlb: remove unused swiotlb_force
  [PATCH v1 2/4] swiotlb: remove useless return
  [PATCH v1 3/4] x86/swiotlb: fix param usage in boot-options.rst
  [PATCH v1 4/4] swiotlb: panic if nslabs is too small


Thank you very much!

Dongli Zhang

[PATCH v1 1/4] swiotlb: remove unused swiotlb_force

[Dongli Zhang]

The 'swiotlb_force' is removed since commit c6af2aa9ffc9 ("swiotlb: make
the swiotlb_init interface more useful").

Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com>
---
 include/linux/swiotlb.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/include/linux/swiotlb.h b/include/linux/swiotlb.h
index 7ed35dd3de6e..bdc58a0e20b1 100644
--- a/include/linux/swiotlb.h
+++ b/include/linux/swiotlb.h
@@ -60,7 +60,6 @@ dma_addr_t swiotlb_map(struct device *dev, phys_addr_t phys,
 		size_t size, enum dma_data_direction dir, unsigned long attrs);
 
 #ifdef CONFIG_SWIOTLB
-extern enum swiotlb_force swiotlb_force;
 
 /**
  * struct io_tlb_mem - IO TLB Memory Pool Descriptor
-- 
2.17.1

[PATCH v1 2/4] swiotlb: remove useless return

[Dongli Zhang]

Both swiotlb_init_remap() and swiotlb_init() have return type void.

Signed-off-by: Dongli Zhang <dongli.zhang@oracle.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..fd21f4162f4b 100644
--- a/kernel/dma/swiotlb.c
+++ b/kernel/dma/swiotlb.c
@@ -282,7 +282,7 @@ void __init swiotlb_init_remap(bool addressing_limit, unsigned int flags,
 
 void __init swiotlb_init(bool addressing_limit, unsigned int flags)
 {
-	return swiotlb_init_remap(addressing_limit, flags, NULL);
+	swiotlb_init_remap(addressing_limit, flags, NULL);
 }
 
 /*
-- 
2.17.1

[PATCH v1 3/4] x86/swiotlb: fix param usage in boot-options.rst

[Dongli Zhang]

Fix the usage of swiotlb param in kernel doc.

Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com>
---
 Documentation/x86/x86_64/boot-options.rst | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/Documentation/x86/x86_64/boot-options.rst b/Documentation/x86/x86_64/boot-options.rst
index 03ec9cf01181..cbd14124a667 100644
--- a/Documentation/x86/x86_64/boot-options.rst
+++ b/Documentation/x86/x86_64/boot-options.rst
@@ -287,11 +287,13 @@ iommu options only relevant to the AMD GART hardware IOMMU:
 iommu options only relevant to the software bounce buffering (SWIOTLB) IOMMU
 implementation:
 
-    swiotlb=<pages>[,force]
-      <pages>
-        Prereserve that many 128K pages for the software IO bounce buffering.
+    swiotlb=<slots>[,force,noforce]
+      <slots>
+        Prereserve that many 2K slots for the software IO bounce buffering.
       force
         Force all IO through the software TLB.
+      noforce
+        Do not initialize the software TLB.
 
 
 Miscellaneous
-- 
2.17.1

[PATCH v1 4/4] swiotlb: panic if nslabs is too small

[Dongli Zhang]

Panic on purpose if nslabs is too small, in order to sync with the remap
retry logic.

In addition, print the number of bytes for tlb alloc failure.

Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com>
---
 kernel/dma/swiotlb.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
index fd21f4162f4b..1758b724c7a8 100644
--- a/kernel/dma/swiotlb.c
+++ b/kernel/dma/swiotlb.c
@@ -242,6 +242,9 @@ void __init swiotlb_init_remap(bool addressing_limit, unsigned int flags,
 	if (swiotlb_force_disable)
 		return;
 
+	if (nslabs < IO_TLB_MIN_SLABS)
+		panic("%s: nslabs = %lu too small\n", __func__, nslabs);
+
 	/*
 	 * By default allocate the bounce buffer memory from low memory, but
 	 * allow to pick a location everywhere for hypervisors with guest
@@ -254,7 +257,8 @@ void __init swiotlb_init_remap(bool addressing_limit, unsigned int flags,
 	else
 		tlb = memblock_alloc_low(bytes, PAGE_SIZE);
 	if (!tlb) {
-		pr_warn("%s: failed to allocate tlb structure\n", __func__);
+		pr_warn("%s: Failed to allocate %zu bytes tlb structure\n",
+			__func__, bytes);
 		return;
 	}
 
-- 
2.17.1

Re: [PATCH v1 4/4] swiotlb: panic if nslabs is too small

[Dongli Zhang]

On 6/11/22 1:25 AM, Dongli Zhang wrote:
> Panic on purpose if nslabs is too small, in order to sync with the remap
> retry logic.
> 
> In addition, print the number of bytes for tlb alloc failure.
> 
> Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com>
> ---
>  kernel/dma/swiotlb.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
> index fd21f4162f4b..1758b724c7a8 100644
> --- a/kernel/dma/swiotlb.c
> +++ b/kernel/dma/swiotlb.c
> @@ -242,6 +242,9 @@ void __init swiotlb_init_remap(bool addressing_limit, unsigned int flags,
>  	if (swiotlb_force_disable)
>  		return;
>  
> +	if (nslabs < IO_TLB_MIN_SLABS)
> +		panic("%s: nslabs = %lu too small\n", __func__, nslabs);
> +
>  	/*
>  	 * By default allocate the bounce buffer memory from low memory, but
>  	 * allow to pick a location everywhere for hypervisors with guest
> @@ -254,7 +257,8 @@ void __init swiotlb_init_remap(bool addressing_limit, unsigned int flags,
>  	else
>  		tlb = memblock_alloc_low(bytes, PAGE_SIZE);
>  	if (!tlb) {
> -		pr_warn("%s: failed to allocate tlb structure\n", __func__);
> +		pr_warn("%s: Failed to allocate %zu bytes tlb structure\n",
> +			__func__, bytes);

Indeed I have a question on this pr_warn(). (I was going to send another patch
to retry with nslabs = ALIGN(nslabs >> 1, IO_TLB_SEGSIZE)).

Why not retry with nslabs = ALIGN(nslabs >> 1, IO_TLB_SEGSIZE), or panic here?

If the QEMU machine of my VM is i440fx, the boot is almost failed even here is
pr_warn. Why not sync with the remap failure handling?

1. retry with nslabs = ALIGN(nslabs >> 1, IO_TLB_SEGSIZE))
2. and finally panic if nslabs is too small.

Thank you very much!

Dongli Zhang

>  		return;
>  	}
>  
> 

Re: [PATCH v1 0/4] swiotlb: some cleanup

[Christoph Hellwig]

Thanks,

I've applied all 4 to the dma-mapping tree for Linux 5.20.

Re: [PATCH v1 4/4] swiotlb: panic if nslabs is too small

[Yu Zhao]

> Panic on purpose if nslabs is too small, in order to sync with the remap
> retry logic.
> 
> In addition, print the number of bytes for tlb alloc failure.
> 
> Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com>
> ---
>  kernel/dma/swiotlb.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
> index fd21f4162f4b..1758b724c7a8 100644
> --- a/kernel/dma/swiotlb.c
> +++ b/kernel/dma/swiotlb.c
> @@ -242,6 +242,9 @@ void __init swiotlb_init_remap(bool addressing_limit, unsigned int flags,
>  	if (swiotlb_force_disable)
>  		return;
>  
> +	if (nslabs < IO_TLB_MIN_SLABS)
> +		panic("%s: nslabs = %lu too small\n", __func__, nslabs);

Hi,

This patch breaks MIPS. Please take a look. Thanks.

On v5.19.0:
  Linux version 5.19.0 (builder@buildhost) (mips64-openwrt-linux-musl-gcc (OpenWrt GCC 11.2.0 r19590-042d558536) 11.2.0, GNU ld (GNU Binutils) 2.37) #0 SMP Sun Jul 31 15:12:47 2022
  Skipping L2 locking due to reduced L2 cache size
  CVMSEG size: 0 cache lines (0 bytes)
  printk: bootconsole [early0] enabled
  CPU0 revision is: 000d9301 (Cavium Octeon II)
  Kernel sections are not in the memory maps
  Wasting 278528 bytes for tracking 4352 unused pages
  Initrd not found or empty - disabling initrd
  Using appended Device Tree.
  software IO TLB: SWIOTLB bounce buffer size adjusted to 0MB
  software IO TLB: mapped [mem 0x0000000004b0c000-0x0000000004b4c000] (0MB)

On v6.0-rc1, with
  commit 0bf28fc40d89 ("swiotlb: panic if nslabs is too small")
  commit 20347fca71a3 ("swiotlb: split up the global swiotlb lock")
  commit 534ea58b3ceb ("Revert "MIPS: octeon: Remove vestiges of CONFIG_CAVIUM_RESERVE32"")

  Linux version 6.0.0-rc1 (builder@buildhost) (mips64-openwrt-linux-musl-gcc (OpenWrt GCC 11.2.0 r19590-042d558536) 11.2.0, GNU ld (GNU Binutils) 2.37) #0 SMP Sun Jul 31 15:12:47 2022
  Failed to allocate CAVIUM_RESERVE32 memory area
  Skipping L2 locking due to reduced L2 cache size
  CVMSEG size: 0 cache lines (0 bytes)
  printk: bootconsole [early0] enabled
  CPU0 revision is: 000d9301 (Cavium Octeon II)
  Kernel sections are not in the memory maps
  Wasting 278528 bytes for tracking 4352 unused pages
  Initrd not found or empty - disabling initrd
  Using appended Device Tree.
  software IO TLB: SWIOTLB bounce buffer size adjusted to 0MB
  software IO TLB: area num 1.
  Kernel panic - not syncing: swiotlb_init_remap: nslabs = 128 too small

On v6.0-rc1, with
  commit 20347fca71a3 ("swiotlb: split up the global swiotlb lock")
  commit 534ea58b3ceb ("Revert "MIPS: octeon: Remove vestiges of CONFIG_CAVIUM_RESERVE32"")

  Linux version 6.0.0-rc1+ (builder@buildhost) (mips64-openwrt-linux-musl-gcc (OpenWrt GCC 11.2.0 r19590-042d558536) 11.2.0, GNU ld (GNU Binutils) 2.37) #0 SMP Sun Jul 31 15:12:47 2022
  Failed to allocate CAVIUM_RESERVE32 memory area
  Skipping L2 locking due to reduced L2 cache size
  CVMSEG size: 0 cache lines (0 bytes)
  printk: bootconsole [early0] enabled
  CPU0 revision is: 000d9301 (Cavium Octeon II)
  Kernel sections are not in the memory maps
  Wasting 278528 bytes for tracking 4352 unused pages
  Initrd not found or empty - disabling initrd
  Using appended Device Tree.
  software IO TLB: SWIOTLB bounce buffer size adjusted to 0MB
  software IO TLB: area num 1.
  software IO TLB: mapped [mem 0x0000000004c0c000-0x0000000004c4c000] (0MB)

Re: [PATCH v1 4/4] swiotlb: panic if nslabs is too small

[Robin Murphy]

On 2022-08-20 02:20, Yu Zhao wrote:
>> Panic on purpose if nslabs is too small, in order to sync with the remap
>> retry logic.
>>
>> In addition, print the number of bytes for tlb alloc failure.
>>
>> Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com>
>> ---
>>   kernel/dma/swiotlb.c | 6 +++++-
>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
>> index fd21f4162f4b..1758b724c7a8 100644
>> --- a/kernel/dma/swiotlb.c
>> +++ b/kernel/dma/swiotlb.c
>> @@ -242,6 +242,9 @@ void __init swiotlb_init_remap(bool addressing_limit, unsigned int flags,
>>   	if (swiotlb_force_disable)
>>   		return;
>>   
>> +	if (nslabs < IO_TLB_MIN_SLABS)
>> +		panic("%s: nslabs = %lu too small\n", __func__, nslabs);
> 
> Hi,
> 
> This patch breaks MIPS. Please take a look. Thanks.

Hmm, it's possible this might be quietly fixed by 20347fca71a3, but 
either way I'm not sure why we would need to panic *before* we've even 
tried to allocate anything, when we could simply return with no harm 
done? If we've ended up calculating (or being told) a buffer size which 
is too small to be usable, that should be no different to disabling 
SWIOTLB entirely.

Historically, passing "swiotlb=1" on the command line has been used to 
save memory when the user knows SWIOTLB isn't needed. That should 
definitely not be allowed to start panicking.

(once again, another patch which was not CCed to the correct reviewers, 
sigh...)

Thanks,
Robin.

> On v5.19.0:
>    Linux version 5.19.0 (builder@buildhost) (mips64-openwrt-linux-musl-gcc (OpenWrt GCC 11.2.0 r19590-042d558536) 11.2.0, GNU ld (GNU Binutils) 2.37) #0 SMP Sun Jul 31 15:12:47 2022
>    Skipping L2 locking due to reduced L2 cache size
>    CVMSEG size: 0 cache lines (0 bytes)
>    printk: bootconsole [early0] enabled
>    CPU0 revision is: 000d9301 (Cavium Octeon II)
>    Kernel sections are not in the memory maps
>    Wasting 278528 bytes for tracking 4352 unused pages
>    Initrd not found or empty - disabling initrd
>    Using appended Device Tree.
>    software IO TLB: SWIOTLB bounce buffer size adjusted to 0MB
>    software IO TLB: mapped [mem 0x0000000004b0c000-0x0000000004b4c000] (0MB)
> 
> On v6.0-rc1, with
>    commit 0bf28fc40d89 ("swiotlb: panic if nslabs is too small")
>    commit 20347fca71a3 ("swiotlb: split up the global swiotlb lock")
>    commit 534ea58b3ceb ("Revert "MIPS: octeon: Remove vestiges of CONFIG_CAVIUM_RESERVE32"")
> 
>    Linux version 6.0.0-rc1 (builder@buildhost) (mips64-openwrt-linux-musl-gcc (OpenWrt GCC 11.2.0 r19590-042d558536) 11.2.0, GNU ld (GNU Binutils) 2.37) #0 SMP Sun Jul 31 15:12:47 2022
>    Failed to allocate CAVIUM_RESERVE32 memory area
>    Skipping L2 locking due to reduced L2 cache size
>    CVMSEG size: 0 cache lines (0 bytes)
>    printk: bootconsole [early0] enabled
>    CPU0 revision is: 000d9301 (Cavium Octeon II)
>    Kernel sections are not in the memory maps
>    Wasting 278528 bytes for tracking 4352 unused pages
>    Initrd not found or empty - disabling initrd
>    Using appended Device Tree.
>    software IO TLB: SWIOTLB bounce buffer size adjusted to 0MB
>    software IO TLB: area num 1.
>    Kernel panic - not syncing: swiotlb_init_remap: nslabs = 128 too small
> 
> On v6.0-rc1, with
>    commit 20347fca71a3 ("swiotlb: split up the global swiotlb lock")
>    commit 534ea58b3ceb ("Revert "MIPS: octeon: Remove vestiges of CONFIG_CAVIUM_RESERVE32"")
> 
>    Linux version 6.0.0-rc1+ (builder@buildhost) (mips64-openwrt-linux-musl-gcc (OpenWrt GCC 11.2.0 r19590-042d558536) 11.2.0, GNU ld (GNU Binutils) 2.37) #0 SMP Sun Jul 31 15:12:47 2022
>    Failed to allocate CAVIUM_RESERVE32 memory area
>    Skipping L2 locking due to reduced L2 cache size
>    CVMSEG size: 0 cache lines (0 bytes)
>    printk: bootconsole [early0] enabled
>    CPU0 revision is: 000d9301 (Cavium Octeon II)
>    Kernel sections are not in the memory maps
>    Wasting 278528 bytes for tracking 4352 unused pages
>    Initrd not found or empty - disabling initrd
>    Using appended Device Tree.
>    software IO TLB: SWIOTLB bounce buffer size adjusted to 0MB
>    software IO TLB: area num 1.
>    software IO TLB: mapped [mem 0x0000000004c0c000-0x0000000004c4c000] (0MB)

Re: [PATCH v1 4/4] swiotlb: panic if nslabs is too small

[Christoph Hellwig]

On Mon, Aug 22, 2022 at 10:49:09AM +0100, Robin Murphy wrote:
> Hmm, it's possible this might be quietly fixed by 20347fca71a3, but either
> way I'm not sure why we would need to panic *before* we've even tried to
> allocate anything, when we could simply return with no harm done? If we've
> ended up calculating (or being told) a buffer size which is too small to be
> usable, that should be no different to disabling SWIOTLB entirely.

Hmm.  I think this might be a philosophical question, but I think
failing the boot with a clear error report for a configuration that is
supposed to work but can't is way better than just panicing later on.

> Historically, passing "swiotlb=1" on the command line has been used to save
> memory when the user knows SWIOTLB isn't needed. That should definitely not
> be allowed to start panicking.

I've never seen swiotlb=1 advertized as a way to disable swiotlb.
That's always been swiotlb=noforce, which cleanly disables it.

Re: [PATCH v1 4/4] swiotlb: panic if nslabs is too small

[Robin Murphy]

On 2022-08-22 12:26, Christoph Hellwig wrote:
> On Mon, Aug 22, 2022 at 10:49:09AM +0100, Robin Murphy wrote:
>> Hmm, it's possible this might be quietly fixed by 20347fca71a3, but either
>> way I'm not sure why we would need to panic *before* we've even tried to
>> allocate anything, when we could simply return with no harm done? If we've
>> ended up calculating (or being told) a buffer size which is too small to be
>> usable, that should be no different to disabling SWIOTLB entirely.
> 
> Hmm.  I think this might be a philosophical question, but I think
> failing the boot with a clear error report for a configuration that is
> supposed to work but can't is way better than just panicing later on.

Depends which context of "supposed to work" you mean there. The most 
logical reason to end up with a tiny SWIOTLB size is because you don't 
expect to need SWIOTLB, therefore if there's now a functional minimum 
size limit, failing gracefully such that the system keeps working as 
before is correct in that context. Even if we assume the expectation 
goes the other way, then it should be on SWIOTLB to adjust the initial 
allocation size to whatever minimum it now needs, which as I say it 
looks like 20347fca71a3 might do anyway. Creating new breakage by 
panicking instead of making a decision one way or the other was never 
the right answer.

>> Historically, passing "swiotlb=1" on the command line has been used to save
>> memory when the user knows SWIOTLB isn't needed. That should definitely not
>> be allowed to start panicking.
> 
> I've never seen swiotlb=1 advertized as a way to disable swiotlb.
> That's always been swiotlb=noforce, which cleanly disables it.

No, it's probably not been advertised as such, but it's what clearly 
fell out of the available options before "noforce" was added (which was 
considerably more recently than "always"), and the fact is that people 
*are* still using it even today (presumably copy-pasted through Android 
BSPs since before 4.10).

Thanks,
Robin.

Re: [PATCH v1 4/4] swiotlb: panic if nslabs is too small

[Dongli Zhang]

Hi Yu, Robin and Christoph,

The mips kernel panic because the SWIOTLB buffer is adjusted to a very small
value (< 1MB, or < 512-slot), so that the swiotlb panic on purpose.

software IO TLB: SWIOTLB bounce buffer size adjusted to 0MB
software IO TLB: area num 1.
Kernel panic - not syncing: swiotlb_init_remap: nslabs = 128 too small


From mips code, the 'swiotlbsize' is set to PAGE_SIZE initially. It is always
PAGE_SIZE unless it is used by CONFIG_PCI or CONFIG_USB_OHCI_HCD_PLATFORM.

Finally, the swiotlb panic on purpose.

189 void __init plat_swiotlb_setup(void)
190 {
... ...
211         swiotlbsize = PAGE_SIZE;
212
213 #ifdef CONFIG_PCI
214         /*
215          * For OCTEON_DMA_BAR_TYPE_SMALL, size the iotlb at 1/4 memory
216          * size to a maximum of 64MB
217          */
218         if (OCTEON_IS_MODEL(OCTEON_CN31XX)
219             || OCTEON_IS_MODEL(OCTEON_CN38XX_PASS2)) {
220                 swiotlbsize = addr_size / 4;
221                 if (swiotlbsize > 64 * (1<<20))
222                         swiotlbsize = 64 * (1<<20);
223         } else if (max_addr > 0xf0000000ul) {
224                 /*
225                  * Otherwise only allocate a big iotlb if there is
226                  * memory past the BAR1 hole.
227                  */
228                 swiotlbsize = 64 * (1<<20);
229         }
230 #endif
231 #ifdef CONFIG_USB_OHCI_HCD_PLATFORM
232         /* OCTEON II ohci is only 32-bit. */
233         if (OCTEON_IS_OCTEON2() && max_addr >= 0x100000000ul)
234                 swiotlbsize = 64 * (1<<20);
235 #endif
236
237         swiotlb_adjust_size(swiotlbsize);
238         swiotlb_init(true, SWIOTLB_VERBOSE);
239 }


Here are some thoughts. Would you mind suggesting which is the right way to go?

1. Will the PAGE_SIZE swiotlb be used by mips when it is only PAGE_SIZE? If it
is not used, why not disable swiotlb completely in the code?

2. The swiotlb panic on purpose when it is less then 1MB. Should we remove that
limitation?

3. ... or explicitly declare the limitation that: "swiotlb should be at least
1MB, otherwise please do not use it"?


The reason I add the panic on purpose is for below case:

The user's kernel is configured with very small swiotlb buffer. As a result, the
device driver may work abnormally. As a result, the issue is reported to a
specific driver's developers, who spend some time to confirm it is swiotlb
issue. Suppose those developers are not familiar with IOMMU/swiotlb, it takes
times until the root cause is identified.

If we panic earlier, the issue will be reported to IOMMU/swiotlb developer. This
is also to sync with the remap failure logic in swiotlb (used by xen).


Thank you very much!

Dongli Zhang

On 8/22/22 5:32 AM, Robin Murphy wrote:
> On 2022-08-22 12:26, Christoph Hellwig wrote:
>> On Mon, Aug 22, 2022 at 10:49:09AM +0100, Robin Murphy wrote:
>>> Hmm, it's possible this might be quietly fixed by 20347fca71a3, but either
>>> way I'm not sure why we would need to panic *before* we've even tried to
>>> allocate anything, when we could simply return with no harm done? If we've
>>> ended up calculating (or being told) a buffer size which is too small to be
>>> usable, that should be no different to disabling SWIOTLB entirely.
>>
>> Hmm.  I think this might be a philosophical question, but I think
>> failing the boot with a clear error report for a configuration that is
>> supposed to work but can't is way better than just panicing later on.
> 
> Depends which context of "supposed to work" you mean there. The most logical
> reason to end up with a tiny SWIOTLB size is because you don't expect to need
> SWIOTLB, therefore if there's now a functional minimum size limit, failing
> gracefully such that the system keeps working as before is correct in that
> context. Even if we assume the expectation goes the other way, then it should be
> on SWIOTLB to adjust the initial allocation size to whatever minimum it now
> needs, which as I say it looks like 20347fca71a3 might do anyway. Creating new
> breakage by panicking instead of making a decision one way or the other was
> never the right answer.
> 
>>> Historically, passing "swiotlb=1" on the command line has been used to save
>>> memory when the user knows SWIOTLB isn't needed. That should definitely not
>>> be allowed to start panicking.
>>
>> I've never seen swiotlb=1 advertized as a way to disable swiotlb.
>> That's always been swiotlb=noforce, which cleanly disables it.
> 
> No, it's probably not been advertised as such, but it's what clearly fell out of
> the available options before "noforce" was added (which was considerably more
> recently than "always"), and the fact is that people *are* still using it even
> today (presumably copy-pasted through Android BSPs since before 4.10).
> 
> Thanks,
> Robin.

Re: [PATCH v1 4/4] swiotlb: panic if nslabs is too small

[Yu Zhao]

On Mon, Aug 22, 2022 at 4:28 PM Dongli Zhang <dongli.zhang@oracle.com> wrote:
>
> Hi Yu, Robin and Christoph,
>
> The mips kernel panic because the SWIOTLB buffer is adjusted to a very small
> value (< 1MB, or < 512-slot), so that the swiotlb panic on purpose.
>
> software IO TLB: SWIOTLB bounce buffer size adjusted to 0MB
> software IO TLB: area num 1.
> Kernel panic - not syncing: swiotlb_init_remap: nslabs = 128 too small
>
>
> From mips code, the 'swiotlbsize' is set to PAGE_SIZE initially. It is always
> PAGE_SIZE unless it is used by CONFIG_PCI or CONFIG_USB_OHCI_HCD_PLATFORM.
>
> Finally, the swiotlb panic on purpose.
>
> 189 void __init plat_swiotlb_setup(void)
> 190 {
> ... ...
> 211         swiotlbsize = PAGE_SIZE;
> 212
> 213 #ifdef CONFIG_PCI
> 214         /*
> 215          * For OCTEON_DMA_BAR_TYPE_SMALL, size the iotlb at 1/4 memory
> 216          * size to a maximum of 64MB
> 217          */
> 218         if (OCTEON_IS_MODEL(OCTEON_CN31XX)
> 219             || OCTEON_IS_MODEL(OCTEON_CN38XX_PASS2)) {
> 220                 swiotlbsize = addr_size / 4;
> 221                 if (swiotlbsize > 64 * (1<<20))
> 222                         swiotlbsize = 64 * (1<<20);
> 223         } else if (max_addr > 0xf0000000ul) {
> 224                 /*
> 225                  * Otherwise only allocate a big iotlb if there is
> 226                  * memory past the BAR1 hole.
> 227                  */
> 228                 swiotlbsize = 64 * (1<<20);
> 229         }
> 230 #endif
> 231 #ifdef CONFIG_USB_OHCI_HCD_PLATFORM
> 232         /* OCTEON II ohci is only 32-bit. */
> 233         if (OCTEON_IS_OCTEON2() && max_addr >= 0x100000000ul)
> 234                 swiotlbsize = 64 * (1<<20);
> 235 #endif
> 236
> 237         swiotlb_adjust_size(swiotlbsize);
> 238         swiotlb_init(true, SWIOTLB_VERBOSE);
> 239 }
>
>
> Here are some thoughts. Would you mind suggesting which is the right way to go?
>
> 1. Will the PAGE_SIZE swiotlb be used by mips when it is only PAGE_SIZE? If it
> is not used, why not disable swiotlb completely in the code?
>
> 2. The swiotlb panic on purpose when it is less then 1MB. Should we remove that
> limitation?
>
> 3. ... or explicitly declare the limitation that: "swiotlb should be at least
> 1MB, otherwise please do not use it"?
>
>
> The reason I add the panic on purpose is for below case:
>
> The user's kernel is configured with very small swiotlb buffer. As a result, the
> device driver may work abnormally.

Which driver? This sounds like that driver is broken, and we should
fix that driver.

> As a result, the issue is reported to a
> specific driver's developers, who spend some time to confirm it is swiotlb
> issue.

Is this a fact or a hypothetical proposition?

> Suppose those developers are not familiar with IOMMU/swiotlb, it takes
> times until the root cause is identified.

Sorry but you are making quite a few assumptions in a series claimed
to be "swiotlb: some cleanup" -- I personally expect cleanup patches
not to have any runtime side effects.

> If we panic earlier, the issue will be reported to IOMMU/swiotlb developer.

Ok, I think we should at least revert this patch, if not the entire series.

> This
> is also to sync with the remap failure logic in swiotlb (used by xen).

We can have it back in after we have better understood how it
interacts with different archs/drivers, or better yet when the needs
arise, if they arise at all.

Thanks.

Re: [PATCH v1 4/4] swiotlb: panic if nslabs is too small

[Dongli Zhang]

Hi Yu,

On 8/22/22 4:10 PM, Yu Zhao wrote:
> On Mon, Aug 22, 2022 at 4:28 PM Dongli Zhang <dongli.zhang@oracle.com> wrote:
>>
>> Hi Yu, Robin and Christoph,
>>
>> The mips kernel panic because the SWIOTLB buffer is adjusted to a very small
>> value (< 1MB, or < 512-slot), so that the swiotlb panic on purpose.
>>
>> software IO TLB: SWIOTLB bounce buffer size adjusted to 0MB
>> software IO TLB: area num 1.
>> Kernel panic - not syncing: swiotlb_init_remap: nslabs = 128 too small
>>
>>
>> From mips code, the 'swiotlbsize' is set to PAGE_SIZE initially. It is always
>> PAGE_SIZE unless it is used by CONFIG_PCI or CONFIG_USB_OHCI_HCD_PLATFORM.
>>
>> Finally, the swiotlb panic on purpose.
>>
>> 189 void __init plat_swiotlb_setup(void)
>> 190 {
>> ... ...
>> 211         swiotlbsize = PAGE_SIZE;
>> 212
>> 213 #ifdef CONFIG_PCI
>> 214         /*
>> 215          * For OCTEON_DMA_BAR_TYPE_SMALL, size the iotlb at 1/4 memory
>> 216          * size to a maximum of 64MB
>> 217          */
>> 218         if (OCTEON_IS_MODEL(OCTEON_CN31XX)
>> 219             || OCTEON_IS_MODEL(OCTEON_CN38XX_PASS2)) {
>> 220                 swiotlbsize = addr_size / 4;
>> 221                 if (swiotlbsize > 64 * (1<<20))
>> 222                         swiotlbsize = 64 * (1<<20);
>> 223         } else if (max_addr > 0xf0000000ul) {
>> 224                 /*
>> 225                  * Otherwise only allocate a big iotlb if there is
>> 226                  * memory past the BAR1 hole.
>> 227                  */
>> 228                 swiotlbsize = 64 * (1<<20);
>> 229         }
>> 230 #endif
>> 231 #ifdef CONFIG_USB_OHCI_HCD_PLATFORM
>> 232         /* OCTEON II ohci is only 32-bit. */
>> 233         if (OCTEON_IS_OCTEON2() && max_addr >= 0x100000000ul)
>> 234                 swiotlbsize = 64 * (1<<20);
>> 235 #endif
>> 236
>> 237         swiotlb_adjust_size(swiotlbsize);
>> 238         swiotlb_init(true, SWIOTLB_VERBOSE);
>> 239 }
>>
>>
>> Here are some thoughts. Would you mind suggesting which is the right way to go?
>>
>> 1. Will the PAGE_SIZE swiotlb be used by mips when it is only PAGE_SIZE? If it
>> is not used, why not disable swiotlb completely in the code?
>>
>> 2. The swiotlb panic on purpose when it is less then 1MB. Should we remove that
>> limitation?
>>
>> 3. ... or explicitly declare the limitation that: "swiotlb should be at least
>> 1MB, otherwise please do not use it"?
>>
>>
>> The reason I add the panic on purpose is for below case:
>>
>> The user's kernel is configured with very small swiotlb buffer. As a result, the
>> device driver may work abnormally.
> 
> Which driver? This sounds like that driver is broken, and we should
> fix that driver.

Any components that may use swiotlb. The driver is not broken.

The kernel is configured with very few swiotlb slots and obviously many drivers
will not be happy with it.

In the mips case, it is equivalent to swiotlb=2.

> 
>> As a result, the issue is reported to a
>> specific driver's developers, who spend some time to confirm it is swiotlb
>> issue.
> 
> Is this a fact or a hypothetical proposition?

I never encounter this in reality myself.

I always encounter the case that "swiotlb: No low mem" so that many drivers
cannot work well (because swiotlb is even not allocated). The OS hangs (and
reasons unknown to bug filer), e.g., the below:

https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1983625

> 
>> Suppose those developers are not familiar with IOMMU/swiotlb, it takes
>> times until the root cause is identified.
> 
> Sorry but you are making quite a few assumptions in a series claimed
> to be "swiotlb: some cleanup" -- I personally expect cleanup patches
> not to have any runtime side effects.

I regarded this as cleanup because swiotlb may panic on purpose in the same
function in a different case (if statement) when the remap function is not able
to map > 1MB memory.

This patch is to sync with that part: line 353-354.

 349         if (remap && remap(tlb, nslabs) < 0) {
 350                 memblock_free(tlb, PAGE_ALIGN(bytes));
 351
 352                 nslabs = ALIGN(nslabs >> 1, IO_TLB_SEGSIZE);
 353                 if (nslabs < IO_TLB_MIN_SLABS)
 354                         panic("%s: Failed to remap %zu bytes\n",
 355                               __func__, bytes);
 356                 goto retry;
 357         }

> 
>> If we panic earlier, the issue will be reported to IOMMU/swiotlb developer.
> 
> Ok, I think we should at least revert this patch, if not the entire series.
> 
>> This
>> is also to sync with the remap failure logic in swiotlb (used by xen).
> 
> We can have it back in after we have better understood how it
> interacts with different archs/drivers, or better yet when the needs
> arise, if they arise at all.

We have already understood everything related to swiotlb. It is good to me to
revert the patch.

The questions are:

1. Is there any case that the OS may use < 1MB swiotlb buffer? E.g., the mips
only needs PAGE_SIZE buffer for DMA?

According to git log, the arch/mips/cavium-octeon/dma-octeon.c uses "swiotlb=2"
(e.g., 4KB PAGE_SIZE) dating back to 2010.

2. Should we panic pro-actively if the swiotlb user is allocating < 1MB buffer
and no one may use < 1MB buffer.

It is good to me to revert the patch. I will leave the decision to Christoph
whether to revert this patch.

Thank you very much!

Dongli Zhang