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 1F3B521164EF7 for ; Wed, 10 Oct 2018 07:00:13 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 909A9C05FEF1; Wed, 10 Oct 2018 14:00:12 +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 C0A7710840D2; Wed, 10 Oct 2018 14:00:10 +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> <5ad782d3-d27c-4b91-fc02-a46602ca7027@redhat.com> <74D8A39837DF1E4DA445A8C0B3885C503ADDC97F@shsmsx102.ccr.corp.intel.com> From: Laszlo Ersek Message-ID: Date: Wed, 10 Oct 2018 16:00:02 +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: <74D8A39837DF1E4DA445A8C0B3885C503ADDC97F@shsmsx102.ccr.corp.intel.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.22 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.31]); Wed, 10 Oct 2018 14:00:12 +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 14:00:13 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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-bounces@lists.01.org] On Behalf Of >> Laszlo Ersek >> Sent: Wednesday, October 10, 2018 9:19 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 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 >>> >> >> _______________________________________________ >> edk2-devel mailing list >> edk2-devel@lists.01.org >> https://lists.01.org/mailman/listinfo/edk2-devel