Re: [edk2-devel] [PATCH V3 2/2] MdeModulePkg/BdsDxe: Use a pcd to control PlatformRecovery

["Gao, Zhichao" <zhichao.gao@intel.com>]

> -----Original Message-----
> From: Wu, Hao A
> Sent: Thursday, June 13, 2019 3:39 PM
> To: devel@edk2.groups.io; Gao, Zhichao <zhichao.gao@intel.com>
> Cc: Wang, Jian J <jian.j.wang@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>; Bret Barkelew
> <Bret.Barkelew@microsoft.com>
> Subject: RE: [edk2-devel] [PATCH V3 2/2] MdeModulePkg/BdsDxe: Use a pcd
> to control PlatformRecovery
> 
> One comment below:
> 
> > -----Original Message-----
> > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> > Gao, Zhichao
> > Sent: Tuesday, June 04, 2019 9:05 AM
> > To: devel@edk2.groups.io
> > Cc: Wang, Jian J; Wu, Hao A; Ni, Ray; Zeng, Star; Gao, Liming; Sean
> > Brogan; Michael Turner; Bret Barkelew
> > Subject: [edk2-devel] [PATCH V3 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@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 | 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..8c8a0b5236 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 (&LoadOption, LoadOptions,
> > LoadOptionCount) == -1) {
> 
> 
> In the above 'if' statement, 'LoadOption' is not being initialized.
> Is this a typo here?

Yes. You are right. 'LoadOption' should change to 'PlatformDefaultBootOption'.
I would update this patch set with your comments. And ignore my push request before.

Thanks,
Zhichao

> 
> Best Regards,
> Hao Wu
> 
> 
> > +      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
> >
> >
> >