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

["PierreGondois" <pierre.gondois@arm.com>]

On 4/21/23 16:57, Ard Biesheuvel wrote:
> 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?

Yes sure, I'm adding the following:

// The UEFI Shell was registered in PlatformRegisterFvBootOption ()
// previously, thus it must still be available in this FV.

> 
>> +  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?

It is indeed not necessary, if this fails, we should continue the normal
process.

Regards,
Pierre