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

["Laszlo Ersek" <lersek@redhat.com>]

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.

Thanks
Laszlo