From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr0-x22d.google.com (mail-wr0-x22d.google.com [IPv6:2a00:1450:400c:c0c::22d]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id C3F882095D8D3 for ; Tue, 18 Jul 2017 09:02:31 -0700 (PDT) Received: by mail-wr0-x22d.google.com with SMTP id v105so5506177wrb.0 for ; Tue, 18 Jul 2017 09:04:25 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=Qxvl68Yk3qM6fFMk5Qx5//cQG6EaZBr8YiLOCXgbP6Q=; b=OCSfA+Ftcvo8Wqv/Ys1NdwApRc6tEI2Y3hszB/JgcwwnRynQTV4L0eU5rvzoNiqkNh 307vZvV65WKptHLHOkHOAZyxDGTWRRU4eW0k4NT8l5VBVLi2fwI/jn6HxPb0Of4l7ZQs YVKzoeuA/5VwtP4X4OwbRu5FPHBdOD1tvXqNQ= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=Qxvl68Yk3qM6fFMk5Qx5//cQG6EaZBr8YiLOCXgbP6Q=; b=Mj+hdp4pqU+YtuuXJUt5qGV4JT/mcg+llFjGbTrtZZoCHr8WGvi1j3hZv2eJfqYEIP 3e6Gzs7tnqKi3GM69xr/WmyYWsU+j6FMGF7Ccka8o0lphJdYBQ51SJRPGbdnbRZhyeoy 9e6B9NSFLmBCHzkk3TLO+sTuqt3I6GTkcrW98EhpL/0CyrXcDNPwcrwm4h9R84bQEctF +X9x9TcR250ziMoKB00Uw3Bpe6f1KnUsVnKP30NrcX+JndUuSKoEy36UhcajfUEYyahM XIsfiGoh72qgnPdj7Hw/pTvELxDAxDBOJ94uqxlQ8OyBcqEQTnx2Nr0h5EWkmVQM+AGZ 5DSA== X-Gm-Message-State: AIVw112CpHr13yUqMRAMtKc9bGGOp4G90aHtKdNbOYvDsjVNfAcgs9m/ +gF0+jgQPAAlKsh/ X-Received: by 10.223.144.106 with SMTP id h97mr1504894wrh.93.1500393863592; Tue, 18 Jul 2017 09:04:23 -0700 (PDT) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id j76sm4490597wmg.14.2017.07.18.09.04.22 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 18 Jul 2017 09:04:22 -0700 (PDT) Date: Tue, 18 Jul 2017 17:04:20 +0100 From: Leif Lindholm To: Jun Nie Cc: haojian.zhuang@linaro.org, ard.biesheuvel@linaro.org, edk2-devel@lists.01.org, linaro-uefi@lists.linaro.org, shawn.guo@linaro.org, jason.liu@linaro.org Message-ID: <20170718160420.GR26676@bivouac.eciton.net> References: <1499347746-16786-1-git-send-email-jun.nie@linaro.org> MIME-Version: 1.0 In-Reply-To: <1499347746-16786-1-git-send-email-jun.nie@linaro.org> User-Agent: Mutt/1.5.23 (2014-03-12) Subject: Re: [PATCH v2 1/2] EmbeddedPkg/AndroidBoot: boot android kernel from storage X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 18 Jul 2017 16:02:32 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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 > --- > .../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.
> + 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 > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > +#include > + > +#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.
> +# 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.
> + 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 > +#include > +#include > + > +#include > +#include > + > +#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.
> + > + 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.
> + 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 > +#include > +#include > +#include > + > +#include > +#include > + > +#include > + > +// Check Val (unsigned) is a power of 2 (has only one bit set) > +#define IS_POWER_OF_2(Val) (Val != 0 && ((Val & (Val - 1)) == 0)) Missing parentheses in macro. > + > +typedef struct { > + MEMMAP_DEVICE_PATH Node1; > + EFI_DEVICE_PATH_PROTOCOL End; > +} MEMORY_DEVICE_PATH; > + > +STATIC ABOOTIMG_PROTOCOL *mAbootimg; > + > +STATIC CONST MEMORY_DEVICE_PATH MemoryDevicePathTemplate = > +{ > + { > + { > + HARDWARE_DEVICE_PATH, > + HW_MEMMAP_DP, > + { > + (UINT8)(sizeof (MEMMAP_DEVICE_PATH)), > + (UINT8)((sizeof (MEMMAP_DEVICE_PATH)) >> 8), > + }, > + }, // Header > + 0, // StartingAddress (set at runtime) > + 0 // EndingAddress (set at runtime) > + }, // Node1 > + { > + END_DEVICE_PATH_TYPE, > + END_ENTIRE_DEVICE_PATH_SUBTYPE, > + { sizeof (EFI_DEVICE_PATH_PROTOCOL), 0 } > + } // End > +}; > + > +EFI_STATUS > +AbootimgGetImgSize ( > + IN VOID *BootImg, > + OUT UINTN *ImgSize > + ) > +{ > + ANDROID_BOOTIMG_HEADER *Header; > + > + Header = (ANDROID_BOOTIMG_HEADER *) BootImg; > + > + if (AsciiStrnCmp (Header->BootMagic, BOOT_MAGIC, BOOT_MAGIC_LENGTH) != 0) { > + return EFI_INVALID_PARAMETER; > + } > + > + ASSERT (IS_POWER_OF_2 (Header->PageSize)); I am not convinced this is the right thing to check for. Not every power of two is a valid page size. Semantically, any macro here should really be called IS_VALID_PAGE_SIZE(). > + > + /* Get real size of abootimg */ > + *ImgSize = ALIGN_VALUE (Header->KernelSize, Header->PageSize) + > + ALIGN_VALUE (Header->RamdiskSize, Header->PageSize) + > + ALIGN_VALUE (Header->SecondStageBootloaderSize, Header->PageSize) + > + Header->PageSize; > + return EFI_SUCCESS; > +} > + > +EFI_STATUS > +AbootimgGetKernelInfo ( > + IN VOID *BootImg, > + OUT VOID **Kernel, > + OUT UINTN *KernelSize > + ) > +{ > + ANDROID_BOOTIMG_HEADER *Header; > + > + Header = (ANDROID_BOOTIMG_HEADER *) BootImg; > + > + if (AsciiStrnCmp (Header->BootMagic, BOOT_MAGIC, BOOT_MAGIC_LENGTH) != 0) { > + return EFI_INVALID_PARAMETER; > + } > + > + if (Header->KernelSize == 0) { > + return EFI_NOT_FOUND; > + } > + > + ASSERT (IS_POWER_OF_2 (Header->PageSize)); > + > + *KernelSize = Header->KernelSize; > + *Kernel = BootImg + Header->PageSize; > + return EFI_SUCCESS; > +} > + > +EFI_STATUS > +AbootimgGetRamdiskInfo ( > + IN VOID *BootImg, > + OUT VOID **Ramdisk, > + OUT UINTN *RamdiskSize > + ) > +{ > + ANDROID_BOOTIMG_HEADER *Header; > + UINT8 *BootImgBytePtr; > + > + // Cast to UINT8 so we can do pointer arithmetic > + BootImgBytePtr = (UINT8 *) BootImg; > + > + Header = (ANDROID_BOOTIMG_HEADER *) BootImg; > + > + if (AsciiStrnCmp (Header->BootMagic, BOOT_MAGIC, BOOT_MAGIC_LENGTH) != 0) { > + return EFI_INVALID_PARAMETER; > + } > + > + ASSERT (IS_POWER_OF_2 (Header->PageSize)); > + > + *RamdiskSize = Header->RamdiskSize; > + > + if (Header->RamdiskSize != 0) { > + *Ramdisk = (VOID *) (BootImgBytePtr > + + Header->PageSize > + + ALIGN_VALUE (Header->KernelSize, Header->PageSize)); > + } > + return EFI_SUCCESS; > +} > + > +EFI_STATUS > +AbootimgGetSecondBootLoaderInfo ( > + IN VOID *BootImg, > + OUT VOID **Second, > + OUT UINTN *SecondSize > + ) > +{ > + ANDROID_BOOTIMG_HEADER *Header; > + UINT8 *BootImgBytePtr; > + > + // Cast to UINT8 so we can do pointer arithmetic > + BootImgBytePtr = (UINT8 *) BootImg; > + > + Header = (ANDROID_BOOTIMG_HEADER *) BootImg; > + > + if (AsciiStrnCmp (Header->BootMagic, BOOT_MAGIC, BOOT_MAGIC_LENGTH) != 0) { > + return EFI_INVALID_PARAMETER; > + } > + > + ASSERT (IS_POWER_OF_2 (Header->PageSize)); > + > + *SecondSize = Header->SecondStageBootloaderSize; > + > + if (Header->SecondStageBootloaderSize != 0) { > + *Second = (VOID *) (BootImgBytePtr > + + Header->PageSize > + + ALIGN_VALUE (Header->KernelSize, Header->PageSize) > + + ALIGN_VALUE (Header->RamdiskSize, Header->PageSize)); > + } > + return EFI_SUCCESS; > +} > + > +EFI_STATUS > +AbootimgGetKernelArgs ( > + IN VOID *BootImg, > + OUT CHAR8 *KernelArgs > + ) > +{ > + ANDROID_BOOTIMG_HEADER *Header; > + > + Header = (ANDROID_BOOTIMG_HEADER *) BootImg; > + AsciiStrnCpyS (KernelArgs, BOOTIMG_KERNEL_ARGS_SIZE, Header->KernelArgs, > + BOOTIMG_KERNEL_ARGS_SIZE); > + > + return EFI_SUCCESS; > +} > + > +EFI_STATUS > +AbootimgInstallFdt ( > + IN VOID *BootImg, > + IN EFI_PHYSICAL_ADDRESS FdtBase, > + OUT VOID *KernelArgs > + ) > +{ > + VOID *Ramdisk; > + UINTN RamdiskSize; > + CHAR8 ImgKernelArgs[BOOTIMG_KERNEL_ARGS_SIZE]; > + INTN err; > + EFI_STATUS Status; > + EFI_PHYSICAL_ADDRESS NewFdtBase; > + > + Status = gBS->LocateProtocol (&gAbootimgProtocolGuid, NULL, (VOID **) &mAbootimg); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + > + if (EFI_ERROR (Status)) { > + return Status; > + } > + > + Status = AbootimgGetRamdiskInfo ( > + BootImg, > + &Ramdisk, > + &RamdiskSize > + ); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + Status = AbootimgGetKernelArgs ( > + BootImg, > + ImgKernelArgs > + ); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + // Get kernel arguments from Android boot image > + AsciiStrToUnicodeStrS (ImgKernelArgs, KernelArgs, BOOTIMG_KERNEL_ARGS_SIZE >> 1); > + // Set the ramdisk in command line arguments > + UnicodeSPrint ( > + (CHAR16 *)KernelArgs + StrLen (KernelArgs), BOOTIMG_KERNEL_ARGS_SIZE, > + L" initrd=0x%x,0x%x", > + (UINTN)Ramdisk, (UINTN)RamdiskSize > + ); This is not an appropriate way to set ramdisk. There are dedicated "linux,initrd-start" and "linux,initrd-end" options in the chosen node. See http://git.savannah.gnu.org/cgit/grub.git/tree/grub-core/loader/arm64/linux.c#n79 for an example. > + > + // Append platform kernel arguments > + Status = mAbootimg->AppendArgs (KernelArgs, BOOTIMG_KERNEL_ARGS_SIZE); Am I missing something? Where is this AppendArgs function defined? > + if (EFI_ERROR (Status)) { > + return Status; > + } > + > + Status = mAbootimg->UpdateDtb (FdtBase, &NewFdtBase); Likewise, where is this function defined? > + if (EFI_ERROR (Status)) { > + return Status; > + } > + > + // > + // Sanity checks on the new FDT blob. > + // > + err = fdt_check_header ((VOID*)(UINTN)NewFdtBase); > + if (err != 0) { > + Print (L"ERROR: Device Tree header not valid (err:%d)\n", err); > + return EFI_INVALID_PARAMETER; > + } > + > + Status = gBS->InstallConfigurationTable ( > + &gFdtTableGuid, > + (VOID *)(UINTN)NewFdtBase > + ); > + return Status; > +} > + > +EFI_STATUS > +AbootimgBoot ( > + IN VOID *Buffer, > + IN UINTN BufferSize > + ) > +{ > + EFI_STATUS Status; > + VOID *Kernel; > + UINTN KernelSize; > + VOID *SecondLoader; > + UINTN SecondLoaderSize; > + MEMORY_DEVICE_PATH KernelDevicePath; > + EFI_HANDLE ImageHandle; > + EFI_PHYSICAL_ADDRESS FdtBase; > + VOID *NewKernelArg; > + EFI_LOADED_IMAGE_PROTOCOL *ImageInfo; > + INTN Err; > + > + Status = AbootimgGetKernelInfo ( > + Buffer, > + &Kernel, > + &KernelSize > + ); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + > + /* For flatten image, Fdt is attached at the end of kernel. > + Get real kernel size. > + */ This is a completely broken usage model :( Is this a requirement for this support? I mean, the solution you have implemented is a nice way of dealing with the mess, but per-kernel-image DT blobs were deprecated at kernel summit 2013. > + KernelSize = *(UINT32 *)((EFI_PHYSICAL_ADDRESS)(UINTN)Kernel + KERNEL_IMAGE_STEXT_OFFSET) + > + *(UINT32 *)((EFI_PHYSICAL_ADDRESS)(UINTN)Kernel + KERNEL_IMAGE_RAW_SIZE_OFFSET); > + > + NewKernelArg = AllocateZeroPool (BOOTIMG_KERNEL_ARGS_SIZE); > + if (NewKernelArg == NULL) { > + DEBUG ((DEBUG_ERROR, "Fail to allocate memory\n")); > + return EFI_OUT_OF_RESOURCES; > + } > + > + /* FDT is at the end of kernel image */ > + FdtBase = (EFI_PHYSICAL_ADDRESS)(UINTN)Kernel + KernelSize; > + // > + // Sanity checks on the original FDT blob. > + // > + Err = fdt_check_header ((VOID*)(UINTN)FdtBase); > + if (Err != 0) { > + /* Check whether FDT is located in second boot loader */ What is a second boot loader? > + Status = AbootimgGetSecondBootLoaderInfo ( > + Buffer, > + &SecondLoader, > + &SecondLoaderSize > + ); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + > + Err = fdt_check_header ((VOID*)(UINTN)SecondLoader); > + if (Err != 0) { > + DEBUG ((DEBUG_ERROR, "ERROR: Device Tree header not valid (Err:%d)\n", Err)); > + return EFI_INVALID_PARAMETER; > + } > + FdtBase = (EFI_PHYSICAL_ADDRESS)SecondLoader; > + } > + The above two scenarios could do with broken out as separate helper functions. Preferably also with a Pcd to determine whether the application should even bother to go looking if a platform-provided device tree was already installed. > + Status = AbootimgInstallFdt (Buffer, FdtBase, NewKernelArg); > + if (EFI_ERROR (Status)) { > + FreePool (NewKernelArg); > + return EFI_INVALID_PARAMETER; > + } > + > + KernelDevicePath = MemoryDevicePathTemplate; > + > + // Have to cast to UINTN before casting to EFI_PHYSICAL_ADDRESS in order to > + // appease GCC. (Redundant comment. It's a common enough problem.) > + KernelDevicePath.Node1.StartingAddress = (EFI_PHYSICAL_ADDRESS)(UINTN) Kernel; > + KernelDevicePath.Node1.EndingAddress = (EFI_PHYSICAL_ADDRESS)(UINTN) Kernel + KernelSize; > + > + Status = gBS->LoadImage (TRUE, gImageHandle, (EFI_DEVICE_PATH *)&KernelDevicePath, (VOID*)(UINTN)Kernel, KernelSize, &ImageHandle); Please split function call up over multiple lines. > + > + // Set kernel arguments > + Status = gBS->HandleProtocol (ImageHandle, &gEfiLoadedImageProtocolGuid, (VOID **) &ImageInfo); Please split function call up over multiple lines. > + ImageInfo->LoadOptions = NewKernelArg; > + ImageInfo->LoadOptionsSize = StrLen (NewKernelArg) * sizeof (CHAR16); > + > + // Before calling the image, enable the Watchdog Timer for the 5 Minute period Please wrap comment at 80 characters. (Or delete the "for the 5 Minute period bit based on the next comment.) > + gBS->SetWatchdogTimer (5 * 60, 0x0000, 0x00, NULL); Please create a define for that timeout. ANDROID_BOOT_WATCHDOG_TIMEOUT or something. Or, hmm, given that the UEFI specification (2.7) says "The watchdog must be set to a period of 5 minutes", this may even be worth putting into MdePkg/MdeModulePkg under a generic name. The UEFI specification says "The firmware reserves codes 0x0000 to 0xFFFF. Loaders and operating systems may use other timeout codes.". So I think this code needs to set this to something else. You don't need to say 0x00 when you mean 0. > + // Start the image > + Status = gBS->StartImage (ImageHandle, NULL, NULL); > + // Clear the Watchdog Timer after the image returns _if_ the image returns, surely? > + gBS->SetWatchdogTimer (0x0000, 0x0000, 0x0000, NULL); Don't say 0x* when you mean 0. > + return EFI_SUCCESS; > +} > diff --git a/EmbeddedPkg/Library/AbootimgLib/AbootimgLib.inf b/EmbeddedPkg/Library/AbootimgLib/AbootimgLib.inf > new file mode 100644 > index 0000000..461dcb8 > --- /dev/null > +++ b/EmbeddedPkg/Library/AbootimgLib/AbootimgLib.inf > @@ -0,0 +1,48 @@ > +#/** @file > +# > +# Copyright (c) 2013-2015, ARM Ltd. All rights reserved.
> +# 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 >