public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [PATCH v3 0/7] New MM Communicate header and interfaces
@ 2021-08-17  5:08 Kun Qin
  2021-08-17  5:08 ` [PATCH v3 1/7] EDK2 Code First: PI Specification: New communicate " Kun Qin
                   ` (7 more replies)
  0 siblings, 8 replies; 11+ messages in thread
From: Kun Qin @ 2021-08-17  5:08 UTC (permalink / raw)
  To: devel
  Cc: Michael D Kinney, Liming Gao, Zhiguang Liu, Andrew Fish,
	Leif Lindholm, Hao A Wu, Marvin Häuser, Bret Barkelew,
	Michael Kubacki, Ard Biesheuvel, Sami Mujawar, Jiewen Yao,
	Supreeth Venkatesh, Jian J Wang, Eric Dong, Ray Ni

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3430

This patch series is a follow up of previous submission:
https://edk2.groups.io/g/devel/message/77017

Since the submission of previous patch series, there were suggestions
made to change the entire existing MM communicate header in order to
leverage compilers to catch all potential code that referenced the
original header.

However, although such header structure and/or name change would break
compilers, there are cases where the OFFSET_OF Data fields is calculated
without involving the header structure at all.

Thus patch series v3 introduced MM communicate interface v3 (both
protocol and PPI) to consume the corresponding new header structure. The
new structure fixed ambiguious data field size caused by UINTN, as well
as integrated flexible arrays for data fields, while maintaining the
backwards compatibility for all existing codebases. A specified GUID is
used to differentiate old MM headers from newly defined v3 header.

The specification change is also included in this patch series v3, where
the standalone MM IPL in PEI phase is specified to install new PPI v3
after setting MM foundation.

v3 patch changes include feedback for v1 series:
a. Introduced v3 MM comminucate protocol and PPI;
b. Applied flexible arrays to new communicate header structures;

Patch v3 branch: https://github.com/kuqin12/edk2/tree/BZ3398-MmCommunicate-Length-v3

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: Leif Lindholm <leif@nuviainc.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>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Sami Mujawar <sami.mujawar@arm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Supreeth Venkatesh <supreeth.venkatesh@arm.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>

Kun Qin (7):
  EDK2 Code First: PI Specification: New communicate header and
    interfaces
  MdePkg: MmCommunication: Introduce EFI_MM_COMMUNICATE_HEADER_V3 to
    MdePkg
  MdePkg: MmCommunication: Introduce EFI_MM_COMMUNICATION3_PROTOCOL to
    MdePkg
  MdePkg: MmCommunication: Introduce EFI_PEI_MM_COMMUNICATION3_PPI to
    MdePkg
  MdeModulePkg: PiSmmCore: Added parser of new MM communicate header
  StandaloneMmPkg: StandaloneMmCore: Parsing new MM communicate header
  MdeModulePkg: PiSmmIpl: Update MessageLength calculation for
    MmCommunicate

 MdeModulePkg/Core/PiSmmCore/PiSmmCore.c    |  42 ++-
 MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c     | 187 +++++++++++++
 StandaloneMmPkg/Core/StandaloneMmCore.c    |  34 ++-
 CodeFirst/BZ3430-SpecChange.md             | 277 ++++++++++++++++++++
 MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf  |   1 +
 MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf   |   2 +
 MdePkg/Include/Pi/PiMultiPhase.h           |  55 ++++
 MdePkg/Include/Ppi/MmCommunication3.h      |  58 ++++
 MdePkg/Include/Protocol/MmCommunication3.h |  70 +++++
 MdePkg/MdePkg.dec                          |  11 +
 StandaloneMmPkg/Core/StandaloneMmCore.inf  |   1 +
 11 files changed, 720 insertions(+), 18 deletions(-)
 create mode 100644 CodeFirst/BZ3430-SpecChange.md
 create mode 100644 MdePkg/Include/Ppi/MmCommunication3.h
 create mode 100644 MdePkg/Include/Protocol/MmCommunication3.h

-- 
2.32.0.windows.1


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v3 1/7] EDK2 Code First: PI Specification: New communicate header and interfaces
  2021-08-17  5:08 [PATCH v3 0/7] New MM Communicate header and interfaces Kun Qin
@ 2021-08-17  5:08 ` Kun Qin
  2021-08-17  5:08 ` [PATCH v3 2/7] MdePkg: MmCommunication: Introduce EFI_MM_COMMUNICATE_HEADER_V3 to MdePkg Kun Qin
                   ` (6 subsequent siblings)
  7 siblings, 0 replies; 11+ messages in thread
From: Kun Qin @ 2021-08-17  5:08 UTC (permalink / raw)
  To: devel
  Cc: Michael D Kinney, Liming Gao, Zhiguang Liu, Andrew Fish,
	Leif Lindholm, Hao A Wu, Marvin Häuser, Bret Barkelew

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3430

This change includes specification update markdown file that describes
the proposed PI Specification v1.7 Errata A in detail and potential
impact to the existing codebase.

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: Leif Lindholm <leif@nuviainc.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Marvin Häuser <mhaeuser@posteo.de>
Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>

Signed-off-by: Kun Qin <kuqin12@gmail.com>
---

Notes:
    v3:
    - switched to use communicate v3 instead of sprinkle structure updates

 CodeFirst/BZ3430-SpecChange.md | 277 ++++++++++++++++++++
 1 file changed, 277 insertions(+)

diff --git a/CodeFirst/BZ3430-SpecChange.md b/CodeFirst/BZ3430-SpecChange.md
new file mode 100644
index 000000000000..39a96b9a44f0
--- /dev/null
+++ b/CodeFirst/BZ3430-SpecChange.md
@@ -0,0 +1,277 @@
+# Title: Introduction of `EFI_MM_COMMUNICATE_HEADER_V3` and `MM_COMMUNICATE3_*` interface
+
+## Status: Draft
+
+## Document: UEFI Platform Initialization Specification Version 1.7 Errata A
+
+## License
+
+SPDX-License-Identifier: CC-BY-4.0
+
+## Submitter: [TianoCore Community](https://www.tianocore.org)
+
+## Summary of the change
+
+Introduce `EFI_PEI_MM_COMMUNICATION3_PPI` and `EFI_MM_COMMUNICATE3_PROTOCOL` that works with communication buffer starts with `EFI_MM_COMMUNICATE_HEADER_V3` to provide better portability between different architectures (IA32 & X64) and adapt to flexible array supported by modern compilers.
+
+## Benefits of the change
+
+In PI Spec v1.7 Errata A, Vol.4, Sec 5.7 MM Communication Protocol, the MessageLength field of `EFI_MM_COMMUNICATE_HEADER` (also defined as `EFI_SMM_COMMUNICATE_HEADER`) is 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 Standalone 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 proposed data structure resolved it by introducing `EFI_MM_COMMUNICATE_HEADER_V3` that defines the MessageSize as UINT64 to remove data size ambiguity.
+
+The data structure still starts with a `HeaderGuid` field that is typed as `EFI_GUID`. This will be populated as `gCommunicateHeaderV3Guid` or `COMMUNICATE_HEADER_V3_GUID` as an indicator to differentiate new data format vesus legacy format.
+
+Furthermore, the addition of 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 `EFI_MM_COMMUNICATE_HEADER`.
+
+Version field is also added to indicate the current version of header in case there is need for minor modification in the future.
+
+In addition, 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.
+
+On the non-MM environment side, the Standalone MM DXE IPL agent can add installation of `EFI_MM_COMMUNICATE3_PROTOCOL`, while the Standalone MM PEI IPL agent that launches MM foundation should publish and only publish `EFI_PEI_MM_COMMUNICATION3_PPI` for MM communication during PEI phase.
+
+For communication data that starts with `EFI_MM_COMMUNICATE_HEADER_V3`, callers always need to use V3 protocol/PPI to communicate with updated MM cores. These interface introductions instead of replacement can maintain the compatibility for existing codebase while resolving size mismatching occurred during data transfer between different architectures.
+
+## Impact of the change
+
+This change will impact the MM cores and IPLs:
+
+```bash
+MdeModulePkg/Core/PiSmmCore/PiSmmCore
+StandaloneMmPkg/Core/StandaloneMmCore
+MdeModulePkg/Core/PiSmmCore/PiSmmIpl
+```
+
+To cooporate with the newly proposed data format, existing MM cores need to be updated to parse incoming data properly to tell if the data is compliant with new or legacy format.
+
+The existing PiSmmIpl will need to be updated to publish `EFI_MM_COMMUNICATE3_PROTOCOL` for consumers that would like to invoke MMI with new data format.
+
+For potential proprietary IPLs that launches Standalone MM in PEI phase, if any, the PEIM should change to publish `EFI_PEI_MM_COMMUNICATION3_PPI` instead of `EFI_MM_COMMUNICATE_PPI`.
+
+Accordingly, all consumers in PEI phase that communicate to PEI launched Standalone MM should switch to use `EFI_PEI_MM_COMMUNICATION3_PPI` and `EFI_MM_COMMUNICATE_HEADER_V3`.
+
+## Detailed description of the change [normative updates]
+
+### Specification Changes
+
+1. In PI Specification v1.7 Errata A: Vol. 4-1.5.1 Initializing MM Standalone Mode in PEI phase, the last bullet point of step 3 should be changed to:
+
+    ```c
+    Publishes the EFI_PEI_MM_COMMUNICATION3_PPI
+    ```
+
+1. In PI Specification v1.7 Errata A: Vol. 4, section 6.5 MM Communication PPI, add the following:
+
+    ```markdown
+    # EFI_PEI_MM_COMMUNICATION_PPI
+
+    ## Summary
+
+    This PPI provides a means of communicating between drivers outside of MM and MMI handlers inside of MM in PEI phase.
+
+    ## GUID
+
+      #define EFI_PEI_MM_COMMUNICATION3_PPI_GUID \
+      { \
+        0xe70febf6, 0x13ef, 0x4a21, { 0x89, 0x9e, 0xb2, 0x36, 0xf8, 0x25, 0xff, 0xc9 } \
+      }
+
+    ## PPI Structure
+
+      typedef struct _EFI_PEI_MM_COMMUNICATION3_PPI {
+        EFI_PEI_MM_COMMUNICATE3  Communicate;
+      } EFI_PEI_MM_COMMUNICATION3_PPI;
+
+    ## Members
+
+    ### Communicate
+
+      Sends/receives a message for a registered handler. See the Communicate() function description.
+
+    ## Description
+
+      This PPI provides services for communicating between PEIM and a registered MMI handler.
+
+    # EFI_PEI_MM_COMMUNICATION_PPI.Communicate()
+
+    ## Summary
+      Communicates with a registered handler.
+
+    ## Prototype
+      typedef
+      EFI_STATUS
+      (EFIAPI *EFI_PEI_MM_COMMUNICATE3)(
+        IN CONST EFI_PEI_MM_COMMUNICATION3_PPI   *This,
+        IN OUT VOID                              *CommBuffer,
+        IN OUT UINTN                             *CommSize
+        );
+
+    ## Parameters
+
+    ### This
+      The EFI_PEI_MM_COMMUNICATE3 instance.
+
+    ### CommBuffer
+
+      Pointer to the buffer to convey into MMRAM.
+
+    ### CommSize
+
+      The size of the data buffer being passed in. On exit, the size of data being returned. Zero if the handler does not wish to reply with any data.
+
+    ## Description
+
+      This function provides a service to send and receive messages from a registered PEI service. The EFI_PEI_MM_COMMUNICATION3_PPI driver is responsible for doing any of the copies such that the data lives in PEI-service-accessible RAM.
+
+      A given implementation of the EFI_PEI_MM_COMMUNICATION3_PPI may choose to use the EFI_MM_CONTROL_PPI for effecting the mode transition, or it may use some other method.
+
+      The agent invoking the communication interface must be physical/virtually 1:1 mapped.
+
+      To avoid confusion in interpreting frames, the CommBuffer parameter should always begin with EFI_MM_COMMUNICATE_HEADER_V3. The header data is mandatory for messages sent into the MM agent.
+
+      Once inside of MM, the MM infrastructure will call all registered handlers with the same HandlerType as the GUID specified by HeaderGuid and the CommBuffer pointing to Data.
+
+      This function is not reentrant.
+
+    ## Status Codes Returned
+      EFI_SUCCESS            The message was successfully posted.
+      EFI_INVALID_PARAMETER  The CommBuffer was NULL.
+    ```
+
+1. In PI Specification v1.7 Errata A: Vol. 4, section 6.5 MM Communication PPI, add the following:
+
+    ```markdown
+    # EFI_MM_COMMUNICATION3_PROTOCOL
+
+    ## Summary
+
+      This protocol provides a means of communicating between drivers outside of MM and MMI handlers inside of MM, for communication buffer that is compliant with EFI_MM_COMMUNICATE_HEADER_V3.
+
+    ## GUID
+
+      #define EFI_MM_COMMUNICATION3_PROTOCOL_GUID \
+      { \
+        0xf7234a14, 0xdf2, 0x46c0, { 0xad, 0x28, 0x90, 0xe6, 0xb8, 0x83, 0xa7, 0x2f } \
+      }
+
+    ## Prototype
+      typedef struct _EFI_MM_COMMUNICATION3_PROTOCOL {
+        EFI_MM_COMMUNICATE3  Communicate;
+      } EFI_MM_COMMUNICATION3_PROTOCOL;
+
+    ## Members
+
+    ### Communicate
+
+      Sends/receives a message for a registered handler. See the Communicate() function description.
+
+    ## Description
+
+      This protocol provides runtime services for communicating between DXE drivers and a registered MMI handler.
+
+    # EFI_MM_COMMUNICATION3_PROTOCOL.Communicate()
+
+    ## Summary
+
+      Communicates with a registered handler.
+
+    ## Prototype
+
+      typedef
+      EFI_STATUS
+      (EFIAPI *EFI_MM_COMMUNICATE3)(
+        IN CONST EFI_MM_COMMUNICATION3_PROTOCOL   *This,
+        IN OUT VOID                               *CommBufferPhysical,
+        IN OUT VOID                               *CommBufferVirtual,
+        IN OUT UINTN                              *CommSize OPTIONAL
+        );
+
+    ## Parameters
+
+    ### This
+
+      The EFI_MM_COMMUNICATION3_PROTOCOL instance.
+
+    ### CommBufferPhysical
+
+      Physical address of the buffer to convey into MMRAM, of which content must start with EFI_MM_COMMUNICATE_HEADER_V3.
+
+    ### CommBufferVirtual
+
+      Virtual address of the buffer to convey into MMRAM, of which content must start with EFI_MM_COMMUNICATE_HEADER_V3.
+
+    ### CommSize
+
+      The size of the data buffer being passed in. On exit, the size of data being returned. Zero if the handler does not wish to reply with any data. This parameter is optional and may be NULL.
+
+    ## Description
+
+      Usage is similar to EFI_MM_COMMUNICATION_PROTOCOL.Communicate() except for the notes below:
+
+      * Communication buffer transfer to MM core should start with EFI_MM_COMMUNICATE_HEADER_V3.
+      * Instead of passing just the physical address via the CommBuffer parameter, the caller must pass both the physical and the virtual addresses of the communication buffer.
+      * If no virtual remapping has taken place, the physical address will be equal to the virtual address, and so the caller is required to pass the same value for both parameters.
+
+    ## Related Definitions
+      typedef struct {
+        EFI_GUID  HeaderGuid;
+        UINT32    Signature;
+        UINT32    Version;
+        EFI_GUID  MessageGuid;
+        UINT64    MessageSize;
+        UINT8     MessageData[ANYSIZE_ARRAY];
+      } EFI_MM_COMMUNICATE_HEADER_V3;
+
+      #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
+
+    ### HeaderGuid
+
+      Indicator GUID for MM core that the communication buffer is compliant with this v3 header. Must be COMMUNICATE_HEADER_V3_GUID.
+
+    ### Signature
+
+      Signature to indicate data is compliant with new MM communicate header structure. Must be "MCH3".
+
+    ### Version
+
+      MM communicate data format version, MM foundation entry point should check if incoming data is a supported format before proceeding. Must be 3.
+
+    ### MessageGuid
+
+      Allows for disambiguation of the message format.
+
+    ### MessageSize
+
+      Describes the size of MessageData (in bytes) and does not include the size of the header.
+
+    ### MessageData
+
+      Designates an array of bytes that is MessageSize in size.
+
+    ## Status Codes Returned
+
+      EFI_SUCCESS                 The message was successfully posted.
+      EFI_INVALID_PARAMETER       CommBufferPhysical was NULL or CommBufferVirtual was NULL.
+      EFI_BAD_BUFFER_SIZE         The buffer is too large for the MM implementation. If this error is returned, the MessageLength field in the CommBuffer header or the integer pointed by CommSize, are updated to reflect the maximum payload size the implementation can accommodate.
+      EFI_ACCESS_DENIED           The CommunicateBuffer parameter or CommSize parameter, if not omitted, are in address range that cannot be accessed by the MM environment.
+    ```
+
+### Code Changes
+
+1. Add data structure and its related definitions in `MdePkg/Include/Pi/PiMultiPhase.h` to match new definition.
+
+1. Add interface definition of `MdePkg/Include/Protocol/MmCommunication3.h` and `MdePkg/Include/Protocol/MmCommunication3.h`, respectively, to match newly proposed interfaces.
+
+1. Extend PiSmmCore to inspect `HeaderGuid` of incoming communication data. If it matches `COMMUNICATE_HEADER_V3_GUID`, parse the incoming data to start with `EFI_MM_COMMUNICATE_HEADER_V3`, otherwise it will be parse as existing way.
+
+1. Extend StandaloneMmCore to inspect `HeaderGuid` of incoming communication data. If it matches `COMMUNICATE_HEADER_V3_GUID`, parse the incoming data to start with `EFI_MM_COMMUNICATE_HEADER_V3`, otherwise it will be parse as existing way.
+
+1. Extend PiSmmIpl to publish `EFI_MM_COMMUNICATION3_PROTOCOL`, the implementation of `EFI_MM_COMMUNICATION3_PROTOCOL.Communicate()` should parse the incoming data as it starts with `EFI_MM_COMMUNICATE_HEADER_V3`, when applicable.
-- 
2.32.0.windows.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v3 2/7] MdePkg: MmCommunication: Introduce EFI_MM_COMMUNICATE_HEADER_V3 to MdePkg
  2021-08-17  5:08 [PATCH v3 0/7] New MM Communicate header and interfaces Kun Qin
  2021-08-17  5:08 ` [PATCH v3 1/7] EDK2 Code First: PI Specification: New communicate " Kun Qin
