[edk2-devel] Use a pcd to control PlatformRecovery
Wang, Sunny (HPS SW)
sunnywang at hpe.com
Tue Oct 15 08:53:15 UTC 2019
Hi Zhichao and Ray,
I have some questions about this code change. Sorry for being late to bring my questions here.
For now, the code block for iterating the PlatformRecovery#### variables is controlled by OsIndications variable. However, it looks to me like that the PlatformRecovery#### should still be attempted for the case of that processing of BootOrder does NOT result in success (according to section 3.4 in UEFI 2.8). In other words, I think we should check PCD "PcdPlatformRecoverySupport" instead of Local variable "PlatformRecovery" (from OsIndications variable) like the code below. What do you guys think? If you need a meeting or short talk to discuss this, feel free to let me know.
if (!BootSuccess) {
- if (PlatformRecovery) {
+ if (PcdGetBool (PcdPlatformRecoverySupport)) {
LoadOptions = EfiBootManagerGetLoadOptions (&LoadOptionCount, LoadOptionTypePlatformRecovery);
ProcessLoadOptions (LoadOptions, LoadOptionCount);
EfiBootManagerFreeLoadOptions (LoadOptions, LoadOptionCount);
} else {
//
// When platform recovery is not enabled, still boot to platform default file path.
//
EfiBootManagerProcessLoadOption (&PlatformDefaultBootOption);
}
In addition, it looks like EDK2 don't have code to process OsRecovery#### variables. Do we need to create a Bug on TianoCore Bugzilla system?
Moreover, I saw that both of you had a discussion about "Default PlatformRecovery", but I can't figure out the connection between the discussion and the final code change. Isn't the "Default PlatformRecovery" part of the Platform Recovery feature? At this moment, we don't have OS recovery support, so I think that NO platform recovery support can be identified as NO boot option recovery support. For this case, shouldn't we use PCD "PcdPlatformRecoverySupport" to control "Default PlatformRecovery" as well? For the next step, I think we need to get further clarification from USWG to either not tie "Default Boot Behavior" with PlatformRecovery or update the description to the following:
- If system firmware supports Platform recovery as described in Section 3.4.2, system firmware must include a PlatformRecovery#### variable specifying a short-form File Path Media Device Path....
Regards,
Sunny Wang
-----Original Message-----
From: devel at edk2.groups.io [mailto:devel at edk2.groups.io] On Behalf Of Gao, Zhichao
Sent: Thursday, June 20, 2019 11:44 AM
To: devel at edk2.groups.io
Cc: Jian J Wang <jian.j.wang at intel.com>; Hao Wu <hao.a.wu at intel.com>; Ray Ni <ray.ni at intel.com>; Star Zeng <star.zeng at intel.com>; Liming Gao <liming.gao at intel.com>; Sean Brogan <sean.brogan at microsoft.com>; Michael Turner <Michael.Turner at microsoft.com>; Bret Barkelew <Bret.Barkelew at microsoft.com>
Subject: [edk2-devel] [PATCH v5 2/2] MdeModulePkg/BdsDxe: Use a pcd to control PlatformRecovery
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1678
Use the PcdPlatformRecoverySupport to control the function of platform recovery in BDS.
First, set the variable's ("OsIndicationsSupported") EFI_OS_INDICATIONS_START_PLATFORM_RECOVERY bit base on the pcd.
It would affect the variable "OsIndications".
While the platform does not support the platform recovery, it is inappropriate to set a PlatformRecovery#### variable. So skip setting the variable. But it should remain the behavior of booting from a default file path (such as \EFI\BOOT\BOOTX64.EFI) to be compatible with the previous version UEFI spec.
Add memory check before build platform default boot option. If fail to allocate memory for the defualt boot file path, put the system into dead loop to indicate it is unable to boot.
Cc: Jian J Wang <jian.j.wang at intel.com>
Cc: Hao Wu <hao.a.wu at intel.com>
Cc: Ray Ni <ray.ni at intel.com>
Cc: Star Zeng <star.zeng at intel.com>
Cc: Liming Gao <liming.gao at intel.com>
Cc: Sean Brogan <sean.brogan at microsoft.com>
Cc: Michael Turner <Michael.Turner at microsoft.com>
Cc: Bret Barkelew <Bret.Barkelew at microsoft.com>
Signed-off-by: Zhichao Gao <zhichao.gao at intel.com>
---
MdeModulePkg/Universal/BdsDxe/BdsDxe.inf | 3 +- MdeModulePkg/Universal/BdsDxe/BdsEntry.c | 71 +++++++++++++++---------
2 files changed, 47 insertions(+), 27 deletions(-)
diff --git a/MdeModulePkg/Universal/BdsDxe/BdsDxe.inf b/MdeModulePkg/Universal/BdsDxe/BdsDxe.inf
index 6913389d34..7f94ca17df 100644
--- a/MdeModulePkg/Universal/BdsDxe/BdsDxe.inf
+++ b/MdeModulePkg/Universal/BdsDxe/BdsDxe.inf
@@ -5,7 +5,7 @@
# gEfiBdsArchProtocolGuid. After DxeCore finish dispatching, DxeCore will invoke Entry # interface of protocol gEfiBdsArchProtocolGuid, then BDS phase is entered.
#
-# Copyright (c) 2008 - 2018, Intel Corporation. All rights reserved.<BR>
+# Copyright (c) 2008 - 2019, Intel Corporation. All rights
+reserved.<BR>
# SPDX-License-Identifier: BSD-2-Clause-Patent # ## @@ -95,6 +95,7 @@
gEfiMdeModulePkgTokenSpaceGuid.PcdConInConnectOnDemand ## CONSUMES
gEfiMdeModulePkgTokenSpaceGuid.PcdErrorCodeSetVariable ## SOMETIMES_CONSUMES
gEfiMdeModulePkgTokenSpaceGuid.PcdTestKeyUsed ## CONSUMES
+ gEfiMdeModulePkgTokenSpaceGuid.PcdPlatformRecoverySupport ## CONSUMES
[Depex]
TRUE
diff --git a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
index 9d312bd982..4f3168b62a 100644
--- a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
+++ b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
@@ -5,7 +5,7 @@
After DxeCore finish DXE phase, gEfiBdsArchProtocolGuid->BdsEntry will be invoked
to enter BDS phase.
-Copyright (c) 2004 - 2018, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2004 - 2019, 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>
SPDX-License-Identifier: BSD-2-Clause-Patent @@ -546,10 +546,14 @@ BdsFormalizeOSIndicationVariable (
//
Status = EfiBootManagerGetBootManagerMenu (&BootManagerMenu);
if (Status != EFI_NOT_FOUND) {
- OsIndicationSupport = EFI_OS_INDICATIONS_BOOT_TO_FW_UI | EFI_OS_INDICATIONS_START_PLATFORM_RECOVERY;
+ OsIndicationSupport = EFI_OS_INDICATIONS_BOOT_TO_FW_UI;
EfiBootManagerFreeLoadOption (&BootManagerMenu);
} else {
- OsIndicationSupport = EFI_OS_INDICATIONS_START_PLATFORM_RECOVERY;
+ OsIndicationSupport = 0;
+ }
+
+ if (PcdGetBool (PcdPlatformRecoverySupport)) {
+ OsIndicationSupport |= EFI_OS_INDICATIONS_START_PLATFORM_RECOVERY;
}
Status = gRT->SetVariable (
@@ -662,6 +666,7 @@ BdsEntry (
BOOLEAN BootSuccess;
EFI_DEVICE_PATH_PROTOCOL *FilePath;
EFI_STATUS BootManagerMenuStatus;
+ EFI_BOOT_MANAGER_LOAD_OPTION PlatformDefaultBootOption;
HotkeyTriggered = NULL;
Status = EFI_SUCCESS;
@@ -763,14 +768,13 @@ BdsEntry (
//
InitializeLanguage (TRUE);
- //
- // System firmware must include a PlatformRecovery#### variable specifying
- // a short-form File Path Media Device Path containing the platform default
- // file path for removable media
- //
FilePath = FileDevicePath (NULL, EFI_REMOVABLE_MEDIA_FILE_NAME);
+ if (FilePath == NULL) {
+ DEBUG ((DEBUG_ERROR, "Fail to allocate memory for defualt boot file path. Unable to boot.\n"));
+ CpuDeadLoop ();
+ }
Status = EfiBootManagerInitializeLoadOption (
- &LoadOption,
+ &PlatformDefaultBootOption,
LoadOptionNumberUnassigned,
LoadOptionTypePlatformRecovery,
LOAD_OPTION_ACTIVE,
@@ -780,24 +784,31 @@ BdsEntry (
0
);
ASSERT_EFI_ERROR (Status);
- LoadOptions = EfiBootManagerGetLoadOptions (&LoadOptionCount, LoadOptionTypePlatformRecovery);
- if (EfiBootManagerFindLoadOption (&LoadOption, LoadOptions, LoadOptionCount) == -1) {
- for (Index = 0; Index < LoadOptionCount; Index++) {
- //
- // The PlatformRecovery#### options are sorted by OptionNumber.
- // Find the the smallest unused number as the new OptionNumber.
- //
- if (LoadOptions[Index].OptionNumber != Index) {
- break;
+
+ //
+ // System firmware must include a PlatformRecovery#### variable
+ specifying // a short-form File Path Media Device Path containing the
+ platform default // file path for removable media if the platform supports Platform Recovery.
+ //
+ if (PcdGetBool (PcdPlatformRecoverySupport)) {
+ LoadOptions = EfiBootManagerGetLoadOptions (&LoadOptionCount, LoadOptionTypePlatformRecovery);
+ if (EfiBootManagerFindLoadOption (&PlatformDefaultBootOption, LoadOptions, LoadOptionCount) == -1) {
+ for (Index = 0; Index < LoadOptionCount; Index++) {
+ //
+ // The PlatformRecovery#### options are sorted by OptionNumber.
+ // Find the the smallest unused number as the new OptionNumber.
+ //
+ if (LoadOptions[Index].OptionNumber != Index) {
+ break;
+ }
}
+ PlatformDefaultBootOption.OptionNumber = Index;
+ Status = EfiBootManagerLoadOptionToVariable (&PlatformDefaultBootOption);
+ ASSERT_EFI_ERROR (Status);
}
- LoadOption.OptionNumber = Index;
- Status = EfiBootManagerLoadOptionToVariable (&LoadOption);
- ASSERT_EFI_ERROR (Status);
+ EfiBootManagerFreeLoadOptions (LoadOptions, LoadOptionCount);
}
- EfiBootManagerFreeLoadOption (&LoadOption);
FreePool (FilePath);
- EfiBootManagerFreeLoadOptions (LoadOptions, LoadOptionCount);
//
// Report Status Code to indicate connecting drivers will happen @@ -1043,10 +1054,18 @@ BdsEntry (
}
if (!BootSuccess) {
- LoadOptions = EfiBootManagerGetLoadOptions (&LoadOptionCount, LoadOptionTypePlatformRecovery);
- ProcessLoadOptions (LoadOptions, LoadOptionCount);
- EfiBootManagerFreeLoadOptions (LoadOptions, LoadOptionCount);
+ if (PlatformRecovery) {
+ LoadOptions = EfiBootManagerGetLoadOptions (&LoadOptionCount, LoadOptionTypePlatformRecovery);
+ ProcessLoadOptions (LoadOptions, LoadOptionCount);
+ EfiBootManagerFreeLoadOptions (LoadOptions, LoadOptionCount);
+ } else {
+ //
+ // When platform recovery is not enabled, still boot to platform default file path.
+ //
+ EfiBootManagerProcessLoadOption (&PlatformDefaultBootOption);
+ }
}
+ EfiBootManagerFreeLoadOption (&PlatformDefaultBootOption);
DEBUG ((EFI_D_ERROR, "[Bds] Unable to boot!\n"));
PlatformBootManagerUnableToBoot ();
--
2.21.0.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#48988): https://edk2.groups.io/g/devel/message/48988
Mute This Topic: https://groups.io/mt/34543609/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list