public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [PATCH v3 0/2] add platform boot manager protocol
@ 2018-04-19 10:42 Haojian Zhuang
  2018-04-19 10:42 ` [PATCH v3 1/2] EmbeddedPkg: " Haojian Zhuang
  2018-04-19 10:42 ` [PATCH v3 2/2] ArmPkg/PlatformBootManagerLib: load platform boot options Haojian Zhuang
  0 siblings, 2 replies; 5+ messages in thread
From: Haojian Zhuang @ 2018-04-19 10:42 UTC (permalink / raw)
  To: edk2-devel; +Cc: Haojian Zhuang

Changelog:
v3:
  * Update the name of interface.
  * Move the initialization into platform driver.
  * Fix comment style.
  * Fix minor issues with comments.
v2:
  * Avoid to use hardcoding value. Create boot options by functions.

Haojian Zhuang (2):
  EmbeddedPkg: add platform boot manager protocol
  ArmPkg/PlatformBootManagerLib: load platform boot options

 ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c | 83 ++++++++++++++++++++++
 .../PlatformBootManagerLib.inf                     |  2 +
 EmbeddedPkg/EmbeddedPkg.dec                        |  1 +
 EmbeddedPkg/Include/Protocol/PlatformBootManager.h | 46 ++++++++++++
 4 files changed, 132 insertions(+)
 create mode 100644 EmbeddedPkg/Include/Protocol/PlatformBootManager.h

-- 
2.7.4



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

* [PATCH v3 1/2] EmbeddedPkg: add platform boot manager protocol
  2018-04-19 10:42 [PATCH v3 0/2] add platform boot manager protocol Haojian Zhuang
@ 2018-04-19 10:42 ` Haojian Zhuang
  2018-04-20 11:33   ` Laszlo Ersek
  2018-04-19 10:42 ` [PATCH v3 2/2] ArmPkg/PlatformBootManagerLib: load platform boot options Haojian Zhuang
  1 sibling, 1 reply; 5+ messages in thread
From: Haojian Zhuang @ 2018-04-19 10:42 UTC (permalink / raw)
  To: edk2-devel; +Cc: Haojian Zhuang, Laszlo Ersek, Leif Lindholm, Ard Biesheuvel

Create the PlatformBootManagerProtocol that is used to add
predefined boot options in platform driver. This interface
will be used in ArmPkg/PlatformBootManagerLib.

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>
---
 EmbeddedPkg/EmbeddedPkg.dec                        |  1 +
 EmbeddedPkg/Include/Protocol/PlatformBootManager.h | 46 ++++++++++++++++++++++
 2 files changed, 47 insertions(+)
 create mode 100644 EmbeddedPkg/Include/Protocol/PlatformBootManager.h

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..8fd63c8b45d6
--- /dev/null
+++ b/EmbeddedPkg/Include/Protocol/PlatformBootManager.h
@@ -0,0 +1,46 @@
+/** @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
+//
+
+/*
+  Get predefined boot options for platform.
+
+  @param[out] BootOptions   An array of predefined boot options.
+  @param[out] BootKeys      An array of predefined keys.
+*/
+typedef
+EFI_STATUS
+(EFIAPI *GET_PLATFORM_BOOT_OPTIONS_AND_KEYS) (
+  OUT EFI_BOOT_MANAGER_LOAD_OPTION       **BootOptions,
+  OUT EFI_INPUT_KEY                      **BootKeys
+  );
+
+struct _PLATFORM_BOOT_MANAGER_PROTOCOL {
+  GET_PLATFORM_BOOT_OPTIONS_AND_KEYS     GetPlatformBootOptionsAndKeys;
+};
+
+extern EFI_GUID gPlatformBootManagerProtocolGuid;
+
+#endif /* __PLATFORM_BOOT_MANAGER_PROTOCOL_H__ */
-- 
2.7.4



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

* [PATCH v3 2/2] ArmPkg/PlatformBootManagerLib: load platform boot options
  2018-04-19 10:42 [PATCH v3 0/2] add platform boot manager protocol Haojian Zhuang
  2018-04-19 10:42 ` [PATCH v3 1/2] EmbeddedPkg: " Haojian Zhuang
@ 2018-04-19 10:42 ` Haojian Zhuang
  2018-04-20 12:55   ` Laszlo Ersek
  1 sibling, 1 reply; 5+ messages in thread
From: Haojian Zhuang @ 2018-04-19 10:42 UTC (permalink / raw)
  To: edk2-devel; +Cc: Haojian Zhuang, Laszlo Ersek, Leif Lindholm, Ard Biesheuvel

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
+             );
+  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
-- 
2.7.4



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

* Re: [PATCH v3 1/2] EmbeddedPkg: add platform boot manager protocol
  2018-04-19 10:42 ` [PATCH v3 1/2] EmbeddedPkg: " Haojian Zhuang
@ 2018-04-20 11:33   ` Laszlo Ersek
  0 siblings, 0 replies; 5+ messages in thread
