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

["Wu, Hao A" <hao.a.wu@intel.com>]

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