Sunny, I guess you may have misunderstanding about the EndOfDxe signal time. This event is not signaled immediately before calling Bds->Entry(). This event is signaled by platform (in PlatformBootManagerBeforeConsole). So it means all the PlatformRecovery#### variables need to be created in core (before PlatformBootManagerBeforeConsole()) or in PlatformBootManagerBeforeConsole().
I agree with your other comments. Regards, Ray -----Original Message----- From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Wang, Sunny (HPS SW) Sent: Thursday, November 5, 2015 9:17 PM To: Ni, Ruiyu <ruiyu...@intel.com>; edk2-devel@lists.01.org Cc: Dong, Eric <eric.d...@intel.com> Subject: Re: [edk2] [Patch 07/11] MdeModulePkg: Add Platform recovery support Hi Ray, I made four comments in this patch. You can search "[Sunny]" to find them 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 07/11] MdeModulePkg: Add Platform recovery support Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Ruiyu Ni <ruiyu...@intel.com> Cc: Eric Dong <eric.d...@intel.com> --- MdeModulePkg/Include/Library/UefiBootManagerLib.h | 1 + .../Library/UefiBootManagerLib/BmLoadOption.c | 130 ++++++++++++++++++--- .../Library/UefiBootManagerLib/InternalBm.h | 3 +- .../UefiBootManagerLib/UefiBootManagerLib.inf | 1 + 4 files changed, 118 insertions(+), 17 deletions(-) diff --git a/MdeModulePkg/Include/Library/UefiBootManagerLib.h b/MdeModulePkg/Include/Library/UefiBootManagerLib.h index 5538d90..1b04a8c 100644 --- a/MdeModulePkg/Include/Library/UefiBootManagerLib.h +++ b/MdeModulePkg/Include/Library/UefiBootManagerLib.h @@ -30,6 +30,7 @@ typedef enum { LoadOptionTypeDriver, LoadOptionTypeSysPrep, LoadOptionTypeBoot, + LoadOptionTypePlatformRecovery, LoadOptionTypeMax } EFI_BOOT_MANAGER_LOAD_OPTION_TYPE; diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmLoadOption.c b/MdeModulePkg/Library/UefiBootManagerLib/BmLoadOption.c index fbd7830..454fe20 100644 --- a/MdeModulePkg/Library/UefiBootManagerLib/BmLoadOption.c +++ b/MdeModulePkg/Library/UefiBootManagerLib/BmLoadOption.c @@ -18,14 +18,16 @@ GLOBAL_REMOVE_IF_UNREFERENCED CHAR16 *mBmLoadOptionName[] = { L"Driver", L"SysPrep", - L"Boot" + L"Boot", + L"PlatformRecovery" }; GLOBAL_REMOVE_IF_UNREFERENCED CHAR16 *mBmLoadOptionOrderName[] = { EFI_DRIVER_ORDER_VARIABLE_NAME, EFI_SYS_PREP_ORDER_VARIABLE_NAME, - EFI_BOOT_ORDER_VARIABLE_NAME + EFI_BOOT_ORDER_VARIABLE_NAME, + NULL // PlatformRecovery#### doesn't have associated *Order + variable }; /** @@ -153,8 +155,9 @@ BmGetFreeOptionNumber ( } /** - Create the Boot####, Driver####, SysPrep####, variable from the load option. - + Create the Boot####, Driver####, SysPrep####, PlatformRecovery#### + variable from the load option. + @param LoadOption Pointer to the load option. @retval EFI_SUCCESS The variable was created. @@ -166,12 +169,14 @@ EfiBootManagerLoadOptionToVariable ( IN CONST EFI_BOOT_MANAGER_LOAD_OPTION *Option ) { + EFI_STATUS Status; UINTN VariableSize; UINT8 *Variable; UINT8 *Ptr; CHAR16 OptionName[BM_OPTION_NAME_LEN]; CHAR16 *Description; CHAR16 NullChar; + EDKII_VARIABLE_LOCK_PROTOCOL *VariableLock; UINT32 VariableAttributes; if ((Option->OptionNumber == LoadOptionNumberUnassigned) || @@ -232,6 +237,17 @@ structure. UnicodeSPrint (OptionName, sizeof (OptionName), L"%s%04x", mBmLoadOptionName[Option->OptionType], Option->OptionNumber); VariableAttributes = EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS | EFI_VARIABLE_NON_VOLATILE; + if (Option->OptionType == LoadOptionTypePlatformRecovery) { + // + // Lock the PlatformRecovery#### + // + Status = gBS->LocateProtocol (&gEdkiiVariableLockProtocolGuid, NULL, (VOID **) &VariableLock); + if (!EFI_ERROR (Status)) { + Status = VariableLock->RequestToLock (VariableLock, OptionName, &gEfiGlobalVariableGuid); + ASSERT_EFI_ERROR (Status); + } + VariableAttributes = EFI_VARIABLE_BOOTSERVICE_ACCESS | + EFI_VARIABLE_RUNTIME_ACCESS; } [Sunny] Should we lock the variable after calling SetVariable? It seems the locked variable can't be set. [Sunny] I saw the UEFI spec said that PlatformRecovery#### variables are only modified by firmware and are read-only to the OS. However, it seems the locked variable would possibly become read-only after Signaling end of DXE event (at the beginning of BDS). If my understanding is correct, that means the firmware will have NO chance to modify the created PlatformRecovery#### variable in BDS once it has been locked. Is it a problem or I have misunderstanding on this? return gRT->SetVariable ( OptionName, @@ -548,6 +564,7 @@ EfiBootManagerDeleteLoadOptionVariable ( UINTN OptionOrderSize; EFI_STATUS Status; UINTN Index; + CHAR16 OptionName[BM_OPTION_NAME_LEN]; if (((UINT32) OptionType >= LoadOptionTypeMax) || (OptionNumber >= LoadOptionNumberMax)) { return EFI_INVALID_PARAMETER; @@ -579,6 +596,19 @@ EfiBootManagerDeleteLoadOptionVariable ( if (OptionOrder != NULL) { FreePool (OptionOrder); } + } else { [Sunny] I would like to make sure that changing to different behavior for the unsupported OptionType is intended. The original code would directly return EFI_NOT_FOUND and do nothing for the unsupported OptionType. However, in this patch, the unsupported OptionType will cause ASSERT and would possibly still run this code block to SetVariable. Could you check whether this is an expected behavior? Is it change to "} else if (OptionType == LoadOptionTypePlatformRecovery) {" instead of "} else {" and "ASSERT (....." + ASSERT (OptionType == LoadOptionTypePlatformRecovery); + // + // PlatformRecovery#### doesn't have assiciated PlatformRecoveryOrder, remove the PlatformRecovery#### itself. + // + UnicodeSPrint (OptionName, sizeof (OptionName), L"%s%04x", mBmLoadOptionName[OptionType], OptionNumber); + Status = gRT->SetVariable ( + OptionName, + &gEfiGlobalVariableGuid, + 0, + 0, + NULL + ); } return Status; @@ -675,7 +705,8 @@ BmStrSizeEx ( } /** - Validate the Boot####, Driver####, SysPrep#### variable (VendorGuid/Name) + Validate the Boot####, Driver####, SysPrep#### and + PlatformRecovery#### variable (VendorGuid/Name) @param Variable The variable data. @param VariableSize The variable size. @@ -918,6 +949,62 @@ EfiBootManagerVariableToLoadOption ( return EfiBootManagerVariableToLoadOptionEx (VariableName, &gEfiGlobalVariableGuid, Option); } +typedef struct { + EFI_BOOT_MANAGER_LOAD_OPTION_TYPE OptionType; + EFI_GUID *Guid; + EFI_BOOT_MANAGER_LOAD_OPTION *Options; + UINTN OptionCount; +} BM_COLLECT_LOAD_OPTIONS_PARAM; + +/** + Visitor function to collect the Platform Recovery load options or OS +Recovery + load options from NV storage. + + @param Name Variable name. + @param Guid Variable GUID. + @param Context The same context passed to BmForEachVariable. +**/ +VOID +BmCollectLoadOptions ( + IN CHAR16 *Name, + IN EFI_GUID *Guid, + IN VOID *Context + ) +{ + EFI_STATUS Status; + EFI_BOOT_MANAGER_LOAD_OPTION_TYPE OptionType; + UINT16 OptionNumber; + EFI_BOOT_MANAGER_LOAD_OPTION Option; + UINTN Index; + BM_COLLECT_LOAD_OPTIONS_PARAM *Param; + + Param = (BM_COLLECT_LOAD_OPTIONS_PARAM *) Context; + + if (CompareGuid (Guid, Param->Guid) && ( + Param->OptionType == LoadOptionTypePlatformRecovery && + BmIsValidLoadOptionVariableName (Name, &OptionType, &OptionNumber) && + OptionType == LoadOptionTypePlatformRecovery + )) { + Status = EfiBootManagerVariableToLoadOptionEx (Name, Guid, &Option); + if (!EFI_ERROR (Status)) { + for (Index = 0; Index < Param->OptionCount; Index++) { + if (Param->Options[Index].OptionNumber > Option.OptionNumber) { + break; + } + } + Param->Options = ReallocatePool ( + Param->OptionCount * sizeof (EFI_BOOT_MANAGER_LOAD_OPTION), + (Param->OptionCount + 1) * sizeof (EFI_BOOT_MANAGER_LOAD_OPTION), + Param->Options + ); + ASSERT (Param->Options != NULL); + CopyMem (&Param->Options[Index + 1], &Param->Options[Index], (Param->OptionCount - Index) * sizeof (EFI_BOOT_MANAGER_LOAD_OPTION)); + CopyMem (&Param->Options[Index], &Option, sizeof (EFI_BOOT_MANAGER_LOAD_OPTION)); + Param->OptionCount++; + } + } +} + /** Returns an array of load options based on the EFI variable L"BootOrder"/L"DriverOrder" and the L"Boot####"/L"Driver####" variables impled by it. @@ -937,16 +1024,18 @@ EfiBootManagerGetLoadOptions ( IN EFI_BOOT_MANAGER_LOAD_OPTION_TYPE LoadOptionType ) { - EFI_STATUS Status; - UINT16 *OptionOrder; - UINTN OptionOrderSize; - UINTN Index; - UINTN OptionIndex; - EFI_BOOT_MANAGER_LOAD_OPTION *Options; - CHAR16 OptionName[BM_OPTION_NAME_LEN]; - UINT16 OptionNumber; + EFI_STATUS Status; + UINT16 *OptionOrder; + UINTN OptionOrderSize; + UINTN Index; + UINTN OptionIndex; + EFI_BOOT_MANAGER_LOAD_OPTION *Options; + CHAR16 OptionName[BM_OPTION_NAME_LEN]; + UINT16 OptionNumber; + BM_COLLECT_LOAD_OPTIONS_PARAM Param; *OptionCount = 0; + Options = NULL; if (LoadOptionType == LoadOptionTypeDriver || LoadOptionType == LoadOptionTypeSysPrep || LoadOptionType == LoadOptionTypeBoot) { // @@ -987,8 +1076,16 @@ EfiBootManagerGetLoadOptions ( *OptionCount = OptionIndex; } - } else { - return NULL; + } else if (LoadOptionType == LoadOptionTypePlatformRecovery) { + Param.OptionType = LoadOptionTypePlatformRecovery; + Param.Options = NULL; + Param.OptionCount = 0; + Param.Guid = &gEfiGlobalVariableGuid; + + BmForEachVariable (BmCollectLoadOptions, (VOID *) &Param); + + *OptionCount = Param.OptionCount; + Options = Param.Options; } return Options; @@ -1116,7 +1213,8 @@ BmIsLoadOptionPeHeaderValid ( if ((Type == LoadOptionTypeDriver && Subsystem == EFI_IMAGE_SUBSYSTEM_EFI_BOOT_SERVICE_DRIVER) || (Type == LoadOptionTypeDriver && Subsystem == EFI_IMAGE_SUBSYSTEM_EFI_RUNTIME_DRIVER) || (Type == LoadOptionTypeSysPrep && Subsystem == EFI_IMAGE_SUBSYSTEM_EFI_APPLICATION) || - (Type == LoadOptionTypeBoot && Subsystem == EFI_IMAGE_SUBSYSTEM_EFI_APPLICATION) + (Type == LoadOptionTypeBoot && Subsystem == EFI_IMAGE_SUBSYSTEM_EFI_APPLICATION) || + (Type == LoadOptionTypePlatformRecovery && Subsystem == + EFI_IMAGE_SUBSYSTEM_EFI_APPLICATION) ) { return TRUE; } diff --git a/MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h b/MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h index 7f52b13..3fc9cc7 100644 --- a/MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h +++ b/MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h @@ -41,6 +41,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. #include <Protocol/BootLogo.h> #include <Protocol/DriverHealth.h> #include <Protocol/FormBrowser2.h> +#include <Protocol/VariableLock.h> #include <Guid/ZeroGuid.h> #include <Guid/MemoryTypeInformation.h> @@ -101,7 +102,7 @@ CHAR16 * IN EFI_HANDLE Handle ); [Sunny] Just a suggestion. Is it a good idea to rename it to BM_OPTION_NAME_MAX_LEN or add comment here to explain why we use PlatformRecovery#### to get BM_OPTION_NAME_LEN (because it is the option variable with longest name)? -#define BM_OPTION_NAME_LEN sizeof ("SysPrep####") +#define BM_OPTION_NAME_LEN sizeof ("PlatformRecovery####") extern CHAR16 *mBmLoadOptionName[]; /** diff --git a/MdeModulePkg/Library/UefiBootManagerLib/UefiBootManagerLib.inf b/MdeModulePkg/Library/UefiBootManagerLib/UefiBootManagerLib.inf index a2c6441..f1f6246 100644 --- a/MdeModulePkg/Library/UefiBootManagerLib/UefiBootManagerLib.inf +++ b/MdeModulePkg/Library/UefiBootManagerLib/UefiBootManagerLib.inf @@ -101,6 +101,7 @@ gEfiDevicePathProtocolGuid ## CONSUMES gEfiBootLogoProtocolGuid ## CONSUMES gEfiSimpleTextInputExProtocolGuid ## CONSUMES + gEdkiiVariableLockProtocolGuid ## CONSUMES gEfiGraphicsOutputProtocolGuid ## SOMETIMES_CONSUMES gEfiUsbIoProtocolGuid ## SOMETIMES_CONSUMES gEfiDiskInfoProtocolGuid ## SOMETIMES_CONSUMES -- 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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
