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

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

Hi Guo,
thanks for your explanation.

I may be wrong, but there is a different logic used.
Passing pointer to NULL means request for callee allocation.
I assume EFI_FIRMWARE_VOLUME2_PROTOCOL implemented in
MdeModulePkg/Core/Dxe/FwVol.
So, Fv->ReadSection() actually is FvReadFileSection() call [1].
This function passes pointer to pointer to buffer (aka void **) into
GetSection() function.
edk2 contains only one implementation of this function in
MdeModulePkg/Core/Dxe/SectionExtraction/CoreSectionExtraction.c
And the code [2] handling Buffer variable is below:

  if (*Buffer != NULL) {
    //
    // Caller allocated buffer.  Fill to size and return required size...
    //
    if (*BufferSize < CopySize) {
      Status   = EFI_WARN_BUFFER_TOO_SMALL;
      CopySize = *BufferSize;
    }
  } else {
    //
    // Callee allocated buffer.  Allocate buffer and return size.
    //
    *Buffer = AllocatePool (CopySize);
    if (*Buffer == NULL) {
      Status = EFI_OUT_OF_RESOURCES;
      goto GetSection_Done;
    }
  }

Same pattern used in FvReadFile() implementation.

[1]  https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/Dxe/FwVol/FwVolRead.c#L508
[2]  https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/Dxe/SectionExtraction/CoreSectionExtraction.c#L1339

Regards,
Mike

On Thu, May 11, 2023 at 8:16 PM Guo Dong <guo.dong@intel.com> wrote:
>
>
> 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]
> > ------------
> >
> >
>
>
>
>
>
>
>
> 
>
>