Re: Proposition of a BmEnumerateBootOptions() hook.

["Ni, Ruiyu" <ruiyu.ni@intel.com>]

Thanks/Ray

> -----Original Message-----
> From: edk2-devel <edk2-devel-bounces@lists.01.org> On Behalf Of Marvin
> Häuser
> Sent: Wednesday, May 16, 2018 1:15 AM
> To: Laszlo Ersek <lersek@redhat.com>; edk2-devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>;
> ard.biesheuvel@linaro.org; Justen, Jordan L <jordan.l.justen@intel.com>;
> gsomlo@gmail.com; Zeng, Star <star.zeng@intel.com>
> Subject: Re: [edk2] Proposition of a BmEnumerateBootOptions() hook.
> 
> Thanks for your feedback!
> Comments inline
> 
> Regards,
> Marvin.
> 
> > -----Original Message-----
> > From: Laszlo Ersek <lersek@redhat.com>
> > Sent: Tuesday, May 15, 2018 6:12 PM
> > To: Marvin Häuser <Marvin.Haeuser@outlook.com>; edk2-
> > devel@lists.01.org
> > Cc: gsomlo@gmail.com; ard.biesheuvel@linaro.org; ruiyu.ni@intel.com;
> > eric.dong@intel.com; star.zeng@intel.com; jordan.l.justen@intel.com
> > Subject: Re: [edk2] Proposition of a BmEnumerateBootOptions() hook.
> >
> > On 05/15/18 17:38, Marvin Häuser wrote:
> > >> -----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:
> >
> > >>> 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?
> >
> > See "3.3 Globally Defined Variables" and "Table 10. Global Variables"
> > in the
> > UEFI-2.7 spec:
> >
> >   [...] The variables with an attribute of NV are nonvolatile. [...]
> >
> >   [...]
> >
> >   Variable Name  Attribute   Description
> >   -------------  ---------   -----------
> >   [...]
> >   Boot####       NV, BS, RT  [...]
> >   [...]
> 
> I had hoped it was a recommendation, but indeed it seems to be mandatory.
> What's your opinion on this fact and how do you think the chances are NV
> could be made optional?
> 
> >
> > >> 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?
> >
> > Sure; it's a simple scenario: the user might want the system to
> > automatically boot off of a USB drive whenever they connect it, before
> > powering on or rebooting the system, rather than boot from the hard drive.
> 
> Remember that point was made in the context of enumeration. What I was
> saying is that boot options created by the enum code, for removable drives,
> are fine to be gone (as they are volatile, or proactively removed when NV)
> after a reboot.
> If a user wants to boot from such an USB device by default, in my opinion
> they should manually create a boot option for it. Otherwise, if I do not
> misunderstand your point, you are suggesting the firmware to keep track of
> every single bootable USB device ever attached.
> 
> >
> > (For USB devices, "USB WWID" and "USB Class" device path nodes are
> > defined by the spec, so the user isn't even expected to plug the drive
> > into the same USB port, for the drive to be found uniquely.)
> 
> Actually didn't know that, thanks for the fact!
> 
> >
> > >>> 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,
> >
> > I disagree. I'll have to defer to Ray here, but in my experience, edk2
> > practice is that if you have a boot option whose devpath ends in a
> > HD() node, then an UEFI image on that partition under the filepath
> > \EFI\BOOT\BOOT<arch>.efi will be tried for booting. As far as I can
> > see, this is neither mandated nor forbidden by the spec. Anyway, for Ray
> to answer authoritatively.
> 
> Indeed it is neither explicitly allowed nor forbidden, I concluded it being
> invalid from the strictness of how the booting protocol is defined.
> If it is forbidden, it is implicit, but if it's not the case, the better it is for this
> purpose.
> 
> >
> > > 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?
> >
> > Personally I remain unconvinced that the second hook is needed (to be
> > invoked from core BdsDxe or UefiBootManagerLib code). But, that's just
> > my opinion :)
> 
> Well, the only alternative I see are modifying the entry pre-BDS (slightly
> unclean), forking off (unpleasant) or not supporting it at all (unpleasant too,
> but support is not terribly important, although would be nice to have).
> 
> >
> > In general I find that library APIs (especially a *set* of library
> > APIs) are very hard to design (because we can't see the future). So,
> > "organic growth" works better in practice (even if it leads to "less
> > well organized" sets of APIs). This is to say that, if you can propose
> > a hook and immediately demonstrate that it saves code for, and/or
> > simplifies, multiple platforms, then the new API might be justified.
> (Personally, I usually argue for "three call sites" as a minimum.
> > Two might be enough if the code extracted is particularly baroque, or
> > undergoes frequent
> > changes.)
> 
> The "problem" is that both hooks are not introduced for sharing code but for
> giving the platform developer more freedom without forking off.
> Whether such are accepted ends up being entirely the maintainer's
> preference, I guess.

Forking off is never an option. In my opinion, if a generic library/driver needs
forking, that's a failure indicator of design.
Freedom is very important. I have seen many internal platform BDS
implementations. The platform requirements are very different and strange sometimes.
So the beforeconsole/afterconsole hook points were used.
They are very natural. First hook point is when the platform doesn't have mouth and ears.
The other hook point is when platform can speak and hear.

> 
> >
> > >> 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.
> >
> > I definitely intend to regression test any such changes :)
> 
> Sure, I just often come across a "I don't merge what I cannot test"
> mentality. :)
> 
> >
> > OTOH, in any larger project, any given maintainer is surely unable to
> > integration-test *all* features in the subsystem that they co-maintain.
> > What's important is that each major feature has at least one
> > maintainer that can integration-test it (possibly with the help of an
> automated test suite).
> >
> > For example, I totally cannot test Xen code in OvmfPkg or ArmVirtPkg,
> > but that's why we have several permanent Reviewers from the Xen
> > project, for OvmfPkg and ArmVirtPkg.
> >
> > Thanks,
> > Laszlo
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel