Re: [PATCH v3 1/2] EmbeddedPkg/AndroidBoot: boot android kernel from storage

[Jun Nie <jun.nie@linaro.org>]

On 2017年07月28日 21:06, Leif Lindholm wrote:
> On Fri, Jul 28, 2017 at 05:47:46PM +0800, Jun Nie wrote:
>> On 2017年07月27日 22:09, Leif Lindholm wrote:
>>> On Thu, Jul 27, 2017 at 06:07:19PM +0800, Jun Nie wrote:
>>>> Add an android kernel loader that could load kernel from storage
>>>> device. This patch is from Haojian's code as below link. The minor
>>>> change is that alternative dtb is searched in second loader binary
>>>> of Android bootimage if dtb is not found after Linux kernel.
>>>> https://patches.linaro.org/patch/94683/
>>>>
>>>> This android boot image BDS add addtitional cmdline/dtb/ramfs
>>>> support besides kernel that is introduced by Android boot header.
>>>
>>> Getting there. A few more comments below.
>>>
>>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>>> Signed-off-by: Jun Nie <jun.nie@linaro.org>
>>>> ---
>>>>   ArmPkg/Include/Library/BdsLib.h                    |   3 +
>>>>   ArmPkg/Library/BdsLib/BdsFilePath.c                |   3 -
>>>>   .../Application/AndroidBoot/AndroidBootApp.c       | 127 +++++++
>>>>   .../Application/AndroidBoot/AndroidBootApp.inf     |  64 ++++
>>>>   .../Application/AndroidFastboot/AndroidBootImg.c   |  35 +-
>>>>   .../AndroidFastboot/AndroidFastbootApp.h           |   1 +
>>>>   .../AndroidFastboot/Arm/BootAndroidBootImg.c       |   2 +-
>>>>   EmbeddedPkg/Include/Library/AndroidBootImgLib.h    |  67 ++++
>>>>   EmbeddedPkg/Include/Protocol/AndroidBootImg.h      |  47 +++
>>>>   .../Library/AndroidBootImgLib/AndroidBootImgLib.c  | 419 +++++++++++++++++++++
>>>>   .../AndroidBootImgLib/AndroidBootImgLib.inf        |  48 +++
>>>>   11 files changed, 782 insertions(+), 34 deletions(-)
>>>>   create mode 100644 EmbeddedPkg/Application/AndroidBoot/AndroidBootApp.c
>>>>   create mode 100644 EmbeddedPkg/Application/AndroidBoot/AndroidBootApp.inf
>>>>   create mode 100644 EmbeddedPkg/Include/Library/AndroidBootImgLib.h
>>>>   create mode 100644 EmbeddedPkg/Include/Protocol/AndroidBootImg.h
>>>>   create mode 100644 EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
>>>>   create mode 100644 EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.inf
>>>>
> 
>>>> diff --git a/EmbeddedPkg/Application/AndroidBoot/AndroidBootApp.c b/EmbeddedPkg/Application/AndroidBoot/AndroidBootApp.c
>>>> new file mode 100644
>>>> index 0000000..2de1d8a
>>>> --- /dev/null
>>>> +++ b/EmbeddedPkg/Application/AndroidBoot/AndroidBootApp.c
>>>> @@ -0,0 +1,127 @@
>>>> +/** @file
>>>> +
>>>> +  Copyright (c) 2013-2014, ARM Ltd. All rights reserved.<BR>
>>>> +  Copyright (c) 2017, Linaro. All rights reserved.
>>>> +
>>>> +  This program and the accompanying materials
>>>> +  are licensed and made available under the terms and conditions of the BSD License
>>>> +  which accompanies this distribution.  The full text of the license may be found at
>>>> +  http://opensource.org/licenses/bsd-license.php
>>>> +
>>>> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
>>>> +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>>>> +
>>>> +**/
>>>> +
>>>> +#include <Library/AndroidBootImgLib.h>
>>>> +#include <Library/BaseMemoryLib.h>
>>>> +#include <Library/BdsLib.h>
>>>> +#include <Library/DebugLib.h>
>>>> +#include <Library/DevicePathLib.h>
>>>> +#include <Library/MemoryAllocationLib.h>
>>>> +#include <Library/UefiBootServicesTableLib.h>
>>>> +
>>>> +#include <Protocol/BlockIo.h>
>>>> +#include <Protocol/DevicePathFromText.h>
>>>> +
>>>> +EFI_STATUS
>>>> +EFIAPI
>>>> +AndroidBootAppEntryPoint (
>>>> +  IN EFI_HANDLE                            ImageHandle,
>>>> +  IN EFI_SYSTEM_TABLE                      *SystemTable
>>>> +  )
>>>> +{
>>>> +  EFI_STATUS                          Status;
>>>> +  CHAR16                              *BootPathStr;
>>>> +  EFI_DEVICE_PATH_FROM_TEXT_PROTOCOL  *EfiDevicePathFromTextProtocol;
>>>> +  EFI_DEVICE_PATH                     *DevicePath;
>>>> +  EFI_DEVICE_PATH_PROTOCOL            *Node, *NextNode;
>>>> +  EFI_BLOCK_IO_PROTOCOL               *BlockIo;
>>>> +  UINT32                              MediaId, BlockSize;
>>>> +  VOID                                *Buffer;
>>>> +  EFI_HANDLE                          Handle;
>>>> +  UINTN                               Size;
>>>> +
>>>> +  BootPathStr = (CHAR16 *)PcdGetPtr (PcdAndroidBootDevicePath);
>>>> +  ASSERT (BootPathStr != NULL);
>>>> +  Status = gBS->LocateProtocol (&gEfiDevicePathFromTextProtocolGuid, NULL,
>>>> +                                (VOID **)&EfiDevicePathFromTextProtocol);
>>>> +  ASSERT_EFI_ERROR(Status);
>>>> +  DevicePath = (EFI_DEVICE_PATH *)EfiDevicePathFromTextProtocol->ConvertTextToDevicePath (BootPathStr);
>>>> +  ASSERT (DevicePath != NULL);
>>>> +
>>>> +  /* Find DevicePath node of Partition */
>>>> +  NextNode = DevicePath;
>>>> +  while (1) {
>>>
>>> Should this not be while (NextNode != NULL), with some check that the
>>> node was found before progressing?
>>
>> (NextNode != NULL) is valid check.
>> The code check node before progressing as below, doesn't it?
> 
> My point is that if you never match the "if (IS_DEVICE_PATH_NODE "
> condition, this loop will never terminate.
> 
> And if we update the loop condition to fix that, we end up calling
> LocateDevicePath with a known bad parameter.

Yeah, besides the check in while(), I should add check before while loop.
> 
>>>
>>>> +    Node = NextNode;
>>>> +    if (IS_DEVICE_PATH_NODE (Node, MEDIA_DEVICE_PATH, MEDIA_HARDDRIVE_DP)) {
>>>> +      break;
>>>> +    }
>>>> +    NextNode = NextDevicePathNode (Node);
>>>> +  }
>>>> +
>>>> +  Status = gBS->LocateDevicePath (&gEfiDevicePathProtocolGuid,
>>>> +                                  &DevicePath, &Handle);
>>>
>>> And should this not use &Node rather than &DevicePath?
>> I suppose we should return error if no MEDIA_HARDDRIVE_DP node is found. I
>> ever did test and found original DevicePath should be used here as a full
>> device path, otherwise the boot image cannot be found.
> 
> So does this mean that the section that initializes "Node" to a
> MEDIA_HARDDRIVE_DP is only there to validate DevicePath?
> If so, I think it should be split out into a static helper function,
> called ValidateDevicePath or similar, with a description of what
> condition it is trying to verify.

Yes, I think it is a validation. Will split it to a helper function.
> 
>>>
>>>> +  if (EFI_ERROR (Status)) {
>>>> +    return Status;
>>>> +  }
>>>> +
>>>> +  Status = gBS->OpenProtocol (
>>>> +                  Handle,
>>>> +                  &gEfiBlockIoProtocolGuid,
>>>> +                  (VOID **) &BlockIo,
>>>> +                  gImageHandle,
>>>> +                  NULL,
>>>> +                  EFI_OPEN_PROTOCOL_GET_PROTOCOL
>>>> +                  );
>>>> +  if (EFI_ERROR (Status)) {
>>>> +    DEBUG ((EFI_D_ERROR, "Failed to get BlockIo: %r\n", Status));
>>>> +    return Status;
>>>> +  }
>>>> +
>>>> +  MediaId = BlockIo->Media->MediaId;
>>>> +  BlockSize = BlockIo->Media->BlockSize;
>>>> +  Buffer = AllocatePages (EFI_SIZE_TO_PAGES (sizeof(ANDROID_BOOTIMG_HEADER)));
>>>> +  if (Buffer == NULL) {
>>>> +    return EFI_BUFFER_TOO_SMALL;
>>>> +  }
>>>> +  /* Load header of boot.img */
>>>> +  Status = BlockIo->ReadBlocks (
>>>> +                      BlockIo,
>>>> +                      MediaId,
>>>> +                      0,
>>>> +                      BlockSize,
>>>> +                      Buffer
>>>> +                      );
>>>> +  Status = AbootimgGetImgSize (Buffer, &Size);
>>>
>>> AndroidBootImgGetImageSize.
>>
>> Will do.
>>>
>>> (The "img" would normally be expected to be expanded to "Image", but
>>> it appears "boot.img" is basically the official name for this format.)
>>>
>>>> +  if (EFI_ERROR (Status)) {
>>>> +    DEBUG ((EFI_D_ERROR, "Failed to get Abootimg Size: %r\n", Status));
>>>> +    return Status;
>>>> +  }
>>>> +  Size = ALIGN_VALUE (Size, BlockSize);
>>>> +  FreePages (Buffer, EFI_SIZE_TO_PAGES (sizeof(ANDROID_BOOTIMG_HEADER)));
>>>> +
>>>> +  /* Both PartitionStart and PartitionSize are counted as block size. */
>>>> +  Buffer = AllocatePages (EFI_SIZE_TO_PAGES (Size));
>>>> +  if (Buffer == NULL) {
>>>> +    return EFI_BUFFER_TOO_SMALL;
>>>> +  }
>>>> +
>>>> +  /* Load header of boot.img */
>>>> +  Status = BlockIo->ReadBlocks (
>>>> +                      BlockIo,
>>>> +                      MediaId,
>>>> +                      0,
>>>> +                      Size,
>>>> +                      Buffer
>>>> +                      );
>>>> +  if (EFI_ERROR (Status)) {
>>>> +    DEBUG ((EFI_D_ERROR, "Failed to read blocks: %r\n", Status));
>>>> +    goto EXIT;
>>>> +  }
>>>> +
>>>> +  Status = AbootimgBoot (Buffer, Size);
>>>
>>> AndroidBootImgBoot.
>>
>> Will do.
>>>
>>>> +
>>>> +EXIT:
>>>> +  return Status;
>>>> +}
> 
> 
>>>> diff --git a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
>>>> new file mode 100644
>>>> index 0000000..72c6322
>>>> --- /dev/null
>>>> +++ b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
>>>> @@ -0,0 +1,419 @@
>>>> +/** @file
>>>> +
>>>> +  Copyright (c) 2013-2014, ARM Ltd. All rights reserved.<BR>
>>>> +  Copyright (c) 2017, Linaro. All rights reserved.
>>>> +
>>>> +  This program and the accompanying materials
>>>> +  are licensed and made available under the terms and conditions of the BSD License
>>>> +  which accompanies this distribution.  The full text of the license may be found at
>>>> +  http://opensource.org/licenses/bsd-license.php
>>>> +
>>>> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
>>>> +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>>>> +
>>>> +**/
>>>> +
>>>> +#include <libfdt.h>
>>>> +#include <Library/AndroidBootImgLib.h>
>>>> +#include <Library/PrintLib.h>
>>>> +#include <Library/UefiBootServicesTableLib.h>
>>>> +#include <Library/UefiLib.h>
>>>> +
>>>> +#include <Protocol/AndroidBootImg.h>
>>>> +#include <Protocol/LoadedImage.h>
>>>> +
>>>> +#include <libfdt.h>
>>>> +
>>>> +#define FDT_ADDITIONAL_ENTRIES_SIZE 0x400
>>>> +
>>>> +typedef struct {
>>>> +  MEMMAP_DEVICE_PATH                      Node1;
>>>> +  EFI_DEVICE_PATH_PROTOCOL                End;
>>>> +} MEMORY_DEVICE_PATH;
>>>> +
>>>> +STATIC ABOOTIMG_PROTOCOL                 *mAbootimg;
>>>
>>> mAndroidBootImg.
>>
>> Will do.
>>>
>>>> +
>>>> +STATIC CONST MEMORY_DEVICE_PATH MemoryDevicePathTemplate =
>>>
>>> Should also have an 'm'-prefix.
>>
>> Will do. But what 'm' prefix stand for?
>>>
>>>> +{
>>>> +  {
>>>> +    {
>>>> +      HARDWARE_DEVICE_PATH,
>>>> +      HW_MEMMAP_DP,
>>>> +      {
>>>> +        (UINT8)(sizeof (MEMMAP_DEVICE_PATH)),
>>>> +        (UINT8)((sizeof (MEMMAP_DEVICE_PATH)) >> 8),
>>>> +      },
>>>> +    }, // Header
>>>> +    0, // StartingAddress (set at runtime)
>>>> +    0  // EndingAddress   (set at runtime)
>>>> +  }, // Node1
>>>> +  {
>>>> +    END_DEVICE_PATH_TYPE,
>>>> +    END_ENTIRE_DEVICE_PATH_SUBTYPE,
>>>> +    { sizeof (EFI_DEVICE_PATH_PROTOCOL), 0 }
>>>> +  } // End
>>>> +};
>>>> +
>>>> +EFI_STATUS
>>>> +AbootimgGetImgSize (
>>>
>>> AndroidBootImgGetImageSize.
>>
>> Will do.
>>>
>>>> +  IN  VOID    *BootImg,
>>>> +  OUT UINTN   *ImgSize
>>>> +  )
>>>> +{
>>>> +  ANDROID_BOOTIMG_HEADER   *Header;
>>>> +
>>>> +  Header = (ANDROID_BOOTIMG_HEADER *) BootImg;
>>>> +
>>>> +  if (AsciiStrnCmp ((CONST CHAR8 *)Header->BootMagic, ANDROID_BOOT_MAGIC,
>>>> +                    ANDROID_BOOT_MAGIC_LENGTH) != 0) {
>>>> +    return EFI_INVALID_PARAMETER;
>>>> +  }
>>>> +
>>>> +  /* The page size is not specified, but it should be power of 2 at least */
>>>> +  ASSERT (IS_VALID_ANDROID_PAGE_SIZE (Header->PageSize));
>>>> +
>>>> +  /* Get real size of abootimg */
>>>> +  *ImgSize = ALIGN_VALUE (Header->KernelSize, Header->PageSize) +
>>>> +             ALIGN_VALUE (Header->RamdiskSize, Header->PageSize) +
>>>> +             ALIGN_VALUE (Header->SecondStageBootloaderSize, Header->PageSize) +
>>>> +             Header->PageSize;
>>>> +  return EFI_SUCCESS;
>>>> +}
>>>> +
>>>> +EFI_STATUS
>>>> +AbootimgGetKernelInfo (
>>>
>>> AndroidBootImgGetKernelInfo.
>>
>> Will do.
>>>
>>>> +  IN  VOID    *BootImg,
>>>> +  OUT VOID   **Kernel,
>>>> +  OUT UINTN   *KernelSize
>>>> +  )
>>>> +{
>>>> +  ANDROID_BOOTIMG_HEADER   *Header;
>>>> +
>>>> +  Header = (ANDROID_BOOTIMG_HEADER *) BootImg;
>>>> +
>>>> +  if (AsciiStrnCmp ((CONST CHAR8 *)Header->BootMagic, ANDROID_BOOT_MAGIC,
>>>> +                    ANDROID_BOOT_MAGIC_LENGTH) != 0) {
>>>> +    return EFI_INVALID_PARAMETER;
>>>> +  }
>>>> +
>>>> +  if (Header->KernelSize == 0) {
>>>> +    return EFI_NOT_FOUND;
>>>> +  }
>>>> +
>>>> +  ASSERT (IS_VALID_ANDROID_PAGE_SIZE (Header->PageSize));
>>>> +
>>>> +  *KernelSize = Header->KernelSize;
>>>> +  *Kernel = BootImg + Header->PageSize;
>>>> +  return EFI_SUCCESS;
>>>> +}
>>>> +
>>>> +EFI_STATUS
>>>> +AbootimgGetRamdiskInfo (
>>>> +  IN  VOID    *BootImg,
>>>> +  OUT VOID   **Ramdisk,
>>>> +  OUT UINTN   *RamdiskSize
>>>> +  )
>>>> +{
>>>> +  ANDROID_BOOTIMG_HEADER   *Header;
>>>> +  UINT8                    *BootImgBytePtr;
>>>> +
>>>> +  // Cast to UINT8 so we can do pointer arithmetic
>>>> +  BootImgBytePtr = (UINT8 *) BootImg;
>>>> +
>>>> +  Header = (ANDROID_BOOTIMG_HEADER *) BootImg;
>>>> +
>>>> +  if (AsciiStrnCmp ((CONST CHAR8 *)Header->BootMagic, ANDROID_BOOT_MAGIC,
>>>> +                    ANDROID_BOOT_MAGIC_LENGTH) != 0) {
>>>> +    return EFI_INVALID_PARAMETER;
>>>> +  }
>>>> +
>>>> +  ASSERT (IS_VALID_ANDROID_PAGE_SIZE (Header->PageSize));
>>>> +
>>>> +  *RamdiskSize = Header->RamdiskSize;
>>>> +
>>>> +  if (Header->RamdiskSize != 0) {
>>>> +    *Ramdisk = (VOID *) (BootImgBytePtr
>>>> +                 + Header->PageSize
>>>> +                 + ALIGN_VALUE (Header->KernelSize, Header->PageSize));
>>>> +  }
>>>> +  return EFI_SUCCESS;
>>>> +}
>>>> +
>>>> +EFI_STATUS
>>>> +AbootimgGetSecondBootLoaderInfo (
>>>
>>> AndroidBootImg...
>>
>> Will do.
>>>
>>>> +  IN  VOID    *BootImg,
>>>> +  OUT VOID   **Second,
>>>> +  OUT UINTN   *SecondSize
>>>> +  )
>>>> +{
>>>> +  ANDROID_BOOTIMG_HEADER   *Header;
>>>> +  UINT8                    *BootImgBytePtr;
>>>> +
>>>> +  // Cast to UINT8 so we can do pointer arithmetic
>>>> +  BootImgBytePtr = (UINT8 *) BootImg;
>>>> +
>>>> +  Header = (ANDROID_BOOTIMG_HEADER *) BootImg;
>>>> +
>>>> +  if (AsciiStrnCmp ((CONST CHAR8 *)Header->BootMagic, ANDROID_BOOT_MAGIC,
>>>> +                    ANDROID_BOOT_MAGIC_LENGTH) != 0) {
>>>> +    return EFI_INVALID_PARAMETER;
>>>> +  }
>>>> +
>>>> +  ASSERT (IS_VALID_ANDROID_PAGE_SIZE (Header->PageSize));
>>>> +
>>>> +  *SecondSize = Header->SecondStageBootloaderSize;
>>>> +
>>>> +  if (Header->SecondStageBootloaderSize != 0) {
>>>> +    *Second = (VOID *) (BootImgBytePtr
>>>> +                 + Header->PageSize
>>>> +                 + ALIGN_VALUE (Header->KernelSize, Header->PageSize)
>>>> +                 + ALIGN_VALUE (Header->RamdiskSize, Header->PageSize));
>>>> +  }
>>>> +  return EFI_SUCCESS;
>>>> +}
>>>> +
>>>> +EFI_STATUS
>>>> +AbootimgGetKernelArgs (
>>>
>>> AndroidBootImg...
>>
>> Will do.
>>>
>>>> +  IN  VOID    *BootImg,
>>>> +  OUT CHAR8   *KernelArgs
>>>> +  )
>>>> +{
>>>> +  ANDROID_BOOTIMG_HEADER   *Header;
>>>> +
>>>> +  Header = (ANDROID_BOOTIMG_HEADER *) BootImg;
>>>> +  AsciiStrnCpyS (KernelArgs, ANDROID_BOOTIMG_KERNEL_ARGS_SIZE, Header->KernelArgs,
>>>> +    ANDROID_BOOTIMG_KERNEL_ARGS_SIZE);
>>>> +
>>>> +  return EFI_SUCCESS;
>>>> +}
>>>> +
>>>> +EFI_STATUS
>>>> +AbootimgGetFdt (
>>>
>>> AndroidBootImg...
>>
>> Will do.
>>>
>>>> +  IN  VOID                  *BootImg,
>>>> +  IN  VOID                  **FdtBase
>>>> +  )
>>>> +{
>>>> +  UINTN                      SecondLoaderSize;
>>>> +  EFI_STATUS                 Status;
>>>> +
>>>> +  /* Check whether FDT is located in second boot loader as some vendor do so,
>>>
>>> It would be more correct to say "second boot loader region" than
>>> "second boot loader".
>>>
>>>> +   * because second loader is never used as far as I know. */
>>>> +  Status = AbootimgGetSecondBootLoaderInfo (
>>>> +          BootImg,
>>>> +          FdtBase,
>>>> +          &SecondLoaderSize
>>>> +          );
>>>> +  return Status;
>>>> +}
>>>> +
>>>> +EFI_STATUS
>>>> +AbootimgUpdateArgsFdt (
>>>
>>> AndroidBootImgUpdateKernelArgs
>>> (The arguments always come through Fdt, so I do not feel that needs to
>>> be explicitly pointed out.)
>> Command line come from Android boot image cmdline field, not from fdt in
>> most cases, if not all. I will split argument update function too.
>>>
>>> General comment: this function needs to be broken down into several
>>> smaller helper functions:
>>> - extract kernel arguments from boot.img
>>> - extract ramdisk information from boot.img
>>> - locate FDT
>>> - update FDT
>>
>> Will do.
>>>
>>>> +  IN  VOID                  *BootImg,
>>>> +  OUT VOID                  *KernelArgs
>>>> +  )
>>>> +{
>>>> +  VOID                      *Ramdisk;
>>>
>>> RamdiskData?
>> Yes, Ramdisk data start address.
>>>
>>>> +  UINT64                     Ramdisk64, RamdiskEnd64;
>>>
>>> RamdiskStart, RamDiskEnd?
>>
>> Yes, will do.
>>>
>>>> +  UINTN                      RamdiskSize;
>>>> +  CHAR8                      ImgKernelArgs[ANDROID_BOOTIMG_KERNEL_ARGS_SIZE];
>>>
>>> ImageKernelArgs
>>> or
>>> BootImgKernelArgs
>>
>> Will do.
>>>
>>>> +  INTN                       Err, NewFdtSize, chosen_node;
>>>
>>> ChosenNode
>>
>> Will do. I had thought fdt library related code shall follow the library
>> coding style :)
> 
> Yes, this is a tricky area.
> I'm considering putting together a wrapper library for common DT
> operations to abstract this away. But for now, I prefer keeping to
> TianoCore coding style everywhere except as is needed to call libfdt.
> 
>>>
>>>> +  EFI_STATUS                 Status;
>>>> +  EFI_PHYSICAL_ADDRESS       FdtBase, UpdatedFdtBase, NewFdtBase;
>>>> +  struct fdt_property       *prop;
>>>
>>> *Property.
>>
>> Will do.
>>>
>>>> +  int                        len;
>>>
>>> INTN Len;
>>
>> Will do.
>>>
>>>> +
>>>> +  Status = gBS->LocateProtocol (&gAndroidBootImgProtocolGuid, NULL,
>>>> +                                (VOID **) &mAbootimg);
>>>> +  if (EFI_ERROR (Status)) {
>>>> +    return Status;
>>>> +  }
>>>> +
>>>> +  Status = AbootimgGetKernelArgs (BootImg, ImgKernelArgs);
>>>> +  if (EFI_ERROR (Status)) {
>>>> +    return Status;
>>>> +  }
>>>> +  // Get kernel arguments from Android boot image
>>>> +  AsciiStrToUnicodeStrS (ImgKernelArgs, KernelArgs,
>>>> +                         ANDROID_BOOTIMG_KERNEL_ARGS_SIZE >> 1);
>>>> +  // Append platform kernel arguments
>>>> +  if(mAbootimg->AppendArgs) {
>>>> +    Status = mAbootimg->AppendArgs (KernelArgs,
>>>> +                                    ANDROID_BOOTIMG_KERNEL_ARGS_SIZE);
>>>> +    if (EFI_ERROR (Status)) {
>>>> +      return Status;
>>>> +    }
>>>> +  }
>>>> +
>>>> +  Status = EfiGetSystemConfigurationTable (&gFdtTableGuid, (VOID **)&FdtBase);
>>>> +  if (!EFI_ERROR (Status)) {
>>>
>>> Should this not be
>>>    if (EFI_ERROR (Status) && Status != EFI_NOT_FOUND)
>>> ?
>>
>> I mean, if fdt is found, we shall return to avoid installing another fdt.
> 
> An FDT presented to you by firmware is just the hardware description.
> Any command line or initrd updates that are required will still need
> to happen in order to boot. So the same manipulations that happen to
> the DT embedded in boot.img need to happen to one presented via a
> configuration table.
> 
>> But actually, I expect fdt is tied with kernel in Android boot image in
>> standard Android boot image usage cases.
>> Though it is agreed to decouple fdt and kernel in community in 2013,
>> Android boot image format has been decided several years before that :) .
>>
>> We can make change in future if Android boot image usage case changes.
>> Maybe I can add a warning message to highlight the new case.
> 
> No.
> 
> We can tolerate booting broken existing images, but we should not
> design to intentionally break things ourselves.
> 
> I guess as this is an application, you could even add a command-line
> option to let you override an existing registered DT with one embedded
> in boot.img.
> 
> But ignoring an existing registered DT is not an option.

So you suggest to override existing registered fdt data with the one in 
boot.img. How to add a command-line, in kernel argument that is embedded 
in boot.img? It is a bit strange if so.
I prefer to override existing registered fdt data directly in current 
Android boot image usage.
> 
>>>> +    return Status;
>>>> +  }
>>>> +
>>>> +  Status = AbootimgGetFdt (BootImg, (VOID **)&FdtBase);
>>>> +  if (EFI_ERROR (Status)) {
>>>> +    return Status;
>>>> +  }
>>>> +  Err = fdt_check_header ((VOID*)(UINTN)FdtBase);
>>>> +  if (Err != 0) {
>>>> +    DEBUG ((DEBUG_ERROR, "ERROR: Device Tree header not valid (Err:%d)\n",
>>>> +           Err));
>>>> +    return EFI_INVALID_PARAMETER;
>>>> +  }
>>>> +
>>>> +  Status = AbootimgGetRamdiskInfo (
>>>> +            BootImg,
>>>> +            &Ramdisk,
>>>> +            &RamdiskSize
>>>> +            );
>>>> +  if (EFI_ERROR (Status)) {
>>>> +    return Status;
>>>> +  }
>>>> +
>>>> +  NewFdtSize = (UINTN)fdt_totalsize ((VOID*)(UINTN)(FdtBase))
>>>> +               + FDT_ADDITIONAL_ENTRIES_SIZE;
>>>> +  Status = gBS->AllocatePages (AllocateAnyPages, EfiBootServicesData,
>>>> +                  EFI_SIZE_TO_PAGES (NewFdtSize), &UpdatedFdtBase);
>>>> +  if (EFI_ERROR (Status)) {
>>>> +    DEBUG ((EFI_D_WARN, "Warning: Failed to reallocate FDT, err %d.\n",
>>>> +           Status));
>>>> +    return Status;
>>>> +  }
>>>> +
>>>> +  // Load the Original FDT tree into the new region
>>>> +  Err = fdt_open_into((VOID*)FdtBase, (VOID*)UpdatedFdtBase, NewFdtSize);
>>>> +  if (Err) {
>>>> +    DEBUG ((EFI_D_ERROR, "fdt_open_into(): %a\n", fdt_strerror (Err)));
>>>> +    Status = EFI_INVALID_PARAMETER;
>>>> +    goto Fdt_Exit;
>>>> +  }
>>>> +
>>>> +  Ramdisk64 = cpu_to_fdt64((UINT64)Ramdisk);
>>>> +  RamdiskEnd64 = cpu_to_fdt64((UINT64)(Ramdisk + RamdiskSize));
>>>> +
>>>> +  chosen_node = fdt_subnode_offset ((const void *)UpdatedFdtBase, 0, "chosen");
>>>> +  if (chosen_node < 0) {
>>>> +    chosen_node = fdt_add_subnode((void *)UpdatedFdtBase, 0, "chosen");
>>>> +      if (chosen_node < 0) {
>>>> +        DEBUG ((EFI_D_ERROR, "Failed to find chosen node in fdt!\n"));
>>>> +        goto Fdt_Exit;
>>>> +    }
>>>> +  }
>>>> +  prop = fdt_get_property_w((void *)UpdatedFdtBase, chosen_node,
>>>> +                            "linux,initrd-start", &len);
>>>> +  if (NULL == prop && len == -FDT_ERR_NOTFOUND) {
>>>> +    fdt_appendprop ((void *)UpdatedFdtBase, chosen_node,
>>>> +                    "linux,initrd-start", &Ramdisk64, sizeof (UINT64));
>>>> +  } else if (prop != NULL) {
>>>> +    fdt_setprop_u64((void *)UpdatedFdtBase, chosen_node,
>>>> +                    "linux,initrd-start", (uint64_t)Ramdisk64);
>>>> +  } else {
>>>> +    DEBUG ((EFI_D_ERROR, "Failed to append fdt prop initrd-start\n",
>>>> +           fdt_strerror (Err)));
>>>> +    Status = EFI_INVALID_PARAMETER;
>>>> +    goto Fdt_Exit;
>>>> +  }
>>>> +
>>>> +  prop = fdt_get_property_w((void *)UpdatedFdtBase, chosen_node,
>>>> +                            "linux,initrd-end", &len);
>>>> +  if (NULL == prop && len == -FDT_ERR_NOTFOUND) {
>>>> +    fdt_appendprop ((void *)UpdatedFdtBase, chosen_node,
>>>> +                    "linux,initrd-end", &RamdiskEnd64, sizeof (UINT64));
>>>> +  } else if (prop != NULL) {
>>>> +    fdt_setprop_u64((void *)UpdatedFdtBase, chosen_node,
>>>> +                    "linux,initrd-end", (uint64_t)RamdiskEnd64);
>>>> +  } else {
>>>> +    DEBUG ((EFI_D_ERROR, "Failed to append fdt prop initrd-end\n",
>>>> +           fdt_strerror (Err)));
>>>> +    Status = EFI_INVALID_PARAMETER;
>>>> +    goto Fdt_Exit;
>>>> +  }
>>>> +
>>>> +  if ( mAbootimg->UpdateDtb) {
>>>> +    Status = mAbootimg->UpdateDtb (UpdatedFdtBase, &NewFdtBase);
>>>> +    if (EFI_ERROR (Status)) {
>>>> +      goto Fdt_Exit;
>>>> +    }
>>>> +  }
>>>> +
>>>> +  //
>>>> +  // Sanity checks on the new FDT blob.
>>>> +  //
>>>> +  Err = fdt_check_header ((VOID*)(UINTN)NewFdtBase);
>>>
>>> I don't think this test is needed. The state of the FDT is completely
>>> under our control at this point. The only thing it could uncover would
>>> be a stray pointer, or a bug in libfdt.
>>
>> Yes. We should check just after Fdt is read from boot image.
> 
> Or existing in configuration table.
> (As is already done.)
> 
> /
>      Leif
> 
>>>
>>>> +  if (Err != 0) {
>>>> +    Print (L"ERROR: Device Tree header not valid (err:%d)\n", Err);
>>>> +    return EFI_INVALID_PARAMETER;
>>>> +  }
>>>> +
>>>> +  Status = gBS->InstallConfigurationTable (
>>>> +                  &gFdtTableGuid,
>>>> +                  (VOID *)(UINTN)NewFdtBase
>>>> +                  );
>>>> +  if (EFI_ERROR (Status)) {
>>>> +    goto Fdt_Exit;
>>>> +  }
>>>> +  return Status;
>>>
>>> This is preference only, but I think
>>>    if (!EFI_ERROR (Status)) {
>>>      return EFI_SUCCESS;
>>>    }
>>> would be more clear.
>>
>> Will do.
>>>
>>>> +
>>>> +Fdt_Exit:
>>>> +  gBS->FreePages (UpdatedFdtBase, EFI_SIZE_TO_PAGES (NewFdtSize));
>>>> +  return Status;
>>>> +}
>>>> +
>>>> +EFI_STATUS
>>>> +AbootimgBoot (
>>>
>>> AndroidBootImgBoot
>>
>> Will do.
>>>
>>>> +  IN VOID                            *Buffer,
>>>> +  IN UINTN                            BufferSize
>>>> +  )
>>>> +{
>>>> +  EFI_STATUS                          Status;
>>>> +  VOID                               *Kernel;
>>>> +  UINTN                               KernelSize;
>>>> +  MEMORY_DEVICE_PATH                  KernelDevicePath;
>>>> +  EFI_HANDLE                          ImageHandle;
>>>> +  VOID                               *NewKernelArg;
>>>> +  EFI_LOADED_IMAGE_PROTOCOL          *ImageInfo;
>>>> +
>>>> +  Status = AbootimgGetKernelInfo (
>>>> +            Buffer,
>>>> +            &Kernel,
>>>> +            &KernelSize
>>>> +            );
>>>> +  if (EFI_ERROR (Status)) {
>>>> +    return Status;
>>>> +  }
>>>> +
>>>> +  NewKernelArg = AllocateZeroPool (ANDROID_BOOTIMG_KERNEL_ARGS_SIZE);
>>>> +  if (NewKernelArg == NULL) {
>>>> +    DEBUG ((DEBUG_ERROR, "Fail to allocate memory\n"));
>>>> +    return EFI_OUT_OF_RESOURCES;
>>>> +  }
>>>> +
>>>> +  Status = AbootimgUpdateArgsFdt (Buffer, NewKernelArg);
>>>> +  if (EFI_ERROR (Status)) {
>>>> +    FreePool (NewKernelArg);
>>>> +    return EFI_INVALID_PARAMETER;
>>>
>>> return Status?
>>>
>> Will do.
>>> /
>>>      Leif
>>>
>>>> +  }
>>>> +
>>>> +  KernelDevicePath = MemoryDevicePathTemplate;
>>>> +
>>>> +  KernelDevicePath.Node1.StartingAddress = (EFI_PHYSICAL_ADDRESS)(UINTN) Kernel;
>>>> +  KernelDevicePath.Node1.EndingAddress   = (EFI_PHYSICAL_ADDRESS)(UINTN) Kernel
>>>> +                                           + KernelSize;
>>>> +
>>>> +  Status = gBS->LoadImage (TRUE, gImageHandle,
>>>> +                           (EFI_DEVICE_PATH *)&KernelDevicePath,
>>>> +                           (VOID*)(UINTN)Kernel, KernelSize, &ImageHandle);
>>>> +
>>>> +  // Set kernel arguments
>>>> +  Status = gBS->HandleProtocol (ImageHandle, &gEfiLoadedImageProtocolGuid,
>>>> +                                (VOID **) &ImageInfo);
>>>> +  ImageInfo->LoadOptions = NewKernelArg;
>>>> +  ImageInfo->LoadOptionsSize = StrLen (NewKernelArg) * sizeof (CHAR16);
>>>> +
>>>> +  // Before calling the image, enable the Watchdog Timer for  the 5 Minute period
>>>> +  gBS->SetWatchdogTimer (5 * 60, 0x10000, 0, NULL);
>>>> +  // Start the image
>>>> +  Status = gBS->StartImage (ImageHandle, NULL, NULL);
>>>> +  // Clear the Watchdog Timer if the image returns
>>>> +  gBS->SetWatchdogTimer (0, 0x10000, 0, NULL);
>>>> +  return EFI_SUCCESS;
>>>> +}