Re: [PATCH 2/2] MdeModulePkg/BdsDxe: Use a pcd to set OS indications bit

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

The PlatformRecovery#### can be set by PlatformBootManagerLib.
We cannot execute these if the feature is not supported.

Wait a sec, can I get clarification on the PCD's meaning a bit more:
Why do we need such a configuration flag?
Turning it off will break the functionality to boot default boot option like
EFI\BOOT\BOOTIA32.EFI
EFI\BOOT\BOOTx64.EFI
...

Is that expected?
If it's not, we still need to support booting from these boot options even
the PCD is OFF.

Thanks,
Ray

> -----Original Message-----
> From: Gao, Zhichao
> Sent: Wednesday, April 10, 2019 8:45 AM
> To: Ni, Ray <ray.ni@intel.com>; 'devel@edk2.groups.io' <devel@edk2.groups.io>
> Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>; Wang, Jian J
> <jian.j.wang@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: RE: [PATCH 2/2] MdeModulePkg/BdsDxe: Use a pcd to set OS
> indications bit
> 
> This section can work fine even through the L"PlatformRecovery####" is not set.
> But if we do not support it, it should be better not to run it.
> Thanks for your comments, I will update it later.
> 
> Thanks,
> Zhichao
> 
> > -----Original Message-----
> > From: Ni, Ray
> > Sent: Tuesday, April 9, 2019 5:32 PM
> > To: Gao, Zhichao <zhichao.gao@intel.com>; 'devel@edk2.groups.io'
> > <devel@edk2.groups.io>
> > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>; Wang, Jian J
> > <jian.j.wang@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: RE: [PATCH 2/2] MdeModulePkg/BdsDxe: Use a pcd to set OS
> > indications bit
> >
> > Resend to groups.io.
> >
> > > -----Original Message-----
> > > From: Ni, Ray
> > > Sent: Tuesday, April 9, 2019 5:31 PM
> > > To: Gao, Zhichao <zhichao.gao@intel.com>; edk2-devel@lists.01.org
> > > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>; Wang, Jian J
> > > <jian.j.wang@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: RE: [PATCH 2/2] MdeModulePkg/BdsDxe: Use a pcd to set OS
> > > indications bit
> > >
> > > Zhichao,
> > > In the very bottom of BdsEntry(), below code is always executed no
> > > matter the PCD is true or false.
> > > I don't think that's the right behavior.
> > >
> > >   if (!BootSuccess) {
> > >     LoadOptions = EfiBootManagerGetLoadOptions (&LoadOptionCount,
> > > LoadOptionTypePlatformRecovery);
> > >     ProcessLoadOptions (LoadOptions, LoadOptionCount);
> > >     EfiBootManagerFreeLoadOptions (LoadOptions, LoadOptionCount);
> > >   }
> > >
> > > > -----Original Message-----
> > > > From: Gao, Zhichao <zhichao.gao@intel.com>
> > > > Sent: Tuesday, April 2, 2019 1:50 PM
> > > > To: edk2-devel@lists.01.org
> > > > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>; 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>
> > > > Subject: [PATCH 2/2] MdeModulePkg/BdsDxe: Use a pcd to set OS
> > > > indications bit
> > > >
> > > > From: Bret Barkelew <Bret.Barkelew@microsoft.com>
> > > >
> > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1678
> > > >
> > > > Use the pcd PcdPlatformRecoverySupport to control whether to set
> > > > the PlatformRecovery#### variable and whether to set the
> > > > EFI_OS_INDICATIONS_START_PLATFORM_RECOVERY bit of variable
> > > > "OsIndicationsSupported".
> > > >
> > > > 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/Universal/BdsDxe/BdsDxe.inf |  1 +
> > > > MdeModulePkg/Universal/BdsDxe/BdsEntry.c | 74 +++++++++++++------
> > --
> > > --
> > > > -
> > > >  2 files changed, 41 insertions(+), 34 deletions(-)
> > > >
> > > > diff --git a/MdeModulePkg/Universal/BdsDxe/BdsDxe.inf
> > > > b/MdeModulePkg/Universal/BdsDxe/BdsDxe.inf
> > > > index 82eb8aafc6..9caabbce7f 100644
> > > > --- a/MdeModulePkg/Universal/BdsDxe/BdsDxe.inf
> > > > +++ b/MdeModulePkg/Universal/BdsDxe/BdsDxe.inf
> > > > @@ -101,6 +101,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 8946d79ab2..ade77adb7d 100644
> > > > --- a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
> > > > +++ b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
> > > > @@ -552,10 +552,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 (
> > > > @@ -769,41 +773,43 @@ 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 (PcdGetBool (PcdPlatformRecoverySupport)) {
> > > > +    //
> > > > +    // 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;
> > > > +        }
> > > >        }
> > > > +      LoadOption.OptionNumber = Index;
> > > > +      Status = EfiBootManagerLoadOptionToVariable (&LoadOption);
> > > > +      ASSERT_EFI_ERROR (Status);
> > > >      }
> > > > -    LoadOption.OptionNumber = Index;
> > > > -    Status = EfiBootManagerLoadOptionToVariable (&LoadOption);
> > > > -    ASSERT_EFI_ERROR (Status);
> > > > +    EfiBootManagerFreeLoadOption (&LoadOption);
> > > > +    FreePool (FilePath);
> > > > +    EfiBootManagerFreeLoadOptions (LoadOptions, LoadOptionCount);
> > > >    }
> > > > -  EfiBootManagerFreeLoadOption (&LoadOption);
> > > > -  FreePool (FilePath);
> > > > -  EfiBootManagerFreeLoadOptions (LoadOptions, LoadOptionCount);
> > > >
> > > >    //
> > > >    // Report Status Code to indicate connecting drivers will
> > > > happen
> > > > --
> > > > 2.21.0.windows.1