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

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

[Jun Nie]

Add an android kernel loader that could load kernel from storage
device. This patch is from Haojian's code. 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 +++
 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)))
+
+#define ALIGN(x, a)        (((x) + ((a) - 1)) & ~((a) - 1))
+
+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);
+  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);
+  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
+
+#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
+
+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];
+  UINT32  Id[32];
+} ANDROID_BOOTIMG_HEADER;
+
+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))
+
+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));
+
+  /* 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
+    );
+
+  // Append platform kernel arguments
+  Status = mAbootimg->AppendArgs (KernelArgs, BOOTIMG_KERNEL_ARGS_SIZE);
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+
+  Status = mAbootimg->UpdateDtb (FdtBase, &NewFdtBase);
+  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.
+   */
+  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 */
+    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;
+  }
+
+  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.
+  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, 0x0000, 0x00, NULL);
+  // Start the image
+  Status = gBS->StartImage (ImageHandle, NULL, NULL);
+  // Clear the Watchdog Timer after the image returns
+  gBS->SetWatchdogTimer (0x0000, 0x0000, 0x0000, NULL);
+  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
+
+[Protocols]
+  gAbootimgProtocolGuid
+
+[Guids]
+  gFdtTableGuid
-- 
1.9.1

[PATCH v2 2/2] EmbeddedPkg: add Android boot device path and guid

[Jun Nie]

The device path specifies where to load android boot image.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jun Nie <jun.nie@linaro.org>
---
 EmbeddedPkg/EmbeddedPkg.dec | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/EmbeddedPkg/EmbeddedPkg.dec b/EmbeddedPkg/EmbeddedPkg.dec
index 4cd528a..cf89af2 100644
--- a/EmbeddedPkg/EmbeddedPkg.dec
+++ b/EmbeddedPkg/EmbeddedPkg.dec
@@ -80,6 +80,7 @@
   gAndroidFastbootPlatformProtocolGuid =  { 0x524685a0, 0x89a0, 0x11e3, {0x9d, 0x4d, 0xbf, 0xa9, 0xf6, 0xa4, 0x03, 0x08}}
   gUsbDeviceProtocolGuid =  { 0x021bd2ca, 0x51d2, 0x11e3, {0x8e, 0x56, 0xb7, 0x54, 0x17, 0xc7,  0x0b, 0x44 }}
   gPlatformGpioProtocolGuid = { 0x52ce9845, 0x5af4, 0x43e2, {0xba, 0xfd, 0x23, 0x08, 0x12, 0x54, 0x7a, 0xc2 }}
+  gAbootimgProtocolGuid = { 0x9859bb19, 0x407c, 0x4f8b, {0xbc, 0xe1, 0xf8, 0xda, 0x65, 0x65, 0xf4, 0xa5 }}
 
 [PcdsFeatureFlag.common]
   gEmbeddedTokenSpaceGuid.PcdEmbeddedMacBoot|FALSE|BOOLEAN|0x00000001
@@ -181,6 +182,7 @@
   gEmbeddedTokenSpaceGuid.PcdAndroidFastbootUsbProductId|0xbeef|UINT32|0x00000023
   gEmbeddedTokenSpaceGuid.PcdAndroidFastbootTcpPort|1234|UINT32|0x00000024
 
+  gEmbeddedTokenSpaceGuid.PcdAndroidBootDevicePath|L""|VOID*|0x00000025
 
 [PcdsFixedAtBuild.ARM]
   gEmbeddedTokenSpaceGuid.PcdPrePiCpuMemorySize|32|UINT8|0x00000010
-- 
1.9.1

Re: [PATCH v2 2/2] EmbeddedPkg: add Android boot device path and guid

[Jun Nie]

