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


Kun Qin
 

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

Join devel@edk2.groups.io to automatically receive all group messages.