Sunny, I reposted the patch serials to follow your suggestions. Regards, Ray
>-----Original Message----- >From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Wang, >Sunny (HPS SW) >Sent: Friday, April 1, 2016 7:42 PM >To: Ni, Ruiyu <ruiyu...@intel.com> >Cc: Fu, Siyuan <siyuan...@intel.com>; edk2-devel@lists.01.org; Tian, Feng ><feng.t...@intel.com> >Subject: Re: [edk2] [Patch v3 2/3] MdeModulePkg/Bds: Free resources after ram >disk boot finishes > >Hi Ray, >For this patch, I just found a minor issue and also have a suggestion below. >Others look good to me. > >Minor issue: >The following newly created functions' parameter lack IN and/or OUT modifiers: >1. BmGetLoadFileRamDisk >2. BmGetRamDiskMemoryInfo >3. BmDestroyRamDisk > >Suggestion: >It looks like both BmGetLoadFileRamDisk and BmGetFileBufferFromLoadFileSystem) >should be better to return >RamDiskDevicePath instead of RamDiskHandle because both BmDestroyRamDisk and >BmGetRamDiskMemoryInfo only need >RamDisk's device path. In other words, if we change to return >RamDiskDevicePath, we will NOT need to have >DevicePathFromHandle function calls in newly added code. What do you think? >By the way, if you agree with my suggestion above, we may also consider to >directly use FullPath instead of adding new >output parameter RamDiskHandle/RamDiskDevicePath to >BmGetFileBufferFromLoadFileSystem(). For this one, I didn't take a >deep look, so it maybe is not feasible. Just mention it here for your >reference. > >Regards, >Sunny Wang > >-----Original Message----- >From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Ruiyu Ni >Sent: Wednesday, March 30, 2016 8:29 AM >To: edk2-devel@lists.01.org >Cc: Ruiyu Ni <ruiyu...@intel.com>; Siyuan Fu <siyuan...@intel.com>; Feng Tian ><feng.t...@intel.com> >Subject: [edk2] [Patch v3 2/3] MdeModulePkg/Bds: Free resources after ram disk >boot finishes > >The resource free includes to un-register the ram disk device and free the >memory occupied by the ram disk. > >Contributed-under: TianoCore Contribution Agreement 1.0 >Signed-off-by: Ruiyu Ni <ruiyu...@intel.com> >Cc: Siyuan Fu <siyuan...@intel.com> >Cc: Feng Tian <feng.t...@intel.com> >--- > MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c | 155 ++++++++++++++++++++- > .../Library/UefiBootManagerLib/InternalBm.h | 1 + > .../UefiBootManagerLib/UefiBootManagerLib.inf | 1 + > 3 files changed, 153 insertions(+), 4 deletions(-) > >diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c >b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c >index 6e5c13b..598de26 100644 >--- a/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c >+++ b/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c >@@ -1095,6 +1095,7 @@ BmMatchHttpBootDevicePath ( > @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. >+ @param RamDiskHandle Return the RAM Disk handle. > > @return The load option buffer. > **/ >@@ -1102,7 +1103,8 @@ VOID * > BmGetFileBufferFromLoadFileSystem ( > IN EFI_HANDLE LoadFileHandle, > OUT EFI_DEVICE_PATH_PROTOCOL **FullPath, >- OUT UINTN *FileSize >+ OUT UINTN *FileSize, >+ OUT EFI_HANDLE *RamDiskHandle > ) > { > EFI_STATUS Status; >@@ -1140,7 +1142,13 @@ BmGetFileBufferFromLoadFileSystem ( > FreePool (Handles); > } > >- if (Index != HandleCount) { >+ if (Index == HandleCount) { >+ Handle = NULL; >+ } >+ >+ *RamDiskHandle = Handle; >+ >+ if (Handle != NULL) { > return BmExpandMediaDevicePath (DevicePathFromHandle (Handle), FullPath, > FileSize); > } else { > return NULL; >@@ -1149,6 +1157,124 @@ BmGetFileBufferFromLoadFileSystem ( > > > /** >+ Return the RAM DISK handle created by LoadFile. >+ >+ @param FilePath The source file path. >+ >+ @return RAM DISK handle. >+**/ >+EFI_HANDLE >+BmGetLoadFileRamDisk ( >+ EFI_DEVICE_PATH_PROTOCOL *FilePath >+ ) >+{ >+ EFI_STATUS Status; >+ EFI_DEVICE_PATH_PROTOCOL *RamDiskDevicePath; >+ EFI_DEVICE_PATH_PROTOCOL *Node; >+ EFI_HANDLE Handle; >+ >+ Node = FilePath; >+ Status = gBS->LocateDevicePath (&gEfiLoadFileProtocolGuid, &Node, >+ &Handle); if (!EFI_ERROR (Status) && >+ (DevicePathType (Node) == MEDIA_DEVICE_PATH) && >+ (DevicePathSubType (Node) == MEDIA_RAM_DISK_DP) >+ ) { >+ >+ // >+ // Construct the device path pointing to RAM Disk >+ // >+ Node = NextDevicePathNode (Node); >+ RamDiskDevicePath = DuplicateDevicePath (FilePath); >+ ASSERT (RamDiskDevicePath != NULL); >+ SetDevicePathEndNode ((VOID *) ((UINTN) RamDiskDevicePath + >+ ((UINTN) Node - (UINTN) FilePath))); >+ >+ // >+ // Query the RAM Disk handle using the constructed device path. >+ // >+ Node = RamDiskDevicePath; >+ Status = gBS->LocateDevicePath (&gEfiBlockIoProtocolGuid, &Node, &Handle); >+ ASSERT ((Status == EFI_SUCCESS) && IsDevicePathEnd (Node)); >+ FreePool (RamDiskDevicePath); >+ return Handle; >+ } >+ >+ return NULL; >+} >+ >+/** >+ Return the buffer and buffer size occupied by the RAM Disk. >+ >+ @param RamDiskHandle RAM Disk handle. >+ @param RamDiskSizeInPages Return RAM Disk size in pages. >+ >+ @retval RAM Disk buffer. >+**/ >+VOID * >+BmGetRamDiskMemoryInfo ( >+ EFI_HANDLE RamDiskHandle, >+ UINTN *RamDiskSizeInPages >+ ) >+{ >+ >+ EFI_STATUS Status; >+ EFI_DEVICE_PATH_PROTOCOL *DevicePath; >+ EFI_HANDLE Handle; >+ UINT64 StartingAddr; >+ UINT64 EndingAddr; >+ >+ *RamDiskSizeInPages = 0; >+ if (RamDiskHandle == NULL) { >+ return NULL; >+ } >+ DevicePath = DevicePathFromHandle (RamDiskHandle); >+ >+ // >+ // Get the buffer occupied by RAM Disk. >+ // >+ Status = gBS->LocateDevicePath (&gEfiLoadFileProtocolGuid, >+&DevicePath, &Handle); >+ ASSERT_EFI_ERROR (Status); >+ ASSERT ((DevicePathType (DevicePath) == MEDIA_DEVICE_PATH) && >+(DevicePathSubType (DevicePath) == MEDIA_RAM_DISK_DP)); >+ StartingAddr = ReadUnaligned64 ((UINT64 *) ((MEDIA_RAM_DISK_DEVICE_PATH *) >DevicePath)->StartingAddr); >+ EndingAddr = ReadUnaligned64 ((UINT64 *) ((MEDIA_RAM_DISK_DEVICE_PATH *) >DevicePath)->EndingAddr); >+ *RamDiskSizeInPages = EFI_SIZE_TO_PAGES ((UINTN) (EndingAddr - >+StartingAddr + 1)); >+ return (VOID *) (UINTN) StartingAddr; } >+ >+/** >+ Destroy the RAM Disk handle. >+ >+ The destroy operation includes to call RamDisk.Unregister to >+ unregister the RAM DISK from RAM DISK driver, free the memory >+ allocated for the RAM Disk. >+ >+ @param RamDiskHandle RAM Disk handle. >+**/ >+VOID >+BmDestroyRamDisk ( >+ EFI_HANDLE RamDiskHandle >+ ) >+{ >+ EFI_STATUS Status; >+ EFI_RAM_DISK_PROTOCOL *RamDisk; >+ VOID *RamDiskBuffer; >+ UINTN RamDiskSizeInPages; >+ >+ if (RamDiskHandle == NULL) { >+ return; >+ } >+ RamDiskBuffer = BmGetRamDiskMemoryInfo (RamDiskHandle, >+ &RamDiskSizeInPages); >+ >+ // >+ // Destroy RAM Disk. >+ // >+ Status = gBS->LocateProtocol (&gEfiRamDiskProtocolGuid, NULL, (VOID >+*) &RamDisk); >+ ASSERT_EFI_ERROR (Status); >+ Status = RamDisk->Unregister (DevicePathFromHandle (RamDiskHandle)); >+ ASSERT_EFI_ERROR (Status); >+ FreePages (RamDiskBuffer, RamDiskSizeInPages); } >+ >+/** > Get the file buffer from the specified Load File instance. > > @param LoadFileHandle The specified Load File instance. >@@ -1170,6 +1296,7 @@ BmGetFileBufferFromLoadFile ( > EFI_LOAD_FILE_PROTOCOL *LoadFile; > VOID *FileBuffer; > BOOLEAN LoadFileSystem; >+ EFI_HANDLE RamDiskHandle; > UINTN BufferSize; > > *FileSize = 0; >@@ -1204,7 +1331,13 @@ BmGetFileBufferFromLoadFile ( > } > > if (LoadFileSystem) { >- FileBuffer = BmGetFileBufferFromLoadFileSystem (LoadFileHandle, FullPath, >FileSize); >+ FileBuffer = BmGetFileBufferFromLoadFileSystem (LoadFileHandle, FullPath, >FileSize, &RamDiskHandle); >+ if (FileBuffer == NULL) { >+ // >+ // If there is no bootable executable in the populated >+ // >+ BmDestroyRamDisk (RamDiskHandle); >+ } > } else { > *FileSize = BufferSize; > *FullPath = DuplicateDevicePath (DevicePathFromHandle (LoadFileHandle)); > @@ -1432,6 +1565,7 @@ >EfiBootManagerBoot ( > UINTN OriginalOptionNumber; > EFI_DEVICE_PATH_PROTOCOL *FilePath; > EFI_DEVICE_PATH_PROTOCOL *Node; >+ EFI_HANDLE RamDiskHandle; > EFI_HANDLE FvHandle; > VOID *FileBuffer; > UINTN FileSize; >@@ -1512,10 +1646,14 @@ EfiBootManagerBoot ( > // > // 5. Load EFI boot option to ImageHandle > // >- ImageHandle = NULL; >+ ImageHandle = NULL; >+ RamDiskHandle = NULL; > if (DevicePathType (BootOption->FilePath) != BBS_DEVICE_PATH) { > Status = EFI_NOT_FOUND; > FileBuffer = BmGetLoadOptionBuffer (BootOption->FilePath, &FilePath, > &FileSize); >+ if (FileBuffer != NULL) { >+ RamDiskHandle = BmGetLoadFileRamDisk (FilePath); >+ } > DEBUG_CODE ( > if (FileBuffer != NULL && CompareMem (BootOption->FilePath, FilePath, > GetDevicePathSize (FilePath)) != 0) { > DEBUG ((EFI_D_INFO, "[Bds] DevicePath expand: ")); @@ -1552,6 > +1690,10 @@ EfiBootManagerBoot ( > (EFI_SOFTWARE_DXE_BS_DRIVER | EFI_SW_DXE_BS_EC_BOOT_OPTION_LOAD_ERROR) > ); > BootOption->Status = Status; >+ // >+ // Destroy the RAM disk >+ // >+ BmDestroyRamDisk (RamDiskHandle); > return; > } > } >@@ -1648,6 +1790,11 @@ EfiBootManagerBoot ( > PERF_END_EX (gImageHandle, "BdsAttempt", NULL, 0, (UINT32) OptionNumber); > > // >+ // Destroy the RAM disk >+ // >+ BmDestroyRamDisk (RamDiskHandle); >+ >+ // > // Clear the Watchdog Timer after the image returns > // > gBS->SetWatchdogTimer (0x0000, 0x0000, 0x0000, NULL); diff --git >a/MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h >b/MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h >index 7b6252a..4660323 100644 >--- a/MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h >+++ b/MdeModulePkg/Library/UefiBootManagerLib/InternalBm.h >@@ -43,6 +43,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER >EXPRESS OR IMPLIED. > #include <Protocol/DriverHealth.h> > #include <Protocol/FormBrowser2.h> > #include <Protocol/VariableLock.h> >+#include <Protocol/RamDisk.h> > > #include <Guid/ZeroGuid.h> > #include <Guid/MemoryTypeInformation.h> diff --git >a/MdeModulePkg/Library/UefiBootManagerLib/UefiBootManagerLib.inf >b/MdeModulePkg/Library/UefiBootManagerLib/UefiBootManagerLib.inf >index e9e74b1..9d62d3d 100644 >--- a/MdeModulePkg/Library/UefiBootManagerLib/UefiBootManagerLib.inf >+++ b/MdeModulePkg/Library/UefiBootManagerLib/UefiBootManagerLib.inf >@@ -108,6 +108,7 @@ > gEfiDiskInfoProtocolGuid ## SOMETIMES_CONSUMES > gEfiDriverHealthProtocolGuid ## SOMETIMES_CONSUMES > gEfiFormBrowser2ProtocolGuid ## SOMETIMES_CONSUMES >+ gEfiRamDiskProtocolGuid ## SOMETIMES_CONSUMES > > [Pcd] > gEfiMdeModulePkgTokenSpaceGuid.PcdResetOnMemoryTypeInformationChange > ## SOMETIMES_CONSUMES >-- >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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
