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

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

[Ard Biesheuvel]

Currently, Armpkg's PlatformBootManagerLib connects all controller 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 hidden, so it is not started by default
  (only by hotkey or BootNext)
- remove the ConnectAll() call from PlatformBootManagerLib
- finally, add a plugin library for UiApp to expose the UEFI Shell via an
  ordinary main menu option (this works around the fact that patch #3 will
  result in the UEFI Shell disappearing from the Boot Manager list).
  Entering the shell this way will resemble the old situation, given that
  UiApp connects all devices and refreshes all boot options etc at launch.

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
  ArmPkg/PlatformBootManagerLib: hide UEFI Shell as a regular boot
    option
  ArmPkg/PlatformBootManagerLib: don't connect all devices on each boot
  ShellPkg: add BootManager library to add UEFI Shell menu option

 .../ShellBootManagerLib.inf                   |  45 +++
 .../ShellBootManagerLib/ShellBootManagerLib.h |  44 +++
 .../PlatformBootManagerLib/PlatformBm.c       |  37 ++-
 .../ShellBootManagerLib/ShellBootManagerLib.c | 258 ++++++++++++++++++
 .../ShellBootManagerLib/ShellBmStrings.uni    |  17 ++
 .../ShellBootManagerLib/ShellBmVfr.Vfr        |  37 +++
 6 files changed, 425 insertions(+), 13 deletions(-)
 create mode 100644 ShellPkg/Library/ShellBootManagerLib/ShellBootManagerLib.inf
 create mode 100644 ShellPkg/Library/ShellBootManagerLib/ShellBootManagerLib.h
 create mode 100644 ShellPkg/Library/ShellBootManagerLib/ShellBootManagerLib.c
 create mode 100644 ShellPkg/Library/ShellBootManagerLib/ShellBmStrings.uni
 create mode 100644 ShellPkg/Library/ShellBootManagerLib/ShellBmVfr.Vfr

-- 
2.17.1

