[PATCH edk2-platforms v4 0/2] add platform boot options

public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed

* [PATCH edk2-platforms v4 0/2] add platform boot options
@ 2018-04-23  6:22 Haojian Zhuang
  2018-04-23  6:22 ` [PATCH edk2-platforms v4 1/2] Platform/HiKey960: register predefined " Haojian Zhuang
  2018-04-23  6:22 ` [PATCH edk2-platforms v4 2/2] Platform/HiKey: create 4 " Haojian Zhuang
  0 siblings, 2 replies; 6+ messages in thread
From: Haojian Zhuang @ 2018-04-23  6:22 UTC (permalink / raw)
  To: edk2-devel; +Cc: Haojian Zhuang

Changelog:
v4:
  * Add BootCount parameter in the interface.
v3:
  * Move in initilization of boot entry.
  * Update the name of interface in platform boot manager protocol.
v2:
  * Update with platform boot manager protocol.

Haojian Zhuang (2):
  Platform/HiKey960: register predefined boot options
  Platform/HiKey: create 4 boot options

 Platform/Hisilicon/HiKey/HiKey.dec                 |   8 +-
 Platform/Hisilicon/HiKey/HiKey.dsc                 |   7 +
 Platform/Hisilicon/HiKey/HiKeyDxe/HiKeyDxe.c       | 181 ++++++++++++++++++++
 Platform/Hisilicon/HiKey/HiKeyDxe/HiKeyDxe.inf     |  11 ++
 .../{HiKey/HiKey.dec => HiKey960/HiKey960.dec}     |  17 +-
 Platform/Hisilicon/HiKey960/HiKey960.dsc           |   6 +
 .../Hisilicon/HiKey960/HiKey960Dxe/HiKey960Dxe.c   | 182 +++++++++++++++++++++
 .../Hisilicon/HiKey960/HiKey960Dxe/HiKey960Dxe.inf |  11 ++
 8 files changed, 411 insertions(+), 12 deletions(-)
 copy Platform/Hisilicon/{HiKey/HiKey.dec => HiKey960/HiKey960.dec} (56%)

-- 
2.7.4



^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH edk2-platforms v4 1/2] Platform/HiKey960: register predefined boot options
  2018-04-23  6:22 [PATCH edk2-platforms v4 0/2] add platform boot options Haojian Zhuang
@ 2018-04-23  6:22 ` Haojian Zhuang
  2018-04-23 21:24   ` Laszlo Ersek
  2018-04-23  6:22 ` [PATCH edk2-platforms v4 2/2] Platform/HiKey: create 4 " Haojian Zhuang
  1 sibling, 1 reply; 6+ messages in thread
From: Haojian Zhuang @ 2018-04-23  6:22 UTC (permalink / raw)
  To: edk2-devel; +Cc: Haojian Zhuang, Laszlo Ersek, Leif Lindholm, Ard Biesheuvel

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

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



^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH edk2-platforms v4 1/2] Platform/HiKey960: register predefined boot options
  2018-04-23  6:22 ` [PATCH edk2-platforms v4 1/2] Platform/HiKey960: register predefined " Haojian Zhuang
@ 2018-04-23 21:24   ` Laszlo Ersek
  2018-04-25  4:59     ` Haojian Zhuang
  0 siblings, 1 reply; 6+ messages in thread
From: Laszlo Ersek @ 2018-04-23 21:24 UTC (permalink / raw)
  To: Haojian Zhuang, edk2-devel; +Cc: Leif Lindholm, Ard Biesheuvel

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
>



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH edk2-platforms v4 1/2] Platform/HiKey960: register predefined boot options
  2018-04-23 21:24   ` Laszlo Ersek
@ 2018-04-25  4:59     ` Haojian Zhuang
  0 siblings, 0 replies; 6+ messages in thread
From: Haojian Zhuang @ 2018-04-25  4:59 UTC (permalink / raw)
  To: Laszlo Ersek, Haojian Zhuang, edk2-devel@lists.01.org
  Cc: Leif Lindholm, Ard Biesheuvel

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>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH edk2-platforms v4 2/2] Platform/HiKey: create 4 boot options
  2018-04-23  6:22 [PATCH edk2-platforms v4 0/2] add platform boot options Haojian Zhuang
  2018-04-23  6:22 ` [PATCH edk2-platforms v4 1/2] Platform/HiKey960: register predefined " Haojian Zhuang
@ 2018-04-23  6:22 ` Haojian Zhuang
  2018-04-23 21:29   ` Laszlo Ersek
  1 sibling, 1 reply; 6+ messages in thread
From: Haojian Zhuang @ 2018-04-23  6:22 UTC (permalink / raw)
  To: edk2-devel; +Cc: Haojian Zhuang, Laszlo Ersek, Leif Lindholm, Ard Biesheuvel

Create 4 predefined boot options for HiKey. 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/HiKey/HiKey.dec             |   8 +-
 Platform/Hisilicon/HiKey/HiKey.dsc             |   7 +
 Platform/Hisilicon/HiKey/HiKeyDxe/HiKeyDxe.c   | 181 +++++++++++++++++++++++++
 Platform/Hisilicon/HiKey/HiKeyDxe/HiKeyDxe.inf |  11 ++
 4 files changed, 204 insertions(+), 3 deletions(-)

diff --git a/Platform/Hisilicon/HiKey/HiKey.dec b/Platform/Hisilicon/HiKey/HiKey.dec
index 537138eb45a1..bb94c5cab13f 100644
--- a/Platform/Hisilicon/HiKey/HiKey.dec
+++ b/Platform/Hisilicon/HiKey/HiKey.dec
@@ -1,5 +1,5 @@
 #
-#  Copyright (c) 2014-2017, Linaro Limited. All rights reserved.
+#  Copyright (c) 2014-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
@@ -32,5 +32,7 @@ [Guids.common]
   gHiKeyTokenSpaceGuid          =  { 0x91148425, 0xcdd2, 0x4830, { 0x8b, 0xd0, 0xc6, 0x1c, 0x6d, 0xea, 0x36, 0x21 } }
 
 [PcdsFixedAtBuild.common]
-  gHiKeyTokenSpaceGuid.PcdAndroidFastbootNvmDevicePath|L""|VOID*|0x00000001
-  gHiKeyTokenSpaceGuid.PcdArmFastbootFlashLimit|L""|VOID*|0x00000002
+  gHiKeyTokenSpaceGuid.PcdAndroidBootDevicePath|L""|VOID*|0x00000001
+  gHiKeyTokenSpaceGuid.PcdAndroidBootFile|{ 0x36, 0x8b, 0x73, 0x3a, 0xc5, 0xb9, 0x63, 0x47, 0xab, 0xbd, 0x6c, 0xbd, 0x4b, 0x25, 0xf9, 0xff }|VOID*|0x00000002
+  gHiKeyTokenSpaceGuid.PcdAndroidFastbootFile|{ 0x2a, 0x50, 0x88, 0x95, 0x70, 0x53, 0xe3, 0x11, 0x86, 0x31, 0xd7, 0xc5, 0x95, 0x13, 0x64, 0xc8 }|VOID*|0x00000003
+  gHiKeyTokenSpaceGuid.PcdSdBootDevicePath|L""|VOID*|0x00000004
diff --git a/Platform/Hisilicon/HiKey/HiKey.dsc b/Platform/Hisilicon/HiKey/HiKey.dsc
index 83dd68a820b1..5b5dc48a693a 100644
--- a/Platform/Hisilicon/HiKey/HiKey.dsc
+++ b/Platform/Hisilicon/HiKey/HiKey.dsc
@@ -138,6 +138,13 @@ [PcdsFixedAtBuild.common]
   gEmbeddedTokenSpaceGuid.PcdAndroidFastbootUsbVendorId|0x18d1
   gEmbeddedTokenSpaceGuid.PcdAndroidFastbootUsbProductId|0xd00d
 
+  #
+  # Android Loader
+  #
+  gHiKeyTokenSpaceGuid.PcdAndroidBootDevicePath|L"VenHw(0D51905B-B77E-452A-A2C0-ECA0CC8D514A,00D023F70000000000)/eMMC(0x0)/Ctrl(0x0)/HD(6,GPT,5C0F213C-17E1-4149-88C8-8B50FB4EC70E,0x7000,0x20000)"
+  gHiKeyTokenSpaceGuid.PcdSdBootDevicePath|L"VenHw(0D51905B-B77E-452A-A2C0-ECA0CC8D514A,00E023F70000000000)/SD(0x0)"
+
+
 ################################################################################
 #
 # Components Section - list of all EDK II Modules needed by this Platform
diff --git a/Platform/Hisilicon/HiKey/HiKeyDxe/HiKeyDxe.c b/Platform/Hisilicon/HiKey/HiKeyDxe/HiKeyDxe.c
index 65e800116b76..6273bc0b213a 100644
--- a/Platform/Hisilicon/HiKey/HiKeyDxe/HiKeyDxe.c
+++ b/Platform/Hisilicon/HiKey/HiKeyDxe/HiKeyDxe.c
@@ -18,12 +18,18 @@
 #include <Library/DevicePathLib.h>
 #include <Library/IoLib.h>
 #include <Library/MemoryAllocationLib.h>
+#include <Library/PcdLib.h>
 #include <Library/PrintLib.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>
 
 #include <Hi6220.h>
@@ -42,6 +48,9 @@
 #define ADB_REBOOT_BOOTLOADER            0x77665500
 #define ADB_REBOOT_NONE                  0x77665501
 
+#define HIKEY_BOOT_OPTION_NUM            4
+
+#define GRUB_FILE_NAME                   L"\\EFI\\BOOT\\GRUBAA64.EFI"
 
 typedef struct {
   UINT64        Magic;
@@ -121,6 +130,168 @@ HiKeyInitPeripherals (
   return EFI_SUCCESS;
 }
 
+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) * HIKEY_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) * HIKEY_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 (
@@ -225,5 +396,15 @@ HiKeyEntryPoint (
                   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/HiKey/HiKeyDxe/HiKeyDxe.inf b/Platform/Hisilicon/HiKey/HiKeyDxe/HiKeyDxe.inf
index 702fdb1eebf0..e97afab8785a 100644
--- a/Platform/Hisilicon/HiKey/HiKeyDxe/HiKeyDxe.inf
+++ b/Platform/Hisilicon/HiKey/HiKeyDxe/HiKeyDxe.inf
@@ -27,6 +27,7 @@ [Packages]
   EmbeddedPkg/EmbeddedPkg.dec
   MdePkg/MdePkg.dec
   MdeModulePkg/MdeModulePkg.dec
+  Platform/Hisilicon/HiKey/HiKey.dec
 
 [LibraryClasses]
   BaseMemoryLib
@@ -40,15 +41,25 @@ [LibraryClasses]
   PrintLib
   SerialPortLib
   TimerLib
+  UefiBootManagerLib
   UefiBootServicesTableLib
   UefiRuntimeServicesTableLib
   UefiLib
   UefiDriverEntryPoint
 
 [Protocols]
+  gEfiDevicePathFromTextProtocolGuid
+  gEfiLoadedImageProtocolGuid
   gEmbeddedGpioProtocolGuid
+  gPlatformBootManagerProtocolGuid
   gPlatformVirtualKeyboardProtocolGuid
 
+[Pcd]
+  gHiKeyTokenSpaceGuid.PcdAndroidBootDevicePath
+  gHiKeyTokenSpaceGuid.PcdAndroidBootFile
+  gHiKeyTokenSpaceGuid.PcdAndroidFastbootFile
+  gHiKeyTokenSpaceGuid.PcdSdBootDevicePath
+
 [Guids]
   gEfiEndOfDxeEventGroupGuid
   gEfiFileInfoGuid
-- 
2.7.4



^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH edk2-platforms v4 2/2] Platform/HiKey: create 4 boot options
  2018-04-23  6:22 ` [PATCH edk2-platforms v4 2/2] Platform/HiKey: create 4 " Haojian Zhuang
