From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com [207.211.31.81]) by mx.groups.io with SMTP id smtpd.web09.12395.1573231088792605761 for ; Fri, 08 Nov 2019 08:38:09 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=J7vH6oCo; spf=pass (domain: redhat.com, ip: 207.211.31.81, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1573231087; 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=HfMdho5lwsEELKbKEBBDs1gVh5bRJnTJ1bDlOoFPFLo=; b=J7vH6oCoV5oUu9gKgLz1uDJhYgK6w91fLtLf7aEfbz/BQKQAB00Xj+c3fs0bpINqlThTG4 oNU2Hq1SnJFF9lg9Kan2Pzhtc8V3aC0/emF3cPILIFXj1HgVn6vZN8cZnWP530SaNb8Eug TZ438y2RGlh3hL+GAKf0Bwi70J28afY= 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-43-SjW7mBBzMV-0KRmxVlDu-w-1; Fri, 08 Nov 2019 11:38:03 -0500 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 8E2661800D7B; Fri, 8 Nov 2019 16:38:02 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-117-227.ams2.redhat.com [10.36.117.227]) by smtp.corp.redhat.com (Postfix) with ESMTP id 42DFC5C290; Fri, 8 Nov 2019 16:38:00 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH] Support skipping automatic BM enumeration To: Jeff Brasen , "Ni, Ray" , "devel@edk2.groups.io" , "afish@apple.com" Cc: Ashish Singhal , "Wang, Jian J" , "Wu, Hao A" , "Gao, Zhichao" , "Kinney, Michael D" References: <37D801DD-41E8-452E-9F24-ADF52BFDB676@apple.com> <72ce1d71-2a65-a6c0-1dd8-7628429c5a3c@redhat.com> <746A8D5E-DC45-4D39-9C4D-97A10BE2E0B0@apple.com> <734D49CCEBEEF84792F5B80ED585239D5C35125E@SHSMSX104.ccr.corp.intel.com> <734D49CCEBEEF84792F5B80ED585239D5C352ECA@SHSMSX104.ccr.corp.intel.com> <734D49CCEBEEF84792F5B80ED585239D5C352F7C@SHSMSX104.ccr.corp.intel.com> From: "Laszlo Ersek" Message-ID: <897d405a-b164-773c-5784-47fb8c4f9040@redhat.com> Date: Fri, 8 Nov 2019 17:37:59 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-MC-Unique: SjW7mBBzMV-0KRmxVlDu-w-1 X-Mimecast-Spam-Score: 0 Content-Language: en-US Content-Type: text/plain; charset=WINDOWS-1252 Content-Transfer-Encoding: quoted-printable On 11/07/19 18:46, Jeff Brasen wrote: > Fixing UiApp seems reasonable, I do think we would want a hook to the pl= atform library in here as the enumeration that occurs in the UiApp is inten= ded to do a full enumeration of the system and there may be platform specif= ics to how that occurs. Fully agreed -- entering UiApp should expose everything bootable in the system, unless (perhaps) PlatformBootManagerLib specifically thinks otherwise. Of course, then we arrive (again) at the problem that a call in UefiBootManagerLib, to a *new* PlatformBootManagerLib API, will break tens of out-of-tree platforms. :) I think that can be prevented, as follows; but it will take quite some tim= e: - introduce the new function declaration in "PlatformBootManagerLib.h", - modify all platforms (in tree and out of tree) to implement (define) the new function, - call the new function from UefiBootManagerLib For some history / background on this kind of problem, I suggest reading through: https://bugzilla.tianocore.org/show_bug.cgi?id=3D982 Thanks, Laszlo > From: Ni, Ray > Sent: Thursday, November 7, 2019 12:21 AM > To: devel@edk2.groups.io; Jeff Brasen ; afish@apple.= com > Cc: Ashish Singhal ; Laszlo Ersek ; Wang, Jian J ; Wu, Hao A ; Gao, Zhichao ; Kinney, Michael D > Subject: RE: [edk2-devel] [PATCH] Support skipping automatic BM enumerat= ion >=20 > I treat the issue in this way: >=20 > 1. Platform Boot Manager library does a good job. It doesn't always c= all RefreshAll() API to auto-create the boot options > 2. But UiApp doesn't. It constantly call RefreshAll(). >=20 > Do you think that we can fix UiApp instead? For example, introducing a P= CD to control the boot option refresh behavior? >=20 > Thanks, > Ray >=20 > From: devel@edk2.groups.io > On Behalf Of Jeff Brasen > Sent: Thursday, November 7, 2019 3:02 PM > To: Ni, Ray >; afish@apple.com= > Cc: devel@edk2.groups.io; Ashish Singhal >; Laszlo Ersek >; Wang, Jian J >; Wu, Hao A >; Gao, Zhichao >; Kinney, Michael D > > Subject: Re: [edk2-devel] [PATCH] Support skipping automatic BM enumerat= ion >=20 > The issue is there are some auto created options we do not want on our p= latform. > Get Outlook for Android >=20 > ________________________________ > From: Ni, Ray > > Sent: Wednesday, November 6, 2019 11:59:31 PM > To: Jeff Brasen >; afish@a= pple.com > > Cc: devel@edk2.groups.io >; Ashish Singhal >; Laszlo Ersek >; Wang, Jian J >; Wu, Hao A >; Gao,= Zhichao >; Kinney, Mic= hael D > > Subject: RE: [edk2-devel] [PATCH] Support skipping automatic BM enumerat= ion >=20 >=20 > Jeff, >=20 > RefreshAllBootOption() only modifies/creates the auto-created boot optio= ns. For the boot options created by platform boot manager library, they sta= y with no one touches. And all auto-created boot options are appended in th= e end of boot option list (through BootOrder). >=20 >=20 >=20 > From: Jeff Brasen > > Sent: Thursday, November 7, 2019 12:13 PM > To: afish@apple.com; Ni, Ray > > Cc: devel@edk2.groups.io; Ashish Singhal >; Laszlo Ersek >; Wang, Jian J >; Wu, Hao A >; Gao, Zhichao >; Kinney, Michael D > > Subject: RE: [edk2-devel] [PATCH] Support skipping automatic BM enumerat= ion >=20 >=20 >=20 > As the suggestions below made sense, we updated our platform boot manage= r library to behave in this manner and for normal boots everything works we= ll. However the UiApp and boot maintenance applications in EDK2 also call E= fiBootManagerRefreshAllBootOption() when ever a user goes into the menu whi= ch will re-create the skipped boot options with no place for the platform c= ode to intervene. >=20 >=20 >=20 > What about a solution where we add a new Platform library function that = allows for override of the behavior of BmEnumerateBootOptions? For example,= either a function or protocol that takes the same parameters as this funct= ion and only if it returns NULL then we continue to the default enumeration= code. Or a function call inserted at the end that would modify the load o= ption array after the system does the standard enumeration. >=20 >=20 >=20 > -Jeff >=20 >=20 >=20 > From: afish@apple.com > > Sent: Wednesday, November 6, 2019 9:20 AM > To: Ni, Ray > > Cc: devel@edk2.groups.io; Jeff Brasen >; Ashish Singhal >; Laszlo Ersek >; Wang, Jian J >; Wu, Hao A >; Gao, Zhichao >; Mi= ke Kinney > > Subject: Re: [edk2-devel] [PATCH] Support skipping automatic BM enumerat= ion >=20 >=20 >=20 > Ray, >=20 >=20 >=20 > Is there an obvious hook point we could point Jeff and Ashish at? >=20 >=20 >=20 > Long term it would be a good idea to have a Wiki page to give some guida= nce on how to customize the BDS. >=20 >=20 >=20 > Thanks, >=20 >=20 >=20 > Andrew Fish >=20 >=20 >=20 > On Nov 5, 2019, at 9:20 PM, Ni, Ray > wrote: >=20 >=20 >=20 > Andrew, >=20 > I agree with your opinion. >=20 > It's expected that Platform Boot Manager lib calls EfiBootManagerRefresh= AllBootOption() only in full configuration boot path. >=20 > The full configuration boot path is chosen when hardware changes happen.= So it's not expected EfiBootManagerRefresh...() be > called in every boot. >=20 > So you could: >=20 > 1. Delete the auto-created option pointing to LoadFile instance > 2. Create your own one with customized description. >=20 >=20 >=20 >=20 >=20 > From: afish@apple.com > > Sent: Wednesday, November 6, 2019 10:47 AM > To: devel@edk2.groups.io; jbrasen@nvidia.co= m > Cc: Ashish Singhal >; Laszlo Ersek >; Ni, Ray <= ray.ni@intel.com>; Wang, Jian J >; Wu, Hao A >; Gao, Zhichao >; Kinney, Michael D > > Subject: Re: [edk2-devel] [PATCH] Support skipping automatic BM enumerat= ion >=20 >=20 >=20 >=20 >=20 >=20 >=20 > On Nov 5, 2019, at 7:34 PM, Jeff Brasen > wrote: >=20 >=20 >=20 > Wouldn't having a variable that we create and delete on every boot put u= nnecessary stress on the SPI-NOR that the variable store lives on? > What about the alternative approach where we allow the platform code to = modify the attributes of the auto created variable to disable it with hidde= n/!active but still match for detection purposes so that it doesn't delete = and recreate the modified variable each boot? That way all the logic on wha= t to disable can still be in the platform code and all the existing logic i= n the boot manager can stay basically the same? >=20 > What changes every boot that forces the variable to need to get modified= ? >=20 > I would assume the NOR driver is smart enough to not update a variable t= hat is not changing. >=20 > The custom BDS could could only create the variable for this device if i= t does not exist. >=20 > [JB] The current flow with no changes in the boot manager would be as fo= llows >=20 >=20 >=20 > 1. Scan for instance of the boot option in the variables > 2. It will not be found, so create a new boot option store it to a va= riable and update BootOrder > 3. Platform code runs creates the options for the boot option it want= s and writes those to variable store > 4. Delete/disable the boot option in the variable store >=20 >=20 >=20 > When you reboot it won't find the variable so 1/2/4 will re-occur >=20 >=20 >=20 > The code that does this (1/2) is EfiBootManagerRefreshAllBootOption in B= mBoot.c >=20 >=20 >=20 > If you modify the variable to disable it with hidden/not active it would= delete that and create a new one as well as the code wouldn't recognize th= at is the same boot option. >=20 >=20 >=20 > If however we modify EfiBootManagerFindLoadOption to not compare the att= ributes (at least allow for differences in active and hidden) then the when= it refreshes every thing it would see the match and not delete/create a ne= w variable in the store and thus we wouldn't have changes every boot. >=20 >=20 >=20 >=20 >=20 > Jeff, >=20 >=20 >=20 > Sorry if I'm a little off on the sequence of things as the platform I wo= rk on day to day has a custom BDS and does not use this library..... I thou= gh the patch changed BmEnumerateBootOptions(), so that is going to change h= ow EfiBootManagerRefreshAllBootOption() works. I'd also point out the patch= as given is invalid as it changed the behavior of the public library API f= or EfiBootManagerRefreshAllBootOption() [1] so for the patch to be valid it= would need to change the comments to reflect the new behavior. This is kin= d of what Laszlo's technical debt comment was about. >=20 >=20 >=20 > I think Laszlo advocated having the BDS platform specific code make sure= the boot variables are in the correct state. That should happen before the= Boot Manager code runs, and it is not clear to me why the Boot Manager co= uld would need to run if you have a valid EFI nvram variable to boot from. >=20 >=20 >=20 > I think the question is how is your use case different than the boot var= iable that Windows installs? If it works kind of the same way then the answ= er is to have the BDS platform specific code write the boot variable. >=20 >=20 >=20 >=20 >=20 > [1] >=20 > /** >=20 > The function creates boot options for all possible bootable medias in = the following order: >=20 > 1. Removable BlockIo - The boot option only points to the r= emovable media >=20 > device, like USB key, DVD, Floppy et= c. >=20 > 2. Fixed BlockIo - The boot option only points to a Fix= ed blockIo device, >=20 > like HardDisk. >=20 > 3. Non-BlockIo SimpleFileSystem - The boot option points to a device s= upporting >=20 > SimpleFileSystem Protocol, but not s= upporting BlockIo >=20 > protocol. >=20 > 4. LoadFile - The boot option points to the media = supporting >=20 > LoadFile protocol. >=20 > Reference: UEFI Spec chapter 3.3 Boot Option Variables Default Boot Be= havior >=20 >=20 >=20 > The function won't delete the boot option not added by itself. >=20 > **/ >=20 > VOID >=20 > EFIAPI >=20 > EfiBootManagerRefreshAllBootOption ( >=20 > VOID >=20 > ); >=20 >=20 >=20 > Thanks, >=20 >=20 >=20 > Andrew Fish >=20 >=20 >=20 > Thanks, >=20 > Andrew Fish >=20 > Thanks, >=20 > Jeff >=20 > ________________________________ >=20 > This email message is for the sole use of the intended recipient(s) and = may contain confidential information. Any unauthorized review, use, disclo= sure or distribution is prohibited. If you are not the intended recipient,= please contact the sender by reply email and destroy all copies of the ori= ginal message. >=20 > ________________________________ >=20 >=20 >=20 >=20