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

[Laszlo Ersek <lersek@redhat.com>]

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

> 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