Re: Proposition of a BmEnumerateBootOptions() hook.

["Marvin Häuser" <Marvin.Haeuser@outlook.com>]

Hi Laszlo, thanks for your feedback!
Comments are inline.

I also CC'd Jordan and Ard because of OVMF references in the third comment.
Thanks in advance for your time!

I have rewritten a few parts multiple times, so, if something doesn't make sense (because I forgot to rework it in the context), please feel free to ask for clarification.

Best regards,
Marvin

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Tuesday, May 15, 2018 10:22 AM
> To: Marvin H?user <Marvin.Haeuser@outlook.com>; edk2-
> devel@lists.01.org
> Cc: eric.dong@intel.com; star.zeng@intel.com
> Subject: Re: [edk2] Proposition of a BmEnumerateBootOptions() hook.
> 
> On 05/14/18 21:00, Marvin H?user wrote:
> > Hey Star, Eric and everyone else,
> >
> > I have seen that some platforms add a Boot Option for the UEFI Shell
> > in "PlatformBootManagerBeforeConsole()", which is called as part of
> > the regular boot flow. This is surely beneficial for development
> > platforms that are supposed to boot to UEFI Shell by default when no
> > other option has been registered,
> 
> (Side point: I sure wish *all* platforms included the UEFI shell, one way or
> another. Debugging issues is the *default* state of any computing system
> (all software has bugs), so debug tools must be a first class citizen. Forums
> are full of end-users asking for the UEFI shell on their physical platforms;
> frequently because their platform firmware gives them an extremely limited
> interface to managing boot and driver options, and the shell is seen as a way
> out of that, justifiedly.)

Definitely, sadly capacity and OEM mentality are still issues.
My point is just that a common user should not land in UEFI Shell by accident, but it should be explicitly launched.