@ 2021-08-17  5:08 ` Kun Qin
  2021-08-17  5:50   ` [edk2-devel] " Ni, Ray
  2021-08-17  5:08 ` [PATCH v3 3/7] MdePkg: MmCommunication: Introduce EFI_MM_COMMUNICATION3_PROTOCOL " Kun Qin
                   ` (5 subsequent siblings)
  7 siblings, 1 reply; 11+ messages in thread
From: Kun Qin @ 2021-08-17  5:08 UTC (permalink / raw)
  To: devel
  Cc: Michael D Kinney, Liming Gao, Zhiguang Liu, Hao A Wu,
	Marvin Häuser, Bret Barkelew, Michael Kubacki

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


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v3 3/7] MdePkg: MmCommunication: Introduce EFI_MM_COMMUNICATION3_PROTOCOL to MdePkg
  2021-08-17  5:08 [PATCH v3 0/7] New MM Communicate header and interfaces Kun Qin
  2021-08-17  5:08 ` [PATCH v3 1/7] EDK2 Code First: PI Specification: New communicate " Kun Qin
  2021-08-17  5:08 ` [PATCH v3 2/7] MdePkg: MmCommunication: Introduce EFI_MM_COMMUNICATE_HEADER_V3 to MdePkg Kun Qin
@ 2021-08-17  5:08 ` Kun Qin
  2021-08-17  5:08 ` [PATCH v3 4/7] MdePkg: MmCommunication: Introduce EFI_PEI_MM_COMMUNICATION3_PPI " Kun Qin
                   ` (4 subsequent siblings)
  7 siblings, 0 replies; 11+ messages in thread
From: Kun Qin @ 2021-08-17  5:08 UTC (permalink / raw)
  To: devel
  Cc: Michael D Kinney, Liming Gao, Zhiguang Liu, Hao A Wu,
	Marvin Häuser, Bret Barkelew, Michael Kubacki

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 protocol v3.

This protocol will be installed under a new GUID in contrast to exisiting
EFI_MM_COMMUNICATION_PROTOCOL.

Data communicated to MM through EFI_MM_COMMUNICATION3_PROTOCOL should
always start with EFI_MM_COMMUNICATE_HEADER_V3 with its HeaderGuid,
Signature and Version fields properly populated.

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 v3 of communicate protocol

 MdePkg/Include/Protocol/MmCommunication3.h | 70 ++++++++++++++++++++
 MdePkg/MdePkg.dec                          |  3 +
 2 files changed, 73 insertions(+)

diff --git a/MdePkg/Include/Protocol/MmCommunication3.h b/MdePkg/Include/Protocol/MmCommunication3.h
new file mode 100644
index 000000000000..1dfebbdddb5e
--- /dev/null
+++ b/MdePkg/Include/Protocol/MmCommunication3.h
@@ -0,0 +1,70 @@
+/** @file
+  EFI MM Communication Protocol 2 as defined in the PI 1.7 errata A specification.
+
+  This protocol provides a means of communicating between drivers outside of MM and MMI
+  handlers inside of MM.
+
+  Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
+  Copyright (c) 2019, Arm Limited. All rights reserved.<BR>
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#ifndef MM_COMMUNICATION3_H_
+#define MM_COMMUNICATION3_H_
+
+#include <Pi/PiMultiPhase.h>
+
+#define EFI_MM_COMMUNICATION3_PROTOCOL_GUID \
+  { \
+    0xf7234a14, 0xdf2, 0x46c0, { 0xad, 0x28, 0x90, 0xe6, 0xb8, 0x83, 0xa7, 0x2f } \
+  }
+
+typedef struct _EFI_MM_COMMUNICATION3_PROTOCOL  EFI_MM_COMMUNICATION3_PROTOCOL;
+
+/**
+  Communicates with a registered handler.
+
+  This function provides a service to send and receive messages from a registered UEFI service.
+
+  @param[in] This                     The EFI_MM_COMMUNICATION3_PROTOCOL instance.
+  @param[in, out] CommBufferPhysical  Physical address of the MM communication buffer, of which content must
+                                      start with EFI_MM_COMMUNICATE_HEADER_V3.
+  @param[in, out] CommBufferVirtual   Virtual address of the MM communication buffer, of which content must
+                                      start with EFI_MM_COMMUNICATE_HEADER_V3.
+  @param[in, out] CommSize            The size of the data buffer being passed in. On exit, the size of data
+                                      being returned. Zero if the handler does not wish to reply with any data.
+                                      This parameter is optional and may be NULL.
+
+  @retval EFI_SUCCESS                 The message was successfully posted.
+  @retval EFI_INVALID_PARAMETER       CommBufferPhysical was NULL or CommBufferVirtual was NULL.
+  @retval EFI_BAD_BUFFER_SIZE         The buffer is too large for the MM implementation.
+                                      If this error is returned, the MessageLength field
+                                      in the CommBuffer header or the integer pointed by
+                                      CommSize, are updated to reflect the maximum payload
+                                      size the implementation can accommodate.
+  @retval EFI_ACCESS_DENIED           The CommunicateBuffer parameter or CommSize parameter,
+                                      if not omitted, are in address range that cannot be
+                                      accessed by the MM environment.
+
+**/
+typedef
+EFI_STATUS
+(EFIAPI *EFI_MM_COMMUNICATE3)(
+  IN CONST EFI_MM_COMMUNICATION3_PROTOCOL   *This,
+  IN OUT VOID                               *CommBufferPhysical,
+  IN OUT VOID                               *CommBufferVirtual,
+  IN OUT UINTN                              *CommSize OPTIONAL
+  );
+
+///
+/// EFI MM Communication Protocol provides runtime services for communicating
+/// between DXE drivers and a registered MMI handler.
+///
+struct _EFI_MM_COMMUNICATION3_PROTOCOL {
+  EFI_MM_COMMUNICATE3  Communicate;
+};
+
+extern EFI_GUID gEfiMmCommunication3ProtocolGuid;
+
+#endif
diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec
index 411079a4343e..3168534951c1 100644
--- a/MdePkg/MdePkg.dec
+++ b/MdePkg/MdePkg.dec
@@ -1348,6 +1348,9 @@ [Protocols]
   ## Include/Protocol/MmCommunication2.h
   gEfiMmCommunication2ProtocolGuid  = { 0x378daedc, 0xf06b, 0x4446, { 0x83, 0x14, 0x40, 0xab, 0x93, 0x3c, 0x87, 0xa3 }}
 
+  ## Include/Protocol/MmCommunication3.h
+  gEfiMmCommunication3ProtocolGuid  = { 0xf7234a14, 0xdf2, 0x46c0, { 0xad, 0x28, 0x90, 0xe6, 0xb8, 0x83, 0xa7, 0x2f }}
+
   #
   # Protocols defined in UEFI2.1/UEFI2.0/EFI1.1
   #
-- 
2.32.0.windows.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v3 4/7] MdePkg: MmCommunication: Introduce EFI_PEI_MM_COMMUNICATION3_PPI to MdePkg
  2021-08-17  5:08 [PATCH v3 0/7] New MM Communicate header and interfaces Kun Qin
                   ` (2 preceding siblings ...)
  2021-08-17  5:08 ` [PATCH v3 3/7] MdePkg: MmCommunication: Introduce EFI_MM_COMMUNICATION3_PROTOCOL " Kun Qin
@ 2021-08-17  5:08 ` Kun Qin
  2021-08-17  5:08 ` [PATCH v3 5/7] MdeModulePkg: PiSmmCore: Added parser of new MM communicate header Kun Qin
                   ` (3 subsequent siblings)
  7 siblings, 0 replies; 11+ messages in thread
From: Kun Qin @ 2021-08-17  5:08 UTC (permalink / raw)
  To: devel
  Cc: Michael D Kinney, Liming Gao, Zhiguang Liu, Hao A Wu,
	Marvin Häuser, Bret Barkelew, Michael Kubacki

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 PPI v3.

This PPI will be installed under a new GUID in contrast to exisiting
EFI_PEI_MM_COMMUNICATION_PPI.

Data communicated to MM through EFI_PEI_MM_COMMUNICATION3_PPI should
always start with EFI_MM_COMMUNICATE_HEADER_V3 with its HeaderGuid,
Signature and Version fields properly populated.

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 v3 communicate PPI

 MdePkg/Include/Ppi/MmCommunication3.h | 58 ++++++++++++++++++++
 MdePkg/MdePkg.dec                     |  3 +
 2 files changed, 61 insertions(+)

diff --git a/MdePkg/Include/Ppi/MmCommunication3.h b/MdePkg/Include/Ppi/MmCommunication3.h
new file mode 100644
index 000000000000..11924099b4c3
--- /dev/null
+++ b/MdePkg/Include/Ppi/MmCommunication3.h
@@ -0,0 +1,58 @@
+/** @file
+  EFI MM Communication v3 PPI definition.
+
+  This Ppi provides a means of communicating between PEIM and MMI
+  handlers inside of MM.
+
+Copyright (c) 2010 - 2018, Intel Corporation. All rights reserved.<BR>
+Copyright (c) Microsoft Corporation.
+
+SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+
+#ifndef MM_COMMUNICATION3_PPI_H_
+#define MM_COMMUNICATION3_PPI_H_
+
+#include <Pi/PiMultiPhase.h>
+
+#define EFI_PEI_MM_COMMUNICATION3_PPI_GUID \
+  { \
+    0xe70febf6, 0x13ef, 0x4a21, { 0x89, 0x9e, 0xb2, 0x36, 0xf8, 0x25, 0xff, 0xc9 } \
+  }
+
+typedef struct _EFI_PEI_MM_COMMUNICATION3_PPI  EFI_PEI_MM_COMMUNICATION3_PPI;
+
+/**
+  Communicates with a registered handler.
+
+  This function provides a service to send and receive messages from a registered UEFI service.
+
+  @param[in] This                The EFI_PEI_MM_COMMUNICATE3 instance.
+  @param[in, out] CommBuffer     A pointer to the buffer to convey into MMRAM.
+  @param[in, out] CommSize       The size of the data buffer being passed in.On exit, the size of data
+                                 being returned. Zero if the handler does not wish to reply with any data.
+
+  @retval EFI_SUCCESS            The message was successfully posted.
+  @retval EFI_INVALID_PARAMETER  The CommBuffer was NULL.
+**/
+typedef
+EFI_STATUS
+(EFIAPI *EFI_PEI_MM_COMMUNICATE3)(
+  IN CONST EFI_PEI_MM_COMMUNICATION3_PPI   *This,
+  IN OUT VOID                              *CommBuffer,
+  IN OUT UINTN                             *CommSize
+  );
+
+///
+/// EFI MM Communication PPI provides runtime services for communicating
+/// between DXE drivers and a registered SMI handler.
+///
+struct _EFI_PEI_MM_COMMUNICATION3_PPI {
+  EFI_PEI_MM_COMMUNICATE3  Communicate;
+};
+
+extern EFI_GUID gEfiPeiMmCommunication3PpiGuid;
+
+#endif
diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec
index 3168534951c1..c194d6225501 100644
--- a/MdePkg/MdePkg.dec
+++ b/MdePkg/MdePkg.dec
@@ -1006,6 +1006,9 @@ [Ppis]
   ## Include/Ppi/DelayedDispatch.h
   gEfiPeiDelayedDispatchPpiGuid  = { 0x869c711d, 0x649c, 0x44fe, { 0x8b, 0x9e, 0x2c, 0xbb, 0x29, 0x11, 0xc3, 0xe6 }}
 
+  ## Include/Ppi/MmCommunication3.h
+  gEfiPeiMmCommunication3PpiGuid = { 0xe70febf6, 0x13ef, 0x4a21, { 0x89, 0x9e, 0xb2, 0x36, 0xf8, 0x25, 0xff, 0xc9 }}
+
 [Protocols]
   ## Include/Protocol/Pcd.h
   gPcdProtocolGuid               = { 0x11B34006, 0xD85B, 0x4D0A, { 0xA2, 0x90, 0xD5, 0xA5, 0x71, 0x31, 0x0E, 0xF7 }}
-- 
2.32.0.windows.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v3 5/7] MdeModulePkg: PiSmmCore: Added parser of new MM communicate header
  2021-08-17  5:08 [PATCH v3 0/7] New MM Communicate header and interfaces Kun Qin
                   ` (3 preceding siblings ...)
  2021-08-17  5:08 ` [PATCH v3 4/7] MdePkg: MmCommunication: Introduce EFI_PEI_MM_COMMUNICATION3_PPI " Kun Qin
@ 2021-08-17  5:08 ` Kun Qin
  2021-08-17  5:08 ` [PATCH v3 6/7] StandaloneMmPkg: StandaloneMmCore: Parsing " Kun Qin
                   ` (2 subsequent siblings)
  7 siblings, 0 replies; 11+ messages in thread
From: Kun Qin @ 2021-08-17  5:08 UTC (permalink / raw)
  To: devel; +Cc: Jian J Wang, Hao A Wu, Eric Dong, Ray Ni

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3398
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3430

MM communicate protocols are expanded with EFI_MM_COMMUNICATE_HEADER_V3
structure that cooperates with updated field types and flexible array.
The PiSmmCore implementation is updated to detect and process incoming
data accordingly.

Two checks are also performed to prevent legacy communicate data or
unsupported data is fed into MM core under agreed header guid.

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>

Signed-off-by: Kun Qin <kuqin12@gmail.com>
---

Notes:
    v3:
    - Newly added

 MdeModulePkg/Core/PiSmmCore/PiSmmCore.c   | 42 +++++++++++++++-----
 MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf |  1 +
 2 files changed, 32 insertions(+), 11 deletions(-)

diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
index cfa9922cbdb5..63ac2b5fcbbd 100644
--- a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
+++ b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c
@@ -646,12 +646,16 @@ SmmEntryPoint (
   IN CONST EFI_SMM_ENTRY_CONTEXT  *SmmEntryContext
 )
 {
-  EFI_STATUS                  Status;
-  EFI_SMM_COMMUNICATE_HEADER  *CommunicateHeader;
-  BOOLEAN                     InLegacyBoot;
-  BOOLEAN                     IsOverlapped;
-  VOID                        *CommunicationBuffer;
-  UINTN                       BufferSize;
+  EFI_STATUS                    Status;
+  EFI_MM_COMMUNICATE_HEADER_V3  *CommunicateHeader;
+  EFI_SMM_COMMUNICATE_HEADER    *LegacyCommunicateHeader;
+  BOOLEAN                       InLegacyBoot;
+  BOOLEAN                       IsOverlapped;
+  VOID                          *CommunicationBuffer;
+  UINTN                         BufferSize;
+  EFI_GUID                      *CommGuid;
+  VOID                          *CommData;
+  UINTN                         CommHeaderSize;
 
   //
   // Update SMST with contents of the SmmEntryContext structure
@@ -707,19 +711,35 @@ SmmEntryPoint (
         gSmmCorePrivate->CommunicationBuffer = NULL;
         gSmmCorePrivate->ReturnStatus = EFI_ACCESS_DENIED;
       } else {
-        CommunicateHeader = (EFI_SMM_COMMUNICATE_HEADER *)CommunicationBuffer;
-        BufferSize -= OFFSET_OF (EFI_SMM_COMMUNICATE_HEADER, Data);
+        CommGuid = &((EFI_MM_COMMUNICATE_HEADER_V3 *)CommunicationBuffer)->HeaderGuid;
+        //
+        // Check if the signature matches EFI_MM_COMMUNICATE_HEADER_V3 definition
+        //
+        if (CompareGuid (CommGuid, &gCommunicateHeaderV3Guid)) {
+          CommunicateHeader = (EFI_MM_COMMUNICATE_HEADER_V3 *)CommunicationBuffer;
+          ASSERT (CommunicateHeader->Signature == EFI_MM_COMMUNICATE_HEADER_V3_SIGNATURE);
+          ASSERT (CommunicateHeader->Version <= EFI_MM_COMMUNICATE_HEADER_V3_VERSION);
+          CommGuid = &CommunicateHeader->MessageGuid;
+          CommData = CommunicateHeader->MessageData;
+          CommHeaderSize = sizeof (EFI_MM_COMMUNICATE_HEADER_V3);
+        } else {
+          LegacyCommunicateHeader = (EFI_SMM_COMMUNICATE_HEADER *)CommunicationBuffer;
+          CommGuid = &LegacyCommunicateHeader->HeaderGuid;
+          CommData = LegacyCommunicateHeader->Data;
+          CommHeaderSize = OFFSET_OF (EFI_SMM_COMMUNICATE_HEADER, Data);
+        }
+        BufferSize -= CommHeaderSize;
         Status = SmiManage (
-                   &CommunicateHeader->HeaderGuid,
+                   CommGuid,
                    NULL,
-                   CommunicateHeader->Data,
+                   CommData,
                    &BufferSize
                    );
         //
         // Update CommunicationBuffer, BufferSize and ReturnStatus
         // Communicate service finished, reset the pointer to CommBuffer to NULL
         //
-        gSmmCorePrivate->BufferSize = BufferSize + OFFSET_OF (EFI_SMM_COMMUNICATE_HEADER, Data);
+        gSmmCorePrivate->BufferSize = BufferSize + CommHeaderSize;
         gSmmCorePrivate->CommunicationBuffer = NULL;
         gSmmCorePrivate->ReturnStatus = (Status == EFI_SUCCESS) ? EFI_SUCCESS : EFI_NOT_FOUND;
       }
diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf
index c8bfae3860fc..5a0929a45e19 100644
--- a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf
+++ b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.inf
@@ -118,6 +118,7 @@ [Guids]
   gSmiHandlerProfileGuid
   gEdkiiEndOfS3ResumeGuid ## SOMETIMES_PRODUCES ## GUID # Install protocol
   gEdkiiS3SmmInitDoneGuid ## SOMETIMES_PRODUCES ## GUID # Install protocol
+  gCommunicateHeaderV3Guid    ## CONSUMES   ## GUID # Communicate header
 
 [UserExtensions.TianoCore."ExtraFiles"]
   PiSmmCoreExtra.uni
-- 
2.32.0.windows.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v3 6/7] StandaloneMmPkg: StandaloneMmCore: Parsing new MM communicate header
  2021-08-17  5:08 [PATCH v3 0/7] New MM Communicate header and interfaces Kun Qin
                   ` (4 preceding siblings ...)
  2021-08-17  5:08 ` [PATCH v3 5/7] MdeModulePkg: PiSmmCore: Added parser of new MM communicate header Kun Qin
@ 2021-08-17  5:08 ` Kun Qin
  2021-08-17  5:08 ` [PATCH v3 7/7] MdeModulePkg: PiSmmIpl: Update MessageLength calculation for MmCommunicate Kun Qin
  2021-09-16  2:15 ` [edk2-devel] [PATCH v3 0/7] New MM Communicate header and interfaces Kun Qin
  7 siblings, 0 replies; 11+ messages in thread
From: Kun Qin @ 2021-08-17  5:08 UTC (permalink / raw)
  To: devel; +Cc: Ard Biesheuvel, Sami Mujawar, Jiewen Yao, Supreeth Venkatesh

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3398
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3430

MM communicate protocols are expanded with EFI_MM_COMMUNICATE_HEADER_V3
structure that cooperates with updated field types and flexible array.
The PiSmmCore implementation is updated to detect and process incoming
data accordingly.

Two checks are also performed to prevent legacy communicate data or
unsupported data is fed into MM core under agreed header guid.

Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Sami Mujawar <sami.mujawar@arm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Supreeth Venkatesh <supreeth.venkatesh@arm.com>

Signed-off-by: Kun Qin <kuqin12@gmail.com>
---

Notes:
    v3:
    - Newly added

 StandaloneMmPkg/Core/StandaloneMmCore.c   | 34 ++++++++++++++++----
 StandaloneMmPkg/Core/StandaloneMmCore.inf |  1 +
 2 files changed, 28 insertions(+), 7 deletions(-)

diff --git a/StandaloneMmPkg/Core/StandaloneMmCore.c b/StandaloneMmPkg/Core/StandaloneMmCore.c
index fbb0ec75e557..000aca098cc8 100644
--- a/StandaloneMmPkg/Core/StandaloneMmCore.c
+++ b/StandaloneMmPkg/Core/StandaloneMmCore.c
@@ -340,8 +340,12 @@ MmEntryPoint (
   IN CONST EFI_MM_ENTRY_CONTEXT  *MmEntryContext
 )
 {
-  EFI_STATUS                  Status;
-  EFI_MM_COMMUNICATE_HEADER  *CommunicateHeader;
+  EFI_STATUS                    Status;
+  EFI_MM_COMMUNICATE_HEADER_V3  *CommunicateHeader;
+  EFI_MM_COMMUNICATE_HEADER     *LegacyCommunicateHeader;
+  EFI_GUID                      *CommGuid;
+  VOID                          *CommData;
+  UINTN                         CommHeaderSize;
 
   DEBUG ((DEBUG_INFO, "MmEntryPoint ...\n"));
 
@@ -379,19 +383,35 @@ MmEntryPoint (
       gMmCorePrivate->CommunicationBuffer = 0;
       gMmCorePrivate->ReturnStatus = EFI_INVALID_PARAMETER;
     } else {
-      CommunicateHeader = (EFI_MM_COMMUNICATE_HEADER *)(UINTN)gMmCorePrivate->CommunicationBuffer;
-      gMmCorePrivate->BufferSize -= OFFSET_OF (EFI_MM_COMMUNICATE_HEADER, Data);
+      CommGuid = &((EFI_MM_COMMUNICATE_HEADER_V3 *)(UINTN)gMmCorePrivate->CommunicationBuffer)->HeaderGuid;
+      //
+      // Check if the signature matches EFI_MM_COMMUNICATE_HEADER_V3 definition
+      //
+      if (CompareGuid (CommGuid, &gCommunicateHeaderV3Guid)) {
+        CommunicateHeader = (EFI_MM_COMMUNICATE_HEADER_V3 *)(UINTN)gMmCorePrivate->CommunicationBuffer;
+        ASSERT (CommunicateHeader->Signature == EFI_MM_COMMUNICATE_HEADER_V3_SIGNATURE);
+        ASSERT (CommunicateHeader->Version <= EFI_MM_COMMUNICATE_HEADER_V3_VERSION);
+        CommGuid = &CommunicateHeader->MessageGuid;
+        CommData = CommunicateHeader->MessageData;
+        CommHeaderSize = sizeof (EFI_MM_COMMUNICATE_HEADER_V3);
+      } else {
+        LegacyCommunicateHeader = (EFI_MM_COMMUNICATE_HEADER *)(UINTN)gMmCorePrivate->CommunicationBuffer;
+        CommGuid = &LegacyCommunicateHeader->HeaderGuid;
+        CommData = LegacyCommunicateHeader->Data;
+        CommHeaderSize = OFFSET_OF (EFI_MM_COMMUNICATE_HEADER, Data);
+      }
+      gMmCorePrivate->BufferSize -= CommHeaderSize;
       Status = MmiManage (
-                 &CommunicateHeader->HeaderGuid,
+                 CommGuid,
                  NULL,
-                 CommunicateHeader->Data,
+                 CommData,
                  (UINTN *)&gMmCorePrivate->BufferSize
                  );
       //
       // Update CommunicationBuffer, BufferSize and ReturnStatus
       // Communicate service finished, reset the pointer to CommBuffer to NULL
       //
-      gMmCorePrivate->BufferSize += OFFSET_OF (EFI_MM_COMMUNICATE_HEADER, Data);
+      gMmCorePrivate->BufferSize += CommHeaderSize;
       gMmCorePrivate->CommunicationBuffer = 0;
       gMmCorePrivate->ReturnStatus = (Status == EFI_SUCCESS) ? EFI_SUCCESS : EFI_NOT_FOUND;
     }
diff --git a/StandaloneMmPkg/Core/StandaloneMmCore.inf b/StandaloneMmPkg/Core/StandaloneMmCore.inf
index 56042b7b39f4..41a49e23fa8f 100644
--- a/StandaloneMmPkg/Core/StandaloneMmCore.inf
+++ b/StandaloneMmPkg/Core/StandaloneMmCore.inf
@@ -75,6 +75,7 @@ [Guids]
   gEfiEventLegacyBootGuid
   gEfiEventExitBootServicesGuid
   gEfiEventReadyToBootGuid
+  gCommunicateHeaderV3Guid    ## CONSUMES   ## GUID # Communicate header
 
 [BuildOptions]
   GCC:*_*_*_CC_FLAGS = -fpie
-- 
2.32.0.windows.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v3 7/7] MdeModulePkg: PiSmmIpl: Update MessageLength calculation for MmCommunicate
  2021-08-17  5:08 [PATCH v3 0/7] New MM Communicate header and interfaces Kun Qin
                   ` (5 preceding siblings ...)
  2021-08-17  5:08 ` [PATCH v3 6/7] StandaloneMmPkg: StandaloneMmCore: Parsing " Kun Qin
@ 2021-08-17  5:08 ` Kun Qin
  2021-09-16  2:15 ` [edk2-devel] [PATCH v3 0/7] New MM Communicate header and interfaces Kun Qin
  7 siblings, 0 replies; 11+ messages in thread
From: Kun Qin @ 2021-08-17  5:08 UTC (permalink / raw)
  To: devel; +Cc: Jian J Wang, Hao A Wu, Eric Dong, Ray Ni

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3398
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3430

This change added support of installing `EFI_MM_COMMUNICATION3_PROTOCOL`.

MmCommunicate v3 routine that calculates message length is also updated
to remove ambiguity in contrast to v1 routine.

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>

Signed-off-by: Kun Qin <kuqin12@gmail.com>
---

Notes:
    v3:
    - Newly added v3 communicate protocol instance

 MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c   | 187 ++++++++++++++++++++
 MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf |   2 +
 2 files changed, 189 insertions(+)

diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
index 599a0cd01d80..356efa172cfd 100644
--- a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
+++ b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
@@ -11,6 +11,7 @@
 #include <Protocol/SmmBase2.h>
 #include <Protocol/SmmCommunication.h>
 #include <Protocol/MmCommunication2.h>
