Re: [edk2] Proposition of a BmEnumerateBootOptions() hook.

2018-05-17 Thread Marvin Häuser
Thanks for your comment,
Comments are inline

Regards,
Marvin.

> -Original Message-
> From: Ni, Ruiyu 
> Sent: Thursday, May 17, 2018 9:58 AM
> To: Marvin Häuser ; Laszlo Ersek
> ; edk2-devel@lists.01.org
> Cc: Dong, Eric ; ard.biesheu...@linaro.org; Justen,
> Jordan L ; gso...@gmail.com; Zeng, Star
> 
> Subject: RE: [edk2] Proposition of a BmEnumerateBootOptions() hook.
> 
> 
> 
> Thanks/Ray
> 
> > -Original Message-
> > From: edk2-devel  On Behalf Of Marvin
> > Häuser
> > Sent: Wednesday, May 16, 2018 1:15 AM
> > To: Laszlo Ersek ; edk2-devel@lists.01.org
> > Cc: Ni, Ruiyu ; Dong, Eric ;
> > ard.biesheu...@linaro.org; Justen, Jordan L
> > ; gso...@gmail.com; Zeng, Star
> > 
> > Subject: Re: [edk2] Proposition of a BmEnumerateBootOptions() hook.
> >
> > Thanks for your feedback!
> > Comments inline
> >
> > Regards,
> > Marvin.
> >
> > > -Original Message-
> > > From: Laszlo Ersek 
> > > Sent: Tuesday, May 15, 2018 6:12 PM
> > > To: Marvin Häuser ; 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 
> > > >> Sent: Tuesday, May 15, 2018 3:53 PM
> > > >> To: Marvin Häuser ; 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) ;
> > > >> Gerd Hoffmann 
> > > >> 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.
> > > > 

Re: [edk2] Proposition of a BmEnumerateBootOptions() hook.

2018-05-17 Thread Ni, Ruiyu


Thanks/Ray

> -Original Message-
> From: edk2-devel  On Behalf Of Marvin
> Häuser
> Sent: Wednesday, May 16, 2018 1:15 AM
> To: Laszlo Ersek ; edk2-devel@lists.01.org
> Cc: Ni, Ruiyu ; Dong, Eric ;
> ard.biesheu...@linaro.org; Justen, Jordan L ;
> gso...@gmail.com; Zeng, Star 
> Subject: Re: [edk2] Proposition of a BmEnumerateBootOptions() hook.
> 
> Thanks for your feedback!
> Comments inline
> 
> Regards,
> Marvin.
> 
> > -Original Message-
> > From: Laszlo Ersek 
> > Sent: Tuesday, May 15, 2018 6:12 PM
> > To: Marvin Häuser ; 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 
> > >> Sent: Tuesday, May 15, 2018 3:53 PM
> > >> To: Marvin Häuser ; 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) ;
> > >> Gerd Hoffmann 
> > >> 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.

Re: [edk2] Proposition of a BmEnumerateBootOptions() hook.

2018-05-15 Thread Laszlo Ersek
On 05/15/18 19:14, Marvin Häuser wrote:
>> -Original Message-
>> From: Laszlo Ersek 
>> Sent: Tuesday, May 15, 2018 6:12 PM
>> To: Marvin Häuser ; 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.

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

I have always accepted it as a fact, and haven't really thought about
volatile Boot options ever :)

> and how do you think the chances are NV could be made optional?

I wouldn't like to guess :)

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

Thanks for the clarification.

I see this all as platform policy. It's up to the platform to
auto-generate a boot option for a USB drive (either by calling
EfiBootManagerRefreshAllBootOption() or by other means). It's also up to
the platform to remove preexistent non-volatile options that it deems
unnecessary, or wrong.

I'm not suggesting that the platform keep track of every single bootable
USB device ever attached. The scenario I described was indeed that the
user once adds such a boot option manually, and then the system should
likely not remove it automatically. I see now that your point was about
auto-generated options only. About those, I think that if a platform
calls EfiBootManagerRefreshAllBootOption(), then the platform is also
responsible for pruning the auto-generated options that are no longer
valid.

In OVMF we do both the auto-generation and the (quite heavy-handed!)
pruning as well. It's mostly very platform-specific code. One example
that I can name without needing many details is the
RemoveStaleFvFileOptions() function.

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