> 
> > however for retail platforms it usually makes more sense to show the
> > UEFI Boot Menu,
> 
> Note that this is already solved in BdsDxe (if I understand your point
> anyway); please refer to commit d1de487dd2e7 ("MdeModulePkg/BdsDxe:
> fall back to a Boot Manager Menu loop before hanging", 2017-11-27).

Oh interesting, thanks! That might resolve it, but I didn't check it out yet.

> 
> > which renders adding the Shell Boot Option as part of the regular boot
> > flow obsolete and just adds up to the boot time. Meanwhile, there is a
> > function in the UefiBootManagerLib, "BmEnumerateBootOptions()", which
> > is called prior to entering the Boot Menu and, in my opinion, would be
> > the perfect place to introduce another PlatformBootManagerLib hook,
> > which retrieves platform-specific boot options, such as an UEFI Shell
> > or other utilities like a Memory Test application.
> 
> Hmmm, I'm not sure. The only function that calls
> BmEnumerateBootOptions() is EfiBootManagerRefreshAllBootOption(). The
> latter is a public UefiBootManagerLib API that several
> PlatformBootManagerLib instances call, usually from
> PlatformBootManagerAfterConsole(). PlatformBootManagerAfterConsole()

I have misunderstood the current edk2 logic to be what I expected (partially), however I have just discovered that scanned boot options are added to NV and to BootOrder. Furthermore, I did not mention another usage case for the proposed hook as I thought it was practically irrelevant to the edk2 community: Supporting Apple's bless boot concept. (For those who wonder, virtualizing macOS on Apple hardware is perfectly legal). So, I will have to explain a bit more in detail, in two parts.

UEFI BOOT

What I imagined to be the boot flow, by the specification and by my personal taste, is the following:
1. Boot BootOrder[] one-by-one. This connects the referenced drive and attempts to start the specified file. This is the case today.
2. Boot *Recovery[]. This is the case today.
3. There are two alternatives here, based on platform preference.
3.1. Boot enumerated drives (via the algorithm defined in the UEFI spec). I think this is not the case today.
3.2. Enumerate the drives (via the algorithm defined in the UEFI spec) and show them in BootManagerMenuApp. I think this is done today and is what I would prefer too.

What I basically do not agree with in the list above are two nuances.
3.1 and 3.2: I would like to see support for a platform-defined logic of enumerating in addition to the one defined in the UEFI spec. This can be used for UEFI Shell, platform diagnostic apps and Apple bless.
3.2: I think adding them as volatile variables is the better approach, except for platform-specific code, which should be capable of adding NV options. The reasoning behind this is that usually the found volumes are removable volumes (such as USB Flash Drives) and will be cleaned away soon again anyway. "Extra wishes" for boot options should be added via the UEFI Setup menu or UEFI Shell.
The reason why I still wish for platform code to be able to add NV options as part of enumeration is that FV-embedded tools are indeed NV (Shell etc.) and will not need to be cleaned up, as well as that we're not in an utopia. Sometimes an unwanted purge of boot option happens ("CMOS reset" on boards, etc) and consumer platform provides do not want to leave their users with a constant boot to BootMenu or no option at all. Platform code often does currently and probably should add a NV Windows boot option in the proposed hook (this should of course not be part of edk2). Furthermore, if an OS boot option is rendered invalid, usually there is no OsRecovery option available as proposed in the specification.

Considering the boot protocol I just described, I think that the call in PlatformBootManagerAfterConsole() is not a good idea, because it refreshes boot options unconditionally. When the first element of BootOrder[] can be booted, there is no reason to do so and it costs time. Such a refresh makes sense when booting the first option fails though (to make sure it is purged to not delay boot again).
The function is also called by BootManagerMenuApp in the entry point, which makes a lot of sense. In the scenario described above, "scanned" boot options are added only when the NV ones cannot be booted and MenuApp is rendered in consequence. It basically moves the scanning delay from the "NV phase" to the following "scanning phase", which is not supposed to be entered when booting a "sane" platform.

APPLE BLESS

This might be interesting for the OVMF maintainers.
I did not want to mention this concept at first, because I don't think there is a terrible huge demand or interest, however I would like to be able to implement Apple bless support for OVMF without having to fork and modify edk2 drivers.
I did not check about a concrete way of implementation, however I will shortly describe what needs to be involved.

Secondary partitions: Supporting this will be easy when accepting the hook proposed above and considering my comments regarding NV vs volatile variables. No boot options are proactively added for those installs and they are scanned on demand, which can be done entirely in the proposed hook function.
Primary partition: The so-called "Startup Volume" unfortunately is a bit trickier. For it, a practically invalid Boot Option is added, which is an expanded device path to the volume to be booted, however without having a File Device Path Node appended. I had hoped to be able to use EFI_LOAD_FILE_PROTOCOL, however obviously EFI_SIMPLE_FILE_SYSTEM_PROTOCOL will attach to the mentioned DevicePath, which means LoadFile will not be called. Support for this would need to be done via a platform-specific error handler for when the DevicePath is determined to be invalid, which can then either fix it up and return success or fail as well. I have not looked into this in detail and I can understand if you are not interested in supporting such a scenario. However if you do, I will look into it as soon as possible and probably perform a few tests in OVMF.

> is already a PlatformBootManagerLib hook, so the suggestion would result in
> one hook calling back into another hook, of the same library instance:
> 
>   BdsEntry()                               [MdeModulePkg/Universal/BdsDxe/BdsEntry.c]
>     PlatformBootManagerAfterConsole()
> [SomePlatformPkg/Library/PlatformBootManagerLib/...]
>       EfiBootManagerRefreshAllBootOption()
> [MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c]
>         BmEnumerateBootOptions()
> [MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c]
>           PlatformBootManagerAnotherHook()
> [SomePlatformPkg/Library/PlatformBootManagerLib/...]
> 
> If I'm being honest, I'd like to avoid this -- if I'm in
> PlatformBootManagerBeforeConsole() or
> PlatformBootManagerAfterConsole(),
> I'd just like to do whatever's necessary by directly calling either public
> UefiBootManagerLib APIs, or STATIC functions from the same
> PlatformBootManagerLib instance that I'm already inside of.
> 
> Now, what I could see as useful is a public helper function in
> UefiBootManagerLib that registers the shell boot option. This functionality is
> usually duplicated across several PlatformBootManagerLib instances.

I'm not sure if this is needed, but I guess it could help sharing code.

> 
> I'll also mention another interface that the edk2-platforms project uses
> -- several platforms there use the same PlatformBootManagerLib instance,
> and they abstract out just the default set of platform boot options.
> Please see
> 
>   [PATCH v4 0/2] add platform boot manager protocol
>   http://mid.mail-archive.com/1524464514-14454-1-git-send-email-
> haojian.zhuang@linaro.org

I'm not sure whether a protocol is the best way to go, as I suppose only BDS is using it in the end.

> 
> Thanks
> Laszlo