Re: Proposition of a BmEnumerateBootOptions() hook.

[Laszlo Ersek <lersek@redhat.com>]

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  [...]
  [...]

>> 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.

(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.)

>>> 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.

> 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 :)

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.)

>> 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 :)

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