From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 42CE6EB64DD for ; Tue, 11 Jul 2023 19:13:50 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 01BFF868CE; Tue, 11 Jul 2023 21:13:40 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=reject dis=none) header.from=google.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (2048-bit key; unprotected) header.d=google.com header.i=@google.com header.b="OVrVQrd4"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id DF8E4868BC; Tue, 11 Jul 2023 21:13:38 +0200 (CEST) Received: from mail-ed1-x52f.google.com (mail-ed1-x52f.google.com [IPv6:2a00:1450:4864:20::52f]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id 8D2BC868C4 for ; Tue, 11 Jul 2023 21:13:35 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=reject dis=none) header.from=google.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=sjg@google.com Received: by mail-ed1-x52f.google.com with SMTP id 4fb4d7f45d1cf-5149aafef44so7635675a12.0 for ; Tue, 11 Jul 2023 12:13:35 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20221208; t=1689102815; x=1691694815; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=9AEahzXY9kUY2RO3zh7l/z1+xm9+9lSN/oZCDe5RhCc=; b=OVrVQrd4QmXZcALvx1aqCltEtuuSdxIrqU3QKfK5aufBc/mypSIb6AiJSJkQTqDWuU +jrSxKaUzi8q+MBp2DOHChSC8W5R6G5I2atTCn8o98Sr/lrxUhN9dzej8LB1SOA/gXd+ fM9z4wcNr0eLjUZHsQkFWsq7EfccLNWlGFVr8ITyPhgZK978Gw4XXY448oIQ5PATkPfR FiaJkMZA6elghzxW46NLhAFCvubbwyNh4bIBX9G5oRbzzT7cSfRqV4uaV0x8dniEiFr4 hRQKHBLwCtnWcoC/hAmC2oLYq566NxxoEQo1uYmkl36WZrJtJ1sy0fHm/wkpTHte2ckd AyDg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1689102815; x=1691694815; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=9AEahzXY9kUY2RO3zh7l/z1+xm9+9lSN/oZCDe5RhCc=; b=DG7unJzhNNdKIHo4nodEMUG5F6/TJ1f/XyzrRAkDoSMPnQPA42uYWw5Q/oyepv7nJX h6T+/R3R2Guc8w65gXa7Uol8DV702X1NvhksjKorIlB3SSASqe1HHJuWVxCNbwGd6fdj 1kUEiqCXnl1842G9WWaWyGeowT+Sxa2KbOeEt65qtnA0VidMkUBSU7vELZ2dBPvgLilD JXzkWO4lYQmS62N0kB5ChLZr8GnSIC5J2Oe2RI9qLJqw2/xBFcOst97yToIwq3V9pFo8 xc32Von5g3+A4xSGWZ68iWfxBXTxWqH5As5KkrjJTN+3sCD5l8D9DHJ1V814FQhRKgew kC8Q== X-Gm-Message-State: ABy/qLYvyGtc9BD/GUTvtmBA0IuS6W2YzmesEMOeTxWdocD5KnQVTqNm q1OYkSta69sTJIMkjKQpQPqe9UeJQQqd2XiY10cfhw== X-Google-Smtp-Source: APBJJlGZAoVr3Wi3xEOoD/6cejq5lJm64D2V0IwKoOWq6olZsjlImLt4vFUWCsbpyrfkLAETQ8pLyx1w7JXTPziv0YA= X-Received: by 2002:a17:906:5349:b0:993:fba5:cdef with SMTP id j9-20020a170906534900b00993fba5cdefmr8542054ejo.8.1689102814803; Tue, 11 Jul 2023 12:13:34 -0700 (PDT) MIME-Version: 1.0 References: <20230710201339.GZ148062@bill-the-cat> <8fa4eead45bf25f5fa4611c71e68d21a94b094a7.camel@gmail.com> In-Reply-To: <8fa4eead45bf25f5fa4611c71e68d21a94b094a7.camel@gmail.com> From: Simon Glass Date: Tue, 11 Jul 2023 13:13:23 -0600 Message-ID: Subject: Re: [BUG] fdt_pack_reg in common/fdt_support.c can cause crash from unaligned access To: David Virag Cc: Tom Rini , u-boot@lists.denx.de Content-Type: text/plain; charset="UTF-8" X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.8 at phobos.denx.de X-Virus-Status: Clean Hi David, On Tue, 11 Jul 2023 at 04:34, David Virag wrote: > > On Mon, 2023-07-10 at 15:38 -0600, Simon Glass wrote: > > Hi, > > > > On Mon, 10 Jul 2023 at 14:13, Tom Rini 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 > > > > 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