Hi Mike, I did some investigation. The driver does affect drivers or libraries that use EDI_LOADED_IMAGE_PROTOCOL or Image Handle. In EDK2 repo, I find the following modules should be modified because of this code change. And after proper modification, the module will remain the same functionally. MdePkg\Library\UefiDriverEntryPoint\UefiDriverEntryPoint.inf MdePkg\Library\DxeServicesLib\DxeServicesLib.inf MdeModulePkg\Library\SmmCorePerformanceLib\SmmCorePerformanceLib.inf
Do you know other modules that depend on the EDI_LOADED_IMAGE_PROTOCOL struct? Because this code change also has effect on internal platform, I will do more research and send other patches in this patch set for review first. Thanks Zhiguang > -----Original Message----- > From: Kinney, Michael D <michael.d.kin...@intel.com> > Sent: Wednesday, June 17, 2020 6:40 AM > To: Liu, Zhiguang <zhiguang....@intel.com>; devel@edk2.groups.io; Kinney, > Michael D <michael.d.kin...@intel.com> > Cc: Tian, Feng <feng.t...@intel.com>; Zeng, Star <star.z...@intel.com>; > Laszlo Ersek <ler...@redhat.com>; Yao, Jiewen <jiewen....@intel.com> > Subject: RE: [PATCH 2/5] MdeModulePkg/SmmCore: Remove UEFI > LOADED_IMAGE from SMM. > > Zhiguang, > > There are some source level debug solutions that depend on the > EDI_LOADED_IMAGE_PROTOCOL structure. Does this change reduce the > source level debug capabilities for SMM drivers under development? > > Thanks, > > Mike > > > -----Original Message----- > > From: Liu, Zhiguang <zhiguang....@intel.com> > > Sent: Tuesday, June 16, 2020 2:05 AM > > To: devel@edk2.groups.io > > Cc: Tian, Feng <feng.t...@intel.com>; Zeng, Star > > <star.z...@intel.com>; Kinney, Michael D <michael.d.kin...@intel.com>; > > Laszlo Ersek <ler...@redhat.com>; Yao, Jiewen <jiewen....@intel.com> > > Subject: [PATCH 2/5] MdeModulePkg/SmmCore: Remove UEFI > LOADED_IMAGE > > from SMM. > > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2317 > > > > This patch removes EFI_LOADED_IMAGE_PROTOCOL from DXE database. > > The SmmCore only install EFI_LOADED_IMAGE_PROTOCOL to SMM > database. > > > > Exposing LOADED_IMAGE_PROTOCOL may cause SMM information > leakage to > > non-SMM component. > > > > Cc: Feng Tian <feng.t...@intel.com> > > Cc: Star Zeng <star.z...@intel.com> > > Cc: Michael D Kinney <michael.d.kin...@intel.com> > > Cc: Laszlo Ersek <ler...@redhat.com> > > Signed-off-by: Jiewen Yao <jiewen....@intel.com> > > Signed-off-by: Zhiguang Liu <zhiguang....@intel.com> > > --- > > MdeModulePkg/Core/PiSmmCore/Dispatcher.c | 104 > > +++++++++++--------------------------------------------- > > ------------------------------------------------ > > MdeModulePkg/Core/PiSmmCore/PiSmmCore.c | 34 -------- > > -------------------------- > > MdeModulePkg/Core/PiSmmCore/PiSmmCore.h | 4 ++-- > > 3 files changed, 13 insertions(+), 129 deletions(-) > > > > diff --git a/MdeModulePkg/Core/PiSmmCore/Dispatcher.c > > b/MdeModulePkg/Core/PiSmmCore/Dispatcher.c > > index 76ee9e0b89..2be2866234 100644 > > --- a/MdeModulePkg/Core/PiSmmCore/Dispatcher.c > > +++ b/MdeModulePkg/Core/PiSmmCore/Dispatcher.c > > @@ -316,7 +316,7 @@ SmmLoadImage ( > > EFI_FIRMWARE_VOLUME2_PROTOCOL *Fv; > > > > PE_COFF_LOADER_IMAGE_CONTEXT ImageContext; > > > > > > > > - PERF_LOAD_IMAGE_BEGIN (DriverEntry->ImageHandle); > > > > + PERF_LOAD_IMAGE_BEGIN (DriverEntry->SmmImageHandle); > > > > > > > > Buffer = NULL; > > > > Size = 0; > > > > @@ -546,51 +546,14 @@ SmmLoadImage ( > > DriverEntry->ImageBuffer = DstBuffer; > > > > DriverEntry->NumberOfPage = PageCount; > > > > > > > > - // > > > > - // Allocate a Loaded Image Protocol in EfiBootServicesData > > > > - // > > > > - Status = gBS->AllocatePool (EfiBootServicesData, sizeof > > (EFI_LOADED_IMAGE_PROTOCOL), (VOID **)&DriverEntry- > >LoadedImage); > > > > - if (EFI_ERROR (Status)) { > > > > - if (Buffer != NULL) { > > > > - gBS->FreePool (Buffer); > > > > - } > > > > - SmmFreePages (DstBuffer, PageCount); > > > > - return Status; > > > > - } > > > > - > > > > - ZeroMem (DriverEntry->LoadedImage, sizeof > > (EFI_LOADED_IMAGE_PROTOCOL)); > > > > // > > > > // Fill in the remaining fields of the Loaded Image Protocol > > instance. > > > > - // Note: ImageBase is an SMRAM address that can not be accessed > > outside of SMRAM if SMRAM window is closed. > > > > // > > > > - DriverEntry->LoadedImage->Revision = > > EFI_LOADED_IMAGE_PROTOCOL_REVISION; > > > > - DriverEntry->LoadedImage->ParentHandle = > > gSmmCorePrivate->SmmIplImageHandle; > > > > - DriverEntry->LoadedImage->SystemTable = gST; > > > > - DriverEntry->LoadedImage->DeviceHandle = DeviceHandle; > > > > - > > > > DriverEntry->SmmLoadedImage.Revision = > > EFI_LOADED_IMAGE_PROTOCOL_REVISION; > > > > DriverEntry->SmmLoadedImage.ParentHandle = > > gSmmCorePrivate->SmmIplImageHandle; > > > > DriverEntry->SmmLoadedImage.SystemTable = gST; > > > > DriverEntry->SmmLoadedImage.DeviceHandle = DeviceHandle; > > > > > > > > - // > > > > - // Make an EfiBootServicesData buffer copy of FilePath > > > > - // > > > > - Status = gBS->AllocatePool (EfiBootServicesData, GetDevicePathSize > > (FilePath), (VOID **)&DriverEntry- > > >LoadedImage->FilePath); > > > > - if (EFI_ERROR (Status)) { > > > > - if (Buffer != NULL) { > > > > - gBS->FreePool (Buffer); > > > > - } > > > > - SmmFreePages (DstBuffer, PageCount); > > > > - return Status; > > > > - } > > > > - CopyMem (DriverEntry->LoadedImage->FilePath, > > FilePath, GetDevicePathSize (FilePath)); > > > > - > > > > - DriverEntry->LoadedImage->ImageBase = (VOID > > *)(UINTN) ImageContext.ImageAddress; > > > > - DriverEntry->LoadedImage->ImageSize = > > ImageContext.ImageSize; > > > > - DriverEntry->LoadedImage->ImageCodeType = EfiRuntimeServicesCode; > > > > - DriverEntry->LoadedImage->ImageDataType = EfiRuntimeServicesData; > > > > - > > > > // > > > > // Make a buffer copy of FilePath > > > > // > > > > @@ -599,7 +562,6 @@ SmmLoadImage ( > > if (Buffer != NULL) { > > > > gBS->FreePool (Buffer); > > > > } > > > > - gBS->FreePool (DriverEntry->LoadedImage->FilePath); > > > > SmmFreePages (DstBuffer, PageCount); > > > > return Status; > > > > } > > > > @@ -610,16 +572,6 @@ SmmLoadImage ( > > DriverEntry->SmmLoadedImage.ImageCodeType = > EfiRuntimeServicesCode; > > > > DriverEntry->SmmLoadedImage.ImageDataType = > EfiRuntimeServicesData; > > > > > > > > - // > > > > - // Create a new image handle in the UEFI handle database for the > > SMM Driver > > > > - // > > > > - DriverEntry->ImageHandle = NULL; > > > > - Status = gBS->InstallMultipleProtocolInterfaces ( > > > > - &DriverEntry->ImageHandle, > > > > - &gEfiLoadedImageProtocolGuid, > > DriverEntry->LoadedImage, > > > > - NULL > > > > - ); > > > > - > > > > // > > > > // Create a new image handle in the SMM handle database for the SMM > > Driver > > > > // > > > > @@ -631,7 +583,7 @@ SmmLoadImage ( > > &DriverEntry->SmmLoadedImage > > > > ); > > > > > > > > - PERF_LOAD_IMAGE_END (DriverEntry->ImageHandle); > > > > + PERF_LOAD_IMAGE_END (DriverEntry->SmmImageHandle); > > > > > > > > // > > > > // Print the load address and the PDB file name if it is available > > > > @@ -856,7 +808,7 @@ SmmDispatcher ( > > // Untrused to Scheduled it would have already been loaded so > > we may need to > > > > // skip the LoadImage > > > > // > > > > - if (DriverEntry->ImageHandle == NULL) { > > > > + if (DriverEntry->SmmImageHandle == NULL) { > > > > Status = SmmLoadImage (DriverEntry); > > > > > > > > // > > > > @@ -885,8 +837,8 @@ SmmDispatcher ( > > REPORT_STATUS_CODE_WITH_EXTENDED_DATA ( > > > > EFI_PROGRESS_CODE, > > > > EFI_SOFTWARE_SMM_DRIVER | EFI_SW_PC_INIT_BEGIN, > > > > - &DriverEntry->ImageHandle, > > > > - sizeof (DriverEntry->ImageHandle) > > > > + &DriverEntry->SmmImageHandle, > > > > + sizeof (DriverEntry->SmmImageHandle) > > > > ); > > > > > > > > // > > > > @@ -898,9 +850,10 @@ SmmDispatcher ( > > // For each SMM driver, pass NULL as ImageHandle > > > > // > > > > RegisterSmramProfileImage (DriverEntry, TRUE); > > > > - PERF_START_IMAGE_BEGIN (DriverEntry- > > >ImageHandle); > > > > - Status = > > ((EFI_IMAGE_ENTRY_POINT)(UINTN)DriverEntry- > > >ImageEntryPoint)(DriverEntry->ImageHandle, gST); > > > > - PERF_START_IMAGE_END (DriverEntry->ImageHandle); > > > > + DEBUG ((DEBUG_INFO, "SMM StartImage - 0x%11p\n", > > DriverEntry->ImageEntryPoint)); > > > > + PERF_START_IMAGE_BEGIN (DriverEntry- > > >SmmImageHandle); > > > > + Status = > > ((EFI_IMAGE_ENTRY_POINT)(UINTN)DriverEntry- > > >ImageEntryPoint)(DriverEntry->SmmImageHandle, gST); > > > > + PERF_START_IMAGE_END (DriverEntry- > > >SmmImageHandle); > > > > if (EFI_ERROR(Status)){ > > > > DEBUG (( > > > > DEBUG_ERROR, > > > > @@ -910,20 +863,6 @@ SmmDispatcher ( > > )); > > > > UnregisterSmramProfileImage (DriverEntry, TRUE); > > > > SmmFreePages(DriverEntry->ImageBuffer, > > DriverEntry->NumberOfPage); > > > > - // > > > > - // Uninstall LoadedImage > > > > - // > > > > - Status = gBS->UninstallProtocolInterface ( > > > > - DriverEntry->ImageHandle, > > > > - &gEfiLoadedImageProtocolGuid, > > > > - DriverEntry->LoadedImage > > > > - ); > > > > - if (!EFI_ERROR (Status)) { > > > > - if (DriverEntry->LoadedImage->FilePath != > > NULL) { > > > > - gBS->FreePool (DriverEntry->LoadedImage- > > >FilePath); > > > > - } > > > > - gBS->FreePool (DriverEntry->LoadedImage); > > > > - } > > > > Status = SmmUninstallProtocolInterface ( > > > > DriverEntry->SmmImageHandle, > > > > &gEfiLoadedImageProtocolGuid, > > > > @@ -939,8 +878,8 @@ SmmDispatcher ( > > REPORT_STATUS_CODE_WITH_EXTENDED_DATA ( > > > > EFI_PROGRESS_CODE, > > > > EFI_SOFTWARE_SMM_DRIVER | EFI_SW_PC_INIT_END, > > > > - &DriverEntry->ImageHandle, > > > > - sizeof (DriverEntry->ImageHandle) > > > > + &DriverEntry->SmmImageHandle, > > > > + sizeof (DriverEntry->SmmImageHandle) > > > > ); > > > > > > > > if (!PreviousSmmEntryPointRegistered && > > gSmmCorePrivate->SmmEntryPointRegistered) { > > > > @@ -1344,27 +1283,6 @@ SmmDriverDispatchHandler ( > > // > > > > // If this is the SMM core fill in it's DevicePath & > > DeviceHandle > > > > // > > > > - if (mSmmCoreLoadedImage->FilePath == NULL) > > { > > > > - // > > > > - // Maybe one special FV contains only one > > SMM_CORE module, so its device path must > > > > - // be initialized completely. > > > > - // > > > > - EfiInitializeFwVolDevicepathNode > > (&mFvDevicePath.File, &NameGuid); > > > > - SetDevicePathEndNode > > (&mFvDevicePath.End); > > > > - > > > > - // > > > > - // Make an EfiBootServicesData buffer > > copy of FilePath > > > > - // > > > > - Status = gBS->AllocatePool ( > > > > - EfiBootServicesData, > > > > - GetDevicePathSize > > ((EFI_DEVICE_PATH_PROTOCOL *)&mFvDevicePath), > > > > - (VOID > > **)&mSmmCoreLoadedImage->FilePath > > > > - ); > > > > - ASSERT_EFI_ERROR (Status); > > > > - CopyMem (mSmmCoreLoadedImage->FilePath, > > &mFvDevicePath, GetDevicePathSize > > ((EFI_DEVICE_PATH_PROTOCOL *)&mFvDevicePath)); > > > > - > > > > - mSmmCoreLoadedImage->DeviceHandle = > > FvHandle; > > > > - } > > > > if (mSmmCoreDriverEntry- > > >SmmLoadedImage.FilePath == NULL) { > > > > // > > > > // Maybe one special FV contains only one SMM_CORE > > module, so its device path must > > > > diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c > > b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c > > index cfa9922cbd..c1b18b047e 100644 > > --- a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c > > +++ b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c > > @@ -104,8 +104,6 @@ EFI_SMRAM_DESCRIPTOR *mFullSmramRanges; > > > > > > EFI_SMM_DRIVER_ENTRY *mSmmCoreDriverEntry; > > > > > > > > -EFI_LOADED_IMAGE_PROTOCOL *mSmmCoreLoadedImage; > > > > - > > > > /** > > > > Place holder function until all the SMM System Table Service are > > available. > > > > > > > > @@ -756,38 +754,6 @@ SmmCoreInstallLoadedImage ( > > ) > > > > { > > > > EFI_STATUS Status; > > > > - EFI_HANDLE Handle; > > > > - > > > > - // > > > > - // Allocate a Loaded Image Protocol in EfiBootServicesData > > > > - // > > > > - Status = gBS->AllocatePool (EfiBootServicesData, > > sizeof(EFI_LOADED_IMAGE_PROTOCOL), (VOID > **)&mSmmCoreLoadedImage); > > > > - ASSERT_EFI_ERROR (Status); > > > > - > > > > - ZeroMem (mSmmCoreLoadedImage, sizeof > (EFI_LOADED_IMAGE_PROTOCOL)); > > > > - // > > > > - // Fill in the remaining fields of the Loaded Image Protocol > > instance. > > > > - // Note: ImageBase is an SMRAM address that can not be accessed > > outside of SMRAM if SMRAM window is closed. > > > > - // > > > > - mSmmCoreLoadedImage->Revision = > > EFI_LOADED_IMAGE_PROTOCOL_REVISION; > > > > - mSmmCoreLoadedImage->ParentHandle = gSmmCorePrivate- > > >SmmIplImageHandle; > > > > - mSmmCoreLoadedImage->SystemTable = gST; > > > > - > > > > - mSmmCoreLoadedImage->ImageBase = (VOID > > *)(UINTN)gSmmCorePrivate->PiSmmCoreImageBase; > > > > - mSmmCoreLoadedImage->ImageSize = gSmmCorePrivate- > > >PiSmmCoreImageSize; > > > > - mSmmCoreLoadedImage->ImageCodeType = EfiRuntimeServicesCode; > > > > - mSmmCoreLoadedImage->ImageDataType = EfiRuntimeServicesData; > > > > - > > > > - // > > > > - // Create a new image handle in the UEFI handle database for the > > SMM Driver > > > > - // > > > > - Handle = NULL; > > > > - Status = gBS->InstallMultipleProtocolInterfaces ( > > > > - &Handle, > > > > - &gEfiLoadedImageProtocolGuid, > > mSmmCoreLoadedImage, > > > > - NULL > > > > - ); > > > > - ASSERT_EFI_ERROR (Status); > > > > > > > > // > > > > // Allocate a Loaded Image Protocol in SMM > > > > diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.h > > b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.h > > index 50a7fc0000..3bbd1e1603 100644 > > --- a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.h > > +++ b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.h > > @@ -122,8 +122,8 @@ typedef struct { > > BOOLEAN Initialized; > > > > BOOLEAN DepexProtocolError; > > > > > > > > - EFI_HANDLE ImageHandle; > > > > - EFI_LOADED_IMAGE_PROTOCOL *LoadedImage; > > > > + EFI_HANDLE > > ImageHandle_Reserved1; > > > > + EFI_LOADED_IMAGE_PROTOCOL > > *LoadedImage_Reserved2; > > > > // > > > > // Image EntryPoint in SMRAM > > > > // > > > > -- > > 2.25.1.windows.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#61581): https://edk2.groups.io/g/devel/message/61581 Mute This Topic: https://groups.io/mt/74912558/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
