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 5F6F321962301 for ; Thu, 11 Oct 2018 05:06:20 -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 6A1D681129; Thu, 11 Oct 2018 12:06:19 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-173.rdu2.redhat.com [10.10.120.173]) by smtp.corp.redhat.com (Postfix) with ESMTP id 13335105706A; Thu, 11 Oct 2018 12:06:17 +0000 (UTC) To: Eric Dong Cc: edk2-devel@lists.01.org, Ruiyu Ni , Jian J Wang References: <20181011070553.6996-1-eric.dong@intel.com> From: Laszlo Ersek Message-ID: Date: Thu, 11 Oct 2018 14:06:16 +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: <20181011070553.6996-1-eric.dong@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.27]); Thu, 11 Oct 2018 12:06:19 +0000 (UTC) Subject: Re: [Patch v5] 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: Thu, 11 Oct 2018 12:06:21 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 10/11/18 09:05, Eric Dong wrote: > V5: > 1. Add ASSERT to indicate this assumption that environment is 32 bit mode. > 2. Add description in INF about this driver's expected result > in different environment. > > 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 > > Cc: Ruiyu Ni > Cc: Laszlo Ersek > Cc: Jian J Wang > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Eric Dong > --- > UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c | 17 +++++++++++++++++ > UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf | 10 ++++++++++ > 2 files changed, 27 insertions(+) > > diff --git a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c > index f164c1713b..8415ab1583 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; > @@ -1045,6 +1046,13 @@ S3RestoreConfig2 ( > // > GuidHob = GetFirstGuidHob (&gEfiAcpiVariableGuid); > if (GuidHob != NULL) { > + // > + // Below SwitchStack/AsmEnablePaging64 function has (1) please strip the trailing space character here > + // assumption that it's in 32 bits mode now. > + // Add ASSERT code to indicate this assumption. > + // > + ASSERT(sizeof (UINTN) == sizeof (UINT32)); > + > Status = PeiServicesLocatePpi ( > &gPeiSmmAccessPpiGuid, > 0, > @@ -1105,6 +1113,15 @@ S3RestoreConfig2 ( > // > SetInterruptState (InterruptStatus); > > + 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); > > // > diff --git a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf > index 6ce1bf944c..1d0740526f 100644 > --- a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf > +++ b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf > @@ -33,6 +33,16 @@ > # VALID_ARCHITECTURES = IA32 X64 > # > > +# > +# This module is not always workable in IA32 and X64 mode. It has below result: (2) please strip the trailing space character here > +# when it works with SMM mode: > +# =============================== > +# SMM:used SMM:unused > +# PEI:IA32 works works > +# PEI:X64 fails works > +# =============================== > +# > + > [Sources] > S3Resume.c > > With the whitespace changes above (no need to repost just because of them): Reviewed-by: Laszlo Ersek I also regression-tested this patch on top of commit 8122c6bc8b6f, with OVMF, in the following configurations: - IA32, SMM, S3 enabled: boot & suspend/resume PASS - IA32X64, SMM, S3 enabled: boot & suspend/resume PASS - X64, SMM, S3 enabled: boot failure, due to S3Verification(). This is by design, see commit 5133d1f1d297. Therefore this test case is also PASS. - X64, SMM, S3 disabled: boot PASS - IA32, no SMM, S3 enabled: boot & suspend/resume PASS - IA32X64, no SMM, S3 enabled: boot & suspend/resume PASS - X64, no SMM, S3 enabled: boot & suspend/resume PASS Regression-tested-by: Laszlo Ersek Thanks, Laszlo