Use a pcd to control PlatformRecovery

Use a pcd to control PlatformRecovery

[Wang, Sunny (HPS SW)]

Hi Zhichao and Ray, 

I have some questions about this code change. Sorry for being late to bring my questions here.

For now, the code block for iterating the PlatformRecovery#### variables is controlled by OsIndications variable. However, it looks to me like that the PlatformRecovery#### should still be attempted for the case of that processing of BootOrder does NOT result in success (according to section 3.4 in UEFI 2.8). In other words, I think we should check PCD "PcdPlatformRecoverySupport" instead of Local variable "PlatformRecovery" (from OsIndications variable) like the code below. What do you guys think? If you need a meeting or short talk to discuss this, feel free to let me know. 

  if (!BootSuccess) {
-   if (PlatformRecovery) {
+  if (PcdGetBool (PcdPlatformRecoverySupport)) {
      LoadOptions = EfiBootManagerGetLoadOptions (&LoadOptionCount, LoadOptionTypePlatformRecovery);
      ProcessLoadOptions (LoadOptions, LoadOptionCount);
      EfiBootManagerFreeLoadOptions (LoadOptions, LoadOptionCount);
    } else {
      //
      // When platform recovery is not enabled, still boot to platform default file path.
      //
      EfiBootManagerProcessLoadOption (&PlatformDefaultBootOption);
    }
 

In addition, it looks like EDK2 don't have code to process OsRecovery#### variables. Do we need to create a Bug on TianoCore Bugzilla system? 

Moreover, I saw that both of you had a discussion about "Default PlatformRecovery", but I can't figure out the connection between the discussion and the final code change. Isn't the "Default PlatformRecovery" part of the Platform Recovery feature? At this moment, we don't have OS recovery support, so I think that NO platform recovery support can be identified as NO boot option recovery support. For this case, shouldn't we use PCD "PcdPlatformRecoverySupport" to control "Default PlatformRecovery" as well? For the next step, I think we need to get further clarification from USWG to either not tie "Default Boot Behavior" with PlatformRecovery or update the description to the following: 
    - If system firmware supports Platform recovery as described in Section 3.4.2, system firmware must include a PlatformRecovery#### variable specifying a short-form File Path Media Device Path....

Regards,
Sunny Wang

-----Original Message-----
From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Gao, Zhichao
Sent: Thursday, June 20, 2019 11:44 AM
To: devel@edk2.groups.io
Cc: Jian J Wang <jian.j.wang@intel.com>; Hao Wu <hao.a.wu@intel.com>; Ray Ni <ray.ni@intel.com>; Star Zeng <star.zeng@intel.com>; Liming Gao <liming.gao@intel.com>; Sean Brogan <sean.brogan@microsoft.com>; Michael Turner <Michael.Turner@microsoft.com>; Bret Barkelew <Bret.Barkelew@microsoft.com>
Subject: [edk2-devel] [PATCH v5 2/2] MdeModulePkg/BdsDxe: Use a pcd to control PlatformRecovery

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

Use the PcdPlatformRecoverySupport to control the function of platform recovery in BDS.
First, set the variable's ("OsIndicationsSupported") EFI_OS_INDICATIONS_START_PLATFORM_RECOVERY bit base on the pcd.
It would affect the variable "OsIndications".
While the platform does not support the platform recovery, it is inappropriate to set a PlatformRecovery#### variable. So skip setting the variable. But it should remain the behavior of booting from a default file path (such as \EFI\BOOT\BOOTX64.EFI) to be compatible with the previous version UEFI spec.

Add memory check before build platform default boot option. If fail to allocate memory for the defualt boot file path, put the system into dead loop to indicate it is unable to boot.

Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao Wu <hao.a.wu@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>
Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
---
 MdeModulePkg/Universal/BdsDxe/BdsDxe.inf |  3 +-  MdeModulePkg/Universal/BdsDxe/BdsEntry.c | 71 +++++++++++++++---------
 2 files changed, 47 insertions(+), 27 deletions(-)

diff --git a/MdeModulePkg/Universal/BdsDxe/BdsDxe.inf b/MdeModulePkg/Universal/BdsDxe/BdsDxe.inf
index 6913389d34..7f94ca17df 100644
--- a/MdeModulePkg/Universal/BdsDxe/BdsDxe.inf
+++ b/MdeModulePkg/Universal/BdsDxe/BdsDxe.inf
@@ -5,7 +5,7 @@
 #  gEfiBdsArchProtocolGuid. After DxeCore finish dispatching, DxeCore will invoke Entry  #  interface of protocol gEfiBdsArchProtocolGuid, then BDS phase is entered.
 #
-#  Copyright (c) 2008 - 2018, Intel Corporation. All rights reserved.<BR>
+#  Copyright (c) 2008 - 2019, Intel Corporation. All rights 
+reserved.<BR>
 #  SPDX-License-Identifier: BSD-2-Clause-Patent  #  ## @@ -95,6 +95,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 9d312bd982..4f3168b62a 100644
--- a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
+++ b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
@@ -5,7 +5,7 @@
   After DxeCore finish DXE phase, gEfiBdsArchProtocolGuid->BdsEntry will be invoked
   to enter BDS phase.
 
-Copyright (c) 2004 - 2018, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2004 - 2019, Intel Corporation. All rights reserved.<BR>
 (C) Copyright 2016 Hewlett Packard Enterprise Development LP<BR>
 (C) Copyright 2015 Hewlett-Packard Development Company, L.P.<BR>
 SPDX-License-Identifier: BSD-2-Clause-Patent @@ -546,10 +546,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 (
@@ -662,6 +666,7 @@ BdsEntry (
   BOOLEAN                         BootSuccess;
   EFI_DEVICE_PATH_PROTOCOL        *FilePath;
   EFI_STATUS                      BootManagerMenuStatus;
+  EFI_BOOT_MANAGER_LOAD_OPTION    PlatformDefaultBootOption;
 
   HotkeyTriggered = NULL;
   Status          = EFI_SUCCESS;
@@ -763,14 +768,13 @@ 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);
+  if (FilePath == NULL) {
+    DEBUG ((DEBUG_ERROR, "Fail to allocate memory for defualt boot file path. Unable to boot.\n"));
+    CpuDeadLoop ();
+  }
   Status = EfiBootManagerInitializeLoadOption (
-             &LoadOption,
+             &PlatformDefaultBootOption,
              LoadOptionNumberUnassigned,
              LoadOptionTypePlatformRecovery,
              LOAD_OPTION_ACTIVE,
@@ -780,24 +784,31 @@ BdsEntry (
              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;
+
+  //
+  // 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 if the platform supports Platform Recovery.
+  //
+  if (PcdGetBool (PcdPlatformRecoverySupport)) {
+    LoadOptions = EfiBootManagerGetLoadOptions (&LoadOptionCount, LoadOptionTypePlatformRecovery);
+    if (EfiBootManagerFindLoadOption (&PlatformDefaultBootOption, 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;
+        }
       }
+      PlatformDefaultBootOption.OptionNumber = Index;
+      Status = EfiBootManagerLoadOptionToVariable (&PlatformDefaultBootOption);
+      ASSERT_EFI_ERROR (Status);
     }
-    LoadOption.OptionNumber = Index;
-    Status = EfiBootManagerLoadOptionToVariable (&LoadOption);
-    ASSERT_EFI_ERROR (Status);
+    EfiBootManagerFreeLoadOptions (LoadOptions, LoadOptionCount);
   }
-  EfiBootManagerFreeLoadOption (&LoadOption);
   FreePool (FilePath);
-  EfiBootManagerFreeLoadOptions (LoadOptions, LoadOptionCount);
 
   //
   // Report Status Code to indicate connecting drivers will happen @@ -1043,10 +1054,18 @@ BdsEntry (
   }
 
   if (!BootSuccess) {
-    LoadOptions = EfiBootManagerGetLoadOptions (&LoadOptionCount, LoadOptionTypePlatformRecovery);
-    ProcessLoadOptions (LoadOptions, LoadOptionCount);
-    EfiBootManagerFreeLoadOptions (LoadOptions, LoadOptionCount);
+    if (PlatformRecovery) {
+      LoadOptions = EfiBootManagerGetLoadOptions (&LoadOptionCount, LoadOptionTypePlatformRecovery);
+      ProcessLoadOptions (LoadOptions, LoadOptionCount);
+      EfiBootManagerFreeLoadOptions (LoadOptions, LoadOptionCount);
+    } else {
+      //
+      // When platform recovery is not enabled, still boot to platform default file path.
+      //
+      EfiBootManagerProcessLoadOption (&PlatformDefaultBootOption);
+    }
   }
+  EfiBootManagerFreeLoadOption (&PlatformDefaultBootOption);
 
   DEBUG ((EFI_D_ERROR, "[Bds] Unable to boot!\n"));
   PlatformBootManagerUnableToBoot ();
--
2.21.0.windows.1

Re: Use a pcd to control PlatformRecovery

[Gao, Zhichao]

First of all, the patch didn't aim to change the other part of the boot flow except PlatformRecovery.

Local variable PlatformRecovery is controlled by OsIndications variable. When the EFI_OS_INDICATIONS_START_PLATFORM_RECOVERY is set, the firmware should try to platform specific recovery. But that doesn't mean the platform must support the specific recovery. i.e. local PlatformRecovery is controlling the boot flow and the Pcd just indicated whether the platform support recovery or not.
So, on my opinion, I don't agree with your change.

Default Platform Recovery refer to the short file path to boot the OS. If the firmware supports platform recovery, then *short file path* option would be one part of the PlatformRecovery#### in case there are no other boot options. If the firmware doesn't support platform recovery, we still need this default boot thru a short file path and it should not depend on the PlatformRecovery#### variable for the compatibility thinking.

Thanks,
Zhichao

> -----Original Message-----
> From: Wang, Sunny (HPS SW) [mailto:sunnywang@hpe.com]
> Sent: Tuesday, October 15, 2019 4:53 PM
> To: devel@edk2.groups.io; Gao, Zhichao <zhichao.gao@intel.com>; Ni, Ray
> <ray.ni@intel.com>
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@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>; Bret Barkelew
> <Bret.Barkelew@microsoft.com>; Li, Walon <walon.li@hpe.com>; Wei, Kent
> (HPS SW) <kent.wei@hpe.com>; Wang, Sunny (HPS SW)
> <sunnywang@hpe.com>
> Subject: Use a pcd to control PlatformRecovery
> 
> Hi Zhichao and Ray,
> 
> I have some questions about this code change. Sorry for being late to bring
> my questions here.
> 
> For now, the code block for iterating the PlatformRecovery#### variables is
> controlled by OsIndications variable. However, it looks to me like that the
> PlatformRecovery#### should still be attempted for the case of that
> processing of BootOrder does NOT result in success (according to section 3.4
> in UEFI 2.8). In other words, I think we should check PCD
> "PcdPlatformRecoverySupport" instead of Local variable "PlatformRecovery"
> (from OsIndications variable) like the code below. What do you guys think? If
> you need a meeting or short talk to discuss this, feel free to let me know.
> 
>   if (!BootSuccess) {
> -   if (PlatformRecovery) {
> +  if (PcdGetBool (PcdPlatformRecoverySupport)) {
>       LoadOptions = EfiBootManagerGetLoadOptions (&LoadOptionCount,
> LoadOptionTypePlatformRecovery);
>       ProcessLoadOptions (LoadOptions, LoadOptionCount);
>       EfiBootManagerFreeLoadOptions (LoadOptions, LoadOptionCount);
>     } else {
>       //
>       // When platform recovery is not enabled, still boot to platform default
> file path.
>       //
>       EfiBootManagerProcessLoadOption (&PlatformDefaultBootOption);
>     }
> 
> 
> In addition, it looks like EDK2 don't have code to process OsRecovery####
> variables. Do we need to create a Bug on TianoCore Bugzilla system?
> 
> Moreover, I saw that both of you had a discussion about "Default
> PlatformRecovery", but I can't figure out the connection between the
> discussion and the final code change. Isn't the "Default PlatformRecovery"
> part of the Platform Recovery feature? At this moment, we don't have OS
> recovery support, so I think that NO platform recovery support can be
> identified as NO boot option recovery support. For this case, shouldn't we
> use PCD "PcdPlatformRecoverySupport" to control "Default
> PlatformRecovery" as well? For the next step, I think we need to get further
> clarification from USWG to either not tie "Default Boot Behavior" with
> PlatformRecovery or update the description to the following:
>     - If system firmware supports Platform recovery as described in Section
> 3.4.2, system firmware must include a PlatformRecovery#### variable
> specifying a short-form File Path Media Device Path....
> 
> Regards,
> Sunny Wang
> 

Re: Use a pcd to control PlatformRecovery

[Gao, Zhichao]

> -----Original Message-----
> From: Gao, Zhichao
> Sent: Wednesday, October 16, 2019 10:09 AM
> To: 'Wang, Sunny (HPS SW)' <sunnywang@hpe.com>; devel@edk2.groups.io;
> Ni, Ray <ray.ni@intel.com>
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@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>; Bret Barkelew
> <Bret.Barkelew@microsoft.com>; Li, Walon <walon.li@hpe.com>; Wei, Kent
> (HPS SW) <kent.wei@hpe.com>
> Subject: RE: Use a pcd to control PlatformRecovery
> 
> First of all, the patch didn't aim to change the other part of the boot flow
> except PlatformRecovery.
> 
> Local variable PlatformRecovery is controlled by OsIndications variable. When
> the EFI_OS_INDICATIONS_START_PLATFORM_RECOVERY is set, the
> firmware should try to platform specific recovery. But that doesn't mean the
> platform must support the specific recovery. i.e. local PlatformRecovery is
> controlling the boot flow and the Pcd just indicated whether the platform
> support recovery or not.
> So, on my opinion, I don't agree with your change.

After discuss with Sunny and Ray, and refer to the spec section 3.4.1 and 3.4.2, the OsRecovery and PlatformRecovery should always be operated regardless of the value of OsIndication variable if fail to boot the BootOrder. I am wrong. We should change to use the PcdPlatformRecoverySupport to control the PlatformRecovery. Please help to send a patch to fix it. Thanks a lot.

> 
> Default Platform Recovery refer to the short file path to boot the OS. If the
> firmware supports platform recovery, then *short file path* option would be
> one part of the PlatformRecovery#### in case there are no other boot
> options. If the firmware doesn't support platform recovery, we still need this
> default boot thru a short file path and it should not depend on the
> PlatformRecovery#### variable for the compatibility thinking.
> 
> Thanks,
> Zhichao
> 
> > -----Original Message-----
> > From: Wang, Sunny (HPS SW) [mailto:sunnywang@hpe.com]
> > Sent: Tuesday, October 15, 2019 4:53 PM
> > To: devel@edk2.groups.io; Gao, Zhichao <zhichao.gao@intel.com>; Ni,
> > Ray <ray.ni@intel.com>
> > Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> > <hao.a.wu@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>; Bret Barkelew
> > <Bret.Barkelew@microsoft.com>; Li, Walon <walon.li@hpe.com>; Wei,
> Kent
> > (HPS SW) <kent.wei@hpe.com>; Wang, Sunny (HPS SW)
> <sunnywang@hpe.com>
> > Subject: Use a pcd to control PlatformRecovery
> >
> > Hi Zhichao and Ray,
> >
> > I have some questions about this code change. Sorry for being late to
> > bring my questions here.
> >
> > For now, the code block for iterating the PlatformRecovery####
> > variables is controlled by OsIndications variable. However, it looks
> > to me like that the PlatformRecovery#### should still be attempted for
> > the case of that processing of BootOrder does NOT result in success
> > (according to section 3.4 in UEFI 2.8). In other words, I think we
> > should check PCD "PcdPlatformRecoverySupport" instead of Local variable
> "PlatformRecovery"
> > (from OsIndications variable) like the code below. What do you guys
> > think? If you need a meeting or short talk to discuss this, feel free to let me
> know.
> >
> >   if (!BootSuccess) {
> > -   if (PlatformRecovery) {
> > +  if (PcdGetBool (PcdPlatformRecoverySupport)) {
> >       LoadOptions = EfiBootManagerGetLoadOptions (&LoadOptionCount,
> > LoadOptionTypePlatformRecovery);
> >       ProcessLoadOptions (LoadOptions, LoadOptionCount);
> >       EfiBootManagerFreeLoadOptions (LoadOptions, LoadOptionCount);
> >     } else {
> >       //
> >       // When platform recovery is not enabled, still boot to platform
> > default file path.
> >       //
> >       EfiBootManagerProcessLoadOption (&PlatformDefaultBootOption);
> >     }
> >
> >
> > In addition, it looks like EDK2 don't have code to process
> > OsRecovery#### variables. Do we need to create a Bug on TianoCore
> Bugzilla system?

OsRecovery#### doesn't have an implement yet, we should co-work with the OS vendor to define the operation. For now, there is no requirement.

> >
> > Moreover, I saw that both of you had a discussion about "Default
> > PlatformRecovery", but I can't figure out the connection between the
> > discussion and the final code change. Isn't the "Default PlatformRecovery"
> > part of the Platform Recovery feature? At this moment, we don't have
> > OS recovery support, so I think that NO platform recovery support can
> > be identified as NO boot option recovery support. For this case,
> > shouldn't we use PCD "PcdPlatformRecoverySupport" to control "Default
> > PlatformRecovery" as well? For the next step, I think we need to get
> > further clarification from USWG to either not tie "Default Boot
> > Behavior" with PlatformRecovery or update the description to the
> following:
> >     - If system firmware supports Platform recovery as described in
> > Section 3.4.2, system firmware must include a PlatformRecovery####
> > variable specifying a short-form File Path Media Device Path....

As I said above the 'Default Platform Recovery' refer to the short-form file path boot. When the PcdPlatformRecoverySupport is TRUE, it is part of PlatformRecovery. Ohterwise, it is not part of platform recovery and we still need to support it because of the capability issue. And that isn't conflict with the spec.

Thanks,
Zhichao

> >
> > Regards,
> > Sunny Wang
> >

Re: [edk2-devel] Use a pcd to control PlatformRecovery

[Wang, Sunny (HPS SW)]

Thanks for checking this, Zhichao and Ray. 
I just sent a patch to fix it with the subject " [edk2-devel] [PATCH] MdeModulePkg/BdsDxe: Make PlatformRecovery work regardless of OsIndications"

Regards,
Sunny Wang

-----Original Message-----
From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Gao, Zhichao
Sent: Wednesday, October 16, 2019 1:57 PM
To: Wang, Sunny (HPS SW) <sunnywang@hpe.com>; devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>
Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@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>; Bret Barkelew <Bret.Barkelew@microsoft.com>; Li, Walon <walon.li@hpe.com>; Wei, Kent (HPS SW) <kent.wei@hpe.com>
Subject: Re: [edk2-devel] Use a pcd to control PlatformRecovery
Importance: High


> -----Original Message-----
> From: Gao, Zhichao
> Sent: Wednesday, October 16, 2019 10:09 AM
> To: 'Wang, Sunny (HPS SW)' <sunnywang@hpe.com>; devel@edk2.groups.io; 
> Ni, Ray <ray.ni@intel.com>
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A 
> <hao.a.wu@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>; Bret Barkelew 
> <Bret.Barkelew@microsoft.com>; Li, Walon <walon.li@hpe.com>; Wei, Kent 
> (HPS SW) <kent.wei@hpe.com>
> Subject: RE: Use a pcd to control PlatformRecovery
> 
> First of all, the patch didn't aim to change the other part of the 
> boot flow except PlatformRecovery.
> 
> Local variable PlatformRecovery is controlled by OsIndications 
> variable. When the EFI_OS_INDICATIONS_START_PLATFORM_RECOVERY is set, 
> the firmware should try to platform specific recovery. But that 
> doesn't mean the platform must support the specific recovery. i.e. 
> local PlatformRecovery is controlling the boot flow and the Pcd just 
> indicated whether the platform support recovery or not.
> So, on my opinion, I don't agree with your change.

After discuss with Sunny and Ray, and refer to the spec section 3.4.1 and 3.4.2, the OsRecovery and PlatformRecovery should always be operated regardless of the value of OsIndication variable if fail to boot the BootOrder. I am wrong. We should change to use the PcdPlatformRecoverySupport to control the PlatformRecovery. Please help to send a patch to fix it. Thanks a lot.

> 
> Default Platform Recovery refer to the short file path to boot the OS. 
> If the firmware supports platform recovery, then *short file path* 
> option would be one part of the PlatformRecovery#### in case there are 
> no other boot options. If the firmware doesn't support platform 
> recovery, we still need this default boot thru a short file path and 
> it should not depend on the PlatformRecovery#### variable for the compatibility thinking.
> 
> Thanks,
> Zhichao
> 
> > -----Original Message-----
> > From: Wang, Sunny (HPS SW) [mailto:sunnywang@hpe.com]
> > Sent: Tuesday, October 15, 2019 4:53 PM
> > To: devel@edk2.groups.io; Gao, Zhichao <zhichao.gao@intel.com>; Ni, 
> > Ray <ray.ni@intel.com>
> > Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A 
> > <hao.a.wu@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>; Bret Barkelew 
> > <Bret.Barkelew@microsoft.com>; Li, Walon <walon.li@hpe.com>; Wei,
> Kent
> > (HPS SW) <kent.wei@hpe.com>; Wang, Sunny (HPS SW)
> <sunnywang@hpe.com>
> > Subject: Use a pcd to control PlatformRecovery
> >
> > Hi Zhichao and Ray,
> >
> > I have some questions about this code change. Sorry for being late 
> > to bring my questions here.
> >
> > For now, the code block for iterating the PlatformRecovery#### 
> > variables is controlled by OsIndications variable. However, it looks 
> > to me like that the PlatformRecovery#### should still be attempted 
> > for the case of that processing of BootOrder does NOT result in 
> > success (according to section 3.4 in UEFI 2.8). In other words, I 
> > think we should check PCD "PcdPlatformRecoverySupport" instead of 
> > Local variable
> "PlatformRecovery"
> > (from OsIndications variable) like the code below. What do you guys 
> > think? If you need a meeting or short talk to discuss this, feel 
> > free to let me
> know.
> >
> >   if (!BootSuccess) {
> > -   if (PlatformRecovery) {
> > +  if (PcdGetBool (PcdPlatformRecoverySupport)) {
> >       LoadOptions = EfiBootManagerGetLoadOptions (&LoadOptionCount, 
> > LoadOptionTypePlatformRecovery);
> >       ProcessLoadOptions (LoadOptions, LoadOptionCount);
> >       EfiBootManagerFreeLoadOptions (LoadOptions, LoadOptionCount);
> >     } else {
> >       //
> >       // When platform recovery is not enabled, still boot to 
> > platform default file path.
> >       //
> >       EfiBootManagerProcessLoadOption (&PlatformDefaultBootOption);
> >     }
> >
> >
> > In addition, it looks like EDK2 don't have code to process 
> > OsRecovery#### variables. Do we need to create a Bug on TianoCore
> Bugzilla system?

OsRecovery#### doesn't have an implement yet, we should co-work with the OS vendor to define the operation. For now, there is no requirement.

> >
> > Moreover, I saw that both of you had a discussion about "Default 
> > PlatformRecovery", but I can't figure out the connection between the 
> > discussion and the final code change. Isn't the "Default PlatformRecovery"
> > part of the Platform Recovery feature? At this moment, we don't have 
> > OS recovery support, so I think that NO platform recovery support 
> > can be identified as NO boot option recovery support. For this case, 
> > shouldn't we use PCD "PcdPlatformRecoverySupport" to control 
> > "Default PlatformRecovery" as well? For the next step, I think we 
> > need to get further clarification from USWG to either not tie 
> > "Default Boot Behavior" with PlatformRecovery or update the 
> > description to the
> following:
> >     - If system firmware supports Platform recovery as described in 
> > Section 3.4.2, system firmware must include a PlatformRecovery#### 
> > variable specifying a short-form File Path Media Device Path....

As I said above the 'Default Platform Recovery' refer to the short-form file path boot. When the PcdPlatformRecoverySupport is TRUE, it is part of PlatformRecovery. Ohterwise, it is not part of platform recovery and we still need to support it because of the capability issue. And that isn't conflict with the spec.

Thanks,
Zhichao

> >
> > Regards,
> > Sunny Wang
> >

Re: [edk2-devel] Use a pcd to control PlatformRecovery

[Ni, Ray]

Sunny,
For the OS recovery question, I had the code change in https://github.com/niruiyu/edk2/tree/bds/osrecovery.

Due to there is no OS support for OS recovery yet, the code was not pushed.

Do you see any needs of the OS recovery?

Thanks,
Ray

> -----Original Message-----
> From: Wang, Sunny (HPS SW) <sunnywang@hpe.com>
> Sent: Wednesday, October 16, 2019 3:42 PM
> To: devel@edk2.groups.io; Gao, Zhichao <zhichao.gao@intel.com>; Ni, Ray
> <ray.ni@intel.com>
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@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>; Bret Barkelew
> <Bret.Barkelew@microsoft.com>; Li, Walon <walon.li@hpe.com>; Wei, Kent
> (HPS SW) <kent.wei@hpe.com>; Spottswood, Jason
> <jason.spottswood@hpe.com>; Wang, Sunny (HPS SW)
> <sunnywang@hpe.com>
> Subject: RE: [edk2-devel] Use a pcd to control PlatformRecovery
> 
> Thanks for checking this, Zhichao and Ray.
> I just sent a patch to fix it with the subject " [edk2-devel] [PATCH]
> MdeModulePkg/BdsDxe: Make PlatformRecovery work regardless of
> OsIndications"
> 
> Regards,
> Sunny Wang
> 
> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Gao, Zhichao
> Sent: Wednesday, October 16, 2019 1:57 PM
> To: Wang, Sunny (HPS SW) <sunnywang@hpe.com>; devel@edk2.groups.io;
> Ni, Ray <ray.ni@intel.com>
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@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>; Bret Barkelew
> <Bret.Barkelew@microsoft.com>; Li, Walon <walon.li@hpe.com>; Wei, Kent
> (HPS SW) <kent.wei@hpe.com>
> Subject: Re: [edk2-devel] Use a pcd to control PlatformRecovery
> Importance: High
> 
> 
> > -----Original Message-----
> > From: Gao, Zhichao
> > Sent: Wednesday, October 16, 2019 10:09 AM
> > To: 'Wang, Sunny (HPS SW)' <sunnywang@hpe.com>;
> devel@edk2.groups.io;
> > Ni, Ray <ray.ni@intel.com>
> > Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> > <hao.a.wu@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>; Bret Barkelew
> > <Bret.Barkelew@microsoft.com>; Li, Walon <walon.li@hpe.com>; Wei,
> Kent
> > (HPS SW) <kent.wei@hpe.com>
> > Subject: RE: Use a pcd to control PlatformRecovery
> >
> > First of all, the patch didn't aim to change the other part of the
> > boot flow except PlatformRecovery.
> >
> > Local variable PlatformRecovery is controlled by OsIndications
> > variable. When the EFI_OS_INDICATIONS_START_PLATFORM_RECOVERY is
> set,
> > the firmware should try to platform specific recovery. But that
> > doesn't mean the platform must support the specific recovery. i.e.
> > local PlatformRecovery is controlling the boot flow and the Pcd just
> > indicated whether the platform support recovery or not.
> > So, on my opinion, I don't agree with your change.
> 
> After discuss with Sunny and Ray, and refer to the spec section 3.4.1 and
> 3.4.2, the OsRecovery and PlatformRecovery should always be operated
> regardless of the value of OsIndication variable if fail to boot the BootOrder. I
> am wrong. We should change to use the PcdPlatformRecoverySupport to
> control the PlatformRecovery. Please help to send a patch to fix it. Thanks a
> lot.
> 
> >
> > Default Platform Recovery refer to the short file path to boot the OS.
> > If the firmware supports platform recovery, then *short file path*
> > option would be one part of the PlatformRecovery#### in case there are
> > no other boot options. If the firmware doesn't support platform
> > recovery, we still need this default boot thru a short file path and
> > it should not depend on the PlatformRecovery#### variable for the
> compatibility thinking.
> >
> > Thanks,
> > Zhichao
> >
> > > -----Original Message-----
> > > From: Wang, Sunny (HPS SW) [mailto:sunnywang@hpe.com]
> > > Sent: Tuesday, October 15, 2019 4:53 PM
> > > To: devel@edk2.groups.io; Gao, Zhichao <zhichao.gao@intel.com>; Ni,
> > > Ray <ray.ni@intel.com>
> > > Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> > > <hao.a.wu@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>; Bret Barkelew
> > > <Bret.Barkelew@microsoft.com>; Li, Walon <walon.li@hpe.com>; Wei,
> > Kent
> > > (HPS SW) <kent.wei@hpe.com>; Wang, Sunny (HPS SW)
> > <sunnywang@hpe.com>
> > > Subject: Use a pcd to control PlatformRecovery
> > >
> > > Hi Zhichao and Ray,
> > >
> > > I have some questions about this code change. Sorry for being late
> > > to bring my questions here.
> > >
> > > For now, the code block for iterating the PlatformRecovery####
> > > variables is controlled by OsIndications variable. However, it looks
> > > to me like that the PlatformRecovery#### should still be attempted
> > > for the case of that processing of BootOrder does NOT result in
> > > success (according to section 3.4 in UEFI 2.8). In other words, I
> > > think we should check PCD "PcdPlatformRecoverySupport" instead of
> > > Local variable
> > "PlatformRecovery"
> > > (from OsIndications variable) like the code below. What do you guys
> > > think? If you need a meeting or short talk to discuss this, feel
> > > free to let me
> > know.
> > >
> > >   if (!BootSuccess) {
> > > -   if (PlatformRecovery) {
> > > +  if (PcdGetBool (PcdPlatformRecoverySupport)) {
> > >       LoadOptions = EfiBootManagerGetLoadOptions (&LoadOptionCount,
> > > LoadOptionTypePlatformRecovery);
> > >       ProcessLoadOptions (LoadOptions, LoadOptionCount);
> > >       EfiBootManagerFreeLoadOptions (LoadOptions, LoadOptionCount);
> > >     } else {
> > >       //
> > >       // When platform recovery is not enabled, still boot to
> > > platform default file path.
> > >       //
> > >       EfiBootManagerProcessLoadOption (&PlatformDefaultBootOption);
> > >     }
> > >
> > >
> > > In addition, it looks like EDK2 don't have code to process
> > > OsRecovery#### variables. Do we need to create a Bug on TianoCore
> > Bugzilla system?
> 
> OsRecovery#### doesn't have an implement yet, we should co-work with
> the OS vendor to define the operation. For now, there is no requirement.
> 
> > >
> > > Moreover, I saw that both of you had a discussion about "Default
> > > PlatformRecovery", but I can't figure out the connection between the
> > > discussion and the final code change. Isn't the "Default
> PlatformRecovery"
> > > part of the Platform Recovery feature? At this moment, we don't have
> > > OS recovery support, so I think that NO platform recovery support
> > > can be identified as NO boot option recovery support. For this case,
> > > shouldn't we use PCD "PcdPlatformRecoverySupport" to control
> > > "Default PlatformRecovery" as well? For the next step, I think we
> > > need to get further clarification from USWG to either not tie
> > > "Default Boot Behavior" with PlatformRecovery or update the
> > > description to the
> > following:
> > >     - If system firmware supports Platform recovery as described in
> > > Section 3.4.2, system firmware must include a PlatformRecovery####
> > > variable specifying a short-form File Path Media Device Path....
> 
> As I said above the 'Default Platform Recovery' refer to the short-form file
> path boot. When the PcdPlatformRecoverySupport is TRUE, it is part of
> PlatformRecovery. Ohterwise, it is not part of platform recovery and we still
> need to support it because of the capability issue. And that isn't conflict with
> the spec.
> 
> Thanks,
> Zhichao
> 
> > >
> > > Regards,
> > > Sunny Wang
> > >
> 
> 
> 

Re: [edk2-devel] Use a pcd to control PlatformRecovery

[Wang, Sunny (HPS SW)]

Sorry for the delay and thanks for checking my question, Ray.

Since the OS recovery and platform recovery options were added to UEFI specification at the same time, I thought we will also implement it and push the code regardless of the OS support. 

No, I didn't see a real need of using the OS recovery option. If my memory serves me well, I have only seen a need of using "One-Time" OS recovery, but it can be done by creating, adjusting, and removing a boot option in boot order. 
However, I can still imagine that the OS recovery option is needed for the use of "persistent" OS recovery. Therefore, if we don't have any concern about adding OS recovery option support, I think it is still better to push the code because the UEFI specification mentioned it. Add Peter. He may know more information about this.

By the way, I think if no one works on the task below, there will be no OS or OS tool supporting it. :)
   - https://github.com/rhboot/efibootmgr/issues/41


Regards,
Sunny Wang

-----Original Message-----
From: Ni, Ray [mailto:ray.ni@intel.com] 
Sent: Wednesday, October 16, 2019 4:43 PM
To: Wang, Sunny (HPS SW) <sunnywang@hpe.com>; devel@edk2.groups.io; Gao, Zhichao <zhichao.gao@intel.com>
Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@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>; Bret Barkelew <Bret.Barkelew@microsoft.com>; Li, Walon <walon.li@hpe.com>; Wei, Kent (HPS SW) <kent.wei@hpe.com>; Spottswood, Jason <jason.spottswood@hpe.com>
Subject: RE: [edk2-devel] Use a pcd to control PlatformRecovery
Importance: High

Sunny,
For the OS recovery question, I had the code change in https://github.com/niruiyu/edk2/tree/bds/osrecovery.

Due to there is no OS support for OS recovery yet, the code was not pushed.

Do you see any needs of the OS recovery?

Thanks,
Ray

> -----Original Message-----
> From: Wang, Sunny (HPS SW) <sunnywang@hpe.com>
> Sent: Wednesday, October 16, 2019 3:42 PM
> To: devel@edk2.groups.io; Gao, Zhichao <zhichao.gao@intel.com>; Ni, 
> Ray <ray.ni@intel.com>
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A 
> <hao.a.wu@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>; Bret Barkelew 
> <Bret.Barkelew@microsoft.com>; Li, Walon <walon.li@hpe.com>; Wei, Kent 
> (HPS SW) <kent.wei@hpe.com>; Spottswood, Jason 
> <jason.spottswood@hpe.com>; Wang, Sunny (HPS SW) <sunnywang@hpe.com>
> Subject: RE: [edk2-devel] Use a pcd to control PlatformRecovery
> 
> Thanks for checking this, Zhichao and Ray.
> I just sent a patch to fix it with the subject " [edk2-devel] [PATCH]
> MdeModulePkg/BdsDxe: Make PlatformRecovery work regardless of 
> OsIndications"
> 
> Regards,
> Sunny Wang
> 
> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of 
> Gao, Zhichao
> Sent: Wednesday, October 16, 2019 1:57 PM
> To: Wang, Sunny (HPS SW) <sunnywang@hpe.com>; devel@edk2.groups.io; 
> Ni, Ray <ray.ni@intel.com>
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A 
> <hao.a.wu@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>; Bret Barkelew 
> <Bret.Barkelew@microsoft.com>; Li, Walon <walon.li@hpe.com>; Wei, Kent 
> (HPS SW) <kent.wei@hpe.com>
> Subject: Re: [edk2-devel] Use a pcd to control PlatformRecovery
> Importance: High
> 
> 
> > -----Original Message-----
> > From: Gao, Zhichao
> > Sent: Wednesday, October 16, 2019 10:09 AM
> > To: 'Wang, Sunny (HPS SW)' <sunnywang@hpe.com>;
> devel@edk2.groups.io;
> > Ni, Ray <ray.ni@intel.com>
> > Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A 
> > <hao.a.wu@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>; Bret Barkelew 
> > <Bret.Barkelew@microsoft.com>; Li, Walon <walon.li@hpe.com>; Wei,
> Kent
> > (HPS SW) <kent.wei@hpe.com>
> > Subject: RE: Use a pcd to control PlatformRecovery
> >
> > First of all, the patch didn't aim to change the other part of the 
> > boot flow except PlatformRecovery.
> >
> > Local variable PlatformRecovery is controlled by OsIndications 
> > variable. When the EFI_OS_INDICATIONS_START_PLATFORM_RECOVERY is
> set,
> > the firmware should try to platform specific recovery. But that 
> > doesn't mean the platform must support the specific recovery. i.e.
> > local PlatformRecovery is controlling the boot flow and the Pcd just 
> > indicated whether the platform support recovery or not.
> > So, on my opinion, I don't agree with your change.
> 
> After discuss with Sunny and Ray, and refer to the spec section 3.4.1 
> and 3.4.2, the OsRecovery and PlatformRecovery should always be 
> operated regardless of the value of OsIndication variable if fail to 
> boot the BootOrder. I am wrong. We should change to use the 
> PcdPlatformRecoverySupport to control the PlatformRecovery. Please 
> help to send a patch to fix it. Thanks a lot.
> 
> >
> > Default Platform Recovery refer to the short file path to boot the OS.
> > If the firmware supports platform recovery, then *short file path* 
> > option would be one part of the PlatformRecovery#### in case there 
> > are no other boot options. If the firmware doesn't support platform 
> > recovery, we still need this default boot thru a short file path and 
> > it should not depend on the PlatformRecovery#### variable for the
> compatibility thinking.
> >
> > Thanks,
> > Zhichao
> >
> > > -----Original Message-----
> > > From: Wang, Sunny (HPS SW) [mailto:sunnywang@hpe.com]
> > > Sent: Tuesday, October 15, 2019 4:53 PM
> > > To: devel@edk2.groups.io; Gao, Zhichao <zhichao.gao@intel.com>; 
> > > Ni, Ray <ray.ni@intel.com>
> > > Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A 
> > > <hao.a.wu@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>; Bret Barkelew 
> > > <Bret.Barkelew@microsoft.com>; Li, Walon <walon.li@hpe.com>; Wei,
> > Kent
> > > (HPS SW) <kent.wei@hpe.com>; Wang, Sunny (HPS SW)
> > <sunnywang@hpe.com>
> > > Subject: Use a pcd to control PlatformRecovery
> > >
> > > Hi Zhichao and Ray,
> > >
> > > I have some questions about this code change. Sorry for being late 
> > > to bring my questions here.
> > >
> > > For now, the code block for iterating the PlatformRecovery#### 
> > > variables is controlled by OsIndications variable. However, it 
> > > looks to me like that the PlatformRecovery#### should still be 
> > > attempted for the case of that processing of BootOrder does NOT 
> > > result in success (according to section 3.4 in UEFI 2.8). In other 
> > > words, I think we should check PCD "PcdPlatformRecoverySupport" 
> > > instead of Local variable
> > "PlatformRecovery"
> > > (from OsIndications variable) like the code below. What do you 
> > > guys think? If you need a meeting or short talk to discuss this, 
> > > feel free to let me
> > know.
> > >
> > >   if (!BootSuccess) {
> > > -   if (PlatformRecovery) {
> > > +  if (PcdGetBool (PcdPlatformRecoverySupport)) {
> > >       LoadOptions = EfiBootManagerGetLoadOptions 
> > > (&LoadOptionCount, LoadOptionTypePlatformRecovery);
> > >       ProcessLoadOptions (LoadOptions, LoadOptionCount);
> > >       EfiBootManagerFreeLoadOptions (LoadOptions, LoadOptionCount);
> > >     } else {
> > >       //
> > >       // When platform recovery is not enabled, still boot to 
> > > platform default file path.
> > >       //
> > >       EfiBootManagerProcessLoadOption (&PlatformDefaultBootOption);
> > >     }
> > >
> > >
> > > In addition, it looks like EDK2 don't have code to process 
> > > OsRecovery#### variables. Do we need to create a Bug on TianoCore
> > Bugzilla system?
> 
> OsRecovery#### doesn't have an implement yet, we should co-work with 
> the OS vendor to define the operation. For now, there is no requirement.
> 
> > >
> > > Moreover, I saw that both of you had a discussion about "Default 
> > > PlatformRecovery", but I can't figure out the connection between 
> > > the discussion and the final code change. Isn't the "Default
> PlatformRecovery"
> > > part of the Platform Recovery feature? At this moment, we don't 
> > > have OS recovery support, so I think that NO platform recovery 
> > > support can be identified as NO boot option recovery support. For 
> > > this case, shouldn't we use PCD "PcdPlatformRecoverySupport" to 
> > > control "Default PlatformRecovery" as well? For the next step, I 
> > > think we need to get further clarification from USWG to either not 
> > > tie "Default Boot Behavior" with PlatformRecovery or update the 
> > > description to the
> > following:
> > >     - If system firmware supports Platform recovery as described 
> > > in Section 3.4.2, system firmware must include a 
> > > PlatformRecovery#### variable specifying a short-form File Path Media Device Path....
> 
> As I said above the 'Default Platform Recovery' refer to the 
> short-form file path boot. When the PcdPlatformRecoverySupport is 
> TRUE, it is part of PlatformRecovery. Ohterwise, it is not part of 
> platform recovery and we still need to support it because of the 
> capability issue. And that isn't conflict with the spec.
> 
> Thanks,
> Zhichao
> 
> > >
> > > Regards,
> > > Sunny Wang
> > >
> 
> 
> 

Re: [edk2-devel] Use a pcd to control PlatformRecovery

[Ni, Ray]

Sean,
Do you have any comments from MS side?

Thanks,
Ray


> -----Original Message-----
> From: Wang, Sunny (HPS SW) <sunnywang@hpe.com>
> Sent: Monday, October 21, 2019 8:45 PM
> To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io; Gao, Zhichao
> <zhichao.gao@intel.com>; pjones@redhat.com
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@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>; Bret Barkelew
> <Bret.Barkelew@microsoft.com>; Li, Walon <walon.li@hpe.com>; Wei, Kent
> (HPS SW) <kent.wei@hpe.com>; Spottswood, Jason
> <jason.spottswood@hpe.com>; Wang, Sunny (HPS SW)
> <sunnywang@hpe.com>
> Subject: RE: [edk2-devel] Use a pcd to control PlatformRecovery
> 
> Sorry for the delay and thanks for checking my question, Ray.
> 
> Since the OS recovery and platform recovery options were added to UEFI
> specification at the same time, I thought we will also implement it and push
> the code regardless of the OS support.
> 
> No, I didn't see a real need of using the OS recovery option. If my memory
> serves me well, I have only seen a need of using "One-Time" OS recovery,
> but it can be done by creating, adjusting, and removing a boot option in boot
> order.
> However, I can still imagine that the OS recovery option is needed for the use
> of "persistent" OS recovery. Therefore, if we don't have any concern about
> adding OS recovery option support, I think it is still better to push the code
> because the UEFI specification mentioned it. Add Peter. He may know more
> information about this.
> 
> By the way, I think if no one works on the task below, there will be no OS or
> OS tool supporting it. :)
>    - https://github.com/rhboot/efibootmgr/issues/41
> 
> 
> Regards,
> Sunny Wang
> 
> -----Original Message-----
> From: Ni, Ray [mailto:ray.ni@intel.com]
> Sent: Wednesday, October 16, 2019 4:43 PM
> To: Wang, Sunny (HPS SW) <sunnywang@hpe.com>; devel@edk2.groups.io;
> Gao, Zhichao <zhichao.gao@intel.com>
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@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>; Bret Barkelew
> <Bret.Barkelew@microsoft.com>; Li, Walon <walon.li@hpe.com>; Wei, Kent
> (HPS SW) <kent.wei@hpe.com>; Spottswood, Jason
> <jason.spottswood@hpe.com>
> Subject: RE: [edk2-devel] Use a pcd to control PlatformRecovery
> Importance: High
> 
> Sunny,
> For the OS recovery question, I had the code change in
> https://github.com/niruiyu/edk2/tree/bds/osrecovery.
> 
> Due to there is no OS support for OS recovery yet, the code was not pushed.
> 
> Do you see any needs of the OS recovery?
> 
> Thanks,
> Ray
> 
> > -----Original Message-----
> > From: Wang, Sunny (HPS SW) <sunnywang@hpe.com>
> > Sent: Wednesday, October 16, 2019 3:42 PM
> > To: devel@edk2.groups.io; Gao, Zhichao <zhichao.gao@intel.com>; Ni,
> > Ray <ray.ni@intel.com>
> > Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> > <hao.a.wu@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>; Bret Barkelew
> > <Bret.Barkelew@microsoft.com>; Li, Walon <walon.li@hpe.com>; Wei,
> Kent
> > (HPS SW) <kent.wei@hpe.com>; Spottswood, Jason
> > <jason.spottswood@hpe.com>; Wang, Sunny (HPS SW)
> <sunnywang@hpe.com>
> > Subject: RE: [edk2-devel] Use a pcd to control PlatformRecovery
> >
> > Thanks for checking this, Zhichao and Ray.
> > I just sent a patch to fix it with the subject " [edk2-devel] [PATCH]
> > MdeModulePkg/BdsDxe: Make PlatformRecovery work regardless of
> > OsIndications"
> >
> > Regards,
> > Sunny Wang
> >
> > -----Original Message-----
> > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> > Gao, Zhichao
> > Sent: Wednesday, October 16, 2019 1:57 PM
> > To: Wang, Sunny (HPS SW) <sunnywang@hpe.com>;
> devel@edk2.groups.io;
> > Ni, Ray <ray.ni@intel.com>
> > Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> > <hao.a.wu@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>; Bret Barkelew
> > <Bret.Barkelew@microsoft.com>; Li, Walon <walon.li@hpe.com>; Wei,
> Kent
> > (HPS SW) <kent.wei@hpe.com>
> > Subject: Re: [edk2-devel] Use a pcd to control PlatformRecovery
> > Importance: High
> >
> >
> > > -----Original Message-----
> > > From: Gao, Zhichao
> > > Sent: Wednesday, October 16, 2019 10:09 AM
> > > To: 'Wang, Sunny (HPS SW)' <sunnywang@hpe.com>;
> > devel@edk2.groups.io;
> > > Ni, Ray <ray.ni@intel.com>
> > > Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> > > <hao.a.wu@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>; Bret Barkelew
> > > <Bret.Barkelew@microsoft.com>; Li, Walon <walon.li@hpe.com>; Wei,
> > Kent
> > > (HPS SW) <kent.wei@hpe.com>
> > > Subject: RE: Use a pcd to control PlatformRecovery
> > >
> > > First of all, the patch didn't aim to change the other part of the
> > > boot flow except PlatformRecovery.
> > >
> > > Local variable PlatformRecovery is controlled by OsIndications
> > > variable. When the EFI_OS_INDICATIONS_START_PLATFORM_RECOVERY
> is
> > set,
> > > the firmware should try to platform specific recovery. But that
> > > doesn't mean the platform must support the specific recovery. i.e.
> > > local PlatformRecovery is controlling the boot flow and the Pcd just
> > > indicated whether the platform support recovery or not.
> > > So, on my opinion, I don't agree with your change.
> >
> > After discuss with Sunny and Ray, and refer to the spec section 3.4.1
> > and 3.4.2, the OsRecovery and PlatformRecovery should always be
> > operated regardless of the value of OsIndication variable if fail to
> > boot the BootOrder. I am wrong. We should change to use the
> > PcdPlatformRecoverySupport to control the PlatformRecovery. Please
> > help to send a patch to fix it. Thanks a lot.
> >
> > >
> > > Default Platform Recovery refer to the short file path to boot the OS.
> > > If the firmware supports platform recovery, then *short file path*
> > > option would be one part of the PlatformRecovery#### in case there
> > > are no other boot options. If the firmware doesn't support platform
> > > recovery, we still need this default boot thru a short file path and
> > > it should not depend on the PlatformRecovery#### variable for the
> > compatibility thinking.
> > >
> > > Thanks,
> > > Zhichao
> > >
> > > > -----Original Message-----
> > > > From: Wang, Sunny (HPS SW) [mailto:sunnywang@hpe.com]
> > > > Sent: Tuesday, October 15, 2019 4:53 PM
> > > > To: devel@edk2.groups.io; Gao, Zhichao <zhichao.gao@intel.com>;
> > > > Ni, Ray <ray.ni@intel.com>
> > > > Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> > > > <hao.a.wu@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>; Bret Barkelew
> > > > <Bret.Barkelew@microsoft.com>; Li, Walon <walon.li@hpe.com>; Wei,
> > > Kent
> > > > (HPS SW) <kent.wei@hpe.com>; Wang, Sunny (HPS SW)
> > > <sunnywang@hpe.com>
> > > > Subject: Use a pcd to control PlatformRecovery
> > > >
> > > > Hi Zhichao and Ray,
> > > >
> > > > I have some questions about this code change. Sorry for being late
> > > > to bring my questions here.
> > > >
> > > > For now, the code block for iterating the PlatformRecovery####
> > > > variables is controlled by OsIndications variable. However, it
> > > > looks to me like that the PlatformRecovery#### should still be
> > > > attempted for the case of that processing of BootOrder does NOT
> > > > result in success (according to section 3.4 in UEFI 2.8). In other
> > > > words, I think we should check PCD "PcdPlatformRecoverySupport"
> > > > instead of Local variable
> > > "PlatformRecovery"
> > > > (from OsIndications variable) like the code below. What do you
> > > > guys think? If you need a meeting or short talk to discuss this,
> > > > feel free to let me
> > > know.
> > > >
> > > >   if (!BootSuccess) {
> > > > -   if (PlatformRecovery) {
> > > > +  if (PcdGetBool (PcdPlatformRecoverySupport)) {
> > > >       LoadOptions = EfiBootManagerGetLoadOptions
> > > > (&LoadOptionCount, LoadOptionTypePlatformRecovery);
> > > >       ProcessLoadOptions (LoadOptions, LoadOptionCount);
> > > >       EfiBootManagerFreeLoadOptions (LoadOptions, LoadOptionCount);
> > > >     } else {
> > > >       //
> > > >       // When platform recovery is not enabled, still boot to
> > > > platform default file path.
> > > >       //
> > > >       EfiBootManagerProcessLoadOption (&PlatformDefaultBootOption);
> > > >     }
> > > >
> > > >
> > > > In addition, it looks like EDK2 don't have code to process
> > > > OsRecovery#### variables. Do we need to create a Bug on TianoCore
> > > Bugzilla system?
> >
> > OsRecovery#### doesn't have an implement yet, we should co-work with
> > the OS vendor to define the operation. For now, there is no requirement.
> >
> > > >
> > > > Moreover, I saw that both of you had a discussion about "Default
> > > > PlatformRecovery", but I can't figure out the connection between
> > > > the discussion and the final code change. Isn't the "Default
> > PlatformRecovery"
> > > > part of the Platform Recovery feature? At this moment, we don't
> > > > have OS recovery support, so I think that NO platform recovery
> > > > support can be identified as NO boot option recovery support. For
> > > > this case, shouldn't we use PCD "PcdPlatformRecoverySupport" to
> > > > control "Default PlatformRecovery" as well? For the next step, I
> > > > think we need to get further clarification from USWG to either not
> > > > tie "Default Boot Behavior" with PlatformRecovery or update the
> > > > description to the
> > > following:
> > > >     - If system firmware supports Platform recovery as described
> > > > in Section 3.4.2, system firmware must include a
> > > > PlatformRecovery#### variable specifying a short-form File Path Media
> Device Path....
> >
> > As I said above the 'Default Platform Recovery' refer to the
> > short-form file path boot. When the PcdPlatformRecoverySupport is
> > TRUE, it is part of PlatformRecovery. Ohterwise, it is not part of
> > platform recovery and we still need to support it because of the
> > capability issue. And that isn't conflict with the spec.
> >
> > Thanks,
> > Zhichao
> >
> > > >
> > > > Regards,
> > > > Sunny Wang
> > > >
> >
> > 
> >
>