Re: [PATCH 1/1] ArmPkg/PlatformBootManagerLib: Add path to boot UEFI Shell over UiApp

["Ard Biesheuvel" <ardb@kernel.org>]

On Tue, 7 Feb 2023 at 10:07, <pierre.gondois@arm.com> wrote:
>
> From: Pierre Gondois <pierre.gondois@arm.com>
>
> The UEFI Shell is a non-active boot option, at the opposite of UiApp.
> If no valid boot option is found, UiApp is selected. UiApp requires a
> human interaction. When installing a new EDKII image in CIs or when
> scripting is required, this is problematic.
>
> If no valid boot option is discovered, add a path to directly go to
> the UEFI Shell where the startup.nsh script is automatically executed.
> The UEFI Shell is launched after connecting possible devices, but
> before the reset that is meant to automatically make them visible.
>
> The new PcdUefiShellDefaultBootEnable must be set to TRUE to enable
> this behaviour. The Pcd is set to false by default.
>
> Signed-off-by: Pierre Gondois <pierre.gondois@arm.com>
> ---
>  ArmPkg/ArmPkg.dec                             |  9 ++-
>  .../PlatformBootManagerLib/PlatformBm.c       | 69 ++++++++++++++++++-
>  .../PlatformBootManagerLib.inf                |  4 +-
>  3 files changed, 79 insertions(+), 3 deletions(-)
>
> diff --git a/ArmPkg/ArmPkg.dec b/ArmPkg/ArmPkg.dec
> index f17ba913e6de..2444457ae58a 100644
> --- a/ArmPkg/ArmPkg.dec
> +++ b/ArmPkg/ArmPkg.dec
> @@ -2,7 +2,7 @@
>  # ARM processor package.
>  #
>  # Copyright (c) 2009 - 2010, Apple Inc. All rights reserved.<BR>
> -# Copyright (c) 2011 - 2022, ARM Limited. All rights reserved.
> +# Copyright (c) 2011 - 2023, ARM Limited. All rights reserved.
>  # Copyright (c) 2021, Ampere Computing LLC. All rights reserved.
>  #
>  #    SPDX-License-Identifier: BSD-2-Clause-Patent
> @@ -221,6 +221,13 @@ [PcdsFixedAtBuild.common]
>    #
>    gArmTokenSpaceGuid.PcdArmDmaDeviceOffset|0x0|UINT64|0x0000044
>
> +  #
> +  # Boot the Uefi Shell instead of UiApp when no valid boot option is found.
> +  # This is useful in CI environment so that startup.nsh can be launched.
> +  # The default value is FALSE.
> +  #
> +  gArmTokenSpaceGuid.PcdUefiShellDefaultBootEnable|FALSE|BOOLEAN|0x0000052
> +
>  [PcdsFixedAtBuild.common, PcdsPatchableInModule.common]
>    gArmTokenSpaceGuid.PcdFdBaseAddress|0|UINT64|0x0000002B
>    gArmTokenSpaceGuid.PcdFvBaseAddress|0|UINT64|0x0000002D
> diff --git a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
> index 2fb1a4aa4fb8..9bdc44d86b54 100644
> --- a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
> +++ b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
> @@ -2,7 +2,7 @@
>    Implementation for PlatformBootManagerLib library class interfaces.
>
>    Copyright (C) 2015-2016, Red Hat, Inc.
> -  Copyright (c) 2014 - 2021, ARM Ltd. All rights reserved.<BR>
> +  Copyright (c) 2014 - 2023, Arm Ltd. All rights reserved.<BR>
>    Copyright (c) 2004 - 2018, Intel Corporation. All rights reserved.<BR>
>    Copyright (c) 2016, Linaro Ltd. All rights reserved.<BR>
>    Copyright (c) 2021, Semihalf All rights reserved.<BR>
> @@ -470,6 +470,61 @@ PlatformRegisterFvBootOption (
>    EfiBootManagerFreeLoadOptions (BootOptions, BootOptionCount);
>  }
>
> +/** Boot a Fv Boot Option.
> + *
> + * This function is useful for booting the UEFI Shell as it is loaded
> + * as a non active boot option.
> + *
> + * @param[in] FileGuid      The File GUID.
> + * @param[in] Description   String describing the Boot Option.
> + */
> +STATIC
> +VOID
> +PlatformBootFvBootOption (
> +  IN  CONST EFI_GUID  *FileGuid,
> +  IN  CHAR16          *Description
> +  )
> +{
> +  EFI_STATUS                         Status;
> +  EFI_BOOT_MANAGER_LOAD_OPTION       NewOption;
> +  MEDIA_FW_VOL_FILEPATH_DEVICE_PATH  FileNode;
> +  EFI_LOADED_IMAGE_PROTOCOL          *LoadedImage;
> +  EFI_DEVICE_PATH_PROTOCOL           *DevicePath;
> +
> +  Status = gBS->HandleProtocol (
> +                  gImageHandle,
> +                  &gEfiLoadedImageProtocolGuid,
> +                  (VOID **)&LoadedImage
> +                  );
> +  ASSERT_EFI_ERROR (Status);
> +
> +  EfiInitializeFwVolDevicepathNode (&FileNode, FileGuid);
> +  DevicePath = DevicePathFromHandle (LoadedImage->DeviceHandle);
> +  ASSERT (DevicePath != NULL);
> +  DevicePath = AppendDevicePathNode (
> +                 DevicePath,
> +                 (EFI_DEVICE_PATH_PROTOCOL *)&FileNode
> +                 );
> +  ASSERT (DevicePath != NULL);
> +

This assumes that the UEFI shell file is in the same FV as the
executable that incorporated this library. Can you include a comment
on why that is a fair assumption?

> +  Status = EfiBootManagerInitializeLoadOption (
> +             &NewOption,
> +             LoadOptionNumberUnassigned,
> +             LoadOptionTypeBoot,
> +             LOAD_OPTION_ACTIVE,
> +             Description,
> +             DevicePath,
> +             NULL,
> +             0
> +             );
> +  ASSERT_EFI_ERROR (Status);
> +  FreePool (DevicePath);
> +
> +  for ( ; ;) {
> +    EfiBootManagerBoot (&NewOption);

Why the endless loop?


> +  }
> +}
> +
>  STATIC
>  VOID
>  GetPlatformOptions (
> @@ -1075,6 +1130,18 @@ PlatformBootManagerUnableToBoot (
>    EfiBootManagerConnectAll ();
>    EfiBootManagerRefreshAllBootOption ();
>
> +  //
> +  // Boot the 'UEFI Shell'. If the Pcd is not set, the UEFI Shell is not
> +  // an active boot option and must be manually selected through UiApp
> +  // (at least during the fist boot).
> +  //
> +  if (FixedPcdGetBool (PcdUefiShellDefaultBootEnable)) {
> +    PlatformBootFvBootOption (
> +      &gUefiShellFileGuid,
> +      L"UEFI Shell (default)"
> +      );
> +  }
> +
>    //
>    // Record the updated number of boot configured boot options
>    //
> diff --git a/ArmPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf b/ArmPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
> index 86751b45f82b..05ed46456cc4 100644
> --- a/ArmPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
> +++ b/ArmPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
> @@ -2,7 +2,7 @@
>  #  Implementation for PlatformBootManagerLib library class interfaces.
>  #
>  #  Copyright (C) 2015-2016, Red Hat, Inc.
> -#  Copyright (c) 2014, ARM Ltd. All rights reserved.<BR>
> +#  Copyright (c) 2014 - 2023, Arm Ltd. All rights reserved.<BR>
>  #  Copyright (c) 2007 - 2014, Intel Corporation. All rights reserved.<BR>
>  #  Copyright (c) 2016, Linaro Ltd. All rights reserved.<BR>
>  #
> @@ -29,6 +29,7 @@ [Sources]
>    PlatformBm.h
>
>  [Packages]
> +  ArmPkg/ArmPkg.dec
>    EmbeddedPkg/EmbeddedPkg.dec
>    MdeModulePkg/MdeModulePkg.dec
>    MdePkg/MdePkg.dec
> @@ -55,6 +56,7 @@ [FeaturePcd]
>    gEfiMdePkgTokenSpaceGuid.PcdUgaConsumeSupport
>
>  [FixedPcd]
> +  gArmTokenSpaceGuid.PcdUefiShellDefaultBootEnable
>    gEfiMdeModulePkgTokenSpaceGuid.PcdEmuVariableNvModeEnable
>    gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVersionString
>    gEfiMdePkgTokenSpaceGuid.PcdUartDefaultBaudRate
> --
> 2.25.1
>