Re: [edk2-devel] [PATCH v8 7/9] UefiCpuPkg: Implements SmmSmramSaveStateLib for Intel

["Ni, Ray" <ray.ni@intel.com>]

> +
> SmmSmramSaveStateLib|UefiCpuPkg/Library/SmmSmramSaveStateLib/Intel
> SmmSmramSaveStateLib.inf

1. Can you rename it to "IntelMmSaveStateLib"?


> +  INF_VERSION                    = 1.29
> +  BASE_NAME                      = IntelSmmSmramSaveStateLib
> +  FILE_GUID                      = 37E8137B-9F74-4250-8951-7A970A3C39C0
> +  MODULE_TYPE                    = DXE_SMM_DRIVER
> +  VERSION_STRING                 = 1.0
> +  LIBRARY_CLASS                  = SmmSmramSaveStateLib

2. Can you check back the comments I left for AMD instance and apply similar changes here?

>  **/
>  EFI_STATUS
> -EFIAPI

3. Why remove "EFIAPI"?


> +/**
> +  Read an SMM Save State register on the target processor.  If this function
> +  returns EFI_UNSUPPORTED, then the caller is responsible for reading the
> +  SMM Save Sate register.
> +
> +  @param[in]  CpuIndex  The index of the CPU to read the SMM Save State.
> The
> +                        value must be between 0 and the NumberOfCpus field in
> +                        the System Management System Table (SMST).
> +  @param[in]  Register  The SMM Save State register to read.
> +  @param[in]  Width     The number of bytes to read from the CPU save
> state.
> +  @param[out] Buffer    Upon return, this holds the CPU register value read
> +                        from the save state.
> +
> +  @retval EFI_SUCCESS           The register was read from Save State.
> +  @retval EFI_INVALID_PARAMTER  Buffer is NULL.
> +  @retval EFI_UNSUPPORTED       This function does not support reading
> Register.
> +  @retval EFI_NOT_FOUND         If desired Register not found.
> +**/
> +EFI_STATUS
> +EFIAPI
> +SmramSaveStateReadRegister (
> +  IN  UINTN                        CpuIndex,
> +  IN  EFI_SMM_SAVE_STATE_REGISTER  Register,
> +  IN  UINTN                        Width,
> +  OUT VOID                         *Buffer
> +  )
> +{
> +  UINT32                      SmmRevId;
> +  SMRAM_SAVE_STATE_IOMISC     IoMisc;
> +  EFI_SMM_SAVE_STATE_IO_INFO  *IoInfo;
> +
> +  //
> +  // Check for special EFI_SMM_SAVE_STATE_REGISTER_LMA
> +  //
> +  if (Register == EFI_SMM_SAVE_STATE_REGISTER_LMA) {
> +    //
> +    // Only byte access is supported for this register
> +    //
> +    if (Width != 1) {
> +      return EFI_INVALID_PARAMETER;
> +    }
> +
> +    *(UINT8 *)Buffer = SmramSaveStateGetRegisterLma ();

4. It's Intel specific flow. I am curious how AMD flow handles the LMA read.

> +
> +    return EFI_SUCCESS;
> +  }
> +
> +  //
> +  // Check for special EFI_SMM_SAVE_STATE_REGISTER_IO
> +  //
> +  if (Register == EFI_SMM_SAVE_STATE_REGISTER_IO) {

5. Similar question here: how AMD flow handles the IO read?


> +
> +  //
> +  // Convert Register to a register lookup table index
> +  //
> +  return SmramSaveStateReadRegisterByIndex (CpuIndex,
> SmramSaveStateGetRegisterIndex (Register), Width, Buffer);

6. Can you double check here? The mSmmSmramCpuWidthOffset[] of Intel/AMD version don't put the GdtBase in the same location.
SMM_SAVE_STATE_REGISTER_MAX_INDEX is 2 for AMD but should be 4 for Intel.
How about define a "CONST UINTN gSmmSaveStateRegisterMaxIndex" with different values in Intel/AmdSmramSaveState.c?



>  EFI_STATUS
> -EFIAPI

7. Why remove "EFIAPI"?

> +UINT8
> +IntelSmramSaveStateGetRegisterLma (

8. Can you implement the SmramSaveStateGetRegisterLma() in Intel/Amd C files?