From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pf0-x230.google.com (mail-pf0-x230.google.com [IPv6:2607:f8b0:400e:c00::230]) (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 2CD5A21E0C31E for ; Fri, 28 Jul 2017 07:16:53 -0700 (PDT) Received: by mail-pf0-x230.google.com with SMTP id e75so18270542pfj.2 for ; Fri, 28 Jul 2017 07:18:58 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=EFvqqLCSQGvwcQG+Nlwn2I71FrtKK1a0VkjN33iOB6I=; b=I7R+C0OI6bjq3vnX1wMGZaJ1m7XhrjRdXu1gS9k5uaOsBt33I2MBE+qVBliLVWrkDa Pk9YAC3TqYs1fzr9ErIvToVqRsDluXv1Bv2Ij1A/1aZgYE45AmXCBVw8m9NaytaLZqis eDEE0z7Csy0i1k2lWe1g9T8FGtXc525l/ZQ+s= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=EFvqqLCSQGvwcQG+Nlwn2I71FrtKK1a0VkjN33iOB6I=; b=h/U15dUHLQ/rwIiTBE1u8V4WQfYcAjHQhqDw2KnlabLciRpyqBsXu101kJ0Gy4quuE S1Hq/Old55T761owo4G1vdC03mvcSMRL5mO2UEFq07L1rmn5UoRBpG3wmbdssj0Oc7sS rz9jQaumuPmPymkwavSI+Pnp/TSL/PbYpylGFP9/Z5jCocH/O81rwDCudYMcmX3ZRRmk cY1Kk3mEsKKSSDZY0WeEbOf52ks7/cg1bK2uZamK+sVkqGmhvRPkL9AQpZovI1iR7tJA iDhQJg7JGI0jn4cocZwZ1ukq7eIKtW5iTvwPgfU+0kMtTt5+lwB2YPi692p7v43ZOQk0 UI7w== X-Gm-Message-State: AIVw111hzOl+SIwSB2DllMedrYFaQG0Rl+ffJfGZNE4eN4WUv0U6NFNu svlnzjgB2MpZ+8mA X-Received: by 10.98.99.131 with SMTP id x125mr7760712pfb.231.1501251537423; Fri, 28 Jul 2017 07:18:57 -0700 (PDT) Received: from [192.168.1.4] ([112.64.60.64]) by smtp.googlemail.com with ESMTPSA id d80sm322193pfl.142.2017.07.28.07.18.52 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 28 Jul 2017 07:18:56 -0700 (PDT) To: Leif Lindholm 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 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> From: Jun Nie Message-ID: <96dd3ea5-b99e-5b12-ed47-160310156ab9@linaro.org> Date: Fri, 28 Jul 2017 22:18:49 +0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: <20170728130643.GP1501@bivouac.eciton.net> 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:16:53 -0000 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit 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/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.
>>>> + 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 >>>> + >>>> +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.
>>>> + 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. 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; >>>> +}