2017-07-06 21:29 GMT+08:00 Jun Nie <jun.nie@linaro.org>:
> The device path specifies where to load android boot image.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jun Nie <jun.nie@linaro.org>
> ---
>  EmbeddedPkg/EmbeddedPkg.dec | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/EmbeddedPkg/EmbeddedPkg.dec b/EmbeddedPkg/EmbeddedPkg.dec
> index 4cd528a..cf89af2 100644
> --- a/EmbeddedPkg/EmbeddedPkg.dec
> +++ b/EmbeddedPkg/EmbeddedPkg.dec
> @@ -80,6 +80,7 @@
>    gAndroidFastbootPlatformProtocolGuid =  { 0x524685a0, 0x89a0, 0x11e3, {0x9d, 0x4d, 0xbf, 0xa9, 0xf6, 0xa4, 0x03, 0x08}}
>    gUsbDeviceProtocolGuid =  { 0x021bd2ca, 0x51d2, 0x11e3, {0x8e, 0x56, 0xb7, 0x54, 0x17, 0xc7,  0x0b, 0x44 }}
>    gPlatformGpioProtocolGuid = { 0x52ce9845, 0x5af4, 0x43e2, {0xba, 0xfd, 0x23, 0x08, 0x12, 0x54, 0x7a, 0xc2 }}
> +  gAbootimgProtocolGuid = { 0x9859bb19, 0x407c, 0x4f8b, {0xbc, 0xe1, 0xf8, 0xda, 0x65, 0x65, 0xf4, 0xa5 }}
>
>  [PcdsFeatureFlag.common]
>    gEmbeddedTokenSpaceGuid.PcdEmbeddedMacBoot|FALSE|BOOLEAN|0x00000001
> @@ -181,6 +182,7 @@
>    gEmbeddedTokenSpaceGuid.PcdAndroidFastbootUsbProductId|0xbeef|UINT32|0x00000023
>    gEmbeddedTokenSpaceGuid.PcdAndroidFastbootTcpPort|1234|UINT32|0x00000024

Just find that the ID 0x00000024 conflict with other guid. Will change
it in next version.

>
> +  gEmbeddedTokenSpaceGuid.PcdAndroidBootDevicePath|L""|VOID*|0x00000025
>
>  [PcdsFixedAtBuild.ARM]
>    gEmbeddedTokenSpaceGuid.PcdPrePiCpuMemorySize|32|UINT8|0x00000010
> --
> 1.9.1
>

Re: [PATCH v2 2/2] EmbeddedPkg: add Android boot device path and guid

[Leif Lindholm]

On Thu, Jul 06, 2017 at 09:29:06PM +0800, Jun Nie wrote:
> The device path specifies where to load android boot image.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jun Nie <jun.nie@linaro.org>

No objections to this one, once you fix the token issue you identified
yourself.

Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>


> ---
>  EmbeddedPkg/EmbeddedPkg.dec | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/EmbeddedPkg/EmbeddedPkg.dec b/EmbeddedPkg/EmbeddedPkg.dec
> index 4cd528a..cf89af2 100644
> --- a/EmbeddedPkg/EmbeddedPkg.dec
> +++ b/EmbeddedPkg/EmbeddedPkg.dec
> @@ -80,6 +80,7 @@
>    gAndroidFastbootPlatformProtocolGuid =  { 0x524685a0, 0x89a0, 0x11e3, {0x9d, 0x4d, 0xbf, 0xa9, 0xf6, 0xa4, 0x03, 0x08}}
>    gUsbDeviceProtocolGuid =  { 0x021bd2ca, 0x51d2, 0x11e3, {0x8e, 0x56, 0xb7, 0x54, 0x17, 0xc7,  0x0b, 0x44 }}
>    gPlatformGpioProtocolGuid = { 0x52ce9845, 0x5af4, 0x43e2, {0xba, 0xfd, 0x23, 0x08, 0x12, 0x54, 0x7a, 0xc2 }}
> +  gAbootimgProtocolGuid = { 0x9859bb19, 0x407c, 0x4f8b, {0xbc, 0xe1, 0xf8, 0xda, 0x65, 0x65, 0xf4, 0xa5 }}
>  
>  [PcdsFeatureFlag.common]
>    gEmbeddedTokenSpaceGuid.PcdEmbeddedMacBoot|FALSE|BOOLEAN|0x00000001
> @@ -181,6 +182,7 @@
>    gEmbeddedTokenSpaceGuid.PcdAndroidFastbootUsbProductId|0xbeef|UINT32|0x00000023
>    gEmbeddedTokenSpaceGuid.PcdAndroidFastbootTcpPort|1234|UINT32|0x00000024
>  
> +  gEmbeddedTokenSpaceGuid.PcdAndroidBootDevicePath|L""|VOID*|0x00000025
>  
>  [PcdsFixedAtBuild.ARM]
>    gEmbeddedTokenSpaceGuid.PcdPrePiCpuMemorySize|32|UINT8|0x00000010
> -- 
> 1.9.1
> 

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

