[edk2-devel] [PATCH v8 3/9] UefiCpuPkg: Implements SmmSmramSaveStateLib library class

[Ni, Ray]

> +
> 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/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3943202/1813853/130120423/xyzzy [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-