On 10/10/18 09:58, Yao, Jiewen wrote: > Hey > I do not think we need add if (sizeof (UINTN) == sizeof (UINT32)) > > This piece of code assume PEI is 32 bit. > The following code AsmEnablePaging64() does not work for X64.
I don't feel strongly about this particular question. Any decent compiler will optimize away the UINTN size check, and IIRC Ray suggested under v1 to explicitly restrict the new code to 32-bit. (Although, we both confirmed that this PEIM only supported 32-bit PEI with SMM.) Now, if the code is *clearly* restricted to 32-bit already -- do we *declare* that fact somewhere? in the code or in the INF file? --, then I agree we might not need the UINTN size check. ... Maybe we should split this driver into [Sources.IA32] and [Sources.X64] more extensively that we currently do, and make the X64+SMM case fail more *obviously* than it currently does. I'll leave it up to you guys to decide if the UINTN size check should be preserved in this patch. Once you have an agreement, I'd like to regression-test the resultant version of the patch. FWIW, this version (v4) does look good to me. Should Jiewen think the UINTN size check is acceptable after all, I'd be ready to give my R-b (and to regression-test the patch as well). Thanks! Laszlo > > Thank you > Yao Jiewen > >> -----Original Message----- >> From: edk2-devel [mailto:[email protected]] On Behalf Of >> Eric Dong >> Sent: Wednesday, October 10, 2018 3:44 PM >> To: [email protected] >> Cc: Ni, Ruiyu <[email protected]>; Laszlo Ersek <[email protected]> >> Subject: [edk2] [Patch] UefiCpuPkg/S3Resume2Pei: disable paging before >> creating new page table. >> >> V4: >> Only disable paging when it is enabled. >> >> V3 changes: >> No need to change inf file. >> >> V2 changes: >> Only disable paging in 32 bit mode, no matter it is enable or not. >> >> V1 changes: >> PEI Stack Guard needs to enable paging. This might cause #GP if code >> trying to write CR3 register with PML4 page table while the processor >> is enabled with PAE paging. >> >> Simply disabling paging before updating CR3 can solve this conflict. >> >> It's an regression caused by change: >> 0a0d5296e448fc350de1594c49b9c0deff7fad60 >> >> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=1232 >> >> Change-Id: I99bfdba5daa48a95a4c4ef97eeca1af086558957 >> Cc: Ruiyu Ni <[email protected]> >> Cc: Laszlo Ersek <[email protected]> >> Cc: Jian J Wang <[email protected]> >> Contributed-under: TianoCore Contribution Agreement 1.1 >> Signed-off-by:Eric Dong <[email protected]> >> Signed-off-by: Eric Dong <[email protected]> >> --- >> UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c | 12 ++++++++++++ >> 1 file changed, 12 insertions(+) >> >> diff --git a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c >> b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c >> index f164c1713b..c059c42db5 100644 >> --- a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c >> +++ b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c >> @@ -964,6 +964,7 @@ S3RestoreConfig2 ( >> VOID *GuidHob; >> BOOLEAN >> Build4GPageTableOnly; >> BOOLEAN >> InterruptStatus; >> + IA32_CR0 CR0Reg; >> >> TempAcpiS3Context = 0; >> TempEfiBootScriptExecutorVariable = 0; >> @@ -1105,6 +1106,17 @@ S3RestoreConfig2 ( >> // >> SetInterruptState (InterruptStatus); >> >> + if (sizeof (UINTN) == sizeof (UINT32)) { >> + CR0Reg.UintN = AsmReadCr0 (); >> + if (CR0Reg.Bits.PG != 0) { >> + // >> + // We're in 32-bit mode, with paging enabled. We can't set CR3 >> to >> + // the 64-bit page tables without first disabling paging. >> + // >> + CR0Reg.Bits.PG = 0; >> + AsmWriteCr0 (CR0Reg.UintN); >> + } >> + } >> AsmWriteCr3 ((UINTN)SmmS3ResumeState->SmmS3Cr3); >> >> // >> -- >> 2.15.0.windows.1 >> >> _______________________________________________ >> edk2-devel mailing list >> [email protected] >> https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
