Re: [Patch v3 3/5] UefiCpuPkg/CpuS3DataDxe: Change Memory Type and address limitation.

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

Eric,
I think you could change:
AcpiCpuDataEx = AllocateBootServiceMemory (sizeof (ACPI_CPU_DATA_EX));
->
AcpiCpuDataEx = AllocatePages (EFI_SIZE_TO_PAGES (sizeof (ACPI_CPU_DATA_EX)));

So the AllocateBootServiceMemory() is not needed.

Thanks/Ray

> -----Original Message-----
> From: Dong, Eric
> Sent: Monday, August 13, 2018 9:51 AM
> To: Laszlo Ersek <lersek@redhat.com>; edk2-devel@lists.01.org
> Cc: Marvin Häuser <Marvin.Haeuser@outlook.com>; Fan Jeff
> <vanjeff_919@hotmail.com>; Ni, Ruiyu <ruiyu.ni@intel.com>
> Subject: RE: [Patch v3 3/5] UefiCpuPkg/CpuS3DataDxe: Change Memory
> Type and address limitation.
> 
> Hi Laszlo,
> 
> I checked the code base and don't find the API AllocateZeroPages (), and I
> agree to Zero the memory before using it. So I think update the original
> function is the best way now.
> 
> Thanks,
> Eric
> 
> > -----Original Message-----
> > From: Laszlo Ersek [mailto:lersek@redhat.com]
> > Sent: Saturday, August 11, 2018 12:15 AM
> > To: Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org
> > Cc: Marvin Häuser <Marvin.Haeuser@outlook.com>; Fan Jeff
> > <vanjeff_919@hotmail.com>; Ni, Ruiyu <ruiyu.ni@intel.com>
> > Subject: Re: [Patch v3 3/5] UefiCpuPkg/CpuS3DataDxe: Change Memory
> > Type and address limitation.
> >
> > On 08/10/18 18:12, Laszlo Ersek wrote:
> > > On 08/10/18 06:19, Eric Dong wrote:
> > >> Because CpuS3Data memory will be copy to smram at SmmReadyToLock
> > >> point, 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.
> > >>
> > >> Pass OS boot and resume from S3 test.
> >
> > [snip]
> >
> > > (2) In the previous patch, we lifted the 4GB limitation on the stack
> > > address (while preserving the memory type restriction as AcpiNVS).
> > > However, you continue to allocate the stack with
> > > AllocateAcpiNvsMemoryBelow4G().
> > >
> > > I don't think that's consistent with the purpose of this patch set,
> > > or with the documentation change in the previous patch. We should
> > > allocate the stack as AcpiNVS without address limitation.
> > >
> > > And then we can remove the AllocateAcpiNvsMemoryBelow4G()
> function
> > > altogether.
> >
> > Nevermind, I'm just seeing the next patch.
> >
> > (You might want to add a hint about the next patch to the commit
> > message of this patch -- "we'll handle the stack in the next patch".)
> >
> > So only my question (1) remains for this patch, i.e. why not use
> > AllocateZeroPages().
> >
> > Thanks,
> > Laszlo