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 <ruiyu...@intel.com>
> Cc: Jordan Justen <jordan.l.jus...@intel.com>
> Cc: Laszlo Ersek <ler...@redhat.com>
> ---
> .../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_STATUS Status;
> + 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_KEY Enter;
> + EFI_INPUT_KEY F2;
> + 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/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 <Protocol/PciRootBridgeIo.h>
> #include <Protocol/S3SaveState.h>
> #include <Protocol/DxeSmmReadyToLock.h>
> +#include <Protocol/LoadedImage.h>
>
> #include <Guid/Acpi.h>
> #include <Guid/SmBios.h>
> 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
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
