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

[Leif Lindholm <leif.lindholm@linaro.org>]

On Thu, Jul 06, 2017 at 09:29:05PM +0800, Jun Nie wrote:
> Add an android kernel loader that could load kernel from storage
> device.

UEFI can already load a kernel (with the EFI stub) from a storage
device. Please explain in the commit message how this support differs
from that.

What relation does this code have to AndroidFastbootApp?

> This patch is from Haojian's code.

Could you put a link to the origin of the code (preferably the
specific commit)?

> The minor change
> is that alternative dtb is searched in second loader binary of
> Android bootimage if dtb is not found after Linux kernel.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jun Nie <jun.nie@linaro.org>
> ---
>  .../Application/AndroidBoot/AndroidBootApp.c       | 129 ++++++++
>  .../Application/AndroidBoot/AndroidBootApp.inf     |  64 ++++
>  EmbeddedPkg/Include/Library/AbootimgLib.h          |  65 ++++
>  EmbeddedPkg/Include/Protocol/Abootimg.h            |  47 +++
>  EmbeddedPkg/Library/AbootimgLib/AbootimgLib.c      | 350 +++++++++++++++++++++
>  EmbeddedPkg/Library/AbootimgLib/AbootimgLib.inf    |  48 +++

For proper CamelCase, I think the library name should be
AndroidBootImageLib, and the filenames (and macros) updated similarly.

