[v11] Adds AmdSmmCpuFeaturesLib and MmSaveStateLib

[edk2-devel] [PATCH v11 0/8] Adds AmdSmmCpuFeaturesLib and MmSaveStateLib

Posted by Abdul Lateef Attar via groups.io 12 months ago

PR: https://github.com/tianocore/edk2/pull/4341

V11: Delta changes
Drop the OVMF implementation of MmSaveStateLib
V10: Delta changes:
  Addressed review comments from Abner.
V9: Delta changes:
  Addressed review comments.
  Rename to MmSaveStateLib.
  Also rename SMM_ defines to MM_.
  Implemented OVMF MmSaveStateLib.
  Removes SmmCpuFeaturesReadSaveStateRegister and SmmCpuFeaturesWriteSaveStateRegister
  function interface.
V8 delta changes:
   Addressed review comments from Abner,
   Fix the whitespace error.
   Seperate the Ovmf changes to another patch
V7 delta changes:
   Adds SmmSmramSaveStateLib for Intel processor.
   Integrate SmmSmramSaveStateLib library.
V6 delta changes:
   Addressed review comments for Ray NI.
   removed unnecessary EFIAPI.
V5 delta changes:
   rebase to master branch.
   updated Reviewed-by
V4 delta changes:
  rebase to master branch.
  added reviewed-by.
V3 delta changes:
  Addressed review comments from Abner chang.
  Re-arranged patch order.

Cc: Paul Grimes <paul.grimes@amd.com>
Cc: Abner Chang <abner.chang@amd.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Zhiguang Liu <zhiguang.liu@intel.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Abdul Lateef Attar <abdattar@amd.com>

Abdul Lateef Attar (8):
  MdePkg: Adds AMD SMRAM save state map
  UefiCpuPkg: Adds MmSaveStateLib library class
  UefiCpuPkg: Implements MmSaveStateLib library instance
  UefiCpuPkg/SmmCpuFeaturesLib: Restructure arch-dependent code
  UefiCpuPkg: Implements SmmCpuFeaturesLib for AMD Family
  UefiCpuPkg: Implements MmSaveStateLib for Intel
  UefiCpuPkg: Removes SmmCpuFeaturesReadSaveStateRegister
  OvmfPkg: Uses MmSaveStateLib library

 UefiCpuPkg/UefiCpuPkg.dec                     |   4 +
 OvmfPkg/OvmfPkgIa32.dsc                       |   1 +
 OvmfPkg/OvmfPkgIa32X64.dsc                    |   3 +
 OvmfPkg/OvmfPkgX64.dsc                        |   1 +
 UefiCpuPkg/UefiCpuPkg.dsc                     |  14 +
 .../MmSaveStateLib/AmdMmSaveStateLib.inf      |  28 +
 .../MmSaveStateLib/IntelMmSaveStateLib.inf    |  28 +
 .../AmdSmmCpuFeaturesLib.inf                  |  38 +
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf  |   2 +
 .../Include/Register/Amd/SmramSaveStateMap.h  | 194 +++++
 UefiCpuPkg/Include/Library/MmSaveStateLib.h   |  70 ++
 .../Include/Library/SmmCpuFeaturesLib.h       |  52 --
 .../Library/MmSaveStateLib/MmSaveState.h      | 102 +++
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h    |  56 +-
 .../SmmCpuFeaturesLib/SmmCpuFeaturesLib.c     | 767 ------------------
 .../Library/MmSaveStateLib/AmdMmSaveState.c   | 309 +++++++
 .../Library/MmSaveStateLib/IntelMmSaveState.c | 413 ++++++++++
 .../MmSaveStateLib/MmSaveStateCommon.c        | 138 ++++
 .../SmmCpuFeaturesLib/AmdSmmCpuFeaturesLib.c  | 387 +++++++++
 .../IntelSmmCpuFeaturesLib.c                  |  70 ++
 .../SmmCpuFeaturesLibCommon.c                 | 128 ---
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c    |  11 +-
 UefiCpuPkg/PiSmmCpuDxeSmm/SmramSaveState.c    | 500 +-----------
 MdePkg/MdePkg.ci.yaml                         |   4 +-
 24 files changed, 1812 insertions(+), 1508 deletions(-)
 create mode 100644 UefiCpuPkg/Library/MmSaveStateLib/AmdMmSaveStateLib.inf
 create mode 100644 UefiCpuPkg/Library/MmSaveStateLib/IntelMmSaveStateLib.inf
 create mode 100644 UefiCpuPkg/Library/SmmCpuFeaturesLib/AmdSmmCpuFeaturesLib.inf
 create mode 100644 MdePkg/Include/Register/Amd/SmramSaveStateMap.h
 create mode 100644 UefiCpuPkg/Include/Library/MmSaveStateLib.h
 create mode 100644 UefiCpuPkg/Library/MmSaveStateLib/MmSaveState.h
 create mode 100644 UefiCpuPkg/Library/MmSaveStateLib/AmdMmSaveState.c
 create mode 100644 UefiCpuPkg/Library/MmSaveStateLib/IntelMmSaveState.c
 create mode 100644 UefiCpuPkg/Library/MmSaveStateLib/MmSaveStateCommon.c
 create mode 100644 UefiCpuPkg/Library/SmmCpuFeaturesLib/AmdSmmCpuFeaturesLib.c

