Re: Proposition of a BmEnumerateBootOptions() hook.

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

Thanks for your comment,
Comments are inline

Regards,
Marvin.

> -----Original Message-----
> From: Ni, Ruiyu <ruiyu.ni@intel.com>
> Sent: Thursday, May 17, 2018 9:58 AM
> To: Marvin Häuser <Marvin.Haeuser@outlook.com>; Laszlo Ersek
> <lersek@redhat.com>; edk2-devel@lists.01.org
> Cc: 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/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.

I think so too, hence my proposition for PlatformAcpiLib a few days ago. However the failure of design can be with both the generic driver and the platform side.
For the first hook, I believe it's a design flaw of edk2 because there is no (known to me) way of adding boot options "on demand", but just via the Before/AfterConsole hooks, which execute unconditioned.
Regarding the second hook - I do not like the idea myself, but it is the only I can think of to support the described scenario properly.

> 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