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

["Marvin Häuser" <Marvin.Haeuser@outlook.com>]

Hey Eric and anyone CC'd,

Are you sure you want to name the function "AllocateZeroPages"? It's analogous to "AllocateZeroPool", so I could see it becoming an API function at some point, which will conflict with this definition and might silently break UefiCpuPkg compilation if not tested before upstreaming. I usually make any module's private functions static and prefix "Internal" if possible, or, if static cannot be used, non-static plus prefix something derived from the module's name to achieve uniqueness. If I am not mistaken, this could be made static, couldn't it?

Thanks,
Marvin

> -----Original Message-----
> From: Eric Dong <eric.dong@intel.com>
> Sent: Wednesday, August 15, 2018 4:15 AM
> To: edk2-devel@lists.01.org
> Cc: Marvin Häuser <Marvin.Haeuser@outlook.com>; Fan Jeff
> <vanjeff_919@hotmail.com>; Laszlo Ersek <lersek@redhat.com>; Ruiyu Ni
> <ruiyu.ni@intel.com>
> Subject: [Patch v4 3/5] UefiCpuPkg/CpuS3DataDxe: Change Memory Type
> and address limitation.
> 
> 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.
> 
> V4 Changes:
> 1. Create AllocateZeroPages and use it. It's easy to use than gBS-
> >AllocatePages.
> 
> Pass OS boot and resume from S3 test.
> 
> Cc: Marvin Häuser <Marvin.Haeuser@outlook.com>
> Cc: Fan Jeff <vanjeff_919@hotmail.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Eric Dong <eric.dong@intel.com>
> ---
>  UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c      | 34
> +++++++++++++++++++++++++-------
>  UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf |  1 +
>  2 files changed, 28 insertions(+), 7 deletions(-)
> 
> diff --git a/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
> b/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
> index dccb406b8d..3e8c8b383c 100644
> --- a/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
> +++ b/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
> @@ -31,6 +31,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY
> KIND, EITHER EXPRESS OR IMPLIED.
>  #include <Library/UefiBootServicesTableLib.h>
>  #include <Library/DebugLib.h>
>  #include <Library/MtrrLib.h>
> +#include <Library/MemoryAllocationLib.h>
> 
>  #include <Protocol/MpService.h>
>  #include <Guid/EventGroup.h>
> @@ -81,6 +82,28 @@ AllocateAcpiNvsMemoryBelow4G (
>    return Buffer;
>  }
> 
> +/**
> +  Allocate memory and clean it with zero.
> +
> +  @param[in] Size   Size of memory to allocate.
> +
> +  @return       Allocated address for output.
> +
> +**/
> +VOID *
> +AllocateZeroPages (
> +  IN UINTN  Size
> +  )
> +{
> +  VOID                  *Buffer;
> +
> +  Buffer = AllocatePages (EFI_SIZE_TO_PAGES (Size));  if (Buffer !=
> + NULL) {
> +    ZeroMem (Buffer, Size);
> +  }
> +
> +  return Buffer;
> +}
>  /**
>    Callback function executed when the EndOfDxe event group is signaled.
> 
> @@ -171,10 +194,7 @@ CpuS3DataInitialize (
>    //
>    OldAcpiCpuData = (ACPI_CPU_DATA *) (UINTN) PcdGet64
> (PcdCpuS3DataAddress);
> 
> -  //
> -  // Allocate ACPI NVS memory below 4G memory for use on ACPI S3
> resume.
> -  //
> -  AcpiCpuDataEx = AllocateAcpiNvsMemoryBelow4G (sizeof
> (ACPI_CPU_DATA_EX));
> +  AcpiCpuDataEx = AllocateZeroPages (sizeof (ACPI_CPU_DATA_EX));
>    ASSERT (AcpiCpuDataEx != NULL);
>    AcpiCpuData = &AcpiCpuDataEx->AcpiCpuData;
> 
> @@ -223,11 +243,11 @@ CpuS3DataInitialize (
>    AsmReadIdtr (&AcpiCpuDataEx->IdtrProfile);
> 
>    //
> -  // Allocate GDT and IDT in ACPI NVS and copy current GDT and IDT contents
> +  // Allocate GDT and IDT and copy current GDT and IDT contents
>    //
>    GdtSize = AcpiCpuDataEx->GdtrProfile.Limit + 1;
>    IdtSize = AcpiCpuDataEx->IdtrProfile.Limit + 1;
> -  Gdt = AllocateAcpiNvsMemoryBelow4G (GdtSize + IdtSize);
> +  Gdt = AllocateZeroPages (GdtSize + IdtSize);
>    ASSERT (Gdt != NULL);
>    Idt = (VOID *)((UINTN)Gdt + GdtSize);
>    CopyMem (Gdt, (VOID *)AcpiCpuDataEx->GdtrProfile.Base, GdtSize); @@ -
> 243,7 +263,7 @@ CpuS3DataInitialize (
>      // Allocate buffer for empty RegisterTable and PreSmmInitRegisterTable
> for all CPUs
>      //
>      TableSize = 2 * NumberOfCpus * sizeof (CPU_REGISTER_TABLE);
> -    RegisterTable = (CPU_REGISTER_TABLE
> *)AllocateAcpiNvsMemoryBelow4G (TableSize);
> +    RegisterTable = (CPU_REGISTER_TABLE *)AllocateZeroPages
> + (TableSize);
>      ASSERT (RegisterTable != NULL);
> 
>      for (Index = 0; Index < NumberOfCpus; Index++) { diff --git
> a/UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf
> b/UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf
> index 480c98ebcd..c16731529c 100644
> --- a/UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf
> +++ b/UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf
> @@ -51,6 +51,7 @@
>    DebugLib
>    BaseLib
>    MtrrLib
> +  MemoryAllocationLib
> 
>  [Guids]
>    gEfiEndOfDxeEventGroupGuid         ## CONSUMES   ## Event
> --
> 2.15.0.windows.1