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

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com>

Thanks/Ray

> -----Original Message-----
> From: edk2-devel <edk2-devel-bounces@lists.01.org> On Behalf Of Eric Dong
> Sent: Wednesday, August 15, 2018 10:15 AM
> To: edk2-devel@lists.01.org
> Cc: Laszlo Ersek <lersek@redhat.com>; Ni, Ruiyu <ruiyu.ni@intel.com>
> Subject: [edk2] [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
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

On 08/15/18 04:14, 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.
> 
> 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(-)

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks,
Laszlo

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

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

On 08/15/18 15:12, Marvin Häuser wrote:
> 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?

I agree that the function's name is not optimal, primarily because the
Allocate*Pages() functions tend to take a page count, not a byte count.
However, I didn't want to ask for another version just because of this;
a lot of review (and now testing) has gone into this set, and this is
just a wart.

I suggest that -- after the stable tag -- we push v4 as-is; however,
Marvin, please go ahead and file a TianoCore BZ that depends on 959
(i.e. on the BZ currently referenced in patch #5), about fixing up the
function name (and about making it static).

Note that an "AllocateZeroPages" function exists in
"IntelSiliconPkg/Feature/VTd/IntelVTdDxe/TranslationTable.c" as well. I
guess both functions should be renamed (and likely not to the same new
name, because they have different parameter lists). And, only the
UefiCpuPkg one can be made static however. Either way, both packages
could be covered by the same BZ.

Thanks
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Hi Marvin & Laszlo,

I'm not very clear about the risk to use this function name. I think it is just used in a driver as an internal function, other drivers or libraries can't use it. I think we don't need to add internal prefix to all internal functions in drivers, maybe need for the libraries, right?  Here we need to add internal prefix just because it has similar name with other common API?

Thanks,
Eric

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Laszlo Ersek
> Sent: Wednesday, August 15, 2018 11:30 PM
> To: Marvin Häuser <Marvin.Haeuser@outlook.com>; edk2-devel@lists.01.org;
> Dong, Eric <eric.dong@intel.com>
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>
> Subject: Re: [edk2] [Patch v4 3/5] UefiCpuPkg/CpuS3DataDxe: Change
> Memory Type and address limitation.
> 
> On 08/15/18 15:12, Marvin Häuser wrote:
> > 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?
> 
> I agree that the function's name is not optimal, primarily because the
> Allocate*Pages() functions tend to take a page count, not a byte count.
> However, I didn't want to ask for another version just because of this; a lot of
> review (and now testing) has gone into this set, and this is just a wart.
> 
> I suggest that -- after the stable tag -- we push v4 as-is; however, Marvin,
> please go ahead and file a TianoCore BZ that depends on 959 (i.e. on the BZ
> currently referenced in patch #5), about fixing up the function name (and
> about making it static).
> 
> Note that an "AllocateZeroPages" function exists in
> "IntelSiliconPkg/Feature/VTd/IntelVTdDxe/TranslationTable.c" as well. I guess
> both functions should be renamed (and likely not to the same new name,
> because they have different parameter lists). And, only the UefiCpuPkg one
> can be made static however. Either way, both packages could be covered by
> the same BZ.


> 
> Thanks
> Laszlo
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

On 08/16/18 02:56, Dong, Eric wrote:
> Hi Marvin & Laszlo,
> 
> I'm not very clear about the risk to use this function name. I think it is just used in a driver as an internal function, other drivers or libraries can't use it. I think we don't need to add internal prefix to all internal functions in drivers, maybe need for the libraries, right?  Here we need to add internal prefix just because it has similar name with other common API?

If I understood correctly, there were two points to Marvin's argument:

- AllocateZeroPages() is the most likely function name that
"MemoryAllocationLib.h" would add, *if* it ever introduced a function
for "allocating boot service data pages, plus zeroing them". In that
case, it would cause a conflict.

- Second, because the function is defined in the same translation unit
where it is called from (and there are no other callers), we can make
the function STATIC.

Regarding the first concern, I don't think it's a very practical one.
I'm neutral on the question. My point is only that, if we really want to
change the name, I think we should do it separately / incrementally.

Regarding the second idea, STATIC is a generally good practice, and we
should do that everywhere we can. But, because I don't want to re-test /
re-review this series after all this effort, I suggest we do the STATIC
thing incrementally as well.

Thanks
Laszlo


> 
> Thanks,
> Eric
> 
>> -----Original Message-----
>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
>> Laszlo Ersek
>> Sent: Wednesday, August 15, 2018 11:30 PM
>> To: Marvin Häuser <Marvin.Haeuser@outlook.com>; edk2-devel@lists.01.org;
>> Dong, Eric <eric.dong@intel.com>
>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>
>> Subject: Re: [edk2] [Patch v4 3/5] UefiCpuPkg/CpuS3DataDxe: Change
>> Memory Type and address limitation.
>>
>> On 08/15/18 15:12, Marvin Häuser wrote:
>>> 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?
>>
>> I agree that the function's name is not optimal, primarily because the
>> Allocate*Pages() functions tend to take a page count, not a byte count.
>> However, I didn't want to ask for another version just because of this; a lot of
>> review (and now testing) has gone into this set, and this is just a wart.
>>
>> I suggest that -- after the stable tag -- we push v4 as-is; however, Marvin,
>> please go ahead and file a TianoCore BZ that depends on 959 (i.e. on the BZ
>> currently referenced in patch #5), about fixing up the function name (and
>> about making it static).
>>
>> Note that an "AllocateZeroPages" function exists in
>> "IntelSiliconPkg/Feature/VTd/IntelVTdDxe/TranslationTable.c" as well. I guess
>> both functions should be renamed (and likely not to the same new name,
>> because they have different parameter lists). And, only the UefiCpuPkg one
>> can be made static however. Either way, both packages could be covered by
>> the same BZ.
> 
> 
>>
>> Thanks
>> Laszlo
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Comments inline.

Thanks and best regards,
Marvin.

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Thursday, August 16, 2018 2:31 PM
> To: Dong, Eric <eric.dong@intel.com>; Marvin Häuser
> <Marvin.Haeuser@outlook.com>; edk2-devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>
> Subject: Re: [edk2] [Patch v4 3/5] UefiCpuPkg/CpuS3DataDxe: Change
> Memory Type and address limitation.
> 
> On 08/16/18 02:56, Dong, Eric wrote:
> > Hi Marvin & Laszlo,
> >
> > I'm not very clear about the risk to use this function name. I think it is just
> used in a driver as an internal function, other drivers or libraries can't use it. I
> think we don't need to add internal prefix to all internal functions in drivers,
> maybe need for the libraries, right?  Here we need to add internal prefix just
> because it has similar name with other common API?
> 
> If I understood correctly, there were two points to Marvin's argument:
> 
> - AllocateZeroPages() is the most likely function name that
> "MemoryAllocationLib.h" would add, *if* it ever introduced a function for
> "allocating boot service data pages, plus zeroing them". In that case, it would
> cause a conflict.

Correct

> 
> - Second, because the function is defined in the same translation unit where
> it is called from (and there are no other callers), we can make the function
> STATIC.

Pretty much, but it was tied to the first point. There are many functions that could be static but aren't in edk2, so this isn't significant itself. I mentioned it due to my personal naming convention to ensure uniqueness.

> 
> Regarding the first concern, I don't think it's a very practical one.
> I'm neutral on the question. My point is only that, if we really want to change
> the name, I think we should do it separately / incrementally.

If it's supposed to be done separately, I don't see a point in fixing it either, it can still be fixed if such an API is ever introduced. It was meant as a "preventive" suggestion to be included in this series. "Just in case"

> 
> Regarding the second idea, STATIC is a generally good practice, and we
> should do that everywhere we can. But, because I don't want to re-test / re-
> review this series after all this effort, I suggest we do the STATIC thing
> incrementally as well.

+1, but that's not worth an own patch to be honest. I should see whether there is some static analyzer that has checks for "can be static" some day.

> 
> Thanks
> Laszlo
> 
> 
> >
> > Thanks,
> > Eric
> >
> >> -----Original Message-----
> >> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf
> >> Of Laszlo Ersek
> >> Sent: Wednesday, August 15, 2018 11:30 PM
> >> To: Marvin Häuser <Marvin.Haeuser@outlook.com>;
> >> edk2-devel@lists.01.org; Dong, Eric <eric.dong@intel.com>
> >> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>
> >> Subject: Re: [edk2] [Patch v4 3/5] UefiCpuPkg/CpuS3DataDxe: Change
> >> Memory Type and address limitation.
> >>
> >> On 08/15/18 15:12, Marvin Häuser wrote:
> >>> 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?
> >>
> >> I agree that the function's name is not optimal, primarily because
> >> the
> >> Allocate*Pages() functions tend to take a page count, not a byte count.
> >> However, I didn't want to ask for another version just because of
> >> this; a lot of review (and now testing) has gone into this set, and this is
> just a wart.
> >>
> >> I suggest that -- after the stable tag -- we push v4 as-is; however,
> >> Marvin, please go ahead and file a TianoCore BZ that depends on 959
> >> (i.e. on the BZ currently referenced in patch #5), about fixing up
> >> the function name (and about making it static).
> >>
> >> Note that an "AllocateZeroPages" function exists in
> >> "IntelSiliconPkg/Feature/VTd/IntelVTdDxe/TranslationTable.c" as well.
> >> I guess both functions should be renamed (and likely not to the same
> >> new name, because they have different parameter lists). And, only the
> >> UefiCpuPkg one can be made static however. Either way, both packages
> >> could be covered by the same BZ.
> >
> >
> >>
> >> Thanks
> >> Laszlo
> >> _______________________________________________
> >> edk2-devel mailing list
> >> edk2-devel@lists.01.org
> >> https://lists.01.org/mailman/listinfo/edk2-devel

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Hi,

Got your points now. I think we can update the code when new library API added.

Thanks,
Eric
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Marvin Häuser
> Sent: Thursday, August 16, 2018 9:00 PM
> To: edk2-devel@lists.01.org; Laszlo Ersek <lersek@redhat.com>; Dong, Eric
> <eric.dong@intel.com>
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>
> Subject: Re: [edk2] [Patch v4 3/5] UefiCpuPkg/CpuS3DataDxe: Change
> Memory Type and address limitation.
> 
> Comments inline.
> 
> Thanks and best regards,
> Marvin.
> 
> > -----Original Message-----
> > From: Laszlo Ersek <lersek@redhat.com>
> > Sent: Thursday, August 16, 2018 2:31 PM
> > To: Dong, Eric <eric.dong@intel.com>; Marvin Häuser
> > <Marvin.Haeuser@outlook.com>; edk2-devel@lists.01.org
> > Cc: Ni, Ruiyu <ruiyu.ni@intel.com>
> > Subject: Re: [edk2] [Patch v4 3/5] UefiCpuPkg/CpuS3DataDxe: Change
> > Memory Type and address limitation.
> >
> > On 08/16/18 02:56, Dong, Eric wrote:
> > > Hi Marvin & Laszlo,
> > >
> > > I'm not very clear about the risk to use this function name. I think
> > > it is just
> > used in a driver as an internal function, other drivers or libraries
> > can't use it. I think we don't need to add internal prefix to all
> > internal functions in drivers, maybe need for the libraries, right?
> > Here we need to add internal prefix just because it has similar name with
> other common API?
> >
> > If I understood correctly, there were two points to Marvin's argument:
> >
> > - AllocateZeroPages() is the most likely function name that
> > "MemoryAllocationLib.h" would add, *if* it ever introduced a function
> > for "allocating boot service data pages, plus zeroing them". In that
> > case, it would cause a conflict.
> 
> Correct
> 
> >
> > - Second, because the function is defined in the same translation unit
> > where it is called from (and there are no other callers), we can make
> > the function STATIC.
> 
> Pretty much, but it was tied to the first point. There are many functions that
> could be static but aren't in edk2, so this isn't significant itself. I mentioned it
> due to my personal naming convention to ensure uniqueness.
> 
> >
> > Regarding the first concern, I don't think it's a very practical one.
> > I'm neutral on the question. My point is only that, if we really want
> > to change the name, I think we should do it separately / incrementally.
> 
> If it's supposed to be done separately, I don't see a point in fixing it either, it
> can still be fixed if such an API is ever introduced. It was meant as a
> "preventive" suggestion to be included in this series. "Just in case"
> 
> >
> > Regarding the second idea, STATIC is a generally good practice, and we
> > should do that everywhere we can. But, because I don't want to re-test
> > / re- review this series after all this effort, I suggest we do the
> > STATIC thing incrementally as well.
> 
> +1, but that's not worth an own patch to be honest. I should see whether there
> is some static analyzer that has checks for "can be static" some day.
> 
> >
> > Thanks
> > Laszlo
> >
> >
> > >
> > > Thanks,
> > > Eric
> > >
> > >> -----Original Message-----
> > >> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf
> > >> Of Laszlo Ersek
> > >> Sent: Wednesday, August 15, 2018 11:30 PM
> > >> To: Marvin Häuser <Marvin.Haeuser@outlook.com>;
> > >> edk2-devel@lists.01.org; Dong, Eric <eric.dong@intel.com>
> > >> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>
> > >> Subject: Re: [edk2] [Patch v4 3/5] UefiCpuPkg/CpuS3DataDxe: Change
> > >> Memory Type and address limitation.
> > >>
> > >> On 08/15/18 15:12, Marvin Häuser wrote:
> > >>> 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?
> > >>
> > >> I agree that the function's name is not optimal, primarily because
> > >> the
> > >> Allocate*Pages() functions tend to take a page count, not a byte count.
> > >> However, I didn't want to ask for another version just because of
> > >> this; a lot of review (and now testing) has gone into this set, and
> > >> this is
> > just a wart.
> > >>
> > >> I suggest that -- after the stable tag -- we push v4 as-is;
> > >> however, Marvin, please go ahead and file a TianoCore BZ that
> > >> depends on 959 (i.e. on the BZ currently referenced in patch #5),
> > >> about fixing up the function name (and about making it static).
> > >>
> > >> Note that an "AllocateZeroPages" function exists in
> > >> "IntelSiliconPkg/Feature/VTd/IntelVTdDxe/TranslationTable.c" as well.
> > >> I guess both functions should be renamed (and likely not to the
> > >> same new name, because they have different parameter lists). And,
> > >> only the UefiCpuPkg one can be made static however. Either way,
> > >> both packages could be covered by the same BZ.
> > >
> > >
> > >>
> > >> Thanks
> > >> Laszlo
> > >> _______________________________________________
> > >> edk2-devel mailing list
> > >> edk2-devel@lists.01.org
> > >> https://lists.01.org/mailman/listinfo/edk2-devel
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel