* [PATCH 0/2] Use a pcd to control Platform Recovery behavior
@ 2019-06-03 6:43 Gao, Zhichao
2019-06-03 6:43 ` [PATCH 1/2] MdeModulePkg: Add a pcd to set the OS indications bit Gao, Zhichao
2019-06-03 6:43 ` [PATCH 2/2] MdeModulePkg/BdsDxe: Use a pcd to control PlatformRecovery Gao, Zhichao
0 siblings, 2 replies; 8+ messages in thread
From: Gao, Zhichao @ 2019-06-03 6:43 UTC (permalink / raw)
To: devel
Cc: Jian J Wang, Hao Wu, Ray Ni, Star Zeng, Liming Gao, Sean Brogan,
Michael Turner, Bret Barkelew
V1:
Add a pcd PcdPlatformRecoverySupport to control the variable
PlatformRecovery#### and the EFI_OS_INDICATIONS_START_PLATFORM_RECOVERY
bit of variable "OsIndicationsSupported".
V2:
While PcdPlatformRecoverySupport is FALSE, do not set a PlatformRecovery####
Variable.
But remain boot from a default file path(such as \EFI\BOOT\BOOTX64.EFI).
Add memory check before build platform default boot option.
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao Wu <hao.a.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Sean Brogan <sean.brogan@microsoft.com>
Cc: Michael Turner <Michael.Turner@microsoft.com>
Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
Bret Barkelew (1):
MdeModulePkg: Add a pcd to set the OS indications bit
Zhichao Gao (1):
MdeModulePkg/BdsDxe: Use a pcd to control PlatformRecovery
MdeModulePkg/MdeModulePkg.dec | 6 ++
MdeModulePkg/Universal/BdsDxe/BdsDxe.inf | 3 +-
MdeModulePkg/Universal/BdsDxe/BdsEntry.c | 89 ++++++++++++++----------
3 files changed, 61 insertions(+), 37 deletions(-)
--
2.21.0.windows.1
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/2] MdeModulePkg: Add a pcd to set the OS indications bit
2019-06-03 6:43 [PATCH 0/2] Use a pcd to control Platform Recovery behavior Gao, Zhichao
@ 2019-06-03 6:43 ` Gao, Zhichao
2019-06-03 6:59 ` Ni, Ray
2019-06-03 6:43 ` [PATCH 2/2] MdeModulePkg/BdsDxe: Use a pcd to control PlatformRecovery Gao, Zhichao
1 sibling, 1 reply; 8+ messages in thread
From: Gao, Zhichao @ 2019-06-03 6:43 UTC (permalink / raw)
To: devel
Cc: Bret Barkelew, Jian J Wang, Hao Wu, Ray Ni, Star Zeng, Liming Gao,
Sean Brogan, Michael Turner
From: Bret Barkelew <Bret.Barkelew@microsoft.com>
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1678
Add a pcd PcdPlatformRecoverySupport to conditionally
set an OS indications bit and set the 'PlatformRecovery####'
variable. This pcd would also control whether the BDS
supports platform recovery or not.
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao Wu <hao.a.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Sean Brogan <sean.brogan@microsoft.com>
Cc: Michael Turner <Michael.Turner@microsoft.com>
Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
---
MdeModulePkg/MdeModulePkg.dec | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
index 0a9fcddecc..da2b85770c 100644
--- a/MdeModulePkg/MdeModulePkg.dec
+++ b/MdeModulePkg/MdeModulePkg.dec
@@ -1660,6 +1660,12 @@
# @Prompt Reset on memory type information change.
gEfiMdeModulePkgTokenSpaceGuid.PcdResetOnMemoryTypeInformationChange|TRUE|BOOLEAN|0x00010056
+ ## Indicates if the BDS supports Platform Recovery.<BR><BR>
+ # TRUE - BDS supports Platform Recovery.<BR>
+ # FALSE - BDS does not support Platform Recovery.<BR>
+ # @Prompt Support Platform Recovery.
+ gEfiMdeModulePkgTokenSpaceGuid.PcdPlatformRecoverySupport|TRUE|BOOLEAN|0x00010078
+
## Specify the foreground color for Subtile text in HII Form Browser. The default value is EFI_BLUE.
# Only following values defined in UEFI specification are valid:<BR><BR>
# 0x00 (EFI_BLACK)<BR>
--
2.21.0.windows.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/2] MdeModulePkg/BdsDxe: Use a pcd to control PlatformRecovery
2019-06-03 6:43 [PATCH 0/2] Use a pcd to control Platform Recovery behavior Gao, Zhichao
2019-06-03 6:43 ` [PATCH 1/2] MdeModulePkg: Add a pcd to set the OS indications bit Gao, Zhichao
@ 2019-06-03 6:43 ` Gao, Zhichao
2019-06-03 6:59 ` Ni, Ray
1 sibling, 1 reply; 8+ messages in thread
From: Gao, Zhichao @ 2019-06-03 6:43 UTC (permalink / raw)
To: devel
Cc: Jian J Wang, Hao Wu, Ray Ni, Star Zeng, Liming Gao, Sean Brogan,
Michael Turner, Bret Barkelew
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.
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao Wu <hao.a.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Sean Brogan <sean.brogan@microsoft.com>
Cc: Michael Turner <Michael.Turner@microsoft.com>
Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
---
MdeModulePkg/Universal/BdsDxe/BdsDxe.inf | 3 +-
MdeModulePkg/Universal/BdsDxe/BdsEntry.c | 89 ++++++++++++++----------
2 files changed, 55 insertions(+), 37 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..3d84d5a8aa 100644
--- a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
+++ b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
@@ -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,41 +768,45 @@ 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);
- Status = EfiBootManagerInitializeLoadOption (
- &LoadOption,
- LoadOptionNumberUnassigned,
- LoadOptionTypePlatformRecovery,
- LOAD_OPTION_ACTIVE,
- L"Default PlatformRecovery",
- FilePath,
- NULL,
- 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;
+ if (FilePath != NULL) {
+ Status = EfiBootManagerInitializeLoadOption (
+ &PlatformDefaultBootOption,
+ LoadOptionNumberUnassigned,
+ LoadOptionTypePlatformRecovery,
+ LOAD_OPTION_ACTIVE,
+ L"Default PlatformRecovery",
+ FilePath,
+ NULL,
+ 0
+ );
+ ASSERT_EFI_ERROR (Status);
+
+ //
+ // 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 (&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;
+ }
+ }
+ PlatformDefaultBootOption.OptionNumber = Index;
+ Status = EfiBootManagerLoadOptionToVariable (&PlatformDefaultBootOption);
+ ASSERT_EFI_ERROR (Status);
}
+ EfiBootManagerFreeLoadOptions (LoadOptions, LoadOptionCount);
}
- LoadOption.OptionNumber = Index;
- Status = EfiBootManagerLoadOptionToVariable (&LoadOption);
- ASSERT_EFI_ERROR (Status);
+ FreePool (FilePath);
}
- EfiBootManagerFreeLoadOption (&LoadOption);
- FreePool (FilePath);
- EfiBootManagerFreeLoadOptions (LoadOptions, LoadOptionCount);
//
// Report Status Code to indicate connecting drivers will happen
@@ -1043,10 +1052,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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] MdeModulePkg/BdsDxe: Use a pcd to control PlatformRecovery
2019-06-03 6:43 ` [PATCH 2/2] MdeModulePkg/BdsDxe: Use a pcd to control PlatformRecovery Gao, Zhichao
@ 2019-06-03 6:59 ` Ni, Ray
2019-06-03 7:13 ` Gao, Zhichao
0 siblings, 1 reply; 8+ messages in thread
From: Ni, Ray @ 2019-06-03 6:59 UTC (permalink / raw)
To: Gao, Zhichao, devel@edk2.groups.io
Cc: Wang, Jian J, Wu, Hao A, Zeng, Star, Gao, Liming, Sean Brogan,
Michael Turner, Bret Barkelew
> + //
> + // When platform recovery is not enabled, still boot to platform default
> file path.
> + //
> + EfiBootManagerProcessLoadOption (&PlatformDefaultBootOption);
> + }
> }
> + EfiBootManagerFreeLoadOption (&PlatformDefaultBootOption);
PlatformDefaultBootOption might be uninitialized if FilePath is NULL.
FilePath is NULL when memory allocation fails.
So I am thinking maybe ASSERT (FilePath != NULL) is enough when
EfiBootManagerInitializeLoadOption().
>
> DEBUG ((EFI_D_ERROR, "[Bds] Unable to boot!\n"));
> PlatformBootManagerUnableToBoot ();
> --
> 2.21.0.windows.1
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] MdeModulePkg: Add a pcd to set the OS indications bit
2019-06-03 6:43 ` [PATCH 1/2] MdeModulePkg: Add a pcd to set the OS indications bit Gao, Zhichao
@ 2019-06-03 6:59 ` Ni, Ray
0 siblings, 0 replies; 8+ messages in thread
From: Ni, Ray @ 2019-06-03 6:59 UTC (permalink / raw)
To: Gao, Zhichao, devel@edk2.groups.io
Cc: Bret Barkelew, Wang, Jian J, Wu, Hao A, Zeng, Star, Gao, Liming,
Sean Brogan, Michael Turner
Reviewed-by: Ray Ni <ray.ni@intel.com>
> -----Original Message-----
> From: Gao, Zhichao <zhichao.gao@intel.com>
> Sent: Monday, June 3, 2019 2:44 PM
> To: devel@edk2.groups.io
> Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>; Wang, Jian J
> <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray
> <ray.ni@intel.com>; Zeng, Star <star.zeng@intel.com>; Gao, Liming
> <liming.gao@intel.com>; Sean Brogan <sean.brogan@microsoft.com>;
> Michael Turner <Michael.Turner@microsoft.com>
> Subject: [PATCH 1/2] MdeModulePkg: Add a pcd to set the OS indications bit
>
> From: Bret Barkelew <Bret.Barkelew@microsoft.com>
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1678
>
> Add a pcd PcdPlatformRecoverySupport to conditionally set an OS indications
> bit and set the 'PlatformRecovery####'
> variable. This pcd would also control whether the BDS supports platform
> recovery or not.
>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Hao Wu <hao.a.wu@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Sean Brogan <sean.brogan@microsoft.com>
> Cc: Michael Turner <Michael.Turner@microsoft.com>
> Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
> Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
> ---
> MdeModulePkg/MdeModulePkg.dec | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/MdeModulePkg/MdeModulePkg.dec
> b/MdeModulePkg/MdeModulePkg.dec index 0a9fcddecc..da2b85770c
> 100644
> --- a/MdeModulePkg/MdeModulePkg.dec
> +++ b/MdeModulePkg/MdeModulePkg.dec
> @@ -1660,6 +1660,12 @@
> # @Prompt Reset on memory type information change.
>
> gEfiMdeModulePkgTokenSpaceGuid.PcdResetOnMemoryTypeInformationC
> hange|TRUE|BOOLEAN|0x00010056
>
> + ## Indicates if the BDS supports Platform Recovery.<BR><BR>
> + # TRUE - BDS supports Platform Recovery.<BR>
> + # FALSE - BDS does not support Platform Recovery.<BR>
> + # @Prompt Support Platform Recovery.
> +
> +
> gEfiMdeModulePkgTokenSpaceGuid.PcdPlatformRecoverySupport|TRUE|B
> OOLEAN
> + |0x00010078
> +
> ## Specify the foreground color for Subtile text in HII Form Browser. The
> default value is EFI_BLUE.
> # Only following values defined in UEFI specification are valid:<BR><BR>
> # 0x00 (EFI_BLACK)<BR>
> --
> 2.21.0.windows.1
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] MdeModulePkg/BdsDxe: Use a pcd to control PlatformRecovery
2019-06-03 6:59 ` Ni, Ray
@ 2019-06-03 7:13 ` Gao, Zhichao
2019-06-03 7:26 ` Ni, Ray
0 siblings, 1 reply; 8+ messages in thread
From: Gao, Zhichao @ 2019-06-03 7:13 UTC (permalink / raw)
To: Ni, Ray, devel@edk2.groups.io
Cc: Wang, Jian J, Wu, Hao A, Zeng, Star, Gao, Liming, Sean Brogan,
Michael Turner, Bret Barkelew
> -----Original Message-----
> From: Ni, Ray
> Sent: Monday, June 3, 2019 2:59 PM
> To: Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>;
> Zeng, Star <star.zeng@intel.com>; Gao, Liming <liming.gao@intel.com>;
> Sean Brogan <sean.brogan@microsoft.com>; Michael Turner
> <Michael.Turner@microsoft.com>; Bret Barkelew
> <Bret.Barkelew@microsoft.com>
> Subject: RE: [PATCH 2/2] MdeModulePkg/BdsDxe: Use a pcd to control
> PlatformRecovery
>
> > + //
> > + // When platform recovery is not enabled, still boot to
> > + platform default
> > file path.
> > + //
> > + EfiBootManagerProcessLoadOption (&PlatformDefaultBootOption);
> > + }
> > }
> > + EfiBootManagerFreeLoadOption (&PlatformDefaultBootOption);
>
> PlatformDefaultBootOption might be uninitialized if FilePath is NULL.
EfiBootManagerFreeLoadOption would check the pointer before free. So it is safe regardless of the PlatformDefaultBootOption is initialized or not.
>
> FilePath is NULL when memory allocation fails.
> So I am thinking maybe ASSERT (FilePath != NULL) is enough when
> EfiBootManagerInitializeLoadOption().
Maybe it is not enough. The ASSERT only work in DEBUG build. Did I take a mistake?
Thanks,
Zhichao
>
>
>
> >
> > DEBUG ((EFI_D_ERROR, "[Bds] Unable to boot!\n"));
> > PlatformBootManagerUnableToBoot ();
> > --
> > 2.21.0.windows.1
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] MdeModulePkg/BdsDxe: Use a pcd to control PlatformRecovery
2019-06-03 7:13 ` Gao, Zhichao
@ 2019-06-03 7:26 ` Ni, Ray
2019-06-03 7:42 ` Gao, Zhichao
0 siblings, 1 reply; 8+ messages in thread
From: Ni, Ray @ 2019-06-03 7:26 UTC (permalink / raw)
To: Gao, Zhichao, devel@edk2.groups.io
Cc: Wang, Jian J, Wu, Hao A, Zeng, Star, Gao, Liming, Sean Brogan,
Michael Turner, Bret Barkelew
> -----Original Message-----
> From: Gao, Zhichao <zhichao.gao@intel.com>
> Sent: Monday, June 3, 2019 3:13 PM
> To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>;
> Zeng, Star <star.zeng@intel.com>; Gao, Liming <liming.gao@intel.com>;
> Sean Brogan <sean.brogan@microsoft.com>; Michael Turner
> <Michael.Turner@microsoft.com>; Bret Barkelew
> <Bret.Barkelew@microsoft.com>
> Subject: RE: [PATCH 2/2] MdeModulePkg/BdsDxe: Use a pcd to control
> PlatformRecovery
>
>
>
> > -----Original Message-----
> > From: Ni, Ray
> > Sent: Monday, June 3, 2019 2:59 PM
> > To: Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io
> > Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> > <hao.a.wu@intel.com>; Zeng, Star <star.zeng@intel.com>; Gao, Liming
> > <liming.gao@intel.com>; Sean Brogan <sean.brogan@microsoft.com>;
> > Michael Turner <Michael.Turner@microsoft.com>; Bret Barkelew
> > <Bret.Barkelew@microsoft.com>
> > Subject: RE: [PATCH 2/2] MdeModulePkg/BdsDxe: Use a pcd to control
> > PlatformRecovery
> >
> > > + //
> > > + // When platform recovery is not enabled, still boot to
> > > + platform default
> > > file path.
> > > + //
> > > + EfiBootManagerProcessLoadOption (&PlatformDefaultBootOption);
> > > + }
> > > }
> > > + EfiBootManagerFreeLoadOption (&PlatformDefaultBootOption);
> >
> > PlatformDefaultBootOption might be uninitialized if FilePath is NULL.
>
> EfiBootManagerFreeLoadOption would check the pointer before free. So it is
> safe regardless of the PlatformDefaultBootOption is initialized or not.
>
> >
> > FilePath is NULL when memory allocation fails.
> > So I am thinking maybe ASSERT (FilePath != NULL) is enough when
> > EfiBootManagerInitializeLoadOption().
>
> Maybe it is not enough. The ASSERT only work in DEBUG build. Did I take a
> mistake?
You are right.
How about do a DEBUG print followed by a CpuDeadLoop() when FilePath is NULL?
Then all code below that point doesn't need to take care this error any more.
>
> Thanks,
> Zhichao
>
> >
> >
> >
> > >
> > > DEBUG ((EFI_D_ERROR, "[Bds] Unable to boot!\n"));
> > > PlatformBootManagerUnableToBoot ();
> > > --
> > > 2.21.0.windows.1
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] MdeModulePkg/BdsDxe: Use a pcd to control PlatformRecovery
2019-06-03 7:26 ` Ni, Ray
@ 2019-06-03 7:42 ` Gao, Zhichao
0 siblings, 0 replies; 8+ messages in thread
From: Gao, Zhichao @ 2019-06-03 7:42 UTC (permalink / raw)
To: Ni, Ray, devel@edk2.groups.io
Cc: Wang, Jian J, Wu, Hao A, Zeng, Star, Gao, Liming, Sean Brogan,
Michael Turner, Bret Barkelew
> -----Original Message-----
> From: Ni, Ray
> Sent: Monday, June 3, 2019 3:27 PM
> To: Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>;
> Zeng, Star <star.zeng@intel.com>; Gao, Liming <liming.gao@intel.com>;
> Sean Brogan <sean.brogan@microsoft.com>; Michael Turner
> <Michael.Turner@microsoft.com>; Bret Barkelew
> <Bret.Barkelew@microsoft.com>
> Subject: RE: [PATCH 2/2] MdeModulePkg/BdsDxe: Use a pcd to control
> PlatformRecovery
>
>
>
> > -----Original Message-----
> > From: Gao, Zhichao <zhichao.gao@intel.com>
> > Sent: Monday, June 3, 2019 3:13 PM
> > To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io
> > Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> > <hao.a.wu@intel.com>; Zeng, Star <star.zeng@intel.com>; Gao, Liming
> > <liming.gao@intel.com>; Sean Brogan <sean.brogan@microsoft.com>;
> > Michael Turner <Michael.Turner@microsoft.com>; Bret Barkelew
> > <Bret.Barkelew@microsoft.com>
> > Subject: RE: [PATCH 2/2] MdeModulePkg/BdsDxe: Use a pcd to control
> > PlatformRecovery
> >
> >
> >
> > > -----Original Message-----
> > > From: Ni, Ray
> > > Sent: Monday, June 3, 2019 2:59 PM
> > > To: Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io
> > > Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> > > <hao.a.wu@intel.com>; Zeng, Star <star.zeng@intel.com>; Gao, Liming
> > > <liming.gao@intel.com>; Sean Brogan <sean.brogan@microsoft.com>;
> > > Michael Turner <Michael.Turner@microsoft.com>; Bret Barkelew
> > > <Bret.Barkelew@microsoft.com>
> > > Subject: RE: [PATCH 2/2] MdeModulePkg/BdsDxe: Use a pcd to control
> > > PlatformRecovery
> > >
> > > > + //
> > > > + // When platform recovery is not enabled, still boot to
> > > > + platform default
> > > > file path.
> > > > + //
> > > > + EfiBootManagerProcessLoadOption (&PlatformDefaultBootOption);
> > > > + }
> > > > }
> > > > + EfiBootManagerFreeLoadOption (&PlatformDefaultBootOption);
> > >
> > > PlatformDefaultBootOption might be uninitialized if FilePath is NULL.
> >
> > EfiBootManagerFreeLoadOption would check the pointer before free. So
> > it is safe regardless of the PlatformDefaultBootOption is initialized or not.
> >
> > >
> > > FilePath is NULL when memory allocation fails.
> > > So I am thinking maybe ASSERT (FilePath != NULL) is enough when
> > > EfiBootManagerInitializeLoadOption().
> >
> > Maybe it is not enough. The ASSERT only work in DEBUG build. Did I
> > take a mistake?
>
> You are right.
> How about do a DEBUG print followed by a CpuDeadLoop() when FilePath is
> NULL?
>
> Then all code below that point doesn't need to take care this error any more.
I am OK with that because fail to allocate memory should be a serious error that would make the boot flow work incorrect. And putting the system into a dead loop to indicate the failure of boot is fine.
>
> >
> > Thanks,
> > Zhichao
> >
> > >
> > >
> > >
> > > >
> > > > DEBUG ((EFI_D_ERROR, "[Bds] Unable to boot!\n"));
> > > > PlatformBootManagerUnableToBoot ();
> > > > --
> > > > 2.21.0.windows.1
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-06-03 7:42 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-06-03 6:43 [PATCH 0/2] Use a pcd to control Platform Recovery behavior Gao, Zhichao
2019-06-03 6:43 ` [PATCH 1/2] MdeModulePkg: Add a pcd to set the OS indications bit Gao, Zhichao
2019-06-03 6:59 ` Ni, Ray
2019-06-03 6:43 ` [PATCH 2/2] MdeModulePkg/BdsDxe: Use a pcd to control PlatformRecovery Gao, Zhichao
2019-06-03 6:59 ` Ni, Ray
2019-06-03 7:13 ` Gao, Zhichao
2019-06-03 7:26 ` Ni, Ray
2019-06-03 7:42 ` Gao, Zhichao
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox