Thanks for your feedback! Comments inline Regards, Marvin.
> -----Original Message----- > From: Laszlo Ersek <ler...@redhat.com> > Sent: Tuesday, May 15, 2018 6:12 PM > To: Marvin Häuser <marvin.haeu...@outlook.com>; edk2- > de...@lists.01.org > Cc: gso...@gmail.com; ard.biesheu...@linaro.org; ruiyu...@intel.com; > eric.d...@intel.com; star.z...@intel.com; jordan.l.jus...@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 <ler...@redhat.com> > >> Sent: Tuesday, May 15, 2018 3:53 PM > >> To: Marvin Häuser <marvin.haeu...@outlook.com>; edk2- > >> de...@lists.01.org > >> Cc: eric.d...@intel.com; star.z...@intel.com; > >> jordan.l.jus...@intel.com; ard.biesheu...@linaro.org; > >> ruiyu...@intel.com; Gabriel L. Somlo (GMail) <gso...@gmail.com>; Gerd > >> Hoffmann <kra...@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. > > >> 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
