Re: [PATCH v3 2/2] ArmPkg/PlatformBootManagerLib: load platform boot options

[Laszlo Ersek <lersek@redhat.com>]

On 04/19/18 12:42, Haojian Zhuang wrote:
> Make platform driver to create predefined boot options and related
> hot keys.
>
> 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>
> ---
>  ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c | 83 ++++++++++++++++++++++
>  .../PlatformBootManagerLib.inf                     |  2 +
>  2 files changed, 85 insertions(+)
>
> diff --git a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
> index 61ab61ccc780..3a40592b58dd 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,86 @@ PlatformRegisterFvBootOption (
>
>  STATIC
>  VOID
> +GetPlatformOptions (
> +  VOID
> +  )
> +{
> +  EFI_STATUS                      Status;
> +  EFI_BOOT_MANAGER_LOAD_OPTION    *CurrentBootOptions;
> +  EFI_BOOT_MANAGER_LOAD_OPTION    *PlatformBootOptionArray;
> +  EFI_INPUT_KEY                   *PlatformKeyArray;
> +  PLATFORM_BOOT_MANAGER_PROTOCOL  *PlatformBootManager;
> +  UINTN                           CurrentBootOptionCount;
> +  UINTN                           OptionIndex;
> +  UINTN                           Index;
> +
> +  Status = gBS->LocateProtocol (&gPlatformBootManagerProtocolGuid, NULL,
> +                  (VOID **)&PlatformBootManager);
> +  if (EFI_ERROR (Status)) {
> +    return;
> +  }
> +  Status = PlatformBootManager->GetPlatformBootOptionsAndKeys (
> +             &PlatformBootOptionArray,
> +             &PlatformKeyArray
> +             );

The indentation is not correct. Please use:

  Status = PlatformBootManager->GetPlatformBootOptionsAndKeys (
                                  &PlatformBootOptionArray,
                                  &PlatformKeyArray
                                  );

(I don't insist that the patch be resubmitted just because of this.)

> +  if (EFI_ERROR (Status)) {
> +    return;
> +  }
> +  CurrentBootOptions = EfiBootManagerGetLoadOptions (
> +                         &CurrentBootOptionCount, LoadOptionTypeBoot
> +                         );

The coding style is not correct. Choose one of the folowing two:

- canonical:

  CurrentBootOptions = EfiBootManagerGetLoadOptions (
                         &CurrentBootOptionCount,
                         LoadOptionTypeBoot
                         );

- less then canonical, but still acceptable:

  CurrentBootOptions = EfiBootManagerGetLoadOptions (&CurrentBootOptionCount,
                         LoadOptionTypeBoot);

Anyway it's up to the ArmPkg maintainers to choose a preference; I shouldn't
comment more on indentation.

> +  OptionIndex = EfiBootManagerFindLoadOption (
> +                  &PlatformBootOptionArray[0],
> +                  CurrentBootOptions,
> +                  CurrentBootOptionCount
> +                  );

EfiBootManagerFindLoadOption() returns an INTN value (so that it can
return -1), but OptionIndex is of type UINTN. Is it your intent to store
MAX_UINTN if the retval is -1?

> +  if (OptionIndex == -1) {
> +    OptionIndex = 1;
> +  } else {
> +    OptionIndex = OptionIndex + 1;
> +  }
> +  Index = 0;
> +  //
> +  // Last entries of PlatformBootOptionArray and PlatformKeyArray are empty.
> +  //
> +  while (PlatformBootOptionArray[Index].Description != NULL) {
> +    if (OptionIndex == 1) {
> +      //
> +      // Append the BootLoadOption
> +      //
> +      Status = EfiBootManagerAddLoadOptionVariable (
> +                 &PlatformBootOptionArray[Index],
> +                 OptionIndex + Index
> +                 );
> +      ASSERT_EFI_ERROR (Status);
> +    }
> +    //
> +    // If UnicodeChar isn't empty, there's a hot key.
> +    //
> +    if (PlatformKeyArray[Index].UnicodeChar) {
> +      //
> +      // The index of Boot Options counts from 1.
> +      // The last index equals to the count of total Boot Options.
> +      //
> +      Status = EfiBootManagerAddKeyOptionVariable (
> +                 NULL, OptionIndex + Index, 0,
> +                 PlatformKeyArray[Index], NULL
> +                 );
> +      ASSERT_EFI_ERROR (Status);
> +    }
> +    Index++;
> +  }

I'm sorry, I don't understand the logic. It seems that you determine the
insertion point in the boot order by looking up the first
platform-specific boot option in the current option list. If there is no
match, you insert the platform-specific boot options at positions 1, 2,
3, ..., leaving the preexistent boot option at position 0 undisturbed,
and pushing the rest of the preexistent boot options back. Why is this
useful?

Instead, I would imagine: iterate over all the platform-specific boot
options, and in the loop body, check whether each one is already in
CurrentBootOptions() or not. If it's already there (somewhere --
anywhere), just skip it. If it's not there yet, then append it, by
passing MAX_UINTN as Position to EfiBootManagerAddLoadOptionVariable().
That will leave the user's BootOrder undisturbed, just make sure that
the platform-specific boot options are always present -- in the worst
case, at the very end of the boot order.

Let me show you what I have in mind (untested, clearly):

> diff --git a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
> index 61ab61ccc780..3daab129043b 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>
>
> @@ -389,6 +390,100 @@ PlatformRegisterFvBootOption (
>    EfiBootManagerFreeLoadOptions (BootOptions, BootOptionCount);
>  }
>
> +STATIC
> +VOID
> +RegisterPlatformOptions (
> +  VOID
> +  )
> +{
> +  EFI_STATUS                     Status;
> +  PLATFORM_BOOT_MANAGER_PROTOCOL *PlatformBootManager;
> +  UINTN                          BootCount;
> +  EFI_BOOT_MANAGER_LOAD_OPTION   *BootOptions;
> +  EFI_INPUT_KEY                  *BootKeys;
> +  UINTN                          CurrentBootCount;
> +  EFI_BOOT_MANAGER_LOAD_OPTION   *CurrentBootOptions;
> +  UINTN                          Index;
> +
> +  //
> +  // Get the platform's boot options.
> +  //
> +  Status = gBS->LocateProtocol (&gPlatformBootManagerProtocolGuid, NULL,
> +                  (VOID **)&PlatformBootManager);
> +  if (EFI_ERROR (Status)) {
> +    return;
> +  }
> +  Status = PlatformBootManager->GetPlatformBootOptionsAndKeys (&BootCount,
> +                                  &BootOptions, &BootKeys);
> +  if (EFI_ERROR (Status)) {
> +    return;
> +  }
> +  //
> +  // Fetch the existent boot options. If there are none, CurrentBootCount will
> +  // be zeroed.
> +  //
> +  CurrentBootOptions = EfiBootManagerGetLoadOptions (&CurrentBootCount,
> +                         LoadOptionTypeBoot);
> +  //
> +  // Process the platform boot options.
> +  //
> +  for (Index = 0; Index < BootCount; Index++) {
> +    INTN  Match;
> +    UINTN BootOptionNumber;
> +
> +    //
> +    // If there are any preexistent boot options, and the subject platform boot
> +    // option is already among them, then don't try to add it. Just get its
> +    // assigned boot option number so we can associate a hotkey with it. Note
> +    // that EfiBootManagerFindLoadOption() deals fine with (CurrentBootOptions
> +    // == NULL) if (CurrentBootCount == 0).
> +    //
> +    Match = EfiBootManagerFindLoadOption (&BootOptions[Index],
> +              CurrentBootOptions, CurrentBootCount);
> +    if (Match >= 0) {
> +      BootOptionNumber = CurrentBootOptions[Match].OptionNumber;
> +    } else {
> +      //
> +      // Add the platform boot option as a new one, at the end of the boot
> +      // order. Note that if the platform provided this boot option with an
> +      // unassigned option number, then the below function call will assign a
> +      // number.
> +      //
> +      Status = EfiBootManagerAddLoadOptionVariable (&BootOptions[Index],
> +                 MAX_UINTN);
> +      if (EFI_ERROR (Status)) {
> +        DEBUG ((DEBUG_ERROR, "%a: failed to register \"%s\": %r\n",
> +          __FUNCTION__, BootOptions[Index].Description, Status));
> +        continue;
> +      }
> +      BootOptionNumber = BootOptions[Index].OptionNumber;
> +    }
> +
> +    //
> +    // Register a hotkey with the boot option, if requested.
> +    //
> +    if (BootKeys[Index].UnicodeChar == 'L\0') {
> +      continue;
> +    }
> +    Status = EfiBootManagerAddKeyOptionVariable (
> +               NULL,             // AddedOption
> +               BootOptionNumber,
> +               0                 // Modifier
> +               &BootKeys[Index],
> +               NULL
> +               );
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((DEBUG_ERROR, "%a: failed to register hotkey for \"%s\": %r\n",
> +        __FUNCTION__, BootOptions[Index].Description, Status));
> +    }
> +  }
> +  //
> +  // (CurrentBootOptions == NULL) is handled gracefully by the below.
> +  //
> +  EfiBootManagerFreeLoadOptions (CurrentBootOptions, CurrentBootCount);
> +  EfiBootManagerFreeLoadOptions (BootOptions, BootCount);
> +  FreePool (BootKeys);
> +}
>
>  STATIC
>  VOID
> @@ -402,6 +497,8 @@ PlatformRegisterOptionsAndKeys (
>    EFI_INPUT_KEY                Esc;
>    EFI_BOOT_MANAGER_LOAD_OPTION BootOption;
>
> +  RegisterPlatformOptions ();
> +
>    //
>    // Register ENTER as CONTINUE key
>    //

I apologize if I misunderstood you.

Thanks
Laszlo