You could show the maintainer the two implementations side-by-side: what
it takes for the platform in question to implement the desired
functionality without the hook (if that's possible at all), and with the
hook. Double work, yes, but sometimes nothing else enlightens
maintainers. (I know this from both the contributor and the maintainer
sides.)

Thanks,
Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel


Re: [edk2] Proposition of a BmEnumerateBootOptions() hook.

2018-05-15 Thread Marvin Häuser
Thanks for your feedback!
Comments inline

Regards,
Marvin.

> -Original Message-
> From: Laszlo Ersek 
> Sent: Tuesday, May 15, 2018 6:12 PM
> To: Marvin Häuser ; 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 
> >> Sent: Tuesday, May 15, 2018 3:53 PM
> >> To: Marvin Häuser ; 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) ; Gerd
> >> Hoffmann 
> >> 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.
> >>
> &

Re: [edk2] Proposition of a BmEnumerateBootOptions() hook.

2018-05-15 Thread Laszlo Ersek
On 05/15/18 17:38, Marvin Häuser wrote:
>> -Original Message-
>> From: Laszlo Ersek 
>> Sent: Tuesday, May 15, 2018 3:53 PM
>> To: Marvin Häuser ; 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)
>> ; Gerd Hoffmann 
>> 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.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.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 AP

Re: [edk2] Proposition of a BmEnumerateBootOptions() hook.

2018-05-15 Thread Marvin Häuser
> -Original Message-
> From: Laszlo Ersek 
> Sent: Tuesday, May 15, 2018 3:53 PM
> To: Marvin Häuser ; 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)
> ; Gerd Hoffmann 
> Subject: Re: [edk2] Proposition of a BmEnumerateBootOptions() hook.
> 
> On 05/15/18 15:02, Marvin Häuser wrote:
> >> -Original Message-
> >> From: Laszlo Ersek 
> >> Sent: Tuesday, May 15, 2018 10:22 AM
> >> To: Marvin Häuser ; edk2-
> >> de...@lists.01.org
> >> Cc: eric.d...@intel.com; star.z...@intel.com
> >> Subject: Re: [edk2] Proposition of a BmEnumerateBootOptions() hook.
> >>
> >> On 05/14/18 21:00, Marvin Häuser wrote:
> >>> [...] there is a
> >>> function in the UefiBootManagerLib, "BmEnumerateBootOptions()",
> >>> which is called prior to entering the Boot Menu and, in my opinion,
> >>> would be the perfect place to introduce another
> >>> PlatformBootManagerLib hook, which retrieves platform-specific boot
> >>> options, such as an UEFI Shell or other utilities like a Memory Test
> >>> application.
> >>
> >> Hmmm, I'm not sure. The only function that calls
> >> BmEnumerateBootOptions() is EfiBootManagerRefreshAllBootOption().
> The
> >> latter is a public UefiBootManagerLib API that several
> >> PlatformBootManagerLib instances call, usually from
> >> PlatformBootManagerAfterConsole().
> PlatformBootManagerAfterConsole()
> >
> > I have misunderstood the current edk2 logic to be what I expected
> > (partially), however I have just discovered that scanned boot options
> > are added to NV and to BootOrder.
> 
> This is not a coincidence. That's what the
> EfiBootManagerRefreshAllBootOption() API does, by design, and that's the
> functionality for which platforms call the API.
> 
> > Furthermore, I did not mention another usage case for the proposed
> > hook as I thought it was practically irrelevant to the edk2 community:
> > Supporting Apple's bless boot concept. (For those who wonder,
> > virtualizing macOS on Apple hardware is perfectly legal). So, I will
> > have to explain a bit more in detail, in two parts.
> >
> > UEFI BOOT
> >
> > What I imagined to be the boot flow, by the specification and by my
> > personal taste, is the following:
> > 1. Boot BootOrder[] one-by-one. This connects the referenced drive and
> >attempts to start the specified file. This is the case today.
> > 2. Boot *Recovery[]. This is the case today.
> > 3. There are two alternatives here, based on platform preference.
> > 3.1. Boot enumerated drives (via the algorithm defined in the UEFI
> >  spec). I think this is not the case today.
> > 3.2. Enumerate the drives (via the algorithm defined in the UEFI spec)
> >  and show them in BootManagerMenuApp. I think this is done today
> >  and is what I would prefer too.
> >
> > What I basically do not agree with in the list above are two nuances.
> > 3.1 and 3.2: I would like to see support for a platform-defined logic
> > of enumerating in addition to the one defined in the UEFI spec. This
> > can be used for UEFI Shell, platform diagnostic apps and Apple bless.
> 
> I think platform-specific boot option generation -- enumeration of various
> protocols that stand for different kinds of devices, and generating boot
> options from them -- is already possible today.
> Platforms call EfiBootManagerRefreshAllBootOption() because (a) it's easy to
> call, (b) it generally does the right thing.

It surely is possible today, the proposition is more concerned about 
performance and "best practice".
I simply don't see a good reason to perform scanning and boot option writing 
when there is no code to read that (yet).
Of course this makes it come down to maintainer's preference because there is 
no significant demand.

> 
> (Note that this API does not connect devices; it just scans devices and turns
> whatever it finds into boot options.)
> 
> Nothing stops a platform from *not* calling
> EfiBootManagerRefreshAllBootOption() -- the platform can simply skip that
> and generate other options. Or the platform may generate options in
> addition to those that are generated by the API.

That is perfectly valid for the Status Quo, however if my NV vs volatile 
comment should be considered, it will not be because it relies on a proactive 
BootMenuApp call (which happens to be to EfiBootManagerRefreshAllBootOption()).

> 
> > 3.2: 

Re: [edk2] Proposition of a BmEnumerateBootOptions() hook.

2018-05-15 Thread Gabriel L. Somlo
On Tue, May 15, 2018 at 03:52:34PM +0200, Laszlo Ersek wrote:
>
> [...]
>
> > APPLE BLESS
> >
> > This might be interesting for the OVMF maintainers.
> > I did not want to mention this concept at first, because I don't think
> > there is a terrible huge demand or interest, however I would like to
> > be able to implement Apple bless support for OVMF without having to
> > fork and modify edk2 drivers.
> > I did not check about a concrete way of implementation, however I will
> > shortly describe what needs to be involved.
> >
> > Secondary partitions: Supporting this will be easy when accepting the
> > hook proposed above and considering my comments regarding NV vs
> > volatile variables. No boot options are proactively added for those
> > installs and they are scanned on demand, which can be done entirely in
> > the proposed hook function.
> 
> I think it could also be done in AfterConsole(). I realize it might
> incur more pflash traffic -- like any other Boot option generation
> -- than what you might consider optimal, but functionally that shouldn't
> be a barrier.
> 
> > 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.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 had hoped to be able to use EFI_LOAD_FILE_PROTOCOL, however
> > obviously EFI_SIMPLE_FILE_SYSTEM_PROTOCOL will attach to the mentioned
> > DevicePath, which means LoadFile will not be called. Support for this
> > would need to be done via a platform-specific error handler for when
> > the DevicePath is determined to be invalid, which can then either fix
> > it up and return success or fail as well. I have not looked into this
> > in detail and I can understand if you are not interested in supporting
> > such a scenario. However if you do, I will look into it as soon as
> > possible and probably perform a few tests in OVMF.
> 
> 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.
> 
> I've added Gabriel and Gerd to the CC list because I believe they might
> be interested in OSX guests (I seem to remember that a sizeable
> out-of-tree patchset is necessary for OSX guests anyway).

For whatever it's worth:

The size of the out-of-tree patchset (available at github.com/gsomlo/edk2,
branches gls-hfsplus -> gls-macboot -> gls-miscopt, with the latter
containing everything, cumulatively) is mainly due to the HFS+ driver
needed to access OSX disk images. The 'macboot' bits are from a GSoC
project by Reza Jelveh, and I haven't had a chance to really understand
how they work yet, since I think HFS+ support in a form acceptable to EDK2
are the bigger priority (and "gls-hfsplus" is not in a form acceptable
to EDK2 at the moment :)

Anyhow, the patched OVMF can boot Sierra right now, there's an
only-slightly-outdated writeup about it at
www.contrib.andrew.cmu.edu/~somlo/OSXKVM/

My long-term wish is to get everything cleaned up and palatable for
upstream inclusion, but haven't found time to really get into it over
the last couple of years.

Cheers,
--Gabriel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel


Re: [edk2] Proposition of a BmEnumerateBootOptions() hook.

2018-05-15 Thread Laszlo Ersek
On 05/15/18 15:02, Marvin Häuser wrote:
>> -Original Message-
>> From: Laszlo Ersek 
>> Sent: Tuesday, May 15, 2018 10:22 AM
>> To: Marvin Häuser ; edk2-
>> de...@lists.01.org
>> Cc: eric.d...@intel.com; star.z...@intel.com
>> Subject: Re: [edk2] Proposition of a BmEnumerateBootOptions() hook.
>>
>> On 05/14/18 21:00, Marvin Häuser wrote:
>>> [...] there is a
>>> function in the UefiBootManagerLib, "BmEnumerateBootOptions()",
>>> which is called prior to entering the Boot Menu and, in my opinion,
>>> would be the perfect place to introduce another
>>> PlatformBootManagerLib hook, which retrieves platform-specific boot
>>> options, such as an UEFI Shell or other utilities like a Memory Test
>>> application.
>>
>> Hmmm, I'm not sure. The only function that calls
>> BmEnumerateBootOptions() is EfiBootManagerRefreshAllBootOption(). The
>> latter is a public UefiBootManagerLib API that several
>> PlatformBootManagerLib instances call, usually from
>> PlatformBootManagerAfterConsole(). PlatformBootManagerAfterConsole()
>
> I have misunderstood the current edk2 logic to be what I expected
> (partially), however I have just discovered that scanned boot options
> are added to NV and to BootOrder.

This is not a coincidence. That's what the
EfiBootManagerRefreshAllBootOption() API does, by design, and that's the
functionality for which platforms call the API.

> Furthermore, I did not mention another usage case for the proposed
> hook as I thought it was practically irrelevant to the edk2 community:
> Supporting Apple's bless boot concept. (For those who wonder,
> virtualizing macOS on Apple hardware is perfectly legal). So, I will
> have to explain a bit more in detail, in two parts.
>
> UEFI BOOT
>
> What I imagined to be the boot flow, by the specification and by my
> personal taste, is the following:
> 1. Boot BootOrder[] one-by-one. This connects the referenced drive and
>attempts to start the specified file. This is the case today.
> 2. Boot *Recovery[]. This is the case today.
> 3. There are two alternatives here, based on platform preference.
> 3.1. Boot enumerated drives (via the algorithm defined in the UEFI
>  spec). I think this is not the case today.
> 3.2. Enumerate the drives (via the algorithm defined in the UEFI spec)
>  and show them in BootManagerMenuApp. I think this is done today
>  and is what I would prefer too.
>
> What I basically do not agree with in the list above are two nuances.
> 3.1 and 3.2: I would like to see support for a platform-defined logic
> of enumerating in addition to the one defined in the UEFI spec. This
> can be used for UEFI Shell, platform diagnostic apps and Apple bless.

I think platform-specific boot option generation -- enumeration of
various protocols that stand for different kinds of devices, and
generating boot options from them -- is already possible today.
Platforms call EfiBootManagerRefreshAllBootOption() because (a) it's
easy to call, (b) it generally does the right thing.

(Note that this API does not connect devices; it just scans devices and
turns whatever it finds into boot options.)

Nothing stops a platform from *not* calling
EfiBootManagerRefreshAllBootOption() -- the platform can simply skip
that and generate other options. Or the platform may generate options in
addition to those that are generated by the API.

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

> The reason why I still wish for platform code to be able to add NV
> options as part of enumeration is that FV-embedded tools are indeed NV
> (Shell etc.) and will not need to be cleaned up, as well as that we're
> not in an utopia. Sometimes an unwanted purge of boot option happens
> ("CMOS reset" on boards, etc) and consumer platform provides do not
> want to leave their users with a constant boot to BootMenu or no
> option at all. Platform code often does currently and probably should
> add a NV Windows boot option in the proposed hook (this should of
> course not be part of edk2). Furthermore, if an OS boot option is
> rendered invalid, u

Re: [edk2] Proposition of a BmEnumerateBootOptions() hook.

2018-05-15 Thread Marvin Häuser
Hi Laszlo, thanks for your feedback!
Comments are inline.

I also CC'd Jordan and Ard because of OVMF references in the third comment.
Thanks in advance for your time!

I have rewritten a few parts multiple times, so, if something doesn't make 
sense (because I forgot to rework it in the context), please feel free to ask 
for clarification.

Best regards,
Marvin

