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

["Ni, Ray" <ray.ni@intel.com>]

Reviewed-by: Ray Ni <ray.ni@intel.com>

> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Gao, Zhichao
> Sent: Tuesday, June 4, 2019 9:05 AM
> To: devel@edk2.groups.io
> Cc: 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>; Bret Barkelew
> <Bret.Barkelew@microsoft.com>
> 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) {
> +      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
> 
> 
>