* Re: [PATCH] ArmPkg/PlatformBootManagerLib: regenerate boot options on boot failure
2020-06-16 17:48 [PATCH] ArmPkg/PlatformBootManagerLib: regenerate boot options on boot failure Ard Biesheuvel
@ 2020-06-16 21:40 ` Samer El-Haj-Mahmoud
2020-06-17 11:12 ` Leif Lindholm
` (2 subsequent siblings)
3 siblings, 0 replies; 13+ messages in thread
From: Samer El-Haj-Mahmoud @ 2020-06-16 21:40 UTC (permalink / raw)
To: Ard Biesheuvel, devel@edk2.groups.io
Cc: leif@nuviainc.com, Ard Biesheuvel, Pete Batard,
Andrei Warkentin (awarkentin@vmware.com)
Reviewed-by: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>
> -----Original Message-----
> From: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Sent: Tuesday, June 16, 2020 1:49 PM
> To: devel@edk2.groups.io
> Cc: leif@nuviainc.com; Ard Biesheuvel <Ard.Biesheuvel@arm.com>; Pete
> Batard <pete@akeo.ie>; Andrei Warkentin (awarkentin@vmware.com)
> <awarkentin@vmware.com>; Samer El-Haj-Mahmoud <Samer.El-Haj-
> Mahmoud@arm.com>
> Subject: [PATCH] ArmPkg/PlatformBootManagerLib: regenerate boot
> options on boot failure
>
> One of the side effects of the recent changes to PlatformBootManagerLib
> changes to avoid connecting all devices on every boot is that we no longer
> default to network boot on a virgin boot, but end up in the UiApp menu. At
> this point, the autogenerated boot options that we used to rely on will be
> instantiated too, but it does break the unattended boot case where devices
> are expected to attempt a network boot on the very first power on.
>
> Let's work around this by refreshing all boot options explicitly in the
> UnableToBoot() handler, and rebooting the system if doing so resulted in a
> change to the total number of configured boot options.
> This way, we ultimately end up in the UiApp as before if no boot options
> could be started, but only after all the autogenerated ones have been
> attempted as well.
> Cc: Pete Batard <pete@akeo.ie>Cc: Andrei Warkentin
> (awarkentin@vmware.com) <awarkentin@vmware.com>Cc: Samer El-Haj-
> Mahmoud <Samer.El-Haj-Mahmoud@arm.com>Signed-off-by: Ard
> Biesheuvel <ard.biesheuvel@arm.com>
> ---
> ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c | 34
> ++++++++++++++++++++
> 1 file changed, 34 insertions(+)
>
> diff --git a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
> b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
> index 15c5cac1bea0..9905cad22908 100644
> --- a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
> +++ b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
> @@ -820,6 +820,40 @@ PlatformBootManagerUnableToBoot (
> { EFI_STATUS Status; EFI_BOOT_MANAGER_LOAD_OPTION
> BootManagerMenu;+ EFI_BOOT_MANAGER_LOAD_OPTION
> *BootOptions;+ UINTN OldBootOptionCount;+ UINTN
> NewBootOptionCount;++ //+ // Record the total number of boot
> configured boot options+ //+ BootOptions =
> EfiBootManagerGetLoadOptions (&OldBootOptionCount,+
> LoadOptionTypeBoot);+ EfiBootManagerFreeLoadOptions (BootOptions,
> OldBootOptionCount);++ //+ // Connect all devices, and regenerate all boot
> options+ //+ EfiBootManagerConnectAll ();+
> EfiBootManagerRefreshAllBootOption ();++ //+ // Record the updated
> number of boot configured boot options+ //+ BootOptions =
> EfiBootManagerGetLoadOptions (&NewBootOptionCount,+
> LoadOptionTypeBoot);+ EfiBootManagerFreeLoadOptions (BootOptions,
> NewBootOptionCount);++ //+ // If the number of configured boot options
> has changed, reboot+ // the system so the new boot options will be taken
> into account+ // while executing the ordinary BDS bootflow sequence.+ //+
> if (NewBootOptionCount != OldBootOptionCount) {+ DEBUG
> ((DEBUG_WARN, "%a: rebooting after refreshing all boot options\n",+
> __FUNCTION__));+ gRT->ResetSystem (EfiResetCold, EFI_SUCCESS, 0,
> NULL);+ } Status = EfiBootManagerGetBootManagerMenu
> (&BootManagerMenu); if (EFI_ERROR (Status)) {--
> 2.27.0
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] ArmPkg/PlatformBootManagerLib: regenerate boot options on boot failure
2020-06-16 17:48 [PATCH] ArmPkg/PlatformBootManagerLib: regenerate boot options on boot failure Ard Biesheuvel
2020-06-16 21:40 ` Samer El-Haj-Mahmoud
@ 2020-06-17 11:12 ` Leif Lindholm
2020-06-17 11:32 ` Ard Biesheuvel
2020-06-17 13:44 ` [edk2-devel] " Laszlo Ersek
2020-06-17 16:21 ` Andrei Warkentin
3 siblings, 1 reply; 13+ messages in thread
From: Leif Lindholm @ 2020-06-17 11:12 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: devel, Pete Batard, Andrei Warkentin, Samer El-Haj-Mahmoud
On Tue, Jun 16, 2020 at 19:48:34 +0200, Ard Biesheuvel wrote:
> One of the side effects of the recent changes to PlatformBootManagerLib
> changes to avoid connecting all devices on every boot is that we no
> longer default to network boot on a virgin boot, but end up in the
> UiApp menu. At this point, the autogenerated boot options that we used
> to rely on will be instantiated too,
The passive voice is confusing me a bit here - who does the updating,
and when specifically?
/
Leif
> but it does break the unattended
> boot case where devices are expected to attempt a network boot on the
> very first power on.
>
> Let's work around this by refreshing all boot options explicitly in
> the UnableToBoot() handler, and rebooting the system if doing so
> resulted in a change to the total number of configured boot options.
> This way, we ultimately end up in the UiApp as before if no boot
> options could be started, but only after all the autogenerated ones
> have been attempted as well.
>
> Cc: Pete Batard <pete@akeo.ie>
> Cc: Andrei Warkentin (awarkentin@vmware.com) <awarkentin@vmware.com>
> Cc: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
> ---
> ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c | 34 ++++++++++++++++++++
> 1 file changed, 34 insertions(+)
>
> diff --git a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
> index 15c5cac1bea0..9905cad22908 100644
> --- a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
> +++ b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
> @@ -820,6 +820,40 @@ PlatformBootManagerUnableToBoot (
> {
> EFI_STATUS Status;
> EFI_BOOT_MANAGER_LOAD_OPTION BootManagerMenu;
> + EFI_BOOT_MANAGER_LOAD_OPTION *BootOptions;
> + UINTN OldBootOptionCount;
> + UINTN NewBootOptionCount;
> +
> + //
> + // Record the total number of boot configured boot options
> + //
> + BootOptions = EfiBootManagerGetLoadOptions (&OldBootOptionCount,
> + LoadOptionTypeBoot);
> + EfiBootManagerFreeLoadOptions (BootOptions, OldBootOptionCount);
> +
> + //
> + // Connect all devices, and regenerate all boot options
> + //
> + EfiBootManagerConnectAll ();
> + EfiBootManagerRefreshAllBootOption ();
> +
> + //
> + // Record the updated number of boot configured boot options
> + //
> + BootOptions = EfiBootManagerGetLoadOptions (&NewBootOptionCount,
> + LoadOptionTypeBoot);
> + EfiBootManagerFreeLoadOptions (BootOptions, NewBootOptionCount);
> +
> + //
> + // If the number of configured boot options has changed, reboot
> + // the system so the new boot options will be taken into account
> + // while executing the ordinary BDS bootflow sequence.
> + //
> + if (NewBootOptionCount != OldBootOptionCount) {
> + DEBUG ((DEBUG_WARN, "%a: rebooting after refreshing all boot options\n",
> + __FUNCTION__));
> + gRT->ResetSystem (EfiResetCold, EFI_SUCCESS, 0, NULL);
> + }
>
> Status = EfiBootManagerGetBootManagerMenu (&BootManagerMenu);
> if (EFI_ERROR (Status)) {
> --
> 2.27.0
>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] ArmPkg/PlatformBootManagerLib: regenerate boot options on boot failure
2020-06-17 11:12 ` Leif Lindholm
@ 2020-06-17 11:32 ` Ard Biesheuvel
2020-06-17 12:16 ` Leif Lindholm
0 siblings, 1 reply; 13+ messages in thread
From: Ard Biesheuvel @ 2020-06-17 11:32 UTC (permalink / raw)
To: Leif Lindholm; +Cc: devel, Pete Batard, Andrei Warkentin, Samer El-Haj-Mahmoud
On 6/17/20 1:12 PM, Leif Lindholm wrote:
> On Tue, Jun 16, 2020 at 19:48:34 +0200, Ard Biesheuvel wrote:
>> One of the side effects of the recent changes to PlatformBootManagerLib
>> changes to avoid connecting all devices on every boot is that we no
>> longer default to network boot on a virgin boot, but end up in the
>> UiApp menu. At this point, the autogenerated boot options that we used
>> to rely on will be instantiated too,
>
> The passive voice is confusing me a bit here - who does the updating,
> and when specifically?
>
Originally, the ArmPkg PlatformBmLib would always refresh all boot
options, but now, only the UiApp does that upon entry, at which point
your sitting in the menu idly, and so automated network boot no longer
works.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] ArmPkg/PlatformBootManagerLib: regenerate boot options on boot failure
2020-06-17 11:32 ` Ard Biesheuvel
@ 2020-06-17 12:16 ` Leif Lindholm
2020-06-17 12:28 ` Ard Biesheuvel
0 siblings, 1 reply; 13+ messages in thread
From: Leif Lindholm @ 2020-06-17 12:16 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: devel, Pete Batard, Andrei Warkentin, Samer El-Haj-Mahmoud
On Wed, Jun 17, 2020 at 13:32:36 +0200, Ard Biesheuvel wrote:
> On 6/17/20 1:12 PM, Leif Lindholm wrote:
> > On Tue, Jun 16, 2020 at 19:48:34 +0200, Ard Biesheuvel wrote:
> > > One of the side effects of the recent changes to PlatformBootManagerLib
> > > changes to avoid connecting all devices on every boot is that we no
> > > longer default to network boot on a virgin boot, but end up in the
> > > UiApp menu. At this point, the autogenerated boot options that we used
> > > to rely on will be instantiated too,
> >
> > The passive voice is confusing me a bit here - who does the updating,
> > and when specifically?
> >
>
> Originally, the ArmPkg PlatformBmLib would always refresh all boot options,
> but now, only the UiApp does that upon entry, at which point your sitting in
> the menu idly, and so automated network boot no longer works.
Sure. But the message should contain some description of agency.
Something like:
"On entry, the UiApp instantiates the autogenerated boot options that
we used to rely on - but it does not consume them. This breaks the
unattended..."
I assume the UiApp only ever *adds* entries, which is why checking
number of entries is sufficient?
With that/if so:
Reviewed-by: Leif Lindholm <leif@nuviainc.com>
/
Leif
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] ArmPkg/PlatformBootManagerLib: regenerate boot options on boot failure
2020-06-17 12:16 ` Leif Lindholm
@ 2020-06-17 12:28 ` Ard Biesheuvel
2020-06-17 12:40 ` Leif Lindholm
0 siblings, 1 reply; 13+ messages in thread
From: Ard Biesheuvel @ 2020-06-17 12:28 UTC (permalink / raw)
To: Leif Lindholm; +Cc: devel, Pete Batard, Andrei Warkentin, Samer El-Haj-Mahmoud
On 6/17/20 2:16 PM, Leif Lindholm wrote:
> On Wed, Jun 17, 2020 at 13:32:36 +0200, Ard Biesheuvel wrote:
>> On 6/17/20 1:12 PM, Leif Lindholm wrote:
>>> On Tue, Jun 16, 2020 at 19:48:34 +0200, Ard Biesheuvel wrote:
>>>> One of the side effects of the recent changes to PlatformBootManagerLib
>>>> changes to avoid connecting all devices on every boot is that we no
>>>> longer default to network boot on a virgin boot, but end up in the
>>>> UiApp menu. At this point, the autogenerated boot options that we used
>>>> to rely on will be instantiated too,
>>>
>>> The passive voice is confusing me a bit here - who does the updating,
>>> and when specifically?
>>>
>>
>> Originally, the ArmPkg PlatformBmLib would always refresh all boot options,
>> but now, only the UiApp does that upon entry, at which point your sitting in
>> the menu idly, and so automated network boot no longer works.
>
> Sure. But the message should contain some description of agency.
>
> Something like:
> "On entry, the UiApp instantiates the autogenerated boot options that
> we used to rely on - but it does not consume them. This breaks the
> unattended..."
>
OK
> I assume the UiApp only ever *adds* entries, which is why checking
> number of entries is sufficient?
>
It only manages entries that it instantiated itself, but it may also
remove entries if the underlying hardware has disappeared.
> With that/if so:
> Reviewed-by: Leif Lindholm <leif@nuviainc.com>
>
> /
> Leif
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] ArmPkg/PlatformBootManagerLib: regenerate boot options on boot failure
2020-06-17 12:28 ` Ard Biesheuvel
@ 2020-06-17 12:40 ` Leif Lindholm
2020-06-17 12:59 ` Ard Biesheuvel
0 siblings, 1 reply; 13+ messages in thread
From: Leif Lindholm @ 2020-06-17 12:40 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: devel, Pete Batard, Andrei Warkentin, Samer El-Haj-Mahmoud
On Wed, Jun 17, 2020 at 14:28:04 +0200, Ard Biesheuvel wrote:
> On 6/17/20 2:16 PM, Leif Lindholm wrote:
> > On Wed, Jun 17, 2020 at 13:32:36 +0200, Ard Biesheuvel wrote:
> > > On 6/17/20 1:12 PM, Leif Lindholm wrote:
> > > > On Tue, Jun 16, 2020 at 19:48:34 +0200, Ard Biesheuvel wrote:
> > > > > One of the side effects of the recent changes to PlatformBootManagerLib
> > > > > changes to avoid connecting all devices on every boot is that we no
> > > > > longer default to network boot on a virgin boot, but end up in the
> > > > > UiApp menu. At this point, the autogenerated boot options that we used
> > > > > to rely on will be instantiated too,
> > > >
> > > > The passive voice is confusing me a bit here - who does the updating,
> > > > and when specifically?
> > > >
> > >
> > > Originally, the ArmPkg PlatformBmLib would always refresh all boot options,
> > > but now, only the UiApp does that upon entry, at which point your sitting in
> > > the menu idly, and so automated network boot no longer works.
> >
> > Sure. But the message should contain some description of agency.
> >
> > Something like:
> > "On entry, the UiApp instantiates the autogenerated boot options that
> > we used to rely on - but it does not consume them. This breaks the
> > unattended..."
>
> OK
>
> > I assume the UiApp only ever *adds* entries, which is why checking
> > number of entries is sufficient?
> >
>
> It only manages entries that it instantiated itself, but it may also remove
> entries if the underlying hardware has disappeared.
Hmm, that's a bit trickier then. I mean, it's unlikely, but I'm sure
there's situations that could happen.
Would we run the risk of getting bug reports like "system fails to
boot from Ethernet when inifiniband switch powered off"? Or if some
virtual devices presented by a BMC appear/disappear?
/
Leif
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] ArmPkg/PlatformBootManagerLib: regenerate boot options on boot failure
2020-06-17 12:40 ` Leif Lindholm
@ 2020-06-17 12:59 ` Ard Biesheuvel
2020-06-17 14:35 ` Leif Lindholm
0 siblings, 1 reply; 13+ messages in thread
From: Ard Biesheuvel @ 2020-06-17 12:59 UTC (permalink / raw)
To: Leif Lindholm; +Cc: devel, Pete Batard, Andrei Warkentin, Samer El-Haj-Mahmoud
On 6/17/20 2:40 PM, Leif Lindholm wrote:
> On Wed, Jun 17, 2020 at 14:28:04 +0200, Ard Biesheuvel wrote:
>> On 6/17/20 2:16 PM, Leif Lindholm wrote:
>>> On Wed, Jun 17, 2020 at 13:32:36 +0200, Ard Biesheuvel wrote:
>>>> On 6/17/20 1:12 PM, Leif Lindholm wrote:
>>>>> On Tue, Jun 16, 2020 at 19:48:34 +0200, Ard Biesheuvel wrote:
>>>>>> One of the side effects of the recent changes to PlatformBootManagerLib
>>>>>> changes to avoid connecting all devices on every boot is that we no
>>>>>> longer default to network boot on a virgin boot, but end up in the
>>>>>> UiApp menu. At this point, the autogenerated boot options that we used
>>>>>> to rely on will be instantiated too,
>>>>>
>>>>> The passive voice is confusing me a bit here - who does the updating,
>>>>> and when specifically?
>>>>>
>>>>
>>>> Originally, the ArmPkg PlatformBmLib would always refresh all boot options,
>>>> but now, only the UiApp does that upon entry, at which point your sitting in
>>>> the menu idly, and so automated network boot no longer works.
>>>
>>> Sure. But the message should contain some description of agency.
>>>
>>> Something like:
>>> "On entry, the UiApp instantiates the autogenerated boot options that
>>> we used to rely on - but it does not consume them. This breaks the
>>> unattended..."
>>
>> OK
>>
>>> I assume the UiApp only ever *adds* entries, which is why checking
>>> number of entries is sufficient?
>>>
>>
>> It only manages entries that it instantiated itself, but it may also remove
>> entries if the underlying hardware has disappeared.
>
> Hmm, that's a bit trickier then. I mean, it's unlikely, but I'm sure
> there's situations that could happen.
> Would we run the risk of getting bug reports like "system fails to
> boot from Ethernet when inifiniband switch powered off"? Or if some
> virtual devices presented by a BMC appear/disappear?
>
If the boot entries are not refreshed, you will retain the old ones. So
the only way this could lead to a boot failure is when you rely on
automatically generated boot entries to device that disappear and
reappear in a different place, e.g., move a Ethernet PCIe card to a
different slot. Note that USB devices plugged into a different port will
still work fine, though, as they rely on the removable boot path in this
case, which will be attempted anyway before doing the UnableToBoot().
Note that the failure mode here is being dropped into the menu, where
before you were always dropped into the Shell. The case we are trying to
address here is zero intervention network boot after putting the device
into circulation, and that should work correctly with this change: if
the network boot path did not exist before, it will be added, in which
case the number of boot options will increase.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] ArmPkg/PlatformBootManagerLib: regenerate boot options on boot failure
2020-06-17 12:59 ` Ard Biesheuvel
@ 2020-06-17 14:35 ` Leif Lindholm
2020-06-17 18:30 ` Ard Biesheuvel
0 siblings, 1 reply; 13+ messages in thread
From: Leif Lindholm @ 2020-06-17 14:35 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: devel, Pete Batard, Andrei Warkentin, Samer El-Haj-Mahmoud
On Wed, Jun 17, 2020 at 14:59:45 +0200, Ard Biesheuvel wrote:
> > > > Something like:
> > > > "On entry, the UiApp instantiates the autogenerated boot options that
> > > > we used to rely on - but it does not consume them. This breaks the
> > > > unattended..."
> > >
> > > OK
> > >
> > > > I assume the UiApp only ever *adds* entries, which is why checking
> > > > number of entries is sufficient?
> > > >
> > >
> > > It only manages entries that it instantiated itself, but it may also remove
> > > entries if the underlying hardware has disappeared.
> >
> > Hmm, that's a bit trickier then. I mean, it's unlikely, but I'm sure
> > there's situations that could happen.
> > Would we run the risk of getting bug reports like "system fails to
> > boot from Ethernet when inifiniband switch powered off"? Or if some
> > virtual devices presented by a BMC appear/disappear?
> >
>
> If the boot entries are not refreshed, you will retain the old ones. So the
> only way this could lead to a boot failure is when you rely on automatically
> generated boot entries to device that disappear and reappear in a different
> place, e.g., move a Ethernet PCIe card to a different slot. Note that USB
> devices plugged into a different port will still work fine, though, as they
> rely on the removable boot path in this case, which will be attempted anyway
> before doing the UnableToBoot().
>
> Note that the failure mode here is being dropped into the menu, where before
> you were always dropped into the Shell. The case we are trying to address
> here is zero intervention network boot after putting the device into
> circulation, and that should work correctly with this change: if the network
> boot path did not exist before, it will be added, in which case the number
> of boot options will increase.
OK. I'm not convinced we're not going to see a report of this
somewhere down the line, but I think you've managed to convince me
it's an unlikely enough situation, and a fallback, that we can bump it
to then (and it *is* a behavioural improvement in all other cases).
Reviewed-by: Leif Lindholm <leif@nuviainc.com>
(with the commit message update)
/
Leif
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] ArmPkg/PlatformBootManagerLib: regenerate boot options on boot failure
2020-06-17 14:35 ` Leif Lindholm
@ 2020-06-17 18:30 ` Ard Biesheuvel
0 siblings, 0 replies; 13+ messages in thread
From: Ard Biesheuvel @ 2020-06-17 18:30 UTC (permalink / raw)
To: Leif Lindholm; +Cc: devel, Pete Batard, Andrei Warkentin, Samer El-Haj-Mahmoud
On 6/17/20 4:35 PM, Leif Lindholm wrote:
> On Wed, Jun 17, 2020 at 14:59:45 +0200, Ard Biesheuvel wrote:
>>>>> Something like:
>>>>> "On entry, the UiApp instantiates the autogenerated boot options that
>>>>> we used to rely on - but it does not consume them. This breaks the
>>>>> unattended..."
>>>>
>>>> OK
>>>>
>>>>> I assume the UiApp only ever *adds* entries, which is why checking
>>>>> number of entries is sufficient?
>>>>>
>>>>
>>>> It only manages entries that it instantiated itself, but it may also remove
>>>> entries if the underlying hardware has disappeared.
>>>
>>> Hmm, that's a bit trickier then. I mean, it's unlikely, but I'm sure
>>> there's situations that could happen.
>>> Would we run the risk of getting bug reports like "system fails to
>>> boot from Ethernet when inifiniband switch powered off"? Or if some
>>> virtual devices presented by a BMC appear/disappear?
>>>
>>
>> If the boot entries are not refreshed, you will retain the old ones. So the
>> only way this could lead to a boot failure is when you rely on automatically
>> generated boot entries to device that disappear and reappear in a different
>> place, e.g., move a Ethernet PCIe card to a different slot. Note that USB
>> devices plugged into a different port will still work fine, though, as they
>> rely on the removable boot path in this case, which will be attempted anyway
>> before doing the UnableToBoot().
>>
>> Note that the failure mode here is being dropped into the menu, where before
>> you were always dropped into the Shell. The case we are trying to address
>> here is zero intervention network boot after putting the device into
>> circulation, and that should work correctly with this change: if the network
>> boot path did not exist before, it will be added, in which case the number
>> of boot options will increase.
>
> OK. I'm not convinced we're not going to see a report of this
> somewhere down the line, but I think you've managed to convince me
> it's an unlikely enough situation, and a fallback, that we can bump it
> to then (and it *is* a behavioural improvement in all other cases).
>
> Reviewed-by: Leif Lindholm <leif@nuviainc.com>
> (with the commit message update)
>
Merged as 2d233af64b8f73d1b1e138b302e6344f7c2e0f4e
Thanks all,
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [edk2-devel] [PATCH] ArmPkg/PlatformBootManagerLib: regenerate boot options on boot failure
2020-06-16 17:48 [PATCH] ArmPkg/PlatformBootManagerLib: regenerate boot options on boot failure Ard Biesheuvel
2020-06-16 21:40 ` Samer El-Haj-Mahmoud
2020-06-17 11:12 ` Leif Lindholm
@ 2020-06-17 13:44 ` Laszlo Ersek
2020-06-17 16:21 ` Andrei Warkentin
3 siblings, 0 replies; 13+ messages in thread
From: Laszlo Ersek @ 2020-06-17 13:44 UTC (permalink / raw)
To: devel, ard.biesheuvel
Cc: leif, Pete Batard, Andrei Warkentin, Samer El-Haj-Mahmoud
On 06/16/20 19:48, Ard Biesheuvel wrote:
> One of the side effects of the recent changes to PlatformBootManagerLib
> changes to avoid connecting all devices on every boot is that we no
> longer default to network boot on a virgin boot, but end up in the
> UiApp menu. At this point, the autogenerated boot options that we used
> to rely on will be instantiated too, but it does break the unattended
> boot case where devices are expected to attempt a network boot on the
> very first power on.
>
> Let's work around this by refreshing all boot options explicitly in
> the UnableToBoot() handler, and rebooting the system if doing so
> resulted in a change to the total number of configured boot options.
> This way, we ultimately end up in the UiApp as before if no boot
> options could be started, but only after all the autogenerated ones
> have been attempted as well.
>
> Cc: Pete Batard <pete@akeo.ie>
> Cc: Andrei Warkentin (awarkentin@vmware.com) <awarkentin@vmware.com>
> Cc: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
> ---
> ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c | 34 ++++++++++++++++++++
> 1 file changed, 34 insertions(+)
>
> diff --git a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
> index 15c5cac1bea0..9905cad22908 100644
> --- a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
> +++ b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
> @@ -820,6 +820,40 @@ PlatformBootManagerUnableToBoot (
> {
> EFI_STATUS Status;
> EFI_BOOT_MANAGER_LOAD_OPTION BootManagerMenu;
> + EFI_BOOT_MANAGER_LOAD_OPTION *BootOptions;
> + UINTN OldBootOptionCount;
> + UINTN NewBootOptionCount;
> +
> + //
> + // Record the total number of boot configured boot options
> + //
> + BootOptions = EfiBootManagerGetLoadOptions (&OldBootOptionCount,
> + LoadOptionTypeBoot);
> + EfiBootManagerFreeLoadOptions (BootOptions, OldBootOptionCount);
> +
> + //
> + // Connect all devices, and regenerate all boot options
> + //
> + EfiBootManagerConnectAll ();
> + EfiBootManagerRefreshAllBootOption ();
> +
> + //
> + // Record the updated number of boot configured boot options
> + //
> + BootOptions = EfiBootManagerGetLoadOptions (&NewBootOptionCount,
> + LoadOptionTypeBoot);
> + EfiBootManagerFreeLoadOptions (BootOptions, NewBootOptionCount);
> +
> + //
> + // If the number of configured boot options has changed, reboot
> + // the system so the new boot options will be taken into account
> + // while executing the ordinary BDS bootflow sequence.
> + //
> + if (NewBootOptionCount != OldBootOptionCount) {
> + DEBUG ((DEBUG_WARN, "%a: rebooting after refreshing all boot options\n",
> + __FUNCTION__));
> + gRT->ResetSystem (EfiResetCold, EFI_SUCCESS, 0, NULL);
> + }
>
> Status = EfiBootManagerGetBootManagerMenu (&BootManagerMenu);
> if (EFI_ERROR (Status)) {
>
This looks like a very nice trick, and a very good utilization of the
PlatformBootManagerUnableToBoot() hook, for physical machines.
FWIW:
Acked-by: Laszlo Ersek <lersek@redhat.com>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] ArmPkg/PlatformBootManagerLib: regenerate boot options on boot failure
2020-06-16 17:48 [PATCH] ArmPkg/PlatformBootManagerLib: regenerate boot options on boot failure Ard Biesheuvel
` (2 preceding siblings ...)
2020-06-17 13:44 ` [edk2-devel] " Laszlo Ersek
@ 2020-06-17 16:21 ` Andrei Warkentin
2020-06-17 18:18 ` Ard Biesheuvel
3 siblings, 1 reply; 13+ messages in thread
From: Andrei Warkentin @ 2020-06-17 16:21 UTC (permalink / raw)
To: Ard Biesheuvel, devel@edk2.groups.io
Cc: leif@nuviainc.com, Pete Batard, Samer El-Haj-Mahmoud
[-- Attachment #1: Type: text/plain, Size: 3706 bytes --]
Thanks for working on this.
Reviewed-by: Andrei Warkentin <awarkentin@vmware.com>
A non-blocking question: is a reboot necessary? Would it be possible to just retry the boot sequence?
A
________________________________
From: Ard Biesheuvel <ard.biesheuvel@arm.com>
Sent: Tuesday, June 16, 2020 12:48 PM
To: devel@edk2.groups.io <devel@edk2.groups.io>
Cc: leif@nuviainc.com <leif@nuviainc.com>; Ard Biesheuvel <ard.biesheuvel@arm.com>; Pete Batard <pete@akeo.ie>; Andrei Warkentin <awarkentin@vmware.com>; Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>
Subject: [PATCH] ArmPkg/PlatformBootManagerLib: regenerate boot options on boot failure
One of the side effects of the recent changes to PlatformBootManagerLib
changes to avoid connecting all devices on every boot is that we no
longer default to network boot on a virgin boot, but end up in the
UiApp menu. At this point, the autogenerated boot options that we used
to rely on will be instantiated too, but it does break the unattended
boot case where devices are expected to attempt a network boot on the
very first power on.
Let's work around this by refreshing all boot options explicitly in
the UnableToBoot() handler, and rebooting the system if doing so
resulted in a change to the total number of configured boot options.
This way, we ultimately end up in the UiApp as before if no boot
options could be started, but only after all the autogenerated ones
have been attempted as well.
Cc: Pete Batard <pete@akeo.ie>
Cc: Andrei Warkentin (awarkentin@vmware.com) <awarkentin@vmware.com>
Cc: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
---
ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c | 34 ++++++++++++++++++++
1 file changed, 34 insertions(+)
diff --git a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
index 15c5cac1bea0..9905cad22908 100644
--- a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
+++ b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
@@ -820,6 +820,40 @@ PlatformBootManagerUnableToBoot (
{
EFI_STATUS Status;
EFI_BOOT_MANAGER_LOAD_OPTION BootManagerMenu;
+ EFI_BOOT_MANAGER_LOAD_OPTION *BootOptions;
+ UINTN OldBootOptionCount;
+ UINTN NewBootOptionCount;
+
+ //
+ // Record the total number of boot configured boot options
+ //
+ BootOptions = EfiBootManagerGetLoadOptions (&OldBootOptionCount,
+ LoadOptionTypeBoot);
+ EfiBootManagerFreeLoadOptions (BootOptions, OldBootOptionCount);
+
+ //
+ // Connect all devices, and regenerate all boot options
+ //
+ EfiBootManagerConnectAll ();
+ EfiBootManagerRefreshAllBootOption ();
+
+ //
+ // Record the updated number of boot configured boot options
+ //
+ BootOptions = EfiBootManagerGetLoadOptions (&NewBootOptionCount,
+ LoadOptionTypeBoot);
+ EfiBootManagerFreeLoadOptions (BootOptions, NewBootOptionCount);
+
+ //
+ // If the number of configured boot options has changed, reboot
+ // the system so the new boot options will be taken into account
+ // while executing the ordinary BDS bootflow sequence.
+ //
+ if (NewBootOptionCount != OldBootOptionCount) {
+ DEBUG ((DEBUG_WARN, "%a: rebooting after refreshing all boot options\n",
+ __FUNCTION__));
+ gRT->ResetSystem (EfiResetCold, EFI_SUCCESS, 0, NULL);
+ }
Status = EfiBootManagerGetBootManagerMenu (&BootManagerMenu);
if (EFI_ERROR (Status)) {
--
2.27.0
[-- Attachment #2: Type: text/html, Size: 7208 bytes --]
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH] ArmPkg/PlatformBootManagerLib: regenerate boot options on boot failure
2020-06-17 16:21 ` Andrei Warkentin
@ 2020-06-17 18:18 ` Ard Biesheuvel
0 siblings, 0 replies; 13+ messages in thread
From: Ard Biesheuvel @ 2020-06-17 18:18 UTC (permalink / raw)
To: Andrei Warkentin, devel@edk2.groups.io
Cc: leif@nuviainc.com, Pete Batard, Samer El-Haj-Mahmoud
On 6/17/20 6:21 PM, Andrei Warkentin wrote:
> Thanks for working on this.
>
> Reviewed-by: Andrei Warkentin <awarkentin@vmware.com>
>
Thanks.
> A non-blocking question: is a reboot necessary? Would it be possible to
> just retry the boot sequence?
>
Not sure how we'd do that.
> A
> ------------------------------------------------------------------------
> *From:* Ard Biesheuvel <ard.biesheuvel@arm.com>
> *Sent:* Tuesday, June 16, 2020 12:48 PM
> *To:* devel@edk2.groups.io <devel@edk2.groups.io>
> *Cc:* leif@nuviainc.com <leif@nuviainc.com>; Ard Biesheuvel
> <ard.biesheuvel@arm.com>; Pete Batard <pete@akeo.ie>; Andrei Warkentin
> <awarkentin@vmware.com>; Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>
> *Subject:* [PATCH] ArmPkg/PlatformBootManagerLib: regenerate boot
> options on boot failure
> One of the side effects of the recent changes to PlatformBootManagerLib
> changes to avoid connecting all devices on every boot is that we no
> longer default to network boot on a virgin boot, but end up in the
> UiApp menu. At this point, the autogenerated boot options that we used
> to rely on will be instantiated too, but it does break the unattended
> boot case where devices are expected to attempt a network boot on the
> very first power on.
>
> Let's work around this by refreshing all boot options explicitly in
> the UnableToBoot() handler, and rebooting the system if doing so
> resulted in a change to the total number of configured boot options.
> This way, we ultimately end up in the UiApp as before if no boot
> options could be started, but only after all the autogenerated ones
> have been attempted as well.
>
>
> Cc: Pete Batard <pete@akeo.ie>
>
> Cc: Andrei Warkentin (awarkentin@vmware.com) <awarkentin@vmware.com>
>
> Cc: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>
>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
> ---
> ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c | 34
> ++++++++++++++++++++
> 1 file changed, 34 insertions(+)
>
> diff --git a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
> b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
> index 15c5cac1bea0..9905cad22908 100644
> --- a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
> +++ b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
> @@ -820,6 +820,40 @@ PlatformBootManagerUnableToBoot (
> {
>
> EFI_STATUS Status;
>
> EFI_BOOT_MANAGER_LOAD_OPTION BootManagerMenu;
>
> + EFI_BOOT_MANAGER_LOAD_OPTION *BootOptions;
>
> + UINTN OldBootOptionCount;
>
> + UINTN NewBootOptionCount;
>
> +
>
> + //
>
> + // Record the total number of boot configured boot options
>
> + //
>
> + BootOptions = EfiBootManagerGetLoadOptions (&OldBootOptionCount,
>
> + LoadOptionTypeBoot);
>
> + EfiBootManagerFreeLoadOptions (BootOptions, OldBootOptionCount);
>
> +
>
> + //
>
> + // Connect all devices, and regenerate all boot options
>
> + //
>
> + EfiBootManagerConnectAll ();
>
> + EfiBootManagerRefreshAllBootOption ();
>
> +
>
> + //
>
> + // Record the updated number of boot configured boot options
>
> + //
>
> + BootOptions = EfiBootManagerGetLoadOptions (&NewBootOptionCount,
>
> + LoadOptionTypeBoot);
>
> + EfiBootManagerFreeLoadOptions (BootOptions, NewBootOptionCount);
>
> +
>
> + //
>
> + // If the number of configured boot options has changed, reboot
>
> + // the system so the new boot options will be taken into account
>
> + // while executing the ordinary BDS bootflow sequence.
>
> + //
>
> + if (NewBootOptionCount != OldBootOptionCount) {
>
> + DEBUG ((DEBUG_WARN, "%a: rebooting after refreshing all boot
> options\n",
>
> + __FUNCTION__));
>
> + gRT->ResetSystem (EfiResetCold, EFI_SUCCESS, 0, NULL);
>
> + }
>
>
>
> Status = EfiBootManagerGetBootManagerMenu (&BootManagerMenu);
>
> if (EFI_ERROR (Status)) {
>
> --
> 2.27.0
>
^ permalink raw reply [flat|nested] 13+ messages in thread