Ted, Can you run uncrustify local? I still see TAB in the patch. > -----Original Message----- > From: Kuo, Ted <ted....@intel.com> > Sent: Friday, December 16, 2022 8:46 PM > To: devel@edk2.groups.io > Cc: Ni, Ray <ray...@intel.com>; Liu, Zhiguang <zhiguang....@intel.com>; Chiu, > Chasel <chasel.c...@intel.com>; Desimone, > Nathaniel L <nathaniel.l.desim...@intel.com>; Zeng, Star > <star.z...@intel.com>; S, Ashraf Ali <ashraf.al...@intel.com>; > Duggapu, Chinni B <chinni.b.dugg...@intel.com> > Subject: [edk2-devel][PATCH v2 1/2] UefiCpuPkg: Supporting S3 in 64bit PEI > > https://bugzilla.tianocore.org/show_bug.cgi?id=4195 > 1.Updated the GDT table in VTF0 to align with the one in S3Resume2Pei. > By doing so can simplify the changes to enable S3 in 64bit PEI. > 2.Use SwitchStack() between PEI and SMM in S3 resume path when both > are in the same execution mode. > 3.Transfer from PEI to OS waking vector by calling SwitchStack() when > both are in the same execution mode. > 4.Removed the debug assertion in S3Resume.c to support 64bit PEI. > > Cc: Ray Ni <ray...@intel.com> > Cc: Zhiguang Liu <zhiguang....@intel.com> > Cc: Chasel Chiu <chasel.c...@intel.com> > Cc: Nate DeSimone <nathaniel.l.desim...@intel.com> > Cc: Star Zeng <star.z...@intel.com> > Cc: Ashraf Ali S <ashraf.al...@intel.com> > Cc: Chinni B Duggapu <chinni.b.dugg...@intel.com> > Signed-off-by: Ted Kuo <ted....@intel.com> > --- > UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c | 13 ++- > UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf | 1 + > .../ResetVector/Vtf0/Ia16/Real16ToFlat32.asm | 63 ++++++++---- > .../Universal/Acpi/S3Resume2Pei/S3Resume.c | 97 ++++++++++++------- > 4 files changed, 117 insertions(+), 57 deletions(-) > > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c > b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c > index 9b45c442c9..fb4a44eab6 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c > @@ -1,7 +1,7 @@ > /** @file > > Code for Processor S3 restoration > > > > -Copyright (c) 2006 - 2021, Intel Corporation. All rights reserved.<BR> > > +Copyright (c) 2006 - 2022, Intel Corporation. All rights reserved.<BR> > > SPDX-License-Identifier: BSD-2-Clause-Patent > > > > **/ > > @@ -783,7 +783,11 @@ SmmRestoreCpu ( > SmmS3ResumeState = mSmmS3ResumeState; > > ASSERT (SmmS3ResumeState != NULL); > > > > - if (SmmS3ResumeState->Signature == SMM_S3_RESUME_SMM_64) { > > + // > > + // Setup 64bit IDT in 64bit SMM env when called from 32bit PEI. > > + // Note: 64bit PEI and 32bit DXE is not a supported combination. > > + // > > + if ((SmmS3ResumeState->Signature == SMM_S3_RESUME_SMM_64) && (FeaturePcdGet > (PcdDxeIplSwitchToLongMode) == TRUE)) { > > // > > // Save the IA32 IDT Descriptor > > // > > @@ -846,9 +850,10 @@ SmmRestoreCpu ( > DEBUG ((DEBUG_INFO, "SMM S3 Return Stack Pointer = %x\n", > SmmS3ResumeState->ReturnStackPointer)); > > > > // > > - // If SMM is in 32-bit mode, then use SwitchStack() to resume PEI Phase > > + // If SMM is in 32-bit mode or PcdDxeIplSwitchToLongMode is FALSE, then > use SwitchStack() to resume PEI Phase. > > + // Note: 64bit PEI and 32bit DXE is not a supported combination. > > // > > - if (SmmS3ResumeState->Signature == SMM_S3_RESUME_SMM_32) { > > + if ((SmmS3ResumeState->Signature == SMM_S3_RESUME_SMM_32) || > (FeaturePcdGet (PcdDxeIplSwitchToLongMode) > == FALSE)) { > > DEBUG ((DEBUG_INFO, "Call SwitchStack() to return to S3 Resume in PEI > Phase\n")); > > > > SwitchStack ( > > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf > b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf > index deef00f9c6..b4b327f60c 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf > @@ -124,6 +124,7 @@ > gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmProfileEnable ## > CONSUMES > > gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmProfileRingBuffer ## > CONSUMES > > gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmFeatureControlMsrLock ## > CONSUMES > > + gEfiMdeModulePkgTokenSpaceGuid.PcdDxeIplSwitchToLongMode ## > CONSUMES > > > > [Pcd] > > gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber ## > SOMETIMES_CONSUMES > > diff --git a/UefiCpuPkg/ResetVector/Vtf0/Ia16/Real16ToFlat32.asm > b/UefiCpuPkg/ResetVector/Vtf0/Ia16/Real16ToFlat32.asm > index 0e79a3984b..f59fc6ead4 100644 > --- a/UefiCpuPkg/ResetVector/Vtf0/Ia16/Real16ToFlat32.asm > +++ b/UefiCpuPkg/ResetVector/Vtf0/Ia16/Real16ToFlat32.asm > @@ -2,7 +2,7 @@ > ; @file > > ; Transition from 16 bit real mode into 32 bit flat protected mode > > ; > > -; Copyright (c) 2008 - 2010, Intel Corporation. All rights reserved.<BR> > > +; Copyright (c) 2008 - 2022, Intel Corporation. All rights reserved.<BR> > > ; SPDX-License-Identifier: BSD-2-Clause-Patent > > ; > > > ;------------------------------------------------------------------------------ > > @@ -92,7 +92,7 @@ ALIGN 16 > > > GDT_BASE: > > ; null descriptor > > -NULL_SEL equ $-GDT_BASE > > +NULL_SEL equ $-GDT_BASE ; Selector [0x0] > > DW 0 ; limit 15:0 > > DW 0 ; base 15:0 > > DB 0 ; base 23:16 > > @@ -100,42 +100,67 @@ NULL_SEL equ $-GDT_BASE > DB 0 ; limit 19:16, flags > > DB 0 ; base 31:24 > > > > +; Spare segment descriptor > > +SPARE1_SEL equ $-GDT_BASE ; Selector [0x8] > > + DW 0 ; limit 15:0 > > + DW 0 ; base 15:0 > > + DB 0 ; base 23:16 > > + DB 0 ; sys flag, dpl, type > > + DB 0 ; limit 19:16, flags > > + DB 0 ; base 31:24 > > + > > +; linear code segment descriptor > > +LINEAR_CODE_SEL equ $-GDT_BASE ; Selector [0x10] > > + DW 0xffff ; limit 15:0 > > + DW 0 ; base 15:0 > > + DB 0 ; base 23:16 > > + DB PRESENT_FLAG(1)|DPL(0)|SYSTEM_FLAG(1)|DESC_TYPE(CODE32_TYPE) > ; 09Bh > > + DB > GRANULARITY_FLAG(1)|DEFAULT_SIZE32(1)|CODE64_FLAG(0)|UPPER_LIMIT(0xf) ; 0CFh > > + DB 0 ; base 31:24 > > + > > ; linear data segment descriptor > > -LINEAR_SEL equ $-GDT_BASE > > +LINEAR_SEL equ $-GDT_BASE ; Selector [0x18] > > DW 0xffff ; limit 15:0 > > DW 0 ; base 15:0 > > DB 0 ; base 23:16 > > - DB PRESENT_FLAG(1)|DPL(0)|SYSTEM_FLAG(1)|DESC_TYPE(DATA32_TYPE) > > - DB > GRANULARITY_FLAG(1)|DEFAULT_SIZE32(1)|CODE64_FLAG(0)|UPPER_LIMIT(0xf) > > + DB PRESENT_FLAG(1)|DPL(0)|SYSTEM_FLAG(1)|DESC_TYPE(DATA32_TYPE) > ; 093h > > + DB > GRANULARITY_FLAG(1)|DEFAULT_SIZE32(1)|CODE64_FLAG(0)|UPPER_LIMIT(0xf) ; 0CFh > > DB 0 ; base 31:24 > > > > -; linear code segment descriptor > > -LINEAR_CODE_SEL equ $-GDT_BASE > > +; Spare segment descriptor > > +SPARE2_SEL equ $-GDT_BASE ; Selector [0x20] > > + DW 0 ; limit 15:0 > > + DW 0 ; base 15:0 > > + DB 0 ; base 23:16 > > + DB 0 ; sys flag, dpl, type > > + DB 0 ; limit 19:16, flags > > + DB 0 ; base 31:24 > > + > > +; linear code (16-bit) segment descriptor > > +LINEAR_CODE16_SEL equ $-GDT_BASE ; Selector [0x28] > > DW 0xffff ; limit 15:0 > > DW 0 ; base 15:0 > > DB 0 ; base 23:16 > > - DB PRESENT_FLAG(1)|DPL(0)|SYSTEM_FLAG(1)|DESC_TYPE(CODE32_TYPE) > > - DB > GRANULARITY_FLAG(1)|DEFAULT_SIZE32(1)|CODE64_FLAG(0)|UPPER_LIMIT(0xf) > > + DB PRESENT_FLAG(1)|DPL(0)|SYSTEM_FLAG(1)|DESC_TYPE(CODE32_TYPE) > ; 09Bh > > + DB > GRANULARITY_FLAG(1)|DEFAULT_SIZE32(0)|CODE64_FLAG(0)|UPPER_LIMIT(0xf) ; 08Fh > > DB 0 ; base 31:24 > > > > -%ifdef ARCH_X64 > > -; linear code (64-bit) segment descriptor > > -LINEAR_CODE64_SEL equ $-GDT_BASE > > +; linear data (16-bit) segment descriptor > > +LINEAR_DATA16_SEL equ $-GDT_BASE ; Selector [0x30] > > DW 0xffff ; limit 15:0 > > DW 0 ; base 15:0 > > DB 0 ; base 23:16 > > - DB PRESENT_FLAG(1)|DPL(0)|SYSTEM_FLAG(1)|DESC_TYPE(CODE64_TYPE) > > - DB > GRANULARITY_FLAG(1)|DEFAULT_SIZE32(0)|CODE64_FLAG(1)|UPPER_LIMIT(0xf) > > + DB PRESENT_FLAG(1)|DPL(0)|SYSTEM_FLAG(1)|DESC_TYPE(DATA32_TYPE) > ; 093h > > + DB 0 > > DB 0 ; base 31:24 > > -%endif > > > > -; linear code segment descriptor > > -LINEAR_CODE16_SEL equ $-GDT_BASE > > +; linear code (64-bit) segment descriptor > > +LINEAR_CODE64_SEL equ $-GDT_BASE ; Selector [0x38] > > DW 0xffff ; limit 15:0 > > DW 0 ; base 15:0 > > DB 0 ; base 23:16 > > - DB PRESENT_FLAG(1)|DPL(0)|SYSTEM_FLAG(1)|DESC_TYPE(CODE32_TYPE) > > - DB > GRANULARITY_FLAG(1)|DEFAULT_SIZE32(0)|CODE64_FLAG(0)|UPPER_LIMIT(0xf) > > + DB PRESENT_FLAG(1)|DPL(0)|SYSTEM_FLAG(1)|DESC_TYPE(CODE64_TYPE) > ; 09Bh > > + DB > GRANULARITY_FLAG(1)|DEFAULT_SIZE32(0)|CODE64_FLAG(1)|UPPER_LIMIT(0xf) ; 0AFh > > DB 0 ; base 31:24 > > > > GDT_END: > > diff --git a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c > b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c > index 8419a4e32a..ec6fdc2d4c 100644 > --- a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c > +++ b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c > @@ -4,7 +4,7 @@ > This module will execute the boot script saved during last boot and after > that, > > control is passed to OS waking up handler. > > > > - Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR> > > + Copyright (c) 2006 - 2022, Intel Corporation. All rights reserved.<BR> > > Copyright (c) 2017, AMD Incorporated. All rights reserved.<BR> > > > > SPDX-License-Identifier: BSD-2-Clause-Patent > > @@ -332,9 +332,8 @@ IsLongModeWakingVector ( > ((Facs->OspmFlags & EFI_ACPI_4_0_OSPM_64BIT_WAKE__F) != 0)) > > { > > // Both BIOS and OS wants 64bit vector > > - if (FeaturePcdGet (PcdDxeIplSwitchToLongMode)) { > > - return TRUE; > > - } > > + ASSERT ((FeaturePcdGet (PcdDxeIplSwitchToLongMode)) || (sizeof (UINTN) > == sizeof (UINT64))); > > + return TRUE; > > } > > } > > > > @@ -521,8 +520,11 @@ S3ResumeBootOs ( > // > > // X64 long mode waking vector > > // > > - DEBUG ((DEBUG_INFO, "Transfer to 64bit OS waking vector - %x\r\n", > (UINTN)Facs->XFirmwareWakingVector)); > > + DEBUG ((DEBUG_INFO, "Transfer from PEI to 64bit OS waking vector - > %x\r\n", (UINTN)Facs- > >XFirmwareWakingVector)); > > if (FeaturePcdGet (PcdDxeIplSwitchToLongMode)) { > > + // > > + // 32bit PEI calls to 64bit OS S3 waking vector > > + // > > AsmEnablePaging64 ( > > 0x38, > > Facs->XFirmwareWakingVector, > > @@ -531,29 +533,58 @@ S3ResumeBootOs ( > (UINT64)(UINTN)TempStackTop > > ); > > } else { > > - // > > - // Report Status code that no valid waking vector is found > > - // > > - REPORT_STATUS_CODE ( > > - EFI_ERROR_CODE | EFI_ERROR_MAJOR, > > - (EFI_SOFTWARE_PEI_MODULE | EFI_SW_PEI_EC_S3_OS_WAKE_ERROR) > > - ); > > - DEBUG ((DEBUG_ERROR, "Unsupported for 32bit DXE transfer to 64bit OS > waking vector!\r\n")); > > - ASSERT (FALSE); > > - CpuDeadLoop (); > > - return; > > + if (sizeof (UINTN) == sizeof (UINT64)) { > > + // > > + // 64bit PEI calls to 64bit OS S3 waking vector > > + // > > + SwitchStack ( > > + (SWITCH_STACK_ENTRY_POINT)(UINTN)Facs->XFirmwareWakingVector, > > + NULL, > > + NULL, > > + (VOID *)(UINTN)TempStackTop > > + ); > > + } else { > > + // > > + // Report Status code that no valid waking vector is found. > > + // Note: 32bit PEI + 32bit DXE firmware calling to 64bit OS S3 > waking vector is an invalid configuration. > > + // > > + REPORT_STATUS_CODE ( > > + EFI_ERROR_CODE | EFI_ERROR_MAJOR, > > + (EFI_SOFTWARE_PEI_MODULE | EFI_SW_PEI_EC_S3_OS_WAKE_ERROR) > > + ); > > + DEBUG ((DEBUG_ERROR, "Unsupported for 32bit DXE transfer to 64bit > OS waking vector!\r\n")); > > + ASSERT (FALSE); > > + CpuDeadLoop (); > > + return; > > + } > > } > > } else { > > // > > // IA32 protected mode waking vector (Page disabled) > > // > > DEBUG ((DEBUG_INFO, "Transfer to 32bit OS waking vector - %x\r\n", > (UINTN)Facs->XFirmwareWakingVector)); > > - SwitchStack ( > > - (SWITCH_STACK_ENTRY_POINT)(UINTN)Facs->XFirmwareWakingVector, > > - NULL, > > - NULL, > > - (VOID *)(UINTN)TempStackTop > > - ); > > + if (sizeof (UINTN) == sizeof (UINT64)) { > > + // > > + // 64bit PEI calls to 32bit OS S3 waking vector > > + // > > + AsmDisablePaging64 ( > > + 0x10, > > + (UINT32)Facs->XFirmwareWakingVector, > > + 0, > > + 0, > > + (UINT32)TempStackTop > > + ); > > + } else { > > + // > > + // 32bit PEI calls to 32bit OS S3 waking vector > > + // > > + SwitchStack ( > > + (SWITCH_STACK_ENTRY_POINT)(UINTN)Facs->XFirmwareWakingVector, > > + NULL, > > + NULL, > > + (VOID *)(UINTN)TempStackTop > > + ); > > + } > > } > > } else { > > // > > @@ -579,7 +610,7 @@ S3ResumeBootOs ( > > > /** > > Restore S3 page table because we do not trust ACPINvs content. > > - If BootScriptExector driver will not run in 64-bit mode, this function > will do nothing. > > + If BootScriptExecutor driver will not run in 64-bit mode, this function > will do nothing. > > > > @param S3NvsPageTableAddress PageTableAddress in ACPINvs > > @param Build4GPageTableOnly If BIOS just build 4G page table only > > @@ -590,7 +621,7 @@ RestoreS3PageTables ( > IN BOOLEAN Build4GPageTableOnly > > ) > > { > > - if (FeaturePcdGet (PcdDxeIplSwitchToLongMode)) { > > + if ((FeaturePcdGet (PcdDxeIplSwitchToLongMode)) || (sizeof (UINTN) == > sizeof (UINT64))) { > > UINT32 RegEax; > > UINT32 RegEdx; > > UINT8 PhysicalAddressBits; > > @@ -825,7 +856,7 @@ S3ResumeExecuteBootScript ( > SignalToSmmByCommunication (&gEdkiiS3SmmInitDoneGuid); > > } > > > > - if (FeaturePcdGet (PcdDxeIplSwitchToLongMode)) { > > + if ((FeaturePcdGet (PcdDxeIplSwitchToLongMode)) || (sizeof (UINTN) == > sizeof (UINT64))) { > > AsmWriteCr3 ((UINTN)AcpiS3Context->S3NvsPageTableAddress); > > } > > > > @@ -1021,7 +1052,7 @@ S3RestoreConfig2 ( > CpuDeadLoop (); > > } > > > > - if (FeaturePcdGet (PcdDxeIplSwitchToLongMode)) { > > + if ((FeaturePcdGet (PcdDxeIplSwitchToLongMode)) || (sizeof (UINTN) == > sizeof (UINT64))) { > > // > > // Need reconstruct page table here, since we do not trust ACPINvs. > > // > > @@ -1039,13 +1070,6 @@ S3RestoreConfig2 ( > // > > GuidHob = GetFirstGuidHob (&gEfiAcpiVariableGuid); > > if (GuidHob != NULL) { > > - // > > - // Below SwitchStack/AsmEnablePaging64 function has > > - // assumption that it's in 32 bits mode now. > > - // Add ASSERT code to indicate this assumption. > > - // > > - ASSERT (sizeof (UINTN) == sizeof (UINT32)); > > - > > Status = PeiServicesLocatePpi ( > > &gPeiSmmAccessPpiGuid, > > 0, > > @@ -1079,7 +1103,12 @@ S3RestoreConfig2 ( > DEBUG ((DEBUG_INFO, "SMM S3 Return Stack Pointer = %x\n", > SmmS3ResumeState->ReturnStackPointer)); > > DEBUG ((DEBUG_INFO, "SMM S3 Smst = %x\n", > SmmS3ResumeState->Smst)); > > > > - if (SmmS3ResumeState->Signature == SMM_S3_RESUME_SMM_32) { > > + // > > + // Directly do the switch stack when PEI and SMM env run in the same > execution mode. > > + // > > + if (((SmmS3ResumeState->Signature == SMM_S3_RESUME_SMM_32) && (sizeof > (UINTN) == sizeof (UINT32))) || > > + ((SmmS3ResumeState->Signature == SMM_S3_RESUME_SMM_64) && (sizeof > (UINTN) == sizeof (UINT64)))) > > + { > > SwitchStack ( > > > (SWITCH_STACK_ENTRY_POINT)(UINTN)SmmS3ResumeState->SmmS3ResumeEntryPoint, > > (VOID *)AcpiS3Context, > > -- > 2.35.3.windows.1
-=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#97535): https://edk2.groups.io/g/devel/message/97535 Mute This Topic: https://groups.io/mt/95708649/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
