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

["Guo Dong" <guo.dong@intel.com>]

Hi Mike,
Thanks for your comments.
The "Buffer" is initialized to NULL for ReadSection call, we don't need free "Buffer" since there is no data really read to Buffer.
With "Buffer" set to NULL, it just test if the file exists in the FV. If it exists, it will return success with file size.

Thanks,
Guo
-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Mike Maslenkin
Sent: Wednesday, May 10, 2023 12:14 AM
To: devel@edk2.groups.io; Dong, Guo <guo.dong@intel.com>
Cc: Ni, Ray <ray.ni@intel.com>; Rhodes, Sean <sean@starlabs.systems>; Lu, James <james.lu@intel.com>; Guo, Gua <gua.guo@intel.com>
Subject: Re: [edk2-devel][PATCH] UefiPayloadPkg: Fix boot shell issue for universal UEFI payload

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/PlatformBootManage
> +++ r.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/PlatformBootManage
> +++ rLib.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|0x100000
> 04
>
> -## 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|U
> INT32|0x10000012
>  
> gUefiPayloadPkgTokenSpaceGuid.PcdMemoryTypeEfiACPIMemoryNVS|0x04|UINT3
> 2|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]
> ------------
>
>