Sunny, I have 7 minor comments. Please check below. Regards, Ray
>-----Original Message----- >From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Sunny >Wang >Sent: Tuesday, June 28, 2016 10:27 AM >To: edk2-devel@lists.01.org >Cc: Ni, Ruiyu <ruiyu...@intel.com> >Subject: [edk2] [PATCH v2] MdeModulePkg: Skip registering BootManagerMenu if >its FFS is not found > >This is a enhancement to support the case when platform firmware doesn’t >support Boot Manager Menu. >For now, if BootManagerMenu FFS can not be retrieved from FV, BDS core code >will still register a boot option for it. Then, this >non-functional boot option will still be booted by user's request (like HotKey >or Exit from shell) to cause additional boot time >and error status code reported. Therefore, it would be good to skip >BootManagerMenu boot option registration and then >return error status and Invalid BootOption data for this case so that the >BootManagerBoot() or other consumers can directly >return without doing anything. > >Contributed-under: TianoCore Contribution Agreement 1.0 >Signed-off-by: Sunny Wang <sunnyw...@hpe.com> >--- > MdeModulePkg/Include/Library/UefiBootManagerLib.h | 7 ++-- > MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c | 39 ++++++++++++++++++---- > MdeModulePkg/Library/UefiBootManagerLib/BmHotkey.c | 9 +++-- > MdeModulePkg/Universal/BdsDxe/BdsEntry.c | 29 +++++++++++----- > 4 files changed, 64 insertions(+), 20 deletions(-) > >diff --git a/MdeModulePkg/Include/Library/UefiBootManagerLib.h >b/MdeModulePkg/Include/Library/UefiBootManagerLib.h >index 0fdb23d..649af9a 100644 >--- a/MdeModulePkg/Include/Library/UefiBootManagerLib.h >+++ b/MdeModulePkg/Include/Library/UefiBootManagerLib.h >@@ -418,12 +418,13 @@ EfiBootManagerBoot ( > ); > > /** >- Return the Boot Manager Menu. >- >+ Return the boot option corresponding to the Boot Manager Menu. >+ It may automatically create one if the boot option hasn't been created yet. >+ > @param BootOption Return the Boot Manager Menu. > > @retval EFI_SUCCESS The Boot Manager Menu is successfully returned. >- @retval EFI_NOT_FOUND The Boot Manager Menu is not found. >+ @retval Status Return status of either gRT->SetVariable () or >GetSectionFromFv(). > **/ 1. How about we change the return status as following: @retval EFI_SUCCESS The Boot Manager Menu is successfully returned. @retval EFI_NOT_FOUND The Boot Manager Menu cannot be found. @retval others Return status of gRT->SetVariable (). BootOption still points to the Boot Manager Menu even the Status is not EFI_SUCCESS and EFI_NOT_FOUND. > EFI_STATUS > EFIAPI >diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c >b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c >index d016517..17e416a 100644 >--- a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c >+++ b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c >@@ -2,7 +2,7 @@ > Library functions which relates with booting. > > Copyright (c) 2011 - 2016, Intel Corporation. All rights reserved.<BR> >-(C) Copyright 2015 Hewlett Packard Enterprise Development LP<BR> >+(C) Copyright 2015-2016 Hewlett Packard Enterprise Development LP<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 >@@ -2159,16 +2159,19 @@ EfiBootManagerRefreshAllBootOption ( > } > > /** >- This function is called to create the boot option for the Boot Manager Menu. >+ This function is called to get or create the boot option for the Boot >Manager Menu. > > The Boot Manager Menu is shown after successfully booting a boot option. > Assume the BootManagerMenuFile is in the same FV as the module links to > this library. > >- @param BootOption Return the boot option of the Boot Manager Menu >+ @param BootOption Return the boot option of the Boot Manager Menu. >+ If BootManagerMenu fails to be set as a Boot#### >variables, BootOption >+ still points to the Boot Manager Menu. >+ If BootManagerMenu FFS section can not be retrieved, >BootOption will >+ be contain invalid data to prevent other function >from using it. > > @retval EFI_SUCCESS Successfully register the Boot Manager Menu. >- @retval Status Return status of gRT->SetVariable (). BootOption >still points >- to the Boot Manager Menu even the Status is not >EFI_SUCCESS. >+ @retval Status Return status of either gRT->SetVariable() or >GetSectionFromFv(). > **/ 2. The return status can be changed as the same as EfiBootManagerGetBootManagerMenu(). Change to @param BootOption isn't needed. > EFI_STATUS > BmRegisterBootManagerMenu ( >@@ -2181,9 +2184,32 @@ BmRegisterBootManagerMenu ( > EFI_DEVICE_PATH_PROTOCOL *DevicePath; > EFI_LOADED_IMAGE_PROTOCOL *LoadedImage; > MEDIA_FW_VOL_FILEPATH_DEVICE_PATH FileNode; >+ VOID *Data; >+ UINTN DataSize; > > Status = GetSectionFromFv ( > PcdGetPtr (PcdBootManagerMenuFile), >+ EFI_SECTION_PE32, >+ 0, >+ (VOID **) &Data, >+ &DataSize >+ ); >+ if (Data != NULL) { >+ FreePool (Data); >+ } >+ if (EFI_ERROR (Status)) { >+ DEBUG ((EFI_D_ERROR, "[Bds]BootManagerMenu FFS section can not be found, >skip its boot option registeration\n")); >+ ZeroMem (BootOption, sizeof (EFI_BOOT_MANAGER_LOAD_OPTION)); >+ BootOption->OptionNumber = LoadOptionNumberUnassigned; >+ BootOption->OptionType = LoadOptionTypeMax; >+ return Status; 3. We can directly return EFI_NOT_FOUND without initializing the BootOption. >+ } >+ >+ // >+ // Get BootManagerMenu application's description from EFI User Interface >Section. >+ // >+ Status = GetSectionFromFv ( >+ PcdGetPtr (PcdBootManagerMenuFile), > EFI_SECTION_USER_INTERFACE, > 0, > (VOID **) &Description, >@@ -2241,8 +2267,7 @@ BmRegisterBootManagerMenu ( > @param BootOption Return the Boot Manager Menu. > > @retval EFI_SUCCESS The Boot Manager Menu is successfully returned. >- @retval Status Return status of gRT->SetVariable (). BootOption >still points >- to the Boot Manager Menu even the Status is not >EFI_SUCCESS. >+ @retval Status Return status of either gRT->SetVariable() or >GetSectionFromFv(). > **/ > EFI_STATUS > EFIAPI >diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmHotkey.c >b/MdeModulePkg/Library/UefiBootManagerLib/BmHotkey.c >index 55df7e9..2c86555 100644 >--- a/MdeModulePkg/Library/UefiBootManagerLib/BmHotkey.c >+++ b/MdeModulePkg/Library/UefiBootManagerLib/BmHotkey.c >@@ -957,8 +957,9 @@ EfiBootManagerStartHotkeyService ( > @param Modifier Key shift state. > @param ... Parameter list of pointer of EFI_INPUT_KEY. > >- @retval EFI_SUCCESS The key option is added. >- @retval EFI_ALREADY_STARTED The hot key is already used by certain key >option. >+ @retval EFI_SUCCESS The key option is added. >+ @retval EFI_ALREADY_STARTED The hot key is already used by certain key >option. >+ @retval EFI_INVALID_PARAMETER Invalid boot option number for the key option. > **/ > EFI_STATUS > EFIAPI >@@ -981,6 +982,10 @@ EfiBootManagerAddKeyOptionVariable ( > UINTN KeyOptionNumber; > CHAR16 KeyOptionName[sizeof ("Key####")]; > >+ if ((BootOptionNumber >= LoadOptionNumberMax) || (BootOptionNumber == >LoadOptionNumberUnassigned)) { >+ return EFI_INVALID_PARAMETER; >+ } >+ 4. This change is not needed. I agree that the return status change of EfiBootManagerGetBootManagerMenu() is an incompatible change. Because old caller assumes this *GetBootManagerMenu() always return the load option pointing to Boot Manager Menu even the return status is failure status. But with the change, when status is EFI_NOT_FOUND, the load option doesn't point to Boot Manager Menu. But the impact should be small because for a platform who provides the Boot Manager Menu, the platform code doesn't need to change to take care of the new EFI_NOT_FOUND status. BDS core code needs to change to take care of EFI_NOT_FOUND. > UnicodeSPrint ( > BootOptionName, sizeof (BootOptionName), L"%s%04x", > mBmLoadOptionName[LoadOptionTypeBoot], BootOptionNumber >diff --git a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c >b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c >index 741ddc3..144f786 100644 >--- a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c >+++ b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c >@@ -5,8 +5,9 @@ > After DxeCore finish DXE phase, gEfiBdsArchProtocolGuid->BdsEntry will be > invoked > to enter BDS phase. > >-(C) Copyright 2015 Hewlett-Packard Development Company, L.P.<BR> > Copyright (c) 2004 - 2016, Intel Corporation. All rights reserved.<BR> >+(C) Copyright 2016 Hewlett Packard Enterprise Development LP<BR> >+(C) Copyright 2015 Hewlett-Packard Development Company, L.P.<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 >@@ -425,22 +426,32 @@ BdsFormalizeConsoleVariable ( > > Item 3 is used to solve case when OS corrupts OsIndications. Here simply > delete this NV variable. > >+ Create a boot option for BootManagerMenu if it hasn't been created yet >+ > **/ > VOID > BdsFormalizeOSIndicationVariable ( > VOID > ) > { >- EFI_STATUS Status; >- UINT64 OsIndicationSupport; >- UINT64 OsIndication; >- UINTN DataSize; >- UINT32 Attributes; >+ EFI_STATUS Status; >+ UINT64 OsIndicationSupport; >+ UINT64 OsIndication; >+ UINTN DataSize; >+ UINT32 Attributes; >+ EFI_BOOT_MANAGER_LOAD_OPTION BootManagerMenu; > > // > // OS indicater support variable > // >- OsIndicationSupport = EFI_OS_INDICATIONS_BOOT_TO_FW_UI | >EFI_OS_INDICATIONS_START_PLATFORM_RECOVERY; >+ Status = EfiBootManagerGetBootManagerMenu (&BootManagerMenu); >+ if (!EFI_ERROR (Status)) { 5. With the above change, the if can be changed: if (Status != EFI_NOT_FOUND) { >+ OsIndicationSupport = EFI_OS_INDICATIONS_BOOT_TO_FW_UI | >EFI_OS_INDICATIONS_START_PLATFORM_RECOVERY; >+ } else { >+ OsIndicationSupport = EFI_OS_INDICATIONS_START_PLATFORM_RECOVERY; >+ } >+ EfiBootManagerFreeLoadOption (&BootManagerMenu); >+ > Status = gRT->SetVariable ( > EFI_OS_INDICATIONS_SUPPORT_VARIABLE_NAME, > &gEfiGlobalVariableGuid, >@@ -851,7 +862,9 @@ BdsEntry ( > ); > > // >- // BootManagerMenu always contains the correct information even call fails. >+ // Get BootManagerMenu boot option. If BootManagerMenu FFS section can not >+ // be retrieved, BootManagerMenu will contain invalid data to let >EfiBootManagerBoot() >+ // or other consumers directly return without doing anything. > // > EfiBootManagerGetBootManagerMenu (&BootManagerMenu); > 6. Above comments can be changed to wording like below: BootManagerMenu doesn't contain the correct information when return status is EFI_NOT_FOUND. And we need to check the return status of *GetBootManagerMenu() and initialize the BootManagerMenu.FilePath to NULL. 7. In BootBootOptions(): The following code needs to change: if (BootOptions[Index].Status == EFI_SUCCESS) { EfiBootManagerBoot (BootManagerMenu); break; } to: if (BootManagerMenu.FilePath != NULL && BootOptions[Index].Status == EFI_SUCCESS) { EfiBootManagerBoot (BootManagerMenu); break; } so that when firmware doesn't contain Boot Manager Menu, the next boot option will be automatically tried. >-- >2.5.0.windows.1 > >_______________________________________________ >edk2-devel mailing list >edk2-devel@lists.01.org >https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
