[edk2-devel][PATCH] UefiPayloadPkg: Fix boot shell issue for universal UEFI payload
Guo Dong
guo.dong at intel.com
Fri May 12 22:43:01 UTC 2023
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.maslenkin at gmail.com>
Sent: Thursday, May 11, 2023 1:00 PM
To: devel at edk2.groups.io; Dong, Guo <guo.dong at intel.com>
Cc: Ni, Ray <ray.ni at intel.com>; Rhodes, Sean <sean at starlabs.systems>; Lu, James <james.lu at intel.com>; Guo, Gua <gua.guo at 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.dong at 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 at edk2.groups.io <devel at edk2.groups.io> On Behalf Of Mike
> Maslenkin
> Sent: Wednesday, May 10, 2023 12:14 AM
> To: devel at edk2.groups.io; Dong, Guo <guo.dong at intel.com>
> Cc: Ni, Ray <ray.ni at intel.com>; Rhodes, Sean <sean at starlabs.systems>;
> Lu, James <james.lu at intel.com>; Guo, Gua <gua.guo at 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 at intel.com> wrote:
> >
> > From: Guo Dong <guo.dong at 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 at intel.com>
> > Cc: Ray Ni <ray.ni at intel.com>
> > Cc: Sean Rhodes <sean at starlabs.systems>
> > Cc: James Lu <james.lu at intel.com>
> > Cc: Gua Guo <gua.guo at 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+owner at edk2.groups.io
> > Unsubscribe: https://edk2.groups.io/g/devel/unsub
> > [mike.maslenkin at 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/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list