On 11/07/19 18:46, Jeff Brasen wrote: > Fixing UiApp seems reasonable, I do think we would want a hook to the > platform library in here as the enumeration that occurs in the UiApp is > intended to do a full enumeration of the system and there may be platform > specifics 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 time: - 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=982 Thanks, Laszlo > From: Ni, Ray <ray...@intel.com> > Sent: Thursday, November 7, 2019 12:21 AM > To: devel@edk2.groups.io; Jeff Brasen <jbra...@nvidia.com>; af...@apple.com > Cc: Ashish Singhal <ashishsin...@nvidia.com>; Laszlo Ersek > <ler...@redhat.com>; Wang, Jian J <jian.j.w...@intel.com>; Wu, Hao A > <hao.a...@intel.com>; Gao, Zhichao <zhichao....@intel.com>; Kinney, Michael D > <michael.d.kin...@intel.com> > Subject: RE: [edk2-devel] [PATCH] Support skipping automatic BM enumeration > > I treat the issue in this way: > > 1. Platform Boot Manager library does a good job. It doesn't always call > RefreshAll() API to auto-create the boot options > 2. But UiApp doesn't. It constantly call RefreshAll(). > > Do you think that we can fix UiApp instead? For example, introducing a PCD to > control the boot option refresh behavior? > > Thanks, > Ray > > From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> > <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of Jeff Brasen > Sent: Thursday, November 7, 2019 3:02 PM > To: Ni, Ray <ray...@intel.com<mailto:ray...@intel.com>>; > af...@apple.com<mailto:af...@apple.com> > Cc: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Ashish Singhal > <ashishsin...@nvidia.com<mailto:ashishsin...@nvidia.com>>; Laszlo Ersek > <ler...@redhat.com<mailto:ler...@redhat.com>>; Wang, Jian J > <jian.j.w...@intel.com<mailto:jian.j.w...@intel.com>>; Wu, Hao A > <hao.a...@intel.com<mailto:hao.a...@intel.com>>; Gao, Zhichao > <zhichao....@intel.com<mailto:zhichao....@intel.com>>; Kinney, Michael D > <michael.d.kin...@intel.com<mailto:michael.d.kin...@intel.com>> > Subject: Re: [edk2-devel] [PATCH] Support skipping automatic BM enumeration > > The issue is there are some auto created options we do not want on our > platform. > Get Outlook for Android<https://aka.ms/ghei36> > > ________________________________ > From: Ni, Ray <ray...@intel.com<mailto:ray...@intel.com>> > Sent: Wednesday, November 6, 2019 11:59:31 PM > To: Jeff Brasen <jbra...@nvidia.com<mailto:jbra...@nvidia.com>>; > af...@apple.com<mailto:af...@apple.com> > <af...@apple.com<mailto:af...@apple.com>> > Cc: devel@edk2.groups.io<mailto:devel@edk2.groups.io> > <devel@edk2.groups.io<mailto:devel@edk2.groups.io>>; Ashish Singhal > <ashishsin...@nvidia.com<mailto:ashishsin...@nvidia.com>>; Laszlo Ersek > <ler...@redhat.com<mailto:ler...@redhat.com>>; Wang, Jian J > <jian.j.w...@intel.com<mailto:jian.j.w...@intel.com>>; Wu, Hao A > <hao.a...@intel.com<mailto:hao.a...@intel.com>>; Gao, Zhichao > <zhichao....@intel.com<mailto:zhichao....@intel.com>>; Kinney, Michael D > <michael.d.kin...@intel.com<mailto:michael.d.kin...@intel.com>> > Subject: RE: [edk2-devel] [PATCH] Support skipping automatic BM enumeration > > > Jeff, > > RefreshAllBootOption() only modifies/creates the auto-created boot options. > For the boot options created by platform boot manager library, they stay with > no one touches. And all auto-created boot options are appended in the end of > boot option list (through BootOrder). > > > > From: Jeff Brasen <jbra...@nvidia.com<mailto:jbra...@nvidia.com>> > Sent: Thursday, November 7, 2019 12:13 PM > To: af...@apple.com<mailto:af...@apple.com>; Ni, Ray > <ray...@intel.com<mailto:ray...@intel.com>> > Cc: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Ashish Singhal > <ashishsin...@nvidia.com<mailto:ashishsin...@nvidia.com>>; Laszlo Ersek > <ler...@redhat.com<mailto:ler...@redhat.com>>; Wang, Jian J > <jian.j.w...@intel.com<mailto:jian.j.w...@intel.com>>; Wu, Hao A > <hao.a...@intel.com<mailto:hao.a...@intel.com>>; Gao, Zhichao > <zhichao....@intel.com<mailto:zhichao....@intel.com>>; Kinney, Michael D > <michael.d.kin...@intel.com<mailto:michael.d.kin...@intel.com>> > Subject: RE: [edk2-devel] [PATCH] Support skipping automatic BM enumeration > > > > As the suggestions below made sense, we updated our platform boot manager > library to behave in this manner and for normal boots everything works well. > However the UiApp and boot maintenance applications in EDK2 also call > EfiBootManagerRefreshAllBootOption() when ever a user goes into the menu > which will re-create the skipped boot options with no place for the platform > code to intervene. > > > > 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 function > 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 option > array after the system does the standard enumeration. > > > > -Jeff > > > > From: af...@apple.com<mailto:af...@apple.com> > <af...@apple.com<mailto:af...@apple.com>> > Sent: Wednesday, November 6, 2019 9:20 AM > To: Ni, Ray <ray...@intel.com<mailto:ray...@intel.com>> > Cc: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Jeff Brasen > <jbra...@nvidia.com<mailto:jbra...@nvidia.com>>; Ashish Singhal > <ashishsin...@nvidia.com<mailto:ashishsin...@nvidia.com>>; Laszlo Ersek > <ler...@redhat.com<mailto:ler...@redhat.com>>; Wang, Jian J > <jian.j.w...@intel.com<mailto:jian.j.w...@intel.com>>; Wu, Hao A > <hao.a...@intel.com<mailto:hao.a...@intel.com>>; Gao, Zhichao > <zhichao....@intel.com<mailto:zhichao....@intel.com>>; Mike Kinney > <michael.d.kin...@intel.com<mailto:michael.d.kin...@intel.com>> > Subject: Re: [edk2-devel] [PATCH] Support skipping automatic BM enumeration > > > > Ray, > > > > Is there an obvious hook point we could point Jeff and Ashish at? > > > > Long term it would be a good idea to have a Wiki page to give some guidance > on how to customize the BDS. > > > > Thanks, > > > > Andrew Fish > > > > On Nov 5, 2019, at 9:20 PM, Ni, Ray > <ray...@intel.com<mailto:ray...@intel.com>> wrote: > > > > Andrew, > > I agree with your opinion. > > It's expected that Platform Boot Manager lib calls > EfiBootManagerRefreshAllBootOption() only in full configuration boot path. > > The full configuration boot path is chosen when hardware changes happen. So > it's not expected EfiBootManagerRefresh...() be > called in every boot. > > So you could: > > 1. Delete the auto-created option pointing to LoadFile instance > 2. Create your own one with customized description. > > > > > > From: af...@apple.com<mailto:af...@apple.com> > <af...@apple.com<mailto:af...@apple.com>> > Sent: Wednesday, November 6, 2019 10:47 AM > To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; > jbra...@nvidia.com<mailto:jbra...@nvidia.com> > Cc: Ashish Singhal <ashishsin...@nvidia.com<mailto:ashishsin...@nvidia.com>>; > Laszlo Ersek <ler...@redhat.com<mailto:ler...@redhat.com>>; Ni, Ray > <ray...@intel.com<mailto:ray...@intel.com>>; Wang, Jian J > <jian.j.w...@intel.com<mailto:jian.j.w...@intel.com>>; Wu, Hao A > <hao.a...@intel.com<mailto:hao.a...@intel.com>>; Gao, Zhichao > <zhichao....@intel.com<mailto:zhichao....@intel.com>>; Kinney, Michael D > <michael.d.kin...@intel.com<mailto:michael.d.kin...@intel.com>> > Subject: Re: [edk2-devel] [PATCH] Support skipping automatic BM enumeration > > > > > > > > On Nov 5, 2019, at 7:34 PM, Jeff Brasen > <jbra...@nvidia.com<mailto:jbra...@nvidia.com>> wrote: > > > > Wouldn't having a variable that we create and delete on every boot put > unnecessary 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 > hidden/!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 what to disable can still be in the platform code and all the existing > logic in the boot manager can stay basically the same? > > What changes every boot that forces the variable to need to get modified? > > I would assume the NOR driver is smart enough to not update a variable that > is not changing. > > The custom BDS could could only create the variable for this device if it > does not exist. > > [JB] The current flow with no changes in the boot manager would be as follows > > > > 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 > variable and update BootOrder > 3. Platform code runs creates the options for the boot option it wants and > writes those to variable store > 4. Delete/disable the boot option in the variable store > > > > When you reboot it won't find the variable so 1/2/4 will re-occur > > > > The code that does this (1/2) is EfiBootManagerRefreshAllBootOption in > BmBoot.c > > > > 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 that > is the same boot option. > > > > If however we modify EfiBootManagerFindLoadOption to not compare the > attributes (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 > new variable in the store and thus we wouldn't have changes every boot. > > > > > > Jeff, > > > > Sorry if I'm a little off on the sequence of things as the platform I work on > day to day has a custom BDS and does not use this library..... I though the > patch changed BmEnumerateBootOptions(), so that is going to change how > EfiBootManagerRefreshAllBootOption() works. I'd also point out the patch as > given is invalid as it changed the behavior of the public library API for > EfiBootManagerRefreshAllBootOption() [1] so for the patch to be valid it > would need to change the comments to reflect the new behavior. This is kind > of what Laszlo's technical debt comment was about. > > > > 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 could > would need to run if you have a valid EFI nvram variable to boot from. > > > > I think the question is how is your use case different than the boot variable > that Windows installs? If it works kind of the same way then the answer is to > have the BDS platform specific code write the boot variable. > > > > > > [1] > > /** > > The function creates boot options for all possible bootable medias in the > following order: > > 1. Removable BlockIo - The boot option only points to the > removable media > > device, like USB key, DVD, Floppy etc. > > 2. Fixed BlockIo - The boot option only points to a Fixed > blockIo device, > > like HardDisk. > > 3. Non-BlockIo SimpleFileSystem - The boot option points to a device > supporting > > SimpleFileSystem Protocol, but not > supporting BlockIo > > protocol. > > 4. LoadFile - The boot option points to the media > supporting > > LoadFile protocol. > > Reference: UEFI Spec chapter 3.3 Boot Option Variables Default Boot Behavior > > > > The function won't delete the boot option not added by itself. > > **/ > > VOID > > EFIAPI > > EfiBootManagerRefreshAllBootOption ( > > VOID > > ); > > > > Thanks, > > > > Andrew Fish > > > > Thanks, > > Andrew Fish > > Thanks, > > Jeff > > ________________________________ > > This email message is for the sole use of the intended recipient(s) and may > contain confidential information. Any unauthorized review, use, disclosure > or distribution is prohibited. If you are not the intended recipient, please > contact the sender by reply email and destroy all copies of the original > message. > > ________________________________ > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#50307): https://edk2.groups.io/g/devel/message/50307 Mute This Topic: https://groups.io/mt/39747302/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
