Re: [PATCH] MdeModulePkg/UefiBootManagerLib: handle ultimate boot failure

["Wang, Sunny (HPS SW)" <sunnywang@hpe.com>]

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