Re: [edk2-devel][PATCH] UefiPayloadPkg: Fix boot shell issue for universal UEFI payload

["Mike Maslenkin" <mike.maslenkin@gmail.com>]

Hi, Guo Dong

Don't you need to free "Buffer" after Fv->ReadSection() call ?

Regards,
Mike.

On Wed, May 10, 2023 at 6:58 AM Guo Dong <guo.dong@intel.com> wrote:
>
> From: Guo Dong <guo.dong@intel.com>
>
> After moving BDS driver to a new FV for universal UEFI payload,
> the shell boot option path is not correct since it used the BDS
> FV instead of DXE FV in its device path.
> This patch would find the correct FV by reading shell file.
> It also removed PcdShellFile by using gUefiShellFileGuid.
>
> Signed-off-by: Guo Dong <guo.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Sean Rhodes <sean@starlabs.systems>
> Cc: James Lu <james.lu@intel.com>
> Cc: Gua Guo <gua.guo@intel.com>
> ---
>  UefiPayloadPkg/Library/PlatformBootManagerLib/PlatformBootManager.c      | 76 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++------
>  UefiPayloadPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf |  5 +++--
>  UefiPayloadPkg/UefiPayloadPkg.dec                                        |  3 ---
>  3 files changed, 73 insertions(+), 11 deletions(-)
>
> diff --git a/UefiPayloadPkg/Library/PlatformBootManagerLib/PlatformBootManager.c b/UefiPayloadPkg/Library/PlatformBootManagerLib/PlatformBootManager.c
> index 62637ae6aa..cf72783af1 100644
> --- a/UefiPayloadPkg/Library/PlatformBootManagerLib/PlatformBootManager.c
> +++ b/UefiPayloadPkg/Library/PlatformBootManagerLib/PlatformBootManager.c
> @@ -2,7 +2,7 @@
>    This file include all platform action which can be customized
>    by IBV/OEM.
>
> -Copyright (c) 2015 - 2021, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2015 - 2023, Intel Corporation. All rights reserved.<BR>
>  SPDX-License-Identifier: BSD-2-Clause-Patent
>
>  **/
> @@ -11,6 +11,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>  #include "PlatformConsole.h"
>  #include <Guid/BootManagerMenu.h>
>  #include <Library/HobLib.h>
> +#include <Protocol/FirmwareVolume2.h>
>
>  /**
>    Signal EndOfDxe event and install SMM Ready to lock protocol.
> @@ -89,6 +90,72 @@ PlatformFindLoadOption (
>    return -1;
>  }
>
> +
> +EFI_DEVICE_PATH_PROTOCOL *
> +BdsGetShellFvDevicePath (
> +  VOID
> +  )
> +{
> +  UINTN                             FvHandleCount;
> +  EFI_HANDLE                        *FvHandleBuffer;
> +  UINTN                             Index;
> +  EFI_STATUS                        Status;
> +  EFI_FIRMWARE_VOLUME2_PROTOCOL     *Fv;
> +  UINTN                             Size;
> +  UINT32                            AuthenticationStatus;
> +  EFI_DEVICE_PATH_PROTOCOL          *DevicePath;
> +  VOID                              *Buffer;
> +
> +  Status = EFI_SUCCESS;
> +  gBS->LocateHandleBuffer (
> +         ByProtocol,
> +         &gEfiFirmwareVolume2ProtocolGuid,
> +         NULL,
> +         &FvHandleCount,
> +         &FvHandleBuffer
> +         );
> +
> +  for (Index = 0; Index < FvHandleCount; Index++) {
> +    Buffer = NULL;
> +    Size   = 0;
> +    gBS->HandleProtocol (
> +           FvHandleBuffer[Index],
> +           &gEfiFirmwareVolume2ProtocolGuid,
> +           (VOID **) &Fv
> +           );
> +    Status = Fv->ReadSection (
> +                   Fv,
> +                   &gUefiShellFileGuid,
> +                   EFI_SECTION_PE32,
> +                   0,
> +                   &Buffer,
> +                   &Size,
> +                   &AuthenticationStatus
> +                   );
> +    if (!EFI_ERROR (Status)) {
> +      //
> +      // Found the shell file
> +      //
> +      break;
> +    }
> +  }
> +
> +  if (EFI_ERROR (Status)) {
> +    if (FvHandleCount) {
> +      FreePool (FvHandleBuffer);
> +    }
> +    return NULL;
> +  }
> +
> +  DevicePath = DevicePathFromHandle (FvHandleBuffer[Index]);
> +
> +  if (FvHandleCount) {
> +    FreePool (FvHandleBuffer);
> +  }
> +
> +  return DevicePath;
> +}
> +
>  /**
>    Register a boot option using a file GUID in the FV.
>
> @@ -109,15 +176,12 @@ PlatformRegisterFvBootOption (
>    EFI_BOOT_MANAGER_LOAD_OPTION       *BootOptions;
>    UINTN                              BootOptionCount;
>    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 = AppendDevicePathNode (
> -                 DevicePathFromHandle (LoadedImage->DeviceHandle),
> +                 BdsGetShellFvDevicePath(),
>                   (EFI_DEVICE_PATH_PROTOCOL *)&FileNode
>                   );
>
> @@ -248,7 +312,7 @@ PlatformBootManagerAfterConsole (
>    //
>    // Register UEFI Shell
>    //
> -  PlatformRegisterFvBootOption (PcdGetPtr (PcdShellFile), L"UEFI Shell", LOAD_OPTION_ACTIVE);
> +  PlatformRegisterFvBootOption (&gUefiShellFileGuid, L"UEFI Shell", LOAD_OPTION_ACTIVE);
>
>    if (FixedPcdGetBool (PcdBootManagerEscape)) {
>      Print (
> diff --git a/UefiPayloadPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf b/UefiPayloadPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
> index f9626175e2..a3951b7a7e 100644
> --- a/UefiPayloadPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
> +++ b/UefiPayloadPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
> @@ -1,7 +1,7 @@
>  ## @file
>  #  Include all platform action which can be customized by IBV/OEM.
>  #
> -#  Copyright (c) 2012 - 2021, Intel Corporation. All rights reserved.<BR>
> +#  Copyright (c) 2012 - 2023, Intel Corporation. All rights reserved.<BR>
>  #  SPDX-License-Identifier: BSD-2-Clause-Patent
>  #
>  ##
> @@ -32,6 +32,7 @@
>    MdePkg/MdePkg.dec
>    MdeModulePkg/MdeModulePkg.dec
>    UefiPayloadPkg/UefiPayloadPkg.dec
> +  ShellPkg/ShellPkg.dec
>
>  [LibraryClasses]
>    BaseLib
> @@ -52,6 +53,7 @@
>  [Guids]
>    gEfiEndOfDxeEventGroupGuid
>    gEdkiiBootManagerMenuFileGuid
> +  gUefiShellFileGuid
>
>  [Protocols]
>    gEfiGenericMemTestProtocolGuid  ## CONSUMES
> @@ -69,7 +71,6 @@
>    gEfiMdeModulePkgTokenSpaceGuid.PcdConOutRow
>    gEfiMdeModulePkgTokenSpaceGuid.PcdConOutColumn
>    gEfiMdeModulePkgTokenSpaceGuid.PcdConInConnectOnDemand
> -  gUefiPayloadPkgTokenSpaceGuid.PcdShellFile
>    gEfiMdePkgTokenSpaceGuid.PcdUartDefaultBaudRate
>    gEfiMdePkgTokenSpaceGuid.PcdUartDefaultDataBits
>    gEfiMdePkgTokenSpaceGuid.PcdUartDefaultParity
> diff --git a/UefiPayloadPkg/UefiPayloadPkg.dec b/UefiPayloadPkg/UefiPayloadPkg.dec
> index a23a7b5a78..8d111f3a90 100644
> --- a/UefiPayloadPkg/UefiPayloadPkg.dec
> +++ b/UefiPayloadPkg/UefiPayloadPkg.dec
> @@ -67,9 +67,6 @@ gUefiPayloadPkgTokenSpaceGuid.PcdPayloadFdMemSize|0|UINT32|0x10000002
>  ## Save bootloader parameter
>  gUefiPayloadPkgTokenSpaceGuid.PcdBootloaderParameter|0|UINT64|0x10000004
>
> -## FFS filename to find the shell application.
> -gUefiPayloadPkgTokenSpaceGuid.PcdShellFile|{ 0x83, 0xA5, 0x04, 0x7C, 0x3E, 0x9E, 0x1c, 0x4f, 0xAD, 0x65, 0xE0, 0x52, 0x68, 0xD0, 0xB4, 0xD1 }|VOID*|0x10000005
> -
>  ## Used to help reduce fragmentation in the EFI memory map
>  gUefiPayloadPkgTokenSpaceGuid.PcdMemoryTypeEfiACPIReclaimMemory|0x19|UINT32|0x10000012
>  gUefiPayloadPkgTokenSpaceGuid.PcdMemoryTypeEfiACPIMemoryNVS|0x04|UINT32|0x10000013
> --
> 2.39.1.windows.1
>
>
>
> ------------
> Groups.io Links: You receive all messages sent to this group.
> View/Reply Online (#104480): https://edk2.groups.io/g/devel/message/104480
> Mute This Topic: https://groups.io/mt/98799622/1770412
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub [mike.maslenkin@gmail.com]
> ------------
>
>