[PATCH 0/2] Use a pcd to control Platform Recovery behavior

[PATCH 0/2] Use a pcd to control Platform Recovery behavior

[Zhichao Gao]

Add a pcd PcdPlatformRecoverySupport to control the variable
PlatformRecovery#### and the EFI_OS_INDICATIONS_START_PLATFORM_RECOVERY
bit of variable "OsIndicationsSupported".

Bret Barkelew (2):
  MdeModulePkg: Add a pcd to set the OS indications bit
  MdeModulePkg/BdsDxe: Use a pcd to set OS indications bit

 MdeModulePkg/MdeModulePkg.dec            |  6 ++
 MdeModulePkg/Universal/BdsDxe/BdsDxe.inf |  1 +
 MdeModulePkg/Universal/BdsDxe/BdsEntry.c | 74 +++++++++++++-----------
 3 files changed, 47 insertions(+), 34 deletions(-)

-- 
2.21.0.windows.1

[PATCH 1/2] MdeModulePkg: Add a pcd to set the OS indications bit

[Zhichao Gao]

From: Bret Barkelew <Bret.Barkelew@microsoft.com>

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1678

Add a pcd PcdPlatformRecoverySupport to conditionally
set an OS indications bit.

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/MdeModulePkg.dec | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
index 43992b7f65..870f49749a 100644
--- a/MdeModulePkg/MdeModulePkg.dec
+++ b/MdeModulePkg/MdeModulePkg.dec
@@ -1669,6 +1669,12 @@
   # @Prompt Reset on memory type information change.
   gEfiMdeModulePkgTokenSpaceGuid.PcdResetOnMemoryTypeInformationChange|TRUE|BOOLEAN|0x00010056
 
+  ## Indicates if the BDS supports Platform Recovery.<BR><BR>
+  #   TRUE  - Platform supports Platform Recovery.<BR>
+  #   FALSE - Does not add Boot Manager to boot order.<BR>
+  # @Prompt Support Platform Recovery.
+  gEfiMdeModulePkgTokenSpaceGuid.PcdPlatformRecoverySupport|TRUE|BOOLEAN|0x00010078
+
   ## Specify the foreground color for Subtile text in HII Form Browser. The default value is EFI_BLUE.
   #  Only following values defined in UEFI specification are valid:<BR><BR>
   #  0x00 (EFI_BLACK)<BR>
-- 
2.21.0.windows.1

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

[Zhichao Gao]

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

Re: [edk2-devel] [PATCH 1/2] MdeModulePkg: Add a pcd to set the OS indications bit

[Ni, Ray]

[-- Attachment #1: Type: text/plain, Size: 40 bytes --]

Reviewed-by: Ray Ni <ray.ni@intel.com>

[-- Attachment #2: Type: text/html, Size: 99 bytes --]

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

[Ni, Ray]

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

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

[Gao, Zhichao]

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

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

[Ni, Ray]

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

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

[Ni, Ray]

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

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

[Gao, Zhichao]

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

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

[Ni, Ray]

> -----Original Message-----
> From: Gao, Zhichao
> Sent: Thursday, April 25, 2019 8:13 PM
> 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
> 
> 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.
Can you explain a bit more? What's the meaning of "let platform control 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