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

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

Hi Ray,
 
Refer to the spec 2.8 section 3.4.3:
If system firmware supports boot option recovery as described in Section 3.4, system firmware must include a PlatformRecovery#### variable specifying a short-form File Path Media Device Path (see Section 3.1.2) containing the platform default file path for removable media (see Table 15).

Can we regard the "PlatformRecovery####" variable as optional? If so, I think it is fine to let the platform to control its behavior.

Thanks,
Zhichao

> -----Original Message-----
> From: Ni, Ray
> Sent: Wednesday, April 10, 2019 10:00 AM
> 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
> 
> Sorry I didn't realize the impact in the beginning.
> 
> > -----Original Message-----
> > From: Ni, Ray
> > Sent: Wednesday, April 10, 2019 10:00 AM
> > 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
> >
> > 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