[PATCH 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>
---
 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 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. With the right UiApp library added
in, the UiApp also gives access to the UEFI Shell.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
---
 ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
index 23c925bbdb9c..f91f7cd09ca1 100644
--- a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
+++ b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
@@ -830,5 +830,19 @@ PlatformBootManagerUnableToBoot (
   VOID
   )
 {
-  return;
+  EFI_STATUS                   Status;
+  EFI_BOOT_MANAGER_LOAD_OPTION BootManagerMenu;
+
+  //
+  // BootManagerMenu doesn't contain the correct information when return status
+  // is EFI_NOT_FOUND.
+  //
+  Status = EfiBootManagerGetBootManagerMenu (&BootManagerMenu);
+  if (EFI_ERROR (Status)) {
+    return;
+  }
+
+  for (;;) {
+    EfiBootManagerBoot (&BootManagerMenu);
+  }
 }
-- 
2.17.1

[PATCH 3/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 when 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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
index f91f7cd09ca1..b465f9ff388f 100644
--- a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
+++ b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
@@ -778,7 +778,7 @@ PlatformBootManagerAfterConsole (
   Key.ScanCode     = SCAN_NULL;
   Key.UnicodeChar  = L's';
   PlatformRegisterFvBootOption (
-    &gUefiShellFileGuid, L"UEFI Shell", LOAD_OPTION_ACTIVE, &Key
+    &gUefiShellFileGuid, L"UEFI Shell", LOAD_OPTION_HIDDEN, &Key
     );
 }
 
-- 
2.17.1

[PATCH 4/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>
---
 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 b465f9ff388f..618072405a50 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

[PATCH 5/5] ShellPkg: add BootManager library to add UEFI Shell menu option

[Ard Biesheuvel]

Add a plug-in library for UiApp that creates a 'UEFI Shell' menu
option at the root level which gives access to a form that allows
the UEFI Shell to be launched.

This gives the PlatformBootManagerLib implementation of the platform
more flexibility in the way it handles boot options pointing to the
UEFI Shell, which will typically be invoked with only the boot path
connected on fast boots.

This library may be incorporated to a platform build by adding a
NULL resolution to the <LibraryClasses> section of the UiApp.inf
{} block in the platform .DSC

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
---
 ShellPkg/Library/ShellBootManagerLib/ShellBootManagerLib.inf |  45 ++++
 ShellPkg/Library/ShellBootManagerLib/ShellBootManagerLib.h   |  44 ++++
 ShellPkg/Library/ShellBootManagerLib/ShellBootManagerLib.c   | 258 ++++++++++++++++++++
 ShellPkg/Library/ShellBootManagerLib/ShellBmStrings.uni      |  17 ++
 ShellPkg/Library/ShellBootManagerLib/ShellBmVfr.Vfr          |  37 +++
 5 files changed, 401 insertions(+)

diff --git a/ShellPkg/Library/ShellBootManagerLib/ShellBootManagerLib.inf b/ShellPkg/Library/ShellBootManagerLib/ShellBootManagerLib.inf
new file mode 100644
index 000000000000..d8b56268c08f
--- /dev/null
+++ b/ShellPkg/Library/ShellBootManagerLib/ShellBootManagerLib.inf
@@ -0,0 +1,45 @@
+## @file
+#
+#  Boot Maintenance Manager Library to add a 'UEFI Shell' option to UiApp
+#
+#  Copyright (c) 2020, Arm Ltd. All rights reserved.<BR>
+#  SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+##
+
+[Defines]
+  INF_VERSION                    = 1.27
+  BASE_NAME                      = ShellBootManagerLib
+  FILE_GUID                      = f84d949a-1ffd-447e-903d-5def3c34040b
+  MODULE_TYPE                    = DXE_DRIVER
+  VERSION_STRING                 = 1.0
+  LIBRARY_CLASS                  = NULL|DXE_DRIVER UEFI_APPLICATION
+  CONSTRUCTOR                    = ShellBootManagerLibConstructor
+  DESTRUCTOR                     = ShellBootManagerLibDestructor
+
+[Sources]
+  ShellBootManagerLib.c
+  ShellBootManagerLib.h
+  ShellBmVfr.Vfr
+  ShellBmStrings.uni
+
+[Packages]
+  MdePkg/MdePkg.dec
+  MdeModulePkg/MdeModulePkg.dec
+  ShellPkg/ShellPkg.dec
+
+[LibraryClasses]
+  DebugLib
+  DevicePathLib
+  DxeServicesLib
+  HiiLib
+  MemoryAllocationLib
+  UefiBootServicesTableLib
+
+[Guids]
+  gEfiIfrFrontPageGuid                          ## CONSUMES ## GUID
+  gUefiShellFileGuid                            ## CONSUMES ## GUID
+
+[Protocols]
+  gEfiHiiConfigAccessProtocolGuid               ## PRODUCES
+  gEfiDevicePathProtocolGuid                    ## PRODUCES
diff --git a/ShellPkg/Library/ShellBootManagerLib/ShellBootManagerLib.h b/ShellPkg/Library/ShellBootManagerLib/ShellBootManagerLib.h
new file mode 100644
index 000000000000..e9cdfb6a8a64
--- /dev/null
+++ b/ShellPkg/Library/ShellBootManagerLib/ShellBootManagerLib.h
@@ -0,0 +1,44 @@
+/** @file
+
+  Boot Maintenance Manager Library to add a 'UEFI Shell' option to UiApp
+
+  Copyright (c) 2020, Arm Ltd. All rights reserved.<BR>
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include <Uefi/UefiBaseType.h>
+
+#include <Protocol/DevicePath.h>
+#include <Protocol/HiiConfigAccess.h>
+
+extern UINT8 ShellBmVfrBin[];
+
+#define FORMSET_GUID  { 0x54335e64, 0x4ebc, 0x4f7d, { 0x8a, 0x9a, 0x94, 0x10, 0xf5, 0x53, 0xae, 0x9d } }
+
+///
+/// HII specific Vendor Device Path definition.
+///
+#pragma pack(1)
+typedef struct {
+  VENDOR_DEVICE_PATH             VendorDevicePath;
+  EFI_DEVICE_PATH_PROTOCOL       End;
+} HII_VENDOR_DEVICE_PATH;
+#pragma pack()
+
+#define SHELL_BM_CALLBACK_DATA_SIGNATURE  SIGNATURE_32 ('S', 'B', 'C', 'D')
+
+typedef struct {
+  UINTN                           Signature;
+
+  //
+  // HII relative handles
+  //
+  EFI_HII_HANDLE                  HiiHandle;
+  EFI_HANDLE                      DriverHandle;
+
+  //
+  // Produced protocols
+  //
+  EFI_HII_CONFIG_ACCESS_PROTOCOL   ConfigAccess;
+} SHELL_BM_CALLBACK_DATA;
diff --git a/ShellPkg/Library/ShellBootManagerLib/ShellBootManagerLib.c b/ShellPkg/Library/ShellBootManagerLib/ShellBootManagerLib.c
new file mode 100644
index 000000000000..195f50601dc0
--- /dev/null
+++ b/ShellPkg/Library/ShellBootManagerLib/ShellBootManagerLib.c
@@ -0,0 +1,258 @@
+/** @file
+
+  Boot Maintenance Manager Library to add a 'UEFI Shell' option to UiApp
+
+  Copyright (c) 2020, Arm Ltd. All rights reserved.<BR>
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include <PiDxe.h>
+
+#include <Library/DebugLib.h>
+#include <Library/DevicePathLib.h>
+#include <Library/DxeServicesLib.h>
+#include <Library/HiiLib.h>
+#include <Library/MemoryAllocationLib.h>
+#include <Library/UefiBootServicesTableLib.h>
+
+#include "ShellBootManagerLib.h"
+
+STATIC CONST EFI_GUID             mFormsetGuid = FORMSET_GUID;
+STATIC EFI_DEVICE_PATH_PROTOCOL   *mShellFileDevicePath;
+
+STATIC HII_VENDOR_DEVICE_PATH     mShellBmHiiVendorDevicePath = {
+  {
+    {
+      HARDWARE_DEVICE_PATH,
+      HW_VENDOR_DP,
+      {
+        (UINT8) (sizeof (VENDOR_DEVICE_PATH)),
+        (UINT8) ((sizeof (VENDOR_DEVICE_PATH)) >> 8)
+      }
+    },
+    //
+    // File GUID: f84d949a-1ffd-447e-903d-5def3c34040b
+    //
+    { 0xf84d949a, 0x1ffd, 0x447e, { 0x90, 0x3d, 0x5d, 0xef, 0x3c, 0x34, 0x04, 0x0b } }
+  },
+  {
+    END_DEVICE_PATH_TYPE,
+    END_ENTIRE_DEVICE_PATH_SUBTYPE,
+    {
+      (UINT8) (END_DEVICE_PATH_LENGTH),
+      (UINT8) ((END_DEVICE_PATH_LENGTH) >> 8)
+    }
+  }
+};
+
+/**
+  This function allows a caller to extract the current configuration for one
+  or more named elements from the target driver.
+
+
+  @param This            Points to the EFI_HII_CONFIG_ACCESS_PROTOCOL.
+  @param Request         A null-terminated Unicode string in <ConfigRequest> format.
+  @param Progress        On return, points to a character in the Request string.
+                         Points to the string's null terminator if request was successful.
+                         Points to the most recent '&' before the first failing name/value
+                         pair (or the beginning of the string if the failure is in the
+                         first name/value pair) if the request was not successful.
+  @param Results         A null-terminated Unicode string in <ConfigAltResp> format which
+                         has all values filled in for the names in the Request string.
+                         String to be allocated by the called function.
+
+  @retval  EFI_SUCCESS            The Results is filled with the requested values.
+  @retval  EFI_OUT_OF_RESOURCES   Not enough memory to store the results.
+  @retval  EFI_INVALID_PARAMETER  Request is illegal syntax, or unknown name.
+  @retval  EFI_NOT_FOUND          Routing data doesn't match any storage in this driver.
+
+**/
+STATIC
+EFI_STATUS
+EFIAPI
+ShellBmExtractConfig (
+  IN  CONST EFI_HII_CONFIG_ACCESS_PROTOCOL   *This,
+  IN  CONST EFI_STRING                       Request,
+  OUT EFI_STRING                             *Progress,
+  OUT EFI_STRING                             *Results
+  )
+{
+  if (Progress == NULL || Results == NULL) {
+    return EFI_INVALID_PARAMETER;
+  }
+  *Progress = Request;
+  return EFI_NOT_FOUND;
+}
+
+/**
+  This function processes the results of changes in configuration.
+
+
+  @param This            Points to the EFI_HII_CONFIG_ACCESS_PROTOCOL.
+  @param Configuration   A null-terminated Unicode string in <ConfigResp> format.
+  @param Progress        A pointer to a string filled in with the offset of the most
+                         recent '&' before the first failing name/value pair (or the
+                         beginning of the string if the failure is in the first
+                         name/value pair) or the terminating NULL if all was successful.
+
+  @retval  EFI_SUCCESS            The Results is processed successfully.
+  @retval  EFI_INVALID_PARAMETER  Configuration is NULL.
+  @retval  EFI_NOT_FOUND          Routing data doesn't match any storage in this driver.
+
+**/
+STATIC
+EFI_STATUS
+EFIAPI
+ShellBmRouteConfig (
+  IN  CONST EFI_HII_CONFIG_ACCESS_PROTOCOL   *This,
+  IN  CONST EFI_STRING                       Configuration,
+  OUT EFI_STRING                             *Progress
+  )
+{
+  if (Configuration == NULL || Progress == NULL) {
+    return EFI_INVALID_PARAMETER;
+  }
+
+  *Progress = Configuration;
+
+  return EFI_NOT_FOUND;
+}
+
+/**
+  This function processes the results of changes in configuration.
+
+
+  @param This            Points to the EFI_HII_CONFIG_ACCESS_PROTOCOL.
+  @param Action          Specifies the type of action taken by the browser.
+  @param QuestionId      A unique value which is sent to the original exporting driver
+                         so that it can identify the type of data to expect.
+  @param Type            The type of value for the question.
+  @param Value           A pointer to the data being sent to the original exporting driver.
+  @param ActionRequest   On return, points to the action requested by the callback function.
+
+  @retval  EFI_SUCCESS           The callback successfully handled the action.
+  @retval  EFI_OUT_OF_RESOURCES  Not enough storage is available to hold the variable and its data.
+  @retval  EFI_DEVICE_ERROR      The variable could not be saved.
+  @retval  EFI_UNSUPPORTED       The specified Action is not supported by the callback.
+
+**/
+STATIC
+EFI_STATUS
+EFIAPI
+ShellBmHiiCallback (
+  IN  CONST EFI_HII_CONFIG_ACCESS_PROTOCOL   *This,
+  IN  EFI_BROWSER_ACTION                     Action,
+  IN  EFI_QUESTION_ID                        QuestionId,
+  IN  UINT8                                  Type,
+  IN  EFI_IFR_TYPE_VALUE                     *Value,
+  OUT EFI_BROWSER_ACTION_REQUEST             *ActionRequest
+  )
+{
+  EFI_HANDLE    ImageHandle;
+  EFI_STATUS    Status;
+
+  if (Action != EFI_BROWSER_ACTION_CHANGED) {
+    return EFI_SUCCESS;
+  }
+
+  Status = gBS->LoadImage (TRUE, gImageHandle, mShellFileDevicePath, NULL, 0,
+                  &ImageHandle);
+  ASSERT_EFI_ERROR (Status);
+
+  //
+  // Clear  the  screen  before.
+  //
+  gST->ConOut->SetAttribute (gST->ConOut, EFI_TEXT_ATTR (EFI_LIGHTGRAY, EFI_BLACK));
+  gST->ConOut->ClearScreen (gST->ConOut);
+
+  Status = gBS->StartImage (ImageHandle, NULL, NULL);
+  ASSERT_EFI_ERROR (Status);
+  if (EFI_ERROR (Status)) {
+    DEBUG ((DEBUG_WARN, "%a: UEFI Shell returned with status %r\n",
+      __FUNCTION__, Status));
+  }
+  return EFI_SUCCESS;
+}
+
+STATIC SHELL_BM_CALLBACK_DATA     mShellBmPrivate = {
+  SHELL_BM_CALLBACK_DATA_SIGNATURE,
+  NULL,
+  NULL,
+  {
+    ShellBmExtractConfig,
+    ShellBmRouteConfig,
+    ShellBmHiiCallback
+  }
+};
+
+EFI_STATUS
+EFIAPI
+ShellBootManagerLibConstructor (
+  IN EFI_HANDLE        ImageHandle,
+  IN EFI_SYSTEM_TABLE  *SystemTable
+  )
+{
+  EFI_STATUS      Status;
+
+  Status = GetFileDevicePathFromAnyFv (&gUefiShellFileGuid, EFI_SECTION_PE32,
+             0, &mShellFileDevicePath);
+  if (EFI_ERROR (Status)) {
+    DEBUG ((DEBUG_WARN, "%a: failed to locate UEFI shell by GUID %g - %r\n",
+      __FUNCTION__, &gUefiShellFileGuid, Status));
+    return EFI_SUCCESS;
+  }
+
+  //
+  // Install Device Path Protocol and Config Access protocol to driver handle
+  //
+  mShellBmPrivate.DriverHandle = NULL;
+  Status = gBS->InstallMultipleProtocolInterfaces (
+                  &mShellBmPrivate.DriverHandle,
+                  &gEfiDevicePathProtocolGuid,
+                  &mShellBmHiiVendorDevicePath,
+                  &gEfiHiiConfigAccessProtocolGuid,
+                  &mShellBmPrivate.ConfigAccess,
+                  NULL
+                  );
+  ASSERT_EFI_ERROR (Status);
+
+  //
+  // Publish our HII data
+  //
+  mShellBmPrivate.HiiHandle = HiiAddPackages (
+                                &mFormsetGuid,
+                                mShellBmPrivate.DriverHandle,
+                                ShellBmVfrBin,
+                                ShellBootManagerLibStrings,
+                                NULL
+                                );
+  ASSERT (mShellBmPrivate.HiiHandle != NULL);
+
+  return EFI_SUCCESS;
+}
+
+EFI_STATUS
+EFIAPI
+ShellBootManagerLibDestructor (
+  IN EFI_HANDLE        ImageHandle,
+  IN EFI_SYSTEM_TABLE  *SystemTable
+  )
+{
+  EFI_STATUS      Status;
+
+  Status = gBS->UninstallMultipleProtocolInterfaces (
+                  mShellBmPrivate.DriverHandle,
+                  &gEfiDevicePathProtocolGuid,
+                  &mShellBmHiiVendorDevicePath,
+                  &gEfiHiiConfigAccessProtocolGuid,
+                  &mShellBmPrivate.ConfigAccess,
+                  NULL
+                  );
+  ASSERT_EFI_ERROR (Status);
+
+  HiiRemovePackages (mShellBmPrivate.HiiHandle);
+
+  FreePool (mShellFileDevicePath);
+  return EFI_SUCCESS;
+}
diff --git a/ShellPkg/Library/ShellBootManagerLib/ShellBmStrings.uni b/ShellPkg/Library/ShellBootManagerLib/ShellBmStrings.uni
new file mode 100644
index 000000000000..6e1962b6d0ec
--- /dev/null
+++ b/ShellPkg/Library/ShellBootManagerLib/ShellBmStrings.uni
@@ -0,0 +1,17 @@
+///** @file
+//
+// Copyright (c) 2020, Arm Ltd. All rights reserved.<BR>
+//
+// SPDX-License-Identifier: BSD-2-Clause-Patent
+//
+// --*/
+
+/=#
+#langdef   en-US "English"
+
+#string STR_SHELL_BM_BANNER            #language en-US  "UEFI Shell"
+#string STR_SHELL_BM_HELP              #language en-US  "This selection will take you to the UEFI Shell"
+#string STR_SHELL_BM_BANNER_FORM_TITLE #language en-US  "UEFI Shell Menu"
+#string STR_SHELL_BM_ENTER_SHELL       #language en-US  "Enter the UEFI Shell"
+#string STR_SHELL_BM_ENTER_SHELL_HELP  #language en-US  "Select this option to enter the UEFI Shell"
+#string STR_LAST_STRING                #language en-US  ""
diff --git a/ShellPkg/Library/ShellBootManagerLib/ShellBmVfr.Vfr b/ShellPkg/Library/ShellBootManagerLib/ShellBmVfr.Vfr
new file mode 100644
index 000000000000..bdff98c7c07a
--- /dev/null
+++ b/ShellPkg/Library/ShellBootManagerLib/ShellBmVfr.Vfr
@@ -0,0 +1,37 @@
+///** @file
+//
+//  UEFI Shell Boot Manager formset.
+//
+//  Copyright (c) 2004 - 2015, Intel Corporation. All rights reserved.<BR>
+//  Copyright (c) 2020, Arm Ltd. All rights reserved.<BR>
+//
+//  SPDX-License-Identifier: BSD-2-Clause-Patent
+//
+//**/
+
+#define FORMSET_GUID  { 0x54335e64, 0x4ebc, 0x4f7d, 0x8a, 0x9a, 0x94, 0x10, 0xf5, 0x53, 0xae, 0x9d }
+
+#define BOOT_MANAGER_FORM_ID     0x1000
+
+formset
+  guid      = FORMSET_GUID,
+  title     = STRING_TOKEN(STR_SHELL_BM_BANNER),
+  help      = STRING_TOKEN(STR_SHELL_BM_HELP),
+  classguid = gEfiIfrFrontPageGuid,
+
+  form formid = BOOT_MANAGER_FORM_ID,
+       title  = STRING_TOKEN(STR_SHELL_BM_BANNER);
+
+    subtitle text = STRING_TOKEN(STR_LAST_STRING);
+    subtitle text = STRING_TOKEN(STR_SHELL_BM_BANNER_FORM_TITLE);
+    subtitle text = STRING_TOKEN(STR_LAST_STRING);
+
+    text
+        help  = STRING_TOKEN(STR_SHELL_BM_ENTER_SHELL_HELP),
+        text  = STRING_TOKEN(STR_SHELL_BM_ENTER_SHELL),
+        flags = INTERACTIVE,
+        key   = 0x1;
+
+  endform;
+
+endformset;
-- 
2.17.1

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

[Leif Lindholm]

On Tue, May 26, 2020 at 18:13:56 +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. With the right UiApp library added
> in, the UiApp also gives access to the UEFI Shell.
>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
> ---
>  ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
> index 23c925bbdb9c..f91f7cd09ca1 100644
> --- a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
> +++ b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
> @@ -830,5 +830,19 @@ PlatformBootManagerUnableToBoot (
>    VOID
>    )
>  {
> -  return;
> +  EFI_STATUS                   Status;
> +  EFI_BOOT_MANAGER_LOAD_OPTION BootManagerMenu;
> +
> +  //
> +  // BootManagerMenu doesn't contain the correct information when return status
> +  // is EFI_NOT_FOUND.
> +  //
> +  Status = EfiBootManagerGetBootManagerMenu (&BootManagerMenu);
> +  if (EFI_ERROR (Status)) {

Nitpick: comment explicitly mentions EFI_NOT_FOUND, but code checks
for any EFI_ERROR match. Since there are various other error codes
that could be returned, change the comment to "when return status is
not EFI_SUCCESS"?

/
    Leif

> +    return;
> +  }
> +
> +  for (;;) {
> +    EfiBootManagerBoot (&BootManagerMenu);
> +  }
>  }
> -- 
> 2.17.1
> 
> 
> 
> 

Re: [edk2-devel] [PATCH 3/5] ArmPkg/PlatformBootManagerLib: hide UEFI Shell as a regular boot option

[Leif Lindholm]

On Tue, May 26, 2020 at 18:13:57 +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 when on boot failure, and made
> the UEFI Shell accessible directly via the 's hotkey if you really need

Typo 's -> 's'.

/
    Leif

> it, let's hide it as an ordinary boot option.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
> ---
>  ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
> index f91f7cd09ca1..b465f9ff388f 100644
> --- a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
> +++ b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
> @@ -778,7 +778,7 @@ PlatformBootManagerAfterConsole (
>    Key.ScanCode     = SCAN_NULL;
>    Key.UnicodeChar  = L's';
>    PlatformRegisterFvBootOption (
> -    &gUefiShellFileGuid, L"UEFI Shell", LOAD_OPTION_ACTIVE, &Key
> +    &gUefiShellFileGuid, L"UEFI Shell", LOAD_OPTION_HIDDEN, &Key
>      );
>  }
>  
> -- 
> 2.17.1
> 
> 
> 
> 

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

[Leif Lindholm]

On Tue, May 26, 2020 at 18:13:54 +0200, Ard Biesheuvel wrote:
> Currently, Armpkg's PlatformBootManagerLib connects all controller 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 hidden, so it is not started by default
>   (only by hotkey or BootNext)
> - remove the ConnectAll() call from PlatformBootManagerLib
> - finally, add a plugin library for UiApp to expose the UEFI Shell via an
>   ordinary main menu option (this works around the fact that patch #3 will
>   result in the UEFI Shell disappearing from the Boot Manager list).
>   Entering the shell this way will resemble the old situation, given that
>   UiApp connects all devices and refreshes all boot options etc at launch.

I get why this set was sent in isolation, but could we also discuss
somewhat what we would plan to do with the edk2-platforms that make
use of the ArmPkg PlatformBootManagerLib?

Not attempting a fallback boot onto network is probably an acceptable
change to pick up, but having an undocumented hotkey as the only way
into a shell that now doesn't map devices could be a bit aggravating.

/
    Leif

> 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
>   ArmPkg/PlatformBootManagerLib: hide UEFI Shell as a regular boot
>     option
>   ArmPkg/PlatformBootManagerLib: don't connect all devices on each boot
>   ShellPkg: add BootManager library to add UEFI Shell menu option
> 
>  .../ShellBootManagerLib.inf                   |  45 +++
>  .../ShellBootManagerLib/ShellBootManagerLib.h |  44 +++
>  .../PlatformBootManagerLib/PlatformBm.c       |  37 ++-
>  .../ShellBootManagerLib/ShellBootManagerLib.c | 258 ++++++++++++++++++
>  .../ShellBootManagerLib/ShellBmStrings.uni    |  17 ++
>  .../ShellBootManagerLib/ShellBmVfr.Vfr        |  37 +++
>  6 files changed, 425 insertions(+), 13 deletions(-)
>  create mode 100644 ShellPkg/Library/ShellBootManagerLib/ShellBootManagerLib.inf
>  create mode 100644 ShellPkg/Library/ShellBootManagerLib/ShellBootManagerLib.h
>  create mode 100644 ShellPkg/Library/ShellBootManagerLib/ShellBootManagerLib.c
>  create mode 100644 ShellPkg/Library/ShellBootManagerLib/ShellBmStrings.uni
>  create mode 100644 ShellPkg/Library/ShellBootManagerLib/ShellBmVfr.Vfr
> 
> -- 
> 2.17.1
> 

Re: [edk2-devel] [PATCH 0/5] ArmPkg/PlatformBootManagerLib: play nice without ConnectAll()

[Ard Biesheuvel]

On 5/27/20 12:01 AM, Leif Lindholm via groups.io wrote:
> On Tue, May 26, 2020 at 18:13:54 +0200, Ard Biesheuvel wrote:
>> Currently, Armpkg's PlatformBootManagerLib connects all controller 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 hidden, so it is not started by default
>>    (only by hotkey or BootNext)
>> - remove the ConnectAll() call from PlatformBootManagerLib
>> - finally, add a plugin library for UiApp to expose the UEFI Shell via an
>>    ordinary main menu option (this works around the fact that patch #3 will
>>    result in the UEFI Shell disappearing from the Boot Manager list).
>>    Entering the shell this way will resemble the old situation, given that
>>    UiApp connects all devices and refreshes all boot options etc at launch.
> 
> I get why this set was sent in isolation, but could we also discuss
> somewhat what we would plan to do with the edk2-platforms that make
> use of the ArmPkg PlatformBootManagerLib?
> 
> Not attempting a fallback boot onto network is probably an acceptable
> change to pick up, but having an undocumented hotkey as the only way
> into a shell that now doesn't map devices could be a bit aggravating.
> 

It is not the only way, and it is not even the preferred way. Patch 5/5 
adds an option to the UiApp root menu to enter the UEFI Shell, in a way 
that is independent from boot option handling. Since you enter UiApp 
first, all handles will be connected and boot options refreshed as usual.

In cases where you want to avoid this from happening, you can use the 
's' key to drop into a shell directly.

Re: [edk2-devel] [PATCH 0/5] ArmPkg/PlatformBootManagerLib: play nice without ConnectAll()

[Leif Lindholm]

On Wed, May 27, 2020 at 07:35:18 +0200, Ard Biesheuvel wrote:
> On 5/27/20 12:01 AM, Leif Lindholm via groups.io wrote:
> > On Tue, May 26, 2020 at 18:13:54 +0200, Ard Biesheuvel wrote:
> > > Currently, Armpkg's PlatformBootManagerLib connects all controller 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 hidden, so it is not started by default
> > >    (only by hotkey or BootNext)
> > > - remove the ConnectAll() call from PlatformBootManagerLib
> > > - finally, add a plugin library for UiApp to expose the UEFI Shell via an
> > >    ordinary main menu option (this works around the fact that patch #3 will
> > >    result in the UEFI Shell disappearing from the Boot Manager list).
> > >    Entering the shell this way will resemble the old situation, given that
> > >    UiApp connects all devices and refreshes all boot options etc at launch.
> > 
> > I get why this set was sent in isolation, but could we also discuss
> > somewhat what we would plan to do with the edk2-platforms that make
> > use of the ArmPkg PlatformBootManagerLib?
> > 
> > Not attempting a fallback boot onto network is probably an acceptable
> > change to pick up, but having an undocumented hotkey as the only way
> > into a shell that now doesn't map devices could be a bit aggravating.
> > 
> 
> It is not the only way, and it is not even the preferred way. Patch 5/5 adds
> an option to the UiApp root menu to enter the UEFI Shell, in a way that is
> independent from boot option handling. Since you enter UiApp first, all
> handles will be connected and boot options refreshed as usual.
> 
> In cases where you want to avoid this from happening, you can use the 's'
> key to drop into a shell directly.

Yes, that's exactly what I am referring to. But in order for the new
(and I agree improved) functionality to be available, the new Shell
library needs to be explicitly added to .dsc for the platforms affected.

I want an active decision to be made about how that is going to
happen, if it is going to happen, as part of the conversation about
*this* set. Merging this and *then* looking into it makes for too
harsh a break in behaviour.

/
    Leif

Re: [edk2-devel] [PATCH 0/5] ArmPkg/PlatformBootManagerLib: play nice without ConnectAll()

[Ard Biesheuvel]

On 5/27/20 12:43 PM, Leif Lindholm wrote:
> On Wed, May 27, 2020 at 07:35:18 +0200, Ard Biesheuvel wrote:
>> On 5/27/20 12:01 AM, Leif Lindholm via groups.io wrote:
>>> On Tue, May 26, 2020 at 18:13:54 +0200, Ard Biesheuvel wrote:
>>>> Currently, Armpkg's PlatformBootManagerLib connects all controller 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 hidden, so it is not started by default
>>>>     (only by hotkey or BootNext)
>>>> - remove the ConnectAll() call from PlatformBootManagerLib
>>>> - finally, add a plugin library for UiApp to expose the UEFI Shell via an
>>>>     ordinary main menu option (this works around the fact that patch #3 will
>>>>     result in the UEFI Shell disappearing from the Boot Manager list).
>>>>     Entering the shell this way will resemble the old situation, given that
>>>>     UiApp connects all devices and refreshes all boot options etc at launch.
>>>
>>> I get why this set was sent in isolation, but could we also discuss
>>> somewhat what we would plan to do with the edk2-platforms that make
>>> use of the ArmPkg PlatformBootManagerLib?
>>>
>>> Not attempting a fallback boot onto network is probably an acceptable
>>> change to pick up, but having an undocumented hotkey as the only way
>>> into a shell that now doesn't map devices could be a bit aggravating.
>>>
>>
>> It is not the only way, and it is not even the preferred way. Patch 5/5 adds
>> an option to the UiApp root menu to enter the UEFI Shell, in a way that is
>> independent from boot option handling. Since you enter UiApp first, all
>> handles will be connected and boot options refreshed as usual.
>>
>> In cases where you want to avoid this from happening, you can use the 's'
>> key to drop into a shell directly.
> 
> Yes, that's exactly what I am referring to. But in order for the new
> (and I agree improved) functionality to be available, the new Shell
> library needs to be explicitly added to .dsc for the platforms affected.
> 
> I want an active decision to be made about how that is going to
> happen, if it is going to happen, as part of the conversation about
> *this* set. Merging this and *then* looking into it makes for too
> harsh a break in behaviour.
> 

All existing platforms that incorporate ArmPkg's PlatformBootManagerLib 
must add this NULL library class resolution to UiApp first. So once we 
agree that this approach is acceptable (including the change to 
ShellPkg), I can prepare a patch for edk2-platforms implementing this 
for all such platforms living there. I suggest that we don't do the 
3-way handshake here (edk2 pre, edk2-platforms, edk2 post), given that 
the build won't break, it's just that if you pull and build your 
platform right at the minute between merging the edk2 changes and the 
edk2-platform ones, you will see the slightly unintuitive behavior if 
you also happen to clear your Boot#### variables at the same time.

Re: [edk2-devel] [PATCH 0/5] ArmPkg/PlatformBootManagerLib: play nice without ConnectAll()

[Leif Lindholm]

On Wed, May 27, 2020 at 12:50:53 +0200, Ard Biesheuvel wrote:
> > > > Not attempting a fallback boot onto network is probably an acceptable
> > > > change to pick up, but having an undocumented hotkey as the only way
> > > > into a shell that now doesn't map devices could be a bit aggravating.
> > > > 
> > > 
> > > It is not the only way, and it is not even the preferred way. Patch 5/5 adds
> > > an option to the UiApp root menu to enter the UEFI Shell, in a way that is
> > > independent from boot option handling. Since you enter UiApp first, all
> > > handles will be connected and boot options refreshed as usual.
> > > 
> > > In cases where you want to avoid this from happening, you can use the 's'
> > > key to drop into a shell directly.
> > 
> > Yes, that's exactly what I am referring to. But in order for the new
> > (and I agree improved) functionality to be available, the new Shell
> > library needs to be explicitly added to .dsc for the platforms affected.
> > 
> > I want an active decision to be made about how that is going to
> > happen, if it is going to happen, as part of the conversation about
> > *this* set. Merging this and *then* looking into it makes for too
> > harsh a break in behaviour.
> > 
> 
> All existing platforms that incorporate ArmPkg's PlatformBootManagerLib must
> add this NULL library class resolution to UiApp first. So once we agree that
> this approach is acceptable (including the change to ShellPkg), I can
> prepare a patch for edk2-platforms implementing this for all such platforms
> living there. I suggest that we don't do the 3-way handshake here (edk2 pre,
> edk2-platforms, edk2 post), given that the build won't break, it's just that
> if you pull and build your platform right at the minute between merging the
> edk2 changes and the edk2-platform ones, you will see the slightly
> unintuitive behavior if you also happen to clear your Boot#### variables at
> the same time.

Yeah, that's fine with me.
But I'd prefer that set to be ready to go immediately afer this one
hits.

(Insert generic note on benefit of *not* breaking the project up into
ever increasing number of repositories.)

Should I interpret this set more as RFC (in which case I may have
slightly overreacted) than the PATCH it says in the subject line?

/
    Leif

Re: [edk2-devel] [PATCH 1/5] ArmPkg/PlatformBootManagerLib: register 's' as UEFI Shell hotkey

[Laszlo Ersek]

On 05/26/20 18:13, Ard Biesheuvel wrote:
> 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>
> ---
>  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
>      );
>  }
>  
> 

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

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

