Hi Ray, The places for launching PlatformRecovery options and boot menu option (for Successful boot) in this code change look good to me. I think this patch can solve problem I mentioned in our previous discussion for PlatformBootManagerDefaultBootFail related patch. Thanks for making this code change. In addition, I made four review comments in this patch. You can search "[Sunny]" to find my comments out. Regards, Sunny Wang
-----Original Message----- From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Ruiyu Ni Sent: Monday, November 02, 2015 7:34 PM To: edk2-devel@lists.01.org Cc: Ruiyu Ni; Eric Dong Subject: [edk2] [Patch 11/11] MdeModulePkg: Enable PlatformRecovery in BdsDxe driver Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Ruiyu Ni <ruiyu...@intel.com> Cc: Eric Dong <eric.d...@intel.com> --- MdeModulePkg/Universal/BdsDxe/Bds.h | 3 - MdeModulePkg/Universal/BdsDxe/BdsEntry.c | 405 ++++++++----------------------- 2 files changed, 95 insertions(+), 313 deletions(-) diff --git a/MdeModulePkg/Universal/BdsDxe/Bds.h b/MdeModulePkg/Universal/BdsDxe/Bds.h index 2171d14..ac54482 100644 --- a/MdeModulePkg/Universal/BdsDxe/Bds.h +++ b/MdeModulePkg/Universal/BdsDxe/Bds.h @@ -24,9 +24,6 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. #include <Protocol/Bds.h> #include <Protocol/LoadedImage.h> #include <Protocol/VariableLock.h> -#include <Protocol/BlockIo.h> -#include <Protocol/LoadFile.h> -#include <Protocol/SimpleFileSystem.h> #include <Library/UefiDriverEntryPoint.h> #include <Library/DebugLib.h> diff --git a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c index b632ada..05cdb73 100644 --- a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c +++ b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c @@ -51,10 +51,10 @@ CHAR16 *mReadOnlyVariables[] = { CHAR16 *mBdsLoadOptionName[] = { L"Driver", L"SysPrep", - L"Boot" + L"Boot", + L"PlatformRecovery" }; -CHAR16 mRecoveryBoot[] = L"Recovery Boot"; /** Event to Connect ConIn. @@ -122,187 +122,6 @@ BdsInitialize ( } /** - Emuerate all possible bootable medias in the following order: - 1. Removable BlockIo - The boot option only points to the removable media - device, like USB key, DVD, Floppy etc. - 2. Fixed BlockIo - The boot option only points to a Fixed blockIo device, - like HardDisk. - 3. Non-BlockIo SimpleFileSystem - The boot option points to a device supporting - SimpleFileSystem Protocol, but not supporting BlockIo - protocol. - 4. LoadFile - The boot option points to the media supporting - LoadFile protocol. - Reference: UEFI Spec chapter 3.3 Boot Option Variables Default Boot Behavior - - @param BootOptionCount Return the boot option count which has been found. - - @retval Pointer to the boot option array. -**/ -EFI_BOOT_MANAGER_LOAD_OPTION * -BdsEnumerateBootOptions ( - UINTN *BootOptionCount - ) -{ - EFI_STATUS Status; - EFI_BOOT_MANAGER_LOAD_OPTION *BootOptions; - UINTN HandleCount; - EFI_HANDLE *Handles; - EFI_BLOCK_IO_PROTOCOL *BlkIo; - UINTN Removable; - UINTN Index; - - ASSERT (BootOptionCount != NULL); - - *BootOptionCount = 0; - BootOptions = NULL; - - // - // Parse removable block io followed by fixed block io - // - gBS->LocateHandleBuffer ( - ByProtocol, - &gEfiBlockIoProtocolGuid, - NULL, - &HandleCount, - &Handles - ); - - for (Removable = 0; Removable < 2; Removable++) { - for (Index = 0; Index < HandleCount; Index++) { - Status = gBS->HandleProtocol ( - Handles[Index], - &gEfiBlockIoProtocolGuid, - (VOID **) &BlkIo - ); - if (EFI_ERROR (Status)) { - continue; - } - - // - // Skip the logical partitions - // - if (BlkIo->Media->LogicalPartition) { - continue; - } - - // - // Skip the fixed block io then the removable block io - // - if (BlkIo->Media->RemovableMedia == ((Removable == 0) ? FALSE : TRUE)) { - continue; - } - - BootOptions = ReallocatePool ( - sizeof (EFI_BOOT_MANAGER_LOAD_OPTION) * (*BootOptionCount), - sizeof (EFI_BOOT_MANAGER_LOAD_OPTION) * (*BootOptionCount + 1), - BootOptions - ); - ASSERT (BootOptions != NULL); - - Status = EfiBootManagerInitializeLoadOption ( - &BootOptions[(*BootOptionCount)++], - LoadOptionNumberUnassigned, - LoadOptionTypeBoot, - LOAD_OPTION_ACTIVE, - mRecoveryBoot, - DevicePathFromHandle (Handles[Index]), - NULL, - 0 - ); - ASSERT_EFI_ERROR (Status); - } - } - - if (HandleCount != 0) { - FreePool (Handles); - } - - // - // Parse simple file system not based on block io - // - gBS->LocateHandleBuffer ( - ByProtocol, - &gEfiSimpleFileSystemProtocolGuid, - NULL, - &HandleCount, - &Handles - ); - for (Index = 0; Index < HandleCount; Index++) { - Status = gBS->HandleProtocol ( - Handles[Index], - &gEfiBlockIoProtocolGuid, - (VOID **) &BlkIo - ); - if (!EFI_ERROR (Status)) { - // - // Skip if the file system handle supports a BlkIo protocol, which we've handled in above - // - continue; - } - BootOptions = ReallocatePool ( - sizeof (EFI_BOOT_MANAGER_LOAD_OPTION) * (*BootOptionCount), - sizeof (EFI_BOOT_MANAGER_LOAD_OPTION) * (*BootOptionCount + 1), - BootOptions - ); - ASSERT (BootOptions != NULL); - - Status = EfiBootManagerInitializeLoadOption ( - &BootOptions[(*BootOptionCount)++], - LoadOptionNumberUnassigned, - LoadOptionTypeBoot, - LOAD_OPTION_ACTIVE, - mRecoveryBoot, - DevicePathFromHandle (Handles[Index]), - NULL, - 0 - ); - ASSERT_EFI_ERROR (Status); - } - - if (HandleCount != 0) { - FreePool (Handles); - } - - // - // Parse load file, assuming UEFI Network boot option - // - gBS->LocateHandleBuffer ( - ByProtocol, - &gEfiLoadFileProtocolGuid, - NULL, - &HandleCount, - &Handles - ); - for (Index = 0; Index < HandleCount; Index++) { - - BootOptions = ReallocatePool ( - sizeof (EFI_BOOT_MANAGER_LOAD_OPTION) * (*BootOptionCount), - sizeof (EFI_BOOT_MANAGER_LOAD_OPTION) * (*BootOptionCount + 1), - BootOptions - ); - ASSERT (BootOptions != NULL); - - Status = EfiBootManagerInitializeLoadOption ( - &BootOptions[(*BootOptionCount)++], - LoadOptionNumberUnassigned, - LoadOptionTypeBoot, - LOAD_OPTION_ACTIVE, - mRecoveryBoot, - DevicePathFromHandle (Handles[Index]), - NULL, - 0 - ); - ASSERT_EFI_ERROR (Status); - } - - if (HandleCount != 0) { - FreePool (Handles); - } - - return BootOptions; -} - -/** Function waits for a given event to fire, or for an optional timeout to expire. @param Event The event to wait for @@ -445,14 +264,16 @@ BdsWait ( @param BootOptions Input boot option array. @param BootOptionCount Input boot option count. + @param BootManagerMenu Input boot manager menu. @retval TRUE Successfully boot one of the boot options. @retval FALSE Failed boot any of the boot options. **/ BOOLEAN -BootAllBootOptions ( +BootBootOptions ( IN EFI_BOOT_MANAGER_LOAD_OPTION *BootOptions, - IN UINTN BootOptionCount + IN UINTN BootOptionCount, + IN EFI_BOOT_MANAGER_LOAD_OPTION *BootManagerMenu ) { UINTN Index; @@ -489,6 +310,7 @@ BootAllBootOptions ( // Successful boot breaks the loop, otherwise tries next boot option [Sunny] It would be good to update the comment as well because it doesn't match your modification. You can cut part of UEFI 2.5 Section 3.1.1 Boot Manager Programming, the 5th paragraph below to update the comment. -If the boot via Boot#### returns with a status of EFI_SUCCESS, platform firmware supports boot manager menu, and if firmware is configured to boot in an interactive mode, the boot manager will stop processing the BootOrder variable and present a boot manager menu to the user. if (BootOptions[Index].Status == EFI_SUCCESS) { + EfiBootManagerBoot (BootManagerMenu); break; } } @@ -497,72 +319,10 @@ BootAllBootOptions ( } /** - This function attempts to boot per the boot order specified by platform policy. - - If the boot via Boot#### returns with a status of EFI_SUCCESS the boot manager will stop - processing the BootOrder variable and present a boot manager menu to the user. If a boot via - Boot#### returns a status other than EFI_SUCCESS, the boot has failed and the next Boot#### - in the BootOrder variable will be tried until all possibilities are exhausted. - -- Chapter 3.1.1 Boot Manager Programming, the 4th paragraph -**/ -VOID -DefaultBootBehavior ( - VOID - ) -{ - UINTN BootOptionCount; - EFI_BOOT_MANAGER_LOAD_OPTION *BootOptions; - EFI_BOOT_MANAGER_LOAD_OPTION BootManagerMenu; - - EfiBootManagerGetBootManagerMenu (&BootManagerMenu); - // - // BootManagerMenu always contains the correct information even the above function returns failure. - // - - BootOptions = EfiBootManagerGetLoadOptions (&BootOptionCount, LoadOptionTypeBoot); - - if (BootAllBootOptions (BootOptions, BootOptionCount)) { - // - // Follow generic rule, Call BdsDxeOnConnectConInCallBack to connect ConIn before enter UI - // - if (PcdGetBool (PcdConInConnectOnDemand)) { - BdsDxeOnConnectConInCallBack (NULL, NULL); - } - - // - // Show the Boot Manager Menu after successful boot - // - EfiBootManagerBoot (&BootManagerMenu); - } else { - EfiBootManagerFreeLoadOptions (BootOptions, BootOptionCount); - // - // Re-scan all EFI boot options in case all the boot#### are deleted or failed to boot - // - // If no valid boot options exist, the boot manager will enumerate all removable media - // devices followed by all fixed media devices. The order within each group is undefined. - // These new default boot options are not saved to non volatile storage.The boot manger - // will then attempt toboot from each boot option. - // -- Chapter 3.3 Boot Manager Programming, the 2nd paragraph - // - EfiBootManagerConnectAll (); - BootOptions = BdsEnumerateBootOptions (&BootOptionCount); - - if (!BootAllBootOptions (BootOptions, BootOptionCount)) { - DEBUG ((EFI_D_ERROR, "[Bds]No bootable device!\n")); - EfiBootManagerBoot (&BootManagerMenu); - } - } - - EfiBootManagerFreeLoadOption (&BootManagerMenu); - EfiBootManagerFreeLoadOptions (BootOptions, BootOptionCount); -} - -/** - The function will load and start every Driver####/SysPrep####. + The function will load and start every Driver####, SysPrep#### or PlatformRecovery####. @param LoadOptions Load option array. @param LoadOptionCount Load option count. - **/ VOID ProcessLoadOptions ( @@ -575,7 +335,7 @@ ProcessLoadOptions ( BOOLEAN ReconnectAll; EFI_BOOT_MANAGER_LOAD_OPTION_TYPE LoadOptionType; - ReconnectAll = FALSE; + ReconnectAll = FALSE; LoadOptionType = LoadOptionTypeMax; [Sunny] Just a suggestion. Why don't we directly set LoadOptions[Index].OptionType to LoadOptionType here instead of LoadOptionTypeMax so that we don't need to check Index in the for-loop? // @@ -585,16 +345,23 @@ ProcessLoadOptions ( // // All the load options in the array should be of the same type. // - if (LoadOptionType == LoadOptionTypeMax) { + if (Index == 0) { LoadOptionType = LoadOptions[Index].OptionType; } ASSERT (LoadOptionType == LoadOptions[Index].OptionType); - ASSERT (LoadOptionType == LoadOptionTypeDriver || LoadOptionType == LoadOptionTypeSysPrep); Status = EfiBootManagerProcessLoadOption (&LoadOptions[Index]); - if (!EFI_ERROR (Status) && ((LoadOptions[Index].Attributes & LOAD_OPTION_FORCE_RECONNECT) != 0)) { - ReconnectAll = TRUE; + if (!EFI_ERROR (Status)) { + if (LoadOptionType == LoadOptionTypePlatformRecovery) { + // + // Stop processing if any entry is successful + // + break; + } + if ((LoadOptions[Index].Attributes & LOAD_OPTION_FORCE_RECONNECT) != 0) { + ReconnectAll = TRUE; + } } } @@ -670,7 +437,7 @@ BdsFormalizeOSIndicationVariable ( // // OS indicater support variable // - OsIndicationSupport = EFI_OS_INDICATIONS_BOOT_TO_FW_UI; + OsIndicationSupport = EFI_OS_INDICATIONS_BOOT_TO_FW_UI | EFI_OS_INDICATIONS_START_PLATFORM_RECOVERY; Status = gRT->SetVariable ( EFI_OS_INDICATIONS_SUPPORT_VARIABLE_NAME, &gEfiGlobalVariableGuid, @@ -828,6 +595,8 @@ BdsEntry ( CHAR16 BootNextVariableName[sizeof ("Boot####")]; EFI_BOOT_MANAGER_LOAD_OPTION BootManagerMenu; BOOLEAN BootFwUi; + BOOLEAN PlatformRecovery; + BOOLEAN BootSuccess; EFI_DEVICE_PATH_PROTOCOL *FilePath; HotkeyTriggered = NULL; [Sunny] Do we need to initialize BootSuccess to FALSE here? It looks like the uninitialized BootSuccess variable would cause build failure. @@ -1038,9 +807,24 @@ BdsEntry ( PERF_START (NULL, "PlatformBootManagerAfterConsole", "BDS", 0); PlatformBootManagerAfterConsole (); PERF_END (NULL, "PlatformBootManagerAfterConsole", "BDS", 0); + // + // Boot to Boot Manager Menu when EFI_OS_INDICATIONS_BOOT_TO_FW_UI is set. Skip HotkeyBoot + // + DataSize = sizeof (UINT64); + Status = gRT->GetVariable ( + EFI_OS_INDICATIONS_VARIABLE_NAME, + &gEfiGlobalVariableGuid, + NULL, + &DataSize, + &OsIndication + ); + if (EFI_ERROR (Status)) { + OsIndication = 0; + } DEBUG_CODE ( EFI_BOOT_MANAGER_LOAD_OPTION_TYPE LoadOptionType; + DEBUG ((EFI_D_INFO, "[Bds]OsIndication: %016x\n", OsIndication)); DEBUG ((EFI_D_INFO, "[Bds]=============Begin Load Options Dumping ...=============\n")); for (LoadOptionType = 0; LoadOptionType < LoadOptionTypeMax; LoadOptionType++) { DEBUG (( @@ -1063,26 +847,17 @@ BdsEntry ( ); // - // Boot to Boot Manager Menu when EFI_OS_INDICATIONS_BOOT_TO_FW_UI is set. Skip HotkeyBoot + // BootManagerMenu always contains the correct information even call fails. // - DataSize = sizeof (UINT64); - Status = gRT->GetVariable ( - EFI_OS_INDICATIONS_VARIABLE_NAME, - &gEfiGlobalVariableGuid, - NULL, - &DataSize, - &OsIndication - ); - if (EFI_ERROR (Status)) { - OsIndication = 0; - } + EfiBootManagerGetBootManagerMenu (&BootManagerMenu); - BootFwUi = (BOOLEAN) ((OsIndication & EFI_OS_INDICATIONS_BOOT_TO_FW_UI) != 0); + BootFwUi = (BOOLEAN) ((OsIndication & EFI_OS_INDICATIONS_BOOT_TO_FW_UI) != 0); + PlatformRecovery = (BOOLEAN) ((OsIndication & EFI_OS_INDICATIONS_START_PLATFORM_RECOVERY) != 0); // // Clear EFI_OS_INDICATIONS_BOOT_TO_FW_UI to acknowledge OS // - if (BootFwUi) { - OsIndication &= ~((UINT64) EFI_OS_INDICATIONS_BOOT_TO_FW_UI); + if (BootFwUi || PlatformRecovery) { + OsIndication &= ~((UINT64) (EFI_OS_INDICATIONS_BOOT_TO_FW_UI | EFI_OS_INDICATIONS_START_PLATFORM_RECOVERY)); Status = gRT->SetVariable ( EFI_OS_INDICATIONS_VARIABLE_NAME, &gEfiGlobalVariableGuid, @@ -1109,62 +884,72 @@ BdsEntry ( // // Directly enter the setup page. - // BootManagerMenu always contains the correct information even call fails. // - EfiBootManagerGetBootManagerMenu (&BootManagerMenu); EfiBootManagerBoot (&BootManagerMenu); - EfiBootManagerFreeLoadOption (&BootManagerMenu); } - // - // Execute SysPrep#### - // - LoadOptions = EfiBootManagerGetLoadOptions (&LoadOptionCount, LoadOptionTypeSysPrep); - ProcessLoadOptions (LoadOptions, LoadOptionCount); - EfiBootManagerFreeLoadOptions (LoadOptions, LoadOptionCount); + Status = EFI_SUCCESS; [Sunny] May I know why we need to set Status to EFI_SUCCESS here? If we need to do this, could you add comment here? :) - // - // Execute Key#### - // - PERF_START (NULL, "BdsWait", "BDS", 0); - BdsWait (HotkeyTriggered); - PERF_END (NULL, "BdsWait", "BDS", 0); + if (!PlatformRecovery) { + // + // Execute SysPrep#### + // + LoadOptions = EfiBootManagerGetLoadOptions (&LoadOptionCount, LoadOptionTypeSysPrep); + ProcessLoadOptions (LoadOptions, LoadOptionCount); + EfiBootManagerFreeLoadOptions (LoadOptions, LoadOptionCount); - // - // BdsReadKeys() be removed after all keyboard drivers invoke callback in timer callback. - // - BdsReadKeys (); + // + // Execute Key#### + // + PERF_START (NULL, "BdsWait", "BDS", 0); + BdsWait (HotkeyTriggered); + PERF_END (NULL, "BdsWait", "BDS", 0); - EfiBootManagerHotkeyBoot (); + // + // BdsReadKeys() can be removed after all keyboard drivers invoke callback in timer callback. + // + BdsReadKeys (); - // - // Boot to "BootNext" - // - if (BootNext != NULL) { - UnicodeSPrint (BootNextVariableName, sizeof (BootNextVariableName), L"Boot%04x", *BootNext); - Status = EfiBootManagerVariableToLoadOption (BootNextVariableName, &LoadOption); - if (!EFI_ERROR (Status)) { - EfiBootManagerBoot (&LoadOption); - EfiBootManagerFreeLoadOption (&LoadOption); - if (LoadOption.Status == EFI_SUCCESS) { - // - // Boot to Boot Manager Menu upon EFI_SUCCESS - // - EfiBootManagerGetBootManagerMenu (&LoadOption); + EfiBootManagerHotkeyBoot (); + + // + // Boot to "BootNext" + // + if (BootNext != NULL) { + UnicodeSPrint (BootNextVariableName, sizeof (BootNextVariableName), L"Boot%04x", *BootNext); + Status = EfiBootManagerVariableToLoadOption (BootNextVariableName, &LoadOption); + if (!EFI_ERROR (Status)) { EfiBootManagerBoot (&LoadOption); EfiBootManagerFreeLoadOption (&LoadOption); + if (LoadOption.Status == EFI_SUCCESS) { + // + // Boot to Boot Manager Menu upon EFI_SUCCESS + // + EfiBootManagerBoot (&BootManagerMenu); + } } } + + do { + // + // Retry to boot if any of the boot succeeds + // + LoadOptions = EfiBootManagerGetLoadOptions (&LoadOptionCount, LoadOptionTypeBoot); + BootSuccess = BootBootOptions (LoadOptions, LoadOptionCount, &BootManagerMenu); + EfiBootManagerFreeLoadOptions (LoadOptions, LoadOptionCount); + } while (BootSuccess); } - while (TRUE) { - // - // BDS select the boot device to load OS - // Try next upon boot failure - // Show Boot Manager Menu upon boot success - // - DefaultBootBehavior (); + EfiBootManagerFreeLoadOption (&BootManagerMenu); + + if (!BootSuccess || PlatformRecovery) { + LoadOptions = EfiBootManagerGetLoadOptions (&LoadOptionCount, LoadOptionTypePlatformRecovery); + ProcessLoadOptions (LoadOptions, LoadOptionCount); + EfiBootManagerFreeLoadOptions (LoadOptions, LoadOptionCount); } + + DEBUG ((EFI_D_ERROR, "[Bds] Unable to boot!\n")); + CpuDeadLoop (); } /** -- 1.9.5.msysgit.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