Re: [PATCH edk2-platforms 0/3] drop GUIDs from NOR flash bank descriptors

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

> On 22 Nov 2018, at 17:30, Laszlo Ersek <lersek@redhat.com> wrote:
> 
>> On 11/22/18 14:08, Ard Biesheuvel wrote:
>>> On Thu, 22 Nov 2018 at 12:37, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>>> 
>>> On Thu, 22 Nov 2018 at 12:19, chandni cherukuri
>>> <chandni.cherukuri@arm.com> wrote:
>>>> 
>>>> On Thu, Nov 22, 2018 at 1:20 PM Ard Biesheuvel
>>>> <ard.biesheuvel@linaro.org> wrote:
>>>>> 
>>>>>> On Thu, 22 Nov 2018 at 05:01, Thomas Abraham <thomas.abraham@arm.com> wrote:
>>>>>> 
>>>>>> Hi Ard,
>>>>>> 
>>>>>> On Thu, Nov 22, 2018 at 3:46 AM Ard Biesheuvel
>>>>>> <ard.biesheuvel@linaro.org> wrote:
>>>>>>> 
>>>>>>>> On Wed, 21 Nov 2018 at 14:48, Thomas Abraham <thomas.abraham@arm.com> wrote:
>>>>>>>> 
>>>>>>>> Hi Ard,
>>>>>>>> 
>>>>>>>> On Wed, Nov 21, 2018 at 5:31 PM Ard Biesheuvel
>>>>>>>> <ard.biesheuvel@linaro.org> 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".
> 

Yes. Yay for multitasking :-)

>> 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.
> 

I tracked this down to BdsLib (a remnant of the illustrious ARM bds), which is a bit of a mess. I’ll have some patches out shortly.