From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=192.55.52.88; helo=mga01.intel.com; envelope-from=ruiyu.ni@intel.com; receiver=edk2-devel@lists.01.org Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) (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 4F6B021A07A92 for ; Tue, 9 Oct 2018 02:05:03 -0700 (PDT) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga005.jf.intel.com ([10.7.209.41]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 09 Oct 2018 02:05:03 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.54,359,1534834800"; d="scan'208";a="264104610" Received: from ray-dev.ccr.corp.intel.com (HELO [10.239.9.11]) ([10.239.9.11]) by orsmga005.jf.intel.com with ESMTP; 09 Oct 2018 01:58:25 -0700 To: Laszlo Ersek , Eric Dong , edk2-devel@lists.01.org References: <20181009060100.6984-1-eric.dong@intel.com> <8861ff1a-d42d-3778-a92f-44ae4039968b@redhat.com> From: "Ni, Ruiyu" Message-ID: <86397fca-fa57-470a-8ca0-31e9ad1204b2@Intel.com> Date: Tue, 9 Oct 2018 16:59:33 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1 MIME-Version: 1.0 In-Reply-To: <8861ff1a-d42d-3778-a92f-44ae4039968b@redhat.com> 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 09:05:03 -0000 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit On 10/9/2018 4:25 PM, Laszlo Ersek wrote: > 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). Actually we could use IA32_CR0 structure to avoid BIT31 usage. > > What do you think? > > Thanks! > Laszlo > -- Thanks, Ray