[Jun Nie]

2017-07-06 21:29 GMT+08:00 Jun Nie <jun.nie@linaro.org>:
> Add an android kernel loader that could load kernel from storage
> device. This patch is from Haojian's code. 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>

Leif,

Could you help comment these two android boot BDS patches? Thank you!

Jun

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

[Leif Lindholm]

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
> 

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

[Jun Nie]

2017-07-19 0:04 GMT+08:00 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.

OK, will add description for addtitional cmdline/dtb/ramfs support besides
 kernel that is introduced by Android boot header.

>
> What relation does this code have to AndroidFastbootApp?

It does not relate to AndroidFastbootApp directly because fastboot is tools
for image download/flash/etc. But both apps comply to Google's spec
and share some common data definition.

>
>> This patch is from Haojian's code.
>
> Could you put a link to the origin of the code (preferably the
> specific commit)?
Will do.
>
>> 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.

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

Will do.
>
>> +
>> +#define ALIGN(x, a)        (((x) + ((a) - 1)) & ~((a) - 1))
>
> Rather use one of the ALIGN_ macros from Base.h.
>

Will do.
>> +
>> +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"?

Will do.
>
>> +  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)?

NET_ROUNDUP is okay for this requirement and is generic. You want to make
a BLOCK_ALIGN_ROUNDUP like macro? The BlockSize variable name may vary
in different place and a new macro shall not introduce benefit. Or I
misunderstand
you?

>
>> +  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?

It is defined by Google code.
https://android.googlesource.com/platform/system/core/+/master/mkbootimg/bootimg.h
>
>> +
>> +#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?
Seems to be offset of PE/COFF data that describe the COFF image size.
We do not need it if we do not search DTB that is attached after kernel image.

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

Will do.
>
>> +
>> +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?

The BootMagic string should be "ANDROID!" to identify it is android
boot image.
>
>> +  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?

Will merge the two definition.
>
>> +
>> +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.

Will fix.
>
>> +
>> +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().

This page_size is not related to UEFI directly because it is defined
by Android code. It is just
the size of boot header in boot image. The kernel image start from
offset of page_size. You can
check the comment in the android boot code link.

