Hi Mike, Thanks to share the details. I think you are right the "buffer" need be freed using ReadSection(). I don't really want to read the shell file, so I would update this patch to use ReadFile() which supports NULL for the file buffer.
Thanks, Guo -----Original Message----- From: Mike Maslenkin <mike.maslen...@gmail.com> Sent: Thursday, May 11, 2023 1:00 PM To: devel@edk2.groups.io; Dong, Guo <guo.d...@intel.com> Cc: Ni, Ray <ray...@intel.com>; Rhodes, Sean <sean@starlabs.systems>; Lu, James <james...@intel.com>; Guo, Gua <gua....@intel.com> Subject: Re: [edk2-devel][PATCH] UefiPayloadPkg: Fix boot shell issue for universal UEFI payload 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.d...@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.d...@intel.com> > Cc: Ni, Ray <ray...@intel.com>; Rhodes, Sean <sean@starlabs.systems>; > Lu, James <james...@intel.com>; Guo, Gua <gua....@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.d...@intel.com> wrote: > > > > From: Guo Dong <guo.d...@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.d...@intel.com> > > Cc: Ray Ni <ray...@intel.com> > > Cc: Sean Rhodes <sean@starlabs.systems> > > Cc: James Lu <james...@intel.com> > > Cc: Gua Guo <gua....@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/PlatformBootMana > > +++ ge > > +++ 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/PlatformBootManagerL > > ib > > .inf > > b/UefiPayloadPkg/Library/PlatformBootManagerLib/PlatformBootManagerL > > ib > > .inf > > index f9626175e2..a3951b7a7e 100644 > > --- > > a/UefiPayloadPkg/Library/PlatformBootManagerLib/PlatformBootManagerL > > ib > > .inf > > +++ b/UefiPayloadPkg/Library/PlatformBootManagerLib/PlatformBootMana > > +++ ge > > +++ 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|0x1000000 > > 2 > > ## Save bootloader parameter > > > > gUefiPayloadPkgTokenSpaceGuid.PcdBootloaderParameter|0|UINT64|0x1000 > > 00 > > 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|UIN > > T3 > > 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+ow...@edk2.groups.io > > Unsubscribe: https://edk2.groups.io/g/devel/unsub > > [mike.maslen...@gmail.com] > > ------------ > > > > > > > > > > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#104811): https://edk2.groups.io/g/devel/message/104811 Mute This Topic: https://groups.io/mt/98799622/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-