Re: [edk2] [Patch v3 13/23] OvmfPkg/PlatformBds: Register boot options and hot keys.
On 04/21/16 08:58, Ruiyu Ni wrote:
> The patch registers "Enter" key as the continue key (hot key to skip
> the boot timeout wait), maps "F2" key to UI, and registers Shell
> boot option.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ruiyu Ni
> Cc: Jordan Justen
> Cc: Laszlo Ersek
> ---
> .../Library/PlatformBootManagerLib/BdsPlatform.c | 71
> ++
> .../Library/PlatformBootManagerLib/BdsPlatform.h | 1 +
> .../PlatformBootManagerLib.inf | 2 +
> 3 files changed, 74 insertions(+)
This patch also looks good, I have just a few requests :)
> diff --git a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
> b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
> index 38e2943..d4bdbe5 100644
> --- a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
> +++ b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
> @@ -88,6 +88,55 @@ InstallDevicePathCallback (
>VOID
>);
>
> +VOID
> +PlatformRegisterFvBootOption (
> + EFI_GUID *FileGuid,
> + CHAR16 *Description,
> + UINT32 Attributes
> + )
> +{
> + EFI_STATUSStatus;
> + UINTN OptionIndex;
> + EFI_BOOT_MANAGER_LOAD_OPTION NewOption;
> + EFI_BOOT_MANAGER_LOAD_OPTION *BootOptions;
> + UINTN BootOptionCount;
> + MEDIA_FW_VOL_FILEPATH_DEVICE_PATH FileNode;
> + EFI_LOADED_IMAGE_PROTOCOL *LoadedImage;
> + EFI_DEVICE_PATH_PROTOCOL *DevicePath;
> +
> + Status = gBS->HandleProtocol (gImageHandle, &gEfiLoadedImageProtocolGuid,
> (VOID **) &LoadedImage);
Please make sure that lines being added in this patch are not longer
than 79 characters.
> + ASSERT_EFI_ERROR (Status);
> +
> + EfiInitializeFwVolDevicepathNode (&FileNode, FileGuid);
> + DevicePath = AppendDevicePathNode (
> + DevicePathFromHandle (LoadedImage->DeviceHandle),
> + (EFI_DEVICE_PATH_PROTOCOL *) &FileNode
> + );
Would it make sense to call DevicePathFromHandle() separately, and
assert that the retval is not NULL?
Also: AppendDevicePathNode() allocates memory dynamically. Please check
if it succeeds (if it fails, we can trigger an assert, or log an
EFI_D_WARN message, and just return -- up to you).
> +
> + Status = EfiBootManagerInitializeLoadOption (
> + &NewOption,
> + LoadOptionNumberUnassigned,
> + LoadOptionTypeBoot,
> + Attributes,
> + Description,
> + DevicePath,
> + NULL,
> + 0
> + );
The variable DevicePath should be freed right here; we are currently
leaking it. (EfiBootManagerInitializeLoadOption() creates a deep copy.)
Also, if this call fails, can you please log an EFI_D_WARN message?
> + if (!EFI_ERROR (Status)) {
> +BootOptions = EfiBootManagerGetLoadOptions (&BootOptionCount,
> LoadOptionTypeBoot);
> +
> +OptionIndex = EfiBootManagerFindLoadOption (&NewOption, BootOptions,
> BootOptionCount);
The type of OptionIndex (declared above) should be INTN then, not UINTN.
> +if (OptionIndex == -1) {
> + Status = EfiBootManagerAddLoadOptionVariable (&NewOption, (UINTN) -1);
For better readability, I propose MAX_UINTN, rather than (UINTN)-1.
> + ASSERT_EFI_ERROR (Status);
> +}
> +EfiBootManagerFreeLoadOption (&NewOption);
> +EfiBootManagerFreeLoadOptions (BootOptions, BootOptionCount);
> + }
> +}
> +
> //
> // BDS Platform Functions
> //
> @@ -111,10 +160,32 @@ Returns:
>
> --*/
> {
> + EFI_INPUT_KEYEnter;
> + EFI_INPUT_KEYF2;
> + EFI_BOOT_MANAGER_LOAD_OPTION BootOption;
> +
>DEBUG ((EFI_D_INFO, "PlatformBootManagerBeforeConsole\n"));
>InstallDevicePathCallback ();
>PlatformInitializeConsole (gPlatformConsole);
>PcdSet16 (PcdPlatformBootTimeOut, GetFrontPageTimeoutFromQemu ());
> +
> + //
> + // Register ENTER as CONTINUE key
> + //
> + Enter.ScanCode= SCAN_NULL;
> + Enter.UnicodeChar = CHAR_CARRIAGE_RETURN;
> + EfiBootManagerRegisterContinueKeyOption (0, &Enter, NULL);
Can you please add an EFI_D_WARN or an ASSERT_EFI_ERROR here?
> + //
> + // Map F2 to Boot Manager Menu
> + //
> + F2.ScanCode= SCAN_F2;
> + F2.UnicodeChar = CHAR_NULL;
> + EfiBootManagerGetBootManagerMenu (&BootOption);
Ditto.
> + EfiBootManagerAddKeyOptionVariable (NULL, (UINT16)
> BootOption.OptionNumber, 0, &F2, NULL);
Ditto.
Also, can you please register the ESC key as well, in addition to F2, to
enter the Boot Manager Menu?
> + //
> + // Register UEFI Shell
> + //
> + PlatformRegisterFvBootOption (PcdGetPtr (PcdShellFile), L"UEFI Shell",
> LOAD_OPTION_ACTIVE);
Can you please extract all these steps into a separate function, called
PlatformRegisterOptionsAndKeys()?
Thanks!
Laszlo
> }
>
>
> diff --git a/OvmfPkg/Library/PlatformBootManagerLib/
[edk2] [Patch v3 13/23] OvmfPkg/PlatformBds: Register boot options and hot keys.
The patch registers "Enter" key as the continue key (hot key to skip
the boot timeout wait), maps "F2" key to UI, and registers Shell
boot option.
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ruiyu Ni
Cc: Jordan Justen
Cc: Laszlo Ersek
---
.../Library/PlatformBootManagerLib/BdsPlatform.c | 71 ++
.../Library/PlatformBootManagerLib/BdsPlatform.h | 1 +
.../PlatformBootManagerLib.inf | 2 +
3 files changed, 74 insertions(+)
diff --git a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
index 38e2943..d4bdbe5 100644
--- a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
+++ b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
@@ -88,6 +88,55 @@ InstallDevicePathCallback (
VOID
);
+VOID
+PlatformRegisterFvBootOption (
+ EFI_GUID *FileGuid,
+ CHAR16 *Description,
+ UINT32 Attributes
+ )
+{
+ EFI_STATUSStatus;
+ UINTN OptionIndex;
+ EFI_BOOT_MANAGER_LOAD_OPTION NewOption;
+ EFI_BOOT_MANAGER_LOAD_OPTION *BootOptions;
+ UINTN BootOptionCount;
+ MEDIA_FW_VOL_FILEPATH_DEVICE_PATH FileNode;
+ EFI_LOADED_IMAGE_PROTOCOL *LoadedImage;
+ EFI_DEVICE_PATH_PROTOCOL *DevicePath;
+
+ Status = gBS->HandleProtocol (gImageHandle, &gEfiLoadedImageProtocolGuid,
(VOID **) &LoadedImage);
+ ASSERT_EFI_ERROR (Status);
+
+ EfiInitializeFwVolDevicepathNode (&FileNode, FileGuid);
+ DevicePath = AppendDevicePathNode (
+ DevicePathFromHandle (LoadedImage->DeviceHandle),
+ (EFI_DEVICE_PATH_PROTOCOL *) &FileNode
+ );
+
+ Status = EfiBootManagerInitializeLoadOption (
+ &NewOption,
+ LoadOptionNumberUnassigned,
+ LoadOptionTypeBoot,
+ Attributes,
+ Description,
+ DevicePath,
+ NULL,
+ 0
+ );
+ if (!EFI_ERROR (Status)) {
+BootOptions = EfiBootManagerGetLoadOptions (&BootOptionCount,
LoadOptionTypeBoot);
+
+OptionIndex = EfiBootManagerFindLoadOption (&NewOption, BootOptions,
BootOptionCount);
+
+if (OptionIndex == -1) {
+ Status = EfiBootManagerAddLoadOptionVariable (&NewOption, (UINTN) -1);
+ ASSERT_EFI_ERROR (Status);
+}
+EfiBootManagerFreeLoadOption (&NewOption);
+EfiBootManagerFreeLoadOptions (BootOptions, BootOptionCount);
+ }
+}
+
//
// BDS Platform Functions
//
@@ -111,10 +160,32 @@ Returns:
--*/
{
+ EFI_INPUT_KEYEnter;
+ EFI_INPUT_KEYF2;
+ EFI_BOOT_MANAGER_LOAD_OPTION BootOption;
+
DEBUG ((EFI_D_INFO, "PlatformBootManagerBeforeConsole\n"));
InstallDevicePathCallback ();
PlatformInitializeConsole (gPlatformConsole);
PcdSet16 (PcdPlatformBootTimeOut, GetFrontPageTimeoutFromQemu ());
+
+ //
+ // Register ENTER as CONTINUE key
+ //
+ Enter.ScanCode= SCAN_NULL;
+ Enter.UnicodeChar = CHAR_CARRIAGE_RETURN;
+ EfiBootManagerRegisterContinueKeyOption (0, &Enter, NULL);
+ //
+ // Map F2 to Boot Manager Menu
+ //
+ F2.ScanCode= SCAN_F2;
+ F2.UnicodeChar = CHAR_NULL;
+ EfiBootManagerGetBootManagerMenu (&BootOption);
+ EfiBootManagerAddKeyOptionVariable (NULL, (UINT16) BootOption.OptionNumber,
0, &F2, NULL);
+ //
+ // Register UEFI Shell
+ //
+ PlatformRegisterFvBootOption (PcdGetPtr (PcdShellFile), L"UEFI Shell",
LOAD_OPTION_ACTIVE);
}
diff --git a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.h
b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.h
index 796b53d..2eab29a 100644
--- a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.h
+++ b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.h
@@ -55,6 +55,7 @@ Abstract:
#include
#include
#include
+#include
#include
#include
diff --git a/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
b/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
index f9cbe65..edf8f14 100644
--- a/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
+++ b/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
@@ -60,6 +60,7 @@ [Pcd]
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashVariablesEnable
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfHostBridgePciDevId
gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut
+ gEfiIntelFrameworkModulePkgTokenSpaceGuid.PcdShellFile
[Pcd.IA32, Pcd.X64]
gEfiMdePkgTokenSpaceGuid.PcdFSBClock
@@ -69,6 +70,7 @@ [Protocols]
gEfiPciRootBridgeIoProtocolGuid
gEfiS3SaveStateProtocolGuid # PROTOCOL SOMETIMES_CONSUMED
gEfiDxeSmmReadyToLockProtocolGuid # PROTOCOL SOMETIMES_PRODUCED
+ gEfiLoadedImageProtocolGuid # PROTOCOL SOMETIMES_PRODUCED
[Guids]
gEfiEndOfDxeEventGroupGuid
--
2.7.0.windows.1
___
edk2-devel m
