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

[Laszlo Ersek <lersek@redhat.com>]

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
>