[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 <mikub...@linux.microsoft.com>
Sent: 10 May 2023 00:42
To: devel@edk2.groups.io; Attar, AbdulLateef (Abdul Lateef)
<abdullateef.at...@amd.com>
Cc: Grimes, Paul <paul.gri...@amd.com>; Chang, Abner <abner.ch...@amd.com>;
Eric Dong <eric.d...@intel.com>; Ray Ni <ray...@intel.com>; Rahul Kumar
<rahul1.ku...@intel.com>; Gerd Hoffmann <kra...@redhat.com>; Michael D Kinney
<michael.d.kin...@intel.com>; Liming Gao <gaolim...@byosoft.com.cn>; Zhiguang
Liu <zhiguang....@intel.com>; Ard Biesheuvel <ardb+tianoc...@kernel.org>;
Jiewen Yao <jiewen....@intel.com>; Jordan Justen <jordan.l.jus...@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.gri...@amd.com>
> Cc: Abner Chang <abner.ch...@amd.com>
> Cc: Eric Dong <eric.d...@intel.com>
> Cc: Ray Ni <ray...@intel.com>
> Cc: Rahul Kumar <rahul1.ku...@intel.com>
> Cc: Gerd Hoffmann <kra...@redhat.com>
> Cc: Michael D Kinney <michael.d.kin...@intel.com>
> Cc: Liming Gao <gaolim...@byosoft.com.cn>
> Cc: Zhiguang Liu <zhiguang....@intel.com>
> Cc: Ard Biesheuvel <ardb+tianoc...@kernel.org>
> Cc: Jiewen Yao <jiewen....@intel.com>
> Cc: Jordan Justen <jordan.l.jus...@intel.com>
> Cc: Abdul Lateef Attar <abdat...@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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
