> +
> SmmSmramSaveStateLib|UefiCpuPkg/Library/SmmSmramSaveStateLib/Am
> dSmmSmramSaveStateLib.inf
1. The lib instance name can be AmdMmSaveStateLib inside X86MmSaveStateLib
folder.
> +[Defines]
> + INF_VERSION = 1.29
> + BASE_NAME = AmdSmmSmramSaveStateLib
> + FILE_GUID = FB7D0A60-E8D4-4EFA-90AA-B357BC569879
> + MODULE_TYPE = DXE_SMM_DRIVER
2. The module type can be BASE.
> + VERSION_STRING = 1.0
> + LIBRARY_CLASS = SmmSmramSaveStateLib
3. The LIBRARY_CLASS can be "MmSaveStateLib|DXE_SMM_DRIVER MM_STANDALONE"
indicating it supports the two types of modules.
> +typedef struct {
> + UINT8 Width32;
> + UINT8 Width64;
> + UINT16 Offset32;
> + UINT16 Offset64Lo;
> + UINT16 Offset64Hi;
4. With above structure definition "Offset64Lo/Offset64Hi", I realized that you
define AMD_SMRAM_SAVE_STATE_MAP64 in a way
to split the lower 32bit and high 32bit to two fields. I think it's only
needed when the lower 32bit and higher 32bit are not adjacent.
Otherwise, you can just define "UINT64 _IDTRBase" in AMD_SMRAM_SAVE_STATE_MAP64.
(Unfortunately, GdtBase/LdtBase/IdtBase are split to two 32bit fields not
adjacent to each other in Intel 64bit save state. But AMD
doesn't need to.)
So the recommendation here is to do some simplification to the AMD 64bit save
state definition.
5. Another thought here: Can we remove "AMD_" prefix from the structure name
"AMD_SMRAM_SAVE_STATE_MAP64"?
Because when we define CPUIDs or MSRs, we don't put "INTEL_" or "AMD_" prefix
either.
The definition file is in "MdePkg/Include/Register/Amd" folder which already
indicates it's AMD specific definition.
> +**/
> +UINTN
> +EFIAPI
6. No need for "EFIAPI".
> +SmramSaveStateGetRegisterIndex (
7. Since it's not an API, can it be renamed as "MmSaveStateLibGetRegisterIndex"?
> + IN EFI_SMM_SAVE_STATE_REGISTER Register
> + );
> +
> +EFI_STATUS
> +EFIAPI
> +SmramSaveStateReadRegisterByIndex (
> + IN UINTN CpuIndex,
> + IN UINTN RegisterIndex,
> + IN UINTN Width,
> + OUT VOID *Buffer
> + )
> +{
> + if (RegisterIndex == 0) {
> + return EFI_NOT_FOUND;
> + }
> +
> + if (SmramSaveStateGetRegisterLma () ==
> EFI_SMM_SAVE_STATE_REGISTER_LMA_32BIT) {
> + //
> + // If 32-bit mode width is zero, then the specified register can not be
> accessed
> + //
> + if (mSmmSmramCpuWidthOffset[RegisterIndex].Width32 == 0) {
> + return EFI_NOT_FOUND;
> + }
> +
> + //
> + // If Width is bigger than the 32-bit mode width, then the specified
> register can not be accessed
> + //
> + if (Width > mSmmSmramCpuWidthOffset[RegisterIndex].Width32) {
> + return EFI_INVALID_PARAMETER;
> + }
> +
> + //
> + // Write return buffer
> + //
> + ASSERT (gSmst->CpuSaveState[CpuIndex] != NULL);
> + CopyMem (Buffer, (UINT8 *)gSmst->CpuSaveState[CpuIndex] +
> mSmmSmramCpuWidthOffset[RegisterIndex].Offset32, Width);
> + } else {
> + //
> + // If 64-bit mode width is zero, then the specified register can not be
> accessed
> + //
> + if (mSmmSmramCpuWidthOffset[RegisterIndex].Width64 == 0) {
> + return EFI_NOT_FOUND;
> + }
> +
> + //
> + // If Width is bigger than the 64-bit mode width, then the specified
> register can not be accessed
> + //
> + if (Width > mSmmSmramCpuWidthOffset[RegisterIndex].Width64) {
> + return EFI_INVALID_PARAMETER;
> + }
> +
> + //
> + // Write lower 32-bits of return buffer
> + //
> + CopyMem (Buffer, (UINT8 *)gSmst->CpuSaveState[CpuIndex] +
> mSmmSmramCpuWidthOffset[RegisterIndex].Offset64Lo, MIN (4, Width));
> + if (Width >= 4) {
> + //
> + // Write upper 32-bits of return buffer
> + //
> + CopyMem ((UINT8 *)Buffer + 4, (UINT8 *)gSmst-
> >CpuSaveState[CpuIndex] +
> mSmmSmramCpuWidthOffset[RegisterIndex].Offset64Hi, Width - 4);
> + }
> + }
> +
> + return EFI_SUCCESS;
8. I feel the above logic that reads from 32bit/64bit SMRAM save state can be
written in a simpler way.
But since the logic aligns with the existing one in CpuSmm driver, I am ok that
we
do the simplification in other time.
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#102812): https://edk2.groups.io/g/devel/message/102812
Mute This Topic: https://groups.io/mt/98172963/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe:
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
