Thanks for the comments. Agree that the order adjustment can make the patch set clearer. Will modify the commits in next version patch set.

Thanks,
Dun

-----Original Message-----
From: Wu, Jiaxin <jiaxin.wu@intel.com>
Sent: Monday, May 13, 2024 10:49 AM
To: devel@edk2.groups.io; Tan, Dun <dun.tan@intel.com>; Ni, Ray <ray.ni@intel.com>
Subject: RE: [edk2-devel] [PATCH 00/19] Remove some S3 related code in CpuS3.c of smm cpu driver

My comments as below:
1. I stronger recommend re-ordering the patches as below so as to combine all related sub-patches together, then we can more easy understand what we are target to resolving: 1) S3 MTRRs operation 2) AP page table unavailiable issue fix. 3) S3 ApHltLoopCode Operation 3) Register table cleanup

1) S3 MTRRs operation:
>   MdeModulePkg: Add gEdkiiS3MtrrSettingGuid
>   OvmfPkg: Save MTRR by lockbox in CpuS3DataDxe
>   UefiCpuPkg: Add locbox lib instance in DSC
>   UefiCpuPkg: Save MTRR by lockbox in CpuS3DataDxe
>   UefiCpuPkg: LoadMtrrData for all cpu in S3Resume
>   UefiCpuPkg: Remove code to load mtrr setting

2) AP page table unavailiable issue fix:
>   UefiCpuPkg: Disable PG in IA32 ApLoopCode

3) S3 ApHltLoopCode Operation:
>   UefiCpuPkg:Abstract some DxeMpLib code to function
>   UefiCpuPkg:Move some code in DxeMpLib to common place
>   UefiCpuPkg: Install gEdkiiEndOfS3ResumeGuid in S3Resume
>   UefiCpuPkg:Relocate AP to new safe buffer in PeiMpLib
>   UefiCpuPkg:Remove code to handle APIC setting and Interrupt
>   UefiCpuPkg:Rremove code to wakeup AP and relocate ap
>   UefiCpuPkg: Remove the duplicated mpservice locate
>   MdeModulePkg: remove MpService2Ppi field in SMM_S3_RESUME_STATE

4) Register table cleanup:
>   UefiCpuPkg:Set PcdCpuFeaturesInitOnS3Resume to TRUE
>   UefiCpuPkg: Remove code to set register table
>   UefiCpuPkg: Remove GetAcpiCpuData() in CpuS3.c

2. Update all intel copyright year.
3. Add all reviewer in the 00 patch.
4. Add PR in the 00 patch (if have).


Thanks,
Jiaxin

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of duntan
> Sent: Friday, May 10, 2024 6:08 PM
> To: devel@edk2.groups.io
> Subject: [edk2-devel] [PATCH 00/19] Remove some S3 related code in
> CpuS3.c of smm cpu driver
>
> This patch set is to remove some S3 related code in CpuS3.c of smm cpu
> driver. It contain commits to:
> 1.Save MTRR setting by lockbox in PEI phase 2.Load MTRR in S3Resume.c
> before transferring to CpuS3.c in smm cpu driver.
> 3.Add callback of gEdkiiEndOfS3ResumeGuid in PeiMpLib to relocate Ap
> to new safe buffer.
> 4.Install gEdkiiEndOfS3ResumeGuid in S3Resume.c before booting OS to
> relocate APs 4.Change PcdCpuFeaturesInitOnS3Resume to TRUE to set
> register table in CpuFeaturesPei 5.Remove code to set register
> table/load mtrr/wakeup AP and relocate ap in CpuS3.c.
>
> With this patch set, CpuS3.c in smm CPU driver can be simplified.
>
> Dun Tan (18):
>   MdeModulePkg: Add gEdkiiS3MtrrSettingGuid
>   OvmfPkg: Save MTRR by lockbox in CpuS3DataDxe
>   UefiCpuPkg: Add locbox lib instance in DSC
>   UefiCpuPkg: Save MTRR by lockbox in CpuS3DataDxe
>   UefiCpuPkg: LoadMtrrData for all cpu in S3Resume
>   UefiCpuPkg: Remove the duplicated mpservice locate
>   UefiCpuPkg: Install gEdkiiEndOfS3ResumeGuid in S3Resume
>   UefiCpuPkg:Abstract some DxeMpLib code to function
>   UefiCpuPkg:Move some code in DxeMpLib to common place
>   UefiCpuPkg:Relocate AP to new safe buffer in PeiMpLib
>   UefiCpuPkg: Disable PG in IA32 ApLoopCode
>   UefiCpuPkg: Remove code to load mtrr setting
>   UefiCpuPkg:Set PcdCpuFeaturesInitOnS3Resume to TRUE
>   UefiCpuPkg: Remove code to set register table
>   UefiCpuPkg:Remove code to handle APIC setting and Interrupt
>   UefiCpuPkg:Rremove code to wakeup AP and relocate ap
>   UefiCpuPkg: Remove GetAcpiCpuData() in CpuS3.c
>   MdeModulePkg: remove MpService2Ppi field in SMM_S3_RESUME_STATE
>
> Ray Ni (1):
>   MdePkg: Add MmUnblockMemoryLib to MdeLibs.dsc
>
> MdeModulePkg/Include/Guid/AcpiS3Context.h               |   1 -
> MdeModulePkg/MdeModulePkg.dec                           |   3 +++
> MdePkg/MdeLibs.dsc.inc                                  |   3 ++-
> OvmfPkg/CpuS3DataDxe/CpuS3Data.c                        | 11 +++++++++++
> OvmfPkg/CpuS3DataDxe/CpuS3DataDxe.inf                   |   2 ++
> UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c                     | 11 +++++++++++
> UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf                |   2 ++
> UefiCpuPkg/Library/MpInitLib/DxeMpLib.c                 | 181
> +++++++++++++++++++++++++++++++++++++++++++++++-------------------
> ----------------------------------------------------------------------
> ------------------------
> -----------
> UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm          |   4 ++++
> UefiCpuPkg/Library/MpInitLib/MpLib.c                    | 142
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> ++++++++++++++++++
> UefiCpuPkg/Library/MpInitLib/MpLib.h                    | 54
> ++++++++++++++++++++++++++++++++++++++++++++++++++-
> UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf           |   4 ++++
> UefiCpuPkg/Library/MpInitLib/PeiMpLib.c                 | 152
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> ++++++++++++++++++++++++++++
> UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c                       | 991 ++++++++---------
> ----------------------------------------------------------------------
> ------------------------
> ----------------------------------------------------------------------
> ------------------------
> --
> UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/MpFuncs.nasm             | 153 ------------
> ----------------------------------------------------------------------
> ------------------------
> ---------------------------------------
> UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c           | 27 ------------
> --------------
> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c              |   6 ++----
> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h              | 24 -----------
> ------------
> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf            |   3 ---
> UefiCpuPkg/PiSmmCpuDxeSmm/X64/MpFuncs.nasm              | 189 ------------
> ----------------------------------------------------------------------
> ------------------------
> -------------------------------------------------------------------------
> UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c            | 28 ------------
> ---------------
> UefiCpuPkg/UefiCpuPkg.dec                               |   2 +-
> UefiCpuPkg/UefiCpuPkg.dsc                               |   1 +
> UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c       | 76
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> -------------
> UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf |   2 ++
> 25 files changed, 511 insertions(+), 1561 deletions(-) delete mode
> 100644 UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/MpFuncs.nasm
> delete mode 100644 UefiCpuPkg/PiSmmCpuDxeSmm/X64/MpFuncs.nasm
>
> --
> 2.31.1.windows.1
>
>
>
>
>