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

2023-04-11 Thread Ni, Ray 
> +
> 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] BufferUpon 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  UINTNCpuIndex,
> +  IN  EFI_SMM_SAVE_STATE_REGISTER  Register,
> +  IN  UINTNWidth,
> +  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?



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#102815): https://edk2.groups.io/g/devel/message/102815
Mute This Topic: https://groups.io/mt/98172953/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: 
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy 
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

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

2023-04-10 Thread Abdul Lateef Attar via groups.io 
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4182

Implements SmmSmramSaveStateLib library interfaces
to read and write save state
registers for Intel processor family.

Moves Intel and AMD common functionality to common area.

Cc: Paul Grimes 
Cc: Garrett Kirkendall 
Cc: Abner Chang 
Cc: Eric Dong 
Cc: Ray Ni 
Cc: Rahul Kumar 
Cc: Gerd Hoffmann 
Signed-off-by: Abdul Lateef Attar 
Reviewed-by: Abner Chang 
---
 UefiCpuPkg/UefiCpuPkg.dsc |   4 +
 .../IntelSmmSmramSaveStateLib.inf |  28 ++
 .../SmmSmramSaveStateLib/SmramSaveState.h |   1 -
 .../SmmSmramSaveStateLib/AmdSmramSaveState.c  |  32 --
 .../IntelSmramSaveState.c | 359 ++
 .../SmramSaveStateCommon.c| 116 +-
 6 files changed, 503 insertions(+), 37 deletions(-)
 create mode 100644 
UefiCpuPkg/Library/SmmSmramSaveStateLib/IntelSmmSmramSaveStateLib.inf
 create mode 100644 
UefiCpuPkg/Library/SmmSmramSaveStateLib/IntelSmramSaveState.c

diff --git a/UefiCpuPkg/UefiCpuPkg.dsc b/UefiCpuPkg/UefiCpuPkg.dsc
index 043eb2dbc1b1..df555fdf32de 100644
--- a/UefiCpuPkg/UefiCpuPkg.dsc
+++ b/UefiCpuPkg/UefiCpuPkg.dsc
@@ -102,6 +102,7 @@ [LibraryClasses.common.DXE_SMM_DRIVER]
   HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
   
CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/SmmCpuExceptionHandlerLib.inf
   
SmmSmramSaveStateLib|UefiCpuPkg/Library/SmmSmramSaveStateLib/AmdSmmSmramSaveStateLib.inf
+  
SmmSmramSaveStateLib|UefiCpuPkg/Library/SmmSmramSaveStateLib/IntelSmmSmramSaveStateLib.inf
 
 [LibraryClasses.common.MM_STANDALONE]
   
MmServicesTableLib|MdePkg/Library/StandaloneMmServicesTableLib/StandaloneMmServicesTableLib.inf
@@ -170,6 +171,7 @@ [Components.IA32, Components.X64]
   FILE_GUID = D1D74FE9-7A4E-41D3-A0B3-67F13AD34D94
 
   
SmmCpuFeaturesLib|UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf
+  
SmmSmramSaveStateLib|UefiCpuPkg/Library/SmmSmramSaveStateLib/IntelSmmSmramSaveStateLib.inf
   }
   UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf {
 
@@ -177,6 +179,7 @@ [Components.IA32, Components.X64]
 
   
SmmCpuFeaturesLib|UefiCpuPkg/Library/SmmCpuFeaturesLib/AmdSmmCpuFeaturesLib.inf
   
SmmCpuPlatformHookLib|UefiCpuPkg/Library/SmmCpuPlatformHookLibNull/SmmCpuPlatformHookLibNull.inf
+  
SmmSmramSaveStateLib|UefiCpuPkg/Library/SmmSmramSaveStateLib/AmdSmmSmramSaveStateLib.inf
   }
   UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
   UefiCpuPkg/ResetVector/Vtf0/Bin/ResetVector.inf
@@ -194,6 +197,7 @@ [Components.IA32, Components.X64]
   
UnitTestResultReportLib|UnitTestFrameworkPkg/Library/UnitTestResultReportLib/UnitTestResultReportLibConOut.inf
   }
   UefiCpuPkg/Library/SmmSmramSaveStateLib/AmdSmmSmramSaveStateLib.inf
+  UefiCpuPkg/Library/SmmSmramSaveStateLib/IntelSmmSmramSaveStateLib.inf
   UefiCpuPkg/Library/SmmCpuFeaturesLib/AmdSmmCpuFeaturesLib.inf
 
 [Components.X64]
diff --git 
a/UefiCpuPkg/Library/SmmSmramSaveStateLib/IntelSmmSmramSaveStateLib.inf 
b/UefiCpuPkg/Library/SmmSmramSaveStateLib/IntelSmmSmramSaveStateLib.inf
new file mode 100644
index ..c9d438027b03
--- /dev/null
+++ b/UefiCpuPkg/Library/SmmSmramSaveStateLib/IntelSmmSmramSaveStateLib.inf
@@ -0,0 +1,28 @@
+## @file
+# SMM Smram save state service lib.
+#
+# This is SMM Smram save state service lib that provide service to read and
+# save savestate area registers.
+#
+# Copyright (C) 2023 Advanced Micro Devices, Inc. All rights reserved.
+#
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+##
+
+[Defines]
+  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
+
+[Sources]
+  SmramSaveState.h
+  SmramSaveStateCommon.c
+  IntelSmramSaveState.c
+
+[Packages]
+  MdePkg/MdePkg.dec
+  UefiCpuPkg/UefiCpuPkg.dec
diff --git a/UefiCpuPkg/Library/SmmSmramSaveStateLib/SmramSaveState.h 
b/UefiCpuPkg/Library/SmmSmramSaveStateLib/SmramSaveState.h
index 6c424e3e36e9..55d9d9f127c0 100644
--- a/UefiCpuPkg/Library/SmmSmramSaveStateLib/SmramSaveState.h
+++ b/UefiCpuPkg/Library/SmmSmramSaveStateLib/SmramSaveState.h
@@ -90,7 +90,6 @@ SmramSaveStateGetRegisterIndex (
 
 **/
 EFI_STATUS
-EFIAPI
 SmramSaveStateReadRegisterByIndex (
   IN UINTN  CpuIndex,
   IN UINTN  RegisterIndex,
diff --git a/UefiCpuPkg/Library/SmmSmramSaveStateLib/AmdSmramSaveState.c 
b/UefiCpuPkg/Library/SmmSmramSaveStateLib/AmdSmramSaveState.c
index 8fc4466f473e..e0acd6182320 100644
--- a/UefiCpuPkg/Library/SmmSmramSaveStateLib/AmdSmramSaveState.c
+++ b/UefiCpuPkg/Library/SmmSmramSaveStateLib/AmdSmramSaveState.c
@@ -11,21 +11,11 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 #include 
 #include 
 
-#define EFER_ADDRESS0XC080ul
 #define