From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=66.187.233.73; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 26B62220E8DF4 for ; Mon, 23 Apr 2018 14:24:59 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 31182A202F; Mon, 23 Apr 2018 21:24:58 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-123.rdu2.redhat.com [10.10.120.123]) by smtp.corp.redhat.com (Postfix) with ESMTP id 35A282026DFD; Mon, 23 Apr 2018 21:24:57 +0000 (UTC) To: Haojian Zhuang , edk2-devel@lists.01.org Cc: Leif Lindholm , Ard Biesheuvel References: <1524464559-14549-1-git-send-email-haojian.zhuang@linaro.org> <1524464559-14549-2-git-send-email-haojian.zhuang@linaro.org> From: Laszlo Ersek Message-ID: Date: Mon, 23 Apr 2018 23:24:56 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: <1524464559-14549-2-git-send-email-haojian.zhuang@linaro.org> X-Scanned-By: MIMEDefang 2.78 on 10.11.54.4 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.2]); Mon, 23 Apr 2018 21:24:58 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.2]); Mon, 23 Apr 2018 21:24:58 +0000 (UTC) for IP:'10.11.54.4' DOMAIN:'int-mx04.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'lersek@redhat.com' RCPT:'' Subject: Re: [PATCH edk2-platforms v4 1/2] Platform/HiKey960: register predefined boot options X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.26 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 23 Apr 2018 21:25:00 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Hello Haojian, On 04/23/18 08:22, Haojian Zhuang wrote: > Create 4 boot options on HiKey960 platform. They're "Boot from SD", > "Grub", "Android Boot" and "Android Fastboot". > > Cc: Laszlo Ersek > Cc: Leif Lindholm > Cc: Ard Biesheuvel > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Haojian Zhuang > --- > Platform/Hisilicon/HiKey960/HiKey960.dec | 35 ++++ > Platform/Hisilicon/HiKey960/HiKey960.dsc | 6 + > .../Hisilicon/HiKey960/HiKey960Dxe/HiKey960Dxe.c | 182 +++++++++++++++++++++ > .../Hisilicon/HiKey960/HiKey960Dxe/HiKey960Dxe.inf | 11 ++ > 4 files changed, 234 insertions(+) > create mode 100644 Platform/Hisilicon/HiKey960/HiKey960.dec This patch looks good to me in general, but there are a number of small warts / improvements that I might as well point out. I will leave it up to you to address or ignore each of these points, as you see fit. > diff --git a/Platform/Hisilicon/HiKey960/HiKey960.dec b/Platform/Hisilicon/HiKey960/HiKey960.dec > new file mode 100644 > index 000000000000..922d7199c5a5 > --- /dev/null > +++ b/Platform/Hisilicon/HiKey960/HiKey960.dec > @@ -0,0 +1,35 @@ > +# > +# Copyright (c) 2018, Linaro Limited. 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] > + DEC_SPECIFICATION = 0x00010019 > + PACKAGE_NAME = HiKey960 > + PACKAGE_GUID = 1892b5b5-d18d-47a3-8fab-e3ae6b4226b0 > + PACKAGE_VERSION = 0.1 > + > +################################################################################ > +# > +# Include Section - list of Include Paths that are provided by this package. > +# Comments are used for Keywords and Module Types. > +# > +# Supported Module Types: > +# BASE SEC PEI_CORE PEIM DXE_CORE DXE_DRIVER DXE_RUNTIME_DRIVER DXE_SMM_DRIVER DXE_SAL_DRIVER UEFI_DRIVER UEFI_APPLICATION > +# > +################################################################################ > +[Guids.common] > + gHiKey960TokenSpaceGuid = { 0x99a14446, 0xaad7, 0xe460, {0xb4, 0xe5, 0x1f, 0x79, 0xaa, 0xa4, 0x93, 0xfd } } > + > +[PcdsFixedAtBuild.common] > + gHiKey960TokenSpaceGuid.PcdAndroidBootDevicePath|L""|VOID*|0x00000001 > + gHiKey960TokenSpaceGuid.PcdAndroidBootFile|{ 0x36, 0x8b, 0x73, 0x3a, 0xc5, 0xb9, 0x63, 0x47, 0xab, 0xbd, 0x6c, 0xbd, 0x4b, 0x25, 0xf9, 0xff }|VOID*|0x00000002 > + gHiKey960TokenSpaceGuid.PcdAndroidFastbootFile|{ 0x2a, 0x50, 0x88, 0x95, 0x70, 0x53, 0xe3, 0x11, 0x86, 0x31, 0xd7, 0xc5, 0x95, 0x13, 0x64, 0xc8 }|VOID*|0x00000003 > + gHiKey960TokenSpaceGuid.PcdSdBootDevicePath|L""|VOID*|0x00000004 > diff --git a/Platform/Hisilicon/HiKey960/HiKey960.dsc b/Platform/Hisilicon/HiKey960/HiKey960.dsc > index 859ab84f8415..475d39916262 100644 > --- a/Platform/Hisilicon/HiKey960/HiKey960.dsc > +++ b/Platform/Hisilicon/HiKey960/HiKey960.dsc > @@ -132,6 +132,12 @@ [PcdsFixedAtBuild.common] > gEmbeddedTokenSpaceGuid.PcdAndroidFastbootUsbVendorId|0x18d1 > gEmbeddedTokenSpaceGuid.PcdAndroidFastbootUsbProductId|0xd00d > > + # > + # Android Loader > + # > + gHiKey960TokenSpaceGuid.PcdAndroidBootDevicePath|L"VenHw(0D51905B-B77E-452A-A2C0-ECA0CC8D514A,00003BFF0000000000)/UFS(0x0,0x3)/HD(7,GPT,D3340696-9B95-4C64-8DF6-E6D4548FBA41,0x12100,0x4000)" > + gHiKey960TokenSpaceGuid.PcdSdBootDevicePath|L"VenHw(0D51905B-B77E-452A-A2C0-ECA0CC8D514A,00F037FF0000000000)/SD(0x0)" > + > ################################################################################ > # > # Components Section - list of all EDK II Modules needed by this Platform > diff --git a/Platform/Hisilicon/HiKey960/HiKey960Dxe/HiKey960Dxe.c b/Platform/Hisilicon/HiKey960/HiKey960Dxe/HiKey960Dxe.c > index 473d61ed384e..da5e3d5a80ce 100644 > --- a/Platform/Hisilicon/HiKey960/HiKey960Dxe/HiKey960Dxe.c > +++ b/Platform/Hisilicon/HiKey960/HiKey960Dxe/HiKey960Dxe.c > @@ -30,10 +30,16 @@ > #include > #include > #include > +#include > #include > +#include > #include > > +#include > +#include > #include > +#include > +#include > #include > > #define ADC_ADCIN0 0 > @@ -86,6 +92,10 @@ > > #define DETECT_SW_FASTBOOT 68 // GPIO8_4 > > +#define HIKEY960_BOOT_OPTION_NUM 4 > + > +#define GRUB_FILE_NAME L"\\EFI\\BOOT\\GRUBAA64.EFI" > + > typedef struct { > UINT64 Magic; > UINT64 Data; > @@ -359,6 +369,168 @@ OnEndOfDxe ( > } > } > > +STATIC > +EFI_STATUS > +GetPlatformBootOptionsAndKeys ( > + OUT UINTN *BootCount, > + OUT EFI_BOOT_MANAGER_LOAD_OPTION **BootOptions, > + OUT EFI_INPUT_KEY **BootKeys > + ) > +{ > + EFI_DEVICE_PATH *DevicePath; > + EFI_DEVICE_PATH *FileDevicePath; > + EFI_DEVICE_PATH_FROM_TEXT_PROTOCOL *DPProtocol; > + FILEPATH_DEVICE_PATH *FilePath; > + EFI_GUID *FileGuid; > + EFI_LOADED_IMAGE_PROTOCOL *LoadedImage; > + MEDIA_FW_VOL_FILEPATH_DEVICE_PATH FileNode; > + CHAR16 *PathStr; > + EFI_STATUS Status; > + UINTN Size; > + > + if ((BootCount == NULL) || (BootOptions == NULL) || (BootKeys == NULL)) { > + return EFI_INVALID_PARAMETER; > + } This is a good check. I didn't suggest specifying EFI_INVALID_PARAMETER at the protocol definition level -- none of the OUT parameters were marked OPTIONAL, and I expect callers and implementors to honor the interface contract, otherwise they invoke undefined behavior --, but, indeed, if extra checking of the parameters for validity isn't hard, then it's justifiable to do it. > + Size = sizeof (EFI_BOOT_MANAGER_LOAD_OPTION) * HIKEY960_BOOT_OPTION_NUM; > + *BootOptions = (EFI_BOOT_MANAGER_LOAD_OPTION *)AllocateZeroPool (Size); > + if (*BootOptions == NULL) { > + DEBUG ((DEBUG_ERROR, "Failed to allocate memory for BootOptions\n")); > + return EFI_BUFFER_TOO_SMALL; > + } (1) This should return EFI_OUT_OF_RESOURCES, as specified at the protocol level. > + Size = sizeof (EFI_INPUT_KEY) * HIKEY960_BOOT_OPTION_NUM; > + *BootKeys = (EFI_INPUT_KEY *)AllocateZeroPool (Size); > + if (*BootKeys == NULL) { > + DEBUG ((DEBUG_ERROR, "Failed to allocate memory for BootKeys\n")); > + FreePool (*BootOptions); > + return EFI_BUFFER_TOO_SMALL; > + } (2) Same as (1). (3) In general I suggest adding "goto"-style error handler labels, because given a number of branches that can fail, the FreePool() calls like the above accumulate too much. Perhaps it's not too bad in this patch; up to you to evaluate. > + > + PathStr = (CHAR16 *)PcdGetPtr (PcdSdBootDevicePath); > + ASSERT (PathStr != NULL); > + Status = gBS->LocateProtocol (&gEfiDevicePathFromTextProtocolGuid, > + NULL, > + (VOID **)&DPProtocol > + ); > + ASSERT_EFI_ERROR (Status); > + DevicePath = (EFI_DEVICE_PATH *)DPProtocol->ConvertTextToDevicePath (PathStr); (4) You can simplify this code by calling the ConvertTextToDevicePath() function from the DevicePathLib class. The library class header is: MdePkg/Include/Library/DevicePathLib.h and the library instance that the platform DSC file should select, for a (presumably) DXE_DRIVER module like this, is MdePkg/Library/UefiDevicePathLibDevicePathProtocol/UefiDevicePathLibDevicePathProtocol.inf which will base the library function atop the protocol. (See also which I just filed.) > + ASSERT (DevicePath != NULL); > + Status = EfiBootManagerInitializeLoadOption ( > + &(*BootOptions)[0], > + LoadOptionNumberUnassigned, > + LoadOptionTypeBoot, > + LOAD_OPTION_ACTIVE, > + L"Boot from SD", > + DevicePath, > + NULL, > + 0 > + ); > + ASSERT_EFI_ERROR (Status); > + FreePool (DevicePath); Yup, good choice, EfiBootManagerInitializeLoadOption() makes deep copies. > + > + PathStr = (CHAR16 *)PcdGetPtr (PcdAndroidBootDevicePath); > + ASSERT (PathStr != NULL); > + Status = gBS->LocateProtocol (&gEfiDevicePathFromTextProtocolGuid, > + NULL, > + (VOID **)&DPProtocol > + ); > + ASSERT_EFI_ERROR (Status); > + DevicePath = (EFI_DEVICE_PATH *)DPProtocol->ConvertTextToDevicePath (PathStr); > + ASSERT (DevicePath != NULL); (5) See (4). > + Size = StrSize (GRUB_FILE_NAME); > + FileDevicePath = AllocatePool (Size + SIZE_OF_FILEPATH_DEVICE_PATH + END_DEVICE_PATH_LENGTH); > + if (FileDevicePath != NULL) { > + FilePath = (FILEPATH_DEVICE_PATH *) FileDevicePath; > + FilePath->Header.Type = MEDIA_DEVICE_PATH; > + FilePath->Header.SubType = MEDIA_FILEPATH_DP; > + CopyMem (&FilePath->PathName, GRUB_FILE_NAME, Size); > + SetDevicePathNodeLength (&FilePath->Header, Size + SIZE_OF_FILEPATH_DEVICE_PATH); > + SetDevicePathEndNode (NextDevicePathNode (&FilePath->Header)); > + > + DevicePath = AppendDevicePath (DevicePath, FileDevicePath); > + FreePool (FileDevicePath); > + } (6) This logic can be replaced with a simple call to FileDevicePath(). Please read its documentation in "MdePkg/Include/Library/DevicePathLib.h". (In general that library class has very useful features.) For that to work however, you first have to kill the local variable called "FileDevicePath", because it shadows the function :) (7) Another important note: whenever you call DevicePath = AppendDevicePath (DevicePath, FileDevicePath); the original DevicePath is forgotten (it is not modified in place). So, if DevicePath originally pointed to dynamically allocated storage that is now unreachable otherwise, it's leaked. And that's the case here, because DevicePath comes from ConvertTextToDevicePath(). Please read the AppendDevicePath() docs in the lib class header. (8) Taking one step back -- is there any particular reason for defining GRUB_FILE_NAME as a macro, and appending it dynamically to "PcdAndroidBootDevicePath"? Why not put the full GRUB pathname into "PcdAndroidBootDevicePath" to begin with? > + Status = EfiBootManagerInitializeLoadOption ( > + &(*BootOptions)[1], > + LoadOptionNumberUnassigned, > + LoadOptionTypeBoot, > + LOAD_OPTION_ACTIVE, > + L"Grub", > + DevicePath, > + NULL, > + 0 > + ); > + ASSERT_EFI_ERROR (Status); > + FreePool (DevicePath); (9) So, with (8) fixed, I believe that you can introduce a helper function for the first two boot options: - A pointer to the textual device path -- you can fill this in from PcdGetPtr() calls, at the call sites, - an EFI_BOOT_MANAGER_LOAD_OPTION to populate (via pointer) No need to duplicate the same code for both boot options. > + > + FileGuid = PcdGetPtr (PcdAndroidBootFile); > + ASSERT (FileGuid != NULL); > + Status = gBS->HandleProtocol ( > + gImageHandle, > + &gEfiLoadedImageProtocolGuid, > + (VOID **) &LoadedImage > + ); > + ASSERT_EFI_ERROR (Status); > + EfiInitializeFwVolDevicepathNode (&FileNode, FileGuid); > + DevicePath = DevicePathFromHandle (LoadedImage->DeviceHandle); > + ASSERT (DevicePath != NULL); > + DevicePath = AppendDevicePathNode ( > + DevicePath, > + (EFI_DEVICE_PATH_PROTOCOL *) &FileNode > + ); > + ASSERT (DevicePath != NULL); > + Status = EfiBootManagerInitializeLoadOption ( > + &(*BootOptions)[2], > + LoadOptionNumberUnassigned, > + LoadOptionTypeBoot, > + LOAD_OPTION_ACTIVE, > + L"Android Boot", > + DevicePath, > + NULL, > + 0 > + ); > + ASSERT_EFI_ERROR (Status); > + FreePool (DevicePath); This looks correct. Here the pointer that DevicePathFromHandle() returns for DevicePath is not an "owner" pointer that we must not forget, but a "navigation" pointer that we are free to forget. > + > + FileGuid = PcdGetPtr (PcdAndroidFastbootFile); > + ASSERT (FileGuid != NULL); > + Status = gBS->HandleProtocol ( > + gImageHandle, > + &gEfiLoadedImageProtocolGuid, > + (VOID **) &LoadedImage > + ); > + ASSERT_EFI_ERROR (Status); > + EfiInitializeFwVolDevicepathNode (&FileNode, FileGuid); > + DevicePath = DevicePathFromHandle (LoadedImage->DeviceHandle); > + ASSERT (DevicePath != NULL); > + DevicePath = AppendDevicePathNode ( > + DevicePath, > + (EFI_DEVICE_PATH_PROTOCOL *) &FileNode > + ); > + ASSERT (DevicePath != NULL); > + Status = EfiBootManagerInitializeLoadOption ( > + &(*BootOptions)[3], > + LoadOptionNumberUnassigned, > + LoadOptionTypeBoot, > + LOAD_OPTION_ACTIVE, > + L"Android Fastboot", > + DevicePath, > + NULL, > + 0 > + ); > + ASSERT_EFI_ERROR (Status); > + FreePool (DevicePath); (10) This is an almost identical duplicate of the previous boot option. Another helper function seems possible, which would take: - an FFS (firmware file system) file GUID, populated from PcdGetPtr() at the call site, - an EFI_BOOT_MANAGER_LOAD_OPTION to populate (via pointer) Thanks, Laszlo > + (*BootKeys)[3].ScanCode = SCAN_NULL; > + (*BootKeys)[3].UnicodeChar = 'f'; > + > + *BootCount = 4; > + > + return EFI_SUCCESS; > +} > + > +PLATFORM_BOOT_MANAGER_PROTOCOL mPlatformBootManager = { > + GetPlatformBootOptionsAndKeys > +}; > + > EFI_STATUS > EFIAPI > VirtualKeyboardRegister ( > @@ -487,5 +659,15 @@ HiKey960EntryPoint ( > EFI_NATIVE_INTERFACE, > &mVirtualKeyboard > ); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + > + Status = gBS->InstallProtocolInterface ( > + &ImageHandle, > + &gPlatformBootManagerProtocolGuid, > + EFI_NATIVE_INTERFACE, > + &mPlatformBootManager > + ); > return Status; > } > diff --git a/Platform/Hisilicon/HiKey960/HiKey960Dxe/HiKey960Dxe.inf b/Platform/Hisilicon/HiKey960/HiKey960Dxe/HiKey960Dxe.inf > index cc517656b340..5adedd79d3e8 100644 > --- a/Platform/Hisilicon/HiKey960/HiKey960Dxe/HiKey960Dxe.inf > +++ b/Platform/Hisilicon/HiKey960/HiKey960Dxe/HiKey960Dxe.inf > @@ -25,6 +25,7 @@ [Packages] > EmbeddedPkg/EmbeddedPkg.dec > MdePkg/MdePkg.dec > MdeModulePkg/MdeModulePkg.dec > + Platform/Hisilicon/HiKey960/HiKey960.dec > > [LibraryClasses] > BaseMemoryLib > @@ -37,15 +38,25 @@ [LibraryClasses] > PrintLib > SerialPortLib > TimerLib > + UefiBootManagerLib > UefiBootServicesTableLib > UefiDriverEntryPoint > UefiLib > UefiRuntimeServicesTableLib > > [Protocols] > + gEfiDevicePathFromTextProtocolGuid > + gEfiLoadedImageProtocolGuid > gEmbeddedGpioProtocolGuid > + gPlatformBootManagerProtocolGuid > gPlatformVirtualKeyboardProtocolGuid > > +[Pcd] > + gHiKey960TokenSpaceGuid.PcdAndroidBootDevicePath > + gHiKey960TokenSpaceGuid.PcdAndroidBootFile > + gHiKey960TokenSpaceGuid.PcdAndroidFastbootFile > + gHiKey960TokenSpaceGuid.PcdSdBootDevicePath > + > [Guids] > gEfiEndOfDxeEventGroupGuid > gEfiFileInfoGuid >