next/master bisection: baseline.login on rk3288-rock2-square

DISCUSSION Guillaume Tucker 6 participants 28 messages Feb 4, 2021 – Feb 6, 2021 1 message not in the archive Arm (32-bit)

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Guillaume Tucker

author

2021-02-04 08:44 UTC #1 lore raw

Hi Ard,

Please see the bisection report below about a boot failure on rk3288 with next-20210203. It was also bisected on imx6q-var-dt6customboard with next-20210202.

Reports aren't automatically sent to the public while we're trialing new bisection features on kernelci.org but this one looks valid.

The kernel is most likely crashing very early on, so there's nothing in the logs. Please let us know if you need some help with debugging or trying a fix on these platforms.

Best wishes,
Guillaume

Quoted 195 lines · KernelCI bot

* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * *
* This automated bisection report was sent to you on the basis  *
* that you may be involved with the breaking commit it has      *
* found.  No manual investigation has been done to verify it,   *
* and the root cause of the problem may be somewhere else.      *
*                                                               *
* If you do send a fix, please include this trailer:            *
*   Reported-by: "kernelci.org bot" <[email protected]>          *
*                                                               *
* Hope this helps!                                              *
* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * *
next/master bisection: baseline.login on rk3288-rock2-square
Summary:
  Start:      58b6c0e507b7 Add linux-next specific files for 20210203
  Plain log:  https://storage.kernelci.org/next/master/next-20210203/arm/multi_v7_defconfig/clang-11/lab-collabora/baseline-rk3288-rock2-square.txt
  HTML log:   https://storage.kernelci.org/next/master/next-20210203/arm/multi_v7_defconfig/clang-11/lab-collabora/baseline-rk3288-rock2-square.html
  Result:     5a29552af92d ARM: 9052/1: decompressor: cover BSS in cache clean and reorder with MMU disable on v7
Checks:
revert: PASS
verify: PASS
Parameters:
  Tree:       next
  URL:        https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
  Branch:     master
  Target:     rk3288-rock2-square
  CPU arch:   arm
  Lab:        lab-collabora
  Compiler:   clang-11
  Config:     multi_v7_defconfig
  Test case:  baseline.login
Breaking commit found:
-------------------------------------------------------------------------------
commit 5a29552af92dbd62c2b6fd1cddf7dad1ef7555b2
Author: Ard Biesheuvel <[email protected]>
Date:   Sun Jan 24 18:03:45 2021 +0100
ARM: 9052/1: decompressor: cover BSS in cache clean and reorder with MMU disable on v7
Commit 401b368caaec ("ARM: decompressor: switch to by-VA cache maintenance for v7 cores") replaced the by-set/way cache maintenance in the decompressor with by-VA cache maintenance, which is more appropriate for the task at hand, especially under virtualization on hosts with non-architected system caches that are not affected by by-set/way maintenance at all.
On such systems, that commit inadvertently removed the cache clean and invalidate of all of the guest's memory that is performed by KVM on behalf of the guest after its MMU is disabled (but only if any by-set/way cache maintenance instructions were issued first). This resulted in various erroneous behaviors observed by Russell, all involving the mini-stack used by the core kernel's v7 boot code, and which resides in BSS. It seems intractable to figure out exactly what goes wrong in each of these cases, but some small experiments did suggest that the lack of a cache clean and invalidate *after* disabling the MMU and caches is what triggers the errors, presumably because cachelines are being allocated or reallocated while the first cache clean and invalidate is in progress.
To ensure that no cache lines cover any of the data that is accessed by the booting kernel with the MMU off, include the uncompressed kernel's BSS region in the cache clean operation.
Also, to ensure that no cachelines are allocated while the cache is being cleaned, perform the cache clean operation *after* disabling the MMU and caches when running on v7 or later, by making a tail call to the clean routine from the cache_off routine. This requires passing the VA range to cache_off(), which means some care needs to be taken to preserve R0 and R1 across the call to cache_off().
Since this makes the first cache clean redundant, call it with the range reduced to zero. This only affects v7, as all other versions ignore R0/R1 entirely.
Link https://lore.kernel.org/linux-arm-kernel/[email protected] open here
Fixes 401b368caaec ("ARM: decompressor: switch to by-VA cache maintenance for v7 cores")
Reported-by Russell King <[email protected]>
Signed-off-by Ard Biesheuvel <[email protected]>
Signed-off-by Russell King <[email protected]>
arch/arm/boot/compressed/head.S +22 -6
index d9cce7238a36..5f231b6f0d1a 100644
@@ -609,11 +609,24 @@ not_relocated:	mov	r0, #0
 		mov	r3, r7
 		bl	decompress_kernel
 
+		@
+		@ Perform a cache clean before disabling the MMU entirely.
+		@ In cases where the MMU needs to be disabled first (v7+),
+		@ the clean is performed again by cache_off(), using by-VA
+		@ operations on the range [R0, R1], making this prior call to
+		@ cache_clean_flush() redundant. In other cases, the clean is
+		@ performed by set/way and R0/R1 are ignored.
+		@
+		mov	r0, #0
+		mov	r1, #0
+		bl	cache_clean_flush
+
 		get_inflated_image_size	r1, r2, r3
+		ldr	r2, =_kernel_bss_size
+		add	r1, r1, r2
 
-		mov	r0, r4			@ start of inflated image
-		add	r1, r1, r0		@ end of inflated image
-		bl	cache_clean_flush
+		mov	r0, r4			@ start of decompressed kernel
+		add	r1, r1, r0		@ end of kernel BSS
 		bl	cache_off
 
 #ifdef CONFIG_ARM_VIRT_EXT
@@ -1124,12 +1137,14 @@ proc_types:
  * reading the control register, but ARMv4 does.
  *
  * On exit,
- *  r0, r1, r2, r3, r9, r12 corrupted
+ *  r0, r1, r2, r3, r9, r10, r11, r12 corrupted
  * This routine must preserve:
  *  r4, r7, r8
  */
 		.align	5
 cache_off:	mov	r3, #12			@ cache_off function
+		mov	r10, r0
+		mov	r11, r1
 		b	call_cache_fn
 
 __armv4_mpu_cache_off:
@@ -1176,7 +1191,9 @@ __armv7_mmu_cache_off:
 		mcr	p15, 0, r0, c7, c5, 6	@ invalidate BTC
 		mcr	p15, 0, r0, c7, c10, 4	@ DSB
 		mcr	p15, 0, r0, c7, c5, 4	@ ISB
