From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by mx.groups.io with SMTP id smtpd.web10.8079.1613571954476008606 for ; Wed, 17 Feb 2021 06:25:54 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=U0dVeeFm; spf=pass (domain: redhat.com, ip: 216.205.24.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1613571953; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=qiAxnYTgBZ8G3hGvd0BNWwWG2BS51ll9s3EqEwzGvDc=; b=U0dVeeFmDqbwh0k00zE6t8WWHEukU33oy86CIBON0TAKiUQ5GXIpl/wR5arQHsB0qESZK1 xnIKiHT6v1Y7zpYf7GhGBVbLj7HOuxp0FTcn9ADftrjZ8PDL+ECVjeYwl28QjQw4TZSuOM oiKtKOu7yyy8dzV4KzLopRNCM3FIwYY= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-569--32JFkZFMQmnofAt-_8Glw-1; Wed, 17 Feb 2021 09:25:48 -0500 X-MC-Unique: -32JFkZFMQmnofAt-_8Glw-1 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 1184A107ACE3; Wed, 17 Feb 2021 14:25:46 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-115-236.ams2.redhat.com [10.36.115.236]) by smtp.corp.redhat.com (Postfix) with ESMTP id B785F19D6C; Wed, 17 Feb 2021 14:25:42 +0000 (UTC) Subject: Re: [EXTERNAL] Re: [edk2-devel] [edk2][PATCH 1/1] MdeModulePkg/UefiBootManagerLib: Signal ReadyToBoot on platform recovery To: Pete Batard , Leif Lindholm , devel@edk2.groups.io Cc: Samer El-Haj-Mahmoud , "Andrei Warkentin (awarkentin@vmware.com)" , "Wang, Sunny (HPS SW)" , "zhichao.gao@intel.com" , "ray.ni@intel.com" , Ard Biesheuvel , Andrew Fish , Michael D Kinney , Jian J Wang , Hao A Wu References: <20200616095622.2820-1-pete@akeo.ie> <20200616095622.2820-2-pete@akeo.ie> <99904809-1e07-6bd8-f7ba-25e87b1fe543@akeo.ie> <161962620CFC252E.28613@groups.io> <20210217114237.GF1664@vanye> <1c551fb6-8b5b-286e-8f74-b28eb797a6ae@akeo.ie> From: "Laszlo Ersek" Message-ID: Date: Wed, 17 Feb 2021 15:25:41 +0100 MIME-Version: 1.0 In-Reply-To: <1c551fb6-8b5b-286e-8f74-b28eb797a6ae@akeo.ie> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit 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/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 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 (awarkentin@vmware.com) >>>> ; Wang, Sunny (HPS SW) ; >>>> pete@akeo.ie >>>> Cc: zhichao.gao@intel.com; ray.ni@intel.com; Ard Biesheuvel >>>> ; leif@nuviainc.com; Samer El-Haj-Mahmoud >>>> >>>> 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 On >>>> Behalf Of Andrei Warkentin via groups.io >>>> Sent: Wednesday, June 17, 2020 12:37 PM >>>> To: Wang, Sunny (HPS SW) ; >>>> mailto:devel@edk2.groups.io; mailto:pete@akeo.ie >>>> Cc: mailto:zhichao.gao@intel.com; mailto:ray.ni@intel.com; Ard >>>> Biesheuvel ; mailto:leif@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 on >>>> behalf of Pete Batard via groups.io >>>> Sent: Wednesday, June 17, 2020 11:34 AM >>>> To: Wang, Sunny (HPS SW) ; >>>> mailto:devel@edk2.groups.io >>>> Cc: mailto:zhichao.gao@intel.com ; >>>> mailto:ray.ni@intel.com ; >>>> mailto:ard.biesheuvel@arm.com ; >>>> mailto:leif@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@akeo.ie] >>>>> Sent: Wednesday, June 17, 2020 6:59 PM >>>>> To: Wang, Sunny (HPS SW) ; >>>>> mailto:devel@edk2.groups.io >>>>> Cc: mailto:zhichao.gao@intel.com; mailto:ray.ni@intel.com; >>>>> mailto:ard.biesheuvel@arm.com; mailto:leif@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.gao@intel.com; mailto:ray.ni@intel.com; >>>>>> mailto:ard.biesheuvel@arm.com; >>>>>> mailto:leif@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 >>>>>> --- >>>>>>      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. >>>> >>> >>> >>> >>> >