Re: [PATCH v1 3/5] MdeModulePkg: MemoryProfileInfo: Updated MessageLength calculation

["Kun Qin" <kuqin12@gmail.com>]

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@gmail.com>
>> Sent: Thursday, June 10, 2021 9:43 AM
>> To: devel@edk2.groups.io
>> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@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@intel.com>
>> Cc: Hao A Wu <hao.a.wu@intel.com>
>>
>> Signed-off-by: Kun Qin <kuqin12@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
>