From: Laszlo Ersek @ 2018-04-20 11:33 UTC (permalink / raw)
  To: Haojian Zhuang, edk2-devel; +Cc: Leif Lindholm, Ard Biesheuvel

Hello Haojian,

On 04/19/18 12:42, Haojian Zhuang wrote:
> Create the PlatformBootManagerProtocol that is used to add
> predefined boot options in platform driver. This interface
> will be used in ArmPkg/PlatformBootManagerLib.
>
> 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>
> ---
>  EmbeddedPkg/EmbeddedPkg.dec                        |  1 +
>  EmbeddedPkg/Include/Protocol/PlatformBootManager.h | 46 ++++++++++++++++++++++
>  2 files changed, 47 insertions(+)
>  create mode 100644 EmbeddedPkg/Include/Protocol/PlatformBootManager.h
>
> 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..8fd63c8b45d6
> --- /dev/null
> +++ b/EmbeddedPkg/Include/Protocol/PlatformBootManager.h
> @@ -0,0 +1,46 @@
> +/** @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
> +//
> +
> +/*
> +  Get predefined boot options for platform.
> +
> +  @param[out] BootOptions   An array of predefined boot options.
> +  @param[out] BootKeys      An array of predefined keys.
> +*/
> +typedef
> +EFI_STATUS
> +(EFIAPI *GET_PLATFORM_BOOT_OPTIONS_AND_KEYS) (
> +  OUT EFI_BOOT_MANAGER_LOAD_OPTION       **BootOptions,
> +  OUT EFI_INPUT_KEY                      **BootKeys
> +  );
> +
> +struct _PLATFORM_BOOT_MANAGER_PROTOCOL {
> +  GET_PLATFORM_BOOT_OPTIONS_AND_KEYS     GetPlatformBootOptionsAndKeys;
> +};
> +
> +extern EFI_GUID gPlatformBootManagerProtocolGuid;
> +
> +#endif /* __PLATFORM_BOOT_MANAGER_PROTOCOL_H__ */
>

this looks good generally, but the protocol documentation is still
seriously lacking. The documentation should enable anyone to call the
member and know everything about the intended use and lifecycle of the
output parameters.

Also, because the two output arrays are supposed to be used in tandem, I
suggest that we replace the terminator zero element with an explicit
element count (a UINTN output parameter). For example:

  @param[out] Count         The number of elements in each of
                            BootOptions and BootKeys. On successful
                            return, Count is at least one.

  @param[out] BootOptions   An array of platform boot options.
                            BootOptions is pool-allocated by
                            GET_PLATFORM_BOOT_OPTIONS_AND_KEYS, and
                            GET_PLATFORM_BOOT_OPTIONS_AND_KEYS populates
                            every element in BootOptions with
                            EfiBootManagerInitializeLoadOption().
                            BootOptions shall not contain duplicate
                            entries. The caller is responsible for
                            releasing BootOptions after use with
                            EfiBootManagerFreeLoadOptions().

  @param[out] BootKeys      A pool-allocated array of platform boot
                            hotkeys. For every 0 <= Index < Count, If
                            BootOptions[Index] is not to be associated
                            with a hotkey, then BootKeys[Index] is
                            zero-filled. Otherwise, BootKeys[Index]
                            specifies the boot hotkey for
                            BootOptions[Index]. BootKeys shall not
                            contain duplicate entries (other than
                            zero-filled entries). The caller is
                            responsible for releasing BootKeys with
                            FreePool() after use.

  @retval EFI_SUCCESS           Count, BootOptions and BootKeys have
                                been set.

  @retval EFI_OUT_OF_RESOURCES  Memory allocation failed.

  @retval EFI_UNSUPPORTED       The platform doesn't provide boot
                                options as a feature.

  @retval EFI_NOT_FOUND         The platform could provide boot options
                                as a feature, but none have been
                                configured.

  @return                       Error codes propagated from underlying
                                functions.

Thanks
Laszlo


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

* Re: [PATCH v3 2/2] ArmPkg/PlatformBootManagerLib: load platform boot options
  2018-04-19 10:42 ` [PATCH v3 2/2] ArmPkg/PlatformBootManagerLib: load platform boot options Haojian Zhuang
@ 2018-04-20 12:55   ` Laszlo Ersek
  0 siblings, 0 replies; 5+ messages in thread
From: Laszlo Ersek @ 2018-04-20 12:55 UTC (permalink / raw)
  To: Haojian Zhuang, edk2-devel; +Cc: Leif Lindholm, Ard Biesheuvel

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


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

end of thread, other threads:[~2018-04-20 12:55 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-04-19 10:42 [PATCH v3 0/2] add platform boot manager protocol Haojian Zhuang
2018-04-19 10:42 ` [PATCH v3 1/2] EmbeddedPkg: " Haojian Zhuang
2018-04-20 11:33   ` Laszlo Ersek
2018-04-19 10:42 ` [PATCH v3 2/2] ArmPkg/PlatformBootManagerLib: load platform boot options Haojian Zhuang
2018-04-20 12:55   ` Laszlo Ersek

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