[EXTERNAL] Re: [edk2-devel] [edk2][PATCH 1/1] MdeModulePkg/UefiBootManagerLib: Signal ReadyToBoot on platform recovery
Samer El-Haj-Mahmoud
samer.el-haj-mahmoud at arm.com
Fri Apr 30 18:36:45 UTC 2021
Resurrecting..
This continues to be a pain point.
At this point, it seems to me that we have multiple people and platforms in this community that are complaining about code not following the spec, but the maintainers are not agreeing because of "legacy implementation of some unidentified platforms". This is very unfortunate, but if the path forward is to fork the library, so be it.
I also recently opened an ECR against the UEFI spec to clarify the language in order to avoid any confusion: https://bugzilla.tianocore.org/show_bug.cgi?id=3336
Thanks,
--Samer
> -----Original Message-----
> From: Laszlo Ersek <lersek at redhat.com>
> Sent: Wednesday, February 17, 2021 9:26 AM
> To: Pete Batard <pete at akeo.ie>; Leif Lindholm <leif at nuviainc.com>;
> devel at edk2.groups.io
> Cc: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud at arm.com>; Andrei
> Warkentin (awarkentin at vmware.com) <awarkentin at vmware.com>; Wang,
> Sunny (HPS SW) <sunnywang at hpe.com>; zhichao.gao at intel.com;
> ray.ni at intel.com; Ard Biesheuvel <ardb+tianocore at kernel.org>; Andrew
> Fish <afish at apple.com>; Michael D Kinney <michael.d.kinney at intel.com>;
> Jian J Wang <jian.j.wang at intel.com>; Hao A Wu <hao.a.wu at intel.com>
> Subject: Re: [EXTERNAL] Re: [edk2-devel] [edk2][PATCH 1/1]
> MdeModulePkg/UefiBootManagerLib: Signal ReadyToBoot on platform
> recovery
>
> On 02/17/21 13:18, Pete Batard wrote:
> > Hi Leif,
> >
> > Thanks for trying to resurrect this issue.
> >
> > At this stage, and despite some initial pushback in the bugzilla
> > ticket, I believe we can all agree with the consensus that
> > UefiBootManagerLib is not in fact specs-compliant and therefore needs
> > to be fixed, one way or another, to make it specs-compliant.
> >
> > My take on this is that, rather than propose a new patch, I'd much
> > rather have the current maintainers agree on the course of action to
> > fix the library (which, as Leif suggests, might very well be to split
> > the library into a specs-compliant and non-specs-compliant version),
> > as it would of course be better if the fix came from people who have
> > better understanding of the ramifications we might face with trying to
> > correct the current behaviour, and especially, who have knowledge of
> > the platforms that might be impacted from making the lib specs-compliant.
> >
> > Especially, I don't think that the patch that I originally submitted
> > for this, or the additional proposals we made, are still receivable,
> > as they seem to fall short of fixing the issue in a manner that all
> > platforms can be happy with. And that is why I'd like to hear from the
> > maintainers on what their preferred approach would be.
>
> A new Feature PCD could satisfy both sets of platforms, could it not?
>
> (Sorry if the original patch already had such a PCD; I don't remember.)
>
> Of course then we'd have a debate around the DEC default for the new PCD
> -- I'd say the default value of the PCD should match the spec-mandated
> behavior.
>
> I don't recall any specifics, but a bug-compat pattern that's sometimes used is
> this:
>
> if (BugCompatEnabled) {
> //
> // do the right thing in the wrong place, for legacy platforms' sake
> //
> Foo ();
> }
>
> //
> // Do some stuff.
> //
> Bar ();
>
> if (!BugCompatEnabled) {
> //
> // do the right thing in the right place, for conformant platforms
> //
> Foo ();
> }
>
> Not sure if it applies here.
>
> Thanks
> Laszlo
>
>
> > On 2021.02.17 11:42, Leif Lindholm wrote:
> >> Hi Pete, +various
> >>
> >> Resurrecting this old thread since Ard pointed out an issue I ran
> >> into myself had already been encountered by Pete.
> >> And the bugzilla ticket (directly below this reply) has had no
> >> relevant progress since August.
> >>
> >> Executive summary:
> >> The current UefiBootManagerLib implementation of the
> PlatformRecovery
> >> path does not notify the EFI_EVENT_GROUP_READY_TO_BOOT event.
> >>
> >> The argument has been made that since changing this would affect an
> >> unnamed number of non-public platforms, the behaviour cannot be
> >> changed even though it violates the UEFI specification.
> >>
> >> I disagree with that statement. If we want to fork UefiBootManagerLib
> >> into a BrokenLegacyUefiBootManagerLib and an actually correct one,
> >> and have those platforms move to the BrokenLegacy variant, I'm OK
> >> with that.
> >>
> >> But using the default version should give specification-compliant
> >> behaviour.
> >>
> >> /
> >> Leif
> >>
> >> On Tue, Jun 30, 2020 at 18:17:10 +0100, Pete Batard wrote:
> >>> Please note that I have created a bug report
> >>> (https://bugzilla.tianocore.org/show_bug.cgi?id=2831) to address the
> >>> non-compliance issue was raised during the course of the discussion
> >>> below.
> >>>
> >>> Regards,
> >>>
> >>> /Pete
> >>>
> >>>
> >>> On 2020.06.17 18:06, Samer El-Haj-Mahmoud wrote:
> >>>> I worked with Pete offline on this..
> >>>>
> >>>> This code seems to be violating the UEFI Spec:
> >>>>
> >>>>
> https://github.com/tianocore/edk2/blob/a56af23f066e2816c67b7c6e64de
> >>>>
> 7ddefcd70780/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c#L176
> 3
> >>>>
> >>>>
> >>>> //
> >>>> // 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/b15646484eaffcf7cc464fdea021
> >>>> 4498f26addc2/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 at edk2.groups.io <devel at edk2.groups.io> On Behalf Of
> >>>> Samer El-Haj-Mahmoud via groups.io
> >>>> Sent: Wednesday, June 17, 2020 12:42 PM
> >>>> To: devel at edk2.groups.io; Andrei Warkentin
> (awarkentin at vmware.com)
> >>>> <awarkentin at vmware.com>; Wang, Sunny (HPS SW)
> <sunnywang at hpe.com>;
> >>>> pete at akeo.ie
> >>>> Cc: zhichao.gao at intel.com; ray.ni at intel.com; Ard Biesheuvel
> >>>> <Ard.Biesheuvel at arm.com>; leif at nuviainc.com; Samer El-Haj-
> Mahmoud
> >>>> <Samer.El-Haj-Mahmoud at 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 at edk2.groups.io <mailto:devel at 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:sunnywang at hpe.com>;
> >>>> mailto:devel at edk2.groups.io; mailto:pete at akeo.ie
> >>>> Cc: mailto:zhichao.gao at intel.com; mailto:ray.ni at intel.com; Ard
> >>>> Biesheuvel <mailto:Ard.Biesheuvel at arm.com>;
> >>>> mailto:leif at 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 at edk2.groups.io <mailto:devel at edk2.groups.io>
> on
> >>>> behalf of Pete Batard via groups.io <mailto:pete=akeo.ie at groups.io>
> >>>> Sent: Wednesday, June 17, 2020 11:34 AM
> >>>> To: Wang, Sunny (HPS SW) <mailto:sunnywang at hpe.com>;
> >>>> mailto:devel at edk2.groups.io <mailto:devel at edk2.groups.io>
> >>>> Cc: mailto:zhichao.gao at intel.com <mailto:zhichao.gao at intel.com>;
> >>>> mailto:ray.ni at intel.com <mailto:ray.ni at intel.com>;
> >>>> mailto:ard.biesheuvel at arm.com <mailto:ard.biesheuvel at arm.com>;
> >>>> mailto:leif at nuviainc.com <mailto:leif at 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:pete at akeo.ie]
> >>>>> Sent: Wednesday, June 17, 2020 6:59 PM
> >>>>> To: Wang, Sunny (HPS SW) <mailto:sunnywang at hpe.com>;
> >>>>> mailto:devel at edk2.groups.io
> >>>>> Cc: mailto:zhichao.gao at intel.com; mailto:ray.ni at intel.com;
> >>>>> mailto:ard.biesheuvel at arm.com; mailto:leif at 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%2
> >>>>>
> Fedk2.groups.io%2Fg%2Fdevel%2Fmessage%2F61327&data=02%7C01%
> 7Ca
> >>>>>
> warkentin%40vmware.com%7C5f90d077bc7949c1122f08d812dc48d3%7Cb391
> 38
> >>>>>
> ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637280084611749324&sdat
> a=
> >>>>>
> 2%2B%2FcvMkrmZGTRRLDGSuMsKbiyDOGtwYwZ7qSqMyMicc%3D&res
> erved=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 at edk2.groups.io [mailto:devel at edk2.groups.io]
> >>>>>> On Behalf Of Pete Batard
> >>>>>> Sent: Tuesday, June 16, 2020 5:56 PM
> >>>>>> To: mailto:devel at edk2.groups.io
> >>>>>> Cc: mailto:zhichao.gao at intel.com; mailto:ray.ni at intel.com;
> >>>>>> mailto:ard.biesheuvel at arm.com; mailto:leif at 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:pete at 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.
> >>>>
> >>>
> >>>
> >>>
> >>>
> >
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.
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#74673): https://edk2.groups.io/g/devel/message/74673
Mute This Topic: https://groups.io/mt/80701195/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list