From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.132.183.28; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id B05E821B02822 for ; Thu, 22 Nov 2018 08:30:58 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 6F5883138BAA; Thu, 22 Nov 2018 16:30:58 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-200.rdu2.redhat.com [10.10.120.200]) by smtp.corp.redhat.com (Postfix) with ESMTP id 166275DDFE; Thu, 22 Nov 2018 16:30:56 +0000 (UTC) To: Ard Biesheuvel , Chandni Cherukuri Cc: "edk2-devel@lists.01.org" , Nariman Poushin References: <20181121120145.3148-1-ard.biesheuvel@linaro.org> From: Laszlo Ersek Message-ID: Date: Thu, 22 Nov 2018 17:30:55 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.42]); Thu, 22 Nov 2018 16:30:58 +0000 (UTC) Subject: Re: [PATCH edk2-platforms 0/3] drop GUIDs from NOR flash bank descriptors X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 22 Nov 2018 16:30:59 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 11/22/18 14:08, Ard Biesheuvel wrote: > On Thu, 22 Nov 2018 at 12:37, Ard Biesheuvel wrote: >> >> On Thu, 22 Nov 2018 at 12:19, chandni cherukuri >> wrote: >>> >>> On Thu, Nov 22, 2018 at 1:20 PM Ard Biesheuvel >>> wrote: >>>> >>>> On Thu, 22 Nov 2018 at 05:01, Thomas Abraham wrote: >>>>> >>>>> Hi Ard, >>>>> >>>>> On Thu, Nov 22, 2018 at 3:46 AM Ard Biesheuvel >>>>> wrote: >>>>>> >>>>>> On Wed, 21 Nov 2018 at 14:48, Thomas Abraham wrote: >>>>>>> >>>>>>> Hi Ard, >>>>>>> >>>>>>> On Wed, Nov 21, 2018 at 5:31 PM Ard Biesheuvel >>>>>>> wrote: >>>>>>>> >>>>>>>> Align edk2-platform with upcoming changes to EDK2 to get rid of per-bank >>>>>>>> NOR flash GUIDs. >>>>>>>> >>>>>>>> Ard Biesheuvel (3): >>>>>>>> Platform/ARM: replace hardcoded VenHW() device paths referring to NOR >>>>>>>> flash >>>>>>>> Silicon/SynQuacer: drop per-bank NOR flash GUIDs >>>>>>>> Platform/ARM: drop per-bank NOR flash GUIDs >>>>>>>> >>>>>>>> Platform/ARM/JunoPkg/ArmJuno.dec | 2 +- >>>>>>>> Platform/ARM/JunoPkg/ArmJuno.dsc | 2 +- >>>>>>>> .../JunoPkg/Library/NorFlashJunoLib/NorFlashJuno.c | 2 -- >>>>>>>> .../ARM/SgiPkg/Library/NorFlashLib/NorFlashLib.c | 2 -- >>>>>>>> Platform/ARM/SgiPkg/SgiPlatform.dsc | 2 +- >>>>>>>> Platform/ARM/VExpressPkg/ArmVExpress-CTA15-A7.dsc | 2 +- >>>>>>>> Platform/ARM/VExpressPkg/ArmVExpress.dsc.inc | 2 +- >>>>>>>> .../NorFlashArmVExpressLib/NorFlashArmVExpress.c | 4 ---- >>>>>>>> .../SynQuacer/Drivers/Fip006Dxe/NorFlashDxe.c | 14 +++++++------- >>>>>>>> .../SynQuacer/Drivers/Fip006Dxe/NorFlashDxe.h | 3 +++ >>>>>>>> .../NorFlashSynQuacerLib/NorFlashSynQuacer.c | 6 ------ >>>>>>>> 11 files changed, 15 insertions(+), 26 deletions(-) >>>>>>> >>>>>>> Tested this patch series and "[PATCH v2 0/5] ArmPlatformPkg, >>>>>>> ArmVirtPkg: discover NOR flash banks from DTB" patch series on the >>>>>>> Juno board. With these patches applied, the boot fails on Juno board >>>>>>> with the following messages. I have not yet tried to debug the issue >>>>>>> but wanted to let you know this. >>>>>>> >>>>>>> [...] >>>>>>> Loading driver at 0x000F830C000 EntryPoint=0x000F831B2AC IScsiDxe.efi >>>>>>> add-symbol-file >>>>>>> /home/thopan01/devel/juno/uefi/uefi/edk2/Build/ArmJuno/DEBUG_GCC5/AARCH64/MdeModulePkg/Universal/Network/Udp4Dxe/Udp4Dxe/DEBUG/Udp4Dxe.dll >>>>>>> 0xF8300000 >>>>>>> Loading driver at 0x000F82FF000 EntryPoint=0x000F8306DF0 Udp4Dxe.efi >>>>>>> add-symbol-file >>>>>>> /home/thopan01/devel/juno/uefi/uefi/edk2/Build/ArmJuno/DEBUG_GCC5/AARCH64/Platform/ARM/Drivers/FdtPlatformDxe/FdtPlatformDxe/DEBUG/FdtPlatformDxe.dll >>>>>>> 0xF82EE000 >>>>>>> Loading driver at 0x000F82ED000 EntryPoint=0x000F82F76EC FdtPlatformDxe.efi >>>>>>> Found image: fip in block 5. >>>>>>> Found image: norkern in block 20. >>>>>>> Found image: ramdisk.img in block 116. >>>>>>> Found image: hdlcdclk in block 151. >>>>>>> Found image: selftest in block 152. >>>>>>> Found image: board.dtb in block 156. >>>>>>> Found image: scp_bl1 in block 249. >>>>>>> Found image: bl1 in block 251. >>>>>>> Found image: startup.nsh in block 252. >>>>>>> ASSERT [BootMonFs] >>>>>>> /home/thopan01/devel/juno/uefi/uefi/edk2/MdePkg/Library/BaseLib/String.c(173): >>>>>>> ((UINTN) String & 0x00000001) == 0 >>>>>>> > ... >> Could you please share the backtrace and the results of >> >> nm -n Build/ArmJuno/DEBUG_GCC5/AARCH64/Platform/ARM/Drivers/BootMonFs/BootMonFs/DEBUG/BootMonFs.dll >> >> for the failing case? >> > > It seems like BootMonFsOpenFile() is being called from FdtClientDxe I think you mean "FdtPlatformDxe". > with a misaligned CHAR16* argument for Filename. Yup, that's not an infrequent bug. Device paths are packed. Once we introduce a UINT8 field to a device path node, the rest of the nodes in the same device path will shift by 1 byte. If there is a CHAR16 array field in one of those "later" device path nodes, such as "FILEPATH_DEVICE_PATH.PathName", then it might go from naturally aligned to mis-aligned, or vice versa. Therefore, passing such CHAR16 fields to string functions in BaseLib, that is, right out of the containing device path node, is never portable. If it happens to work, that is by chance only. For example: https://bugzilla.tianocore.org/show_bug.cgi?id=1003 https://bugzilla.tianocore.org/show_bug.cgi?id=1008 I suggest grepping "FdtPlatformDxe" (or, well, rather "BootMonFs"!) for "PathName", and wherever it is passed to StrLen() -- because MdePkg/Library/BaseLib/String.c(173) is in StrLen() --, make sure the pathname is first re-aligned naturally, temporarily. For example, with AllocateCopyPool(). gBS->AllocatePool() guarantees 8-byte alignment per spec. While AllocatePool() / AllocateCopyPool() from MemoryAllocationLib don't explicitly guarantee the same "on the tin", I think that's not by design, but by omission. FWIW, for TianoCore BZs 1003 and 1008 above, we assumed AllocateCopyPool() would ensure 8-byte alignment as well. Thanks Laszlo