[Laszlo Ersek]

On 05/26/20 18:13, 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. With the right UiApp library added
> in, the UiApp also gives access to the UEFI Shell.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
> ---
>  ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
> index 23c925bbdb9c..f91f7cd09ca1 100644
> --- a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
> +++ b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
> @@ -830,5 +830,19 @@ PlatformBootManagerUnableToBoot (
>    VOID
>    )
>  {
> -  return;
> +  EFI_STATUS                   Status;
> +  EFI_BOOT_MANAGER_LOAD_OPTION BootManagerMenu;
> +
> +  //
> +  // BootManagerMenu doesn't contain the correct information when return status
> +  // is EFI_NOT_FOUND.
> +  //
> +  Status = EfiBootManagerGetBootManagerMenu (&BootManagerMenu);
> +  if (EFI_ERROR (Status)) {
> +    return;
> +  }
> +
> +  for (;;) {
> +    EfiBootManagerBoot (&BootManagerMenu);
> +  }
>  }
> 

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

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

[Laszlo Ersek]

Hi Leif,

On 05/26/20 23:24, Leif Lindholm wrote:
> On Tue, May 26, 2020 at 18:13:56 +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. With the right UiApp library added
>> in, the UiApp also gives access to the UEFI Shell.
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
>> ---
>>  ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c | 16 +++++++++++++++-
>>  1 file changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
>> index 23c925bbdb9c..f91f7cd09ca1 100644
>> --- a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
>> +++ b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
>> @@ -830,5 +830,19 @@ PlatformBootManagerUnableToBoot (
>>    VOID
>>    )
>>  {
>> -  return;
>> +  EFI_STATUS                   Status;
>> +  EFI_BOOT_MANAGER_LOAD_OPTION BootManagerMenu;
>> +
>> +  //
>> +  // BootManagerMenu doesn't contain the correct information when return status
>> +  // is EFI_NOT_FOUND.
>> +  //
>> +  Status = EfiBootManagerGetBootManagerMenu (&BootManagerMenu);
>> +  if (EFI_ERROR (Status)) {
>
> Nitpick: comment explicitly mentions EFI_NOT_FOUND, but code checks
> for any EFI_ERROR match. Since there are various other error codes
> that could be returned, change the comment to "when return status is
> not EFI_SUCCESS"?

I agree the (likely) original code (see commit 5f66615bb504,
"OvmfPkg/PlatformBds: Implement PlatformBootManagerUnableToBoot",
2018-07-27) is a bit confusing.

Namely, both the comment and the subsequent error check are correct. The
problem is that there is not much connection between the comment and the
subsequent check. In other words, the comment does not *explain* the
check.

The EfiBootManagerGetBootManagerMenu() function in
"MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c" is documented like
this:

> /**
>   Return the boot option corresponding to the Boot Manager Menu.
>   It may automatically create one if the boot option hasn't been created yet.
>
>   @param BootOption    Return the Boot Manager Menu.
>
>   @retval EFI_SUCCESS   The Boot Manager Menu is successfully returned.
>   @retval EFI_NOT_FOUND The Boot Manager Menu cannot be found.
>   @retval others        Return status of gRT->SetVariable (). BootOption still points
>                         to the Boot Manager Menu even the Status is not EFI_SUCCESS
>                         and EFI_NOT_FOUND.
> **/

So the comment change you're proposing wouldn't be technically correct,
I believe.

I think at best we should drop the comment altogether. If
EfiBootManagerGetBootManagerMenu() fails due to EFI_NOT_FOUND, then
"BootManagerMenu" is indeterminate, so we need to bail. If
EfiBootManagerGetBootManagerMenu() fails with something else, then
"BootManagerMenu" is set, but we *still* need to bail (as much as I
understand from the EfiBootManagerGetBootManagerMenu() documentation).
And that seems to mean we should simply not highlight EFI_NOT_FOUND with
a comment.

Thanks
Laszlo

Re: [edk2-devel] [PATCH 3/5] ArmPkg/PlatformBootManagerLib: hide UEFI Shell as a regular boot option

[Laszlo Ersek]

On 05/26/20 18:13, 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 when 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 | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
> index f91f7cd09ca1..b465f9ff388f 100644
> --- a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
> +++ b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
> @@ -778,7 +778,7 @@ PlatformBootManagerAfterConsole (
>    Key.ScanCode     = SCAN_NULL;
>    Key.UnicodeChar  = L's';
>    PlatformRegisterFvBootOption (
> -    &gUefiShellFileGuid, L"UEFI Shell", LOAD_OPTION_ACTIVE, &Key
> +    &gUefiShellFileGuid, L"UEFI Shell", LOAD_OPTION_HIDDEN, &Key
>      );
>  }
>  
> 

This is a policy decision for ArmPkg/PlatformBootManagerLib, which
affects ArmVirtXen and probably a number of physical ARM platforms. I'm
OK with that.

This patch does two things (which I don't mean as a request to split the
patch), it clears LOAD_OPTION_ACTIVE, and sets LOAD_OPTION_HIDDEN. From
the 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."

- "If any Boot#### load option is marked as LOAD_OPTION_HIDDEN, then the
load option will not appear in the menu (if any) provided by the boot
manager for load option selection."

So this change will both stop auto-booting the shell, and hide it from
the UiApp menu. I'm OK with that. (Maybe add this one sentence to the
commit message.)

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

Thanks
Laszlo

Re: [edk2-devel] [PATCH 4/5] ArmPkg/PlatformBootManagerLib: don't connect all devices on each boot

[Laszlo Ersek]

On 05/26/20 18:13, Ard Biesheuvel wrote:
> 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.

I would slightly extend the commit message:

"It will be called by UiApp (or DeviceManagerUiLib, per commit
13406bdeb5c5)"

Not strictly necessary, I just think mentioning it wouldn't be useless.

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

EfiBootManagerRefreshAllBootOption() makes sure we have boot options for
everything that we *do* connect.

If the "set of controllers we connect" does not change independently of
the "set of boot options we have", then I agree removing
EfiBootManagerRefreshAllBootOption() as well makes sense. (This
condition does not hold on the QEMU platforms.)

So,

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

Thanks
Laszlo

> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.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 b465f9ff388f..618072405a50 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
>    //
> 

Re: [edk2-devel] [PATCH 5/5] ShellPkg: add BootManager library to add UEFI Shell menu option

[Laszlo Ersek]

On 05/26/20 18:13, Ard Biesheuvel wrote:
> Add a plug-in library for UiApp that creates a 'UEFI Shell' menu
> option at the root level which gives access to a form that allows
> the UEFI Shell to be launched.
>
> This gives the PlatformBootManagerLib implementation of the platform
> more flexibility in the way it handles boot options pointing to the
> UEFI Shell, which will typically be invoked with only the boot path
> connected on fast boots.
>
> This library may be incorporated to a platform build by adding a
> NULL resolution to the <LibraryClasses> section of the UiApp.inf
> {} block in the platform .DSC
>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
> ---
>  ShellPkg/Library/ShellBootManagerLib/ShellBootManagerLib.inf |  45 ++++
>  ShellPkg/Library/ShellBootManagerLib/ShellBootManagerLib.h   |  44 ++++
>  ShellPkg/Library/ShellBootManagerLib/ShellBootManagerLib.c   | 258 ++++++++++++++++++++
>  ShellPkg/Library/ShellBootManagerLib/ShellBmStrings.uni      |  17 ++
>  ShellPkg/Library/ShellBootManagerLib/ShellBmVfr.Vfr          |  37 +++
>  5 files changed, 401 insertions(+)

I've had to go back to the blurb and re-read this part, to understand
the goal of this patch:

> - finally, add a plugin library for UiApp to expose the UEFI Shell via an
>   ordinary main menu option (this works around the fact that patch #3 will
>   result in the UEFI Shell disappearing from the Boot Manager list).
>   Entering the shell this way will resemble the old situation, given that
>   UiApp connects all devices and refreshes all boot options etc at launch.

If I understand correctly:

- patch #3 does two things: it clears LOAD_OPTION_ACTIVE (preventing the
  boot manager from auto-booting the shell), and sets LOAD_OPTION_HIDDEN
  (hiding the boot option from UiApp),

- patch #5 undoes LOAD_OPTION_HIDDEN, in effect -- it makes sure that we
  still see the shell option "somewhere" in UiApp (not among the boot
  options, but at the root level)

Can we:

- drop patch#5, and

- pass zero (0) as "Attributes" to PlatformRegisterFvBootOption() in
  patch#3, rather than LOAD_OPTION_HIDDEN?

Because, per spec, Attributes=0 should prevent the auto-booting of the
shell *without* hiding the shell boot option from the menu.

Thanks,
Laszlo

Re: [edk2-devel] [PATCH 5/5] ShellPkg: add BootManager library to add UEFI Shell menu option

[Ard Biesheuvel]

On 5/27/20 5:57 PM, Laszlo Ersek wrote:
> On 05/26/20 18:13, Ard Biesheuvel wrote:
>> Add a plug-in library for UiApp that creates a 'UEFI Shell' menu
>> option at the root level which gives access to a form that allows
>> the UEFI Shell to be launched.
>>
>> This gives the PlatformBootManagerLib implementation of the platform
>> more flexibility in the way it handles boot options pointing to the
>> UEFI Shell, which will typically be invoked with only the boot path
>> connected on fast boots.
>>
>> This library may be incorporated to a platform build by adding a
>> NULL resolution to the <LibraryClasses> section of the UiApp.inf
>> {} block in the platform .DSC
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
>> ---
>>   ShellPkg/Library/ShellBootManagerLib/ShellBootManagerLib.inf |  45 ++++
>>   ShellPkg/Library/ShellBootManagerLib/ShellBootManagerLib.h   |  44 ++++
>>   ShellPkg/Library/ShellBootManagerLib/ShellBootManagerLib.c   | 258 ++++++++++++++++++++
>>   ShellPkg/Library/ShellBootManagerLib/ShellBmStrings.uni      |  17 ++
>>   ShellPkg/Library/ShellBootManagerLib/ShellBmVfr.Vfr          |  37 +++
>>   5 files changed, 401 insertions(+)
> 
> I've had to go back to the blurb and re-read this part, to understand
> the goal of this patch:
> 
>> - finally, add a plugin library for UiApp to expose the UEFI Shell via an
>>    ordinary main menu option (this works around the fact that patch #3 will
>>    result in the UEFI Shell disappearing from the Boot Manager list).
>>    Entering the shell this way will resemble the old situation, given that
>>    UiApp connects all devices and refreshes all boot options etc at launch.
> 
> If I understand correctly:
> 
> - patch #3 does two things: it clears LOAD_OPTION_ACTIVE (preventing the
>    boot manager from auto-booting the shell), and sets LOAD_OPTION_HIDDEN
>    (hiding the boot option from UiApp),
> 
> - patch #5 undoes LOAD_OPTION_HIDDEN, in effect -- it makes sure that we
>    still see the shell option "somewhere" in UiApp (not among the boot
>    options, but at the root level)
> 
> Can we:
> 
> - drop patch#5, and
> 
> - pass zero (0) as "Attributes" to PlatformRegisterFvBootOption() in
>    patch#3, rather than LOAD_OPTION_HIDDEN?
> 
> Because, per spec, Attributes=0 should prevent the auto-booting of the
> shell *without* hiding the shell boot option from the menu.
> 

