[PATCH 1/2] MdeModulePkg: Add a pcd to set the OS indications bit

[PATCH 1/2] MdeModulePkg: Add a pcd to set the OS indications bit

[Zhichao Gao]

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.

Cc: Jian J Wang <jian.j.wang@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>
Contributed-under: TianoCore Contribution Agreement 1.1
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 43992b7f65..870f49749a 100644
--- a/MdeModulePkg/MdeModulePkg.dec
+++ b/MdeModulePkg/MdeModulePkg.dec
@@ -1669,6 +1669,12 @@
   # @Prompt Reset on memory type information change.
   gEfiMdeModulePkgTokenSpaceGuid.PcdResetOnMemoryTypeInformationChange|TRUE|BOOLEAN|0x00010056
 
+  ## Indicates if the BDS supports Platform Recovery.<BR><BR>
+  #   TRUE  - Platform supports Platform Recovery.<BR>
+  #   FALSE - Does not add Boot Manager to boot order.<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

[PATCH 0/2] Use a pcd to control Platform Recovery behavior

[Gao, Zhichao]

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

[PATCH 1/2] MdeModulePkg: Add a pcd to set the OS indications bit

[Gao, Zhichao]

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

[PATCH 2/2] MdeModulePkg/BdsDxe: Use a pcd to control PlatformRecovery

[Gao, Zhichao]

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

Re: [PATCH 2/2] MdeModulePkg/BdsDxe: Use a pcd to control PlatformRecovery

[Ni, Ray]

> +      //
> +      // 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

Re: [PATCH 1/2] MdeModulePkg: Add a pcd to set the OS indications bit

[Ni, Ray]

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

Re: [PATCH 2/2] MdeModulePkg/BdsDxe: Use a pcd to control PlatformRecovery

[Gao, Zhichao]

> -----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

Re: [PATCH 2/2] MdeModulePkg/BdsDxe: Use a pcd to control PlatformRecovery

[Ni, Ray]

> -----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

Re: [PATCH 2/2] MdeModulePkg/BdsDxe: Use a pcd to control PlatformRecovery

[Gao, Zhichao]

> -----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