public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: Laszlo Ersek <lersek@redhat.com>
To: "Wang, Sunny (HPS SW)" <sunnywang@hpe.com>,
	Ruiyu Ni <ruiyu.ni@intel.com>,
	"edk2-devel@lists.01.org" <edk2-devel@lists.01.org>
Cc: Michael Turner <Michael.Turner@microsoft.com>
Subject: Re: [PATCH] MdeModulePkg/UefiBootManagerLib: handle ultimate boot failure
Date: Thu, 28 Jun 2018 17:04:05 +0200	[thread overview]
Message-ID: <01641a9a-d151-283f-28e0-5db34527b558@redhat.com> (raw)
In-Reply-To: <CS1PR8401MB0933983261A58461805D6BF4A84F0@CS1PR8401MB0933.NAMPRD84.PROD.OUTLOOK.COM>

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


  reply	other threads:[~2018-06-28 15:04 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
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 [this message]
2018-06-29  4:16     ` Wang, Sunny (HPS SW)
2018-06-29  7:25     ` Ni, Ruiyu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=01641a9a-d151-283f-28e0-5db34527b558@redhat.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox