From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-x22b.google.com (mail-wm0-x22b.google.com [IPv6:2a00:1450:400c:c09::22b]) (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 699F021CE7492 for ; Fri, 28 Jul 2017 07:50:22 -0700 (PDT) Received: by mail-wm0-x22b.google.com with SMTP id t201so127939321wmt.1 for ; Fri, 28 Jul 2017 07:52:27 -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:content-transfer-encoding:in-reply-to :user-agent; bh=E7yFKkJSbGhZo1erPI4m2FQvGZ1HwX5Z8B4BlYcK0no=; b=LYQJIHSRLR4uqQgI0/8o5R3J9jHwYxFghBD88leYh3wFjFNH63oK1X1Cta09/mF5WU E0YZvRS5KKGtyBmwc2rTlAZHhtd+HIdfe7FelbSFthmHzaymYmcyclbmCBpfXxP2K5N6 iBlwP8sHVBjxSrGisSkpNKGf/BoOjhhGoOQZg= 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:content-transfer-encoding :in-reply-to:user-agent; bh=E7yFKkJSbGhZo1erPI4m2FQvGZ1HwX5Z8B4BlYcK0no=; b=DV7bQUjNSmUfF2JMbtdjfFq+RMxiqu1n7c1qCvDRFTZ7BJdflcNaPtT+EGnVYTbsup ACFkSAWXGf9DYjCsakwDQy7z0Llb1WqIx+P37Gw9RrQKKcyiM3O/Xc/XQq+unNk/PPor c1dziKfBHhdBSy2coCuarnRH2M+NtaoqoOd+uFonnFicUmrn1Df4Qp3NJPkzx/M6nGcg zp26cy7QvSdGXI8E+O5XLnD6IzK1+ryvGKaxOKb2UEmSnjCJ1RwdcmNRW1KBfr6ueRxA 7iK1U+0cvSNaiBvIRT0Gbydx3HpqyLpmBkfBb4RbdFGEJsNYfixcMUZ6+P30lm4wJQPT Si5A== X-Gm-Message-State: AIVw112eR9L5qxsKJgPIQREoW7O/Jabl/jS+kijjWV+0Mg0iI054Aznd DDvfP/mU/sUKdjTf X-Received: by 10.28.4.148 with SMTP id 142mr5582392wme.9.1501253545431; Fri, 28 Jul 2017 07:52:25 -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 q18sm14289945wmd.44.2017.07.28.07.52.24 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 28 Jul 2017 07:52:24 -0700 (PDT) Date: Fri, 28 Jul 2017 15:52:22 +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: <20170728145222.GQ1501@bivouac.eciton.net> References: <1501150040-32613-1-git-send-email-jun.nie@linaro.org> <20170727140859.GL1501@bivouac.eciton.net> <8033c2f9-cfbb-e879-22d7-a0f484d2d4dd@linaro.org> <20170728130643.GP1501@bivouac.eciton.net> <96dd3ea5-b99e-5b12-ed47-160310156ab9@linaro.org> MIME-Version: 1.0 In-Reply-To: <96dd3ea5-b99e-5b12-ed47-160310156ab9@linaro.org> User-Agent: Mutt/1.5.23 (2014-03-12) Subject: Re: [PATCH v3 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: Fri, 28 Jul 2017 14:50:23 -0000 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit 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 > >>>>--- > >>>> 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.
> >>>>+ 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 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