-		mov	pc, lr
+
+		mov	r0, r10
+		b	__armv7_mmu_cache_flush
 
 /*
  * Clean and flush the cache to maintain consistency.
-------------------------------------------------------------------------------
Git bisection log:
-------------------------------------------------------------------------------
git bisect start
# good: [62c31574cdb770c78f67e7aa6e0b0244ad122901] Merge tag 'imx-fixes-5.11-3' of git://git.kernel.org/pub/scm/linux/kernel/git/shawnguo/linux into arm/fixes git bisect good 62c31574cdb770c78f67e7aa6e0b0244ad122901
# bad: [58b6c0e507b7421b03b2f2a92bddbb8c6fa1b2f6] Add linux-next specific files for 20210203
git bisect bad 58b6c0e507b7421b03b2f2a92bddbb8c6fa1b2f6
# bad: [18c1afa6bb9b6277d20910eb7cdc5eb01d9d87f2] Merge remote-tracking branch 'net-next/master'
git bisect bad 18c1afa6bb9b6277d20910eb7cdc5eb01d9d87f2
# bad: [58d92989a8d24b6aaaabee52624d891b5103e04a] Merge remote-tracking branch 'parisc-hd/for-next'
git bisect bad 58d92989a8d24b6aaaabee52624d891b5103e04a
# bad: [b0b5c935b4dcf824ef30f6ddf719b49f729c2795] Merge remote-tracking branch 'sound-current/for-linus'
git bisect bad b0b5c935b4dcf824ef30f6ddf719b49f729c2795
# good: [d3921cb8be29ce5668c64e23ffdaeec5f8c69399] mm: fix initialization of struct page for holes in memory layout
git bisect good d3921cb8be29ce5668c64e23ffdaeec5f8c69399
# good: [c64396cc36c6e60704ab06c1fb1c4a46179c9120] Merge tag 'locking-urgent-2021-01-28' of git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip
git bisect good c64396cc36c6e60704ab06c1fb1c4a46179c9120
# good: [2ba1c4d1a4b5fb9961452286bdcad502b0c8b78a] Merge tag 'block-5.11-2021-01-29' of git://git.kernel.dk/linux-block
git bisect good 2ba1c4d1a4b5fb9961452286bdcad502b0c8b78a
# good: [88bb507a74ea7d75fa49edd421eaa710a7d80598] Merge tag 'media/v5.11-3' of git://git.kernel.org/pub/scm/linux/kernel/git/mchehab/linux-media
git bisect good 88bb507a74ea7d75fa49edd421eaa710a7d80598
# good: [2e02677e961fd4b96d8cf106b5979e6a3cdb7362] Merge tag 'for_linus' of git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost
git bisect good 2e02677e961fd4b96d8cf106b5979e6a3cdb7362
# bad: [d3aa3465622d6d96645611b331312b773806d1a7] Merge remote-tracking branch 'arm64-fixes/for-next/fixes'
git bisect bad d3aa3465622d6d96645611b331312b773806d1a7
# good: [245a7d47066ac0a266004110bd4d57d0d1329823] scripts: switch some more scripts explicitly to Python 3
git bisect good 245a7d47066ac0a266004110bd4d57d0d1329823
# bad: [199a427c3a3da01c5db4784a75b37251e7befa64] ARM: ensure the signal page contains defined contents
git bisect bad 199a427c3a3da01c5db4784a75b37251e7befa64
# good: [538eea5362a1179dfa7770dd2b6607dc30cc50c6] ARM: 9043/1: tegra: Fix misplaced tegra_uart_config in decompressor
git bisect good 538eea5362a1179dfa7770dd2b6607dc30cc50c6
# bad: [d80cd9abcd942eb217b6c68e5bd0d5c3feb2f956] ARM: decompressor: tidy up register usage
git bisect bad d80cd9abcd942eb217b6c68e5bd0d5c3feb2f956
# bad: [5a29552af92dbd62c2b6fd1cddf7dad1ef7555b2] ARM: 9052/1: decompressor: cover BSS in cache clean and reorder with MMU disable on v7 git bisect bad 5a29552af92dbd62c2b6fd1cddf7dad1ef7555b2
# first bad commit: [5a29552af92dbd62c2b6fd1cddf7dad1ef7555b2] ARM: 9052/1: decompressor: cover BSS in cache clean and reorder with MMU disable on v7
-------------------------------------------------------------------------------
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#6431): https://groups.io/g/kernelci-results/message/6431
Mute This Topic: https://groups.io/mt/80373377/924702
Group Owner: [email protected]
Unsubscribe: https://groups.io/g/kernelci-results/unsub [[email protected]]
-=-=-=-=-=-=-=-=-=-=-=-

Ard Biesheuvel

2021-02-04 09:09 UTC #2 lore raw

Quoted 7 lines · Guillaume Tucker

Hi Ard,
Please see the bisection report below about a boot failure on rk3288 with next-20210203. It was also bisected on imx6q-var-dt6customboard with next-20210202.

Quoted 8 lines

Reports aren't automatically sent to the public while we're trialing new bisection features on kernelci.org but this one looks valid.
The kernel is most likely crashing very early on, so there's nothing in the logs. Please let us know if you need some help with debugging or trying a fix on these platforms.

Thanks for the report.

Mind trying the following fix?

arch/arm/boot/compressed/head.S +4 -2

@@ -617,8 +617,10 @@ not_relocated:     mov     r0, #0
                @ cache_clean_flush() redundant. In other cases, the clean is
                @ performed by set/way and R0/R1 are ignored.
                @
-               mov     r0, #0
-               mov     r1, #0
+               get_inflated_image_size r1, r2, r3
+
+               mov     r0, r4                  @ start of decompressed kernel
+               add     r1, r1, r0              @ end of kernel BSS
                bl      cache_clean_flush
 
                get_inflated_image_size r1, r2, r3

Quoted 187 lines · KernelCI bot

Quoted 186 lines
* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * *
* This automated bisection report was sent to you on the basis  *
* that you may be involved with the breaking commit it has      *
* found.  No manual investigation has been done to verify it,   *
* and the root cause of the problem may be somewhere else.      *
*                                                               *
* If you do send a fix, please include this trailer:            *
*   Reported-by: "kernelci.org bot" <[email protected]>          *
*                                                               *
* Hope this helps!                                              *
* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * *
next/master bisection: baseline.login on rk3288-rock2-square
Summary:
  Start:      58b6c0e507b7 Add linux-next specific files for 20210203
  Plain log:  https://storage.kernelci.org/next/master/next-20210203/arm/multi_v7_defconfig/clang-11/lab-collabora/baseline-rk3288-rock2-square.txt
  HTML log:   https://storage.kernelci.org/next/master/next-20210203/arm/multi_v7_defconfig/clang-11/lab-collabora/baseline-rk3288-rock2-square.html
  Result:     5a29552af92d ARM: 9052/1: decompressor: cover BSS in cache clean and reorder with MMU disable on v7
Checks:
revert: PASS
verify: PASS
Parameters:
  Tree:       next
  URL:        https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
  Branch:     master
  Target:     rk3288-rock2-square
  CPU arch:   arm
  Lab:        lab-collabora
  Compiler:   clang-11
  Config:     multi_v7_defconfig
  Test case:  baseline.login
Breaking commit found:
-------------------------------------------------------------------------------
commit 5a29552af92dbd62c2b6fd1cddf7dad1ef7555b2
Author: Ard Biesheuvel <[email protected]>
Date:   Sun Jan 24 18:03:45 2021 +0100
ARM: 9052/1: decompressor: cover BSS in cache clean and reorder with MMU disable on v7
Commit 401b368caaec ("ARM: decompressor: switch to by-VA cache maintenance for v7 cores") replaced the by-set/way cache maintenance in the decompressor with by-VA cache maintenance, which is more appropriate for the task at hand, especially under virtualization on hosts with non-architected system caches that are not affected by by-set/way maintenance at all.
On such systems, that commit inadvertently removed the cache clean and invalidate of all of the guest's memory that is performed by KVM on behalf of the guest after its MMU is disabled (but only if any by-set/way cache maintenance instructions were issued first). This resulted in various erroneous behaviors observed by Russell, all involving the mini-stack used by the core kernel's v7 boot code, and which resides in BSS. It seems intractable to figure out exactly what goes wrong in each of these cases, but some small experiments did suggest that the lack of a cache clean and invalidate *after* disabling the MMU and caches is what triggers the errors, presumably because cachelines are being allocated or reallocated while the first cache clean and invalidate is in progress.
To ensure that no cache lines cover any of the data that is accessed by the booting kernel with the MMU off, include the uncompressed kernel's BSS region in the cache clean operation.
Also, to ensure that no cachelines are allocated while the cache is being cleaned, perform the cache clean operation *after* disabling the MMU and caches when running on v7 or later, by making a tail call to the clean routine from the cache_off routine. This requires passing the VA range to cache_off(), which means some care needs to be taken to preserve R0 and R1 across the call to cache_off().
Since this makes the first cache clean redundant, call it with the range reduced to zero. This only affects v7, as all other versions ignore R0/R1 entirely.
Link https://lore.kernel.org/linux-arm-kernel/[email protected] open here
Fixes 401b368caaec ("ARM: decompressor: switch to by-VA cache maintenance for v7 cores")
Reported-by Russell King <[email protected]>
Signed-off-by Ard Biesheuvel <[email protected]>
Signed-off-by Russell King <[email protected]>
arch/arm/boot/compressed/head.S +22 -6
index d9cce7238a36..5f231b6f0d1a 100644
@@ -609,11 +609,24 @@ not_relocated:  mov     r0, #0
              mov     r3, r7
              bl      decompress_kernel
 
+             @
+             @ Perform a cache clean before disabling the MMU entirely.
+             @ In cases where the MMU needs to be disabled first (v7+),
+             @ the clean is performed again by cache_off(), using by-VA
+             @ operations on the range [R0, R1], making this prior call to
+             @ cache_clean_flush() redundant. In other cases, the clean is
+             @ performed by set/way and R0/R1 are ignored.
+             @
+             mov     r0, #0
+             mov     r1, #0
+             bl      cache_clean_flush
+
              get_inflated_image_size r1, r2, r3
+             ldr     r2, =_kernel_bss_size
+             add     r1, r1, r2
 
-             mov     r0, r4                  @ start of inflated image
-             add     r1, r1, r0              @ end of inflated image
-             bl      cache_clean_flush
+             mov     r0, r4                  @ start of decompressed kernel
+             add     r1, r1, r0              @ end of kernel BSS
              bl      cache_off
 
 #ifdef CONFIG_ARM_VIRT_EXT
@@ -1124,12 +1137,14 @@ proc_types:
  * reading the control register, but ARMv4 does.
  *
  * On exit,
- *  r0, r1, r2, r3, r9, r12 corrupted
+ *  r0, r1, r2, r3, r9, r10, r11, r12 corrupted
  * This routine must preserve:
  *  r4, r7, r8
  */
              .align  5
 cache_off:   mov     r3, #12                 @ cache_off function
+             mov     r10, r0
+             mov     r11, r1
              b       call_cache_fn
 
 __armv4_mpu_cache_off:
@@ -1176,7 +1191,9 @@ __armv7_mmu_cache_off:
              mcr     p15, 0, r0, c7, c5, 6   @ invalidate BTC
              mcr     p15, 0, r0, c7, c10, 4  @ DSB
              mcr     p15, 0, r0, c7, c5, 4   @ ISB
-             mov     pc, lr
+
+             mov     r0, r10
+             b       __armv7_mmu_cache_flush
 
 /*
  * Clean and flush the cache to maintain consistency.
-------------------------------------------------------------------------------
Git bisection log:
-------------------------------------------------------------------------------
git bisect start
# good: [62c31574cdb770c78f67e7aa6e0b0244ad122901] Merge tag 'imx-fixes-5.11-3' of git://git.kernel.org/pub/scm/linux/kernel/git/shawnguo/linux into arm/fixes git bisect good 62c31574cdb770c78f67e7aa6e0b0244ad122901
# bad: [58b6c0e507b7421b03b2f2a92bddbb8c6fa1b2f6] Add linux-next specific files for 20210203
git bisect bad 58b6c0e507b7421b03b2f2a92bddbb8c6fa1b2f6
# bad: [18c1afa6bb9b6277d20910eb7cdc5eb01d9d87f2] Merge remote-tracking branch 'net-next/master'
git bisect bad 18c1afa6bb9b6277d20910eb7cdc5eb01d9d87f2
# bad: [58d92989a8d24b6aaaabee52624d891b5103e04a] Merge remote-tracking branch 'parisc-hd/for-next'
git bisect bad 58d92989a8d24b6aaaabee52624d891b5103e04a
# bad: [b0b5c935b4dcf824ef30f6ddf719b49f729c2795] Merge remote-tracking branch 'sound-current/for-linus'
git bisect bad b0b5c935b4dcf824ef30f6ddf719b49f729c2795
# good: [d3921cb8be29ce5668c64e23ffdaeec5f8c69399] mm: fix initialization of struct page for holes in memory layout
git bisect good d3921cb8be29ce5668c64e23ffdaeec5f8c69399
# good: [c64396cc36c6e60704ab06c1fb1c4a46179c9120] Merge tag 'locking-urgent-2021-01-28' of git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip
git bisect good c64396cc36c6e60704ab06c1fb1c4a46179c9120
# good: [2ba1c4d1a4b5fb9961452286bdcad502b0c8b78a] Merge tag 'block-5.11-2021-01-29' of git://git.kernel.dk/linux-block
git bisect good 2ba1c4d1a4b5fb9961452286bdcad502b0c8b78a
# good: [88bb507a74ea7d75fa49edd421eaa710a7d80598] Merge tag 'media/v5.11-3' of git://git.kernel.org/pub/scm/linux/kernel/git/mchehab/linux-media
git bisect good 88bb507a74ea7d75fa49edd421eaa710a7d80598
# good: [2e02677e961fd4b96d8cf106b5979e6a3cdb7362] Merge tag 'for_linus' of git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost
git bisect good 2e02677e961fd4b96d8cf106b5979e6a3cdb7362
# bad: [d3aa3465622d6d96645611b331312b773806d1a7] Merge remote-tracking branch 'arm64-fixes/for-next/fixes'
git bisect bad d3aa3465622d6d96645611b331312b773806d1a7
# good: [245a7d47066ac0a266004110bd4d57d0d1329823] scripts: switch some more scripts explicitly to Python 3
git bisect good 245a7d47066ac0a266004110bd4d57d0d1329823
# bad: [199a427c3a3da01c5db4784a75b37251e7befa64] ARM: ensure the signal page contains defined contents
git bisect bad 199a427c3a3da01c5db4784a75b37251e7befa64
# good: [538eea5362a1179dfa7770dd2b6607dc30cc50c6] ARM: 9043/1: tegra: Fix misplaced tegra_uart_config in decompressor
git bisect good 538eea5362a1179dfa7770dd2b6607dc30cc50c6
# bad: [d80cd9abcd942eb217b6c68e5bd0d5c3feb2f956] ARM: decompressor: tidy up register usage
git bisect bad d80cd9abcd942eb217b6c68e5bd0d5c3feb2f956
# bad: [5a29552af92dbd62c2b6fd1cddf7dad1ef7555b2] ARM: 9052/1: decompressor: cover BSS in cache clean and reorder with MMU disable on v7 git bisect bad 5a29552af92dbd62c2b6fd1cddf7dad1ef7555b2
# first bad commit: [5a29552af92dbd62c2b6fd1cddf7dad1ef7555b2] ARM: 9052/1: decompressor: cover BSS in cache clean and reorder with MMU disable on v7
-------------------------------------------------------------------------------

Quoted 10 lines

Quoted 9 lines
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#6431): https://groups.io/g/kernelci-results/message/6431
Mute This Topic: https://groups.io/mt/80373377/924702
Group Owner: [email protected]
Unsubscribe: https://groups.io/g/kernelci-results/unsub [[email protected]]
-=-=-=-=-=-=-=-=-=-=-=-

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Russell King - ARM Linux admin

maintainer

2021-02-04 10:07 UTC #3 lore raw

Quoted 13 lines · Ard Biesheuvel

Quoted 11 lines · Guillaume Tucker
Hi Ard,
Please see the bisection report below about a boot failure on rk3288 with next-20210203. It was also bisected on imx6q-var-dt6customboard with next-20210202.
Reports aren't automatically sent to the public while we're trialing new bisection features on kernelci.org but this one looks valid.

Quoted 6 lines

Quoted 4 lines
The kernel is most likely crashing very early on, so there's nothing in the logs. Please let us know if you need some help with debugging or trying a fix on these platforms.
Thanks for the report.

Ard,

I want to send my fixes branch today which includes your regression fix that caused this regression.

As this is proving difficult to fix, I can only drop your fix from my fixes branch - and given that this seems to be problematical, I'm tempted to revert the original change at this point which should fix both of these regressions - and then we have another go at getting rid of the set/way instructions during the next cycle.

Thoughts?

Signature · 2 lines

RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Ard Biesheuvel

2021-02-04 10:28 UTC #4 lore raw

Quoted 27 lines · Russell King - ARM Linux admin

Quoted 13 lines · Ard Biesheuvel
Quoted 11 lines · Guillaume Tucker
Hi Ard,
Please see the bisection report below about a boot failure on rk3288 with next-20210203. It was also bisected on imx6q-var-dt6customboard with next-20210202.
Reports aren't automatically sent to the public while we're trialing new bisection features on kernelci.org but this one looks valid.
Quoted 6 lines
Quoted 4 lines
The kernel is most likely crashing very early on, so there's nothing in the logs. Please let us know if you need some help with debugging or trying a fix on these platforms.
Thanks for the report.
Ard,
I want to send my fixes branch today which includes your regression fix that caused this regression.

Quoted 8 lines

As this is proving difficult to fix, I can only drop your fix from my fixes branch - and given that this seems to be problematical, I'm tempted to revert the original change at this point which should fix both of these regressions - and then we have another go at getting rid of the set/way instructions during the next cycle.
Thoughts?

Hi Russell,

If Guillaume is willing to do the experiment, and it fixes the issue, it proves that rk3288 is relying on the flush before the MMU is disabled, and so in that case, the fix is trivial, and we can just apply it.

If the experiment fails (which would mean rk3288 does not tolerate the cache maintenance being performed after cache off), it is going to be hairy, and so it will definitely take more time.

So in the latter case (or if Guillaume does not get back to us), I think reverting my queued fix is the only sane option. But in that case, may I suggest that we queue the revert of the original by-VA change for v5.12 so it gets lots of coverage in -next, and allows us an opportunity to come up with a proper fix in the same timeframe, and backport the revert and the subsequent fix as a pair? Otherwise, we'll end up in the situation where v5.10.x until today has by-va, v5.10.x-y has set/way, and v5.10y+ has by-va again. (I don't think we care about anything before that, given that v5.4 predates any of this)

But in the end, I'm happy to go along with whatever works best for you.

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Guillaume Tucker

author

2021-02-04 10:34 UTC #5 lore raw

Quoted 35 lines · Ard Biesheuvel

Quoted 33 lines · Russell King - ARM Linux admin
Quoted 13 lines · Ard Biesheuvel
Quoted 11 lines · Guillaume Tucker
Hi Ard,
Please see the bisection report below about a boot failure on rk3288 with next-20210203. It was also bisected on imx6q-var-dt6customboard with next-20210202.
Reports aren't automatically sent to the public while we're trialing new bisection features on kernelci.org but this one looks valid.
Quoted 6 lines
Quoted 4 lines
The kernel is most likely crashing very early on, so there's nothing in the logs. Please let us know if you need some help with debugging or trying a fix on these platforms.
Thanks for the report.
Ard,
I want to send my fixes branch today which includes your regression fix that caused this regression.
As this is proving difficult to fix, I can only drop your fix from my fixes branch - and given that this seems to be problematical, I'm tempted to revert the original change at this point which should fix both of these regressions - and then we have another go at getting rid of the set/way instructions during the next cycle.

Quoted 6 lines

Quoted 2 lines
Thoughts?
Hi Russell,
If Guillaume is willing to do the experiment, and it fixes the issue,

Yes, I'm running some tests with that fix now and should have some results shortly.

Quoted 18 lines

it proves that rk3288 is relying on the flush before the MMU is disabled, and so in that case, the fix is trivial, and we can just apply it.
If the experiment fails (which would mean rk3288 does not tolerate the cache maintenance being performed after cache off), it is going to be hairy, and so it will definitely take more time.
So in the latter case (or if Guillaume does not get back to us), I think reverting my queued fix is the only sane option. But in that case, may I suggest that we queue the revert of the original by-VA change for v5.12 so it gets lots of coverage in -next, and allows us an opportunity to come up with a proper fix in the same timeframe, and backport the revert and the subsequent fix as a pair? Otherwise, we'll end up in the situation where v5.10.x until today has by-va, v5.10.x-y has set/way, and v5.10y+ has by-va again. (I don't think we care about anything before that, given that v5.4 predates any of this)

Quoted 1 line

But in the end, I'm happy to go along with whatever works best for you.

Thanks,
Guillaume

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Guillaume Tucker

author

2021-02-04 11:33 UTC #6 lore raw

Quoted 39 lines · Guillaume Tucker

Quoted 38 lines · Ard Biesheuvel
Quoted 35 lines · Russell King - ARM Linux admin
Quoted 13 lines · Ard Biesheuvel
Quoted 11 lines · Guillaume Tucker
Hi Ard,
Please see the bisection report below about a boot failure on rk3288 with next-20210203. It was also bisected on imx6q-var-dt6customboard with next-20210202.
Reports aren't automatically sent to the public while we're trialing new bisection features on kernelci.org but this one looks valid.
Quoted 6 lines
Quoted 4 lines
The kernel is most likely crashing very early on, so there's nothing in the logs. Please let us know if you need some help with debugging or trying a fix on these platforms.
Thanks for the report.
Ard,
I want to send my fixes branch today which includes your regression fix that caused this regression.
As this is proving difficult to fix, I can only drop your fix from my fixes branch - and given that this seems to be problematical, I'm tempted to revert the original change at this point which should fix both of these regressions - and then we have another go at getting rid of the set/way instructions during the next cycle.
Thoughts?

Quoted 6 lines

Quoted 3 lines
Hi Russell,
If Guillaume is willing to do the experiment, and it fixes the issue,
Yes, I'm running some tests with that fix now and should have some results shortly.

Yes it does fix the issue:

https://lava.collabora.co.uk/scheduler/job/3173819

with Ard's fix applied to this test branch:

https://gitlab.collabora.com/gtucker/linux/-/commits/next-20210203-ard-fix/

+clang +Nick

It's worth mentioning that the issue only happens with kernels built with Clang. As you can see there are several other arm platforms failing with clang-11 builds but booting fine with gcc-8:

https://kernelci.org/test/job/next/branch/master/kernel/next-20210203/plan/baseline/

Here's a sample build log:

https://storage.staging.kernelci.org/gtucker/next-20210203-ard-fix/v5.10-rc4-24722-g58b6c0e507b7-gtucker_single-staging-33/arm/multi_v7_defconfig/clang-11/build.log

Essentially:

make -j18 ARCH=arm CROSS_COMPILE=arm-linux-gnueabihf- LLVM=1 CC="ccache clang" zImage

I believe it should be using the GNU assembler as LLVM_IAS=1 is not defined, but there may be something more subtle about it.

Thanks,
Guillaume

Quoted 18 lines

Quoted 18 lines
it proves that rk3288 is relying on the flush before the MMU is disabled, and so in that case, the fix is trivial, and we can just apply it.
If the experiment fails (which would mean rk3288 does not tolerate the cache maintenance being performed after cache off), it is going to be hairy, and so it will definitely take more time.
So in the latter case (or if Guillaume does not get back to us), I think reverting my queued fix is the only sane option. But in that case, may I suggest that we queue the revert of the original by-VA change for v5.12 so it gets lots of coverage in -next, and allows us an opportunity to come up with a proper fix in the same timeframe, and backport the revert and the subsequent fix as a pair? Otherwise, we'll end up in the situation where v5.10.x until today has by-va, v5.10.x-y has set/way, and v5.10y+ has by-va again. (I don't think we care about anything before that, given that v5.4 predates any of this)

Quoted 5 lines

Quoted 1 line
But in the end, I'm happy to go along with whatever works best for you.
Thanks,
Guillaume

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Russell King - ARM Linux admin

maintainer

2021-02-04 11:46 UTC #7 lore raw

Quoted 9 lines · Guillaume Tucker

Yes it does fix the issue:
https://lava.collabora.co.uk/scheduler/job/3173819
with Ard's fix applied to this test branch:
https://gitlab.collabora.com/gtucker/linux/-/commits/next-20210203-ard-fix/

Quoted 6 lines

+clang +Nick
It's worth mentioning that the issue only happens with kernels built with Clang. As you can see there are several other arm platforms failing with clang-11 builds but booting fine with gcc-8:

My gut feeling is that it isn't Clang specific - it's likely down to the exact code/data placement, how things end up during decompression, and exactly what state the cache ends up in.

That certainly was the case with the original regression.

Signature · 2 lines

RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Ard Biesheuvel

2021-02-04 12:10 UTC #8 lore raw

Quoted 18 lines · Russell King - ARM Linux admin

Quoted 9 lines · Guillaume Tucker
Yes it does fix the issue:
https://lava.collabora.co.uk/scheduler/job/3173819
with Ard's fix applied to this test branch:
https://gitlab.collabora.com/gtucker/linux/-/commits/next-20210203-ard-fix/
Quoted 6 lines
+clang +Nick
It's worth mentioning that the issue only happens with kernels built with Clang. As you can see there are several other arm platforms failing with clang-11 builds but booting fine with gcc-8:

Quoted 6 lines

My gut feeling is that it isn't Clang specific - it's likely down to the exact code/data placement, how things end up during decompression, and exactly what state the cache ends up in.
That certainly was the case with the original regression.

Agreed.

So given that my queued fix turns this

cache_clean
cache_off

into this

cache_off
cache_clean

for v7 only, and considering that turning this into

cache_clean
cache_off
cache_clean

(as the diff tested by Guillaume does) fixes the reported issue, it seems like the safest option to me at this point.

Reverting both patches, one of which has been in mainline since v5.7, seems unwise to me at this point in the cycle.

Ard Biesheuvel

2021-02-04 15:43 UTC #9 lore raw

In reply to Guillaume Tucker

Quoted 68 lines · Guillaume Tucker

Quoted 39 lines · Guillaume Tucker
Quoted 38 lines · Ard Biesheuvel
Quoted 35 lines · Russell King - ARM Linux admin
Quoted 13 lines · Ard Biesheuvel
Quoted 11 lines · Guillaume Tucker
Hi Ard,
Please see the bisection report below about a boot failure on rk3288 with next-20210203. It was also bisected on imx6q-var-dt6customboard with next-20210202.
Reports aren't automatically sent to the public while we're trialing new bisection features on kernelci.org but this one looks valid.
Quoted 6 lines
Quoted 4 lines
The kernel is most likely crashing very early on, so there's nothing in the logs. Please let us know if you need some help with debugging or trying a fix on these platforms.
Thanks for the report.
Ard,
I want to send my fixes branch today which includes your regression fix that caused this regression.
As this is proving difficult to fix, I can only drop your fix from my fixes branch - and given that this seems to be problematical, I'm tempted to revert the original change at this point which should fix both of these regressions - and then we have another go at getting rid of the set/way instructions during the next cycle.
Thoughts?
Quoted 6 lines
Quoted 3 lines
Hi Russell,
If Guillaume is willing to do the experiment, and it fixes the issue,
Yes, I'm running some tests with that fix now and should have some results shortly.
Yes it does fix the issue:
https://lava.collabora.co.uk/scheduler/job/3173819
with Ard's fix applied to this test branch:
https://gitlab.collabora.com/gtucker/linux/-/commits/next-20210203-ard-fix/
+clang +Nick
It's worth mentioning that the issue only happens with kernels built with Clang. As you can see there are several other arm platforms failing with clang-11 builds but booting fine with gcc-8:
https://kernelci.org/test/job/next/branch/master/kernel/next-20210203/plan/baseline/
Here's a sample build log:

Quoted 9 lines

https://storage.staging.kernelci.org/gtucker/next-20210203-ard-fix/v5.10-rc4-24722-g58b6c0e507b7-gtucker_single-staging-33/arm/multi_v7_defconfig/clang-11/build.log
Essentially:
make -j18 ARCH=arm CROSS_COMPILE=arm-linux-gnueabihf- LLVM=1 CC="ccache clang" zImage
I believe it should be using the GNU assembler as LLVM_IAS=1 is not defined, but there may be something more subtle about it.

Do you have a link for a failing zImage built from multi_v7_defconfig?

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Guillaume Tucker

author

2021-02-04 15:54 UTC #10 lore raw

Quoted 74 lines · Ard Biesheuvel

Quoted 72 lines · Guillaume Tucker
Quoted 39 lines · Guillaume Tucker
Quoted 38 lines · Ard Biesheuvel
Quoted 35 lines · Russell King - ARM Linux admin
Quoted 13 lines · Ard Biesheuvel
Quoted 11 lines · Guillaume Tucker
Hi Ard,
Please see the bisection report below about a boot failure on rk3288 with next-20210203. It was also bisected on imx6q-var-dt6customboard with next-20210202.
Reports aren't automatically sent to the public while we're trialing new bisection features on kernelci.org but this one looks valid.
Quoted 6 lines
Quoted 4 lines
The kernel is most likely crashing very early on, so there's nothing in the logs. Please let us know if you need some help with debugging or trying a fix on these platforms.
Thanks for the report.
Ard,
I want to send my fixes branch today which includes your regression fix that caused this regression.
As this is proving difficult to fix, I can only drop your fix from my fixes branch - and given that this seems to be problematical, I'm tempted to revert the original change at this point which should fix both of these regressions - and then we have another go at getting rid of the set/way instructions during the next cycle.
Thoughts?
Quoted 6 lines
Quoted 3 lines
Hi Russell,
If Guillaume is willing to do the experiment, and it fixes the issue,
Yes, I'm running some tests with that fix now and should have some results shortly.
Yes it does fix the issue:
https://lava.collabora.co.uk/scheduler/job/3173819
with Ard's fix applied to this test branch:
https://gitlab.collabora.com/gtucker/linux/-/commits/next-20210203-ard-fix/
+clang +Nick
It's worth mentioning that the issue only happens with kernels built with Clang. As you can see there are several other arm platforms failing with clang-11 builds but booting fine with gcc-8:
https://kernelci.org/test/job/next/branch/master/kernel/next-20210203/plan/baseline/
Here's a sample build log:
https://storage.staging.kernelci.org/gtucker/next-20210203-ard-fix/v5.10-rc4-24722-g58b6c0e507b7-gtucker_single-staging-33/arm/multi_v7_defconfig/clang-11/build.log
Essentially:

Quoted 8 lines

Quoted 5 lines
make -j18 ARCH=arm CROSS_COMPILE=arm-linux-gnueabihf- LLVM=1 CC="ccache clang" zImage
I believe it should be using the GNU assembler as LLVM_IAS=1 is not defined, but there may be something more subtle about it.
Do you have a link for a failing zImage built from multi_v7_defconfig?

Sure, this one was built from a plain next-20210203:

http://storage.staging.kernelci.org/gtucker/next-20210203-ard-fix/v5.10-rc4-24722-g58b6c0e507b7-gtucker_single-staging-33/arm/multi_v7_defconfig/clang-11/zImage

You can also find the dtbs, modules and other things in that same directory.

For the record, here's the test job that used it:

https://lava.collabora.co.uk/scheduler/job/3173792

Guillaume

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Ard Biesheuvel

2021-02-04 16:03 UTC #11 lore raw

Quoted 87 lines · Guillaume Tucker

Quoted 74 lines · Ard Biesheuvel
Quoted 72 lines · Guillaume Tucker
Quoted 39 lines · Guillaume Tucker
Quoted 38 lines · Ard Biesheuvel
Quoted 35 lines · Russell King - ARM Linux admin
Quoted 13 lines · Ard Biesheuvel
Quoted 11 lines · Guillaume Tucker
Hi Ard,
Please see the bisection report below about a boot failure on rk3288 with next-20210203. It was also bisected on imx6q-var-dt6customboard with next-20210202.
Reports aren't automatically sent to the public while we're trialing new bisection features on kernelci.org but this one looks valid.
Quoted 6 lines
Quoted 4 lines
The kernel is most likely crashing very early on, so there's nothing in the logs. Please let us know if you need some help with debugging or trying a fix on these platforms.
Thanks for the report.
Ard,
I want to send my fixes branch today which includes your regression fix that caused this regression.
As this is proving difficult to fix, I can only drop your fix from my fixes branch - and given that this seems to be problematical, I'm tempted to revert the original change at this point which should fix both of these regressions - and then we have another go at getting rid of the set/way instructions during the next cycle.
Thoughts?
Quoted 6 lines
Quoted 3 lines
Hi Russell,
If Guillaume is willing to do the experiment, and it fixes the issue,
Yes, I'm running some tests with that fix now and should have some results shortly.
Yes it does fix the issue:
https://lava.collabora.co.uk/scheduler/job/3173819
with Ard's fix applied to this test branch:
https://gitlab.collabora.com/gtucker/linux/-/commits/next-20210203-ard-fix/
+clang +Nick
It's worth mentioning that the issue only happens with kernels built with Clang. As you can see there are several other arm platforms failing with clang-11 builds but booting fine with gcc-8:
https://kernelci.org/test/job/next/branch/master/kernel/next-20210203/plan/baseline/
Here's a sample build log:
https://storage.staging.kernelci.org/gtucker/next-20210203-ard-fix/v5.10-rc4-24722-g58b6c0e507b7-gtucker_single-staging-33/arm/multi_v7_defconfig/clang-11/build.log
Essentially:
Quoted 8 lines
Quoted 5 lines
make -j18 ARCH=arm CROSS_COMPILE=arm-linux-gnueabihf- LLVM=1 CC="ccache clang" zImage
I believe it should be using the GNU assembler as LLVM_IAS=1 is not defined, but there may be something more subtle about it.
Do you have a link for a failing zImage built from multi_v7_defconfig?
Sure, this one was built from a plain next-20210203:

Quoted 9 lines

http://storage.staging.kernelci.org/gtucker/next-20210203-ard-fix/v5.10-rc4-24722-g58b6c0e507b7-gtucker_single-staging-33/arm/multi_v7_defconfig/clang-11/zImage
You can also find the dtbs, modules and other things in that same directory.
For the record, here's the test job that used it:
https://lava.collabora.co.uk/scheduler/job/3173792

Thanks.

That zImage boots fine locally. Unfortunately, I don't have rk3288 hardware to reproduce.

Could you please point me to the list of all the other platforms that failed to boot this image?

To be honest, I am slightly annoyed that a change that works fine with GCC but does not work with Clang version

11.1.0-++20210130110826+3a8282376b6c-1~exp1~20210130221445.158

(where exp means experimental, I suppose) is the reason for this discussion, especially because the change is in asm code. Is it possible to build with Clang but use the GNU linker?

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Nick Desaulniers

2021-02-04 18:07 UTC #12 lore raw

Quoted 11 lines · Ard Biesheuvel

Quoted 8 lines · Guillaume Tucker
Quoted 6 lines · Ard Biesheuvel
Quoted 4 lines · Guillaume Tucker
Essentially:
make -j18 ARCH=arm CROSS_COMPILE=arm-linux-gnueabihf- LLVM=1 CC="ccache clang" zImage

This command should link with BFD (and assemble with GAS; it's only using clang as the compiler.

Quoted 9 lines

To be honest, I am slightly annoyed that a change that works fine with GCC but does not work with Clang version
11.1.0-++20210130110826+3a8282376b6c-1~exp1~20210130221445.158
(where exp means experimental, I suppose) is the reason for this discussion, especially because the change is in asm code. Is it possible to build with Clang but use the GNU linker?

rk3288 might be the last 32b ARM platform ChromeOS uses. "veyron"

Signature · 7 lines

Thanks,
~Nick Desaulniers

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Nathan Chancellor

2021-02-04 18:13 UTC #13 lore raw

Quoted 9 lines · Nick Desaulniers' via Clang Built Linux

Quoted 8 lines · Ard Biesheuvel
Quoted 5 lines · Guillaume Tucker
Quoted 3 lines · Ard Biesheuvel
Quoted 1 line · Guillaume Tucker

Quoted 6 lines

Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Essentially:
make -j18 ARCH=arm CROSS_COMPILE=arm-linux-gnueabihf- LLVM=1 CC="ccache clang" zImage
This command should link with BFD (and assemble with GAS; it's only using clang as the compiler.

I think you missed the 'LLVM=1' before CC="ccache clang". That should use all of the LLVM utilities minus the integrated assembler while wrapping clang with ccache.

Cheers,
Nathan

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Nick Desaulniers

2021-02-04 18:24 UTC #14 lore raw

Quoted 13 lines · Nathan Chancellor

Quoted 11 lines · Nick Desaulniers' via Clang Built Linux
Quoted 10 lines · Ard Biesheuvel
Quoted 7 lines · Guillaume Tucker
Quoted 5 lines · Ard Biesheuvel
Quoted 3 lines · Guillaume Tucker
Essentially:

Quoted 8 lines

Quoted 4 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
make -j18 ARCH=arm CROSS_COMPILE=arm-linux-gnueabihf- LLVM=1 CC="ccache clang" zImage
This command should link with BFD (and assemble with GAS; it's only using clang as the compiler.
I think you missed the 'LLVM=1' before CC="ccache clang". That should use all of the LLVM utilities minus the integrated assembler while wrapping clang with ccache.

You're right, I missed LLVM=1. Adding LD=ld.bfd I think should permit fallback to BFD.

Signature · 7 lines

Thanks,
~Nick Desaulniers

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Guillaume Tucker

author

2021-02-04 21:31 UTC #15 lore raw

Quoted 19 lines · Nick Desaulniers

Quoted 18 lines · Nathan Chancellor
Quoted 15 lines · Nick Desaulniers' via Clang Built Linux
Quoted 11 lines · Ard Biesheuvel
Quoted 8 lines · Guillaume Tucker
Quoted 6 lines · Ard Biesheuvel
Quoted 4 lines · Guillaume Tucker
Essentially:
make -j18 ARCH=arm CROSS_COMPILE=arm-linux-gnueabihf- LLVM=1 CC="ccache clang" zImage
This command should link with BFD (and assemble with GAS; it's only using clang as the compiler.

Quoted 6 lines

Quoted 3 lines
I think you missed the 'LLVM=1' before CC="ccache clang". That should use all of the LLVM utilities minus the integrated assembler while wrapping clang with ccache.
You're right, I missed LLVM=1. Adding LD=ld.bfd I think should permit fallback to BFD.

That was close, except we're cross-compiling with GCC for arm. So I've now built a plain next-20210203 (without Ard's fix) using this command line:

make LD=arm-linux-gnueabihf-ld.bfd -j18 ARCH=arm CROSS_COMPILE=arm-linux-gnueabihf- LLVM=1 CC="ccache clang" zImage

I'm using a modified Docker image gtucker/kernelci-build-clang-11 with the very latest LLVM 11 and gcc-8-arm-linux-gnueabihf packages added to be able to use the GNU linker. BTW I guess we should enable this kind of hybrid build setup on kernelci.org as well.

Full build log + kernel binaries can be found here:

    https://storage.staging.kernelci.org/gtucker/next-20210203-ard-fix/v5.10-rc4-24722-g58b6c0e507b7-gtucker_single-staging-41/arm/multi_v7_defconfig/clang-11/

And this booted fine, which confirms it's really down to how ld.lld puts together the kernel image. Does it actually solve the debate whether this is an issue to fix in the assembly code or at link time?

Full test job details for the record:

    https://lava.collabora.co.uk/scheduler/job/3176004

Hope that helps,
Guillaume

Russell King - ARM Linux admin

maintainer

2021-02-04 21:51 UTC #16 lore raw

Quoted 15 lines · Guillaume Tucker

Quoted 2 lines · Nick Desaulniers
You're right, I missed LLVM=1. Adding LD=ld.bfd I think should permit fallback to BFD.
That was close, except we're cross-compiling with GCC for arm. So I've now built a plain next-20210203 (without Ard's fix) using this command line:
make LD=arm-linux-gnueabihf-ld.bfd -j18 ARCH=arm CROSS_COMPILE=arm-linux-gnueabihf- LLVM=1 CC="ccache clang" zImage
I'm using a modified Docker image gtucker/kernelci-build-clang-11 with the very latest LLVM 11 and gcc-8-arm-linux-gnueabihf packages added to be able to use the GNU linker. BTW I guess we should enable this kind of hybrid build setup on kernelci.org as well.

...

Quoted 4 lines

And this booted fine, which confirms it's really down to how ld.lld puts together the kernel image. Does it actually solve the debate whether this is an issue to fix in the assembly code or at link time?

Well... as I mentioned previously, we don't really understand what is going on between the decompressor running with the caches on, turning the caches off, jumping into the decompressed kernel, and then getting to the v7 setup code.

The results from various attempts at solving the problem which lead to Ard's original patch that caused your breakage were not making a whole lot of sense (I think I wrote that all up in a previous email thread, so won't repeat it here.)

So, I was slightly nervous about merging Ard's fix - and your report suggested that there is indeed more going on here that we don't understand.

When I was tracking down what was going on, I had this patch applied (I've had to recreate it, so may not be exactly what I had), with the DEBUG_LL stuff appropriately enabled. It may be worth applying this patch, enabling the DEBUG_LL stuff appropriately for one of your failing boards, and try booting it.

You should get two strings of identical hex numbers that look something like:

ffffffff480000000900040140003000000000004820071d40008090

If they're looking like instructions, for example:

ee060f37e3a00080ee020f10ee020f30ee030f10e3a00903ee050f30

Then it's likely that you are seeing a very similar problem as I was without Ard's patch. If you do get instruction-like content, then you will likely find the sequence of instructions in the decompressor code.

arch/arm/mm/proc-v7.S +32 -0

index 28c9d32fa99a..19fa93ae282c 100644
@@ -475,7 +475,39 @@ ENDPROC(cpu_pj4b_do_resume)
 	ldr	r12, [r0]
 	add	r12, r12, r0			@ the local stack
 	stmia	r12, {r1-r6, lr}		@ v7_invalidate_l1 touches r0-r6
+	ldr	r0, [r12, #0]
+	bl	printhex8
+	ldr	r0, [r12, #4]
+	bl	printhex8
+	ldr	r0, [r12, #8]
+	bl	printhex8
+	ldr	r0, [r12, #12]
+	bl	printhex8
+	ldr	r0, [r12, #16]
+	bl	printhex8
+	ldr	r0, [r12, #20]
+	bl	printhex8
+	ldr	r0, [r12, #24]
+	bl	printhex8
+	mov	r0, #'\n'
+	bl	printch
 	bl      v7_invalidate_l1
+	ldr	r0, [r12, #0]
+	bl	printhex8
+	ldr	r0, [r12, #4]
+	bl	printhex8
+	ldr	r0, [r12, #8]
+	bl	printhex8
+	ldr	r0, [r12, #12]
+	bl	printhex8
+	ldr	r0, [r12, #16]
+	bl	printhex8
+	ldr	r0, [r12, #20]
+	bl	printhex8
+	ldr	r0, [r12, #24]
+	bl	printhex8
+	mov	r0, #'\n'
+	bl	printch
 	ldmia	r12, {r1-r6, lr}
 
 __v7_setup_cont:

Signature · 7 lines

RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Ard Biesheuvel

2021-02-05 08:23 UTC #17 lore raw

In reply to Guillaume Tucker

Quoted 44 lines · Guillaume Tucker

Quoted 19 lines · Nick Desaulniers
Quoted 18 lines · Nathan Chancellor
Quoted 15 lines · Nick Desaulniers' via Clang Built Linux
Quoted 11 lines · Ard Biesheuvel
Quoted 8 lines · Guillaume Tucker
Quoted 6 lines · Ard Biesheuvel
Quoted 4 lines · Guillaume Tucker
Essentially:
make -j18 ARCH=arm CROSS_COMPILE=arm-linux-gnueabihf- LLVM=1 CC="ccache clang" zImage
This command should link with BFD (and assemble with GAS; it's only using clang as the compiler.
Quoted 6 lines
Quoted 3 lines
I think you missed the 'LLVM=1' before CC="ccache clang". That should use all of the LLVM utilities minus the integrated assembler while wrapping clang with ccache.
You're right, I missed LLVM=1. Adding LD=ld.bfd I think should permit fallback to BFD.
That was close, except we're cross-compiling with GCC for arm. So I've now built a plain next-20210203 (without Ard's fix) using this command line:
make LD=arm-linux-gnueabihf-ld.bfd -j18 ARCH=arm CROSS_COMPILE=arm-linux-gnueabihf- LLVM=1 CC="ccache clang" zImage
I'm using a modified Docker image gtucker/kernelci-build-clang-11 with the very latest LLVM 11 and gcc-8-arm-linux-gnueabihf packages added to be able to use the GNU linker. BTW I guess we should enable this kind of hybrid build setup on kernelci.org as well.
Full build log + kernel binaries can be found here:
    https://storage.staging.kernelci.org/gtucker/next-20210203-ard-fix/v5.10-rc4-24722-g58b6c0e507b7-gtucker_single-staging-41/arm/multi_v7_defconfig/clang-11/

Quoted 9 lines

And this booted fine, which confirms it's really down to how ld.lld puts together the kernel image. Does it actually solve the debate whether this is an issue to fix in the assembly code or at link time?
Full test job details for the record:
    https://lava.collabora.co.uk/scheduler/job/3176004

So the issue appears to be in the way the linker generates the _kernel_bss_size symbol, which obviously has an impact, given that the queued fix takes it into account in the cache_clean operation.

On GNU ld, I see

479: 00065e14 0 NOTYPE GLOBAL DEFAULT ABS _kernel_bss_size

whereas n LLVM ld.lld, I see

433: c1c86e98 0 NOTYPE GLOBAL DEFAULT ABS _kernel_bss_size

and adding this value may cause the cache clean to operate on unmapped addresses, or cause the addition to wrap and not perform a cache clean at all.

AFAICT, this also breaks the appended DTB case in LLVM, so this needs a separate fix in any case.

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Ard Biesheuvel

2021-02-05 12:07 UTC #18 lore raw

Quoted 68 lines · Ard Biesheuvel

Quoted 44 lines · Guillaume Tucker
Quoted 19 lines · Nick Desaulniers
Quoted 18 lines · Nathan Chancellor
Quoted 15 lines · Nick Desaulniers' via Clang Built Linux
Quoted 11 lines · Ard Biesheuvel
Quoted 8 lines · Guillaume Tucker
Quoted 6 lines · Ard Biesheuvel
Quoted 4 lines · Guillaume Tucker
Essentially:
make -j18 ARCH=arm CROSS_COMPILE=arm-linux-gnueabihf- LLVM=1 CC="ccache clang" zImage
This command should link with BFD (and assemble with GAS; it's only using clang as the compiler.
Quoted 6 lines
Quoted 3 lines
I think you missed the 'LLVM=1' before CC="ccache clang". That should use all of the LLVM utilities minus the integrated assembler while wrapping clang with ccache.
You're right, I missed LLVM=1. Adding LD=ld.bfd I think should permit fallback to BFD.
That was close, except we're cross-compiling with GCC for arm. So I've now built a plain next-20210203 (without Ard's fix) using this command line:
make LD=arm-linux-gnueabihf-ld.bfd -j18 ARCH=arm CROSS_COMPILE=arm-linux-gnueabihf- LLVM=1 CC="ccache clang" zImage
I'm using a modified Docker image gtucker/kernelci-build-clang-11 with the very latest LLVM 11 and gcc-8-arm-linux-gnueabihf packages added to be able to use the GNU linker. BTW I guess we should enable this kind of hybrid build setup on kernelci.org as well.
Full build log + kernel binaries can be found here:
    https://storage.staging.kernelci.org/gtucker/next-20210203-ard-fix/v5.10-rc4-24722-g58b6c0e507b7-gtucker_single-staging-41/arm/multi_v7_defconfig/clang-11/
Quoted 9 lines
And this booted fine, which confirms it's really down to how ld.lld puts together the kernel image. Does it actually solve the debate whether this is an issue to fix in the assembly code or at link time?
Full test job details for the record:
    https://lava.collabora.co.uk/scheduler/job/3176004
So the issue appears to be in the way the linker generates the _kernel_bss_size symbol, which obviously has an impact, given that the queued fix takes it into account in the cache_clean operation.
On GNU ld, I see
479: 00065e14 0 NOTYPE GLOBAL DEFAULT ABS _kernel_bss_size
whereas n LLVM ld.lld, I see

Quoted 8 lines

433: c1c86e98 0 NOTYPE GLOBAL DEFAULT ABS _kernel_bss_size
and adding this value may cause the cache clean to operate on unmapped addresses, or cause the addition to wrap and not perform a cache clean at all.
AFAICT, this also breaks the appended DTB case in LLVM, so this needs a separate fix in any case.

I pushed a combined branch of torvalds/master, rmk/fixes (still containing my 9052/1 fix) and this patch to my for-kernelci branch

https://git.kernel.org/pub/scm/linux/kernel/git/ardb/linux.git/log/

Guillaume,

It seems there is no Clang-11 coverage there, right? Mind giving this branch a spin? If this fixes the regressions, we can get these queued up.

Thanks,
Ard.

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Guillaume Tucker

author

2021-02-06 13:11 UTC #19 lore raw

Quoted 81 lines · Ard Biesheuvel

Quoted 68 lines · Ard Biesheuvel
Quoted 44 lines · Guillaume Tucker
Quoted 19 lines · Nick Desaulniers
Quoted 18 lines · Nathan Chancellor
Quoted 15 lines · Nick Desaulniers' via Clang Built Linux
Quoted 11 lines · Ard Biesheuvel
Quoted 8 lines · Guillaume Tucker
Quoted 6 lines · Ard Biesheuvel
Quoted 4 lines · Guillaume Tucker
Essentially:
make -j18 ARCH=arm CROSS_COMPILE=arm-linux-gnueabihf- LLVM=1 CC="ccache clang" zImage
This command should link with BFD (and assemble with GAS; it's only using clang as the compiler.
Quoted 6 lines
Quoted 3 lines
I think you missed the 'LLVM=1' before CC="ccache clang". That should use all of the LLVM utilities minus the integrated assembler while wrapping clang with ccache.
You're right, I missed LLVM=1. Adding LD=ld.bfd I think should permit fallback to BFD.
That was close, except we're cross-compiling with GCC for arm. So I've now built a plain next-20210203 (without Ard's fix) using this command line:
make LD=arm-linux-gnueabihf-ld.bfd -j18 ARCH=arm CROSS_COMPILE=arm-linux-gnueabihf- LLVM=1 CC="ccache clang" zImage
I'm using a modified Docker image gtucker/kernelci-build-clang-11 with the very latest LLVM 11 and gcc-8-arm-linux-gnueabihf packages added to be able to use the GNU linker. BTW I guess we should enable this kind of hybrid build setup on kernelci.org as well.
Full build log + kernel binaries can be found here:
    https://storage.staging.kernelci.org/gtucker/next-20210203-ard-fix/v5.10-rc4-24722-g58b6c0e507b7-gtucker_single-staging-41/arm/multi_v7_defconfig/clang-11/
Quoted 9 lines
And this booted fine, which confirms it's really down to how ld.lld puts together the kernel image. Does it actually solve the debate whether this is an issue to fix in the assembly code or at link time?
Full test job details for the record:
    https://lava.collabora.co.uk/scheduler/job/3176004
So the issue appears to be in the way the linker generates the _kernel_bss_size symbol, which obviously has an impact, given that the queued fix takes it into account in the cache_clean operation.
On GNU ld, I see
479: 00065e14 0 NOTYPE GLOBAL DEFAULT ABS _kernel_bss_size
whereas n LLVM ld.lld, I see
Quoted 8 lines
433: c1c86e98 0 NOTYPE GLOBAL DEFAULT ABS _kernel_bss_size
and adding this value may cause the cache clean to operate on unmapped addresses, or cause the addition to wrap and not perform a cache clean at all.
AFAICT, this also breaks the appended DTB case in LLVM, so this needs a separate fix in any case.
I pushed a combined branch of torvalds/master, rmk/fixes (still containing my 9052/1 fix) and this patch to my for-kernelci branch

Quoted 7 lines

https://git.kernel.org/pub/scm/linux/kernel/git/ardb/linux.git/log/
Guillaume,
It seems there is no Clang-11 coverage there, right? Mind giving this branch a spin? If this fixes the regressions, we can get these queued up.

That's right, Clang builds are only enabled on linux-next and mainline at the moment. We could enable it on any other branch where it makes sense. How about just the main defconfig for arm, arm64 and x86_64 on your ardb/for-kernelci branch?

For now I've run another set of builds with clang-11 and got the following test results with your branch on staging:

https://staging.kernelci.org/test/job/ardb/branch/for-kernelci/kernel/v5.11-rc6-146-g923ca344043a/plan/baseline/

which are all passing.

I'll reply to the thread with your patch to confirm.

Guillaume

Ard Biesheuvel

2021-02-06 13:13 UTC #20 lore raw

Quoted 87 lines · Guillaume Tucker

Quoted 85 lines · Ard Biesheuvel
Quoted 68 lines · Ard Biesheuvel
Quoted 44 lines · Guillaume Tucker
Quoted 19 lines · Nick Desaulniers
Quoted 18 lines · Nathan Chancellor
Quoted 15 lines · Nick Desaulniers' via Clang Built Linux
Quoted 11 lines · Ard Biesheuvel
Quoted 8 lines · Guillaume Tucker
Quoted 6 lines · Ard Biesheuvel
Quoted 4 lines · Guillaume Tucker
Essentially:
make -j18 ARCH=arm CROSS_COMPILE=arm-linux-gnueabihf- LLVM=1 CC="ccache clang" zImage
This command should link with BFD (and assemble with GAS; it's only using clang as the compiler.
Quoted 6 lines
Quoted 3 lines
I think you missed the 'LLVM=1' before CC="ccache clang". That should use all of the LLVM utilities minus the integrated assembler while wrapping clang with ccache.
You're right, I missed LLVM=1. Adding LD=ld.bfd I think should permit fallback to BFD.
That was close, except we're cross-compiling with GCC for arm. So I've now built a plain next-20210203 (without Ard's fix) using this command line:
make LD=arm-linux-gnueabihf-ld.bfd -j18 ARCH=arm CROSS_COMPILE=arm-linux-gnueabihf- LLVM=1 CC="ccache clang" zImage
I'm using a modified Docker image gtucker/kernelci-build-clang-11 with the very latest LLVM 11 and gcc-8-arm-linux-gnueabihf packages added to be able to use the GNU linker. BTW I guess we should enable this kind of hybrid build setup on kernelci.org as well.
Full build log + kernel binaries can be found here:
    https://storage.staging.kernelci.org/gtucker/next-20210203-ard-fix/v5.10-rc4-24722-g58b6c0e507b7-gtucker_single-staging-41/arm/multi_v7_defconfig/clang-11/
Quoted 9 lines
And this booted fine, which confirms it's really down to how ld.lld puts together the kernel image. Does it actually solve the debate whether this is an issue to fix in the assembly code or at link time?
Full test job details for the record:
    https://lava.collabora.co.uk/scheduler/job/3176004
So the issue appears to be in the way the linker generates the _kernel_bss_size symbol, which obviously has an impact, given that the queued fix takes it into account in the cache_clean operation.
On GNU ld, I see
479: 00065e14 0 NOTYPE GLOBAL DEFAULT ABS _kernel_bss_size
whereas n LLVM ld.lld, I see
Quoted 8 lines
433: c1c86e98 0 NOTYPE GLOBAL DEFAULT ABS _kernel_bss_size
and adding this value may cause the cache clean to operate on unmapped addresses, or cause the addition to wrap and not perform a cache clean at all.
AFAICT, this also breaks the appended DTB case in LLVM, so this needs a separate fix in any case.
I pushed a combined branch of torvalds/master, rmk/fixes (still containing my 9052/1 fix) and this patch to my for-kernelci branch
https://git.kernel.org/pub/scm/linux/kernel/git/ardb/linux.git/log/
Guillaume,

Quoted 9 lines

Quoted 3 lines
It seems there is no Clang-11 coverage there, right? Mind giving this branch a spin? If this fixes the regressions, we can get these queued up.
That's right, Clang builds are only enabled on linux-next and mainline at the moment. We could enable it on any other branch where it makes sense. How about just the main defconfig for arm, arm64 and x86_64 on your ardb/for-kernelci branch?

Yes, please.

Quoted 9 lines

For now I've run another set of builds with clang-11 and got the following test results with your branch on staging:
https://staging.kernelci.org/test/job/ardb/branch/for-kernelci/kernel/v5.11-rc6-146-g923ca344043a/plan/baseline/
which are all passing.
I'll reply to the thread with your patch to confirm.

Excellent, thanks a lot.

Guillaume Tucker

author

2021-02-04 21:10 UTC #21 lore raw

In reply to Ard Biesheuvel

Quoted 99 lines · Ard Biesheuvel

Quoted 87 lines · Guillaume Tucker
Quoted 74 lines · Ard Biesheuvel
Quoted 72 lines · Guillaume Tucker
Quoted 39 lines · Guillaume Tucker
Quoted 38 lines · Ard Biesheuvel
Quoted 35 lines · Russell King - ARM Linux admin
Quoted 13 lines · Ard Biesheuvel
Quoted 11 lines · Guillaume Tucker
Hi Ard,
Please see the bisection report below about a boot failure on rk3288 with next-20210203. It was also bisected on imx6q-var-dt6customboard with next-20210202.
Reports aren't automatically sent to the public while we're trialing new bisection features on kernelci.org but this one looks valid.
Quoted 6 lines
Quoted 4 lines
The kernel is most likely crashing very early on, so there's nothing in the logs. Please let us know if you need some help with debugging or trying a fix on these platforms.
Thanks for the report.
Ard,
I want to send my fixes branch today which includes your regression fix that caused this regression.
As this is proving difficult to fix, I can only drop your fix from my fixes branch - and given that this seems to be problematical, I'm tempted to revert the original change at this point which should fix both of these regressions - and then we have another go at getting rid of the set/way instructions during the next cycle.
Thoughts?
Quoted 6 lines
Quoted 3 lines
Hi Russell,
If Guillaume is willing to do the experiment, and it fixes the issue,
Yes, I'm running some tests with that fix now and should have some results shortly.
Yes it does fix the issue:
https://lava.collabora.co.uk/scheduler/job/3173819
with Ard's fix applied to this test branch:
https://gitlab.collabora.com/gtucker/linux/-/commits/next-20210203-ard-fix/
+clang +Nick
It's worth mentioning that the issue only happens with kernels built with Clang. As you can see there are several other arm platforms failing with clang-11 builds but booting fine with gcc-8:
https://kernelci.org/test/job/next/branch/master/kernel/next-20210203/plan/baseline/
Here's a sample build log:
https://storage.staging.kernelci.org/gtucker/next-20210203-ard-fix/v5.10-rc4-24722-g58b6c0e507b7-gtucker_single-staging-33/arm/multi_v7_defconfig/clang-11/build.log
Essentially:
Quoted 8 lines
Quoted 5 lines
make -j18 ARCH=arm CROSS_COMPILE=arm-linux-gnueabihf- LLVM=1 CC="ccache clang" zImage
I believe it should be using the GNU assembler as LLVM_IAS=1 is not defined, but there may be something more subtle about it.
Do you have a link for a failing zImage built from multi_v7_defconfig?
Sure, this one was built from a plain next-20210203:
Quoted 9 lines
http://storage.staging.kernelci.org/gtucker/next-20210203-ard-fix/v5.10-rc4-24722-g58b6c0e507b7-gtucker_single-staging-33/arm/multi_v7_defconfig/clang-11/zImage
You can also find the dtbs, modules and other things in that same directory.
For the record, here's the test job that used it:
https://lava.collabora.co.uk/scheduler/job/3173792

Quoted 7 lines

Thanks.
That zImage boots fine locally. Unfortunately, I don't have rk3288 hardware to reproduce.
Could you please point me to the list of all the other platforms that failed to boot this image?

This is the list of platforms from kernelci.org I've gathered which appeared to be impacted:

imx6q-sabrelite
imx6q-var-dt6customboard
imx6dl-riotboard
imx6qp-wandboard-revd1
imx7ulp-evk
odroid-xu3
rk3288-rock2-square
rk3288-veyron-jaq
stm32mp157c-dk2
sun4i-a10-olinuxino-lime
sun5i-a13-olinuxino-micro
sun7i-a20-cubieboard2
sun7i-a20-olinuxino-lime2
sun8i-a33-olinuxino
sun8i-a83t-bananapi-m3
sun8i-h2-plus-libretech-all-h3-cc
sun8i-h2-plus-orangepi-r1
sun8i-h2-plus-orangepi-zero
sun8i-h3-libretech-all-h3-cc
sun8i-h3-bananapi-m2-plus
sun8i-h3-orangepi-pc
sun8i-r40-bananapi-m2-ultra

They were all booting next-20210203 with gcc-8 but not with clang-11. I've run checks on a good share of them with your patch applied and they're now booting with clang-11, just like the rk3288 and imx6q platforms that were used for the bisections.

Quoted 6 lines

To be honest, I am slightly annoyed that a change that works fine with GCC but does not work with Clang version
11.1.0-++20210130110826+3a8282376b6c-1~exp1~20210130221445.158
(where exp means experimental, I suppose) is the reason for this

Well it's the standard one from the LLVM Debian package repo:

deb http://apt.llvm.org/buster/ llvm-toolchain-buster-11 main

There's a slightly newer version, I doubt it would make any difference in this respect unless there's a particular fix in ld.lld:

# apt policy clang-11
clang-11:
  Installed: 1:11.1.0~++20210130110826+3a8282376b6c-1~exp1~20210130221445.158
  Candidate: 1:11.1.0~++20210204120158+1fdec59bffc1-1~exp1~20210203230823.159

Quoted 2 lines

discussion, especially because the change is in asm code. Is it possible to build with Clang but use the GNU linker?

As mentioned by Nick, it is using everything from LLVM except the assembler - so not the GNU linker. I've now built a new Docker container with the latest LLVM package version (.159) as well as gcc-8-arm-linux-gnueabihf to try with the GNU linker and see if that makes any difference. More on that shortly...

Guillaume

Russell King - ARM Linux admin

maintainer

2021-02-04 10:49 UTC #22 lore raw

In reply to Ard Biesheuvel

Quoted 11 lines · Ard Biesheuvel

Hi Russell,
If Guillaume is willing to do the experiment, and it fixes the issue, it proves that rk3288 is relying on the flush before the MMU is disabled, and so in that case, the fix is trivial, and we can just apply it.
If the experiment fails (which would mean rk3288 does not tolerate the cache maintenance being performed after cache off), it is going to be hairy, and so it will definitely take more time.

Quoted 9 lines

So in the latter case (or if Guillaume does not get back to us), I think reverting my queued fix is the only sane option. But in that case, may I suggest that we queue the revert of the original by-VA change for v5.12 so it gets lots of coverage in -next, and allows us an opportunity to come up with a proper fix in the same timeframe, and backport the revert and the subsequent fix as a pair? Otherwise, we'll end up in the situation where v5.10.x until today has by-va, v5.10.x-y has set/way, and v5.10y+ has by-va again. (I don't think we care about anything before that, given that v5.4 predates any of this)

I'm suggesting dropping your fix (9052/1) and reverting "ARM: decompressor: switch to by-VA cache maintenance for v7 cores" which gets us to a point where _both_ regressions are fixed.

I'm of the opinion that the by-VA patch was incorrect when it was merged (it caused a regression), and it's only a performance improvement. Our attempts so far to fix it are just causing other regressions. So, I think it is reasonable to revert both back to a known good point which has worked over a decade. If doing so causes regressions (which I think is unlikely), then that would be unfortunate but alas is a price that's worth paying to get back to a known good point - since then we're not stacking regression fixes on top of other regression fixes.

Signature · 2 lines

RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Ard Biesheuvel

2021-02-04 10:56 UTC #23 lore raw

(cc Marc)

Quoted 23 lines · Russell King - ARM Linux admin

Quoted 11 lines · Ard Biesheuvel
Hi Russell,
If Guillaume is willing to do the experiment, and it fixes the issue, it proves that rk3288 is relying on the flush before the MMU is disabled, and so in that case, the fix is trivial, and we can just apply it.
If the experiment fails (which would mean rk3288 does not tolerate the cache maintenance being performed after cache off), it is going to be hairy, and so it will definitely take more time.
Quoted 9 lines
So in the latter case (or if Guillaume does not get back to us), I think reverting my queued fix is the only sane option. But in that case, may I suggest that we queue the revert of the original by-VA change for v5.12 so it gets lots of coverage in -next, and allows us an opportunity to come up with a proper fix in the same timeframe, and backport the revert and the subsequent fix as a pair? Otherwise, we'll end up in the situation where v5.10.x until today has by-va, v5.10.x-y has set/way, and v5.10y+ has by-va again. (I don't think we care about anything before that, given that v5.4 predates any of this)

Quoted 4 lines

I'm suggesting dropping your fix (9052/1) and reverting "ARM: decompressor: switch to by-VA cache maintenance for v7 cores" which gets us to a point where _both_ regressions are fixed.

I understand, but we don't know whether doing so might regress other platforms that were added in the mean time.

Quoted 3 lines

I'm of the opinion that the by-VA patch was incorrect when it was merged (it caused a regression), and it's only a performance improvement.

It is a correctness improvement, not a performance improvement.

Without that change, the 32-bit ARM kernel cannot boot bare metal on platforms with a system cache such as 8040 or SynQuacer, and can only boot under KVM on such systems because of the special handling of set/way instructions by the host.

The performance issue related to set/way ops under KVM was already fixed by describing data and unified caches as 1 set and 1 way when running in 32-bit mode.

Quoted 8 lines

Our attempts so far to fix it are just causing other regressions. So, I think it is reasonable to revert both back to a known good point which has worked over a decade. If doing so causes regressions (which I think is unlikely), then that would be unfortunate but alas is a price that's worth paying to get back to a known good point - since then we're not stacking regression fixes on top of other regression fixes.

This is exactly why I am proposing to queue the revert of the original patch for v5.12, and only backport it to v5.10 and v5.11 once we are sure it does not break anything else.

Marc Zyngier

2021-02-04 12:27 UTC #24 lore raw

Quoted 39 lines · Ard Biesheuvel

(cc Marc)
Quoted 23 lines · Russell King - ARM Linux admin
Quoted 11 lines · Ard Biesheuvel
Hi Russell,
If Guillaume is willing to do the experiment, and it fixes the issue, it proves that rk3288 is relying on the flush before the MMU is disabled, and so in that case, the fix is trivial, and we can just apply it.
If the experiment fails (which would mean rk3288 does not tolerate the cache maintenance being performed after cache off), it is going to be hairy, and so it will definitely take more time.
Quoted 9 lines
So in the latter case (or if Guillaume does not get back to us), I think reverting my queued fix is the only sane option. But in that case, may I suggest that we queue the revert of the original by-VA change for v5.12 so it gets lots of coverage in -next, and allows us an opportunity to come up with a proper fix in the same timeframe, and backport the revert and the subsequent fix as a pair? Otherwise, we'll end up in the situation where v5.10.x until today has by-va, v5.10.x-y has set/way, and v5.10y+ has by-va again. (I don't think we care about anything before that, given that v5.4 predates any of this)
Quoted 4 lines
I'm suggesting dropping your fix (9052/1) and reverting "ARM: decompressor: switch to by-VA cache maintenance for v7 cores" which gets us to a point where _both_ regressions are fixed.
I understand, but we don't know whether doing so might regress other platforms that were added in the mean time.
Quoted 3 lines
I'm of the opinion that the by-VA patch was incorrect when it was merged (it caused a regression), and it's only a performance improvement.

Quoted 6 lines

It is a correctness improvement, not a performance improvement.
Without that change, the 32-bit ARM kernel cannot boot bare metal on platforms with a system cache such as 8040 or SynQuacer, and can only boot under KVM on such systems because of the special handling of set/way instructions by the host.

I agree. With set/way CMOs, there is no way to reach the PoC if it beyond the system cache, leading to an unbootable kernel. This is actually pretty well documented in the architecture, and it did bite us for the first time on XGene-1, 7 years ago.

In retrospect, having KVM to handle set/way CMOs in was a mistake, as it just papered over the problem for the sake of running older 32bit guests. It violated the principle of KVM/arm being strictly architectural and provided unrealistic expectations. I'll take the blame for this.

Thanks,

M.

Signature · 1 line

Jazz is not dead. It just smells funny...

Russell King - ARM Linux admin

maintainer

2021-02-04 14:10 UTC #25 lore raw

Quoted 4 lines · Marc Zyngier

I agree. With set/way CMOs, there is no way to reach the PoC if it beyond the system cache, leading to an unbootable kernel. This is actually pretty well documented in the architecture, and it did bite us for the first time on XGene-1, 7 years ago.

That may be, however we still do set/way maintenance to invalidate the L1 cache as that is required for ARMv7 to place the cache into a known state, as stated by the architecture reference manual.

Arguably, that should be done by firmware, but when starting secondary CPUs, there are platforms out there which do not bring the L1 cache to a defined state. So we are pretty much stuck with doing set/way operations during CPU initialisation in the main kernel.

If ARMv8 decides that this is not supportable, then that's a matter for ARMv8 to address without impacting the requirements of ARMv7.

Signature · 7 lines

RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Ard Biesheuvel

2021-02-04 14:26 UTC #26 lore raw

Quoted 11 lines · Russell King - ARM Linux admin

Quoted 4 lines · Marc Zyngier
I agree. With set/way CMOs, there is no way to reach the PoC if it beyond the system cache, leading to an unbootable kernel. This is actually pretty well documented in the architecture, and it did bite us for the first time on XGene-1, 7 years ago.
That may be, however we still do set/way maintenance to invalidate the L1 cache as that is required for ARMv7 to place the cache into a known state, as stated by the architecture reference manual.

Getting a certain cache at a certain level into a known state is a valid use of set/way ops, and is simply unnecessary when running under virtualization, but doesn't do any harm.

Pushing contents of the cache hierarchy to main memory is *not* a valid use of set/way ops, and so there is no point in pretending that set/way ops will produce the same results as by-VA ops. Only the by-VA ops give the architectural guarantees that we rely on for correctness.

Quoted 6 lines

Arguably, that should be done by firmware, but when starting secondary CPUs, there are platforms out there which do not bring the L1 cache to a defined state. So we are pretty much stuck with doing set/way operations during CPU initialisation in the main kernel.

Indeed. And this is unfortunate, but not the end of the world.

Quoted 3 lines

If ARMv8 decides that this is not supportable, then that's a matter for ARMv8 to address without impacting the requirements of ARMv7.

I'm not sure what you mean here. The v7 architecture is crystal clear about the difference between set/way ops (managing a single cache), and by-VA ops (managing the 'cachedness' state of a memory region). The semantics are radically different, regardless of v7 vs v8 or AArch32 vs AArch64.

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Russell King - ARM Linux admin

maintainer

2021-02-04 14:39 UTC #27 lore raw

Quoted 4 lines · Ard Biesheuvel

Pushing contents of the cache hierarchy to main memory is *not* a valid use of set/way ops, and so there is no point in pretending that set/way ops will produce the same results as by-VA ops. Only the by-VA ops give the architectural guarantees that we rely on for correctness.

... yet we /were/ doing that, and it worked fine for 13 years - from 1st June 2007 until the by-VA merge into mainline on the 3rd April 2020.

You may be right that it wasn't the most correct way, but it worked for those 13 years without issue, and it's only recently that it's become a problem, and trying to "fix" that introduced a regression, and fixing that regression has caused another regression... and I what I'm wondering is how many more regression fixing cycles it's going to take - how many regression fixes on top of other regression fixes are we going to end up seeing here.

The fact is, we never properly understood why your patch caused the regression I was seeing. If we don't understand it, then we can never say that we've fixed the problem properly. That is highlighted by the fact that fixing the regression I was seeing has caused another regression.

Signature · 2 lines

RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Ard Biesheuvel

2021-02-04 15:54 UTC #28 lore raw

Quoted 19 lines · Russell King - ARM Linux admin

Quoted 4 lines · Ard Biesheuvel
Pushing contents of the cache hierarchy to main memory is *not* a valid use of set/way ops, and so there is no point in pretending that set/way ops will produce the same results as by-VA ops. Only the by-VA ops give the architectural guarantees that we rely on for correctness.
... yet we /were/ doing that, and it worked fine for 13 years - from 1st June 2007 until the by-VA merge into mainline on the 3rd April 2020.
You may be right that it wasn't the most correct way, but it worked for those 13 years without issue, and it's only recently that it's become a problem, and trying to "fix" that introduced a regression, and fixing that regression has caused another regression... and I what I'm wondering is how many more regression fixing cycles it's going to take - how many regression fixes on top of other regression fixes are we going to end up seeing here.

Quoted 6 lines

The fact is, we never properly understood why your patch caused the regression I was seeing. If we don't understand it, then we can never say that we've fixed the problem properly. That is highlighted by the fact that fixing the regression I was seeing has caused another regression.

I agree with all these points.

But as I pointed out, reverting the original by-VA change, which has been there for almost a year now, has some risks of its own. If we don't understand the details of how this is broken, how can we be sure we don't break something else if we revert it again?

So I'm not arguing that reverting the original patch is unreasonable, just that doing so at this point in the cycle is risky, and that it would be better to queue the revert for v5.12, and only backport it after some soak time in -next. And in a sense, reinstating the cache_clean() before cache_off() already amounts to a partial revert of the queued fix.

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel