Re: [edk2-devel] [PATCH 5/5] ShellPkg: add BootManager library to add UEFI Shell menu option

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

On 05/27/20 19:22, Ard Biesheuvel wrote:
> On 5/27/20 5:57 PM, Laszlo Ersek wrote:
>> On 05/26/20 18:13, Ard Biesheuvel wrote:
>>> Add a plug-in library for UiApp that creates a 'UEFI Shell' menu
>>> option at the root level which gives access to a form that allows
>>> the UEFI Shell to be launched.
>>>
>>> This gives the PlatformBootManagerLib implementation of the platform
>>> more flexibility in the way it handles boot options pointing to the
>>> UEFI Shell, which will typically be invoked with only the boot path
>>> connected on fast boots.
>>>
>>> This library may be incorporated to a platform build by adding a
>>> NULL resolution to the <LibraryClasses> section of the UiApp.inf
>>> {} block in the platform .DSC
>>>
>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
>>> ---
>>>   ShellPkg/Library/ShellBootManagerLib/ShellBootManagerLib.inf |  45
>>> ++++
>>>   ShellPkg/Library/ShellBootManagerLib/ShellBootManagerLib.h   |  44
>>> ++++
>>>   ShellPkg/Library/ShellBootManagerLib/ShellBootManagerLib.c   | 258
>>> ++++++++++++++++++++
>>>   ShellPkg/Library/ShellBootManagerLib/ShellBmStrings.uni      |  17 ++
>>>   ShellPkg/Library/ShellBootManagerLib/ShellBmVfr.Vfr          |  37 +++
>>>   5 files changed, 401 insertions(+)
>>
>> I've had to go back to the blurb and re-read this part, to understand
>> the goal of this patch:
>>
>>> - finally, add a plugin library for UiApp to expose the UEFI Shell
>>> via an
>>>    ordinary main menu option (this works around the fact that patch
>>> #3 will
>>>    result in the UEFI Shell disappearing from the Boot Manager list).
>>>    Entering the shell this way will resemble the old situation, given
>>> that
>>>    UiApp connects all devices and refreshes all boot options etc at
>>> launch.
>>
>> If I understand correctly:
>>
>> - patch #3 does two things: it clears LOAD_OPTION_ACTIVE (preventing the
>>    boot manager from auto-booting the shell), and sets LOAD_OPTION_HIDDEN
>>    (hiding the boot option from UiApp),
>>
>> - patch #5 undoes LOAD_OPTION_HIDDEN, in effect -- it makes sure that we
>>    still see the shell option "somewhere" in UiApp (not among the boot
>>    options, but at the root level)
>>
>> Can we:
>>
>> - drop patch#5, and
>>
>> - pass zero (0) as "Attributes" to PlatformRegisterFvBootOption() in
>>    patch#3, rather than LOAD_OPTION_HIDDEN?
>>
>> Because, per spec, Attributes=0 should prevent the auto-booting of the
>> shell *without* hiding the shell boot option from the menu.
>>
> 
> I feel slightly silly having gone through all the trouble of writing
> this patch. I tried playing with the ACTIVE and HIDDEN options, and
> couldn't get this to work. If I understand these quotes correctly, this
> is an error, and instead of working around this, we should apply the
> following patch to correct it:
> 
> --- a/MdeModulePkg/Library/BootManagerUiLib/BootManager.c
> +++ b/MdeModulePkg/Library/BootManagerUiLib/BootManager.c
> @@ -537,7 +537,7 @@ UpdateBootManager (
>      //
>      // Don't display the hidden/inactive boot option
>      //
> -    if (((BootOption[Index].Attributes & LOAD_OPTION_HIDDEN) != 0) ||
> ((BootOption[Index].Attributes & LOAD_OPTION_ACTIVE) == 0)) {
> +    if (((BootOption[Index].Attributes & LOAD_OPTION_HIDDEN) != 0)) {
>        continue;
>      }
> 
> 
> With this change applied, adding the shell option without the 'active'
> or 'hidden' flags works as expected: it appears in the boot manager
> menu, but is not booted automatically.

I enthusiastically agree that we should apply your above
BootManagerUiLib patch. I don't see why (per spec) the UI should hide a
boot option just because it is inactive.

Thanks!
Laszlo