* [PATCH v2 0/1] load boot options from platform
@ 2018-04-17 5:11 Haojian Zhuang
2018-04-17 5:11 ` [PATCH v2 1/1] ArmPkg/PlatformBootManagerLib: " Haojian Zhuang
0 siblings, 1 reply; 5+ messages in thread
From: Haojian Zhuang @ 2018-04-17 5:11 UTC (permalink / raw)
To: edk2-devel; +Cc: Haojian Zhuang
Changelog:
v2:
* Avoid to use hardcoding value. Create boot options by functions.
Haojian Zhuang (1):
ArmPkg/PlatformBootManagerLib: load boot options from platform
ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c | 81 ++++++++++++++++++++++
.../PlatformBootManagerLib.inf | 2 +
EmbeddedPkg/EmbeddedPkg.dec | 1 +
EmbeddedPkg/Include/Protocol/PlatformBootManager.h | 39 +++++++++++
4 files changed, 123 insertions(+)
create mode 100644 EmbeddedPkg/Include/Protocol/PlatformBootManager.h
--
2.7.4
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v2 1/1] ArmPkg/PlatformBootManagerLib: load boot options from platform
2018-04-17 5:11 [PATCH v2 0/1] load boot options from platform Haojian Zhuang
@ 2018-04-17 5:11 ` Haojian Zhuang
2018-04-17 9:02 ` Laszlo Ersek
0 siblings, 1 reply; 5+ messages in thread
From: Haojian Zhuang @ 2018-04-17 5:11 UTC (permalink / raw)
To: edk2-devel; +Cc: Haojian Zhuang
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 +++++++++++
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)) {
+ if (PlatformBootManager->Register) {
+ // Last entries of PlatformBootOptionArray and PlatformKeyArray are empty.
+ Status = PlatformBootManager->Register (
+ &PlatformBootOptionArray,
+ &PlatformKeyArray
+ );
+ if (!EFI_ERROR (Status)) {
+ PlatformOption = PlatformBootOptionArray;
+ PlatformKey = PlatformKeyArray;
+ while (PlatformOption->Description != NULL) {
+ Status = EfiBootManagerInitializeLoadOption (
+ &NewOption,
+ LoadOptionNumberUnassigned,
+ LoadOptionTypeBoot,
+ LOAD_OPTION_ACTIVE,
+ PlatformOption->Description,
+ PlatformOption->FilePath,
+ NULL,
+ 0
+ );
+ ASSERT_EFI_ERROR (Status);
+ CurrentBootOptions = EfiBootManagerGetLoadOptions (
+ &CurrentBootOptionCount, LoadOptionTypeBoot
+ );
+ OptionIndex = EfiBootManagerFindLoadOption (
+ &NewOption, CurrentBootOptions, CurrentBootOptionCount
+ );
+ if (OptionIndex == -1) {
+ // Append the BootLoadOption
+ Status = EfiBootManagerAddLoadOptionVariable (&NewOption, MAX_UINTN);
+ ASSERT_EFI_ERROR (Status);
+ }
+
+ // If UnicodeChar isn't empty, there's a hot key.
+ if (PlatformKey->UnicodeChar) {
+ // The index of Boot Options counts from 1.
+ // The last index equals to the count of total Boot Options.
+ Status = EfiBootManagerAddKeyOptionVariable (
+ NULL, CurrentBootOptionCount, 0, PlatformKey, NULL
+ );
+ }
+
+ EfiBootManagerFreeLoadOption (&NewOption);
+ EfiBootManagerFreeLoadOptions (
+ CurrentBootOptions, CurrentBootOptionCount
+ );
+
+ PlatformOption++;
+ PlatformKey++;
+ }
+ FreePool (PlatformBootOptionArray);
+ 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
+ );
+
+struct _PLATFORM_BOOT_MANAGER_PROTOCOL {
+ REGISTER_PLATFORM_BOOT_MANAGER Register;
+};
+
+extern EFI_GUID gPlatformBootManagerProtocolGuid;
+
+#endif /* __PLATFORM_BOOT_PROTOCOL_H__ */
--
2.7.4
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2 1/1] ArmPkg/PlatformBootManagerLib: load boot options from platform
2018-04-17 5:11 ` [PATCH v2 1/1] ArmPkg/PlatformBootManagerLib: " Haojian Zhuang
@ 2018-04-17 9:02 ` Laszlo Ersek
2018-04-17 9:45 ` Laszlo Ersek
2018-04-19 9:30 ` Haojian Zhuang
0 siblings, 2 replies; 5+ messages in thread
From: Laszlo Ersek @ 2018-04-17 9:02 UTC (permalink / raw)
To: Haojian Zhuang; +Cc: edk2-devel, Ard Biesheuvel, Leif Lindholm (Linaro address)
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2 1/1] ArmPkg/PlatformBootManagerLib: load boot options from platform
2018-04-17 9:02 ` Laszlo Ersek
@ 2018-04-17 9:45 ` Laszlo Ersek
2018-04-19 9:30 ` Haojian Zhuang
1 sibling, 0 replies; 5+ messages in thread
From: Laszlo Ersek @ 2018-04-17 9:45 UTC (permalink / raw)
To: Haojian Zhuang; +Cc: edk2-devel, Leif Lindholm (Linaro address), Ard Biesheuvel
On 04/17/18 11:02, Laszlo Ersek wrote:
> 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.
This may not have been obvious from the
EfiBootManagerAddLoadOptionVariable() prototype and/or documentation;
I've just submitted a patch to fix that:
[edk2] [PATCH] MdeModulePkg/UefiBootManagerLib: fix
AddLoadOptionVariable docs/prototype
Laszlo
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2 1/1] ArmPkg/PlatformBootManagerLib: load boot options from platform
2018-04-17 9:02 ` Laszlo Ersek
2018-04-17 9:45 ` Laszlo Ersek
@ 2018-04-19 9:30 ` Haojian Zhuang
1 sibling, 0 replies; 5+ messages in thread
From: Haojian Zhuang @ 2018-04-19 9:30 UTC (permalink / raw)
To: Laszlo Ersek, Haojian Zhuang
Cc: edk2-devel@lists.01.org, Ard Biesheuvel,
Leif Lindholm (Linaro address)
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-04-19 9:31 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-04-17 5:11 [PATCH v2 0/1] load boot options from platform Haojian Zhuang
2018-04-17 5:11 ` [PATCH v2 1/1] ArmPkg/PlatformBootManagerLib: " Haojian Zhuang
2018-04-17 9:02 ` Laszlo Ersek
2018-04-17 9:45 ` Laszlo Ersek
2018-04-19 9:30 ` Haojian Zhuang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox