Re: [PATCH v2 1/1] ArmPkg/PlatformBootManagerLib: load boot options from platform

[Haojian Zhuang <haojian.zhuang@linaro.org>]

Hi Laszlo,

I'll fix them in v3.

Best Regards
Haojian

________________________________________
From: Laszlo Ersek <lersek@redhat.com>
Sent: Tuesday, April 17, 2018 9:02 AM
To: Haojian Zhuang
Cc: edk2-devel@lists.01.org; Ard Biesheuvel; Leif Lindholm (Linaro address)
Subject: Re: [edk2] [PATCH v2 1/1] ArmPkg/PlatformBootManagerLib: load boot options from platform

On 04/17/18 07:11, Haojian Zhuang wrote:
> Platform drivers sets up the array of predefined boot options.
> ArmPkg/PlatformBootManager converts the array to boot options.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>
> ---
>  ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c | 81 ++++++++++++++++++++++
>  .../PlatformBootManagerLib.inf                     |  2 +
>  EmbeddedPkg/EmbeddedPkg.dec                        |  1 +
>  EmbeddedPkg/Include/Protocol/PlatformBootManager.h | 39 +++++++++++

(1) Please split this patch in two, minimally; one per package.

>  4 files changed, 123 insertions(+)
>  create mode 100644 EmbeddedPkg/Include/Protocol/PlatformBootManager.h
>
> diff --git a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
> index 61ab61ccc780..68a06f5855d8 100644
> --- a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
> +++ b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
> @@ -30,6 +30,7 @@
>  #include <Protocol/LoadedImage.h>
>  #include <Protocol/PciIo.h>
>  #include <Protocol/PciRootBridgeIo.h>
> +#include <Protocol/PlatformBootManager.h>
>  #include <Guid/EventGroup.h>
>  #include <Guid/TtyTerm.h>
>
> @@ -392,6 +393,84 @@ PlatformRegisterFvBootOption (
>
>  STATIC
>  VOID
> +GetPlatformOptions (
> +  VOID
> +  )
> +{
> +  EFI_STATUS                      Status;
> +  EFI_BOOT_MANAGER_LOAD_OPTION    NewOption;
> +  EFI_BOOT_MANAGER_LOAD_OPTION    *CurrentBootOptions;
> +  EFI_BOOT_MANAGER_LOAD_OPTION    *PlatformOption;
> +  EFI_BOOT_MANAGER_LOAD_OPTION    *PlatformBootOptionArray;
> +  EFI_INPUT_KEY                   *PlatformKeyArray;
> +  EFI_INPUT_KEY                   *PlatformKey;
> +  PLATFORM_BOOT_MANAGER_PROTOCOL  *PlatformBootManager;
> +  UINTN                           CurrentBootOptionCount;
> +  UINTN                           OptionIndex;
> +
> +  Status = gBS->LocateProtocol (&gPlatformBootManagerProtocolGuid, NULL,
> +                  (VOID **)&PlatformBootManager);
> +  if (!EFI_ERROR (Status)) {

(2) It would simplify nesting if you returned from the function at once
on error.

> +    if (PlatformBootManager->Register) {

(3) This check should be unnecessary. I haven't heard of any protocol
where a function pointer member was allowed to be absent. Members may
return constant EFI_UNSUPPORTED, but they always exist.

> +      // Last entries of PlatformBootOptionArray and PlatformKeyArray are empty.

(4) Comment style is not valid.

> +      Status = PlatformBootManager->Register (
> +                 &PlatformBootOptionArray,
> +                 &PlatformKeyArray
> +                 );
> +      if (!EFI_ERROR (Status)) {

(5) While this kind of nesting / error handling / resource release is
entirely valid, in edk2 we prefer error handling labels and "goto"
statements. Edk2 identifiers are incredibly long, compared to common C
source code, so columns are a premium. We should avoid unnecessary
nesting if we can.

> +        PlatformOption = PlatformBootOptionArray;
> +        PlatformKey = PlatformKeyArray;
> +        while (PlatformOption->Description != NULL) {

OK, this seems to be a valid check. Description should always be
non-NULL for actual entries (if there is nothing to say, it should be an
empty string).

> +          Status = EfiBootManagerInitializeLoadOption (
> +                     &NewOption,
> +                     LoadOptionNumberUnassigned,
> +                     LoadOptionTypeBoot,
> +                     LOAD_OPTION_ACTIVE,
> +                     PlatformOption->Description,
> +                     PlatformOption->FilePath,
> +                     NULL,
> +                     0
> +                     );

I see no reason for *not* passing in PlatformOption->OptionalData and
PlatformOption->OptionalDataSize as well.

(6) In fact, there's an argument to be made that this entire
initialization should be done by the "platform boot manager protocol"
itself -- each element in the returned array of platform boot options
should already be initialized with EfiBootManagerInitializeLoadOption()
inside the protocol, and the loop here should use (even modify -- see
below) those elements in-place. The "NewOption" variable should not be
necessary.

> +          ASSERT_EFI_ERROR (Status);
> +          CurrentBootOptions = EfiBootManagerGetLoadOptions (
> +                                 &CurrentBootOptionCount, LoadOptionTypeBoot
> +                                 );

(7) I don't think it's a good idea to call
EfiBootManagerGetLoadOptions() within the loop; it's an expensive
operation. It should be good enough to call it once before the loop --
we expect the "platform boot manager protocol" to provide a unique list
of options (such that those don't overlap between each other), so it's
enough to check every one of them against the initial list, within the loop.

I do see that you use CurrentBootOptionCount below, for hotkey
association. I don't think that's correct either (more on that below). I
really think EfiBootManagerGetLoadOptions() should be moved out of the loop.

> +          OptionIndex = EfiBootManagerFindLoadOption (
> +                          &NewOption, CurrentBootOptions, CurrentBootOptionCount
> +                          );
> +          if (OptionIndex == -1) {
> +            // Append the BootLoadOption

(8) Comment style.

> +            Status = EfiBootManagerAddLoadOptionVariable (&NewOption, MAX_UINTN);
> +            ASSERT_EFI_ERROR (Status);
> +          }

(9) OK, so here's what I suggest. If the option is found in the array,
then CurrentBootOptions[OptionIndex].OptionNumber gives the #### that
you need for hotkey association.

Whereas, if the option is *not* found in the original array, then you
add it -- and then EfiBootManagerAddLoadOptionVariable() *modifies*
"NewOption.OptionNumber", from "LoadOptionNumberUnassigned" to the
actual #### assigned. Therefore, after the addition,
"NewOption.OptionNumber" gives you the #### that you should use for
hotkey association. I suggest using a local variable for that.

> +
> +          // If UnicodeChar isn't empty, there's a hot key.

(10) comment style...

> +          if (PlatformKey->UnicodeChar) {
> +            // The index of Boot Options counts from 1.
> +            // The last index equals to the count of total Boot Options.

(11) This comment is wrong and should be removed.

> +            Status = EfiBootManagerAddKeyOptionVariable (
> +                       NULL, CurrentBootOptionCount, 0, PlatformKey, NULL
> +                       );

(12) Beyond referring back to (9): the arguments aren't correctly
indented, IMO. Please use one of the "sanctioned" styles:
<http://mid.mail-archive.com/8f48b191-a74a-d7c1-0103-c15a6505549a@redhat.com>.

(13) Also, Status is never checked after the call.

> +          }
> +
> +          EfiBootManagerFreeLoadOption (&NewOption);
> +          EfiBootManagerFreeLoadOptions (
> +            CurrentBootOptions, CurrentBootOptionCount
> +            );

(14) So both of these should be updated -- the first call falls away due
to (6), while the second should be moved after the loop due to (7).

> +
> +          PlatformOption++;
> +          PlatformKey++;
> +        }
> +        FreePool (PlatformBootOptionArray);

(15) I think it's insufficient to free "PlatformBootOptionArray" like
this, especially given my comment (6). Each element of the array should
be uninitialized first with EfiBootManagerFreeLoadOption(), to match the
EfiBootManagerInitializeLoadOption() call that happens within the
protocol member.

> +        FreePool (PlatformKeyArray);
> +      }
> +    }
> +  }
> +
> +}
> +
> +STATIC
> +VOID
>  PlatformRegisterOptionsAndKeys (
>    VOID
>    )
> @@ -402,6 +481,8 @@ PlatformRegisterOptionsAndKeys (
>    EFI_INPUT_KEY                Esc;
>    EFI_BOOT_MANAGER_LOAD_OPTION BootOption;
>
> +  GetPlatformOptions ();
> +
>    //
>    // Register ENTER as CONTINUE key
>    //
> diff --git a/ArmPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf b/ArmPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
> index 71f1d04a87ee..e8cbb10dabdd 100644
> --- a/ArmPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
> +++ b/ArmPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
> @@ -35,6 +35,7 @@ [Sources]
>    PlatformBm.c
>
>  [Packages]
> +  EmbeddedPkg/EmbeddedPkg.dec
>    MdeModulePkg/MdeModulePkg.dec
>    MdePkg/MdePkg.dec
>    ShellPkg/ShellPkg.dec
> @@ -84,3 +85,4 @@ [Protocols]
>    gEfiPciRootBridgeIoProtocolGuid
>    gEfiSimpleFileSystemProtocolGuid
>    gEsrtManagementProtocolGuid
> +  gPlatformBootManagerProtocolGuid
> diff --git a/EmbeddedPkg/EmbeddedPkg.dec b/EmbeddedPkg/EmbeddedPkg.dec
> index eb135340b173..9cd50738363b 100644
> --- a/EmbeddedPkg/EmbeddedPkg.dec
> +++ b/EmbeddedPkg/EmbeddedPkg.dec
> @@ -81,6 +81,7 @@ [Protocols.common]
>    gPlatformGpioProtocolGuid = { 0x52ce9845, 0x5af4, 0x43e2, {0xba, 0xfd, 0x23, 0x08, 0x12, 0x54, 0x7a, 0xc2 }}
>    gAndroidBootImgProtocolGuid = { 0x9859bb19, 0x407c, 0x4f8b, {0xbc, 0xe1, 0xf8, 0xda, 0x65, 0x65, 0xf4, 0xa5 }}
>    gPlatformVirtualKeyboardProtocolGuid = { 0x0e3606d2, 0x1dc3, 0x4e6f, { 0xbe, 0x65, 0x39, 0x49, 0x82, 0xa2, 0x65, 0x47 }}
> +  gPlatformBootManagerProtocolGuid = { 0x7197c8a7, 0x6559, 0x4c93, { 0x93, 0xd5, 0x8a, 0x84, 0xf9, 0x88, 0x79, 0x8b }}
>
>  [Ppis]
>    gEdkiiEmbeddedGpioPpiGuid = { 0x21c3b115, 0x4e0b, 0x470c, { 0x85, 0xc7, 0xe1, 0x05, 0xa5, 0x75, 0xc9, 0x7b }}
> diff --git a/EmbeddedPkg/Include/Protocol/PlatformBootManager.h b/EmbeddedPkg/Include/Protocol/PlatformBootManager.h
> new file mode 100644
> index 000000000000..9335cb143585
> --- /dev/null
> +++ b/EmbeddedPkg/Include/Protocol/PlatformBootManager.h
> @@ -0,0 +1,39 @@
> +/** @file
> +
> +  Copyright (c) 2018, Linaro. All rights reserved.<BR>
> +
> +  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.
> +
> +**/
> +
> +#ifndef __PLATFORM_BOOT_MANAGER_PROTOCOL_H__
> +#define __PLATFORM_BOOT_MANAGER_PROTOCOL_H__
> +
> +//
> +// Protocol interface structure
> +//
> +typedef struct _PLATFORM_BOOT_MANAGER_PROTOCOL    PLATFORM_BOOT_MANAGER_PROTOCOL;
> +
> +//
> +// Function Prototypes
> +//
> +typedef
> +EFI_STATUS
> +(EFIAPI *REGISTER_PLATFORM_BOOT_MANAGER) (
> +  OUT EFI_BOOT_MANAGER_LOAD_OPTION       **BootOptions,
> +  OUT EFI_INPUT_KEY                      **BootKeys
> +  );

(16) Please add detailed documentation for this member type, including a
description of both output parameters, how they relate to each other,
their lifecyles, and the return values of the member.

(17) I think it also wouldn't hurt to rename this member to
GetPlatformBootOptionsAndKeys(), or something similar.

> +
> +struct _PLATFORM_BOOT_MANAGER_PROTOCOL {
> +  REGISTER_PLATFORM_BOOT_MANAGER         Register;
> +};

> +
> +extern EFI_GUID gPlatformBootManagerProtocolGuid;
> +
> +#endif /* __PLATFORM_BOOT_PROTOCOL_H__ */

(18) The comment is mismatched with the actual header guard macro.

>

Thanks
Laszlo