Re: [Patch v2 1/2] UefiCpuPkg/RegisterCpuFeaturesLib: Combine implementation.

[Laszlo Ersek <lersek@redhat.com>]

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