S3 cleanup #5606

td36 · 2024-04-30T02:05:09Z

No description provided.

niruiyu · 2024-04-30T07:59:44Z

UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c

+    //
+    // Get MP Services2 Ppi to pass it to Smm S3.
+    //
+    Status = PeiServicesLocatePpi (
+               &gEdkiiPeiMpServices2PpiGuid,
+               0,
+               NULL,
+               (VOID **)&MpService2Ppi
+               );
+    ASSERT_EFI_ERROR (Status);
+


This patch is a bit confusing.
You can update the next patch to LocatePpi and use it and create a new patch to remove the extra PPI-locate.

sounds good.

mergify · 2024-05-08T01:55:02Z

PR can not be merged due to conflict. Please rebase and resubmit

jiaxinwu · 2024-05-09T02:29:25Z

UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c

-
-  ZeroMem (&mAcpiCpuData.CpuFeatureInitData, sizeof (CPU_FEATURE_INIT_DATA));
-
-  if (!PcdGetBool (PcdCpuFeaturesInitOnS3Resume)) {


please add one more patch to change PcdCpuFeaturesInitOnS3Resume PCD default to true since we will drop the PcdCpuFeaturesInitOnS3Resume support.

jiaxinwu · 2024-05-09T04:09:35Z

Below patch should after "[UefiCpuPkg:Move some code in DxeMpLib to common place]" and before "[UefiCpuPkg: Relocate AP to new safe buffer in PeiMpLib]":
[UefiCpuPkg: Install gEdkiiEndOfS3ResumeGuid in S3Resume]

jiaxinwu · 2024-05-09T04:18:47Z

UefiCpuPkg/Library/MpInitLib/DxeMpLib.c

+  EFI_GCD_MEMORY_SPACE_DESCRIPTOR  MemDesc;
+
+  //
+  // TODO: Check EFI_MEMORY_XP bit set or not once it's available in DXE GCD


This TODO gap closed?

Seems the comments was left long time ago. Not sure about it. Will figure it out

td36 · 2024-05-10T10:00:33Z

Below patch should after "[UefiCpuPkg:Move some code in DxeMpLib to common place]" and before "[UefiCpuPkg: Relocate AP to new safe buffer in PeiMpLib]": [UefiCpuPkg: Install gEdkiiEndOfS3ResumeGuid in S3Resume]

I think the current order is ok. Installation of gEdkiiEndOfS3ResumeGuid doesn't impact anything till the callback of this GUID is registered in PeiMplib.

Add gEdkiiS3MtrrSettingGuid a new GUID for s3 MTRR setting. This GUID will be used to save MTRR_SETTINGS at EndOfDxe by LockBox and restore at S3 boot PEI phase for s3 usage. Signed-off-by: Dun Tan <dun.tan@intel.com> Reviewed-by: Ray Ni <ray.ni@intel.com> Cc: Liming Gao <gaoliming@byosoft.com.cn> Cc: Jiaxin Wu <jiaxin.wu@intel.com>

Save MTRR by lockbox in CpuS3DataDxe. In S3 boot, The MTRR setting will be restored in S3Resume.c in following patches. Then S3Resume.c will wakeup all APs to load the MTRR setting. This can avoid waking up APs in CpuS3.c. Signed-off-by: Dun Tan <dun.tan@intel.com> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org> Cc: Jiewen Yao <jiewen.yao@intel.com> Cc: Gerd Hoffmann <kraxel@redhat.com> Reviewed-by: Ray Ni <ray.ni@intel.com> Cc: Jiaxin Wu <jiaxin.wu@intel.com>

Add locbox lib instance in DSC. The SmmLockBoxDxeLib will be consumed by CpuS3DataDxe driver Signed-off-by: Dun Tan <dun.tan@intel.com> Reviewed-by: Ray Ni <ray.ni@intel.com> Cc: Rahul Kumar <rahul1.kumar@intel.com> Cc: Gerd Hoffmann <kraxel@redhat.com> Cc: Jiaxin Wu <jiaxin.wu@intel.com>

Save MTRR by lockbox in CpuS3DataDxe. In S3 boot, The MTRR setting will be restored in S3Resume.c in following patches. Then S3Resume.c will wakeup all APs to load the MTRR setting. This can avoid waking up APs in CpuS3.c. Signed-off-by: Dun Tan <dun.tan@intel.com> Reviewed-by: Ray Ni <ray.ni@intel.com> Cc: Rahul Kumar <rahul1.kumar@intel.com> Cc: Gerd Hoffmann <kraxel@redhat.com> Cc: Jiaxin Wu <jiaxin.wu@intel.com>

In this commit, S3Resume.c wakeup all Aps to run LoadMtrrData for all cpu before transfer to CpuS3.c in smm cpu driver. The MtrrSetting table can be restored by gEdkiiS3MtrrSettingGuid which is saved by lockbox in PEI phase. This can avoid waking up APs in CpuS3.c. Signed-off-by: Dun Tan <dun.tan@intel.com> Reviewed-by: Ray Ni <ray.ni@intel.com> Cc: Rahul Kumar <rahul1.kumar@intel.com> Cc: Gerd Hoffmann <kraxel@redhat.com> Cc: Jiaxin Wu <jiaxin.wu@intel.com>

Remove code to load mtrr setting in CpuS3.c. In previous commits, before transferring to CpuS3.c, MTRR setting has been loaded in S3RestoreConfig2() for all CPU. Signed-off-by: Dun Tan <dun.tan@intel.com> Reviewed-by: Ray Ni <ray.ni@intel.com> Cc: Rahul Kumar <rahul1.kumar@intel.com> Cc: Gerd Hoffmann <kraxel@redhat.com> Cc: Jiaxin Wu <jiaxin.wu@intel.com>

Set PcdCpuFeaturesInitOnS3Resume to TRUE. So that CpuFeaturesPei PEIM will initialize the CPU registers and perform CPU features initialization. Signed-off-by: Dun Tan <dun.tan@intel.com> Reviewed-by: Ray Ni <ray.ni@intel.com> Cc: Rahul Kumar <rahul1.kumar@intel.com> Cc: Gerd Hoffmann <kraxel@redhat.com> Cc: Jiaxin Wu <jiaxin.wu@intel.com>

Remove code to set register table in CpuS3.c. In previous commit, PcdCpuFeaturesInitOnS3Resume has been set to TRUE. So that CpuFeaturesPei PEIM will initialize the CPU registers and perform CPU features initialization. Signed-off-by: Dun Tan <dun.tan@intel.com> Reviewed-by: Ray Ni <ray.ni@intel.com> Cc: Rahul Kumar <rahul1.kumar@intel.com> Cc: Gerd Hoffmann <kraxel@redhat.com> Cc: Jiaxin Wu <jiaxin.wu@intel.com>

Disable paging in IA32 RelocateApLoop assembly code to fix the issue that the AP page table is unavailiable after boot OS under IA32 execution mode. This issue exist in IA32 PEI + IA32 DXE normal boot (also S3 boot with IA32 PEI after previous three commits are accepted). In current MpLib code, the IA32 execution mode code did not create page table in reserved memory like what X64 code did. If PcdCpuStackGuard is TRUE, the PG is enabled for AP in current RelocateApLoop assembly code. And the page table for AP is unavailiable after boot OS. This might cause potential issue. So disable PG in IA32 RelocateApLoop. Signed-off-by: Dun Tan <dun.tan@intel.com> Reviewed-by: Ray Ni <ray.ni@intel.com> Cc: Rahul Kumar <rahul1.kumar@intel.com> Cc: Gerd Hoffmann <kraxel@redhat.com> Cc: Jiaxin Wu <jiaxin.wu@intel.com>

Abstract some DxeMpLib code to function in this commit. Some of these internal functions will be moved to common MpLib.c in following commits. Then PeiMpLib can reuse the code. Signed-off-by: Dun Tan <dun.tan@intel.com> Reviewed-by: Ray Ni <ray.ni@intel.com> Cc: Rahul Kumar <rahul1.kumar@intel.com> Cc: Gerd Hoffmann <kraxel@redhat.com> Cc: Jiaxin Wu <jiaxin.wu@intel.com>

Move some code in DxeMpLib.C to common MpLib.c. The related code is to relocate Ap to new safe buffer before booting into OS. In next commits, these code also will be used by PeiMpLib. This commit doesn't change any code functionality. Signed-off-by: Dun Tan <dun.tan@intel.com> Reviewed-by: Ray Ni <ray.ni@intel.com> Cc: Rahul Kumar <rahul1.kumar@intel.com> Cc: Gerd Hoffmann <kraxel@redhat.com> Cc: Jiaxin Wu <jiaxin.wu@intel.com>

Install gEdkiiEndOfS3ResumeGuid in S3Resume to trigger callback registered by PeiMpLib. The callback is to relocate Ap to new safe memory before jump to OS waking vector in S3 boot flow. Signed-off-by: Dun Tan <dun.tan@intel.com> Reviewed-by: Ray Ni <ray.ni@intel.com> Cc: Rahul Kumar <rahul1.kumar@intel.com> Cc: Gerd Hoffmann <kraxel@redhat.com> Cc: Jiaxin Wu <jiaxin.wu@intel.com>

In this commit, change PeiMpLib to install callback of gEdkiiEndOfS3ResumeGuid to relocate AP to new safe buffer. The gEdkiiEndOfS3ResumeGuid is installed in S3Resume.c before jmping to OS waking vector. Previously, code in CpuS3.c of PiSmmCpuDxe driver will prepare the new safe buffer for AP and place AP in hlt loop state. With this code change, we can remove the Machine Instructions of mApHltLoopCode in PiSmmCpuDxe. Also we can reuse the related code in DxeMpLib for PeiMpLib. Signed-off-by: Dun Tan <dun.tan@intel.com> Reviewed-by: Ray Ni <ray.ni@intel.com> Cc: Rahul Kumar <rahul1.kumar@intel.com> Cc: Gerd Hoffmann <kraxel@redhat.com> Cc: Jiaxin Wu <jiaxin.wu@intel.com>

Remove ProgramVirtualWireMode()/DisableLvtInterrupts() since APs won't be waken by INIT-SIPI-SIPI in CpuS3.c any more. The two functions has been executed in MpInitLibInitialize() in PeiMplib. Signed-off-by: Dun Tan <dun.tan@intel.com> Reviewed-by: Ray Ni <ray.ni@intel.com> Cc: Rahul Kumar <rahul1.kumar@intel.com> Cc: Gerd Hoffmann <kraxel@redhat.com> Cc: Jiaxin Wu <jiaxin.wu@intel.com>

After the code to load mtrr setting, set register table, handle APIC setting and Interrupt after INIT-SIPI-SIPI is moved, the InitializeCpuProcedure() only contains following code logic: 1.Bsp runs ExecuteFirstSmiInit(). 2.Bsp transfers AP to safe hlt-loop During S3 boot, since APs will be relocated to new safe buffer by the callback of gEdkiiEndOfS3ResumeGuid in PeiMpLib, Bsp doesn't need to transfer AP to safe hlt-loop any more. SmmRestoreCpu() in CpuS3 only needs to runs the ExecuteFirstSmiInit() on BSP. So remove code to wakeup AP by INIT-SIPI-SIPI and remove code to relocate ap to safe hlt-loop. Signed-off-by: Dun Tan <dun.tan@intel.com> Reviewed-by: Ray Ni <ray.ni@intel.com> Cc: Rahul Kumar <rahul1.kumar@intel.com> Cc: Gerd Hoffmann <kraxel@redhat.com> Cc: Jiaxin Wu <jiaxin.wu@intel.com>

Remove the unneeded assignment of MpService2Ppi field in SmmS3ResumeState. Previously, when the execution combination of PEI and DXE are the same, the pointer of mpservice ppi will be passed to CpuS3.c in smm cpu driver to wakeup all APs, instead of init-sipi-sipi. Currently, CpuS3.c doesn't need to wakeup Aps anymore. So remove the duplicated mpservice locate and assignment to MpService2Ppi field in SmmS3ResumeState. Signed-off-by: Dun Tan <dun.tan@intel.com> Reviewed-by: Ray Ni <ray.ni@intel.com> Cc: Rahul Kumar <rahul1.kumar@intel.com> Cc: Gerd Hoffmann <kraxel@redhat.com> Cc: Jiaxin Wu <jiaxin.wu@intel.com>

This MpService2Ppi field in SMM_S3_RESUME_STATE is used to wakeup AP to do the CPU initialization during smm s3 boot when the execution mode of PEI and DXE are the same. Currently, in CpuS3.c of smm cpu driver, BSP doesn't need to wakeup AP anymore. The initialization for AP will be done in S3Resume.c before transfer to CpuS3.c of smm cpu driver. So we can remove the MpService2Ppi field in SMM_S3_RESUME_STATE. Signed-off-by: Dun Tan <dun.tan@intel.com> Reviewed-by: Ray Ni <ray.ni@intel.com> Cc: Jian J Wang <jian.j.wang@intel.com> Cc: Liming Gao <gaoliming@byosoft.com.cn>

Remove GetAcpiCpuData() in CpuS3.c. The mAcpiCpuData is not needed in S3 boot anymore. Signed-off-by: Dun Tan <dun.tan@intel.com> Reviewed-by: Ray Ni <ray.ni@intel.com> Cc: Rahul Kumar <rahul1.kumar@intel.com> Cc: Gerd Hoffmann <kraxel@redhat.com> Cc: Jiaxin Wu <jiaxin.wu@intel.com>

td36 force-pushed the s3_cleanup_upstream branch from 749343e to 63a9371 Compare April 30, 2024 03:10

niruiyu reviewed Apr 30, 2024

View reviewed changes

jiaxinwu reviewed May 9, 2024

View reviewed changes

td36 force-pushed the s3_cleanup_upstream branch 2 times, most recently from 22424d5 to 4287f2b Compare May 10, 2024 09:49

td36 added 9 commits May 17, 2024 16:53

td36 force-pushed the s3_cleanup_upstream branch from 4287f2b to 1f18bf1 Compare May 17, 2024 09:15

td36 added 9 commits May 17, 2024 17:27

td36 force-pushed the s3_cleanup_upstream branch from 1f18bf1 to 4ff6f28 Compare May 17, 2024 09:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

S3 cleanup #5606

S3 cleanup #5606

td36 commented Apr 30, 2024

niruiyu Apr 30, 2024

jiaxinwu May 9, 2024

mergify bot commented May 8, 2024

jiaxinwu May 9, 2024

td36 May 10, 2024

jiaxinwu commented May 9, 2024

jiaxinwu May 9, 2024

td36 May 10, 2024

td36 commented May 10, 2024


		ZeroMem (&mAcpiCpuData.CpuFeatureInitData, sizeof (CPU_FEATURE_INIT_DATA));

		if (!PcdGetBool (PcdCpuFeaturesInitOnS3Resume)) {

S3 cleanup #5606

Are you sure you want to change the base?

S3 cleanup #5606

Conversation

td36 commented Apr 30, 2024

niruiyu Apr 30, 2024

Choose a reason for hiding this comment

jiaxinwu May 9, 2024

Choose a reason for hiding this comment

mergify bot commented May 8, 2024

jiaxinwu May 9, 2024

Choose a reason for hiding this comment

td36 May 10, 2024

Choose a reason for hiding this comment

jiaxinwu commented May 9, 2024

jiaxinwu May 9, 2024

Choose a reason for hiding this comment

td36 May 10, 2024

Choose a reason for hiding this comment

td36 commented May 10, 2024