From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-yb1-f171.google.com (mail-yb1-f171.google.com [209.85.219.171]) by mx.groups.io with SMTP id smtpd.web11.6862.1683835217359474361 for ; Thu, 11 May 2023 13:00:17 -0700 Authentication-Results: mx.groups.io; dkim=fail reason="signature has expired" header.i=@gmail.com header.s=20221208 header.b=dJBqBqUN; spf=pass (domain: gmail.com, ip: 209.85.219.171, mailfrom: mike.maslenkin@gmail.com) Received: by mail-yb1-f171.google.com with SMTP id 3f1490d57ef6-ba693af163dso1886913276.3 for ; Thu, 11 May 2023 13:00:17 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1683835216; x=1686427216; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=XzDAhgWCT2zW0vAh6gymP6bjIAHLqoAkU1U+gz3US3U=; b=dJBqBqUNLp7QkRAhHb1cGJeP2UMc2ZjMqJ7FSK4WQWtVLEin1qyEeLtZj3WT2vb3jd ABz4l49LBKY60dNJE+XbRFfrtANjzcbY0+Tw+doxRJonHT/8jODMGCEHOUB7FKb35l6M yG7k4L/r2mgudO92gefqpC3j3+sw8bsezWE5lNz/X5OpzdKnrvFugEADnNBMTKkFrKVD eIAU6t95sbzrL7u/R80fzG9YsRGLQ5oAEy3lBuChI2I0Tlf8cBCBQkEGHrgc+P6ae0v6 F6eMEe1ed30yY2lgCNdvTV5UrDkWoctWL63rWedMDT3MNG1sbB0EVtwCrboWw5YEBl8w B5og== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1683835216; x=1686427216; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=XzDAhgWCT2zW0vAh6gymP6bjIAHLqoAkU1U+gz3US3U=; b=csv6qhpItYs+u7ziuhJpedF+urQSgutZ0ViCExHrfpnWaaoY39nLtr4T7AztSHxi43 crjHYF/Ry0FvOUX2+OJVKHDw3W5qhrZ4xa4UkGyRE9go7iIf2h7qe4DTfbCOkti+0Tda OIx8UYJ+nQBVhYSj+in3ROSeP4ipK/rf/MLRiA1PCoLhF6xh9C4GPAn0LwlhXB8crIZQ +D7CoT+XCojw5b+U9FflibFcS54KqSX3FuQvtb3RUJjzySNy0SJZFB1rFG7V2S+0anNV t6fK3YG9RdmL4doK7fAAtq2jqDNld3uVWaEnziQhPjhuzFj9XenxPldRMv0C6wC0r9vS jpzA== X-Gm-Message-State: AC+VfDy40/mzq2/oMRTlAGyL904fbedVVd88jVQ0KYBQa561DelLWYX5 ajj85fV8Dowi3eGTOmh/Ycsts4ep2Rnb8b/NNC7ckfHqqK1tfw== X-Google-Smtp-Source: ACHHUZ4aexBtZ+/b1vNFyqYN47PRummAHUD64lpXUZJH7ffVBYMJt4ENGaFexreLAJ2cQbivbhEiwbCsldeqM3WrE7c= X-Received: by 2002:a05:6902:1249:b0:b4c:6d0b:1e99 with SMTP id t9-20020a056902124900b00b4c6d0b1e99mr23475073ybu.48.1683835216384; Thu, 11 May 2023 13:00:16 -0700 (PDT) MIME-Version: 1.0 References: <20230510035817.1023-1-guo.dong@intel.com> In-Reply-To: From: "Mike Maslenkin" Date: Thu, 11 May 2023 22:59:40 +0300 Message-ID: Subject: Re: [edk2-devel][PATCH] UefiPayloadPkg: Fix boot shell issue for universal UEFI payload To: devel@edk2.groups.io, guo.dong@intel.com Cc: "Ni, Ray" , "Rhodes, Sean" , "Lu, James" , "Guo, Gua" Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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 !=3D NULL) { // // Caller allocated buffer. Fill to size and return required size... // if (*BufferSize < CopySize) { Status =3D EFI_WARN_BUFFER_TOO_SMALL; CopySize =3D *BufferSize; } } else { // // Callee allocated buffer. Allocate buffer and return size. // *Buffer =3D AllocatePool (CopySize); if (*Buffer =3D=3D NULL) { Status =3D EFI_OUT_OF_RESOURCES; goto GetSection_Done; } } Same pattern used in FvReadFile() implementation. [1] https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/Dxe/Fw= Vol/FwVolRead.c#L508 [2] https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/Dxe/Se= ctionExtraction/CoreSectionExtraction.c#L1339 Regards, Mike On Thu, May 11, 2023 at 8:16=E2=80=AFPM Guo Dong wrote= : > > > Hi Mike, > Thanks for your comments. > The "Buffer" is initialized to NULL for ReadSection call, we don't need f= ree "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 On Behalf Of Mike Masle= nkin > Sent: Wednesday, May 10, 2023 12:14 AM > To: devel@edk2.groups.io; Dong, Guo > Cc: Ni, Ray ; Rhodes, Sean ; Lu,= James ; Guo, Gua > 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=E2=80=AFAM Guo Dong wro= te: > > > > From: Guo Dong > > > > 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 > > Cc: Ray Ni > > Cc: Sean Rhodes > > Cc: James Lu > > Cc: Gua Guo > > --- > > UefiPayloadPkg/Library/PlatformBootManagerLib/PlatformBootManager.c = | 76 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++= +++------ > > UefiPayloadPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.i= nf | 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.
> > +Copyright (c) 2015 - 2023, Intel Corporation. All rights > > +reserved.
> > SPDX-License-Identifier: BSD-2-Clause-Patent > > > > **/ > > @@ -11,6 +11,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent > > #include "PlatformConsole.h" > > #include > > #include > > +#include > > > > /** > > 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 =3D EFI_SUCCESS; > > + gBS->LocateHandleBuffer ( > > + ByProtocol, > > + &gEfiFirmwareVolume2ProtocolGuid, > > + NULL, > > + &FvHandleCount, > > + &FvHandleBuffer > > + ); > > + > > + for (Index =3D 0; Index < FvHandleCount; Index++) { > > + Buffer =3D NULL; > > + Size =3D 0; > > + gBS->HandleProtocol ( > > + FvHandleBuffer[Index], > > + &gEfiFirmwareVolume2ProtocolGuid, > > + (VOID **) &Fv > > + ); > > + Status =3D 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 =3D 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 =3D gBS->HandleProtocol (gImageHandle, > > &gEfiLoadedImageProtocolGuid, (VOID **)&LoadedImage); > > - ASSERT_EFI_ERROR (Status); > > > > EfiInitializeFwVolDevicepathNode (&FileNode, FileGuid); > > DevicePath =3D 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.
> > +# Copyright (c) 2012 - 2023, Intel Corporation. All rights > > +reserved.
> > # 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] > > ------------ > > > > > > > > > > > >=20 > >