> -Original Message-
> From: Laszlo Ersek 
> Sent: Tuesday, May 15, 2018 10:22 AM
> To: Marvin H?user ; edk2-
> de...@lists.01.org
> Cc: eric.d...@intel.com; star.z...@intel.com
> Subject: Re: [edk2] Proposition of a BmEnumerateBootOptions() hook.
> 
> On 05/14/18 21:00, Marvin H?user wrote:
> > Hey Star, Eric and everyone else,
> >
> > I have seen that some platforms add a Boot Option for the UEFI Shell
> > in "PlatformBootManagerBeforeConsole()", which is called as part of
> > the regular boot flow. This is surely beneficial for development
> > platforms that are supposed to boot to UEFI Shell by default when no
> > other option has been registered,
> 
> (Side point: I sure wish *all* platforms included the UEFI shell, one way or
> another. Debugging issues is the *default* state of any computing system
> (all software has bugs), so debug tools must be a first class citizen. Forums
> are full of end-users asking for the UEFI shell on their physical platforms;
> frequently because their platform firmware gives them an extremely limited
> interface to managing boot and driver options, and the shell is seen as a way
> out of that, justifiedly.)

Definitely, sadly capacity and OEM mentality are still issues.
My point is just that a common user should not land in UEFI Shell by accident, 
but it should be explicitly launched.

> 
> > however for retail platforms it usually makes more sense to show the
> > UEFI Boot Menu,
> 
> Note that this is already solved in BdsDxe (if I understand your point
> anyway); please refer to commit d1de487dd2e7 ("MdeModulePkg/BdsDxe:
> fall back to a Boot Manager Menu loop before hanging", 2017-11-27).

Oh interesting, thanks! That might resolve it, but I didn't check it out yet.

> 
> > which renders adding the Shell Boot Option as part of the regular boot
> > flow obsolete and just adds up to the boot time. Meanwhile, there is a
> > function in the UefiBootManagerLib, "BmEnumerateBootOptions()", which
> > is called prior to entering the Boot Menu and, in my opinion, would be
> > the perfect place to introduce another PlatformBootManagerLib hook,
> > which retrieves platform-specific boot options, such as an UEFI Shell
> > or other utilities like a Memory Test application.
> 
> Hmmm, I'm not sure. The only function that calls
> BmEnumerateBootOptions() is EfiBootManagerRefreshAllBootOption(). The
> latter is a public UefiBootManagerLib API that several
> PlatformBootManagerLib instances call, usually from
> PlatformBootManagerAfterConsole(). PlatformBootManagerAfterConsole()

I have misunderstood the current edk2 logic to be what I expected (partially), 
however I have just discovered that scanned boot options are added to NV and to 
BootOrder. Furthermore, I did not mention another usage case for the proposed 
hook as I thought it was practically irrelevant to the edk2 community: 
Supporting Apple's bless boot concept. (For those who wonder, virtualizing 
macOS on Apple hardware is perfectly legal). So, I will have to explain a bit 
more in detail, in two parts.

UEFI BOOT

What I imagined to be the boot flow, by the specification and by my personal 
taste, is the following:
1. Boot BootOrder[] one-by-one. This connects the referenced drive and attempts 
to start the specified file. This is the case today.
2. Boot *Recovery[]. This is the case today.
3. There are two alternatives here, based on platform preference.
3.1. Boot enumerated drives (via the algorithm defined in the UEFI spec). I 
think this is not the case today.
3.2. Enumerate the drives (via the algorithm defined in the UEFI spec) and show 
them in BootManagerMenuApp. I think this is done today and is what I would 
prefer too.

What I basically do not agree with in the list above are two nuances.
3.1 and 3.2: I would like to see support for a platform-defined logic of 
enumerating in addition to the one defined in the UEFI spec. This can be used 
for UEFI Shell, platform diagnostic apps and Apple bless.
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.
The reason w

Re: [edk2] Proposition of a BmEnumerateBootOptions() hook.

2018-05-15 Thread Laszlo Ersek
On 05/14/18 21:00, Marvin H?user wrote:
> Hey Star, Eric and everyone else,
>
> I have seen that some platforms add a Boot Option for the UEFI Shell
> in "PlatformBootManagerBeforeConsole()", which is called as part of
> the regular boot flow. This is surely beneficial for development
> platforms that are supposed to boot to UEFI Shell by default when no
> other option has been registered,

