[PATCH v2 0/6] EDK2 Code First: PI Specification: Update EFI_MM_COMMUNICATE_HEADER

[PATCH v2 0/6] EDK2 Code First: PI Specification: Update EFI_MM_COMMUNICATE_HEADER

[Kun Qin]

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

v2 patch changes include feedback for v1 series:
a. Added "Reviewed-by" tag for applicable patch;
b. Removed "BZ3398" tags for applicable patches;
c. Added a patch that covers changes needed for SmmLockBoxDxeLib;

There is also concern raised from v1 patch review:
https://edk2.groups.io/g/devel/message/76570

Please advise if any tool/checks could help catching errors as such.

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

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 (6):
  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
  MdeModulePkg: SmmLockBoxDxeLib: Updated MessageLength calculation
  MdePkg: MmCommunication: Extend MessageLength field size to UINT64

 MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.c         | 28 ++++--
 MdeModulePkg/Application/SmiHandlerProfileInfo/SmiHandlerProfileInfo.c | 10 ++-
 MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c                                 | 11 ++-
 MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxDxeLib.c                  | 23 +++--
 BZ3430-SpecChange.md                                                   | 90 ++++++++++++++++++++
 MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf                               |  1 +
 MdePkg/Include/Protocol/MmCommunication.h                              |  2 +-
 7 files changed, 142 insertions(+), 23 deletions(-)
 create mode 100644 BZ3430-SpecChange.md

-- 
2.31.1.windows.1

[PATCH v2 1/6] EDK2 Code First: PI Specification: EFI_MM_COMMUNICATE_HEADER Update

[Kun Qin]

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: Laszlo Ersek <lersek@redhat.com>
Cc: Leif Lindholm <leif@nuviainc.com>
Cc: Hao A Wu <hao.a.wu@intel.com>

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

Notes:
    v2:
    - Updated change impact analysis regarding SmmLockBoxDxeLib [Hao]

 BZ3430-SpecChange.md | 90 ++++++++++++++++++++
 1 file changed, 90 insertions(+)

diff --git a/BZ3430-SpecChange.md b/BZ3430-SpecChange.md
new file mode 100644
index 000000000000..a4f8b397e822
--- /dev/null
+++ b/BZ3430-SpecChange.md
@@ -0,0 +1,90 @@
+# Title: Change MessageLength Field of EFI_MM_COMMUNICATE_HEADER to UINT64
+
+## 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
+
+Change the `MessageLength` Field of `EFI_MM_COMMUNICATE_HEADER` from UINTN to UINT64 to remove architecture dependency:
+
+```c
+typedef struct {
+  EFI_GUID  HeaderGuid;
+  UINT64    MessageLength;
+  UINT8     Data[ANYSIZE_ARRAY];
+} EFI_MM_COMMUNICATE_HEADER;
+```
+
+## 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 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.
+
+## Impact of the change
+
+This change will impact the known structure consumers including:
+
+```bash
+MdeModulePkg/Core/PiSmmCore/PiSmmIpl
+MdeModulePkg/Application/SmiHandlerProfileInfo
+MdeModulePkg/Application/MemoryProfileInfo
+MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxDxeLib
+```
+
+For consumers that are not using `OFFSET_OF(EFI_MM_COMMUNICATE_HEADER, Data)`, but performing explicit addition such as the existing MdeModulePkg/Application/SmiHandlerProfileInfo/SmiHandlerProfileInfo.c, one will need to change code implementation to match new structure definition. Otherwise, the code compiled on IA32 architecture will experience structure field dereference error.
+
+User who currently uses UINTN local variables as place holder of MessageLength will need to use caution to make cast from UINTN to UINT64 and vice versa. It is recommended to use `SafeUint64ToUintn` for such operations when the value is indeterministic.
+
+Note: MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxPeiLib is also consuming this structure, but it handled this size discrepancy internally. If this potential spec change is not applied, all applicable PEI MM communicate callers will need to use the same routine as that of SmmLockBoxPeiLib to invoke a properly populated EFI_MM_COMMUNICATE_HEADER to be used in X64 MM foundation.
+
+## Detailed description of the change [normative updates]
+
+### Specification Changes
+
+1. In PI Specification v1.7 Errata A: Vol. 4 Page-91, the definition of `EFI_MM_COMMUNICATE_HEADER` should be changed from current:
+
+```c
+typedef struct {
+  EFI_GUID  HeaderGuid;
+  UINTN     MessageLength;
+  UINT8     Data[ANYSIZE_ARRAY];
+} EFI_MM_COMMUNICATE_HEADER;
+```
+
+to:
+
+```c
+typedef struct {
+  EFI_GUID  HeaderGuid;
+  UINT64    MessageLength;
+  UINT8     Data[ANYSIZE_ARRAY];
+} EFI_MM_COMMUNICATE_HEADER;
+```
+
+### Code Changes
+
+1. Remove the explicit calculation of the offset of `Data` in `EFI_MM_COMMUNICATE_HEADER`. Thus applicable calculations of `sizeof(EFI_GUID) + sizeof(UINTN)` should be replaced with `OFFSET_OF(EFI_MM_COMMUNICATE_HEADER, Data)` or similar alternatives. These calculations are identified in:
+
+```bash
+MdeModulePkg/Application/SmiHandlerProfileInfo/SmiHandlerProfileInfo.c
+MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.c
+MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxDxeLib.c
+```
+
+1. Resolve potentially mismatched type between `UINTN` and `UINT64`. This would occur when `MessageLength` or its derivitive are used for local calculation with existing `UINTN` typed variables. Code change regarding this perspective is per case evaluation: if the variables involved are all deterministic values, and there is no overflow or underflow risk, a cast operation (from `UINTN` to `UINT64`) can be safely used. Otherwise, the calculation will be performed in `UINT64` bitwidth and then convert to `UINTN` using `SafeUint64*` and `SafeUint64ToUintn`, respectively. These operations are identified in:
+
+```bash
+MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
+MdeModulePkg/Application/SmiHandlerProfileInfo/SmiHandlerProfileInfo.c
+MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.c
+```
+
+1. After all above changes applied and specification updated, `MdePkg/Include/Protocol/MmCommunication.h` will need to be updated to match new definition that includes the field type update.
-- 
2.31.1.windows.1

[PATCH v2 2/6] MdeModulePkg: PiSmmIpl: Update MessageLength calculation for MmCommunicate

[Kun Qin]

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

This change updated calculation routine for MM communication in PiSmmIpl.
It removes ambiguity brought in by UINTN variables from this routine and
paves way for updating definition of field MessageLength in
EFI_MM_COMMUNICATE_HEADER to definitive size.

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>
Reviewed-by: Hao A Wu <hao.a.wu@intel.com>
---

Notes:
    v2:
    - Removed "BZ" tags from comments and variables [Hao]
    - Added "Reviewed-by" tag [Hao]

 MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c   | 11 ++++++++++-
 MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf |  1 +
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
index 599a0cd01d80..01cde6cfc3e4 100644
--- a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
+++ b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
@@ -34,6 +34,7 @@
 #include <Library/UefiRuntimeLib.h>
 #include <Library/PcdLib.h>
 #include <Library/ReportStatusCodeLib.h>
+#include <Library/SafeIntLib.h>
 
 #include "PiSmmCorePrivateData.h"
 
@@ -515,6 +516,7 @@ SmmCommunicationCommunicate (
   EFI_STATUS                  Status;
   EFI_SMM_COMMUNICATE_HEADER  *CommunicateHeader;
   BOOLEAN                     OldInSmm;
+  UINT64                      LongCommSize;
   UINTN                       TempCommSize;
 
   //
@@ -527,7 +529,14 @@ SmmCommunicationCommunicate (
   CommunicateHeader = (EFI_SMM_COMMUNICATE_HEADER *) CommBuffer;
 
   if (CommSize == NULL) {
-    TempCommSize = OFFSET_OF (EFI_SMM_COMMUNICATE_HEADER, Data) + CommunicateHeader->MessageLength;
+    Status = SafeUint64Add (OFFSET_OF (EFI_SMM_COMMUNICATE_HEADER, Data), CommunicateHeader->MessageLength, &LongCommSize);
+    if (EFI_ERROR (Status)) {
+      return EFI_INVALID_PARAMETER;
+    }
+    Status = SafeUint64ToUintn (LongCommSize, &TempCommSize);
+    if (EFI_ERROR (Status)) {
+      return EFI_INVALID_PARAMETER;
+    }
   } else {
     TempCommSize = *CommSize;
     //
diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf
index 6109d6b5449c..ddeb39cee266 100644
--- a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf
+++ b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf
@@ -46,6 +46,7 @@ [LibraryClasses]
   DxeServicesLib
   PcdLib
   ReportStatusCodeLib
+  SafeIntLib
 
 [Protocols]
   gEfiSmmBase2ProtocolGuid                      ## PRODUCES
-- 
2.31.1.windows.1

[PATCH v2 3/6] MdeModulePkg: MemoryProfileInfo: Updated MessageLength calculation

[Kun Qin]

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.

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

Notes:
    v2:
    - Added a missed case this change should cover [Hao]
    - Removed "BZ" tags from comments [Hao]

 MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.c | 28 +++++++++++++++-----
 1 file changed, 21 insertions(+), 7 deletions(-)

diff --git a/MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.c b/MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.c
index 191c31068545..69f78c090e7c 100644
--- a/MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.c
+++ b/MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.c
@@ -1140,8 +1140,7 @@ GetSmramProfileData (
     return Status;
   }
 
-  MinimalSizeNeeded = sizeof (EFI_GUID) +
-                      sizeof (UINTN) +
+  MinimalSizeNeeded = OFFSET_OF(EFI_SMM_COMMUNICATE_HEADER, Data) +
                       MAX (sizeof (SMRAM_PROFILE_PARAMETER_GET_PROFILE_INFO),
                            MAX (sizeof (SMRAM_PROFILE_PARAMETER_GET_PROFILE_DATA_BY_OFFSET),
                                 sizeof (SMRAM_PROFILE_PARAMETER_RECORDING_STATE)));
@@ -1190,7 +1189,10 @@ GetSmramProfileData (
   CommRecordingState->Header.ReturnStatus = (UINT64)-1;
   CommRecordingState->RecordingState      = MEMORY_PROFILE_RECORDING_DISABLE;
 
-  CommSize = sizeof (EFI_GUID) + sizeof (UINTN) + CommHeader->MessageLength;
+  //
+  // 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);
   if (EFI_ERROR (Status)) {
     DEBUG ((EFI_D_ERROR, "SmramProfile: SmmCommunication - %r\n", Status));
@@ -1213,7 +1215,10 @@ GetSmramProfileData (
     CommRecordingState->Header.ReturnStatus = (UINT64)-1;
     CommRecordingState->RecordingState      = MEMORY_PROFILE_RECORDING_DISABLE;
 
-    CommSize = sizeof (EFI_GUID) + sizeof (UINTN) + CommHeader->MessageLength;
+    //
+    // 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 +1235,10 @@ GetSmramProfileData (
   CommGetProfileInfo->Header.ReturnStatus = (UINT64)-1;
   CommGetProfileInfo->ProfileSize         = 0;
 
-  CommSize = sizeof (EFI_GUID) + sizeof (UINTN) + CommHeader->MessageLength;
+  //
+  // 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 +1269,10 @@ GetSmramProfileData (
   CommGetProfileData->Header.DataLength   = sizeof (*CommGetProfileData);
   CommGetProfileData->Header.ReturnStatus = (UINT64)-1;
 
-  CommSize = sizeof (EFI_GUID) + sizeof (UINTN) + CommHeader->MessageLength;
+  //
+  // 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 +1331,10 @@ GetSmramProfileData (
     CommRecordingState->Header.ReturnStatus = (UINT64)-1;
     CommRecordingState->RecordingState      = MEMORY_PROFILE_RECORDING_ENABLE;
 
-    CommSize = sizeof (EFI_GUID) + sizeof (UINTN) + CommHeader->MessageLength;
+    //
+    // 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

[PATCH v2 4/6] MdeModulePkg: SmiHandlerProfileInfo: Updated MessageLength calculation

[Kun Qin]

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.

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:
    v2:
    - Updated comments by removing "BZ" tags [Hao]

 MdeModulePkg/Application/SmiHandlerProfileInfo/SmiHandlerProfileInfo.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/MdeModulePkg/Application/SmiHandlerProfileInfo/SmiHandlerProfileInfo.c b/MdeModulePkg/Application/SmiHandlerProfileInfo/SmiHandlerProfileInfo.c
index 4153074b7a80..4bfd5946caba 100644
--- a/MdeModulePkg/Application/SmiHandlerProfileInfo/SmiHandlerProfileInfo.c
+++ b/MdeModulePkg/Application/SmiHandlerProfileInfo/SmiHandlerProfileInfo.c
@@ -116,7 +116,10 @@ GetSmiHandlerProfileDatabase(
   CommGetInfo->Header.ReturnStatus = (UINT64)-1;
   CommGetInfo->DataSize = 0;
 
-  CommSize = sizeof(EFI_GUID) + sizeof(UINTN) + CommHeader->MessageLength;
+  //
+  // 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);
   if (EFI_ERROR(Status)) {
     Print(L"SmiHandlerProfile: SmmCommunication - %r\n", Status);
@@ -149,7 +152,10 @@ GetSmiHandlerProfileDatabase(
   CommGetData->Header.DataLength = sizeof(*CommGetData);
   CommGetData->Header.ReturnStatus = (UINT64)-1;
 
-  CommSize = sizeof(EFI_GUID) + sizeof(UINTN) + CommHeader->MessageLength;
+  //
+  // 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;
 
-- 
2.31.1.windows.1

[PATCH v2 5/6] MdeModulePkg: SmmLockBoxDxeLib: Updated MessageLength calculation

[Kun Qin]

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.

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:
    v2:
    - Newly added in v2

 MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxDxeLib.c | 23 ++++++++++----------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxDxeLib.c b/MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxDxeLib.c
index 2cbffe889e1f..66fbe4dd961c 100644
--- a/MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxDxeLib.c
+++ b/MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxDxeLib.c
@@ -79,8 +79,7 @@ LockBoxGetSmmCommBuffer (
     return mLockBoxSmmCommBuffer;
   }
 
-  MinimalSizeNeeded = sizeof (EFI_GUID) +
-                      sizeof (UINTN) +
+  MinimalSizeNeeded = OFFSET_OF (EFI_SMM_COMMUNICATE_HEADER, Data) +
                       MAX (sizeof (EFI_SMM_LOCK_BOX_PARAMETER_SAVE),
                            MAX (sizeof (EFI_SMM_LOCK_BOX_PARAMETER_SET_ATTRIBUTES),
                                 MAX (sizeof (EFI_SMM_LOCK_BOX_PARAMETER_UPDATE),
@@ -142,7 +141,7 @@ SaveLockBox (
   EFI_SMM_COMMUNICATION_PROTOCOL  *SmmCommunication;
   EFI_SMM_LOCK_BOX_PARAMETER_SAVE *LockBoxParameterSave;
   EFI_SMM_COMMUNICATE_HEADER      *CommHeader;
-  UINT8                           TempCommBuffer[sizeof(EFI_GUID) + sizeof(UINTN) + sizeof(EFI_SMM_LOCK_BOX_PARAMETER_SAVE)];
+  UINT8                           TempCommBuffer[OFFSET_OF (EFI_SMM_COMMUNICATE_HEADER, Data) + sizeof(EFI_SMM_LOCK_BOX_PARAMETER_SAVE)];
   UINT8                           *CommBuffer;
   UINTN                           CommSize;
 
@@ -182,7 +181,7 @@ SaveLockBox (
   //
   // Send command
   //
-  CommSize = sizeof(EFI_GUID) + sizeof(UINTN) + sizeof(EFI_SMM_LOCK_BOX_PARAMETER_SAVE);
+  CommSize = OFFSET_OF (EFI_SMM_COMMUNICATE_HEADER, Data) + sizeof(EFI_SMM_LOCK_BOX_PARAMETER_SAVE);
   Status = SmmCommunication->Communicate (
                                SmmCommunication,
                                &CommBuffer[0],
@@ -224,7 +223,7 @@ SetLockBoxAttributes (
   EFI_SMM_COMMUNICATION_PROTOCOL            *SmmCommunication;
   EFI_SMM_LOCK_BOX_PARAMETER_SET_ATTRIBUTES *LockBoxParameterSetAttributes;
   EFI_SMM_COMMUNICATE_HEADER                *CommHeader;
-  UINT8                                     TempCommBuffer[sizeof(EFI_GUID) + sizeof(UINTN) + sizeof(EFI_SMM_LOCK_BOX_PARAMETER_SET_ATTRIBUTES)];
+  UINT8                                     TempCommBuffer[OFFSET_OF (EFI_SMM_COMMUNICATE_HEADER, Data) + sizeof(EFI_SMM_LOCK_BOX_PARAMETER_SET_ATTRIBUTES)];
   UINT8                                     *CommBuffer;
   UINTN                                     CommSize;
 
@@ -264,7 +263,7 @@ SetLockBoxAttributes (
   //
   // Send command
   //
-  CommSize = sizeof(EFI_GUID) + sizeof(UINTN) + sizeof(EFI_SMM_LOCK_BOX_PARAMETER_SET_ATTRIBUTES);
+  CommSize = OFFSET_OF (EFI_SMM_COMMUNICATE_HEADER, Data) + sizeof(EFI_SMM_LOCK_BOX_PARAMETER_SET_ATTRIBUTES);
   Status = SmmCommunication->Communicate (
                                SmmCommunication,
                                &CommBuffer[0],
@@ -314,7 +313,7 @@ UpdateLockBox (
   EFI_SMM_COMMUNICATION_PROTOCOL    *SmmCommunication;
   EFI_SMM_LOCK_BOX_PARAMETER_UPDATE *LockBoxParameterUpdate;
   EFI_SMM_COMMUNICATE_HEADER        *CommHeader;
-  UINT8                             TempCommBuffer[sizeof(EFI_GUID) + sizeof(UINTN) + sizeof(EFI_SMM_LOCK_BOX_PARAMETER_UPDATE)];
+  UINT8                             TempCommBuffer[OFFSET_OF (EFI_SMM_COMMUNICATE_HEADER, Data) + sizeof(EFI_SMM_LOCK_BOX_PARAMETER_UPDATE)];
   UINT8                             *CommBuffer;
   UINTN                             CommSize;
 
@@ -355,7 +354,7 @@ UpdateLockBox (
   //
   // Send command
   //
-  CommSize = sizeof(EFI_GUID) + sizeof(UINTN) + sizeof(EFI_SMM_LOCK_BOX_PARAMETER_UPDATE);
+  CommSize = OFFSET_OF (EFI_SMM_COMMUNICATE_HEADER, Data) + sizeof(EFI_SMM_LOCK_BOX_PARAMETER_UPDATE);
   Status = SmmCommunication->Communicate (
                                SmmCommunication,
                                &CommBuffer[0],
@@ -403,7 +402,7 @@ RestoreLockBox (
   EFI_SMM_COMMUNICATION_PROTOCOL     *SmmCommunication;
   EFI_SMM_LOCK_BOX_PARAMETER_RESTORE *LockBoxParameterRestore;
   EFI_SMM_COMMUNICATE_HEADER         *CommHeader;
-  UINT8                              TempCommBuffer[sizeof(EFI_GUID) + sizeof(UINTN) + sizeof(EFI_SMM_LOCK_BOX_PARAMETER_RESTORE)];
+  UINT8                              TempCommBuffer[OFFSET_OF (EFI_SMM_COMMUNICATE_HEADER, Data) + sizeof(EFI_SMM_LOCK_BOX_PARAMETER_RESTORE)];
   UINT8                              *CommBuffer;
   UINTN                              CommSize;
 
@@ -449,7 +448,7 @@ RestoreLockBox (
   //
   // Send command
   //
-  CommSize = sizeof(EFI_GUID) + sizeof(UINTN) + sizeof(EFI_SMM_LOCK_BOX_PARAMETER_RESTORE);
+  CommSize = OFFSET_OF (EFI_SMM_COMMUNICATE_HEADER, Data) + sizeof(EFI_SMM_LOCK_BOX_PARAMETER_RESTORE);
   Status = SmmCommunication->Communicate (
                                SmmCommunication,
                                &CommBuffer[0],
@@ -488,7 +487,7 @@ RestoreAllLockBoxInPlace (
   EFI_SMM_COMMUNICATION_PROTOCOL                  *SmmCommunication;
   EFI_SMM_LOCK_BOX_PARAMETER_RESTORE_ALL_IN_PLACE *LockBoxParameterRestoreAllInPlace;
   EFI_SMM_COMMUNICATE_HEADER                      *CommHeader;
-  UINT8                                           TempCommBuffer[sizeof(EFI_GUID) + sizeof(UINTN) + sizeof(EFI_SMM_LOCK_BOX_PARAMETER_RESTORE_ALL_IN_PLACE)];
+  UINT8                                           TempCommBuffer[OFFSET_OF (EFI_SMM_COMMUNICATE_HEADER, Data) + sizeof(EFI_SMM_LOCK_BOX_PARAMETER_RESTORE_ALL_IN_PLACE)];
   UINT8                                           *CommBuffer;
   UINTN                                           CommSize;
 
@@ -518,7 +517,7 @@ RestoreAllLockBoxInPlace (
   //
   // Send command
   //
-  CommSize = sizeof(EFI_GUID) + sizeof(UINTN) + sizeof(EFI_SMM_LOCK_BOX_PARAMETER_RESTORE_ALL_IN_PLACE);
+  CommSize = OFFSET_OF (EFI_SMM_COMMUNICATE_HEADER, Data) + sizeof(EFI_SMM_LOCK_BOX_PARAMETER_RESTORE_ALL_IN_PLACE);
   Status = SmmCommunication->Communicate (
                                SmmCommunication,
                                &CommBuffer[0],
-- 
2.31.1.windows.1

[PATCH v2 6/6] MdePkg: MmCommunication: Extend MessageLength field size to UINT64

[Kun Qin]

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

The MessageLength field of EFI_MM_COMMUNICATE_HEADER, as a generic
definition, could be 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 change removes the architecture dependent field by extending this
field definition as UINT64.

Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Zhiguang Liu <zhiguang.liu@intel.com>

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

Notes:
    v2:
    - Removed comments with "BZ"

 MdePkg/Include/Protocol/MmCommunication.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/MdePkg/Include/Protocol/MmCommunication.h b/MdePkg/Include/Protocol/MmCommunication.h
index 34c3e2b5a9e3..d42b00bbeb7c 100644
--- a/MdePkg/Include/Protocol/MmCommunication.h
+++ b/MdePkg/Include/Protocol/MmCommunication.h
@@ -26,7 +26,7 @@ typedef struct {
   ///
   /// Describes the size of Data (in bytes) and does not include the size of the header.
   ///
-  UINTN     MessageLength;
+  UINT64    MessageLength;
   ///
   /// Designates an array of bytes that is MessageLength in size.
   ///
-- 
2.31.1.windows.1

Re: [edk2-devel] [PATCH v2 3/6] MdeModulePkg: MemoryProfileInfo: Updated MessageLength calculation

[Wu, Hao A]

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Kun Qin
> Sent: Friday, June 18, 2021 5:03 PM
> To: devel@edk2.groups.io
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>
> Subject: [edk2-devel] [PATCH v2 3/6] 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.
> 
> 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>
> ---
> 
> Notes:
>     v2:
>     - Added a missed case this change should cover [Hao]
>     - Removed "BZ" tags from comments [Hao]


Reviewed-by: Hao A Wu <hao.a.wu@intel.com>

Best Regards,
Hao Wu


> 
>  MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.c | 28
> +++++++++++++++-----
>  1 file changed, 21 insertions(+), 7 deletions(-)
> 
> diff --git
> a/MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.c
> b/MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.c
> index 191c31068545..69f78c090e7c 100644
> --- a/MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.c
> +++
> b/MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.c
> @@ -1140,8 +1140,7 @@ GetSmramProfileData (
>      return Status;
>    }
> 
> -  MinimalSizeNeeded = sizeof (EFI_GUID) +
> -                      sizeof (UINTN) +
> +  MinimalSizeNeeded = OFFSET_OF(EFI_SMM_COMMUNICATE_HEADER,
> Data) +
>                        MAX (sizeof
> (SMRAM_PROFILE_PARAMETER_GET_PROFILE_INFO),
>                             MAX (sizeof
> (SMRAM_PROFILE_PARAMETER_GET_PROFILE_DATA_BY_OFFSET),
>                                  sizeof
> (SMRAM_PROFILE_PARAMETER_RECORDING_STATE)));
> @@ -1190,7 +1189,10 @@ GetSmramProfileData (
>    CommRecordingState->Header.ReturnStatus = (UINT64)-1;
>    CommRecordingState->RecordingState      =
> MEMORY_PROFILE_RECORDING_DISABLE;
> 
> -  CommSize = sizeof (EFI_GUID) + sizeof (UINTN) + CommHeader-
> >MessageLength;
> +  //
> +  // 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);
>    if (EFI_ERROR (Status)) {
>      DEBUG ((EFI_D_ERROR, "SmramProfile: SmmCommunication - %r\n",
> Status));
> @@ -1213,7 +1215,10 @@ GetSmramProfileData (
>      CommRecordingState->Header.ReturnStatus = (UINT64)-1;
>      CommRecordingState->RecordingState      =
> MEMORY_PROFILE_RECORDING_DISABLE;
> 
> -    CommSize = sizeof (EFI_GUID) + sizeof (UINTN) + CommHeader-
> >MessageLength;
> +    //
> +    // 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 +1235,10 @@ GetSmramProfileData (
>    CommGetProfileInfo->Header.ReturnStatus = (UINT64)-1;
>    CommGetProfileInfo->ProfileSize         = 0;
> 
> -  CommSize = sizeof (EFI_GUID) + sizeof (UINTN) + CommHeader-
> >MessageLength;
> +  //
> +  // 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 +1269,10 @@ GetSmramProfileData (
>    CommGetProfileData->Header.DataLength   = sizeof
> (*CommGetProfileData);
>    CommGetProfileData->Header.ReturnStatus = (UINT64)-1;
> 
> -  CommSize = sizeof (EFI_GUID) + sizeof (UINTN) + CommHeader-
> >MessageLength;
> +  //
> +  // 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 +1331,10 @@ GetSmramProfileData (
>      CommRecordingState->Header.ReturnStatus = (UINT64)-1;
>      CommRecordingState->RecordingState      =
> MEMORY_PROFILE_RECORDING_ENABLE;
> 
> -    CommSize = sizeof (EFI_GUID) + sizeof (UINTN) + CommHeader-
> >MessageLength;
> +    //
> +    // 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
> 
> 
> 
> 
> 

Re: [edk2-devel] [PATCH v2 4/6] MdeModulePkg: SmiHandlerProfileInfo: Updated MessageLength calculation

[Wu, Hao A]

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Kun Qin
> Sent: Friday, June 18, 2021 5:03 PM
> To: devel@edk2.groups.io
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>;
> Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>
> Subject: [edk2-devel] [PATCH v2 4/6] MdeModulePkg: SmiHandlerProfileInfo:
> 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.
> 
> 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:
>     v2:
>     - Updated comments by removing "BZ" tags [Hao]


Reviewed-by: Hao A Wu <hao.a.wu@intel.com>

Best Regards,
Hao Wu


> 
>  MdeModulePkg/Application/SmiHandlerProfileInfo/SmiHandlerProfileInfo.c
> | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git
> a/MdeModulePkg/Application/SmiHandlerProfileInfo/SmiHandlerProfileInfo
> .c
> b/MdeModulePkg/Application/SmiHandlerProfileInfo/SmiHandlerProfileInfo
> .c
> index 4153074b7a80..4bfd5946caba 100644
> ---
> a/MdeModulePkg/Application/SmiHandlerProfileInfo/SmiHandlerProfileInfo
> .c
> +++
> b/MdeModulePkg/Application/SmiHandlerProfileInfo/SmiHandlerProfileIn
> +++ fo.c
> @@ -116,7 +116,10 @@ GetSmiHandlerProfileDatabase(
>    CommGetInfo->Header.ReturnStatus = (UINT64)-1;
>    CommGetInfo->DataSize = 0;
> 
> -  CommSize = sizeof(EFI_GUID) + sizeof(UINTN) + CommHeader-
> >MessageLength;
> +  //
> +  // 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);
>    if (EFI_ERROR(Status)) {
>      Print(L"SmiHandlerProfile: SmmCommunication - %r\n", Status); @@ -
> 149,7 +152,10 @@ GetSmiHandlerProfileDatabase(
>    CommGetData->Header.DataLength = sizeof(*CommGetData);
>    CommGetData->Header.ReturnStatus = (UINT64)-1;
> 
> -  CommSize = sizeof(EFI_GUID) + sizeof(UINTN) + CommHeader-
> >MessageLength;
> +  //
> +  // 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;
> 
> --
> 2.31.1.windows.1
> 
> 
> 
> 
> 

Re: [PATCH v2 5/6] MdeModulePkg: SmmLockBoxDxeLib: Updated MessageLength calculation

[Wu, Hao A]

> -----Original Message-----
> From: Kun Qin <kuqin12@gmail.com>
> Sent: Friday, June 18, 2021 5:03 PM
> To: devel@edk2.groups.io
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>;
> Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>
> Subject: [PATCH v2 5/6] MdeModulePkg: SmmLockBoxDxeLib: 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.
> 
> 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:
>     v2:
>     - Newly added in v2
> 
>  MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxDxeLib.c | 23
> ++++++++++----------
>  1 file changed, 11 insertions(+), 12 deletions(-)
> 
> diff --git a/MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxDxeLib.c
> b/MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxDxeLib.c
> index 2cbffe889e1f..66fbe4dd961c 100644
> --- a/MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxDxeLib.c
> +++ b/MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxDxeLib.c
> @@ -79,8 +79,7 @@ LockBoxGetSmmCommBuffer (
>      return mLockBoxSmmCommBuffer;
>    }
> 
> -  MinimalSizeNeeded = sizeof (EFI_GUID) +
> -                      sizeof (UINTN) +
> +  MinimalSizeNeeded = OFFSET_OF (EFI_SMM_COMMUNICATE_HEADER,
> Data) +
>                        MAX (sizeof (EFI_SMM_LOCK_BOX_PARAMETER_SAVE),
>                             MAX (sizeof
> (EFI_SMM_LOCK_BOX_PARAMETER_SET_ATTRIBUTES),
>                                  MAX (sizeof (EFI_SMM_LOCK_BOX_PARAMETER_UPDATE),
> @@ -142,7 +141,7 @@ SaveLockBox (
>    EFI_SMM_COMMUNICATION_PROTOCOL  *SmmCommunication;
>    EFI_SMM_LOCK_BOX_PARAMETER_SAVE *LockBoxParameterSave;
>    EFI_SMM_COMMUNICATE_HEADER      *CommHeader;
> -  UINT8                           TempCommBuffer[sizeof(EFI_GUID) + sizeof(UINTN) +
> sizeof(EFI_SMM_LOCK_BOX_PARAMETER_SAVE)];
> +  UINT8                           TempCommBuffer[OFFSET_OF
> (EFI_SMM_COMMUNICATE_HEADER, Data) +
> sizeof(EFI_SMM_LOCK_BOX_PARAMETER_SAVE)];
>    UINT8                           *CommBuffer;
>    UINTN                           CommSize;
> 
> @@ -182,7 +181,7 @@ SaveLockBox (
>    //
>    // Send command
>    //
> -  CommSize = sizeof(EFI_GUID) + sizeof(UINTN) +
> sizeof(EFI_SMM_LOCK_BOX_PARAMETER_SAVE);
> +  CommSize = OFFSET_OF (EFI_SMM_COMMUNICATE_HEADER, Data) +
> sizeof(EFI_SMM_LOCK_BOX_PARAMETER_SAVE);
>    Status = SmmCommunication->Communicate (
>                                 SmmCommunication,
>                                 &CommBuffer[0],
> @@ -224,7 +223,7 @@ SetLockBoxAttributes (
>    EFI_SMM_COMMUNICATION_PROTOCOL            *SmmCommunication;
>    EFI_SMM_LOCK_BOX_PARAMETER_SET_ATTRIBUTES
> *LockBoxParameterSetAttributes;
>    EFI_SMM_COMMUNICATE_HEADER                *CommHeader;
> -  UINT8                                     TempCommBuffer[sizeof(EFI_GUID) +
> sizeof(UINTN) +
> sizeof(EFI_SMM_LOCK_BOX_PARAMETER_SET_ATTRIBUTES)];
> +  UINT8                                     TempCommBuffer[OFFSET_OF
> (EFI_SMM_COMMUNICATE_HEADER, Data) +
> sizeof(EFI_SMM_LOCK_BOX_PARAMETER_SET_ATTRIBUTES)];
>    UINT8                                     *CommBuffer;
>    UINTN                                     CommSize;
> 
> @@ -264,7 +263,7 @@ SetLockBoxAttributes (
>    //
>    // Send command
>    //
> -  CommSize = sizeof(EFI_GUID) + sizeof(UINTN) +
> sizeof(EFI_SMM_LOCK_BOX_PARAMETER_SET_ATTRIBUTES);
> +  CommSize = OFFSET_OF (EFI_SMM_COMMUNICATE_HEADER, Data) +
> sizeof(EFI_SMM_LOCK_BOX_PARAMETER_SET_ATTRIBUTES);
>    Status = SmmCommunication->Communicate (
>                                 SmmCommunication,
>                                 &CommBuffer[0],
> @@ -314,7 +313,7 @@ UpdateLockBox (
>    EFI_SMM_COMMUNICATION_PROTOCOL    *SmmCommunication;
>    EFI_SMM_LOCK_BOX_PARAMETER_UPDATE *LockBoxParameterUpdate;
>    EFI_SMM_COMMUNICATE_HEADER        *CommHeader;
> -  UINT8                             TempCommBuffer[sizeof(EFI_GUID) + sizeof(UINTN)
> + sizeof(EFI_SMM_LOCK_BOX_PARAMETER_UPDATE)];
> +  UINT8                             TempCommBuffer[OFFSET_OF
> (EFI_SMM_COMMUNICATE_HEADER, Data) +
> sizeof(EFI_SMM_LOCK_BOX_PARAMETER_UPDATE)];
>    UINT8                             *CommBuffer;
>    UINTN                             CommSize;
> 
> @@ -355,7 +354,7 @@ UpdateLockBox (
>    //
>    // Send command
>    //
> -  CommSize = sizeof(EFI_GUID) + sizeof(UINTN) +
> sizeof(EFI_SMM_LOCK_BOX_PARAMETER_UPDATE);
> +  CommSize = OFFSET_OF (EFI_SMM_COMMUNICATE_HEADER, Data) +
> sizeof(EFI_SMM_LOCK_BOX_PARAMETER_UPDATE);
>    Status = SmmCommunication->Communicate (
>                                 SmmCommunication,
>                                 &CommBuffer[0],
> @@ -403,7 +402,7 @@ RestoreLockBox (
>    EFI_SMM_COMMUNICATION_PROTOCOL     *SmmCommunication;
>    EFI_SMM_LOCK_BOX_PARAMETER_RESTORE *LockBoxParameterRestore;
>    EFI_SMM_COMMUNICATE_HEADER         *CommHeader;
> -  UINT8                              TempCommBuffer[sizeof(EFI_GUID) + sizeof(UINTN)
> + sizeof(EFI_SMM_LOCK_BOX_PARAMETER_RESTORE)];
> +  UINT8                              TempCommBuffer[OFFSET_OF
> (EFI_SMM_COMMUNICATE_HEADER, Data) +
> sizeof(EFI_SMM_LOCK_BOX_PARAMETER_RESTORE)];
>    UINT8                              *CommBuffer;
>    UINTN                              CommSize;
> 
> @@ -449,7 +448,7 @@ RestoreLockBox (
>    //
>    // Send command
>    //
> -  CommSize = sizeof(EFI_GUID) + sizeof(UINTN) +
> sizeof(EFI_SMM_LOCK_BOX_PARAMETER_RESTORE);
> +  CommSize = OFFSET_OF (EFI_SMM_COMMUNICATE_HEADER, Data) +
> sizeof(EFI_SMM_LOCK_BOX_PARAMETER_RESTORE);
>    Status = SmmCommunication->Communicate (
>                                 SmmCommunication,
>                                 &CommBuffer[0],
> @@ -488,7 +487,7 @@ RestoreAllLockBoxInPlace (
>    EFI_SMM_COMMUNICATION_PROTOCOL                  *SmmCommunication;
>    EFI_SMM_LOCK_BOX_PARAMETER_RESTORE_ALL_IN_PLACE
> *LockBoxParameterRestoreAllInPlace;
>    EFI_SMM_COMMUNICATE_HEADER                      *CommHeader;
> -  UINT8                                           TempCommBuffer[sizeof(EFI_GUID) +
> sizeof(UINTN) +
> sizeof(EFI_SMM_LOCK_BOX_PARAMETER_RESTORE_ALL_IN_PLACE)];
> +  UINT8                                           TempCommBuffer[OFFSET_OF
> (EFI_SMM_COMMUNICATE_HEADER, Data) +
> sizeof(EFI_SMM_LOCK_BOX_PARAMETER_RESTORE_ALL_IN_PLACE)];
>    UINT8                                           *CommBuffer;
>    UINTN                                           CommSize;
> 
> @@ -518,7 +517,7 @@ RestoreAllLockBoxInPlace (
>    //
>    // Send command
>    //
> -  CommSize = sizeof(EFI_GUID) + sizeof(UINTN) +
> sizeof(EFI_SMM_LOCK_BOX_PARAMETER_RESTORE_ALL_IN_PLACE);
> +  CommSize = OFFSET_OF (EFI_SMM_COMMUNICATE_HEADER, Data) +
> sizeof(EFI_SMM_LOCK_BOX_PARAMETER_RESTORE_ALL_IN_PLACE);


Reviewed-by: Hao A Wu <hao.a.wu@intel.com>

Best Regards,
Hao Wu


>    Status = SmmCommunication->Communicate (
>                                 SmmCommunication,
>                                 &CommBuffer[0],
> --
> 2.31.1.windows.1

Re: [edk2-devel] [PATCH v2 1/6] EDK2 Code First: PI Specification: EFI_MM_COMMUNICATE_HEADER Update

[Laszlo Ersek]

On 06/18/21 11:02, Kun Qin wrote:
> 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: Laszlo Ersek <lersek@redhat.com>
> Cc: Leif Lindholm <leif@nuviainc.com>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> 
> Signed-off-by: Kun Qin <kuqin12@gmail.com>
> ---
> 
> Notes:
>     v2:
>     - Updated change impact analysis regarding SmmLockBoxDxeLib [Hao]
> 
>  BZ3430-SpecChange.md | 90 ++++++++++++++++++++
>  1 file changed, 90 insertions(+)

Placing such a document in the edk2 root directory looks very strange to me.

https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Code-First-Process#tianocore-github

    [...] Specification text changes are held within the affected source
    repository, using the GitHub flavor of markdown, in a file (or split
    across several files) with .md suffix [...]

The wiki does not seem to specify what directory should contain the spec
change document.

Should we create a "CodeFirst" top-level directory?

Thanks,
Laszlo

Re: [edk2-devel] [PATCH v2 1/6] EDK2 Code First: PI Specification: EFI_MM_COMMUNICATE_HEADER Update

[Kun Qin]

I can place this file under "CodeFirst" folder on the next round of 
patch series.

It would be helpful to update the code-first process page so that others 
can be consistent on the process next time.

Thanks,
Kun

On 06/23/2021 03:02, Laszlo Ersek wrote:
> On 06/18/21 11:02, Kun Qin wrote:
>> 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: Laszlo Ersek <lersek@redhat.com>
>> Cc: Leif Lindholm <leif@nuviainc.com>
>> Cc: Hao A Wu <hao.a.wu@intel.com>
>>
>> Signed-off-by: Kun Qin <kuqin12@gmail.com>
>> ---
>>
>> Notes:
>>      v2:
>>      - Updated change impact analysis regarding SmmLockBoxDxeLib [Hao]
>>
>>   BZ3430-SpecChange.md | 90 ++++++++++++++++++++
>>   1 file changed, 90 insertions(+)
> 
> Placing such a document in the edk2 root directory looks very strange to me.
> 
> https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Code-First-Process#tianocore-github
> 
>      [...] Specification text changes are held within the affected source
>      repository, using the GitHub flavor of markdown, in a file (or split
>      across several files) with .md suffix [...]
> 
> The wiki does not seem to specify what directory should contain the spec
> change document.
> 
> Should we create a "CodeFirst" top-level directory?
> 
> Thanks,
> Laszlo
>