This reverts commit 0bf28fc40d89b1a3e00d1b79473bad4e9ca20ad1.
Reasons:
1. new panic()s shouldn't be added [1].
2. It does no "cleanup" but breaks MIPS [2].
[1]: https://lore.kernel.org/r/CAHk-=wit-DmhMfQErY29JSPjFgebx_Ld+pnerc4J2Ag990WwAA@mail.gmail.com/
[2]: https://lore.kernel.org/r/[email protected]/
Fixes: 0bf28fc40d89b ("swiotlb: panic if nslabs is too small")
Signed-off-by: Yu Zhao <[email protected]>
---
kernel/dma/swiotlb.c | 7 +------
1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
index c5a9190b218f..b3ede72eba5d 100644
--- a/kernel/dma/swiotlb.c
+++ b/kernel/dma/swiotlb.c
@@ -325,10 +325,6 @@ void __init swiotlb_init_remap(bool addressing_limit, unsigned int flags,
if (!default_nareas)
swiotlb_adjust_nareas(num_possible_cpus());
- nslabs = default_nslabs;
- 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
@@ -341,8 +337,7 @@ 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 %zu bytes tlb structure\n",
- __func__, bytes);
+ pr_warn("%s: failed to allocate tlb structure\n", __func__);
return;
}
base-commit: c40e8341e3b3bb27e3a65b06b5b454626234c4f0
--
2.37.2.672.g94769d06f0-goog
Hi Yu,
url: https://github.com/intel-lab-lkp/linux/commits/Yu-Zhao/Revert-swiotlb-panic-if-nslabs-is-too-small/20220830-073123
base: c40e8341e3b3bb27e3a65b06b5b454626234c4f0
config: x86_64-randconfig-m001 (https://download.01.org/0day-ci/archive/20220831/[email protected]/config)
compiler: gcc-11 (Debian 11.3.0-5) 11.3.0
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <[email protected]>
Reported-by: Dan Carpenter <[email protected]>
smatch warnings:
kernel/dma/swiotlb.c:334 swiotlb_init_remap() error: uninitialized symbol 'nslabs'.
vim +/nslabs +334 kernel/dma/swiotlb.c
7374153d294eb5 kernel/dma/swiotlb.c Christoph Hellwig 2022-03-14 307 void __init swiotlb_init_remap(bool addressing_limit, unsigned int flags,
7374153d294eb5 kernel/dma/swiotlb.c Christoph Hellwig 2022-03-14 308 int (*remap)(void *tlb, unsigned long nslabs))
^^^^^^
abbceff7d7a884 lib/swiotlb.c FUJITA Tomonori 2010-05-10 309 {
6424e31b1c050a kernel/dma/swiotlb.c Christoph Hellwig 2022-03-15 310 struct io_tlb_mem *mem = &io_tlb_default_mem;
20347fca71a387 kernel/dma/swiotlb.c Tianyu Lan 2022-07-08 311 unsigned long nslabs;
^^^^^^
Merge issue? How does this compile?
a5e891321a2196 kernel/dma/swiotlb.c Christoph Hellwig 2022-05-11 312 size_t alloc_size;
7374153d294eb5 kernel/dma/swiotlb.c Christoph Hellwig 2022-03-14 313 size_t bytes;
2d29960af0bee8 kernel/dma/swiotlb.c Christoph Hellwig 2021-03-18 314 void *tlb;
abbceff7d7a884 lib/swiotlb.c FUJITA Tomonori 2010-05-10 315
c6af2aa9ffc976 kernel/dma/swiotlb.c Christoph Hellwig 2022-03-29 316 if (!addressing_limit && !swiotlb_force_bounce)
c6af2aa9ffc976 kernel/dma/swiotlb.c Christoph Hellwig 2022-03-29 317 return;
c6af2aa9ffc976 kernel/dma/swiotlb.c Christoph Hellwig 2022-03-29 318 if (swiotlb_force_disable)
2726bf3ff2520d kernel/dma/swiotlb.c Florian Fainelli 2021-03-22 319 return;
2726bf3ff2520d kernel/dma/swiotlb.c Florian Fainelli 2021-03-22 320
20347fca71a387 kernel/dma/swiotlb.c Tianyu Lan 2022-07-08 321 /*
20347fca71a387 kernel/dma/swiotlb.c Tianyu Lan 2022-07-08 322 * default_nslabs maybe changed when adjust area number.
20347fca71a387 kernel/dma/swiotlb.c Tianyu Lan 2022-07-08 323 * So allocate bounce buffer after adjusting area number.
20347fca71a387 kernel/dma/swiotlb.c Tianyu Lan 2022-07-08 324 */
20347fca71a387 kernel/dma/swiotlb.c Tianyu Lan 2022-07-08 325 if (!default_nareas)
20347fca71a387 kernel/dma/swiotlb.c Tianyu Lan 2022-07-08 326 swiotlb_adjust_nareas(num_possible_cpus());
20347fca71a387 kernel/dma/swiotlb.c Tianyu Lan 2022-07-08 327
8ba2ed1be90fc2 kernel/dma/swiotlb.c Christoph Hellwig 2022-02-28 328 /*
8ba2ed1be90fc2 kernel/dma/swiotlb.c Christoph Hellwig 2022-02-28 329 * By default allocate the bounce buffer memory from low memory, but
8ba2ed1be90fc2 kernel/dma/swiotlb.c Christoph Hellwig 2022-02-28 330 * allow to pick a location everywhere for hypervisors with guest
8ba2ed1be90fc2 kernel/dma/swiotlb.c Christoph Hellwig 2022-02-28 331 * memory encryption.
8ba2ed1be90fc2 kernel/dma/swiotlb.c Christoph Hellwig 2022-02-28 332 */
7374153d294eb5 kernel/dma/swiotlb.c Christoph Hellwig 2022-03-14 333 retry:
a5e891321a2196 kernel/dma/swiotlb.c Christoph Hellwig 2022-05-11 @334 bytes = PAGE_ALIGN(nslabs << IO_TLB_SHIFT);
^^^^^^
8ba2ed1be90fc2 kernel/dma/swiotlb.c Christoph Hellwig 2022-02-28 335 if (flags & SWIOTLB_ANY)
8ba2ed1be90fc2 kernel/dma/swiotlb.c Christoph Hellwig 2022-02-28 336 tlb = memblock_alloc(bytes, PAGE_SIZE);
8ba2ed1be90fc2 kernel/dma/swiotlb.c Christoph Hellwig 2022-02-28 337 else
2d29960af0bee8 kernel/dma/swiotlb.c Christoph Hellwig 2021-03-18 338 tlb = memblock_alloc_low(bytes, PAGE_SIZE);
1521c607cabe7c kernel/dma/swiotlb.c Christoph Hellwig 2022-05-11 339 if (!tlb) {
b253fbc6b9640a kernel/dma/swiotlb.c Yu Zhao 2022-08-29 340 pr_warn("%s: failed to allocate tlb structure\n", __func__);
1521c607cabe7c kernel/dma/swiotlb.c Christoph Hellwig 2022-05-11 341 return;
1521c607cabe7c kernel/dma/swiotlb.c Christoph Hellwig 2022-05-11 342 }
6424e31b1c050a kernel/dma/swiotlb.c Christoph Hellwig 2022-03-15 343
7374153d294eb5 kernel/dma/swiotlb.c Christoph Hellwig 2022-03-14 344 if (remap && remap(tlb, nslabs) < 0) {
7374153d294eb5 kernel/dma/swiotlb.c Christoph Hellwig 2022-03-14 345 memblock_free(tlb, PAGE_ALIGN(bytes));
7374153d294eb5 kernel/dma/swiotlb.c Christoph Hellwig 2022-03-14 346
7374153d294eb5 kernel/dma/swiotlb.c Christoph Hellwig 2022-03-14 347 nslabs = ALIGN(nslabs >> 1, IO_TLB_SEGSIZE);
7374153d294eb5 kernel/dma/swiotlb.c Christoph Hellwig 2022-03-14 348 if (nslabs < IO_TLB_MIN_SLABS)
7374153d294eb5 kernel/dma/swiotlb.c Christoph Hellwig 2022-03-14 349 panic("%s: Failed to remap %zu bytes\n",
7374153d294eb5 kernel/dma/swiotlb.c Christoph Hellwig 2022-03-14 350 __func__, bytes);
7374153d294eb5 kernel/dma/swiotlb.c Christoph Hellwig 2022-03-14 351 goto retry;
7374153d294eb5 kernel/dma/swiotlb.c Christoph Hellwig 2022-03-14 352 }
abbceff7d7a884 lib/swiotlb.c FUJITA Tomonori 2010-05-10 353
a5e891321a2196 kernel/dma/swiotlb.c Christoph Hellwig 2022-05-11 354 alloc_size = PAGE_ALIGN(array_size(sizeof(*mem->slots), nslabs));
6424e31b1c050a kernel/dma/swiotlb.c Christoph Hellwig 2022-03-15 355 mem->slots = memblock_alloc(alloc_size, PAGE_SIZE);
6424e31b1c050a kernel/dma/swiotlb.c Christoph Hellwig 2022-03-15 356 if (!mem->slots)
6424e31b1c050a kernel/dma/swiotlb.c Christoph Hellwig 2022-03-15 357 panic("%s: Failed to allocate %zu bytes align=0x%lx\n",
6424e31b1c050a kernel/dma/swiotlb.c Christoph Hellwig 2022-03-15 358 __func__, alloc_size, PAGE_SIZE);
6424e31b1c050a kernel/dma/swiotlb.c Christoph Hellwig 2022-03-15 359
72311809031217 kernel/dma/swiotlb.c Tianyu Lan 2022-07-21 360 mem->areas = memblock_alloc(array_size(sizeof(struct io_tlb_area),
72311809031217 kernel/dma/swiotlb.c Tianyu Lan 2022-07-21 361 default_nareas), SMP_CACHE_BYTES);
20347fca71a387 kernel/dma/swiotlb.c Tianyu Lan 2022-07-08 362 if (!mem->areas)
20347fca71a387 kernel/dma/swiotlb.c Tianyu Lan 2022-07-08 363 panic("%s: Failed to allocate mem->areas.\n", __func__);
20347fca71a387 kernel/dma/swiotlb.c Tianyu Lan 2022-07-08 364
20347fca71a387 kernel/dma/swiotlb.c Tianyu Lan 2022-07-08 365 swiotlb_init_io_tlb_mem(mem, __pa(tlb), nslabs, flags, false,
20347fca71a387 kernel/dma/swiotlb.c Tianyu Lan 2022-07-08 366 default_nareas);
6424e31b1c050a kernel/dma/swiotlb.c Christoph Hellwig 2022-03-15 367
6424e31b1c050a kernel/dma/swiotlb.c Christoph Hellwig 2022-03-15 368 if (flags & SWIOTLB_VERBOSE)
6424e31b1c050a kernel/dma/swiotlb.c Christoph Hellwig 2022-03-15 369 swiotlb_print_info();
^1da177e4c3f41 arch/ia64/lib/swiotlb.c Linus Torvalds 2005-04-16 370 }
--
0-DAY CI Kernel Test Service
https://01.org/lkp
This reverts commit 0bf28fc40d89b1a3e00d1b79473bad4e9ca20ad1.
Reasons:
1. new panic()s shouldn't be added [1].
2. It does no "cleanup" but breaks MIPS [2].
v2: properly solved the conflict [3] with
commit 20347fca71a38 ("swiotlb: split up the global swiotlb lock")
Reported-by: kernel test robot <[email protected]>
Reported-by: Dan Carpenter <[email protected]>
[1] https://lore.kernel.org/r/CAHk-=wit-DmhMfQErY29JSPjFgebx_Ld+pnerc4J2Ag990WwAA@mail.gmail.com/
[2] https://lore.kernel.org/r/[email protected]/
[3] https://lore.kernel.org/r/[email protected]/
Fixes: 0bf28fc40d89b ("swiotlb: panic if nslabs is too small")
Signed-off-by: Yu Zhao <[email protected]>
---
kernel/dma/swiotlb.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
index c5a9190b218f..dd8863987e0c 100644
--- a/kernel/dma/swiotlb.c
+++ b/kernel/dma/swiotlb.c
@@ -326,9 +326,6 @@ void __init swiotlb_init_remap(bool addressing_limit, unsigned int flags,
swiotlb_adjust_nareas(num_possible_cpus());
nslabs = default_nslabs;
- 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
@@ -341,8 +338,7 @@ 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 %zu bytes tlb structure\n",
- __func__, bytes);
+ pr_warn("%s: failed to allocate tlb structure\n", __func__);
return;
}
base-commit: dcf8e5633e2e69ad60b730ab5905608b756a032f
--
2.37.2.672.g94769d06f0-goog
On Wed, Aug 31, 2022 at 12:01 AM Dan Carpenter <[email protected]> wrote:
>
> Hi Yu,
>
> url: https://github.com/intel-lab-lkp/linux/commits/Yu-Zhao/Revert-swiotlb-panic-if-nslabs-is-too-small/20220830-073123
> base: c40e8341e3b3bb27e3a65b06b5b454626234c4f0
> config: x86_64-randconfig-m001 (https://download.01.org/0day-ci/archive/20220831/[email protected]/config)
> compiler: gcc-11 (Debian 11.3.0-5) 11.3.0
>
> If you fix the issue, kindly add following tag where applicable
> Reported-by: kernel test robot <[email protected]>
> Reported-by: Dan Carpenter <[email protected]>
>
> smatch warnings:
> kernel/dma/swiotlb.c:334 swiotlb_init_remap() error: uninitialized symbol 'nslabs'.
>
> vim +/nslabs +334 kernel/dma/swiotlb.c
>
> 7374153d294eb5 kernel/dma/swiotlb.c Christoph Hellwig 2022-03-14 307 void __init swiotlb_init_remap(bool addressing_limit, unsigned int flags,
> 7374153d294eb5 kernel/dma/swiotlb.c Christoph Hellwig 2022-03-14 308 int (*remap)(void *tlb, unsigned long nslabs))
> ^^^^^^
>
> abbceff7d7a884 lib/swiotlb.c FUJITA Tomonori 2010-05-10 309 {
> 6424e31b1c050a kernel/dma/swiotlb.c Christoph Hellwig 2022-03-15 310 struct io_tlb_mem *mem = &io_tlb_default_mem;
> 20347fca71a387 kernel/dma/swiotlb.c Tianyu Lan 2022-07-08 311 unsigned long nslabs;
> ^^^^^^
> Merge issue? How does this compile?
Sorry, I deleted an extra line while resolving the conflict.
Resending the patch.
Hi Yu,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on c40e8341e3b3bb27e3a65b06b5b454626234c4f0]
url: https://github.com/intel-lab-lkp/linux/commits/Yu-Zhao/Revert-swiotlb-panic-if-nslabs-is-too-small/20220830-073123
base: c40e8341e3b3bb27e3a65b06b5b454626234c4f0
config: s390-randconfig-r044-20220830
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project 4c106463674a6f6de085d9a91510c385be4a620d)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install s390 cross compiling tool for clang build
# apt-get install binutils-s390x-linux-gnu
# https://github.com/intel-lab-lkp/linux/commit/b253fbc6b9640aeebffc73d72bf0177c2a1ede1e
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Yu-Zhao/Revert-swiotlb-panic-if-nslabs-is-too-small/20220830-073123
git checkout b253fbc6b9640aeebffc73d72bf0177c2a1ede1e
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=s390 SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
In file included from kernel/dma/swiotlb.c:27:
In file included from include/linux/dma-direct.h:9:
In file included from include/linux/dma-mapping.h:10:
In file included from include/linux/scatterlist.h:9:
In file included from arch/s390/include/asm/io.h:75:
include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __raw_readb(PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
~~~~~~~~~~ ^
include/uapi/linux/byteorder/big_endian.h:37:59: note: expanded from macro '__le16_to_cpu'
#define __le16_to_cpu(x) __swab16((__force __u16)(__le16)(x))
^
include/uapi/linux/swab.h:102:54: note: expanded from macro '__swab16'
#define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
^
In file included from kernel/dma/swiotlb.c:27:
In file included from include/linux/dma-direct.h:9:
In file included from include/linux/dma-mapping.h:10:
In file included from include/linux/scatterlist.h:9:
In file included from arch/s390/include/asm/io.h:75:
include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
~~~~~~~~~~ ^
include/uapi/linux/byteorder/big_endian.h:35:59: note: expanded from macro '__le32_to_cpu'
#define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
^
include/uapi/linux/swab.h:115:54: note: expanded from macro '__swab32'
#define __swab32(x) (__u32)__builtin_bswap32((__u32)(x))
^
In file included from kernel/dma/swiotlb.c:27:
In file included from include/linux/dma-direct.h:9:
In file included from include/linux/dma-mapping.h:10:
In file included from include/linux/scatterlist.h:9:
In file included from arch/s390/include/asm/io.h:75:
include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writeb(value, PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:692:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
readsb(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:700:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
readsw(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:708:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
readsl(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:717:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
writesb(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:726:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
writesw(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:735:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
writesl(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
>> kernel/dma/swiotlb.c:318:6: warning: variable 'nslabs' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
if (swiotlb_force_disable)
^~~~~~~~~~~~~~~~~~~~~
kernel/dma/swiotlb.c:334:21: note: uninitialized use occurs here
bytes = PAGE_ALIGN(nslabs << IO_TLB_SHIFT);
^~~~~~
include/linux/mm.h:223:32: note: expanded from macro 'PAGE_ALIGN'
#define PAGE_ALIGN(addr) ALIGN(addr, PAGE_SIZE)
^~~~
include/linux/align.h:8:38: note: expanded from macro 'ALIGN'
#define ALIGN(x, a) __ALIGN_KERNEL((x), (a))
^
include/uapi/linux/const.h:31:51: note: expanded from macro '__ALIGN_KERNEL'
#define __ALIGN_KERNEL(x, a) __ALIGN_KERNEL_MASK(x, (typeof(x))(a) - 1)
^
include/uapi/linux/const.h:32:41: note: expanded from macro '__ALIGN_KERNEL_MASK'
#define __ALIGN_KERNEL_MASK(x, mask) (((x) + (mask)) & ~(mask))
^
kernel/dma/swiotlb.c:318:2: note: remove the 'if' if its condition is always true
if (swiotlb_force_disable)
^~~~~~~~~~~~~~~~~~~~~~~~~~
kernel/dma/swiotlb.c:311:22: note: initialize the variable 'nslabs' to silence this warning
unsigned long nslabs;
^
= 0
13 warnings generated.
vim +318 kernel/dma/swiotlb.c
0a65579cdd28be kernel/dma/swiotlb.c Claire Chang 2021-06-19 302
abbceff7d7a884 lib/swiotlb.c FUJITA Tomonori 2010-05-10 303 /*
abbceff7d7a884 lib/swiotlb.c FUJITA Tomonori 2010-05-10 304 * Statically reserve bounce buffer space and initialize bounce buffer data
abbceff7d7a884 lib/swiotlb.c FUJITA Tomonori 2010-05-10 305 * structures for the software IO TLB used to implement the DMA API.
abbceff7d7a884 lib/swiotlb.c FUJITA Tomonori 2010-05-10 306 */
7374153d294eb5 kernel/dma/swiotlb.c Christoph Hellwig 2022-03-14 307 void __init swiotlb_init_remap(bool addressing_limit, unsigned int flags,
7374153d294eb5 kernel/dma/swiotlb.c Christoph Hellwig 2022-03-14 308 int (*remap)(void *tlb, unsigned long nslabs))
abbceff7d7a884 lib/swiotlb.c FUJITA Tomonori 2010-05-10 309 {
6424e31b1c050a kernel/dma/swiotlb.c Christoph Hellwig 2022-03-15 310 struct io_tlb_mem *mem = &io_tlb_default_mem;
20347fca71a387 kernel/dma/swiotlb.c Tianyu Lan 2022-07-08 311 unsigned long nslabs;
a5e891321a2196 kernel/dma/swiotlb.c Christoph Hellwig 2022-05-11 312 size_t alloc_size;
7374153d294eb5 kernel/dma/swiotlb.c Christoph Hellwig 2022-03-14 313 size_t bytes;
2d29960af0bee8 kernel/dma/swiotlb.c Christoph Hellwig 2021-03-18 314 void *tlb;
abbceff7d7a884 lib/swiotlb.c FUJITA Tomonori 2010-05-10 315
c6af2aa9ffc976 kernel/dma/swiotlb.c Christoph Hellwig 2022-03-29 316 if (!addressing_limit && !swiotlb_force_bounce)
c6af2aa9ffc976 kernel/dma/swiotlb.c Christoph Hellwig 2022-03-29 317 return;
c6af2aa9ffc976 kernel/dma/swiotlb.c Christoph Hellwig 2022-03-29 @318 if (swiotlb_force_disable)
2726bf3ff2520d kernel/dma/swiotlb.c Florian Fainelli 2021-03-22 319 return;
2726bf3ff2520d kernel/dma/swiotlb.c Florian Fainelli 2021-03-22 320
20347fca71a387 kernel/dma/swiotlb.c Tianyu Lan 2022-07-08 321 /*
20347fca71a387 kernel/dma/swiotlb.c Tianyu Lan 2022-07-08 322 * default_nslabs maybe changed when adjust area number.
20347fca71a387 kernel/dma/swiotlb.c Tianyu Lan 2022-07-08 323 * So allocate bounce buffer after adjusting area number.
20347fca71a387 kernel/dma/swiotlb.c Tianyu Lan 2022-07-08 324 */
20347fca71a387 kernel/dma/swiotlb.c Tianyu Lan 2022-07-08 325 if (!default_nareas)
20347fca71a387 kernel/dma/swiotlb.c Tianyu Lan 2022-07-08 326 swiotlb_adjust_nareas(num_possible_cpus());
20347fca71a387 kernel/dma/swiotlb.c Tianyu Lan 2022-07-08 327
8ba2ed1be90fc2 kernel/dma/swiotlb.c Christoph Hellwig 2022-02-28 328 /*
8ba2ed1be90fc2 kernel/dma/swiotlb.c Christoph Hellwig 2022-02-28 329 * By default allocate the bounce buffer memory from low memory, but
8ba2ed1be90fc2 kernel/dma/swiotlb.c Christoph Hellwig 2022-02-28 330 * allow to pick a location everywhere for hypervisors with guest
8ba2ed1be90fc2 kernel/dma/swiotlb.c Christoph Hellwig 2022-02-28 331 * memory encryption.
8ba2ed1be90fc2 kernel/dma/swiotlb.c Christoph Hellwig 2022-02-28 332 */
7374153d294eb5 kernel/dma/swiotlb.c Christoph Hellwig 2022-03-14 333 retry:
a5e891321a2196 kernel/dma/swiotlb.c Christoph Hellwig 2022-05-11 334 bytes = PAGE_ALIGN(nslabs << IO_TLB_SHIFT);
8ba2ed1be90fc2 kernel/dma/swiotlb.c Christoph Hellwig 2022-02-28 335 if (flags & SWIOTLB_ANY)
8ba2ed1be90fc2 kernel/dma/swiotlb.c Christoph Hellwig 2022-02-28 336 tlb = memblock_alloc(bytes, PAGE_SIZE);
8ba2ed1be90fc2 kernel/dma/swiotlb.c Christoph Hellwig 2022-02-28 337 else
2d29960af0bee8 kernel/dma/swiotlb.c Christoph Hellwig 2021-03-18 338 tlb = memblock_alloc_low(bytes, PAGE_SIZE);
1521c607cabe7c kernel/dma/swiotlb.c Christoph Hellwig 2022-05-11 339 if (!tlb) {
b253fbc6b9640a kernel/dma/swiotlb.c Yu Zhao 2022-08-29 340 pr_warn("%s: failed to allocate tlb structure\n", __func__);
1521c607cabe7c kernel/dma/swiotlb.c Christoph Hellwig 2022-05-11 341 return;
1521c607cabe7c kernel/dma/swiotlb.c Christoph Hellwig 2022-05-11 342 }
6424e31b1c050a kernel/dma/swiotlb.c Christoph Hellwig 2022-03-15 343
7374153d294eb5 kernel/dma/swiotlb.c Christoph Hellwig 2022-03-14 344 if (remap && remap(tlb, nslabs) < 0) {
7374153d294eb5 kernel/dma/swiotlb.c Christoph Hellwig 2022-03-14 345 memblock_free(tlb, PAGE_ALIGN(bytes));
7374153d294eb5 kernel/dma/swiotlb.c Christoph Hellwig 2022-03-14 346
7374153d294eb5 kernel/dma/swiotlb.c Christoph Hellwig 2022-03-14 347 nslabs = ALIGN(nslabs >> 1, IO_TLB_SEGSIZE);
7374153d294eb5 kernel/dma/swiotlb.c Christoph Hellwig 2022-03-14 348 if (nslabs < IO_TLB_MIN_SLABS)
7374153d294eb5 kernel/dma/swiotlb.c Christoph Hellwig 2022-03-14 349 panic("%s: Failed to remap %zu bytes\n",
7374153d294eb5 kernel/dma/swiotlb.c Christoph Hellwig 2022-03-14 350 __func__, bytes);
7374153d294eb5 kernel/dma/swiotlb.c Christoph Hellwig 2022-03-14 351 goto retry;
7374153d294eb5 kernel/dma/swiotlb.c Christoph Hellwig 2022-03-14 352 }
abbceff7d7a884 lib/swiotlb.c FUJITA Tomonori 2010-05-10 353
a5e891321a2196 kernel/dma/swiotlb.c Christoph Hellwig 2022-05-11 354 alloc_size = PAGE_ALIGN(array_size(sizeof(*mem->slots), nslabs));
6424e31b1c050a kernel/dma/swiotlb.c Christoph Hellwig 2022-03-15 355 mem->slots = memblock_alloc(alloc_size, PAGE_SIZE);
6424e31b1c050a kernel/dma/swiotlb.c Christoph Hellwig 2022-03-15 356 if (!mem->slots)
6424e31b1c050a kernel/dma/swiotlb.c Christoph Hellwig 2022-03-15 357 panic("%s: Failed to allocate %zu bytes align=0x%lx\n",
6424e31b1c050a kernel/dma/swiotlb.c Christoph Hellwig 2022-03-15 358 __func__, alloc_size, PAGE_SIZE);
6424e31b1c050a kernel/dma/swiotlb.c Christoph Hellwig 2022-03-15 359
72311809031217 kernel/dma/swiotlb.c Tianyu Lan 2022-07-21 360 mem->areas = memblock_alloc(array_size(sizeof(struct io_tlb_area),
72311809031217 kernel/dma/swiotlb.c Tianyu Lan 2022-07-21 361 default_nareas), SMP_CACHE_BYTES);
20347fca71a387 kernel/dma/swiotlb.c Tianyu Lan 2022-07-08 362 if (!mem->areas)
20347fca71a387 kernel/dma/swiotlb.c Tianyu Lan 2022-07-08 363 panic("%s: Failed to allocate mem->areas.\n", __func__);
20347fca71a387 kernel/dma/swiotlb.c Tianyu Lan 2022-07-08 364
20347fca71a387 kernel/dma/swiotlb.c Tianyu Lan 2022-07-08 365 swiotlb_init_io_tlb_mem(mem, __pa(tlb), nslabs, flags, false,
20347fca71a387 kernel/dma/swiotlb.c Tianyu Lan 2022-07-08 366 default_nareas);
6424e31b1c050a kernel/dma/swiotlb.c Christoph Hellwig 2022-03-15 367
6424e31b1c050a kernel/dma/swiotlb.c Christoph Hellwig 2022-03-15 368 if (flags & SWIOTLB_VERBOSE)
6424e31b1c050a kernel/dma/swiotlb.c Christoph Hellwig 2022-03-15 369 swiotlb_print_info();
^1da177e4c3f41 arch/ia64/lib/swiotlb.c Linus Torvalds 2005-04-16 370 }
^1da177e4c3f41 arch/ia64/lib/swiotlb.c Linus Torvalds 2005-04-16 371
--
0-DAY CI Kernel Test Service
https://01.org/lkp
Greeting,
FYI, we noticed the following commit (built with gcc-11):
commit: b253fbc6b9640aeebffc73d72bf0177c2a1ede1e ("[PATCH] Revert "swiotlb: panic if nslabs is too small"")
url: https://github.com/intel-lab-lkp/linux/commits/Yu-Zhao/Revert-swiotlb-panic-if-nslabs-is-too-small/20220830-073123
patch link: https://lore.kernel.org/lkml/[email protected]
in testcase: boot
on test machine: qemu-system-x86_64 -enable-kvm -cpu SandyBridge -smp 2 -m 16G
caused below changes (please refer to attached dmesg/kmsg for entire log/backtrace):
If you fix the issue, kindly add following tag
Reported-by: kernel test robot <[email protected]>
[ 5.008908][ T1] ------------[ cut here ]------------
[ 5.009988][ T1] e1000 0000:00:03.0: DMA addr 0x000000010036f8c0+1522 overflow (mask ffffffff, bus limit 0).
[ 5.011794][ T1] WARNING: CPU: 0 PID: 1 at kernel/dma/direct.h:103 dma_direct_map_page+0x1a5/0x1d0
[ 5.013544][ T1] Modules linked in:
[ 5.014406][ T1] CPU: 0 PID: 1 Comm: swapper Not tainted 6.0.0-rc2-00055-gb253fbc6b964 #1
[ 5.015977][ T1] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.0-debian-1.16.0-4 04/01/2014
[ 5.017844][ T1] RIP: 0010:dma_direct_map_page+0x1a5/0x1d0
[ 5.018945][ T1] Code: 4d 8b 34 24 4c 89 e7 e8 f9 4d 26 00 49 89 d9 4d 89 e8 4c 89 f2 55 48 89 c6 48 c7 c7 b8 b4 ae 81 48 8d 4c 24 10 e8 57 5f 43 00 <0f> 0b 58 48 c7 c0 ff ff ff ff e9 e1 fe ff ff 48 89 e8 48 2b 41 18
[ 5.022279][ T1] RSP: 0000:ffffc90000013ca8 EFLAGS: 00010286
[ 5.023430][ T1] RAX: 0000000000000000 RBX: 00000000ffffffff RCX: ffffffff81fe1bc8
[ 5.024926][ T1] RDX: 0000000000000000 RSI: 0000000000000003 RDI: 0000000000000003
[ 5.026458][ T1] RBP: 0000000000000000 R08: 0000000000000003 R09: 0000000000000019
[ 5.027939][ T1] R10: 33303a30303a3030 R11: 3030203030303165 R12: ffff8881001480b0
[ 5.029449][ T1] R13: 00000000000005f2 R14: ffff88810023b140 R15: ffff8881001480b0
[ 5.030894][ T1] FS: 0000000000000000(0000) GS:ffffffff81c53000(0000) knlGS:0000000000000000
[ 5.032596][ T1] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 5.033901][ T1] CR2: ffff88843ffff000 CR3: 0000000001c1e000 CR4: 00000000000406f0
[ 5.035415][ T1] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 5.036850][ T1] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[ 5.038427][ T1] Call Trace:
[ 5.039111][ T1] <TASK>
[ 5.039749][ T1] e1000_alloc_rx_buffers+0x141/0x3a0
[ 5.040721][ T1] e1000_configure+0x10b/0x120
[ 5.041601][ T1] e1000_open+0xb8/0x190
[ 5.042413][ T1] __dev_open+0xd3/0x180
[ 5.043284][ T1] __dev_change_flags+0x198/0x210
[ 5.044254][ T1] ? __dev_notify_flags+0x53/0xe0
[ 5.045181][ T1] dev_change_flags+0x1c/0x60
[ 5.046084][ T1] ic_open_devs+0xe1/0x2a0
[ 5.046956][ T1] ip_auto_config+0x155/0x4c2
[ 5.047875][ T1] ? root_nfs_parse_addr+0x9d/0x9d
[ 5.048795][ T1] do_one_initcall+0x46/0x190
[ 5.049684][ T1] do_initcalls+0x11a/0x13b
[ 5.050542][ T1] kernel_init_freeable+0xa3/0xd4
[ 5.051458][ T1] ? rest_init+0xa0/0xa0
[ 5.052313][ T1] kernel_init+0x11/0x110
[ 5.053188][ T1] ret_from_fork+0x22/0x30
[ 5.054073][ T1] </TASK>
[ 5.054697][ T1] ---[ end trace 0000000000000000 ]---
To reproduce:
# build kernel
cd linux
cp config-6.0.0-rc2-00055-gb253fbc6b964 .config
make HOSTCC=gcc-11 CC=gcc-11 ARCH=x86_64 olddefconfig prepare modules_prepare bzImage modules
make HOSTCC=gcc-11 CC=gcc-11 ARCH=x86_64 INSTALL_MOD_PATH=<mod-install-dir> modules_install
cd <mod-install-dir>
find lib/ | cpio -o -H newc --quiet | gzip > modules.cgz
git clone https://github.com/intel/lkp-tests.git
cd lkp-tests
bin/lkp qemu -k <bzImage> -m modules.cgz job-script # job-script is attached in this email
# if come across any failure that blocks the test,
# please remove ~/.lkp and /lkp dir to run from a clean state.
--
0-DAY CI Kernel Test Service
https://01.org/lkp
Hi Yu,
As we discussed in the past, the swiotlb panic on purpose because the
mips/cavium-octeon/dma-octeon.c requests to allocate only PAGE_SIZE swiotlb
buffer. This is smaller than IO_TLB_MIN_SLABS.
The below comments mentioned that IO_TLB_MIN_SLABS is the "Minimum IO TLB size
to bother booting with".
56 /*
57 * Minimum IO TLB size to bother booting with. Systems with mainly
58 * 64bit capable cards will only lightly use the swiotlb. If we can't
59 * allocate a contiguous 1MB, we're probably in trouble anyway.
60 */
61 #define IO_TLB_MIN_SLABS ((1<<20) >> IO_TLB_SHIFT)
The arm may create swiotlb conditionally. That is, the swiotlb is not
initialized if (1) CONFIG_ARM_LPAE is not set (line 273), or (2) max_pfn <=
arm_dma_pfn_limit (line 274).
arch/arm/mm/init.c
271 void __init mem_init(void)
272 {
273 #ifdef CONFIG_ARM_LPAE
274 swiotlb_init(max_pfn > arm_dma_pfn_limit, SWIOTLB_VERBOSE);
275 #endif
276
277 set_max_mapnr(pfn_to_page(max_pfn) - mem_map);
On x86, the swiotlb is not initialized if the memory is small (> MAX_DMA32_PFN,
at line 47), or the secure memory is not required.
44 static void __init pci_swiotlb_detect(void)
45 {
46 /* don't initialize swiotlb if iommu=off (no_iommu=1) */
47 if (!no_iommu && max_possible_pfn > MAX_DMA32_PFN)
48 x86_swiotlb_enable = true;
49
50 /*
51 * Set swiotlb to 1 so that bounce buffers are allocated and used for
52 * devices that can't support DMA to encrypted memory.
53 */
54 if (cc_platform_has(CC_ATTR_HOST_MEM_ENCRYPT))
55 x86_swiotlb_enable = true;
56
57 /*
58 * Guest with guest memory encryption currently perform all DMA through
59 * bounce buffers as the hypervisor can't access arbitrary VM memory
60 * that is not explicitly shared with it.
61 */
62 if (cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT)) {
63 x86_swiotlb_enable = true;
64 x86_swiotlb_flags |= SWIOTLB_FORCE;
65 }
66 }
Regardless whether the current patch will be reverted, unless there is specific
reason (e.g., those PAGE_SIZE will be used), I do not think it is a good idea to
allocate <IO_TLB_MIN_SLABS swiotlb buffer. I will wait for the suggestion from
the swiotlb maintainer.
Since I do not have a mips environment, I am not able to test if the below makes
any trouble in your situation at arch/mips/cavium-octeon/dma-octeon.c.
@@ -234,6 +234,8 @@ void __init plat_swiotlb_setup(void)
swiotlbsize = 64 * (1<<20);
#endif
- swiotlb_adjust_size(swiotlbsize);
- swiotlb_init(true, SWIOTLB_VERBOSE);
+ if (swiotlbsize != PAGE_SIZE) {
+ swiotlb_adjust_size(swiotlbsize);
+ swiotlb_init(true, SWIOTLB_VERBOSE);
+ }
}
Thank you very much!
Dongli Zhang
On 8/30/22 11:38 PM, Yu Zhao wrote:
> This reverts commit 0bf28fc40d89b1a3e00d1b79473bad4e9ca20ad1.
>
> Reasons:
> 1. new panic()s shouldn't be added [1].
> 2. It does no "cleanup" but breaks MIPS [2].
>
> v2: properly solved the conflict [3] with
> commit 20347fca71a38 ("swiotlb: split up the global swiotlb lock")
> Reported-by: kernel test robot <[email protected]>
> Reported-by: Dan Carpenter <[email protected]>
>
> [1] https://urldefense.com/v3/__https://lore.kernel.org/r/CAHk-=wit-DmhMfQErY29JSPjFgebx_Ld*[email protected]/__;Kw!!ACWV5N9M2RV99hQ!PPVATbHVDT6TZ4sqoj5G6vfAJGPAEz-Lmp9njTsM2PPYPQqCP6aq5RF8FDmrXDlSzxJmTUUSgOW3yjKDtg$
> [2] https://urldefense.com/v3/__https://lore.kernel.org/r/[email protected]/__;!!ACWV5N9M2RV99hQ!PPVATbHVDT6TZ4sqoj5G6vfAJGPAEz-Lmp9njTsM2PPYPQqCP6aq5RF8FDmrXDlSzxJmTUUSgOXQRsYjKQ$
> [3] https://urldefense.com/v3/__https://lore.kernel.org/r/[email protected]/__;!!ACWV5N9M2RV99hQ!PPVATbHVDT6TZ4sqoj5G6vfAJGPAEz-Lmp9njTsM2PPYPQqCP6aq5RF8FDmrXDlSzxJmTUUSgOW_tjcVMA$
>
> Fixes: 0bf28fc40d89b ("swiotlb: panic if nslabs is too small")
> Signed-off-by: Yu Zhao <[email protected]>
> ---
> kernel/dma/swiotlb.c | 6 +-----
> 1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
> index c5a9190b218f..dd8863987e0c 100644
> --- a/kernel/dma/swiotlb.c
> +++ b/kernel/dma/swiotlb.c
> @@ -326,9 +326,6 @@ void __init swiotlb_init_remap(bool addressing_limit, unsigned int flags,
> swiotlb_adjust_nareas(num_possible_cpus());
>
> nslabs = default_nslabs;
> - 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
> @@ -341,8 +338,7 @@ 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 %zu bytes tlb structure\n",
> - __func__, bytes);
> + pr_warn("%s: failed to allocate tlb structure\n", __func__);
> return;
> }
>
>
> base-commit: dcf8e5633e2e69ad60b730ab5905608b756a032f
>
On Wed, Aug 31, 2022 at 7:42 AM kernel test robot <[email protected]> wrote:
>
> Greeting,
>
> FYI, we noticed the following commit (built with gcc-11):
>
> commit: b253fbc6b9640aeebffc73d72bf0177c2a1ede1e ("[PATCH] Revert "swiotlb: panic if nslabs is too small"")
> url: https://github.com/intel-lab-lkp/linux/commits/Yu-Zhao/Revert-swiotlb-panic-if-nslabs-is-too-small/20220830-073123
> patch link: https://lore.kernel.org/lkml/[email protected]
Thanks.
It was reported and followed up on yesterday [1].
https://lore.kernel.org/r/[email protected]/
On Wed, Aug 31, 2022 at 4:20 PM Dongli Zhang <[email protected]> wrote:
>
> Hi Yu,
>
> As we discussed in the past, the swiotlb panic on purpose
We should not panic() at all, especially on a platform that has been
working well since at least 4.14.
Did you check out this link I previously shared with you?
https://lore.kernel.org/r/CAHk-=wit-DmhMfQErY29JSPjFgebx_Ld+pnerc4J2Ag990WwAA@mail.gmail.com/
> because the
> mips/cavium-octeon/dma-octeon.c requests to allocate only PAGE_SIZE swiotlb
> buffer.
What's wrong with PAGE_SIZE swiotlb?
> This is smaller than IO_TLB_MIN_SLABS.
IO_TLB_MIN_SLABS is an arbitrary number, and it's inherited from IA64.
So does the comment below. What exactly is the rationale behind it?
> The below comments mentioned that IO_TLB_MIN_SLABS is the "Minimum IO TLB size
> to bother booting with".
>
> 56 /*
> 57 * Minimum IO TLB size to bother booting with. Systems with mainly
> 58 * 64bit capable cards will only lightly use the swiotlb. If we can't
> 59 * allocate a contiguous 1MB, we're probably in trouble anyway.
> 60 */
> 61 #define IO_TLB_MIN_SLABS ((1<<20) >> IO_TLB_SHIFT)
>
>
> The arm may create swiotlb conditionally. That is, the swiotlb is not
> initialized if (1) CONFIG_ARM_LPAE is not set (line 273), or (2) max_pfn <=
> arm_dma_pfn_limit (line 274).
>
> arch/arm/mm/init.c
>
> 271 void __init mem_init(void)
> 272 {
> 273 #ifdef CONFIG_ARM_LPAE
> 274 swiotlb_init(max_pfn > arm_dma_pfn_limit, SWIOTLB_VERBOSE);
> 275 #endif
> 276
> 277 set_max_mapnr(pfn_to_page(max_pfn) - mem_map);
>
>
> On x86, the swiotlb is not initialized if the memory is small (> MAX_DMA32_PFN,
> at line 47), or the secure memory is not required.
>
> 44 static void __init pci_swiotlb_detect(void)
> 45 {
> 46 /* don't initialize swiotlb if iommu=off (no_iommu=1) */
> 47 if (!no_iommu && max_possible_pfn > MAX_DMA32_PFN)
> 48 x86_swiotlb_enable = true;
> 49
> 50 /*
> 51 * Set swiotlb to 1 so that bounce buffers are allocated and used for
> 52 * devices that can't support DMA to encrypted memory.
> 53 */
> 54 if (cc_platform_has(CC_ATTR_HOST_MEM_ENCRYPT))
> 55 x86_swiotlb_enable = true;
> 56
> 57 /*
> 58 * Guest with guest memory encryption currently perform all DMA through
> 59 * bounce buffers as the hypervisor can't access arbitrary VM memory
> 60 * that is not explicitly shared with it.
> 61 */
> 62 if (cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT)) {
> 63 x86_swiotlb_enable = true;
> 64 x86_swiotlb_flags |= SWIOTLB_FORCE;
> 65 }
> 66 }
Thanks for the code snippets. But I failed to see a point.
> Regardless whether the current patch will be reverted, unless there is specific
> reason (e.g., those PAGE_SIZE will be used), I do not think it is a good idea to
> allocate <IO_TLB_MIN_SLABS swiotlb buffer.
For what specific reason? My current understanding is that you want to
be future/fool-proof. If so, then you got the priority wrong. We need
to make sure currently working systems continue to work first, then
future/fool-proof.
> I will wait for the suggestion from
> the swiotlb maintainer.
Chris is on vacation. I sure can wait.
But it sounds like you are unsure about what to do. If so, it's not
what you claimed "we have already understood everything related to
swiotlb" previously.
> Since I do not have a mips environment, I am not able to test if the below makes
> any trouble in your situation at arch/mips/cavium-octeon/dma-octeon.c.
>
> @@ -234,6 +234,8 @@ void __init plat_swiotlb_setup(void)
> swiotlbsize = 64 * (1<<20);
> #endif
>
> - swiotlb_adjust_size(swiotlbsize);
> - swiotlb_init(true, SWIOTLB_VERBOSE);
> + if (swiotlbsize != PAGE_SIZE) {
> + swiotlb_adjust_size(swiotlbsize);
> + swiotlb_init(true, SWIOTLB_VERBOSE);
> + }
> }
Please ask the MIPS/Octeon maintainers to check this first.
Hi Yu,
On 8/31/22 5:24 PM, Yu Zhao wrote:
> On Wed, Aug 31, 2022 at 4:20 PM Dongli Zhang <[email protected]> wrote:
>>
>> Hi Yu,
>>
>> As we discussed in the past, the swiotlb panic on purpose
>
> We should not panic() at all, especially on a platform that has been
> working well since at least 4.14.
>
> Did you check out this link I previously shared with you?
> https://urldefense.com/v3/__https://lore.kernel.org/r/CAHk-=wit-DmhMfQErY29JSPjFgebx_Ld*[email protected]/__;Kw!!ACWV5N9M2RV99hQ!PXzSLurBv7VqxI1451TV4zO3_-BYj4grk-HYBsXzSnA6nZcXaBzdsQ-rF2DAqlICSRPMt-efYv_Uu2A2CQ$
Thanks for sharing! I used to read that in the past. To be honest I am still
confused on when to use BUG/panic and when to not, as I still see many usage in
some patches.
Just about swiotlb, it may panic in many cases during boot, e.g.,:
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1955655
>
>> because the
>> mips/cavium-octeon/dma-octeon.c requests to allocate only PAGE_SIZE swiotlb
>> buffer.
>
> What's wrong with PAGE_SIZE swiotlb?
The swiotlb is like a software IOMMU. When the device supports only <=32-bit DMA
while the IOMMU is not available, it may do the below for each DMA operation:
1. The swiotlb inits and pre-allocates many 32-bit memory during boot.
2. For a DMA read/write, if the page is 64-bit but 32-bit DMA is required, the
kernel swaps the 64-bit DMA page with the pre-allocated swiotlb 32-bit memory.
3. After the DMA is complete, the 32-bit memory is swapped back to swiotlb.
The size of pre-allocated swiotlb is fixed during boot. The DMA will fail later
if the pre-allocated swiotlb is not enough.
That's why I think the objective to set swiotlb=PAGE_SIZE is just to minimize
the swiotlb pre-allocation when it is not required. I do not see a reason to
pre-allocate a PAGE_SIZE for DMA.
There was a discussion between Robin and Christoph whether it is a good idea to
disable swiotlb by allocating a small chunk of memory.
https://lore.kernel.org/all/[email protected]/
>
>> This is smaller than IO_TLB_MIN_SLABS.
>
> IO_TLB_MIN_SLABS is an arbitrary number, and it's inherited from IA64.
> So does the comment below. What exactly is the rationale behind it?
To be honest I do not have an answer as different people may have different
preferences. Some people may prefer to increase IO_TLB_MIN_SLABS, while some may
prefer to decrease.
That value of IO_TLB_MIN_SLABS was there since I started working on swiotlb.
>
>> The below comments mentioned that IO_TLB_MIN_SLABS is the "Minimum IO TLB size
>> to bother booting with".
>>
>> 56 /*
>> 57 * Minimum IO TLB size to bother booting with. Systems with mainly
>> 58 * 64bit capable cards will only lightly use the swiotlb. If we can't
>> 59 * allocate a contiguous 1MB, we're probably in trouble anyway.
>> 60 */
>> 61 #define IO_TLB_MIN_SLABS ((1<<20) >> IO_TLB_SHIFT)
>>
>>
>> The arm may create swiotlb conditionally. That is, the swiotlb is not
>> initialized if (1) CONFIG_ARM_LPAE is not set (line 273), or (2) max_pfn <=
>> arm_dma_pfn_limit (line 274).
>>
>> arch/arm/mm/init.c
>>
>> 271 void __init mem_init(void)
>> 272 {
>> 273 #ifdef CONFIG_ARM_LPAE
>> 274 swiotlb_init(max_pfn > arm_dma_pfn_limit, SWIOTLB_VERBOSE);
>> 275 #endif
>> 276
>> 277 set_max_mapnr(pfn_to_page(max_pfn) - mem_map);
>>
>>
>> On x86, the swiotlb is not initialized if the memory is small (> MAX_DMA32_PFN,
>> at line 47), or the secure memory is not required.
>>
>> 44 static void __init pci_swiotlb_detect(void)
>> 45 {
>> 46 /* don't initialize swiotlb if iommu=off (no_iommu=1) */
>> 47 if (!no_iommu && max_possible_pfn > MAX_DMA32_PFN)
>> 48 x86_swiotlb_enable = true;
>> 49
>> 50 /*
>> 51 * Set swiotlb to 1 so that bounce buffers are allocated and used for
>> 52 * devices that can't support DMA to encrypted memory.
>> 53 */
>> 54 if (cc_platform_has(CC_ATTR_HOST_MEM_ENCRYPT))
>> 55 x86_swiotlb_enable = true;
>> 56
>> 57 /*
>> 58 * Guest with guest memory encryption currently perform all DMA through
>> 59 * bounce buffers as the hypervisor can't access arbitrary VM memory
>> 60 * that is not explicitly shared with it.
>> 61 */
>> 62 if (cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT)) {
>> 63 x86_swiotlb_enable = true;
>> 64 x86_swiotlb_flags |= SWIOTLB_FORCE;
>> 65 }
>> 66 }
>
> Thanks for the code snippets. But I failed to see a point.
>
>> Regardless whether the current patch will be reverted, unless there is specific
>> reason (e.g., those PAGE_SIZE will be used), I do not think it is a good idea to
>> allocate <IO_TLB_MIN_SLABS swiotlb buffer.
>
> For what specific reason? My current understanding is that you want to
> be future/fool-proof. If so, then you got the priority wrong. We need
> to make sure currently working systems continue to work first, then
> future/fool-proof.
My objective was not to prove which option was correct/incorrect. I was just
going to share my understanding of the situation to people on this list, so that
a discussion can continue.
But I do agree with your point on 'priority'.
>
>> I will wait for the suggestion from
>> the swiotlb maintainer.
>
> Chris is on vacation. I sure can wait.
>
> But it sounds like you are unsure about what to do. If so, it's not
> what you claimed "we have already understood everything related to
> swiotlb" previously.
The swiotlb is a component used by many arch (e.g., x86, arm/arm64, mips and
xen) ... while I believe I understand the swiotlb well, I do not understand what
other components are expecting from the swiotlb.
That's why my objective was to trigger the discussion but not to prove my
statement was correct.
The swiotlb is still evolving due to it is more widely used recently (e.g., AMD
SEV). The discussion helps people avoid introducing more issue because there are
many components that may use swiotlb now or in the future.
About what to do, I think it is to either (1) revert the patch or (2) avoid
initializing swiotlb if it is not going to be used. However, I think my thoughts
do not count that much, and that's why I would wait for the suggestion from more
experienced maintainers.
Thank you very much!
Dongli Zhang
>
>> Since I do not have a mips environment, I am not able to test if the below makes
>> any trouble in your situation at arch/mips/cavium-octeon/dma-octeon.c.
>>
>> @@ -234,6 +234,8 @@ void __init plat_swiotlb_setup(void)
>> swiotlbsize = 64 * (1<<20);
>> #endif
>>
>> - swiotlb_adjust_size(swiotlbsize);
>> - swiotlb_init(true, SWIOTLB_VERBOSE);
>> + if (swiotlbsize != PAGE_SIZE) {
>> + swiotlb_adjust_size(swiotlbsize);
>> + swiotlb_init(true, SWIOTLB_VERBOSE);
>> + }
>> }
>
> Please ask the MIPS/Octeon maintainers to check this first.
>
On 2022-09-01 03:18, Dongli Zhang wrote:
> Hi Yu,
>
> On 8/31/22 5:24 PM, Yu Zhao wrote:
>> On Wed, Aug 31, 2022 at 4:20 PM Dongli Zhang <[email protected]> wrote:
>>>
>>> Hi Yu,
>>>
>>> As we discussed in the past, the swiotlb panic on purpose
>>
>> We should not panic() at all, especially on a platform that has been
>> working well since at least 4.14.
>>
>> Did you check out this link I previously shared with you?
>> https://urldefense.com/v3/__https://lore.kernel.org/r/CAHk-=wit-DmhMfQErY29JSPjFgebx_Ld*[email protected]/__;Kw!!ACWV5N9M2RV99hQ!PXzSLurBv7VqxI1451TV4zO3_-BYj4grk-HYBsXzSnA6nZcXaBzdsQ-rF2DAqlICSRPMt-efYv_Uu2A2CQ$
>
> Thanks for sharing! I used to read that in the past. To be honest I am still
> confused on when to use BUG/panic and when to not, as I still see many usage in
> some patches.
>
> Just about swiotlb, it may panic in many cases during boot, e.g.,:
>
> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1955655
That's really a different thing, but frankly that panic is also bogus
anyway - there is no good reason to have different behaviour for failing
to allocate a buffer slot because the buffer is full vs. failing to
allocate a buffer slot because there is no buffer. If we can fail
gracefully some of the time we should fail gracefully all of the time.
Yes, there's a slight difference in that one case has a chance of
succeeding if retried in future while the other definitely doesn't, but
it's not SWIOTLB's place to decide that the entire system is terminally
unusable just because some device can't make a DMA mapping.
Similarly for the other panics at init time, which seem to have
originated from a mechanical refactoring of the memblock API with the
expectation of preserving the existing behaviour at the time. Those have
then just been moved around without anyone thinking to question why
they're there, and if memblock *does* now return usable error
information, why don't we start handling that error properly like we do
in other init paths?
Really there is no reason to panic anywhere in SWIOTLB. This is old
code, things have moved on over the last 20 years, and we can and should
do much better. I'll add this to my list of things to look at for
cleanup once I find a bit of free time, unless anyone else fancies
taking it on.
Thanks,
Robin.
Hi Robin,
On 9/1/22 5:24 AM, Robin Murphy wrote:
> On 2022-09-01 03:18, Dongli Zhang wrote:
>> Hi Yu,
>>
>> On 8/31/22 5:24 PM, Yu Zhao wrote:
>>> On Wed, Aug 31, 2022 at 4:20 PM Dongli Zhang <[email protected]> wrote:
>>>>
>>>> Hi Yu,
>>>>
>>>> As we discussed in the past, the swiotlb panic on purpose
>>>
>>> We should not panic() at all, especially on a platform that has been
>>> working well since at least 4.14.
>>>
>>> Did you check out this link I previously shared with you?
>>> https://urldefense.com/v3/__https://lore.kernel.org/r/CAHk-=wit-DmhMfQErY29JSPjFgebx_Ld*[email protected]/__;Kw!!ACWV5N9M2RV99hQ!PXzSLurBv7VqxI1451TV4zO3_-BYj4grk-HYBsXzSnA6nZcXaBzdsQ-rF2DAqlICSRPMt-efYv_Uu2A2CQ$
>>>
>>
>> Thanks for sharing! I used to read that in the past. To be honest I am still
>> confused on when to use BUG/panic and when to not, as I still see many usage in
>> some patches.
>>
>> Just about swiotlb, it may panic in many cases during boot, e.g.,:
>>
>> https://urldefense.com/v3/__https://bugs.launchpad.net/ubuntu/*source/linux/*bug/1955655__;Kys!!ACWV5N9M2RV99hQ!MO4S9r0PisrW6Z-kZqUitAMbuGNMX6aceQd5VApqXllP6f1jaPCyL9x50um7chsn2uGL_pNqdBzTjZK5GebeKrI$
>
>
> That's really a different thing, but frankly that panic is also bogus anyway -
> there is no good reason to have different behaviour for failing to allocate a
> buffer slot because the buffer is full vs. failing to allocate a buffer slot
> because there is no buffer. If we can fail gracefully some of the time we should
> fail gracefully all of the time. Yes, there's a slight difference in that one
Thank you very much for the feedback!
Currently the swiotlb remap logic is more gracefully to handle failure, to
re-try with smaller nslabs (line 352), but will still panic at the end if the
value is reduced to smaller than IO_TLB_MIN_SLABS.
337 retry:
338 bytes = PAGE_ALIGN(nslabs << IO_TLB_SHIFT);
339 if (flags & SWIOTLB_ANY)
340 tlb = memblock_alloc(bytes, PAGE_SIZE);
341 else
342 tlb = memblock_alloc_low(bytes, PAGE_SIZE);
343 if (!tlb) {
344 pr_warn("%s: Failed to allocate %zu bytes tlb structure\n",
345 __func__, bytes);
346 return;
347 }
348
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 }
> case has a chance of succeeding if retried in future while the other definitely
> doesn't, but it's not SWIOTLB's place to decide that the entire system is
> terminally unusable just because some device can't make a DMA mapping.
Currently the swiotlb panic at line 735 on purpose if swiotlb initialization is
failed, but when any DMA requires a swiotlb mapped slot.
724 phys_addr_t swiotlb_tbl_map_single(struct device *dev, phys_addr_t orig_addr,
725 size_t mapping_size, size_t alloc_size,
726 unsigned int alloc_align_mask, enum dma_data_direction dir,
727 unsigned long attrs)
728 {
729 struct io_tlb_mem *mem = dev->dma_io_tlb_mem;
730 unsigned int offset = swiotlb_align_offset(dev, orig_addr);
731 unsigned int i;
732 int index;
733 phys_addr_t tlb_addr;
734
735 if (!mem || !mem->nslabs)
736 panic("Can not allocate SWIOTLB buffer earlier and can't
now provide you with the DMA bounce buffer");
I agree it is a very interesting point whether it is a good idea to leave the
decision to swiotlb for a panic. One thing to take care is the driver should
gracefully handle the error returned by swiotlb. Otherwise, there may be IO hang
(e.g., hung_task) instead of an IO failure.
>
> Similarly for the other panics at init time, which seem to have originated from
> a mechanical refactoring of the memblock API with the expectation of preserving
> the existing behaviour at the time. Those have then just been moved around
> without anyone thinking to question why they're there, and if memblock *does*
> now return usable error information, why don't we start handling that error
> properly like we do in other init paths?
>
> Really there is no reason to panic anywhere in SWIOTLB. This is old code, things
> have moved on over the last 20 years, and we can and should do much better. I'll
> add this to my list of things to look at for cleanup once I find a bit of free
> time, unless anyone else fancies taking it on.
Thank you very much for the feedback. Looking forward to how this can be improved!
Dongli Zhang