From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.132.183.28; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id EF60C21A09130 for ; Wed, 10 Oct 2018 06:03:59 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 8F06B8831A; Wed, 10 Oct 2018 13:03:58 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-122.rdu2.redhat.com [10.10.120.122]) by smtp.corp.redhat.com (Postfix) with ESMTP id 5DB29825E4; Wed, 10 Oct 2018 13:03:57 +0000 (UTC) To: "Yao, Jiewen" , "Dong, Eric" , "edk2-devel@lists.01.org" Cc: "Ni, Ruiyu" References: <20181010074339.7804-1-eric.dong@intel.com> <74D8A39837DF1E4DA445A8C0B3885C503ADDBB86@shsmsx102.ccr.corp.intel.com> From: Laszlo Ersek Message-ID: <5af8d5f4-1cc2-747f-16b0-71f5bc9aee9e@redhat.com> Date: Wed, 10 Oct 2018 15:03:56 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <74D8A39837DF1E4DA445A8C0B3885C503ADDBB86@shsmsx102.ccr.corp.intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.28]); Wed, 10 Oct 2018 13:03:58 +0000 (UTC) Subject: Re: [Patch] UefiCpuPkg/S3Resume2Pei: disable paging before creating new page table. X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 10 Oct 2018 13:04:00 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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-bounces@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 ; Laszlo Ersek >> 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 >> Cc: Laszlo Ersek >> Cc: Jian J Wang >> Contributed-under: TianoCore Contribution Agreement 1.1 >> Signed-off-by:Eric Dong >> Signed-off-by: Eric Dong >> --- >> 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