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

["Ni, Ruiyu" <ruiyu.ni@intel.com>]

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