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

["Ni, Ruiyu" <ruiyu.ni@intel.com>]

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Thursday, August 9, 2018 5:29 AM
> To: Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>
> 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.
We've reviewed the whole producer/consumer code and came to the conclusion
that 4GB/NVS restriction is unnecessary.

> 
> 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