REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3430 This patch series is a follow up of previous submission: https://edk2.groups.io/g/devel/message/76303 v2 patch changes include feedback for v1 series: a. Added "Reviewed-by" tag for applicable patch; b. Removed "BZ3398" tags for applicable patches; c. Added a patch that covers changes needed for SmmLockBoxDxeLib; There is also concern raised from v1 patch review: https://edk2.groups.io/g/devel/message/76570 Please advise if any tool/checks could help catching errors as such. Patch v2 branch: https://github.com/kuqin12/edk2/tree/BZ3398-MmCommunicate-Length-v2 Cc: Jian J Wang <jian.j.wang@intel.com> Cc: Hao A Wu <hao.a.wu@intel.com> Cc: Eric Dong <eric.dong@intel.com> Cc: Ray Ni <ray.ni@intel.com> Cc: Michael D Kinney <michael.d.kinney@intel.com> Cc: Liming Gao <gaoliming@byosoft.com.cn> Cc: Zhiguang Liu <zhiguang.liu@intel.com> Cc: Andrew Fish <afish@apple.com> Cc: Laszlo Ersek <lersek@redhat.com> Cc: Leif Lindholm <leif@nuviainc.com> Kun Qin (6): EDK2 Code First: PI Specification: EFI_MM_COMMUNICATE_HEADER Update MdeModulePkg: PiSmmIpl: Update MessageLength calculation for MmCommunicate MdeModulePkg: MemoryProfileInfo: Updated MessageLength calculation MdeModulePkg: SmiHandlerProfileInfo: Updated MessageLength calculation MdeModulePkg: SmmLockBoxDxeLib: Updated MessageLength calculation MdePkg: MmCommunication: Extend MessageLength field size to UINT64 MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.c | 28 ++++-- MdeModulePkg/Application/SmiHandlerProfileInfo/SmiHandlerProfileInfo.c | 10 ++- MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c | 11 ++- MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxDxeLib.c | 23 +++-- BZ3430-SpecChange.md | 90 ++++++++++++++++++++ MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf | 1 + MdePkg/Include/Protocol/MmCommunication.h | 2 +- 7 files changed, 142 insertions(+), 23 deletions(-) create mode 100644 BZ3430-SpecChange.md -- 2.31.1.windows.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#76738): https://edk2.groups.io/g/devel/message/76738 Mute This Topic: https://groups.io/mt/83624114/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=- REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3430 This change includes specification update markdown file that describes the proposed PI Specification v1.7 Errata A in detail and potential impact to the existing codebase. Cc: Michael D Kinney <michael.d.kinney@intel.com> Cc: Liming Gao <gaoliming@byosoft.com.cn> Cc: Zhiguang Liu <zhiguang.liu@intel.com> Cc: Andrew Fish <afish@apple.com> Cc: Laszlo Ersek <lersek@redhat.com> Cc: Leif Lindholm <leif@nuviainc.com> Cc: Hao A Wu <hao.a.wu@intel.com> Signed-off-by: Kun Qin <kuqin12@gmail.com> --- Notes: v2: - Updated change impact analysis regarding SmmLockBoxDxeLib [Hao] BZ3430-SpecChange.md | 90 ++++++++++++++++++++ 1 file changed, 90 insertions(+) diff --git a/BZ3430-SpecChange.md b/BZ3430-SpecChange.md new file mode 100644 index XXXXXXX..XXXXXXX --- /dev/null +++ b/BZ3430-SpecChange.md @@ -XXX,XX +XXX,XX @@ +# Title: Change MessageLength Field of EFI_MM_COMMUNICATE_HEADER to UINT64 + +## Status: Draft + +## Document: UEFI Platform Initialization Specification Version 1.7 Errata A + +## License + +SPDX-License-Identifier: CC-BY-4.0 + +## Submitter: [TianoCore Community](https://www.tianocore.org) + +## Summary of the change + +Change the `MessageLength` Field of `EFI_MM_COMMUNICATE_HEADER` from UINTN to UINT64 to remove architecture dependency: + +```c +typedef struct { + EFI_GUID HeaderGuid; + UINT64 MessageLength; + UINT8 Data[ANYSIZE_ARRAY]; +} EFI_MM_COMMUNICATE_HEADER; +``` + +## Benefits of the change + +In PI Spec v1.7 Errata A, Vol.4, Sec 5.7 MM Communication Protocol, the MessageLength field of `EFI_MM_COMMUNICATE_HEADER` (also defined as `EFI_SMM_COMMUNICATE_HEADER`) is defined as type UINTN. + +But this structure, as a generic definition, could be used for both PEI and DXE MM communication. Thus for a system that supports PEI MM launch, but operates PEI in 32bit mode and MM foundation in 64bit, the current `EFI_MM_COMMUNICATE_HEADER` definition will cause structure parse error due to UINTN used. + +## Impact of the change + +This change will impact the known structure consumers including: + +```bash +MdeModulePkg/Core/PiSmmCore/PiSmmIpl +MdeModulePkg/Application/SmiHandlerProfileInfo +MdeModulePkg/Application/MemoryProfileInfo +MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxDxeLib +``` + +For consumers that are not using `OFFSET_OF(EFI_MM_COMMUNICATE_HEADER, Data)`, but performing explicit addition such as the existing MdeModulePkg/Application/SmiHandlerProfileInfo/SmiHandlerProfileInfo.c, one will need to change code implementation to match new structure definition. Otherwise, the code compiled on IA32 architecture will experience structure field dereference error. + +User who currently uses UINTN local variables as place holder of MessageLength will need to use caution to make cast from UINTN to UINT64 and vice versa. It is recommended to use `SafeUint64ToUintn` for such operations when the value is indeterministic. + +Note: MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxPeiLib is also consuming this structure, but it handled this size discrepancy internally. If this potential spec change is not applied, all applicable PEI MM communicate callers will need to use the same routine as that of SmmLockBoxPeiLib to invoke a properly populated EFI_MM_COMMUNICATE_HEADER to be used in X64 MM foundation. + +## Detailed description of the change [normative updates] + +### Specification Changes + +1. In PI Specification v1.7 Errata A: Vol. 4 Page-91, the definition of `EFI_MM_COMMUNICATE_HEADER` should be changed from current: + +```c +typedef struct { + EFI_GUID HeaderGuid; + UINTN MessageLength; + UINT8 Data[ANYSIZE_ARRAY]; +} EFI_MM_COMMUNICATE_HEADER; +``` + +to: + +```c +typedef struct { + EFI_GUID HeaderGuid; + UINT64 MessageLength; + UINT8 Data[ANYSIZE_ARRAY]; +} EFI_MM_COMMUNICATE_HEADER; +``` + +### Code Changes + +1. Remove the explicit calculation of the offset of `Data` in `EFI_MM_COMMUNICATE_HEADER`. Thus applicable calculations of `sizeof(EFI_GUID) + sizeof(UINTN)` should be replaced with `OFFSET_OF(EFI_MM_COMMUNICATE_HEADER, Data)` or similar alternatives. These calculations are identified in: + +```bash +MdeModulePkg/Application/SmiHandlerProfileInfo/SmiHandlerProfileInfo.c +MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.c +MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxDxeLib.c +``` + +1. Resolve potentially mismatched type between `UINTN` and `UINT64`. This would occur when `MessageLength` or its derivitive are used for local calculation with existing `UINTN` typed variables. Code change regarding this perspective is per case evaluation: if the variables involved are all deterministic values, and there is no overflow or underflow risk, a cast operation (from `UINTN` to `UINT64`) can be safely used. Otherwise, the calculation will be performed in `UINT64` bitwidth and then convert to `UINTN` using `SafeUint64*` and `SafeUint64ToUintn`, respectively. These operations are identified in: + +```bash +MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c +MdeModulePkg/Application/SmiHandlerProfileInfo/SmiHandlerProfileInfo.c +MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.c +``` + +1. After all above changes applied and specification updated, `MdePkg/Include/Protocol/MmCommunication.h` will need to be updated to match new definition that includes the field type update. -- 2.31.1.windows.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#76739): https://edk2.groups.io/g/devel/message/76739 Mute This Topic: https://groups.io/mt/83624115/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=- REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3398 This change updated calculation routine for MM communication in PiSmmIpl. It removes ambiguity brought in by UINTN variables from this routine and paves way for updating definition of field MessageLength in EFI_MM_COMMUNICATE_HEADER to definitive size. Cc: Jian J Wang <jian.j.wang@intel.com> Cc: Hao A Wu <hao.a.wu@intel.com> Cc: Eric Dong <eric.dong@intel.com> Cc: Ray Ni <ray.ni@intel.com> Signed-off-by: Kun Qin <kuqin12@gmail.com> Reviewed-by: Hao A Wu <hao.a.wu@intel.com> --- Notes: v2: - Removed "BZ" tags from comments and variables [Hao] - Added "Reviewed-by" tag [Hao] MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c | 11 ++++++++++- MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf | 1 + 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c index XXXXXXX..XXXXXXX 100644 --- a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c +++ b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c @@ -XXX,XX +XXX,XX @@ #include <Library/UefiRuntimeLib.h> #include <Library/PcdLib.h> #include <Library/ReportStatusCodeLib.h> +#include <Library/SafeIntLib.h> #include "PiSmmCorePrivateData.h" @@ -XXX,XX +XXX,XX @@ SmmCommunicationCommunicate ( EFI_STATUS Status; EFI_SMM_COMMUNICATE_HEADER *CommunicateHeader; BOOLEAN OldInSmm; + UINT64 LongCommSize; UINTN TempCommSize; // @@ -XXX,XX +XXX,XX @@ SmmCommunicationCommunicate ( CommunicateHeader = (EFI_SMM_COMMUNICATE_HEADER *) CommBuffer; if (CommSize == NULL) { - TempCommSize = OFFSET_OF (EFI_SMM_COMMUNICATE_HEADER, Data) + CommunicateHeader->MessageLength; + Status = SafeUint64Add (OFFSET_OF (EFI_SMM_COMMUNICATE_HEADER, Data), CommunicateHeader->MessageLength, &LongCommSize); + if (EFI_ERROR (Status)) { + return EFI_INVALID_PARAMETER; + } + Status = SafeUint64ToUintn (LongCommSize, &TempCommSize); + if (EFI_ERROR (Status)) { + return EFI_INVALID_PARAMETER; + } } else { TempCommSize = *CommSize; // diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf index XXXXXXX..XXXXXXX 100644 --- a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf +++ b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf @@ -XXX,XX +XXX,XX @@ [LibraryClasses] DxeServicesLib PcdLib ReportStatusCodeLib + SafeIntLib [Protocols] gEfiSmmBase2ProtocolGuid ## PRODUCES -- 2.31.1.windows.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#76740): https://edk2.groups.io/g/devel/message/76740 Mute This Topic: https://groups.io/mt/83624116/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=- REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3398 This change replaced the calculation of communication buffer size from explicitly adding the size of each member with the OFFSET macro function. This will make the structure field defition change transparent to consumers. Cc: Jian J Wang <jian.j.wang@intel.com> Cc: Hao A Wu <hao.a.wu@intel.com> Signed-off-by: Kun Qin <kuqin12@gmail.com> --- Notes: v2: - Added a missed case this change should cover [Hao] - Removed "BZ" tags from comments [Hao] MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.c | 28 +++++++++++++++----- 1 file changed, 21 insertions(+), 7 deletions(-) diff --git a/MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.c b/MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.c index XXXXXXX..XXXXXXX 100644 --- a/MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.c +++ b/MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.c @@ -XXX,XX +XXX,XX @@ GetSmramProfileData ( return Status; } - MinimalSizeNeeded = sizeof (EFI_GUID) + - sizeof (UINTN) + + MinimalSizeNeeded = OFFSET_OF(EFI_SMM_COMMUNICATE_HEADER, Data) + MAX (sizeof (SMRAM_PROFILE_PARAMETER_GET_PROFILE_INFO), MAX (sizeof (SMRAM_PROFILE_PARAMETER_GET_PROFILE_DATA_BY_OFFSET), sizeof (SMRAM_PROFILE_PARAMETER_RECORDING_STATE))); @@ -XXX,XX +XXX,XX @@ GetSmramProfileData ( CommRecordingState->Header.ReturnStatus = (UINT64)-1; CommRecordingState->RecordingState = MEMORY_PROFILE_RECORDING_DISABLE; - CommSize = sizeof (EFI_GUID) + sizeof (UINTN) + CommHeader->MessageLength; + // + // The CommHeader->MessageLength contains a definitive value, thus UINTN cast is safe here. + // + CommSize = OFFSET_OF(EFI_SMM_COMMUNICATE_HEADER, Data) + (UINTN)CommHeader->MessageLength; Status = SmmCommunication->Communicate (SmmCommunication, CommBuffer, &CommSize); if (EFI_ERROR (Status)) { DEBUG ((EFI_D_ERROR, "SmramProfile: SmmCommunication - %r\n", Status)); @@ -XXX,XX +XXX,XX @@ GetSmramProfileData ( CommRecordingState->Header.ReturnStatus = (UINT64)-1; CommRecordingState->RecordingState = MEMORY_PROFILE_RECORDING_DISABLE; - CommSize = sizeof (EFI_GUID) + sizeof (UINTN) + CommHeader->MessageLength; + // + // The CommHeader->MessageLength contains a definitive value, thus UINTN cast is safe here. + // + CommSize = OFFSET_OF(EFI_SMM_COMMUNICATE_HEADER, Data) + (UINTN)CommHeader->MessageLength; SmmCommunication->Communicate (SmmCommunication, CommBuffer, &CommSize); } @@ -XXX,XX +XXX,XX @@ GetSmramProfileData ( CommGetProfileInfo->Header.ReturnStatus = (UINT64)-1; CommGetProfileInfo->ProfileSize = 0; - CommSize = sizeof (EFI_GUID) + sizeof (UINTN) + CommHeader->MessageLength; + // + // The CommHeader->MessageLength contains a definitive value, thus UINTN cast is safe here. + // + CommSize = OFFSET_OF(EFI_SMM_COMMUNICATE_HEADER, Data) + (UINTN)CommHeader->MessageLength; Status = SmmCommunication->Communicate (SmmCommunication, CommBuffer, &CommSize); ASSERT_EFI_ERROR (Status); @@ -XXX,XX +XXX,XX @@ GetSmramProfileData ( CommGetProfileData->Header.DataLength = sizeof (*CommGetProfileData); CommGetProfileData->Header.ReturnStatus = (UINT64)-1; - CommSize = sizeof (EFI_GUID) + sizeof (UINTN) + CommHeader->MessageLength; + // + // The CommHeader->MessageLength contains a definitive value, thus UINTN cast is safe here. + // + CommSize = OFFSET_OF(EFI_SMM_COMMUNICATE_HEADER, Data) + (UINTN)CommHeader->MessageLength; Buffer = (UINT8 *) CommHeader + CommSize; Size -= CommSize; @@ -XXX,XX +XXX,XX @@ GetSmramProfileData ( CommRecordingState->Header.ReturnStatus = (UINT64)-1; CommRecordingState->RecordingState = MEMORY_PROFILE_RECORDING_ENABLE; - CommSize = sizeof (EFI_GUID) + sizeof (UINTN) + CommHeader->MessageLength; + // + // The CommHeader->MessageLength contains a definitive value, thus UINTN cast is safe here. + // + CommSize = OFFSET_OF(EFI_SMM_COMMUNICATE_HEADER, Data) + (UINTN)CommHeader->MessageLength; SmmCommunication->Communicate (SmmCommunication, CommBuffer, &CommSize); } -- 2.31.1.windows.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#76741): https://edk2.groups.io/g/devel/message/76741 Mute This Topic: https://groups.io/mt/83624117/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=- REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3398 This change replaced the calculation of communication buffer size from explicitly adding the size of each member with the OFFSET macro function. This will make the structure field defition change transparent to consumers. Cc: Jian J Wang <jian.j.wang@intel.com> Cc: Hao A Wu <hao.a.wu@intel.com> Cc: Eric Dong <eric.dong@intel.com> Cc: Ray Ni <ray.ni@intel.com> Signed-off-by: Kun Qin <kuqin12@gmail.com> --- Notes: v2: - Updated comments by removing "BZ" tags [Hao] MdeModulePkg/Application/SmiHandlerProfileInfo/SmiHandlerProfileInfo.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/MdeModulePkg/Application/SmiHandlerProfileInfo/SmiHandlerProfileInfo.c b/MdeModulePkg/Application/SmiHandlerProfileInfo/SmiHandlerProfileInfo.c index XXXXXXX..XXXXXXX 100644 --- a/MdeModulePkg/Application/SmiHandlerProfileInfo/SmiHandlerProfileInfo.c +++ b/MdeModulePkg/Application/SmiHandlerProfileInfo/SmiHandlerProfileInfo.c @@ -XXX,XX +XXX,XX @@ GetSmiHandlerProfileDatabase( CommGetInfo->Header.ReturnStatus = (UINT64)-1; CommGetInfo->DataSize = 0; - CommSize = sizeof(EFI_GUID) + sizeof(UINTN) + CommHeader->MessageLength; + // + // The CommHeader->MessageLength contains a definitive value, thus UINTN cast is safe here. + // + CommSize = OFFSET_OF(EFI_SMM_COMMUNICATE_HEADER, Data) + (UINTN)CommHeader->MessageLength; Status = SmmCommunication->Communicate(SmmCommunication, CommBuffer, &CommSize); if (EFI_ERROR(Status)) { Print(L"SmiHandlerProfile: SmmCommunication - %r\n", Status); @@ -XXX,XX +XXX,XX @@ GetSmiHandlerProfileDatabase( CommGetData->Header.DataLength = sizeof(*CommGetData); CommGetData->Header.ReturnStatus = (UINT64)-1; - CommSize = sizeof(EFI_GUID) + sizeof(UINTN) + CommHeader->MessageLength; + // + // The CommHeader->MessageLength contains a definitive value, thus UINTN cast is safe here. + // + CommSize = OFFSET_OF(EFI_SMM_COMMUNICATE_HEADER, Data) + (UINTN)CommHeader->MessageLength; Buffer = (UINT8 *)CommHeader + CommSize; Size -= CommSize; -- 2.31.1.windows.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#76742): https://edk2.groups.io/g/devel/message/76742 Mute This Topic: https://groups.io/mt/83624118/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=- REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3398 This change replaced the calculation of communication buffer size from explicitly adding the size of each member with the OFFSET macro function. This will make the structure field defition change transparent to consumers. Cc: Jian J Wang <jian.j.wang@intel.com> Cc: Hao A Wu <hao.a.wu@intel.com> Cc: Eric Dong <eric.dong@intel.com> Cc: Ray Ni <ray.ni@intel.com> Signed-off-by: Kun Qin <kuqin12@gmail.com> --- Notes: v2: - Newly added in v2 MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxDxeLib.c | 23 ++++++++++---------- 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxDxeLib.c b/MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxDxeLib.c index XXXXXXX..XXXXXXX 100644 --- a/MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxDxeLib.c +++ b/MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxDxeLib.c @@ -XXX,XX +XXX,XX @@ LockBoxGetSmmCommBuffer ( return mLockBoxSmmCommBuffer; } - MinimalSizeNeeded = sizeof (EFI_GUID) + - sizeof (UINTN) + + MinimalSizeNeeded = OFFSET_OF (EFI_SMM_COMMUNICATE_HEADER, Data) + MAX (sizeof (EFI_SMM_LOCK_BOX_PARAMETER_SAVE), MAX (sizeof (EFI_SMM_LOCK_BOX_PARAMETER_SET_ATTRIBUTES), MAX (sizeof (EFI_SMM_LOCK_BOX_PARAMETER_UPDATE), @@ -XXX,XX +XXX,XX @@ SaveLockBox ( EFI_SMM_COMMUNICATION_PROTOCOL *SmmCommunication; EFI_SMM_LOCK_BOX_PARAMETER_SAVE *LockBoxParameterSave; EFI_SMM_COMMUNICATE_HEADER *CommHeader; - UINT8 TempCommBuffer[sizeof(EFI_GUID) + sizeof(UINTN) + sizeof(EFI_SMM_LOCK_BOX_PARAMETER_SAVE)]; + UINT8 TempCommBuffer[OFFSET_OF (EFI_SMM_COMMUNICATE_HEADER, Data) + sizeof(EFI_SMM_LOCK_BOX_PARAMETER_SAVE)]; UINT8 *CommBuffer; UINTN CommSize; @@ -XXX,XX +XXX,XX @@ SaveLockBox ( // // Send command // - CommSize = sizeof(EFI_GUID) + sizeof(UINTN) + sizeof(EFI_SMM_LOCK_BOX_PARAMETER_SAVE); + CommSize = OFFSET_OF (EFI_SMM_COMMUNICATE_HEADER, Data) + sizeof(EFI_SMM_LOCK_BOX_PARAMETER_SAVE); Status = SmmCommunication->Communicate ( SmmCommunication, &CommBuffer[0], @@ -XXX,XX +XXX,XX @@ SetLockBoxAttributes ( EFI_SMM_COMMUNICATION_PROTOCOL *SmmCommunication; EFI_SMM_LOCK_BOX_PARAMETER_SET_ATTRIBUTES *LockBoxParameterSetAttributes; EFI_SMM_COMMUNICATE_HEADER *CommHeader; - UINT8 TempCommBuffer[sizeof(EFI_GUID) + sizeof(UINTN) + sizeof(EFI_SMM_LOCK_BOX_PARAMETER_SET_ATTRIBUTES)]; + UINT8 TempCommBuffer[OFFSET_OF (EFI_SMM_COMMUNICATE_HEADER, Data) + sizeof(EFI_SMM_LOCK_BOX_PARAMETER_SET_ATTRIBUTES)]; UINT8 *CommBuffer; UINTN CommSize; @@ -XXX,XX +XXX,XX @@ SetLockBoxAttributes ( // // Send command // - CommSize = sizeof(EFI_GUID) + sizeof(UINTN) + sizeof(EFI_SMM_LOCK_BOX_PARAMETER_SET_ATTRIBUTES); + CommSize = OFFSET_OF (EFI_SMM_COMMUNICATE_HEADER, Data) + sizeof(EFI_SMM_LOCK_BOX_PARAMETER_SET_ATTRIBUTES); Status = SmmCommunication->Communicate ( SmmCommunication, &CommBuffer[0], @@ -XXX,XX +XXX,XX @@ UpdateLockBox ( EFI_SMM_COMMUNICATION_PROTOCOL *SmmCommunication; EFI_SMM_LOCK_BOX_PARAMETER_UPDATE *LockBoxParameterUpdate; EFI_SMM_COMMUNICATE_HEADER *CommHeader; - UINT8 TempCommBuffer[sizeof(EFI_GUID) + sizeof(UINTN) + sizeof(EFI_SMM_LOCK_BOX_PARAMETER_UPDATE)]; + UINT8 TempCommBuffer[OFFSET_OF (EFI_SMM_COMMUNICATE_HEADER, Data) + sizeof(EFI_SMM_LOCK_BOX_PARAMETER_UPDATE)]; UINT8 *CommBuffer; UINTN CommSize; @@ -XXX,XX +XXX,XX @@ UpdateLockBox ( // // Send command // - CommSize = sizeof(EFI_GUID) + sizeof(UINTN) + sizeof(EFI_SMM_LOCK_BOX_PARAMETER_UPDATE); + CommSize = OFFSET_OF (EFI_SMM_COMMUNICATE_HEADER, Data) + sizeof(EFI_SMM_LOCK_BOX_PARAMETER_UPDATE); Status = SmmCommunication->Communicate ( SmmCommunication, &CommBuffer[0], @@ -XXX,XX +XXX,XX @@ RestoreLockBox ( EFI_SMM_COMMUNICATION_PROTOCOL *SmmCommunication; EFI_SMM_LOCK_BOX_PARAMETER_RESTORE *LockBoxParameterRestore; EFI_SMM_COMMUNICATE_HEADER *CommHeader; - UINT8 TempCommBuffer[sizeof(EFI_GUID) + sizeof(UINTN) + sizeof(EFI_SMM_LOCK_BOX_PARAMETER_RESTORE)]; + UINT8 TempCommBuffer[OFFSET_OF (EFI_SMM_COMMUNICATE_HEADER, Data) + sizeof(EFI_SMM_LOCK_BOX_PARAMETER_RESTORE)]; UINT8 *CommBuffer; UINTN CommSize; @@ -XXX,XX +XXX,XX @@ RestoreLockBox ( // // Send command // - CommSize = sizeof(EFI_GUID) + sizeof(UINTN) + sizeof(EFI_SMM_LOCK_BOX_PARAMETER_RESTORE); + CommSize = OFFSET_OF (EFI_SMM_COMMUNICATE_HEADER, Data) + sizeof(EFI_SMM_LOCK_BOX_PARAMETER_RESTORE); Status = SmmCommunication->Communicate ( SmmCommunication, &CommBuffer[0], @@ -XXX,XX +XXX,XX @@ RestoreAllLockBoxInPlace ( EFI_SMM_COMMUNICATION_PROTOCOL *SmmCommunication; EFI_SMM_LOCK_BOX_PARAMETER_RESTORE_ALL_IN_PLACE *LockBoxParameterRestoreAllInPlace; EFI_SMM_COMMUNICATE_HEADER *CommHeader; - UINT8 TempCommBuffer[sizeof(EFI_GUID) + sizeof(UINTN) + sizeof(EFI_SMM_LOCK_BOX_PARAMETER_RESTORE_ALL_IN_PLACE)]; + UINT8 TempCommBuffer[OFFSET_OF (EFI_SMM_COMMUNICATE_HEADER, Data) + sizeof(EFI_SMM_LOCK_BOX_PARAMETER_RESTORE_ALL_IN_PLACE)]; UINT8 *CommBuffer; UINTN CommSize; @@ -XXX,XX +XXX,XX @@ RestoreAllLockBoxInPlace ( // // Send command // - CommSize = sizeof(EFI_GUID) + sizeof(UINTN) + sizeof(EFI_SMM_LOCK_BOX_PARAMETER_RESTORE_ALL_IN_PLACE); + CommSize = OFFSET_OF (EFI_SMM_COMMUNICATE_HEADER, Data) + sizeof(EFI_SMM_LOCK_BOX_PARAMETER_RESTORE_ALL_IN_PLACE); Status = SmmCommunication->Communicate ( SmmCommunication, &CommBuffer[0], -- 2.31.1.windows.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#76743): https://edk2.groups.io/g/devel/message/76743 Mute This Topic: https://groups.io/mt/83624119/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=- REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3398 REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3430 The MessageLength field of EFI_MM_COMMUNICATE_HEADER, as a generic definition, could be used for both PEI and DXE MM communication. On a system that supports PEI MM launch, but operates PEI in 32bit mode and MM foundation in 64bit, the current EFI_MM_COMMUNICATE_HEADER definition will cause structure parse error due to UINTN used. This change removes the architecture dependent field by extending this field definition as UINT64. Cc: Michael D Kinney <michael.d.kinney@intel.com> Cc: Liming Gao <gaoliming@byosoft.com.cn> Cc: Zhiguang Liu <zhiguang.liu@intel.com> Signed-off-by: Kun Qin <kuqin12@gmail.com> --- Notes: v2: - Removed comments with "BZ" MdePkg/Include/Protocol/MmCommunication.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/MdePkg/Include/Protocol/MmCommunication.h b/MdePkg/Include/Protocol/MmCommunication.h index XXXXXXX..XXXXXXX 100644 --- a/MdePkg/Include/Protocol/MmCommunication.h +++ b/MdePkg/Include/Protocol/MmCommunication.h @@ -XXX,XX +XXX,XX @@ typedef struct { /// /// Describes the size of Data (in bytes) and does not include the size of the header. /// - UINTN MessageLength; + UINT64 MessageLength; /// /// Designates an array of bytes that is MessageLength in size. /// -- 2.31.1.windows.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#76744): https://edk2.groups.io/g/devel/message/76744 Mute This Topic: https://groups.io/mt/83624120/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=- REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3794 This patch series is a rebase of previous submission: https://edk2.groups.io/g/devel/message/85302 In current Status Codes definitions of PI spec v1.7 errata, there are a few instances where the software could trigger system reboots while the corresponding case were not covered by the already defined status codes. One scenario that OEMs would be interested is that fragmented memory map from boot to boot would fail to meet certain OS ACPI requirements (i.e. S4 resume boot requires consistent memory maps) and trigger system reboots. Yet the corresponding case was not covered by the already defined status codes. The unexpected system reboots above could indicate decay of system health and reporting of such generic events would provide helpful information to OEMs to investigate/prevent system failures in general. The change intends to expand definitions of `EFI_SW_EC_**` under Status Codes to cover more unexpected system reboot events, which could improve Status Code futility and readability. Compared to v1 series, v2 patch changes include: a. Removed "RELEASE_ASSERT" definition; b. Removed reference to memory type information; Patch v2 branch: https://github.com/kuqin12/edk2/tree/BZ3794-expand_status_codes_v2 Cc: Andrew Fish <afish@apple.com> Cc: Leif Lindholm <leif@nuviainc.com> Cc: Michael D Kinney <michael.d.kinney@intel.com> Cc: Liming Gao <gaoliming@byosoft.com.cn> Cc: Zhiguang Liu <zhiguang.liu@intel.com> Kun Qin (2): EDK2 Code First: PI Specification: New error codes of Host Software class MdePkg: MmCommunication: Add new Host Software class Error Code to MdePkg CodeFirst/BZ3794-SpecChange.md | 55 ++++++++++++++++++++ MdePkg/Include/Pi/PiStatusCode.h | 1 + 2 files changed, 56 insertions(+) create mode 100644 CodeFirst/BZ3794-SpecChange.md -- 2.34.1.windows.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#85335): https://edk2.groups.io/g/devel/message/85335 Mute This Topic: https://groups.io/mt/88276262/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=- REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3794 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: Andrew Fish <afish@apple.com> Cc: Leif Lindholm <leif@nuviainc.com> Cc: Michael D Kinney <michael.d.kinney@intel.com> Cc: Liming Gao <gaoliming@byosoft.com.cn> Cc: Zhiguang Liu <zhiguang.liu@intel.com> Signed-off-by: Kun Qin <kuqin12@gmail.com> --- Notes: v2: - Removed "RELEASE_ASSERT" definition - Removed reference to EDK2 based memory type info [Mike] CodeFirst/BZ3794-SpecChange.md | 55 ++++++++++++++++++++ 1 file changed, 55 insertions(+) diff --git a/CodeFirst/BZ3794-SpecChange.md b/CodeFirst/BZ3794-SpecChange.md new file mode 100644 index XXXXXXX..XXXXXXX --- /dev/null +++ b/CodeFirst/BZ3794-SpecChange.md @@ -XXX,XX +XXX,XX @@ +# Title: Introduction of `EFI_SW_EC_FRAGMENTED_MEMORY_MAP` Status Code + +## 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 + +Add `EFI_SW_EC_FRAGMENTED_MEMORY_MAP` into Status Codes definition. + +## Benefits of the change + +Current Status Codes covered various [software class error code definitions](https://github.com/tianocore/edk2/blob/master/MdePkg/Include/Pi/PiStatusCode.h). + +However, fragmented memory map from boot to boot would fail to meet certain OS ACPI requirements (i.e. S4 resume boot requires consistent memory maps) and trigger system reboots. Yet the corresponding case was not covered by the already defined status codes. + +The unexpected system reboots above could indicate decay of system health and reporting of such generic events would provide helpful information to OEMs to investigate/prevent system failures in general. + +The request of this change intends to expand definitions of `EFI_SW_EC_**` under Status Codes to cover more unexpected system reboot events, which could improve Status Code futility and readability. + +## Impact of the change + +Occupy a new macro definitions of Error Codes under Software class Status Codes. + +## Detailed description of the change [normative updates] + +### Specification Changes + +1. In PI Specification v1.7 Errata A: Vol. 3, Table 3-61: Error Code Operations: Host Software Class, add one new rows below `EFI_SW_EC_FV_CORRUPTED` definition: + + | Operation | Description | Extended Data | + | --- | --- | --- | + | EFI_SW_EC_FRAGMENTED_MEMORY_MAP | System will reboot due to fragmented memory maps | None | + +1. In PI Specification v1.7 Errata A: Vol. 3, Table 3-61: Error Code Operations: Host Software Class, replace the row of `0x0014-0x00FF` to: + + | Operation | Description | Extended Data | + | --- | --- | --- | + | 0x0015-0x00FF | Reserved for future use by this specification for Host Software class error codes. | None | + +1. In PI Specification v1.7 Errata A: Vol. 3, Section 6.7.4.3 Error Code Definitions: Prototype, add one new definitions below `EFI_SW_EC_FV_CORRUPTED` definition: + + ```c + #define EFI_SW_EC_FRAGMENTED_MEMORY_MAP 0x00000014 + ``` + +### Code Changes + +1. Add macro definitions in `MdePkg/Include/Pi/PiStatusCode.h` to match new specification. -- 2.34.1.windows.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#85336): https://edk2.groups.io/g/devel/message/85336 Mute This Topic: https://groups.io/mt/88276263/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=- REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3794 This change introduces a new error code definitions under Host Software class. The new error code definition will cover system reboot events under the conditions of fragmented memory map from one boot to another. These error codes could provide helpful datapoints to OEMs to investigate and prevent system failures in general. Cc: Michael D Kinney <michael.d.kinney@intel.com> Cc: Liming Gao <gaoliming@byosoft.com.cn> Cc: Zhiguang Liu <zhiguang.liu@intel.com> Signed-off-by: Kun Qin <kuqin12@gmail.com> --- Notes: v2: - Removed "RELEASE_ASSERT" definition - Removed reference to EDK2 based memory type info [Mike] MdePkg/Include/Pi/PiStatusCode.h | 1 + 1 file changed, 1 insertion(+) diff --git a/MdePkg/Include/Pi/PiStatusCode.h b/MdePkg/Include/Pi/PiStatusCode.h index XXXXXXX..XXXXXXX 100644 --- a/MdePkg/Include/Pi/PiStatusCode.h +++ b/MdePkg/Include/Pi/PiStatusCode.h @@ -XXX,XX +XXX,XX @@ typedef struct { #define EFI_SW_EC_EVENT_LOG_FULL 0x00000011 #define EFI_SW_EC_WRITE_PROTECTED 0x00000012 #define EFI_SW_EC_FV_CORRUPTED 0x00000013 +#define EFI_SW_EC_FRAGMENTED_MEMORY_MAP 0x00000014 ///@} // -- 2.34.1.windows.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#85337): https://edk2.groups.io/g/devel/message/85337 Mute This Topic: https://groups.io/mt/88276264/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-