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

[PATCH v3 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 as below link. The minor
change is that alternative dtb is searched in second loader binary
of Android bootimage if dtb is not found after Linux kernel.
https://patches.linaro.org/patch/94683/

This android boot image BDS add addtitional cmdline/dtb/ramfs
support besides kernel that is introduced by Android boot header.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jun Nie <jun.nie@linaro.org>
---
 ArmPkg/Include/Library/BdsLib.h                    |   3 +
 ArmPkg/Library/BdsLib/BdsFilePath.c                |   3 -
 .../Application/AndroidBoot/AndroidBootApp.c       | 127 +++++++
 .../Application/AndroidBoot/AndroidBootApp.inf     |  64 ++++
 .../Application/AndroidFastboot/AndroidBootImg.c   |  35 +-
 .../AndroidFastboot/AndroidFastbootApp.h           |   1 +
 .../AndroidFastboot/Arm/BootAndroidBootImg.c       |   2 +-
 EmbeddedPkg/Include/Library/AndroidBootImgLib.h    |  67 ++++
 EmbeddedPkg/Include/Protocol/AndroidBootImg.h      |  47 +++
 .../Library/AndroidBootImgLib/AndroidBootImgLib.c  | 419 +++++++++++++++++++++
 .../AndroidBootImgLib/AndroidBootImgLib.inf        |  48 +++
 11 files changed, 782 insertions(+), 34 deletions(-)
 create mode 100644 EmbeddedPkg/Application/AndroidBoot/AndroidBootApp.c
 create mode 100644 EmbeddedPkg/Application/AndroidBoot/AndroidBootApp.inf
 create mode 100644 EmbeddedPkg/Include/Library/AndroidBootImgLib.h
 create mode 100644 EmbeddedPkg/Include/Protocol/AndroidBootImg.h
 create mode 100644 EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
 create mode 100644 EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.inf

diff --git a/ArmPkg/Include/Library/BdsLib.h b/ArmPkg/Include/Library/BdsLib.h
index c58f47e..4528c2e 100644
--- a/ArmPkg/Include/Library/BdsLib.h
+++ b/ArmPkg/Include/Library/BdsLib.h
@@ -15,6 +15,9 @@
 #ifndef __BDS_ENTRY_H__
 #define __BDS_ENTRY_H__
 
+#define IS_DEVICE_PATH_NODE(node,type,subtype)    \
+        (((node)->Type == (type)) && ((node)->SubType == (subtype)))
+
 /**
   This is defined by the UEFI specs, don't change it
 **/
diff --git a/ArmPkg/Library/BdsLib/BdsFilePath.c b/ArmPkg/Library/BdsLib/BdsFilePath.c
index f9d8c4c..41557bb 100644
--- a/ArmPkg/Library/BdsLib/BdsFilePath.c
+++ b/ArmPkg/Library/BdsLib/BdsFilePath.c
@@ -24,9 +24,6 @@
 #include <Protocol/Dhcp4.h>
 #include <Protocol/Mtftp4.h>
 
-
-#define IS_DEVICE_PATH_NODE(node,type,subtype) (((node)->Type == (type)) && ((node)->SubType == (subtype)))
-
 /* Type and defines to set up the DHCP4 options */
 
 typedef struct {
diff --git a/EmbeddedPkg/Application/AndroidBoot/AndroidBootApp.c b/EmbeddedPkg/Application/AndroidBoot/AndroidBootApp.c
new file mode 100644
index 0000000..2de1d8a
--- /dev/null
+++ b/EmbeddedPkg/Application/AndroidBoot/AndroidBootApp.c
@@ -0,0 +1,127 @@
+/** @file
+
+  Copyright (c) 2013-2014, ARM Ltd. All rights reserved.<BR>
+  Copyright (c) 2017, Linaro. All rights reserved.
+
+  This program and the accompanying materials
+  are licensed and made available under the terms and conditions of the BSD License
+  which accompanies this distribution.  The full text of the license may be found at
+  http://opensource.org/licenses/bsd-license.php
+
+  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+
+**/
+
+#include <Library/AndroidBootImgLib.h>
+#include <Library/BaseMemoryLib.h>
+#include <Library/BdsLib.h>
+#include <Library/DebugLib.h>
+#include <Library/DevicePathLib.h>
+#include <Library/MemoryAllocationLib.h>
+#include <Library/UefiBootServicesTableLib.h>
+
+#include <Protocol/BlockIo.h>
+#include <Protocol/DevicePathFromText.h>
+
+EFI_STATUS
+EFIAPI
+AndroidBootAppEntryPoint (
+  IN EFI_HANDLE                            ImageHandle,
+  IN EFI_SYSTEM_TABLE                      *SystemTable
+  )
+{
+  EFI_STATUS                          Status;
+  CHAR16                              *BootPathStr;
+  EFI_DEVICE_PATH_FROM_TEXT_PROTOCOL  *EfiDevicePathFromTextProtocol;
+  EFI_DEVICE_PATH                     *DevicePath;
+  EFI_DEVICE_PATH_PROTOCOL            *Node, *NextNode;
+  EFI_BLOCK_IO_PROTOCOL               *BlockIo;
+  UINT32                              MediaId, BlockSize;
+  VOID                                *Buffer;
+  EFI_HANDLE                          Handle;
+  UINTN                               Size;
+
+  BootPathStr = (CHAR16 *)PcdGetPtr (PcdAndroidBootDevicePath);
+  ASSERT (BootPathStr != NULL);
+  Status = gBS->LocateProtocol (&gEfiDevicePathFromTextProtocolGuid, NULL,
+                                (VOID **)&EfiDevicePathFromTextProtocol);
+  ASSERT_EFI_ERROR(Status);
+  DevicePath = (EFI_DEVICE_PATH *)EfiDevicePathFromTextProtocol->ConvertTextToDevicePath (BootPathStr);
+  ASSERT (DevicePath != NULL);
+
+  /* Find DevicePath node of Partition */
+  NextNode = DevicePath;
+  while (1) {
+    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 (EFI_SIZE_TO_PAGES (sizeof(ANDROID_BOOTIMG_HEADER)));
+  if (Buffer == NULL) {
+    return EFI_BUFFER_TOO_SMALL;
+  }
+  /* Load header of boot.img */
+  Status = BlockIo->ReadBlocks (
+                      BlockIo,
+                      MediaId,
+                      0,
+                      BlockSize,
+                      Buffer
+                      );
+  Status = AbootimgGetImgSize (Buffer, &Size);
+  if (EFI_ERROR (Status)) {
+    DEBUG ((EFI_D_ERROR, "Failed to get Abootimg Size: %r\n", Status));
+    return Status;
+  }
+  Size = ALIGN_VALUE (Size, BlockSize);
+  FreePages (Buffer, EFI_SIZE_TO_PAGES (sizeof(ANDROID_BOOTIMG_HEADER)));
+
+  /* Both PartitionStart and PartitionSize are counted as block size. */
+  Buffer = AllocatePages (EFI_SIZE_TO_PAGES (Size));
+  if (Buffer == NULL) {
+    return EFI_BUFFER_TOO_SMALL;
+  }
+
+  /* Load header of boot.img */
+  Status = BlockIo->ReadBlocks (
+                      BlockIo,
+                      MediaId,
+                      0,
+                      Size,
+                      Buffer
+                      );
+  if (EFI_ERROR (Status)) {
+    DEBUG ((EFI_D_ERROR, "Failed to read blocks: %r\n", Status));
+    goto EXIT;
+  }
+
+  Status = AbootimgBoot (Buffer, Size);
+
+EXIT:
+  return Status;
+}
diff --git a/EmbeddedPkg/Application/AndroidBoot/AndroidBootApp.inf b/EmbeddedPkg/Application/AndroidBoot/AndroidBootApp.inf
new file mode 100644
index 0000000..f1ee0bd
--- /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]
+  AndroidBootImgLib
+  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/Application/AndroidFastboot/AndroidBootImg.c b/EmbeddedPkg/Application/AndroidFastboot/AndroidBootImg.c
index f3e770b..2f7f093 100644
--- a/EmbeddedPkg/Application/AndroidFastboot/AndroidBootImg.c
+++ b/EmbeddedPkg/Application/AndroidFastboot/AndroidBootImg.c
@@ -14,32 +14,6 @@
 
 #include "AndroidFastbootApp.h"
 
-#define BOOT_MAGIC        "ANDROID!"
-#define BOOT_MAGIC_LENGTH sizeof (BOOT_MAGIC) - 1
-
-// 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))
-
-// No documentation for this really - sizes of fields has been determined
-// empirically.
-#pragma pack(1)
-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;
-#pragma pack()
-
 // Find the kernel and ramdisk in an Android boot.img.
 // return EFI_INVALID_PARAMTER if the boot.img is invalid (i.e. doesn't have the
 //  right magic value),
@@ -64,7 +38,8 @@ ParseAndroidBootImg (
 
   Header = (ANDROID_BOOTIMG_HEADER *) BootImg;
 
-  if (AsciiStrnCmp (Header->BootMagic, BOOT_MAGIC, BOOT_MAGIC_LENGTH) != 0) {
+  if (AsciiStrnCmp ((CONST CHAR8 *)Header->BootMagic, ANDROID_BOOT_MAGIC,
+                    ANDROID_BOOT_MAGIC_LENGTH) != 0) {
     return EFI_INVALID_PARAMETER;
   }
 
@@ -72,7 +47,7 @@ ParseAndroidBootImg (
     return EFI_NOT_FOUND;
   }
 
-  ASSERT (IS_POWER_OF_2 (Header->PageSize));
+  ASSERT (IS_VALID_ANDROID_PAGE_SIZE (Header->PageSize));
 
   *KernelSize = Header->KernelSize;
   *Kernel = BootImgBytePtr + Header->PageSize;
@@ -84,8 +59,8 @@ ParseAndroidBootImg (
                  + ALIGN_VALUE (Header->KernelSize, Header->PageSize));
   }
 
-  AsciiStrnCpyS (KernelArgs, BOOTIMG_KERNEL_ARGS_SIZE, Header->KernelArgs,
-    BOOTIMG_KERNEL_ARGS_SIZE);
+  AsciiStrnCpyS (KernelArgs, ANDROID_BOOTIMG_KERNEL_ARGS_SIZE, Header->KernelArgs,
+    ANDROID_BOOTIMG_KERNEL_ARGS_SIZE);
 
   return EFI_SUCCESS;
 }
diff --git a/EmbeddedPkg/Application/AndroidFastboot/AndroidFastbootApp.h b/EmbeddedPkg/Application/AndroidFastboot/AndroidFastbootApp.h
index f62660f..e4c5aa3 100644
--- a/EmbeddedPkg/Application/AndroidFastboot/AndroidFastbootApp.h
+++ b/EmbeddedPkg/Application/AndroidFastboot/AndroidFastbootApp.h
@@ -15,6 +15,7 @@
 #ifndef __ANDROID_FASTBOOT_APP_H__
 #define __ANDROID_FASTBOOT_APP_H__
 
+#include <Library/AndroidBootImgLib.h>
 #include <Library/BaseLib.h>
 #include <Library/DebugLib.h>
 #include <Library/MemoryAllocationLib.h>
diff --git a/EmbeddedPkg/Application/AndroidFastboot/Arm/BootAndroidBootImg.c b/EmbeddedPkg/Application/AndroidFastboot/Arm/BootAndroidBootImg.c
index f446cce..1d9024b 100644
--- a/EmbeddedPkg/Application/AndroidFastboot/Arm/BootAndroidBootImg.c
+++ b/EmbeddedPkg/Application/AndroidFastboot/Arm/BootAndroidBootImg.c
@@ -112,7 +112,7 @@ BootAndroidBootImg (
   )
 {
   EFI_STATUS                          Status;
-  CHAR8                               KernelArgs[BOOTIMG_KERNEL_ARGS_SIZE];
+  CHAR8                               KernelArgs[ANDROID_BOOTIMG_KERNEL_ARGS_SIZE];
   VOID                               *Kernel;
   UINTN                               KernelSize;
   VOID                               *Ramdisk;
diff --git a/EmbeddedPkg/Include/Library/AndroidBootImgLib.h b/EmbeddedPkg/Include/Library/AndroidBootImgLib.h
new file mode 100644
index 0000000..3c825eb
--- /dev/null
+++ b/EmbeddedPkg/Include/Library/AndroidBootImgLib.h
@@ -0,0 +1,67 @@
+/** @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 ANDROID_BOOTIMG_KERNEL_ARGS_SIZE  512
+
+#define ANDROID_BOOT_MAGIC                "ANDROID!"
+#define ANDROID_BOOT_MAGIC_LENGTH         (sizeof (ANDROID_BOOT_MAGIC) - 1)
+
+/* https://android.googlesource.com/platform/system/core/+/master/mkbootimg/bootimg.h */
+typedef struct {
+  UINT8   BootMagic[ANDROID_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[ANDROID_BOOTIMG_KERNEL_ARGS_SIZE];
+  UINT32  Id[32];
+} ANDROID_BOOTIMG_HEADER;
+
+/* 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))
+/* Android boot image page size is not specified, but it should be power of 2
+ * and larger than boot header */
+#define IS_VALID_ANDROID_PAGE_SIZE(Val)   \
+             (IS_POWER_OF_2(Val) && (Val > sizeof(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/AndroidBootImg.h b/EmbeddedPkg/Include/Protocol/AndroidBootImg.h
new file mode 100644
index 0000000..6bee5cf
--- /dev/null
+++ b/EmbeddedPkg/Include/Protocol/AndroidBootImg.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 gAndroidBootImgProtocolGuid;
+
+#endif /* __ABOOTIMG_PROTOCOL_H__ */
diff --git a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
new file mode 100644
index 0000000..72c6322
--- /dev/null
+++ b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
@@ -0,0 +1,419 @@
+/** @file
+
+  Copyright (c) 2013-2014, ARM Ltd. All rights reserved.<BR>
+  Copyright (c) 2017, Linaro. All rights reserved.
+
+  This program and the accompanying materials
+  are licensed and made available under the terms and conditions of the BSD License
+  which accompanies this distribution.  The full text of the license may be found at
+  http://opensource.org/licenses/bsd-license.php
+
+  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+
+**/
+
+#include <libfdt.h>
+#include <Library/AndroidBootImgLib.h>
+#include <Library/PrintLib.h>
+#include <Library/UefiBootServicesTableLib.h>
+#include <Library/UefiLib.h>
+
+#include <Protocol/AndroidBootImg.h>
+#include <Protocol/LoadedImage.h>
+
+#include <libfdt.h>
+
+#define FDT_ADDITIONAL_ENTRIES_SIZE 0x400
+
+typedef struct {
+  MEMMAP_DEVICE_PATH                      Node1;
+  EFI_DEVICE_PATH_PROTOCOL                End;
+} MEMORY_DEVICE_PATH;
+
+STATIC ABOOTIMG_PROTOCOL                 *mAbootimg;
+
+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 ((CONST CHAR8 *)Header->BootMagic, ANDROID_BOOT_MAGIC,
+                    ANDROID_BOOT_MAGIC_LENGTH) != 0) {
+    return EFI_INVALID_PARAMETER;
+  }
+
+  /* The page size is not specified, but it should be power of 2 at least */
+  ASSERT (IS_VALID_ANDROID_PAGE_SIZE (Header->PageSize));
+
+  /* Get real size of abootimg */
+  *ImgSize = ALIGN_VALUE (Header->KernelSize, Header->PageSize) +
+             ALIGN_VALUE (Header->RamdiskSize, Header->PageSize) +
+             ALIGN_VALUE (Header->SecondStageBootloaderSize, Header->PageSize) +
+             Header->PageSize;
+  return EFI_SUCCESS;
+}
+
+EFI_STATUS
+AbootimgGetKernelInfo (
+  IN  VOID    *BootImg,
+  OUT VOID   **Kernel,
+  OUT UINTN   *KernelSize
+  )
+{
+  ANDROID_BOOTIMG_HEADER   *Header;
+
+  Header = (ANDROID_BOOTIMG_HEADER *) BootImg;
+
+  if (AsciiStrnCmp ((CONST CHAR8 *)Header->BootMagic, ANDROID_BOOT_MAGIC,
+                    ANDROID_BOOT_MAGIC_LENGTH) != 0) {
+    return EFI_INVALID_PARAMETER;
+  }
+
+  if (Header->KernelSize == 0) {
+    return EFI_NOT_FOUND;
+  }
+
+  ASSERT (IS_VALID_ANDROID_PAGE_SIZE (Header->PageSize));
+
+  *KernelSize = Header->KernelSize;
+  *Kernel = BootImg + Header->PageSize;
+  return EFI_SUCCESS;
+}
+
+EFI_STATUS
+AbootimgGetRamdiskInfo (
+  IN  VOID    *BootImg,
+  OUT VOID   **Ramdisk,
+  OUT UINTN   *RamdiskSize
+  )
+{
+  ANDROID_BOOTIMG_HEADER   *Header;
+  UINT8                    *BootImgBytePtr;
+
+  // Cast to UINT8 so we can do pointer arithmetic
+  BootImgBytePtr = (UINT8 *) BootImg;
+
+  Header = (ANDROID_BOOTIMG_HEADER *) BootImg;
+
+  if (AsciiStrnCmp ((CONST CHAR8 *)Header->BootMagic, ANDROID_BOOT_MAGIC,
+                    ANDROID_BOOT_MAGIC_LENGTH) != 0) {
+    return EFI_INVALID_PARAMETER;
+  }
+
+  ASSERT (IS_VALID_ANDROID_PAGE_SIZE (Header->PageSize));
+
+  *RamdiskSize = Header->RamdiskSize;
+
+  if (Header->RamdiskSize != 0) {
+    *Ramdisk = (VOID *) (BootImgBytePtr
+                 + Header->PageSize
+                 + ALIGN_VALUE (Header->KernelSize, Header->PageSize));
+  }
+  return EFI_SUCCESS;
+}
+
+EFI_STATUS
+AbootimgGetSecondBootLoaderInfo (
+  IN  VOID    *BootImg,
+  OUT VOID   **Second,
+  OUT UINTN   *SecondSize
+  )
+{
+  ANDROID_BOOTIMG_HEADER   *Header;
+  UINT8                    *BootImgBytePtr;
+
+  // Cast to UINT8 so we can do pointer arithmetic
+  BootImgBytePtr = (UINT8 *) BootImg;
+
+  Header = (ANDROID_BOOTIMG_HEADER *) BootImg;
+
+  if (AsciiStrnCmp ((CONST CHAR8 *)Header->BootMagic, ANDROID_BOOT_MAGIC,
+                    ANDROID_BOOT_MAGIC_LENGTH) != 0) {
+    return EFI_INVALID_PARAMETER;
+  }
+
+  ASSERT (IS_VALID_ANDROID_PAGE_SIZE (Header->PageSize));
+
+  *SecondSize = Header->SecondStageBootloaderSize;
+
+  if (Header->SecondStageBootloaderSize != 0) {
+    *Second = (VOID *) (BootImgBytePtr
+                 + Header->PageSize
+                 + ALIGN_VALUE (Header->KernelSize, Header->PageSize)
+                 + ALIGN_VALUE (Header->RamdiskSize, Header->PageSize));
+  }
+  return EFI_SUCCESS;
+}
+
+EFI_STATUS
+AbootimgGetKernelArgs (
+  IN  VOID    *BootImg,
+  OUT CHAR8   *KernelArgs
+  )
+{
+  ANDROID_BOOTIMG_HEADER   *Header;
+
+  Header = (ANDROID_BOOTIMG_HEADER *) BootImg;
+  AsciiStrnCpyS (KernelArgs, ANDROID_BOOTIMG_KERNEL_ARGS_SIZE, Header->KernelArgs,
+    ANDROID_BOOTIMG_KERNEL_ARGS_SIZE);
+
+  return EFI_SUCCESS;
+}
+
+EFI_STATUS
+AbootimgGetFdt (
+  IN  VOID                  *BootImg,
+  IN  VOID                  **FdtBase
+  )
+{
+  UINTN                      SecondLoaderSize;
+  EFI_STATUS                 Status;
+
+  /* Check whether FDT is located in second boot loader as some vendor do so,
+   * because second loader is never used as far as I know. */
+  Status = AbootimgGetSecondBootLoaderInfo (
+          BootImg,
+          FdtBase,
+          &SecondLoaderSize
+          );
+  return Status;
+}
+
+EFI_STATUS
+AbootimgUpdateArgsFdt (
+  IN  VOID                  *BootImg,
+  OUT VOID                  *KernelArgs
+  )
+{
+  VOID                      *Ramdisk;
+  UINT64                     Ramdisk64, RamdiskEnd64;
+  UINTN                      RamdiskSize;
+  CHAR8                      ImgKernelArgs[ANDROID_BOOTIMG_KERNEL_ARGS_SIZE];
+  INTN                       Err, NewFdtSize, chosen_node;
+  EFI_STATUS                 Status;
+  EFI_PHYSICAL_ADDRESS       FdtBase, UpdatedFdtBase, NewFdtBase;
+  struct fdt_property       *prop;
+  int                        len;
+
+  Status = gBS->LocateProtocol (&gAndroidBootImgProtocolGuid, NULL,
+                                (VOID **) &mAbootimg);
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+
+  Status = AbootimgGetKernelArgs (BootImg, ImgKernelArgs);
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+  // Get kernel arguments from Android boot image
+  AsciiStrToUnicodeStrS (ImgKernelArgs, KernelArgs,
+                         ANDROID_BOOTIMG_KERNEL_ARGS_SIZE >> 1);
+  // Append platform kernel arguments
+  if(mAbootimg->AppendArgs) {
+    Status = mAbootimg->AppendArgs (KernelArgs,
+                                    ANDROID_BOOTIMG_KERNEL_ARGS_SIZE);
+    if (EFI_ERROR (Status)) {
+      return Status;
+    }
+  }
+
+  Status = EfiGetSystemConfigurationTable (&gFdtTableGuid, (VOID **)&FdtBase);
+  if (!EFI_ERROR (Status)) {
+    return Status;
+  }
+
+  Status = AbootimgGetFdt (BootImg, (VOID **)&FdtBase);
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+  Err = fdt_check_header ((VOID*)(UINTN)FdtBase);
+  if (Err != 0) {
+    DEBUG ((DEBUG_ERROR, "ERROR: Device Tree header not valid (Err:%d)\n",
+           Err));
+    return EFI_INVALID_PARAMETER;
+  }
+
+  Status = AbootimgGetRamdiskInfo (
+            BootImg,
+            &Ramdisk,
+            &RamdiskSize
+            );
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+
+  NewFdtSize = (UINTN)fdt_totalsize ((VOID*)(UINTN)(FdtBase))
+               + FDT_ADDITIONAL_ENTRIES_SIZE;
+  Status = gBS->AllocatePages (AllocateAnyPages, EfiBootServicesData,
+                  EFI_SIZE_TO_PAGES (NewFdtSize), &UpdatedFdtBase);
+  if (EFI_ERROR (Status)) {
+    DEBUG ((EFI_D_WARN, "Warning: Failed to reallocate FDT, err %d.\n",
+           Status));
+    return Status;
+  }
+
+  // Load the Original FDT tree into the new region
+  Err = fdt_open_into((VOID*)FdtBase, (VOID*)UpdatedFdtBase, NewFdtSize);
+  if (Err) {
+    DEBUG ((EFI_D_ERROR, "fdt_open_into(): %a\n", fdt_strerror (Err)));
+    Status = EFI_INVALID_PARAMETER;
+    goto Fdt_Exit;
+  }
+
+  Ramdisk64 = cpu_to_fdt64((UINT64)Ramdisk);
+  RamdiskEnd64 = cpu_to_fdt64((UINT64)(Ramdisk + RamdiskSize));
+
+  chosen_node = fdt_subnode_offset ((const void *)UpdatedFdtBase, 0, "chosen");
+  if (chosen_node < 0) {
+    chosen_node = fdt_add_subnode((void *)UpdatedFdtBase, 0, "chosen");
+      if (chosen_node < 0) {
+        DEBUG ((EFI_D_ERROR, "Failed to find chosen node in fdt!\n"));
+        goto Fdt_Exit;
+    }
+  }
+  prop = fdt_get_property_w((void *)UpdatedFdtBase, chosen_node,
+                            "linux,initrd-start", &len);
+  if (NULL == prop && len == -FDT_ERR_NOTFOUND) {
+    fdt_appendprop ((void *)UpdatedFdtBase, chosen_node,
+                    "linux,initrd-start", &Ramdisk64, sizeof (UINT64));
+  } else if (prop != NULL) {
+    fdt_setprop_u64((void *)UpdatedFdtBase, chosen_node,
+                    "linux,initrd-start", (uint64_t)Ramdisk64);
+  } else {
+    DEBUG ((EFI_D_ERROR, "Failed to append fdt prop initrd-start\n",
+           fdt_strerror (Err)));
+    Status = EFI_INVALID_PARAMETER;
+    goto Fdt_Exit;
+  }
+
+  prop = fdt_get_property_w((void *)UpdatedFdtBase, chosen_node,
+                            "linux,initrd-end", &len);
+  if (NULL == prop && len == -FDT_ERR_NOTFOUND) {
+    fdt_appendprop ((void *)UpdatedFdtBase, chosen_node,
+                    "linux,initrd-end", &RamdiskEnd64, sizeof (UINT64));
+  } else if (prop != NULL) {
+    fdt_setprop_u64((void *)UpdatedFdtBase, chosen_node,
+                    "linux,initrd-end", (uint64_t)RamdiskEnd64);
+  } else {
+    DEBUG ((EFI_D_ERROR, "Failed to append fdt prop initrd-end\n",
+           fdt_strerror (Err)));
+    Status = EFI_INVALID_PARAMETER;
+    goto Fdt_Exit;
+  }
+
+  if ( mAbootimg->UpdateDtb) {
+    Status = mAbootimg->UpdateDtb (UpdatedFdtBase, &NewFdtBase);
+    if (EFI_ERROR (Status)) {
+      goto Fdt_Exit;
+    }
+  }
+
+  //
+  // Sanity checks on the new FDT blob.
+  //
+  Err = fdt_check_header ((VOID*)(UINTN)NewFdtBase);
+  if (Err != 0) {
+    Print (L"ERROR: Device Tree header not valid (err:%d)\n", Err);
+    return EFI_INVALID_PARAMETER;
+  }
+
+  Status = gBS->InstallConfigurationTable (
+                  &gFdtTableGuid,
+                  (VOID *)(UINTN)NewFdtBase
+                  );
+  if (EFI_ERROR (Status)) {
+    goto Fdt_Exit;
+  }
+  return Status;
+
+Fdt_Exit:
+  gBS->FreePages (UpdatedFdtBase, EFI_SIZE_TO_PAGES (NewFdtSize));
+  return Status;
+}
+
+EFI_STATUS
+AbootimgBoot (
+  IN VOID                            *Buffer,
+  IN UINTN                            BufferSize
+  )
+{
+  EFI_STATUS                          Status;
+  VOID                               *Kernel;
+  UINTN                               KernelSize;
+  MEMORY_DEVICE_PATH                  KernelDevicePath;
+  EFI_HANDLE                          ImageHandle;
+  VOID                               *NewKernelArg;
+  EFI_LOADED_IMAGE_PROTOCOL          *ImageInfo;
+
+  Status = AbootimgGetKernelInfo (
+            Buffer,
+            &Kernel,
+            &KernelSize
+            );
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+
+  NewKernelArg = AllocateZeroPool (ANDROID_BOOTIMG_KERNEL_ARGS_SIZE);
+  if (NewKernelArg == NULL) {
+    DEBUG ((DEBUG_ERROR, "Fail to allocate memory\n"));
+    return EFI_OUT_OF_RESOURCES;
+  }
+
+  Status = AbootimgUpdateArgsFdt (Buffer, NewKernelArg);
+  if (EFI_ERROR (Status)) {
+    FreePool (NewKernelArg);
+    return EFI_INVALID_PARAMETER;
+  }
+
+  KernelDevicePath = MemoryDevicePathTemplate;
+
+  KernelDevicePath.Node1.StartingAddress = (EFI_PHYSICAL_ADDRESS)(UINTN) Kernel;
+  KernelDevicePath.Node1.EndingAddress   = (EFI_PHYSICAL_ADDRESS)(UINTN) Kernel
+                                           + KernelSize;
+
+  Status = gBS->LoadImage (TRUE, gImageHandle,
+                           (EFI_DEVICE_PATH *)&KernelDevicePath,
+                           (VOID*)(UINTN)Kernel, KernelSize, &ImageHandle);
+
+  // Set kernel arguments
+  Status = gBS->HandleProtocol (ImageHandle, &gEfiLoadedImageProtocolGuid,
+                                (VOID **) &ImageInfo);
+  ImageInfo->LoadOptions = NewKernelArg;
+  ImageInfo->LoadOptionsSize = StrLen (NewKernelArg) * sizeof (CHAR16);
+
+  // Before calling the image, enable the Watchdog Timer for  the 5 Minute period
+  gBS->SetWatchdogTimer (5 * 60, 0x10000, 0, NULL);
+  // Start the image
+  Status = gBS->StartImage (ImageHandle, NULL, NULL);
+  // Clear the Watchdog Timer if the image returns
+  gBS->SetWatchdogTimer (0, 0x10000, 0, NULL);
+  return EFI_SUCCESS;
+}
diff --git a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.inf b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.inf
new file mode 100644
index 0000000..c92bac0
--- /dev/null
+++ b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.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                      = AndroidBootImgLib
+  FILE_GUID                      = ed3b8739-6fa7-4cb1-8aeb-2496f8fcaefa
+  MODULE_TYPE                    = BASE
+  VERSION_STRING                 = 1.0
+  LIBRARY_CLASS                  = AndroidBootImgLib
+
+#
+# The following information is for reference only and not required by the build tools.
+#
+#  VALID_ARCHITECTURES           = ARM AARCH64
+#
+
+[Sources]
+  AndroidBootImgLib.c
+
+[LibraryClasses]
+  DebugLib
+  FdtLib
+  PrintLib
+  UefiBootServicesTableLib
+  UefiLib
+
+[Packages]
+  EmbeddedPkg/EmbeddedPkg.dec
+  MdePkg/MdePkg.dec
+
+[Protocols]
+  gAndroidBootImgProtocolGuid
+
+[Guids]
+  gFdtTableGuid
-- 
1.9.1

[PATCH v3 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..8ad2a84 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 }}
+  gAndroidBootImgProtocolGuid = { 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*|0x00000057
 
 [PcdsFixedAtBuild.ARM]
   gEmbeddedTokenSpaceGuid.PcdPrePiCpuMemorySize|32|UINT8|0x00000010
-- 
1.9.1

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

[Leif Lindholm]

On Thu, Jul 27, 2017 at 06:07:19PM +0800, Jun Nie wrote:
> Add an android kernel loader that could load kernel from storage
> device. This patch is from Haojian's code as below link. The minor
> change is that alternative dtb is searched in second loader binary
> of Android bootimage if dtb is not found after Linux kernel.
> https://patches.linaro.org/patch/94683/
> 
> This android boot image BDS add addtitional cmdline/dtb/ramfs
> support besides kernel that is introduced by Android boot header.

Getting there. A few more comments below.

> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jun Nie <jun.nie@linaro.org>
> ---
>  ArmPkg/Include/Library/BdsLib.h                    |   3 +
>  ArmPkg/Library/BdsLib/BdsFilePath.c                |   3 -
>  .../Application/AndroidBoot/AndroidBootApp.c       | 127 +++++++
>  .../Application/AndroidBoot/AndroidBootApp.inf     |  64 ++++
>  .../Application/AndroidFastboot/AndroidBootImg.c   |  35 +-
>  .../AndroidFastboot/AndroidFastbootApp.h           |   1 +
>  .../AndroidFastboot/Arm/BootAndroidBootImg.c       |   2 +-
>  EmbeddedPkg/Include/Library/AndroidBootImgLib.h    |  67 ++++
>  EmbeddedPkg/Include/Protocol/AndroidBootImg.h      |  47 +++
>  .../Library/AndroidBootImgLib/AndroidBootImgLib.c  | 419 +++++++++++++++++++++
>  .../AndroidBootImgLib/AndroidBootImgLib.inf        |  48 +++
>  11 files changed, 782 insertions(+), 34 deletions(-)
>  create mode 100644 EmbeddedPkg/Application/AndroidBoot/AndroidBootApp.c
>  create mode 100644 EmbeddedPkg/Application/AndroidBoot/AndroidBootApp.inf
>  create mode 100644 EmbeddedPkg/Include/Library/AndroidBootImgLib.h
>  create mode 100644 EmbeddedPkg/Include/Protocol/AndroidBootImg.h
>  create mode 100644 EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
>  create mode 100644 EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.inf
> 
> diff --git a/ArmPkg/Include/Library/BdsLib.h b/ArmPkg/Include/Library/BdsLib.h
> index c58f47e..4528c2e 100644
> --- a/ArmPkg/Include/Library/BdsLib.h
> +++ b/ArmPkg/Include/Library/BdsLib.h
> @@ -15,6 +15,9 @@
>  #ifndef __BDS_ENTRY_H__
>  #define __BDS_ENTRY_H__
>  
> +#define IS_DEVICE_PATH_NODE(node,type,subtype)    \
> +        (((node)->Type == (type)) && ((node)->SubType == (subtype)))
> +
>  /**
>    This is defined by the UEFI specs, don't change it
>  **/
> diff --git a/ArmPkg/Library/BdsLib/BdsFilePath.c b/ArmPkg/Library/BdsLib/BdsFilePath.c
> index f9d8c4c..41557bb 100644
> --- a/ArmPkg/Library/BdsLib/BdsFilePath.c
> +++ b/ArmPkg/Library/BdsLib/BdsFilePath.c
> @@ -24,9 +24,6 @@
>  #include <Protocol/Dhcp4.h>
>  #include <Protocol/Mtftp4.h>
>  
> -
> -#define IS_DEVICE_PATH_NODE(node,type,subtype) (((node)->Type == (type)) && ((node)->SubType == (subtype)))
> -

Could you break these bits of moving macros and definitions into
common header files as a separate patch, preceding the rest of the changes?

>  /* Type and defines to set up the DHCP4 options */
>  
>  typedef struct {
> diff --git a/EmbeddedPkg/Application/AndroidBoot/AndroidBootApp.c b/EmbeddedPkg/Application/AndroidBoot/AndroidBootApp.c
> new file mode 100644
> index 0000000..2de1d8a
> --- /dev/null
> +++ b/EmbeddedPkg/Application/AndroidBoot/AndroidBootApp.c
> @@ -0,0 +1,127 @@
> +/** @file
> +
> +  Copyright (c) 2013-2014, ARM Ltd. All rights reserved.<BR>
> +  Copyright (c) 2017, Linaro. All rights reserved.
> +
> +  This program and the accompanying materials
> +  are licensed and made available under the terms and conditions of the BSD License
> +  which accompanies this distribution.  The full text of the license may be found at
> +  http://opensource.org/licenses/bsd-license.php
> +
> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +
> +**/
> +
> +#include <Library/AndroidBootImgLib.h>
> +#include <Library/BaseMemoryLib.h>
> +#include <Library/BdsLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/DevicePathLib.h>
> +#include <Library/MemoryAllocationLib.h>
> +#include <Library/UefiBootServicesTableLib.h>
> +
> +#include <Protocol/BlockIo.h>
> +#include <Protocol/DevicePathFromText.h>
> +
> +EFI_STATUS
> +EFIAPI
> +AndroidBootAppEntryPoint (
> +  IN EFI_HANDLE                            ImageHandle,
> +  IN EFI_SYSTEM_TABLE                      *SystemTable
> +  )
> +{
> +  EFI_STATUS                          Status;
> +  CHAR16                              *BootPathStr;
> +  EFI_DEVICE_PATH_FROM_TEXT_PROTOCOL  *EfiDevicePathFromTextProtocol;
> +  EFI_DEVICE_PATH                     *DevicePath;
> +  EFI_DEVICE_PATH_PROTOCOL            *Node, *NextNode;
> +  EFI_BLOCK_IO_PROTOCOL               *BlockIo;
> +  UINT32                              MediaId, BlockSize;
> +  VOID                                *Buffer;
> +  EFI_HANDLE                          Handle;
> +  UINTN                               Size;
> +
> +  BootPathStr = (CHAR16 *)PcdGetPtr (PcdAndroidBootDevicePath);
> +  ASSERT (BootPathStr != NULL);
> +  Status = gBS->LocateProtocol (&gEfiDevicePathFromTextProtocolGuid, NULL,
> +                                (VOID **)&EfiDevicePathFromTextProtocol);
> +  ASSERT_EFI_ERROR(Status);
> +  DevicePath = (EFI_DEVICE_PATH *)EfiDevicePathFromTextProtocol->ConvertTextToDevicePath (BootPathStr);
> +  ASSERT (DevicePath != NULL);
> +
> +  /* Find DevicePath node of Partition */
> +  NextNode = DevicePath;
> +  while (1) {

Should this not be while (NextNode != NULL), with some check that the
node was found before progressing?

> +    Node = NextNode;
> +    if (IS_DEVICE_PATH_NODE (Node, MEDIA_DEVICE_PATH, MEDIA_HARDDRIVE_DP)) {
> +      break;
> +    }
> +    NextNode = NextDevicePathNode (Node);
> +  }
> +
> +  Status = gBS->LocateDevicePath (&gEfiDevicePathProtocolGuid,
> +                                  &DevicePath, &Handle);

And should this not use &Node rather than &DevicePath?

> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  Status = gBS->OpenProtocol (
> +                  Handle,
> +                  &gEfiBlockIoProtocolGuid,
> +                  (VOID **) &BlockIo,
> +                  gImageHandle,
> +                  NULL,
> +                  EFI_OPEN_PROTOCOL_GET_PROTOCOL
> +                  );
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((EFI_D_ERROR, "Failed to get BlockIo: %r\n", Status));
> +    return Status;
> +  }
> +
> +  MediaId = BlockIo->Media->MediaId;
> +  BlockSize = BlockIo->Media->BlockSize;
> +  Buffer = AllocatePages (EFI_SIZE_TO_PAGES (sizeof(ANDROID_BOOTIMG_HEADER)));
> +  if (Buffer == NULL) {
> +    return EFI_BUFFER_TOO_SMALL;
> +  }
> +  /* Load header of boot.img */
> +  Status = BlockIo->ReadBlocks (
> +                      BlockIo,
> +                      MediaId,
> +                      0,
> +                      BlockSize,
> +                      Buffer
> +                      );
> +  Status = AbootimgGetImgSize (Buffer, &Size);

AndroidBootImgGetImageSize.

(The "img" would normally be expected to be expanded to "Image", but
it appears "boot.img" is basically the official name for this format.)

> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((EFI_D_ERROR, "Failed to get Abootimg Size: %r\n", Status));
> +    return Status;
> +  }
> +  Size = ALIGN_VALUE (Size, BlockSize);
> +  FreePages (Buffer, EFI_SIZE_TO_PAGES (sizeof(ANDROID_BOOTIMG_HEADER)));
> +
> +  /* Both PartitionStart and PartitionSize are counted as block size. */
> +  Buffer = AllocatePages (EFI_SIZE_TO_PAGES (Size));
> +  if (Buffer == NULL) {
> +    return EFI_BUFFER_TOO_SMALL;
> +  }
> +
> +  /* Load header of boot.img */
> +  Status = BlockIo->ReadBlocks (
> +                      BlockIo,
> +                      MediaId,
> +                      0,
> +                      Size,
> +                      Buffer
> +                      );
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((EFI_D_ERROR, "Failed to read blocks: %r\n", Status));
> +    goto EXIT;
> +  }
> +
> +  Status = AbootimgBoot (Buffer, Size);

AndroidBootImgBoot.

> +
> +EXIT:
> +  return Status;
> +}
> diff --git a/EmbeddedPkg/Application/AndroidBoot/AndroidBootApp.inf b/EmbeddedPkg/Application/AndroidBoot/AndroidBootApp.inf
> new file mode 100644
> index 0000000..f1ee0bd
> --- /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]
> +  AndroidBootImgLib
> +  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/Application/AndroidFastboot/AndroidBootImg.c b/EmbeddedPkg/Application/AndroidFastboot/AndroidBootImg.c
> index f3e770b..2f7f093 100644
> --- a/EmbeddedPkg/Application/AndroidFastboot/AndroidBootImg.c
> +++ b/EmbeddedPkg/Application/AndroidFastboot/AndroidBootImg.c
> @@ -14,32 +14,6 @@
>  
>  #include "AndroidFastbootApp.h"
>  
> -#define BOOT_MAGIC        "ANDROID!"
> -#define BOOT_MAGIC_LENGTH sizeof (BOOT_MAGIC) - 1
> -
> -// 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))
> -
> -// No documentation for this really - sizes of fields has been determined
> -// empirically.
> -#pragma pack(1)
> -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;
> -#pragma pack()
> -

(This bit also in separate commit, with other things moving to common headers?)

>  // Find the kernel and ramdisk in an Android boot.img.
>  // return EFI_INVALID_PARAMTER if the boot.img is invalid (i.e. doesn't have the
>  //  right magic value),
> @@ -64,7 +38,8 @@ ParseAndroidBootImg (
>  
>    Header = (ANDROID_BOOTIMG_HEADER *) BootImg;
>  
> -  if (AsciiStrnCmp (Header->BootMagic, BOOT_MAGIC, BOOT_MAGIC_LENGTH) != 0) {
> +  if (AsciiStrnCmp ((CONST CHAR8 *)Header->BootMagic, ANDROID_BOOT_MAGIC,
> +                    ANDROID_BOOT_MAGIC_LENGTH) != 0) {
>      return EFI_INVALID_PARAMETER;
>    }
>  
> @@ -72,7 +47,7 @@ ParseAndroidBootImg (
>      return EFI_NOT_FOUND;
>    }
>  
> -  ASSERT (IS_POWER_OF_2 (Header->PageSize));
> +  ASSERT (IS_VALID_ANDROID_PAGE_SIZE (Header->PageSize));
>  
>    *KernelSize = Header->KernelSize;
>    *Kernel = BootImgBytePtr + Header->PageSize;
> @@ -84,8 +59,8 @@ ParseAndroidBootImg (
>                   + ALIGN_VALUE (Header->KernelSize, Header->PageSize));
>    }
>  
> -  AsciiStrnCpyS (KernelArgs, BOOTIMG_KERNEL_ARGS_SIZE, Header->KernelArgs,
> -    BOOTIMG_KERNEL_ARGS_SIZE);
> +  AsciiStrnCpyS (KernelArgs, ANDROID_BOOTIMG_KERNEL_ARGS_SIZE, Header->KernelArgs,
> +    ANDROID_BOOTIMG_KERNEL_ARGS_SIZE);
>  
>    return EFI_SUCCESS;
>  }
> diff --git a/EmbeddedPkg/Application/AndroidFastboot/AndroidFastbootApp.h b/EmbeddedPkg/Application/AndroidFastboot/AndroidFastbootApp.h
> index f62660f..e4c5aa3 100644
> --- a/EmbeddedPkg/Application/AndroidFastboot/AndroidFastbootApp.h
> +++ b/EmbeddedPkg/Application/AndroidFastboot/AndroidFastbootApp.h
> @@ -15,6 +15,7 @@
>  #ifndef __ANDROID_FASTBOOT_APP_H__
>  #define __ANDROID_FASTBOOT_APP_H__
>  
> +#include <Library/AndroidBootImgLib.h>
>  #include <Library/BaseLib.h>
>  #include <Library/DebugLib.h>
>  #include <Library/MemoryAllocationLib.h>
> diff --git a/EmbeddedPkg/Application/AndroidFastboot/Arm/BootAndroidBootImg.c b/EmbeddedPkg/Application/AndroidFastboot/Arm/BootAndroidBootImg.c
> index f446cce..1d9024b 100644
> --- a/EmbeddedPkg/Application/AndroidFastboot/Arm/BootAndroidBootImg.c
> +++ b/EmbeddedPkg/Application/AndroidFastboot/Arm/BootAndroidBootImg.c
> @@ -112,7 +112,7 @@ BootAndroidBootImg (
>    )
>  {
>    EFI_STATUS                          Status;
> -  CHAR8                               KernelArgs[BOOTIMG_KERNEL_ARGS_SIZE];
> +  CHAR8                               KernelArgs[ANDROID_BOOTIMG_KERNEL_ARGS_SIZE];
>    VOID                               *Kernel;
>    UINTN                               KernelSize;
>    VOID                               *Ramdisk;
> diff --git a/EmbeddedPkg/Include/Library/AndroidBootImgLib.h b/EmbeddedPkg/Include/Library/AndroidBootImgLib.h
> new file mode 100644
> index 0000000..3c825eb
> --- /dev/null
> +++ b/EmbeddedPkg/Include/Library/AndroidBootImgLib.h
> @@ -0,0 +1,67 @@
> +/** @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 ANDROID_BOOTIMG_KERNEL_ARGS_SIZE  512
> +
> +#define ANDROID_BOOT_MAGIC                "ANDROID!"
> +#define ANDROID_BOOT_MAGIC_LENGTH         (sizeof (ANDROID_BOOT_MAGIC) - 1)
> +
> +/* https://android.googlesource.com/platform/system/core/+/master/mkbootimg/bootimg.h */
> +typedef struct {
> +  UINT8   BootMagic[ANDROID_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[ANDROID_BOOTIMG_KERNEL_ARGS_SIZE];
> +  UINT32  Id[32];
> +} ANDROID_BOOTIMG_HEADER;
> +
> +/* 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))
> +/* Android boot image page size is not specified, but it should be power of 2
> + * and larger than boot header */
> +#define IS_VALID_ANDROID_PAGE_SIZE(Val)   \
> +             (IS_POWER_OF_2(Val) && (Val > sizeof(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/AndroidBootImg.h b/EmbeddedPkg/Include/Protocol/AndroidBootImg.h
> new file mode 100644
> index 0000000..6bee5cf
> --- /dev/null
> +++ b/EmbeddedPkg/Include/Protocol/AndroidBootImg.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;

ANDROID_BOOTIMG_PROTOCOL

> +
> +//
> +// Function Prototypes
> +//
> +typedef
> +EFI_STATUS
> +(EFIAPI *ABOOTIMG_APPEND_KERNEL_ARGS) (

ANDROID_BOOTIMG

> +  IN CHAR16            *Args,
> +  IN UINTN              Size
> +  );
> +
> +typedef
> +EFI_STATUS
> +(EFIAPI *ABOOTIMG_UPDATE_DTB) (

ANDROID_BOOTIMG

> +  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 gAndroidBootImgProtocolGuid;
> +
> +#endif /* __ABOOTIMG_PROTOCOL_H__ */
> diff --git a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
> new file mode 100644
> index 0000000..72c6322
> --- /dev/null
> +++ b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
> @@ -0,0 +1,419 @@
> +/** @file
> +
> +  Copyright (c) 2013-2014, ARM Ltd. All rights reserved.<BR>
> +  Copyright (c) 2017, Linaro. All rights reserved.
> +
> +  This program and the accompanying materials
> +  are licensed and made available under the terms and conditions of the BSD License
> +  which accompanies this distribution.  The full text of the license may be found at
> +  http://opensource.org/licenses/bsd-license.php
> +
> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +
> +**/
> +
> +#include <libfdt.h>
> +#include <Library/AndroidBootImgLib.h>
> +#include <Library/PrintLib.h>
> +#include <Library/UefiBootServicesTableLib.h>
> +#include <Library/UefiLib.h>
> +
> +#include <Protocol/AndroidBootImg.h>
> +#include <Protocol/LoadedImage.h>
> +
> +#include <libfdt.h>
> +
> +#define FDT_ADDITIONAL_ENTRIES_SIZE 0x400
> +
> +typedef struct {
> +  MEMMAP_DEVICE_PATH                      Node1;
> +  EFI_DEVICE_PATH_PROTOCOL                End;
> +} MEMORY_DEVICE_PATH;
> +
> +STATIC ABOOTIMG_PROTOCOL                 *mAbootimg;

mAndroidBootImg.

> +
> +STATIC CONST MEMORY_DEVICE_PATH MemoryDevicePathTemplate =

Should also have an 'm'-prefix.

> +{
> +  {
> +    {
> +      HARDWARE_DEVICE_PATH,
> +      HW_MEMMAP_DP,
> +      {
> +        (UINT8)(sizeof (MEMMAP_DEVICE_PATH)),
> +        (UINT8)((sizeof (MEMMAP_DEVICE_PATH)) >> 8),
> +      },
> +    }, // Header
> +    0, // StartingAddress (set at runtime)
> +    0  // EndingAddress   (set at runtime)
> +  }, // Node1
> +  {
> +    END_DEVICE_PATH_TYPE,
> +    END_ENTIRE_DEVICE_PATH_SUBTYPE,
> +    { sizeof (EFI_DEVICE_PATH_PROTOCOL), 0 }
> +  } // End
> +};
> +
> +EFI_STATUS
> +AbootimgGetImgSize (

AndroidBootImgGetImageSize.

> +  IN  VOID    *BootImg,
> +  OUT UINTN   *ImgSize
> +  )
> +{
> +  ANDROID_BOOTIMG_HEADER   *Header;
> +
> +  Header = (ANDROID_BOOTIMG_HEADER *) BootImg;
> +
> +  if (AsciiStrnCmp ((CONST CHAR8 *)Header->BootMagic, ANDROID_BOOT_MAGIC,
> +                    ANDROID_BOOT_MAGIC_LENGTH) != 0) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  /* The page size is not specified, but it should be power of 2 at least */
> +  ASSERT (IS_VALID_ANDROID_PAGE_SIZE (Header->PageSize));
> +
> +  /* Get real size of abootimg */
> +  *ImgSize = ALIGN_VALUE (Header->KernelSize, Header->PageSize) +
> +             ALIGN_VALUE (Header->RamdiskSize, Header->PageSize) +
> +             ALIGN_VALUE (Header->SecondStageBootloaderSize, Header->PageSize) +
> +             Header->PageSize;
> +  return EFI_SUCCESS;
> +}
> +
> +EFI_STATUS
> +AbootimgGetKernelInfo (

AndroidBootImgGetKernelInfo.

> +  IN  VOID    *BootImg,
> +  OUT VOID   **Kernel,
> +  OUT UINTN   *KernelSize
> +  )
> +{
> +  ANDROID_BOOTIMG_HEADER   *Header;
> +
> +  Header = (ANDROID_BOOTIMG_HEADER *) BootImg;
> +
> +  if (AsciiStrnCmp ((CONST CHAR8 *)Header->BootMagic, ANDROID_BOOT_MAGIC,
> +                    ANDROID_BOOT_MAGIC_LENGTH) != 0) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  if (Header->KernelSize == 0) {
> +    return EFI_NOT_FOUND;
> +  }
> +
> +  ASSERT (IS_VALID_ANDROID_PAGE_SIZE (Header->PageSize));
> +
> +  *KernelSize = Header->KernelSize;
> +  *Kernel = BootImg + Header->PageSize;
> +  return EFI_SUCCESS;
> +}
> +
> +EFI_STATUS
> +AbootimgGetRamdiskInfo (
> +  IN  VOID    *BootImg,
> +  OUT VOID   **Ramdisk,
> +  OUT UINTN   *RamdiskSize
> +  )
> +{
> +  ANDROID_BOOTIMG_HEADER   *Header;
> +  UINT8                    *BootImgBytePtr;
> +
> +  // Cast to UINT8 so we can do pointer arithmetic
> +  BootImgBytePtr = (UINT8 *) BootImg;
> +
> +  Header = (ANDROID_BOOTIMG_HEADER *) BootImg;
> +
> +  if (AsciiStrnCmp ((CONST CHAR8 *)Header->BootMagic, ANDROID_BOOT_MAGIC,
> +                    ANDROID_BOOT_MAGIC_LENGTH) != 0) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  ASSERT (IS_VALID_ANDROID_PAGE_SIZE (Header->PageSize));
> +
> +  *RamdiskSize = Header->RamdiskSize;
> +
> +  if (Header->RamdiskSize != 0) {
> +    *Ramdisk = (VOID *) (BootImgBytePtr
> +                 + Header->PageSize
> +                 + ALIGN_VALUE (Header->KernelSize, Header->PageSize));
> +  }
> +  return EFI_SUCCESS;
> +}
> +
> +EFI_STATUS
> +AbootimgGetSecondBootLoaderInfo (

AndroidBootImg...

> +  IN  VOID    *BootImg,
> +  OUT VOID   **Second,
> +  OUT UINTN   *SecondSize
> +  )
> +{
> +  ANDROID_BOOTIMG_HEADER   *Header;
> +  UINT8                    *BootImgBytePtr;
> +
> +  // Cast to UINT8 so we can do pointer arithmetic
> +  BootImgBytePtr = (UINT8 *) BootImg;
> +
> +  Header = (ANDROID_BOOTIMG_HEADER *) BootImg;
> +
> +  if (AsciiStrnCmp ((CONST CHAR8 *)Header->BootMagic, ANDROID_BOOT_MAGIC,
> +                    ANDROID_BOOT_MAGIC_LENGTH) != 0) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  ASSERT (IS_VALID_ANDROID_PAGE_SIZE (Header->PageSize));
> +
> +  *SecondSize = Header->SecondStageBootloaderSize;
> +
> +  if (Header->SecondStageBootloaderSize != 0) {
> +    *Second = (VOID *) (BootImgBytePtr
> +                 + Header->PageSize
> +                 + ALIGN_VALUE (Header->KernelSize, Header->PageSize)
> +                 + ALIGN_VALUE (Header->RamdiskSize, Header->PageSize));
> +  }
> +  return EFI_SUCCESS;
> +}
> +
> +EFI_STATUS
> +AbootimgGetKernelArgs (

AndroidBootImg...

> +  IN  VOID    *BootImg,
> +  OUT CHAR8   *KernelArgs
> +  )
> +{
> +  ANDROID_BOOTIMG_HEADER   *Header;
> +
> +  Header = (ANDROID_BOOTIMG_HEADER *) BootImg;
> +  AsciiStrnCpyS (KernelArgs, ANDROID_BOOTIMG_KERNEL_ARGS_SIZE, Header->KernelArgs,
> +    ANDROID_BOOTIMG_KERNEL_ARGS_SIZE);
> +
> +  return EFI_SUCCESS;
> +}
> +
> +EFI_STATUS
> +AbootimgGetFdt (

AndroidBootImg...

> +  IN  VOID                  *BootImg,
> +  IN  VOID                  **FdtBase
> +  )
> +{
> +  UINTN                      SecondLoaderSize;
> +  EFI_STATUS                 Status;
> +
> +  /* Check whether FDT is located in second boot loader as some vendor do so,

It would be more correct to say "second boot loader region" than
"second boot loader".

> +   * because second loader is never used as far as I know. */
> +  Status = AbootimgGetSecondBootLoaderInfo (
> +          BootImg,
> +          FdtBase,
> +          &SecondLoaderSize
> +          );
> +  return Status;
> +}
> +
> +EFI_STATUS
> +AbootimgUpdateArgsFdt (

AndroidBootImgUpdateKernelArgs
(The arguments always come through Fdt, so I do not feel that needs to
be explicitly pointed out.)

General comment: this function needs to be broken down into several
smaller helper functions:
- extract kernel arguments from boot.img
- extract ramdisk information from boot.img
- locate FDT
- update FDT

> +  IN  VOID                  *BootImg,
> +  OUT VOID                  *KernelArgs
> +  )
> +{
> +  VOID                      *Ramdisk;

RamdiskData?

> +  UINT64                     Ramdisk64, RamdiskEnd64;

RamdiskStart, RamDiskEnd?

> +  UINTN                      RamdiskSize;
> +  CHAR8                      ImgKernelArgs[ANDROID_BOOTIMG_KERNEL_ARGS_SIZE];

ImageKernelArgs
or
BootImgKernelArgs

> +  INTN                       Err, NewFdtSize, chosen_node;

ChosenNode

> +  EFI_STATUS                 Status;
> +  EFI_PHYSICAL_ADDRESS       FdtBase, UpdatedFdtBase, NewFdtBase;
> +  struct fdt_property       *prop;

*Property.

> +  int                        len;

INTN Len;

> +
> +  Status = gBS->LocateProtocol (&gAndroidBootImgProtocolGuid, NULL,
> +                                (VOID **) &mAbootimg);
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  Status = AbootimgGetKernelArgs (BootImg, ImgKernelArgs);
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +  // Get kernel arguments from Android boot image
> +  AsciiStrToUnicodeStrS (ImgKernelArgs, KernelArgs,
> +                         ANDROID_BOOTIMG_KERNEL_ARGS_SIZE >> 1);
> +  // Append platform kernel arguments
> +  if(mAbootimg->AppendArgs) {
> +    Status = mAbootimg->AppendArgs (KernelArgs,
> +                                    ANDROID_BOOTIMG_KERNEL_ARGS_SIZE);
> +    if (EFI_ERROR (Status)) {
> +      return Status;
> +    }
> +  }
> +
> +  Status = EfiGetSystemConfigurationTable (&gFdtTableGuid, (VOID **)&FdtBase);
> +  if (!EFI_ERROR (Status)) {

Should this not be
  if (EFI_ERROR (Status) && Status != EFI_NOT_FOUND)
?

> +    return Status;
> +  }
> +
> +  Status = AbootimgGetFdt (BootImg, (VOID **)&FdtBase);
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +  Err = fdt_check_header ((VOID*)(UINTN)FdtBase);
> +  if (Err != 0) {
> +    DEBUG ((DEBUG_ERROR, "ERROR: Device Tree header not valid (Err:%d)\n",
> +           Err));
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  Status = AbootimgGetRamdiskInfo (
> +            BootImg,
> +            &Ramdisk,
> +            &RamdiskSize
> +            );
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  NewFdtSize = (UINTN)fdt_totalsize ((VOID*)(UINTN)(FdtBase))
> +               + FDT_ADDITIONAL_ENTRIES_SIZE;
> +  Status = gBS->AllocatePages (AllocateAnyPages, EfiBootServicesData,
> +                  EFI_SIZE_TO_PAGES (NewFdtSize), &UpdatedFdtBase);
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((EFI_D_WARN, "Warning: Failed to reallocate FDT, err %d.\n",
> +           Status));
> +    return Status;
> +  }
> +
> +  // Load the Original FDT tree into the new region
> +  Err = fdt_open_into((VOID*)FdtBase, (VOID*)UpdatedFdtBase, NewFdtSize);
> +  if (Err) {
> +    DEBUG ((EFI_D_ERROR, "fdt_open_into(): %a\n", fdt_strerror (Err)));
> +    Status = EFI_INVALID_PARAMETER;
> +    goto Fdt_Exit;
> +  }
> +
> +  Ramdisk64 = cpu_to_fdt64((UINT64)Ramdisk);
> +  RamdiskEnd64 = cpu_to_fdt64((UINT64)(Ramdisk + RamdiskSize));
> +
> +  chosen_node = fdt_subnode_offset ((const void *)UpdatedFdtBase, 0, "chosen");
> +  if (chosen_node < 0) {
> +    chosen_node = fdt_add_subnode((void *)UpdatedFdtBase, 0, "chosen");
> +      if (chosen_node < 0) {
> +        DEBUG ((EFI_D_ERROR, "Failed to find chosen node in fdt!\n"));
> +        goto Fdt_Exit;
> +    }
> +  }
> +  prop = fdt_get_property_w((void *)UpdatedFdtBase, chosen_node,
> +                            "linux,initrd-start", &len);
> +  if (NULL == prop && len == -FDT_ERR_NOTFOUND) {
> +    fdt_appendprop ((void *)UpdatedFdtBase, chosen_node,
> +                    "linux,initrd-start", &Ramdisk64, sizeof (UINT64));
> +  } else if (prop != NULL) {
> +    fdt_setprop_u64((void *)UpdatedFdtBase, chosen_node,
> +                    "linux,initrd-start", (uint64_t)Ramdisk64);
> +  } else {
> +    DEBUG ((EFI_D_ERROR, "Failed to append fdt prop initrd-start\n",
> +           fdt_strerror (Err)));
> +    Status = EFI_INVALID_PARAMETER;
> +    goto Fdt_Exit;
> +  }
> +
> +  prop = fdt_get_property_w((void *)UpdatedFdtBase, chosen_node,
> +                            "linux,initrd-end", &len);
> +  if (NULL == prop && len == -FDT_ERR_NOTFOUND) {
> +    fdt_appendprop ((void *)UpdatedFdtBase, chosen_node,
> +                    "linux,initrd-end", &RamdiskEnd64, sizeof (UINT64));
> +  } else if (prop != NULL) {
> +    fdt_setprop_u64((void *)UpdatedFdtBase, chosen_node,
> +                    "linux,initrd-end", (uint64_t)RamdiskEnd64);
> +  } else {
> +    DEBUG ((EFI_D_ERROR, "Failed to append fdt prop initrd-end\n",
> +           fdt_strerror (Err)));
> +    Status = EFI_INVALID_PARAMETER;
> +    goto Fdt_Exit;
> +  }
> +
> +  if ( mAbootimg->UpdateDtb) {
> +    Status = mAbootimg->UpdateDtb (UpdatedFdtBase, &NewFdtBase);
> +    if (EFI_ERROR (Status)) {
> +      goto Fdt_Exit;
> +    }
> +  }
> +
> +  //
> +  // Sanity checks on the new FDT blob.
> +  //
> +  Err = fdt_check_header ((VOID*)(UINTN)NewFdtBase);

I don't think this test is needed. The state of the FDT is completely
under our control at this point. The only thing it could uncover would
be a stray pointer, or a bug in libfdt.

> +  if (Err != 0) {
> +    Print (L"ERROR: Device Tree header not valid (err:%d)\n", Err);
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  Status = gBS->InstallConfigurationTable (
> +                  &gFdtTableGuid,
> +                  (VOID *)(UINTN)NewFdtBase
> +                  );
> +  if (EFI_ERROR (Status)) {
> +    goto Fdt_Exit;
> +  }
> +  return Status;

This is preference only, but I think
  if (!EFI_ERROR (Status)) {
    return EFI_SUCCESS;
  }
would be more clear.

> +
> +Fdt_Exit:
> +  gBS->FreePages (UpdatedFdtBase, EFI_SIZE_TO_PAGES (NewFdtSize));
> +  return Status;
> +}
> +
> +EFI_STATUS
> +AbootimgBoot (

AndroidBootImgBoot

> +  IN VOID                            *Buffer,
> +  IN UINTN                            BufferSize
> +  )
> +{
> +  EFI_STATUS                          Status;
> +  VOID                               *Kernel;
> +  UINTN                               KernelSize;
> +  MEMORY_DEVICE_PATH                  KernelDevicePath;
> +  EFI_HANDLE                          ImageHandle;
> +  VOID                               *NewKernelArg;
> +  EFI_LOADED_IMAGE_PROTOCOL          *ImageInfo;
> +
> +  Status = AbootimgGetKernelInfo (
> +            Buffer,
> +            &Kernel,
> +            &KernelSize
> +            );
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  NewKernelArg = AllocateZeroPool (ANDROID_BOOTIMG_KERNEL_ARGS_SIZE);
> +  if (NewKernelArg == NULL) {
> +    DEBUG ((DEBUG_ERROR, "Fail to allocate memory\n"));
> +    return EFI_OUT_OF_RESOURCES;
> +  }
> +
> +  Status = AbootimgUpdateArgsFdt (Buffer, NewKernelArg);
> +  if (EFI_ERROR (Status)) {
> +    FreePool (NewKernelArg);
> +    return EFI_INVALID_PARAMETER;

return Status?

/
    Leif

> +  }
> +
> +  KernelDevicePath = MemoryDevicePathTemplate;
> +
> +  KernelDevicePath.Node1.StartingAddress = (EFI_PHYSICAL_ADDRESS)(UINTN) Kernel;
> +  KernelDevicePath.Node1.EndingAddress   = (EFI_PHYSICAL_ADDRESS)(UINTN) Kernel
> +                                           + KernelSize;
> +
> +  Status = gBS->LoadImage (TRUE, gImageHandle,
> +                           (EFI_DEVICE_PATH *)&KernelDevicePath,
> +                           (VOID*)(UINTN)Kernel, KernelSize, &ImageHandle);
> +
> +  // Set kernel arguments
> +  Status = gBS->HandleProtocol (ImageHandle, &gEfiLoadedImageProtocolGuid,
> +                                (VOID **) &ImageInfo);
> +  ImageInfo->LoadOptions = NewKernelArg;
> +  ImageInfo->LoadOptionsSize = StrLen (NewKernelArg) * sizeof (CHAR16);
> +
> +  // Before calling the image, enable the Watchdog Timer for  the 5 Minute period
> +  gBS->SetWatchdogTimer (5 * 60, 0x10000, 0, NULL);
> +  // Start the image
> +  Status = gBS->StartImage (ImageHandle, NULL, NULL);
> +  // Clear the Watchdog Timer if the image returns
> +  gBS->SetWatchdogTimer (0, 0x10000, 0, NULL);
> +  return EFI_SUCCESS;
> +}
> diff --git a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.inf b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.inf
> new file mode 100644
> index 0000000..c92bac0
> --- /dev/null
> +++ b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.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                      = AndroidBootImgLib
> +  FILE_GUID                      = ed3b8739-6fa7-4cb1-8aeb-2496f8fcaefa
> +  MODULE_TYPE                    = BASE
> +  VERSION_STRING                 = 1.0
> +  LIBRARY_CLASS                  = AndroidBootImgLib
> +
> +#
> +# The following information is for reference only and not required by the build tools.
> +#
> +#  VALID_ARCHITECTURES           = ARM AARCH64
> +#
> +
> +[Sources]
> +  AndroidBootImgLib.c
> +
> +[LibraryClasses]
> +  DebugLib
> +  FdtLib
> +  PrintLib
> +  UefiBootServicesTableLib
> +  UefiLib
> +
> +[Packages]
> +  EmbeddedPkg/EmbeddedPkg.dec
> +  MdePkg/MdePkg.dec
> +
> +[Protocols]
> +  gAndroidBootImgProtocolGuid
> +
> +[Guids]
> +  gFdtTableGuid
> -- 
> 1.9.1
> 

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

[Leif Lindholm]

On Thu, Jul 27, 2017 at 06:07:20PM +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>

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..8ad2a84 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 }}
> +  gAndroidBootImgProtocolGuid = { 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*|0x00000057
>  
>  [PcdsFixedAtBuild.ARM]
>    gEmbeddedTokenSpaceGuid.PcdPrePiCpuMemorySize|32|UINT8|0x00000010
> -- 
> 1.9.1
> 

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

[Jun Nie]

On 2017年07月27日 22:09, Leif Lindholm wrote:
> On Thu, Jul 27, 2017 at 06:07:19PM +0800, Jun Nie wrote:
>> Add an android kernel loader that could load kernel from storage
>> device. This patch is from Haojian's code as below link. The minor
>> change is that alternative dtb is searched in second loader binary
>> of Android bootimage if dtb is not found after Linux kernel.
>> https://patches.linaro.org/patch/94683/
>>
>> This android boot image BDS add addtitional cmdline/dtb/ramfs
>> support besides kernel that is introduced by Android boot header.
> 
> Getting there. A few more comments below.
> 
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Jun Nie <jun.nie@linaro.org>
>> ---
>>   ArmPkg/Include/Library/BdsLib.h                    |   3 +
>>   ArmPkg/Library/BdsLib/BdsFilePath.c                |   3 -
>>   .../Application/AndroidBoot/AndroidBootApp.c       | 127 +++++++
>>   .../Application/AndroidBoot/AndroidBootApp.inf     |  64 ++++
>>   .../Application/AndroidFastboot/AndroidBootImg.c   |  35 +-
>>   .../AndroidFastboot/AndroidFastbootApp.h           |   1 +
>>   .../AndroidFastboot/Arm/BootAndroidBootImg.c       |   2 +-
>>   EmbeddedPkg/Include/Library/AndroidBootImgLib.h    |  67 ++++
>>   EmbeddedPkg/Include/Protocol/AndroidBootImg.h      |  47 +++
>>   .../Library/AndroidBootImgLib/AndroidBootImgLib.c  | 419 +++++++++++++++++++++
>>   .../AndroidBootImgLib/AndroidBootImgLib.inf        |  48 +++
>>   11 files changed, 782 insertions(+), 34 deletions(-)
>>   create mode 100644 EmbeddedPkg/Application/AndroidBoot/AndroidBootApp.c
>>   create mode 100644 EmbeddedPkg/Application/AndroidBoot/AndroidBootApp.inf
>>   create mode 100644 EmbeddedPkg/Include/Library/AndroidBootImgLib.h
>>   create mode 100644 EmbeddedPkg/Include/Protocol/AndroidBootImg.h
>>   create mode 100644 EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
>>   create mode 100644 EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.inf
>>
>> diff --git a/ArmPkg/Include/Library/BdsLib.h b/ArmPkg/Include/Library/BdsLib.h
>> index c58f47e..4528c2e 100644
>> --- a/ArmPkg/Include/Library/BdsLib.h
>> +++ b/ArmPkg/Include/Library/BdsLib.h
>> @@ -15,6 +15,9 @@
>>   #ifndef __BDS_ENTRY_H__
>>   #define __BDS_ENTRY_H__
>>   
>> +#define IS_DEVICE_PATH_NODE(node,type,subtype)    \
>> +        (((node)->Type == (type)) && ((node)->SubType == (subtype)))
>> +
>>   /**
>>     This is defined by the UEFI specs, don't change it
>>   **/
>> diff --git a/ArmPkg/Library/BdsLib/BdsFilePath.c b/ArmPkg/Library/BdsLib/BdsFilePath.c
>> index f9d8c4c..41557bb 100644
>> --- a/ArmPkg/Library/BdsLib/BdsFilePath.c
>> +++ b/ArmPkg/Library/BdsLib/BdsFilePath.c
>> @@ -24,9 +24,6 @@
>>   #include <Protocol/Dhcp4.h>
>>   #include <Protocol/Mtftp4.h>
>>   
>> -
>> -#define IS_DEVICE_PATH_NODE(node,type,subtype) (((node)->Type == (type)) && ((node)->SubType == (subtype)))
>> -
> 
> Could you break these bits of moving macros and definitions into
> common header files as a separate patch, preceding the rest of the changes?

Will do.
> 
>>   /* Type and defines to set up the DHCP4 options */
>>   
>>   typedef struct {
>> diff --git a/EmbeddedPkg/Application/AndroidBoot/AndroidBootApp.c b/EmbeddedPkg/Application/AndroidBoot/AndroidBootApp.c
>> new file mode 100644
>> index 0000000..2de1d8a
>> --- /dev/null
>> +++ b/EmbeddedPkg/Application/AndroidBoot/AndroidBootApp.c
>> @@ -0,0 +1,127 @@
>> +/** @file
>> +
>> +  Copyright (c) 2013-2014, ARM Ltd. All rights reserved.<BR>
>> +  Copyright (c) 2017, Linaro. All rights reserved.
>> +
>> +  This program and the accompanying materials
>> +  are licensed and made available under the terms and conditions of the BSD License
>> +  which accompanies this distribution.  The full text of the license may be found at
>> +  http://opensource.org/licenses/bsd-license.php
>> +
>> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
>> +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>> +
>> +**/
>> +
>> +#include <Library/AndroidBootImgLib.h>
>> +#include <Library/BaseMemoryLib.h>
>> +#include <Library/BdsLib.h>
>> +#include <Library/DebugLib.h>
>> +#include <Library/DevicePathLib.h>
>> +#include <Library/MemoryAllocationLib.h>
>> +#include <Library/UefiBootServicesTableLib.h>
>> +
>> +#include <Protocol/BlockIo.h>
>> +#include <Protocol/DevicePathFromText.h>
>> +
>> +EFI_STATUS
>> +EFIAPI
>> +AndroidBootAppEntryPoint (
>> +  IN EFI_HANDLE                            ImageHandle,
>> +  IN EFI_SYSTEM_TABLE                      *SystemTable
>> +  )
>> +{
>> +  EFI_STATUS                          Status;
>> +  CHAR16                              *BootPathStr;
>> +  EFI_DEVICE_PATH_FROM_TEXT_PROTOCOL  *EfiDevicePathFromTextProtocol;
>> +  EFI_DEVICE_PATH                     *DevicePath;
>> +  EFI_DEVICE_PATH_PROTOCOL            *Node, *NextNode;
>> +  EFI_BLOCK_IO_PROTOCOL               *BlockIo;
>> +  UINT32                              MediaId, BlockSize;
>> +  VOID                                *Buffer;
>> +  EFI_HANDLE                          Handle;
>> +  UINTN                               Size;
>> +
>> +  BootPathStr = (CHAR16 *)PcdGetPtr (PcdAndroidBootDevicePath);
>> +  ASSERT (BootPathStr != NULL);
>> +  Status = gBS->LocateProtocol (&gEfiDevicePathFromTextProtocolGuid, NULL,
>> +                                (VOID **)&EfiDevicePathFromTextProtocol);
>> +  ASSERT_EFI_ERROR(Status);
>> +  DevicePath = (EFI_DEVICE_PATH *)EfiDevicePathFromTextProtocol->ConvertTextToDevicePath (BootPathStr);
>> +  ASSERT (DevicePath != NULL);
>> +
>> +  /* Find DevicePath node of Partition */
>> +  NextNode = DevicePath;
>> +  while (1) {
> 
> Should this not be while (NextNode != NULL), with some check that the
> node was found before progressing?

(NextNode != NULL) is valid check.
The code check node before progressing as below, doesn't it?
> 
>> +    Node = NextNode;
>> +    if (IS_DEVICE_PATH_NODE (Node, MEDIA_DEVICE_PATH, MEDIA_HARDDRIVE_DP)) {
>> +      break;
>> +    }
>> +    NextNode = NextDevicePathNode (Node);
>> +  }
>> +
>> +  Status = gBS->LocateDevicePath (&gEfiDevicePathProtocolGuid,
>> +                                  &DevicePath, &Handle);
> 
> And should this not use &Node rather than &DevicePath?
I suppose we should return error if no MEDIA_HARDDRIVE_DP node is found. 
I ever did test and found original DevicePath should be used here as a 
full device path, otherwise the boot image cannot be found.
> 
>> +  if (EFI_ERROR (Status)) {
>> +    return Status;
>> +  }
>> +
>> +  Status = gBS->OpenProtocol (
>> +                  Handle,
>> +                  &gEfiBlockIoProtocolGuid,
>> +                  (VOID **) &BlockIo,
>> +                  gImageHandle,
>> +                  NULL,
>> +                  EFI_OPEN_PROTOCOL_GET_PROTOCOL
>> +                  );
>> +  if (EFI_ERROR (Status)) {
>> +    DEBUG ((EFI_D_ERROR, "Failed to get BlockIo: %r\n", Status));
>> +    return Status;
>> +  }
>> +
>> +  MediaId = BlockIo->Media->MediaId;
>> +  BlockSize = BlockIo->Media->BlockSize;
>> +  Buffer = AllocatePages (EFI_SIZE_TO_PAGES (sizeof(ANDROID_BOOTIMG_HEADER)));
>> +  if (Buffer == NULL) {
>> +    return EFI_BUFFER_TOO_SMALL;
>> +  }
>> +  /* Load header of boot.img */
>> +  Status = BlockIo->ReadBlocks (
>> +                      BlockIo,
>> +                      MediaId,
>> +                      0,
>> +                      BlockSize,
>> +                      Buffer
>> +                      );
>> +  Status = AbootimgGetImgSize (Buffer, &Size);
> 
> AndroidBootImgGetImageSize.

Will do.
> 
> (The "img" would normally be expected to be expanded to "Image", but
> it appears "boot.img" is basically the official name for this format.)
> 
>> +  if (EFI_ERROR (Status)) {
>> +    DEBUG ((EFI_D_ERROR, "Failed to get Abootimg Size: %r\n", Status));
>> +    return Status;
>> +  }
>> +  Size = ALIGN_VALUE (Size, BlockSize);
>> +  FreePages (Buffer, EFI_SIZE_TO_PAGES (sizeof(ANDROID_BOOTIMG_HEADER)));
>> +
>> +  /* Both PartitionStart and PartitionSize are counted as block size. */
>> +  Buffer = AllocatePages (EFI_SIZE_TO_PAGES (Size));
>> +  if (Buffer == NULL) {
>> +    return EFI_BUFFER_TOO_SMALL;
>> +  }
>> +
>> +  /* Load header of boot.img */
>> +  Status = BlockIo->ReadBlocks (
>> +                      BlockIo,
>> +                      MediaId,
>> +                      0,
>> +                      Size,
>> +                      Buffer
>> +                      );
>> +  if (EFI_ERROR (Status)) {
>> +    DEBUG ((EFI_D_ERROR, "Failed to read blocks: %r\n", Status));
>> +    goto EXIT;
>> +  }
>> +
>> +  Status = AbootimgBoot (Buffer, Size);
> 
> AndroidBootImgBoot.

Will do.
> 
>> +
>> +EXIT:
>> +  return Status;
>> +}
>> diff --git a/EmbeddedPkg/Application/AndroidBoot/AndroidBootApp.inf b/EmbeddedPkg/Application/AndroidBoot/AndroidBootApp.inf
>> new file mode 100644
>> index 0000000..f1ee0bd
>> --- /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]
>> +  AndroidBootImgLib
>> +  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/Application/AndroidFastboot/AndroidBootImg.c b/EmbeddedPkg/Application/AndroidFastboot/AndroidBootImg.c
>> index f3e770b..2f7f093 100644
>> --- a/EmbeddedPkg/Application/AndroidFastboot/AndroidBootImg.c
>> +++ b/EmbeddedPkg/Application/AndroidFastboot/AndroidBootImg.c
>> @@ -14,32 +14,6 @@
>>   
>>   #include "AndroidFastbootApp.h"
>>   
>> -#define BOOT_MAGIC        "ANDROID!"
>> -#define BOOT_MAGIC_LENGTH sizeof (BOOT_MAGIC) - 1
>> -
>> -// 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))
>> -
>> -// No documentation for this really - sizes of fields has been determined
>> -// empirically.
>> -#pragma pack(1)
>> -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;
>> -#pragma pack()
>> -
> 
> (This bit also in separate commit, with other things moving to common headers?)

Will do.
> 
>>   // Find the kernel and ramdisk in an Android boot.img.
>>   // return EFI_INVALID_PARAMTER if the boot.img is invalid (i.e. doesn't have the
>>   //  right magic value),
>> @@ -64,7 +38,8 @@ ParseAndroidBootImg (
>>   
>>     Header = (ANDROID_BOOTIMG_HEADER *) BootImg;
>>   
>> -  if (AsciiStrnCmp (Header->BootMagic, BOOT_MAGIC, BOOT_MAGIC_LENGTH) != 0) {
>> +  if (AsciiStrnCmp ((CONST CHAR8 *)Header->BootMagic, ANDROID_BOOT_MAGIC,
>> +                    ANDROID_BOOT_MAGIC_LENGTH) != 0) {
>>       return EFI_INVALID_PARAMETER;
>>     }
>>   
>> @@ -72,7 +47,7 @@ ParseAndroidBootImg (
>>       return EFI_NOT_FOUND;
>>     }
>>   
>> -  ASSERT (IS_POWER_OF_2 (Header->PageSize));
>> +  ASSERT (IS_VALID_ANDROID_PAGE_SIZE (Header->PageSize));
>>   
>>     *KernelSize = Header->KernelSize;
>>     *Kernel = BootImgBytePtr + Header->PageSize;
>> @@ -84,8 +59,8 @@ ParseAndroidBootImg (
>>                    + ALIGN_VALUE (Header->KernelSize, Header->PageSize));
>>     }
>>   
>> -  AsciiStrnCpyS (KernelArgs, BOOTIMG_KERNEL_ARGS_SIZE, Header->KernelArgs,
>> -    BOOTIMG_KERNEL_ARGS_SIZE);
>> +  AsciiStrnCpyS (KernelArgs, ANDROID_BOOTIMG_KERNEL_ARGS_SIZE, Header->KernelArgs,
>> +    ANDROID_BOOTIMG_KERNEL_ARGS_SIZE);
>>   
>>     return EFI_SUCCESS;
>>   }
>> diff --git a/EmbeddedPkg/Application/AndroidFastboot/AndroidFastbootApp.h b/EmbeddedPkg/Application/AndroidFastboot/AndroidFastbootApp.h
>> index f62660f..e4c5aa3 100644
>> --- a/EmbeddedPkg/Application/AndroidFastboot/AndroidFastbootApp.h
>> +++ b/EmbeddedPkg/Application/AndroidFastboot/AndroidFastbootApp.h
>> @@ -15,6 +15,7 @@
>>   #ifndef __ANDROID_FASTBOOT_APP_H__
>>   #define __ANDROID_FASTBOOT_APP_H__
>>   
>> +#include <Library/AndroidBootImgLib.h>
>>   #include <Library/BaseLib.h>
>>   #include <Library/DebugLib.h>
>>   #include <Library/MemoryAllocationLib.h>
>> diff --git a/EmbeddedPkg/Application/AndroidFastboot/Arm/BootAndroidBootImg.c b/EmbeddedPkg/Application/AndroidFastboot/Arm/BootAndroidBootImg.c
>> index f446cce..1d9024b 100644
>> --- a/EmbeddedPkg/Application/AndroidFastboot/Arm/BootAndroidBootImg.c
>> +++ b/EmbeddedPkg/Application/AndroidFastboot/Arm/BootAndroidBootImg.c
>> @@ -112,7 +112,7 @@ BootAndroidBootImg (
>>     )
>>   {
>>     EFI_STATUS                          Status;
>> -  CHAR8                               KernelArgs[BOOTIMG_KERNEL_ARGS_SIZE];
>> +  CHAR8                               KernelArgs[ANDROID_BOOTIMG_KERNEL_ARGS_SIZE];
>>     VOID                               *Kernel;
>>     UINTN                               KernelSize;
>>     VOID                               *Ramdisk;
>> diff --git a/EmbeddedPkg/Include/Library/AndroidBootImgLib.h b/EmbeddedPkg/Include/Library/AndroidBootImgLib.h
>> new file mode 100644
>> index 0000000..3c825eb
>> --- /dev/null
>> +++ b/EmbeddedPkg/Include/Library/AndroidBootImgLib.h
>> @@ -0,0 +1,67 @@
>> +/** @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 ANDROID_BOOTIMG_KERNEL_ARGS_SIZE  512
>> +
>> +#define ANDROID_BOOT_MAGIC                "ANDROID!"
>> +#define ANDROID_BOOT_MAGIC_LENGTH         (sizeof (ANDROID_BOOT_MAGIC) - 1)
>> +
>> +/* https://android.googlesource.com/platform/system/core/+/master/mkbootimg/bootimg.h */
>> +typedef struct {
>> +  UINT8   BootMagic[ANDROID_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[ANDROID_BOOTIMG_KERNEL_ARGS_SIZE];
>> +  UINT32  Id[32];
>> +} ANDROID_BOOTIMG_HEADER;
>> +
>> +/* 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))
>> +/* Android boot image page size is not specified, but it should be power of 2
>> + * and larger than boot header */
>> +#define IS_VALID_ANDROID_PAGE_SIZE(Val)   \
>> +             (IS_POWER_OF_2(Val) && (Val > sizeof(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/AndroidBootImg.h b/EmbeddedPkg/Include/Protocol/AndroidBootImg.h
>> new file mode 100644
>> index 0000000..6bee5cf
>> --- /dev/null
>> +++ b/EmbeddedPkg/Include/Protocol/AndroidBootImg.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;
> 
> ANDROID_BOOTIMG_PROTOCOL

Will do.
> 
>> +
>> +//
>> +// Function Prototypes
>> +//
>> +typedef
>> +EFI_STATUS
>> +(EFIAPI *ABOOTIMG_APPEND_KERNEL_ARGS) (
> 
> ANDROID_BOOTIMG

Will do.
> 
>> +  IN CHAR16            *Args,
>> +  IN UINTN              Size
>> +  );
>> +
>> +typedef
>> +EFI_STATUS
>> +(EFIAPI *ABOOTIMG_UPDATE_DTB) (
> 
> ANDROID_BOOTIMG

Will do.
> 
>> +  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 gAndroidBootImgProtocolGuid;
>> +
>> +#endif /* __ABOOTIMG_PROTOCOL_H__ */
>> diff --git a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
>> new file mode 100644
>> index 0000000..72c6322
>> --- /dev/null
>> +++ b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
>> @@ -0,0 +1,419 @@
>> +/** @file
>> +
>> +  Copyright (c) 2013-2014, ARM Ltd. All rights reserved.<BR>
>> +  Copyright (c) 2017, Linaro. All rights reserved.
>> +
>> +  This program and the accompanying materials
>> +  are licensed and made available under the terms and conditions of the BSD License
>> +  which accompanies this distribution.  The full text of the license may be found at
>> +  http://opensource.org/licenses/bsd-license.php
>> +
>> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
>> +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>> +
>> +**/
>> +
>> +#include <libfdt.h>
>> +#include <Library/AndroidBootImgLib.h>
>> +#include <Library/PrintLib.h>
>> +#include <Library/UefiBootServicesTableLib.h>
>> +#include <Library/UefiLib.h>
>> +
>> +#include <Protocol/AndroidBootImg.h>
>> +#include <Protocol/LoadedImage.h>
>> +
>> +#include <libfdt.h>
>> +
>> +#define FDT_ADDITIONAL_ENTRIES_SIZE 0x400
>> +
>> +typedef struct {
>> +  MEMMAP_DEVICE_PATH                      Node1;
>> +  EFI_DEVICE_PATH_PROTOCOL                End;
>> +} MEMORY_DEVICE_PATH;
>> +
>> +STATIC ABOOTIMG_PROTOCOL                 *mAbootimg;
> 
> mAndroidBootImg.

Will do.
> 
>> +
>> +STATIC CONST MEMORY_DEVICE_PATH MemoryDevicePathTemplate =
> 
> Should also have an 'm'-prefix.

Will do. But what 'm' prefix stand for?
> 
>> +{
>> +  {
>> +    {
>> +      HARDWARE_DEVICE_PATH,
>> +      HW_MEMMAP_DP,
>> +      {
>> +        (UINT8)(sizeof (MEMMAP_DEVICE_PATH)),
>> +        (UINT8)((sizeof (MEMMAP_DEVICE_PATH)) >> 8),
>> +      },
>> +    }, // Header
>> +    0, // StartingAddress (set at runtime)
>> +    0  // EndingAddress   (set at runtime)
>> +  }, // Node1
>> +  {
>> +    END_DEVICE_PATH_TYPE,
>> +    END_ENTIRE_DEVICE_PATH_SUBTYPE,
>> +    { sizeof (EFI_DEVICE_PATH_PROTOCOL), 0 }
>> +  } // End
>> +};
>> +
>> +EFI_STATUS
>> +AbootimgGetImgSize (
> 
> AndroidBootImgGetImageSize.

Will do.
> 
>> +  IN  VOID    *BootImg,
>> +  OUT UINTN   *ImgSize
>> +  )
>> +{
>> +  ANDROID_BOOTIMG_HEADER   *Header;
>> +
>> +  Header = (ANDROID_BOOTIMG_HEADER *) BootImg;
>> +
>> +  if (AsciiStrnCmp ((CONST CHAR8 *)Header->BootMagic, ANDROID_BOOT_MAGIC,
>> +                    ANDROID_BOOT_MAGIC_LENGTH) != 0) {
>> +    return EFI_INVALID_PARAMETER;
>> +  }
>> +
>> +  /* The page size is not specified, but it should be power of 2 at least */
>> +  ASSERT (IS_VALID_ANDROID_PAGE_SIZE (Header->PageSize));
>> +
>> +  /* Get real size of abootimg */
>> +  *ImgSize = ALIGN_VALUE (Header->KernelSize, Header->PageSize) +
>> +             ALIGN_VALUE (Header->RamdiskSize, Header->PageSize) +
>> +             ALIGN_VALUE (Header->SecondStageBootloaderSize, Header->PageSize) +
>> +             Header->PageSize;
>> +  return EFI_SUCCESS;
>> +}
>> +
>> +EFI_STATUS
>> +AbootimgGetKernelInfo (
> 
> AndroidBootImgGetKernelInfo.

Will do.
> 
>> +  IN  VOID    *BootImg,
>> +  OUT VOID   **Kernel,
>> +  OUT UINTN   *KernelSize
>> +  )
>> +{
>> +  ANDROID_BOOTIMG_HEADER   *Header;
>> +
>> +  Header = (ANDROID_BOOTIMG_HEADER *) BootImg;
>> +
>> +  if (AsciiStrnCmp ((CONST CHAR8 *)Header->BootMagic, ANDROID_BOOT_MAGIC,
>> +                    ANDROID_BOOT_MAGIC_LENGTH) != 0) {
>> +    return EFI_INVALID_PARAMETER;
>> +  }
>> +
>> +  if (Header->KernelSize == 0) {
>> +    return EFI_NOT_FOUND;
>> +  }
>> +
>> +  ASSERT (IS_VALID_ANDROID_PAGE_SIZE (Header->PageSize));
>> +
>> +  *KernelSize = Header->KernelSize;
>> +  *Kernel = BootImg + Header->PageSize;
>> +  return EFI_SUCCESS;
>> +}
>> +
>> +EFI_STATUS
>> +AbootimgGetRamdiskInfo (
>> +  IN  VOID    *BootImg,
>> +  OUT VOID   **Ramdisk,
>> +  OUT UINTN   *RamdiskSize
>> +  )
>> +{
>> +  ANDROID_BOOTIMG_HEADER   *Header;
>> +  UINT8                    *BootImgBytePtr;
>> +
>> +  // Cast to UINT8 so we can do pointer arithmetic
>> +  BootImgBytePtr = (UINT8 *) BootImg;
>> +
>> +  Header = (ANDROID_BOOTIMG_HEADER *) BootImg;
>> +
>> +  if (AsciiStrnCmp ((CONST CHAR8 *)Header->BootMagic, ANDROID_BOOT_MAGIC,
>> +                    ANDROID_BOOT_MAGIC_LENGTH) != 0) {
>> +    return EFI_INVALID_PARAMETER;
>> +  }
>> +
>> +  ASSERT (IS_VALID_ANDROID_PAGE_SIZE (Header->PageSize));
>> +
>> +  *RamdiskSize = Header->RamdiskSize;
>> +
>> +  if (Header->RamdiskSize != 0) {
>> +    *Ramdisk = (VOID *) (BootImgBytePtr
>> +                 + Header->PageSize
>> +                 + ALIGN_VALUE (Header->KernelSize, Header->PageSize));
>> +  }
>> +  return EFI_SUCCESS;
>> +}
>> +
>> +EFI_STATUS
>> +AbootimgGetSecondBootLoaderInfo (
> 
> AndroidBootImg...

Will do.
> 
>> +  IN  VOID    *BootImg,
>> +  OUT VOID   **Second,
>> +  OUT UINTN   *SecondSize
>> +  )
>> +{
>> +  ANDROID_BOOTIMG_HEADER   *Header;
>> +  UINT8                    *BootImgBytePtr;
>> +
>> +  // Cast to UINT8 so we can do pointer arithmetic
>> +  BootImgBytePtr = (UINT8 *) BootImg;
>> +
>> +  Header = (ANDROID_BOOTIMG_HEADER *) BootImg;
>> +
>> +  if (AsciiStrnCmp ((CONST CHAR8 *)Header->BootMagic, ANDROID_BOOT_MAGIC,
>> +                    ANDROID_BOOT_MAGIC_LENGTH) != 0) {
>> +    return EFI_INVALID_PARAMETER;
>> +  }
>> +
>> +  ASSERT (IS_VALID_ANDROID_PAGE_SIZE (Header->PageSize));
>> +
>> +  *SecondSize = Header->SecondStageBootloaderSize;
>> +
>> +  if (Header->SecondStageBootloaderSize != 0) {
>> +    *Second = (VOID *) (BootImgBytePtr
>> +                 + Header->PageSize
>> +                 + ALIGN_VALUE (Header->KernelSize, Header->PageSize)
>> +                 + ALIGN_VALUE (Header->RamdiskSize, Header->PageSize));
>> +  }
>> +  return EFI_SUCCESS;
>> +}
>> +
>> +EFI_STATUS
>> +AbootimgGetKernelArgs (
> 
> AndroidBootImg...

Will do.
> 
>> +  IN  VOID    *BootImg,
>> +  OUT CHAR8   *KernelArgs
>> +  )
>> +{
>> +  ANDROID_BOOTIMG_HEADER   *Header;
>> +
>> +  Header = (ANDROID_BOOTIMG_HEADER *) BootImg;
>> +  AsciiStrnCpyS (KernelArgs, ANDROID_BOOTIMG_KERNEL_ARGS_SIZE, Header->KernelArgs,
>> +    ANDROID_BOOTIMG_KERNEL_ARGS_SIZE);
>> +
>> +  return EFI_SUCCESS;
>> +}
>> +
>> +EFI_STATUS
>> +AbootimgGetFdt (
> 
> AndroidBootImg...

Will do.
> 
>> +  IN  VOID                  *BootImg,
>> +  IN  VOID                  **FdtBase
>> +  )
>> +{
>> +  UINTN                      SecondLoaderSize;
>> +  EFI_STATUS                 Status;
>> +
>> +  /* Check whether FDT is located in second boot loader as some vendor do so,
> 
> It would be more correct to say "second boot loader region" than
> "second boot loader".
> 
>> +   * because second loader is never used as far as I know. */
>> +  Status = AbootimgGetSecondBootLoaderInfo (
>> +          BootImg,
>> +          FdtBase,
>> +          &SecondLoaderSize
>> +          );
>> +  return Status;
>> +}
>> +
>> +EFI_STATUS
>> +AbootimgUpdateArgsFdt (
> 
> AndroidBootImgUpdateKernelArgs
> (The arguments always come through Fdt, so I do not feel that needs to
> be explicitly pointed out.)
Command line come from Android boot image cmdline field, not from fdt in
most cases, if not all. I will split argument update function too.
> 
> General comment: this function needs to be broken down into several
> smaller helper functions:
> - extract kernel arguments from boot.img
> - extract ramdisk information from boot.img
> - locate FDT
> - update FDT

Will do.
> 
>> +  IN  VOID                  *BootImg,
>> +  OUT VOID                  *KernelArgs
>> +  )
>> +{
>> +  VOID                      *Ramdisk;
> 
> RamdiskData?
Yes, Ramdisk data start address.
> 
>> +  UINT64                     Ramdisk64, RamdiskEnd64;
> 
> RamdiskStart, RamDiskEnd?

Yes, will do.
> 
>> +  UINTN                      RamdiskSize;
>> +  CHAR8                      ImgKernelArgs[ANDROID_BOOTIMG_KERNEL_ARGS_SIZE];
> 
> ImageKernelArgs
> or
> BootImgKernelArgs

Will do.
> 
>> +  INTN                       Err, NewFdtSize, chosen_node;
> 
> ChosenNode

Will do. I had thought fdt library related code shall follow the library 
coding style :)
> 
>> +  EFI_STATUS                 Status;
>> +  EFI_PHYSICAL_ADDRESS       FdtBase, UpdatedFdtBase, NewFdtBase;
>> +  struct fdt_property       *prop;
> 
> *Property.

Will do.
> 
>> +  int                        len;
> 
> INTN Len;

Will do.
> 
>> +
>> +  Status = gBS->LocateProtocol (&gAndroidBootImgProtocolGuid, NULL,
>> +                                (VOID **) &mAbootimg);
>> +  if (EFI_ERROR (Status)) {
>> +    return Status;
>> +  }
>> +
>> +  Status = AbootimgGetKernelArgs (BootImg, ImgKernelArgs);
>> +  if (EFI_ERROR (Status)) {
>> +    return Status;
>> +  }
>> +  // Get kernel arguments from Android boot image
>> +  AsciiStrToUnicodeStrS (ImgKernelArgs, KernelArgs,
>> +                         ANDROID_BOOTIMG_KERNEL_ARGS_SIZE >> 1);
>> +  // Append platform kernel arguments
>> +  if(mAbootimg->AppendArgs) {
>> +    Status = mAbootimg->AppendArgs (KernelArgs,
>> +                                    ANDROID_BOOTIMG_KERNEL_ARGS_SIZE);
>> +    if (EFI_ERROR (Status)) {
>> +      return Status;
>> +    }
>> +  }
>> +
>> +  Status = EfiGetSystemConfigurationTable (&gFdtTableGuid, (VOID **)&FdtBase);
>> +  if (!EFI_ERROR (Status)) {
> 
> Should this not be
>    if (EFI_ERROR (Status) && Status != EFI_NOT_FOUND)
> ?
I mean, if fdt is found, we shall return to avoid installing another 
fdt. But actually, I expect fdt is tied with kernel in Android boot 
image in standard Android boot image usage cases. Though it is agreed
to decouple fdt and kernel in community in 2013, Android boot image
format has been decided several years before that :) .

We can make change in future if Android boot image usage case changes.
Maybe I can add a warning message to highlight the new case.

> 
>> +    return Status;
>> +  }
>> +
>> +  Status = AbootimgGetFdt (BootImg, (VOID **)&FdtBase);
>> +  if (EFI_ERROR (Status)) {
>> +    return Status;
>> +  }
>> +  Err = fdt_check_header ((VOID*)(UINTN)FdtBase);
>> +  if (Err != 0) {
>> +    DEBUG ((DEBUG_ERROR, "ERROR: Device Tree header not valid (Err:%d)\n",
>> +           Err));
>> +    return EFI_INVALID_PARAMETER;
>> +  }
>> +
>> +  Status = AbootimgGetRamdiskInfo (
>> +            BootImg,
>> +            &Ramdisk,
>> +            &RamdiskSize
>> +            );
>> +  if (EFI_ERROR (Status)) {
>> +    return Status;
>> +  }
>> +
>> +  NewFdtSize = (UINTN)fdt_totalsize ((VOID*)(UINTN)(FdtBase))
>> +               + FDT_ADDITIONAL_ENTRIES_SIZE;
>> +  Status = gBS->AllocatePages (AllocateAnyPages, EfiBootServicesData,
>> +                  EFI_SIZE_TO_PAGES (NewFdtSize), &UpdatedFdtBase);
>> +  if (EFI_ERROR (Status)) {
>> +    DEBUG ((EFI_D_WARN, "Warning: Failed to reallocate FDT, err %d.\n",
>> +           Status));
>> +    return Status;
>> +  }
>> +
>> +  // Load the Original FDT tree into the new region
>> +  Err = fdt_open_into((VOID*)FdtBase, (VOID*)UpdatedFdtBase, NewFdtSize);
>> +  if (Err) {
>> +    DEBUG ((EFI_D_ERROR, "fdt_open_into(): %a\n", fdt_strerror (Err)));
>> +    Status = EFI_INVALID_PARAMETER;
>> +    goto Fdt_Exit;
>> +  }
>> +
>> +  Ramdisk64 = cpu_to_fdt64((UINT64)Ramdisk);
>> +  RamdiskEnd64 = cpu_to_fdt64((UINT64)(Ramdisk + RamdiskSize));
>> +
>> +  chosen_node = fdt_subnode_offset ((const void *)UpdatedFdtBase, 0, "chosen");
>> +  if (chosen_node < 0) {
>> +    chosen_node = fdt_add_subnode((void *)UpdatedFdtBase, 0, "chosen");
>> +      if (chosen_node < 0) {
>> +        DEBUG ((EFI_D_ERROR, "Failed to find chosen node in fdt!\n"));
>> +        goto Fdt_Exit;
>> +    }
>> +  }
>> +  prop = fdt_get_property_w((void *)UpdatedFdtBase, chosen_node,
>> +                            "linux,initrd-start", &len);
>> +  if (NULL == prop && len == -FDT_ERR_NOTFOUND) {
>> +    fdt_appendprop ((void *)UpdatedFdtBase, chosen_node,
>> +                    "linux,initrd-start", &Ramdisk64, sizeof (UINT64));
>> +  } else if (prop != NULL) {
>> +    fdt_setprop_u64((void *)UpdatedFdtBase, chosen_node,
>> +                    "linux,initrd-start", (uint64_t)Ramdisk64);
>> +  } else {
>> +    DEBUG ((EFI_D_ERROR, "Failed to append fdt prop initrd-start\n",
>> +           fdt_strerror (Err)));
>> +    Status = EFI_INVALID_PARAMETER;
>> +    goto Fdt_Exit;
>> +  }
>> +
>> +  prop = fdt_get_property_w((void *)UpdatedFdtBase, chosen_node,
>> +                            "linux,initrd-end", &len);
>> +  if (NULL == prop && len == -FDT_ERR_NOTFOUND) {
>> +    fdt_appendprop ((void *)UpdatedFdtBase, chosen_node,
>> +                    "linux,initrd-end", &RamdiskEnd64, sizeof (UINT64));
>> +  } else if (prop != NULL) {
>> +    fdt_setprop_u64((void *)UpdatedFdtBase, chosen_node,
>> +                    "linux,initrd-end", (uint64_t)RamdiskEnd64);
>> +  } else {
>> +    DEBUG ((EFI_D_ERROR, "Failed to append fdt prop initrd-end\n",
>> +           fdt_strerror (Err)));
>> +    Status = EFI_INVALID_PARAMETER;
>> +    goto Fdt_Exit;
>> +  }
>> +
>> +  if ( mAbootimg->UpdateDtb) {
>> +    Status = mAbootimg->UpdateDtb (UpdatedFdtBase, &NewFdtBase);
>> +    if (EFI_ERROR (Status)) {
>> +      goto Fdt_Exit;
>> +    }
>> +  }
>> +
>> +  //
>> +  // Sanity checks on the new FDT blob.
>> +  //
>> +  Err = fdt_check_header ((VOID*)(UINTN)NewFdtBase);
> 
> I don't think this test is needed. The state of the FDT is completely
> under our control at this point. The only thing it could uncover would
> be a stray pointer, or a bug in libfdt.

Yes. We should check just after Fdt is read from boot image.
> 
>> +  if (Err != 0) {
>> +    Print (L"ERROR: Device Tree header not valid (err:%d)\n", Err);
>> +    return EFI_INVALID_PARAMETER;
>> +  }
>> +
>> +  Status = gBS->InstallConfigurationTable (
>> +                  &gFdtTableGuid,
>> +                  (VOID *)(UINTN)NewFdtBase
>> +                  );
>> +  if (EFI_ERROR (Status)) {
>> +    goto Fdt_Exit;
>> +  }
>> +  return Status;
> 
> This is preference only, but I think
>    if (!EFI_ERROR (Status)) {
>      return EFI_SUCCESS;
>    }
> would be more clear.

Will do.
> 
>> +
>> +Fdt_Exit:
>> +  gBS->FreePages (UpdatedFdtBase, EFI_SIZE_TO_PAGES (NewFdtSize));
>> +  return Status;
>> +}
>> +
>> +EFI_STATUS
>> +AbootimgBoot (
> 
> AndroidBootImgBoot

Will do.
> 
>> +  IN VOID                            *Buffer,
>> +  IN UINTN                            BufferSize
>> +  )
>> +{
>> +  EFI_STATUS                          Status;
>> +  VOID                               *Kernel;
>> +  UINTN                               KernelSize;
>> +  MEMORY_DEVICE_PATH                  KernelDevicePath;
>> +  EFI_HANDLE                          ImageHandle;
>> +  VOID                               *NewKernelArg;
>> +  EFI_LOADED_IMAGE_PROTOCOL          *ImageInfo;
>> +
>> +  Status = AbootimgGetKernelInfo (
>> +            Buffer,
>> +            &Kernel,
>> +            &KernelSize
>> +            );
>> +  if (EFI_ERROR (Status)) {
>> +    return Status;
>> +  }
>> +
>> +  NewKernelArg = AllocateZeroPool (ANDROID_BOOTIMG_KERNEL_ARGS_SIZE);
>> +  if (NewKernelArg == NULL) {
>> +    DEBUG ((DEBUG_ERROR, "Fail to allocate memory\n"));
>> +    return EFI_OUT_OF_RESOURCES;
>> +  }
>> +
>> +  Status = AbootimgUpdateArgsFdt (Buffer, NewKernelArg);
>> +  if (EFI_ERROR (Status)) {
>> +    FreePool (NewKernelArg);
>> +    return EFI_INVALID_PARAMETER;
> 
> return Status?
> 
Will do.
> /
>      Leif
> 
>> +  }
>> +
>> +  KernelDevicePath = MemoryDevicePathTemplate;
>> +
>> +  KernelDevicePath.Node1.StartingAddress = (EFI_PHYSICAL_ADDRESS)(UINTN) Kernel;
>> +  KernelDevicePath.Node1.EndingAddress   = (EFI_PHYSICAL_ADDRESS)(UINTN) Kernel
>> +                                           + KernelSize;
>> +
>> +  Status = gBS->LoadImage (TRUE, gImageHandle,
>> +                           (EFI_DEVICE_PATH *)&KernelDevicePath,
>> +                           (VOID*)(UINTN)Kernel, KernelSize, &ImageHandle);
>> +
>> +  // Set kernel arguments
>> +  Status = gBS->HandleProtocol (ImageHandle, &gEfiLoadedImageProtocolGuid,
>> +                                (VOID **) &ImageInfo);
>> +  ImageInfo->LoadOptions = NewKernelArg;
>> +  ImageInfo->LoadOptionsSize = StrLen (NewKernelArg) * sizeof (CHAR16);
>> +
>> +  // Before calling the image, enable the Watchdog Timer for  the 5 Minute period
>> +  gBS->SetWatchdogTimer (5 * 60, 0x10000, 0, NULL);
>> +  // Start the image
>> +  Status = gBS->StartImage (ImageHandle, NULL, NULL);
>> +  // Clear the Watchdog Timer if the image returns
>> +  gBS->SetWatchdogTimer (0, 0x10000, 0, NULL);
>> +  return EFI_SUCCESS;
>> +}
>> diff --git a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.inf b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.inf
>> new file mode 100644
>> index 0000000..c92bac0
>> --- /dev/null
>> +++ b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.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                      = AndroidBootImgLib
>> +  FILE_GUID                      = ed3b8739-6fa7-4cb1-8aeb-2496f8fcaefa
>> +  MODULE_TYPE                    = BASE
>> +  VERSION_STRING                 = 1.0
>> +  LIBRARY_CLASS                  = AndroidBootImgLib
>> +
>> +#
>> +# The following information is for reference only and not required by the build tools.
>> +#
>> +#  VALID_ARCHITECTURES           = ARM AARCH64
>> +#
>> +
>> +[Sources]
>> +  AndroidBootImgLib.c
>> +
>> +[LibraryClasses]
>> +  DebugLib
>> +  FdtLib
>> +  PrintLib
>> +  UefiBootServicesTableLib
>> +  UefiLib
>> +
>> +[Packages]
>> +  EmbeddedPkg/EmbeddedPkg.dec
>> +  MdePkg/MdePkg.dec
>> +
>> +[Protocols]
>> +  gAndroidBootImgProtocolGuid
>> +
>> +[Guids]
>> +  gFdtTableGuid
>> -- 
>> 1.9.1
>>

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

[Leif Lindholm]

On Fri, Jul 28, 2017 at 05:47:46PM +0800, Jun Nie wrote:
> On 2017年07月27日 22:09, Leif Lindholm wrote:
> >On Thu, Jul 27, 2017 at 06:07:19PM +0800, Jun Nie wrote:
> >>Add an android kernel loader that could load kernel from storage
> >>device. This patch is from Haojian's code as below link. The minor
> >>change is that alternative dtb is searched in second loader binary
> >>of Android bootimage if dtb is not found after Linux kernel.
> >>https://patches.linaro.org/patch/94683/
> >>
> >>This android boot image BDS add addtitional cmdline/dtb/ramfs
> >>support besides kernel that is introduced by Android boot header.
> >
> >Getting there. A few more comments below.
> >
> >>Contributed-under: TianoCore Contribution Agreement 1.0
> >>Signed-off-by: Jun Nie <jun.nie@linaro.org>
> >>---
> >>  ArmPkg/Include/Library/BdsLib.h                    |   3 +
> >>  ArmPkg/Library/BdsLib/BdsFilePath.c                |   3 -
> >>  .../Application/AndroidBoot/AndroidBootApp.c       | 127 +++++++
> >>  .../Application/AndroidBoot/AndroidBootApp.inf     |  64 ++++
> >>  .../Application/AndroidFastboot/AndroidBootImg.c   |  35 +-
> >>  .../AndroidFastboot/AndroidFastbootApp.h           |   1 +
> >>  .../AndroidFastboot/Arm/BootAndroidBootImg.c       |   2 +-
> >>  EmbeddedPkg/Include/Library/AndroidBootImgLib.h    |  67 ++++
> >>  EmbeddedPkg/Include/Protocol/AndroidBootImg.h      |  47 +++
> >>  .../Library/AndroidBootImgLib/AndroidBootImgLib.c  | 419 +++++++++++++++++++++
> >>  .../AndroidBootImgLib/AndroidBootImgLib.inf        |  48 +++
> >>  11 files changed, 782 insertions(+), 34 deletions(-)
> >>  create mode 100644 EmbeddedPkg/Application/AndroidBoot/AndroidBootApp.c
> >>  create mode 100644 EmbeddedPkg/Application/AndroidBoot/AndroidBootApp.inf
> >>  create mode 100644 EmbeddedPkg/Include/Library/AndroidBootImgLib.h
> >>  create mode 100644 EmbeddedPkg/Include/Protocol/AndroidBootImg.h
> >>  create mode 100644 EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
> >>  create mode 100644 EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.inf
> >>

> >>diff --git a/EmbeddedPkg/Application/AndroidBoot/AndroidBootApp.c b/EmbeddedPkg/Application/AndroidBoot/AndroidBootApp.c
> >>new file mode 100644
> >>index 0000000..2de1d8a
> >>--- /dev/null
> >>+++ b/EmbeddedPkg/Application/AndroidBoot/AndroidBootApp.c
> >>@@ -0,0 +1,127 @@
> >>+/** @file
> >>+
> >>+  Copyright (c) 2013-2014, ARM Ltd. All rights reserved.<BR>
> >>+  Copyright (c) 2017, Linaro. All rights reserved.
> >>+
> >>+  This program and the accompanying materials
> >>+  are licensed and made available under the terms and conditions of the BSD License
> >>+  which accompanies this distribution.  The full text of the license may be found at
> >>+  http://opensource.org/licenses/bsd-license.php
> >>+
> >>+  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> >>+  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> >>+
> >>+**/
> >>+
> >>+#include <Library/AndroidBootImgLib.h>
> >>+#include <Library/BaseMemoryLib.h>
> >>+#include <Library/BdsLib.h>
> >>+#include <Library/DebugLib.h>
> >>+#include <Library/DevicePathLib.h>
> >>+#include <Library/MemoryAllocationLib.h>
> >>+#include <Library/UefiBootServicesTableLib.h>
> >>+
> >>+#include <Protocol/BlockIo.h>
> >>+#include <Protocol/DevicePathFromText.h>
> >>+
> >>+EFI_STATUS
> >>+EFIAPI
> >>+AndroidBootAppEntryPoint (
> >>+  IN EFI_HANDLE                            ImageHandle,
> >>+  IN EFI_SYSTEM_TABLE                      *SystemTable
> >>+  )
> >>+{
> >>+  EFI_STATUS                          Status;
> >>+  CHAR16                              *BootPathStr;
> >>+  EFI_DEVICE_PATH_FROM_TEXT_PROTOCOL  *EfiDevicePathFromTextProtocol;
> >>+  EFI_DEVICE_PATH                     *DevicePath;
> >>+  EFI_DEVICE_PATH_PROTOCOL            *Node, *NextNode;
> >>+  EFI_BLOCK_IO_PROTOCOL               *BlockIo;
> >>+  UINT32                              MediaId, BlockSize;
> >>+  VOID                                *Buffer;
> >>+  EFI_HANDLE                          Handle;
> >>+  UINTN                               Size;
> >>+
> >>+  BootPathStr = (CHAR16 *)PcdGetPtr (PcdAndroidBootDevicePath);
> >>+  ASSERT (BootPathStr != NULL);
> >>+  Status = gBS->LocateProtocol (&gEfiDevicePathFromTextProtocolGuid, NULL,
> >>+                                (VOID **)&EfiDevicePathFromTextProtocol);
> >>+  ASSERT_EFI_ERROR(Status);
> >>+  DevicePath = (EFI_DEVICE_PATH *)EfiDevicePathFromTextProtocol->ConvertTextToDevicePath (BootPathStr);
> >>+  ASSERT (DevicePath != NULL);
> >>+
> >>+  /* Find DevicePath node of Partition */
> >>+  NextNode = DevicePath;
> >>+  while (1) {
> >
> >Should this not be while (NextNode != NULL), with some check that the
> >node was found before progressing?
> 
> (NextNode != NULL) is valid check.
> The code check node before progressing as below, doesn't it?

My point is that if you never match the "if (IS_DEVICE_PATH_NODE "
condition, this loop will never terminate.

And if we update the loop condition to fix that, we end up calling
LocateDevicePath with a known bad parameter.

> >
> >>+    Node = NextNode;
> >>+    if (IS_DEVICE_PATH_NODE (Node, MEDIA_DEVICE_PATH, MEDIA_HARDDRIVE_DP)) {
> >>+      break;
> >>+    }
> >>+    NextNode = NextDevicePathNode (Node);
> >>+  }
> >>+
> >>+  Status = gBS->LocateDevicePath (&gEfiDevicePathProtocolGuid,
> >>+                                  &DevicePath, &Handle);
> >
> >And should this not use &Node rather than &DevicePath?
> I suppose we should return error if no MEDIA_HARDDRIVE_DP node is found. I
> ever did test and found original DevicePath should be used here as a full
> device path, otherwise the boot image cannot be found.

So does this mean that the section that initializes "Node" to a
MEDIA_HARDDRIVE_DP is only there to validate DevicePath?
If so, I think it should be split out into a static helper function,
called ValidateDevicePath or similar, with a description of what
condition it is trying to verify.

> >
> >>+  if (EFI_ERROR (Status)) {
> >>+    return Status;
> >>+  }
> >>+
> >>+  Status = gBS->OpenProtocol (
> >>+                  Handle,
> >>+                  &gEfiBlockIoProtocolGuid,
> >>+                  (VOID **) &BlockIo,
> >>+                  gImageHandle,
> >>+                  NULL,
> >>+                  EFI_OPEN_PROTOCOL_GET_PROTOCOL
> >>+                  );
> >>+  if (EFI_ERROR (Status)) {
> >>+    DEBUG ((EFI_D_ERROR, "Failed to get BlockIo: %r\n", Status));
> >>+    return Status;
> >>+  }
> >>+
> >>+  MediaId = BlockIo->Media->MediaId;
> >>+  BlockSize = BlockIo->Media->BlockSize;
> >>+  Buffer = AllocatePages (EFI_SIZE_TO_PAGES (sizeof(ANDROID_BOOTIMG_HEADER)));
> >>+  if (Buffer == NULL) {
> >>+    return EFI_BUFFER_TOO_SMALL;
> >>+  }
> >>+  /* Load header of boot.img */
> >>+  Status = BlockIo->ReadBlocks (
> >>+                      BlockIo,
> >>+                      MediaId,
> >>+                      0,
> >>+                      BlockSize,
> >>+                      Buffer
> >>+                      );
> >>+  Status = AbootimgGetImgSize (Buffer, &Size);
> >
> >AndroidBootImgGetImageSize.
> 
> Will do.
> >
> >(The "img" would normally be expected to be expanded to "Image", but
> >it appears "boot.img" is basically the official name for this format.)
> >
> >>+  if (EFI_ERROR (Status)) {
> >>+    DEBUG ((EFI_D_ERROR, "Failed to get Abootimg Size: %r\n", Status));
> >>+    return Status;
> >>+  }
> >>+  Size = ALIGN_VALUE (Size, BlockSize);
> >>+  FreePages (Buffer, EFI_SIZE_TO_PAGES (sizeof(ANDROID_BOOTIMG_HEADER)));
> >>+
> >>+  /* Both PartitionStart and PartitionSize are counted as block size. */
> >>+  Buffer = AllocatePages (EFI_SIZE_TO_PAGES (Size));
> >>+  if (Buffer == NULL) {
> >>+    return EFI_BUFFER_TOO_SMALL;
> >>+  }
> >>+
> >>+  /* Load header of boot.img */
> >>+  Status = BlockIo->ReadBlocks (
> >>+                      BlockIo,
> >>+                      MediaId,
> >>+                      0,
> >>+                      Size,
> >>+                      Buffer
> >>+                      );
> >>+  if (EFI_ERROR (Status)) {
> >>+    DEBUG ((EFI_D_ERROR, "Failed to read blocks: %r\n", Status));
> >>+    goto EXIT;
> >>+  }
> >>+
> >>+  Status = AbootimgBoot (Buffer, Size);
> >
> >AndroidBootImgBoot.
> 
> Will do.
> >
> >>+
> >>+EXIT:
> >>+  return Status;
> >>+}


> >>diff --git a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
> >>new file mode 100644
> >>index 0000000..72c6322
> >>--- /dev/null
> >>+++ b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
> >>@@ -0,0 +1,419 @@
> >>+/** @file
> >>+
> >>+  Copyright (c) 2013-2014, ARM Ltd. All rights reserved.<BR>
> >>+  Copyright (c) 2017, Linaro. All rights reserved.
> >>+
> >>+  This program and the accompanying materials
> >>+  are licensed and made available under the terms and conditions of the BSD License
> >>+  which accompanies this distribution.  The full text of the license may be found at
> >>+  http://opensource.org/licenses/bsd-license.php
> >>+
> >>+  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> >>+  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> >>+
> >>+**/
> >>+
> >>+#include <libfdt.h>
> >>+#include <Library/AndroidBootImgLib.h>
> >>+#include <Library/PrintLib.h>
> >>+#include <Library/UefiBootServicesTableLib.h>
> >>+#include <Library/UefiLib.h>
> >>+
> >>+#include <Protocol/AndroidBootImg.h>
> >>+#include <Protocol/LoadedImage.h>
> >>+
> >>+#include <libfdt.h>
> >>+
> >>+#define FDT_ADDITIONAL_ENTRIES_SIZE 0x400
> >>+
> >>+typedef struct {
> >>+  MEMMAP_DEVICE_PATH                      Node1;
> >>+  EFI_DEVICE_PATH_PROTOCOL                End;
> >>+} MEMORY_DEVICE_PATH;
> >>+
> >>+STATIC ABOOTIMG_PROTOCOL                 *mAbootimg;
> >
> >mAndroidBootImg.
> 
> Will do.
> >
> >>+
> >>+STATIC CONST MEMORY_DEVICE_PATH MemoryDevicePathTemplate =
> >
> >Should also have an 'm'-prefix.
> 
> Will do. But what 'm' prefix stand for?
> >
> >>+{
> >>+  {
> >>+    {
> >>+      HARDWARE_DEVICE_PATH,
> >>+      HW_MEMMAP_DP,
> >>+      {
> >>+        (UINT8)(sizeof (MEMMAP_DEVICE_PATH)),
> >>+        (UINT8)((sizeof (MEMMAP_DEVICE_PATH)) >> 8),
> >>+      },
> >>+    }, // Header
> >>+    0, // StartingAddress (set at runtime)
> >>+    0  // EndingAddress   (set at runtime)
> >>+  }, // Node1
> >>+  {
> >>+    END_DEVICE_PATH_TYPE,
> >>+    END_ENTIRE_DEVICE_PATH_SUBTYPE,
> >>+    { sizeof (EFI_DEVICE_PATH_PROTOCOL), 0 }
> >>+  } // End
> >>+};
> >>+
> >>+EFI_STATUS
> >>+AbootimgGetImgSize (
> >
> >AndroidBootImgGetImageSize.
> 
> Will do.
> >
> >>+  IN  VOID    *BootImg,
> >>+  OUT UINTN   *ImgSize
> >>+  )
> >>+{
> >>+  ANDROID_BOOTIMG_HEADER   *Header;
> >>+
> >>+  Header = (ANDROID_BOOTIMG_HEADER *) BootImg;
> >>+
> >>+  if (AsciiStrnCmp ((CONST CHAR8 *)Header->BootMagic, ANDROID_BOOT_MAGIC,
> >>+                    ANDROID_BOOT_MAGIC_LENGTH) != 0) {
> >>+    return EFI_INVALID_PARAMETER;
> >>+  }
> >>+
> >>+  /* The page size is not specified, but it should be power of 2 at least */
> >>+  ASSERT (IS_VALID_ANDROID_PAGE_SIZE (Header->PageSize));
> >>+
> >>+  /* Get real size of abootimg */
> >>+  *ImgSize = ALIGN_VALUE (Header->KernelSize, Header->PageSize) +
> >>+             ALIGN_VALUE (Header->RamdiskSize, Header->PageSize) +
> >>+             ALIGN_VALUE (Header->SecondStageBootloaderSize, Header->PageSize) +
> >>+             Header->PageSize;
> >>+  return EFI_SUCCESS;
> >>+}
> >>+
> >>+EFI_STATUS
> >>+AbootimgGetKernelInfo (
> >
> >AndroidBootImgGetKernelInfo.
> 
> Will do.
> >
> >>+  IN  VOID    *BootImg,
> >>+  OUT VOID   **Kernel,
> >>+  OUT UINTN   *KernelSize
> >>+  )
> >>+{
> >>+  ANDROID_BOOTIMG_HEADER   *Header;
> >>+
> >>+  Header = (ANDROID_BOOTIMG_HEADER *) BootImg;
> >>+
> >>+  if (AsciiStrnCmp ((CONST CHAR8 *)Header->BootMagic, ANDROID_BOOT_MAGIC,
> >>+                    ANDROID_BOOT_MAGIC_LENGTH) != 0) {
> >>+    return EFI_INVALID_PARAMETER;
> >>+  }
> >>+
> >>+  if (Header->KernelSize == 0) {
> >>+    return EFI_NOT_FOUND;
> >>+  }
> >>+
> >>+  ASSERT (IS_VALID_ANDROID_PAGE_SIZE (Header->PageSize));
> >>+
> >>+  *KernelSize = Header->KernelSize;
> >>+  *Kernel = BootImg + Header->PageSize;
> >>+  return EFI_SUCCESS;
> >>+}
> >>+
> >>+EFI_STATUS
> >>+AbootimgGetRamdiskInfo (
> >>+  IN  VOID    *BootImg,
> >>+  OUT VOID   **Ramdisk,
> >>+  OUT UINTN   *RamdiskSize
> >>+  )
> >>+{
> >>+  ANDROID_BOOTIMG_HEADER   *Header;
> >>+  UINT8                    *BootImgBytePtr;
> >>+
> >>+  // Cast to UINT8 so we can do pointer arithmetic
> >>+  BootImgBytePtr = (UINT8 *) BootImg;
> >>+
> >>+  Header = (ANDROID_BOOTIMG_HEADER *) BootImg;
> >>+
> >>+  if (AsciiStrnCmp ((CONST CHAR8 *)Header->BootMagic, ANDROID_BOOT_MAGIC,
> >>+                    ANDROID_BOOT_MAGIC_LENGTH) != 0) {
> >>+    return EFI_INVALID_PARAMETER;
> >>+  }
> >>+
> >>+  ASSERT (IS_VALID_ANDROID_PAGE_SIZE (Header->PageSize));
> >>+
> >>+  *RamdiskSize = Header->RamdiskSize;
> >>+
> >>+  if (Header->RamdiskSize != 0) {
> >>+    *Ramdisk = (VOID *) (BootImgBytePtr
> >>+                 + Header->PageSize
> >>+                 + ALIGN_VALUE (Header->KernelSize, Header->PageSize));
> >>+  }
> >>+  return EFI_SUCCESS;
> >>+}
> >>+
> >>+EFI_STATUS
> >>+AbootimgGetSecondBootLoaderInfo (
> >
> >AndroidBootImg...
> 
> Will do.
> >
> >>+  IN  VOID    *BootImg,
> >>+  OUT VOID   **Second,
> >>+  OUT UINTN   *SecondSize
> >>+  )
> >>+{
> >>+  ANDROID_BOOTIMG_HEADER   *Header;
> >>+  UINT8                    *BootImgBytePtr;
> >>+
> >>+  // Cast to UINT8 so we can do pointer arithmetic
> >>+  BootImgBytePtr = (UINT8 *) BootImg;
> >>+
> >>+  Header = (ANDROID_BOOTIMG_HEADER *) BootImg;
> >>+
> >>+  if (AsciiStrnCmp ((CONST CHAR8 *)Header->BootMagic, ANDROID_BOOT_MAGIC,
> >>+                    ANDROID_BOOT_MAGIC_LENGTH) != 0) {
> >>+    return EFI_INVALID_PARAMETER;
> >>+  }
> >>+
> >>+  ASSERT (IS_VALID_ANDROID_PAGE_SIZE (Header->PageSize));
> >>+
> >>+  *SecondSize = Header->SecondStageBootloaderSize;
> >>+
> >>+  if (Header->SecondStageBootloaderSize != 0) {
> >>+    *Second = (VOID *) (BootImgBytePtr
> >>+                 + Header->PageSize
> >>+                 + ALIGN_VALUE (Header->KernelSize, Header->PageSize)
> >>+                 + ALIGN_VALUE (Header->RamdiskSize, Header->PageSize));
> >>+  }
> >>+  return EFI_SUCCESS;
> >>+}
> >>+
> >>+EFI_STATUS
> >>+AbootimgGetKernelArgs (
> >
> >AndroidBootImg...
> 
> Will do.
> >
> >>+  IN  VOID    *BootImg,
> >>+  OUT CHAR8   *KernelArgs
> >>+  )
> >>+{
> >>+  ANDROID_BOOTIMG_HEADER   *Header;
> >>+
> >>+  Header = (ANDROID_BOOTIMG_HEADER *) BootImg;
> >>+  AsciiStrnCpyS (KernelArgs, ANDROID_BOOTIMG_KERNEL_ARGS_SIZE, Header->KernelArgs,
> >>+    ANDROID_BOOTIMG_KERNEL_ARGS_SIZE);
> >>+
> >>+  return EFI_SUCCESS;
> >>+}
> >>+
> >>+EFI_STATUS
> >>+AbootimgGetFdt (
> >
> >AndroidBootImg...
> 
> Will do.
> >
> >>+  IN  VOID                  *BootImg,
> >>+  IN  VOID                  **FdtBase
> >>+  )
> >>+{
> >>+  UINTN                      SecondLoaderSize;
> >>+  EFI_STATUS                 Status;
> >>+
> >>+  /* Check whether FDT is located in second boot loader as some vendor do so,
> >
> >It would be more correct to say "second boot loader region" than
> >"second boot loader".
> >
> >>+   * because second loader is never used as far as I know. */
> >>+  Status = AbootimgGetSecondBootLoaderInfo (
> >>+          BootImg,
> >>+          FdtBase,
> >>+          &SecondLoaderSize
> >>+          );
> >>+  return Status;
> >>+}
> >>+
> >>+EFI_STATUS
> >>+AbootimgUpdateArgsFdt (
> >
> >AndroidBootImgUpdateKernelArgs
> >(The arguments always come through Fdt, so I do not feel that needs to
> >be explicitly pointed out.)
> Command line come from Android boot image cmdline field, not from fdt in
> most cases, if not all. I will split argument update function too.
> >
> >General comment: this function needs to be broken down into several
> >smaller helper functions:
> >- extract kernel arguments from boot.img
> >- extract ramdisk information from boot.img
> >- locate FDT
> >- update FDT
> 
> Will do.
> >
> >>+  IN  VOID                  *BootImg,
> >>+  OUT VOID                  *KernelArgs
> >>+  )
> >>+{
> >>+  VOID                      *Ramdisk;
> >
> >RamdiskData?
> Yes, Ramdisk data start address.
> >
> >>+  UINT64                     Ramdisk64, RamdiskEnd64;
> >
> >RamdiskStart, RamDiskEnd?
> 
> Yes, will do.
> >
> >>+  UINTN                      RamdiskSize;
> >>+  CHAR8                      ImgKernelArgs[ANDROID_BOOTIMG_KERNEL_ARGS_SIZE];
> >
> >ImageKernelArgs
> >or
> >BootImgKernelArgs
> 
> Will do.
> >
> >>+  INTN                       Err, NewFdtSize, chosen_node;
> >
> >ChosenNode
> 
> Will do. I had thought fdt library related code shall follow the library
> coding style :)

Yes, this is a tricky area.
I'm considering putting together a wrapper library for common DT
operations to abstract this away. But for now, I prefer keeping to
TianoCore coding style everywhere except as is needed to call libfdt.

> >
> >>+  EFI_STATUS                 Status;
> >>+  EFI_PHYSICAL_ADDRESS       FdtBase, UpdatedFdtBase, NewFdtBase;
> >>+  struct fdt_property       *prop;
> >
> >*Property.
> 
> Will do.
> >
> >>+  int                        len;
> >
> >INTN Len;
> 
> Will do.
> >
> >>+
> >>+  Status = gBS->LocateProtocol (&gAndroidBootImgProtocolGuid, NULL,
> >>+                                (VOID **) &mAbootimg);
> >>+  if (EFI_ERROR (Status)) {
> >>+    return Status;
> >>+  }
> >>+
> >>+  Status = AbootimgGetKernelArgs (BootImg, ImgKernelArgs);
> >>+  if (EFI_ERROR (Status)) {
> >>+    return Status;
> >>+  }
> >>+  // Get kernel arguments from Android boot image
> >>+  AsciiStrToUnicodeStrS (ImgKernelArgs, KernelArgs,
> >>+                         ANDROID_BOOTIMG_KERNEL_ARGS_SIZE >> 1);
> >>+  // Append platform kernel arguments
> >>+  if(mAbootimg->AppendArgs) {
> >>+    Status = mAbootimg->AppendArgs (KernelArgs,
> >>+                                    ANDROID_BOOTIMG_KERNEL_ARGS_SIZE);
> >>+    if (EFI_ERROR (Status)) {
> >>+      return Status;
> >>+    }
> >>+  }
> >>+
> >>+  Status = EfiGetSystemConfigurationTable (&gFdtTableGuid, (VOID **)&FdtBase);
> >>+  if (!EFI_ERROR (Status)) {
> >
> >Should this not be
> >   if (EFI_ERROR (Status) && Status != EFI_NOT_FOUND)
> >?
>
> I mean, if fdt is found, we shall return to avoid installing another fdt.

An FDT presented to you by firmware is just the hardware description.
Any command line or initrd updates that are required will still need
to happen in order to boot. So the same manipulations that happen to
the DT embedded in boot.img need to happen to one presented via a
configuration table.

> But actually, I expect fdt is tied with kernel in Android boot image in
> standard Android boot image usage cases.
> Though it is agreed to decouple fdt and kernel in community in 2013,
> Android boot image format has been decided several years before that :) .
> 
> We can make change in future if Android boot image usage case changes.
> Maybe I can add a warning message to highlight the new case.

No.

We can tolerate booting broken existing images, but we should not
design to intentionally break things ourselves.

I guess as this is an application, you could even add a command-line
option to let you override an existing registered DT with one embedded
in boot.img.

But ignoring an existing registered DT is not an option.

> >>+    return Status;
> >>+  }
> >>+
> >>+  Status = AbootimgGetFdt (BootImg, (VOID **)&FdtBase);
> >>+  if (EFI_ERROR (Status)) {
> >>+    return Status;
> >>+  }
> >>+  Err = fdt_check_header ((VOID*)(UINTN)FdtBase);
> >>+  if (Err != 0) {
> >>+    DEBUG ((DEBUG_ERROR, "ERROR: Device Tree header not valid (Err:%d)\n",
> >>+           Err));
> >>+    return EFI_INVALID_PARAMETER;
> >>+  }
> >>+
> >>+  Status = AbootimgGetRamdiskInfo (
> >>+            BootImg,
> >>+            &Ramdisk,
> >>+            &RamdiskSize
> >>+            );
> >>+  if (EFI_ERROR (Status)) {
> >>+    return Status;
> >>+  }
> >>+
> >>+  NewFdtSize = (UINTN)fdt_totalsize ((VOID*)(UINTN)(FdtBase))
> >>+               + FDT_ADDITIONAL_ENTRIES_SIZE;
> >>+  Status = gBS->AllocatePages (AllocateAnyPages, EfiBootServicesData,
> >>+                  EFI_SIZE_TO_PAGES (NewFdtSize), &UpdatedFdtBase);
> >>+  if (EFI_ERROR (Status)) {
> >>+    DEBUG ((EFI_D_WARN, "Warning: Failed to reallocate FDT, err %d.\n",
> >>+           Status));
> >>+    return Status;
> >>+  }
> >>+
> >>+  // Load the Original FDT tree into the new region
> >>+  Err = fdt_open_into((VOID*)FdtBase, (VOID*)UpdatedFdtBase, NewFdtSize);
> >>+  if (Err) {
> >>+    DEBUG ((EFI_D_ERROR, "fdt_open_into(): %a\n", fdt_strerror (Err)));
> >>+    Status = EFI_INVALID_PARAMETER;
> >>+    goto Fdt_Exit;
> >>+  }
> >>+
> >>+  Ramdisk64 = cpu_to_fdt64((UINT64)Ramdisk);
> >>+  RamdiskEnd64 = cpu_to_fdt64((UINT64)(Ramdisk + RamdiskSize));
> >>+
> >>+  chosen_node = fdt_subnode_offset ((const void *)UpdatedFdtBase, 0, "chosen");
> >>+  if (chosen_node < 0) {
> >>+    chosen_node = fdt_add_subnode((void *)UpdatedFdtBase, 0, "chosen");
> >>+      if (chosen_node < 0) {
> >>+        DEBUG ((EFI_D_ERROR, "Failed to find chosen node in fdt!\n"));
> >>+        goto Fdt_Exit;
> >>+    }
> >>+  }
> >>+  prop = fdt_get_property_w((void *)UpdatedFdtBase, chosen_node,
> >>+                            "linux,initrd-start", &len);
> >>+  if (NULL == prop && len == -FDT_ERR_NOTFOUND) {
> >>+    fdt_appendprop ((void *)UpdatedFdtBase, chosen_node,
> >>+                    "linux,initrd-start", &Ramdisk64, sizeof (UINT64));
> >>+  } else if (prop != NULL) {
> >>+    fdt_setprop_u64((void *)UpdatedFdtBase, chosen_node,
> >>+                    "linux,initrd-start", (uint64_t)Ramdisk64);
> >>+  } else {
> >>+    DEBUG ((EFI_D_ERROR, "Failed to append fdt prop initrd-start\n",
> >>+           fdt_strerror (Err)));
> >>+    Status = EFI_INVALID_PARAMETER;
> >>+    goto Fdt_Exit;
> >>+  }
> >>+
> >>+  prop = fdt_get_property_w((void *)UpdatedFdtBase, chosen_node,
> >>+                            "linux,initrd-end", &len);
> >>+  if (NULL == prop && len == -FDT_ERR_NOTFOUND) {
> >>+    fdt_appendprop ((void *)UpdatedFdtBase, chosen_node,
> >>+                    "linux,initrd-end", &RamdiskEnd64, sizeof (UINT64));
> >>+  } else if (prop != NULL) {
> >>+    fdt_setprop_u64((void *)UpdatedFdtBase, chosen_node,
> >>+                    "linux,initrd-end", (uint64_t)RamdiskEnd64);
> >>+  } else {
> >>+    DEBUG ((EFI_D_ERROR, "Failed to append fdt prop initrd-end\n",
> >>+           fdt_strerror (Err)));
> >>+    Status = EFI_INVALID_PARAMETER;
> >>+    goto Fdt_Exit;
> >>+  }
> >>+
> >>+  if ( mAbootimg->UpdateDtb) {
> >>+    Status = mAbootimg->UpdateDtb (UpdatedFdtBase, &NewFdtBase);
> >>+    if (EFI_ERROR (Status)) {
> >>+      goto Fdt_Exit;
> >>+    }
> >>+  }
> >>+
> >>+  //
> >>+  // Sanity checks on the new FDT blob.
> >>+  //
> >>+  Err = fdt_check_header ((VOID*)(UINTN)NewFdtBase);
> >
> >I don't think this test is needed. The state of the FDT is completely
> >under our control at this point. The only thing it could uncover would
> >be a stray pointer, or a bug in libfdt.
> 
> Yes. We should check just after Fdt is read from boot image.

Or existing in configuration table.
(As is already done.)

/
    Leif

> >
> >>+  if (Err != 0) {
> >>+    Print (L"ERROR: Device Tree header not valid (err:%d)\n", Err);
> >>+    return EFI_INVALID_PARAMETER;
> >>+  }
> >>+
> >>+  Status = gBS->InstallConfigurationTable (
> >>+                  &gFdtTableGuid,
> >>+                  (VOID *)(UINTN)NewFdtBase
> >>+                  );
> >>+  if (EFI_ERROR (Status)) {
> >>+    goto Fdt_Exit;
> >>+  }
> >>+  return Status;
> >
> >This is preference only, but I think
> >   if (!EFI_ERROR (Status)) {
> >     return EFI_SUCCESS;
> >   }
> >would be more clear.
> 
> Will do.
> >
> >>+
> >>+Fdt_Exit:
> >>+  gBS->FreePages (UpdatedFdtBase, EFI_SIZE_TO_PAGES (NewFdtSize));
> >>+  return Status;
> >>+}
> >>+
> >>+EFI_STATUS
> >>+AbootimgBoot (
> >
> >AndroidBootImgBoot
> 
> Will do.
> >
> >>+  IN VOID                            *Buffer,
> >>+  IN UINTN                            BufferSize
> >>+  )
> >>+{
> >>+  EFI_STATUS                          Status;
> >>+  VOID                               *Kernel;
> >>+  UINTN                               KernelSize;
> >>+  MEMORY_DEVICE_PATH                  KernelDevicePath;
> >>+  EFI_HANDLE                          ImageHandle;
> >>+  VOID                               *NewKernelArg;
> >>+  EFI_LOADED_IMAGE_PROTOCOL          *ImageInfo;
> >>+
> >>+  Status = AbootimgGetKernelInfo (
> >>+            Buffer,
> >>+            &Kernel,
> >>+            &KernelSize
> >>+            );
> >>+  if (EFI_ERROR (Status)) {
> >>+    return Status;
> >>+  }
> >>+
> >>+  NewKernelArg = AllocateZeroPool (ANDROID_BOOTIMG_KERNEL_ARGS_SIZE);
> >>+  if (NewKernelArg == NULL) {
> >>+    DEBUG ((DEBUG_ERROR, "Fail to allocate memory\n"));
> >>+    return EFI_OUT_OF_RESOURCES;
> >>+  }
> >>+
> >>+  Status = AbootimgUpdateArgsFdt (Buffer, NewKernelArg);
> >>+  if (EFI_ERROR (Status)) {
> >>+    FreePool (NewKernelArg);
> >>+    return EFI_INVALID_PARAMETER;
> >
> >return Status?
> >
> Will do.
> >/
> >     Leif
> >
> >>+  }
> >>+
> >>+  KernelDevicePath = MemoryDevicePathTemplate;
> >>+
> >>+  KernelDevicePath.Node1.StartingAddress = (EFI_PHYSICAL_ADDRESS)(UINTN) Kernel;
> >>+  KernelDevicePath.Node1.EndingAddress   = (EFI_PHYSICAL_ADDRESS)(UINTN) Kernel
> >>+                                           + KernelSize;
> >>+
> >>+  Status = gBS->LoadImage (TRUE, gImageHandle,
> >>+                           (EFI_DEVICE_PATH *)&KernelDevicePath,
> >>+                           (VOID*)(UINTN)Kernel, KernelSize, &ImageHandle);
> >>+
> >>+  // Set kernel arguments
> >>+  Status = gBS->HandleProtocol (ImageHandle, &gEfiLoadedImageProtocolGuid,
> >>+                                (VOID **) &ImageInfo);
> >>+  ImageInfo->LoadOptions = NewKernelArg;
> >>+  ImageInfo->LoadOptionsSize = StrLen (NewKernelArg) * sizeof (CHAR16);
> >>+
> >>+  // Before calling the image, enable the Watchdog Timer for  the 5 Minute period
> >>+  gBS->SetWatchdogTimer (5 * 60, 0x10000, 0, NULL);
> >>+  // Start the image
> >>+  Status = gBS->StartImage (ImageHandle, NULL, NULL);
> >>+  // Clear the Watchdog Timer if the image returns
> >>+  gBS->SetWatchdogTimer (0, 0x10000, 0, NULL);
> >>+  return EFI_SUCCESS;
> >>+}

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

[Jun Nie]

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

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

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

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

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

[Leif Lindholm]

On Fri, Jul 28, 2017 at 10:18:49PM +0800, Jun Nie wrote:
> On 2017年07月28日 21:06, Leif Lindholm wrote:
> >On Fri, Jul 28, 2017 at 05:47:46PM +0800, Jun Nie wrote:
> >>On 2017年07月27日 22:09, Leif Lindholm wrote:
> >>>On Thu, Jul 27, 2017 at 06:07:19PM +0800, Jun Nie wrote:
> >>>>Add an android kernel loader that could load kernel from storage
> >>>>device. This patch is from Haojian's code as below link. The minor
> >>>>change is that alternative dtb is searched in second loader binary
> >>>>of Android bootimage if dtb is not found after Linux kernel.
> >>>>https://patches.linaro.org/patch/94683/
> >>>>
> >>>>This android boot image BDS add addtitional cmdline/dtb/ramfs
> >>>>support besides kernel that is introduced by Android boot header.
> >>>
> >>>Getting there. A few more comments below.
> >>>
> >>>>Contributed-under: TianoCore Contribution Agreement 1.0
> >>>>Signed-off-by: Jun Nie <jun.nie@linaro.org>
> >>>>---
> >>>>  ArmPkg/Include/Library/BdsLib.h                    |   3 +
> >>>>  ArmPkg/Library/BdsLib/BdsFilePath.c                |   3 -
> >>>>  .../Application/AndroidBoot/AndroidBootApp.c       | 127 +++++++
> >>>>  .../Application/AndroidBoot/AndroidBootApp.inf     |  64 ++++
> >>>>  .../Application/AndroidFastboot/AndroidBootImg.c   |  35 +-
> >>>>  .../AndroidFastboot/AndroidFastbootApp.h           |   1 +
> >>>>  .../AndroidFastboot/Arm/BootAndroidBootImg.c       |   2 +-
> >>>>  EmbeddedPkg/Include/Library/AndroidBootImgLib.h    |  67 ++++
> >>>>  EmbeddedPkg/Include/Protocol/AndroidBootImg.h      |  47 +++
> >>>>  .../Library/AndroidBootImgLib/AndroidBootImgLib.c  | 419 +++++++++++++++++++++
> >>>>  .../AndroidBootImgLib/AndroidBootImgLib.inf        |  48 +++
> >>>>  11 files changed, 782 insertions(+), 34 deletions(-)
> >>>>  create mode 100644 EmbeddedPkg/Application/AndroidBoot/AndroidBootApp.c
> >>>>  create mode 100644 EmbeddedPkg/Application/AndroidBoot/AndroidBootApp.inf
> >>>>  create mode 100644 EmbeddedPkg/Include/Library/AndroidBootImgLib.h
> >>>>  create mode 100644 EmbeddedPkg/Include/Protocol/AndroidBootImg.h
> >>>>  create mode 100644 EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
> >>>>  create mode 100644 EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.inf
> >>>>
> >

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

No, I think that is a horrible idea, but you are saying that some
platforms are so fundamentally broken as to need a different DT for
every different kernel image.

If this is the case, I can stretch to accepting a mechanism to
override the sane default behaviour of using the existing device tree
provided by the platform.

But the platform registered device tree will still need the same
chosen node updates as the one extracted from boot.img.

> How to add a command-line, in kernel argument that is embedded in
> boot.img? It is a bit strange if so.

The command line argument would be to AndroidBootApp, not the kernel.

> I prefer to override existing registered fdt data directly in current
> Android boot image usage.

The alternative to using the registered device tree by default, with a
mechanism to override, is to always use the registered device tree and
always ignore anything provided in boot.img.

/
    Leif

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

[Jun Nie]

2017-07-28 22:52 GMT+08:00 Leif Lindholm <leif.lindholm@linaro.org>:
> On Fri, Jul 28, 2017 at 10:18:49PM +0800, Jun Nie wrote:
>> On 2017年07月28日 21:06, Leif Lindholm wrote:
>> >On Fri, Jul 28, 2017 at 05:47:46PM +0800, Jun Nie wrote:
>> >>On 2017年07月27日 22:09, Leif Lindholm wrote:
>> >>>On Thu, Jul 27, 2017 at 06:07:19PM +0800, Jun Nie wrote:
>> >>>>Add an android kernel loader that could load kernel from storage
>> >>>>device. This patch is from Haojian's code as below link. The minor
>> >>>>change is that alternative dtb is searched in second loader binary
>> >>>>of Android bootimage if dtb is not found after Linux kernel.
>> >>>>https://patches.linaro.org/patch/94683/
>> >>>>
>> >>>>This android boot image BDS add addtitional cmdline/dtb/ramfs
>> >>>>support besides kernel that is introduced by Android boot header.
>> >>>
>> >>>Getting there. A few more comments below.
>> >>>
>> >>>>Contributed-under: TianoCore Contribution Agreement 1.0
>> >>>>Signed-off-by: Jun Nie <jun.nie@linaro.org>
>> >>>>---
>> >>>>  ArmPkg/Include/Library/BdsLib.h                    |   3 +
>> >>>>  ArmPkg/Library/BdsLib/BdsFilePath.c                |   3 -
>> >>>>  .../Application/AndroidBoot/AndroidBootApp.c       | 127 +++++++
>> >>>>  .../Application/AndroidBoot/AndroidBootApp.inf     |  64 ++++
>> >>>>  .../Application/AndroidFastboot/AndroidBootImg.c   |  35 +-
>> >>>>  .../AndroidFastboot/AndroidFastbootApp.h           |   1 +
>> >>>>  .../AndroidFastboot/Arm/BootAndroidBootImg.c       |   2 +-
>> >>>>  EmbeddedPkg/Include/Library/AndroidBootImgLib.h    |  67 ++++
>> >>>>  EmbeddedPkg/Include/Protocol/AndroidBootImg.h      |  47 +++
>> >>>>  .../Library/AndroidBootImgLib/AndroidBootImgLib.c  | 419 +++++++++++++++++++++
>> >>>>  .../AndroidBootImgLib/AndroidBootImgLib.inf        |  48 +++
>> >>>>  11 files changed, 782 insertions(+), 34 deletions(-)
>> >>>>  create mode 100644 EmbeddedPkg/Application/AndroidBoot/AndroidBootApp.c
>> >>>>  create mode 100644 EmbeddedPkg/Application/AndroidBoot/AndroidBootApp.inf
>> >>>>  create mode 100644 EmbeddedPkg/Include/Library/AndroidBootImgLib.h
>> >>>>  create mode 100644 EmbeddedPkg/Include/Protocol/AndroidBootImg.h
>> >>>>  create mode 100644 EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
>> >>>>  create mode 100644 EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.inf
>> >>>>
>> >
>
>> >>>>diff --git a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
>> >>>>new file mode 100644
>> >>>>index 0000000..72c6322
>> >>>>--- /dev/null
>> >>>>+++ b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
>> >>>>@@ -0,0 +1,419 @@
>> >>>>+/** @file
>> >>>>+
>> >>>>+  Copyright (c) 2013-2014, ARM Ltd. All rights reserved.<BR>
>> >>>>+  Copyright (c) 2017, Linaro. All rights reserved.
>> >>>>+
>> >>>>+  This program and the accompanying materials
>> >>>>+  are licensed and made available under the terms and conditions of the BSD License
>> >>>>+  which accompanies this distribution.  The full text of the license may be found at
>> >>>>+  http://opensource.org/licenses/bsd-license.php
>> >>>>+
>> >>>>+  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
>> >>>>+  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>> >>>>+
>> >>>>+**/
>> >>>>+
>> >>>>+#include <libfdt.h>
>> >>>>+#include <Library/AndroidBootImgLib.h>
>> >>>>+#include <Library/PrintLib.h>
>> >>>>+#include <Library/UefiBootServicesTableLib.h>
>> >>>>+#include <Library/UefiLib.h>
>> >>>>+
>> >>>>+#include <Protocol/AndroidBootImg.h>
>> >>>>+#include <Protocol/LoadedImage.h>
>> >>>>+
>> >>>>+#include <libfdt.h>
>> >>>>+
>> >>>>+#define FDT_ADDITIONAL_ENTRIES_SIZE 0x400
>> >>>>+
>> >>>>+typedef struct {
>> >>>>+  MEMMAP_DEVICE_PATH                      Node1;
>> >>>>+  EFI_DEVICE_PATH_PROTOCOL                End;
>> >>>>+} MEMORY_DEVICE_PATH;
>> >>>>+
>> >>>>+STATIC ABOOTIMG_PROTOCOL                 *mAbootimg;
>> >>>
>> >>>mAndroidBootImg.
>> >>
>> >>Will do.
>> >>>
>> >>>>+
>> >>>>+STATIC CONST MEMORY_DEVICE_PATH MemoryDevicePathTemplate =
>> >>>
>> >>>Should also have an 'm'-prefix.
>> >>
>> >>Will do. But what 'm' prefix stand for?
>> >>>
>> >>>>+{
>> >>>>+  {
>> >>>>+    {
>> >>>>+      HARDWARE_DEVICE_PATH,
>> >>>>+      HW_MEMMAP_DP,
>> >>>>+      {
>> >>>>+        (UINT8)(sizeof (MEMMAP_DEVICE_PATH)),
>> >>>>+        (UINT8)((sizeof (MEMMAP_DEVICE_PATH)) >> 8),
>> >>>>+      },
>> >>>>+    }, // Header
>> >>>>+    0, // StartingAddress (set at runtime)
>> >>>>+    0  // EndingAddress   (set at runtime)
>> >>>>+  }, // Node1
>> >>>>+  {
>> >>>>+    END_DEVICE_PATH_TYPE,
>> >>>>+    END_ENTIRE_DEVICE_PATH_SUBTYPE,
>> >>>>+    { sizeof (EFI_DEVICE_PATH_PROTOCOL), 0 }
>> >>>>+  } // End
>> >>>>+};
>> >>>>+
>> >>>>+EFI_STATUS
>> >>>>+AbootimgGetImgSize (
>> >>>
>> >>>AndroidBootImgGetImageSize.
>> >>
>> >>Will do.
>> >>>
>> >>>>+  IN  VOID    *BootImg,
>> >>>>+  OUT UINTN   *ImgSize
>> >>>>+  )
>> >>>>+{
>> >>>>+  ANDROID_BOOTIMG_HEADER   *Header;
>> >>>>+
>> >>>>+  Header = (ANDROID_BOOTIMG_HEADER *) BootImg;
>> >>>>+
>> >>>>+  if (AsciiStrnCmp ((CONST CHAR8 *)Header->BootMagic, ANDROID_BOOT_MAGIC,
>> >>>>+                    ANDROID_BOOT_MAGIC_LENGTH) != 0) {
>> >>>>+    return EFI_INVALID_PARAMETER;
>> >>>>+  }
>> >>>>+
>> >>>>+  /* The page size is not specified, but it should be power of 2 at least */
>> >>>>+  ASSERT (IS_VALID_ANDROID_PAGE_SIZE (Header->PageSize));
>> >>>>+
>> >>>>+  /* Get real size of abootimg */
>> >>>>+  *ImgSize = ALIGN_VALUE (Header->KernelSize, Header->PageSize) +
>> >>>>+             ALIGN_VALUE (Header->RamdiskSize, Header->PageSize) +
>> >>>>+             ALIGN_VALUE (Header->SecondStageBootloaderSize, Header->PageSize) +
>> >>>>+             Header->PageSize;
>> >>>>+  return EFI_SUCCESS;
>> >>>>+}
>> >>>>+
>> >>>>+EFI_STATUS
>> >>>>+AbootimgGetKernelInfo (
>> >>>
>> >>>AndroidBootImgGetKernelInfo.
>> >>
>> >>Will do.
>> >>>
>> >>>>+  IN  VOID    *BootImg,
>> >>>>+  OUT VOID   **Kernel,
>> >>>>+  OUT UINTN   *KernelSize
>> >>>>+  )
>> >>>>+{
>> >>>>+  ANDROID_BOOTIMG_HEADER   *Header;
>> >>>>+
>> >>>>+  Header = (ANDROID_BOOTIMG_HEADER *) BootImg;
>> >>>>+
>> >>>>+  if (AsciiStrnCmp ((CONST CHAR8 *)Header->BootMagic, ANDROID_BOOT_MAGIC,
>> >>>>+                    ANDROID_BOOT_MAGIC_LENGTH) != 0) {
>> >>>>+    return EFI_INVALID_PARAMETER;
>> >>>>+  }
>> >>>>+
>> >>>>+  if (Header->KernelSize == 0) {
>> >>>>+    return EFI_NOT_FOUND;
>> >>>>+  }
>> >>>>+
>> >>>>+  ASSERT (IS_VALID_ANDROID_PAGE_SIZE (Header->PageSize));
>> >>>>+
>> >>>>+  *KernelSize = Header->KernelSize;
>> >>>>+  *Kernel = BootImg + Header->PageSize;
>> >>>>+  return EFI_SUCCESS;
>> >>>>+}
>> >>>>+
>> >>>>+EFI_STATUS
>> >>>>+AbootimgGetRamdiskInfo (
>> >>>>+  IN  VOID    *BootImg,
>> >>>>+  OUT VOID   **Ramdisk,
>> >>>>+  OUT UINTN   *RamdiskSize
>> >>>>+  )
>> >>>>+{
>> >>>>+  ANDROID_BOOTIMG_HEADER   *Header;
>> >>>>+  UINT8                    *BootImgBytePtr;
>> >>>>+
>> >>>>+  // Cast to UINT8 so we can do pointer arithmetic
>> >>>>+  BootImgBytePtr = (UINT8 *) BootImg;
>> >>>>+
>> >>>>+  Header = (ANDROID_BOOTIMG_HEADER *) BootImg;
>> >>>>+
>> >>>>+  if (AsciiStrnCmp ((CONST CHAR8 *)Header->BootMagic, ANDROID_BOOT_MAGIC,
>> >>>>+                    ANDROID_BOOT_MAGIC_LENGTH) != 0) {
>> >>>>+    return EFI_INVALID_PARAMETER;
>> >>>>+  }
>> >>>>+
>> >>>>+  ASSERT (IS_VALID_ANDROID_PAGE_SIZE (Header->PageSize));
>> >>>>+
>> >>>>+  *RamdiskSize = Header->RamdiskSize;
>> >>>>+
>> >>>>+  if (Header->RamdiskSize != 0) {
>> >>>>+    *Ramdisk = (VOID *) (BootImgBytePtr
>> >>>>+                 + Header->PageSize
>> >>>>+                 + ALIGN_VALUE (Header->KernelSize, Header->PageSize));
>> >>>>+  }
>> >>>>+  return EFI_SUCCESS;
>> >>>>+}
>> >>>>+
>> >>>>+EFI_STATUS
>> >>>>+AbootimgGetSecondBootLoaderInfo (
>> >>>
>> >>>AndroidBootImg...
>> >>
>> >>Will do.
>> >>>
>> >>>>+  IN  VOID    *BootImg,
>> >>>>+  OUT VOID   **Second,
>> >>>>+  OUT UINTN   *SecondSize
>> >>>>+  )
>> >>>>+{
>> >>>>+  ANDROID_BOOTIMG_HEADER   *Header;
>> >>>>+  UINT8                    *BootImgBytePtr;
>> >>>>+
>> >>>>+  // Cast to UINT8 so we can do pointer arithmetic
>> >>>>+  BootImgBytePtr = (UINT8 *) BootImg;
>> >>>>+
>> >>>>+  Header = (ANDROID_BOOTIMG_HEADER *) BootImg;
>> >>>>+
>> >>>>+  if (AsciiStrnCmp ((CONST CHAR8 *)Header->BootMagic, ANDROID_BOOT_MAGIC,
>> >>>>+                    ANDROID_BOOT_MAGIC_LENGTH) != 0) {
>> >>>>+    return EFI_INVALID_PARAMETER;
>> >>>>+  }
>> >>>>+
>> >>>>+  ASSERT (IS_VALID_ANDROID_PAGE_SIZE (Header->PageSize));
>> >>>>+
>> >>>>+  *SecondSize = Header->SecondStageBootloaderSize;
>> >>>>+
>> >>>>+  if (Header->SecondStageBootloaderSize != 0) {
>> >>>>+    *Second = (VOID *) (BootImgBytePtr
>> >>>>+                 + Header->PageSize
>> >>>>+                 + ALIGN_VALUE (Header->KernelSize, Header->PageSize)
>> >>>>+                 + ALIGN_VALUE (Header->RamdiskSize, Header->PageSize));
>> >>>>+  }
>> >>>>+  return EFI_SUCCESS;
>> >>>>+}
>> >>>>+
>> >>>>+EFI_STATUS
>> >>>>+AbootimgGetKernelArgs (
>> >>>
>> >>>AndroidBootImg...
>> >>
>> >>Will do.
>> >>>
>> >>>>+  IN  VOID    *BootImg,
>> >>>>+  OUT CHAR8   *KernelArgs
>> >>>>+  )
>> >>>>+{
>> >>>>+  ANDROID_BOOTIMG_HEADER   *Header;
>> >>>>+
>> >>>>+  Header = (ANDROID_BOOTIMG_HEADER *) BootImg;
>> >>>>+  AsciiStrnCpyS (KernelArgs, ANDROID_BOOTIMG_KERNEL_ARGS_SIZE, Header->KernelArgs,
>> >>>>+    ANDROID_BOOTIMG_KERNEL_ARGS_SIZE);
>> >>>>+
>> >>>>+  return EFI_SUCCESS;
>> >>>>+}
>> >>>>+
>> >>>>+EFI_STATUS
>> >>>>+AbootimgGetFdt (
>> >>>
>> >>>AndroidBootImg...
>> >>
>> >>Will do.
>> >>>
>> >>>>+  IN  VOID                  *BootImg,
>> >>>>+  IN  VOID                  **FdtBase
>> >>>>+  )
>> >>>>+{
>> >>>>+  UINTN                      SecondLoaderSize;
>> >>>>+  EFI_STATUS                 Status;
>> >>>>+
>> >>>>+  /* Check whether FDT is located in second boot loader as some vendor do so,
>> >>>
>> >>>It would be more correct to say "second boot loader region" than
>> >>>"second boot loader".
>> >>>
>> >>>>+   * because second loader is never used as far as I know. */
>> >>>>+  Status = AbootimgGetSecondBootLoaderInfo (
>> >>>>+          BootImg,
>> >>>>+          FdtBase,
>> >>>>+          &SecondLoaderSize
>> >>>>+          );
>> >>>>+  return Status;
>> >>>>+}
>> >>>>+
>> >>>>+EFI_STATUS
>> >>>>+AbootimgUpdateArgsFdt (
>> >>>
>> >>>AndroidBootImgUpdateKernelArgs
>> >>>(The arguments always come through Fdt, so I do not feel that needs to
>> >>>be explicitly pointed out.)
>> >>Command line come from Android boot image cmdline field, not from fdt in
>> >>most cases, if not all. I will split argument update function too.
>> >>>
>> >>>General comment: this function needs to be broken down into several
>> >>>smaller helper functions:
>> >>>- extract kernel arguments from boot.img
>> >>>- extract ramdisk information from boot.img
>> >>>- locate FDT
>> >>>- update FDT
>> >>
>> >>Will do.
>> >>>
>> >>>>+  IN  VOID                  *BootImg,
>> >>>>+  OUT VOID                  *KernelArgs
>> >>>>+  )
>> >>>>+{
>> >>>>+  VOID                      *Ramdisk;
>> >>>
>> >>>RamdiskData?
>> >>Yes, Ramdisk data start address.
>> >>>
>> >>>>+  UINT64                     Ramdisk64, RamdiskEnd64;
>> >>>
>> >>>RamdiskStart, RamDiskEnd?
>> >>
>> >>Yes, will do.
>> >>>
>> >>>>+  UINTN                      RamdiskSize;
>> >>>>+  CHAR8                      ImgKernelArgs[ANDROID_BOOTIMG_KERNEL_ARGS_SIZE];
>> >>>
>> >>>ImageKernelArgs
>> >>>or
>> >>>BootImgKernelArgs
>> >>
>> >>Will do.
>> >>>
>> >>>>+  INTN                       Err, NewFdtSize, chosen_node;
>> >>>
>> >>>ChosenNode
>> >>
>> >>Will do. I had thought fdt library related code shall follow the library
>> >>coding style :)
>> >
>> >Yes, this is a tricky area.
>> >I'm considering putting together a wrapper library for common DT
>> >operations to abstract this away. But for now, I prefer keeping to
>> >TianoCore coding style everywhere except as is needed to call libfdt.
>> >
>> >>>
>> >>>>+  EFI_STATUS                 Status;
>> >>>>+  EFI_PHYSICAL_ADDRESS       FdtBase, UpdatedFdtBase, NewFdtBase;
>> >>>>+  struct fdt_property       *prop;
>> >>>
>> >>>*Property.
>> >>
>> >>Will do.
>> >>>
>> >>>>+  int                        len;
>> >>>
>> >>>INTN Len;
>> >>
>> >>Will do.
>> >>>
>> >>>>+
>> >>>>+  Status = gBS->LocateProtocol (&gAndroidBootImgProtocolGuid, NULL,
>> >>>>+                                (VOID **) &mAbootimg);
>> >>>>+  if (EFI_ERROR (Status)) {
>> >>>>+    return Status;
>> >>>>+  }
>> >>>>+
>> >>>>+  Status = AbootimgGetKernelArgs (BootImg, ImgKernelArgs);
>> >>>>+  if (EFI_ERROR (Status)) {
>> >>>>+    return Status;
>> >>>>+  }
>> >>>>+  // Get kernel arguments from Android boot image
>> >>>>+  AsciiStrToUnicodeStrS (ImgKernelArgs, KernelArgs,
>> >>>>+                         ANDROID_BOOTIMG_KERNEL_ARGS_SIZE >> 1);
>> >>>>+  // Append platform kernel arguments
>> >>>>+  if(mAbootimg->AppendArgs) {
>> >>>>+    Status = mAbootimg->AppendArgs (KernelArgs,
>> >>>>+                                    ANDROID_BOOTIMG_KERNEL_ARGS_SIZE);
>> >>>>+    if (EFI_ERROR (Status)) {
>> >>>>+      return Status;
>> >>>>+    }
>> >>>>+  }
>> >>>>+
>> >>>>+  Status = EfiGetSystemConfigurationTable (&gFdtTableGuid, (VOID **)&FdtBase);
>> >>>>+  if (!EFI_ERROR (Status)) {
>> >>>
>> >>>Should this not be
>> >>>   if (EFI_ERROR (Status) && Status != EFI_NOT_FOUND)
>> >>>?
>> >>
>> >>I mean, if fdt is found, we shall return to avoid installing another fdt.
>> >
>> >An FDT presented to you by firmware is just the hardware description.
>> >Any command line or initrd updates that are required will still need
>> >to happen in order to boot. So the same manipulations that happen to
>> >the DT embedded in boot.img need to happen to one presented via a
>> >configuration table.
>> >
>> >>But actually, I expect fdt is tied with kernel in Android boot image in
>> >>standard Android boot image usage cases.
>> >>Though it is agreed to decouple fdt and kernel in community in 2013,
>> >>Android boot image format has been decided several years before that :) .
>> >>
>> >>We can make change in future if Android boot image usage case changes.
>> >>Maybe I can add a warning message to highlight the new case.
>> >
>> >No.
>> >
>> >We can tolerate booting broken existing images, but we should not
>> >design to intentionally break things ourselves.
>> >
>> >I guess as this is an application, you could even add a command-line
>> >option to let you override an existing registered DT with one embedded
>> >in boot.img.
>> >
>> >But ignoring an existing registered DT is not an option.
>>
>> So you suggest to override existing registered fdt data with the one in
>> boot.img.
>
> No, I think that is a horrible idea, but you are saying that some
> platforms are so fundamentally broken as to need a different DT for
> every different kernel image.
>
> If this is the case, I can stretch to accepting a mechanism to
> override the sane default behaviour of using the existing device tree
> provided by the platform.
>
> But the platform registered device tree will still need the same
> chosen node updates as the one extracted from boot.img.
>
>> How to add a command-line, in kernel argument that is embedded in
>> boot.img? It is a bit strange if so.
>
> The command line argument would be to AndroidBootApp, not the kernel.
>
>> I prefer to override existing registered fdt data directly in current
>> Android boot image usage.
>
> The alternative to using the registered device tree by default, with a
> mechanism to override, is to always use the registered device tree and
> always ignore anything provided in boot.img.

So the solution should be like this. the existing registered device tree will be
used and chosen node will be updated. If no existing registered device tree
is found, the one provided by boot.img will be used. As I do not expect
existing registered device tree case for Android boot usage, fdt in boot.img
will be used in most cases. If exception happens, we can check the detail
situation.
>
> /
>     Leif

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

[Leif Lindholm]

On Sun, Jul 30, 2017 at 10:22:15PM +0800, Jun Nie wrote:
> >> >>>>+  Status = EfiGetSystemConfigurationTable (&gFdtTableGuid, (VOID **)&FdtBase);
> >> >>>>+  if (!EFI_ERROR (Status)) {
> >> >>>
> >> >>>Should this not be
> >> >>>   if (EFI_ERROR (Status) && Status != EFI_NOT_FOUND)
> >> >>>?
> >> >>
> >> >>I mean, if fdt is found, we shall return to avoid installing another fdt.
> >> >
> >> >An FDT presented to you by firmware is just the hardware description.
> >> >Any command line or initrd updates that are required will still need
> >> >to happen in order to boot. So the same manipulations that happen to
> >> >the DT embedded in boot.img need to happen to one presented via a
> >> >configuration table.
> >> >
> >> >>But actually, I expect fdt is tied with kernel in Android boot image in
> >> >>standard Android boot image usage cases.
> >> >>Though it is agreed to decouple fdt and kernel in community in 2013,
> >> >>Android boot image format has been decided several years before that :) .
> >> >>
> >> >>We can make change in future if Android boot image usage case changes.
> >> >>Maybe I can add a warning message to highlight the new case.
> >> >
> >> >No.
> >> >
> >> >We can tolerate booting broken existing images, but we should not
> >> >design to intentionally break things ourselves.
> >> >
> >> >I guess as this is an application, you could even add a command-line
> >> >option to let you override an existing registered DT with one embedded
> >> >in boot.img.
> >> >
> >> >But ignoring an existing registered DT is not an option.
> >>
> >> So you suggest to override existing registered fdt data with the one in
> >> boot.img.
> >
> > No, I think that is a horrible idea, but you are saying that some
> > platforms are so fundamentally broken as to need a different DT for
> > every different kernel image.
> >
> > If this is the case, I can stretch to accepting a mechanism to
> > override the sane default behaviour of using the existing device tree
> > provided by the platform.
> >
> > But the platform registered device tree will still need the same
> > chosen node updates as the one extracted from boot.img.
> >
> >> How to add a command-line, in kernel argument that is embedded in
> >> boot.img? It is a bit strange if so.
> >
> > The command line argument would be to AndroidBootApp, not the kernel.
> >
> >> I prefer to override existing registered fdt data directly in current
> >> Android boot image usage.
> >
> > The alternative to using the registered device tree by default, with a
> > mechanism to override, is to always use the registered device tree and
> > always ignore anything provided in boot.img.
> 
> So the solution should be like this. the existing registered device tree will be
> used and chosen node will be updated. If no existing registered device tree
> is found, the one provided by boot.img will be used. As I do not expect
> existing registered device tree case for Android boot usage, fdt in boot.img
> will be used in most cases. If exception happens, we can check the detail
> situation.

Sure, that works for me.

/
    Leif