diff mbox series

[edk2,edk2-platforms,v4,1/2] Platform/HiKey960: register predefined boot options

Message ID 1524464559-14549-2-git-send-email-haojian.zhuang@linaro.org
State New
Headers show
Series add platform boot options | expand

Commit Message

Haojian Zhuang April 23, 2018, 6:22 a.m. UTC 
Create 4 boot options on HiKey960 platform. They're "Boot from SD",
"Grub", "Android Boot" and "Android Fastboot".

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>

---
 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

-- 
2.7.4

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Comments

Laszlo Ersek April 23, 2018, 9:24 p.m. UTC | #1 
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 <lersek@redhat.com>

> Cc: Leif Lindholm <leif.lindholm@linaro.org>

> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> Contributed-under: TianoCore Contribution Agreement 1.1

> Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>

> ---

>  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 <Library/PrintLib.h>

>  #include <Library/SerialPortLib.h>

>  #include <Library/TimerLib.h>

> +#include <Library/UefiBootManagerLib.h>

>  #include <Library/UefiBootServicesTableLib.h>

> +#include <Library/UefiLib.h>

>  #include <Library/UefiRuntimeServicesTableLib.h>

>

> +#include <Protocol/DevicePath.h>

> +#include <Protocol/DevicePathFromText.h>

>  #include <Protocol/EmbeddedGpio.h>

> +#include <Protocol/LoadedImage.h>

> +#include <Protocol/PlatformBootManager.h>

>  #include <Protocol/PlatformVirtualKeyboard.h>

>

>  #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
<https://bugzilla.tianocore.org/show_bug.cgi?id=940> 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

>


_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Haojian Zhuang April 25, 2018, 4:59 a.m. UTC | #2 
Hi Laszlo,

Thanks a lot. Fix them in v5.

Best Regards
Haojian
________________________________________
From: Laszlo Ersek <lersek@redhat.com>

Sent: Monday, April 23, 2018 9:24 PM
To: Haojian Zhuang; edk2-devel@lists.01.org
Cc: Leif Lindholm; Ard Biesheuvel
Subject: Re: [edk2] [PATCH edk2-platforms v4 1/2] Platform/HiKey960: register predefined boot options

Hello Haojian,

On 04/23/18 08:22, Haojian Zhuang wrote:
&gt; Create 4 boot options on HiKey960 platform. They're "Boot from SD",
&gt; "Grub", "Android Boot" and "Android Fastboot".
&gt;
&gt; Cc: Laszlo Ersek <lersek@redhat.com>
&gt; Cc: Leif Lindholm <leif.lindholm@linaro.org>
&gt; Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
&gt; Contributed-under: TianoCore Contribution Agreement 1.1
&gt; Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>
&gt; ---
&gt;  Platform/Hisilicon/HiKey960/HiKey960.dec           |  35 ++++
&gt;  Platform/Hisilicon/HiKey960/HiKey960.dsc           |   6 +
&gt;  .../Hisilicon/HiKey960/HiKey960Dxe/HiKey960Dxe.c   | 182 +++++++++++++++++++++
&gt;  .../Hisilicon/HiKey960/HiKey960Dxe/HiKey960Dxe.inf |  11 ++
&gt;  4 files changed, 234 insertions(+)
&gt;  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.

