Sorry for late to reply you. (Both Monday and Tuesday are public holidays in my
location)
Yeah, you concern is more effective than preventing potential issue. Thanks
for the clarifying this.
Reviewed-by: Sunny Wang <sunnyw...@hpe.com>
-----Original Message-----
From: Ni, Ruiyu [mailto:ruiyu...@intel.com]
Sent: Tuesday, April 05, 2016 11:11 AM
To: Wang, Sunny (HPS SW) <sunnyw...@hpe.com>
Cc: Fu, Siyuan <siyuan...@intel.com>; El-Haj-Mahmoud, Samer
<samer.el-haj-mahm...@hpe.com>; edk2-devel@lists.01.org
Subject: RE: [edk2] [Patch v3 1/3] MdeModulePkg/Bds: Allocate reserved memory
for RAM Disk boot media
Importance: High
Sunny,
Thanks for your comments.
ZeroMem I think is very useful to initialize a structure where the default
value of many fields are 0.
But in such case, ZeroMem() is not optimal because only when code contains bug
the ZeroMem() is useful.
Otherwise, it costs time to zero the memory.
For a RAM disk boot, the memory allocated may be 1GB or larger. Zero such big
memory will need lots of time.
Regards,
Ray
>-----Original Message-----
>From: Wang, Sunny (HPS SW) [mailto:sunnyw...@hpe.com]
>Sent: Friday, April 1, 2016 7:35 PM
>To: Ni, Ruiyu <ruiyu...@intel.com>
>Cc: Fu, Siyuan <siyuan...@intel.com>; El-Haj-Mahmoud, Samer
><samer.el-haj-mahm...@hpe.com>; Wang, Sunny (HPS SW)
><sunnyw...@hpe.com>; edk2-devel@lists.01.org
>Subject: RE: [edk2] [Patch v3 1/3] MdeModulePkg/Bds: Allocate reserved
>memory for RAM Disk boot media
>
>Hi Ray,
>
>For this patch, I just have a suggestion below. Others look good to me.
>For the memory allocation code block below, It looks better to add a
>ZeroMem() call right after it like the following so that we can avoid running
>into some problems caused by remaining garbage data in memory. What do you
>think?
>+ FileBuffer = LoadFileSystem ? AllocateReservedPages
>+ (EFI_SIZE_TO_PAGES
>+ (BufferSize)) : AllocatePool (BufferSize); if (FileBuffer == NULL) {
>+ return NULL;
>+ }
>ZeroMem (FileBuffer, BufferSize);
>
>Regards,
>Sunny Wang
>
>-----Original Message-----
>From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
>Ruiyu Ni
>Sent: Thursday, March 31, 2016 10:37 AM
>To: edk2-devel@lists.01.org
>Cc: Ruiyu Ni <ruiyu...@intel.com>; Siyuan Fu <siyuan...@intel.com>
>Subject: [edk2] [Patch v3 1/3] MdeModulePkg/Bds: Allocate reserved
>memory for RAM Disk boot media
>
>Use reserved memory to hold the buffer for the RAM disk to follow the ACPI
>spec requirement.
>
>Contributed-under: TianoCore Contribution Agreement 1.0
>Signed-off-by: Ruiyu Ni <ruiyu...@intel.com>
>Cc: Siyuan Fu <siyuan...@intel.com>
>---
> MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c | 149 +++++++++++----------
> .../Library/UefiBootManagerLib/InternalBm.h | 35 ++---
> 2 files changed, 95 insertions(+), 89 deletions(-)
>
>diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
>b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
>index f6c6845..a582b90 100644
>--- a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
>+++ b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c
>@@ -688,7 +688,6 @@ BmExpandUriDevicePath (
> UINTN Index;
> UINTN HandleCount;
> EFI_HANDLE *Handles;
>- EFI_LOAD_FILE_PROTOCOL *LoadFile;
> VOID *FileBuffer;
>
> EfiBootManagerConnectAll ();
>@@ -698,37 +697,10 @@ BmExpandUriDevicePath (
> Handles = NULL;
> }
>
>- FileBuffer = NULL;
>-
> for (Index = 0; Index < HandleCount; Index++) {
>- Status = gBS->HandleProtocol (Handles[Index], &gEfiLoadFileProtocolGuid,
>(VOID *) &LoadFile);
>- ASSERT_EFI_ERROR (Status);
>-
>- FileBuffer = NULL;
>- Status = LoadFile->LoadFile (LoadFile, FilePath, TRUE, FileSize,
>FileBuffer);
>- if (Status == EFI_BUFFER_TOO_SMALL) {
>- FileBuffer = AllocatePool (*FileSize);
>- if (FileBuffer == NULL) {
>- break;
>- }
>- Status = LoadFile->LoadFile (LoadFile, FilePath, TRUE, FileSize,
>FileBuffer);
>- }
>-
>- if (!EFI_ERROR (Status)) {
>- //
>- // LoadFile() returns a file buffer mapping to a file system.
>- //
>- if (Status == EFI_WARN_FILE_SYSTEM) {
>- return BmGetFileBufferFromLoadFileFileSystem (Handles[Index],
>FullPath, FileSize);
>- }
>-
>- ASSERT (Status == EFI_SUCCESS);
>- *FullPath = DuplicateDevicePath (DevicePathFromHandle (Handles[Index]));
>- break;
>- }
>-
>+ FileBuffer = BmGetFileBufferFromLoadFile (Handles[Index],
>+ FilePath, FullPath, FileSize);
> if (FileBuffer != NULL) {
>- FreePool (FileBuffer);
>+ break;
> }
> }
>
>@@ -1127,7 +1099,7 @@ BmMatchHttpBootDevicePath (
> @return The load option buffer.
> **/
> VOID *
>-BmGetFileBufferFromLoadFileFileSystem (
>+BmGetFileBufferFromLoadFileSystem (
> IN EFI_HANDLE LoadFileHandle,
> OUT EFI_DEVICE_PATH_PROTOCOL **FullPath,
> OUT UINTN *FileSize
>@@ -1175,8 +1147,78 @@ BmGetFileBufferFromLoadFileFileSystem (
> }
> }
>
>+
> /**
>- Get the file buffer from Load File instance.
>+ Get the file buffer from the specified Load File instance.
>+
>+ @param LoadFileHandle The specified Load File instance.
>+ @param FilePath The file path which will pass to LoadFile().
>+ @param FullPath Return the full device path pointing to the load
>option.
>+ @param FileSize Return the size of the load option.
>+
>+ @return The load option buffer or NULL if fails.
>+**/
>+VOID *
>+BmGetFileBufferFromLoadFile (
>+ EFI_HANDLE LoadFileHandle,
>+ IN EFI_DEVICE_PATH_PROTOCOL *FilePath,
>+ OUT EFI_DEVICE_PATH_PROTOCOL **FullPath,
>+ OUT UINTN *FileSize
>+ )
>+{
>+ EFI_STATUS Status;
>+ EFI_LOAD_FILE_PROTOCOL *LoadFile;
>+ VOID *FileBuffer;
>+ BOOLEAN LoadFileSystem;
>+ UINTN BufferSize;
>+
>+ *FileSize = 0;
>+
>+ Status = gBS->OpenProtocol (
>+ LoadFileHandle,
>+ &gEfiLoadFileProtocolGuid,
>+ (VOID **) &LoadFile,
>+ gImageHandle,
>+ NULL,
>+ EFI_OPEN_PROTOCOL_GET_PROTOCOL
>+ );
>+ ASSERT_EFI_ERROR (Status);
>+
>+ FileBuffer = NULL;
>+ BufferSize = 0;
>+ Status = LoadFile->LoadFile (LoadFile, FilePath, TRUE, &BufferSize,
>+ FileBuffer); if ((Status != EFI_WARN_FILE_SYSTEM) && (Status !=
>EFI_BUFFER_TOO_SMALL)) {
>+ return NULL;
>+ }
>+
>+ LoadFileSystem = (BOOLEAN) (Status == EFI_WARN_FILE_SYSTEM);
>+ FileBuffer = LoadFileSystem ? AllocateReservedPages
>+ (EFI_SIZE_TO_PAGES
>+ (BufferSize)) : AllocatePool (BufferSize); if (FileBuffer == NULL) {
>+ return NULL;
>+ }
>+
>+ Status = LoadFile->LoadFile (LoadFile, FilePath, TRUE, &BufferSize,
>+ FileBuffer); if (EFI_ERROR (Status)) {
>+ if (LoadFileSystem) {
>+ FreePages (FileBuffer, EFI_SIZE_TO_PAGES (BufferSize));
>+ } else {
>+ FreePool (FileBuffer);
>+ }
>+ return NULL;
>+ }
>+
>+ if (LoadFileSystem) {
>+ FileBuffer = BmGetFileBufferFromLoadFileSystem (LoadFileHandle,
>+ FullPath, FileSize); } else {
>+ *FileSize = BufferSize;
>+ *FullPath = DuplicateDevicePath (DevicePathFromHandle
>+ (LoadFileHandle)); }
>+
>+ return FileBuffer;
>+}
>+
>+/**
>+ Get the file buffer from all the Load File instances.
>
> @param FilePath The media device path pointing to a LoadFile instance.
> @param FullPath Return the full device path pointing to the load option.
>@@ -1185,7 +1227,7 @@ BmGetFileBufferFromLoadFileFileSystem (
> @return The load option buffer.
> **/
> VOID *
>-BmGetFileBufferFromLoadFile (
>+BmGetFileBufferFromLoadFiles (
> IN EFI_DEVICE_PATH_PROTOCOL *FilePath,
> OUT EFI_DEVICE_PATH_PROTOCOL **FullPath,
> OUT UINTN *FileSize
>@@ -1193,13 +1235,10 @@ BmGetFileBufferFromLoadFile ( {
> EFI_STATUS Status;
> EFI_HANDLE Handle;
>- VOID *FileBuffer;
> EFI_HANDLE *Handles;
> UINTN HandleCount;
> UINTN Index;
> EFI_DEVICE_PATH_PROTOCOL *Node;
>- EFI_LOAD_FILE_PROTOCOL *LoadFile;
>- UINTN BufferSize;
>
> //
> // Get file buffer from load file instance.
>@@ -1244,41 +1283,7 @@ BmGetFileBufferFromLoadFile (
> return NULL;
> }
>
>- Status = gBS->HandleProtocol (Handle, &gEfiLoadFileProtocolGuid,
>(VOID **) &LoadFile);
>- ASSERT_EFI_ERROR (Status);
>-
>- BufferSize = 0;
>- FileBuffer = NULL;
>- Status = LoadFile->LoadFile (LoadFile, FilePath, TRUE, &BufferSize,
>FileBuffer);
>- if (Status == EFI_BUFFER_TOO_SMALL) {
>- FileBuffer = AllocatePool (BufferSize);
>- if (FileBuffer != NULL) {
>- Status = EFI_SUCCESS;
>- }
>- }
>-
>- if (!EFI_ERROR (Status)) {
>- Status = LoadFile->LoadFile (LoadFile, FilePath, TRUE, &BufferSize,
>FileBuffer);
>- }
>-
>- if (!EFI_ERROR (Status)) {
>- //
>- // LoadFile() returns a file buffer mapping to a file system.
>- //
>- if (Status == EFI_WARN_FILE_SYSTEM) {
>- return BmGetFileBufferFromLoadFileFileSystem (Handle, FullPath,
>FileSize);
>- }
>-
>- ASSERT (Status == EFI_SUCCESS);
>- //
>- // LoadFile () may cause the device path of the Handle be updated.
>- //
>- *FullPath = DuplicateDevicePath (DevicePathFromHandle (Handle));
>- *FileSize = BufferSize;
>- return FileBuffer;
>- } else {
>- return NULL;
>- }
>+ return BmGetFileBufferFromLoadFile (Handle, FilePath, FullPath,
>+ FileSize);
> }
>
> /**
>@@ -1394,7 +1399,7 @@ BmGetLoadOptionBuffer (
> return FileBuffer;
> }
>
>- return BmGetFileBufferFromLoadFile (FilePath, FullPath, FileSize);
>+ return BmGetFileBufferFromLoadFiles (FilePath, FullPath, FileSize);
> }
>
> /**
>diff --git a/MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h
>b/MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h
>index b261d76..7b6252a 100644
>--- a/MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h
>+++ b/MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h
>@@ -410,23 +410,6 @@ BmCharToUint (
> IN CHAR16 Char
> );
>
>-
>-/**
>- Get the file buffer from the file system produced by Load File instance.
>-
>- @param LoadFileHandle The handle of LoadFile instance.
>- @param FullPath Return the full device path pointing to the load
>option.
>- @param FileSize Return the size of the load option.
>-
>- @return The load option buffer.
>-**/
>-VOID *
>-BmGetFileBufferFromLoadFileFileSystem (
>- IN EFI_HANDLE LoadFileHandle,
>- OUT EFI_DEVICE_PATH_PROTOCOL **FullPath,
>- OUT UINTN *FileSize
>- );
>-
> /**
> Return the boot description for the controller.
>
>@@ -451,4 +434,22 @@ BmMakeBootOptionDescriptionUnique (
> EFI_BOOT_MANAGER_LOAD_OPTION *BootOptions,
> UINTN BootOptionCount
> );
>+
>+/**
>+ Get the file buffer from the specified Load File instance.
>+
>+ @param LoadFileHandle The specified Load File instance.
>+ @param FilePath The file path which will pass to LoadFile().
>+ @param FullPath Return the full device path pointing to the load
>option.
>+ @param FileSize Return the size of the load option.
>+
>+ @return The load option buffer or NULL if fails.
>+**/
>+VOID *
>+BmGetFileBufferFromLoadFile (
>+ EFI_HANDLE LoadFileHandle,
>+ IN EFI_DEVICE_PATH_PROTOCOL *FilePath,
>+ OUT EFI_DEVICE_PATH_PROTOCOL **FullPath,
>+ OUT UINTN *FileSize
>+ );
> #endif // _INTERNAL_BM_H_
>--
>2.7.0.windows.1
>
>_______________________________________________
>edk2-devel mailing list
>edk2-devel@lists.01.org
>https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
