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 0F8E321BADAB9 for ; Thu, 9 Aug 2018 04:21:07 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.rdu2.redhat.com [10.11.54.3]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 4790140241C9; Thu, 9 Aug 2018 11:21:06 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-121-52.rdu2.redhat.com [10.10.121.52]) by smtp.corp.redhat.com (Postfix) with ESMTP id 825BB10CD794; Thu, 9 Aug 2018 11:21:05 +0000 (UTC) To: "Ni, Ruiyu" , "Dong, Eric" , "edk2-devel@lists.01.org" References: <20180808074006.21360-1-eric.dong@intel.com> <12da7e20-6f8e-0e3b-f026-42041e6b2415@redhat.com> <734D49CCEBEEF84792F5B80ED585239D5BDC96E8@SHSMSX104.ccr.corp.intel.com> From: Laszlo Ersek Message-ID: Date: Thu, 9 Aug 2018 13:21:04 +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: <734D49CCEBEEF84792F5B80ED585239D5BDC96E8@SHSMSX104.ccr.corp.intel.com> X-Scanned-By: MIMEDefang 2.78 on 10.11.54.3 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.7]); Thu, 09 Aug 2018 11:21:06 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.7]); Thu, 09 Aug 2018 11:21:06 +0000 (UTC) for IP:'10.11.54.3' DOMAIN:'int-mx03.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'lersek@redhat.com' RCPT:'' Subject: Re: [Patch v2 1/2] UefiCpuPkg/RegisterCpuFeaturesLib: Combine implementation. 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: Thu, 09 Aug 2018 11:21:08 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 08/09/18 05:18, Ni, Ruiyu wrote: >> -----Original Message----- >> From: Laszlo Ersek >> Sent: Thursday, August 9, 2018 5:29 AM >> To: Dong, Eric ; edk2-devel@lists.01.org >> Cc: Ni, Ruiyu >> Subject: Re: [edk2] [Patch v2 1/2] UefiCpuPkg/RegisterCpuFeaturesLib: >> Combine implementation. >> >> On 08/08/18 09:40, Eric Dong wrote: >>> V1 changes: >>>> Current code logic can't confirm CpuS3DataDxe driver start before >>>> CpuFeaturesDxe driver. So the assumption in CpuFeaturesDxe not valid. >>>> Add implementation for AllocateAcpiCpuData function to remove this >>>> assumption. >>> >>> V2 changes: >>>> Because CpuS3Data memory will be copy to smram at SmmReadToLock >>>> point, so the memory type no need to be ACPI NVS type, also the >>>> address not limit to below 4G. >>>> This change remove the limit of ACPI NVS memory type and below 4G. >> >> I'm returning to this patch (v2 1/2) after my other comments (for v2 2/2). >> >> It seems that allocating ACPI_CPU_DATA in BootServicesData type memory >> breaks at least the docs / specs in "UefiCpuPkg/Include/AcpiCpuData.h", >> even if we do the BootServicesData allocation in RegisterCpuFeaturesLib >> instances (i.e. in CpuFeaturesPei / CpuFeaturesDxe), and not in >> CpuS3DataDxe. >> >> With that in mind, should we return to your v1 patch, >> "UefiCpuPkg/RegisterCpuFeaturesLib: Implement AllocateAcpiCpuData >> function"? >> >> And, looking back at my question (4) there, where I suggested >> AllocatePeiAccessiblePages() -- I'm now thinking that it does not apply, >> because the ACPI_CPU_DATA spec in "UefiCpuPkg/Include/AcpiCpuData.h" >> requires the 4GB limit. > > I guess Eric forgot to update the comments in AcpiCpuData.h regarding the > 4GB/NVS restriction. If patch #2 is fixed, *and* the "AcpiCpuData.h" documentation is updated, relaxing the allocation restriction, then this patch set could be viable, yes. I would still like Mike to confirm as well. > We've reviewed the whole producer/consumer code and came to the conclusion > that 4GB/NVS restriction is unnecessary. I think you both missed the in-place restoration of the GDT and the IDT, to AcpiNVS memory allocated originally by CpuS3DataDxe. Please re-read section (10) of my other email carefully: http://mid.mail-archive.com/f48935e4-96b3-978e-d67c-84a169414ccb@redhat.com Basically, the GdtrProfile and IdtrProfile fields in ACPI_CPU_DATA are *doubly* indirect references. The fields themselves point to IDT and GDT *descriptors*, and those descriptors (the Base fields in them) point to the tables (IDT and GDT). Indeed, PiSmmCpuDxeSmm saves everything into SMRAM on the normal boot path: (a) ACPI_CPU_DATA, (b) the descriptors pointed-to by GdtrProfile and IdtrProfile, and (c) the tables pointed-to by GdtrProfile->Base and IdtrProfile->Base. However, on the S3 resume path, PiSmmCpuDxeSmm does not use everything from SMRAM. In the PrepareApStartupVector() function, the GDT and the IDT tables -- at the end of the pointer chain -- are restored to the *original* AcpiNVS locations (*not* SMRAM), and they are then used from there. This is not a security bug, because even if the OS overwrites the AcpiNVS area between normal boot and S3 resume, PiSmmCpuDxeSmm never reads that area at S3 before restoring it, from the SMRAM objects "mGdtForAp" and "mIdtForAp". It does mean though that the OS must be informed in advance to stay away from that area, because PiSmmCpuDxeSmm will overwrite it at S3 resume. This is why that allocation must be kept as AcpiNVS. (Or moved to SMRAM entirely.) Laszlo