[PATCH] ArmPkg/PlatformBootManagerLib: regenerate boot options on boot failure

[PATCH] ArmPkg/PlatformBootManagerLib: regenerate boot options on boot failure

[Ard Biesheuvel]

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

Re: [PATCH] ArmPkg/PlatformBootManagerLib: regenerate boot options on boot failure

[Samer El-Haj-Mahmoud]

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.

Re: [PATCH] ArmPkg/PlatformBootManagerLib: regenerate boot options on boot failure

[Leif Lindholm]

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
> 

Re: [PATCH] ArmPkg/PlatformBootManagerLib: regenerate boot options on boot failure

[Ard Biesheuvel]

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.

Re: [PATCH] ArmPkg/PlatformBootManagerLib: regenerate boot options on boot failure

[Leif Lindholm]

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

Re: [PATCH] ArmPkg/PlatformBootManagerLib: regenerate boot options on boot failure

[Ard Biesheuvel]

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
> 

Re: [PATCH] ArmPkg/PlatformBootManagerLib: regenerate boot options on boot failure

[Leif Lindholm]

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

Re: [PATCH] ArmPkg/PlatformBootManagerLib: regenerate boot options on boot failure

[Ard Biesheuvel]

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.

Re: [edk2-devel] [PATCH] ArmPkg/PlatformBootManagerLib: regenerate boot options on boot failure

[Laszlo Ersek]

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>

Re: [PATCH] ArmPkg/PlatformBootManagerLib: regenerate boot options on boot failure

[Leif Lindholm]

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

Re: [PATCH] ArmPkg/PlatformBootManagerLib: regenerate boot options on boot failure

[Andrei Warkentin]

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

Re: [PATCH] ArmPkg/PlatformBootManagerLib: regenerate boot options on boot failure

[Ard Biesheuvel]

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
> 

Re: [PATCH] ArmPkg/PlatformBootManagerLib: regenerate boot options on boot failure

[Ard Biesheuvel]

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,