Re: [edk2-devel] [PATCH 5/5] ShellPkg: add BootManager library to add UEFI Shell menu option
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 section of the UiApp.inf
>>> {} block in the platform .DSC
>>>
>>> Signed-off-by: Ard Biesheuvel
>>> ---
>>> 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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#60409): https://edk2.groups.io/g/devel/message/60409
Mute This Topic: https://groups.io/mt/74481040/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 5/5] ShellPkg: add BootManager library to add UEFI Shell menu option
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 section of the UiApp.inf
{} block in the platform .DSC
Signed-off-by: Ard Biesheuvel
---
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.
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#60364): https://edk2.groups.io/g/devel/message/60364
Mute This Topic: https://groups.io/mt/74481040/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 5/5] ShellPkg: add BootManager library to add UEFI Shell menu option
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 section of the UiApp.inf
> {} block in the platform .DSC
>
> Signed-off-by: Ard Biesheuvel
> ---
> 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.
Thanks,
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#60361): https://edk2.groups.io/g/devel/message/60361
Mute This Topic: https://groups.io/mt/74481040/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
