Hi David, On Tue, 11 Jul 2023 at 04:34, David Virag <[email protected]> wrote: > > On Mon, 2023-07-10 at 15:38 -0600, Simon Glass wrote: > > Hi, > > > > On Mon, 10 Jul 2023 at 14:13, Tom Rini <[email protected]> wrote: > > > > > > On Mon, Jul 10, 2023 at 01:45:46PM -0600, Simon Glass wrote: > > > > Hi David, > > > > > > > > On Sun, 9 Jul 2023 at 19:11, David Virag > > > > <[email protected]> wrote: > > > > > > > > > > Hi, > > > > > > > > > > I'm trying to port U-Boot to a new board (Samsung JACKPOTLTE, > > > > > ARMv8, > > > > > Exynos7885) but when CONFIG_ARCH_FIXUP_FDT_MEMORY is enabled, > > > > > the bootm > > > > > command leads to an unaligned memory access, which results in a > > > > > synchronous abort. > > > > > > > > > > After a long debugging session, I concluded that fdt_pack_reg > > > > > in > > > > > common/fdt_support.c writes to unaligned addresses in its for > > > > > loop. > > > > > In the case of address_cells being 2, and size_cells being 1, > > > > > the > > > > > buffer pointer gets incremented by 12 in each loop, making the > > > > > second > > > > > iteration (i=1) write a 64bit value to a non 64bit aligned > > > > > address. > > > > > > > > > > Turning the alignment check enable bit (A) off in SCTLR makes > > > > > the > > > > > function work as intended. I couldn't find code that touches > > > > > this bit, > > > > > but I may have missed something. I don't think writing in two > > > > > parts > > > > > should be the fix, but something should be done about this. As > > > > > far as I > > > > > understand, any arm64 board that has this bit turned on, either > > > > > from > > > > > previous code or just the initial status of the bit after power > > > > > on, > > > > > could crash here. > > > > > > > > > > This is on top of the latest commit as of now > > > > > (0beb649053b86b2cfd5cf55a0fc68bc2fe91a430) > > > > > > > > > > What should be done here? > > > > > > > > +Tom Rini > > > > > > ... I was hoping you had an idea Simon. Is this part of the code we > > > share with libfdt itself, or one of the helpers we made? > > Since the offending code is in common/fdt_support.c and not somewhere > in lib/libfdt, I'd say it's one of the helpers. > > > > > Hmmm, is the DT itself 64-bit aligned? It needs to be. > > It should be. I forgot to mention, but I'm loading the DT itself from > the FIT image to address 0x82000000. I'm trying to boot a Linux kernel > with it. > > According to uart output, working FDT gets set to 0x82000000, then for > some reason gets loaded to 0x8f908000. Both are aligned, so this > shouldn't be a problem. > > Here is the uart output, maybe there's something interesting in it > (probably should've provided it earlier): > > ## Loading fdt from FIT Image at 89000000 ... > Using 'alarm' configuration > Trying 'fdt' fdt subimage > Description: DTB > Type: Flat Device Tree > Compression: uncompressed > Data Start: 0x8a5be9a0 > Data Size: 21936 Bytes = 21.4 KiB > Architecture: AArch64 > Load Address: 0x82000000 > Hash algo: sha1 > Hash value: b289ae4aab26ae514e06ca76b4ad8f3f9c6d6cab > Verifying Hash Integrity ... sha1+ OK > Loading fdt from 0x8a5be9a0 to 0x82000000 > Booting using the fdt blob at 0x82000000 > Working FDT set to 82000000 > Loading Kernel Image > Loading Ramdisk to 8f911000, end 8ffff885 ... OK > Loading Device Tree to 000000008f908000, end 000000008f9105af ... OK > Working FDT set to 8f908000 > "Synchronous Abort" handler, esr 0x96000061, far 0xffb4d28c > elr: 000000000001aec8 lr : 000000000001ae70 (reloc) > elr: 00000000fff88ec8 lr : 00000000fff88e70 > x0 : 0000000000000001 x1 : 0000000000000001 > x2 : 0000009000000000 x3 : 0000000090000000 > x4 : 000000000000000c x5 : 0000000000000008 > x6 : 0000000000000008 x7 : 000000008f908000 > x8 : 000000000000004c x9 : 00000000ffb4d0fc > x10: 0000000000000003 x11: 0000000000004e78 > x12: 00000000ffb4d1f8 x13: 000000008f908000 > x14: 00000000ffffffff x15: 00000000ffb4d448 > x16: 0000000000000000 x17: 0000000000000004 > x18: 00000000ffb4ecb0 x19: 00000000ffb4d28c > x20: 0000000000000010 x21: 0000000000000002 > x22: 00000000ffb4d390 x23: 00000000ffb4d410 > x24: 000000008f908000 x25: 00000000fffcbbc9 > x26: 00000000fff70834 x27: 0000000000000000 > x28: 0000000000000000 x29: 00000000ffb4d1f0 > > Code: b8666ac2 71000abf 54000181 dac00c62 (f9000262) > Resetting CPU ... > > resetting ... > > > From u-boot.map, addresses 000000000001aec8 in elr and 000000000001ae70 > in lr should be the fdt_pack_reg function (000000000001ae30). > > The thing is that the function above does access data unaligned, > since... well, the data is not always aligned there. With 64bit address > cells and 32bit size cells, even if the first reads are aligned, it > will shift in and out of being 32 bits off from aligned. > > We read 64 bits, increment the ptr by 64 bit, read 32 bits, increment > by 32 bits, and read 64 bits again, which is 96 bits from the start. > > > > > Looking at fdt_find_separate() it needs _end > > > > Looking at arch/arm/cpu/armv8/u-boot.lds I don't see an ALIGN before > > _end. > > > > So perhaps that is the problem? > > > > I don't know about this, but it's not related to the problem I'm > running into.
Thinking about this a bit more, there is no requirement that FDT properties start on an 64-bit byte boundary. The requirement for 32-bit. So I suspect the answer might be that we have a problem here, on ARM. One solution might be to add a helper like put_unaligned_be64() which uses a CONFIG to indicate whether 32-bit-aligned 64-bit read/write is supported, and either just does the write, or calls put_unaligned_be64(). Another option might be to adjust fdt_pack_reg() to write the cells one at a time. Regards, Simon
