Re: [edk2-devel] [PATCH 2/5] ArmPkg/PlatformBootManagerLib: fall back to the UiApp on boot failure

["Leif Lindholm" <leif@nuviainc.com>]

On Wed, May 27, 2020 at 17:34:13 +0200, Laszlo Ersek wrote:
> Hi Leif,
> 
> On 05/26/20 23:24, Leif Lindholm wrote:
> > On Tue, May 26, 2020 at 18:13:56 +0200, Ard Biesheuvel wrote:
> >> As a last resort, drop into the UiApp application when no active boot
> >> options could be started. Doing so will connect all devices, and so
> >> it will allow the user to enter the Boot Manager submenu and pick a
> >> network or removable disk option. With the right UiApp library added
> >> in, the UiApp also gives access to the UEFI Shell.
> >>
> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
> >> ---
> >>  ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c | 16 +++++++++++++++-
> >>  1 file changed, 15 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
> >> index 23c925bbdb9c..f91f7cd09ca1 100644
> >> --- a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
> >> +++ b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
> >> @@ -830,5 +830,19 @@ PlatformBootManagerUnableToBoot (
> >>    VOID
> >>    )
> >>  {
> >> -  return;
> >> +  EFI_STATUS                   Status;
> >> +  EFI_BOOT_MANAGER_LOAD_OPTION BootManagerMenu;
> >> +
> >> +  //
> >> +  // BootManagerMenu doesn't contain the correct information when return status
> >> +  // is EFI_NOT_FOUND.
> >> +  //
> >> +  Status = EfiBootManagerGetBootManagerMenu (&BootManagerMenu);
> >> +  if (EFI_ERROR (Status)) {
> >
> > Nitpick: comment explicitly mentions EFI_NOT_FOUND, but code checks
> > for any EFI_ERROR match. Since there are various other error codes
> > that could be returned, change the comment to "when return status is
> > not EFI_SUCCESS"?
> 
> I agree the (likely) original code (see commit 5f66615bb504,
> "OvmfPkg/PlatformBds: Implement PlatformBootManagerUnableToBoot",
> 2018-07-27) is a bit confusing.
> 
> Namely, both the comment and the subsequent error check are correct. The
> problem is that there is not much connection between the comment and the
> subsequent check. In other words, the comment does not *explain* the
> check.
> 
> The EfiBootManagerGetBootManagerMenu() function in
> "MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c" is documented like
> this:
> 
> > /**
> >   Return the boot option corresponding to the Boot Manager Menu.
> >   It may automatically create one if the boot option hasn't been created yet.
> >
> >   @param BootOption    Return the Boot Manager Menu.
> >
> >   @retval EFI_SUCCESS   The Boot Manager Menu is successfully returned.
> >   @retval EFI_NOT_FOUND The Boot Manager Menu cannot be found.
> >   @retval others        Return status of gRT->SetVariable (). BootOption still points
> >                         to the Boot Manager Menu even the Status is not EFI_SUCCESS
> >                         and EFI_NOT_FOUND.
> > **/
> 
> So the comment change you're proposing wouldn't be technically correct,
> I believe.
> 
> I think at best we should drop the comment altogether. If
> EfiBootManagerGetBootManagerMenu() fails due to EFI_NOT_FOUND, then
> "BootManagerMenu" is indeterminate, so we need to bail. If
> EfiBootManagerGetBootManagerMenu() fails with something else, then
> "BootManagerMenu" is set, but we *still* need to bail (as much as I
> understand from the EfiBootManagerGetBootManagerMenu() documentation).
> And that seems to mean we should simply not highlight EFI_NOT_FOUND with
> a comment.

Ah, I see what you're saying.
Yeah, that works.

/
    Leif