public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [PATCH] MdeModulePkg/UefiBootManagerLib: handle ultimate boot failure
@ 2018-06-28  7:40 Ruiyu Ni
  2018-06-28  9:02 ` Wang, Sunny (HPS SW)
  0 siblings, 1 reply; 5+ messages in thread
From: Ruiyu Ni @ 2018-06-28  7:40 UTC (permalink / raw)
  To: edk2-devel; +Cc: Sean Brogan, Michael Turner, Laszlo Ersek

When no boot option could be launched including platform recovery
options and options pointing to applications built into firmware
volumes, a platform callback registered through
EfiBootManagerRegisterUnableBootHandler() is called.

If there is no platform callback registered, default behavior is
to print an error message to the screen and wait for user input.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Sean Brogan <sean.brogan@microsoft.com>
Cc: Michael Turner <Michael.Turner@microsoft.com>
Cc: Laszlo Ersek <lersek@redhat.com>
---
 MdeModulePkg/Include/Library/UefiBootManagerLib.h | 48 +++++++++++++++++++++++
 MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c  | 41 +++++++++++++++++++
 MdeModulePkg/Universal/BdsDxe/BdsEntry.c          | 44 +++++++++++----------
 3 files changed, 113 insertions(+), 20 deletions(-)

diff --git a/MdeModulePkg/Include/Library/UefiBootManagerLib.h b/MdeModulePkg/Include/Library/UefiBootManagerLib.h
index bfc0cb86f8..0035c41082 100644
--- a/MdeModulePkg/Include/Library/UefiBootManagerLib.h
+++ b/MdeModulePkg/Include/Library/UefiBootManagerLib.h
@@ -800,4 +800,52 @@ EFIAPI
 EfiBootManagerDispatchDeferredImages (
   VOID
   );
+
+/**
+  The function is called when no boot option could be launched,
+  including platform recovery options and options pointing to applications
+  built into firmware volumes.
+
+  The platform may register this function to inform the user about the above fact.
+  If this function returns, BDS core attempts to enter an infinite loop of pulling
+  up the Boot Manager Menu (if the platform includes the Boot Manager Menu).
+  If the Boot Manager Menu is unavailable, BDS will hang.
+**/
+typedef
+VOID
+(EFIAPI *EFI_BOOT_MANAGER_UNABLE_BOOT) (
+  VOID
+  );
+
+/**
+  Register the callback function when no boot option could be launched,
+  including platform recovery options and options pointing to applications
+  built into firmware volumes.
+
+  @param Handler  The callback function.
+**/
+VOID
+EFIAPI
+EfiBootManagerRegisterUnableBootHandler (
+  EFI_BOOT_MANAGER_UNABLE_BOOT  Handler
+  );
+
+/**
+  The function is called when no boot option could be launched,
+  including platform recovery options and options pointing to applications
+  built into firmware volumes.
+
+  If this function returns, BDS core attempts to enter an infinite loop of pulling
+  up the Boot Manager Menu (if the platform includes the Boot Manager Menu).
+  If the Boot Manager Menu is unavailable, BDS will hang.
+
+  @retval EFI_SUCCESS     The unable-boot callback is called successfully.
+  @retval EFI_UNSUPPORTED There is no unable-boot callback registered.
+**/
+EFI_STATUS
+EFIAPI
+EfiBootManagerUnableBoot (
+  VOID
+  );
+
 #endif
diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
index 6a23477eb8..122068267d 100644
--- a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
+++ b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
@@ -19,6 +19,7 @@ EFI_RAM_DISK_PROTOCOL                        *mRamDisk                  = NULL;
 
 EFI_BOOT_MANAGER_REFRESH_LEGACY_BOOT_OPTION  mBmRefreshLegacyBootOption = NULL;
 EFI_BOOT_MANAGER_LEGACY_BOOT                 mBmLegacyBoot              = NULL;
+EFI_BOOT_MANAGER_UNABLE_BOOT                 mBmUnableBoot              = NULL;
 
 ///
 /// This GUID is used for an EFI Variable that stores the front device pathes
@@ -2461,3 +2462,43 @@ EfiBootManagerGetBootManagerMenu (
   }
 }
 
+/**
+  Register the callback function when no boot option could be launched,
+  including platform recovery options and options pointing to applications
+  built into firmware volumes.
+
+  @param Handler  The callback function.
+**/
+VOID
+EFIAPI
+EfiBootManagerRegisterUnableBootHandler (
+  EFI_BOOT_MANAGER_UNABLE_BOOT  Handler
+  )
+{
+  mBmUnableBoot = Handler;
+}
+
+/**
+  The function is called when no boot option could be launched,
+  including platform recovery options and options pointing to applications
+  built into firmware volumes.
+
+  If this function returns, BDS core attempts to enter an infinite loop of pulling
+  up the Boot Manager Menu (if the platform includes the Boot Manager Menu).
+  If the Boot Manager Menu is unavailable, BDS will hang.
+
+  @retval EFI_SUCCESS     The unable-boot callback is called successfully.
+  @retval EFI_UNSUPPORTED There is no unable-boot callback registered.
+**/
+EFI_STATUS
+EFIAPI
+EfiBootManagerUnableBoot (
+  VOID
+  )
+{
+  if (mBmUnableBoot != NULL) {
+    mBmUnableBoot ();
+    return EFI_SUCCESS;
+  }
+  return EFI_UNSUPPORTED;
+}
diff --git a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
index 49e403e181..0cb9b04dfb 100644
--- a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
+++ b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
@@ -649,32 +649,36 @@ BdsBootManagerMenuLoop (
   IN EFI_BOOT_MANAGER_LOAD_OPTION *BootManagerMenu
   )
 {
+  EFI_STATUS    Status;
   EFI_INPUT_KEY Key;
 
-  //
-  // Normally BdsDxe does not print anything to the system console, but this is
-  // a last resort -- the end-user will likely not see any DEBUG messages
-  // logged in this situation.
-  //
-  // AsciiPrint() will NULL-check gST->ConOut internally. We check gST->ConIn
-  // here to see if it makes sense to request and wait for a keypress.
-  //
-  if (gST->ConIn != NULL) {
-    AsciiPrint (
-      "%a: No bootable option or device was found.\n"
-      "%a: Press any key to enter the Boot Manager Menu.\n",
-      gEfiCallerBaseName,
-      gEfiCallerBaseName
-      );
-    BdsWaitForSingleEvent (gST->ConIn->WaitForKey, 0);
-
+  Status = EfiBootManagerUnableBoot ();
+  if (EFI_ERROR (Status)) {
+    //
+    // If platform doesn't register any unable-boot callback, this is
+    // a last resort -- the end-user will likely not see any DEBUG messages
+    // logged in this situation.
     //
-    // Drain any queued keys.
+    // AsciiPrint() will NULL-check gST->ConOut internally. We check gST->ConIn
+    // here to see if it makes sense to request and wait for a keypress.
     //
-    while (!EFI_ERROR (gST->ConIn->ReadKeyStroke (gST->ConIn, &Key))) {
+    if (gST->ConIn != NULL) {
+      AsciiPrint (
+        "%a: No bootable option or device was found.\n"
+        "%a: Press any key to enter the Boot Manager Menu.\n",
+        gEfiCallerBaseName,
+        gEfiCallerBaseName
+        );
+      BdsWaitForSingleEvent (gST->ConIn->WaitForKey, 0);
+
       //
-      // just throw away Key
+      // Drain any queued keys.
       //
+      while (!EFI_ERROR (gST->ConIn->ReadKeyStroke (gST->ConIn, &Key))) {
+        //
+        // just throw away Key
+        //
+      }
     }
   }
 
-- 
2.16.1.windows.1



^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] MdeModulePkg/UefiBootManagerLib: handle ultimate boot failure
  2018-06-28  7:40 [PATCH] MdeModulePkg/UefiBootManagerLib: handle ultimate boot failure Ruiyu Ni
@ 2018-06-28  9:02 ` Wang, Sunny (HPS SW)
  2018-06-28 15:04   ` Laszlo Ersek
  0 siblings, 1 reply; 5+ messages in thread
From: Wang, Sunny (HPS SW) @ 2018-06-28  9:02 UTC (permalink / raw)
  To: Ruiyu Ni, edk2-devel@lists.01.org
  Cc: Laszlo Ersek, Michael Turner, Wang, Sunny (HPS SW)

Hi Ray,

Does the ultimate boot failure include the case where the system doesn't have "BootManagerMenu" application?  If so, I think we should move EfiBootManagerUnableBoot() call out of BdsBootManagerMenuLoop(). The BdsBootManagerMenuLoop() is only called when system has BootManagerMenu application. Therefore, if the system doesn't have BootManagerMenu application, the EfiBootManagerUnableBoot() will have no chance to be called for handling the ultimate boot failure case.  
	
Regards,
Sunny Wang

-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ruiyu Ni
Sent: Thursday, June 28, 2018 3:40 PM
To: edk2-devel@lists.01.org
Cc: Laszlo Ersek <lersek@redhat.com>; Michael Turner <Michael.Turner@microsoft.com>
Subject: [edk2] [PATCH] MdeModulePkg/UefiBootManagerLib: handle ultimate boot failure

When no boot option could be launched including platform recovery options and options pointing to applications built into firmware volumes, a platform callback registered through
EfiBootManagerRegisterUnableBootHandler() is called.

If there is no platform callback registered, default behavior is to print an error message to the screen and wait for user input.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Sean Brogan <sean.brogan@microsoft.com>
Cc: Michael Turner <Michael.Turner@microsoft.com>
Cc: Laszlo Ersek <lersek@redhat.com>
---
 MdeModulePkg/Include/Library/UefiBootManagerLib.h | 48 +++++++++++++++++++++++  MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c  | 41 +++++++++++++++++++
 MdeModulePkg/Universal/BdsDxe/BdsEntry.c          | 44 +++++++++++----------
 3 files changed, 113 insertions(+), 20 deletions(-)

diff --git a/MdeModulePkg/Include/Library/UefiBootManagerLib.h b/MdeModulePkg/Include/Library/UefiBootManagerLib.h
index bfc0cb86f8..0035c41082 100644
--- a/MdeModulePkg/Include/Library/UefiBootManagerLib.h
+++ b/MdeModulePkg/Include/Library/UefiBootManagerLib.h
@@ -800,4 +800,52 @@ EFIAPI
 EfiBootManagerDispatchDeferredImages (
   VOID
   );
+
+/**
+  The function is called when no boot option could be launched,
+  including platform recovery options and options pointing to 
+applications
+  built into firmware volumes.
+
+  The platform may register this function to inform the user about the above fact.
+  If this function returns, BDS core attempts to enter an infinite loop 
+of pulling
+  up the Boot Manager Menu (if the platform includes the Boot Manager Menu).
+  If the Boot Manager Menu is unavailable, BDS will hang.
+**/
+typedef
+VOID
+(EFIAPI *EFI_BOOT_MANAGER_UNABLE_BOOT) (
+  VOID
+  );
+
+/**
+  Register the callback function when no boot option could be launched,
+  including platform recovery options and options pointing to 
+applications
+  built into firmware volumes.
+
+  @param Handler  The callback function.
+**/
+VOID
+EFIAPI
+EfiBootManagerRegisterUnableBootHandler (
+  EFI_BOOT_MANAGER_UNABLE_BOOT  Handler
+  );
+
+/**
+  The function is called when no boot option could be launched,
+  including platform recovery options and options pointing to 
+applications
+  built into firmware volumes.
+
+  If this function returns, BDS core attempts to enter an infinite loop 
+ of pulling  up the Boot Manager Menu (if the platform includes the Boot Manager Menu).
+  If the Boot Manager Menu is unavailable, BDS will hang.
+
+  @retval EFI_SUCCESS     The unable-boot callback is called successfully.
+  @retval EFI_UNSUPPORTED There is no unable-boot callback registered.
+**/
+EFI_STATUS
+EFIAPI
+EfiBootManagerUnableBoot (
+  VOID
+  );
+
 #endif
diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
index 6a23477eb8..122068267d 100644
--- a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
+++ b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
@@ -19,6 +19,7 @@ EFI_RAM_DISK_PROTOCOL                        *mRamDisk                  = NULL;
 
 EFI_BOOT_MANAGER_REFRESH_LEGACY_BOOT_OPTION  mBmRefreshLegacyBootOption = NULL;
 EFI_BOOT_MANAGER_LEGACY_BOOT                 mBmLegacyBoot              = NULL;
+EFI_BOOT_MANAGER_UNABLE_BOOT                 mBmUnableBoot              = NULL;
 
 ///
 /// This GUID is used for an EFI Variable that stores the front device pathes @@ -2461,3 +2462,43 @@ EfiBootManagerGetBootManagerMenu (
   }
 }
 
+/**
+  Register the callback function when no boot option could be launched,
+  including platform recovery options and options pointing to 
+applications
+  built into firmware volumes.
+
+  @param Handler  The callback function.
+**/
+VOID
+EFIAPI
+EfiBootManagerRegisterUnableBootHandler (
+  EFI_BOOT_MANAGER_UNABLE_BOOT  Handler
+  )
+{
+  mBmUnableBoot = Handler;
+}
+
+/**
+  The function is called when no boot option could be launched,
+  including platform recovery options and options pointing to 
+applications
+  built into firmware volumes.
+
+  If this function returns, BDS core attempts to enter an infinite loop 
+ of pulling  up the Boot Manager Menu (if the platform includes the Boot Manager Menu).
+  If the Boot Manager Menu is unavailable, BDS will hang.
+
+  @retval EFI_SUCCESS     The unable-boot callback is called successfully.
+  @retval EFI_UNSUPPORTED There is no unable-boot callback registered.
+**/
+EFI_STATUS
+EFIAPI
+EfiBootManagerUnableBoot (
+  VOID
+  )
+{
+  if (mBmUnableBoot != NULL) {
+    mBmUnableBoot ();
+    return EFI_SUCCESS;
+  }
+  return EFI_UNSUPPORTED;
+}
diff --git a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
index 49e403e181..0cb9b04dfb 100644
--- a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
+++ b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
@@ -649,32 +649,36 @@ BdsBootManagerMenuLoop (
   IN EFI_BOOT_MANAGER_LOAD_OPTION *BootManagerMenu
   )
 {
+  EFI_STATUS    Status;
   EFI_INPUT_KEY Key;
 
-  //
-  // Normally BdsDxe does not print anything to the system console, but this is
-  // a last resort -- the end-user will likely not see any DEBUG messages
-  // logged in this situation.
-  //
-  // AsciiPrint() will NULL-check gST->ConOut internally. We check gST->ConIn
-  // here to see if it makes sense to request and wait for a keypress.
-  //
-  if (gST->ConIn != NULL) {
-    AsciiPrint (
-      "%a: No bootable option or device was found.\n"
-      "%a: Press any key to enter the Boot Manager Menu.\n",
-      gEfiCallerBaseName,
-      gEfiCallerBaseName
-      );
-    BdsWaitForSingleEvent (gST->ConIn->WaitForKey, 0);
-
+  Status = EfiBootManagerUnableBoot ();  if (EFI_ERROR (Status)) {
+    //
+    // If platform doesn't register any unable-boot callback, this is
+    // a last resort -- the end-user will likely not see any DEBUG messages
+    // logged in this situation.
     //
-    // Drain any queued keys.
+    // AsciiPrint() will NULL-check gST->ConOut internally. We check gST->ConIn
+    // here to see if it makes sense to request and wait for a keypress.
     //
-    while (!EFI_ERROR (gST->ConIn->ReadKeyStroke (gST->ConIn, &Key))) {
+    if (gST->ConIn != NULL) {
+      AsciiPrint (
+        "%a: No bootable option or device was found.\n"
+        "%a: Press any key to enter the Boot Manager Menu.\n",
+        gEfiCallerBaseName,
+        gEfiCallerBaseName
+        );
+      BdsWaitForSingleEvent (gST->ConIn->WaitForKey, 0);
+
       //
-      // just throw away Key
+      // Drain any queued keys.
       //
+      while (!EFI_ERROR (gST->ConIn->ReadKeyStroke (gST->ConIn, &Key))) {
+        //
+        // just throw away Key
+        //
+      }
     }
   }
 
--
2.16.1.windows.1

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


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] MdeModulePkg/UefiBootManagerLib: handle ultimate boot failure
  2018-06-28  9:02 ` Wang, Sunny (HPS SW)
@ 2018-06-28 15:04   ` Laszlo Ersek
  2018-06-29  4:16     ` Wang, Sunny (HPS SW)
  2018-06-29  7:25     ` Ni, Ruiyu
  0 siblings, 2 replies; 5+ messages in thread
From: Laszlo Ersek @ 2018-06-28 15:04 UTC (permalink / raw)
  To: Wang, Sunny (HPS SW), Ruiyu Ni, edk2-devel@lists.01.org; +Cc: Michael Turner

On 06/28/18 11:02, Wang, Sunny (HPS SW) wrote:
> Hi Ray,
>
> Does the ultimate boot failure include the case where the system
> doesn't have "BootManagerMenu" application?  If so, I think we should
> move EfiBootManagerUnableBoot() call out of BdsBootManagerMenuLoop().
> The BdsBootManagerMenuLoop() is only called when system has
> BootManagerMenu application. Therefore, if the system doesn't have
> BootManagerMenu application, the EfiBootManagerUnableBoot() will have
> no chance to be called for handling the ultimate boot failure case.

Personally I'd be very happy with the current version of the patch as
well, but I agree Sunny's request makes sense. How about this, for
BdsDxe:

> diff --git a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
> index 3191a986304b..cb4196a56c87 100644
> --- a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
> +++ b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
> @@ -1088,11 +1088,26 @@ BdsEntry (
>      EfiBootManagerFreeLoadOptions (LoadOptions, LoadOptionCount);
>    }
>
> -  //
> -  // If BootManagerMenu is available, fall back to it indefinitely.
> -  //
> -  if (BootManagerMenuStatus != EFI_NOT_FOUND) {
> -    BdsBootManagerMenuLoop (&BootManagerMenu);
> +  if (BootManagerMenuStatus == EFI_NOT_FOUND) {
> +    //
> +    // Inform the platform that we're unable to boot, and that there's no
> +    // BootManagerMenu.
> +    //
> +    EfiBootManagerUnableToBoot (NULL);
> +  } else {
> +    //
> +    // Inform the platform that we're unable to boot. The platform may enter
> +    // BootManagerMenu with the public EfiBootManagerBoot() interface, if so
> +    // desired.
> +    //
> +    Status = EfiBootManagerUnableToBoot (&BootManagerMenu);
> +    if (EFI_ERROR (Status)) {
> +      //
> +      // The platform didn't register a callback; fall back to BootManagerMenu
> +      // internally, indefinitely.
> +      //
> +      BdsBootManagerMenuLoop (&BootManagerMenu);
> +    }
>    }
>
>    DEBUG ((EFI_D_ERROR, "[Bds] Unable to boot!\n"));

Note that this requires changing the declaration of
EfiBootManagerUnableBoot(), so that it takes the parameter

  IN EFI_BOOT_MANAGER_LOAD_OPTION *BootManagerMenu OPTIONAL

The structure EFI_BOOT_MANAGER_LOAD_OPTION is from
"MdeModulePkg/Include/Library/UefiBootManagerLib.h", so it is OK to
expose to platforms.

Just an idea, of course.

--*--

Anyway, for a v2, I have some superficial reuqests / questions for Ray:

* Please replace "UNABLE_BOOT" with "UNABLE_TO_BOOT". Same for
  "UnableBoot" and "UnableToBoot".

  (Compare: READY_TO_BOOT, ReadyToBoot.)

  Note that this affects the commit message too.

* Should we split the BdsDxe modification to a separate patch?

* Can you please reference
  <https://bugzilla.tianocore.org/show_bug.cgi?id=982> in the commit
  message?

Thank you very much Ray for doing this!
Laszlo


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] MdeModulePkg/UefiBootManagerLib: handle ultimate boot failure
  2018-06-28 15:04   ` Laszlo Ersek
@ 2018-06-29  4:16     ` Wang, Sunny (HPS SW)
  2018-06-29  7:25     ` Ni, Ruiyu
  1 sibling, 0 replies; 5+ messages in thread
From: Wang, Sunny (HPS SW) @ 2018-06-29  4:16 UTC (permalink / raw)
  To: Laszlo Ersek, Ruiyu Ni, edk2-devel@lists.01.org
  Cc: Michael Turner, Wang, Sunny (HPS SW)

Thanks, Laszlo. 

Hi Ray, 
After looking into  <https://bugzilla.tianocore.org/show_bug.cgi?id=982>, I just realized that I think too deep about the purpose of adding the EfiBootManagerUnableToBoot (). 
EfiBootManagerUnableToBoot () is added for solving the problem that user don't want to see the messages " No bootable option..." printed by BDS core code. The design is based on the concept that "print in a platformbds lib, loop in core code", so your current patch already solved that problem. Therefore, if there is no need to deal with the case where system doesn't have BootManagerMenu application, I'm totally fine with your current patch. 
    
Regards,
Sunny Wang

-----Original Message-----
From: Laszlo Ersek [mailto:lersek@redhat.com] 
Sent: Thursday, June 28, 2018 11:04 PM
To: Wang, Sunny (HPS SW) <sunnywang@hpe.com>; Ruiyu Ni <ruiyu.ni@intel.com>; edk2-devel@lists.01.org
Cc: Michael Turner <Michael.Turner@microsoft.com>
Subject: Re: [edk2] [PATCH] MdeModulePkg/UefiBootManagerLib: handle ultimate boot failure
Importance: High

On 06/28/18 11:02, Wang, Sunny (HPS SW) wrote:
> Hi Ray,
>
> Does the ultimate boot failure include the case where the system 
> doesn't have "BootManagerMenu" application?  If so, I think we should 
> move EfiBootManagerUnableBoot() call out of BdsBootManagerMenuLoop().
> The BdsBootManagerMenuLoop() is only called when system has 
> BootManagerMenu application. Therefore, if the system doesn't have 
> BootManagerMenu application, the EfiBootManagerUnableBoot() will have 
> no chance to be called for handling the ultimate boot failure case.

Personally I'd be very happy with the current version of the patch as well, but I agree Sunny's request makes sense. How about this, for
BdsDxe:

> diff --git a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c 
> b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
> index 3191a986304b..cb4196a56c87 100644
> --- a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
> +++ b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
> @@ -1088,11 +1088,26 @@ BdsEntry (
>      EfiBootManagerFreeLoadOptions (LoadOptions, LoadOptionCount);
>    }
>
> -  //
> -  // If BootManagerMenu is available, fall back to it indefinitely.
> -  //
> -  if (BootManagerMenuStatus != EFI_NOT_FOUND) {
> -    BdsBootManagerMenuLoop (&BootManagerMenu);
> +  if (BootManagerMenuStatus == EFI_NOT_FOUND) {
> +    //
> +    // Inform the platform that we're unable to boot, and that there's no
> +    // BootManagerMenu.
> +    //
> +    EfiBootManagerUnableToBoot (NULL);  } else {
> +    //
> +    // Inform the platform that we're unable to boot. The platform may enter
> +    // BootManagerMenu with the public EfiBootManagerBoot() interface, if so
> +    // desired.
> +    //
> +    Status = EfiBootManagerUnableToBoot (&BootManagerMenu);
> +    if (EFI_ERROR (Status)) {
> +      //
> +      // The platform didn't register a callback; fall back to BootManagerMenu
> +      // internally, indefinitely.
> +      //
> +      BdsBootManagerMenuLoop (&BootManagerMenu);
> +    }
>    }
>
>    DEBUG ((EFI_D_ERROR, "[Bds] Unable to boot!\n"));

Note that this requires changing the declaration of EfiBootManagerUnableBoot(), so that it takes the parameter

  IN EFI_BOOT_MANAGER_LOAD_OPTION *BootManagerMenu OPTIONAL

The structure EFI_BOOT_MANAGER_LOAD_OPTION is from "MdeModulePkg/Include/Library/UefiBootManagerLib.h", so it is OK to expose to platforms.

Just an idea, of course.

--*--

Anyway, for a v2, I have some superficial reuqests / questions for Ray:

* Please replace "UNABLE_BOOT" with "UNABLE_TO_BOOT". Same for
  "UnableBoot" and "UnableToBoot".

  (Compare: READY_TO_BOOT, ReadyToBoot.)

  Note that this affects the commit message too.

* Should we split the BdsDxe modification to a separate patch?

* Can you please reference
  <https://bugzilla.tianocore.org/show_bug.cgi?id=982> in the commit
  message?

Thank you very much Ray for doing this!
Laszlo

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] MdeModulePkg/UefiBootManagerLib: handle ultimate boot failure
  2018-06-28 15:04   ` Laszlo Ersek
  2018-06-29  4:16     ` Wang, Sunny (HPS SW)
@ 2018-06-29  7:25     ` Ni, Ruiyu
  1 sibling, 0 replies; 5+ messages in thread
From: Ni, Ruiyu @ 2018-06-29  7:25 UTC (permalink / raw)
  To: Laszlo Ersek, Wang, Sunny (HPS SW), edk2-devel@lists.01.org
  Cc: Michael Turner

On 6/28/2018 11:04 PM, Laszlo Ersek wrote:
> Personally I'd be very happy with the current version of the patch as
> well, but I agree Sunny's request makes sense. How about this, for
> BdsDxe:
> 
>> diff --git a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
>> index 3191a986304b..cb4196a56c87 100644
>> --- a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
>> +++ b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c
>> @@ -1088,11 +1088,26 @@ BdsEntry (
>>       EfiBootManagerFreeLoadOptions (LoadOptions, LoadOptionCount);
>>     }
>>
>> -  //
>> -  // If BootManagerMenu is available, fall back to it indefinitely.
>> -  //
>> -  if (BootManagerMenuStatus != EFI_NOT_FOUND) {
>> -    BdsBootManagerMenuLoop (&BootManagerMenu);
>> +  if (BootManagerMenuStatus == EFI_NOT_FOUND) {
>> +    //
>> +    // Inform the platform that we're unable to boot, and that there's no
>> +    // BootManagerMenu.
>> +    //
>> +    EfiBootManagerUnableToBoot (NULL);
>> +  } else {
>> +    //
>> +    // Inform the platform that we're unable to boot. The platform may enter
>> +    // BootManagerMenu with the public EfiBootManagerBoot() interface, if so
>> +    // desired.
>> +    //
>> +    Status = EfiBootManagerUnableToBoot (&BootManagerMenu);
>> +    if (EFI_ERROR (Status)) {
>> +      //
>> +      // The platform didn't register a callback; fall back to BootManagerMenu
>> +      // internally, indefinitely.
>> +      //
>> +      BdsBootManagerMenuLoop (&BootManagerMenu);
>> +    }
>>     }
>>
>>     DEBUG ((EFI_D_ERROR, "[Bds] Unable to boot!\n"));
> Note that this requires changing the declaration of
> EfiBootManagerUnableBoot(), so that it takes the parameter
> 
>    IN EFI_BOOT_MANAGER_LOAD_OPTION *BootManagerMenu OPTIONAL
> 
> The structure EFI_BOOT_MANAGER_LOAD_OPTION is from
> "MdeModulePkg/Include/Library/UefiBootManagerLib.h", so it is OK to
> expose to platforms.
> 
> Just an idea, of course.

Platform can use EfiBootManagerGetBootManagerMenu() to get the boot 
manager menu. So there is no need to add an extra parameter for 
EfiBootManagerUnableToBoot().
I agree with your idea to only pop up boot manager menu as the default 
behavior.

> 
> --*--
> 
> Anyway, for a v2, I have some superficial reuqests / questions for Ray:
> 
> * Please replace "UNABLE_BOOT" with "UNABLE_TO_BOOT". Same for
>    "UnableBoot" and "UnableToBoot".
> 
>    (Compare: READY_TO_BOOT, ReadyToBoot.)
> 
>    Note that this affects the commit message too.
OK.
> 
> * Should we split the BdsDxe modification to a separate patch?
OK.
> 
> * Can you please reference
>    <https://bugzilla.tianocore.org/show_bug.cgi?id=982>  in the commit
>    message?
OK.
> 
> Thank you very much Ray for doing this!
> Laszlo


-- 
Thanks,
Ray


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2018-06-29  7:25 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-06-28  7:40 [PATCH] MdeModulePkg/UefiBootManagerLib: handle ultimate boot failure Ruiyu Ni
2018-06-28  9:02 ` Wang, Sunny (HPS SW)
2018-06-28 15:04   ` Laszlo Ersek
2018-06-29  4:16     ` Wang, Sunny (HPS SW)
2018-06-29  7:25     ` Ni, Ruiyu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox