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

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

[Neal Gompa]

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.
   //
-- 
2.41.0



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

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

[Laszlo Ersek]

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]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Laszlo Ersek]

On 10/26/23 18:19, Laszlo Ersek wrote:
> 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).

In fact,

EfiBootManagerProcessLoadOption() already checks
"LoadOption->OptionType" higher up, and if its LoadOptionTypeBoot, then
the function returns early, with EFI_UNSUPPORTED.

Therefore, IMO, you need to restrict the logic you are proposing to

  LoadOption->OptionType == LoadOptionTypePlatformRecovery

exclusively.

Laszlo

> 
> (
> 
> 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 (#110128): https://edk2.groups.io/g/devel/message/110128
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]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Neal Gompa]

On Thu, Oct 26, 2023 at 12:39 PM Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 10/26/23 18:19, Laszlo Ersek wrote:
> > 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).
>
> In fact,
>
> EfiBootManagerProcessLoadOption() already checks
> "LoadOption->OptionType" higher up, and if its LoadOptionTypeBoot, then
> the function returns early, with EFI_UNSUPPORTED.
>
> Therefore, IMO, you need to restrict the logic you are proposing to
>
>   LoadOption->OptionType == LoadOptionTypePlatformRecovery
>
> exclusively.
>

I've addressed this in the v2: https://edk2.groups.io/g/devel/message/110438


-- 
真実はいつも一つ！/ Always, there's only one truth!


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