> -----Original Message----- > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Kun Qin > Sent: Saturday, June 12, 2021 5:30 AM > To: Wu, Hao A <hao.a...@intel.com>; devel@edk2.groups.io > Cc: Wang, Jian J <jian.j.w...@intel.com> > Subject: Re: [edk2-devel] [PATCH v1 3/5] MdeModulePkg: MemoryProfileInfo: > Updated MessageLength calculation > > Hi Hao, > > Thanks for pointing out the missing place. Will update this accordingly. > > This patch series needs a PI spec update, I thought I should mark all changes > with BZ#### before the spec update is taken. But I can drop them for the next > patch version.
Thanks a lot. Please also help to check my other comments sent previously for other patches in the series. Best Regards, Hao Wu > > Regards, > Kun > > On 06/11/2021 00:46, Wu, Hao A wrote: > >> -----Original Message----- > >> From: Kun Qin <kuqi...@gmail.com> > >> Sent: Thursday, June 10, 2021 9:43 AM > >> To: devel@edk2.groups.io > >> Cc: Wang, Jian J <jian.j.w...@intel.com>; Wu, Hao A > >> <hao.a...@intel.com> > >> Subject: [PATCH v1 3/5] MdeModulePkg: MemoryProfileInfo: Updated > >> MessageLength calculation > >> > >> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3398 > >> > >> This change replaced the calculation of communication buffer size > >> from explicitly adding the size of each member with the OFFSET macro > function. > >> This will make the structure field defition change transparent to > >> consumers. > > > > > > I think there is one missing place in function GetSmramProfileData(): > > > > MinimalSizeNeeded = sizeof (EFI_GUID) + > > sizeof (UINTN) + > > MAX (sizeof > > (SMRAM_PROFILE_PARAMETER_GET_PROFILE_INFO), > > MAX (sizeof > (SMRAM_PROFILE_PARAMETER_GET_PROFILE_DATA_BY_OFFSET), > > sizeof > > (SMRAM_PROFILE_PARAMETER_RECORDING_STATE))); > > > > More inline comments below: > > > > > >> > >> Cc: Jian J Wang <jian.j.w...@intel.com> > >> Cc: Hao A Wu <hao.a...@intel.com> > >> > >> Signed-off-by: Kun Qin <kuqi...@gmail.com> > >> --- > >> MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.c | 20 > >> +++++++++++++++----- > >> 1 file changed, 15 insertions(+), 5 deletions(-) > >> > >> diff --git > >> a/MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.c > >> b/MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.c > >> index 191c31068545..39ed8b2e0484 100644 > >> --- a/MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.c > >> +++ > >> b/MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.c > >> @@ -1190,7 +1190,9 @@ GetSmramProfileData ( > >> CommRecordingState->Header.ReturnStatus = (UINT64)-1; > >> CommRecordingState->RecordingState = > >> MEMORY_PROFILE_RECORDING_DISABLE; > >> > >> - CommSize = sizeof (EFI_GUID) + sizeof (UINTN) + CommHeader- > >>> MessageLength; > >> + // BZ3398: Make MessageLength the same size in > >> EFI_MM_COMMUNICATE_HEADER for both IA32 and X64. > >> + // The CommHeader->MessageLength contains a definitive value, thus > >> UINTN cast is safe here. > > > > > > Please help to drop the explicit mention of BZ3398 in the comment. > > How about using: > > // > > // The CommHeader->MessageLength contains a definitive value, thus > UINTN cast is safe here. > > // > > > > There are 4 more similar cases below. > > > > Best Regards, > > Hao Wu > > > > > >> + CommSize = OFFSET_OF(EFI_SMM_COMMUNICATE_HEADER, Data) + > >> + (UINTN)CommHeader->MessageLength; > >> Status = SmmCommunication->Communicate (SmmCommunication, > >> CommBuffer, &CommSize); > >> if (EFI_ERROR (Status)) { > >> DEBUG ((EFI_D_ERROR, "SmramProfile: SmmCommunication - %r\n", > >> Status)); @@ -1213,7 +1215,9 @@ GetSmramProfileData ( > >> CommRecordingState->Header.ReturnStatus = (UINT64)-1; > >> CommRecordingState->RecordingState = > >> MEMORY_PROFILE_RECORDING_DISABLE; > >> > >> - CommSize = sizeof (EFI_GUID) + sizeof (UINTN) + CommHeader- > >>> MessageLength; > >> + // BZ3398: Make MessageLength the same size in > >> EFI_MM_COMMUNICATE_HEADER for both IA32 and X64. > >> + // The CommHeader->MessageLength contains a definitive value, > >> + thus > >> UINTN cast is safe here. > >> + CommSize = OFFSET_OF(EFI_SMM_COMMUNICATE_HEADER, Data) + > >> + (UINTN)CommHeader->MessageLength; > >> SmmCommunication->Communicate (SmmCommunication, CommBuffer, > >> &CommSize); > >> } > >> > >> @@ -1230,7 +1234,9 @@ GetSmramProfileData ( > >> CommGetProfileInfo->Header.ReturnStatus = (UINT64)-1; > >> CommGetProfileInfo->ProfileSize = 0; > >> > >> - CommSize = sizeof (EFI_GUID) + sizeof (UINTN) + CommHeader- > >>> MessageLength; > >> + // BZ3398: Make MessageLength the same size in > >> EFI_MM_COMMUNICATE_HEADER for both IA32 and X64. > >> + // The CommHeader->MessageLength contains a definitive value, thus > >> UINTN cast is safe here. > >> + CommSize = OFFSET_OF(EFI_SMM_COMMUNICATE_HEADER, Data) + > >> + (UINTN)CommHeader->MessageLength; > >> Status = SmmCommunication->Communicate (SmmCommunication, > >> CommBuffer, &CommSize); > >> ASSERT_EFI_ERROR (Status); > >> > >> @@ -1261,7 +1267,9 @@ GetSmramProfileData ( > >> CommGetProfileData->Header.DataLength = sizeof > >> (*CommGetProfileData); > >> CommGetProfileData->Header.ReturnStatus = (UINT64)-1; > >> > >> - CommSize = sizeof (EFI_GUID) + sizeof (UINTN) + CommHeader- > >>> MessageLength; > >> + // BZ3398: Make MessageLength the same size in > >> EFI_MM_COMMUNICATE_HEADER for both IA32 and X64. > >> + // The CommHeader->MessageLength contains a definitive value, thus > >> UINTN cast is safe here. > >> + CommSize = OFFSET_OF(EFI_SMM_COMMUNICATE_HEADER, Data) + > >> + (UINTN)CommHeader->MessageLength; > >> Buffer = (UINT8 *) CommHeader + CommSize; > >> Size -= CommSize; > >> > >> @@ -1320,7 +1328,9 @@ GetSmramProfileData ( > >> CommRecordingState->Header.ReturnStatus = (UINT64)-1; > >> CommRecordingState->RecordingState = > >> MEMORY_PROFILE_RECORDING_ENABLE; > >> > >> - CommSize = sizeof (EFI_GUID) + sizeof (UINTN) + CommHeader- > >>> MessageLength; > >> + // BZ3398: Make MessageLength the same size in > >> EFI_MM_COMMUNICATE_HEADER for both IA32 and X64. > >> + // The CommHeader->MessageLength contains a definitive value, > >> + thus > >> UINTN cast is safe here. > >> + CommSize = OFFSET_OF(EFI_SMM_COMMUNICATE_HEADER, Data) + > >> + (UINTN)CommHeader->MessageLength; > >> SmmCommunication->Communicate (SmmCommunication, CommBuffer, > >> &CommSize); > >> } > >> > >> -- > >> 2.31.1.windows.1 > > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#76491): https://edk2.groups.io/g/devel/message/76491 Mute This Topic: https://groups.io/mt/83435924/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
