[edk2-devel] [PATCH v1 3/5] MdeModulePkg: MemoryProfileInfo: Updated MessageLength calculation
Kun Qin
kuqin12 at gmail.com
Fri Jun 11 21:29:48 UTC 2021
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.
Regards,
Kun
On 06/11/2021 00:46, Wu, Hao A wrote:
>> -----Original Message-----
>> From: Kun Qin <kuqin12 at gmail.com>
>> Sent: Thursday, June 10, 2021 9:43 AM
>> To: devel at edk2.groups.io
>> Cc: Wang, Jian J <jian.j.wang at intel.com>; Wu, Hao A <hao.a.wu at 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.wang at intel.com>
>> Cc: Hao A Wu <hao.a.wu at intel.com>
>>
>> Signed-off-by: Kun Qin <kuqin12 at 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 (#76412): https://edk2.groups.io/g/devel/message/76412
Mute This Topic: https://groups.io/mt/83435924/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list