I worked with Pete offline on this..
This code seems to be violating the UEFI Spec:
https://github.com/tianocore/edk2/blob/a56af23f066e2816c67b7c6e64de7ddefcd70780/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c#L1763
//
// 3. Signal the EVT_SIGNAL_READY_TO_BOOT event when we are about to load
and execute
// the boot option.
//
if (BmIsBootManagerMenuFilePath (BootOption->FilePath)) {
DEBUG ((EFI_D_INFO, "[Bds] Booting Boot Manager Menu.\n"));
BmStopHotkeyService (NULL, NULL);
} else {
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));
//
// 4. Repair system through DriverHealth protocol
//
BmRepairAllControllers (0);
}
The UEFI Spec section 3.1.7 clearly states that Boot Options (and their
FilePathList) *shall not* be evaluated prior to the completion of
EFI_EVENT_GROUP_READY_TO_BOOT event group processing:
"After all SysPrep#### variables have been launched and exited, the platform shall
notify EFI_EVENT_GROUP_READY_TO_BOOT event group and begin to evaluate Boot#### variables
with Attributes set to LOAD_OPTION_CATEGORY_BOOT according to the order defined by
BootOrder. The FilePathList of variables marked LOAD_OPTION_CATEGORY_BOOT shall not be
evaluated prior to the completion of EFI_EVENT_GROUP_READY_TO_BOOT event group
processing."
This is a prescriptive language that is stronger than the language in section
7.1 which defines the ReadyToBoot event group in a general way:
"EFI_EVENT_GROUP_RESET_SYSTEM
This event group is notified by the system when ResetSystem() is invoked and the
system is about to be reset. The event group is only notified prior to
ExitBootServices() invocation."
The EDK2 code in the else block above (to call EfiSignalEventReadyToBoot() ) need
to move before the code that is processing BootOption->FilePath. In fact, why
is this signaling even a BootManager task? It should be a higher level BDS task
(after processing SysPrp and before processing Boot options, per the spec). This
would be somewhere around
https://github.com/tianocore/edk2/blob/b15646484eaffcf7cc464fdea0214498f26addc2/MdeModulePkg/Universal/BdsDxe/BdsEntry.c#L1007
where SysPrep is processed.
This should also take care of the issue Pete reported in this thread, without
the need for explicitly signaling ReadyToBoot from PlatformRecovery (or
changing the UEFI spec).
Thanks,
--Samer
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Samer
El-Haj-Mahmoud via groups.io
Sent: Wednesday, June 17, 2020 12:42 PM
To: devel@edk2.groups.io; Andrei Warkentin (awarken...@vmware.com)
<awarken...@vmware.com>; Wang, Sunny (HPS SW) <sunnyw...@hpe.com>; p...@akeo.ie
Cc: zhichao....@intel.com; ray...@intel.com; Ard Biesheuvel <ard.biesheu...@arm.com>;
l...@nuviainc.com; Samer El-Haj-Mahmoud <samer.el-haj-mahm...@arm.com>
Subject: Re: [edk2-devel] [edk2][PATCH 1/1] MdeModulePkg/UefiBootManagerLib:
Signal ReadyToBoot on platform recovery
The UEFI spec (3.1.7) says:
"After all SysPrep#### variables have been launched and exited, the platform shall
notify EFI_EVENT_GROUP_READY_TO_BOOT event group and begin to evaluate Boot#### variables
with Attributes set to LOAD_OPTION_CATEGORY_BOOT according to the order defined by
BootOrder. The FilePathList of variables marked LOAD_OPTION_CATEGORY_BOOT shall not be
evaluated prior to the completion of EFI_EVENT_GROUP_READY_TO_BOOT event group
processing."
The way I read this, I expect ReadyToBoot to be signaled after SysPrep#### (if
any) are processed, but before Boot#### are processed. Is my understanding
correct that this language implies ReadyToBoot need to be signaled even if
BootOrder does not contain any Boot#### options marked as
LOAD_OPTION_CATEGORY_BOOT? And if so, is EDK2 not doing this, which leads us to
this patch (signaling it in PlatformRecovery?)
From: mailto:devel@edk2.groups.io <mailto:devel@edk2.groups.io> On Behalf Of
Andrei Warkentin via groups.io
Sent: Wednesday, June 17, 2020 12:37 PM
To: Wang, Sunny (HPS SW) <mailto:sunnyw...@hpe.com>;
mailto:devel@edk2.groups.io; mailto:p...@akeo.ie
Cc: mailto:zhichao....@intel.com; mailto:ray...@intel.com; Ard Biesheuvel
<mailto:ard.biesheu...@arm.com>; mailto:l...@nuviainc.com
Subject: Re: [edk2-devel] [edk2][PATCH 1/1] MdeModulePkg/UefiBootManagerLib:
Signal ReadyToBoot on platform recovery
Thanks Pete.
I think the question I have, that I hope Tiano veterans can chime in, is
whether we are doing the right thing, or if we should be overriding the boot
mode? I.e. is it normal that we boot up in recovery until options are saved?
A
________________________________________
From: mailto:devel@edk2.groups.io <mailto:devel@edk2.groups.io> on behalf of Pete
Batard via groups.io <mailto:pete=akeo...@groups.io>
Sent: Wednesday, June 17, 2020 11:34 AM
To: Wang, Sunny (HPS SW) <mailto:sunnyw...@hpe.com>; mailto:devel@edk2.groups.io
<mailto:devel@edk2.groups.io>
Cc: mailto:zhichao....@intel.com <mailto:zhichao....@intel.com>; mailto:ray...@intel.com
<mailto:ray...@intel.com>; mailto:ard.biesheu...@arm.com <mailto:ard.biesheu...@arm.com>;
mailto:l...@nuviainc.com <mailto:l...@nuviainc.com>
Subject: Re: [edk2-devel] [edk2][PATCH 1/1] MdeModulePkg/UefiBootManagerLib:
Signal ReadyToBoot on platform recovery
On 2020.06.17 14:04, Wang, Sunny (HPS SW) wrote:
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.
Okay.
One element that I'm going to point out is that, with the current EDK2
code (i.e. without this proposal applied), and after a user goes into
the setup to save their boot options in order for regular boot options
to get executed instead of PlaformRecovery, the OnReadyToBoot event is
actually called twice.
So my understanding is that, while we of course want to avoid this and
any patch proposal should actively try to prevent it, it seems we
already have behaviour in EDK2 that can lead to OnReadyToBoot being
signalled more than once.
At least the current Pi 4 platform does demonstrate this behaviour. For
instance, if you run DEBUG, you will see two instances of:
RemoveDtStdoutPath: could not retrieve DT blob - Not Found
which is a one-instance message generated from the ConsolePrefDxe's
OnReadyToBoot() call. I've also confirmed more specifically that
OnReadyToBoot() is indeed called twice.
I don't recall us doing much of any special with regards to boot options
for the Pi platform, so my guess is that it's probably not the only
platform where OnReadyToBoot might be signalled more than once, and that
this might be tied to a default EDK2 behaviour. Therefore I don't see
having a repeated event as a major deal breaker (though, again, if we
can avoid that, we of course will want to).
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.
So that would basically be adding code that duplicates, in part, what
Platform Recovery already does.
I have to be honest: Even outside of the extra work this would require,
I don't really like the idea of having to write our own application, as
it will introduce new possible points of failures and require extra
maintenance (especially as we will want to be able to handle network
boot and other options, and before long, I fear that we're going to have
to write our own Pi specific boot manager). Doing so simply because the
current Platform Recovery, which does suit our needs otherwise, is not
designed to call ReadyToBoot does not seem like the best course of
action in my book.
Instead, I still logically believe that any option that calls a boot
loader should signal ReadyToBoot, regardless of whether it was launched
from Boot Manager or Platform Recovery, and that it shouldn't be left to
each platform to work around that.
Of course, I understand that this would require a specs change, and that
it also may have ramifications for existing platforms that interpret the
current specs pedantically. But to me, regardless of what the specs
appear to be limiting it to right now, the logic of a "ReadyToBoot"
event is that it should be signalled whenever a bootloader is about to
be executed, rather than only when a bootloader happened to be launched
through a formal Boot Manager option.
I would therefore appreciate if other people could weigh in on this
matter, to see if I'm the only one who believes that we could ultimately
have more to gain from signalling ReadyToBoot with PlatformRecovery
options than leaving things as they stand right now...
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,
The way I see it is that the Pi platform is unlikely to be the only one
where PlatformRecovery is seen as a means to install an OS. Granted,
this may seem like abusing the option, but since UEFI doesn't provide an
"Initial OS Install" mode, I would assert that it as good a use of this
option as any.
In other words, I don't think this improvement would only benefit the Pi
platform.
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.
That's a legitimate concern, and I would agree the one major potential
pitfall of this proposal, if there happens to exist a system where an
OnReadyToBoot even before running the recovery option can have adverse
effects.
I don't really believe that such a system exists, because I expect most
recovery boot loaders to also work (or at least have been designed to
work) as regular boot options. But I don't have enough experience with
platform recovery to know if that's a correct assertion to make...
If the alternative approach I mentioned works for you, I think that would be an
easier solution.
Right now, even as the patch proposal has multiple issues that require
it to be amended (Don't signal ReadyToBoot except for PlatformRecovery
+ Prevent situations where ReadyToBoot could be signalled multiple
times) I still see it as both an easier solution than the alternative,
as well as one that *should* benefit people who design Platform Recovery
UEFI applications in the long run. So that is why I am still trying to
advocate for it.
But I very much hear your concerns, and I agree that specs changes are
better avoided when possible.
Thus, at this stage, even as I don't want to drag this discussion much
further, I don't feel like I want to commit to one solution or the other
before we have had a chance to hear other people, who may have their own
opinion on the matter, express their views.
Regards,
/Pete
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) <mailto:sunnyw...@hpe.com>; mailto:devel@edk2.groups.io
Cc: mailto:zhichao....@intel.com; mailto:ray...@intel.com;
mailto:ard.biesheu...@arm.com; mailto: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://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fedk2.groups.io%2Fg%2Fdevel%2Fmessage%2F61327&data=02%7C01%7Cawarkentin%40vmware.com%7C5f90d077bc7949c1122f08d812dc48d3%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637280084611749324&sdata=2%2B%2FcvMkrmZGTRRLDGSuMsKbiyDOGtwYwZ7qSqMyMicc%3D&reserved=0
), 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: mailto:devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
Pete Batard
Sent: Tuesday, June 16, 2020 5:56 PM
To: mailto:devel@edk2.groups.io
Cc: mailto:zhichao....@intel.com; mailto:ray...@intel.com;
mailto:ard.biesheu...@arm.com;
mailto: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 <mailto: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
IMPORTANT NOTICE: The contents of this email and any attachments are
confidential and may also be privileged. If you are not the intended recipient,
please notify the sender immediately and do not disclose the contents to any
other person, use it for any purpose, or store or copy the information in any
medium. Thank you.
IMPORTANT NOTICE: The contents of this email and any attachments are
confidential and may also be privileged. If you are not the intended recipient,
please notify the sender immediately and do not disclose the contents to any
other person, use it for any purpose, or store or copy the information in any
medium. Thank you.
