On Thu, Jul 06, 2017 at 09:29:05PM +0800, Jun Nie wrote:
Add an android kernel loader that could load kernel from storage device.
UEFI can already load a kernel (with the EFI stub) from a storage device. Please explain in the commit message how this support differs from that.
What relation does this code have to AndroidFastbootApp?
This patch is from Haojian's code.
Could you put a link to the origin of the code (preferably the specific commit)?
The minor change is that alternative dtb is searched in second loader binary of Android bootimage if dtb is not found after Linux kernel.
Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Jun Nie jun.nie@linaro.org
.../Application/AndroidBoot/AndroidBootApp.c | 129 ++++++++ .../Application/AndroidBoot/AndroidBootApp.inf | 64 ++++ EmbeddedPkg/Include/Library/AbootimgLib.h | 65 ++++ EmbeddedPkg/Include/Protocol/Abootimg.h | 47 +++ EmbeddedPkg/Library/AbootimgLib/AbootimgLib.c | 350 +++++++++++++++++++++ EmbeddedPkg/Library/AbootimgLib/AbootimgLib.inf | 48 +++
For proper CamelCase, I think the library name should be AndroidBootImageLib, and the filenames (and macros) updated similarly.
6 files changed, 703 insertions(+) create mode 100644 EmbeddedPkg/Application/AndroidBoot/AndroidBootApp.c create mode 100644 EmbeddedPkg/Application/AndroidBoot/AndroidBootApp.inf create mode 100644 EmbeddedPkg/Include/Library/AbootimgLib.h create mode 100644 EmbeddedPkg/Include/Protocol/Abootimg.h create mode 100644 EmbeddedPkg/Library/AbootimgLib/AbootimgLib.c create mode 100644 EmbeddedPkg/Library/AbootimgLib/AbootimgLib.inf
diff --git a/EmbeddedPkg/Application/AndroidBoot/AndroidBootApp.c b/EmbeddedPkg/Application/AndroidBoot/AndroidBootApp.c new file mode 100644 index 0000000..9ed931b --- /dev/null +++ b/EmbeddedPkg/Application/AndroidBoot/AndroidBootApp.c @@ -0,0 +1,129 @@ +/** @file
- Copyright (c) 2013-2014, ARM Ltd. All rights reserved.<BR>
- Copyright (c) 2017, Linaro. All rights reserved.
- This program and the accompanying materials
- are licensed and made available under the terms and conditions of the BSD License
- which accompanies this distribution. The full text of the license may be found at
- http://opensource.org/licenses/bsd-license.php
- THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
- WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+**/
+#include <Library/AbootimgLib.h> +#include <Library/BaseMemoryLib.h> +#include <Library/BdsLib.h> +#include <Library/DebugLib.h> +#include <Library/DevicePathLib.h> +#include <Library/MemoryAllocationLib.h> +#include <Library/UefiBootServicesTableLib.h>
+#include <Protocol/BlockIo.h> +#include <Protocol/DevicePathFromText.h>
+#define IS_DEVICE_PATH_NODE(node,type,subtype) (((node)->Type == (type)) && ((node)->SubType == (subtype)))
Would prefer for this to be moved into a common header file (BdsLib.h ?) rather than copied into multiple .c files.
+#define ALIGN(x, a) (((x) + ((a) - 1)) & ~((a) - 1))
Rather use one of the ALIGN_ macros from Base.h.
+EFI_STATUS +EFIAPI +AndroidBootAppEntryPoint (
- IN EFI_HANDLE ImageHandle,
- IN EFI_SYSTEM_TABLE *SystemTable
- )
+{
- EFI_STATUS Status;
- CHAR16 *BootPathStr;
- EFI_DEVICE_PATH_FROM_TEXT_PROTOCOL *EfiDevicePathFromTextProtocol;
- EFI_DEVICE_PATH *DevicePath;
- EFI_DEVICE_PATH_PROTOCOL *Node, *NextNode;
- EFI_BLOCK_IO_PROTOCOL *BlockIo;
- UINT32 MediaId, BlockSize;
- VOID *Buffer;
- EFI_HANDLE Handle;
- UINTN Size;
- BootPathStr = (CHAR16 *)PcdGetPtr (PcdAndroidBootDevicePath);
- ASSERT (BootPathStr != NULL);
- Status = gBS->LocateProtocol (&gEfiDevicePathFromTextProtocolGuid, NULL, (VOID **)&EfiDevicePathFromTextProtocol);
- ASSERT_EFI_ERROR(Status);
- DevicePath = (EFI_DEVICE_PATH *)EfiDevicePathFromTextProtocol->ConvertTextToDevicePath (BootPathStr);
- ASSERT (DevicePath != NULL);
- /* Find DevicePath node of Partition */
- NextNode = DevicePath;
- while (1) {
- Node = NextNode;
- if (IS_DEVICE_PATH_NODE (Node, MEDIA_DEVICE_PATH, MEDIA_HARDDRIVE_DP)) {
break;- }
- NextNode = NextDevicePathNode (Node);
- }
- Status = gBS->LocateDevicePath (&gEfiDevicePathProtocolGuid, &DevicePath, &Handle);
- if (EFI_ERROR (Status)) {
- return Status;
- }
- Status = gBS->OpenProtocol (
Handle,&gEfiBlockIoProtocolGuid,(VOID **) &BlockIo,gImageHandle,NULL,EFI_OPEN_PROTOCOL_GET_PROTOCOL);- if (EFI_ERROR (Status)) {
- DEBUG ((EFI_D_ERROR, "Failed to get BlockIo: %r\n", Status));
- return Status;
- }
- MediaId = BlockIo->Media->MediaId;
- BlockSize = BlockIo->Media->BlockSize;
- Buffer = AllocatePages (1);
I dislike magic numbers even where the code is reasonably clear. Could this be EFI_SIZE_TO_PAGES (ANDROID_BOOTIMG_HEADER) instead of "1"?
- if (Buffer == NULL) {
- return EFI_BUFFER_TOO_SMALL;
- }
- /* Load header of boot.img */
- Status = BlockIo->ReadBlocks (
BlockIo,MediaId,0,BlockSize,Buffer);- Status = AbootimgGetImgSize (Buffer, &Size);
- if (EFI_ERROR (Status)) {
- DEBUG ((EFI_D_ERROR, "Failed to get Abootimg Size: %r\n", Status));
- return Status;
- }
- Size = ALIGN (Size, BlockSize);
Although this is not an align operation, but rather a "round up" operation. We have the NET_ROUNDUP macro already, but perhaps a new generic one (and then redefine the NET_ROUNDUP value to point to this)?
- FreePages (Buffer, 1);
- /* Both PartitionStart and PartitionSize are counted as block size. */
- Buffer = AllocatePages (EFI_SIZE_TO_PAGES (Size));
- if (Buffer == NULL) {
- return EFI_BUFFER_TOO_SMALL;
- }
- /* Load header of boot.img */
- Status = BlockIo->ReadBlocks (
BlockIo,MediaId,0,Size,Buffer);- if (EFI_ERROR (Status)) {
- DEBUG ((EFI_D_ERROR, "Failed to read blocks: %r\n", Status));
- goto EXIT;
- }
- Status = AbootimgBoot (Buffer, Size);
+EXIT:
- return Status;
+} diff --git a/EmbeddedPkg/Application/AndroidBoot/AndroidBootApp.inf b/EmbeddedPkg/Application/AndroidBoot/AndroidBootApp.inf new file mode 100644 index 0000000..8f6c8186 --- /dev/null +++ b/EmbeddedPkg/Application/AndroidBoot/AndroidBootApp.inf @@ -0,0 +1,64 @@ +#/** @file +# +# Copyright (c) 2013-2015, ARM Ltd. All rights reserved.<BR> +# Copyright (c) 2017, Linaro. All rights reserved. +# +# This program and the accompanying materials +# are licensed and made available under the terms and conditions of the BSD License +# which accompanies this distribution. The full text of the license may be found at +# http://opensource.org/licenses/bsd-license.php +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, +# WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. +# +# +#**/
+[Defines]
- INF_VERSION = 0x00010019
- BASE_NAME = AndroidBootApp
- FILE_GUID = 3a738b36-b9c5-4763-abbd-6cbd4b25f9ff
- MODULE_TYPE = UEFI_APPLICATION
- VERSION_STRING = 1.0
- ENTRY_POINT = AndroidBootAppEntryPoint
+[Sources.common]
- AndroidBootApp.c
+[LibraryClasses]
- AbootimgLib
- BaseLib
- BaseMemoryLib
- BdsLib
- DebugLib
- DevicePathLib
- DxeServicesTableLib
- FdtLib
- MemoryAllocationLib
- PcdLib
- PrintLib
- UefiApplicationEntryPoint
- UefiBootServicesTableLib
- UefiLib
- UefiRuntimeServicesTableLib
+[Protocols]
- gAndroidFastbootPlatformProtocolGuid
- gEfiBlockIoProtocolGuid
- gEfiDevicePathFromTextProtocolGuid
- gEfiSimpleTextOutProtocolGuid
- gEfiSimpleTextInProtocolGuid
+[Packages]
- EmbeddedPkg/EmbeddedPkg.dec
- MdeModulePkg/MdeModulePkg.dec
- MdePkg/MdePkg.dec
+[Packages.ARM, Packages.AARCH64]
- ArmPkg/ArmPkg.dec
- ArmPlatformPkg/ArmPlatformPkg.dec
+[Guids]
- gFdtTableGuid
+[Pcd]
- gEmbeddedTokenSpaceGuid.PcdAndroidBootDevicePath
diff --git a/EmbeddedPkg/Include/Library/AbootimgLib.h b/EmbeddedPkg/Include/Library/AbootimgLib.h new file mode 100644 index 0000000..c0372d4 --- /dev/null +++ b/EmbeddedPkg/Include/Library/AbootimgLib.h @@ -0,0 +1,65 @@ +/** @file
- Copyright (c) 2013-2014, ARM Ltd. All rights reserved.<BR>
- Copyright (c) 2017, Linaro.
- This program and the accompanying materials
- are licensed and made available under the terms and conditions of the BSD License
- which accompanies this distribution. The full text of the license may be found at
- http://opensource.org/licenses/bsd-license.php
- THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
- WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+**/
+#ifndef __ABOOTIMG_H__ +#define __ABOOTIMG_H__
+#include <Library/BaseLib.h> +#include <Library/DebugLib.h> +#include <Library/MemoryAllocationLib.h>
+#include <Uefi/UefiBaseType.h> +#include <Uefi/UefiSpec.h>
+#define BOOTIMG_KERNEL_ARGS_SIZE 512
Is this value defined somewhere? Or is it just an arbitrary size?
+#define BOOT_MAGIC "ANDROID!" +#define BOOT_MAGIC_LENGTH (sizeof (BOOT_MAGIC) - 1)
+/* It's the value of arm64 efi stub kernel */ +#define KERNEL_IMAGE_STEXT_OFFSET 0x12C +#define KERNEL_IMAGE_RAW_SIZE_OFFSET 0x130
+#define FDT_SIZE_OFFSET 0x4
What are these offsets?
All of these names (BOOT/KERNEL/FDT) are a little bit to generic for an exported header file. Need ANDROID_BOOT_ (or similar_) prefix.
+typedef struct {
- CHAR8 BootMagic[BOOT_MAGIC_LENGTH];
- UINT32 KernelSize;
- UINT32 KernelAddress;
- UINT32 RamdiskSize;
- UINT32 RamdiskAddress;
- UINT32 SecondStageBootloaderSize;
- UINT32 SecondStageBootloaderAddress;
- UINT32 KernelTaggsAddress;
- UINT32 PageSize;
- UINT32 Reserved[2];
- CHAR8 ProductName[16];
- CHAR8 KernelArgs[BOOTIMG_KERNEL_ARGS_SIZE];
Does the protocol not specify signedness of chars for strings?
- UINT32 Id[32];
+} ANDROID_BOOTIMG_HEADER;
This looks identical to the definition in Application/AndroidFastboot/AndroidBootImg.c, only missing the #pragma pack(1) statement.
In general there looks like a lot of code duplication between these two applications. Can this not be broken out into a common library used by both?
+EFI_STATUS +AbootimgGetImgSize (
- IN VOID *BootImg,
- OUT UINTN *ImgSize
- );
+EFI_STATUS +AbootimgBoot (
- IN VOID *Buffer,
- IN UINTN BufferSize
- );
+#endif /* __ABOOTIMG_H__ */ diff --git a/EmbeddedPkg/Include/Protocol/Abootimg.h b/EmbeddedPkg/Include/Protocol/Abootimg.h new file mode 100644 index 0000000..c85dad2 --- /dev/null +++ b/EmbeddedPkg/Include/Protocol/Abootimg.h @@ -0,0 +1,47 @@ +/** @file
- Copyright (c) 2017, Linaro. All rights reserved.<BR>
- This program and the accompanying materials
- are licensed and made available under the terms and conditions of the BSD License
- which accompanies this distribution. The full text of the license may be found at
- http://opensource.org/licenses/bsd-license.php
- THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
- WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+**/
+#ifndef __ABOOTIMG_PROTOCOL_H__ +#define __ABOOTIMG_PROTOCOL_H__
+// +// Protocol interface structure +// +typedef struct _ABOOTIMG_PROTOCOL ABOOTIMG_PROTOCOL;
+// +// Function Prototypes +// +typedef +EFI_STATUS +(EFIAPI *ABOOTIMG_APPEND_KERNEL_ARGS) (
- IN CHAR16 *Args,
- IN UINTN Size
- );
+typedef +EFI_STATUS +(EFIAPI *ABOOTIMG_UPDATE_DTB) (
- IN EFI_PHYSICAL_ADDRESS OrigDtbBase;
- OUT EFI_PHYSICAL_ADDRESS *NewDtbBase;
- );
+struct _ABOOTIMG_PROTOCOL {
- ABOOTIMG_APPEND_KERNEL_ARGS AppendArgs;
- ABOOTIMG_UPDATE_DTB UpdateDtb;
+};
+extern EFI_GUID gAbootimgProtocolGuid;
+#endif /* __ABOOTIMG_PROTOCOL_H__ */ diff --git a/EmbeddedPkg/Library/AbootimgLib/AbootimgLib.c b/EmbeddedPkg/Library/AbootimgLib/AbootimgLib.c new file mode 100644 index 0000000..ea30a01 --- /dev/null +++ b/EmbeddedPkg/Library/AbootimgLib/AbootimgLib.c @@ -0,0 +1,350 @@ +/** @file
- Copyright (c) 2013-2014, ARM Ltd. All rights reserved.<BR>
- Copyright (c) 2017, Linaro. All rights reserved.
- This program and the accompanying materials
- are licensed and made available under the terms and conditions of the BSD License
- which accompanies this distribution. The full text of the license may be found at
- http://opensource.org/licenses/bsd-license.php
- THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
- WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+**/
+#include <Library/AbootimgLib.h> +#include <Library/PrintLib.h> +#include <Library/UefiBootServicesTableLib.h> +#include <Library/UefiLib.h>
+#include <Protocol/Abootimg.h> +#include <Protocol/LoadedImage.h>
+#include <libfdt.h>
+// Check Val (unsigned) is a power of 2 (has only one bit set) +#define IS_POWER_OF_2(Val) (Val != 0 && ((Val & (Val - 1)) == 0))
Missing parentheses in macro.
+typedef struct {
- MEMMAP_DEVICE_PATH Node1;
- EFI_DEVICE_PATH_PROTOCOL End;
+} MEMORY_DEVICE_PATH;
+STATIC ABOOTIMG_PROTOCOL *mAbootimg;
+STATIC CONST MEMORY_DEVICE_PATH MemoryDevicePathTemplate = +{
- {
- {
HARDWARE_DEVICE_PATH,HW_MEMMAP_DP,{(UINT8)(sizeof (MEMMAP_DEVICE_PATH)),(UINT8)((sizeof (MEMMAP_DEVICE_PATH)) >> 8),},- }, // Header
- 0, // StartingAddress (set at runtime)
- 0 // EndingAddress (set at runtime)
- }, // Node1
- {
- END_DEVICE_PATH_TYPE,
- END_ENTIRE_DEVICE_PATH_SUBTYPE,
- { sizeof (EFI_DEVICE_PATH_PROTOCOL), 0 }
- } // End
+};
+EFI_STATUS +AbootimgGetImgSize (
- IN VOID *BootImg,
- OUT UINTN *ImgSize
- )
+{
- ANDROID_BOOTIMG_HEADER *Header;
- Header = (ANDROID_BOOTIMG_HEADER *) BootImg;
- if (AsciiStrnCmp (Header->BootMagic, BOOT_MAGIC, BOOT_MAGIC_LENGTH) != 0) {
- return EFI_INVALID_PARAMETER;
- }
- ASSERT (IS_POWER_OF_2 (Header->PageSize));
I am not convinced this is the right thing to check for. Not every power of two is a valid page size. Semantically, any macro here should really be called IS_VALID_PAGE_SIZE().
- /* Get real size of abootimg */
- *ImgSize = ALIGN_VALUE (Header->KernelSize, Header->PageSize) +
ALIGN_VALUE (Header->RamdiskSize, Header->PageSize) +ALIGN_VALUE (Header->SecondStageBootloaderSize, Header->PageSize) +Header->PageSize;- return EFI_SUCCESS;
+}
+EFI_STATUS +AbootimgGetKernelInfo (
- IN VOID *BootImg,
- OUT VOID **Kernel,
- OUT UINTN *KernelSize
- )
+{
- ANDROID_BOOTIMG_HEADER *Header;
- Header = (ANDROID_BOOTIMG_HEADER *) BootImg;
- if (AsciiStrnCmp (Header->BootMagic, BOOT_MAGIC, BOOT_MAGIC_LENGTH) != 0) {
- return EFI_INVALID_PARAMETER;
- }
- if (Header->KernelSize == 0) {
- return EFI_NOT_FOUND;
- }
- ASSERT (IS_POWER_OF_2 (Header->PageSize));
- *KernelSize = Header->KernelSize;
- *Kernel = BootImg + Header->PageSize;
- return EFI_SUCCESS;
+}
+EFI_STATUS +AbootimgGetRamdiskInfo (
- IN VOID *BootImg,
- OUT VOID **Ramdisk,
- OUT UINTN *RamdiskSize
- )
+{
- ANDROID_BOOTIMG_HEADER *Header;
- UINT8 *BootImgBytePtr;
- // Cast to UINT8 so we can do pointer arithmetic
- BootImgBytePtr = (UINT8 *) BootImg;
- Header = (ANDROID_BOOTIMG_HEADER *) BootImg;
- if (AsciiStrnCmp (Header->BootMagic, BOOT_MAGIC, BOOT_MAGIC_LENGTH) != 0) {
- return EFI_INVALID_PARAMETER;
- }
- ASSERT (IS_POWER_OF_2 (Header->PageSize));
- *RamdiskSize = Header->RamdiskSize;
- if (Header->RamdiskSize != 0) {
- *Ramdisk = (VOID *) (BootImgBytePtr
+ Header->PageSize+ ALIGN_VALUE (Header->KernelSize, Header->PageSize));- }
- return EFI_SUCCESS;
+}
+EFI_STATUS +AbootimgGetSecondBootLoaderInfo (
- IN VOID *BootImg,
- OUT VOID **Second,
- OUT UINTN *SecondSize
- )
+{
- ANDROID_BOOTIMG_HEADER *Header;
- UINT8 *BootImgBytePtr;
- // Cast to UINT8 so we can do pointer arithmetic
- BootImgBytePtr = (UINT8 *) BootImg;
- Header = (ANDROID_BOOTIMG_HEADER *) BootImg;
- if (AsciiStrnCmp (Header->BootMagic, BOOT_MAGIC, BOOT_MAGIC_LENGTH) != 0) {
- return EFI_INVALID_PARAMETER;
- }
- ASSERT (IS_POWER_OF_2 (Header->PageSize));
- *SecondSize = Header->SecondStageBootloaderSize;
- if (Header->SecondStageBootloaderSize != 0) {
- *Second = (VOID *) (BootImgBytePtr
+ Header->PageSize+ ALIGN_VALUE (Header->KernelSize, Header->PageSize)+ ALIGN_VALUE (Header->RamdiskSize, Header->PageSize));- }
- return EFI_SUCCESS;
+}
+EFI_STATUS +AbootimgGetKernelArgs (
- IN VOID *BootImg,
- OUT CHAR8 *KernelArgs
- )
+{
- ANDROID_BOOTIMG_HEADER *Header;
- Header = (ANDROID_BOOTIMG_HEADER *) BootImg;
- AsciiStrnCpyS (KernelArgs, BOOTIMG_KERNEL_ARGS_SIZE, Header->KernelArgs,
- BOOTIMG_KERNEL_ARGS_SIZE);
- return EFI_SUCCESS;
+}
+EFI_STATUS +AbootimgInstallFdt (
- IN VOID *BootImg,
- IN EFI_PHYSICAL_ADDRESS FdtBase,
- OUT VOID *KernelArgs
- )
+{
- VOID *Ramdisk;
- UINTN RamdiskSize;
- CHAR8 ImgKernelArgs[BOOTIMG_KERNEL_ARGS_SIZE];
- INTN err;
- EFI_STATUS Status;
- EFI_PHYSICAL_ADDRESS NewFdtBase;
- Status = gBS->LocateProtocol (&gAbootimgProtocolGuid, NULL, (VOID **) &mAbootimg);
- if (EFI_ERROR (Status)) {
- return Status;
- }
- if (EFI_ERROR (Status)) {
- return Status;
- }
- Status = AbootimgGetRamdiskInfo (
BootImg,&Ramdisk,&RamdiskSize);- if (EFI_ERROR (Status)) {
- return Status;
- }
- Status = AbootimgGetKernelArgs (
BootImg,ImgKernelArgs);- if (EFI_ERROR (Status)) {
- return Status;
- }
- // Get kernel arguments from Android boot image
- AsciiStrToUnicodeStrS (ImgKernelArgs, KernelArgs, BOOTIMG_KERNEL_ARGS_SIZE >> 1);
- // Set the ramdisk in command line arguments
- UnicodeSPrint (
- (CHAR16 *)KernelArgs + StrLen (KernelArgs), BOOTIMG_KERNEL_ARGS_SIZE,
- L" initrd=0x%x,0x%x",
- (UINTN)Ramdisk, (UINTN)RamdiskSize
- );
This is not an appropriate way to set ramdisk. There are dedicated "linux,initrd-start" and "linux,initrd-end" options in the chosen node. See http://git.savannah.gnu.org/cgit/grub.git/tree/grub-core/loader/arm64/linux.... for an example.
- // Append platform kernel arguments
- Status = mAbootimg->AppendArgs (KernelArgs, BOOTIMG_KERNEL_ARGS_SIZE);
Am I missing something? Where is this AppendArgs function defined?
- if (EFI_ERROR (Status)) {
- return Status;
- }
- Status = mAbootimg->UpdateDtb (FdtBase, &NewFdtBase);
Likewise, where is this function defined?
- if (EFI_ERROR (Status)) {
- return Status;
- }
- //
- // Sanity checks on the new FDT blob.
- //
- err = fdt_check_header ((VOID*)(UINTN)NewFdtBase);
- if (err != 0) {
- Print (L"ERROR: Device Tree header not valid (err:%d)\n", err);
- return EFI_INVALID_PARAMETER;
- }
- Status = gBS->InstallConfigurationTable (
&gFdtTableGuid,(VOID *)(UINTN)NewFdtBase);- return Status;
+}
+EFI_STATUS +AbootimgBoot (
- IN VOID *Buffer,
- IN UINTN BufferSize
- )
+{
- EFI_STATUS Status;
- VOID *Kernel;
- UINTN KernelSize;
- VOID *SecondLoader;
- UINTN SecondLoaderSize;
- MEMORY_DEVICE_PATH KernelDevicePath;
- EFI_HANDLE ImageHandle;
- EFI_PHYSICAL_ADDRESS FdtBase;
- VOID *NewKernelArg;
- EFI_LOADED_IMAGE_PROTOCOL *ImageInfo;
- INTN Err;
- Status = AbootimgGetKernelInfo (
Buffer,&Kernel,&KernelSize);- if (EFI_ERROR (Status)) {
- return Status;
- }
- /* For flatten image, Fdt is attached at the end of kernel.
Get real kernel size.- */
This is a completely broken usage model :( Is this a requirement for this support?
I mean, the solution you have implemented is a nice way of dealing with the mess, but per-kernel-image DT blobs were deprecated at kernel summit 2013.
- KernelSize = *(UINT32 *)((EFI_PHYSICAL_ADDRESS)(UINTN)Kernel + KERNEL_IMAGE_STEXT_OFFSET) +
*(UINT32 *)((EFI_PHYSICAL_ADDRESS)(UINTN)Kernel + KERNEL_IMAGE_RAW_SIZE_OFFSET);- NewKernelArg = AllocateZeroPool (BOOTIMG_KERNEL_ARGS_SIZE);
- if (NewKernelArg == NULL) {
- DEBUG ((DEBUG_ERROR, "Fail to allocate memory\n"));
- return EFI_OUT_OF_RESOURCES;
- }
- /* FDT is at the end of kernel image */
- FdtBase = (EFI_PHYSICAL_ADDRESS)(UINTN)Kernel + KernelSize;
- //
- // Sanity checks on the original FDT blob.
- //
- Err = fdt_check_header ((VOID*)(UINTN)FdtBase);
- if (Err != 0) {
- /* Check whether FDT is located in second boot loader */
What is a second boot loader?
- Status = AbootimgGetSecondBootLoaderInfo (
Buffer,&SecondLoader,&SecondLoaderSize);- if (EFI_ERROR (Status)) {
return Status;- }
- Err = fdt_check_header ((VOID*)(UINTN)SecondLoader);
- if (Err != 0) {
DEBUG ((DEBUG_ERROR, "ERROR: Device Tree header not valid (Err:%d)\n", Err));return EFI_INVALID_PARAMETER;- }
- FdtBase = (EFI_PHYSICAL_ADDRESS)SecondLoader;
- }
The above two scenarios could do with broken out as separate helper functions. Preferably also with a Pcd to determine whether the application should even bother to go looking if a platform-provided device tree was already installed.
- Status = AbootimgInstallFdt (Buffer, FdtBase, NewKernelArg);
- if (EFI_ERROR (Status)) {
- FreePool (NewKernelArg);
- return EFI_INVALID_PARAMETER;
- }
- KernelDevicePath = MemoryDevicePathTemplate;
- // Have to cast to UINTN before casting to EFI_PHYSICAL_ADDRESS in order to
- // appease GCC.
(Redundant comment. It's a common enough problem.)
- KernelDevicePath.Node1.StartingAddress = (EFI_PHYSICAL_ADDRESS)(UINTN) Kernel;
- KernelDevicePath.Node1.EndingAddress = (EFI_PHYSICAL_ADDRESS)(UINTN) Kernel + KernelSize;
- Status = gBS->LoadImage (TRUE, gImageHandle, (EFI_DEVICE_PATH *)&KernelDevicePath, (VOID*)(UINTN)Kernel, KernelSize, &ImageHandle);
Please split function call up over multiple lines.
- // Set kernel arguments
- Status = gBS->HandleProtocol (ImageHandle, &gEfiLoadedImageProtocolGuid, (VOID **) &ImageInfo);
Please split function call up over multiple lines.
- ImageInfo->LoadOptions = NewKernelArg;
- ImageInfo->LoadOptionsSize = StrLen (NewKernelArg) * sizeof (CHAR16);
- // Before calling the image, enable the Watchdog Timer for the 5 Minute period
Please wrap comment at 80 characters. (Or delete the "for the 5 Minute period bit based on the next comment.)
- gBS->SetWatchdogTimer (5 * 60, 0x0000, 0x00, NULL);
Please create a define for that timeout. ANDROID_BOOT_WATCHDOG_TIMEOUT or something. Or, hmm, given that the UEFI specification (2.7) says "The watchdog must be set to a period of 5 minutes", this may even be worth putting into MdePkg/MdeModulePkg under a generic name.
The UEFI specification says "The firmware reserves codes 0x0000 to 0xFFFF. Loaders and operating systems may use other timeout codes.". So I think this code needs to set this to something else.
You don't need to say 0x00 when you mean 0.
- // Start the image
- Status = gBS->StartImage (ImageHandle, NULL, NULL);
- // Clear the Watchdog Timer after the image returns
_if_ the image returns, surely?
- gBS->SetWatchdogTimer (0x0000, 0x0000, 0x0000, NULL);
Don't say 0x* when you mean 0.
- return EFI_SUCCESS;
+} diff --git a/EmbeddedPkg/Library/AbootimgLib/AbootimgLib.inf b/EmbeddedPkg/Library/AbootimgLib/AbootimgLib.inf new file mode 100644 index 0000000..461dcb8 --- /dev/null +++ b/EmbeddedPkg/Library/AbootimgLib/AbootimgLib.inf @@ -0,0 +1,48 @@ +#/** @file +# +# Copyright (c) 2013-2015, ARM Ltd. All rights reserved.<BR> +# Copyright (c) 2017, Linaro. All rights reserved. +# +# This program and the accompanying materials +# are licensed and made available under the terms and conditions of the BSD License +# which accompanies this distribution. The full text of the license may be found at +# http://opensource.org/licenses/bsd-license.php +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, +# WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. +# +# +#**/
+[Defines]
- INF_VERSION = 0x00010019
- BASE_NAME = AbootimgLib
- FILE_GUID = ed3b8739-6fa7-4cb1-8aeb-2496f8fcaefa
- MODULE_TYPE = BASE
- VERSION_STRING = 1.0
- LIBRARY_CLASS = AbootimgLib
+# +# The following information is for reference only and not required by the build tools. +# +# VALID_ARCHITECTURES = ARM AARCH64 +#
+[Sources]
- AbootimgLib.c
+[LibraryClasses]
- DebugLib
- FdtLib
- PrintLib
- UefiBootServicesTableLib
- UefiLib
+[Packages]
- MdePkg/MdePkg.dec
- EmbeddedPkg/EmbeddedPkg.dec
Please sort alphabetically.
/ Leif
+[Protocols]
- gAbootimgProtocolGuid
+[Guids]
- gFdtTableGuid
-- 1.9.1