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

2019-06-13 Thread Gao, Zhichao



> -Original Message-
> From: Wu, Hao A
> Sent: Thursday, June 13, 2019 3:39 PM
> To: devel@edk2.groups.io; Gao, Zhichao 
> Cc: Wang, Jian J ; Ni, Ray ; Zeng,
> Star ; Gao, Liming ; Sean
> Brogan ; Michael Turner
> ; Bret Barkelew
> 
> 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 
> > Cc: Hao Wu 
> > Cc: Ray Ni 
> > Cc: Star Zeng 
> > Cc: Liming Gao 
> > Cc: Sean Brogan 
> > Cc: Michael Turner 
> > Cc: Bret Barkelew 
> > Signed-off-by: Zhichao Gao 
> > ---
> >  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.
> > +#  Copyright (c) 2008 - 2019, Intel Corporation. All rights
> > +reserved.
> >  #  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.
> > +Copyright (c) 2004 - 2019, Intel Corporation. All rights
> > +reserved.
> >  (C) Copyright 2016 Hewlett Packard Enterprise Development LP
> >  (C) Copyright 2015 Hewlett-Packard Development Company, L.P.
> >  SPDX-License-Identifier: BSD-2-Clause-Patent @@ -546,10 +546,14 @@
> > BdsFormalizeOSIndicationVariable (
> >//
> >Status = EfiBootManagerGetBootManagerMenu ();
> >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 ();
> >} else {
> > -OsIndicationSupport =
> > EFI_OS_INDICATIONS_START_PLATFORM_RECO

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

2019-06-04 Thread Ni, Ray
Reviewed-by: Ray Ni 

> -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 ; 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 
> Cc: Hao Wu 
> Cc: Ray Ni 
> Cc: Star Zeng 
> Cc: Liming Gao 
> Cc: Sean Brogan 
> Cc: Michael Turner 
> Cc: Bret Barkelew 
> Signed-off-by: Zhichao Gao 
> ---
>  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.
> +#  Copyright (c) 2008 - 2019, Intel Corporation. All rights
> +reserved.
>  #  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.
> +Copyright (c) 2004 - 2019, Intel Corporation. All rights reserved.
>  (C) Copyright 2016 Hewlett Packard Enterprise Development LP
>  (C) Copyright 2015 Hewlett-Packard Development Company, L.P.
>  SPDX-License-Identifier: BSD-2-Clause-Patent @@ -546,10 +546,14 @@
> BdsFormalizeOSIndicationVariable (
>//
>Status = EfiBootManagerGetBootManagerMenu ();
>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 ();
>} 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_OPTIONPlatformDefaultBootOption;
> 
>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 (
> - ,
> + ,
>   LoadOptionNumberUnassigned,
>