Re: [edk2-devel] [PATCH v3 2/7] MdePkg: MmCommunication: Introduce EFI_MM_COMMUNICATE_HEADER_V3 to MdePkg

["Ni, Ray" <ray.ni@intel.com>]

Can you kindly explain why you choose to define the definitions in PiMultiPhase.h instead of MmCommunication.h?

Thanks,
Ray

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Kun Qin
> Sent: Tuesday, August 17, 2021 1:08 PM
> To: devel@edk2.groups.io
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>; Liu, Zhiguang
> <zhiguang.liu@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Marvin Häuser <mhaeuser@posteo.de>; Bret Barkelew
> <Bret.Barkelew@microsoft.com>; Michael Kubacki <michael.kubacki@microsoft.com>
> Subject: [edk2-devel] [PATCH v3 2/7] MdePkg: MmCommunication: Introduce EFI_MM_COMMUNICATE_HEADER_V3 to
> MdePkg
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3398
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3430
> 
> This change introduces a new definition for MM communicate header
> structure, intending to provide better portability between different
> architectures (IA32 & X64) and adapt to flexible array supported by
> modern compilers.
> 
> The original MessageLength field of EFI_MM_COMMUNICATE_HEADER, as a
> generic definition, was used for both PEI and DXE MM communication. On 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 used. This introduction
> removes the architecture dependent field by defining this field as
> UINT64.
> 
> The new signature could help identifying whether the data received is
> compiliant with this new data structure, which will help for binary
> release modules to identify usage of legacy data structure.
> 
> Version field is also added to indicate the current version of header in
> case there is need for minor modification in the future.
> 
> The data field of MM communicate message is replaced with flexible array
> to allow users not having to consume extra data during communicate and
> author code more intrinsically.
> 
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Zhiguang Liu <zhiguang.liu@intel.com>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Marvin Häuser <mhaeuser@posteo.de>
> Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
> Cc: Michael Kubacki <michael.kubacki@microsoft.com>
> 
> Signed-off-by: Kun Qin <kuqin12@gmail.com>
> ---
> 
> Notes:
>     v3:
>     - Newly introduced communicate v3
>     - Used flexible arrays [Marvin]
>     - Added static assert [Michael]
> 
>  MdePkg/Include/Pi/PiMultiPhase.h | 55 ++++++++++++++++++++
>  MdePkg/MdePkg.dec                |  5 ++
>  2 files changed, 60 insertions(+)
> 
> diff --git a/MdePkg/Include/Pi/PiMultiPhase.h b/MdePkg/Include/Pi/PiMultiPhase.h
> index 89280d9d3506..8c60338091b3 100644
> --- a/MdePkg/Include/Pi/PiMultiPhase.h
> +++ b/MdePkg/Include/Pi/PiMultiPhase.h
> @@ -103,6 +103,17 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>  #define EFI_SMRAM_CLOSED                EFI_MMRAM_CLOSED
>  #define EFI_SMRAM_LOCKED                EFI_MMRAM_LOCKED
> 
> +///
> +/// MM Communicate header constants
> +///
> +#define COMMUNICATE_HEADER_V3_GUID \
> +  { \
> +    0x68e8c853, 0x2ba9, 0x4dd7, { 0x9a, 0xc0, 0x91, 0xe1, 0x61, 0x55, 0xc9, 0x35 } \
> +  }
> +
> +#define EFI_MM_COMMUNICATE_HEADER_V3_SIGNATURE  0x4D434833 // "MCH3"
> +#define EFI_MM_COMMUNICATE_HEADER_V3_VERSION    3
> +
>  ///
>  /// Structure describing a MMRAM region and its accessibility attributes.
>  ///
> @@ -149,6 +160,48 @@ typedef struct _EFI_MM_RESERVED_MMRAM_REGION {
>    UINT64                  MmramReservedSize;
>  } EFI_MM_RESERVED_MMRAM_REGION;
> 
> +#pragma pack(1)
> +
> +///
> +/// To avoid confusion in interpreting frames, the buffer communicating to MM core through
> +/// EFI_MM_COMMUNICATE3 or later should always start with EFI_MM_COMMUNICATE_HEADER_V3.
> +///
> +typedef struct {
> +  ///
> +  /// Indicator GUID for MM core that the communication buffer is compliant with this v3 header.
> +  /// Must be gCommunicateHeaderV3Guid.
> +  ///
> +  EFI_GUID  HeaderGuid;
> +  ///
> +  /// Signature to indicate data is compliant with new MM communicate header structure.
> +  /// Must be "MCH3".
> +  ///
> +  UINT32    Signature;
> +  ///
> +  /// MM communicate data format version, MM foundation entry point should check if incoming
> +  /// data is a supported format before proceeding.
> +  /// Must be 3.
> +  ///
> +  UINT32    Version;
> +  ///
> +  /// Allows for disambiguation of the message format.
> +  ///
> +  EFI_GUID  MessageGuid;
> +  ///
> +  /// Describes the size of MessageData (in bytes) and does not include the size of the header.
> +  ///
> +  UINT64    MessageSize;
> +  ///
> +  /// Designates an array of bytes that is MessageSize in size.
> +  ///
> +  UINT8     MessageData[];
> +} EFI_MM_COMMUNICATE_HEADER_V3;
> +
> +#pragma pack()
> +
> +STATIC_ASSERT ((sizeof (EFI_MM_COMMUNICATE_HEADER_V3) == OFFSET_OF (EFI_MM_COMMUNICATE_HEADER_V3,
> MessageData)), \
> +  "sizeof (EFI_MM_COMMUNICATE_HEADER_V3) does not align with the beginning of flexible array MessageData");
> +
>  typedef enum {
>    EFI_PCD_TYPE_8,
>    EFI_PCD_TYPE_16,
> @@ -208,4 +261,6 @@ EFI_STATUS
>    IN VOID  *ProcedureArgument
>  );
> 
> +extern EFI_GUID   gCommunicateHeaderV3Guid;
> +
>  #endif
> diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec
> index a28a2daaffa8..411079a4343e 100644
> --- a/MdePkg/MdePkg.dec
> +++ b/MdePkg/MdePkg.dec
> @@ -823,6 +823,11 @@ [Guids]
>    #
>    gLinuxEfiInitrdMediaGuid       = {0x5568e427, 0x68fc, 0x4f3d, {0xac, 0x74, 0xca, 0x55, 0x52, 0x31, 0xcc, 0x68}}
> 
> +  #
> +  # GUID indicates the MM communication data is compliant with v3 communication header.
> +  #
> +  gCommunicateHeaderV3Guid       = { 0x68e8c853, 0x2ba9, 0x4dd7, { 0x9a, 0xc0, 0x91, 0xe1, 0x61, 0x55, 0xc9, 0x35 } }
> +
>  [Guids.IA32, Guids.X64]
>    ## Include/Guid/Cper.h
>    gEfiIa32X64ErrorTypeCacheCheckGuid = { 0xA55701F5, 0xE3EF, 0x43de, { 0xAC, 0x72, 0x24, 0x9B, 0x57, 0x3F, 0xAD, 0x2C }}
> --
> 2.32.0.windows.1
> 
> 
> 
> 
>