Thanks for your comment, Comments are inline Regards, Marvin.
> -----Original Message----- > From: Ni, Ruiyu <[email protected]> > Sent: Thursday, May 17, 2018 9:58 AM > To: Marvin Häuser <[email protected]>; Laszlo Ersek > <[email protected]>; [email protected] > Cc: Dong, Eric <[email protected]>; [email protected]; Justen, > Jordan L <[email protected]>; [email protected]; Zeng, Star > <[email protected]> > Subject: RE: [edk2] Proposition of a BmEnumerateBootOptions() hook. > > > > Thanks/Ray > > > -----Original Message----- > > From: edk2-devel <[email protected]> On Behalf Of Marvin > > Häuser > > Sent: Wednesday, May 16, 2018 1:15 AM > > To: Laszlo Ersek <[email protected]>; [email protected] > > Cc: Ni, Ruiyu <[email protected]>; Dong, Eric <[email protected]>; > > [email protected]; Justen, Jordan L > > <[email protected]>; [email protected]; Zeng, Star > > <[email protected]> > > Subject: Re: [edk2] Proposition of a BmEnumerateBootOptions() hook. > > > > Thanks for your feedback! > > Comments inline > > > > Regards, > > Marvin. > > > > > -----Original Message----- > > > From: Laszlo Ersek <[email protected]> > > > Sent: Tuesday, May 15, 2018 6:12 PM > > > To: Marvin Häuser <[email protected]>; edk2- > > > [email protected] > > > Cc: [email protected]; [email protected]; [email protected]; > > > [email protected]; [email protected]; [email protected] > > > Subject: Re: [edk2] Proposition of a BmEnumerateBootOptions() hook. > > > > > > On 05/15/18 17:38, Marvin Häuser wrote: > > > >> -----Original Message----- > > > >> From: Laszlo Ersek <[email protected]> > > > >> Sent: Tuesday, May 15, 2018 3:53 PM > > > >> To: Marvin Häuser <[email protected]>; edk2- > > > >> [email protected] > > > >> Cc: [email protected]; [email protected]; > > > >> [email protected]; [email protected]; > > > >> [email protected]; Gabriel L. Somlo (GMail) <[email protected]>; > > > >> Gerd Hoffmann <[email protected]> > > > >> 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 > > [email protected] > > https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
