From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from NAM04-SN1-obe.outbound.protection.outlook.com (NAM04-SN1-obe.outbound.protection.outlook.com [40.107.70.77]) by mx.groups.io with SMTP id smtpd.web10.807.1592411821900305451 for ; Wed, 17 Jun 2020 09:37:02 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@vmware.com header.s=selector2 header.b=ED4ueqSP; spf=pass (domain: vmware.com, ip: 40.107.70.77, mailfrom: awarkentin@vmware.com) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=UDCO0BZTapQ+OLA33X8FAlUm9GzYMOr8emXg1Exs8Ri7uZpIwpFuLLsQi3Xi2dADacPSAtH5bVyCbGj4MbM1j7UsH3varsDub4JYOaivCJs9p80NDY+UsSl9a1GjBmbUMwkbdSpp1ixROoFTiVfEX/h1t/QhlSlz05DIDOU8ghF/oZSdC+sK46PNPT9fnyScc5a43D8DXT0aP2RfTMRzCIuKK+lnZ7Pmsulm2lWW1QELXWh+3DqI0mecUJlx9kvfmHat1nHns+Itellg7Fy3BXmFJXpNKQbUhD5iw3EjTOfTNR0fqhnE8qAAutykMfQRM7fLl4Z5dVwXX5hneJyX1w== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=hCxasOdU92N2GlVaYFQcdyG/Zi9gntwgkapJy6mQEIk=; b=L5fsm7fsTdA85BusI5o0YFwZmKL+6FJA0g7uDFy1V+/d8B2WcZzoqsAYEMfw/IcxaUa202h4YyU8Ci4WUmmfhE46Hby01pehadQExVZ19a/bv+HucahwolN5HU7ndskJSOP0y3rxrPIyPOV2lqKM6Q0ITlcAMDX9bX8XbWtElIQDQfA96fme7YT+/wwPw1HV4mNfU5Huu+Tjy6ZufETTyBqHmlP1+vkzTZ1FUfazt/huuUyXH6i3EhqgKa/4pw7KLaRnbKJiDPcCgt3yM3gil3lf3ZwjO0DfimznSuS7spewu0g8LjL/FGkJGFCoVAMuiTS7zLVRjVNN0or0Eyctcw== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=vmware.com; dmarc=pass action=none header.from=vmware.com; dkim=pass header.d=vmware.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=vmware.com; s=selector2; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=hCxasOdU92N2GlVaYFQcdyG/Zi9gntwgkapJy6mQEIk=; b=ED4ueqSPlkJy6AuAWzgtTv6+QuElRFKQTVviM1g8nFq0qoeK635lEaMXEc6uNLh3CFfy4yu4bOFkdrNl9dySwm+pS3Mt9+NOOibTHdMFX1BwYwo8nOJNj43tB679W9GwhW8kKDjzkuxQErF9D7lUCX04MD/H0Pv2EotK5ZQEuCw= Received: from BN6PR05MB3411.namprd05.prod.outlook.com (2603:10b6:405:43::23) by BN6PR05MB2897.namprd05.prod.outlook.com (2603:10b6:404:36::20) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3131.10; Wed, 17 Jun 2020 16:36:59 +0000 Received: from BN6PR05MB3411.namprd05.prod.outlook.com ([fe80::e1ef:31eb:c802:aef0]) by BN6PR05MB3411.namprd05.prod.outlook.com ([fe80::e1ef:31eb:c802:aef0%3]) with mapi id 15.20.3109.018; Wed, 17 Jun 2020 16:36:59 +0000 From: "Andrei Warkentin" To: "Wang, Sunny (HPS SW)" , "devel@edk2.groups.io" , "pete@akeo.ie" CC: "zhichao.gao@intel.com" , "ray.ni@intel.com" , "ard.biesheuvel@arm.com" , "leif@nuviainc.com" Subject: Re: [edk2-devel] [edk2][PATCH 1/1] MdeModulePkg/UefiBootManagerLib: Signal ReadyToBoot on platform recovery Thread-Topic: [edk2-devel] [edk2][PATCH 1/1] MdeModulePkg/UefiBootManagerLib: Signal ReadyToBoot on platform recovery Thread-Index: AQHWQ8RvLygIMjSfK06SRsg5HFfqkKjcd5IAgAAtmwCAACLVAIAAOrsAgAAAdBs= Date: Wed, 17 Jun 2020 16:36:59 +0000 Message-ID: References: <20200616095622.2820-1-pete@akeo.ie> <20200616095622.2820-2-pete@akeo.ie> <99904809-1e07-6bd8-f7ba-25e87b1fe543@akeo.ie> , In-Reply-To: Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: hpe.com; dkim=none (message not signed) header.d=none;hpe.com; dmarc=none action=none header.from=vmware.com; x-originating-ip: [98.214.99.181] x-ms-publictraffictype: Email x-ms-office365-filtering-correlation-id: e7593cbb-e34e-4d34-00e2-08d812dca834 x-ms-traffictypediagnostic: BN6PR05MB2897: x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:10000; x-forefront-prvs: 04371797A5 x-ms-exchange-senderadcheck: 1 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: vlG53YYLoRORA2G2r0XZvys++LOMfUFi9mnND1cKeZGPFtBhgVwTAKAMwHGsoU/RUsXj1wFwZvr6UqiFsEj5/MWIBdf4c6adioUw5sjBButsQ15QiG6IKc6f0dba5EUjuaH2G1Bq2+CNadTF9r5WRYAszEBgXcgKTsCnnPK86CwnZlRR1b2ee0EqQHBvl1VmvvWfZ0c1jUqLFbhbuPZEPpusp5b7t10ROYyoTkHi2P49adqOPdmy2cDIHKF1pBulJH7jWuRktKFTYMKYFuiX+qgf98V3mYOFKS5k0RQdLD0xmkwX7opJxJl3dD84VGRCfE/RlixuMnl4ZqB1e7notX5vSnUznF3R0mldHDQXckYJtmprjKXV81SCQHE99WZQFnsSIL7ExluJjhKfR82Qsv83JQneYcbv/Z5UkNjgf4IiRbTQwjm39NyG/0uGJm8u0GDJP1yxKHUxDwmrymLniA== x-forefront-antispam-report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:BN6PR05MB3411.namprd05.prod.outlook.com;PTR:;CAT:NONE;SFTY:;SFS:(6029001)(4636009)(396003)(136003)(366004)(376002)(39860400002)(346002)(8676002)(4326008)(33656002)(2906002)(71200400001)(186003)(26005)(55016002)(166002)(966005)(18265965003)(8936002)(54906003)(45080400002)(66946007)(110136005)(5660300002)(478600001)(30864003)(9686003)(83380400001)(76116006)(7696005)(52536014)(66476007)(66446008)(86362001)(296002)(316002)(66556008)(64756008)(6506007)(19627405001)(53546011);DIR:OUT;SFP:1101; x-ms-exchange-antispam-messagedata: Dkl4ugbBDWKqln6Zubr+zQrM0OLXGaMN6PjCMaO593RNDJfjJzF/uQYn4wSiN+k2bpYqiStOWZ8DS51tIumOe4rO0VwAMzkLn54Z1gXfBae2vvSv40SB/XxO5XztJY/SOPxmMlB/6tFV0cJMPVwJ9NocrNTlBjWEyAQFlJV0w/hfopeVeBcrTzr8s+inPGRWwJpXqETCdgw90zBbwOi4D6TstMVEa6dj5wZb7IsP74vuzgXQYrtlydUzLzP+r8D1ENfYuMDZJnj2ayG3Txqj+01ajy0qIJnJHrh1C6hIdXTw3V2DQskmsimSeyHfaOglGvjpahgINA8Z6A34omd0CI9WjG6qvSDs2OY9QjbkWqGJug02q2P/iyVK7TIyTV4aI3wUr50oxkzFRb8fAyQ7m6J+iXnI6JoVFYPOHmlni4+PKLv798G3dxpLn6YulynCp65fIx9KmzNws1V4gr+/4gx5nCz6zO+sL9THIT+MXVs3zCiXZa5+0deozcL3sizX x-ms-exchange-transport-forked: True MIME-Version: 1.0 X-OriginatorOrg: vmware.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-AuthSource: BN6PR05MB3411.namprd05.prod.outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: e7593cbb-e34e-4d34-00e2-08d812dca834 X-MS-Exchange-CrossTenant-originalarrivaltime: 17 Jun 2020 16:36:59.6291 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: b39138ca-3cee-4b4a-a4d6-cd83d9dd62f0 X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: BNEsfd2eSGcjcsZ38sJcdz+BXzA2X1evtJ1SQNuazx0t/KmCNZDqg7FJNIbGm6HwMaslLxof9V+R8u6f+iB4OQ== X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN6PR05MB2897 Content-Language: en-US Content-Type: multipart/alternative; boundary="_000_BN6PR05MB3411C7F40F78D34CAC02EE3EB99A0BN6PR05MB3411namp_" --_000_BN6PR05MB3411C7F40F78D34CAC02EE3EB99A0BN6PR05MB3411namp_ Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Thanks Pete. I think the question I have, that I hope Tiano veterans can chime in, is w= hether 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 save= d? A ________________________________ From: 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) ; devel@edk2.groups.io Cc: zhichao.gao@intel.com ; ray.ni@intel.com ; ard.biesheuvel@arm.com ; leif@nuviain= c.com Subject: Re: [edk2-devel] [edk2][PATCH 1/1] MdeModulePkg/UefiBootManagerLi= b: 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 PlatformRecove= ry option (a UEFI application) calls EfiBootManagerBoot() to launch the rec= overed 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 y= ou have in mind? > [Sunny] Sure. I added more information below. If it is still not clear e= nough, feel free to let me know. > 1. Create a UEFI application with the code to signal ReadyToBoot a= nd 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 followin= g in a DXE driver or other places before "Default PlatformRecovery" registr= ation: > Status =3D EfiBootManagerInitializeLoadOption ( > &LoadOption, > 0, = -> 0 is the OptionNumber to let application be load be= fore " 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 boo= tloader 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 c= over letter for this patch. I was just thinking that we may not really nee= d to make this behavior change in both EDK II code and UEFI specification f= or solving the problem specific to the case that OS is loaded by "Default P= latformRecovery" 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 o= f the system or BIOS vendors who have implemented their PlatformRecovery op= tion. 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 woul= d 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) ; devel@edk2.groups.io > Cc: zhichao.gao@intel.com; ray.ni@intel.com; ard.biesheuvel@arm.com; lei= f@nuviainc.com > Subject: Re: [edk2-devel] [edk2][PATCH 1/1] MdeModulePkg/UefiBootManager= Lib: 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 ProcessLoadOptio= ns 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 boo= t 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 t= he PlatformRecovery option, so isn't what you describe above exactly what w= e 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 ProcessLoadOpt= ions(), 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 Efi= BootManagerProcessLoadOption() 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 smal= lest option number and using it instead of default one (with short-form Fil= e 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 yo= u have in mind? > > Our main issue here is that we must have ReadyToBoot signalled so that t= he 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 im= prove things. > > If it helps, here is what we currently default to, in terms of boot opti= ons, on a Raspberry Pi 4 platform with a newly build firmware: > > [Bds]=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3DBegin Load Options Dumping .= ..=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > Driver Options: > SysPrep Options: > Boot Options: > Boot0000: UiApp 0x0109 > Boot0001: UEFI Shell 0x0000 > PlatformRecovery Options: > PlatformRecovery0000: Default PlatformRecovery 0x000= 1 > [Bds]=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3DEnd Load Options Dumping=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > > 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 an= d run it, the only issue being that, because ReadyToBoot has not been execu= ted, the graphical console is not operative so users can't interact with th= e OS installer. > > So I'm really not sure how adding an extra PlatformRecovery#### would he= lp. 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 sens= e 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 boot= loader 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 Read= yToBoot for a formal Boot#### option. But that isn't the case, so I think i= t is reasonable to want to have ReadyToBoot also occur when a /efi/boot/boo= t####.efi bootloader is executed from PlatformRecovery####., especially whe= n 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=3Dhttps%3A%2F%2Fedk2.groups.io%2Fg%2F= devel%2Fmessage%2F61327&data=3D02%7C01%7Cawarkentin%40vmware.com%7C5f90= d077bc7949c1122f08d812dc48d3%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C6= 37280084611749324&sdata=3D2%2B%2FcvMkrmZGTRRLDGSuMsKbiyDOGtwYwZ7qSqMyMi= cc%3D&reserved=3D0 ), which also describes the issue we are trying to s= olve in greater details. This is what I wrote: > > ------------------------------------------------------------------------ > Note however that this may require a specs update, as the current UEFI s= pecs 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 sectio= n 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 s= pecs and the code to have ReadyToBoot also signalled then, because that's t= he 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 insta= llation the bootloader. So if you can provide mode details on how exactly y= ou 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.gao@intel.com; ray.ni@intel.com; ard.biesheuvel@arm.com; >> 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 Ma= nager 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 becom= es necessary to signal ReadyToBoot when a Platform Recovery boot loader run= s. >> >> 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 lo= ad 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 >> >> >> >> > --_000_BN6PR05MB3411C7F40F78D34CAC02EE3EB99A0BN6PR05MB3411namp_ Content-Type: text/html; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable
Thanks Pete.

I think the question I have, that I hope Tiano veterans can chime in, is w= hether 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 save= d?


A


From: devel@edk2.groups.io= <devel@edk2.groups.io> on behalf of Pete Batard via groups.io <pe= te=3Dakeo.ie@groups.io>
Sent: Wednesday, June 17, 2020 11:34 AM
To: Wang, Sunny (HPS SW) <sunnywang@hpe.com>; devel@edk2.grou= ps.io <devel@edk2.groups.io>
Cc: zhichao.gao@intel.com <zhichao.gao@intel.com>; ray.ni@int= el.com <ray.ni@intel.com>; ard.biesheuvel@arm.com <ard.biesheuvel@= arm.com>; leif@nuviainc.com <leif@nuviainc.com>
Subject: Re: [edk2-devel] [edk2][PATCH 1/1] MdeModulePkg/UefiBootMa= nagerLib: Signal ReadyToBoot on platform recovery
 
On 2020.06.17 14:04, Wang, Sunny (HPS SW) wrote:<= br> > Thanks for checking my comments, Pete.
>
>> Or is the "one more" the issue, meaning that it would g= et signaled more than once?
> [Sunny] Yeah, it would get signaled more than once if the PlatformRec= overy 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 <= br> 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 <= br> 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 understa= nd 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 clea= r 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 <= br> 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 <= br> each platform to work around that.

Of course, I understand that this would require a specs change, and that <= br> it also may have ramifications for existing platforms that interpret the <= br> current specs pedantically. But to me, regardless of what the specs
appear to be limiting it to right now, the logic of a "ReadyToBoot&qu= ot;
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 <= br> have more to gain from signalling ReadyToBoot with PlatformRecovery
options than leaving things as they stand right now...

>       2. Then, call EfiBootManagerIniti= alizeLoadOption like the following in a DXE driver or other places before &= quot;Default PlatformRecovery" registration:
>    Status =3D EfiBootManagerInitializeLoadOption (
>           &nbs= p;   &LoadOption,
>           &nbs= p;   0,         &nbs= p;            &= nbsp;           &nbs= p;            &= nbsp;           &nbs= p;            &= nbsp;    -> 0 is the OptionNumber to let application be l= oad before " Default PlatformRecovery" option
>           &nbs= p;   LoadOptionTypePlatformRecovery,
>           &nbs= p;   LOAD_OPTION_ACTIVE,
>           &nbs= p;   L"Application for recovering boot options",
>           &nbs= p;   FilePath,        &nb= sp;            =             &nb= sp;            =             &nb= sp;     -> FilePath is the Application's device path= ,
>           &nbs= p;   NULL,
>           &nbs= p;   0
>           &nbs= p;   );
>
>
>> My reasoning is that, if PlatformRecovery#### can execute a regul= ar bootloader like /efi/boot/boot####.efi from installation media, then it = should go through the same kind of initialization that happens for a regula= r boot option, and that should include signaling the ReadyToBoot event.
> [Sunny] Thanks for clarifying this, and Sorry about that I missed you= r cover letter for this patch.  I was just thinking that we may not re= ally need to make this behavior change in both EDK II code and UEFI specifi= cation 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 <= br> "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 <= br> platform.

> and I'm also not sure if it is worth making this change to affect som= e 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 w= ould 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 <= br> 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 <= br> before we have had a chance to hear other people, who may have their own <= br> opinion on the matter, express their views.

Regards,

/Pete


>
> Regards,
> Sunny Wang
>
> -----Original Message-----
> From: Pete Batard [mailto:pete@akeo.i= e]
> Sent: Wednesday, June 17, 2020 6:59 PM
> To: Wang, Sunny (HPS SW) <sunnywang@hpe.com>; devel@edk2.groups= .io
> Cc: zhichao.gao@intel.com; ray.ni@intel.com; ard.biesheuvel@arm.com; = leif@nuviainc.com
> Subject: Re: [edk2-devel] [edk2][PATCH 1/1] MdeModulePkg/UefiBootMana= gerLib: 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 ProcessLoa= dOptions as well, your change would also cause some unexpected behavior lik= e:
>> 1. Signal one more ReadyToBoot for the PlatformRecovery option wh= ich is an application that calls EfiBootManagerBoot() to launch its recover= ed 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 fo= r the PlatformRecovery option, so isn't what you describe above exactly wha= t we want?
>
> Or is the "one more" the issue, meaning that it would get s= ignalled more than once?
>
>
>> 2. Signal ReadyToBoot for SysPrep#### or Driver#### that is not r= eally a "boot" option.
>
> Yes, I've been wondering about that, because BdsEntry.c's ProcessLoad= Options(), which calls EfiBootManagerProcessLoadOption(),
> mentions that it will load will load and start every Driver####, SysP= rep#### 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 opt= ion and only signal ReadyToBoot for PlatformRecovery####.
>
>> To solve your problem, creating a PlatformRecovery option with th= e smallest option number and using it instead of default one (with short-fo= rm File Path Media Device Path) looks like a simpler solution.
>
> I don't mind trying an alternative approach, but I don't understand h= ow 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 tha= t the ReadyToBoot() function callback from EmbeddedPkg/Drivers/ConsolePrefD= xe 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 Pla= tformRecovery#### option to another would improve things.
>
> If it helps, here is what we currently default to, in terms of boot o= ptions, on a Raspberry Pi 4 platform with a newly build firmware:
>
> [Bds]=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3DBegin Load Options Dumpin= g ...=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
>     Driver Options:
>     SysPrep Options:
>     Boot Options:
>       Boot0000: UiApp   =            0x0109
>       Boot0001: UEFI Shell  &= nbsp;           &nbs= p;  0x0000
>     PlatformRecovery Options:
>       PlatformRecovery0000: Default Pla= tformRecovery          &n= bsp;    0x0001
> [Bds]=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3DEnd Load Options Dumping= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
>
> With this, PlatformRecovery0000 gets executed by default, which is wh= at 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 ex= ecuted, 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 su= ch an entry...
>
>> By the way, I also checked the UEFI specification. It looks makin= g sense to only signal ReadyToBoot for boot option (Boot####).
>
> That's something I considered too, but I disagree with this conclusio= n.
>
> My reasoning is that, if PlatformRecovery#### can execute a regular b= ootloader like /efi/boot/boot####.efi from installation media, then it shou= ld go through the same kind of initialization that happens for a regular bo= ot 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 R= eadyToBoot for a formal Boot#### option. But that isn't the case, so I thin= k 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=3Dhttps%3A%2F%2Fe= dk2.groups.io%2Fg%2Fdevel%2Fmessage%2F61327&amp;data=3D02%7C01%7Cawarke= ntin%40vmware.com%7C5f90d077bc7949c1122f08d812dc48d3%7Cb39138ca3cee4b4aa4d6= cd83d9dd62f0%7C0%7C0%7C637280084611749324&amp;sdata=3D2%2B%2FcvMkrmZGTR= RLDGSuMsKbiyDOGtwYwZ7qSqMyMicc%3D&amp;reserved=3D0 ), which also describes the issue we are trying to solve in greater detai= ls. This is what I wrote:
>
> ---------------------------------------------------------------------= ---
> Note however that this may require a specs update, as the current UEF= I specs for EFI_BOOT_SERVICES.CreateEventEx() have:
>
>   >  EFI_EVENT_GROUP_READY_TO_BOOT
>   >    This event group is notified by th= e system when the Boot Manager
>   >    is about to load and execute a boo= t option.
>
> and, once this patch has been applied, we may want to update this sec= tion 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 PlatformRecover= y#### can launch a /efi/boot/boot####.efi bootloader then we must update th= e specs and the code to have ReadyToBoot also signalled then, because that's the logical thing to do). But right n= ow, I'm not seeing how to achieve that when PlatformRecovery#### is the opt= ion that is used to launch the OS installation the bootloader. So if you ca= n provide mode details on how exactly you think creating an alternate PlatformRecovery option would help, I wou= ld 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.gao@intel.com; ray.ni@intel.com; ard.biesheuvel@arm.c= om;
>> 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 B= oot 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 requireme= nts 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 c= onsole is actually usable during platform recovery, as some platforms do re= ly on the ConsolePrefDxe driver, which only performs console initialization= after ReadyToBoot is triggered.
>>
>> This patch fixes that behaviour by calling EfiSignalEventReadyToB= oot () in EfiBootManagerProcessLoadOption (), which is the function that se= ts up the platform recovery boot process.
>>
>> Signed-off-by: Pete Batard <pete@akeo.ie>
>> ---
>>    MdeModulePkg/Library/UefiBootManagerLib/BmLoadO= ption.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/BmLoadO= ption.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 sig= nalled  //
>> + 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.<= br> >>      //
>> --
>> 2.21.0.windows.1
>>
>>
>>
>>
>




--_000_BN6PR05MB3411C7F40F78D34CAC02EE3EB99A0BN6PR05MB3411namp_--