[edk2,v3,2/2] ArmPkg/PlatformBootManagerLib: load platform boot options

Message ID 1524134560-1245-3-git-send-email-haojian.zhuang@linaro.org
State New
Headers show
Series
  • add platform boot manager protocol
Related show

Commit Message

Haojian Zhuang April 19, 2018, 10:42 a.m.
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(+)

-- 
2.7.4

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

Comments

Laszlo Ersek April 20, 2018, 12:55 p.m. | #1
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Patch

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
+             );
+  if (EFI_ERROR (Status)) {
+    return;
+  }
+  CurrentBootOptions = EfiBootManagerGetLoadOptions (
+                         &CurrentBootOptionCount, LoadOptionTypeBoot
+                         );
+  OptionIndex = EfiBootManagerFindLoadOption (
+                  &PlatformBootOptionArray[0],
+                  CurrentBootOptions,
+                  CurrentBootOptionCount
+                  );
+  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++;
+  }
+  EfiBootManagerFreeLoadOptions (
+    CurrentBootOptions, CurrentBootOptionCount
+    );
+  EfiBootManagerFreeLoadOptions (
+    PlatformBootOptionArray, Index
+    );
+  FreePool (PlatformKeyArray);
+}
+
+STATIC
+VOID
 PlatformRegisterOptionsAndKeys (
   VOID
   )
@@ -402,6 +483,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