Hi Benjamin,

The implementation logic looks good. We do have to consider the case of a 
system without BIOS setup, in which case UiApp will not be present in the 
image. However, that appears to be well handled by new implementation. If UiApp 
does not exist in the FV that contains BdsDxe then the file will not be found 
and the new boot option will not be added.

I do have two comments however.

First, if UiApp does exist in the FV, could you change the F2 hot key to go 
straight into the setup menu? The hot key is registered here: 
https://github.com/tianocore/edk2-platforms/blob/c9fff3e9efd2d5daab76b703cc94fd7cbf2ac0b2/Platform/Intel/BoardModulePkg/Library/BoardBdsHookLib/BoardBootOption.c#L472

Normally F2 is used to enter setup but the current implementation is designed 
to handle the case of a system not having setup rather lazily... it just routes 
both F2 and F7 to the boot option menu 100% of the time. Could you modify this 
implementation so that if UiApp exists in the FV, then F2 goes straight into 
setup instead?

My second comment is kind of a nitpick, we generally use the string L"Enter 
Setup" for UiApp instead of L"Platform Configuration" and sadly some of the old 
code has special handling of that string. For example: 
https://github.com/tianocore/edk2-platforms/blob/c9fff3e9efd2d5daab76b703cc94fd7cbf2ac0b2/Platform/Intel/PurleyOpenBoardPkg/Override/Platform/Intel/MinPlatformPkg/Bds/Library/DxePlatformBootManagerLib/BdsPlatform.c#L1244

Could you please change that string to L"Enter Setup"?

Thanks,
Nate

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Benjamin Doron
Sent: Wednesday, July 28, 2021 12:46 PM
To: devel@edk2.groups.io
Cc: Dong, Eric <eric.d...@intel.com>; Liming Gao <gaolim...@byosoft.com.cn>
Subject: [edk2-devel] [edk2-platforms][PATCH v2] 
BoardModulePkg/BoardBdsHookLib: Register UiApp as boot option

BootManagerMenuApp is the default PcdBootManagerMenuFile. It allows choosing a 
boot device, but system configuration is performed in UiApp.
Therefore, un-comment and fix UiApp boot option registration.

Tested, UiApp can be entered through the new boot option.

Cc: Eric Dong <eric.d...@intel.com>
Cc: Liming Gao <gaolim...@byosoft.com.cn>
Signed-off-by: Benjamin Doron <benjamin.doro...@gmail.com>
---
 Platform/Intel/BoardModulePkg/Library/BoardBdsHookLib/BoardBootOption.c | 13 
++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git 
a/Platform/Intel/BoardModulePkg/Library/BoardBdsHookLib/BoardBootOption.c 
b/Platform/Intel/BoardModulePkg/Library/BoardBdsHookLib/BoardBootOption.c
index 87138bdd79ff..2dd0b250d44e 100644
--- a/Platform/Intel/BoardModulePkg/Library/BoardBdsHookLib/BoardBootOption.c
+++ b/Platform/Intel/BoardModulePkg/Library/BoardBdsHookLib/BoardBootOpt
+++ ion.c
@@ -361,20 +361,11 @@ RegisterDefaultBootOption (
   if (mBootMenuOptionNumber == LoadOptionNumberUnassigned) {     DEBUG 
((DEBUG_INFO, "BootMenuOptionNumber (%d) should not be same to 
LoadOptionNumberUnassigned(%d).\n", mBootMenuOptionNumber, 
LoadOptionNumberUnassigned));   }-#if 0+   //   // Boot Manager Menu   //-  
EfiInitializeFwVolDevicepathNode (&FileNode, &mUiFile);--  gBS->HandleProtocol 
(-         gImageHandle,-         &gEfiLoadedImageProtocolGuid,-         (VOID 
**) &LoadedImage-         );-  DevicePath = AppendDevicePathNode 
(DevicePathFromHandle (LoadedImage->DeviceHandle), (EFI_DEVICE_PATH_PROTOCOL *) 
&FileNode);-#endif-+  RegisterFvBootOption (&mUiFile, L"Platform 
Configuration", (UINTN) -1, LOAD_OPTION_ACTIVE, NULL, 0); }  /**-- 
2.31.1



-=-=-=-=-=-=
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#78309): https://edk2.groups.io/g/devel/message/78309
Mute This Topic: https://groups.io/mt/84469836/1767664
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub 
[nathaniel.l.desim...@intel.com] -=-=-=-=-=-=




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#79251): https://edk2.groups.io/g/devel/message/79251
Mute This Topic: https://groups.io/mt/84469836/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-


Reply via email to