On 10/10/18 15:30, Yao, Jiewen wrote: > OK. I forget that OVMF uses a different lock box implementation. > > If so, how about below: > ================== > GuidHob = GetFirstGuidHob (&gEfiAcpiVariableGuid); > if (GuidHob != NULL) { > + ASSERT(sizeof(UINTN) == sizeof(UINT32)); > ==================
Looks good! > > I also think we need add your table to INF. > ================== > SMM:used SMM:unused > PEI:IA32 works works > PEI:X64 fails works > ================== Ditto. :) Thanks! Laszlo >> -----Original Message----- >> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of >> Laszlo Ersek >> Sent: Wednesday, October 10, 2018 9:19 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 15:14, Yao, Jiewen wrote: >>> 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. >> >> This would be a bit heavy-handed however, as S3Resume2Pei works fine >> with X64 PEI, but only without SMM. >> >> There are four cases: >> >> SMM:used SMM:unused >> PEI:IA32 works works >> PEI:X64 fails works >> >> Only the X64+SMM case fails. >> >> Thanks >> Laszlo >> >> >>>> -----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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel