-----Original Message-----
From: Kun Qin <kuqi...@gmail.com>
Sent: Wednesday, June 16, 2021 4:51 AM
To: Wu, Hao A <hao.a...@intel.com>; devel@edk2.groups.io
Cc: Kinney, Michael D <michael.d.kin...@intel.com>; Liming Gao
<gaolim...@byosoft.com.cn>; Liu, Zhiguang <zhiguang....@intel.com>;
Andrew Fish <af...@apple.com>; Laszlo Ersek <ler...@redhat.com>; Leif
Lindholm <l...@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.kin...@intel.com>; Liming Gao
<gaolim...@byosoft.com.cn>; Liu, Zhiguang <zhiguang....@intel.com>;
Andrew Fish <af...@apple.com>; Laszlo Ersek <ler...@redhat.com>; Leif
Lindholm <l...@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.kin...@intel.com>
Cc: Liming Gao <gaolim...@byosoft.com.cn>
Cc: Zhiguang Liu <zhiguang....@intel.com>
Cc: Andrew Fish <af...@apple.com>
Cc: Laszlo Ersek <ler...@redhat.com>
Cc: Leif Lindholm <l...@nuviainc.com>
Signed-off-by: Kun Qin <kuqi...@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
