Re: [edk2-devel] [PATCH v1 0/5] EDK2 Code First: PI Specification: Update EFI_MM_COMMUNICATE_HEADER

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

Hi Marvin,

Thanks for reaching out. Please see my comment inline.

Regards,
Kun

On 06/16/2021 00:02, Marvin Häuser wrote:
> Good day,
> 
> May I ask about two small things?
> 
> 1) Was there any rationale as to e.g. code compatibility with choosing 
> UINT64 for the data length? I understand that this is the maximum of the 
> two as of currently, but I wonder whether a message length that exceeds 
> the UINT32 range (4 GB!) can possibly be considered sane or a good idea.
I agree that >4GB communication buffer may not be practical as of today. 
That is why we modified the SMM communication routine in PiSmmIpl to use 
SafeInt functions in Patch 2 of this series. With that change, at least 
for 32bit MM, larger than 4GB will be considered insane. But in the 
meantime, I do not want to rule out the >4GB communication capability 
that a 64bit MM currently already have.

Please let me know if I missed anything in regards to adding SafeInt 
functions to SmmCommunication routine.
> 
> 2) Is it feasible yet with the current set of supported compilers to 
> support flexible arrays?
My impression is that flexible arrays are already supported (as seen in 
UnitTestFrameworkPkg/PrivateInclude/UnitTestFrameworkTypes.h). Please 
correct me if I am wrong.

Would you mind letting me know why this is applicable here? We are 
trying to seek ideas on how to catch developer mistakes caused by this 
change. So any input is appreciated.
> 
> Thank you for your work!
> 
> Best regards,
> Marvin
> 
> On 10.06.21 03:42, Kun Qin wrote:
>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3430
>>
>> In PI Spec v1.7 Errata A, Vol.4, Sec 5.7 MM Communication Protocol, the
>> MessageLength field of EFI_MM_COMMUNICATE_HEADER (also derived as
>> EFI_SMM_COMMUNICATE_HEADER) is currently defined as type UINTN.
>>
>> But this structure, as a generic definition, could be used for both PEI
>> and DXE MM communication. Thus for a system that supports PEI MM launch,
>> but operates PEI in 32bit mode and MM foundation in 64bit, the current
>> EFI_MM_COMMUNICATE_HEADER definition will cause structure parse error due
>> to UINTN being used.
>>
>> The suggested change is to make the MessageLength field defined with
>> definitive size as below:
>> ```
>> typedef struct {
>> EFI_GUID  HeaderGuid;
>> UINT64    MessageLength;
>> UINT8     Data[ANYSIZE_ARRAY];
>> } EFI_MM_COMMUNICATE_HEADER;
>> ```
>>
>> Patch v1 branch: 
>> https://github.com/kuqin12/edk2/tree/BZ3398-MmCommunicate-Length
>>
>> Cc: Jian J Wang <jian.j.wang@intel.com>
>> Cc: Hao A Wu <hao.a.wu@intel.com>
>> Cc: Eric Dong <eric.dong@intel.com>
>> Cc: Ray Ni <ray.ni@intel.com>
>> Cc: Michael D Kinney <michael.d.kinney@intel.com>
>> Cc: Liming Gao <gaoliming@byosoft.com.cn>
>> Cc: Zhiguang Liu <zhiguang.liu@intel.com>
>> Cc: Andrew Fish <afish@apple.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Leif Lindholm <leif@nuviainc.com>
>>
>> Kun Qin (5):
>>    EDK2 Code First: PI Specification: EFI_MM_COMMUNICATE_HEADER Update
>>    MdeModulePkg: PiSmmIpl: Update MessageLength calculation for
>>      MmCommunicate
>>    MdeModulePkg: MemoryProfileInfo: Updated MessageLength calculation
>>    MdeModulePkg: SmiHandlerProfileInfo: Updated MessageLength calculation
>>    MdePkg: MmCommunication: Extend MessageLength field size to UINT64
>>
>>   
>> MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.c         
>> | 20 +++--
>>   
>> MdeModulePkg/Application/SmiHandlerProfileInfo/SmiHandlerProfileInfo.c 
>> |  8 +-
>>   
>> MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c                                 
>> | 13 ++-
>>   
>> BZ3430-SpecChange.md                                                   
>> | 88 ++++++++++++++++++++
>>   
>> MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf                               
>> |  1 +
>>   
>> MdePkg/Include/Protocol/MmCommunication.h                              
>> |  3 +-
>>   6 files changed, 124 insertions(+), 9 deletions(-)
>>   create mode 100644 BZ3430-SpecChange.md
>>
>