+#include <Protocol/MmCommunication3.h>
 #include <Protocol/SmmAccess2.h>
 #include <Protocol/SmmConfiguration.h>
 #include <Protocol/SmmControl2.h>
@@ -34,6 +35,7 @@
 #include <Library/UefiRuntimeLib.h>
 #include <Library/PcdLib.h>
 #include <Library/ReportStatusCodeLib.h>
+#include <Library/SafeIntLib.h>
 
 #include "PiSmmCorePrivateData.h"
 
@@ -146,6 +148,41 @@ SmmCommunicationMmCommunicate2 (
   IN OUT UINTN                              *CommSize OPTIONAL
   );
 
+/**
+  Communicates with a registered handler.
+
+  This function provides a service to send and receive messages from a registered UEFI service.
+
+  @param[in] This                     The EFI_MM_COMMUNICATION3_PROTOCOL instance.
+  @param[in, out] CommBufferPhysical  Physical address of the MM communication buffer, of which content must
+                                      start with EFI_MM_COMMUNICATE_HEADER_V3.
+  @param[in, out] CommBufferVirtual   Virtual address of the MM communication buffer, of which content must
+                                      start with EFI_MM_COMMUNICATE_HEADER_V3.
+  @param[in, out] CommSize            The size of the data buffer being passed in. On exit, the size of data
+                                      being returned. Zero if the handler does not wish to reply with any data.
+                                      This parameter is optional and may be NULL.
+
+  @retval EFI_SUCCESS                 The message was successfully posted.
+  @retval EFI_INVALID_PARAMETER       CommBufferPhysical was NULL or CommBufferVirtual was NULL.
+  @retval EFI_BAD_BUFFER_SIZE         The buffer is too large for the MM implementation.
+                                      If this error is returned, the MessageLength field
+                                      in the CommBuffer header or the integer pointed by
+                                      CommSize, are updated to reflect the maximum payload
+                                      size the implementation can accommodate.
+  @retval EFI_ACCESS_DENIED           The CommunicateBuffer parameter or CommSize parameter,
+                                      if not omitted, are in address range that cannot be
+                                      accessed by the MM environment.
+
+**/
+EFI_STATUS
+EFIAPI
+MmCommunicationMmCommunicate3 (
+  IN CONST EFI_MM_COMMUNICATION3_PROTOCOL   *This,
+  IN OUT VOID                               *CommBufferPhysical,
+  IN OUT VOID                               *CommBufferVirtual,
+  IN OUT UINTN                              *CommSize OPTIONAL
+  );
+
 /**
   Event notification that is fired every time a gEfiSmmConfigurationProtocol installs.
 
@@ -275,6 +312,13 @@ EFI_MM_COMMUNICATION2_PROTOCOL  mMmCommunication2 = {
   SmmCommunicationMmCommunicate2
 };
 
+//
+// PI 1.7 MM Communication Protocol 3 instance
+//
+EFI_MM_COMMUNICATION3_PROTOCOL  mMmCommunication3 = {
+  MmCommunicationMmCommunicate3
+};
+
 //
 // SMM Core Private Data structure that contains the data shared between
 // the SMM IPL and the SMM Core.
@@ -649,6 +693,148 @@ SmmCommunicationMmCommunicate2 (
                                       CommSize);
 }
 
+/**
+  Communicates with a registered handler.
+
+  This function provides a service to send and receive messages from a registered UEFI service.
+
+  @param[in] This                     The EFI_MM_COMMUNICATION3_PROTOCOL instance.
+  @param[in, out] CommBufferPhysical  Physical address of the MM communication buffer, of which content must
+                                      start with EFI_MM_COMMUNICATE_HEADER_V3.
+  @param[in, out] CommBufferVirtual   Virtual address of the MM communication buffer, of which content must
+                                      start with EFI_MM_COMMUNICATE_HEADER_V3.
+  @param[in, out] CommSize            The size of the data buffer being passed in. On exit, the size of data
+                                      being returned. Zero if the handler does not wish to reply with any data.
+                                      This parameter is optional and may be NULL.
+
+  @retval EFI_SUCCESS                 The message was successfully posted.
+  @retval EFI_INVALID_PARAMETER       CommBufferPhysical was NULL or CommBufferVirtual was NULL.
+  @retval EFI_BAD_BUFFER_SIZE         The buffer is too large for the MM implementation.
+                                      If this error is returned, the MessageLength field
+                                      in the CommBuffer header or the integer pointed by
+                                      CommSize, are updated to reflect the maximum payload
+                                      size the implementation can accommodate.
+  @retval EFI_ACCESS_DENIED           The CommunicateBuffer parameter or CommSize parameter,
+                                      if not omitted, are in address range that cannot be
+                                      accessed by the MM environment.
+
+**/
+EFI_STATUS
+EFIAPI
+MmCommunicationMmCommunicate3 (
+  IN CONST EFI_MM_COMMUNICATION3_PROTOCOL   *This,
+  IN OUT VOID                               *CommBufferPhysical,
+  IN OUT VOID                               *CommBufferVirtual,
+  IN OUT UINTN                              *CommSize OPTIONAL
+  )
+{
+  EFI_STATUS                    Status;
+  EFI_MM_COMMUNICATE_HEADER_V3  *CommunicateHeader;
+  BOOLEAN                       OldInSmm;
+  UINTN                         TempCommSize;
+  UINT64                        LongCommSize;
+
+  //
+  // Check parameters
+  //
+  if (CommBufferPhysical == NULL) {
+    return EFI_INVALID_PARAMETER;
+  }
+
+  CommunicateHeader = (EFI_MM_COMMUNICATE_HEADER_V3 *) CommBufferPhysical;
+
+  if (CommSize == NULL) {
+    Status = SafeUint64Add (sizeof (EFI_MM_COMMUNICATE_HEADER_V3), CommunicateHeader->MessageSize, &LongCommSize);
+    if (EFI_ERROR (Status)) {
+      return EFI_INVALID_PARAMETER;
+    }
+    Status = SafeUint64ToUintn (LongCommSize, &TempCommSize);
+    if (EFI_ERROR (Status)) {
+      return EFI_INVALID_PARAMETER;
+    }
+  } else {
+    TempCommSize = *CommSize;
+    //
+    // CommSize must hold the entire EFI_MM_COMMUNICATE_HEADER_V3
+    //
+    if (TempCommSize < sizeof (EFI_MM_COMMUNICATE_HEADER_V3)) {
+      return EFI_INVALID_PARAMETER;
+    }
+  }
+
+  //
+  // If not already in SMM, then generate a Software SMI
+  //
+  if (!gSmmCorePrivate->InSmm && gSmmCorePrivate->SmmEntryPointRegistered) {
+    //
+    // Put arguments for Software SMI in gSmmCorePrivate
+    //
+    gSmmCorePrivate->CommunicationBuffer = CommBufferPhysical;
+    gSmmCorePrivate->BufferSize          = TempCommSize;
+
+    //
+    // Generate Software SMI
+    //
+    Status = mSmmControl2->Trigger (mSmmControl2, NULL, NULL, FALSE, 0);
+    if (EFI_ERROR (Status)) {
+      return EFI_UNSUPPORTED;
+    }
+
+    //
+    // Return status from software SMI
+    //
+    if (CommSize != NULL) {
+      *CommSize = gSmmCorePrivate->BufferSize;
+    }
+    return gSmmCorePrivate->ReturnStatus;
+  }
+
+  //
+  // If we are in SMM, then the execution mode must be physical, which means that
+  // OS established virtual addresses can not be used.  If SetVirtualAddressMap()
+  // has been called, then a direct invocation of the Software SMI is not allowed,
+  // so return EFI_INVALID_PARAMETER.
+  //
+  if (EfiGoneVirtual()) {
+    return EFI_INVALID_PARAMETER;
+  }
+
+  //
+  // If we are not in SMM, don't allow call SmiManage() directly when SMRAM is closed or locked.
+  //
+  if ((!gSmmCorePrivate->InSmm) && (!mSmmAccess->OpenState || mSmmAccess->LockState)) {
+    return EFI_INVALID_PARAMETER;
+  }
+
+  //
+  // Save current InSmm state and set InSmm state to TRUE
+  //
+  OldInSmm = gSmmCorePrivate->InSmm;
+  gSmmCorePrivate->InSmm = TRUE;
+
+  //
+  // Before SetVirtualAddressMap(), we are in SMM or SMRAM is open and unlocked, call SmiManage() directly.
+  //
+  TempCommSize -= sizeof (EFI_MM_COMMUNICATE_HEADER_V3);
+  Status = gSmmCorePrivate->Smst->SmiManage (
+                                    &CommunicateHeader->MessageGuid,
+                                    NULL,
+                                    CommunicateHeader->MessageData,
+                                    &TempCommSize
+                                    );
+  TempCommSize += sizeof (EFI_MM_COMMUNICATE_HEADER_V3);
+  if (CommSize != NULL) {
+    *CommSize = TempCommSize;
+  }
+
+  //
+  // Restore original InSmm state
+  //
+  gSmmCorePrivate->InSmm = OldInSmm;
+
+  return (Status == EFI_SUCCESS) ? EFI_SUCCESS : EFI_NOT_FOUND;
+}
+
 /**
   Event notification that is fired when GUIDed Event Group is signaled.
 
@@ -1832,6 +2018,7 @@ SmmIplEntry (
                   &gEfiSmmBase2ProtocolGuid,         &mSmmBase2,
                   &gEfiSmmCommunicationProtocolGuid, &mSmmCommunication,
                   &gEfiMmCommunication2ProtocolGuid, &mMmCommunication2,
+                  &gEfiMmCommunication3ProtocolGuid, &mMmCommunication3,
                   NULL
                   );
   ASSERT_EFI_ERROR (Status);
diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf
index 6109d6b5449c..afab228cc04c 100644
--- a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf
+++ b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf
@@ -46,11 +46,13 @@ [LibraryClasses]
   DxeServicesLib
   PcdLib
   ReportStatusCodeLib
+  SafeIntLib
 
 [Protocols]
   gEfiSmmBase2ProtocolGuid                      ## PRODUCES
   gEfiSmmCommunicationProtocolGuid              ## PRODUCES
   gEfiMmCommunication2ProtocolGuid              ## PRODUCES
+  gEfiMmCommunication3ProtocolGuid              ## PRODUCES
   gEfiSmmAccess2ProtocolGuid                    ## CONSUMES
   ## NOTIFY
   ## CONSUMES
-- 
2.32.0.windows.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [edk2-devel] [PATCH v3 2/7] MdePkg: MmCommunication: Introduce EFI_MM_COMMUNICATE_HEADER_V3 to MdePkg
  2021-08-17  5:08 ` [PATCH v3 2/7] MdePkg: MmCommunication: Introduce EFI_MM_COMMUNICATE_HEADER_V3 to MdePkg Kun Qin
@ 2021-08-17  5:50   ` Ni, Ray
  2021-08-17  6:00     ` Kun Qin
  0 siblings, 1 reply; 11+ messages in thread
From: Ni, Ray @ 2021-08-17  5:50 UTC (permalink / raw)
  To: devel@edk2.groups.io, kuqin12@gmail.com
  Cc: Kinney, Michael D, Liming Gao, Liu, Zhiguang, Wu, Hao A,
	Marvin Häuser, Bret Barkelew, Michael Kubacki

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
> 
> 
> 
> 
> 


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [edk2-devel] [PATCH v3 2/7] MdePkg: MmCommunication: Introduce EFI_MM_COMMUNICATE_HEADER_V3 to MdePkg
  2021-08-17  5:50   ` [edk2-devel] " Ni, Ray
@ 2021-08-17  6:00     ` Kun Qin
  0 siblings, 0 replies; 11+ messages in thread
From: Kun Qin @ 2021-08-17  6:00 UTC (permalink / raw)
  To: Ni, Ray, devel@edk2.groups.io
  Cc: Kinney, Michael D, Liming Gao, Liu, Zhiguang, Wu, Hao A,
	Marvin Häuser, Bret Barkelew, Michael Kubacki

Hi Ray,

This same header will be commonly used for both MM communicate PPI and 
Protocol. If including this definition in MmCommunicate protocol header, 
the potential PEIM that installs MM communicate PPI will need to include 
a PPI header as well as the aforementioned protocol header, which to me 
seems counter-intuitive.

I think PiMultiPhase header seems to be a file more ideal to resolve the 
entanglement above, as this structure indeed covers multiple phases 
during boot process.

Please let me know if you have any other thoughts.

Thanks,
Kun

On 08/16/2021 22:50, Ni, Ray wrote:
> 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
>>
>>
>>
>> 
>>
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [edk2-devel] [PATCH v3 0/7] New MM Communicate header and interfaces
  2021-08-17  5:08 [PATCH v3 0/7] New MM Communicate header and interfaces Kun Qin
                   ` (6 preceding siblings ...)
  2021-08-17  5:08 ` [PATCH v3 7/7] MdeModulePkg: PiSmmIpl: Update MessageLength calculation for MmCommunicate Kun Qin
@ 2021-09-16  2:15 ` Kun Qin
  7 siblings, 0 replies; 11+ messages in thread
From: Kun Qin @ 2021-09-16  2:15 UTC (permalink / raw)
  To: Kun Qin, devel

[-- Attachment #1: Type: text/plain, Size: 190 bytes --]

Hi EDK2 maintainers,

It has been a while since this v3 patch series were sent. May I please have some feedback on the proposed PI spec change? Any input is appreciated.

Regards,
Kun

[-- Attachment #2: Type: text/html, Size: 210 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2021-09-16  2:15 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-08-17  5:08 [PATCH v3 0/7] New MM Communicate header and interfaces Kun Qin
2021-08-17  5:08 ` [PATCH v3 1/7] EDK2 Code First: PI Specification: New communicate " Kun Qin
2021-08-17  5:08 ` [PATCH v3 2/7] MdePkg: MmCommunication: Introduce EFI_MM_COMMUNICATE_HEADER_V3 to MdePkg Kun Qin
2021-08-17  5:50   ` [edk2-devel] " Ni, Ray
2021-08-17  6:00     ` Kun Qin
2021-08-17  5:08 ` [PATCH v3 3/7] MdePkg: MmCommunication: Introduce EFI_MM_COMMUNICATION3_PROTOCOL " Kun Qin
2021-08-17  5:08 ` [PATCH v3 4/7] MdePkg: MmCommunication: Introduce EFI_PEI_MM_COMMUNICATION3_PPI " Kun Qin
2021-08-17  5:08 ` [PATCH v3 5/7] MdeModulePkg: PiSmmCore: Added parser of new MM communicate header Kun Qin
2021-08-17  5:08 ` [PATCH v3 6/7] StandaloneMmPkg: StandaloneMmCore: Parsing " Kun Qin
2021-08-17  5:08 ` [PATCH v3 7/7] MdeModulePkg: PiSmmIpl: Update MessageLength calculation for MmCommunicate Kun Qin
2021-09-16  2:15 ` [edk2-devel] [PATCH v3 0/7] New MM Communicate header and interfaces Kun Qin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox