Thanks for checking my comments, Pete. 

> Or is the "one more" the issue, meaning that it would get signaled more than 
> once?
[Sunny] Yeah, it would get signaled more than once if the PlatformRecovery 
option (a UEFI application) calls EfiBootManagerBoot() to launch the recovered 
boot option inside of the application.      

> I don't mind trying an alternative approach, but I don't understand how what 
> you describe would help. Can you please be more specific about what you have 
> in mind?
[Sunny] Sure. I added more information below. If it is still not clear enough, 
feel free to let me know.
     1. Create a UEFI application with the code to signal ReadyToBoot and pick 
/efi/boot/bootaa64.efi from either SD or USB and run it.
     2. Then, call EfiBootManagerInitializeLoadOption like the following in a 
DXE driver or other places before "Default PlatformRecovery" registration:  
  Status = EfiBootManagerInitializeLoadOption (
             &LoadOption,
             0,                                                                 
            -> 0 is the OptionNumber to let application be load before " 
Default PlatformRecovery" option
             LoadOptionTypePlatformRecovery,
             LOAD_OPTION_ACTIVE,
             L"Application for recovering boot options",
             FilePath,                                                          
      -> FilePath is the Application's device path,
             NULL,
             0
             );


> My reasoning is that, if PlatformRecovery#### can execute a regular 
> bootloader like /efi/boot/boot####.efi from installation media, then it 
> should go through the same kind of initialization that happens for a regular 
> boot option, and that should include signaling the ReadyToBoot event. 
[Sunny] Thanks for clarifying this, and Sorry about that I missed your cover 
letter for this patch.  I was just thinking that we may not really need to make 
this behavior change in both EDK II code and UEFI specification for solving the 
problem specific to the case that OS is loaded by "Default PlatformRecovery" 
option, and I'm also not sure if it is worth making this change to affect some 
of the system or BIOS vendors who have implemented their PlatformRecovery 
option. If the alternative approach I mentioned works for you, I think that 
would be an easier solution.

Regards,
Sunny Wang

-----Original Message-----
From: Pete Batard [mailto:p...@akeo.ie] 
Sent: Wednesday, June 17, 2020 6:59 PM
To: Wang, Sunny (HPS SW) <sunnyw...@hpe.com>; devel@edk2.groups.io
Cc: zhichao....@intel.com; ray...@intel.com; ard.biesheu...@arm.com; 
l...@nuviainc.com
Subject: Re: [edk2-devel] [edk2][PATCH 1/1] MdeModulePkg/UefiBootManagerLib: 
Signal ReadyToBoot on platform recovery

Hi Sunny, thanks for looking into this.

On 2020.06.17 09:16, Wang, Sunny (HPS SW) wrote:
> Hi Pete.
> 
> Since the EfiBootManagerProcessLoadOption is called by ProcessLoadOptions as 
> well, your change would also cause some unexpected behavior like:
> 1. Signal one more ReadyToBoot for the PlatformRecovery option which is an 
> application that calls EfiBootManagerBoot() to launch its recovered boot 
> option.

I'm not sure I understand how this part is unwanted.

The point of this patch is to ensure that ReadyToBoot is signalled for the 
PlatformRecovery option, so isn't what you describe above exactly what we want?

Or is the "one more" the issue, meaning that it would get signalled more than 
once?


> 2. Signal ReadyToBoot for SysPrep#### or Driver#### that is not really a 
> "boot" option.

Yes, I've been wondering about that, because BdsEntry.c's ProcessLoadOptions(), 
which calls EfiBootManagerProcessLoadOption(),
mentions that it will load will load and start every Driver####, SysPrep#### or 
PlatformRecovery####. But the comment about the while() loop in 
EfiBootManagerProcessLoadOption() only mentions PlatformRecovery####.

If needed, I guess we could amend the patch to detect the type of option and 
only signal ReadyToBoot for PlatformRecovery####.

> To solve your problem, creating a PlatformRecovery option with the smallest 
> option number and using it instead of default one (with short-form File Path 
> Media Device Path) looks like a simpler solution.

I don't mind trying an alternative approach, but I don't understand how what 
you describe would help. Can you please be more specific about what you have in 
mind?

Our main issue here is that we must have ReadyToBoot signalled so that the 
ReadyToBoot() function callback from EmbeddedPkg/Drivers/ConsolePrefDxe gets 
executed in order for the boot loader invoked from PlatformRecovery####  to use 
a properly initialized graphical console. So I'm not sure I quite get how 
switching from one PlatformRecovery#### option to another would improve things.

If it helps, here is what we currently default to, in terms of boot options, on 
a Raspberry Pi 4 platform with a newly build firmware:

[Bds]=============Begin Load Options Dumping ...=============
   Driver Options:
   SysPrep Options:
   Boot Options:
     Boot0000: UiApp              0x0109
     Boot0001: UEFI Shell                 0x0000
   PlatformRecovery Options:
     PlatformRecovery0000: Default PlatformRecovery               0x0001
[Bds]=============End Load Options Dumping=============

With this, PlatformRecovery0000 gets executed by default, which is what we 
want, since it will pick /efi/boot/bootaa64.efi from either SD or USB and run 
it, the only issue being that, because ReadyToBoot has not been executed, the 
graphical console is not operative so users can't interact with the OS 
installer.

So I'm really not sure how adding an extra PlatformRecovery#### would help. And 
I'm also not too familiar with how one would go around to add such an entry...

> By the way, I also checked the UEFI specification. It looks making sense to 
> only signal ReadyToBoot for boot option (Boot####).

That's something I considered too, but I disagree with this conclusion.

My reasoning is that, if PlatformRecovery#### can execute a regular bootloader 
like /efi/boot/boot####.efi from installation media, then it should go through 
the same kind of initialization that happens for a regular boot option, and 
that should include signalling the ReadyToBoot event.

If there was a special bootloader for PlatformRecovery#### (e.g. 
/efi/boot/recovery####.efi) then I would agree with only signalling ReadyToBoot 
for a formal Boot#### option. But that isn't the case, so I think it is 
reasonable to want to have ReadyToBoot also occur when a /efi/boot/boot####.efi 
bootloader is executed from PlatformRecovery####., especially when we know it 
can be crucial to ensuring that the end user can actually use the graphical 
console.

> Therefore, your change may also require specification change.

Yes, I mentioned that in the cover letter for this patch 
(https://edk2.groups.io/g/devel/message/61327 ), which also describes the issue 
we are trying to solve in greater details. This is what I wrote:

------------------------------------------------------------------------
Note however that this may require a specs update, as the current UEFI specs 
for EFI_BOOT_SERVICES.CreateEventEx() have:

 >  EFI_EVENT_GROUP_READY_TO_BOOT
 >    This event group is notified by the system when the Boot Manager
 >    is about to load and execute a boot option.

and, once this patch has been applied, we may want to update this section to 
mention that it applies to both Boot Manager and Platform Recovery.
------------------------------------------------------------------------


Again, I don't have an issue with trying to use an alternate approach to solve 
our problem (though I ultimately believe that, if PlatformRecovery#### can 
launch a /efi/boot/boot####.efi bootloader then we must update the specs and 
the code to have ReadyToBoot also signalled then, because that's the logical 
thing to do). But right now, I'm not seeing how to achieve that when 
PlatformRecovery#### is the option that is used to launch the OS installation 
the bootloader. So if you can provide mode details on how exactly you think 
creating an alternate PlatformRecovery option would help, I would appreciate it.

Regards,

/Pete

> 
> Regards,
> Sunny Wang
> 
> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of 
> Pete Batard
> Sent: Tuesday, June 16, 2020 5:56 PM
> To: devel@edk2.groups.io
> Cc: zhichao....@intel.com; ray...@intel.com; ard.biesheu...@arm.com; 
> l...@nuviainc.com
> Subject: [edk2-devel] [edk2][PATCH 1/1] 
> MdeModulePkg/UefiBootManagerLib: Signal ReadyToBoot on platform 
> recovery
> 
> Currently, the ReadyToBoot event is only signaled when a formal Boot Manager 
> option is executed (in BmBoot.c -> EfiBootManagerBoot ()).
> 
> However, with the introduction of Platform Recovery in UEFI 2.5, which may 
> lead to the execution of a boot loader that has similar requirements to a 
> regular one, yet is not launched as a Boot Manager option, it also becomes 
> necessary to signal ReadyToBoot when a Platform Recovery boot loader runs.
> 
> Especially, this can be 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 behaviour by calling EfiSignalEventReadyToBoot () in 
> EfiBootManagerProcessLoadOption (), which is the function that sets up the 
> platform recovery boot process.
> 
> Signed-off-by: Pete Batard <p...@akeo.ie>
> ---
>   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 89372b3b97b8..117f1f5b124c 100644
> --- a/MdeModulePkg/Library/UefiBootManagerLib/BmLoadOption.c
> +++ b/MdeModulePkg/Library/UefiBootManagerLib/BmLoadOption.c
> @@ -1376,6 +1376,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.21.0.windows.1
> 
> 
> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#61414): https://edk2.groups.io/g/devel/message/61414
Mute This Topic: https://groups.io/mt/74912987/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Reply via email to