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 9D68C21A09130 for ; Wed, 10 Oct 2018 06:19:10 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 30E56305D465; Wed, 10 Oct 2018 13:19:10 +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 EC63881B46; Wed, 10 Oct 2018 13:19:08 +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> <5af8d5f4-1cc2-747f-16b0-71f5bc9aee9e@redhat.com> <74D8A39837DF1E4DA445A8C0B3885C503ADDC8E0@shsmsx102.ccr.corp.intel.com> From: Laszlo Ersek Message-ID: <5ad782d3-d27c-4b91-fc02-a46602ca7027@redhat.com> Date: Wed, 10 Oct 2018 15:19:06 +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: <74D8A39837DF1E4DA445A8C0B3885C503ADDC8E0@shsmsx102.ccr.corp.intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.46]); Wed, 10 Oct 2018 13:19:10 +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:19:10 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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:lersek@redhat.com] >> Sent: Wednesday, October 10, 2018 9:04 PM >> To: Yao, Jiewen ; Dong, Eric ; >> edk2-devel@lists.01.org >> Cc: Ni, Ruiyu >> 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-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 >