* [edk2-devel][PATCH] UefiPayloadPkg: Fix boot shell issue for universal UEFI payload
@ 2023-05-10 3:58 Guo Dong
2023-05-10 6:49 ` Lu, James
2023-05-10 7:14 ` Mike Maslenkin
0 siblings, 2 replies; 6+ messages in thread
From: Guo Dong @ 2023-05-10 3:58 UTC (permalink / raw)
To: devel; +Cc: Guo Dong, Ray Ni, Sean Rhodes, James Lu, Gua Guo
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/PlatformBootManager.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/PlatformBootManagerLib.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|0x10000004
-## 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|UINT32|0x10000012
gUefiPayloadPkgTokenSpaceGuid.PcdMemoryTypeEfiACPIMemoryNVS|0x04|UINT32|0x10000013
--
2.39.1.windows.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [edk2-devel][PATCH] UefiPayloadPkg: Fix boot shell issue for universal UEFI payload
2023-05-10 3:58 [edk2-devel][PATCH] UefiPayloadPkg: Fix boot shell issue for universal UEFI payload Guo Dong
@ 2023-05-10 6:49 ` Lu, James
2023-05-10 7:14 ` Mike Maslenkin
1 sibling, 0 replies; 6+ messages in thread
From: Lu, James @ 2023-05-10 6:49 UTC (permalink / raw)
To: Dong, Guo, devel@edk2.groups.io; +Cc: Ni, Ray, Rhodes, Sean, Guo, Gua
Reviewed-by: James Lu <james.lu@intel.com>
Thanks,
James
-----Original Message-----
From: Dong, Guo <guo.dong@intel.com>
Sent: Wednesday, May 10, 2023 11:58 AM
To: devel@edk2.groups.io
Cc: Dong, Guo <guo.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Rhodes, Sean <sean@starlabs.systems>; Lu, James <james.lu@intel.com>; Guo, Gua <gua.guo@intel.com>
Subject: [edk2-devel][PATCH] UefiPayloadPkg: Fix boot shell issue for universal UEFI payload
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/PlatformBootManager.
+++ 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/PlatformBootManagerL
+++ ib.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.PcdUartDefaultParitydiff --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|0x10000004 -## 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|UINT32|0x10000012 gUefiPayloadPkgTokenSpaceGuid.PcdMemoryTypeEfiACPIMemoryNVS|0x04|UINT32|0x10000013--
2.39.1.windows.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [edk2-devel][PATCH] UefiPayloadPkg: Fix boot shell issue for universal UEFI payload
2023-05-10 3:58 [edk2-devel][PATCH] UefiPayloadPkg: Fix boot shell issue for universal UEFI payload Guo Dong
2023-05-10 6:49 ` Lu, James
@ 2023-05-10 7:14 ` Mike Maslenkin
2023-05-11 17:15 ` Guo Dong
1 sibling, 1 reply; 6+ messages in thread
From: Mike Maslenkin @ 2023-05-10 7:14 UTC (permalink / raw)
To: devel, guo.dong; +Cc: Ray Ni, Sean Rhodes, James Lu, Gua Guo
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/PlatformBootManager.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/PlatformBootManagerLib.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|0x10000004
>
> -## 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|UINT32|0x10000012
> gUefiPayloadPkgTokenSpaceGuid.PcdMemoryTypeEfiACPIMemoryNVS|0x04|UINT32|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]
> ------------
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [edk2-devel][PATCH] UefiPayloadPkg: Fix boot shell issue for universal UEFI payload
2023-05-10 7:14 ` Mike Maslenkin
@ 2023-05-11 17:15 ` Guo Dong
2023-05-11 19:59 ` Mike Maslenkin
0 siblings, 1 reply; 6+ messages in thread
From: Guo Dong @ 2023-05-11 17:15 UTC (permalink / raw)
To: devel@edk2.groups.io, mike.maslenkin@gmail.com
Cc: Ni, Ray, Rhodes, Sean, Lu, James, Guo, Gua
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]
> ------------
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [edk2-devel][PATCH] UefiPayloadPkg: Fix boot shell issue for universal UEFI payload
2023-05-11 17:15 ` Guo Dong
@ 2023-05-11 19:59 ` Mike Maslenkin
2023-05-12 22:43 ` Guo Dong
0 siblings, 1 reply; 6+ messages in thread
From: Mike Maslenkin @ 2023-05-11 19:59 UTC (permalink / raw)
To: devel, guo.dong; +Cc: Ni, Ray, Rhodes, Sean, Lu, James, Guo, Gua
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]
> > ------------
> >
> >
>
>
>
>
>
>
>
>
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [edk2-devel][PATCH] UefiPayloadPkg: Fix boot shell issue for universal UEFI payload
2023-05-11 19:59 ` Mike Maslenkin
@ 2023-05-12 22:43 ` Guo Dong
0 siblings, 0 replies; 6+ messages in thread
From: Guo Dong @ 2023-05-12 22:43 UTC (permalink / raw)
To: Mike Maslenkin, devel@edk2.groups.io
Cc: Ni, Ray, Rhodes, Sean, Lu, James, Guo, Gua
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@gmail.com>
Sent: Thursday, May 11, 2023 1:00 PM
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,
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/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@edk2.groups.io
> > Unsubscribe: https://edk2.groups.io/g/devel/unsub
> > [mike.maslenkin@gmail.com]
> > ------------
> >
> >
>
>
>
>
>
>
>
>
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-05-12 22:43 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-05-10 3:58 [edk2-devel][PATCH] UefiPayloadPkg: Fix boot shell issue for universal UEFI payload Guo Dong
2023-05-10 6:49 ` Lu, James
2023-05-10 7:14 ` Mike Maslenkin
2023-05-11 17:15 ` Guo Dong
2023-05-11 19:59 ` Mike Maslenkin
2023-05-12 22:43 ` Guo Dong
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox