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
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 <[email protected]>
---
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
The 'swiotlb_force' is removed since commit c6af2aa9ffc9 ("swiotlb: make
the swiotlb_init interface more useful").
Signed-off-by: Dongli Zhang <[email protected]>
---
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
Both swiotlb_init_remap() and swiotlb_init() have return type void.
Signed-off-by: Dongli Zhang <[email protected]>
---
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
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 <[email protected]>
> ---
> 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;
> }
>
>
Thanks,
I've applied all 4 to the dma-mapping tree for Linux 5.20.
> 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 <[email protected]>
> ---
> 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)
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 <[email protected]>
>> ---
>> 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)
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.
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.
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.
Hi Yu,
On 8/22/22 4:10 PM, Yu Zhao wrote:
> On Mon, Aug 22, 2022 at 4:28 PM Dongli Zhang <[email protected]> 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
On Mon, Aug 22, 2022 at 4:28 PM Dongli Zhang <[email protected]> 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.