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. So, currently I think that your v1 patch is best, just add the Reported-by and Suggested-by tags, plus the TianoCore BZ reference. Sorry that I've taken so long to grasp the implications. (Anyway we're now in the quiet period.) Thanks, Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