(Side point: I sure wish *all* platforms included the UEFI shell, one
way or another. Debugging issues is the *default* state of any computing
system (all software has bugs), so debug tools must be a first class
citizen. Forums are full of end-users asking for the UEFI shell on their
physical platforms; frequently because their platform firmware gives
them an extremely limited interface to managing boot and driver options,
and the shell is seen as a way out of that, justifiedly.)

> however for retail platforms it usually makes more sense to show the
> UEFI Boot Menu,

Note that this is already solved in BdsDxe (if I understand your point
anyway); please refer to commit d1de487dd2e7 ("MdeModulePkg/BdsDxe: fall
back to a Boot Manager Menu loop before hanging", 2017-11-27).

> which renders adding the Shell Boot Option as part of the regular boot
> flow obsolete and just adds up to the boot time. Meanwhile, there is a
> function in the UefiBootManagerLib, "BmEnumerateBootOptions()", which
> is called prior to entering the Boot Menu and, in my opinion, would be
> the perfect place to introduce another PlatformBootManagerLib hook,
> which retrieves platform-specific boot options, such as an UEFI Shell
> or other utilities like a Memory Test application.

Hmmm, I'm not sure. The only function that calls
BmEnumerateBootOptions() is EfiBootManagerRefreshAllBootOption(). The
latter is a public UefiBootManagerLib API that several
PlatformBootManagerLib instances call, usually from
PlatformBootManagerAfterConsole(). PlatformBootManagerAfterConsole() is
already a PlatformBootManagerLib hook, so the suggestion would result in
one hook calling back into another hook, of the same library instance:

  BdsEntry()   
[MdeModulePkg/Universal/BdsDxe/BdsEntry.c]
PlatformBootManagerAfterConsole()  
[SomePlatformPkg/Library/PlatformBootManagerLib/...]
  EfiBootManagerRefreshAllBootOption() 
[MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c]
BmEnumerateBootOptions()   
[MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c]
  PlatformBootManagerAnotherHook() 
[SomePlatformPkg/Library/PlatformBootManagerLib/...]

If I'm being honest, I'd like to avoid this -- if I'm in
PlatformBootManagerBeforeConsole() or PlatformBootManagerAfterConsole(),
I'd just like to do whatever's necessary by directly calling either
public UefiBootManagerLib APIs, or STATIC functions from the same
PlatformBootManagerLib instance that I'm already inside of.

Now, what I could see as useful is a public helper function in
UefiBootManagerLib that registers the shell boot option. This
functionality is usually duplicated across several
PlatformBootManagerLib instances.

I'll also mention another interface that the edk2-platforms project uses
-- several platforms there use the same PlatformBootManagerLib instance,
and they abstract out just the default set of platform boot options.
Please see

  [PATCH v4 0/2] add platform boot manager protocol
  
1524464514-14454-1-git-send-email-haojian.zhuang@linaro.org">http://mid.mail-archive.com/1524464514-14454-1-git-send-email-haojian.zhuang@linaro.org

Thanks
Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel


Re: [edk2] Proposition of a BmEnumerateBootOptions() hook.

2018-05-14 Thread Zeng, Star
Cc Ray.

Thanks,
Star
From: Marvin H?user [mailto:marvin.haeu...@outlook.com]
Sent: Tuesday, May 15, 2018 3:00 AM
To: edk2-devel@lists.01.org
Cc: Zeng, Star ; Dong, Eric 
Subject: Proposition of a BmEnumerateBootOptions() hook.

Hey Star, Eric and everyone else,

I have seen that some platforms add a Boot Option for the UEFI Shell in 
"PlatformBootManagerBeforeConsole()", which is called as part of the regular 
boot flow.
This is surely beneficial for development platforms that are supposed to boot 
to UEFI Shell by default when no other option has been registered, however for 
retail platforms it usually makes more sense to show the UEFI Boot Menu, which 
renders adding the Shell Boot Option as part of the regular boot flow obsolete 
and just adds up to the boot time. Meanwhile, there is a function in the 
UefiBootManagerLib, "BmEnumerateBootOptions()", which is called prior to 
entering the Boot Menu and, in my opinion, would be the perfect place to 
introduce another PlatformBootManagerLib hook, which retrieves 
platform-specific boot options, such as an UEFI Shell or other utilities like a 
Memory Test application.
If you have a few spare minutes, I'll be happy for feedback.

Thanks in advance for your time.

Best regards,
Marvin
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel