[PATCH 0/3] Fix 2 bugs relating to PlatformRecovery

[PATCH 0/3] Fix 2 bugs relating to PlatformRecovery

[Ruiyu Ni]

Please refer to individual patch for the bug details.
1/3 is only to refine the debug message.
2/3 and 3/3 are for bug fixes.

Ruiyu Ni (3):
  MdeModulePkg/UefiBootManagerLib: Refine the debug message
  MdeModulePkg/BdsDxe: Fix bug to run non-first PlatformRecovery####
  MdeModulePkg/BdsDxe: Avoid overwriting PlatformRecovery####

 .../Library/UefiBootManagerLib/BmLoadOption.c      |  7 ++--
 MdeModulePkg/Universal/BdsDxe/BdsEntry.c           | 37 ++++++++++++++++++----
 2 files changed, 34 insertions(+), 10 deletions(-)

-- 
2.9.0.windows.1

[PATCH 1/3] MdeModulePkg/UefiBootManagerLib: Refine the debug message

[Ruiyu Ni]

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
---
 MdeModulePkg/Library/UefiBootManagerLib/BmLoadOption.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmLoadOption.c b/MdeModulePkg/Library/UefiBootManagerLib/BmLoadOption.c
index e638e5f..6f705bd 100644
--- a/MdeModulePkg/Library/UefiBootManagerLib/BmLoadOption.c
+++ b/MdeModulePkg/Library/UefiBootManagerLib/BmLoadOption.c
@@ -1277,8 +1277,9 @@ EfiBootManagerProcessLoadOption (
   // Load and start the load option.
   //
   DEBUG ((
-    DEBUG_INFO | DEBUG_LOAD, "Process Load Option (%s%04x) ...\n",
-    mBmLoadOptionName[LoadOption->OptionType], LoadOption->OptionNumber
+    DEBUG_INFO | DEBUG_LOAD, "Process %s%04x (%s) ...\n",
+    mBmLoadOptionName[LoadOption->OptionType], LoadOption->OptionNumber,
+    LoadOption->Description
     ));
   ImageHandle = NULL;
   FileBuffer = EfiBootManagerGetLoadOptionBuffer (LoadOption->FilePath, &FilePath, &FileSize);
@@ -1321,7 +1322,7 @@ EfiBootManagerProcessLoadOption (
 
     LoadOption->Status = gBS->StartImage (ImageHandle, &LoadOption->ExitDataSize, &LoadOption->ExitData);
     DEBUG ((
-      DEBUG_INFO | DEBUG_LOAD, "Load Option (%s%04x) Return Status = %r\n",
+      DEBUG_INFO | DEBUG_LOAD, "%s%04x Return Status = %r\n",
       mBmLoadOptionName[LoadOption->OptionType], LoadOption->OptionNumber, LoadOption->Status
       ));
 
-- 
2.9.0.windows.1

[PATCH 2/3] MdeModulePkg/BdsDxe: Fix bug to run non-first PlatformRecovery####

[Ruiyu Ni]

The implementation doesn't check the LoadOptions[Index].Status but
only depends on the Status returned from
EfiBootManagerProcessLoadOption(), which results only the first
PlatformRecovery#### runs.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Jie Lin <jie.lin@intel.com>
---
 MdeModulePkg/Universal/BdsDxe/BdsEntry.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
index 72adc51..48e5351 100644
--- a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
+++ b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
@@ -443,17 +443,25 @@ ProcessLoadOptions (
       LoadOptionType = LoadOptions[Index].OptionType;
     }
     ASSERT (LoadOptionType == LoadOptions[Index].OptionType);
+    ASSERT (LoadOptionType != LoadOptionTypeBoot);
 
     Status = EfiBootManagerProcessLoadOption (&LoadOptions[Index]);
 
+    //
+    // Status indicates whether the load option is loaded and executed
+    // LoadOptions[Index].Status is what the load option returns
+    //
     if (!EFI_ERROR (Status)) {
-      if (LoadOptionType == LoadOptionTypePlatformRecovery) {
-        //
-        // Stop processing if any entry is successful
-        //
+      //
+      // Stop processing if any PlatformRecovery#### returns success.
+      //
+      if ((LoadOptions[Index].Status == EFI_SUCCESS) &&
+          (LoadOptionType == LoadOptionTypePlatformRecovery)) {
         break;
       }
-      if ((LoadOptions[Index].Attributes & LOAD_OPTION_FORCE_RECONNECT) != 0) {
+
+      if (!EFI_ERROR (LoadOptions[Index].Status) &&
+          (LoadOptions[Index].Attributes & LOAD_OPTION_FORCE_RECONNECT) != 0) {
         ReconnectAll = TRUE;
       }
     }
-- 
2.9.0.windows.1

[PATCH 3/3] MdeModulePkg/BdsDxe: Avoid overwriting PlatformRecovery####

[Ruiyu Ni]

Current implementation always creates PlatformRecovery0000
pointing to \EFI\BOOT\BOOT$(ARCH).efi but it may overwrite
PlatformRecovery#### created before (maybe by a DXE driver).

The patch only uses the smallest unused option number for
the \EFI\BOOT\BOOT$(ARCH).efi PlatformRecovery#### to avoid
overwriting already-created PlatformRecovery####.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Jie Lin <jie.lin@intel.com>
---
 MdeModulePkg/Universal/BdsDxe/BdsEntry.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
index 48e5351..56034f5 100644
--- a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
+++ b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
@@ -834,7 +834,7 @@ BdsEntry (
   FilePath = FileDevicePath (NULL, EFI_REMOVABLE_MEDIA_FILE_NAME);
   Status = EfiBootManagerInitializeLoadOption (
              &LoadOption,
-             0,
+             LoadOptionNumberUnassigned,
              LoadOptionTypePlatformRecovery,
              LOAD_OPTION_ACTIVE,
              L"Default PlatformRecovery",
@@ -843,9 +843,24 @@ BdsEntry (
              0
              );
   ASSERT_EFI_ERROR (Status);
-  EfiBootManagerLoadOptionToVariable (&LoadOption);
+  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);
+  }
   EfiBootManagerFreeLoadOption (&LoadOption);
   FreePool (FilePath);
+  EfiBootManagerFreeLoadOptions (LoadOptions, LoadOptionCount);
 
   //
   // Report Status Code to indicate connecting drivers will happen
-- 
2.9.0.windows.1

Re: [PATCH 1/3] MdeModulePkg/UefiBootManagerLib: Refine the debug message

[Wang, Sunny (HPS SW)]

Looks good to me. 
Reviewed-by: Sunny Wang <sunnywang@hpe.com>

-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ruiyu Ni
Sent: Tuesday, November 15, 2016 6:04 PM
To: edk2-devel@lists.01.org
Cc: Eric Dong <eric.dong@intel.com>
Subject: [edk2] [PATCH 1/3] MdeModulePkg/UefiBootManagerLib: Refine the debug message

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
---
 MdeModulePkg/Library/UefiBootManagerLib/BmLoadOption.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmLoadOption.c b/MdeModulePkg/Library/UefiBootManagerLib/BmLoadOption.c
index e638e5f..6f705bd 100644
--- a/MdeModulePkg/Library/UefiBootManagerLib/BmLoadOption.c
+++ b/MdeModulePkg/Library/UefiBootManagerLib/BmLoadOption.c
@@ -1277,8 +1277,9 @@ EfiBootManagerProcessLoadOption (
   // Load and start the load option.
   //
   DEBUG ((
-    DEBUG_INFO | DEBUG_LOAD, "Process Load Option (%s%04x) ...\n",
-    mBmLoadOptionName[LoadOption->OptionType], LoadOption->OptionNumber
+    DEBUG_INFO | DEBUG_LOAD, "Process %s%04x (%s) ...\n",
+    mBmLoadOptionName[LoadOption->OptionType], LoadOption->OptionNumber,
+    LoadOption->Description
     ));
   ImageHandle = NULL;
   FileBuffer = EfiBootManagerGetLoadOptionBuffer (LoadOption->FilePath, &FilePath, &FileSize); @@ -1321,7 +1322,7 @@ EfiBootManagerProcessLoadOption (
 
     LoadOption->Status = gBS->StartImage (ImageHandle, &LoadOption->ExitDataSize, &LoadOption->ExitData);
     DEBUG ((
-      DEBUG_INFO | DEBUG_LOAD, "Load Option (%s%04x) Return Status = %r\n",
+      DEBUG_INFO | DEBUG_LOAD, "%s%04x Return Status = %r\n",
       mBmLoadOptionName[LoadOption->OptionType], LoadOption->OptionNumber, LoadOption->Status
       ));
 
--
2.9.0.windows.1

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH 2/3] MdeModulePkg/BdsDxe: Fix bug to run non-first PlatformRecovery####

[Wang, Sunny (HPS SW)]

Looks good to me. 
Reviewed-by: Sunny Wang <sunnywang@hpe.com>

Just have a suggestion below. 
For the change below, it is related to Driver#### rather than PlatformRecovery####. Could you add more comment for this Driver#### related change into your log? It's fine to not update your patch. You can directly update it when you check in your code change.
+
+      if (!EFI_ERROR (LoadOptions[Index].Status) &&
+          (LoadOptions[Index].Attributes & LOAD_OPTION_FORCE_RECONNECT) 
+ != 0) {
         ReconnectAll = TRUE;
       }

-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ruiyu Ni
Sent: Tuesday, November 15, 2016 6:04 PM
To: edk2-devel@lists.01.org
Cc: Jie Lin <jie.lin@intel.com>
Subject: [edk2] [PATCH 2/3] MdeModulePkg/BdsDxe: Fix bug to run non-first PlatformRecovery####

The implementation doesn't check the LoadOptions[Index].Status but only depends on the Status returned from EfiBootManagerProcessLoadOption(), which results only the first PlatformRecovery#### runs.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Jie Lin <jie.lin@intel.com>
---
 MdeModulePkg/Universal/BdsDxe/BdsEntry.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
index 72adc51..48e5351 100644
--- a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
+++ b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
@@ -443,17 +443,25 @@ ProcessLoadOptions (
       LoadOptionType = LoadOptions[Index].OptionType;
     }
     ASSERT (LoadOptionType == LoadOptions[Index].OptionType);
+    ASSERT (LoadOptionType != LoadOptionTypeBoot);
 
     Status = EfiBootManagerProcessLoadOption (&LoadOptions[Index]);
 
+    //
+    // Status indicates whether the load option is loaded and executed
+    // LoadOptions[Index].Status is what the load option returns
+    //
     if (!EFI_ERROR (Status)) {
-      if (LoadOptionType == LoadOptionTypePlatformRecovery) {
-        //
-        // Stop processing if any entry is successful
-        //
+      //
+      // Stop processing if any PlatformRecovery#### returns success.
+      //
+      if ((LoadOptions[Index].Status == EFI_SUCCESS) &&
+          (LoadOptionType == LoadOptionTypePlatformRecovery)) {
         break;
       }
-      if ((LoadOptions[Index].Attributes & LOAD_OPTION_FORCE_RECONNECT) != 0) {
+
+      if (!EFI_ERROR (LoadOptions[Index].Status) &&
+          (LoadOptions[Index].Attributes & LOAD_OPTION_FORCE_RECONNECT) 
+ != 0) {
         ReconnectAll = TRUE;
       }
     }
--
2.9.0.windows.1

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH 3/3] MdeModulePkg/BdsDxe: Avoid overwriting PlatformRecovery####

[Wang, Sunny (HPS SW)]

Hi Ray,
This patch looks good to me. 
Reviewed-by: Sunny Wang <sunnywang@hpe.com>

Just a suggestion. If we can add the code block below into BmGetFreeOptionNumber(), I think it would be better.

+  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);
+  }

-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ruiyu Ni
Sent: Tuesday, November 15, 2016 6:04 PM
To: edk2-devel@lists.01.org
Cc: Jie Lin <jie.lin@intel.com>
Subject: [edk2] [PATCH 3/3] MdeModulePkg/BdsDxe: Avoid overwriting PlatformRecovery####

Current implementation always creates PlatformRecovery0000 pointing to \EFI\BOOT\BOOT$(ARCH).efi but it may overwrite PlatformRecovery#### created before (maybe by a DXE driver).

The patch only uses the smallest unused option number for the \EFI\BOOT\BOOT$(ARCH).efi PlatformRecovery#### to avoid overwriting already-created PlatformRecovery####.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Jie Lin <jie.lin@intel.com>
---
 MdeModulePkg/Universal/BdsDxe/BdsEntry.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
index 48e5351..56034f5 100644
--- a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
+++ b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
@@ -834,7 +834,7 @@ BdsEntry (
   FilePath = FileDevicePath (NULL, EFI_REMOVABLE_MEDIA_FILE_NAME);
   Status = EfiBootManagerInitializeLoadOption (
              &LoadOption,
-             0,
+             LoadOptionNumberUnassigned,
              LoadOptionTypePlatformRecovery,
              LOAD_OPTION_ACTIVE,
              L"Default PlatformRecovery", @@ -843,9 +843,24 @@ BdsEntry (
              0
              );
   ASSERT_EFI_ERROR (Status);
-  EfiBootManagerLoadOptionToVariable (&LoadOption);
+  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);
+  }
   EfiBootManagerFreeLoadOption (&LoadOption);
   FreePool (FilePath);
+  EfiBootManagerFreeLoadOptions (LoadOptions, LoadOptionCount);
 
   //
   // Report Status Code to indicate connecting drivers will happen
--
2.9.0.windows.1

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH 3/3] MdeModulePkg/BdsDxe: Avoid overwriting PlatformRecovery####

[Ni, Ruiyu]

BmGetFreeOptionNumber() is internal function. It cannot be used by BdsDxe driver.
If we found more usages for BmGetFreeOptionNumber() we can expose it publicly and
enhance the logic to support find free number for load option who doesn't have associated
*Order NV variable.


Regards,
Ray

>-----Original Message-----
>From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Wang, Sunny (HPS SW)
>Sent: Wednesday, November 16, 2016 5:08 PM
>To: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org
>Cc: Haskell, Darrell <darrell.haskell@hpe.com>; Lin, Jie <jie.lin@intel.com>
>Subject: Re: [edk2] [PATCH 3/3] MdeModulePkg/BdsDxe: Avoid overwriting PlatformRecovery####
>
>Hi Ray,
>This patch looks good to me.
>Reviewed-by: Sunny Wang <sunnywang@hpe.com>
>
>Just a suggestion. If we can add the code block below into BmGetFreeOptionNumber(), I think it would be better.
>
>+  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);
>+  }
>
>-----Original Message-----
>From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ruiyu Ni
>Sent: Tuesday, November 15, 2016 6:04 PM
>To: edk2-devel@lists.01.org
>Cc: Jie Lin <jie.lin@intel.com>
>Subject: [edk2] [PATCH 3/3] MdeModulePkg/BdsDxe: Avoid overwriting PlatformRecovery####
>
>Current implementation always creates PlatformRecovery0000 pointing to \EFI\BOOT\BOOT$(ARCH).efi but it may overwrite
>PlatformRecovery#### created before (maybe by a DXE driver).
>
>The patch only uses the smallest unused option number for the \EFI\BOOT\BOOT$(ARCH).efi PlatformRecovery#### to avoid
>overwriting already-created PlatformRecovery####.
>
>Contributed-under: TianoCore Contribution Agreement 1.0
>Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
>Cc: Jie Lin <jie.lin@intel.com>
>---
> MdeModulePkg/Universal/BdsDxe/BdsEntry.c | 19 +++++++++++++++++--
> 1 file changed, 17 insertions(+), 2 deletions(-)
>
>diff --git a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
>index 48e5351..56034f5 100644
>--- a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
>+++ b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
>@@ -834,7 +834,7 @@ BdsEntry (
>   FilePath = FileDevicePath (NULL, EFI_REMOVABLE_MEDIA_FILE_NAME);
>   Status = EfiBootManagerInitializeLoadOption (
>              &LoadOption,
>-             0,
>+             LoadOptionNumberUnassigned,
>              LoadOptionTypePlatformRecovery,
>              LOAD_OPTION_ACTIVE,
>              L"Default PlatformRecovery", @@ -843,9 +843,24 @@ BdsEntry (
>              0
>              );
>   ASSERT_EFI_ERROR (Status);
>-  EfiBootManagerLoadOptionToVariable (&LoadOption);
>+  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);
>+  }
>   EfiBootManagerFreeLoadOption (&LoadOption);
>   FreePool (FilePath);
>+  EfiBootManagerFreeLoadOptions (LoadOptions, LoadOptionCount);
>
>   //
>   // Report Status Code to indicate connecting drivers will happen
>--
>2.9.0.windows.1
>
>_______________________________________________
>edk2-devel mailing list
>edk2-devel@lists.01.org
>https://lists.01.org/mailman/listinfo/edk2-devel
>_______________________________________________
>edk2-devel mailing list
>edk2-devel@lists.01.org
>https://lists.01.org/mailman/listinfo/edk2-devel