Re: [edk2-devel] [PATCH] MdeModulePkg/UefiBootManagerLib: Signal ReadyToBoot on platform recovery

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

On 10/26/23 15:53, Neal Gompa wrote:
> From: Neal Gompa <ngompa@fedoraproject.org>
> 
> Currently, the ReadyToBoot event is only signaled when a formal Boot
> Manager option is executed (in BmBoot.c -> EfiBootManagerBoot ()).
> 
> However, the introduction of Platform Recovery in UEFI 2.5 makes it
> necessary to signal ReadyToBoot when a Platform Recovery boot loader
> runs because otherwise it may lead to the execution of a boot loader
> that has similar requirements to a regular one that is not launched
> as a Boot Manager option.
> 
> This is especially critical to ensuring that the graphical console
> is actually usable during platform recovery, as some platforms do
> rely on the ConsolePrefDxe driver, which only performs console
> initialization after ReadyToBoot is triggered.
> 
> This patch fixes that behavior by calling EfiSignalEventReadyToBoot ()
> in EfiBootManagerProcessLoadOption (), which is the function that sets
> up the platform recovery boot process.
> 
> The expected behavior has been clarified in the UEFI 2.10 specification
> to explicitly indicate this behavior is required for correct operation.
> 
> This is a rebased version of the patch originally written by Pete Batard.
> 
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2831
> 
> Cc: Pete Batard <pete@akeo.ie>
> Cc: Daniel P. Berrangé <berrange@redhat.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Samer El-Haj-Mahmoud <samer.el-haj-mahmoud@arm.com>
> 
> Co-authored-by: Pete Batard <pete@akeo.ie>
> Signed-off-by: Neal Gompa <ngompa@fedoraproject.org>
> ---
>  MdeModulePkg/Library/UefiBootManagerLib/BmLoadOption.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmLoadOption.c b/MdeModulePkg/Library/UefiBootManagerLib/BmLoadOption.c
> index 2087f0b91d..31ed608817 100644
> --- a/MdeModulePkg/Library/UefiBootManagerLib/BmLoadOption.c
> +++ b/MdeModulePkg/Library/UefiBootManagerLib/BmLoadOption.c
> @@ -1416,6 +1416,15 @@ EfiBootManagerProcessLoadOption (
>      return EFI_SUCCESS;
>    }
>  
> +  //
> +  // Signal the EVT_SIGNAL_READY_TO_BOOT event when we are about to load and execute the boot option.
> +  //
> +  EfiSignalEventReadyToBoot ();
> +  //
> +  // Report Status Code to indicate ReadyToBoot was signalled
> +  //
> +  REPORT_STATUS_CODE (EFI_PROGRESS_CODE, (EFI_SOFTWARE_DXE_BS_DRIVER | EFI_SW_DXE_BS_PC_READY_TO_BOOT_EVENT));
> +
>    //
>    // Load and start the load option.
>    //

While the patch does the right thing for the latest UEFI spec language,
it does a bit too much.

The spec says (v2.10), under 7.1.2 "EFI_BOOT_SERVICES.CreateEventEx()":

-----------
EFI_EVENT_GROUP_READY_TO_BOOT

This event group is notified by the system right before notifying
EFI_EVENT_GROUP_AFTER_READY_TO_BOOT event group when the Boot Manager is
about to load and execute a boot option or a platform or OS recovery
option. The event group presents the last chance to modify device or
system configuration prior to passing control to a boot option.
-----------

NB "boot option", or "platform or OS recovery option".

However, EfiBootManagerProcessLoadOption() is also used for launching
Driver#### and SysPrep#### options.

EfiBootManagerProcessLoadOption() has two call sites:

(1) in ProcessLoadOptions() [MdeModulePkg/Universal/BdsDxe/BdsEntry.c]

(2) near the end of BdsEntry() [MdeModulePkg/Universal/BdsDxe/BdsEntry.c]

Call site (2) bears the comment "When platform recovery is not enabled,
still boot to platform default file path", so I figure signaling
ready-to-boot at that point is fine.

Call site (1) requires further investigation. Namely,
ProcessLoadOptions() itself is called from three locations, all inside
BdsEntry() [MdeModulePkg/Universal/BdsDxe/BdsEntry.c]:

(1.1) under comment "Execute Driver Options", for "LoadOptionTypeDriver"
type load options

(1.2) under comment "Execute SysPrep####", for "LoadOptionTypeSysPrep"
type load options

(1.3) for "LoadOptionTypePlatformRecovery" type load options.

I figure the intended extension is for case (1.3), but the patch, as
proposed, will also affect (1.1) and (1.2); that is, when Driver#### and
SysPrep#### options are processes. That's beyond what the spec says, in
my opinion.

Now, EfiBootManagerProcessLoadOption() takes a pointer to an
EFI_BOOT_MANAGER_LOAD_OPTION structure. This structure has a field
called OptionType (of type EFI_BOOT_MANAGER_LOAD_OPTION_TYPE). You might
want to restrict the signaling and the status code reporting to when
LoadOption->OptionType is either LoadOptionTypeBoot or
LoadOptionTypePlatformRecovery (i.e., exclude LoadOptionTypeDriver and
LoadOptionTypeSysPrep).

(

I've made an attempt to check the above-noted four call paths (i.e.,
(1.1) through (1.3), and (2)), and I *think* the OptionType field is
populated on all of them; thus, the check I recommend should be safe.

Call paths (1.1) through (1.3) call EfiBootManagerGetLoadOptions(),
which sets the OptionType field in each returned structure according to
the input parameter "LoadOptionType" (note the tricky call to
EfiBootManagerVariableToLoadOption()).

And (2) relies on "PlatformDefaultBootOption", which is created in
BdsEntry() with LoadOptionTypePlatformRecovery.

)

Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110126): https://edk2.groups.io/g/devel/message/110126
Mute This Topic: https://groups.io/mt/102200076/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-