On 08/09/18 05:18, Ni, Ruiyu wrote: >> -----Original Message----- >> From: Laszlo Ersek <[email protected]> >> Sent: Thursday, August 9, 2018 5:29 AM >> To: Dong, Eric <[email protected]>; [email protected] >> Cc: Ni, Ruiyu <[email protected]> >> 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: [email protected]">http://mid.mail-archive.com/[email protected] 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 _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
