* [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