>  6 files changed, 703 insertions(+)
>  create mode 100644 EmbeddedPkg/Application/AndroidBoot/AndroidBootApp.c
>  create mode 100644 EmbeddedPkg/Application/AndroidBoot/AndroidBootApp.inf
>  create mode 100644 EmbeddedPkg/Include/Library/AbootimgLib.h
>  create mode 100644 EmbeddedPkg/Include/Protocol/Abootimg.h
>  create mode 100644 EmbeddedPkg/Library/AbootimgLib/AbootimgLib.c
>  create mode 100644 EmbeddedPkg/Library/AbootimgLib/AbootimgLib.inf
> 
> diff --git a/EmbeddedPkg/Application/AndroidBoot/AndroidBootApp.c b/EmbeddedPkg/Application/AndroidBoot/AndroidBootApp.c
> new file mode 100644
> index 0000000..9ed931b
> --- /dev/null
> +++ b/EmbeddedPkg/Application/AndroidBoot/AndroidBootApp.c
> @@ -0,0 +1,129 @@
> +/** @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/AbootimgLib.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>
> +
> +#define IS_DEVICE_PATH_NODE(node,type,subtype) (((node)->Type == (type)) && ((node)->SubType == (subtype)))

Would prefer for this to be moved into a common header file (BdsLib.h
?) rather than copied into multiple .c files.

> +
> +#define ALIGN(x, a)        (((x) + ((a) - 1)) & ~((a) - 1))

Rather use one of the ALIGN_ macros from Base.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) {
> +    Node = NextNode;
> +    if (IS_DEVICE_PATH_NODE (Node, MEDIA_DEVICE_PATH, MEDIA_HARDDRIVE_DP)) {
> +      break;
> +    }
> +    NextNode = NextDevicePathNode (Node);
> +  }
> +
> +  Status = gBS->LocateDevicePath (&gEfiDevicePathProtocolGuid, &DevicePath, &Handle);
> +  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 (1);

I dislike magic numbers even where the code is reasonably clear.
Could this be EFI_SIZE_TO_PAGES (ANDROID_BOOTIMG_HEADER) instead of
"1"?

> +  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);
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((EFI_D_ERROR, "Failed to get Abootimg Size: %r\n", Status));
> +    return Status;
> +  }
> +  Size = ALIGN (Size, BlockSize);

Although this is not an align operation, but rather a "round up"
operation. We have the NET_ROUNDUP macro already, but perhaps a new
generic one (and then redefine the NET_ROUNDUP value to point to
this)?

> +  FreePages (Buffer, 1);
> +
> +  /* 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);
> +
> +EXIT:
> +  return Status;
> +}
> diff --git a/EmbeddedPkg/Application/AndroidBoot/AndroidBootApp.inf b/EmbeddedPkg/Application/AndroidBoot/AndroidBootApp.inf
> new file mode 100644
> index 0000000..8f6c8186
> --- /dev/null
> +++ b/EmbeddedPkg/Application/AndroidBoot/AndroidBootApp.inf
> @@ -0,0 +1,64 @@
> +#/** @file
> +#
> +#  Copyright (c) 2013-2015, 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.
> +#
> +#
> +#**/
> +
> +[Defines]
> +  INF_VERSION                    = 0x00010019
> +  BASE_NAME                      = AndroidBootApp
> +  FILE_GUID                      = 3a738b36-b9c5-4763-abbd-6cbd4b25f9ff
> +  MODULE_TYPE                    = UEFI_APPLICATION
> +  VERSION_STRING                 = 1.0
> +  ENTRY_POINT                    = AndroidBootAppEntryPoint
> +
> +[Sources.common]
> +  AndroidBootApp.c
> +
> +[LibraryClasses]
> +  AbootimgLib
> +  BaseLib
> +  BaseMemoryLib
> +  BdsLib
> +  DebugLib
> +  DevicePathLib
> +  DxeServicesTableLib
> +  FdtLib
> +  MemoryAllocationLib
> +  PcdLib
> +  PrintLib
> +  UefiApplicationEntryPoint
> +  UefiBootServicesTableLib
> +  UefiLib
> +  UefiRuntimeServicesTableLib
> +
> +[Protocols]
> +  gAndroidFastbootPlatformProtocolGuid
> +  gEfiBlockIoProtocolGuid
> +  gEfiDevicePathFromTextProtocolGuid
> +  gEfiSimpleTextOutProtocolGuid
> +  gEfiSimpleTextInProtocolGuid
> +
> +[Packages]
> +  EmbeddedPkg/EmbeddedPkg.dec
> +  MdeModulePkg/MdeModulePkg.dec
> +  MdePkg/MdePkg.dec
> +
> +[Packages.ARM, Packages.AARCH64]
> +  ArmPkg/ArmPkg.dec
> +  ArmPlatformPkg/ArmPlatformPkg.dec
> +
> +[Guids]
> +  gFdtTableGuid
> +
> +[Pcd]
> +  gEmbeddedTokenSpaceGuid.PcdAndroidBootDevicePath
> diff --git a/EmbeddedPkg/Include/Library/AbootimgLib.h b/EmbeddedPkg/Include/Library/AbootimgLib.h
> new file mode 100644
> index 0000000..c0372d4
> --- /dev/null
> +++ b/EmbeddedPkg/Include/Library/AbootimgLib.h
> @@ -0,0 +1,65 @@
> +/** @file
> +
> +  Copyright (c) 2013-2014, ARM Ltd. All rights reserved.<BR>
> +  Copyright (c) 2017, Linaro.
> +
> +  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.
> +
> +**/
> +
> +#ifndef __ABOOTIMG_H__
> +#define __ABOOTIMG_H__
> +
> +#include <Library/BaseLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/MemoryAllocationLib.h>
> +
> +#include <Uefi/UefiBaseType.h>
> +#include <Uefi/UefiSpec.h>
> +
> +#define BOOTIMG_KERNEL_ARGS_SIZE          512

Is this value defined somewhere? Or is it just an arbitrary size?

> +
> +#define BOOT_MAGIC                        "ANDROID!"
> +#define BOOT_MAGIC_LENGTH                 (sizeof (BOOT_MAGIC) - 1)
> +
> +/* It's the value of arm64 efi stub kernel */
> +#define KERNEL_IMAGE_STEXT_OFFSET         0x12C
> +#define KERNEL_IMAGE_RAW_SIZE_OFFSET      0x130
> +
> +#define FDT_SIZE_OFFSET                   0x4

What are these offsets?

All of these names (BOOT/KERNEL/FDT) are a little bit to generic for
an exported header file. Need ANDROID_BOOT_ (or similar_) prefix.

> +
> +typedef struct {
> +  CHAR8   BootMagic[BOOT_MAGIC_LENGTH];
> +  UINT32  KernelSize;
> +  UINT32  KernelAddress;
> +  UINT32  RamdiskSize;
> +  UINT32  RamdiskAddress;
> +  UINT32  SecondStageBootloaderSize;
> +  UINT32  SecondStageBootloaderAddress;
> +  UINT32  KernelTaggsAddress;
> +  UINT32  PageSize;
> +  UINT32  Reserved[2];
> +  CHAR8   ProductName[16];
> +  CHAR8   KernelArgs[BOOTIMG_KERNEL_ARGS_SIZE];

Does the protocol not specify signedness of chars for strings?

> +  UINT32  Id[32];
> +} ANDROID_BOOTIMG_HEADER;

This looks identical to the definition in
Application/AndroidFastboot/AndroidBootImg.c, only missing the
#pragma pack(1) statement.

In general there looks like a lot of code duplication between these
two applications. Can this not be broken out into a common library
used by both?

> +
> +EFI_STATUS
> +AbootimgGetImgSize (
> +  IN  VOID    *BootImg,
> +  OUT UINTN   *ImgSize
> +  );
> +
> +EFI_STATUS
> +AbootimgBoot (
> +  IN VOID                   *Buffer,
> +  IN UINTN                   BufferSize
> +  );
> +
> +#endif /* __ABOOTIMG_H__ */
> diff --git a/EmbeddedPkg/Include/Protocol/Abootimg.h b/EmbeddedPkg/Include/Protocol/Abootimg.h
> new file mode 100644
> index 0000000..c85dad2
> --- /dev/null
> +++ b/EmbeddedPkg/Include/Protocol/Abootimg.h
> @@ -0,0 +1,47 @@
> +/** @file
> +
> +  Copyright (c) 2017, Linaro. All rights reserved.<BR>
> +
> +  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.
> +
> +**/
> +
> +#ifndef __ABOOTIMG_PROTOCOL_H__
> +#define __ABOOTIMG_PROTOCOL_H__
> +
> +//
> +// Protocol interface structure
> +//
> +typedef struct _ABOOTIMG_PROTOCOL    ABOOTIMG_PROTOCOL;
> +
> +//
> +// Function Prototypes
> +//
> +typedef
> +EFI_STATUS
> +(EFIAPI *ABOOTIMG_APPEND_KERNEL_ARGS) (
> +  IN CHAR16            *Args,
> +  IN UINTN              Size
> +  );
> +
> +typedef
> +EFI_STATUS
> +(EFIAPI *ABOOTIMG_UPDATE_DTB) (
> +  IN  EFI_PHYSICAL_ADDRESS    OrigDtbBase;
> +  OUT EFI_PHYSICAL_ADDRESS   *NewDtbBase;
> +  );
> +
> +struct _ABOOTIMG_PROTOCOL {
> +  ABOOTIMG_APPEND_KERNEL_ARGS        AppendArgs;
> +  ABOOTIMG_UPDATE_DTB                UpdateDtb;
> +};
> +
> +extern EFI_GUID gAbootimgProtocolGuid;
> +
> +#endif /* __ABOOTIMG_PROTOCOL_H__ */
> diff --git a/EmbeddedPkg/Library/AbootimgLib/AbootimgLib.c b/EmbeddedPkg/Library/AbootimgLib/AbootimgLib.c
> new file mode 100644
> index 0000000..ea30a01
> --- /dev/null
> +++ b/EmbeddedPkg/Library/AbootimgLib/AbootimgLib.c
> @@ -0,0 +1,350 @@
> +/** @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/AbootimgLib.h>
> +#include <Library/PrintLib.h>
> +#include <Library/UefiBootServicesTableLib.h>
> +#include <Library/UefiLib.h>
> +
> +#include <Protocol/Abootimg.h>
> +#include <Protocol/LoadedImage.h>
> +
> +#include <libfdt.h>
> +
> +// Check Val (unsigned) is a power of 2 (has only one bit set)
> +#define IS_POWER_OF_2(Val)                (Val != 0 && ((Val & (Val - 1)) == 0))

Missing parentheses in macro.

> +
> +typedef struct {
> +  MEMMAP_DEVICE_PATH                      Node1;
> +  EFI_DEVICE_PATH_PROTOCOL                End;
> +} MEMORY_DEVICE_PATH;
> +
> +STATIC ABOOTIMG_PROTOCOL                 *mAbootimg;
> +
> +STATIC CONST MEMORY_DEVICE_PATH MemoryDevicePathTemplate =
> +{
> +  {
> +    {
> +      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 (
> +  IN  VOID    *BootImg,
> +  OUT UINTN   *ImgSize
> +  )
> +{
> +  ANDROID_BOOTIMG_HEADER   *Header;
> +
> +  Header = (ANDROID_BOOTIMG_HEADER *) BootImg;
> +
> +  if (AsciiStrnCmp (Header->BootMagic, BOOT_MAGIC, BOOT_MAGIC_LENGTH) != 0) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  ASSERT (IS_POWER_OF_2 (Header->PageSize));

I am not convinced this is the right thing to check for.
Not every power of two is a valid page size.
Semantically, any macro here should really be called
IS_VALID_PAGE_SIZE().

> +
> +  /* 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 (
> +  IN  VOID    *BootImg,
> +  OUT VOID   **Kernel,
> +  OUT UINTN   *KernelSize
> +  )
> +{
> +  ANDROID_BOOTIMG_HEADER   *Header;
> +
> +  Header = (ANDROID_BOOTIMG_HEADER *) BootImg;
> +
> +  if (AsciiStrnCmp (Header->BootMagic, BOOT_MAGIC, BOOT_MAGIC_LENGTH) != 0) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  if (Header->KernelSize == 0) {
> +    return EFI_NOT_FOUND;
> +  }
> +
> +  ASSERT (IS_POWER_OF_2 (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 (Header->BootMagic, BOOT_MAGIC, BOOT_MAGIC_LENGTH) != 0) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  ASSERT (IS_POWER_OF_2 (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 (
> +  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 (Header->BootMagic, BOOT_MAGIC, BOOT_MAGIC_LENGTH) != 0) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  ASSERT (IS_POWER_OF_2 (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 (
> +  IN  VOID    *BootImg,
> +  OUT CHAR8   *KernelArgs
> +  )
> +{
> +  ANDROID_BOOTIMG_HEADER   *Header;
> +
> +  Header = (ANDROID_BOOTIMG_HEADER *) BootImg;
> +  AsciiStrnCpyS (KernelArgs, BOOTIMG_KERNEL_ARGS_SIZE, Header->KernelArgs,
> +    BOOTIMG_KERNEL_ARGS_SIZE);
> +
> +  return EFI_SUCCESS;
> +}
> +
> +EFI_STATUS
> +AbootimgInstallFdt (
> +  IN  VOID                  *BootImg,
> +  IN  EFI_PHYSICAL_ADDRESS   FdtBase,
> +  OUT VOID                  *KernelArgs
> +  )
> +{
> +  VOID                      *Ramdisk;
> +  UINTN                      RamdiskSize;
> +  CHAR8                      ImgKernelArgs[BOOTIMG_KERNEL_ARGS_SIZE];
> +  INTN                       err;
> +  EFI_STATUS                 Status;
> +  EFI_PHYSICAL_ADDRESS       NewFdtBase;
> +
> +  Status = gBS->LocateProtocol (&gAbootimgProtocolGuid, NULL, (VOID **) &mAbootimg);
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  Status = AbootimgGetRamdiskInfo (
> +            BootImg,
> +            &Ramdisk,
> +            &RamdiskSize
> +            );
> +  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, BOOTIMG_KERNEL_ARGS_SIZE >> 1);
> +  // Set the ramdisk in command line arguments
> +  UnicodeSPrint (
> +    (CHAR16 *)KernelArgs + StrLen (KernelArgs), BOOTIMG_KERNEL_ARGS_SIZE,
> +    L" initrd=0x%x,0x%x",
> +    (UINTN)Ramdisk, (UINTN)RamdiskSize
> +    );

This is not an appropriate way to set ramdisk. There are dedicated
"linux,initrd-start" and "linux,initrd-end" options in the chosen node.
See
http://git.savannah.gnu.org/cgit/grub.git/tree/grub-core/loader/arm64/linux.c#n79
for an example.

> +
> +  // Append platform kernel arguments
> +  Status = mAbootimg->AppendArgs (KernelArgs, BOOTIMG_KERNEL_ARGS_SIZE);

Am I missing something? Where is this AppendArgs function defined?

> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  Status = mAbootimg->UpdateDtb (FdtBase, &NewFdtBase);

Likewise, where is this function defined?

> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  //
> +  // Sanity checks on the new FDT blob.
> +  //
> +  err = fdt_check_header ((VOID*)(UINTN)NewFdtBase);
> +  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
> +                  );
> +  return Status;
> +}
> +
> +EFI_STATUS
> +AbootimgBoot (
> +  IN VOID                            *Buffer,
> +  IN UINTN                            BufferSize
> +  )
> +{
> +  EFI_STATUS                          Status;
> +  VOID                               *Kernel;
> +  UINTN                               KernelSize;
> +  VOID                               *SecondLoader;
> +  UINTN                               SecondLoaderSize;
> +  MEMORY_DEVICE_PATH                  KernelDevicePath;
> +  EFI_HANDLE                          ImageHandle;
> +  EFI_PHYSICAL_ADDRESS                FdtBase;
> +  VOID                               *NewKernelArg;
> +  EFI_LOADED_IMAGE_PROTOCOL          *ImageInfo;
> +  INTN                                Err;
> +
> +  Status = AbootimgGetKernelInfo (
> +            Buffer,
> +            &Kernel,
> +            &KernelSize
> +            );
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  /* For flatten image, Fdt is attached at the end of kernel.
> +     Get real kernel size.
> +   */

