Re: Proposition of a BmEnumerateBootOptions() hook.

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

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Tuesday, May 15, 2018 3:53 PM
> To: Marvin Häuser <Marvin.Haeuser@outlook.com>; edk2-
> devel@lists.01.org
> Cc: eric.dong@intel.com; star.zeng@intel.com; jordan.l.justen@intel.com;
> ard.biesheuvel@linaro.org; ruiyu.ni@intel.com; Gabriel L. Somlo (GMail)
> <gsomlo@gmail.com>; Gerd Hoffmann <kraxel@redhat.com>
> Subject: Re: [edk2] Proposition of a BmEnumerateBootOptions() hook.
> 
> On 05/15/18 15:02, Marvin Häuser wrote:
> >> -----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:
> >>> [...] 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.
> 
> This is not a coincidence. That's what the
> EfiBootManagerRefreshAllBootOption() API does, by design, and that's the
> functionality for which platforms call the API.
> 
> > 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.
> 
> I think platform-specific boot option generation -- enumeration of various
> protocols that stand for different kinds of devices, and generating boot
> options from them -- is already possible today.
> Platforms call EfiBootManagerRefreshAllBootOption() because (a) it's easy to
> call, (b) it generally does the right thing.

It surely is possible today, the proposition is more concerned about performance and "best practice".
I simply don't see a good reason to perform scanning and boot option writing when there is no code to read that (yet).
Of course this makes it come down to maintainer's preference because there is no significant demand.

> 
> (Note that this API does not connect devices; it just scans devices and turns
> whatever it finds into boot options.)
> 
> Nothing stops a platform from *not* calling
> EfiBootManagerRefreshAllBootOption() -- the platform can simply skip that
> and generate other options. Or the platform may generate options in
> addition to those that are generated by the API.

That is perfectly valid for the Status Quo, however if my NV vs volatile comment should be considered, it will not be because it relies on a proactive BootMenuApp call (which happens to be to EfiBootManagerRefreshAllBootOption()).

> 
> > 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.
> 
> I can't really comment on this -- the fact that Boot#### options are non-
> volatile comes from the UEFI spec. I'm not immediately sure what I think of
> their suggested volatility, but this is likely something for the USWG to discuss.

I think I might have been unclear here, I was solely referring to the Boot options created by the enumeration process and not Boot#### in general.
I wouldn't know of anything restricting all Boot#### variables to be NV, do you happen to have a quote?

> 
> > 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).
> 
> I think there are two possibilities for disagreement here. First, if the first boot
> option cares about the full set of Boot#### options (for whatever reasons),
> then this is an incompatible change.

This is definitely true, however I'm not sure about the difference "all currently valid options" and "all currently set options".
If that boot option happens to be a Boot Menu editor of some sort, the proposed behavior would remain fine, in my opinion.
Otherwise, you're right with your comment, such as if a custom Boot Menu GUI is invoked and relies on all Boot#### to be set.
I don't actually think the specification demands all options to be up-to-date, so maybe the idea of a protocol handling such things is great after all?

> 
> Second, just because a boot option fails, it should not be removed. For
> example, a netboot option could fail, or a USB drive might not be connected
> (temporarily). I don't think that's grounds enough for summarily removing a
> boot option, in shared reference code.

Oh, I was just talking about the possibility, because the current code does remove options under certain conditions.
However, for USB devices as you have mentioned, I do see this good practice so long as the option is volatile.
There is no point in exposing a boot option to a removable device that is not attached if not to prevent unnecessary Flash cycles as far as I can think.
Do you happen to have worries about a scenario I did not consider?

> 
> > 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.
> 
> For OVMF specifically, calling EfiBootManagerRefreshAllBootOption() at
> every boot is necessary. The set of bootable devices and their desired boot
> order may change at every boot, and the boot order spec that comes from
> QEMU isn't expressive enough for the *generation* of boot options; it's
> expressive enough only for the *filtering* and reordering of boot options.

Sure, that comment was mostly for end-user platforms.

> 
> > 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.
> 
> I think it could also be done in AfterConsole(). I realize it might incur more
> pflash traffic -- like any other Boot#### option generation
> -- than what you might consider optimal, but functionally that shouldn't be a
> barrier.

Yes, those options would basically to be rewritten or at least verified on every boot (which requires a boot option refresh).
Verification has the disadvantage that it might catch user-created options, however I saw the BM code attaching own OptionalData, which might be sufficient to distinguish.

> 
> > 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.
> 
> This doesn't immediately seem invalid -- if memory serves, you can have
> \EFI\BOOT\BOOT<arch>.efi on fixed media as well, and if a boot option only
> names the HD() in question, that default boot path will be launched off of it.

I think for a Boot#### option, it is always invalid, because the default path is defined to be used in the "scanning phase" once no Boot#### variable could be found.
Also, unfortunately the path is also stored via bless and definitely never the UEFI standard one, so supporting this vendor-specific scenario definitely requires special handling.
Do you have any opinion on the proposed second hook?

> 
> > 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.
> 
> I have a more general comment in the end: I doubt I could legally test an OSX
> guest on non-Apple hardware, so I won't try (and I'll most likely not buy or
> otherwise procure OSX, let alone Apple hardware, just for this). That means,
> if you post patches, my main focus will be on keeping the current behavior
> regression-free, and (secondarily) the implementation preferably simple &
> separated.

Thank you that you are ready to merge changes you cannot even really test.
This is probably off to some point in the future to make sure to supply well-tested patches.

> 
> I've added Gabriel and Gerd to the CC list because I believe they might be
> interested in OSX guests (I seem to remember that a sizeable out-of-tree
> patchset is necessary for OSX guests anyway).
> 
> Thanks,
> Laszlo