&gt; diff --git a/Platform/Hisilicon/HiKey960/HiKey960.dec b/Platform/Hisilicon/HiKey960/HiKey960.dec
&gt; new file mode 100644
&gt; index 000000000000..922d7199c5a5
&gt; --- /dev/null
&gt; +++ b/Platform/Hisilicon/HiKey960/HiKey960.dec
&gt; @@ -0,0 +1,35 @@
&gt; +#
&gt; +#  Copyright (c) 2018, Linaro Limited. All rights reserved.
&gt; +#
&gt; +#  This program and the accompanying materials
&gt; +#  are licensed and made available under the terms and conditions of the BSD License
&gt; +#  which accompanies this distribution.  The full text of the license may be found at
&gt; +#  http://opensource.org/licenses/bsd-license.php
&gt; +#
&gt; +#  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
&gt; +#  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
&gt; +#
&gt; +
&gt; +[Defines]
&gt; +  DEC_SPECIFICATION              = 0x00010019
&gt; +  PACKAGE_NAME                   = HiKey960
&gt; +  PACKAGE_GUID                   = 1892b5b5-d18d-47a3-8fab-e3ae6b4226b0
&gt; +  PACKAGE_VERSION                = 0.1
&gt; +
&gt; +################################################################################
&gt; +#
&gt; +# Include Section - list of Include Paths that are provided by this package.
&gt; +#                   Comments are used for Keywords and Module Types.
&gt; +#
&gt; +# Supported Module Types:
&gt; +#  BASE SEC PEI_CORE PEIM DXE_CORE DXE_DRIVER DXE_RUNTIME_DRIVER DXE_SMM_DRIVER DXE_SAL_DRIVER UEFI_DRIVER UEFI_APPLICATION
&gt; +#
&gt; +################################################################################
&gt; +[Guids.common]
&gt; +  gHiKey960TokenSpaceGuid        = { 0x99a14446, 0xaad7, 0xe460, {0xb4, 0xe5, 0x1f, 0x79, 0xaa, 0xa4, 0x93, 0xfd } }
&gt; +
&gt; +[PcdsFixedAtBuild.common]
&gt; +  gHiKey960TokenSpaceGuid.PcdAndroidBootDevicePath|L""|VOID*|0x00000001
&gt; +  gHiKey960TokenSpaceGuid.PcdAndroidBootFile|{ 0x36, 0x8b, 0x73, 0x3a, 0xc5, 0xb9, 0x63, 0x47, 0xab, 0xbd, 0x6c, 0xbd, 0x4b, 0x25, 0xf9, 0xff }|VOID*|0x00000002
&gt; +  gHiKey960TokenSpaceGuid.PcdAndroidFastbootFile|{ 0x2a, 0x50, 0x88, 0x95, 0x70, 0x53, 0xe3, 0x11, 0x86, 0x31, 0xd7, 0xc5, 0x95, 0x13, 0x64, 0xc8 }|VOID*|0x00000003
&gt; +  gHiKey960TokenSpaceGuid.PcdSdBootDevicePath|L""|VOID*|0x00000004
&gt; diff --git a/Platform/Hisilicon/HiKey960/HiKey960.dsc b/Platform/Hisilicon/HiKey960/HiKey960.dsc
&gt; index 859ab84f8415..475d39916262 100644
&gt; --- a/Platform/Hisilicon/HiKey960/HiKey960.dsc
&gt; +++ b/Platform/Hisilicon/HiKey960/HiKey960.dsc
&gt; @@ -132,6 +132,12 @@ [PcdsFixedAtBuild.common]
&gt;    gEmbeddedTokenSpaceGuid.PcdAndroidFastbootUsbVendorId|0x18d1
&gt;    gEmbeddedTokenSpaceGuid.PcdAndroidFastbootUsbProductId|0xd00d
&gt;
&gt; +  #
&gt; +  # Android Loader
&gt; +  #
&gt; +  gHiKey960TokenSpaceGuid.PcdAndroidBootDevicePath|L"VenHw(0D51905B-B77E-452A-A2C0-ECA0CC8D514A,00003BFF0000000000)/UFS(0x0,0x3)/HD(7,GPT,D3340696-9B95-4C64-8DF6-E6D4548FBA41,0x12100,0x4000)"
&gt; +  gHiKey960TokenSpaceGuid.PcdSdBootDevicePath|L"VenHw(0D51905B-B77E-452A-A2C0-ECA0CC8D514A,00F037FF0000000000)/SD(0x0)"
&gt; +
&gt;  ################################################################################
&gt;  #
&gt;  # Components Section - list of all EDK II Modules needed by this Platform
&gt; diff --git a/Platform/Hisilicon/HiKey960/HiKey960Dxe/HiKey960Dxe.c b/Platform/Hisilicon/HiKey960/HiKey960Dxe/HiKey960Dxe.c
&gt; index 473d61ed384e..da5e3d5a80ce 100644
&gt; --- a/Platform/Hisilicon/HiKey960/HiKey960Dxe/HiKey960Dxe.c
&gt; +++ b/Platform/Hisilicon/HiKey960/HiKey960Dxe/HiKey960Dxe.c
&gt; @@ -30,10 +30,16 @@
&gt;  #include <library printlib.h="">
&gt;  #include <library serialportlib.h="">
&gt;  #include <library timerlib.h="">
&gt; +#include <library uefibootmanagerlib.h="">
&gt;  #include <library uefibootservicestablelib.h="">
&gt; +#include <library uefilib.h="">
&gt;  #include <library uefiruntimeservicestablelib.h="">
&gt;
&gt; +#include <protocol devicepath.h="">
&gt; +#include <protocol devicepathfromtext.h="">
&gt;  #include <protocol embeddedgpio.h="">
&gt; +#include <protocol loadedimage.h="">
&gt; +#include <protocol platformbootmanager.h="">
&gt;  #include <protocol platformvirtualkeyboard.h="">
&gt;
&gt;  #define ADC_ADCIN0                       0
&gt; @@ -86,6 +92,10 @@
&gt;
&gt;  #define DETECT_SW_FASTBOOT               68        // GPIO8_4
&gt;
&gt; +#define HIKEY960_BOOT_OPTION_NUM         4
&gt; +
&gt; +#define GRUB_FILE_NAME                   L"\\EFI\\BOOT\\GRUBAA64.EFI"
&gt; +
&gt;  typedef struct {
&gt;    UINT64        Magic;
&gt;    UINT64        Data;
&gt; @@ -359,6 +369,168 @@ OnEndOfDxe (
&gt;    }
&gt;  }
&gt;
&gt; +STATIC
&gt; +EFI_STATUS
&gt; +GetPlatformBootOptionsAndKeys (
&gt; +  OUT UINTN                              *BootCount,
&gt; +  OUT EFI_BOOT_MANAGER_LOAD_OPTION       **BootOptions,
&gt; +  OUT EFI_INPUT_KEY                      **BootKeys
&gt; +  )
&gt; +{
&gt; +  EFI_DEVICE_PATH                        *DevicePath;
&gt; +  EFI_DEVICE_PATH                        *FileDevicePath;
&gt; +  EFI_DEVICE_PATH_FROM_TEXT_PROTOCOL     *DPProtocol;
&gt; +  FILEPATH_DEVICE_PATH                   *FilePath;
&gt; +  EFI_GUID                               *FileGuid;
&gt; +  EFI_LOADED_IMAGE_PROTOCOL              *LoadedImage;
&gt; +  MEDIA_FW_VOL_FILEPATH_DEVICE_PATH      FileNode;
&gt; +  CHAR16                                 *PathStr;
&gt; +  EFI_STATUS                             Status;
&gt; +  UINTN                                  Size;
&gt; +
&gt; +  if ((BootCount == NULL) || (BootOptions == NULL) || (BootKeys == NULL)) {
&gt; +    return EFI_INVALID_PARAMETER;
&gt; +  }

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.

&gt; +  Size = sizeof (EFI_BOOT_MANAGER_LOAD_OPTION) * HIKEY960_BOOT_OPTION_NUM;
&gt; +  *BootOptions = (EFI_BOOT_MANAGER_LOAD_OPTION *)AllocateZeroPool (Size);
&gt; +  if (*BootOptions == NULL) {
&gt; +    DEBUG ((DEBUG_ERROR, "Failed to allocate memory for BootOptions\n"));
&gt; +    return EFI_BUFFER_TOO_SMALL;
&gt; +  }

(1) This should return EFI_OUT_OF_RESOURCES, as specified at the
protocol level.

&gt; +  Size = sizeof (EFI_INPUT_KEY) * HIKEY960_BOOT_OPTION_NUM;
&gt; +  *BootKeys = (EFI_INPUT_KEY *)AllocateZeroPool (Size);
&gt; +  if (*BootKeys == NULL) {
&gt; +    DEBUG ((DEBUG_ERROR, "Failed to allocate memory for BootKeys\n"));
&gt; +    FreePool (*BootOptions);
&gt; +    return EFI_BUFFER_TOO_SMALL;
&gt; +  }

(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.

&gt; +
&gt; +  PathStr = (CHAR16 *)PcdGetPtr (PcdSdBootDevicePath);
&gt; +  ASSERT (PathStr != NULL);
&gt; +  Status = gBS-&gt;LocateProtocol (&amp;gEfiDevicePathFromTextProtocolGuid,
&gt; +                  NULL,
&gt; +                  (VOID **)&amp;DPProtocol
&gt; +                  );
&gt; +  ASSERT_EFI_ERROR (Status);
&gt; +  DevicePath = (EFI_DEVICE_PATH *)DPProtocol-&gt;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
<https: show_bug.cgi?id="940" bugzilla.tianocore.org=""> which I just
filed.)

&gt; +  ASSERT (DevicePath != NULL);
&gt; +  Status = EfiBootManagerInitializeLoadOption (
&gt; +             &amp;(*BootOptions)[0],
&gt; +             LoadOptionNumberUnassigned,
&gt; +             LoadOptionTypeBoot,
&gt; +             LOAD_OPTION_ACTIVE,
&gt; +             L"Boot from SD",
&gt; +             DevicePath,
&gt; +             NULL,
&gt; +             0
&gt; +             );
&gt; +  ASSERT_EFI_ERROR (Status);
&gt; +  FreePool (DevicePath);

Yup, good choice, EfiBootManagerInitializeLoadOption() makes deep
copies.

&gt; +
&gt; +  PathStr = (CHAR16 *)PcdGetPtr (PcdAndroidBootDevicePath);
&gt; +  ASSERT (PathStr != NULL);
&gt; +  Status = gBS-&gt;LocateProtocol (&amp;gEfiDevicePathFromTextProtocolGuid,
&gt; +                  NULL,
&gt; +                  (VOID **)&amp;DPProtocol
&gt; +                  );
&gt; +  ASSERT_EFI_ERROR (Status);
&gt; +  DevicePath = (EFI_DEVICE_PATH *)DPProtocol-&gt;ConvertTextToDevicePath (PathStr);
&gt; +  ASSERT (DevicePath != NULL);

(5) See (4).

&gt; +  Size = StrSize (GRUB_FILE_NAME);
&gt; +  FileDevicePath = AllocatePool (Size + SIZE_OF_FILEPATH_DEVICE_PATH + END_DEVICE_PATH_LENGTH);
&gt; +  if (FileDevicePath != NULL) {
&gt; +    FilePath = (FILEPATH_DEVICE_PATH *) FileDevicePath;
&gt; +    FilePath-&gt;Header.Type    = MEDIA_DEVICE_PATH;
&gt; +    FilePath-&gt;Header.SubType = MEDIA_FILEPATH_DP;
&gt; +    CopyMem (&amp;FilePath-&gt;PathName, GRUB_FILE_NAME, Size);
&gt; +    SetDevicePathNodeLength (&amp;FilePath-&gt;Header, Size + SIZE_OF_FILEPATH_DEVICE_PATH);
&gt; +    SetDevicePathEndNode (NextDevicePathNode (&amp;FilePath-&gt;Header));
&gt; +
&gt; +    DevicePath = AppendDevicePath (DevicePath, FileDevicePath);
&gt; +    FreePool (FileDevicePath);
&gt; +  }

(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?

&gt; +  Status = EfiBootManagerInitializeLoadOption (
&gt; +             &amp;(*BootOptions)[1],
&gt; +             LoadOptionNumberUnassigned,
&gt; +             LoadOptionTypeBoot,
&gt; +             LOAD_OPTION_ACTIVE,
&gt; +             L"Grub",
&gt; +             DevicePath,
&gt; +             NULL,
&gt; +             0
&gt; +             );
&gt; +  ASSERT_EFI_ERROR (Status);
&gt; +  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.

&gt; +
&gt; +  FileGuid = PcdGetPtr (PcdAndroidBootFile);
&gt; +  ASSERT (FileGuid != NULL);
&gt; +  Status = gBS-&gt;HandleProtocol (
&gt; +                  gImageHandle,
&gt; +                  &amp;gEfiLoadedImageProtocolGuid,
&gt; +                  (VOID **) &amp;LoadedImage
&gt; +                  );
&gt; +  ASSERT_EFI_ERROR (Status);
&gt; +  EfiInitializeFwVolDevicepathNode (&amp;FileNode, FileGuid);
&gt; +  DevicePath = DevicePathFromHandle (LoadedImage-&gt;DeviceHandle);
&gt; +  ASSERT (DevicePath != NULL);
&gt; +  DevicePath = AppendDevicePathNode (
&gt; +                 DevicePath,
&gt; +                 (EFI_DEVICE_PATH_PROTOCOL *) &amp;FileNode
&gt; +                 );
&gt; +  ASSERT (DevicePath != NULL);
&gt; +  Status = EfiBootManagerInitializeLoadOption (
&gt; +             &amp;(*BootOptions)[2],
&gt; +             LoadOptionNumberUnassigned,
&gt; +             LoadOptionTypeBoot,
&gt; +             LOAD_OPTION_ACTIVE,
&gt; +             L"Android Boot",
&gt; +             DevicePath,
&gt; +             NULL,
&gt; +             0
&gt; +             );
&gt; +  ASSERT_EFI_ERROR (Status);
&gt; +  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.

&gt; +
&gt; +  FileGuid = PcdGetPtr (PcdAndroidFastbootFile);
&gt; +  ASSERT (FileGuid != NULL);
&gt; +  Status = gBS-&gt;HandleProtocol (
&gt; +                  gImageHandle,
&gt; +                  &amp;gEfiLoadedImageProtocolGuid,
&gt; +                  (VOID **) &amp;LoadedImage
&gt; +                  );
&gt; +  ASSERT_EFI_ERROR (Status);
&gt; +  EfiInitializeFwVolDevicepathNode (&amp;FileNode, FileGuid);
&gt; +  DevicePath = DevicePathFromHandle (LoadedImage-&gt;DeviceHandle);
&gt; +  ASSERT (DevicePath != NULL);
&gt; +  DevicePath = AppendDevicePathNode (
&gt; +                 DevicePath,
&gt; +                 (EFI_DEVICE_PATH_PROTOCOL *) &amp;FileNode
&gt; +                 );
&gt; +  ASSERT (DevicePath != NULL);
&gt; +  Status = EfiBootManagerInitializeLoadOption (
&gt; +             &amp;(*BootOptions)[3],
&gt; +             LoadOptionNumberUnassigned,
&gt; +             LoadOptionTypeBoot,
&gt; +             LOAD_OPTION_ACTIVE,
&gt; +             L"Android Fastboot",
&gt; +             DevicePath,
&gt; +             NULL,
&gt; +             0
&gt; +             );
&gt; +  ASSERT_EFI_ERROR (Status);
&gt; +  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

&gt; +  (*BootKeys)[3].ScanCode = SCAN_NULL;
&gt; +  (*BootKeys)[3].UnicodeChar = 'f';
&gt; +
&gt; +  *BootCount = 4;
&gt; +
&gt; +  return EFI_SUCCESS;
&gt; +}
&gt; +
&gt; +PLATFORM_BOOT_MANAGER_PROTOCOL mPlatformBootManager = {
&gt; +  GetPlatformBootOptionsAndKeys
&gt; +};
&gt; +
&gt;  EFI_STATUS
&gt;  EFIAPI
&gt;  VirtualKeyboardRegister (
&gt; @@ -487,5 +659,15 @@ HiKey960EntryPoint (
&gt;                    EFI_NATIVE_INTERFACE,
&gt;                    &amp;mVirtualKeyboard
&gt;                    );
&gt; +  if (EFI_ERROR (Status)) {
&gt; +    return Status;
&gt; +  }
&gt; +
&gt; +  Status = gBS-&gt;InstallProtocolInterface (
&gt; +                  &amp;ImageHandle,
&gt; +                  &amp;gPlatformBootManagerProtocolGuid,
&gt; +                  EFI_NATIVE_INTERFACE,
&gt; +                  &amp;mPlatformBootManager
&gt; +                  );
&gt;    return Status;
&gt;  }
&gt; diff --git a/Platform/Hisilicon/HiKey960/HiKey960Dxe/HiKey960Dxe.inf b/Platform/Hisilicon/HiKey960/HiKey960Dxe/HiKey960Dxe.inf
&gt; index cc517656b340..5adedd79d3e8 100644
&gt; --- a/Platform/Hisilicon/HiKey960/HiKey960Dxe/HiKey960Dxe.inf
&gt; +++ b/Platform/Hisilicon/HiKey960/HiKey960Dxe/HiKey960Dxe.inf
&gt; @@ -25,6 +25,7 @@ [Packages]
&gt;    EmbeddedPkg/EmbeddedPkg.dec
&gt;    MdePkg/MdePkg.dec
&gt;    MdeModulePkg/MdeModulePkg.dec
&gt; +  Platform/Hisilicon/HiKey960/HiKey960.dec
&gt;
&gt;  [LibraryClasses]
&gt;    BaseMemoryLib
&gt; @@ -37,15 +38,25 @@ [LibraryClasses]
&gt;    PrintLib
&gt;    SerialPortLib
&gt;    TimerLib
&gt; +  UefiBootManagerLib
&gt;    UefiBootServicesTableLib
&gt;    UefiDriverEntryPoint
&gt;    UefiLib
&gt;    UefiRuntimeServicesTableLib
&gt;
&gt;  [Protocols]
&gt; +  gEfiDevicePathFromTextProtocolGuid
&gt; +  gEfiLoadedImageProtocolGuid
&gt;    gEmbeddedGpioProtocolGuid
&gt; +  gPlatformBootManagerProtocolGuid
&gt;    gPlatformVirtualKeyboardProtocolGuid
&gt;
&gt; +[Pcd]
&gt; +  gHiKey960TokenSpaceGuid.PcdAndroidBootDevicePath
&gt; +  gHiKey960TokenSpaceGuid.PcdAndroidBootFile
&gt; +  gHiKey960TokenSpaceGuid.PcdAndroidFastbootFile
&gt; +  gHiKey960TokenSpaceGuid.PcdSdBootDevicePath
&gt; +
&gt;  [Guids]
&gt;    gEfiEndOfDxeEventGroupGuid
&gt;    gEfiFileInfoGuid
&gt;

</https:></protocol></protocol></protocol></protocol></protocol></protocol></library></library></library></library></library></library></library></haojian.zhuang@linaro.org></ard.biesheuvel@linaro.org></leif.lindholm@linaro.org></lersek@redhat.com></lersek@redhat.com>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
diff mbox series

Patch

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 <Library/PrintLib.h>
 #include <Library/SerialPortLib.h>
 #include <Library/TimerLib.h>
+#include <Library/UefiBootManagerLib.h>
 #include <Library/UefiBootServicesTableLib.h>
+#include <Library/UefiLib.h>
 #include <Library/UefiRuntimeServicesTableLib.h>
 
+#include <Protocol/DevicePath.h>
+#include <Protocol/DevicePathFromText.h>
 #include <Protocol/EmbeddedGpio.h>
+#include <Protocol/LoadedImage.h>
+#include <Protocol/PlatformBootManager.h>
 #include <Protocol/PlatformVirtualKeyboard.h>
 
 #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;
+  }
+  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;
+  }
+  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;
+  }
+
+  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);
+  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);
+
+  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);
+  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);
+  }
+  Status = EfiBootManagerInitializeLoadOption (
+             &(*BootOptions)[1],
+             LoadOptionNumberUnassigned,
+             LoadOptionTypeBoot,
+             LOAD_OPTION_ACTIVE,
+             L"Grub",
+             DevicePath,
+             NULL,
+             0
+             );
+  ASSERT_EFI_ERROR (Status);
+  FreePool (DevicePath);
+
+  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);
+
+  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);
+  (*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