I feel slightly silly having gone through all the trouble of writing 
this patch. I tried playing with the ACTIVE and HIDDEN options, and 
couldn't get this to work. If I understand these quotes correctly, this 
is an error, and instead of working around this, we should apply the 
following patch to correct it:

--- a/MdeModulePkg/Library/BootManagerUiLib/BootManager.c
+++ b/MdeModulePkg/Library/BootManagerUiLib/BootManager.c
@@ -537,7 +537,7 @@ UpdateBootManager (
      //
      // Don't display the hidden/inactive boot option
      //
-    if (((BootOption[Index].Attributes & LOAD_OPTION_HIDDEN) != 0) || 
((BootOption[Index].Attributes & LOAD_OPTION_ACTIVE) == 0)) {
+    if (((BootOption[Index].Attributes & LOAD_OPTION_HIDDEN) != 0)) {
        continue;
      }


With this change applied, adding the shell option without the 'active' 
or 'hidden' flags works as expected: it appears in the boot manager 
menu, but is not booted automatically.

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

[Leif Lindholm]

On Wed, May 27, 2020 at 17:34:13 +0200, Laszlo Ersek wrote:
> Hi Leif,
> 
> On 05/26/20 23:24, Leif Lindholm wrote:
> > On Tue, May 26, 2020 at 18:13:56 +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. With the right UiApp library added
> >> in, the UiApp also gives access to the UEFI Shell.
> >>
> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
> >> ---
> >>  ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c | 16 +++++++++++++++-
> >>  1 file changed, 15 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
> >> index 23c925bbdb9c..f91f7cd09ca1 100644
> >> --- a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
> >> +++ b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
> >> @@ -830,5 +830,19 @@ PlatformBootManagerUnableToBoot (
> >>    VOID
> >>    )
> >>  {
> >> -  return;
> >> +  EFI_STATUS                   Status;
> >> +  EFI_BOOT_MANAGER_LOAD_OPTION BootManagerMenu;
> >> +
> >> +  //
> >> +  // BootManagerMenu doesn't contain the correct information when return status
> >> +  // is EFI_NOT_FOUND.
> >> +  //
> >> +  Status = EfiBootManagerGetBootManagerMenu (&BootManagerMenu);
> >> +  if (EFI_ERROR (Status)) {
> >
> > Nitpick: comment explicitly mentions EFI_NOT_FOUND, but code checks
> > for any EFI_ERROR match. Since there are various other error codes
> > that could be returned, change the comment to "when return status is
> > not EFI_SUCCESS"?
> 
> I agree the (likely) original code (see commit 5f66615bb504,
> "OvmfPkg/PlatformBds: Implement PlatformBootManagerUnableToBoot",
> 2018-07-27) is a bit confusing.
> 
> Namely, both the comment and the subsequent error check are correct. The
> problem is that there is not much connection between the comment and the
> subsequent check. In other words, the comment does not *explain* the
> check.
> 
> The EfiBootManagerGetBootManagerMenu() function in
> "MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c" is documented like
> this:
> 
> > /**
> >   Return the boot option corresponding to the Boot Manager Menu.
> >   It may automatically create one if the boot option hasn't been created yet.
> >
> >   @param BootOption    Return the Boot Manager Menu.
> >
> >   @retval EFI_SUCCESS   The Boot Manager Menu is successfully returned.
> >   @retval EFI_NOT_FOUND The Boot Manager Menu cannot be found.
> >   @retval others        Return status of gRT->SetVariable (). BootOption still points
> >                         to the Boot Manager Menu even the Status is not EFI_SUCCESS
> >                         and EFI_NOT_FOUND.
> > **/
> 
> So the comment change you're proposing wouldn't be technically correct,
> I believe.
> 
> I think at best we should drop the comment altogether. If
> EfiBootManagerGetBootManagerMenu() fails due to EFI_NOT_FOUND, then
> "BootManagerMenu" is indeterminate, so we need to bail. If
> EfiBootManagerGetBootManagerMenu() fails with something else, then
> "BootManagerMenu" is set, but we *still* need to bail (as much as I
> understand from the EfiBootManagerGetBootManagerMenu() documentation).
> And that seems to mean we should simply not highlight EFI_NOT_FOUND with
> a comment.

Ah, I see what you're saying.
Yeah, that works.

/
    Leif

Re: [edk2-devel] [PATCH 5/5] ShellPkg: add BootManager library to add UEFI Shell menu option

[Laszlo Ersek]

On 05/27/20 19:22, Ard Biesheuvel wrote:
> On 5/27/20 5:57 PM, Laszlo Ersek wrote:
>> On 05/26/20 18:13, Ard Biesheuvel wrote:
>>> Add a plug-in library for UiApp that creates a 'UEFI Shell' menu
>>> option at the root level which gives access to a form that allows
>>> the UEFI Shell to be launched.
>>>
>>> This gives the PlatformBootManagerLib implementation of the platform
>>> more flexibility in the way it handles boot options pointing to the
>>> UEFI Shell, which will typically be invoked with only the boot path
>>> connected on fast boots.
>>>
>>> This library may be incorporated to a platform build by adding a
>>> NULL resolution to the <LibraryClasses> section of the UiApp.inf
>>> {} block in the platform .DSC
>>>
>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
>>> ---
>>>   ShellPkg/Library/ShellBootManagerLib/ShellBootManagerLib.inf |  45
>>> ++++
>>>   ShellPkg/Library/ShellBootManagerLib/ShellBootManagerLib.h   |  44
>>> ++++
>>>   ShellPkg/Library/ShellBootManagerLib/ShellBootManagerLib.c   | 258
>>> ++++++++++++++++++++
>>>   ShellPkg/Library/ShellBootManagerLib/ShellBmStrings.uni      |  17 ++
>>>   ShellPkg/Library/ShellBootManagerLib/ShellBmVfr.Vfr          |  37 +++
>>>   5 files changed, 401 insertions(+)
>>
>> I've had to go back to the blurb and re-read this part, to understand
>> the goal of this patch:
>>
>>> - finally, add a plugin library for UiApp to expose the UEFI Shell
>>> via an
>>>    ordinary main menu option (this works around the fact that patch
>>> #3 will
>>>    result in the UEFI Shell disappearing from the Boot Manager list).
>>>    Entering the shell this way will resemble the old situation, given
>>> that
>>>    UiApp connects all devices and refreshes all boot options etc at
>>> launch.
>>
>> If I understand correctly:
>>
>> - patch #3 does two things: it clears LOAD_OPTION_ACTIVE (preventing the
>>    boot manager from auto-booting the shell), and sets LOAD_OPTION_HIDDEN
>>    (hiding the boot option from UiApp),
>>
>> - patch #5 undoes LOAD_OPTION_HIDDEN, in effect -- it makes sure that we
>>    still see the shell option "somewhere" in UiApp (not among the boot
>>    options, but at the root level)
>>
>> Can we:
>>
>> - drop patch#5, and
>>
>> - pass zero (0) as "Attributes" to PlatformRegisterFvBootOption() in
>>    patch#3, rather than LOAD_OPTION_HIDDEN?
>>
>> Because, per spec, Attributes=0 should prevent the auto-booting of the
>> shell *without* hiding the shell boot option from the menu.
>>
> 
> I feel slightly silly having gone through all the trouble of writing
> this patch. I tried playing with the ACTIVE and HIDDEN options, and
> couldn't get this to work. If I understand these quotes correctly, this
> is an error, and instead of working around this, we should apply the
> following patch to correct it:
> 
> --- a/MdeModulePkg/Library/BootManagerUiLib/BootManager.c
> +++ b/MdeModulePkg/Library/BootManagerUiLib/BootManager.c
> @@ -537,7 +537,7 @@ UpdateBootManager (
>      //
>      // Don't display the hidden/inactive boot option
>      //
> -    if (((BootOption[Index].Attributes & LOAD_OPTION_HIDDEN) != 0) ||
> ((BootOption[Index].Attributes & LOAD_OPTION_ACTIVE) == 0)) {
> +    if (((BootOption[Index].Attributes & LOAD_OPTION_HIDDEN) != 0)) {
>        continue;
>      }
> 
> 
> With this change applied, adding the shell option without the 'active'
> or 'hidden' flags works as expected: it appears in the boot manager
> menu, but is not booted automatically.

I enthusiastically agree that we should apply your above
BootManagerUiLib patch. I don't see why (per spec) the UI should hide a
boot option just because it is inactive.

Thanks!
Laszlo