From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mx.groups.io with SMTP id smtpd.web10.73739.1682405873635442274 for ; Mon, 24 Apr 2023 23:57:54 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: arm.com, ip: 217.140.110.172, mailfrom: pierre.gondois@arm.com) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 9F9624B3; Mon, 24 Apr 2023 23:58:36 -0700 (PDT) Received: from [10.34.100.101] (pierre123.nice.arm.com [10.34.100.101]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id C09133F587; Mon, 24 Apr 2023 23:57:51 -0700 (PDT) Message-ID: <17c7814b-0ab5-5dce-18af-b6029626e2bf@arm.com> Date: Tue, 25 Apr 2023 08:57:46 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.10.0 Subject: Re: [PATCH 1/1] ArmPkg/PlatformBootManagerLib: Add path to boot UEFI Shell over UiApp To: Ard Biesheuvel Cc: devel@edk2.groups.io, ardb+tianocore@kernel.org, leif@nuviainc.com, sami.mujawar@arm.com, patrik.berglund@arm.com References: <20230207090653.395992-1-pierre.gondois@arm.com> From: "PierreGondois" In-Reply-To: Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit On 4/21/23 16:57, Ard Biesheuvel wrote: > On Tue, 7 Feb 2023 at 10:07, wrote: >> >> From: Pierre Gondois >> >> 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 >> --- >> 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.
>> -# 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.
>> + Copyright (c) 2014 - 2023, Arm Ltd. All rights reserved.
>> Copyright (c) 2004 - 2018, Intel Corporation. All rights reserved.
>> Copyright (c) 2016, Linaro Ltd. All rights reserved.
>> Copyright (c) 2021, Semihalf All rights reserved.
>> @@ -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