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 2EA1321163DFE for ; Tue, 9 Oct 2018 01:25:52 -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 BA4745F724; Tue, 9 Oct 2018 08:25:51 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-35.rdu2.redhat.com [10.10.120.35]) by smtp.corp.redhat.com (Postfix) with ESMTP id 8BE2D106A780; Tue, 9 Oct 2018 08:25:50 +0000 (UTC) To: Eric Dong , edk2-devel@lists.01.org Cc: Ruiyu Ni , Jian J Wang References: <20181009060100.6984-1-eric.dong@intel.com> From: Laszlo Ersek Message-ID: <8861ff1a-d42d-3778-a92f-44ae4039968b@redhat.com> Date: Tue, 9 Oct 2018 10:25:49 +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: <20181009060100.6984-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.39]); Tue, 09 Oct 2018 08:25:51 +0000 (UTC) Subject: Re: [Patch v3] 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: Tue, 09 Oct 2018 08:25:52 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 10/09/18 08:01, Eric Dong wrote: > V3 changes: > No need to change inf file. Also update commit message to include regression info. > > 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 | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c > index f164c1713b..53ed76c6e6 100644 > --- a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c > +++ b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c > @@ -1105,6 +1105,14 @@ S3RestoreConfig2 ( > // > SetInterruptState (InterruptStatus); > > + if (sizeof(UINTN) == sizeof(UINT32)) { I think we usually insert a space character after the "sizeof" operator in such cases. > + // > + // Paging maybe enabled. If current mode is 32 bit mode and code try to > + // enable 64 bit mode page table, it will cause GP fault. > + // To avoid conflict configuration, disable paging first anyway. > + // > + AsmWriteCr0 (AsmReadCr0 () & (~BIT31)); The bit-manipulation is valid, but only because this code is restricted to 32-bit mode, where UINTN is UINT32. For clarity, I think ~(UINTN)BIT31 would be better. (The AsmWriteCr0() and AsmReadCr0() BaseLib functions work with UINTN, but the type of BIT31 is UINT32.) > + } > AsmWriteCr3 ((UINTN)SmmS3ResumeState->SmmS3Cr3); > > // > So, my main point: Would it make sense to avoid a write to CR0 if paging is disabled? Such as: UINTN Cr0; if (sizeof (UINTN) == sizeof (UINT32)) { Cr0 = AsmReadCr0 (); if ((Cr0 & BIT31) != 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. // Cr0 &= ~(UINTN)BIT31; AsmWriteCr0 (Cr0); } } I haven't tested this patch yet, it's just that I'm generally concerned about CR *writes* under KVM that aren't absolutely necessary. OVMF does not enable the PEI Stack Guard, so in practice, disabling paging is not necessary (because it is never enabled anyway, for now). Therefore I would like to save the CR0 write, in the IA32X64 build of OVMF. Of course, I don't insist on the exact code example that I wrote above; it's just illustration. In summary, I suggest: - please consider making the CR0 write conditional on *both* being in 32-bit mode *and* BIT31 being set in CR0, - for clarity, please use ~(UINTN)BIT31 as mask (even though it makes no practical difference). What do you think? Thanks! Laszlo