Good idea, Laszlo. If we know something will fail, we had better make it fail as early as possible, and as obvious as possible.
I think we can do sth in INF: 1) Change # VALID_ARCHITECTURES = IA32 2) Change: [Sources.IA32] S3Resume.c and remove [Sources.X64] If we decide to enable X64 S3Resume, we can go back and add such support. Thank you Yao Jiewen > -----Original Message----- > From: Laszlo Ersek [mailto:ler...@redhat.com] > Sent: Wednesday, October 10, 2018 9:04 PM > To: Yao, Jiewen <jiewen....@intel.com>; Dong, Eric <eric.d...@intel.com>; > edk2-devel@lists.01.org > Cc: Ni, Ruiyu <ruiyu...@intel.com> > Subject: Re: [edk2] [Patch] UefiCpuPkg/S3Resume2Pei: disable paging > before creating new page table. > > 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:edk2-devel-boun...@lists.01.org] On Behalf Of > >> Eric Dong > >> Sent: Wednesday, October 10, 2018 3:44 PM > >> To: edk2-devel@lists.01.org > >> Cc: Ni, Ruiyu <ruiyu...@intel.com>; Laszlo Ersek <ler...@redhat.com> > >> 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 <ruiyu...@intel.com> > >> Cc: Laszlo Ersek <ler...@redhat.com> > >> Cc: Jian J Wang <jian.j.w...@intel.com> > >> Contributed-under: TianoCore Contribution Agreement 1.1 > >> Signed-off-by:Eric Dong <eric.d...@intel.com> > >> Signed-off-by: Eric Dong <eric.d...@intel.com> > >> --- > >> 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 > >> edk2-devel@lists.01.org > >> https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