This is a completely broken usage model :(
Is this a requirement for this support?

I mean, the solution you have implemented is a nice way of dealing
with the mess, but per-kernel-image DT blobs were deprecated at kernel
summit 2013.

> +  KernelSize = *(UINT32 *)((EFI_PHYSICAL_ADDRESS)(UINTN)Kernel + KERNEL_IMAGE_STEXT_OFFSET) +
> +               *(UINT32 *)((EFI_PHYSICAL_ADDRESS)(UINTN)Kernel + KERNEL_IMAGE_RAW_SIZE_OFFSET);
> +
> +  NewKernelArg = AllocateZeroPool (BOOTIMG_KERNEL_ARGS_SIZE);
> +  if (NewKernelArg == NULL) {
> +    DEBUG ((DEBUG_ERROR, "Fail to allocate memory\n"));
> +    return EFI_OUT_OF_RESOURCES;
> +  }
> +
> +  /* FDT is at the end of kernel image */
> +  FdtBase = (EFI_PHYSICAL_ADDRESS)(UINTN)Kernel + KernelSize;
> +  //
> +  // Sanity checks on the original FDT blob.
> +  //
> +  Err = fdt_check_header ((VOID*)(UINTN)FdtBase);
> +  if (Err != 0) {
> +    /* Check whether FDT is located in second boot loader */

What is a second boot loader?

> +    Status = AbootimgGetSecondBootLoaderInfo (
> +            Buffer,
> +            &SecondLoader,
> +            &SecondLoaderSize
> +            );
> +    if (EFI_ERROR (Status)) {
> +      return Status;
> +    }
> +
> +    Err = fdt_check_header ((VOID*)(UINTN)SecondLoader);
> +    if (Err != 0) {
> +      DEBUG ((DEBUG_ERROR, "ERROR: Device Tree header not valid (Err:%d)\n", Err));
> +      return EFI_INVALID_PARAMETER;
> +    }
> +    FdtBase = (EFI_PHYSICAL_ADDRESS)SecondLoader;
> +  }
> +

The above two scenarios could do with broken out as separate helper
functions. Preferably also with a Pcd to determine whether the
application should even bother to go looking if a platform-provided
device tree was already installed.

> +  Status = AbootimgInstallFdt (Buffer, FdtBase, NewKernelArg);
> +  if (EFI_ERROR (Status)) {
> +    FreePool (NewKernelArg);
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  KernelDevicePath = MemoryDevicePathTemplate;
> +
> +  // Have to cast to UINTN before casting to EFI_PHYSICAL_ADDRESS in order to
> +  // appease GCC.

(Redundant comment. It's a common enough problem.)

> +  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);

Please split function call up over multiple lines.

> +
> +  // Set kernel arguments
> +  Status = gBS->HandleProtocol (ImageHandle, &gEfiLoadedImageProtocolGuid, (VOID **) &ImageInfo);

Please split function call up over multiple lines.

> +  ImageInfo->LoadOptions = NewKernelArg;
> +  ImageInfo->LoadOptionsSize = StrLen (NewKernelArg) * sizeof (CHAR16);
> +
> +  // Before calling the image, enable the Watchdog Timer for  the 5 Minute period

Please wrap comment at 80 characters. (Or delete the "for the 5 Minute
period bit based on the next comment.)

> +  gBS->SetWatchdogTimer (5 * 60, 0x0000, 0x00, NULL);

Please create a define for that timeout. ANDROID_BOOT_WATCHDOG_TIMEOUT
or something.
Or, hmm, given that the UEFI specification (2.7) says "The watchdog
must be set to a period of 5 minutes", this may even be worth putting
into MdePkg/MdeModulePkg under a generic name.

The UEFI specification says "The firmware reserves codes 0x0000 to
0xFFFF. Loaders and operating systems may use other timeout codes.".
So I think this code needs to set this to something else.

You don't need to say 0x00 when you mean 0.

> +  // Start the image
> +  Status = gBS->StartImage (ImageHandle, NULL, NULL);
> +  // Clear the Watchdog Timer after the image returns

_if_ the image returns, surely?

> +  gBS->SetWatchdogTimer (0x0000, 0x0000, 0x0000, NULL);

Don't say 0x* when you mean 0.

> +  return EFI_SUCCESS;
> +}
> diff --git a/EmbeddedPkg/Library/AbootimgLib/AbootimgLib.inf b/EmbeddedPkg/Library/AbootimgLib/AbootimgLib.inf
> new file mode 100644
> index 0000000..461dcb8
> --- /dev/null
> +++ b/EmbeddedPkg/Library/AbootimgLib/AbootimgLib.inf
> @@ -0,0 +1,48 @@
> +#/** @file
> +#
> +#  Copyright (c) 2013-2015, 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.
> +#
> +#
> +#**/
> +
> +[Defines]
> +  INF_VERSION                    = 0x00010019
> +  BASE_NAME                      = AbootimgLib
> +  FILE_GUID                      = ed3b8739-6fa7-4cb1-8aeb-2496f8fcaefa
> +  MODULE_TYPE                    = BASE
> +  VERSION_STRING                 = 1.0
> +  LIBRARY_CLASS                  = AbootimgLib
> +
> +#
> +# The following information is for reference only and not required by the build tools.
> +#
> +#  VALID_ARCHITECTURES           = ARM AARCH64
> +#
> +
> +[Sources]
> +  AbootimgLib.c
> +
> +[LibraryClasses]
> +  DebugLib
> +  FdtLib
> +  PrintLib
> +  UefiBootServicesTableLib
> +  UefiLib
> +
> +[Packages]
> +  MdePkg/MdePkg.dec
> +  EmbeddedPkg/EmbeddedPkg.dec

Please sort alphabetically.

/
    Leif

> +
> +[Protocols]
> +  gAbootimgProtocolGuid
> +
> +[Guids]
> +  gFdtTableGuid
> -- 
> 1.9.1
>