>
>> +
>> +  /* 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.

Will try this one.
>
>> +
>> +  // Append platform kernel arguments
>> +  Status = mAbootimg->AppendArgs (KernelArgs, BOOTIMG_KERNEL_ARGS_SIZE);
>
> Am I missing something? Where is this AppendArgs function defined?

The function body is implemented in platform driver. We can check the
NULL pointer
before call it.

>
>> +  if (EFI_ERROR (Status)) {
>> +    return Status;
>> +  }
>> +
>> +  Status = mAbootimg->UpdateDtb (FdtBase, &NewFdtBase);
>
> Likewise, where is this function defined?
implemented in platform driver too.

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

This is to support legacy hikey bootimage. We can drop this support if
Haojian does not object.

>
>> +  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?

Per description in code, it is the second stage boot loader that come first
 than kernel. But it seems that no platform adopt this boot loader usage
 and second_size is commonly 0. On the contrary, there are cases to put
DTB in the second stage as far as I know. UEFI android boot app parse
 kernel directly and second stage surely can be occupied by DTB.

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

Could you help elaborate more on how to check DTB installation status?
This android boot app will read block device for kernel/dtb and cmd lines
directly as the first place. So I do not know what code will install the DTB
before android boot image is read.

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

Will remove.
>
>> +  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.

Will do.
>
>> +
>> +  // Set kernel arguments
>> +  Status = gBS->HandleProtocol (ImageHandle, &gEfiLoadedImageProtocolGuid, (VOID **) &ImageInfo);
>
> Please split function call up over multiple lines.

Will do.
>
>> +  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.)

Will do.
>
>> +  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.

You want to add below service call usage? I prefer a definition of max timeout.
We can make another patch to change all max timeout usage later.
gBS->SetWatchdogMaxTimeout (0x0000, 0x00, NULL);
#define MAX_WATCHDOG_TIMEOUT (60*5)

>
> 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 are right. 0x0000 should be a copy/past bug here. Will change to 0x10000
as fastboot app.
>
> 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?
You mean it never return? If coff data of kernel image is corrupted,
it surely return.
>
>> +  gBS->SetWatchdogTimer (0x0000, 0x0000, 0x0000, NULL);
>
> Don't say 0x* when you mean 0.
Will change it.
>
>> +  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.

Will do.
>
> /
>     Leif
>
>> +
>> +[Protocols]
>> +  gAbootimgProtocolGuid
>> +
>> +[Guids]
>> +  gFdtTableGuid
>> --
>> 1.9.1
>>

Thanks for review with so much details.

Jun

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

[Leif Lindholm]

On Wed, Jul 19, 2017 at 06:00:04PM +0800, Jun Nie wrote:
> 2017-07-19 0:04 GMT+08:00 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.
> 
> OK, will add description for addtitional cmdline/dtb/ramfs support besides
>  kernel that is introduced by Android boot header.

Allright, after having read through your responses, I think I
understand what is going on. But yeah, a link to some documentation of
this Android boot image format would be useful.

> > What relation does this code have to AndroidFastbootApp?
> 
> It does not relate to AndroidFastbootApp directly because fastboot is tools
> for image download/flash/etc. But both apps comply to Google's spec
> and share some common data definition.

Got it, thanks.

> >> This patch is from Haojian's code.
> >
> > Could you put a link to the origin of the code (preferably the
> > specific commit)?
> Will do.
> >
> >> 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.
> 
> Will do.
> >
> >>  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.
> 
> Will do.
> >
> >> +
> >> +#define ALIGN(x, a)        (((x) + ((a) - 1)) & ~((a) - 1))
> >
> > Rather use one of the ALIGN_ macros from Base.h.
> >
> 
> Will do.
> >> +
> >> +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"?
> 
> Will do.
> >
> >> +  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)?
> 
> NET_ROUNDUP is okay for this requirement and is generic. You want to make
> a BLOCK_ALIGN_ROUNDUP like macro? The BlockSize variable name may vary
> in different place and a new macro shall not introduce benefit. Or I
> misunderstand you?

The problem with NET_ROUNDUP is that it resides in NetLib.h.
That's why it sounds like a generic ROUNDUP would be a useful addition
to Base.h. (NET_ROUNDUP could then become an alias for that.)

> >
> >> +  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/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?
> 
> It is defined by Google code.
> https://android.googlesource.com/platform/system/core/+/master/mkbootimg/bootimg.h

OK. It would be good to have some link in here to a more formal
specification, but if one is missing, please just add this link in a
comment.

> >
> >> +
> >> +#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?
> Seems to be offset of PE/COFF data that describe the COFF image size.
> We do not need it if we do not search DTB that is attached after kernel image.

OK. That would be nice if we could get rid of.
If not, it would be preferable if these could be extracted via
existing PE/COFF definitions.

> > All of these names (BOOT/KERNEL/FDT) are a little bit to generic for
> > an exported header file. Need ANDROID_BOOT_ (or similar_) prefix.
> 
> Will do.
> >
> >> +
> >> +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?
> 
> The BootMagic string should be "ANDROID!" to identify it is android
> boot image.

Sorry, what I was referring to was the use of the CHAR8 type.
Checking against the link above, the original uses uint8_t, so I would
have expected to see UINT8 here. Or if the original was int8_t, INT8.
Since default char signedness differs between x86/ARM...

> >
> >> +  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?
> 
> Will merge the two definition.
> >
> >> +
> >> +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/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.
> 
> Will fix.
> >
> >> +
> >> +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().
> 
> This page_size is not related to UEFI directly because it is defined
> by Android code. It is just
> the size of boot header in boot image. The kernel image start from
> offset of page_size. You can
> check the comment in the android boot code link.

Right, OK.
That still sort of reinforces my view that it being a power of 2 is
not really adding more robustness. By all means check if the field has
actually been initialized, but beyond that, surely we need to expect
that its value is relevant to the rest of the contents of the image?
Or is this more a safety check based on arithmetic elsewhere depending
on it being a power of 2?

> >> +
> >> +  /* 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.
> 
> Will try this one.
> >
> >> +
> >> +  // Append platform kernel arguments
> >> +  Status = mAbootimg->AppendArgs (KernelArgs, BOOTIMG_KERNEL_ARGS_SIZE);
> >
> > Am I missing something? Where is this AppendArgs function defined?
> 
> The function body is implemented in platform driver. We can check the
> NULL pointer before call it.

OK. It would be nice to see the code being upstreamed including code
that actually uses it. Otherwise, I am only reviewing one side of the
interface.

> >
> >> +  if (EFI_ERROR (Status)) {
> >> +    return Status;
> >> +  }
> >> +
> >> +  Status = mAbootimg->UpdateDtb (FdtBase, &NewFdtBase);
> >
> > Likewise, where is this function defined?
> implemented in platform driver too.
> 
> >
> >> +  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.
> 
> This is to support legacy hikey bootimage. We can drop this support if
> Haojian does not object.

I will leave that to him to answer.

> >> +  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?
> 
> Per description in code, it is the second stage boot loader that come first
>  than kernel. But it seems that no platform adopt this boot loader usage
>  and second_size is commonly 0. On the contrary, there are cases to put
> DTB in the second stage as far as I know. UEFI android boot app parse
>  kernel directly

Yes, I would have hoped so.

> and second stage surely can be occupied by DTB.

This does get semantically dubious. But if it is hidden away in a
helper function, and well commented, then I guess this is OK.

> >> +    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.
> 
> Could you help elaborate more on how to check DTB installation status?
> This android boot app will read block device for kernel/dtb and cmd lines
> directly as the first place. So I do not know what code will install the DTB
> before android boot image is read.

Use EfiGetSystemConfigurationTable to attempt to retrieve the guid you
are currently installing here.

Tying kernel image and DTB together is a travesty. I know some people
just can't get past the point, so we need to enable them to keep doing
it, but the expected behaviour is that the platform initialisation
code registers a table (whatever its origin).

> >> +  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.)
> 
> Will remove.
> >
> >> +  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.
> 
> Will do.
> >
> >> +
> >> +  // Set kernel arguments
> >> +  Status = gBS->HandleProtocol (ImageHandle, &gEfiLoadedImageProtocolGuid, (VOID **) &ImageInfo);
> >
> > Please split function call up over multiple lines.
> 
> Will do.
> >
> >> +  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.)
> 
> Will do.
> >
> >> +  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.
> 
> You want to add below service call usage? I prefer a definition of max timeout.
> We can make another patch to change all max timeout usage later.
> gBS->SetWatchdogMaxTimeout (0x0000, 0x00, NULL);
> #define MAX_WATCHDOG_TIMEOUT (60*5)

Yes, that works.

> >
> > 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 are right. 0x0000 should be a copy/past bug here. Will change to 0x10000
> as fastboot app.
> >
> > 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?
> You mean it never return? If coff data of kernel image is corrupted,
> it surely return.

Of course, but that is the error condition - not the expected
behaviour. So it is "if", not "when" (which implies inevitability).

> >
> >> +  gBS->SetWatchdogTimer (0x0000, 0x0000, 0x0000, NULL);
> >
> > Don't say 0x* when you mean 0.
> Will change it.
> >
> >> +  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.
> 
> Will do.
> >
> > /
> >     Leif
> >
> >> +
> >> +[Protocols]
> >> +  gAbootimgProtocolGuid
> >> +
> >> +[Guids]
> >> +  gFdtTableGuid
> >> --
> >> 1.9.1
> >>
> 
> Thanks for review with so much details.
> 
> Jun