[PATCH v2 0/5] ArmPkg/PlatformBootManagerLib: play nice without ConnectAll()

[PATCH v2 0/5] ArmPkg/PlatformBootManagerLib: play nice without ConnectAll()

[Ard Biesheuvel]

Currently, Armpkg's PlatformBootManagerLib connects all controllers to
their drivers recursively, even if the active boot option does not
require it. There are some historical reasons for this, some of which are
being addressed in separate patches.

This series addresses the way ArmPkg's PlatformBootManagerLib implementation
deals with the UEFI Shell and the UiApp: currently, the shell is always
added as an ordinary boot option, which will be started if no other boot
options can be started, or if it is the first one in the boot order.

Once we remove the ConnectAll() call from PlatformBootManagerLib, those shells
will be launched without any block or other devices connected, which may
confuse novice users. So before doing so, let's make the handling a bit more
sane:
- add a 's' hotkey that enters the UEFI shell regardless of its priority
  in the BootOrder - this shell will be entered with no devices connected
  after patch #4
- enter the UiApp as a last resort if no boot options can be started
- make the UEFI Shell boot option inactive, so it is not started by default
  (only by hotkey or BootNext), but is still accessible via the UiApp boot
  manager menu
- remove the ConnectAll() call from PlatformBootManagerLib

Changes since v1:
- add Laszlo's R-b to patches #1, #2 and #5, but omit it from #4, which has
  changed
- make the generated boot option for the shell inactive but not hidden - this
  will prevent it from being selected by default
- add a patch to deal with inactive boot options correctly, by not omitting
  them from the boot selection menu presented by the boot manager in UiApp
- some commit log tweaks to fix typos and align the text with the above
  changes

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Leif Lindholm <leif@nuviainc.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Zhichao Gao <zhichao.gao@intel.com>

Ard Biesheuvel (5):
  ArmPkg/PlatformBootManagerLib: register 's' as UEFI Shell hotkey
  ArmPkg/PlatformBootManagerLib: fall back to the UiApp on boot failure
  MdeModulePkg/BootManagerUiLib: show inactive boot options
  ArmPkg/PlatformBootManagerLib: hide UEFI Shell as a regular boot
    option
  ArmPkg/PlatformBootManagerLib: don't connect all devices on each boot

 .../PlatformBootManagerLib/PlatformBm.c       | 35 +++++++++++--------
 .../Library/BootManagerUiLib/BootManager.c    |  4 +--
 2 files changed, 22 insertions(+), 17 deletions(-)

-- 
2.17.1

[PATCH v2 1/5] ArmPkg/PlatformBootManagerLib: register 's' as UEFI Shell hotkey

[Ard Biesheuvel]

In preparation of hiding the UEFI Shell boot option as an ordinary
boot option, make sure we can invoke it directly using the 's'
hotkey. Without ConnectAll() having been called, this results in
a shell that may have no block devices or other things connected,
so don't advertise the 's' in the console string that is printed
at boot - for novice users, we will go through the UiApp which
connects everything first. For advanced use, having the ability
to invoke the UEFI shell without any devices connected may be an
advantage, so let's keep this behavior as is for now.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---
 ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
index 4aca1382b042..23c925bbdb9c 100644
--- a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
+++ b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
@@ -357,7 +357,8 @@ VOID
 PlatformRegisterFvBootOption (
   CONST EFI_GUID                   *FileGuid,
   CHAR16                           *Description,
-  UINT32                           Attributes
+  UINT32                           Attributes,
+  EFI_INPUT_KEY                    *Key
   )
 {
   EFI_STATUS                        Status;
@@ -409,6 +410,9 @@ PlatformRegisterFvBootOption (
   if (OptionIndex == -1) {
     Status = EfiBootManagerAddLoadOptionVariable (&NewOption, MAX_UINTN);
     ASSERT_EFI_ERROR (Status);
+    Status = EfiBootManagerAddKeyOptionVariable (NULL,
+               (UINT16)NewOption.OptionNumber, 0, Key, NULL);
+    ASSERT (Status == EFI_SUCCESS || Status == EFI_ALREADY_STARTED);
   }
   EfiBootManagerFreeLoadOption (&NewOption);
   EfiBootManagerFreeLoadOptions (BootOptions, BootOptionCount);
@@ -721,6 +725,7 @@ PlatformBootManagerAfterConsole (
   UINTN                         FirmwareVerLength;
   UINTN                         PosX;
   UINTN                         PosY;
+  EFI_INPUT_KEY                 Key;
 
   FirmwareVerLength = StrLen (PcdGetPtr (PcdFirmwareVersionString));
 
@@ -770,8 +775,10 @@ PlatformBootManagerAfterConsole (
   //
   // Register UEFI Shell
   //
+  Key.ScanCode     = SCAN_NULL;
+  Key.UnicodeChar  = L's';
   PlatformRegisterFvBootOption (
-    &gUefiShellFileGuid, L"UEFI Shell", LOAD_OPTION_ACTIVE
+    &gUefiShellFileGuid, L"UEFI Shell", LOAD_OPTION_ACTIVE, &Key
     );
 }
 
-- 
2.17.1

[PATCH v2 2/5] ArmPkg/PlatformBootManagerLib: fall back to the UiApp on boot failure

[Ard Biesheuvel]

As a last resort, drop into the UiApp application when no active boot
options could be started. Doing so will connect all devices, and so
it will allow the user to enter the Boot Manager submenu and pick a
network or removable disk option.

Note that this only occurs if even the default removable filepath
could not be booted (e.g., \EFI\BOOT\BOOTAA64.EFI on AArch64)

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---
 ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
index 23c925bbdb9c..85cb32f6d7cd 100644
--- a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
+++ b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
@@ -830,5 +830,15 @@ PlatformBootManagerUnableToBoot (
   VOID
   )
 {
-  return;
+  EFI_STATUS                   Status;
+  EFI_BOOT_MANAGER_LOAD_OPTION BootManagerMenu;
+
+  Status = EfiBootManagerGetBootManagerMenu (&BootManagerMenu);
+  if (EFI_ERROR (Status)) {
+    return;
+  }
+
+  for (;;) {
+    EfiBootManagerBoot (&BootManagerMenu);
+  }
 }
-- 
2.17.1

[PATCH v2 3/5] MdeModulePkg/BootManagerUiLib: show inactive boot options

[Ard Biesheuvel]

UEFI boot options may exist but have the LOAD_OPTION_ACTIVE flag
cleared. This means that the boot option should not be selected
by default, but it does not mean it should be omitted from the
boot selection presented by the boot manager: for this purpose,
another flag LOAD_OPTION_HIDDEN exists.

Given that the latter flag exists solely for the purpose of omitting
boot options from the boot selection menu, and LOAD_OPTION_XXX flags
can be combined if desired, hiding inactive boot options as well is
a mistake, and violates the intent of paragraph 3.1.3 of the UEFI
specification (revision 2.8 errata A). Let's fix this by dropping
the LOAD_OPTION_ACTIVE check from the code that populates the boot
selection menu.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
---
 MdeModulePkg/Library/BootManagerUiLib/BootManager.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/MdeModulePkg/Library/BootManagerUiLib/BootManager.c b/MdeModulePkg/Library/BootManagerUiLib/BootManager.c
index 13b40e11b396..4b2c4c77a124 100644
--- a/MdeModulePkg/Library/BootManagerUiLib/BootManager.c
+++ b/MdeModulePkg/Library/BootManagerUiLib/BootManager.c
@@ -535,9 +535,9 @@ UpdateBootManager (
     mKeyInput++;
 
     //
-    // Don't display the hidden/inactive boot option
+    // Don't display hidden boot options, but retain inactive ones.
     //
-    if (((BootOption[Index].Attributes & LOAD_OPTION_HIDDEN) != 0) || ((BootOption[Index].Attributes & LOAD_OPTION_ACTIVE) == 0)) {
+    if ((BootOption[Index].Attributes & LOAD_OPTION_HIDDEN) != 0) {
       continue;
     }
 
-- 
2.17.1

[PATCH v2 4/5] ArmPkg/PlatformBootManagerLib: hide UEFI Shell as a regular boot option

[Ard Biesheuvel]

Without ConnectAll() being called on the boot path, the UEFI shell will
be entered with no block devices or anything else connected, and so for
the novice user, this is not a very accommodating environment. Now that
we have made the UiApp the last resort on boot failure, and made the
UEFI Shell accessible directly via the 's' hotkey if you really need
it, let's hide it as an ordinary boot option.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
---
 ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
index 85cb32f6d7cd..1e9b736993d0 100644
--- a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
+++ b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
@@ -777,9 +777,7 @@ PlatformBootManagerAfterConsole (
   //
   Key.ScanCode     = SCAN_NULL;
   Key.UnicodeChar  = L's';
-  PlatformRegisterFvBootOption (
-    &gUefiShellFileGuid, L"UEFI Shell", LOAD_OPTION_ACTIVE, &Key
-    );
+  PlatformRegisterFvBootOption (&gUefiShellFileGuid, L"UEFI Shell", 0, &Key);
 }
 
 /**
-- 
2.17.1

[PATCH v2 5/5] ArmPkg/PlatformBootManagerLib: don't connect all devices on each boot

[Ard Biesheuvel]

In order to avoid boot delays from devices such as network controllers
that may not even be involved in booting at all, drop the call to
EfiBootManagerConnectAll () from the boot path. It will be called by
UiApp, so when going through the menu, all devices will be connected
as usual, but for the default boot, it is really not necessary so
let's get rid of this.

Enumerating all possible boot options and creating Boot#### variables
for them is equally unnecessary in the default case, and also happens
automatically in UiApp, so drop that as well.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---
 ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
index 1e9b736993d0..15c5cac1bea0 100644
--- a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
+++ b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
@@ -753,11 +753,6 @@ PlatformBootManagerAfterConsole (
     }
   }
 
-  //
-  // Connect the rest of the devices.
-  //
-  EfiBootManagerConnectAll ();
-
   //
   // On ARM, there is currently no reason to use the phased capsule
   // update approach where some capsules are dispatched before EndOfDxe
@@ -767,11 +762,6 @@ PlatformBootManagerAfterConsole (
   //
   HandleCapsules ();
 
-  //
-  // Enumerate all possible boot options.
-  //
-  EfiBootManagerRefreshAllBootOption ();
-
   //
   // Register UEFI Shell
   //
-- 
2.17.1

Re: [PATCH v2 3/5] MdeModulePkg/BootManagerUiLib: show inactive boot options

[Laszlo Ersek]

On 05/28/20 11:17, Ard Biesheuvel wrote:
> UEFI boot options may exist but have the LOAD_OPTION_ACTIVE flag
> cleared. This means that the boot option should not be selected
> by default, but it does not mean it should be omitted from the
> boot selection presented by the boot manager: for this purpose,
> another flag LOAD_OPTION_HIDDEN exists.
> 
> Given that the latter flag exists solely for the purpose of omitting
> boot options from the boot selection menu, and LOAD_OPTION_XXX flags
> can be combined if desired, hiding inactive boot options as well is
> a mistake, and violates the intent of paragraph 3.1.3 of the UEFI
> specification (revision 2.8 errata A). Let's fix this by dropping
> the LOAD_OPTION_ACTIVE check from the code that populates the boot
> selection menu.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
> ---
>  MdeModulePkg/Library/BootManagerUiLib/BootManager.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/MdeModulePkg/Library/BootManagerUiLib/BootManager.c b/MdeModulePkg/Library/BootManagerUiLib/BootManager.c
> index 13b40e11b396..4b2c4c77a124 100644
> --- a/MdeModulePkg/Library/BootManagerUiLib/BootManager.c
> +++ b/MdeModulePkg/Library/BootManagerUiLib/BootManager.c
> @@ -535,9 +535,9 @@ UpdateBootManager (
>      mKeyInput++;
>  
>      //
> -    // Don't display the hidden/inactive boot option
> +    // Don't display hidden boot options, but retain inactive ones.
>      //
> -    if (((BootOption[Index].Attributes & LOAD_OPTION_HIDDEN) != 0) || ((BootOption[Index].Attributes & LOAD_OPTION_ACTIVE) == 0)) {
> +    if ((BootOption[Index].Attributes & LOAD_OPTION_HIDDEN) != 0) {
>        continue;
>      }
>  
> 

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Re: [PATCH v2 4/5] ArmPkg/PlatformBootManagerLib: hide UEFI Shell as a regular boot option

[Laszlo Ersek]

On 05/28/20 11:17, Ard Biesheuvel wrote:
> Without ConnectAll() being called on the boot path, the UEFI shell will
> be entered with no block devices or anything else connected, and so for
> the novice user, this is not a very accommodating environment. Now that
> we have made the UiApp the last resort on boot failure, and made the
> UEFI Shell accessible directly via the 's' hotkey if you really need
> it, let's hide it as an ordinary boot option.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
> ---
>  ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
> index 85cb32f6d7cd..1e9b736993d0 100644
> --- a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
> +++ b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
> @@ -777,9 +777,7 @@ PlatformBootManagerAfterConsole (
>    //
>    Key.ScanCode     = SCAN_NULL;
>    Key.UnicodeChar  = L's';
> -  PlatformRegisterFvBootOption (
> -    &gUefiShellFileGuid, L"UEFI Shell", LOAD_OPTION_ACTIVE, &Key
> -    );
> +  PlatformRegisterFvBootOption (&gUefiShellFileGuid, L"UEFI Shell", 0, &Key);
>  }
>  
>  /**
> 

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Re: [PATCH v2 2/5] ArmPkg/PlatformBootManagerLib: fall back to the UiApp on boot failure

[Leif Lindholm]

On Thu, May 28, 2020 at 11:17:38 +0200, Ard Biesheuvel wrote:
> As a last resort, drop into the UiApp application when no active boot
> options could be started. Doing so will connect all devices, and so
> it will allow the user to enter the Boot Manager submenu and pick a
> network or removable disk option.
> 
> Note that this only occurs if even the default removable filepath
> could not be booted (e.g., \EFI\BOOT\BOOTAA64.EFI on AArch64)
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Reviewed-by: Leif Lindholm <leif@nuviainc.com>
In fact, feel free to merge this one independently if set still being
discussed after stable tag is made.

> ---
>  ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
> index 23c925bbdb9c..85cb32f6d7cd 100644
> --- a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
> +++ b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
> @@ -830,5 +830,15 @@ PlatformBootManagerUnableToBoot (
>    VOID
>    )
>  {
> -  return;
> +  EFI_STATUS                   Status;
> +  EFI_BOOT_MANAGER_LOAD_OPTION BootManagerMenu;
> +
> +  Status = EfiBootManagerGetBootManagerMenu (&BootManagerMenu);
> +  if (EFI_ERROR (Status)) {
> +    return;
> +  }
> +
> +  for (;;) {
> +    EfiBootManagerBoot (&BootManagerMenu);
> +  }
>  }
> -- 
> 2.17.1
> 

Re: [PATCH v2 3/5] MdeModulePkg/BootManagerUiLib: show inactive boot options

[Leif Lindholm]

On Thu, May 28, 2020 at 11:17:39 +0200, Ard Biesheuvel wrote:
> UEFI boot options may exist but have the LOAD_OPTION_ACTIVE flag
> cleared. This means that the boot option should not be selected
> by default, but it does not mean it should be omitted from the
> boot selection presented by the boot manager: for this purpose,
> another flag LOAD_OPTION_HIDDEN exists.
> 
> Given that the latter flag exists solely for the purpose of omitting
> boot options from the boot selection menu, and LOAD_OPTION_XXX flags
> can be combined if desired, hiding inactive boot options as well is
> a mistake, and violates the intent of paragraph 3.1.3 of the UEFI
> specification (revision 2.8 errata A). Let's fix this by dropping
> the LOAD_OPTION_ACTIVE check from the code that populates the boot
> selection menu.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>

Acked-by: Leif Lindholm <leif@nuviainc.com>

> ---
>  MdeModulePkg/Library/BootManagerUiLib/BootManager.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/MdeModulePkg/Library/BootManagerUiLib/BootManager.c b/MdeModulePkg/Library/BootManagerUiLib/BootManager.c
> index 13b40e11b396..4b2c4c77a124 100644
> --- a/MdeModulePkg/Library/BootManagerUiLib/BootManager.c
> +++ b/MdeModulePkg/Library/BootManagerUiLib/BootManager.c
> @@ -535,9 +535,9 @@ UpdateBootManager (
>      mKeyInput++;
>  
>      //
> -    // Don't display the hidden/inactive boot option
> +    // Don't display hidden boot options, but retain inactive ones.
>      //
> -    if (((BootOption[Index].Attributes & LOAD_OPTION_HIDDEN) != 0) || ((BootOption[Index].Attributes & LOAD_OPTION_ACTIVE) == 0)) {
> +    if ((BootOption[Index].Attributes & LOAD_OPTION_HIDDEN) != 0) {
>        continue;
>      }
>  
> -- 
> 2.17.1
> 

Re: [PATCH v2 4/5] ArmPkg/PlatformBootManagerLib: hide UEFI Shell as a regular boot option

[Leif Lindholm]

On Thu, May 28, 2020 at 11:17:40 +0200, Ard Biesheuvel wrote:
> Without ConnectAll() being called on the boot path, the UEFI shell will
> be entered with no block devices or anything else connected, and so for
> the novice user, this is not a very accommodating environment. Now that
> we have made the UiApp the last resort on boot failure, and made the
> UEFI Shell accessible directly via the 's' hotkey if you really need
> it, let's hide it as an ordinary boot option.

Am I being confused here, or is the word "hide" a bit unfortunate in
the above sentence? (It'll still be visible in the UiApp menu, right?)

/
    Leif

> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
> ---
>  ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
> index 85cb32f6d7cd..1e9b736993d0 100644
> --- a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
> +++ b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
> @@ -777,9 +777,7 @@ PlatformBootManagerAfterConsole (
>    //
>    Key.ScanCode     = SCAN_NULL;
>    Key.UnicodeChar  = L's';
> -  PlatformRegisterFvBootOption (
> -    &gUefiShellFileGuid, L"UEFI Shell", LOAD_OPTION_ACTIVE, &Key
> -    );
> +  PlatformRegisterFvBootOption (&gUefiShellFileGuid, L"UEFI Shell", 0, &Key);
>  }
>  
>  /**
> -- 
> 2.17.1
> 

Re: [PATCH v2 4/5] ArmPkg/PlatformBootManagerLib: hide UEFI Shell as a regular boot option

[Ard Biesheuvel]

On 6/1/20 2:01 PM, Leif Lindholm wrote:
> On Thu, May 28, 2020 at 11:17:40 +0200, Ard Biesheuvel wrote:
>> Without ConnectAll() being called on the boot path, the UEFI shell will
>> be entered with no block devices or anything else connected, and so for
>> the novice user, this is not a very accommodating environment. Now that
>> we have made the UiApp the last resort on boot failure, and made the
>> UEFI Shell accessible directly via the 's' hotkey if you really need
>> it, let's hide it as an ordinary boot option.
> 
> Am I being confused here, or is the word "hide" a bit unfortunate in
> the above sentence? (It'll still be visible in the UiApp menu, right?)
> 

Ah yes, the wording is slightly off now that the UEFI shell is being 
kept accessible via the boot manager menu rather than via a completely 
separate root menu option and form.


> 
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
>> ---
>>   ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c | 4 +---
>>   1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
>> index 85cb32f6d7cd..1e9b736993d0 100644
>> --- a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
>> +++ b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
>> @@ -777,9 +777,7 @@ PlatformBootManagerAfterConsole (
>>     //
>>     Key.ScanCode     = SCAN_NULL;
>>     Key.UnicodeChar  = L's';
>> -  PlatformRegisterFvBootOption (
>> -    &gUefiShellFileGuid, L"UEFI Shell", LOAD_OPTION_ACTIVE, &Key
>> -    );
>> +  PlatformRegisterFvBootOption (&gUefiShellFileGuid, L"UEFI Shell", 0, &Key);
>>   }
>>   
>>   /**
>> -- 
>> 2.17.1
>>

Re: [PATCH v2 4/5] ArmPkg/PlatformBootManagerLib: hide UEFI Shell as a regular boot option

[Leif Lindholm]

On Mon, Jun 01, 2020 at 14:08:18 +0200, Ard Biesheuvel wrote:
> On 6/1/20 2:01 PM, Leif Lindholm wrote:
> > On Thu, May 28, 2020 at 11:17:40 +0200, Ard Biesheuvel wrote:
> > > Without ConnectAll() being called on the boot path, the UEFI shell will
> > > be entered with no block devices or anything else connected, and so for
> > > the novice user, this is not a very accommodating environment. Now that
> > > we have made the UiApp the last resort on boot failure, and made the
> > > UEFI Shell accessible directly via the 's' hotkey if you really need
> > > it, let's hide it as an ordinary boot option.
> > 
> > Am I being confused here, or is the word "hide" a bit unfortunate in
> > the above sentence? (It'll still be visible in the UiApp menu, right?)
> > 
> 
> Ah yes, the wording is slightly off now that the UEFI shell is being kept
> accessible via the boot manager menu rather than via a completely separate
> root menu option and form.

Right. Well, with that addessed - for all of the set (apart from the
MdeModulePkg one, which has an ACK):
Reviewed-by: Leif Lindholm <leif@nuviainc.com>

> 
> > 
> > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
> > > ---
> > >   ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c | 4 +---
> > >   1 file changed, 1 insertion(+), 3 deletions(-)
> > > 
> > > diff --git a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
> > > index 85cb32f6d7cd..1e9b736993d0 100644
> > > --- a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
> > > +++ b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
> > > @@ -777,9 +777,7 @@ PlatformBootManagerAfterConsole (
> > >     //
> > >     Key.ScanCode     = SCAN_NULL;
> > >     Key.UnicodeChar  = L's';
> > > -  PlatformRegisterFvBootOption (
> > > -    &gUefiShellFileGuid, L"UEFI Shell", LOAD_OPTION_ACTIVE, &Key
> > > -    );
> > > +  PlatformRegisterFvBootOption (&gUefiShellFileGuid, L"UEFI Shell", 0, &Key);
> > >   }
> > >   /**
> > > -- 
> > > 2.17.1
> > > 
> 

Re: [PATCH v2 3/5] MdeModulePkg/BootManagerUiLib: show inactive boot options

[Ard Biesheuvel]

Ray, Zhichao,

Do you have any comments on this patch?


On 5/28/20 11:17 AM, Ard Biesheuvel wrote:
> UEFI boot options may exist but have the LOAD_OPTION_ACTIVE flag
> cleared. This means that the boot option should not be selected
> by default, but it does not mean it should be omitted from the
> boot selection presented by the boot manager: for this purpose,
> another flag LOAD_OPTION_HIDDEN exists.
> 
> Given that the latter flag exists solely for the purpose of omitting
> boot options from the boot selection menu, and LOAD_OPTION_XXX flags
> can be combined if desired, hiding inactive boot options as well is
> a mistake, and violates the intent of paragraph 3.1.3 of the UEFI
> specification (revision 2.8 errata A). Let's fix this by dropping
> the LOAD_OPTION_ACTIVE check from the code that populates the boot
> selection menu.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
> ---
>   MdeModulePkg/Library/BootManagerUiLib/BootManager.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/MdeModulePkg/Library/BootManagerUiLib/BootManager.c b/MdeModulePkg/Library/BootManagerUiLib/BootManager.c
> index 13b40e11b396..4b2c4c77a124 100644
> --- a/MdeModulePkg/Library/BootManagerUiLib/BootManager.c
> +++ b/MdeModulePkg/Library/BootManagerUiLib/BootManager.c
> @@ -535,9 +535,9 @@ UpdateBootManager (
>       mKeyInput++;
>   
>       //
> -    // Don't display the hidden/inactive boot option
> +    // Don't display hidden boot options, but retain inactive ones.
>       //
> -    if (((BootOption[Index].Attributes & LOAD_OPTION_HIDDEN) != 0) || ((BootOption[Index].Attributes & LOAD_OPTION_ACTIVE) == 0)) {
> +    if ((BootOption[Index].Attributes & LOAD_OPTION_HIDDEN) != 0) {
>         continue;
>       }
>   
> 

Re: [PATCH v2 3/5] MdeModulePkg/BootManagerUiLib: show inactive boot options

[Gao, Zhichao]

Hi Ard,

Form the Uefi spec:
'''
If a load option is marked as LOAD_OPTION_ACTIVE, the boot manager will attempt to boot
automatically using the device path information in the load option. This provides an easy way to disable
or enable load options without needing to delete and re-add them.
'''
The ACTIVE flag seems to enable/disable the option.

'''
Boot#### load options with LOAD_OPTION_CATEGORY set to LOAD_OPTION_CATEGORY_APP are
executables which are not part of the normal boot processing but can be optionally chosen for execution
if boot menu is provided, or via Hot Keys. See Section 3.1.6 for details
'''
There is another flag to implement your function. The option with ACTIVE | CATEGORY | CATEGORY_APP should show in the boot menu but not in the normal boot flow.

Thanks,
Zhichao

> -----Original Message-----
> From: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Sent: Tuesday, June 2, 2020 4:26 PM
> To: devel@edk2.groups.io
> Cc: jon@solid-run.com; Laszlo Ersek <lersek@redhat.com>; Leif Lindholm
> <leif@nuviainc.com>; Ni, Ray <ray.ni@intel.com>; Gao, Zhichao
> <zhichao.gao@intel.com>
> Subject: Re: [PATCH v2 3/5] MdeModulePkg/BootManagerUiLib: show
> inactive boot options
> 
> Ray, Zhichao,
> 
> Do you have any comments on this patch?
> 
> 
> On 5/28/20 11:17 AM, Ard Biesheuvel wrote:
> > UEFI boot options may exist but have the LOAD_OPTION_ACTIVE flag
> > cleared. This means that the boot option should not be selected by
> > default, but it does not mean it should be omitted from the boot
> > selection presented by the boot manager: for this purpose, another
> > flag LOAD_OPTION_HIDDEN exists.
> >
> > Given that the latter flag exists solely for the purpose of omitting
> > boot options from the boot selection menu, and LOAD_OPTION_XXX flags
> > can be combined if desired, hiding inactive boot options as well is a
> > mistake, and violates the intent of paragraph 3.1.3 of the UEFI
> > specification (revision 2.8 errata A). Let's fix this by dropping the
> > LOAD_OPTION_ACTIVE check from the code that populates the boot
> > selection menu.
> >
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
> > ---
> >   MdeModulePkg/Library/BootManagerUiLib/BootManager.c | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/MdeModulePkg/Library/BootManagerUiLib/BootManager.c
> > b/MdeModulePkg/Library/BootManagerUiLib/BootManager.c
> > index 13b40e11b396..4b2c4c77a124 100644
> > --- a/MdeModulePkg/Library/BootManagerUiLib/BootManager.c
> > +++ b/MdeModulePkg/Library/BootManagerUiLib/BootManager.c
> > @@ -535,9 +535,9 @@ UpdateBootManager (
> >       mKeyInput++;
> >
> >       //
> > -    // Don't display the hidden/inactive boot option
> > +    // Don't display hidden boot options, but retain inactive ones.
> >       //
> > -    if (((BootOption[Index].Attributes & LOAD_OPTION_HIDDEN) != 0) ||
> ((BootOption[Index].Attributes & LOAD_OPTION_ACTIVE) == 0)) {
> > +    if ((BootOption[Index].Attributes & LOAD_OPTION_HIDDEN) != 0) {
> >         continue;
> >       }
> >
> >

Re: [PATCH v2 3/5] MdeModulePkg/BootManagerUiLib: show inactive boot options

[Ard Biesheuvel]

On 6/2/20 11:11 AM, Gao, Zhichao wrote:
> Hi Ard,
> 
> Form the Uefi spec:
> '''
> If a load option is marked as LOAD_OPTION_ACTIVE, the boot manager will attempt to boot
> automatically using the device path information in the load option. This provides an easy way to disable
> or enable load options without needing to delete and re-add them.
> '''
> The ACTIVE flag seems to enable/disable the option.
> 

Sure. But this change does not affect what gets booted automatically, it 
changes what gets presented as available options in the UiApp boot 
manager menu.

To hide options from this menu, a different flag HIDDEN is provided. The 
UEFI spec clearly mentions that
- the ACTIVE flag defines which boot options may be booted *automatically*
- the HIDDEN flag defines which boot options are hidden from the menu 
which allows options to be booted *manually*

So preventing ACTIVE options from being booted manually is incorrect. 
That is what the patch fixes.


> '''
> Boot#### load options with LOAD_OPTION_CATEGORY set to LOAD_OPTION_CATEGORY_APP are
> executables which are not part of the normal boot processing but can be optionally chosen for execution
> if boot menu is provided, or via Hot Keys. See Section 3.1.6 for details
> '''
> There is another flag to implement your function. The option with ACTIVE | CATEGORY | CATEGORY_APP should show in the boot menu but not in the normal boot flow.
> 

But this prevents me from setting BootNext to the Shell option, and boot 
it automatically on the next boot, right?

Re: [PATCH v2 3/5] MdeModulePkg/BootManagerUiLib: show inactive boot options

[Gao, Zhichao]

> -----Original Message-----
> From: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Sent: Tuesday, June 2, 2020 5:27 PM
> To: Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io
> Cc: jon@solid-run.com; Laszlo Ersek <lersek@redhat.com>; Leif Lindholm
> <leif@nuviainc.com>; Ni, Ray <ray.ni@intel.com>
> Subject: Re: [PATCH v2 3/5] MdeModulePkg/BootManagerUiLib: show
> inactive boot options
> 
> 
> 
> On 6/2/20 11:11 AM, Gao, Zhichao wrote:
> > Hi Ard,
> >
> > Form the Uefi spec:
> > '''
> > If a load option is marked as LOAD_OPTION_ACTIVE, the boot manager
> > will attempt to boot automatically using the device path information
> > in the load option. This provides an easy way to disable or enable load
> options without needing to delete and re-add them.
> > '''
> > The ACTIVE flag seems to enable/disable the option.
> >
> 
> Sure. But this change does not affect what gets booted automatically, it
> changes what gets presented as available options in the UiApp boot manager
> menu.
> 
> To hide options from this menu, a different flag HIDDEN is provided. The UEFI
> spec clearly mentions that
> - the ACTIVE flag defines which boot options may be booted *automatically*
> - the HIDDEN flag defines which boot options are hidden from the menu
> which allows options to be booted *manually*
> 
> So preventing ACTIVE options from being booted manually is incorrect.
> That is what the patch fixes.

I don't see the boot function prevent no-ACTIVE option to boot. So I agree with your point.

> 
> 
> > '''
> > Boot#### load options with LOAD_OPTION_CATEGORY set to
> > LOAD_OPTION_CATEGORY_APP are executables which are not part of the
> > normal boot processing but can be optionally chosen for execution if
> > boot menu is provided, or via Hot Keys. See Section 3.1.6 for details '''
> > There is another flag to implement your function. The option with ACTIVE |
> CATEGORY | CATEGORY_APP should show in the boot menu but not in the
> normal boot flow.
> >
> 
> But this prevents me from setting BootNext to the Shell option, and boot it
> automatically on the next boot, right?

Sorry, I miss the requirement of next boot. Next boot would called by EfiBootManagerBoot.
And the APP flag only affect BmSetMemoryTypeInformationVariable result. I don't think it would affect the next boot running.

Anyway, I agree with your patch and thanks for your explain.

Reviewed-by: Zhichao Gao <zhichao.gao@intel.com>

Thanks,
Zhichao