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

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

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