From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-x22f.google.com (mail-wm0-x22f.google.com [IPv6:2a00:1450:400c:c09::22f]) (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 3CD8B21E0C2F5 for ; Tue, 8 Aug 2017 06:32:31 -0700 (PDT) Received: by mail-wm0-x22f.google.com with SMTP id m85so7481073wma.0 for ; Tue, 08 Aug 2017 06:34:48 -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=YNz0FEzdCs1nrSFucVq/2Htk1x63j71GVGLRX5AVkmE=; b=V7sfdGMom5MQoZEVqxN/qVIIQ/2cjqRNrTPz8hVYtpARxNiKRGTQmM8Sp6HLbpC85E ag7fTXxqUHzyfhs4yd7gfFPsnJ5FtFf/rps+C8kXFoFEym2yBBv1+NdR6ByY/os22XfQ 4iiGtctROP5tmQ0QIUmp58wjXlDSw/IVjD7cc= 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=YNz0FEzdCs1nrSFucVq/2Htk1x63j71GVGLRX5AVkmE=; b=jmmOQVxmfwxHmqePGwjIBAIHXtBFHgJSZbDiAN9FFaS+PiUbs+6ZnpxYLmRaKibQVZ n64zxjU4cuFokCR4Ih1ExA1OdfpxAo8VSB/aQlE2d+iVrLpDupv4kQZ6B7W3ZuzAL0xY a7qNy3tY8/OYfuT2FM4cji1kDImsKUNpye6YQBk4X2VLjEaLyHSsO/NRZwN0tLOwyEmn 4Fb0Bw29Y9rjyoSqCIAUwDO79idw2pcPgEIKgEDdvqm/w6sSQRM074pI1XpMEwgWqMpl mj/yA4OVoXIB2I5wdz9tBIjBP/Z/9C+1GhMC1vI2hFQdhMVxEH92W7JqUchrOrlt/WtI KvjA== X-Gm-Message-State: AHYfb5jD7CjO8AbLEiMybr8EC+JzfncXzbhmHxy4WpU7qkC7UEOXZoqH AC/OvTeJFEIFZxeL X-Received: by 10.28.16.19 with SMTP id 19mr2802806wmq.83.1502199286911; Tue, 08 Aug 2017 06:34:46 -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 6sm1984673wrn.52.2017.08.08.06.34.45 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Tue, 08 Aug 2017 06:34:45 -0700 (PDT) Date: Tue, 8 Aug 2017 14:34:43 +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: <20170808133443.mjdz7c5fkkg3aazb@bivouac.eciton.net> References: <1501682622-8223-1-git-send-email-jun.nie@linaro.org> <1501682622-8223-2-git-send-email-jun.nie@linaro.org> MIME-Version: 1.0 In-Reply-To: <1501682622-8223-2-git-send-email-jun.nie@linaro.org> User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [PATCH v5 2/3] 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, 08 Aug 2017 13:32:31 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Wed, Aug 02, 2017 at 10:03:41PM +0800, Jun Nie wrote: > Add an android kernel loader that could load kernel from storage > device. > This android boot image BDS add addtitional cmdline/dtb/ramfs > support besides kernel that is introduced by Android boot header. > > This patch is derived from Haojian's code as below link. > https://patches.linaro.org/patch/94683/ > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Jun Nie I have one style-related comment below. Other than that, my final remaining concern for this series is that this _still_ breaks bisect. This application still cannot be built until what is currently 3/3 has been applied. My view is that there is no separate need for 3/3 and it can simply be squashed into this patch. > --- > .../Application/AndroidBoot/AndroidBootApp.c | 140 ++++++ > .../Application/AndroidBoot/AndroidBootApp.inf | 64 +++ > EmbeddedPkg/Include/Library/AndroidBootImgLib.h | 13 + > EmbeddedPkg/Include/Protocol/AndroidBootImg.h | 47 ++ > .../Library/AndroidBootImgLib/AndroidBootImgLib.c | 471 +++++++++++++++++++++ > .../AndroidBootImgLib/AndroidBootImgLib.inf | 48 +++ > 6 files changed, 783 insertions(+) > create mode 100644 EmbeddedPkg/Application/AndroidBoot/AndroidBootApp.c > create mode 100644 EmbeddedPkg/Application/AndroidBoot/AndroidBootApp.inf > 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..da5b5d2 > --- /dev/null > +++ b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c > @@ -0,0 +1,471 @@ > +/** @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 > + > +#define FDT_ADDITIONAL_ENTRIES_SIZE 0x400 > + > +typedef struct { > + MEMMAP_DEVICE_PATH Node1; > + EFI_DEVICE_PATH_PROTOCOL End; > +} MEMORY_DEVICE_PATH; > + > +STATIC ANDROID_BOOTIMG_PROTOCOL *mAndroidBootImg; > + > +STATIC CONST MEMORY_DEVICE_PATH mMemoryDevicePathTemplate = > +{ > + { > + { > + 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 > +AndroidBootImgGetImgSize ( > + 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 > +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 > +AndroidBootImgGetRamdiskInfo ( > + 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 > +AndroidBootImgGetSecondBootLoaderInfo ( > + 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; Can you not replace this bit of casting... > + > + 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 With a simple (UINTN)BootImg here? If not, could you flip the input parameter to be a UINTN BootImgBase? > + + Header->PageSize > + + ALIGN_VALUE (Header->KernelSize, Header->PageSize) > + + ALIGN_VALUE (Header->RamdiskSize, Header->PageSize)); > + } > + return EFI_SUCCESS; > +} / Leif