@ 2018-04-23 21:29   ` Laszlo Ersek
  0 siblings, 0 replies; 6+ messages in thread
From: Laszlo Ersek @ 2018-04-23 21:29 UTC (permalink / raw)
  To: Haojian Zhuang, edk2-devel; +Cc: Leif Lindholm, Ard Biesheuvel

On 04/23/18 08:22, Haojian Zhuang wrote:
> Create 4 predefined boot options for HiKey. 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/HiKey/HiKey.dec             |   8 +-
>  Platform/Hisilicon/HiKey/HiKey.dsc             |   7 +
>  Platform/Hisilicon/HiKey/HiKeyDxe/HiKeyDxe.c   | 181 +++++++++++++++++++++++++
>  Platform/Hisilicon/HiKey/HiKeyDxe/HiKeyDxe.inf |  11 ++
>  4 files changed, 204 insertions(+), 3 deletions(-)

>From a quick skim, this looks very similar to patch #1.

I'm not familiar with the edk2-platforms tree. Ard and Leif might be
able to suggest a way to centralize this code between HiKey and HiKey960.

Thanks,
Laszlo


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2018-04-25  4:59 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-04-23  6:22 [PATCH edk2-platforms v4 0/2] add platform boot options Haojian Zhuang
2018-04-23  6:22 ` [PATCH edk2-platforms v4 1/2] Platform/HiKey960: register predefined " Haojian Zhuang
2018-04-23 21:24   ` Laszlo Ersek
2018-04-25  4:59     ` Haojian Zhuang
2018-04-23  6:22 ` [PATCH edk2-platforms v4 2/2] Platform/HiKey: create 4 " Haojian Zhuang
2018-04-23 21:29   ` Laszlo Ersek

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox