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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#76559): https://edk2.groups.io/g/devel/message/76559
Mute This Topic: https://groups.io/mt/83435922/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
