From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mx.groups.io with SMTP id smtpd.web11.2041.1590600126596574896 for ; Wed, 27 May 2020 10:22:06 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: arm.com, ip: 217.140.110.172, mailfrom: ard.biesheuvel@arm.com) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 171E231B; Wed, 27 May 2020 10:22:05 -0700 (PDT) Received: from [192.168.1.81] (unknown [10.37.8.249]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 3E04E3F305; Wed, 27 May 2020 10:22:04 -0700 (PDT) Subject: Re: [edk2-devel] [PATCH 5/5] ShellPkg: add BootManager library to add UEFI Shell menu option To: Laszlo Ersek , devel@edk2.groups.io Cc: jon@solid-run.com References: <20200526161359.4810-1-ard.biesheuvel@arm.com> <20200526161359.4810-6-ard.biesheuvel@arm.com> From: "Ard Biesheuvel" Message-ID: <9475af7a-f6f8-cbaf-5a31-fe2e42d3d522@arm.com> Date: Wed, 27 May 2020 19:22:01 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.7.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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.