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

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

[Kun Qin]

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

In PI Spec v1.7 Errata A, Vol.4, Sec 5.7 MM Communication Protocol, the
MessageLength field of EFI_MM_COMMUNICATE_HEADER (also derived as
EFI_SMM_COMMUNICATE_HEADER) is currently defined as type UINTN.

But this structure, as a generic definition, could be used for both PEI
and DXE MM communication. Thus for a system that supports PEI MM launch,
but operates PEI in 32bit mode and MM foundation in 64bit, the current
EFI_MM_COMMUNICATE_HEADER definition will cause structure parse error due
to UINTN being used.

The suggested change is to make the MessageLength field defined with
definitive size as below:
```
typedef struct {
EFI_GUID  HeaderGuid;
UINT64    MessageLength;
UINT8     Data[ANYSIZE_ARRAY];
} EFI_MM_COMMUNICATE_HEADER;
```

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

Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Zhiguang Liu <zhiguang.liu@intel.com>
Cc: Andrew Fish <afish@apple.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Leif Lindholm <leif@nuviainc.com>

Kun Qin (5):
  EDK2 Code First: PI Specification: EFI_MM_COMMUNICATE_HEADER Update
  MdeModulePkg: PiSmmIpl: Update MessageLength calculation for
    MmCommunicate
  MdeModulePkg: MemoryProfileInfo: Updated MessageLength calculation
  MdeModulePkg: SmiHandlerProfileInfo: Updated MessageLength calculation
  MdePkg: MmCommunication: Extend MessageLength field size to UINT64

 MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.c         | 20 +++--
 MdeModulePkg/Application/SmiHandlerProfileInfo/SmiHandlerProfileInfo.c |  8 +-
 MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c                                 | 13 ++-
 BZ3430-SpecChange.md                                                   | 88 ++++++++++++++++++++
 MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf                               |  1 +
 MdePkg/Include/Protocol/MmCommunication.h                              |  3 +-
 6 files changed, 124 insertions(+), 9 deletions(-)
 create mode 100644 BZ3430-SpecChange.md

-- 
2.31.1.windows.1

[PATCH v1 1/5] 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>

Signed-off-by: Kun Qin <kuqin12@gmail.com>
---
 BZ3430-SpecChange.md | 88 ++++++++++++++++++++
 1 file changed, 88 insertions(+)

diff --git a/BZ3430-SpecChange.md b/BZ3430-SpecChange.md
new file mode 100644
index 000000000000..33a1ffda447b
--- /dev/null
+++ b/BZ3430-SpecChange.md
@@ -0,0 +1,88 @@
+# 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
+```
+
+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
+```
+
+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 v1 2/5] 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>
---
 MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c   | 13 ++++++++++++-
 MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf |  1 +
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
index 599a0cd01d80..9508715fda24 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> // BZ3398
 
 #include "PiSmmCorePrivateData.h"
 
@@ -515,6 +516,7 @@ SmmCommunicationCommunicate (
   EFI_STATUS                  Status;
   EFI_SMM_COMMUNICATE_HEADER  *CommunicateHeader;
   BOOLEAN                     OldInSmm;
+  UINT64                      BZ3398_LongCommSize;
   UINTN                       TempCommSize;
 
   //
@@ -527,7 +529,16 @@ SmmCommunicationCommunicate (
   CommunicateHeader = (EFI_SMM_COMMUNICATE_HEADER *) CommBuffer;
 
   if (CommSize == NULL) {
-    TempCommSize = OFFSET_OF (EFI_SMM_COMMUNICATE_HEADER, Data) + CommunicateHeader->MessageLength;
+    // BZ3398 Starts: Make MessageLength the same size in EFI_MM_COMMUNICATE_HEADER for both IA32 and X64.
+    Status = SafeUint64Add (OFFSET_OF (EFI_SMM_COMMUNICATE_HEADER, Data), CommunicateHeader->MessageLength, &BZ3398_LongCommSize);
+    if (EFI_ERROR (Status)) {
+      return EFI_INVALID_PARAMETER;
+    }
+    Status = SafeUint64ToUintn (BZ3398_LongCommSize, &TempCommSize);
+    if (EFI_ERROR (Status)) {
+      return EFI_INVALID_PARAMETER;
+    }
+    // BZ3398 Ends
   } else {
     TempCommSize = *CommSize;
     //
diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf
index 6109d6b5449c..87142e27fa47 100644
--- a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf
+++ b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf
@@ -46,6 +46,7 @@ [LibraryClasses]
   DxeServicesLib
   PcdLib
   ReportStatusCodeLib
+  SafeIntLib  #BZ3398
 
 [Protocols]
   gEfiSmmBase2ProtocolGuid                      ## PRODUCES
-- 
2.31.1.windows.1

[PATCH v1 3/5] 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>
---
 MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.c b/MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.c
index 191c31068545..39ed8b2e0484 100644
--- a/MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.c
+++ b/MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.c
@@ -1190,7 +1190,9 @@ GetSmramProfileData (
   CommRecordingState->Header.ReturnStatus = (UINT64)-1;
   CommRecordingState->RecordingState      = MEMORY_PROFILE_RECORDING_DISABLE;
 
-  CommSize = sizeof (EFI_GUID) + sizeof (UINTN) + CommHeader->MessageLength;
+  // BZ3398: Make MessageLength the same size in EFI_MM_COMMUNICATE_HEADER for both IA32 and X64.
+  // 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,9 @@ GetSmramProfileData (
     CommRecordingState->Header.ReturnStatus = (UINT64)-1;
     CommRecordingState->RecordingState      = MEMORY_PROFILE_RECORDING_DISABLE;
 
-    CommSize = sizeof (EFI_GUID) + sizeof (UINTN) + CommHeader->MessageLength;
+    // BZ3398: Make MessageLength the same size in EFI_MM_COMMUNICATE_HEADER for both IA32 and X64.
+    // 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 +1234,9 @@ GetSmramProfileData (
   CommGetProfileInfo->Header.ReturnStatus = (UINT64)-1;
   CommGetProfileInfo->ProfileSize         = 0;
 
-  CommSize = sizeof (EFI_GUID) + sizeof (UINTN) + CommHeader->MessageLength;
+  // BZ3398: Make MessageLength the same size in EFI_MM_COMMUNICATE_HEADER for both IA32 and X64.
+  // 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 +1267,9 @@ GetSmramProfileData (
   CommGetProfileData->Header.DataLength   = sizeof (*CommGetProfileData);
   CommGetProfileData->Header.ReturnStatus = (UINT64)-1;
 
-  CommSize = sizeof (EFI_GUID) + sizeof (UINTN) + CommHeader->MessageLength;
+  // BZ3398: Make MessageLength the same size in EFI_MM_COMMUNICATE_HEADER for both IA32 and X64.
+  // 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 +1328,9 @@ GetSmramProfileData (
     CommRecordingState->Header.ReturnStatus = (UINT64)-1;
     CommRecordingState->RecordingState      = MEMORY_PROFILE_RECORDING_ENABLE;
 
-    CommSize = sizeof (EFI_GUID) + sizeof (UINTN) + CommHeader->MessageLength;
+    // BZ3398: Make MessageLength the same size in EFI_MM_COMMUNICATE_HEADER for both IA32 and X64.
+    // 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 v1 4/5] 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>
---
 MdeModulePkg/Application/SmiHandlerProfileInfo/SmiHandlerProfileInfo.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/MdeModulePkg/Application/SmiHandlerProfileInfo/SmiHandlerProfileInfo.c b/MdeModulePkg/Application/SmiHandlerProfileInfo/SmiHandlerProfileInfo.c
index 4153074b7a80..56d80d1a9ce1 100644
--- a/MdeModulePkg/Application/SmiHandlerProfileInfo/SmiHandlerProfileInfo.c
+++ b/MdeModulePkg/Application/SmiHandlerProfileInfo/SmiHandlerProfileInfo.c
@@ -116,7 +116,9 @@ GetSmiHandlerProfileDatabase(
   CommGetInfo->Header.ReturnStatus = (UINT64)-1;
   CommGetInfo->DataSize = 0;
 
-  CommSize = sizeof(EFI_GUID) + sizeof(UINTN) + CommHeader->MessageLength;
+  // BZ3398: Make MessageLength the same size in EFI_MM_COMMUNICATE_HEADER for both IA32 and X64.
+  // 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 +151,9 @@ GetSmiHandlerProfileDatabase(
   CommGetData->Header.DataLength = sizeof(*CommGetData);
   CommGetData->Header.ReturnStatus = (UINT64)-1;
 
-  CommSize = sizeof(EFI_GUID) + sizeof(UINTN) + CommHeader->MessageLength;
+  // BZ3398: Make MessageLength the same size in EFI_MM_COMMUNICATE_HEADER for both IA32 and X64.
+  // 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 v1 5/5] 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>
---
 MdePkg/Include/Protocol/MmCommunication.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/MdePkg/Include/Protocol/MmCommunication.h b/MdePkg/Include/Protocol/MmCommunication.h
index 34c3e2b5a9e3..24d88d3e0b68 100644
--- a/MdePkg/Include/Protocol/MmCommunication.h
+++ b/MdePkg/Include/Protocol/MmCommunication.h
@@ -26,7 +26,8 @@ typedef struct {
   ///
   /// Describes the size of Data (in bytes) and does not include the size of the header.
   ///
-  UINTN     MessageLength;
+  /// BZ3398: Make MessageLength the same size in EFI_MM_COMMUNICATE_HEADER for both IA32 and X64.
+  UINT64    MessageLength;
   ///
   /// Designates an array of bytes that is MessageLength in size.
   ///
-- 
2.31.1.windows.1

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

[Wu, Hao A]

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Kun Qin
> Sent: Thursday, June 10, 2021 9:43 AM
> 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>;
> Andrew Fish <afish@apple.com>; Laszlo Ersek <lersek@redhat.com>; Leif
> Lindholm <leif@nuviainc.com>
> Subject: [edk2-devel] [PATCH v1 1/5] EDK2 Code First: PI Specification:
> EFI_MM_COMMUNICATE_HEADER Update
> 
> 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>
> 
> Signed-off-by: Kun Qin <kuqin12@gmail.com>
> ---
>  BZ3430-SpecChange.md | 88 ++++++++++++++++++++
>  1 file changed, 88 insertions(+)
> 
> diff --git a/BZ3430-SpecChange.md b/BZ3430-SpecChange.md new file mode
> 100644 index 000000000000..33a1ffda447b
> --- /dev/null
> +++ b/BZ3430-SpecChange.md
> @@ -0,0 +1,88 @@
> +# 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
> +```
> +
> +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


Hello Kun,

Sorry for a question.
I am not sure why the current codes in file SmmLockBoxDxeLib.c will work properly after the UINTN -> UINT64 change.

For example:
LockBoxGetSmmCommBuffer():
  MinimalSizeNeeded = sizeof (EFI_GUID) +
                      sizeof (UINTN) +
                      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),
                                     MAX (sizeof (EFI_SMM_LOCK_BOX_PARAMETER_RESTORE),
                                          sizeof (EFI_SMM_LOCK_BOX_PARAMETER_RESTORE_ALL_IN_PLACE)))));

SaveLockBox():
  UINT8                           TempCommBuffer[sizeof(EFI_GUID) + sizeof(UINTN) + sizeof(EFI_SMM_LOCK_BOX_PARAMETER_SAVE)];

Is the series missed changes for SmmLockBoxDxeLib or I got something wrong?

Best Regards,
Hao Wu


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

Re: [PATCH v1 2/5] MdeModulePkg: PiSmmIpl: Update MessageLength calculation for MmCommunicate

[Wu, Hao A]

A couple of minor comments below:


> -----Original Message-----
> From: Kun Qin <kuqin12@gmail.com>
> Sent: Thursday, June 10, 2021 9:43 AM
> 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 v1 2/5] MdeModulePkg: PiSmmIpl: Update MessageLength
> calculation for MmCommunicate
> 
> 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>
> ---
>  MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c   | 13 ++++++++++++-
>  MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf |  1 +
>  2 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
> b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
> index 599a0cd01d80..9508715fda24 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> // BZ3398


I suggest to remove the comment "// BZ3398" here.

I think users can use the 'blame' feature of the version control systems
together with the commit log message to find out the information.


> 
>  #include "PiSmmCorePrivateData.h"
> 
> @@ -515,6 +516,7 @@ SmmCommunicationCommunicate (
>    EFI_STATUS                  Status;
>    EFI_SMM_COMMUNICATE_HEADER  *CommunicateHeader;
>    BOOLEAN                     OldInSmm;
> +  UINT64                      BZ3398_LongCommSize;


Suggest to drop the "BZ3398_" prefix for the variable name.


>    UINTN                       TempCommSize;
> 
>    //
> @@ -527,7 +529,16 @@ SmmCommunicationCommunicate (
>    CommunicateHeader = (EFI_SMM_COMMUNICATE_HEADER *)
> CommBuffer;
> 
>    if (CommSize == NULL) {
> -    TempCommSize = OFFSET_OF (EFI_SMM_COMMUNICATE_HEADER, Data)
> + CommunicateHeader->MessageLength;
> +    // BZ3398 Starts: Make MessageLength the same size in
> EFI_MM_COMMUNICATE_HEADER for both IA32 and X64.


Suggest to drop the "// BZ3398 Starts:" and "// BZ3398 Ends" comments pair here.

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

Best Regards,
Hao Wu


> +    Status = SafeUint64Add (OFFSET_OF
> (EFI_SMM_COMMUNICATE_HEADER, Data), CommunicateHeader-
> >MessageLength, &BZ3398_LongCommSize);
> +    if (EFI_ERROR (Status)) {
> +      return EFI_INVALID_PARAMETER;
> +    }
> +    Status = SafeUint64ToUintn (BZ3398_LongCommSize, &TempCommSize);
> +    if (EFI_ERROR (Status)) {
> +      return EFI_INVALID_PARAMETER;
> +    }
> +    // BZ3398 Ends
>    } else {
>      TempCommSize = *CommSize;
>      //
> diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf
> b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf
> index 6109d6b5449c..87142e27fa47 100644
> --- a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf
> +++ b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf
> @@ -46,6 +46,7 @@ [LibraryClasses]
>    DxeServicesLib
>    PcdLib
>    ReportStatusCodeLib
> +  SafeIntLib  #BZ3398
> 
>  [Protocols]
>    gEfiSmmBase2ProtocolGuid                      ## PRODUCES
> --
> 2.31.1.windows.1

Re: [PATCH v1 3/5] MdeModulePkg: MemoryProfileInfo: Updated MessageLength calculation

[Wu, Hao A]

> -----Original Message-----
> From: Kun Qin <kuqin12@gmail.com>
> Sent: Thursday, June 10, 2021 9:43 AM
> To: devel@edk2.groups.io
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>
> Subject: [PATCH v1 3/5] 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.


I think there is one missing place in function GetSmramProfileData():

  MinimalSizeNeeded = sizeof (EFI_GUID) +
                      sizeof (UINTN) +
                      MAX (sizeof (SMRAM_PROFILE_PARAMETER_GET_PROFILE_INFO),
                           MAX (sizeof (SMRAM_PROFILE_PARAMETER_GET_PROFILE_DATA_BY_OFFSET),
                                sizeof (SMRAM_PROFILE_PARAMETER_RECORDING_STATE)));

More inline comments below:


> 
> 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>
> ---
>  MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.c | 20
> +++++++++++++++-----
>  1 file changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git
> a/MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.c
> b/MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.c
> index 191c31068545..39ed8b2e0484 100644
> --- a/MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.c
> +++
> b/MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.c
> @@ -1190,7 +1190,9 @@ GetSmramProfileData (
>    CommRecordingState->Header.ReturnStatus = (UINT64)-1;
>    CommRecordingState->RecordingState      =
> MEMORY_PROFILE_RECORDING_DISABLE;
> 
> -  CommSize = sizeof (EFI_GUID) + sizeof (UINTN) + CommHeader-
> >MessageLength;
> +  // BZ3398: Make MessageLength the same size in
> EFI_MM_COMMUNICATE_HEADER for both IA32 and X64.
> +  // The CommHeader->MessageLength contains a definitive value, thus
> UINTN cast is safe here.


Please help to drop the explicit mention of BZ3398 in the comment.
How about using:
  //
  // The CommHeader->MessageLength contains a definitive value, thus UINTN cast is safe here.
  //

There are 4 more similar cases below.

Best Regards,
Hao Wu


> +  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,9 @@ GetSmramProfileData (
>      CommRecordingState->Header.ReturnStatus = (UINT64)-1;
>      CommRecordingState->RecordingState      =
> MEMORY_PROFILE_RECORDING_DISABLE;
> 
> -    CommSize = sizeof (EFI_GUID) + sizeof (UINTN) + CommHeader-
> >MessageLength;
> +    // BZ3398: Make MessageLength the same size in
> EFI_MM_COMMUNICATE_HEADER for both IA32 and X64.
> +    // 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 +1234,9 @@ GetSmramProfileData (
>    CommGetProfileInfo->Header.ReturnStatus = (UINT64)-1;
>    CommGetProfileInfo->ProfileSize         = 0;
> 
> -  CommSize = sizeof (EFI_GUID) + sizeof (UINTN) + CommHeader-
> >MessageLength;
> +  // BZ3398: Make MessageLength the same size in
> EFI_MM_COMMUNICATE_HEADER for both IA32 and X64.
> +  // 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 +1267,9 @@ GetSmramProfileData (
>    CommGetProfileData->Header.DataLength   = sizeof
> (*CommGetProfileData);
>    CommGetProfileData->Header.ReturnStatus = (UINT64)-1;
> 
> -  CommSize = sizeof (EFI_GUID) + sizeof (UINTN) + CommHeader-
> >MessageLength;
> +  // BZ3398: Make MessageLength the same size in
> EFI_MM_COMMUNICATE_HEADER for both IA32 and X64.
> +  // 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 +1328,9 @@ GetSmramProfileData (
>      CommRecordingState->Header.ReturnStatus = (UINT64)-1;
>      CommRecordingState->RecordingState      =
> MEMORY_PROFILE_RECORDING_ENABLE;
> 
> -    CommSize = sizeof (EFI_GUID) + sizeof (UINTN) + CommHeader-
> >MessageLength;
> +    // BZ3398: Make MessageLength the same size in
> EFI_MM_COMMUNICATE_HEADER for both IA32 and X64.
> +    // 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: [PATCH v1 4/5] MdeModulePkg: SmiHandlerProfileInfo: Updated MessageLength calculation

[Wu, Hao A]

> -----Original Message-----
> From: Kun Qin <kuqin12@gmail.com>
> Sent: Thursday, June 10, 2021 9:43 AM
> 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 v1 4/5] 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>
> ---
>  MdeModulePkg/Application/SmiHandlerProfileInfo/SmiHandlerProfileInfo.c
> | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git
> a/MdeModulePkg/Application/SmiHandlerProfileInfo/SmiHandlerProfileInfo
> .c
> b/MdeModulePkg/Application/SmiHandlerProfileInfo/SmiHandlerProfileInfo
> .c
> index 4153074b7a80..56d80d1a9ce1 100644
> ---
> a/MdeModulePkg/Application/SmiHandlerProfileInfo/SmiHandlerProfileInfo
> .c
> +++
> b/MdeModulePkg/Application/SmiHandlerProfileInfo/SmiHandlerProfileIn
> +++ fo.c
> @@ -116,7 +116,9 @@ GetSmiHandlerProfileDatabase(
>    CommGetInfo->Header.ReturnStatus = (UINT64)-1;
>    CommGetInfo->DataSize = 0;
> 
> -  CommSize = sizeof(EFI_GUID) + sizeof(UINTN) + CommHeader-
> >MessageLength;
> +  // BZ3398: Make MessageLength the same size in
> EFI_MM_COMMUNICATE_HEADER for both IA32 and X64.


Similar comments as in patch 3/5.

Best Regards,
Hao Wu


> +  // 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 +151,9 @@ GetSmiHandlerProfileDatabase(
>    CommGetData->Header.DataLength = sizeof(*CommGetData);
>    CommGetData->Header.ReturnStatus = (UINT64)-1;
> 
> -  CommSize = sizeof(EFI_GUID) + sizeof(UINTN) + CommHeader-
> >MessageLength;
> +  // BZ3398: Make MessageLength the same size in
> EFI_MM_COMMUNICATE_HEADER for both IA32 and X64.
> +  // 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 v1 3/5] MdeModulePkg: MemoryProfileInfo: Updated MessageLength calculation

[Kun Qin]

Hi Hao,

Thanks for pointing out the missing place. Will update this accordingly.

This patch series needs a PI spec update, I thought I should mark all 
changes with BZ#### before the spec update is taken. But I can drop them 
for the next patch version.

Regards,
Kun

On 06/11/2021 00:46, Wu, Hao A wrote:
>> -----Original Message-----
>> From: Kun Qin <kuqin12@gmail.com>
>> Sent: Thursday, June 10, 2021 9:43 AM
>> To: devel@edk2.groups.io
>> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>
>> Subject: [PATCH v1 3/5] 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.
> 
> 
> I think there is one missing place in function GetSmramProfileData():
> 
>    MinimalSizeNeeded = sizeof (EFI_GUID) +
>                        sizeof (UINTN) +
>                        MAX (sizeof (SMRAM_PROFILE_PARAMETER_GET_PROFILE_INFO),
>                             MAX (sizeof (SMRAM_PROFILE_PARAMETER_GET_PROFILE_DATA_BY_OFFSET),
>                                  sizeof (SMRAM_PROFILE_PARAMETER_RECORDING_STATE)));
> 
> More inline comments below:
> 
> 
>>
>> 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>
>> ---
>>   MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.c | 20
>> +++++++++++++++-----
>>   1 file changed, 15 insertions(+), 5 deletions(-)
>>
>> diff --git
>> a/MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.c
>> b/MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.c
>> index 191c31068545..39ed8b2e0484 100644
>> --- a/MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.c
>> +++
>> b/MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.c
>> @@ -1190,7 +1190,9 @@ GetSmramProfileData (
>>     CommRecordingState->Header.ReturnStatus = (UINT64)-1;
>>     CommRecordingState->RecordingState      =
>> MEMORY_PROFILE_RECORDING_DISABLE;
>>
>> -  CommSize = sizeof (EFI_GUID) + sizeof (UINTN) + CommHeader-
>>> MessageLength;
>> +  // BZ3398: Make MessageLength the same size in
>> EFI_MM_COMMUNICATE_HEADER for both IA32 and X64.
>> +  // The CommHeader->MessageLength contains a definitive value, thus
>> UINTN cast is safe here.
> 
> 
> Please help to drop the explicit mention of BZ3398 in the comment.
> How about using:
>    //
>    // The CommHeader->MessageLength contains a definitive value, thus UINTN cast is safe here.
>    //
> 
> There are 4 more similar cases below.
> 
> Best Regards,
> Hao Wu
> 
> 
>> +  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,9 @@ GetSmramProfileData (
>>       CommRecordingState->Header.ReturnStatus = (UINT64)-1;
>>       CommRecordingState->RecordingState      =
>> MEMORY_PROFILE_RECORDING_DISABLE;
>>
>> -    CommSize = sizeof (EFI_GUID) + sizeof (UINTN) + CommHeader-
>>> MessageLength;
>> +    // BZ3398: Make MessageLength the same size in
>> EFI_MM_COMMUNICATE_HEADER for both IA32 and X64.
>> +    // 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 +1234,9 @@ GetSmramProfileData (
>>     CommGetProfileInfo->Header.ReturnStatus = (UINT64)-1;
>>     CommGetProfileInfo->ProfileSize         = 0;
>>
>> -  CommSize = sizeof (EFI_GUID) + sizeof (UINTN) + CommHeader-
>>> MessageLength;
>> +  // BZ3398: Make MessageLength the same size in
>> EFI_MM_COMMUNICATE_HEADER for both IA32 and X64.
>> +  // 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 +1267,9 @@ GetSmramProfileData (
>>     CommGetProfileData->Header.DataLength   = sizeof
>> (*CommGetProfileData);
>>     CommGetProfileData->Header.ReturnStatus = (UINT64)-1;
>>
>> -  CommSize = sizeof (EFI_GUID) + sizeof (UINTN) + CommHeader-
>>> MessageLength;
>> +  // BZ3398: Make MessageLength the same size in
>> EFI_MM_COMMUNICATE_HEADER for both IA32 and X64.
>> +  // 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 +1328,9 @@ GetSmramProfileData (
>>       CommRecordingState->Header.ReturnStatus = (UINT64)-1;
>>       CommRecordingState->RecordingState      =
>> MEMORY_PROFILE_RECORDING_ENABLE;
>>
>> -    CommSize = sizeof (EFI_GUID) + sizeof (UINTN) + CommHeader-
>>> MessageLength;
>> +    // BZ3398: Make MessageLength the same size in
>> EFI_MM_COMMUNICATE_HEADER for both IA32 and X64.
>> +    // 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 v1 3/5] 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: Saturday, June 12, 2021 5:30 AM
> To: Wu, Hao A <hao.a.wu@intel.com>; devel@edk2.groups.io
> Cc: Wang, Jian J <jian.j.wang@intel.com>
> Subject: Re: [edk2-devel] [PATCH v1 3/5] MdeModulePkg: MemoryProfileInfo:
> Updated MessageLength calculation
> 
> Hi Hao,
> 
> Thanks for pointing out the missing place. Will update this accordingly.
> 
> This patch series needs a PI spec update, I thought I should mark all changes
> with BZ#### before the spec update is taken. But I can drop them for the next
> patch version.


Thanks a lot.
Please also help to check my other comments sent previously for other patches in the series.

Best Regards,
Hao Wu


> 
> Regards,
> Kun
> 
> On 06/11/2021 00:46, Wu, Hao A wrote:
> >> -----Original Message-----
> >> From: Kun Qin <kuqin12@gmail.com>
> >> Sent: Thursday, June 10, 2021 9:43 AM
> >> To: devel@edk2.groups.io
> >> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> >> <hao.a.wu@intel.com>
> >> Subject: [PATCH v1 3/5] 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.
> >
> >
> > I think there is one missing place in function GetSmramProfileData():
> >
> >    MinimalSizeNeeded = sizeof (EFI_GUID) +
> >                        sizeof (UINTN) +
> >                        MAX (sizeof (SMRAM_PROFILE_PARAMETER_GET_PROFILE_INFO),
> >                             MAX (sizeof
> (SMRAM_PROFILE_PARAMETER_GET_PROFILE_DATA_BY_OFFSET),
> >                                  sizeof
> > (SMRAM_PROFILE_PARAMETER_RECORDING_STATE)));
> >
> > More inline comments below:
> >
> >
> >>
> >> 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>
> >> ---
> >>   MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.c | 20
> >> +++++++++++++++-----
> >>   1 file changed, 15 insertions(+), 5 deletions(-)
> >>
> >> diff --git
> >> a/MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.c
> >> b/MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.c
> >> index 191c31068545..39ed8b2e0484 100644
> >> --- a/MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.c
> >> +++
> >> b/MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.c
> >> @@ -1190,7 +1190,9 @@ GetSmramProfileData (
> >>     CommRecordingState->Header.ReturnStatus = (UINT64)-1;
> >>     CommRecordingState->RecordingState      =
> >> MEMORY_PROFILE_RECORDING_DISABLE;
> >>
> >> -  CommSize = sizeof (EFI_GUID) + sizeof (UINTN) + CommHeader-
> >>> MessageLength;
> >> +  // BZ3398: Make MessageLength the same size in
> >> EFI_MM_COMMUNICATE_HEADER for both IA32 and X64.
> >> +  // The CommHeader->MessageLength contains a definitive value, thus
> >> UINTN cast is safe here.
> >
> >
> > Please help to drop the explicit mention of BZ3398 in the comment.
> > How about using:
> >    //
> >    // The CommHeader->MessageLength contains a definitive value, thus
> UINTN cast is safe here.
> >    //
> >
> > There are 4 more similar cases below.
> >
> > Best Regards,
> > Hao Wu
> >
> >
> >> +  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,9 @@ GetSmramProfileData (
> >>       CommRecordingState->Header.ReturnStatus = (UINT64)-1;
> >>       CommRecordingState->RecordingState      =
> >> MEMORY_PROFILE_RECORDING_DISABLE;
> >>
> >> -    CommSize = sizeof (EFI_GUID) + sizeof (UINTN) + CommHeader-
> >>> MessageLength;
> >> +    // BZ3398: Make MessageLength the same size in
> >> EFI_MM_COMMUNICATE_HEADER for both IA32 and X64.
> >> +    // 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 +1234,9 @@ GetSmramProfileData (
> >>     CommGetProfileInfo->Header.ReturnStatus = (UINT64)-1;
> >>     CommGetProfileInfo->ProfileSize         = 0;
> >>
> >> -  CommSize = sizeof (EFI_GUID) + sizeof (UINTN) + CommHeader-
> >>> MessageLength;
> >> +  // BZ3398: Make MessageLength the same size in
> >> EFI_MM_COMMUNICATE_HEADER for both IA32 and X64.
> >> +  // 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 +1267,9 @@ GetSmramProfileData (
> >>     CommGetProfileData->Header.DataLength   = sizeof
> >> (*CommGetProfileData);
> >>     CommGetProfileData->Header.ReturnStatus = (UINT64)-1;
> >>
> >> -  CommSize = sizeof (EFI_GUID) + sizeof (UINTN) + CommHeader-
> >>> MessageLength;
> >> +  // BZ3398: Make MessageLength the same size in
> >> EFI_MM_COMMUNICATE_HEADER for both IA32 and X64.
> >> +  // 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 +1328,9 @@ GetSmramProfileData (
> >>       CommRecordingState->Header.ReturnStatus = (UINT64)-1;
> >>       CommRecordingState->RecordingState      =
> >> MEMORY_PROFILE_RECORDING_ENABLE;
> >>
> >> -    CommSize = sizeof (EFI_GUID) + sizeof (UINTN) + CommHeader-
> >>> MessageLength;
> >> +    // BZ3398: Make MessageLength the same size in
> >> EFI_MM_COMMUNICATE_HEADER for both IA32 and X64.
> >> +    // 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 v1 1/5] EDK2 Code First: PI Specification: EFI_MM_COMMUNICATE_HEADER Update

[Kun Qin]

Hi Hao,

Sorry that I missed comments for this patch earlier. You are correct. I 
only inspected SmmLockBoxPeiLib. The PEI instance handled mode switch 
with ```OFFSET_OF ``` function. But DXE instance still have a few use 
cases that will be impacted.

I will update this read me file and add a code change patch for this 
library in v2. Thanks for pointing this out.

Regards,
Kun

On 06/11/2021 00:46, Wu, Hao A wrote:
>> -----Original Message-----
>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Kun Qin
>> Sent: Thursday, June 10, 2021 9:43 AM
>> 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>;
>> Andrew Fish <afish@apple.com>; Laszlo Ersek <lersek@redhat.com>; Leif
>> Lindholm <leif@nuviainc.com>
>> Subject: [edk2-devel] [PATCH v1 1/5] EDK2 Code First: PI Specification:
>> EFI_MM_COMMUNICATE_HEADER Update
>>
>> 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>
>>
>> Signed-off-by: Kun Qin <kuqin12@gmail.com>
>> ---
>>   BZ3430-SpecChange.md | 88 ++++++++++++++++++++
>>   1 file changed, 88 insertions(+)
>>
>> diff --git a/BZ3430-SpecChange.md b/BZ3430-SpecChange.md new file mode
>> 100644 index 000000000000..33a1ffda447b
>> --- /dev/null
>> +++ b/BZ3430-SpecChange.md
>> @@ -0,0 +1,88 @@
>> +# 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
>> +```
>> +
>> +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
> 
> 
> Hello Kun,
> 
> Sorry for a question.
> I am not sure why the current codes in file SmmLockBoxDxeLib.c will work properly after the UINTN -> UINT64 change.
> 
> For example:
> LockBoxGetSmmCommBuffer():
>    MinimalSizeNeeded = sizeof (EFI_GUID) +
>                        sizeof (UINTN) +
>                        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),
>                                       MAX (sizeof (EFI_SMM_LOCK_BOX_PARAMETER_RESTORE),
>                                            sizeof (EFI_SMM_LOCK_BOX_PARAMETER_RESTORE_ALL_IN_PLACE)))));
> 
> SaveLockBox():
>    UINT8                           TempCommBuffer[sizeof(EFI_GUID) + sizeof(UINTN) + sizeof(EFI_SMM_LOCK_BOX_PARAMETER_SAVE)];
> 
> Is the series missed changes for SmmLockBoxDxeLib or I got something wrong?
> 
> Best Regards,
> Hao Wu
> 
> 
>> 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
>> +```
>> +
>> +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
>>
>>
>>
>> 
>>
> 

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

[Wu, Hao A]

Thanks Kun,

I am a little concerned on whether there will be other missing cases that are not covered by this patch series.

I am also wondering is there any detection can be made to warn the cases that code modification should be made after this structure update.
Otherwise, it will be the burden for the platform owners to review the platform codes following your guide mentioned in this patch.

Hoping others can provide some inputs on this.

Best Regards,
Hao Wu

> -----Original Message-----
> From: Kun Qin <kuqin12@gmail.com>
> Sent: Wednesday, June 16, 2021 4:51 AM
> To: Wu, Hao A <hao.a.wu@intel.com>; 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>;
> Andrew Fish <afish@apple.com>; Laszlo Ersek <lersek@redhat.com>; Leif
> Lindholm <leif@nuviainc.com>
> Subject: Re: [edk2-devel] [PATCH v1 1/5] EDK2 Code First: PI Specification:
> EFI_MM_COMMUNICATE_HEADER Update
> 
> Hi Hao,
> 
> Sorry that I missed comments for this patch earlier. You are correct. I only
> inspected SmmLockBoxPeiLib. The PEI instance handled mode switch with
> ```OFFSET_OF ``` function. But DXE instance still have a few use cases that will
> be impacted.
> 
> I will update this read me file and add a code change patch for this library in
> v2. Thanks for pointing this out.
> 
> Regards,
> Kun
> 
> On 06/11/2021 00:46, Wu, Hao A wrote:
> >> -----Original Message-----
> >> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Kun
> >> Qin
> >> Sent: Thursday, June 10, 2021 9:43 AM
> >> 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>;
> >> Andrew Fish <afish@apple.com>; Laszlo Ersek <lersek@redhat.com>; Leif
> >> Lindholm <leif@nuviainc.com>
> >> Subject: [edk2-devel] [PATCH v1 1/5] EDK2 Code First: PI Specification:
> >> EFI_MM_COMMUNICATE_HEADER Update
> >>
> >> 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>
> >>
> >> Signed-off-by: Kun Qin <kuqin12@gmail.com>
> >> ---
> >>   BZ3430-SpecChange.md | 88 ++++++++++++++++++++
> >>   1 file changed, 88 insertions(+)
> >>
> >> diff --git a/BZ3430-SpecChange.md b/BZ3430-SpecChange.md new file
> >> mode
> >> 100644 index 000000000000..33a1ffda447b
> >> --- /dev/null
> >> +++ b/BZ3430-SpecChange.md
> >> @@ -0,0 +1,88 @@
> >> +# 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
> >> +```
> >> +
> >> +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
> >
> >
> > Hello Kun,
> >
> > Sorry for a question.
> > I am not sure why the current codes in file SmmLockBoxDxeLib.c will work
> properly after the UINTN -> UINT64 change.
> >
> > For example:
> > LockBoxGetSmmCommBuffer():
> >    MinimalSizeNeeded = sizeof (EFI_GUID) +
> >                        sizeof (UINTN) +
> >                        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),
> >                                       MAX (sizeof
> (EFI_SMM_LOCK_BOX_PARAMETER_RESTORE),
> >                                            sizeof
> > (EFI_SMM_LOCK_BOX_PARAMETER_RESTORE_ALL_IN_PLACE)))));
> >
> > SaveLockBox():
> >    UINT8                           TempCommBuffer[sizeof(EFI_GUID) + sizeof(UINTN)
> + sizeof(EFI_SMM_LOCK_BOX_PARAMETER_SAVE)];
> >
> > Is the series missed changes for SmmLockBoxDxeLib or I got something
> wrong?
> >
> > Best Regards,
> > Hao Wu
> >
> >
> >> 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
> >> +```
> >> +
> >> +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
> >>
> >>
> >>
> >> 
> >>
> >

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

[Marvin Häuser]

Good day,

May I ask about two small things?

1) Was there any rationale as to e.g. code compatibility with choosing 
UINT64 for the data length? I understand that this is the maximum of the 
two as of currently, but I wonder whether a message length that exceeds 
the UINT32 range (4 GB!) can possibly be considered sane or a good idea.

2) Is it feasible yet with the current set of supported compilers to 
support flexible arrays?

Thank you for your work!

Best regards,
Marvin

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

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

[Kun Qin]

Hi Marvin,

Thanks for reaching out. Please see my comment inline.

Regards,
Kun

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

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

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

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

[Marvin Häuser]

Hey Kun,

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

I didn't see anything missed, but that is part of the point. If it was 
UINT32, no such validation would be required. Also I feel like in 
high-security scenarios, you would have a cap (that is well below 4 GB I 
imagine) anyway past which you reject transmission for looking insane. I 
totally understand it's a tough choice to reduce the capabilities and to 
go with a capacity less than what is possible, but I do not think 
current transmission realistically exceed a very few megabytes? This 
would still allow for incredible headroom.

>>
>> 2) Is it feasible yet with the current set of supported compilers to 
>> support flexible arrays?
> My impression is that flexible arrays are already supported (as seen 
> in UnitTestFrameworkPkg/PrivateInclude/UnitTestFrameworkTypes.h). 
> Please correct me if I am wrong.
>
> Would you mind letting me know why this is applicable here? We are 
> trying to seek ideas on how to catch developer mistakes caused by this 
> change. So any input is appreciated.

Huh, interesting. Last time I tried I was told about incompatibilities 
with MSVC, but I know some have been dropped since then (2005 and 2008 
if I recall correctly?), so that'd be great to allow globally. I feel 
like if the structure is modified anyway, it should probably get a 
trailing flexible array over the 1-sized hack. What do you think?

Best regards,
Marvin

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

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

[Laszlo Ersek]

On 06/18/21 11:37, Marvin Häuser wrote:
> On 16.06.21 22:58, Kun Qin wrote:
>> On 06/16/2021 00:02, Marvin Häuser wrote:

>>> 2) Is it feasible yet with the current set of supported compilers to
>>> support flexible arrays?
>> My impression is that flexible arrays are already supported (as seen
>> in UnitTestFrameworkPkg/PrivateInclude/UnitTestFrameworkTypes.h).
>> Please correct me if I am wrong.
>>
>> Would you mind letting me know why this is applicable here? We are
>> trying to seek ideas on how to catch developer mistakes caused by this
>> change. So any input is appreciated.
> 
> Huh, interesting. Last time I tried I was told about incompatibilities
> with MSVC, but I know some have been dropped since then (2005 and 2008
> if I recall correctly?), so that'd be great to allow globally.

I too am surprised to see
"UnitTestFrameworkPkg/PrivateInclude/UnitTestFrameworkTypes.h". The
flexible array member is a C99 feature, and I didn't even know that we
disallowed it for the sake of particular VS toolchains -- I thought we
had a more general reason than just "not supported by VS versions X and Y".

The behavior of OFFSET_OF() would be interesting -- the OFFSET_OF()
macro definition for non-gcc / non-clang:

#define OFFSET_OF(TYPE, Field) ((UINTN) &(((TYPE *)0)->Field))

borders on undefined behavior as far as I can tell, so its behavior is
totally up to the compiler. It works thus far okay on Visual Studio, but
I couldn't say if it extended correctly to flexible array members.

Thanks
Laszlo

> I feel
> like if the structure is modified anyway, it should probably get a
> trailing flexible array over the 1-sized hack. What do you think?

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

[Marvin Häuser]

On 22.06.21 17:34, Laszlo Ersek wrote:
> On 06/18/21 11:37, Marvin Häuser wrote:
>> On 16.06.21 22:58, Kun Qin wrote:
>>> On 06/16/2021 00:02, Marvin Häuser wrote:
>>>> 2) Is it feasible yet with the current set of supported compilers to
>>>> support flexible arrays?
>>> My impression is that flexible arrays are already supported (as seen
>>> in UnitTestFrameworkPkg/PrivateInclude/UnitTestFrameworkTypes.h).
>>> Please correct me if I am wrong.
>>>
>>> Would you mind letting me know why this is applicable here? We are
>>> trying to seek ideas on how to catch developer mistakes caused by this
>>> change. So any input is appreciated.
>> Huh, interesting. Last time I tried I was told about incompatibilities
>> with MSVC, but I know some have been dropped since then (2005 and 2008
>> if I recall correctly?), so that'd be great to allow globally.
> I too am surprised to see
> "UnitTestFrameworkPkg/PrivateInclude/UnitTestFrameworkTypes.h". The
> flexible array member is a C99 feature, and I didn't even know that we
> disallowed it for the sake of particular VS toolchains -- I thought we
> had a more general reason than just "not supported by VS versions X and Y".
>
> The behavior of OFFSET_OF() would be interesting -- the OFFSET_OF()
> macro definition for non-gcc / non-clang:
>
> #define OFFSET_OF(TYPE, Field) ((UINTN) &(((TYPE *)0)->Field))
>
> borders on undefined behavior as far as I can tell, so its behavior is
> totally up to the compiler. It works thus far okay on Visual Studio, but
> I couldn't say if it extended correctly to flexible array members.

Yes, it's UB by the standard, but this is actually how MS implements 
them (or used to anyway?). I don't see why it'd cause issues with 
flexible arrays, as only the start of the array is relevant (which is 
constant for all instances of the structure no matter the amount of 
elements actually stored). Any specific concern? If so, they could be 
addressed by appropriate STATIC_ASSERTs.

Best regards,
Marvin

>
> Thanks
> Laszlo
>
>> I feel
>> like if the structure is modified anyway, it should probably get a
>> trailing flexible array over the 1-sized hack. What do you think?

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

[Laszlo Ersek]

On 06/23/21 08:54, Marvin Häuser wrote:
> On 22.06.21 17:34, Laszlo Ersek wrote:
>> On 06/18/21 11:37, Marvin Häuser wrote:
>>> On 16.06.21 22:58, Kun Qin wrote:
>>>> On 06/16/2021 00:02, Marvin Häuser wrote:
>>>>> 2) Is it feasible yet with the current set of supported compilers to
>>>>> support flexible arrays?
>>>> My impression is that flexible arrays are already supported (as seen
>>>> in UnitTestFrameworkPkg/PrivateInclude/UnitTestFrameworkTypes.h).
>>>> Please correct me if I am wrong.
>>>>
>>>> Would you mind letting me know why this is applicable here? We are
>>>> trying to seek ideas on how to catch developer mistakes caused by this
>>>> change. So any input is appreciated.
>>> Huh, interesting. Last time I tried I was told about incompatibilities
>>> with MSVC, but I know some have been dropped since then (2005 and 2008
>>> if I recall correctly?), so that'd be great to allow globally.
>> I too am surprised to see
>> "UnitTestFrameworkPkg/PrivateInclude/UnitTestFrameworkTypes.h". The
>> flexible array member is a C99 feature, and I didn't even know that we
>> disallowed it for the sake of particular VS toolchains -- I thought we
>> had a more general reason than just "not supported by VS versions X
>> and Y".
>>
>> The behavior of OFFSET_OF() would be interesting -- the OFFSET_OF()
>> macro definition for non-gcc / non-clang:
>>
>> #define OFFSET_OF(TYPE, Field) ((UINTN) &(((TYPE *)0)->Field))
>>
>> borders on undefined behavior as far as I can tell, so its behavior is
>> totally up to the compiler. It works thus far okay on Visual Studio, but
>> I couldn't say if it extended correctly to flexible array members.
> 
> Yes, it's UB by the standard, but this is actually how MS implements
> them (or used to anyway?). I don't see why it'd cause issues with
> flexible arrays, as only the start of the array is relevant (which is
> constant for all instances of the structure no matter the amount of
> elements actually stored). Any specific concern? If so, they could be
> addressed by appropriate STATIC_ASSERTs.

No specific concern; my point was that two aspects of the same "class"
of undefined behavior didn't need to be consistent with each other.

Thanks
Laszlo

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

[Kun Qin]

Hi Marvin,

I would prefer not to rely on undefined behaviors from different 
compilers. Instead of using flexible arrays, is it better to remove the 
`Data` field, pack the structure and follow 
"VARIABLE_LOCK_ON_VAR_STATE_POLICY" pattern?

In that case, OFFSET_OF will be forced to change to sizeof, and 
read/write to `Data` will follow the range indicated by MessageLength. 
But yes, that will enforce developers to update their platform level 
implementations accordingly.

Regards,
Kun

On 06/23/2021 08:26, Laszlo Ersek wrote:
> On 06/23/21 08:54, Marvin Häuser wrote:
>> On 22.06.21 17:34, Laszlo Ersek wrote:
>>> On 06/18/21 11:37, Marvin Häuser wrote:
>>>> On 16.06.21 22:58, Kun Qin wrote:
>>>>> On 06/16/2021 00:02, Marvin Häuser wrote:
>>>>>> 2) Is it feasible yet with the current set of supported compilers to
>>>>>> support flexible arrays?
>>>>> My impression is that flexible arrays are already supported (as seen
>>>>> in UnitTestFrameworkPkg/PrivateInclude/UnitTestFrameworkTypes.h).
>>>>> Please correct me if I am wrong.
>>>>>
>>>>> Would you mind letting me know why this is applicable here? We are
>>>>> trying to seek ideas on how to catch developer mistakes caused by this
>>>>> change. So any input is appreciated.
>>>> Huh, interesting. Last time I tried I was told about incompatibilities
>>>> with MSVC, but I know some have been dropped since then (2005 and 2008
>>>> if I recall correctly?), so that'd be great to allow globally.
>>> I too am surprised to see
>>> "UnitTestFrameworkPkg/PrivateInclude/UnitTestFrameworkTypes.h". The
>>> flexible array member is a C99 feature, and I didn't even know that we
>>> disallowed it for the sake of particular VS toolchains -- I thought we
>>> had a more general reason than just "not supported by VS versions X
>>> and Y".
>>>
>>> The behavior of OFFSET_OF() would be interesting -- the OFFSET_OF()
>>> macro definition for non-gcc / non-clang:
>>>
>>> #define OFFSET_OF(TYPE, Field) ((UINTN) &(((TYPE *)0)->Field))
>>>
>>> borders on undefined behavior as far as I can tell, so its behavior is
>>> totally up to the compiler. It works thus far okay on Visual Studio, but
>>> I couldn't say if it extended correctly to flexible array members.
>>
>> Yes, it's UB by the standard, but this is actually how MS implements
>> them (or used to anyway?). I don't see why it'd cause issues with
>> flexible arrays, as only the start of the array is relevant (which is
>> constant for all instances of the structure no matter the amount of
>> elements actually stored). Any specific concern? If so, they could be
>> addressed by appropriate STATIC_ASSERTs.
> 
> No specific concern; my point was that two aspects of the same "class"
> of undefined behavior didn't need to be consistent with each other.
> 
> Thanks
> Laszlo
> 

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

[Kun Qin]

Hi Hao,

We have been circulating different ideas internally about how to enforce 
a warning around this change.

There is an idea of deprecating the old structure and replacing it with 
a newly defined EFI_MM_COMMUNICATE_HEADER_V2. That way all consumers 
will be enforced to update their implementation and (hopefully) inspect 
the compatibility accordingly. In addition, we could add other fields 
such as signature and/or header version number for further extensibility.

If there are code instances that uses "sizeof(EFI_GUID) + sizeof(UINTN)" 
in lieu of OFFSET_OF, this implementation is essentially decoupled from 
the structure definition. So compiler might not be able to help. In that 
case, runtime protections such as pool guard or stack guard might be useful.

Please let me know if you think header structure V2 is an idea worth to try.

Thanks,
Kun

On 06/15/2021 18:15, Wu, Hao A wrote:
> Thanks Kun,
> 
> I am a little concerned on whether there will be other missing cases that are not covered by this patch series.
> 
> I am also wondering is there any detection can be made to warn the cases that code modification should be made after this structure update.
> Otherwise, it will be the burden for the platform owners to review the platform codes following your guide mentioned in this patch.
> 
> Hoping others can provide some inputs on this.
> 
> Best Regards,
> Hao Wu
> 
>> -----Original Message-----
>> From: Kun Qin <kuqin12@gmail.com>
>> Sent: Wednesday, June 16, 2021 4:51 AM
>> To: Wu, Hao A <hao.a.wu@intel.com>; 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>;
>> Andrew Fish <afish@apple.com>; Laszlo Ersek <lersek@redhat.com>; Leif
>> Lindholm <leif@nuviainc.com>
>> Subject: Re: [edk2-devel] [PATCH v1 1/5] EDK2 Code First: PI Specification:
>> EFI_MM_COMMUNICATE_HEADER Update
>>
>> Hi Hao,
>>
>> Sorry that I missed comments for this patch earlier. You are correct. I only
>> inspected SmmLockBoxPeiLib. The PEI instance handled mode switch with
>> ```OFFSET_OF ``` function. But DXE instance still have a few use cases that will
>> be impacted.
>>
>> I will update this read me file and add a code change patch for this library in
>> v2. Thanks for pointing this out.
>>
>> Regards,
>> Kun
>>
>> On 06/11/2021 00:46, Wu, Hao A wrote:
>>>> -----Original Message-----
>>>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Kun
>>>> Qin
>>>> Sent: Thursday, June 10, 2021 9:43 AM
>>>> 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>;
>>>> Andrew Fish <afish@apple.com>; Laszlo Ersek <lersek@redhat.com>; Leif
>>>> Lindholm <leif@nuviainc.com>
>>>> Subject: [edk2-devel] [PATCH v1 1/5] EDK2 Code First: PI Specification:
>>>> EFI_MM_COMMUNICATE_HEADER Update
>>>>
>>>> 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>
>>>>
>>>> Signed-off-by: Kun Qin <kuqin12@gmail.com>
>>>> ---
>>>>    BZ3430-SpecChange.md | 88 ++++++++++++++++++++
>>>>    1 file changed, 88 insertions(+)
>>>>
>>>> diff --git a/BZ3430-SpecChange.md b/BZ3430-SpecChange.md new file
>>>> mode
>>>> 100644 index 000000000000..33a1ffda447b
>>>> --- /dev/null
>>>> +++ b/BZ3430-SpecChange.md
>>>> @@ -0,0 +1,88 @@
>>>> +# 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
>>>> +```
>>>> +
>>>> +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
>>>
>>>
>>> Hello Kun,
>>>
>>> Sorry for a question.
>>> I am not sure why the current codes in file SmmLockBoxDxeLib.c will work
>> properly after the UINTN -> UINT64 change.
>>>
>>> For example:
>>> LockBoxGetSmmCommBuffer():
>>>     MinimalSizeNeeded = sizeof (EFI_GUID) +
>>>                         sizeof (UINTN) +
>>>                         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),
>>>                                        MAX (sizeof
>> (EFI_SMM_LOCK_BOX_PARAMETER_RESTORE),
>>>                                             sizeof
>>> (EFI_SMM_LOCK_BOX_PARAMETER_RESTORE_ALL_IN_PLACE)))));
>>>
>>> SaveLockBox():
>>>     UINT8                           TempCommBuffer[sizeof(EFI_GUID) + sizeof(UINTN)
>> + sizeof(EFI_SMM_LOCK_BOX_PARAMETER_SAVE)];
>>>
>>> Is the series missed changes for SmmLockBoxDxeLib or I got something
>> wrong?
>>>
>>> Best Regards,
>>> Hao Wu
>>>
>>>
>>>> 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
>>>> +```
>>>> +
>>>> +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
>>>>
>>>>
>>>>
>>>> 
>>>>
>>>

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

[Bret Barkelew]

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

Yeah, the only other thought I had for moving forwards quickly (i.e. hackily) is to define a new GUID that just means “I use an updated header configuration” since the EFI_GUID field comes first and is a well-understood size.

I would prefer the “deprecate, break builds, fix code, move forward” approach.

- Bret

From: Kun Qin via groups.io<mailto:kuqin12=gmail.com@groups.io>
Sent: Wednesday, June 23, 2021 5:53 PM
To: Wu, Hao A<mailto:hao.a.wu@intel.com>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>
Cc: Kinney, Michael D<mailto:michael.d.kinney@intel.com>; Liming Gao<mailto:gaoliming@byosoft.com.cn>; Liu, Zhiguang<mailto:zhiguang.liu@intel.com>; Andrew Fish<mailto:afish@apple.com>; Laszlo Ersek<mailto:lersek@redhat.com>; Lindholm, Leif<mailto:leif@nuviainc.com>; Bret Barkelew<mailto:Bret.Barkelew@microsoft.com>; Michael Kubacki<mailto:Michael.Kubacki@microsoft.com>
Subject: [EXTERNAL] Re: [edk2-devel] [PATCH v1 1/5] EDK2 Code First: PI Specification: EFI_MM_COMMUNICATE_HEADER Update

Hi Hao,

We have been circulating different ideas internally about how to enforce
a warning around this change.

There is an idea of deprecating the old structure and replacing it with
a newly defined EFI_MM_COMMUNICATE_HEADER_V2. That way all consumers
will be enforced to update their implementation and (hopefully) inspect
the compatibility accordingly. In addition, we could add other fields
such as signature and/or header version number for further extensibility.

If there are code instances that uses "sizeof(EFI_GUID) + sizeof(UINTN)"
in lieu of OFFSET_OF, this implementation is essentially decoupled from
the structure definition. So compiler might not be able to help. In that
case, runtime protections such as pool guard or stack guard might be useful.

Please let me know if you think header structure V2 is an idea worth to try.

Thanks,
Kun

On 06/15/2021 18:15, Wu, Hao A wrote:
> Thanks Kun,
>
> I am a little concerned on whether there will be other missing cases that are not covered by this patch series.
>
> I am also wondering is there any detection can be made to warn the cases that code modification should be made after this structure update.
> Otherwise, it will be the burden for the platform owners to review the platform codes following your guide mentioned in this patch.
>
> Hoping others can provide some inputs on this.
>
> Best Regards,
> Hao Wu
>
>> -----Original Message-----
>> From: Kun Qin <kuqin12@gmail.com>
>> Sent: Wednesday, June 16, 2021 4:51 AM
>> To: Wu, Hao A <hao.a.wu@intel.com>; 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>;
>> Andrew Fish <afish@apple.com>; Laszlo Ersek <lersek@redhat.com>; Leif
>> Lindholm <leif@nuviainc.com>
>> Subject: Re: [edk2-devel] [PATCH v1 1/5] EDK2 Code First: PI Specification:
>> EFI_MM_COMMUNICATE_HEADER Update
>>
>> Hi Hao,
>>
>> Sorry that I missed comments for this patch earlier. You are correct. I only
>> inspected SmmLockBoxPeiLib. The PEI instance handled mode switch with
>> ```OFFSET_OF ``` function. But DXE instance still have a few use cases that will
>> be impacted.
>>
>> I will update this read me file and add a code change patch for this library in
>> v2. Thanks for pointing this out.
>>
>> Regards,
>> Kun
>>
>> On 06/11/2021 00:46, Wu, Hao A wrote:
>>>> -----Original Message-----
>>>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Kun
>>>> Qin
>>>> Sent: Thursday, June 10, 2021 9:43 AM
>>>> 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>;
>>>> Andrew Fish <afish@apple.com>; Laszlo Ersek <lersek@redhat.com>; Leif
>>>> Lindholm <leif@nuviainc.com>
>>>> Subject: [edk2-devel] [PATCH v1 1/5] EDK2 Code First: PI Specification:
>>>> EFI_MM_COMMUNICATE_HEADER Update
>>>>
>>>> REF: https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3430&amp;data=04%7C01%7CBret.Barkelew%40microsoft.com%7Ca480819ff5e84e12239f08d936aa7bc5%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637600928127681913%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=483mG24s%2Bp0xSF90Wf%2Fmh2TN1atrIQa5mOrLyEPCTuE%3D&amp;reserved=0
>>>>
>>>> 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>
>>>>
>>>> Signed-off-by: Kun Qin <kuqin12@gmail.com>
>>>> ---
>>>>    BZ3430-SpecChange.md | 88 ++++++++++++++++++++
>>>>    1 file changed, 88 insertions(+)
>>>>
>>>> diff --git a/BZ3430-SpecChange.md b/BZ3430-SpecChange.md new file
>>>> mode
>>>> 100644 index 000000000000..33a1ffda447b
>>>> --- /dev/null
>>>> +++ b/BZ3430-SpecChange.md
>>>> @@ -0,0 +1,88 @@
>>>> +# 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://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.tianocore.org%2F&amp;data=04%7C01%7CBret.Barkelew%40microsoft.com%7Ca480819ff5e84e12239f08d936aa7bc5%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637600928127681913%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=eRu4We7iSzuwrsS9MqIO2iqLxXHZ6CpUkInVpty554c%3D&amp;reserved=0)
>>>> +
>>>> +## 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
>>>> +```
>>>> +
>>>> +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
>>>
>>>
>>> Hello Kun,
>>>
>>> Sorry for a question.
>>> I am not sure why the current codes in file SmmLockBoxDxeLib.c will work
>> properly after the UINTN -> UINT64 change.
>>>
>>> For example:
>>> LockBoxGetSmmCommBuffer():
>>>     MinimalSizeNeeded = sizeof (EFI_GUID) +
>>>                         sizeof (UINTN) +
>>>                         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),
>>>                                        MAX (sizeof
>> (EFI_SMM_LOCK_BOX_PARAMETER_RESTORE),
>>>                                             sizeof
>>> (EFI_SMM_LOCK_BOX_PARAMETER_RESTORE_ALL_IN_PLACE)))));
>>>
>>> SaveLockBox():
>>>     UINT8                           TempCommBuffer[sizeof(EFI_GUID) + sizeof(UINTN)
>> + sizeof(EFI_SMM_LOCK_BOX_PARAMETER_SAVE)];
>>>
>>> Is the series missed changes for SmmLockBoxDxeLib or I got something
>> wrong?
>>>
>>> Best Regards,
>>> Hao Wu
>>>
>>>
>>>> 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
>>>> +```
>>>> +
>>>> +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
>>>>
>>>>
>>>>
>>>>
>>>>
>>>






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

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

[Marvin Häuser]

Hey Kun,

Why would you rely on undefined behaviours? The OFFSET_OF macro is 
well-defined for GCC and Clang as it's implemented by an intrinsic, and 
while the expression for the MSVC compiler is undefined behaviour as per 
the C standard, it is well-defined for MSVC due to their own 
implementation being identical. From my standpoint, all supported 
compilers will yield well-defined behaviour even this way. OFFSET_OF on 
flexible arrays is not UB in any case to my knowledge.

However, the same way as your new suggestion, you can replace OFFSET_OF 
with sizeof. While this *can* lead to wasted space with certain 
structure layouts (e.g. when the flexible array overlays padding bytes), 
this is not the case here, and otherwise just loses you a few bytes. I 
think this comes down to preference.

The pattern you mentioned arguably is less nice syntax when used 
(involves address calculation and casting), but the biggest problem here 
is alignment constraints. For packed structures, you lose the ability of 
automatic unaligned accesses (irrelevant here because the structure is 
manually padded anyway). For non-packed structures, you still need to 
ensure the alignment requirement of the trailing array data is met 
manually. With flexible array members, the compiler takes care of both 
cases automatically.

Best regards,
Marvin

On 24.06.21 02:24, Kun Qin wrote:
> Hi Marvin,
>
> I would prefer not to rely on undefined behaviors from different 
> compilers. Instead of using flexible arrays, is it better to remove 
> the `Data` field, pack the structure and follow 
> "VARIABLE_LOCK_ON_VAR_STATE_POLICY" pattern?
>
> In that case, OFFSET_OF will be forced to change to sizeof, and 
> read/write to `Data` will follow the range indicated by MessageLength. 
> But yes, that will enforce developers to update their platform level 
> implementations accordingly.
>
> Regards,
> Kun
>
> On 06/23/2021 08:26, Laszlo Ersek wrote:
>> On 06/23/21 08:54, Marvin Häuser wrote:
>>> On 22.06.21 17:34, Laszlo Ersek wrote:
>>>> On 06/18/21 11:37, Marvin Häuser wrote:
>>>>> On 16.06.21 22:58, Kun Qin wrote:
>>>>>> On 06/16/2021 00:02, Marvin Häuser wrote:
>>>>>>> 2) Is it feasible yet with the current set of supported 
>>>>>>> compilers to
>>>>>>> support flexible arrays?
>>>>>> My impression is that flexible arrays are already supported (as seen
>>>>>> in UnitTestFrameworkPkg/PrivateInclude/UnitTestFrameworkTypes.h).
>>>>>> Please correct me if I am wrong.
>>>>>>
>>>>>> Would you mind letting me know why this is applicable here? We are
>>>>>> trying to seek ideas on how to catch developer mistakes caused by 
>>>>>> this
>>>>>> change. So any input is appreciated.
>>>>> Huh, interesting. Last time I tried I was told about 
>>>>> incompatibilities
>>>>> with MSVC, but I know some have been dropped since then (2005 and 
>>>>> 2008
>>>>> if I recall correctly?), so that'd be great to allow globally.
>>>> I too am surprised to see
>>>> "UnitTestFrameworkPkg/PrivateInclude/UnitTestFrameworkTypes.h". The
>>>> flexible array member is a C99 feature, and I didn't even know that we
>>>> disallowed it for the sake of particular VS toolchains -- I thought we
>>>> had a more general reason than just "not supported by VS versions X
>>>> and Y".
>>>>
>>>> The behavior of OFFSET_OF() would be interesting -- the OFFSET_OF()
>>>> macro definition for non-gcc / non-clang:
>>>>
>>>> #define OFFSET_OF(TYPE, Field) ((UINTN) &(((TYPE *)0)->Field))
>>>>
>>>> borders on undefined behavior as far as I can tell, so its behavior is
>>>> totally up to the compiler. It works thus far okay on Visual 
>>>> Studio, but
>>>> I couldn't say if it extended correctly to flexible array members.
>>>
>>> Yes, it's UB by the standard, but this is actually how MS implements
>>> them (or used to anyway?). I don't see why it'd cause issues with
>>> flexible arrays, as only the start of the array is relevant (which is
>>> constant for all instances of the structure no matter the amount of
>>> elements actually stored). Any specific concern? If so, they could be
>>> addressed by appropriate STATIC_ASSERTs.
>>
>> No specific concern; my point was that two aspects of the same "class"
>> of undefined behavior didn't need to be consistent with each other.
>>
>> Thanks
>> Laszlo
>>

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

[Michael D Kinney]

Hello,

Flexible array members are supported and should be used.  The old style
of adding an array of size [1] at the end of a structure was used at a time
flexible array members were not supported by all compilers (late 1990's).
The workarounds used to handle the array of size [1] are very confusing when
reading the C  code and the fact that sizeof() does not produce the expected
result make it even worse.

If we use flexible array members in this proposed change then there is
no need to use OFFSET_OF().  Correct?

Mike


> -----Original Message-----
> From: Marvin Häuser <mhaeuser@posteo.de>
> Sent: Thursday, June 24, 2021 1:00 AM
> To: Kun Qin <kuqin12@gmail.com>; Laszlo Ersek <lersek@redhat.com>; 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>; Kinney, Michael D <michael.d.kinney@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>; Liu, Zhiguang
> <zhiguang.liu@intel.com>; Andrew Fish <afish@apple.com>; Leif Lindholm <leif@nuviainc.com>; Bret Barkelew
> <Bret.Barkelew@microsoft.com>; michael.kubacki@microsoft.com
> Subject: Re: [edk2-devel] [PATCH v1 0/5] EDK2 Code First: PI Specification: Update EFI_MM_COMMUNICATE_HEADER
> 
> Hey Kun,
> 
> Why would you rely on undefined behaviours? The OFFSET_OF macro is
> well-defined for GCC and Clang as it's implemented by an intrinsic, and
> while the expression for the MSVC compiler is undefined behaviour as per
> the C standard, it is well-defined for MSVC due to their own
> implementation being identical. From my standpoint, all supported
> compilers will yield well-defined behaviour even this way. OFFSET_OF on
> flexible arrays is not UB in any case to my knowledge.
> 
> However, the same way as your new suggestion, you can replace OFFSET_OF
> with sizeof. While this *can* lead to wasted space with certain
> structure layouts (e.g. when the flexible array overlays padding bytes),
> this is not the case here, and otherwise just loses you a few bytes. I
> think this comes down to preference.
> 
> The pattern you mentioned arguably is less nice syntax when used
> (involves address calculation and casting), but the biggest problem here
> is alignment constraints. For packed structures, you lose the ability of
> automatic unaligned accesses (irrelevant here because the structure is
> manually padded anyway). For non-packed structures, you still need to
> ensure the alignment requirement of the trailing array data is met
> manually. With flexible array members, the compiler takes care of both
> cases automatically.
> 
> Best regards,
> Marvin
> 
> On 24.06.21 02:24, Kun Qin wrote:
> > Hi Marvin,
> >
> > I would prefer not to rely on undefined behaviors from different
> > compilers. Instead of using flexible arrays, is it better to remove
> > the `Data` field, pack the structure and follow
> > "VARIABLE_LOCK_ON_VAR_STATE_POLICY" pattern?
> >
> > In that case, OFFSET_OF will be forced to change to sizeof, and
> > read/write to `Data` will follow the range indicated by MessageLength.
> > But yes, that will enforce developers to update their platform level
> > implementations accordingly.
> >
> > Regards,
> > Kun
> >
> > On 06/23/2021 08:26, Laszlo Ersek wrote:
> >> On 06/23/21 08:54, Marvin Häuser wrote:
> >>> On 22.06.21 17:34, Laszlo Ersek wrote:
> >>>> On 06/18/21 11:37, Marvin Häuser wrote:
> >>>>> On 16.06.21 22:58, Kun Qin wrote:
> >>>>>> On 06/16/2021 00:02, Marvin Häuser wrote:
> >>>>>>> 2) Is it feasible yet with the current set of supported
> >>>>>>> compilers to
> >>>>>>> support flexible arrays?
> >>>>>> My impression is that flexible arrays are already supported (as seen
> >>>>>> in UnitTestFrameworkPkg/PrivateInclude/UnitTestFrameworkTypes.h).
> >>>>>> Please correct me if I am wrong.
> >>>>>>
> >>>>>> Would you mind letting me know why this is applicable here? We are
> >>>>>> trying to seek ideas on how to catch developer mistakes caused by
> >>>>>> this
> >>>>>> change. So any input is appreciated.
> >>>>> Huh, interesting. Last time I tried I was told about
> >>>>> incompatibilities
> >>>>> with MSVC, but I know some have been dropped since then (2005 and
> >>>>> 2008
> >>>>> if I recall correctly?), so that'd be great to allow globally.
> >>>> I too am surprised to see
> >>>> "UnitTestFrameworkPkg/PrivateInclude/UnitTestFrameworkTypes.h". The
> >>>> flexible array member is a C99 feature, and I didn't even know that we
> >>>> disallowed it for the sake of particular VS toolchains -- I thought we
> >>>> had a more general reason than just "not supported by VS versions X
> >>>> and Y".
> >>>>
> >>>> The behavior of OFFSET_OF() would be interesting -- the OFFSET_OF()
> >>>> macro definition for non-gcc / non-clang:
> >>>>
> >>>> #define OFFSET_OF(TYPE, Field) ((UINTN) &(((TYPE *)0)->Field))
> >>>>
> >>>> borders on undefined behavior as far as I can tell, so its behavior is
> >>>> totally up to the compiler. It works thus far okay on Visual
> >>>> Studio, but
> >>>> I couldn't say if it extended correctly to flexible array members.
> >>>
> >>> Yes, it's UB by the standard, but this is actually how MS implements
> >>> them (or used to anyway?). I don't see why it'd cause issues with
> >>> flexible arrays, as only the start of the array is relevant (which is
> >>> constant for all instances of the structure no matter the amount of
> >>> elements actually stored). Any specific concern? If so, they could be
> >>> addressed by appropriate STATIC_ASSERTs.
> >>
> >> No specific concern; my point was that two aspects of the same "class"
> >> of undefined behavior didn't need to be consistent with each other.
> >>
> >> Thanks
> >> Laszlo
> >>

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

[Kun Qin]

Hi Mike,

Thanks for the information. I can update the patch and proposed spec 
change to use flexible array in v-next if there is no other concerns.

After switching to flexible array, OFFSET_OF (Data) should lead to the 
same result as sizeof. While sizeof would be a preferred way to move 
forward.

Regards,
Kun

On 06/24/2021 08:25, Kinney, Michael D wrote:
> Hello,
> 
> Flexible array members are supported and should be used.  The old style
> of adding an array of size [1] at the end of a structure was used at a time
> flexible array members were not supported by all compilers (late 1990's).
> The workarounds used to handle the array of size [1] are very confusing when
> reading the C  code and the fact that sizeof() does not produce the expected
> result make it even worse.
> 
> If we use flexible array members in this proposed change then there is
> no need to use OFFSET_OF().  Correct?
> 
> Mike
> 
> 
>> -----Original Message-----
>> From: Marvin Häuser <mhaeuser@posteo.de>
>> Sent: Thursday, June 24, 2021 1:00 AM
>> To: Kun Qin <kuqin12@gmail.com>; Laszlo Ersek <lersek@redhat.com>; 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>; Kinney, Michael D <michael.d.kinney@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>; Liu, Zhiguang
>> <zhiguang.liu@intel.com>; Andrew Fish <afish@apple.com>; Leif Lindholm <leif@nuviainc.com>; Bret Barkelew
>> <Bret.Barkelew@microsoft.com>; michael.kubacki@microsoft.com
>> Subject: Re: [edk2-devel] [PATCH v1 0/5] EDK2 Code First: PI Specification: Update EFI_MM_COMMUNICATE_HEADER
>>
>> Hey Kun,
>>
>> Why would you rely on undefined behaviours? The OFFSET_OF macro is
>> well-defined for GCC and Clang as it's implemented by an intrinsic, and
>> while the expression for the MSVC compiler is undefined behaviour as per
>> the C standard, it is well-defined for MSVC due to their own
>> implementation being identical. From my standpoint, all supported
>> compilers will yield well-defined behaviour even this way. OFFSET_OF on
>> flexible arrays is not UB in any case to my knowledge.
>>
>> However, the same way as your new suggestion, you can replace OFFSET_OF
>> with sizeof. While this *can* lead to wasted space with certain
>> structure layouts (e.g. when the flexible array overlays padding bytes),
>> this is not the case here, and otherwise just loses you a few bytes. I
>> think this comes down to preference.
>>
>> The pattern you mentioned arguably is less nice syntax when used
>> (involves address calculation and casting), but the biggest problem here
>> is alignment constraints. For packed structures, you lose the ability of
>> automatic unaligned accesses (irrelevant here because the structure is
>> manually padded anyway). For non-packed structures, you still need to
>> ensure the alignment requirement of the trailing array data is met
>> manually. With flexible array members, the compiler takes care of both
>> cases automatically.
>>
>> Best regards,
>> Marvin
>>
>> On 24.06.21 02:24, Kun Qin wrote:
>>> Hi Marvin,
>>>
>>> I would prefer not to rely on undefined behaviors from different
>>> compilers. Instead of using flexible arrays, is it better to remove
>>> the `Data` field, pack the structure and follow
>>> "VARIABLE_LOCK_ON_VAR_STATE_POLICY" pattern?
>>>
>>> In that case, OFFSET_OF will be forced to change to sizeof, and
>>> read/write to `Data` will follow the range indicated by MessageLength.
>>> But yes, that will enforce developers to update their platform level
>>> implementations accordingly.
>>>
>>> Regards,
>>> Kun
>>>
>>> On 06/23/2021 08:26, Laszlo Ersek wrote:
>>>> On 06/23/21 08:54, Marvin Häuser wrote:
>>>>> On 22.06.21 17:34, Laszlo Ersek wrote:
>>>>>> On 06/18/21 11:37, Marvin Häuser wrote:
>>>>>>> On 16.06.21 22:58, Kun Qin wrote:
>>>>>>>> On 06/16/2021 00:02, Marvin Häuser wrote:
>>>>>>>>> 2) Is it feasible yet with the current set of supported
>>>>>>>>> compilers to
>>>>>>>>> support flexible arrays?
>>>>>>>> My impression is that flexible arrays are already supported (as seen
>>>>>>>> in UnitTestFrameworkPkg/PrivateInclude/UnitTestFrameworkTypes.h).
>>>>>>>> Please correct me if I am wrong.
>>>>>>>>
>>>>>>>> Would you mind letting me know why this is applicable here? We are
>>>>>>>> trying to seek ideas on how to catch developer mistakes caused by
>>>>>>>> this
>>>>>>>> change. So any input is appreciated.
>>>>>>> Huh, interesting. Last time I tried I was told about
>>>>>>> incompatibilities
>>>>>>> with MSVC, but I know some have been dropped since then (2005 and
>>>>>>> 2008
>>>>>>> if I recall correctly?), so that'd be great to allow globally.
>>>>>> I too am surprised to see
>>>>>> "UnitTestFrameworkPkg/PrivateInclude/UnitTestFrameworkTypes.h". The
>>>>>> flexible array member is a C99 feature, and I didn't even know that we
>>>>>> disallowed it for the sake of particular VS toolchains -- I thought we
>>>>>> had a more general reason than just "not supported by VS versions X
>>>>>> and Y".
>>>>>>
>>>>>> The behavior of OFFSET_OF() would be interesting -- the OFFSET_OF()
>>>>>> macro definition for non-gcc / non-clang:
>>>>>>
>>>>>> #define OFFSET_OF(TYPE, Field) ((UINTN) &(((TYPE *)0)->Field))
>>>>>>
>>>>>> borders on undefined behavior as far as I can tell, so its behavior is
>>>>>> totally up to the compiler. It works thus far okay on Visual
>>>>>> Studio, but
>>>>>> I couldn't say if it extended correctly to flexible array members.
>>>>>
>>>>> Yes, it's UB by the standard, but this is actually how MS implements
>>>>> them (or used to anyway?). I don't see why it'd cause issues with
>>>>> flexible arrays, as only the start of the array is relevant (which is
>>>>> constant for all instances of the structure no matter the amount of
>>>>> elements actually stored). Any specific concern? If so, they could be
>>>>> addressed by appropriate STATIC_ASSERTs.
>>>>
>>>> No specific concern; my point was that two aspects of the same "class"
>>>> of undefined behavior didn't need to be consistent with each other.
>>>>
>>>> Thanks
>>>> Laszlo
>>>>
> 

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

[Wu, Hao A]

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

Hello Kun and Bret, sorry for the delayed response.

I think I agree with the approach mentioned by Bret below: "deprecate, break builds, fix code, move forward".
If we just modify the structure, it is likely that platform drivers that consumes EFI_(S)MM_COMMUNICATE_HEADER may not be aware of the change and potentially consuming data at wrong offset.
A build failure can be an indication to platform owners that certain drivers should be reviewed upon a checklist mentioned in this patch.

Best Regards,
Hao Wu

From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Bret Barkelew via groups.io
Sent: Thursday, June 24, 2021 11:27 AM
To: devel@edk2.groups.io; kuqin12@gmail.com; Wu, Hao A <hao.a.wu@intel.com>
Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>; Liu, Zhiguang <zhiguang.liu@intel.com>; Andrew Fish <afish@apple.com>; Laszlo Ersek <lersek@redhat.com>; Lindholm, Leif <leif@nuviainc.com>; Michael Kubacki <Michael.Kubacki@microsoft.com>
Subject: Re: [EXTERNAL] Re: [edk2-devel] [PATCH v1 1/5] EDK2 Code First: PI Specification: EFI_MM_COMMUNICATE_HEADER Update

Yeah, the only other thought I had for moving forwards quickly (i.e. hackily) is to define a new GUID that just means "I use an updated header configuration" since the EFI_GUID field comes first and is a well-understood size.

I would prefer the "deprecate, break builds, fix code, move forward" approach.

- Bret

From: Kun Qin via groups.io<mailto:kuqin12=gmail.com@groups.io>
Sent: Wednesday, June 23, 2021 5:53 PM
To: Wu, Hao A<mailto:hao.a.wu@intel.com>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>
Cc: Kinney, Michael D<mailto:michael.d.kinney@intel.com>; Liming Gao<mailto:gaoliming@byosoft.com.cn>; Liu, Zhiguang<mailto:zhiguang.liu@intel.com>; Andrew Fish<mailto:afish@apple.com>; Laszlo Ersek<mailto:lersek@redhat.com>; Lindholm, Leif<mailto:leif@nuviainc.com>; Bret Barkelew<mailto:Bret.Barkelew@microsoft.com>; Michael Kubacki<mailto:Michael.Kubacki@microsoft.com>
Subject: [EXTERNAL] Re: [edk2-devel] [PATCH v1 1/5] EDK2 Code First: PI Specification: EFI_MM_COMMUNICATE_HEADER Update

Hi Hao,

We have been circulating different ideas internally about how to enforce
a warning around this change.

There is an idea of deprecating the old structure and replacing it with
a newly defined EFI_MM_COMMUNICATE_HEADER_V2. That way all consumers
will be enforced to update their implementation and (hopefully) inspect
the compatibility accordingly. In addition, we could add other fields
such as signature and/or header version number for further extensibility.

If there are code instances that uses "sizeof(EFI_GUID) + sizeof(UINTN)"
in lieu of OFFSET_OF, this implementation is essentially decoupled from
the structure definition. So compiler might not be able to help. In that
case, runtime protections such as pool guard or stack guard might be useful.

Please let me know if you think header structure V2 is an idea worth to try.

Thanks,
Kun

On 06/15/2021 18:15, Wu, Hao A wrote:
> Thanks Kun,
>
> I am a little concerned on whether there will be other missing cases that are not covered by this patch series.
>
> I am also wondering is there any detection can be made to warn the cases that code modification should be made after this structure update.
> Otherwise, it will be the burden for the platform owners to review the platform codes following your guide mentioned in this patch.
>
> Hoping others can provide some inputs on this.
>
> Best Regards,
> Hao Wu
>
>> -----Original Message-----
>> From: Kun Qin <kuqin12@gmail.com<mailto:kuqin12@gmail.com>>
>> Sent: Wednesday, June 16, 2021 4:51 AM
>> To: Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>
>> Cc: Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; Liming Gao
>> <gaoliming@byosoft.com.cn<mailto:gaoliming@byosoft.com.cn>>; Liu, Zhiguang <zhiguang.liu@intel.com<mailto:zhiguang.liu@intel.com>>;
>> Andrew Fish <afish@apple.com<mailto:afish@apple.com>>; Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>; Leif
>> Lindholm <leif@nuviainc.com<mailto:leif@nuviainc.com>>
>> Subject: Re: [edk2-devel] [PATCH v1 1/5] EDK2 Code First: PI Specification:
>> EFI_MM_COMMUNICATE_HEADER Update
>>
>> Hi Hao,
>>
>> Sorry that I missed comments for this patch earlier. You are correct. I only
>> inspected SmmLockBoxPeiLib. The PEI instance handled mode switch with
>> ```OFFSET_OF ``` function. But DXE instance still have a few use cases that will
>> be impacted.
>>
>> I will update this read me file and add a code change patch for this library in
>> v2. Thanks for pointing this out.
>>
>> Regards,
>> Kun
>>
>> On 06/11/2021 00:46, Wu, Hao A wrote:
>>>> -----Original Message-----
>>>> From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of Kun
>>>> Qin
>>>> Sent: Thursday, June 10, 2021 9:43 AM
>>>> To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>
>>>> Cc: Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; Liming Gao
>>>> <gaoliming@byosoft.com.cn<mailto:gaoliming@byosoft.com.cn>>; Liu, Zhiguang <zhiguang.liu@intel.com<mailto:zhiguang.liu@intel.com>>;
>>>> Andrew Fish <afish@apple.com<mailto:afish@apple.com>>; Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>; Leif
>>>> Lindholm <leif@nuviainc.com<mailto:leif@nuviainc.com>>
>>>> Subject: [edk2-devel] [PATCH v1 1/5] EDK2 Code First: PI Specification:
>>>> EFI_MM_COMMUNICATE_HEADER Update
>>>>
>>>> REF: https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3430&amp;data=04%7C01%7CBret.Barkelew%40microsoft.com%7Ca480819ff5e84e12239f08d936aa7bc5%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637600928127681913%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=483mG24s%2Bp0xSF90Wf%2Fmh2TN1atrIQa5mOrLyEPCTuE%3D&amp;reserved=0
>>>>
>>>> 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<mailto:michael.d.kinney@intel.com>>
>>>> Cc: Liming Gao <gaoliming@byosoft.com.cn<mailto:gaoliming@byosoft.com.cn>>
>>>> Cc: Zhiguang Liu <zhiguang.liu@intel.com<mailto:zhiguang.liu@intel.com>>
>>>> Cc: Andrew Fish <afish@apple.com<mailto:afish@apple.com>>
>>>> Cc: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
>>>> Cc: Leif Lindholm <leif@nuviainc.com<mailto:leif@nuviainc.com>>
>>>>
>>>> Signed-off-by: Kun Qin <kuqin12@gmail.com<mailto:kuqin12@gmail.com>>
>>>> ---
>>>>    BZ3430-SpecChange.md | 88 ++++++++++++++++++++
>>>>    1 file changed, 88 insertions(+)
>>>>
>>>> diff --git a/BZ3430-SpecChange.md b/BZ3430-SpecChange.md new file
>>>> mode
>>>> 100644 index 000000000000..33a1ffda447b
>>>> --- /dev/null
>>>> +++ b/BZ3430-SpecChange.md
>>>> @@ -0,0 +1,88 @@
>>>> +# 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://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.tianocore.org%2F&amp;data=04%7C01%7CBret.Barkelew%40microsoft.com%7Ca480819ff5e84e12239f08d936aa7bc5%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637600928127681913%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=eRu4We7iSzuwrsS9MqIO2iqLxXHZ6CpUkInVpty554c%3D&amp;reserved=0)
>>>> +
>>>> +## 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
>>>> +```
>>>> +
>>>> +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
>>>
>>>
>>> Hello Kun,
>>>
>>> Sorry for a question.
>>> I am not sure why the current codes in file SmmLockBoxDxeLib.c will work
>> properly after the UINTN -> UINT64 change.
>>>
>>> For example:
>>> LockBoxGetSmmCommBuffer():
>>>     MinimalSizeNeeded = sizeof (EFI_GUID) +
>>>                         sizeof (UINTN) +
>>>                         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),
>>>                                        MAX (sizeof
>> (EFI_SMM_LOCK_BOX_PARAMETER_RESTORE),
>>>                                             sizeof
>>> (EFI_SMM_LOCK_BOX_PARAMETER_RESTORE_ALL_IN_PLACE)))));
>>>
>>> SaveLockBox():
>>>     UINT8                           TempCommBuffer[sizeof(EFI_GUID) + sizeof(UINTN)
>> + sizeof(EFI_SMM_LOCK_BOX_PARAMETER_SAVE)];
>>>
>>> Is the series missed changes for SmmLockBoxDxeLib or I got something
>> wrong?
>>>
>>> Best Regards,
>>> Hao Wu
>>>
>>>
>>>> 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
>>>> +```
>>>> +
>>>> +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
>>>>
>>>>
>>>>
>>>>
>>>>
>>>






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

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

[Laszlo Ersek]

On 06/25/21 20:47, Kun Qin wrote:
> Hi Mike,
> 
> Thanks for the information. I can update the patch and proposed spec
> change to use flexible array in v-next if there is no other concerns.
> 
> After switching to flexible array, OFFSET_OF (Data) should lead to the
> same result as sizeof.

This may be true on specific compilers, but it is *not* guaranteed by
the C language standard.

Quoting C99 6.7.2.1 "Structure and union specifiers", paragraph 16:

"In most situations, the flexible array member is ignored. In
particular, the size of the structure is as if the flexible array member
were omitted except that it may have more trailing padding than the
omission would imply."

Quoting footnotes 17 and 19,

(17)  [...]
      struct s { int n; double d[]; };
      [...]

(19)  [...]
      the expressions:
      [...]
      sizeof (struct s) >= offsetof(struct s, d)

      are always equal to 1.

Thanks
Laszlo



> While sizeof would be a preferred way to move
> forward.
> 
> Regards,
> Kun
> 
> On 06/24/2021 08:25, Kinney, Michael D wrote:
>> Hello,
>>
>> Flexible array members are supported and should be used.  The old style
>> of adding an array of size [1] at the end of a structure was used at a
>> time
>> flexible array members were not supported by all compilers (late 1990's).
>> The workarounds used to handle the array of size [1] are very
>> confusing when
>> reading the C  code and the fact that sizeof() does not produce the
>> expected
>> result make it even worse.
>>
>> If we use flexible array members in this proposed change then there is
>> no need to use OFFSET_OF().  Correct?
>>
>> Mike
>>
>>
>>> -----Original Message-----
>>> From: Marvin Häuser <mhaeuser@posteo.de>
>>> Sent: Thursday, June 24, 2021 1:00 AM
>>> To: Kun Qin <kuqin12@gmail.com>; Laszlo Ersek <lersek@redhat.com>;
>>> 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>; Kinney, Michael D <michael.d.kinney@intel.com>;
>>> Liming Gao <gaoliming@byosoft.com.cn>; Liu, Zhiguang
>>> <zhiguang.liu@intel.com>; Andrew Fish <afish@apple.com>; Leif
>>> Lindholm <leif@nuviainc.com>; Bret Barkelew
>>> <Bret.Barkelew@microsoft.com>; michael.kubacki@microsoft.com
>>> Subject: Re: [edk2-devel] [PATCH v1 0/5] EDK2 Code First: PI
>>> Specification: Update EFI_MM_COMMUNICATE_HEADER
>>>
>>> Hey Kun,
>>>
>>> Why would you rely on undefined behaviours? The OFFSET_OF macro is
>>> well-defined for GCC and Clang as it's implemented by an intrinsic, and
>>> while the expression for the MSVC compiler is undefined behaviour as per
>>> the C standard, it is well-defined for MSVC due to their own
>>> implementation being identical. From my standpoint, all supported
>>> compilers will yield well-defined behaviour even this way. OFFSET_OF on
>>> flexible arrays is not UB in any case to my knowledge.
>>>
>>> However, the same way as your new suggestion, you can replace OFFSET_OF
>>> with sizeof. While this *can* lead to wasted space with certain
>>> structure layouts (e.g. when the flexible array overlays padding bytes),
>>> this is not the case here, and otherwise just loses you a few bytes. I
>>> think this comes down to preference.
>>>
>>> The pattern you mentioned arguably is less nice syntax when used
>>> (involves address calculation and casting), but the biggest problem here
>>> is alignment constraints. For packed structures, you lose the ability of
>>> automatic unaligned accesses (irrelevant here because the structure is
>>> manually padded anyway). For non-packed structures, you still need to
>>> ensure the alignment requirement of the trailing array data is met
>>> manually. With flexible array members, the compiler takes care of both
>>> cases automatically.
>>>
>>> Best regards,
>>> Marvin
>>>
>>> On 24.06.21 02:24, Kun Qin wrote:
>>>> Hi Marvin,
>>>>
>>>> I would prefer not to rely on undefined behaviors from different
>>>> compilers. Instead of using flexible arrays, is it better to remove
>>>> the `Data` field, pack the structure and follow
>>>> "VARIABLE_LOCK_ON_VAR_STATE_POLICY" pattern?
>>>>
>>>> In that case, OFFSET_OF will be forced to change to sizeof, and
>>>> read/write to `Data` will follow the range indicated by MessageLength.
>>>> But yes, that will enforce developers to update their platform level
>>>> implementations accordingly.
>>>>
>>>> Regards,
>>>> Kun
>>>>
>>>> On 06/23/2021 08:26, Laszlo Ersek wrote:
>>>>> On 06/23/21 08:54, Marvin Häuser wrote:
>>>>>> On 22.06.21 17:34, Laszlo Ersek wrote:
>>>>>>> On 06/18/21 11:37, Marvin Häuser wrote:
>>>>>>>> On 16.06.21 22:58, Kun Qin wrote:
>>>>>>>>> On 06/16/2021 00:02, Marvin Häuser wrote:
>>>>>>>>>> 2) Is it feasible yet with the current set of supported
>>>>>>>>>> compilers to
>>>>>>>>>> support flexible arrays?
>>>>>>>>> My impression is that flexible arrays are already supported (as
>>>>>>>>> seen
>>>>>>>>> in UnitTestFrameworkPkg/PrivateInclude/UnitTestFrameworkTypes.h).
>>>>>>>>> Please correct me if I am wrong.
>>>>>>>>>
>>>>>>>>> Would you mind letting me know why this is applicable here? We are
>>>>>>>>> trying to seek ideas on how to catch developer mistakes caused by
>>>>>>>>> this
>>>>>>>>> change. So any input is appreciated.
>>>>>>>> Huh, interesting. Last time I tried I was told about
>>>>>>>> incompatibilities
>>>>>>>> with MSVC, but I know some have been dropped since then (2005 and
>>>>>>>> 2008
>>>>>>>> if I recall correctly?), so that'd be great to allow globally.
>>>>>>> I too am surprised to see
>>>>>>> "UnitTestFrameworkPkg/PrivateInclude/UnitTestFrameworkTypes.h". The
>>>>>>> flexible array member is a C99 feature, and I didn't even know
>>>>>>> that we
>>>>>>> disallowed it for the sake of particular VS toolchains -- I
>>>>>>> thought we
>>>>>>> had a more general reason than just "not supported by VS versions X
>>>>>>> and Y".
>>>>>>>
>>>>>>> The behavior of OFFSET_OF() would be interesting -- the OFFSET_OF()
>>>>>>> macro definition for non-gcc / non-clang:
>>>>>>>
>>>>>>> #define OFFSET_OF(TYPE, Field) ((UINTN) &(((TYPE *)0)->Field))
>>>>>>>
>>>>>>> borders on undefined behavior as far as I can tell, so its
>>>>>>> behavior is
>>>>>>> totally up to the compiler. It works thus far okay on Visual
>>>>>>> Studio, but
>>>>>>> I couldn't say if it extended correctly to flexible array members.
>>>>>>
>>>>>> Yes, it's UB by the standard, but this is actually how MS implements
>>>>>> them (or used to anyway?). I don't see why it'd cause issues with
>>>>>> flexible arrays, as only the start of the array is relevant (which is
>>>>>> constant for all instances of the structure no matter the amount of
>>>>>> elements actually stored). Any specific concern? If so, they could be
>>>>>> addressed by appropriate STATIC_ASSERTs.
>>>>>
>>>>> No specific concern; my point was that two aspects of the same "class"
>>>>> of undefined behavior didn't need to be consistent with each other.
>>>>>
>>>>> Thanks
>>>>> Laszlo
>>>>>
>>
> 

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

[Marvin Häuser]

On 28.06.21 16:57, Laszlo Ersek wrote:
> On 06/25/21 20:47, Kun Qin wrote:
>> Hi Mike,
>>
>> Thanks for the information. I can update the patch and proposed spec
>> change to use flexible array in v-next if there is no other concerns.
>>
>> After switching to flexible array, OFFSET_OF (Data) should lead to the
>> same result as sizeof.
> This may be true on specific compilers, but it is *not* guaranteed by
> the C language standard.

Sorry, for completeness sake... :)

I don't think it really depends on the compiler (but can vary per ABI), 
but it's a conceptual issue with alignment requirements. Assuming 
natural alignment and the usual stuff, for "struct s { uint64_t a; 
uint32_t b; uint8_t c[]; }" the alignment requirement is 8 Bytes, where 
there are 4 Bytes of padding after "b" (as "c" may as well be empty). 
"c" however is guaranteed to start after b in the same fashion as if it 
was declared with the maximum amount of elements that fit the memory. So 
if we take 4 elements for "c", and note that "c" has an alignment 
requirement of 1 Byte, c[0 .. 3] will alias the padding after "b". For 
"sizeof" this means that the padding is included, whereas for "offsetof" 
it is not, yielding "sizeof(struct s) == offsetof(struct s, c) + 4". 
That is what I meant by "wasted space" earlier, but this could possibly 
be made nicer with macros as necessary.

As for this specific struct, the values should be identical as it is padded.

Best regards,
Marvin

>
> Quoting C99 6.7.2.1 "Structure and union specifiers", paragraph 16:
>
> "In most situations, the flexible array member is ignored. In
> particular, the size of the structure is as if the flexible array member
> were omitted except that it may have more trailing padding than the
> omission would imply."
>
> Quoting footnotes 17 and 19,
>
> (17)  [...]
>        struct s { int n; double d[]; };
>        [...]
>
> (19)  [...]
>        the expressions:
>        [...]
>        sizeof (struct s) >= offsetof(struct s, d)
>
>        are always equal to 1.
>
> Thanks
> Laszlo
>
>
>
>> While sizeof would be a preferred way to move
>> forward.
>>
>> Regards,
>> Kun
>>
>> On 06/24/2021 08:25, Kinney, Michael D wrote:
>>> Hello,
>>>
>>> Flexible array members are supported and should be used.  The old style
>>> of adding an array of size [1] at the end of a structure was used at a
>>> time
>>> flexible array members were not supported by all compilers (late 1990's).
>>> The workarounds used to handle the array of size [1] are very
>>> confusing when
>>> reading the C  code and the fact that sizeof() does not produce the
>>> expected
>>> result make it even worse.
>>>
>>> If we use flexible array members in this proposed change then there is
>>> no need to use OFFSET_OF().  Correct?
>>>
>>> Mike
>>>
>>>
>>>> -----Original Message-----
>>>> From: Marvin Häuser <mhaeuser@posteo.de>
>>>> Sent: Thursday, June 24, 2021 1:00 AM
>>>> To: Kun Qin <kuqin12@gmail.com>; Laszlo Ersek <lersek@redhat.com>;
>>>> 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>; Kinney, Michael D <michael.d.kinney@intel.com>;
>>>> Liming Gao <gaoliming@byosoft.com.cn>; Liu, Zhiguang
>>>> <zhiguang.liu@intel.com>; Andrew Fish <afish@apple.com>; Leif
>>>> Lindholm <leif@nuviainc.com>; Bret Barkelew
>>>> <Bret.Barkelew@microsoft.com>; michael.kubacki@microsoft.com
>>>> Subject: Re: [edk2-devel] [PATCH v1 0/5] EDK2 Code First: PI
>>>> Specification: Update EFI_MM_COMMUNICATE_HEADER
>>>>
>>>> Hey Kun,
>>>>
>>>> Why would you rely on undefined behaviours? The OFFSET_OF macro is
>>>> well-defined for GCC and Clang as it's implemented by an intrinsic, and
>>>> while the expression for the MSVC compiler is undefined behaviour as per
>>>> the C standard, it is well-defined for MSVC due to their own
>>>> implementation being identical. From my standpoint, all supported
>>>> compilers will yield well-defined behaviour even this way. OFFSET_OF on
>>>> flexible arrays is not UB in any case to my knowledge.
>>>>
>>>> However, the same way as your new suggestion, you can replace OFFSET_OF
>>>> with sizeof. While this *can* lead to wasted space with certain
>>>> structure layouts (e.g. when the flexible array overlays padding bytes),
>>>> this is not the case here, and otherwise just loses you a few bytes. I
>>>> think this comes down to preference.
>>>>
>>>> The pattern you mentioned arguably is less nice syntax when used
>>>> (involves address calculation and casting), but the biggest problem here
>>>> is alignment constraints. For packed structures, you lose the ability of
>>>> automatic unaligned accesses (irrelevant here because the structure is
>>>> manually padded anyway). For non-packed structures, you still need to
>>>> ensure the alignment requirement of the trailing array data is met
>>>> manually. With flexible array members, the compiler takes care of both
>>>> cases automatically.
>>>>
>>>> Best regards,
>>>> Marvin
>>>>
>>>> On 24.06.21 02:24, Kun Qin wrote:
>>>>> Hi Marvin,
>>>>>
>>>>> I would prefer not to rely on undefined behaviors from different
>>>>> compilers. Instead of using flexible arrays, is it better to remove
>>>>> the `Data` field, pack the structure and follow
>>>>> "VARIABLE_LOCK_ON_VAR_STATE_POLICY" pattern?
>>>>>
>>>>> In that case, OFFSET_OF will be forced to change to sizeof, and
>>>>> read/write to `Data` will follow the range indicated by MessageLength.
>>>>> But yes, that will enforce developers to update their platform level
>>>>> implementations accordingly.
>>>>>
>>>>> Regards,
>>>>> Kun
>>>>>
>>>>> On 06/23/2021 08:26, Laszlo Ersek wrote:
>>>>>> On 06/23/21 08:54, Marvin Häuser wrote:
>>>>>>> On 22.06.21 17:34, Laszlo Ersek wrote:
>>>>>>>> On 06/18/21 11:37, Marvin Häuser wrote:
>>>>>>>>> On 16.06.21 22:58, Kun Qin wrote:
>>>>>>>>>> On 06/16/2021 00:02, Marvin Häuser wrote:
>>>>>>>>>>> 2) Is it feasible yet with the current set of supported
>>>>>>>>>>> compilers to
>>>>>>>>>>> support flexible arrays?
>>>>>>>>>> My impression is that flexible arrays are already supported (as
>>>>>>>>>> seen
>>>>>>>>>> in UnitTestFrameworkPkg/PrivateInclude/UnitTestFrameworkTypes.h).
>>>>>>>>>> Please correct me if I am wrong.
>>>>>>>>>>
>>>>>>>>>> Would you mind letting me know why this is applicable here? We are
>>>>>>>>>> trying to seek ideas on how to catch developer mistakes caused by
>>>>>>>>>> this
>>>>>>>>>> change. So any input is appreciated.
>>>>>>>>> Huh, interesting. Last time I tried I was told about
>>>>>>>>> incompatibilities
>>>>>>>>> with MSVC, but I know some have been dropped since then (2005 and
>>>>>>>>> 2008
>>>>>>>>> if I recall correctly?), so that'd be great to allow globally.
>>>>>>>> I too am surprised to see
>>>>>>>> "UnitTestFrameworkPkg/PrivateInclude/UnitTestFrameworkTypes.h". The
>>>>>>>> flexible array member is a C99 feature, and I didn't even know
>>>>>>>> that we
>>>>>>>> disallowed it for the sake of particular VS toolchains -- I
>>>>>>>> thought we
>>>>>>>> had a more general reason than just "not supported by VS versions X
>>>>>>>> and Y".
>>>>>>>>
>>>>>>>> The behavior of OFFSET_OF() would be interesting -- the OFFSET_OF()
>>>>>>>> macro definition for non-gcc / non-clang:
>>>>>>>>
>>>>>>>> #define OFFSET_OF(TYPE, Field) ((UINTN) &(((TYPE *)0)->Field))
>>>>>>>>
>>>>>>>> borders on undefined behavior as far as I can tell, so its
>>>>>>>> behavior is
>>>>>>>> totally up to the compiler. It works thus far okay on Visual
>>>>>>>> Studio, but
>>>>>>>> I couldn't say if it extended correctly to flexible array members.
>>>>>>> Yes, it's UB by the standard, but this is actually how MS implements
>>>>>>> them (or used to anyway?). I don't see why it'd cause issues with
>>>>>>> flexible arrays, as only the start of the array is relevant (which is
>>>>>>> constant for all instances of the structure no matter the amount of
>>>>>>> elements actually stored). Any specific concern? If so, they could be
>>>>>>> addressed by appropriate STATIC_ASSERTs.
>>>>>> No specific concern; my point was that two aspects of the same "class"
>>>>>> of undefined behavior didn't need to be consistent with each other.
>>>>>>
>>>>>> Thanks
>>>>>> Laszlo
>>>>>>

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

[Bret Barkelew]

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

The fact that it may vary per ABI seems like a pretty big gotcha if the SMM/MM Core was compiled at a different time or on a different system than the module that’s invoking the communication.

- Bret

From: Marvin Häuser<mailto:mhaeuser@posteo.de>
Sent: Monday, June 28, 2021 8:43 AM
To: Laszlo Ersek<mailto:lersek@redhat.com>; Kun Qin<mailto:kuqin12@gmail.com>; Kinney, Michael D<mailto:michael.d.kinney@intel.com>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>
Cc: Wang, Jian J<mailto:jian.j.wang@intel.com>; Wu, Hao A<mailto:hao.a.wu@intel.com>; Dong, Eric<mailto:eric.dong@intel.com>; Ni, Ray<mailto:ray.ni@intel.com>; Liming Gao<mailto:gaoliming@byosoft.com.cn>; Liu, Zhiguang<mailto:zhiguang.liu@intel.com>; Andrew Fish<mailto:afish@apple.com>; Lindholm, Leif<mailto:leif@nuviainc.com>; Bret Barkelew<mailto:Bret.Barkelew@microsoft.com>; Michael Kubacki<mailto:Michael.Kubacki@microsoft.com>
Subject: [EXTERNAL] Re: [edk2-devel] [PATCH v1 0/5] EDK2 Code First: PI Specification: Update EFI_MM_COMMUNICATE_HEADER

On 28.06.21 16:57, Laszlo Ersek wrote:
> On 06/25/21 20:47, Kun Qin wrote:
>> Hi Mike,
>>
>> Thanks for the information. I can update the patch and proposed spec
>> change to use flexible array in v-next if there is no other concerns.
>>
>> After switching to flexible array, OFFSET_OF (Data) should lead to the
>> same result as sizeof.
> This may be true on specific compilers, but it is *not* guaranteed by
> the C language standard.

Sorry, for completeness sake... :)

I don't think it really depends on the compiler (but can vary per ABI),
but it's a conceptual issue with alignment requirements. Assuming
natural alignment and the usual stuff, for "struct s { uint64_t a;
uint32_t b; uint8_t c[]; }" the alignment requirement is 8 Bytes, where
there are 4 Bytes of padding after "b" (as "c" may as well be empty).
"c" however is guaranteed to start after b in the same fashion as if it
was declared with the maximum amount of elements that fit the memory. So
if we take 4 elements for "c", and note that "c" has an alignment
requirement of 1 Byte, c[0 .. 3] will alias the padding after "b". For
"sizeof" this means that the padding is included, whereas for "offsetof"
it is not, yielding "sizeof(struct s) == offsetof(struct s, c) + 4".
That is what I meant by "wasted space" earlier, but this could possibly
be made nicer with macros as necessary.

As for this specific struct, the values should be identical as it is padded.

Best regards,
Marvin

>
> Quoting C99 6.7.2.1 "Structure and union specifiers", paragraph 16:
>
> "In most situations, the flexible array member is ignored. In
> particular, the size of the structure is as if the flexible array member
> were omitted except that it may have more trailing padding than the
> omission would imply."
>
> Quoting footnotes 17 and 19,
>
> (17)  [...]
>        struct s { int n; double d[]; };
>        [...]
>
> (19)  [...]
>        the expressions:
>        [...]
>        sizeof (struct s) >= offsetof(struct s, d)
>
>        are always equal to 1.
>
> Thanks
> Laszlo
>
>
>
>> While sizeof would be a preferred way to move
>> forward.
>>
>> Regards,
>> Kun
>>
>> On 06/24/2021 08:25, Kinney, Michael D wrote:
>>> Hello,
>>>
>>> Flexible array members are supported and should be used.  The old style
>>> of adding an array of size [1] at the end of a structure was used at a
>>> time
>>> flexible array members were not supported by all compilers (late 1990's).
>>> The workarounds used to handle the array of size [1] are very
>>> confusing when
>>> reading the C  code and the fact that sizeof() does not produce the
>>> expected
>>> result make it even worse.
>>>
>>> If we use flexible array members in this proposed change then there is
>>> no need to use OFFSET_OF().  Correct?
>>>
>>> Mike
>>>
>>>
>>>> -----Original Message-----
>>>> From: Marvin Häuser <mhaeuser@posteo.de>
>>>> Sent: Thursday, June 24, 2021 1:00 AM
>>>> To: Kun Qin <kuqin12@gmail.com>; Laszlo Ersek <lersek@redhat.com>;
>>>> 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>; Kinney, Michael D <michael.d.kinney@intel.com>;
>>>> Liming Gao <gaoliming@byosoft.com.cn>; Liu, Zhiguang
>>>> <zhiguang.liu@intel.com>; Andrew Fish <afish@apple.com>; Leif
>>>> Lindholm <leif@nuviainc.com>; Bret Barkelew
>>>> <Bret.Barkelew@microsoft.com>; michael.kubacki@microsoft.com
>>>> Subject: Re: [edk2-devel] [PATCH v1 0/5] EDK2 Code First: PI
>>>> Specification: Update EFI_MM_COMMUNICATE_HEADER
>>>>
>>>> Hey Kun,
>>>>
>>>> Why would you rely on undefined behaviours? The OFFSET_OF macro is
>>>> well-defined for GCC and Clang as it's implemented by an intrinsic, and
>>>> while the expression for the MSVC compiler is undefined behaviour as per
>>>> the C standard, it is well-defined for MSVC due to their own
>>>> implementation being identical. From my standpoint, all supported
>>>> compilers will yield well-defined behaviour even this way. OFFSET_OF on
>>>> flexible arrays is not UB in any case to my knowledge.
>>>>
>>>> However, the same way as your new suggestion, you can replace OFFSET_OF
>>>> with sizeof. While this *can* lead to wasted space with certain
>>>> structure layouts (e.g. when the flexible array overlays padding bytes),
>>>> this is not the case here, and otherwise just loses you a few bytes. I
>>>> think this comes down to preference.
>>>>
>>>> The pattern you mentioned arguably is less nice syntax when used
>>>> (involves address calculation and casting), but the biggest problem here
>>>> is alignment constraints. For packed structures, you lose the ability of
>>>> automatic unaligned accesses (irrelevant here because the structure is
>>>> manually padded anyway). For non-packed structures, you still need to
>>>> ensure the alignment requirement of the trailing array data is met
>>>> manually. With flexible array members, the compiler takes care of both
>>>> cases automatically.
>>>>
>>>> Best regards,
>>>> Marvin
>>>>
>>>> On 24.06.21 02:24, Kun Qin wrote:
>>>>> Hi Marvin,
>>>>>
>>>>> I would prefer not to rely on undefined behaviors from different
>>>>> compilers. Instead of using flexible arrays, is it better to remove
>>>>> the `Data` field, pack the structure and follow
>>>>> "VARIABLE_LOCK_ON_VAR_STATE_POLICY" pattern?
>>>>>
>>>>> In that case, OFFSET_OF will be forced to change to sizeof, and
>>>>> read/write to `Data` will follow the range indicated by MessageLength.
>>>>> But yes, that will enforce developers to update their platform level
>>>>> implementations accordingly.
>>>>>
>>>>> Regards,
>>>>> Kun
>>>>>
>>>>> On 06/23/2021 08:26, Laszlo Ersek wrote:
>>>>>> On 06/23/21 08:54, Marvin Häuser wrote:
>>>>>>> On 22.06.21 17:34, Laszlo Ersek wrote:
>>>>>>>> On 06/18/21 11:37, Marvin Häuser wrote:
>>>>>>>>> On 16.06.21 22:58, Kun Qin wrote:
>>>>>>>>>> On 06/16/2021 00:02, Marvin Häuser wrote:
>>>>>>>>>>> 2) Is it feasible yet with the current set of supported
>>>>>>>>>>> compilers to
>>>>>>>>>>> support flexible arrays?
>>>>>>>>>> My impression is that flexible arrays are already supported (as
>>>>>>>>>> seen
>>>>>>>>>> in UnitTestFrameworkPkg/PrivateInclude/UnitTestFrameworkTypes.h).
>>>>>>>>>> Please correct me if I am wrong.
>>>>>>>>>>
>>>>>>>>>> Would you mind letting me know why this is applicable here? We are
>>>>>>>>>> trying to seek ideas on how to catch developer mistakes caused by
>>>>>>>>>> this
>>>>>>>>>> change. So any input is appreciated.
>>>>>>>>> Huh, interesting. Last time I tried I was told about
>>>>>>>>> incompatibilities
>>>>>>>>> with MSVC, but I know some have been dropped since then (2005 and
>>>>>>>>> 2008
>>>>>>>>> if I recall correctly?), so that'd be great to allow globally.
>>>>>>>> I too am surprised to see
>>>>>>>> "UnitTestFrameworkPkg/PrivateInclude/UnitTestFrameworkTypes.h". The
>>>>>>>> flexible array member is a C99 feature, and I didn't even know
>>>>>>>> that we
>>>>>>>> disallowed it for the sake of particular VS toolchains -- I
>>>>>>>> thought we
>>>>>>>> had a more general reason than just "not supported by VS versions X
>>>>>>>> and Y".
>>>>>>>>
>>>>>>>> The behavior of OFFSET_OF() would be interesting -- the OFFSET_OF()
>>>>>>>> macro definition for non-gcc / non-clang:
>>>>>>>>
>>>>>>>> #define OFFSET_OF(TYPE, Field) ((UINTN) &(((TYPE *)0)->Field))
>>>>>>>>
>>>>>>>> borders on undefined behavior as far as I can tell, so its
>>>>>>>> behavior is
>>>>>>>> totally up to the compiler. It works thus far okay on Visual
>>>>>>>> Studio, but
>>>>>>>> I couldn't say if it extended correctly to flexible array members.
>>>>>>> Yes, it's UB by the standard, but this is actually how MS implements
>>>>>>> them (or used to anyway?). I don't see why it'd cause issues with
>>>>>>> flexible arrays, as only the start of the array is relevant (which is
>>>>>>> constant for all instances of the structure no matter the amount of
>>>>>>> elements actually stored). Any specific concern? If so, they could be
>>>>>>> addressed by appropriate STATIC_ASSERTs.
>>>>>> No specific concern; my point was that two aspects of the same "class"
>>>>>> of undefined behavior didn't need to be consistent with each other.
>>>>>>
>>>>>> Thanks
>>>>>> Laszlo
>>>>>>


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

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

[Marvin Häuser]

Generally yes, but gladly not for EDK II. Default GNU ABI uses 32-bit 
alignment for 64-bit integers on IA32 (which led to a (non-critical) 
mistake in our PE paper :( ) for example, but UEFI / EDK II (seem to) 
successfully dictate natural alignment consistently. Possibly we could 
introduce some STATIC_ASSERTs around important cases (e.g. UINT64 on 
32-bit platforms) to ensure compilers keep it that way, once the ALIGNOF 
macro is introduced.

Best regards,
Marvin

On 29.06.21 08:49, Bret Barkelew wrote:
>
> The fact that it may vary per ABI seems like a pretty big gotcha if 
> the SMM/MM Core was compiled at a different time or on a different 
> system than the module that’s invoking the communication.
>
> - Bret
>
> *From: *Marvin Häuser <mailto:mhaeuser@posteo.de>
> *Sent: *Monday, June 28, 2021 8:43 AM
> *To: *Laszlo Ersek <mailto:lersek@redhat.com>; Kun Qin 
> <mailto:kuqin12@gmail.com>; Kinney, Michael D 
> <mailto:michael.d.kinney@intel.com>; devel@edk2.groups.io 
> <mailto:devel@edk2.groups.io>
> *Cc: *Wang, Jian J <mailto:jian.j.wang@intel.com>; Wu, Hao A 
> <mailto:hao.a.wu@intel.com>; Dong, Eric <mailto:eric.dong@intel.com>; 
> Ni, Ray <mailto:ray.ni@intel.com>; Liming Gao 
> <mailto:gaoliming@byosoft.com.cn>; Liu, Zhiguang 
> <mailto:zhiguang.liu@intel.com>; Andrew Fish <mailto:afish@apple.com>; 
> Lindholm, Leif <mailto:leif@nuviainc.com>; Bret Barkelew 
> <mailto:Bret.Barkelew@microsoft.com>; Michael Kubacki 
> <mailto:Michael.Kubacki@microsoft.com>
> *Subject: *[EXTERNAL] Re: [edk2-devel] [PATCH v1 0/5] EDK2 Code First: 
> PI Specification: Update EFI_MM_COMMUNICATE_HEADER
>
> On 28.06.21 16:57, Laszlo Ersek wrote:
> > On 06/25/21 20:47, Kun Qin wrote:
> >> Hi Mike,
> >>
> >> Thanks for the information. I can update the patch and proposed spec
> >> change to use flexible array in v-next if there is no other concerns.
> >>
> >> After switching to flexible array, OFFSET_OF (Data) should lead to the
> >> same result as sizeof.
> > This may be true on specific compilers, but it is *not* guaranteed by
> > the C language standard.
>
> Sorry, for completeness sake... :)
>
> I don't think it really depends on the compiler (but can vary per ABI),
> but it's a conceptual issue with alignment requirements. Assuming
> natural alignment and the usual stuff, for "struct s { uint64_t a;
> uint32_t b; uint8_t c[]; }" the alignment requirement is 8 Bytes, where
> there are 4 Bytes of padding after "b" (as "c" may as well be empty).
> "c" however is guaranteed to start after b in the same fashion as if it
> was declared with the maximum amount of elements that fit the memory. So
> if we take 4 elements for "c", and note that "c" has an alignment
> requirement of 1 Byte, c[0 .. 3] will alias the padding after "b". For
> "sizeof" this means that the padding is included, whereas for "offsetof"
> it is not, yielding "sizeof(struct s) == offsetof(struct s, c) + 4".
> That is what I meant by "wasted space" earlier, but this could possibly
> be made nicer with macros as necessary.
>
> As for this specific struct, the values should be identical as it is 
> padded.
>
> Best regards,
> Marvin
>
> >
> > Quoting C99 6.7.2.1 "Structure and union specifiers", paragraph 16:
> >
> > "In most situations, the flexible array member is ignored. In
> > particular, the size of the structure is as if the flexible array member
> > were omitted except that it may have more trailing padding than the
> > omission would imply."
> >
> > Quoting footnotes 17 and 19,
> >
> > (17)  [...]
> >        struct s { int n; double d[]; };
> >        [...]
> >
> > (19)  [...]
> >        the expressions:
> >        [...]
> >        sizeof (struct s) >= offsetof(struct s, d)
> >
> >        are always equal to 1.
> >
> > Thanks
> > Laszlo
> >
> >
> >
> >> While sizeof would be a preferred way to move
> >> forward.
> >>
> >> Regards,
> >> Kun
> >>
> >> On 06/24/2021 08:25, Kinney, Michael D wrote:
> >>> Hello,
> >>>
> >>> Flexible array members are supported and should be used.  The old 
> style
> >>> of adding an array of size [1] at the end of a structure was used at a
> >>> time
> >>> flexible array members were not supported by all compilers (late 
> 1990's).
> >>> The workarounds used to handle the array of size [1] are very
> >>> confusing when
> >>> reading the C  code and the fact that sizeof() does not produce the
> >>> expected
> >>> result make it even worse.
> >>>
> >>> If we use flexible array members in this proposed change then there is
> >>> no need to use OFFSET_OF().  Correct?
> >>>
> >>> Mike
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: Marvin Häuser <mhaeuser@posteo.de>
> >>>> Sent: Thursday, June 24, 2021 1:00 AM
> >>>> To: Kun Qin <kuqin12@gmail.com>; Laszlo Ersek <lersek@redhat.com>;
> >>>> 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>; Kinney, Michael D <michael.d.kinney@intel.com>;
> >>>> Liming Gao <gaoliming@byosoft.com.cn>; Liu, Zhiguang
> >>>> <zhiguang.liu@intel.com>; Andrew Fish <afish@apple.com>; Leif
> >>>> Lindholm <leif@nuviainc.com>; Bret Barkelew
> >>>> <Bret.Barkelew@microsoft.com>; michael.kubacki@microsoft.com
> >>>> Subject: Re: [edk2-devel] [PATCH v1 0/5] EDK2 Code First: PI
> >>>> Specification: Update EFI_MM_COMMUNICATE_HEADER
> >>>>
> >>>> Hey Kun,
> >>>>
> >>>> Why would you rely on undefined behaviours? The OFFSET_OF macro is
> >>>> well-defined for GCC and Clang as it's implemented by an 
> intrinsic, and
> >>>> while the expression for the MSVC compiler is undefined behaviour 
> as per
> >>>> the C standard, it is well-defined for MSVC due to their own
> >>>> implementation being identical. From my standpoint, all supported
> >>>> compilers will yield well-defined behaviour even this way. 
> OFFSET_OF on
> >>>> flexible arrays is not UB in any case to my knowledge.
> >>>>
> >>>> However, the same way as your new suggestion, you can replace 
> OFFSET_OF
> >>>> with sizeof. While this *can* lead to wasted space with certain
> >>>> structure layouts (e.g. when the flexible array overlays padding 
> bytes),
> >>>> this is not the case here, and otherwise just loses you a few 
> bytes. I
> >>>> think this comes down to preference.
> >>>>
> >>>> The pattern you mentioned arguably is less nice syntax when used
> >>>> (involves address calculation and casting), but the biggest 
> problem here
> >>>> is alignment constraints. For packed structures, you lose the 
> ability of
> >>>> automatic unaligned accesses (irrelevant here because the 
> structure is
> >>>> manually padded anyway). For non-packed structures, you still need to
> >>>> ensure the alignment requirement of the trailing array data is met
> >>>> manually. With flexible array members, the compiler takes care of 
> both
> >>>> cases automatically.
> >>>>
> >>>> Best regards,
> >>>> Marvin
> >>>>
> >>>> On 24.06.21 02:24, Kun Qin wrote:
> >>>>> Hi Marvin,
> >>>>>
> >>>>> I would prefer not to rely on undefined behaviors from different
> >>>>> compilers. Instead of using flexible arrays, is it better to remove
> >>>>> the `Data` field, pack the structure and follow
> >>>>> "VARIABLE_LOCK_ON_VAR_STATE_POLICY" pattern?
> >>>>>
> >>>>> In that case, OFFSET_OF will be forced to change to sizeof, and
> >>>>> read/write to `Data` will follow the range indicated by 
> MessageLength.
> >>>>> But yes, that will enforce developers to update their platform level
> >>>>> implementations accordingly.
> >>>>>
> >>>>> Regards,
> >>>>> Kun
> >>>>>
> >>>>> On 06/23/2021 08:26, Laszlo Ersek wrote:
> >>>>>> On 06/23/21 08:54, Marvin Häuser wrote:
> >>>>>>> On 22.06.21 17:34, Laszlo Ersek wrote:
> >>>>>>>> On 06/18/21 11:37, Marvin Häuser wrote:
> >>>>>>>>> On 16.06.21 22:58, Kun Qin wrote:
> >>>>>>>>>> On 06/16/2021 00:02, Marvin Häuser wrote:
> >>>>>>>>>>> 2) Is it feasible yet with the current set of supported
> >>>>>>>>>>> compilers to
> >>>>>>>>>>> support flexible arrays?
> >>>>>>>>>> My impression is that flexible arrays are already supported (as
> >>>>>>>>>> seen
> >>>>>>>>>> in 
> UnitTestFrameworkPkg/PrivateInclude/UnitTestFrameworkTypes.h).
> >>>>>>>>>> Please correct me if I am wrong.
> >>>>>>>>>>
> >>>>>>>>>> Would you mind letting me know why this is applicable here? 
> We are
> >>>>>>>>>> trying to seek ideas on how to catch developer mistakes 
> caused by
> >>>>>>>>>> this
> >>>>>>>>>> change. So any input is appreciated.
> >>>>>>>>> Huh, interesting. Last time I tried I was told about
> >>>>>>>>> incompatibilities
> >>>>>>>>> with MSVC, but I know some have been dropped since then 
> (2005 and
> >>>>>>>>> 2008
> >>>>>>>>> if I recall correctly?), so that'd be great to allow globally.
> >>>>>>>> I too am surprised to see
> >>>>>>>> 
> "UnitTestFrameworkPkg/PrivateInclude/UnitTestFrameworkTypes.h". The
> >>>>>>>> flexible array member is a C99 feature, and I didn't even know
> >>>>>>>> that we
> >>>>>>>> disallowed it for the sake of particular VS toolchains -- I
> >>>>>>>> thought we
> >>>>>>>> had a more general reason than just "not supported by VS 
> versions X
> >>>>>>>> and Y".
> >>>>>>>>
> >>>>>>>> The behavior of OFFSET_OF() would be interesting -- the 
> OFFSET_OF()
> >>>>>>>> macro definition for non-gcc / non-clang:
> >>>>>>>>
> >>>>>>>> #define OFFSET_OF(TYPE, Field) ((UINTN) &(((TYPE *)0)->Field))
> >>>>>>>>
> >>>>>>>> borders on undefined behavior as far as I can tell, so its
> >>>>>>>> behavior is
> >>>>>>>> totally up to the compiler. It works thus far okay on Visual
> >>>>>>>> Studio, but
> >>>>>>>> I couldn't say if it extended correctly to flexible array 
> members.
> >>>>>>> Yes, it's UB by the standard, but this is actually how MS 
> implements
> >>>>>>> them (or used to anyway?). I don't see why it'd cause issues with
> >>>>>>> flexible arrays, as only the start of the array is relevant 
> (which is
> >>>>>>> constant for all instances of the structure no matter the 
> amount of
> >>>>>>> elements actually stored). Any specific concern? If so, they 
> could be
> >>>>>>> addressed by appropriate STATIC_ASSERTs.
> >>>>>> No specific concern; my point was that two aspects of the same 
> "class"
> >>>>>> of undefined behavior didn't need to be consistent with each other.
> >>>>>>
> >>>>>> Thanks
> >>>>>> Laszlo
> >>>>>>
>

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

[Bret Barkelew]

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

Good note. Thanks!

- Bret

From: Marvin Häuser<mailto:mhaeuser@posteo.de>
Sent: Tuesday, June 29, 2021 1:58 AM
To: Bret Barkelew<mailto:Bret.Barkelew@microsoft.com>; Laszlo Ersek<mailto:lersek@redhat.com>; Kun Qin<mailto:kuqin12@gmail.com>; Kinney, Michael D<mailto:michael.d.kinney@intel.com>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>
Cc: Wang, Jian J<mailto:jian.j.wang@intel.com>; Wu, Hao A<mailto:hao.a.wu@intel.com>; Dong, Eric<mailto:eric.dong@intel.com>; Ni, Ray<mailto:ray.ni@intel.com>; Liming Gao<mailto:gaoliming@byosoft.com.cn>; Liu, Zhiguang<mailto:zhiguang.liu@intel.com>; Andrew Fish<mailto:afish@apple.com>; Lindholm, Leif<mailto:leif@nuviainc.com>; Michael Kubacki<mailto:Michael.Kubacki@microsoft.com>
Subject: Re: [EXTERNAL] Re: [edk2-devel] [PATCH v1 0/5] EDK2 Code First: PI Specification: Update EFI_MM_COMMUNICATE_HEADER

Generally yes, but gladly not for EDK II. Default GNU ABI uses 32-bit
alignment for 64-bit integers on IA32 (which led to a (non-critical)
mistake in our PE paper :( ) for example, but UEFI / EDK II (seem to)
successfully dictate natural alignment consistently. Possibly we could
introduce some STATIC_ASSERTs around important cases (e.g. UINT64 on
32-bit platforms) to ensure compilers keep it that way, once the ALIGNOF
macro is introduced.

Best regards,
Marvin

On 29.06.21 08:49, Bret Barkelew wrote:
>
> The fact that it may vary per ABI seems like a pretty big gotcha if
> the SMM/MM Core was compiled at a different time or on a different
> system than the module that’s invoking the communication.
>
> - Bret
>
> *From: *Marvin Häuser <mailto:mhaeuser@posteo.de>
> *Sent: *Monday, June 28, 2021 8:43 AM
> *To: *Laszlo Ersek <mailto:lersek@redhat.com>; Kun Qin
> <mailto:kuqin12@gmail.com>; Kinney, Michael D
> <mailto:michael.d.kinney@intel.com>; devel@edk2.groups.io
> <mailto:devel@edk2.groups.io>
> *Cc: *Wang, Jian J <mailto:jian.j.wang@intel.com>; Wu, Hao A
> <mailto:hao.a.wu@intel.com>; Dong, Eric <mailto:eric.dong@intel.com>;
> Ni, Ray <mailto:ray.ni@intel.com>; Liming Gao
> <mailto:gaoliming@byosoft.com.cn>; Liu, Zhiguang
> <mailto:zhiguang.liu@intel.com>; Andrew Fish <mailto:afish@apple.com>;
> Lindholm, Leif <mailto:leif@nuviainc.com>; Bret Barkelew
> <mailto:Bret.Barkelew@microsoft.com>; Michael Kubacki
> <mailto:Michael.Kubacki@microsoft.com>
> *Subject: *[EXTERNAL] Re: [edk2-devel] [PATCH v1 0/5] EDK2 Code First:
> PI Specification: Update EFI_MM_COMMUNICATE_HEADER
>
> On 28.06.21 16:57, Laszlo Ersek wrote:
> > On 06/25/21 20:47, Kun Qin wrote:
> >> Hi Mike,
> >>
> >> Thanks for the information. I can update the patch and proposed spec
> >> change to use flexible array in v-next if there is no other concerns.
> >>
> >> After switching to flexible array, OFFSET_OF (Data) should lead to the
> >> same result as sizeof.
> > This may be true on specific compilers, but it is *not* guaranteed by
> > the C language standard.
>
> Sorry, for completeness sake... :)
>
> I don't think it really depends on the compiler (but can vary per ABI),
> but it's a conceptual issue with alignment requirements. Assuming
> natural alignment and the usual stuff, for "struct s { uint64_t a;
> uint32_t b; uint8_t c[]; }" the alignment requirement is 8 Bytes, where
> there are 4 Bytes of padding after "b" (as "c" may as well be empty).
> "c" however is guaranteed to start after b in the same fashion as if it
> was declared with the maximum amount of elements that fit the memory. So
> if we take 4 elements for "c", and note that "c" has an alignment
> requirement of 1 Byte, c[0 .. 3] will alias the padding after "b". For
> "sizeof" this means that the padding is included, whereas for "offsetof"
> it is not, yielding "sizeof(struct s) == offsetof(struct s, c) + 4".
> That is what I meant by "wasted space" earlier, but this could possibly
> be made nicer with macros as necessary.
>
> As for this specific struct, the values should be identical as it is
> padded.
>
> Best regards,
> Marvin
>
> >
> > Quoting C99 6.7.2.1 "Structure and union specifiers", paragraph 16:
> >
> > "In most situations, the flexible array member is ignored. In
> > particular, the size of the structure is as if the flexible array member
> > were omitted except that it may have more trailing padding than the
> > omission would imply."
> >
> > Quoting footnotes 17 and 19,
> >
> > (17)  [...]
> >        struct s { int n; double d[]; };
> >        [...]
> >
> > (19)  [...]
> >        the expressions:
> >        [...]
> >        sizeof (struct s) >= offsetof(struct s, d)
> >
> >        are always equal to 1.
> >
> > Thanks
> > Laszlo
> >
> >
> >
> >> While sizeof would be a preferred way to move
> >> forward.
> >>
> >> Regards,
> >> Kun
> >>
> >> On 06/24/2021 08:25, Kinney, Michael D wrote:
> >>> Hello,
> >>>
> >>> Flexible array members are supported and should be used.  The old
> style
> >>> of adding an array of size [1] at the end of a structure was used at a
> >>> time
> >>> flexible array members were not supported by all compilers (late
> 1990's).
> >>> The workarounds used to handle the array of size [1] are very
> >>> confusing when
> >>> reading the C  code and the fact that sizeof() does not produce the
> >>> expected
> >>> result make it even worse.
> >>>
> >>> If we use flexible array members in this proposed change then there is
> >>> no need to use OFFSET_OF().  Correct?
> >>>
> >>> Mike
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: Marvin Häuser <mhaeuser@posteo.de>
> >>>> Sent: Thursday, June 24, 2021 1:00 AM
> >>>> To: Kun Qin <kuqin12@gmail.com>; Laszlo Ersek <lersek@redhat.com>;
> >>>> 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>; Kinney, Michael D <michael.d.kinney@intel.com>;
> >>>> Liming Gao <gaoliming@byosoft.com.cn>; Liu, Zhiguang
> >>>> <zhiguang.liu@intel.com>; Andrew Fish <afish@apple.com>; Leif
> >>>> Lindholm <leif@nuviainc.com>; Bret Barkelew
> >>>> <Bret.Barkelew@microsoft.com>; michael.kubacki@microsoft.com
> >>>> Subject: Re: [edk2-devel] [PATCH v1 0/5] EDK2 Code First: PI
> >>>> Specification: Update EFI_MM_COMMUNICATE_HEADER
> >>>>
> >>>> Hey Kun,
> >>>>
> >>>> Why would you rely on undefined behaviours? The OFFSET_OF macro is
> >>>> well-defined for GCC and Clang as it's implemented by an
> intrinsic, and
> >>>> while the expression for the MSVC compiler is undefined behaviour
> as per
> >>>> the C standard, it is well-defined for MSVC due to their own
> >>>> implementation being identical. From my standpoint, all supported
> >>>> compilers will yield well-defined behaviour even this way.
> OFFSET_OF on
> >>>> flexible arrays is not UB in any case to my knowledge.
> >>>>
> >>>> However, the same way as your new suggestion, you can replace
> OFFSET_OF
> >>>> with sizeof. While this *can* lead to wasted space with certain
> >>>> structure layouts (e.g. when the flexible array overlays padding
> bytes),
> >>>> this is not the case here, and otherwise just loses you a few
> bytes. I
> >>>> think this comes down to preference.
> >>>>
> >>>> The pattern you mentioned arguably is less nice syntax when used
> >>>> (involves address calculation and casting), but the biggest
> problem here
> >>>> is alignment constraints. For packed structures, you lose the
> ability of
> >>>> automatic unaligned accesses (irrelevant here because the
> structure is
> >>>> manually padded anyway). For non-packed structures, you still need to
> >>>> ensure the alignment requirement of the trailing array data is met
> >>>> manually. With flexible array members, the compiler takes care of
> both
> >>>> cases automatically.
> >>>>
> >>>> Best regards,
> >>>> Marvin
> >>>>
> >>>> On 24.06.21 02:24, Kun Qin wrote:
> >>>>> Hi Marvin,
> >>>>>
> >>>>> I would prefer not to rely on undefined behaviors from different
> >>>>> compilers. Instead of using flexible arrays, is it better to remove
> >>>>> the `Data` field, pack the structure and follow
> >>>>> "VARIABLE_LOCK_ON_VAR_STATE_POLICY" pattern?
> >>>>>
> >>>>> In that case, OFFSET_OF will be forced to change to sizeof, and
> >>>>> read/write to `Data` will follow the range indicated by
> MessageLength.
> >>>>> But yes, that will enforce developers to update their platform level
> >>>>> implementations accordingly.
> >>>>>
> >>>>> Regards,
> >>>>> Kun
> >>>>>
> >>>>> On 06/23/2021 08:26, Laszlo Ersek wrote:
> >>>>>> On 06/23/21 08:54, Marvin Häuser wrote:
> >>>>>>> On 22.06.21 17:34, Laszlo Ersek wrote:
> >>>>>>>> On 06/18/21 11:37, Marvin Häuser wrote:
> >>>>>>>>> On 16.06.21 22:58, Kun Qin wrote:
> >>>>>>>>>> On 06/16/2021 00:02, Marvin Häuser wrote:
> >>>>>>>>>>> 2) Is it feasible yet with the current set of supported
> >>>>>>>>>>> compilers to
> >>>>>>>>>>> support flexible arrays?
> >>>>>>>>>> My impression is that flexible arrays are already supported (as
> >>>>>>>>>> seen
> >>>>>>>>>> in
> UnitTestFrameworkPkg/PrivateInclude/UnitTestFrameworkTypes.h).
> >>>>>>>>>> Please correct me if I am wrong.
> >>>>>>>>>>
> >>>>>>>>>> Would you mind letting me know why this is applicable here?
> We are
> >>>>>>>>>> trying to seek ideas on how to catch developer mistakes
> caused by
> >>>>>>>>>> this
> >>>>>>>>>> change. So any input is appreciated.
> >>>>>>>>> Huh, interesting. Last time I tried I was told about
> >>>>>>>>> incompatibilities
> >>>>>>>>> with MSVC, but I know some have been dropped since then
> (2005 and
> >>>>>>>>> 2008
> >>>>>>>>> if I recall correctly?), so that'd be great to allow globally.
> >>>>>>>> I too am surprised to see
> >>>>>>>>
> "UnitTestFrameworkPkg/PrivateInclude/UnitTestFrameworkTypes.h". The
> >>>>>>>> flexible array member is a C99 feature, and I didn't even know
> >>>>>>>> that we
> >>>>>>>> disallowed it for the sake of particular VS toolchains -- I
> >>>>>>>> thought we
> >>>>>>>> had a more general reason than just "not supported by VS
> versions X
> >>>>>>>> and Y".
> >>>>>>>>
> >>>>>>>> The behavior of OFFSET_OF() would be interesting -- the
> OFFSET_OF()
> >>>>>>>> macro definition for non-gcc / non-clang:
> >>>>>>>>
> >>>>>>>> #define OFFSET_OF(TYPE, Field) ((UINTN) &(((TYPE *)0)->Field))
> >>>>>>>>
> >>>>>>>> borders on undefined behavior as far as I can tell, so its
> >>>>>>>> behavior is
> >>>>>>>> totally up to the compiler. It works thus far okay on Visual
> >>>>>>>> Studio, but
> >>>>>>>> I couldn't say if it extended correctly to flexible array
> members.
> >>>>>>> Yes, it's UB by the standard, but this is actually how MS
> implements
> >>>>>>> them (or used to anyway?). I don't see why it'd cause issues with
> >>>>>>> flexible arrays, as only the start of the array is relevant
> (which is
> >>>>>>> constant for all instances of the structure no matter the
> amount of
> >>>>>>> elements actually stored). Any specific concern? If so, they
> could be
> >>>>>>> addressed by appropriate STATIC_ASSERTs.
> >>>>>> No specific concern; my point was that two aspects of the same
> "class"
> >>>>>> of undefined behavior didn't need to be consistent with each other.
> >>>>>>
> >>>>>> Thanks
> >>>>>> Laszlo
> >>>>>>
>


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

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

[Michael D Kinney]

Hi Marvin,

Thank you for checking this specific structure.

I think this should be part of the evaluation when proposing the use of a
flexible array member to the UEFI/PI specs as part of the EDK II Code First
Process.  We should verify that the sizeof() and offsetof() return the same
value when using the structure in all supported compilers/CPU archs configured
for the UEFI ABI.

Thanks,

Mike

> -----Original Message-----
> From: Marvin Häuser <mhaeuser@posteo.de>
> Sent: Monday, June 28, 2021 8:43 AM
> To: Laszlo Ersek <lersek@redhat.com>; Kun Qin <kuqin12@gmail.com>; Kinney, Michael D <michael.d.kinney@intel.com>;
> 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>; Liming Gao <gaoliming@byosoft.com.cn>; Liu, Zhiguang <zhiguang.liu@intel.com>; Andrew Fish
> <afish@apple.com>; Leif Lindholm <leif@nuviainc.com>; Bret Barkelew <Bret.Barkelew@microsoft.com>;
> michael.kubacki@microsoft.com
> Subject: Re: [edk2-devel] [PATCH v1 0/5] EDK2 Code First: PI Specification: Update EFI_MM_COMMUNICATE_HEADER
> 
> On 28.06.21 16:57, Laszlo Ersek wrote:
> > On 06/25/21 20:47, Kun Qin wrote:
> >> Hi Mike,
> >>
> >> Thanks for the information. I can update the patch and proposed spec
> >> change to use flexible array in v-next if there is no other concerns.
> >>
> >> After switching to flexible array, OFFSET_OF (Data) should lead to the
> >> same result as sizeof.
> > This may be true on specific compilers, but it is *not* guaranteed by
> > the C language standard.
> 
> Sorry, for completeness sake... :)
> 
> I don't think it really depends on the compiler (but can vary per ABI),
> but it's a conceptual issue with alignment requirements. Assuming
> natural alignment and the usual stuff, for "struct s { uint64_t a;
> uint32_t b; uint8_t c[]; }" the alignment requirement is 8 Bytes, where
> there are 4 Bytes of padding after "b" (as "c" may as well be empty).
> "c" however is guaranteed to start after b in the same fashion as if it
> was declared with the maximum amount of elements that fit the memory. So
> if we take 4 elements for "c", and note that "c" has an alignment
> requirement of 1 Byte, c[0 .. 3] will alias the padding after "b". For
> "sizeof" this means that the padding is included, whereas for "offsetof"
> it is not, yielding "sizeof(struct s) == offsetof(struct s, c) + 4".
> That is what I meant by "wasted space" earlier, but this could possibly
> be made nicer with macros as necessary.
> 
> As for this specific struct, the values should be identical as it is padded.
> 
> Best regards,
> Marvin
> 
> >
> > Quoting C99 6.7.2.1 "Structure and union specifiers", paragraph 16:
> >
> > "In most situations, the flexible array member is ignored. In
> > particular, the size of the structure is as if the flexible array member
> > were omitted except that it may have more trailing padding than the
> > omission would imply."
> >
> > Quoting footnotes 17 and 19,
> >
> > (17)  [...]
> >        struct s { int n; double d[]; };
> >        [...]
> >
> > (19)  [...]
> >        the expressions:
> >        [...]
> >        sizeof (struct s) >= offsetof(struct s, d)
> >
> >        are always equal to 1.
> >
> > Thanks
> > Laszlo
> >
> >
> >
> >> While sizeof would be a preferred way to move
> >> forward.
> >>
> >> Regards,
> >> Kun
> >>
> >> On 06/24/2021 08:25, Kinney, Michael D wrote:
> >>> Hello,
> >>>
> >>> Flexible array members are supported and should be used.  The old style
> >>> of adding an array of size [1] at the end of a structure was used at a
> >>> time
> >>> flexible array members were not supported by all compilers (late 1990's).
> >>> The workarounds used to handle the array of size [1] are very
> >>> confusing when
> >>> reading the C  code and the fact that sizeof() does not produce the
> >>> expected
> >>> result make it even worse.
> >>>
> >>> If we use flexible array members in this proposed change then there is
> >>> no need to use OFFSET_OF().  Correct?
> >>>
> >>> Mike
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: Marvin Häuser <mhaeuser@posteo.de>
> >>>> Sent: Thursday, June 24, 2021 1:00 AM
> >>>> To: Kun Qin <kuqin12@gmail.com>; Laszlo Ersek <lersek@redhat.com>;
> >>>> 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>; Kinney, Michael D <michael.d.kinney@intel.com>;
> >>>> Liming Gao <gaoliming@byosoft.com.cn>; Liu, Zhiguang
> >>>> <zhiguang.liu@intel.com>; Andrew Fish <afish@apple.com>; Leif
> >>>> Lindholm <leif@nuviainc.com>; Bret Barkelew
> >>>> <Bret.Barkelew@microsoft.com>; michael.kubacki@microsoft.com
> >>>> Subject: Re: [edk2-devel] [PATCH v1 0/5] EDK2 Code First: PI
> >>>> Specification: Update EFI_MM_COMMUNICATE_HEADER
> >>>>
> >>>> Hey Kun,
> >>>>
> >>>> Why would you rely on undefined behaviours? The OFFSET_OF macro is
> >>>> well-defined for GCC and Clang as it's implemented by an intrinsic, and
> >>>> while the expression for the MSVC compiler is undefined behaviour as per
> >>>> the C standard, it is well-defined for MSVC due to their own
> >>>> implementation being identical. From my standpoint, all supported
> >>>> compilers will yield well-defined behaviour even this way. OFFSET_OF on
> >>>> flexible arrays is not UB in any case to my knowledge.
> >>>>
> >>>> However, the same way as your new suggestion, you can replace OFFSET_OF
> >>>> with sizeof. While this *can* lead to wasted space with certain
> >>>> structure layouts (e.g. when the flexible array overlays padding bytes),
> >>>> this is not the case here, and otherwise just loses you a few bytes. I
> >>>> think this comes down to preference.
> >>>>
> >>>> The pattern you mentioned arguably is less nice syntax when used
> >>>> (involves address calculation and casting), but the biggest problem here
> >>>> is alignment constraints. For packed structures, you lose the ability of
> >>>> automatic unaligned accesses (irrelevant here because the structure is
> >>>> manually padded anyway). For non-packed structures, you still need to
> >>>> ensure the alignment requirement of the trailing array data is met
> >>>> manually. With flexible array members, the compiler takes care of both
> >>>> cases automatically.
> >>>>
> >>>> Best regards,
> >>>> Marvin
> >>>>
> >>>> On 24.06.21 02:24, Kun Qin wrote:
> >>>>> Hi Marvin,
> >>>>>
> >>>>> I would prefer not to rely on undefined behaviors from different
> >>>>> compilers. Instead of using flexible arrays, is it better to remove
> >>>>> the `Data` field, pack the structure and follow
> >>>>> "VARIABLE_LOCK_ON_VAR_STATE_POLICY" pattern?
> >>>>>
> >>>>> In that case, OFFSET_OF will be forced to change to sizeof, and
> >>>>> read/write to `Data` will follow the range indicated by MessageLength.
> >>>>> But yes, that will enforce developers to update their platform level
> >>>>> implementations accordingly.
> >>>>>
> >>>>> Regards,
> >>>>> Kun
> >>>>>
> >>>>> On 06/23/2021 08:26, Laszlo Ersek wrote:
> >>>>>> On 06/23/21 08:54, Marvin Häuser wrote:
> >>>>>>> On 22.06.21 17:34, Laszlo Ersek wrote:
> >>>>>>>> On 06/18/21 11:37, Marvin Häuser wrote:
> >>>>>>>>> On 16.06.21 22:58, Kun Qin wrote:
> >>>>>>>>>> On 06/16/2021 00:02, Marvin Häuser wrote:
> >>>>>>>>>>> 2) Is it feasible yet with the current set of supported
> >>>>>>>>>>> compilers to
> >>>>>>>>>>> support flexible arrays?
> >>>>>>>>>> My impression is that flexible arrays are already supported (as
> >>>>>>>>>> seen
> >>>>>>>>>> in UnitTestFrameworkPkg/PrivateInclude/UnitTestFrameworkTypes.h).
> >>>>>>>>>> Please correct me if I am wrong.
> >>>>>>>>>>
> >>>>>>>>>> Would you mind letting me know why this is applicable here? We are
> >>>>>>>>>> trying to seek ideas on how to catch developer mistakes caused by
> >>>>>>>>>> this
> >>>>>>>>>> change. So any input is appreciated.
> >>>>>>>>> Huh, interesting. Last time I tried I was told about
> >>>>>>>>> incompatibilities
> >>>>>>>>> with MSVC, but I know some have been dropped since then (2005 and
> >>>>>>>>> 2008
> >>>>>>>>> if I recall correctly?), so that'd be great to allow globally.
> >>>>>>>> I too am surprised to see
> >>>>>>>> "UnitTestFrameworkPkg/PrivateInclude/UnitTestFrameworkTypes.h". The
> >>>>>>>> flexible array member is a C99 feature, and I didn't even know
> >>>>>>>> that we
> >>>>>>>> disallowed it for the sake of particular VS toolchains -- I
> >>>>>>>> thought we
> >>>>>>>> had a more general reason than just "not supported by VS versions X
> >>>>>>>> and Y".
> >>>>>>>>
> >>>>>>>> The behavior of OFFSET_OF() would be interesting -- the OFFSET_OF()
> >>>>>>>> macro definition for non-gcc / non-clang:
> >>>>>>>>
> >>>>>>>> #define OFFSET_OF(TYPE, Field) ((UINTN) &(((TYPE *)0)->Field))
> >>>>>>>>
> >>>>>>>> borders on undefined behavior as far as I can tell, so its
> >>>>>>>> behavior is
> >>>>>>>> totally up to the compiler. It works thus far okay on Visual
> >>>>>>>> Studio, but
> >>>>>>>> I couldn't say if it extended correctly to flexible array members.
> >>>>>>> Yes, it's UB by the standard, but this is actually how MS implements
> >>>>>>> them (or used to anyway?). I don't see why it'd cause issues with
> >>>>>>> flexible arrays, as only the start of the array is relevant (which is
> >>>>>>> constant for all instances of the structure no matter the amount of
> >>>>>>> elements actually stored). Any specific concern? If so, they could be
> >>>>>>> addressed by appropriate STATIC_ASSERTs.
> >>>>>> No specific concern; my point was that two aspects of the same "class"
> >>>>>> of undefined behavior didn't need to be consistent with each other.
> >>>>>>
> >>>>>> Thanks
> >>>>>> Laszlo
> >>>>>>

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

[Michael D Kinney]

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

Good idea on use of STATIC_ASSERT().

For structures that use flexible array members, we can add a STATIC_ASSERT() for the sizeof() and OFFSET_OF() returning the same result.

For example:

    STATIC_ASSERT (sizeof (EFI_MM_COMMUNICATE_HEADER) == OFFSET_OF (EFI_MM_COMMUNICATE_HEADER, Data));

Mike

From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Bret Barkelew via groups.io
Sent: Tuesday, June 29, 2021 9:00 AM
To: Marvin Häuser <mhaeuser@posteo.de>; Laszlo Ersek <lersek@redhat.com>; Kun Qin <kuqin12@gmail.com>; Kinney, Michael D <michael.d.kinney@intel.com>; 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>; Liming Gao <gaoliming@byosoft.com.cn>; Liu, Zhiguang <zhiguang.liu@intel.com>; Andrew Fish <afish@apple.com>; Lindholm, Leif <leif@nuviainc.com>; Michael Kubacki <Michael.Kubacki@microsoft.com>
Subject: Re: [EXTERNAL] Re: [edk2-devel] [PATCH v1 0/5] EDK2 Code First: PI Specification: Update EFI_MM_COMMUNICATE_HEADER

Good note. Thanks!

- Bret

From: Marvin Häuser<mailto:mhaeuser@posteo.de>
Sent: Tuesday, June 29, 2021 1:58 AM
To: Bret Barkelew<mailto:Bret.Barkelew@microsoft.com>; Laszlo Ersek<mailto:lersek@redhat.com>; Kun Qin<mailto:kuqin12@gmail.com>; Kinney, Michael D<mailto:michael.d.kinney@intel.com>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>
Cc: Wang, Jian J<mailto:jian.j.wang@intel.com>; Wu, Hao A<mailto:hao.a.wu@intel.com>; Dong, Eric<mailto:eric.dong@intel.com>; Ni, Ray<mailto:ray.ni@intel.com>; Liming Gao<mailto:gaoliming@byosoft.com.cn>; Liu, Zhiguang<mailto:zhiguang.liu@intel.com>; Andrew Fish<mailto:afish@apple.com>; Lindholm, Leif<mailto:leif@nuviainc.com>; Michael Kubacki<mailto:Michael.Kubacki@microsoft.com>
Subject: Re: [EXTERNAL] Re: [edk2-devel] [PATCH v1 0/5] EDK2 Code First: PI Specification: Update EFI_MM_COMMUNICATE_HEADER

Generally yes, but gladly not for EDK II. Default GNU ABI uses 32-bit
alignment for 64-bit integers on IA32 (which led to a (non-critical)
mistake in our PE paper :( ) for example, but UEFI / EDK II (seem to)
successfully dictate natural alignment consistently. Possibly we could
introduce some STATIC_ASSERTs around important cases (e.g. UINT64 on
32-bit platforms) to ensure compilers keep it that way, once the ALIGNOF
macro is introduced.

Best regards,
Marvin

On 29.06.21 08:49, Bret Barkelew wrote:
>
> The fact that it may vary per ABI seems like a pretty big gotcha if
> the SMM/MM Core was compiled at a different time or on a different
> system than the module that’s invoking the communication.
>
> - Bret
>
> *From: *Marvin Häuser <mailto:mhaeuser@posteo.de>
> *Sent: *Monday, June 28, 2021 8:43 AM
> *To: *Laszlo Ersek <mailto:lersek@redhat.com>; Kun Qin
> <mailto:kuqin12@gmail.com>; Kinney, Michael D
> <mailto:michael.d.kinney@intel.com>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>
> <mailto:devel@edk2.groups.io>
> *Cc: *Wang, Jian J <mailto:jian.j.wang@intel.com>; Wu, Hao A
> <mailto:hao.a.wu@intel.com>; Dong, Eric <mailto:eric.dong@intel.com>;
> Ni, Ray <mailto:ray.ni@intel.com>; Liming Gao
> <mailto:gaoliming@byosoft.com.cn>; Liu, Zhiguang
> <mailto:zhiguang.liu@intel.com>; Andrew Fish <mailto:afish@apple.com>;
> Lindholm, Leif <mailto:leif@nuviainc.com>; Bret Barkelew
> <mailto:Bret.Barkelew@microsoft.com>; Michael Kubacki
> <mailto:Michael.Kubacki@microsoft.com>
> *Subject: *[EXTERNAL] Re: [edk2-devel] [PATCH v1 0/5] EDK2 Code First:
> PI Specification: Update EFI_MM_COMMUNICATE_HEADER
>
> On 28.06.21 16:57, Laszlo Ersek wrote:
> > On 06/25/21 20:47, Kun Qin wrote:
> >> Hi Mike,
> >>
> >> Thanks for the information. I can update the patch and proposed spec
> >> change to use flexible array in v-next if there is no other concerns.
> >>
> >> After switching to flexible array, OFFSET_OF (Data) should lead to the
> >> same result as sizeof.
> > This may be true on specific compilers, but it is *not* guaranteed by
> > the C language standard.
>
> Sorry, for completeness sake... :)
>
> I don't think it really depends on the compiler (but can vary per ABI),
> but it's a conceptual issue with alignment requirements. Assuming
> natural alignment and the usual stuff, for "struct s { uint64_t a;
> uint32_t b; uint8_t c[]; }" the alignment requirement is 8 Bytes, where
> there are 4 Bytes of padding after "b" (as "c" may as well be empty).
> "c" however is guaranteed to start after b in the same fashion as if it
> was declared with the maximum amount of elements that fit the memory. So
> if we take 4 elements for "c", and note that "c" has an alignment
> requirement of 1 Byte, c[0 .. 3] will alias the padding after "b". For
> "sizeof" this means that the padding is included, whereas for "offsetof"
> it is not, yielding "sizeof(struct s) == offsetof(struct s, c) + 4".
> That is what I meant by "wasted space" earlier, but this could possibly
> be made nicer with macros as necessary.
>
> As for this specific struct, the values should be identical as it is
> padded.
>
> Best regards,
> Marvin
>
> >
> > Quoting C99 6.7.2.1 "Structure and union specifiers", paragraph 16:
> >
> > "In most situations, the flexible array member is ignored. In
> > particular, the size of the structure is as if the flexible array member
> > were omitted except that it may have more trailing padding than the
> > omission would imply."
> >
> > Quoting footnotes 17 and 19,
> >
> > (17)  [...]
> >        struct s { int n; double d[]; };
> >        [...]
> >
> > (19)  [...]
> >        the expressions:
> >        [...]
> >        sizeof (struct s) >= offsetof(struct s, d)
> >
> >        are always equal to 1.
> >
> > Thanks
> > Laszlo
> >
> >
> >
> >> While sizeof would be a preferred way to move
> >> forward.
> >>
> >> Regards,
> >> Kun
> >>
> >> On 06/24/2021 08:25, Kinney, Michael D wrote:
> >>> Hello,
> >>>
> >>> Flexible array members are supported and should be used.  The old
> style
> >>> of adding an array of size [1] at the end of a structure was used at a
> >>> time
> >>> flexible array members were not supported by all compilers (late
> 1990's).
> >>> The workarounds used to handle the array of size [1] are very
> >>> confusing when
> >>> reading the C  code and the fact that sizeof() does not produce the
> >>> expected
> >>> result make it even worse.
> >>>
> >>> If we use flexible array members in this proposed change then there is
> >>> no need to use OFFSET_OF().  Correct?
> >>>
> >>> Mike
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: Marvin Häuser <mhaeuser@posteo.de<mailto:mhaeuser@posteo.de>>
> >>>> Sent: Thursday, June 24, 2021 1:00 AM
> >>>> To: Kun Qin <kuqin12@gmail.com<mailto:kuqin12@gmail.com>>; Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>;
> >>>> devel@edk2.groups.io<mailto:devel@edk2.groups.io>
> >>>> Cc: Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Wu, Hao A
> >>>> <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; Ni, Ray
> >>>> <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>;
> >>>> Liming Gao <gaoliming@byosoft.com.cn<mailto:gaoliming@byosoft.com.cn>>; Liu, Zhiguang
> >>>> <zhiguang.liu@intel.com<mailto:zhiguang.liu@intel.com>>; Andrew Fish <afish@apple.com<mailto:afish@apple.com>>; Leif
> >>>> Lindholm <leif@nuviainc.com<mailto:leif@nuviainc.com>>; Bret Barkelew
> >>>> <Bret.Barkelew@microsoft.com<mailto:Bret.Barkelew@microsoft.com>>; michael.kubacki@microsoft.com<mailto:michael.kubacki@microsoft.com>
> >>>> Subject: Re: [edk2-devel] [PATCH v1 0/5] EDK2 Code First: PI
> >>>> Specification: Update EFI_MM_COMMUNICATE_HEADER
> >>>>
> >>>> Hey Kun,
> >>>>
> >>>> Why would you rely on undefined behaviours? The OFFSET_OF macro is
> >>>> well-defined for GCC and Clang as it's implemented by an
> intrinsic, and
> >>>> while the expression for the MSVC compiler is undefined behaviour
> as per
> >>>> the C standard, it is well-defined for MSVC due to their own
> >>>> implementation being identical. From my standpoint, all supported
> >>>> compilers will yield well-defined behaviour even this way.
> OFFSET_OF on
> >>>> flexible arrays is not UB in any case to my knowledge.
> >>>>
> >>>> However, the same way as your new suggestion, you can replace
> OFFSET_OF
> >>>> with sizeof. While this *can* lead to wasted space with certain
> >>>> structure layouts (e.g. when the flexible array overlays padding
> bytes),
> >>>> this is not the case here, and otherwise just loses you a few
> bytes. I
> >>>> think this comes down to preference.
> >>>>
> >>>> The pattern you mentioned arguably is less nice syntax when used
> >>>> (involves address calculation and casting), but the biggest
> problem here
> >>>> is alignment constraints. For packed structures, you lose the
> ability of
> >>>> automatic unaligned accesses (irrelevant here because the
> structure is
> >>>> manually padded anyway). For non-packed structures, you still need to
> >>>> ensure the alignment requirement of the trailing array data is met
> >>>> manually. With flexible array members, the compiler takes care of
> both
> >>>> cases automatically.
> >>>>
> >>>> Best regards,
> >>>> Marvin
> >>>>
> >>>> On 24.06.21 02:24, Kun Qin wrote:
> >>>>> Hi Marvin,
> >>>>>
> >>>>> I would prefer not to rely on undefined behaviors from different
> >>>>> compilers. Instead of using flexible arrays, is it better to remove
> >>>>> the `Data` field, pack the structure and follow
> >>>>> "VARIABLE_LOCK_ON_VAR_STATE_POLICY" pattern?
> >>>>>
> >>>>> In that case, OFFSET_OF will be forced to change to sizeof, and
> >>>>> read/write to `Data` will follow the range indicated by
> MessageLength.
> >>>>> But yes, that will enforce developers to update their platform level
> >>>>> implementations accordingly.
> >>>>>
> >>>>> Regards,
> >>>>> Kun
> >>>>>
> >>>>> On 06/23/2021 08:26, Laszlo Ersek wrote:
> >>>>>> On 06/23/21 08:54, Marvin Häuser wrote:
> >>>>>>> On 22.06.21 17:34, Laszlo Ersek wrote:
> >>>>>>>> On 06/18/21 11:37, Marvin Häuser wrote:
> >>>>>>>>> On 16.06.21 22:58, Kun Qin wrote:
> >>>>>>>>>> On 06/16/2021 00:02, Marvin Häuser wrote:
> >>>>>>>>>>> 2) Is it feasible yet with the current set of supported
> >>>>>>>>>>> compilers to
> >>>>>>>>>>> support flexible arrays?
> >>>>>>>>>> My impression is that flexible arrays are already supported (as
> >>>>>>>>>> seen
> >>>>>>>>>> in
> UnitTestFrameworkPkg/PrivateInclude/UnitTestFrameworkTypes.h).
> >>>>>>>>>> Please correct me if I am wrong.
> >>>>>>>>>>
> >>>>>>>>>> Would you mind letting me know why this is applicable here?
> We are
> >>>>>>>>>> trying to seek ideas on how to catch developer mistakes
> caused by
> >>>>>>>>>> this
> >>>>>>>>>> change. So any input is appreciated.
> >>>>>>>>> Huh, interesting. Last time I tried I was told about
> >>>>>>>>> incompatibilities
> >>>>>>>>> with MSVC, but I know some have been dropped since then
> (2005 and
> >>>>>>>>> 2008
> >>>>>>>>> if I recall correctly?), so that'd be great to allow globally.
> >>>>>>>> I too am surprised to see
> >>>>>>>>
> "UnitTestFrameworkPkg/PrivateInclude/UnitTestFrameworkTypes.h". The
> >>>>>>>> flexible array member is a C99 feature, and I didn't even know
> >>>>>>>> that we
> >>>>>>>> disallowed it for the sake of particular VS toolchains -- I
> >>>>>>>> thought we
> >>>>>>>> had a more general reason than just "not supported by VS
> versions X
> >>>>>>>> and Y".
> >>>>>>>>
> >>>>>>>> The behavior of OFFSET_OF() would be interesting -- the
> OFFSET_OF()
> >>>>>>>> macro definition for non-gcc / non-clang:
> >>>>>>>>
> >>>>>>>> #define OFFSET_OF(TYPE, Field) ((UINTN) &(((TYPE *)0)->Field))
> >>>>>>>>
> >>>>>>>> borders on undefined behavior as far as I can tell, so its
> >>>>>>>> behavior is
> >>>>>>>> totally up to the compiler. It works thus far okay on Visual
> >>>>>>>> Studio, but
> >>>>>>>> I couldn't say if it extended correctly to flexible array
> members.
> >>>>>>> Yes, it's UB by the standard, but this is actually how MS
> implements
> >>>>>>> them (or used to anyway?). I don't see why it'd cause issues with
> >>>>>>> flexible arrays, as only the start of the array is relevant
> (which is
> >>>>>>> constant for all instances of the structure no matter the
> amount of
> >>>>>>> elements actually stored). Any specific concern? If so, they
> could be
> >>>>>>> addressed by appropriate STATIC_ASSERTs.
> >>>>>> No specific concern; my point was that two aspects of the same
> "class"
> >>>>>> of undefined behavior didn't need to be consistent with each other.
> >>>>>>
> >>>>>> Thanks
> >>>>>> Laszlo
> >>>>>>
>



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

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

[Kun Qin]

Hi Mike,

Thanks for the note. I will add this check for sanity check in v-next, 
assuming this will not fail for currently supported compilers.

Just curious, what do we normally do if this type of check start to 
break in the future?

Thanks,
Kun

On 06/29/2021 10:28, Kinney, Michael D wrote:
> Good idea on use of STATIC_ASSERT().
> 
> For structures that use flexible array members, we can add a 
> STATIC_ASSERT() for the sizeof() and OFFSET_OF() returning the same result.
> 
> For example:
> 
> STATIC_ASSERT (sizeof (EFI_MM_COMMUNICATE_HEADER) == OFFSET_OF 
> (EFI_MM_COMMUNICATE_HEADER, Data));
> 
> Mike
> 
> *From:*devel@edk2.groups.io <devel@edk2.groups.io> *On Behalf Of *Bret 
> Barkelew via groups.io
> *Sent:* Tuesday, June 29, 2021 9:00 AM
> *To:* Marvin Häuser <mhaeuser@posteo.de>; Laszlo Ersek 
> <lersek@redhat.com>; Kun Qin <kuqin12@gmail.com>; Kinney, Michael D 
> <michael.d.kinney@intel.com>; 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>; Liming Gao <gaoliming@byosoft.com.cn>; Liu, Zhiguang 
> <zhiguang.liu@intel.com>; Andrew Fish <afish@apple.com>; Lindholm, Leif 
> <leif@nuviainc.com>; Michael Kubacki <Michael.Kubacki@microsoft.com>
> *Subject:* Re: [EXTERNAL] Re: [edk2-devel] [PATCH v1 0/5] EDK2 Code 
> First: PI Specification: Update EFI_MM_COMMUNICATE_HEADER
> 
> Good note. Thanks!
> 
> - Bret
> 
> *From: *Marvin Häuser <mailto:mhaeuser@posteo.de>
> *Sent: *Tuesday, June 29, 2021 1:58 AM
> *To: *Bret Barkelew <mailto:Bret.Barkelew@microsoft.com>; Laszlo Ersek 
> <mailto:lersek@redhat.com>; Kun Qin <mailto:kuqin12@gmail.com>; Kinney, 
> Michael D <mailto:michael.d.kinney@intel.com>; devel@edk2.groups.io 
> <mailto:devel@edk2.groups.io>
> *Cc: *Wang, Jian J <mailto:jian.j.wang@intel.com>; Wu, Hao A 
> <mailto:hao.a.wu@intel.com>; Dong, Eric <mailto:eric.dong@intel.com>; 
> Ni, Ray <mailto:ray.ni@intel.com>; Liming Gao 
> <mailto:gaoliming@byosoft.com.cn>; Liu, Zhiguang 
> <mailto:zhiguang.liu@intel.com>; Andrew Fish <mailto:afish@apple.com>; 
> Lindholm, Leif <mailto:leif@nuviainc.com>; Michael Kubacki 
> <mailto:Michael.Kubacki@microsoft.com>
> *Subject: *Re: [EXTERNAL] Re: [edk2-devel] [PATCH v1 0/5] EDK2 Code 
> First: PI Specification: Update EFI_MM_COMMUNICATE_HEADER
> 
> Generally yes, but gladly not for EDK II. Default GNU ABI uses 32-bit
> alignment for 64-bit integers on IA32 (which led to a (non-critical)
> mistake in our PE paper :( ) for example, but UEFI / EDK II (seem to)
> successfully dictate natural alignment consistently. Possibly we could
> introduce some STATIC_ASSERTs around important cases (e.g. UINT64 on
> 32-bit platforms) to ensure compilers keep it that way, once the ALIGNOF
> macro is introduced.
> 
> Best regards,
> Marvin
> 
> On 29.06.21 08:49, Bret Barkelew wrote:
>  >
>  > The fact that it may vary per ABI seems like a pretty big gotcha if
>  > the SMM/MM Core was compiled at a different time or on a different
>  > system than the module that’s invoking the communication.
>  >
>  > - Bret
>  >
>  > *From: *Marvin Häuser <mailto:mhaeuser@posteo.de 
> <mailto:mhaeuser@posteo.de>>
>  > *Sent: *Monday, June 28, 2021 8:43 AM
>  > *To: *Laszlo Ersek <mailto:lersek@redhat.com 
> <mailto:lersek@redhat.com>>; Kun Qin
>  > <mailto:kuqin12@gmail.com <mailto:kuqin12@gmail.com>>; Kinney, Michael D
>  > <mailto:michael.d.kinney@intel.com 
> <mailto:michael.d.kinney@intel.com>>; devel@edk2.groups.io 
> <mailto:devel@edk2.groups.io>
>  > <mailto:devel@edk2.groups.io <mailto:devel@edk2.groups.io>>
>  > *Cc: *Wang, Jian J <mailto:jian.j.wang@intel.com 
> <mailto:jian.j.wang@intel.com>>; Wu, Hao A
>  > <mailto:hao.a.wu@intel.com <mailto:hao.a.wu@intel.com>>; Dong, Eric 
> <mailto:eric.dong@intel.com <mailto:eric.dong@intel.com>>;
>  > Ni, Ray <mailto:ray.ni@intel.com <mailto:ray.ni@intel.com>>; Liming Gao
>  > <mailto:gaoliming@byosoft.com.cn <mailto:gaoliming@byosoft.com.cn>>; 
> Liu, Zhiguang
>  > <mailto:zhiguang.liu@intel.com <mailto:zhiguang.liu@intel.com>>; 
> Andrew Fish <mailto:afish@apple.com <mailto:afish@apple.com>>;
>  > Lindholm, Leif <mailto:leif@nuviainc.com <mailto:leif@nuviainc.com>>; 
> Bret Barkelew
>  > <mailto:Bret.Barkelew@microsoft.com 
> <mailto:Bret.Barkelew@microsoft.com>>; Michael Kubacki
>  > <mailto:Michael.Kubacki@microsoft.com 
> <mailto:Michael.Kubacki@microsoft.com>>
>  > *Subject: *[EXTERNAL] Re: [edk2-devel] [PATCH v1 0/5] EDK2 Code First:
>  > PI Specification: Update EFI_MM_COMMUNICATE_HEADER
>  >
>  > On 28.06.21 16:57, Laszlo Ersek wrote:
>  > > On 06/25/21 20:47, Kun Qin wrote:
>  > >> Hi Mike,
>  > >>
>  > >> Thanks for the information. I can update the patch and proposed spec
>  > >> change to use flexible array in v-next if there is no other concerns.
>  > >>
>  > >> After switching to flexible array, OFFSET_OF (Data) should lead to the
>  > >> same result as sizeof.
>  > > This may be true on specific compilers, but it is *not* guaranteed by
>  > > the C language standard.
>  >
>  > Sorry, for completeness sake... :)
>  >
>  > I don't think it really depends on the compiler (but can vary per ABI),
>  > but it's a conceptual issue with alignment requirements. Assuming
>  > natural alignment and the usual stuff, for "struct s { uint64_t a;
>  > uint32_t b; uint8_t c[]; }" the alignment requirement is 8 Bytes, where
>  > there are 4 Bytes of padding after "b" (as "c" may as well be empty).
>  > "c" however is guaranteed to start after b in the same fashion as if it
>  > was declared with the maximum amount of elements that fit the memory. So
>  > if we take 4 elements for "c", and note that "c" has an alignment
>  > requirement of 1 Byte, c[0 .. 3] will alias the padding after "b". For
>  > "sizeof" this means that the padding is included, whereas for "offsetof"
>  > it is not, yielding "sizeof(struct s) == offsetof(struct s, c) + 4".
>  > That is what I meant by "wasted space" earlier, but this could possibly
>  > be made nicer with macros as necessary.
>  >
>  > As for this specific struct, the values should be identical as it is
>  > padded.
>  >
>  > Best regards,
>  > Marvin
>  >
>  > >
>  > > Quoting C99 6.7.2.1 "Structure and union specifiers", paragraph 16:
>  > >
>  > > "In most situations, the flexible array member is ignored. In
>  > > particular, the size of the structure is as if the flexible array 
> member
>  > > were omitted except that it may have more trailing padding than the
>  > > omission would imply."
>  > >
>  > > Quoting footnotes 17 and 19,
>  > >
>  > > (17)  [...]
>  > >        struct s { int n; double d[]; };
>  > >        [...]
>  > >
>  > > (19)  [...]
>  > >        the expressions:
>  > >        [...]
>  > >        sizeof (struct s) >= offsetof(struct s, d)
>  > >
>  > >        are always equal to 1.
>  > >
>  > > Thanks
>  > > Laszlo
>  > >
>  > >
>  > >
>  > >> While sizeof would be a preferred way to move
>  > >> forward.
>  > >>
>  > >> Regards,
>  > >> Kun
>  > >>
>  > >> On 06/24/2021 08:25, Kinney, Michael D wrote:
>  > >>> Hello,
>  > >>>
>  > >>> Flexible array members are supported and should be used.  The old
>  > style
>  > >>> of adding an array of size [1] at the end of a structure was used 
> at a
>  > >>> time
>  > >>> flexible array members were not supported by all compilers (late
>  > 1990's).
>  > >>> The workarounds used to handle the array of size [1] are very
>  > >>> confusing when
>  > >>> reading the C  code and the fact that sizeof() does not produce the
>  > >>> expected
>  > >>> result make it even worse.
>  > >>>
>  > >>> If we use flexible array members in this proposed change then 
> there is
>  > >>> no need to use OFFSET_OF().  Correct?
>  > >>>
>  > >>> Mike
>  > >>>
>  > >>>
>  > >>>> -----Original Message-----
>  > >>>> From: Marvin Häuser <mhaeuser@posteo.de <mailto:mhaeuser@posteo.de>>
>  > >>>> Sent: Thursday, June 24, 2021 1:00 AM
>  > >>>> To: Kun Qin <kuqin12@gmail.com <mailto:kuqin12@gmail.com>>; 
> Laszlo Ersek <lersek@redhat.com <mailto:lersek@redhat.com>>;
>  > >>>> devel@edk2.groups.io <mailto:devel@edk2.groups.io>
>  > >>>> Cc: Wang, Jian J <jian.j.wang@intel.com 
> <mailto:jian.j.wang@intel.com>>; Wu, Hao A
>  > >>>> <hao.a.wu@intel.com <mailto:hao.a.wu@intel.com>>; Dong, Eric 
> <eric.dong@intel.com <mailto:eric.dong@intel.com>>; Ni, Ray
>  > >>>> <ray.ni@intel.com <mailto:ray.ni@intel.com>>; Kinney, Michael D 
> <michael.d.kinney@intel.com <mailto:michael.d.kinney@intel.com>>;
>  > >>>> Liming Gao <gaoliming@byosoft.com.cn 
> <mailto:gaoliming@byosoft.com.cn>>; Liu, Zhiguang
>  > >>>> <zhiguang.liu@intel.com <mailto:zhiguang.liu@intel.com>>; Andrew 
> Fish <afish@apple.com <mailto:afish@apple.com>>; Leif
>  > >>>> Lindholm <leif@nuviainc.com <mailto:leif@nuviainc.com>>; Bret 
> Barkelew
>  > >>>> <Bret.Barkelew@microsoft.com 
> <mailto:Bret.Barkelew@microsoft.com>>; michael.kubacki@microsoft.com 
> <mailto:michael.kubacki@microsoft.com>
>  > >>>> Subject: Re: [edk2-devel] [PATCH v1 0/5] EDK2 Code First: PI
>  > >>>> Specification: Update EFI_MM_COMMUNICATE_HEADER
>  > >>>>
>  > >>>> Hey Kun,
>  > >>>>
>  > >>>> Why would you rely on undefined behaviours? The OFFSET_OF macro is
>  > >>>> well-defined for GCC and Clang as it's implemented by an
>  > intrinsic, and
>  > >>>> while the expression for the MSVC compiler is undefined behaviour
>  > as per
>  > >>>> the C standard, it is well-defined for MSVC due to their own
>  > >>>> implementation being identical. From my standpoint, all supported
>  > >>>> compilers will yield well-defined behaviour even this way.
>  > OFFSET_OF on
>  > >>>> flexible arrays is not UB in any case to my knowledge.
>  > >>>>
>  > >>>> However, the same way as your new suggestion, you can replace
>  > OFFSET_OF
>  > >>>> with sizeof. While this *can* lead to wasted space with certain
>  > >>>> structure layouts (e.g. when the flexible array overlays padding
>  > bytes),
>  > >>>> this is not the case here, and otherwise just loses you a few
>  > bytes. I
>  > >>>> think this comes down to preference.
>  > >>>>
>  > >>>> The pattern you mentioned arguably is less nice syntax when used
>  > >>>> (involves address calculation and casting), but the biggest
>  > problem here
>  > >>>> is alignment constraints. For packed structures, you lose the
>  > ability of
>  > >>>> automatic unaligned accesses (irrelevant here because the
>  > structure is
>  > >>>> manually padded anyway). For non-packed structures, you still 
> need to
>  > >>>> ensure the alignment requirement of the trailing array data is met
>  > >>>> manually. With flexible array members, the compiler takes care of
>  > both
>  > >>>> cases automatically.
>  > >>>>
>  > >>>> Best regards,
>  > >>>> Marvin
>  > >>>>
>  > >>>> On 24.06.21 02:24, Kun Qin wrote:
>  > >>>>> Hi Marvin,
>  > >>>>>
>  > >>>>> I would prefer not to rely on undefined behaviors from different
>  > >>>>> compilers. Instead of using flexible arrays, is it better to remove
>  > >>>>> the `Data` field, pack the structure and follow
>  > >>>>> "VARIABLE_LOCK_ON_VAR_STATE_POLICY" pattern?
>  > >>>>>
>  > >>>>> In that case, OFFSET_OF will be forced to change to sizeof, and
>  > >>>>> read/write to `Data` will follow the range indicated by
>  > MessageLength.
>  > >>>>> But yes, that will enforce developers to update their platform 
> level
>  > >>>>> implementations accordingly.
>  > >>>>>
>  > >>>>> Regards,
>  > >>>>> Kun
>  > >>>>>
>  > >>>>> On 06/23/2021 08:26, Laszlo Ersek wrote:
>  > >>>>>> On 06/23/21 08:54, Marvin Häuser wrote:
>  > >>>>>>> On 22.06.21 17:34, Laszlo Ersek wrote:
>  > >>>>>>>> On 06/18/21 11:37, Marvin Häuser wrote:
>  > >>>>>>>>> On 16.06.21 22:58, Kun Qin wrote:
>  > >>>>>>>>>> On 06/16/2021 00:02, Marvin Häuser wrote:
>  > >>>>>>>>>>> 2) Is it feasible yet with the current set of supported
>  > >>>>>>>>>>> compilers to
>  > >>>>>>>>>>> support flexible arrays?
>  > >>>>>>>>>> My impression is that flexible arrays are already 
> supported (as
>  > >>>>>>>>>> seen
>  > >>>>>>>>>> in
>  > UnitTestFrameworkPkg/PrivateInclude/UnitTestFrameworkTypes.h).
>  > >>>>>>>>>> Please correct me if I am wrong.
>  > >>>>>>>>>>
>  > >>>>>>>>>> Would you mind letting me know why this is applicable here?
>  > We are
>  > >>>>>>>>>> trying to seek ideas on how to catch developer mistakes
>  > caused by
>  > >>>>>>>>>> this
>  > >>>>>>>>>> change. So any input is appreciated.
>  > >>>>>>>>> Huh, interesting. Last time I tried I was told about
>  > >>>>>>>>> incompatibilities
>  > >>>>>>>>> with MSVC, but I know some have been dropped since then
>  > (2005 and
>  > >>>>>>>>> 2008
>  > >>>>>>>>> if I recall correctly?), so that'd be great to allow globally.
>  > >>>>>>>> I too am surprised to see
>  > >>>>>>>>
>  > "UnitTestFrameworkPkg/PrivateInclude/UnitTestFrameworkTypes.h". The
>  > >>>>>>>> flexible array member is a C99 feature, and I didn't even know
>  > >>>>>>>> that we
>  > >>>>>>>> disallowed it for the sake of particular VS toolchains -- I
>  > >>>>>>>> thought we
>  > >>>>>>>> had a more general reason than just "not supported by VS
>  > versions X
>  > >>>>>>>> and Y".
>  > >>>>>>>>
>  > >>>>>>>> The behavior of OFFSET_OF() would be interesting -- the
>  > OFFSET_OF()
>  > >>>>>>>> macro definition for non-gcc / non-clang:
>  > >>>>>>>>
>  > >>>>>>>> #define OFFSET_OF(TYPE, Field) ((UINTN) &(((TYPE *)0)->Field))
>  > >>>>>>>>
>  > >>>>>>>> borders on undefined behavior as far as I can tell, so its
>  > >>>>>>>> behavior is
>  > >>>>>>>> totally up to the compiler. It works thus far okay on Visual
>  > >>>>>>>> Studio, but
>  > >>>>>>>> I couldn't say if it extended correctly to flexible array
>  > members.
>  > >>>>>>> Yes, it's UB by the standard, but this is actually how MS
>  > implements
>  > >>>>>>> them (or used to anyway?). I don't see why it'd cause issues with
>  > >>>>>>> flexible arrays, as only the start of the array is relevant
>  > (which is
>  > >>>>>>> constant for all instances of the structure no matter the
>  > amount of
>  > >>>>>>> elements actually stored). Any specific concern? If so, they
>  > could be
>  > >>>>>>> addressed by appropriate STATIC_ASSERTs.
>  > >>>>>> No specific concern; my point was that two aspects of the same
>  > "class"
>  > >>>>>> of undefined behavior didn't need to be consistent with each 
> other.
>  > >>>>>>
>  > >>>>>> Thanks
>  > >>>>>> Laszlo
>  > >>>>>>
>  >
> 
> 
> 

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

[Michael D Kinney]

If it breaks in the future, then that would be due to a new compiler that
or changes to the configuration of an existing compiler that break compatibility
with UEFI ABI.  The compiler issue must be resolved before the new compiler
or change to existing compiler are accepted.

Mike

> -----Original Message-----
> From: Kun Qin <kuqin12@gmail.com>
> Sent: Tuesday, June 29, 2021 4:11 PM
> To: Kinney, Michael D <michael.d.kinney@intel.com>; devel@edk2.groups.io; bret.barkelew@microsoft.com; Marvin Häuser
> <mhaeuser@posteo.de>; Laszlo Ersek <lersek@redhat.com>
> 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>; Liming Gao <gaoliming@byosoft.com.cn>; Liu, Zhiguang <zhiguang.liu@intel.com>; Andrew Fish
> <afish@apple.com>; Lindholm, Leif <leif@nuviainc.com>; Michael Kubacki <Michael.Kubacki@microsoft.com>
> Subject: Re: [EXTERNAL] Re: [edk2-devel] [PATCH v1 0/5] EDK2 Code First: PI Specification: Update
> EFI_MM_COMMUNICATE_HEADER
> 
> Hi Mike,
> 
> Thanks for the note. I will add this check for sanity check in v-next,
> assuming this will not fail for currently supported compilers.
> 
> Just curious, what do we normally do if this type of check start to
> break in the future?
> 
> Thanks,
> Kun
> 
> On 06/29/2021 10:28, Kinney, Michael D wrote:
> > Good idea on use of STATIC_ASSERT().
> >
> > For structures that use flexible array members, we can add a
> > STATIC_ASSERT() for the sizeof() and OFFSET_OF() returning the same result.
> >
> > For example:
> >
> > STATIC_ASSERT (sizeof (EFI_MM_COMMUNICATE_HEADER) == OFFSET_OF
> > (EFI_MM_COMMUNICATE_HEADER, Data));
> >
> > Mike
> >
> > *From:*devel@edk2.groups.io <devel@edk2.groups.io> *On Behalf Of *Bret
> > Barkelew via groups.io
> > *Sent:* Tuesday, June 29, 2021 9:00 AM
> > *To:* Marvin Häuser <mhaeuser@posteo.de>; Laszlo Ersek
> > <lersek@redhat.com>; Kun Qin <kuqin12@gmail.com>; Kinney, Michael D
> > <michael.d.kinney@intel.com>; 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>; Liming Gao <gaoliming@byosoft.com.cn>; Liu, Zhiguang
> > <zhiguang.liu@intel.com>; Andrew Fish <afish@apple.com>; Lindholm, Leif
> > <leif@nuviainc.com>; Michael Kubacki <Michael.Kubacki@microsoft.com>
> > *Subject:* Re: [EXTERNAL] Re: [edk2-devel] [PATCH v1 0/5] EDK2 Code
> > First: PI Specification: Update EFI_MM_COMMUNICATE_HEADER
> >
> > Good note. Thanks!
> >
> > - Bret
> >
> > *From: *Marvin Häuser <mailto:mhaeuser@posteo.de>
> > *Sent: *Tuesday, June 29, 2021 1:58 AM
> > *To: *Bret Barkelew <mailto:Bret.Barkelew@microsoft.com>; Laszlo Ersek
> > <mailto:lersek@redhat.com>; Kun Qin <mailto:kuqin12@gmail.com>; Kinney,
> > Michael D <mailto:michael.d.kinney@intel.com>; devel@edk2.groups.io
> > <mailto:devel@edk2.groups.io>
> > *Cc: *Wang, Jian J <mailto:jian.j.wang@intel.com>; Wu, Hao A
> > <mailto:hao.a.wu@intel.com>; Dong, Eric <mailto:eric.dong@intel.com>;
> > Ni, Ray <mailto:ray.ni@intel.com>; Liming Gao
> > <mailto:gaoliming@byosoft.com.cn>; Liu, Zhiguang
> > <mailto:zhiguang.liu@intel.com>; Andrew Fish <mailto:afish@apple.com>;
> > Lindholm, Leif <mailto:leif@nuviainc.com>; Michael Kubacki
> > <mailto:Michael.Kubacki@microsoft.com>
> > *Subject: *Re: [EXTERNAL] Re: [edk2-devel] [PATCH v1 0/5] EDK2 Code
> > First: PI Specification: Update EFI_MM_COMMUNICATE_HEADER
> >
> > Generally yes, but gladly not for EDK II. Default GNU ABI uses 32-bit
> > alignment for 64-bit integers on IA32 (which led to a (non-critical)
> > mistake in our PE paper :( ) for example, but UEFI / EDK II (seem to)
> > successfully dictate natural alignment consistently. Possibly we could
> > introduce some STATIC_ASSERTs around important cases (e.g. UINT64 on
> > 32-bit platforms) to ensure compilers keep it that way, once the ALIGNOF
> > macro is introduced.
> >
> > Best regards,
> > Marvin
> >
> > On 29.06.21 08:49, Bret Barkelew wrote:
> >  >
> >  > The fact that it may vary per ABI seems like a pretty big gotcha if
> >  > the SMM/MM Core was compiled at a different time or on a different
> >  > system than the module that’s invoking the communication.
> >  >
> >  > - Bret
> >  >
> >  > *From: *Marvin Häuser <mailto:mhaeuser@posteo.de
> > <mailto:mhaeuser@posteo.de>>
> >  > *Sent: *Monday, June 28, 2021 8:43 AM
> >  > *To: *Laszlo Ersek <mailto:lersek@redhat.com
> > <mailto:lersek@redhat.com>>; Kun Qin
> >  > <mailto:kuqin12@gmail.com <mailto:kuqin12@gmail.com>>; Kinney, Michael D
> >  > <mailto:michael.d.kinney@intel.com
> > <mailto:michael.d.kinney@intel.com>>; devel@edk2.groups.io
> > <mailto:devel@edk2.groups.io>
> >  > <mailto:devel@edk2.groups.io <mailto:devel@edk2.groups.io>>
> >  > *Cc: *Wang, Jian J <mailto:jian.j.wang@intel.com
> > <mailto:jian.j.wang@intel.com>>; Wu, Hao A
> >  > <mailto:hao.a.wu@intel.com <mailto:hao.a.wu@intel.com>>; Dong, Eric
> > <mailto:eric.dong@intel.com <mailto:eric.dong@intel.com>>;
> >  > Ni, Ray <mailto:ray.ni@intel.com <mailto:ray.ni@intel.com>>; Liming Gao
> >  > <mailto:gaoliming@byosoft.com.cn <mailto:gaoliming@byosoft.com.cn>>;
> > Liu, Zhiguang
> >  > <mailto:zhiguang.liu@intel.com <mailto:zhiguang.liu@intel.com>>;
> > Andrew Fish <mailto:afish@apple.com <mailto:afish@apple.com>>;
> >  > Lindholm, Leif <mailto:leif@nuviainc.com <mailto:leif@nuviainc.com>>;
> > Bret Barkelew
> >  > <mailto:Bret.Barkelew@microsoft.com
> > <mailto:Bret.Barkelew@microsoft.com>>; Michael Kubacki
> >  > <mailto:Michael.Kubacki@microsoft.com
> > <mailto:Michael.Kubacki@microsoft.com>>
> >  > *Subject: *[EXTERNAL] Re: [edk2-devel] [PATCH v1 0/5] EDK2 Code First:
> >  > PI Specification: Update EFI_MM_COMMUNICATE_HEADER
> >  >
> >  > On 28.06.21 16:57, Laszlo Ersek wrote:
> >  > > On 06/25/21 20:47, Kun Qin wrote:
> >  > >> Hi Mike,
> >  > >>
> >  > >> Thanks for the information. I can update the patch and proposed spec
> >  > >> change to use flexible array in v-next if there is no other concerns.
> >  > >>
> >  > >> After switching to flexible array, OFFSET_OF (Data) should lead to the
> >  > >> same result as sizeof.
> >  > > This may be true on specific compilers, but it is *not* guaranteed by
> >  > > the C language standard.
> >  >
> >  > Sorry, for completeness sake... :)
> >  >
> >  > I don't think it really depends on the compiler (but can vary per ABI),
> >  > but it's a conceptual issue with alignment requirements. Assuming
> >  > natural alignment and the usual stuff, for "struct s { uint64_t a;
> >  > uint32_t b; uint8_t c[]; }" the alignment requirement is 8 Bytes, where
> >  > there are 4 Bytes of padding after "b" (as "c" may as well be empty).
> >  > "c" however is guaranteed to start after b in the same fashion as if it
> >  > was declared with the maximum amount of elements that fit the memory. So
> >  > if we take 4 elements for "c", and note that "c" has an alignment
> >  > requirement of 1 Byte, c[0 .. 3] will alias the padding after "b". For
> >  > "sizeof" this means that the padding is included, whereas for "offsetof"
> >  > it is not, yielding "sizeof(struct s) == offsetof(struct s, c) + 4".
> >  > That is what I meant by "wasted space" earlier, but this could possibly
> >  > be made nicer with macros as necessary.
> >  >
> >  > As for this specific struct, the values should be identical as it is
> >  > padded.
> >  >
> >  > Best regards,
> >  > Marvin
> >  >
> >  > >
> >  > > Quoting C99 6.7.2.1 "Structure and union specifiers", paragraph 16:
> >  > >
> >  > > "In most situations, the flexible array member is ignored. In
> >  > > particular, the size of the structure is as if the flexible array
> > member
> >  > > were omitted except that it may have more trailing padding than the
> >  > > omission would imply."
> >  > >
> >  > > Quoting footnotes 17 and 19,
> >  > >
> >  > > (17)  [...]
> >  > >        struct s { int n; double d[]; };
> >  > >        [...]
> >  > >
> >  > > (19)  [...]
> >  > >        the expressions:
> >  > >        [...]
> >  > >        sizeof (struct s) >= offsetof(struct s, d)
> >  > >
> >  > >        are always equal to 1.
> >  > >
> >  > > Thanks
> >  > > Laszlo
> >  > >
> >  > >
> >  > >
> >  > >> While sizeof would be a preferred way to move
> >  > >> forward.
> >  > >>
> >  > >> Regards,
> >  > >> Kun
> >  > >>
> >  > >> On 06/24/2021 08:25, Kinney, Michael D wrote:
> >  > >>> Hello,
> >  > >>>
> >  > >>> Flexible array members are supported and should be used.  The old
> >  > style
> >  > >>> of adding an array of size [1] at the end of a structure was used
> > at a
> >  > >>> time
> >  > >>> flexible array members were not supported by all compilers (late
> >  > 1990's).
> >  > >>> The workarounds used to handle the array of size [1] are very
> >  > >>> confusing when
> >  > >>> reading the C  code and the fact that sizeof() does not produce the
> >  > >>> expected
> >  > >>> result make it even worse.
> >  > >>>
> >  > >>> If we use flexible array members in this proposed change then
> > there is
> >  > >>> no need to use OFFSET_OF().  Correct?
> >  > >>>
> >  > >>> Mike
> >  > >>>
> >  > >>>
> >  > >>>> -----Original Message-----
> >  > >>>> From: Marvin Häuser <mhaeuser@posteo.de <mailto:mhaeuser@posteo.de>>
> >  > >>>> Sent: Thursday, June 24, 2021 1:00 AM
> >  > >>>> To: Kun Qin <kuqin12@gmail.com <mailto:kuqin12@gmail.com>>;
> > Laszlo Ersek <lersek@redhat.com <mailto:lersek@redhat.com>>;
> >  > >>>> devel@edk2.groups.io <mailto:devel@edk2.groups.io>
> >  > >>>> Cc: Wang, Jian J <jian.j.wang@intel.com
> > <mailto:jian.j.wang@intel.com>>; Wu, Hao A
> >  > >>>> <hao.a.wu@intel.com <mailto:hao.a.wu@intel.com>>; Dong, Eric
> > <eric.dong@intel.com <mailto:eric.dong@intel.com>>; Ni, Ray
> >  > >>>> <ray.ni@intel.com <mailto:ray.ni@intel.com>>; Kinney, Michael D
> > <michael.d.kinney@intel.com <mailto:michael.d.kinney@intel.com>>;
> >  > >>>> Liming Gao <gaoliming@byosoft.com.cn
> > <mailto:gaoliming@byosoft.com.cn>>; Liu, Zhiguang
> >  > >>>> <zhiguang.liu@intel.com <mailto:zhiguang.liu@intel.com>>; Andrew
> > Fish <afish@apple.com <mailto:afish@apple.com>>; Leif
> >  > >>>> Lindholm <leif@nuviainc.com <mailto:leif@nuviainc.com>>; Bret
> > Barkelew
> >  > >>>> <Bret.Barkelew@microsoft.com
> > <mailto:Bret.Barkelew@microsoft.com>>; michael.kubacki@microsoft.com
> > <mailto:michael.kubacki@microsoft.com>
> >  > >>>> Subject: Re: [edk2-devel] [PATCH v1 0/5] EDK2 Code First: PI
> >  > >>>> Specification: Update EFI_MM_COMMUNICATE_HEADER
> >  > >>>>
> >  > >>>> Hey Kun,
> >  > >>>>
> >  > >>>> Why would you rely on undefined behaviours? The OFFSET_OF macro is
> >  > >>>> well-defined for GCC and Clang as it's implemented by an
> >  > intrinsic, and
> >  > >>>> while the expression for the MSVC compiler is undefined behaviour
> >  > as per
> >  > >>>> the C standard, it is well-defined for MSVC due to their own
> >  > >>>> implementation being identical. From my standpoint, all supported
> >  > >>>> compilers will yield well-defined behaviour even this way.
> >  > OFFSET_OF on
> >  > >>>> flexible arrays is not UB in any case to my knowledge.
> >  > >>>>
> >  > >>>> However, the same way as your new suggestion, you can replace
> >  > OFFSET_OF
> >  > >>>> with sizeof. While this *can* lead to wasted space with certain
> >  > >>>> structure layouts (e.g. when the flexible array overlays padding
> >  > bytes),
> >  > >>>> this is not the case here, and otherwise just loses you a few
> >  > bytes. I
> >  > >>>> think this comes down to preference.
> >  > >>>>
> >  > >>>> The pattern you mentioned arguably is less nice syntax when used
> >  > >>>> (involves address calculation and casting), but the biggest
> >  > problem here
> >  > >>>> is alignment constraints. For packed structures, you lose the
> >  > ability of
> >  > >>>> automatic unaligned accesses (irrelevant here because the
> >  > structure is
> >  > >>>> manually padded anyway). For non-packed structures, you still
> > need to
> >  > >>>> ensure the alignment requirement of the trailing array data is met
> >  > >>>> manually. With flexible array members, the compiler takes care of
> >  > both
> >  > >>>> cases automatically.
> >  > >>>>
> >  > >>>> Best regards,
> >  > >>>> Marvin
> >  > >>>>
> >  > >>>> On 24.06.21 02:24, Kun Qin wrote:
> >  > >>>>> Hi Marvin,
> >  > >>>>>
> >  > >>>>> I would prefer not to rely on undefined behaviors from different
> >  > >>>>> compilers. Instead of using flexible arrays, is it better to remove
> >  > >>>>> the `Data` field, pack the structure and follow
> >  > >>>>> "VARIABLE_LOCK_ON_VAR_STATE_POLICY" pattern?
> >  > >>>>>
> >  > >>>>> In that case, OFFSET_OF will be forced to change to sizeof, and
> >  > >>>>> read/write to `Data` will follow the range indicated by
> >  > MessageLength.
> >  > >>>>> But yes, that will enforce developers to update their platform
> > level
> >  > >>>>> implementations accordingly.
> >  > >>>>>
> >  > >>>>> Regards,
> >  > >>>>> Kun
> >  > >>>>>
> >  > >>>>> On 06/23/2021 08:26, Laszlo Ersek wrote:
> >  > >>>>>> On 06/23/21 08:54, Marvin Häuser wrote:
> >  > >>>>>>> On 22.06.21 17:34, Laszlo Ersek wrote:
> >  > >>>>>>>> On 06/18/21 11:37, Marvin Häuser wrote:
> >  > >>>>>>>>> On 16.06.21 22:58, Kun Qin wrote:
> >  > >>>>>>>>>> On 06/16/2021 00:02, Marvin Häuser wrote:
> >  > >>>>>>>>>>> 2) Is it feasible yet with the current set of supported
> >  > >>>>>>>>>>> compilers to
> >  > >>>>>>>>>>> support flexible arrays?
> >  > >>>>>>>>>> My impression is that flexible arrays are already
> > supported (as
> >  > >>>>>>>>>> seen
> >  > >>>>>>>>>> in
> >  > UnitTestFrameworkPkg/PrivateInclude/UnitTestFrameworkTypes.h).
> >  > >>>>>>>>>> Please correct me if I am wrong.
> >  > >>>>>>>>>>
> >  > >>>>>>>>>> Would you mind letting me know why this is applicable here?
> >  > We are
> >  > >>>>>>>>>> trying to seek ideas on how to catch developer mistakes
> >  > caused by
> >  > >>>>>>>>>> this
> >  > >>>>>>>>>> change. So any input is appreciated.
> >  > >>>>>>>>> Huh, interesting. Last time I tried I was told about
> >  > >>>>>>>>> incompatibilities
> >  > >>>>>>>>> with MSVC, but I know some have been dropped since then
> >  > (2005 and
> >  > >>>>>>>>> 2008
> >  > >>>>>>>>> if I recall correctly?), so that'd be great to allow globally.
> >  > >>>>>>>> I too am surprised to see
> >  > >>>>>>>>
> >  > "UnitTestFrameworkPkg/PrivateInclude/UnitTestFrameworkTypes.h". The
> >  > >>>>>>>> flexible array member is a C99 feature, and I didn't even know
> >  > >>>>>>>> that we
> >  > >>>>>>>> disallowed it for the sake of particular VS toolchains -- I
> >  > >>>>>>>> thought we
> >  > >>>>>>>> had a more general reason than just "not supported by VS
> >  > versions X
> >  > >>>>>>>> and Y".
> >  > >>>>>>>>
> >  > >>>>>>>> The behavior of OFFSET_OF() would be interesting -- the
> >  > OFFSET_OF()
> >  > >>>>>>>> macro definition for non-gcc / non-clang:
> >  > >>>>>>>>
> >  > >>>>>>>> #define OFFSET_OF(TYPE, Field) ((UINTN) &(((TYPE *)0)->Field))
> >  > >>>>>>>>
> >  > >>>>>>>> borders on undefined behavior as far as I can tell, so its
> >  > >>>>>>>> behavior is
> >  > >>>>>>>> totally up to the compiler. It works thus far okay on Visual
> >  > >>>>>>>> Studio, but
> >  > >>>>>>>> I couldn't say if it extended correctly to flexible array
> >  > members.
> >  > >>>>>>> Yes, it's UB by the standard, but this is actually how MS
> >  > implements
> >  > >>>>>>> them (or used to anyway?). I don't see why it'd cause issues with
> >  > >>>>>>> flexible arrays, as only the start of the array is relevant
> >  > (which is
> >  > >>>>>>> constant for all instances of the structure no matter the
> >  > amount of
> >  > >>>>>>> elements actually stored). Any specific concern? If so, they
> >  > could be
> >  > >>>>>>> addressed by appropriate STATIC_ASSERTs.
> >  > >>>>>> No specific concern; my point was that two aspects of the same
> >  > "class"
> >  > >>>>>> of undefined behavior didn't need to be consistent with each
> > other.
> >  > >>>>>>
> >  > >>>>>> Thanks
> >  > >>>>>> Laszlo
> >  > >>>>>>
> >  >
> >
> > 
> >

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

[Kun Qin]

Thanks for the clarification. I will work on v-next with flexible array 
as Data field.

Regards,
Kun

On 06/29/2021 18:07, Kinney, Michael D wrote:
> If it breaks in the future, then that would be due to a new compiler that
> or changes to the configuration of an existing compiler that break compatibility
> with UEFI ABI.  The compiler issue must be resolved before the new compiler
> or change to existing compiler are accepted.
> 
> Mike
> 
>> -----Original Message-----
>> From: Kun Qin <kuqin12@gmail.com>
>> Sent: Tuesday, June 29, 2021 4:11 PM
>> To: Kinney, Michael D <michael.d.kinney@intel.com>; devel@edk2.groups.io; bret.barkelew@microsoft.com; Marvin Häuser
>> <mhaeuser@posteo.de>; Laszlo Ersek <lersek@redhat.com>
>> 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>; Liming Gao <gaoliming@byosoft.com.cn>; Liu, Zhiguang <zhiguang.liu@intel.com>; Andrew Fish
>> <afish@apple.com>; Lindholm, Leif <leif@nuviainc.com>; Michael Kubacki <Michael.Kubacki@microsoft.com>
>> Subject: Re: [EXTERNAL] Re: [edk2-devel] [PATCH v1 0/5] EDK2 Code First: PI Specification: Update
>> EFI_MM_COMMUNICATE_HEADER
>>
>> Hi Mike,
>>
>> Thanks for the note. I will add this check for sanity check in v-next,
>> assuming this will not fail for currently supported compilers.
>>
>> Just curious, what do we normally do if this type of check start to
>> break in the future?
>>
>> Thanks,
>> Kun
>>
>> On 06/29/2021 10:28, Kinney, Michael D wrote:
>>> Good idea on use of STATIC_ASSERT().
>>>
>>> For structures that use flexible array members, we can add a
>>> STATIC_ASSERT() for the sizeof() and OFFSET_OF() returning the same result.
>>>
>>> For example:
>>>
>>> STATIC_ASSERT (sizeof (EFI_MM_COMMUNICATE_HEADER) == OFFSET_OF
>>> (EFI_MM_COMMUNICATE_HEADER, Data));
>>>
>>> Mike
>>>
>>> *From:*devel@edk2.groups.io <devel@edk2.groups.io> *On Behalf Of *Bret
>>> Barkelew via groups.io
>>> *Sent:* Tuesday, June 29, 2021 9:00 AM
>>> *To:* Marvin Häuser <mhaeuser@posteo.de>; Laszlo Ersek
>>> <lersek@redhat.com>; Kun Qin <kuqin12@gmail.com>; Kinney, Michael D
>>> <michael.d.kinney@intel.com>; 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>; Liming Gao <gaoliming@byosoft.com.cn>; Liu, Zhiguang
>>> <zhiguang.liu@intel.com>; Andrew Fish <afish@apple.com>; Lindholm, Leif
>>> <leif@nuviainc.com>; Michael Kubacki <Michael.Kubacki@microsoft.com>
>>> *Subject:* Re: [EXTERNAL] Re: [edk2-devel] [PATCH v1 0/5] EDK2 Code
>>> First: PI Specification: Update EFI_MM_COMMUNICATE_HEADER
>>>
>>> Good note. Thanks!
>>>
>>> - Bret
>>>
>>> *From: *Marvin Häuser <mailto:mhaeuser@posteo.de>
>>> *Sent: *Tuesday, June 29, 2021 1:58 AM
>>> *To: *Bret Barkelew <mailto:Bret.Barkelew@microsoft.com>; Laszlo Ersek
>>> <mailto:lersek@redhat.com>; Kun Qin <mailto:kuqin12@gmail.com>; Kinney,
>>> Michael D <mailto:michael.d.kinney@intel.com>; devel@edk2.groups.io
>>> <mailto:devel@edk2.groups.io>
>>> *Cc: *Wang, Jian J <mailto:jian.j.wang@intel.com>; Wu, Hao A
>>> <mailto:hao.a.wu@intel.com>; Dong, Eric <mailto:eric.dong@intel.com>;
>>> Ni, Ray <mailto:ray.ni@intel.com>; Liming Gao
>>> <mailto:gaoliming@byosoft.com.cn>; Liu, Zhiguang
>>> <mailto:zhiguang.liu@intel.com>; Andrew Fish <mailto:afish@apple.com>;
>>> Lindholm, Leif <mailto:leif@nuviainc.com>; Michael Kubacki
>>> <mailto:Michael.Kubacki@microsoft.com>
>>> *Subject: *Re: [EXTERNAL] Re: [edk2-devel] [PATCH v1 0/5] EDK2 Code
>>> First: PI Specification: Update EFI_MM_COMMUNICATE_HEADER
>>>
>>> Generally yes, but gladly not for EDK II. Default GNU ABI uses 32-bit
>>> alignment for 64-bit integers on IA32 (which led to a (non-critical)
>>> mistake in our PE paper :( ) for example, but UEFI / EDK II (seem to)
>>> successfully dictate natural alignment consistently. Possibly we could
>>> introduce some STATIC_ASSERTs around important cases (e.g. UINT64 on
>>> 32-bit platforms) to ensure compilers keep it that way, once the ALIGNOF
>>> macro is introduced.
>>>
>>> Best regards,
>>> Marvin
>>>
>>> On 29.06.21 08:49, Bret Barkelew wrote:
>>>   >
>>>   > The fact that it may vary per ABI seems like a pretty big gotcha if
>>>   > the SMM/MM Core was compiled at a different time or on a different
>>>   > system than the module that’s invoking the communication.
>>>   >
>>>   > - Bret
>>>   >
>>>   > *From: *Marvin Häuser <mailto:mhaeuser@posteo.de
>>> <mailto:mhaeuser@posteo.de>>
>>>   > *Sent: *Monday, June 28, 2021 8:43 AM
>>>   > *To: *Laszlo Ersek <mailto:lersek@redhat.com
>>> <mailto:lersek@redhat.com>>; Kun Qin
>>>   > <mailto:kuqin12@gmail.com <mailto:kuqin12@gmail.com>>; Kinney, Michael D
>>>   > <mailto:michael.d.kinney@intel.com
>>> <mailto:michael.d.kinney@intel.com>>; devel@edk2.groups.io
>>> <mailto:devel@edk2.groups.io>
>>>   > <mailto:devel@edk2.groups.io <mailto:devel@edk2.groups.io>>
>>>   > *Cc: *Wang, Jian J <mailto:jian.j.wang@intel.com
>>> <mailto:jian.j.wang@intel.com>>; Wu, Hao A
>>>   > <mailto:hao.a.wu@intel.com <mailto:hao.a.wu@intel.com>>; Dong, Eric
>>> <mailto:eric.dong@intel.com <mailto:eric.dong@intel.com>>;
>>>   > Ni, Ray <mailto:ray.ni@intel.com <mailto:ray.ni@intel.com>>; Liming Gao
>>>   > <mailto:gaoliming@byosoft.com.cn <mailto:gaoliming@byosoft.com.cn>>;
>>> Liu, Zhiguang
>>>   > <mailto:zhiguang.liu@intel.com <mailto:zhiguang.liu@intel.com>>;
>>> Andrew Fish <mailto:afish@apple.com <mailto:afish@apple.com>>;
>>>   > Lindholm, Leif <mailto:leif@nuviainc.com <mailto:leif@nuviainc.com>>;
>>> Bret Barkelew
>>>   > <mailto:Bret.Barkelew@microsoft.com
>>> <mailto:Bret.Barkelew@microsoft.com>>; Michael Kubacki
>>>   > <mailto:Michael.Kubacki@microsoft.com
>>> <mailto:Michael.Kubacki@microsoft.com>>
>>>   > *Subject: *[EXTERNAL] Re: [edk2-devel] [PATCH v1 0/5] EDK2 Code First:
>>>   > PI Specification: Update EFI_MM_COMMUNICATE_HEADER
>>>   >
>>>   > On 28.06.21 16:57, Laszlo Ersek wrote:
>>>   > > On 06/25/21 20:47, Kun Qin wrote:
>>>   > >> Hi Mike,
>>>   > >>
>>>   > >> Thanks for the information. I can update the patch and proposed spec
>>>   > >> change to use flexible array in v-next if there is no other concerns.
>>>   > >>
>>>   > >> After switching to flexible array, OFFSET_OF (Data) should lead to the
>>>   > >> same result as sizeof.
>>>   > > This may be true on specific compilers, but it is *not* guaranteed by
>>>   > > the C language standard.
>>>   >
>>>   > Sorry, for completeness sake... :)
>>>   >
>>>   > I don't think it really depends on the compiler (but can vary per ABI),
>>>   > but it's a conceptual issue with alignment requirements. Assuming
>>>   > natural alignment and the usual stuff, for "struct s { uint64_t a;
>>>   > uint32_t b; uint8_t c[]; }" the alignment requirement is 8 Bytes, where
>>>   > there are 4 Bytes of padding after "b" (as "c" may as well be empty).
>>>   > "c" however is guaranteed to start after b in the same fashion as if it
>>>   > was declared with the maximum amount of elements that fit the memory. So
>>>   > if we take 4 elements for "c", and note that "c" has an alignment
>>>   > requirement of 1 Byte, c[0 .. 3] will alias the padding after "b". For
>>>   > "sizeof" this means that the padding is included, whereas for "offsetof"
>>>   > it is not, yielding "sizeof(struct s) == offsetof(struct s, c) + 4".
>>>   > That is what I meant by "wasted space" earlier, but this could possibly
>>>   > be made nicer with macros as necessary.
>>>   >
>>>   > As for this specific struct, the values should be identical as it is
>>>   > padded.
>>>   >
>>>   > Best regards,
>>>   > Marvin
>>>   >
>>>   > >
>>>   > > Quoting C99 6.7.2.1 "Structure and union specifiers", paragraph 16:
>>>   > >
>>>   > > "In most situations, the flexible array member is ignored. In
>>>   > > particular, the size of the structure is as if the flexible array
>>> member
>>>   > > were omitted except that it may have more trailing padding than the
>>>   > > omission would imply."
>>>   > >
>>>   > > Quoting footnotes 17 and 19,
>>>   > >
>>>   > > (17)  [...]
>>>   > >        struct s { int n; double d[]; };
>>>   > >        [...]
>>>   > >
>>>   > > (19)  [...]
>>>   > >        the expressions:
>>>   > >        [...]
>>>   > >        sizeof (struct s) >= offsetof(struct s, d)
>>>   > >
>>>   > >        are always equal to 1.
>>>   > >
>>>   > > Thanks
>>>   > > Laszlo
>>>   > >
>>>   > >
>>>   > >
>>>   > >> While sizeof would be a preferred way to move
>>>   > >> forward.
>>>   > >>
>>>   > >> Regards,
>>>   > >> Kun
>>>   > >>
>>>   > >> On 06/24/2021 08:25, Kinney, Michael D wrote:
>>>   > >>> Hello,
>>>   > >>>
>>>   > >>> Flexible array members are supported and should be used.  The old
>>>   > style
>>>   > >>> of adding an array of size [1] at the end of a structure was used
>>> at a
>>>   > >>> time
>>>   > >>> flexible array members were not supported by all compilers (late
>>>   > 1990's).
>>>   > >>> The workarounds used to handle the array of size [1] are very
>>>   > >>> confusing when
>>>   > >>> reading the C  code and the fact that sizeof() does not produce the
>>>   > >>> expected
>>>   > >>> result make it even worse.
>>>   > >>>
>>>   > >>> If we use flexible array members in this proposed change then
>>> there is
>>>   > >>> no need to use OFFSET_OF().  Correct?
>>>   > >>>
>>>   > >>> Mike
>>>   > >>>
>>>   > >>>
>>>   > >>>> -----Original Message-----
>>>   > >>>> From: Marvin Häuser <mhaeuser@posteo.de <mailto:mhaeuser@posteo.de>>
>>>   > >>>> Sent: Thursday, June 24, 2021 1:00 AM
>>>   > >>>> To: Kun Qin <kuqin12@gmail.com <mailto:kuqin12@gmail.com>>;
>>> Laszlo Ersek <lersek@redhat.com <mailto:lersek@redhat.com>>;
>>>   > >>>> devel@edk2.groups.io <mailto:devel@edk2.groups.io>
>>>   > >>>> Cc: Wang, Jian J <jian.j.wang@intel.com
>>> <mailto:jian.j.wang@intel.com>>; Wu, Hao A
>>>   > >>>> <hao.a.wu@intel.com <mailto:hao.a.wu@intel.com>>; Dong, Eric
>>> <eric.dong@intel.com <mailto:eric.dong@intel.com>>; Ni, Ray
>>>   > >>>> <ray.ni@intel.com <mailto:ray.ni@intel.com>>; Kinney, Michael D
>>> <michael.d.kinney@intel.com <mailto:michael.d.kinney@intel.com>>;
>>>   > >>>> Liming Gao <gaoliming@byosoft.com.cn
>>> <mailto:gaoliming@byosoft.com.cn>>; Liu, Zhiguang
>>>   > >>>> <zhiguang.liu@intel.com <mailto:zhiguang.liu@intel.com>>; Andrew
>>> Fish <afish@apple.com <mailto:afish@apple.com>>; Leif
>>>   > >>>> Lindholm <leif@nuviainc.com <mailto:leif@nuviainc.com>>; Bret
>>> Barkelew
>>>   > >>>> <Bret.Barkelew@microsoft.com
>>> <mailto:Bret.Barkelew@microsoft.com>>; michael.kubacki@microsoft.com
>>> <mailto:michael.kubacki@microsoft.com>
>>>   > >>>> Subject: Re: [edk2-devel] [PATCH v1 0/5] EDK2 Code First: PI
>>>   > >>>> Specification: Update EFI_MM_COMMUNICATE_HEADER
>>>   > >>>>
>>>   > >>>> Hey Kun,
>>>   > >>>>
>>>   > >>>> Why would you rely on undefined behaviours? The OFFSET_OF macro is
>>>   > >>>> well-defined for GCC and Clang as it's implemented by an
>>>   > intrinsic, and
>>>   > >>>> while the expression for the MSVC compiler is undefined behaviour
>>>   > as per
>>>   > >>>> the C standard, it is well-defined for MSVC due to their own
>>>   > >>>> implementation being identical. From my standpoint, all supported
>>>   > >>>> compilers will yield well-defined behaviour even this way.
>>>   > OFFSET_OF on
>>>   > >>>> flexible arrays is not UB in any case to my knowledge.
>>>   > >>>>
>>>   > >>>> However, the same way as your new suggestion, you can replace
>>>   > OFFSET_OF
>>>   > >>>> with sizeof. While this *can* lead to wasted space with certain
>>>   > >>>> structure layouts (e.g. when the flexible array overlays padding
>>>   > bytes),
>>>   > >>>> this is not the case here, and otherwise just loses you a few
>>>   > bytes. I
>>>   > >>>> think this comes down to preference.
>>>   > >>>>
>>>   > >>>> The pattern you mentioned arguably is less nice syntax when used
>>>   > >>>> (involves address calculation and casting), but the biggest
>>>   > problem here
>>>   > >>>> is alignment constraints. For packed structures, you lose the
>>>   > ability of
>>>   > >>>> automatic unaligned accesses (irrelevant here because the
>>>   > structure is
>>>   > >>>> manually padded anyway). For non-packed structures, you still
>>> need to
>>>   > >>>> ensure the alignment requirement of the trailing array data is met
>>>   > >>>> manually. With flexible array members, the compiler takes care of
>>>   > both
>>>   > >>>> cases automatically.
>>>   > >>>>
>>>   > >>>> Best regards,
>>>   > >>>> Marvin
>>>   > >>>>
>>>   > >>>> On 24.06.21 02:24, Kun Qin wrote:
>>>   > >>>>> Hi Marvin,
>>>   > >>>>>
>>>   > >>>>> I would prefer not to rely on undefined behaviors from different
>>>   > >>>>> compilers. Instead of using flexible arrays, is it better to remove
>>>   > >>>>> the `Data` field, pack the structure and follow
>>>   > >>>>> "VARIABLE_LOCK_ON_VAR_STATE_POLICY" pattern?
>>>   > >>>>>
>>>   > >>>>> In that case, OFFSET_OF will be forced to change to sizeof, and
>>>   > >>>>> read/write to `Data` will follow the range indicated by
>>>   > MessageLength.
>>>   > >>>>> But yes, that will enforce developers to update their platform
>>> level
>>>   > >>>>> implementations accordingly.
>>>   > >>>>>
>>>   > >>>>> Regards,
>>>   > >>>>> Kun
>>>   > >>>>>
>>>   > >>>>> On 06/23/2021 08:26, Laszlo Ersek wrote:
>>>   > >>>>>> On 06/23/21 08:54, Marvin Häuser wrote:
>>>   > >>>>>>> On 22.06.21 17:34, Laszlo Ersek wrote:
>>>   > >>>>>>>> On 06/18/21 11:37, Marvin Häuser wrote:
>>>   > >>>>>>>>> On 16.06.21 22:58, Kun Qin wrote:
>>>   > >>>>>>>>>> On 06/16/2021 00:02, Marvin Häuser wrote:
>>>   > >>>>>>>>>>> 2) Is it feasible yet with the current set of supported
>>>   > >>>>>>>>>>> compilers to
>>>   > >>>>>>>>>>> support flexible arrays?
>>>   > >>>>>>>>>> My impression is that flexible arrays are already
>>> supported (as
>>>   > >>>>>>>>>> seen
>>>   > >>>>>>>>>> in
>>>   > UnitTestFrameworkPkg/PrivateInclude/UnitTestFrameworkTypes.h).
>>>   > >>>>>>>>>> Please correct me if I am wrong.
>>>   > >>>>>>>>>>
>>>   > >>>>>>>>>> Would you mind letting me know why this is applicable here?
>>>   > We are
>>>   > >>>>>>>>>> trying to seek ideas on how to catch developer mistakes
>>>   > caused by
>>>   > >>>>>>>>>> this
>>>   > >>>>>>>>>> change. So any input is appreciated.
>>>   > >>>>>>>>> Huh, interesting. Last time I tried I was told about
>>>   > >>>>>>>>> incompatibilities
>>>   > >>>>>>>>> with MSVC, but I know some have been dropped since then
>>>   > (2005 and
>>>   > >>>>>>>>> 2008
>>>   > >>>>>>>>> if I recall correctly?), so that'd be great to allow globally.
>>>   > >>>>>>>> I too am surprised to see
>>>   > >>>>>>>>
>>>   > "UnitTestFrameworkPkg/PrivateInclude/UnitTestFrameworkTypes.h". The
>>>   > >>>>>>>> flexible array member is a C99 feature, and I didn't even know
>>>   > >>>>>>>> that we
>>>   > >>>>>>>> disallowed it for the sake of particular VS toolchains -- I
>>>   > >>>>>>>> thought we
>>>   > >>>>>>>> had a more general reason than just "not supported by VS
>>>   > versions X
>>>   > >>>>>>>> and Y".
>>>   > >>>>>>>>
>>>   > >>>>>>>> The behavior of OFFSET_OF() would be interesting -- the
>>>   > OFFSET_OF()
>>>   > >>>>>>>> macro definition for non-gcc / non-clang:
>>>   > >>>>>>>>
>>>   > >>>>>>>> #define OFFSET_OF(TYPE, Field) ((UINTN) &(((TYPE *)0)->Field))
>>>   > >>>>>>>>
>>>   > >>>>>>>> borders on undefined behavior as far as I can tell, so its
>>>   > >>>>>>>> behavior is
>>>   > >>>>>>>> totally up to the compiler. It works thus far okay on Visual
>>>   > >>>>>>>> Studio, but
>>>   > >>>>>>>> I couldn't say if it extended correctly to flexible array
>>>   > members.
>>>   > >>>>>>> Yes, it's UB by the standard, but this is actually how MS
>>>   > implements
>>>   > >>>>>>> them (or used to anyway?). I don't see why it'd cause issues with
>>>   > >>>>>>> flexible arrays, as only the start of the array is relevant
>>>   > (which is
>>>   > >>>>>>> constant for all instances of the structure no matter the
>>>   > amount of
>>>   > >>>>>>> elements actually stored). Any specific concern? If so, they
>>>   > could be
>>>   > >>>>>>> addressed by appropriate STATIC_ASSERTs.
>>>   > >>>>>> No specific concern; my point was that two aspects of the same
>>>   > "class"
>>>   > >>>>>> of undefined behavior didn't need to be consistent with each
>>> other.
>>>   > >>>>>>
>>>   > >>>>>> Thanks
>>>   > >>>>>> Laszlo
>>>   > >>>>>>
>>>   >
>>>
>>> 
>>>