-- 
2.25.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#104172): https://edk2.groups.io/g/devel/message/104172
Mute This Topic: https://groups.io/mt/98720163/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v11 0/8] Adds AmdSmmCpuFeaturesLib and MmSaveStateLib

Posted by Ni, Ray 11 months, 3 weeks ago

>   UefiCpuPkg: Adds MmSaveStateLib library class
You can take "Reviewed-by: Ray Ni <ray.ni@intel.com>" for this patch.

> UefiCpuPkg/SmmCpuFeaturesLib: Restructure arch-dependent code
You can take "Reviewed-by: Ray Ni <ray.ni@intel.com>" for this patch.

> UefiCpuPkg: Implements SmmCpuFeaturesLib for AMD Family
One comment: can you please change the BASE_NAME in lib INF to "AmdSmmCpuFeaturesLib"?
The BASE_NAME guides tool to generate the .Lib file in the disk.
Choosing different BASE_NAME for Intel and AMD lib instance can avoid one LIB file is replaced by the other during pkg build.

> UefiCpuPkg: Implements MmSaveStateLib for Intel
1. MmSaveStateLib local functions should not have "EFIAPI". Please only add "EFIAPI" for lib APIs.
2. MmSaveStateGetRegisterLma() doesn't need to carry "CpuIndex" as parameter. Can you please remove the parameter?
3. MmSaveStateGetRegisterIndex () returns wrong value for Intel processors because it uses Offset ( = 2) but Intel implementation uses Offset ( = 4).

(I remember comments #1, #3 were both raised last time when I reviewed the patch.)

> UefiCpuPkg: Removes SmmCpuFeaturesReadSaveStateRegister
You can take "Reviewed-by: Ray Ni <ray.ni@intel.com>" for this patch.


One more comment:
Can you please avoid FILE_GUID overridden in UefiCpuPkg.dsc for Intel instance of SmmCpu driver?
AMD instance can override the FILE_GUID for sure.

@Wu, Jiaxin, we will need to change the close-source SmmCpuFeatureLib accordingly (separating the SaveState access code into a different lib) when this patch series are merged.

> -----Original Message-----
> From: Abdul Lateef Attar <abdattar@amd.com>
> Sent: Saturday, May 6, 2023 12:07 PM
> To: devel@edk2.groups.io
> Cc: Abdul Lateef Attar <abdattar@amd.com>; Paul Grimes
> <paul.grimes@amd.com>; Abner Chang <abner.chang@amd.com>; Dong, Eric
> <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Kumar, Rahul R
> <rahul.r.kumar@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; Kinney,
> Michael D <michael.d.kinney@intel.com>; Gao, Liming
> <gaoliming@byosoft.com.cn>; Liu, Zhiguang <zhiguang.liu@intel.com>; Ard
> Biesheuvel <ardb+tianocore@kernel.org>; Yao, Jiewen <jiewen.yao@intel.com>;
> Justen, Jordan L <jordan.l.justen@intel.com>
> Subject: [PATCH v11 0/8] Adds AmdSmmCpuFeaturesLib and MmSaveStateLib
> 
> PR: https://github.com/tianocore/edk2/pull/4341
> 
> V11: Delta changes
> Drop the OVMF implementation of MmSaveStateLib
> V10: Delta changes:
>   Addressed review comments from Abner.
> V9: Delta changes:
>   Addressed review comments.
>   Rename to MmSaveStateLib.
>   Also rename SMM_ defines to MM_.
>   Implemented OVMF MmSaveStateLib.
>   Removes SmmCpuFeaturesReadSaveStateRegister and
> SmmCpuFeaturesWriteSaveStateRegister
>   function interface.
> V8 delta changes:
>    Addressed review comments from Abner,
>    Fix the whitespace error.
>    Seperate the Ovmf changes to another patch
> V7 delta changes:
>    Adds SmmSmramSaveStateLib for Intel processor.
>    Integrate SmmSmramSaveStateLib library.
> V6 delta changes:
>    Addressed review comments for Ray NI.
>    removed unnecessary EFIAPI.
> V5 delta changes:
>    rebase to master branch.
>    updated Reviewed-by
> V4 delta changes:
>   rebase to master branch.
>   added reviewed-by.
> V3 delta changes:
>   Addressed review comments from Abner chang.
>   Re-arranged patch order.
> 
> Cc: Paul Grimes <paul.grimes@amd.com>
> Cc: Abner Chang <abner.chang@amd.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Zhiguang Liu <zhiguang.liu@intel.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Abdul Lateef Attar <abdattar@amd.com>
> 
> Abdul Lateef Attar (8):
>   MdePkg: Adds AMD SMRAM save state map
>   UefiCpuPkg: Adds MmSaveStateLib library class
>   UefiCpuPkg: Implements MmSaveStateLib library instance
>   UefiCpuPkg/SmmCpuFeaturesLib: Restructure arch-dependent code
>   UefiCpuPkg: Implements SmmCpuFeaturesLib for AMD Family
>   UefiCpuPkg: Implements MmSaveStateLib for Intel
>   UefiCpuPkg: Removes SmmCpuFeaturesReadSaveStateRegister
>   OvmfPkg: Uses MmSaveStateLib library
> 
>  UefiCpuPkg/UefiCpuPkg.dec                     |   4 +
>  OvmfPkg/OvmfPkgIa32.dsc                       |   1 +
>  OvmfPkg/OvmfPkgIa32X64.dsc                    |   3 +
>  OvmfPkg/OvmfPkgX64.dsc                        |   1 +
>  UefiCpuPkg/UefiCpuPkg.dsc                     |  14 +
>  .../MmSaveStateLib/AmdMmSaveStateLib.inf      |  28 +
>  .../MmSaveStateLib/IntelMmSaveStateLib.inf    |  28 +
>  .../AmdSmmCpuFeaturesLib.inf                  |  38 +
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf  |   2 +
>  .../Include/Register/Amd/SmramSaveStateMap.h  | 194 +++++
>  UefiCpuPkg/Include/Library/MmSaveStateLib.h   |  70 ++
>  .../Include/Library/SmmCpuFeaturesLib.h       |  52 --
>  .../Library/MmSaveStateLib/MmSaveState.h      | 102 +++
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h    |  56 +-
>  .../SmmCpuFeaturesLib/SmmCpuFeaturesLib.c     | 767 ------------------
>  .../Library/MmSaveStateLib/AmdMmSaveState.c   | 309 +++++++
>  .../Library/MmSaveStateLib/IntelMmSaveState.c | 413 ++++++++++
>  .../MmSaveStateLib/MmSaveStateCommon.c        | 138 ++++
>  .../SmmCpuFeaturesLib/AmdSmmCpuFeaturesLib.c  | 387 +++++++++
>  .../IntelSmmCpuFeaturesLib.c                  |  70 ++
>  .../SmmCpuFeaturesLibCommon.c                 | 128 ---
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c    |  11 +-
>  UefiCpuPkg/PiSmmCpuDxeSmm/SmramSaveState.c    | 500 +-----------
>  MdePkg/MdePkg.ci.yaml                         |   4 +-
>  24 files changed, 1812 insertions(+), 1508 deletions(-)
>  create mode 100644
> UefiCpuPkg/Library/MmSaveStateLib/AmdMmSaveStateLib.inf
>  create mode 100644
> UefiCpuPkg/Library/MmSaveStateLib/IntelMmSaveStateLib.inf
>  create mode 100644
> UefiCpuPkg/Library/SmmCpuFeaturesLib/AmdSmmCpuFeaturesLib.inf
>  create mode 100644 MdePkg/Include/Register/Amd/SmramSaveStateMap.h
>  create mode 100644 UefiCpuPkg/Include/Library/MmSaveStateLib.h
>  create mode 100644 UefiCpuPkg/Library/MmSaveStateLib/MmSaveState.h
>  create mode 100644 UefiCpuPkg/Library/MmSaveStateLib/AmdMmSaveState.c
>  create mode 100644 UefiCpuPkg/Library/MmSaveStateLib/IntelMmSaveState.c
>  create mode 100644
> UefiCpuPkg/Library/MmSaveStateLib/MmSaveStateCommon.c
>  create mode 100644
> UefiCpuPkg/Library/SmmCpuFeaturesLib/AmdSmmCpuFeaturesLib.c
> 
> --
> 2.25.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#104669): https://edk2.groups.io/g/devel/message/104669
Mute This Topic: https://groups.io/mt/98720163/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v11 0/8] Adds AmdSmmCpuFeaturesLib and MmSaveStateLib

Posted by Michael Kubacki 11 months, 3 weeks ago

I haven't followed the whole thread but looking at v11, I wanted to 
check on the following.

1. The library source files (for AmdMmSaveStateLib and 
IntelMmSaveStateLib) include several library headers (particularly in 
MmSaveStateLib/MmSaveState.h) but do not have a [LibraryClasses] section 
in their INF files. Is there a reason?

2. AmdMmSaveStateLib and IntelMmSaveStateLib depend on 
SmmServicesTableLib. Can they depend on MmServicesTableLib instead?

3. It seems resources like the EFER register LMA bit should be defined 
somewhere agnostic to the MmSaveState.h file in MmSaveStateLib. It is 
referenced in MdePkg/Include/Register/Intel/ArchitecturalMsr.h:

https://github.com/tianocore/edk2/blob/5215cd5baf6609e54050c69909273b7f5161c59e/MdePkg/Include/Register/Intel/ArchitecturalMsr.h#L6342

In any case, can the bit be defined in one location?

4. Your change to the SmmCpuFeaturesLib.h interface is not backward 
compatible and a build breaking change. I think that should be called 
out in your cover letter.

Thanks,
Michael

On 5/6/2023 12:06 AM, Abdul Lateef Attar via groups.io wrote:
> PR: https://github.com/tianocore/edk2/pull/4341
> 
> V11: Delta changes
> Drop the OVMF implementation of MmSaveStateLib
> V10: Delta changes:
>    Addressed review comments from Abner.
> V9: Delta changes:
>    Addressed review comments.
>    Rename to MmSaveStateLib.
>    Also rename SMM_ defines to MM_.
>    Implemented OVMF MmSaveStateLib.
>    Removes SmmCpuFeaturesReadSaveStateRegister and SmmCpuFeaturesWriteSaveStateRegister
>    function interface.
> V8 delta changes:
>     Addressed review comments from Abner,
>     Fix the whitespace error.
>     Seperate the Ovmf changes to another patch
> V7 delta changes:
>     Adds SmmSmramSaveStateLib for Intel processor.
>     Integrate SmmSmramSaveStateLib library.
> V6 delta changes:
>     Addressed review comments for Ray NI.
>     removed unnecessary EFIAPI.
> V5 delta changes:
>     rebase to master branch.
>     updated Reviewed-by
> V4 delta changes:
>    rebase to master branch.
>    added reviewed-by.
> V3 delta changes:
>    Addressed review comments from Abner chang.
>    Re-arranged patch order.
> 
> Cc: Paul Grimes <paul.grimes@amd.com>
> Cc: Abner Chang <abner.chang@amd.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Zhiguang Liu <zhiguang.liu@intel.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Abdul Lateef Attar <abdattar@amd.com>
> 
> Abdul Lateef Attar (8):
>    MdePkg: Adds AMD SMRAM save state map
>    UefiCpuPkg: Adds MmSaveStateLib library class
>    UefiCpuPkg: Implements MmSaveStateLib library instance
>    UefiCpuPkg/SmmCpuFeaturesLib: Restructure arch-dependent code
>    UefiCpuPkg: Implements SmmCpuFeaturesLib for AMD Family
>    UefiCpuPkg: Implements MmSaveStateLib for Intel
>    UefiCpuPkg: Removes SmmCpuFeaturesReadSaveStateRegister
>    OvmfPkg: Uses MmSaveStateLib library
> 
>   UefiCpuPkg/UefiCpuPkg.dec                     |   4 +
>   OvmfPkg/OvmfPkgIa32.dsc                       |   1 +
>   OvmfPkg/OvmfPkgIa32X64.dsc                    |   3 +
>   OvmfPkg/OvmfPkgX64.dsc                        |   1 +
>   UefiCpuPkg/UefiCpuPkg.dsc                     |  14 +
>   .../MmSaveStateLib/AmdMmSaveStateLib.inf      |  28 +
>   .../MmSaveStateLib/IntelMmSaveStateLib.inf    |  28 +
>   .../AmdSmmCpuFeaturesLib.inf                  |  38 +
>   UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf  |   2 +
>   .../Include/Register/Amd/SmramSaveStateMap.h  | 194 +++++
>   UefiCpuPkg/Include/Library/MmSaveStateLib.h   |  70 ++
>   .../Include/Library/SmmCpuFeaturesLib.h       |  52 --
>   .../Library/MmSaveStateLib/MmSaveState.h      | 102 +++
>   UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h    |  56 +-
>   .../SmmCpuFeaturesLib/SmmCpuFeaturesLib.c     | 767 ------------------
>   .../Library/MmSaveStateLib/AmdMmSaveState.c   | 309 +++++++
>   .../Library/MmSaveStateLib/IntelMmSaveState.c | 413 ++++++++++
>   .../MmSaveStateLib/MmSaveStateCommon.c        | 138 ++++
>   .../SmmCpuFeaturesLib/AmdSmmCpuFeaturesLib.c  | 387 +++++++++
>   .../IntelSmmCpuFeaturesLib.c                  |  70 ++
>   .../SmmCpuFeaturesLibCommon.c                 | 128 ---
>   UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c    |  11 +-
>   UefiCpuPkg/PiSmmCpuDxeSmm/SmramSaveState.c    | 500 +-----------
>   MdePkg/MdePkg.ci.yaml                         |   4 +-
>   24 files changed, 1812 insertions(+), 1508 deletions(-)
>   create mode 100644 UefiCpuPkg/Library/MmSaveStateLib/AmdMmSaveStateLib.inf
>   create mode 100644 UefiCpuPkg/Library/MmSaveStateLib/IntelMmSaveStateLib.inf
>   create mode 100644 UefiCpuPkg/Library/SmmCpuFeaturesLib/AmdSmmCpuFeaturesLib.inf
>   create mode 100644 MdePkg/Include/Register/Amd/SmramSaveStateMap.h
>   create mode 100644 UefiCpuPkg/Include/Library/MmSaveStateLib.h
>   create mode 100644 UefiCpuPkg/Library/MmSaveStateLib/MmSaveState.h
>   create mode 100644 UefiCpuPkg/Library/MmSaveStateLib/AmdMmSaveState.c
>   create mode 100644 UefiCpuPkg/Library/MmSaveStateLib/IntelMmSaveState.c
>   create mode 100644 UefiCpuPkg/Library/MmSaveStateLib/MmSaveStateCommon.c
>   create mode 100644 UefiCpuPkg/Library/SmmCpuFeaturesLib/AmdSmmCpuFeaturesLib.c
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#104427): https://edk2.groups.io/g/devel/message/104427
Mute This Topic: https://groups.io/mt/98720163/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v11 0/8] Adds AmdSmmCpuFeaturesLib and MmSaveStateLib

Posted by Attar, AbdulLateef (Abdul Lateef) via groups.io 11 months, 3 weeks ago

[AMD Official Use Only - General]

Hi Michael,
        Thanks for providing the inputs, I will make the necessary changes. Please see inline for my reply.

-----Original Message-----
From: Michael Kubacki <mikuback@linux.microsoft.com>
Sent: 10 May 2023 00:42
To: devel@edk2.groups.io; Attar, AbdulLateef (Abdul Lateef) <AbdulLateef.Attar@amd.com>
Cc: Grimes, Paul <Paul.Grimes@amd.com>; Chang, Abner <Abner.Chang@amd.com>; Eric Dong <eric.dong@intel.com>; Ray Ni <ray.ni@intel.com>; Rahul Kumar <rahul1.kumar@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; Michael D Kinney <michael.d.kinney@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>; Zhiguang Liu <zhiguang.liu@intel.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>; Jiewen Yao <jiewen.yao@intel.com>; Jordan Justen <jordan.l.justen@intel.com>
Subject: Re: [edk2-devel] [PATCH v11 0/8] Adds AmdSmmCpuFeaturesLib and MmSaveStateLib

Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.


I haven't followed the whole thread but looking at v11, I wanted to check on the following.

1. The library source files (for AmdMmSaveStateLib and
IntelMmSaveStateLib) include several library headers (particularly in
MmSaveStateLib/MmSaveState.h) but do not have a [LibraryClasses] section in their INF files. Is there a reason?
[Attar, AbdulLateef (Abdul Lateef)]  I will make changes and submit V12 version.

2. AmdMmSaveStateLib and IntelMmSaveStateLib depend on SmmServicesTableLib. Can they depend on MmServicesTableLib instead?
[Attar, AbdulLateef (Abdul Lateef)] MmSaveStateLib is mainly used by PiSmmCpuDxeSmm driver which still uses Smm convention and preserve the SAVE_STATE pointed by gSmst(Instead of gMmst).
Hence I don’t think we can move to MmServicesTableLib.

3. It seems resources like the EFER register LMA bit should be defined somewhere agnostic to the MmSaveState.h file in MmSaveStateLib. It is referenced in MdePkg/Include/Register/Intel/ArchitecturalMsr.h:

https://github.com/tianocore/edk2/blob/5215cd5baf6609e54050c69909273b7f5161c59e/MdePkg/Include/Register/Intel/ArchitecturalMsr.h#L6342

In any case, can the bit be defined in one location?
[Attar, AbdulLateef (Abdul Lateef)] This AMD specific implementation, I'll move the MACRO definition under the AmdMmSaveStateLib.c file.

4. Your change to the SmmCpuFeaturesLib.h interface is not backward compatible and a build breaking change. I think that should be called out in your cover letter.
[Attar, AbdulLateef (Abdul Lateef)] I will update the cover letter in V12 version.

Thanks,
Michael

On 5/6/2023 12:06 AM, Abdul Lateef Attar via groups.io wrote:
> PR: https://github.com/tianocore/edk2/pull/4341
>
> V11: Delta changes
> Drop the OVMF implementation of MmSaveStateLib
> V10: Delta changes:
>    Addressed review comments from Abner.
> V9: Delta changes:
>    Addressed review comments.
>    Rename to MmSaveStateLib.
>    Also rename SMM_ defines to MM_.
>    Implemented OVMF MmSaveStateLib.
>    Removes SmmCpuFeaturesReadSaveStateRegister and SmmCpuFeaturesWriteSaveStateRegister
>    function interface.
> V8 delta changes:
>     Addressed review comments from Abner,
>     Fix the whitespace error.
>     Seperate the Ovmf changes to another patch
> V7 delta changes:
>     Adds SmmSmramSaveStateLib for Intel processor.
>     Integrate SmmSmramSaveStateLib library.
> V6 delta changes:
>     Addressed review comments for Ray NI.
>     removed unnecessary EFIAPI.
> V5 delta changes:
>     rebase to master branch.
>     updated Reviewed-by
> V4 delta changes:
>    rebase to master branch.
>    added reviewed-by.
> V3 delta changes:
>    Addressed review comments from Abner chang.
>    Re-arranged patch order.
>
> Cc: Paul Grimes <paul.grimes@amd.com>
> Cc: Abner Chang <abner.chang@amd.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Zhiguang Liu <zhiguang.liu@intel.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Abdul Lateef Attar <abdattar@amd.com>
>
> Abdul Lateef Attar (8):
>    MdePkg: Adds AMD SMRAM save state map
>    UefiCpuPkg: Adds MmSaveStateLib library class
>    UefiCpuPkg: Implements MmSaveStateLib library instance
>    UefiCpuPkg/SmmCpuFeaturesLib: Restructure arch-dependent code
>    UefiCpuPkg: Implements SmmCpuFeaturesLib for AMD Family
>    UefiCpuPkg: Implements MmSaveStateLib for Intel
>    UefiCpuPkg: Removes SmmCpuFeaturesReadSaveStateRegister
>    OvmfPkg: Uses MmSaveStateLib library
>
>   UefiCpuPkg/UefiCpuPkg.dec                     |   4 +
>   OvmfPkg/OvmfPkgIa32.dsc                       |   1 +
>   OvmfPkg/OvmfPkgIa32X64.dsc                    |   3 +
>   OvmfPkg/OvmfPkgX64.dsc                        |   1 +
>   UefiCpuPkg/UefiCpuPkg.dsc                     |  14 +
>   .../MmSaveStateLib/AmdMmSaveStateLib.inf      |  28 +
>   .../MmSaveStateLib/IntelMmSaveStateLib.inf    |  28 +
>   .../AmdSmmCpuFeaturesLib.inf                  |  38 +
>   UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf  |   2 +
>   .../Include/Register/Amd/SmramSaveStateMap.h  | 194 +++++
>   UefiCpuPkg/Include/Library/MmSaveStateLib.h   |  70 ++
>   .../Include/Library/SmmCpuFeaturesLib.h       |  52 --
>   .../Library/MmSaveStateLib/MmSaveState.h      | 102 +++
>   UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h    |  56 +-
>   .../SmmCpuFeaturesLib/SmmCpuFeaturesLib.c     | 767 ------------------
>   .../Library/MmSaveStateLib/AmdMmSaveState.c   | 309 +++++++
>   .../Library/MmSaveStateLib/IntelMmSaveState.c | 413 ++++++++++
>   .../MmSaveStateLib/MmSaveStateCommon.c        | 138 ++++
>   .../SmmCpuFeaturesLib/AmdSmmCpuFeaturesLib.c  | 387 +++++++++
>   .../IntelSmmCpuFeaturesLib.c                  |  70 ++
>   .../SmmCpuFeaturesLibCommon.c                 | 128 ---
>   UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c    |  11 +-
>   UefiCpuPkg/PiSmmCpuDxeSmm/SmramSaveState.c    | 500 +-----------
>   MdePkg/MdePkg.ci.yaml                         |   4 +-
>   24 files changed, 1812 insertions(+), 1508 deletions(-)
>   create mode 100644 UefiCpuPkg/Library/MmSaveStateLib/AmdMmSaveStateLib.inf
>   create mode 100644 UefiCpuPkg/Library/MmSaveStateLib/IntelMmSaveStateLib.inf
>   create mode 100644 UefiCpuPkg/Library/SmmCpuFeaturesLib/AmdSmmCpuFeaturesLib.inf
>   create mode 100644 MdePkg/Include/Register/Amd/SmramSaveStateMap.h
>   create mode 100644 UefiCpuPkg/Include/Library/MmSaveStateLib.h
>   create mode 100644 UefiCpuPkg/Library/MmSaveStateLib/MmSaveState.h
>   create mode 100644 UefiCpuPkg/Library/MmSaveStateLib/AmdMmSaveState.c
>   create mode 100644 UefiCpuPkg/Library/MmSaveStateLib/IntelMmSaveState.c
>   create mode 100644 UefiCpuPkg/Library/MmSaveStateLib/MmSaveStateCommon.c
>   create mode 100644
> UefiCpuPkg/Library/SmmCpuFeaturesLib/AmdSmmCpuFeaturesLib.c
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#104709): https://edk2.groups.io/g/devel/message/104709
Mute This Topic: https://groups.io/mt/98720163/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v11 0/8] Adds AmdSmmCpuFeaturesLib and MmSaveStateLib

Posted by Gerd Hoffmann 11 months, 3 weeks ago

On Sat, May 06, 2023 at 09:36:56AM +0530, Abdul Lateef Attar wrote:
> PR: https://github.com/tianocore/edk2/pull/4341
> 
> V11: Delta changes
> Drop the OVMF implementation of MmSaveStateLib

Tested-by: Gerd Hoffmann <kraxel@redhat.com>
Acked-by: Gerd Hoffmann <kraxel@redhat.com>

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#104330): https://edk2.groups.io/g/devel/message/104330
Mute This Topic: https://groups.io/mt/98720163/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v11 0/8] Adds AmdSmmCpuFeaturesLib and MmSaveStateLib

Posted by Chang, Abner via groups.io 11 months, 3 weeks ago

[AMD Official Use Only - General]

Hi @Ray Ni,
Could you please help to review this patch set as the Hard Freeze is coming soon?

Thanks
Abner

> -----Original Message-----
> From: Abdul Lateef Attar <abdattar@amd.com>
> Sent: Saturday, May 6, 2023 12:07 PM
> To: devel@edk2.groups.io
> Cc: Attar, AbdulLateef (Abdul Lateef) <AbdulLateef.Attar@amd.com>;
> Grimes, Paul <Paul.Grimes@amd.com>; Chang, Abner
> <Abner.Chang@amd.com>; Eric Dong <eric.dong@intel.com>; Ray Ni
> <ray.ni@intel.com>; Rahul Kumar <rahul1.kumar@intel.com>; Gerd
> Hoffmann <kraxel@redhat.com>; Michael D Kinney
> <michael.d.kinney@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>;
> Zhiguang Liu <zhiguang.liu@intel.com>; Ard Biesheuvel
> <ardb+tianocore@kernel.org>; Jiewen Yao <jiewen.yao@intel.com>; Jordan
> Justen <jordan.l.justen@intel.com>
> Subject: [PATCH v11 0/8] Adds AmdSmmCpuFeaturesLib and
> MmSaveStateLib
> 
> PR: https://github.com/tianocore/edk2/pull/4341
> 
> V11: Delta changes
> Drop the OVMF implementation of MmSaveStateLib
> V10: Delta changes:
>   Addressed review comments from Abner.
> V9: Delta changes:
>   Addressed review comments.
>   Rename to MmSaveStateLib.
>   Also rename SMM_ defines to MM_.
>   Implemented OVMF MmSaveStateLib.
>   Removes SmmCpuFeaturesReadSaveStateRegister and
> SmmCpuFeaturesWriteSaveStateRegister
>   function interface.
> V8 delta changes:
>    Addressed review comments from Abner,
>    Fix the whitespace error.
>    Seperate the Ovmf changes to another patch
> V7 delta changes:
>    Adds SmmSmramSaveStateLib for Intel processor.
>    Integrate SmmSmramSaveStateLib library.
> V6 delta changes:
>    Addressed review comments for Ray NI.
>    removed unnecessary EFIAPI.
> V5 delta changes:
>    rebase to master branch.
>    updated Reviewed-by
> V4 delta changes:
>   rebase to master branch.
>   added reviewed-by.
> V3 delta changes:
>   Addressed review comments from Abner chang.
>   Re-arranged patch order.
> 
> Cc: Paul Grimes <paul.grimes@amd.com>
> Cc: Abner Chang <abner.chang@amd.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Zhiguang Liu <zhiguang.liu@intel.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Abdul Lateef Attar <abdattar@amd.com>
> 
> Abdul Lateef Attar (8):
>   MdePkg: Adds AMD SMRAM save state map
>   UefiCpuPkg: Adds MmSaveStateLib library class
>   UefiCpuPkg: Implements MmSaveStateLib library instance
>   UefiCpuPkg/SmmCpuFeaturesLib: Restructure arch-dependent code
>   UefiCpuPkg: Implements SmmCpuFeaturesLib for AMD Family
>   UefiCpuPkg: Implements MmSaveStateLib for Intel
>   UefiCpuPkg: Removes SmmCpuFeaturesReadSaveStateRegister
>   OvmfPkg: Uses MmSaveStateLib library
> 
>  UefiCpuPkg/UefiCpuPkg.dec                     |   4 +
>  OvmfPkg/OvmfPkgIa32.dsc                       |   1 +
>  OvmfPkg/OvmfPkgIa32X64.dsc                    |   3 +
>  OvmfPkg/OvmfPkgX64.dsc                        |   1 +
>  UefiCpuPkg/UefiCpuPkg.dsc                     |  14 +
>  .../MmSaveStateLib/AmdMmSaveStateLib.inf      |  28 +
>  .../MmSaveStateLib/IntelMmSaveStateLib.inf    |  28 +
>  .../AmdSmmCpuFeaturesLib.inf                  |  38 +
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf  |   2 +
>  .../Include/Register/Amd/SmramSaveStateMap.h  | 194 +++++
>  UefiCpuPkg/Include/Library/MmSaveStateLib.h   |  70 ++
>  .../Include/Library/SmmCpuFeaturesLib.h       |  52 --
>  .../Library/MmSaveStateLib/MmSaveState.h      | 102 +++
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h    |  56 +-
>  .../SmmCpuFeaturesLib/SmmCpuFeaturesLib.c     | 767 ------------------
>  .../Library/MmSaveStateLib/AmdMmSaveState.c   | 309 +++++++
>  .../Library/MmSaveStateLib/IntelMmSaveState.c | 413 ++++++++++
>  .../MmSaveStateLib/MmSaveStateCommon.c        | 138 ++++
>  .../SmmCpuFeaturesLib/AmdSmmCpuFeaturesLib.c  | 387 +++++++++
>  .../IntelSmmCpuFeaturesLib.c                  |  70 ++
>  .../SmmCpuFeaturesLibCommon.c                 | 128 ---
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c    |  11 +-
>  UefiCpuPkg/PiSmmCpuDxeSmm/SmramSaveState.c    | 500 +-----------
>  MdePkg/MdePkg.ci.yaml                         |   4 +-
>  24 files changed, 1812 insertions(+), 1508 deletions(-)  create mode 100644
> UefiCpuPkg/Library/MmSaveStateLib/AmdMmSaveStateLib.inf
>  create mode 100644
> UefiCpuPkg/Library/MmSaveStateLib/IntelMmSaveStateLib.inf
>  create mode 100644
> UefiCpuPkg/Library/SmmCpuFeaturesLib/AmdSmmCpuFeaturesLib.inf
>  create mode 100644
> MdePkg/Include/Register/Amd/SmramSaveStateMap.h
>  create mode 100644 UefiCpuPkg/Include/Library/MmSaveStateLib.h
>  create mode 100644 UefiCpuPkg/Library/MmSaveStateLib/MmSaveState.h
>  create mode 100644
> UefiCpuPkg/Library/MmSaveStateLib/AmdMmSaveState.c
>  create mode 100644
> UefiCpuPkg/Library/MmSaveStateLib/IntelMmSaveState.c
>  create mode 100644
> UefiCpuPkg/Library/MmSaveStateLib/MmSaveStateCommon.c
>  create mode 100644
> UefiCpuPkg/Library/SmmCpuFeaturesLib/AmdSmmCpuFeaturesLib.c
> 
> --
> 2.25.1


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#104329): https://edk2.groups.io/g/devel/message/104329
Mute This Topic: https://groups.io/mt/98720163/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-