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

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

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