From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=66.187.233.73; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) (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 E42CE210E8D6D for ; Fri, 10 Aug 2018 08:40:06 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id F25F3B4DF; Fri, 10 Aug 2018 15:40:05 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-207.rdu2.redhat.com [10.10.120.207]) by smtp.corp.redhat.com (Postfix) with ESMTP id 181E52026D65; Fri, 10 Aug 2018 15:40:04 +0000 (UTC) To: Eric Dong , edk2-devel@lists.01.org Cc: Ruiyu Ni References: <20180810041909.12776-1-eric.dong@intel.com> <20180810041909.12776-2-eric.dong@intel.com> From: Laszlo Ersek Message-ID: Date: Fri, 10 Aug 2018 17:40:03 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: <20180810041909.12776-2-eric.dong@intel.com> X-Scanned-By: MIMEDefang 2.78 on 10.11.54.4 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.1]); Fri, 10 Aug 2018 15:40:06 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.1]); Fri, 10 Aug 2018 15:40:06 +0000 (UTC) for IP:'10.11.54.4' DOMAIN:'int-mx04.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'lersek@redhat.com' RCPT:'' Subject: Re: [Patch v3 1/5] UefiCpuPkg/PiSmmCpuDxeSmm: Use GDT/IDT saved in Smram. X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.27 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 10 Aug 2018 15:40:07 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 08/10/18 06:19, Eric Dong wrote: > Current implementation copies GDT/IDT at SmmReadyToLock point > from ACPI NVS memory to Smram. Later at S3 resume phase, it restores > memory saved in Smram to ACPI NVS. Driver can directly use GDT/IDT > saved in Smram instead of restore the original ACPI NVS memory. > > This patch do this change. > > Test Done: > Do the OS boot and S3 resume test. > > Cc: Laszlo Ersek > Cc: Ruiyu Ni > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Eric Dong > --- > UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c | 11 ++++------- > 1 file changed, 4 insertions(+), 7 deletions(-) > > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c > index 0b8ef70359..764d8276d3 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c > @@ -448,13 +448,6 @@ PrepareApStartupVector ( > CopyMem ((VOID *) (UINTN) &mExchangeInfo->GdtrProfile, (VOID *) (UINTN) mAcpiCpuData.GdtrProfile, sizeof (IA32_DESCRIPTOR)); > CopyMem ((VOID *) (UINTN) &mExchangeInfo->IdtrProfile, (VOID *) (UINTN) mAcpiCpuData.IdtrProfile, sizeof (IA32_DESCRIPTOR)); > > - // > - // Copy AP's GDT, IDT and Machine Check handler from SMRAM to ACPI NVS memory > - // > - CopyMem ((VOID *) mExchangeInfo->GdtrProfile.Base, mGdtForAp, mExchangeInfo->GdtrProfile.Limit + 1); > - CopyMem ((VOID *) mExchangeInfo->IdtrProfile.Base, mIdtForAp, mExchangeInfo->IdtrProfile.Limit + 1); > - CopyMem ((VOID *)(UINTN) mAcpiCpuData.ApMachineCheckHandlerBase, mMachineCheckHandlerForAp, mAcpiCpuData.ApMachineCheckHandlerSize); > - > mExchangeInfo->StackStart = (VOID *) (UINTN) mAcpiCpuData.StackAddress; > mExchangeInfo->StackSize = mAcpiCpuData.StackSize; > mExchangeInfo->BufferStart = (UINT32) StartupVector; > @@ -901,6 +894,10 @@ GetAcpiCpuData ( > CopyMem (mGdtForAp, (VOID *)Gdtr->Base, Gdtr->Limit + 1); > CopyMem (mIdtForAp, (VOID *)Idtr->Base, Idtr->Limit + 1); > CopyMem (mMachineCheckHandlerForAp, (VOID *)(UINTN)mAcpiCpuData.ApMachineCheckHandlerBase, mAcpiCpuData.ApMachineCheckHandlerSize); > + > + Gdtr->Base = (UINTN)mGdtForAp; > + Idtr->Base = (UINTN)mIdtForAp; > + mAcpiCpuData.ApMachineCheckHandlerBase = (EFI_PHYSICAL_ADDRESS)(UINTN)mMachineCheckHandlerForAp; > } > > /** > This patch looks good to me (I'm ready to give R-b), but I think we can simplify the code further. Can we replace the mGdtForAp, mIdtForAp and mMachineCheckHandlerForAp global variables, with GdtForAp, IdtForAp and MachineCheckHandlerForAp local variables, used only in the GetAcpiCpuData() function? If you prefer to do that as an incremental patch, that's fine too. Thanks, Laszlo