[Patch v4 0/5] Change CpuS3Data memory type and address limitation

[Patch v4 0/5] Change CpuS3Data memory type and address limitation

[Eric Dong]

Because CpuS3Data memory will be copy to smram at SmmReadToLock point by PiSmmCpuDxeSmm driver, 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.

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>

Eric Dong (5):
  UefiCpuPkg/PiSmmCpuDxeSmm: Use GDT/IDT saved in Smram.
  UefiCpuPkg/AcpiCpuData.h: Remove AcpiNVS and Below 4G limitation.
  UefiCpuPkg/CpuS3DataDxe: Change Memory Type and address limitation.
  UefiCpuPkg/CpuS3DataDxe: Remove below 4G limitation.
  UefiCpuPkg/RegisterCpuFeaturesLib: Combine implementation.

 UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c                |  51 +++++---
 UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf           |   1 +
 UefiCpuPkg/Include/AcpiCpuData.h                   |  34 ++----
 .../DxeRegisterCpuFeaturesLib.c                    |  67 -----------
 .../PeiRegisterCpuFeaturesLib.c                    | 131 ---------------------
 .../RegisterCpuFeaturesLib/RegisterCpuFeatures.h   |  20 ----
 .../RegisterCpuFeaturesLib.c                       |  92 +++++++++++++++
 UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c                  |  31 +++--
 8 files changed, 155 insertions(+), 272 deletions(-)

-- 
2.15.0.windows.1

[Patch v4 1/5] UefiCpuPkg/PiSmmCpuDxeSmm: Use GDT/IDT saved in Smram.

[Eric Dong]

Current implementation will copy GDT/IDT at SmmReadyToLock point
from ACPI NVS memory to Smram. Later at S3 resume phase, it restore
the memory saved in Smram to ACPI NVS. It can directly use GDT/IDT
saved in Smram instead of restore the original ACPI NVS memory.
This patch do this change.

V4 changes:
1. Remove global variables mGdtForAp/mIdtForAp/mMachineCheckHandlerForAp.

Test Done:
  Do the OS boot and S3 resume test.

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/PiSmmCpuDxeSmm/CpuS3.c | 31 ++++++++++++++-----------------
 1 file changed, 14 insertions(+), 17 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
index 0b8ef70359..abd8a5a07b 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
@@ -66,9 +66,6 @@ ACPI_CPU_DATA                mAcpiCpuData;
 volatile UINT32              mNumberToFinish;
 MP_CPU_EXCHANGE_INFO         *mExchangeInfo;
 BOOLEAN                      mRestoreSmmConfigurationInS3 = FALSE;
-VOID                         *mGdtForAp = NULL;
-VOID                         *mIdtForAp = NULL;
-VOID                         *mMachineCheckHandlerForAp = NULL;
 MP_MSR_LOCK                  *mMsrSpinLocks = NULL;
 UINTN                        mMsrSpinLockCount;
 UINTN                        mMsrCount = 0;
@@ -448,13 +445,6 @@ PrepareApStartupVector (
   CopyMem ((VOID *) (UINTN) &mExchangeInfo->GdtrProfile, (VOID *) (UINTN) mAcpiCpuData.GdtrProfile, sizeof (IA32_DESCRIPTOR));
   CopyMem ((VOID *) (UINTN) &mExchangeInfo->IdtrProfile, (VOID *) (UINTN) mAcpiCpuData.IdtrProfile, sizeof (IA32_DESCRIPTOR));
 
-  //
-  // Copy AP's GDT, IDT and Machine Check handler from SMRAM to ACPI NVS memory
-  //
-  CopyMem ((VOID *) mExchangeInfo->GdtrProfile.Base, mGdtForAp, mExchangeInfo->GdtrProfile.Limit + 1);
-  CopyMem ((VOID *) mExchangeInfo->IdtrProfile.Base, mIdtForAp, mExchangeInfo->IdtrProfile.Limit + 1);
-  CopyMem ((VOID *)(UINTN) mAcpiCpuData.ApMachineCheckHandlerBase, mMachineCheckHandlerForAp, mAcpiCpuData.ApMachineCheckHandlerSize);
-
   mExchangeInfo->StackStart  = (VOID *) (UINTN) mAcpiCpuData.StackAddress;
   mExchangeInfo->StackSize   = mAcpiCpuData.StackSize;
   mExchangeInfo->BufferStart = (UINT32) StartupVector;
@@ -831,6 +821,9 @@ GetAcpiCpuData (
   ACPI_CPU_DATA              *AcpiCpuData;
   IA32_DESCRIPTOR            *Gdtr;
   IA32_DESCRIPTOR            *Idtr;
+  VOID                       *GdtForAp;
+  VOID                       *IdtForAp;
+  VOID                       *MachineCheckHandlerForAp;
 
   if (!mAcpiS3Enable) {
     return;
@@ -893,14 +886,18 @@ GetAcpiCpuData (
   Gdtr = (IA32_DESCRIPTOR *)(UINTN)mAcpiCpuData.GdtrProfile;
   Idtr = (IA32_DESCRIPTOR *)(UINTN)mAcpiCpuData.IdtrProfile;
 
-  mGdtForAp = AllocatePool ((Gdtr->Limit + 1) + (Idtr->Limit + 1) +  mAcpiCpuData.ApMachineCheckHandlerSize);
-  ASSERT (mGdtForAp != NULL);
-  mIdtForAp = (VOID *) ((UINTN)mGdtForAp + (Gdtr->Limit + 1));
-  mMachineCheckHandlerForAp = (VOID *) ((UINTN)mIdtForAp + (Idtr->Limit + 1));
+  GdtForAp = AllocatePool ((Gdtr->Limit + 1) + (Idtr->Limit + 1) +  mAcpiCpuData.ApMachineCheckHandlerSize);
+  ASSERT (GdtForAp != NULL);
+  IdtForAp = (VOID *) ((UINTN)GdtForAp + (Gdtr->Limit + 1));
+  MachineCheckHandlerForAp = (VOID *) ((UINTN)IdtForAp + (Idtr->Limit + 1));
+
+  CopyMem (GdtForAp, (VOID *)Gdtr->Base, Gdtr->Limit + 1);
+  CopyMem (IdtForAp, (VOID *)Idtr->Base, Idtr->Limit + 1);
+  CopyMem (MachineCheckHandlerForAp, (VOID *)(UINTN)mAcpiCpuData.ApMachineCheckHandlerBase, mAcpiCpuData.ApMachineCheckHandlerSize);
 
-  CopyMem (mGdtForAp, (VOID *)Gdtr->Base, Gdtr->Limit + 1);
-  CopyMem (mIdtForAp, (VOID *)Idtr->Base, Idtr->Limit + 1);
-  CopyMem (mMachineCheckHandlerForAp, (VOID *)(UINTN)mAcpiCpuData.ApMachineCheckHandlerBase, mAcpiCpuData.ApMachineCheckHandlerSize);
+  Gdtr->Base = (UINTN)GdtForAp;
+  Idtr->Base = (UINTN)IdtForAp;
+  mAcpiCpuData.ApMachineCheckHandlerBase = (EFI_PHYSICAL_ADDRESS)(UINTN)MachineCheckHandlerForAp;
 }
 
 /**
-- 
2.15.0.windows.1

Re: [Patch v4 1/5] UefiCpuPkg/PiSmmCpuDxeSmm: Use GDT/IDT saved in Smram.

[Ni, Ruiyu]

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: Ni, Ruiyu <ruiyu.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>
> Subject: [edk2] [Patch v4 1/5] UefiCpuPkg/PiSmmCpuDxeSmm: Use GDT/IDT
> saved in Smram.
> 
> Current implementation will copy GDT/IDT at SmmReadyToLock point from ACPI
> NVS memory to Smram. Later at S3 resume phase, it restore the memory saved
> in Smram to ACPI NVS. It can directly use GDT/IDT saved in Smram instead of
> restore the original ACPI NVS memory.
> This patch do this change.
> 
> V4 changes:
> 1. Remove global variables
> mGdtForAp/mIdtForAp/mMachineCheckHandlerForAp.
> 
> Test Done:
>   Do the OS boot and S3 resume test.
> 
> 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/PiSmmCpuDxeSmm/CpuS3.c | 31 ++++++++++++++-----------------
>  1 file changed, 14 insertions(+), 17 deletions(-)
> 
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> index 0b8ef70359..abd8a5a07b 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> @@ -66,9 +66,6 @@ ACPI_CPU_DATA                mAcpiCpuData;
>  volatile UINT32              mNumberToFinish;
>  MP_CPU_EXCHANGE_INFO         *mExchangeInfo;
>  BOOLEAN                      mRestoreSmmConfigurationInS3 = FALSE;
> -VOID                         *mGdtForAp = NULL;
> -VOID                         *mIdtForAp = NULL;
> -VOID                         *mMachineCheckHandlerForAp = NULL;
>  MP_MSR_LOCK                  *mMsrSpinLocks = NULL;
>  UINTN                        mMsrSpinLockCount;
>  UINTN                        mMsrCount = 0;
> @@ -448,13 +445,6 @@ PrepareApStartupVector (
>    CopyMem ((VOID *) (UINTN) &mExchangeInfo->GdtrProfile, (VOID *) (UINTN)
> mAcpiCpuData.GdtrProfile, sizeof (IA32_DESCRIPTOR));
>    CopyMem ((VOID *) (UINTN) &mExchangeInfo->IdtrProfile, (VOID *) (UINTN)
> mAcpiCpuData.IdtrProfile, sizeof (IA32_DESCRIPTOR));
> 
> -  //
> -  // Copy AP's GDT, IDT and Machine Check handler from SMRAM to ACPI NVS
> memory
> -  //
> -  CopyMem ((VOID *) mExchangeInfo->GdtrProfile.Base, mGdtForAp,
> mExchangeInfo->GdtrProfile.Limit + 1);
> -  CopyMem ((VOID *) mExchangeInfo->IdtrProfile.Base, mIdtForAp,
> mExchangeInfo->IdtrProfile.Limit + 1);
> -  CopyMem ((VOID *)(UINTN) mAcpiCpuData.ApMachineCheckHandlerBase,
> mMachineCheckHandlerForAp, mAcpiCpuData.ApMachineCheckHandlerSize);
> -
>    mExchangeInfo->StackStart  = (VOID *) (UINTN) mAcpiCpuData.StackAddress;
>    mExchangeInfo->StackSize   = mAcpiCpuData.StackSize;
>    mExchangeInfo->BufferStart = (UINT32) StartupVector; @@ -831,6 +821,9
> @@ GetAcpiCpuData (
>    ACPI_CPU_DATA              *AcpiCpuData;
>    IA32_DESCRIPTOR            *Gdtr;
>    IA32_DESCRIPTOR            *Idtr;
> +  VOID                       *GdtForAp;
> +  VOID                       *IdtForAp;
> +  VOID                       *MachineCheckHandlerForAp;
> 
>    if (!mAcpiS3Enable) {
>      return;
> @@ -893,14 +886,18 @@ GetAcpiCpuData (
>    Gdtr = (IA32_DESCRIPTOR *)(UINTN)mAcpiCpuData.GdtrProfile;
>    Idtr = (IA32_DESCRIPTOR *)(UINTN)mAcpiCpuData.IdtrProfile;
> 
> -  mGdtForAp = AllocatePool ((Gdtr->Limit + 1) + (Idtr->Limit + 1) +
> mAcpiCpuData.ApMachineCheckHandlerSize);
> -  ASSERT (mGdtForAp != NULL);
> -  mIdtForAp = (VOID *) ((UINTN)mGdtForAp + (Gdtr->Limit + 1));
> -  mMachineCheckHandlerForAp = (VOID *) ((UINTN)mIdtForAp + (Idtr->Limit +
> 1));
> +  GdtForAp = AllocatePool ((Gdtr->Limit + 1) + (Idtr->Limit + 1) +
> + mAcpiCpuData.ApMachineCheckHandlerSize);
> +  ASSERT (GdtForAp != NULL);
> +  IdtForAp = (VOID *) ((UINTN)GdtForAp + (Gdtr->Limit + 1));
> + MachineCheckHandlerForAp = (VOID *) ((UINTN)IdtForAp + (Idtr->Limit +
> + 1));
> +
> +  CopyMem (GdtForAp, (VOID *)Gdtr->Base, Gdtr->Limit + 1);  CopyMem
> + (IdtForAp, (VOID *)Idtr->Base, Idtr->Limit + 1);  CopyMem
> + (MachineCheckHandlerForAp, (VOID
> + *)(UINTN)mAcpiCpuData.ApMachineCheckHandlerBase,
> + mAcpiCpuData.ApMachineCheckHandlerSize);
> 
> -  CopyMem (mGdtForAp, (VOID *)Gdtr->Base, Gdtr->Limit + 1);
> -  CopyMem (mIdtForAp, (VOID *)Idtr->Base, Idtr->Limit + 1);
> -  CopyMem (mMachineCheckHandlerForAp, (VOID
> *)(UINTN)mAcpiCpuData.ApMachineCheckHandlerBase,
> mAcpiCpuData.ApMachineCheckHandlerSize);
> +  Gdtr->Base = (UINTN)GdtForAp;
> +  Idtr->Base = (UINTN)IdtForAp;
> +  mAcpiCpuData.ApMachineCheckHandlerBase =
> + (EFI_PHYSICAL_ADDRESS)(UINTN)MachineCheckHandlerForAp;
>  }
> 
>  /**
> --
> 2.15.0.windows.1
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [Patch v4 1/5] UefiCpuPkg/PiSmmCpuDxeSmm: Use GDT/IDT saved in Smram.

[Laszlo Ersek]

On 08/15/18 04:14, Eric Dong wrote:
> Current implementation will copy GDT/IDT at SmmReadyToLock point
> from ACPI NVS memory to Smram. Later at S3 resume phase, it restore
> the memory saved in Smram to ACPI NVS. It can directly use GDT/IDT
> saved in Smram instead of restore the original ACPI NVS memory.
> This patch do this change.
> 
> V4 changes:
> 1. Remove global variables mGdtForAp/mIdtForAp/mMachineCheckHandlerForAp.
> 
> Test Done:
>   Do the OS boot and S3 resume test.
> 
> 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/PiSmmCpuDxeSmm/CpuS3.c | 31 ++++++++++++++-----------------
>  1 file changed, 14 insertions(+), 17 deletions(-)

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

Thanks,
Laszlo

> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> index 0b8ef70359..abd8a5a07b 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> @@ -66,9 +66,6 @@ ACPI_CPU_DATA                mAcpiCpuData;
>  volatile UINT32              mNumberToFinish;
>  MP_CPU_EXCHANGE_INFO         *mExchangeInfo;
>  BOOLEAN                      mRestoreSmmConfigurationInS3 = FALSE;
> -VOID                         *mGdtForAp = NULL;
> -VOID                         *mIdtForAp = NULL;
> -VOID                         *mMachineCheckHandlerForAp = NULL;
>  MP_MSR_LOCK                  *mMsrSpinLocks = NULL;
>  UINTN                        mMsrSpinLockCount;
>  UINTN                        mMsrCount = 0;
> @@ -448,13 +445,6 @@ PrepareApStartupVector (
>    CopyMem ((VOID *) (UINTN) &mExchangeInfo->GdtrProfile, (VOID *) (UINTN) mAcpiCpuData.GdtrProfile, sizeof (IA32_DESCRIPTOR));
>    CopyMem ((VOID *) (UINTN) &mExchangeInfo->IdtrProfile, (VOID *) (UINTN) mAcpiCpuData.IdtrProfile, sizeof (IA32_DESCRIPTOR));
>  
> -  //
> -  // Copy AP's GDT, IDT and Machine Check handler from SMRAM to ACPI NVS memory
> -  //
> -  CopyMem ((VOID *) mExchangeInfo->GdtrProfile.Base, mGdtForAp, mExchangeInfo->GdtrProfile.Limit + 1);
> -  CopyMem ((VOID *) mExchangeInfo->IdtrProfile.Base, mIdtForAp, mExchangeInfo->IdtrProfile.Limit + 1);
> -  CopyMem ((VOID *)(UINTN) mAcpiCpuData.ApMachineCheckHandlerBase, mMachineCheckHandlerForAp, mAcpiCpuData.ApMachineCheckHandlerSize);
> -
>    mExchangeInfo->StackStart  = (VOID *) (UINTN) mAcpiCpuData.StackAddress;
>    mExchangeInfo->StackSize   = mAcpiCpuData.StackSize;
>    mExchangeInfo->BufferStart = (UINT32) StartupVector;
> @@ -831,6 +821,9 @@ GetAcpiCpuData (
>    ACPI_CPU_DATA              *AcpiCpuData;
>    IA32_DESCRIPTOR            *Gdtr;
>    IA32_DESCRIPTOR            *Idtr;
> +  VOID                       *GdtForAp;
> +  VOID                       *IdtForAp;
> +  VOID                       *MachineCheckHandlerForAp;
>  
>    if (!mAcpiS3Enable) {
>      return;
> @@ -893,14 +886,18 @@ GetAcpiCpuData (
>    Gdtr = (IA32_DESCRIPTOR *)(UINTN)mAcpiCpuData.GdtrProfile;
>    Idtr = (IA32_DESCRIPTOR *)(UINTN)mAcpiCpuData.IdtrProfile;
>  
> -  mGdtForAp = AllocatePool ((Gdtr->Limit + 1) + (Idtr->Limit + 1) +  mAcpiCpuData.ApMachineCheckHandlerSize);
> -  ASSERT (mGdtForAp != NULL);
> -  mIdtForAp = (VOID *) ((UINTN)mGdtForAp + (Gdtr->Limit + 1));
> -  mMachineCheckHandlerForAp = (VOID *) ((UINTN)mIdtForAp + (Idtr->Limit + 1));
> +  GdtForAp = AllocatePool ((Gdtr->Limit + 1) + (Idtr->Limit + 1) +  mAcpiCpuData.ApMachineCheckHandlerSize);
> +  ASSERT (GdtForAp != NULL);
> +  IdtForAp = (VOID *) ((UINTN)GdtForAp + (Gdtr->Limit + 1));
> +  MachineCheckHandlerForAp = (VOID *) ((UINTN)IdtForAp + (Idtr->Limit + 1));
> +
> +  CopyMem (GdtForAp, (VOID *)Gdtr->Base, Gdtr->Limit + 1);
> +  CopyMem (IdtForAp, (VOID *)Idtr->Base, Idtr->Limit + 1);
> +  CopyMem (MachineCheckHandlerForAp, (VOID *)(UINTN)mAcpiCpuData.ApMachineCheckHandlerBase, mAcpiCpuData.ApMachineCheckHandlerSize);
>  
> -  CopyMem (mGdtForAp, (VOID *)Gdtr->Base, Gdtr->Limit + 1);
> -  CopyMem (mIdtForAp, (VOID *)Idtr->Base, Idtr->Limit + 1);
> -  CopyMem (mMachineCheckHandlerForAp, (VOID *)(UINTN)mAcpiCpuData.ApMachineCheckHandlerBase, mAcpiCpuData.ApMachineCheckHandlerSize);
> +  Gdtr->Base = (UINTN)GdtForAp;
> +  Idtr->Base = (UINTN)IdtForAp;
> +  mAcpiCpuData.ApMachineCheckHandlerBase = (EFI_PHYSICAL_ADDRESS)(UINTN)MachineCheckHandlerForAp;
>  }
>  
>  /**
> 

[Patch v4 2/5] UefiCpuPkg/AcpiCpuData.h: Remove AcpiNVS and Below 4G limitation.

[Eric Dong]

ACPI_CPU_DATA structure first introduced to save data in
normal boot phase. Also this data will be used in S3 phase
by one PEI driver. So in first phase, this data is been
defined to use ACPI NVS memory type and must below 4G.

Later in order to fix potential security issue,
PiSmmCpuDxeSmm driver added logic to copy ACPI_CPU_DATA
(except ResetVector and Stack buffer) to  smram at smm
ready to lock point. ResetVector must below 1M and Stack
buffer is write only in S3 phase, so these two fields not
copy to smram. Also PiSmmCpuDxeSmm driver owned the task
to restore the CPU setting and it's a SMM driver.

After above change, the acpi nvs memory type and below 4G
limitation is no longer needed.

This change remove the limitation in the comments for
ACPI_CPU_DATA definition.

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>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com>
---
 UefiCpuPkg/Include/AcpiCpuData.h | 34 ++++++++++++----------------------
 1 file changed, 12 insertions(+), 22 deletions(-)

diff --git a/UefiCpuPkg/Include/AcpiCpuData.h b/UefiCpuPkg/Include/AcpiCpuData.h
index ec092074ce..9e51145c08 100644
--- a/UefiCpuPkg/Include/AcpiCpuData.h
+++ b/UefiCpuPkg/Include/AcpiCpuData.h
@@ -1,7 +1,7 @@
 /** @file
 Definitions for CPU S3 data.
 
-Copyright (c) 2013 - 2017, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2013 - 2018, Intel Corporation. All rights reserved.<BR>
 This program and the accompanying materials
 are licensed and made available under the terms and conditions of the BSD License
 which accompanies this distribution.  The full text of the license may be found at
@@ -57,15 +57,13 @@ typedef struct {
   //
   UINT32                    InitialApicId;
   //
-  // Physical address of CPU_REGISTER_TABLE_ENTRY structures.  This buffer must be
-  // allocated below 4GB from memory of type EfiACPIMemoryNVS.
+  // Physical address of CPU_REGISTER_TABLE_ENTRY structures.
   //
   EFI_PHYSICAL_ADDRESS      RegisterTableEntry;
 } CPU_REGISTER_TABLE;
 
 //
-// Data structure that is required for ACPI S3 resume.  This structure must be
-// allocated below 4GB from memory of type EfiACPIMemoryNVS.  The PCD
+// Data structure that is required for ACPI S3 resume. The PCD
 // PcdCpuS3DataAddress must be set to the physical address where this structure
 // is allocated
 //
@@ -78,21 +76,17 @@ typedef struct {
   //
   EFI_PHYSICAL_ADDRESS  StartupVector;
   //
-  // Physical address of structure of type IA32_DESCRIPTOR.  This structure must
-  // be allocated below 4GB from memory of type EfiACPIMemoryNVS.  The
+  // Physical address of structure of type IA32_DESCRIPTOR. The
   // IA32_DESCRIPTOR structure provides the base address and length of a GDT
-  // The buffer for GDT must also be allocated below 4GB from memory of type
-  // EfiACPIMemoryNVS.  The GDT must be filled in with the GDT contents that are
+  // The GDT must be filled in with the GDT contents that are
   // used during an ACPI S3 resume.  This is typically the contents of the GDT
   // used by the boot processor when the platform is booted.
   //
   EFI_PHYSICAL_ADDRESS  GdtrProfile;
   //
-  // Physical address of structure of type IA32_DESCRIPTOR.  This structure must
-  // be allocated below 4GB from memory of type EfiACPIMemoryNVS.  The
+  // Physical address of structure of type IA32_DESCRIPTOR.  The
   // IA32_DESCRIPTOR structure provides the base address and length of an IDT.
-  // The buffer for IDT must also be allocated below 4GB from memory of type
-  // EfiACPIMemoryNVS.  The IDT must be filled in with the IDT contents that are
+  // The IDT must be filled in with the IDT contents that are
   // used during an ACPI S3 resume.  This is typically the contents of the IDT
   // used by the boot processor when the platform is booted.
   //
@@ -100,7 +94,7 @@ typedef struct {
   //
   // Physical address of a buffer that is used as stacks during ACPI S3 resume.
   // The total size of this buffer, in bytes, is NumberOfCpus * StackSize.  This
-  // structure must be allocated below 4GB from memory of type EfiACPIMemoryNVS.
+  // structure must be allocated from memory of type EfiACPIMemoryNVS.
   //
   EFI_PHYSICAL_ADDRESS  StackAddress;
   //
@@ -118,14 +112,12 @@ typedef struct {
   // Physical address of structure of type MTRR_SETTINGS that contains a copy
   // of the MTRR settings that are compatible with the MTRR settings used by
   // the boot processor when the platform was booted.  These MTRR settings are
-  // used during an ACPI S3 resume.  This structure must be allocated below 4GB
-  // from memory of type EfiACPIMemoryNVS.
+  // used during an ACPI S3 resume.
   //
   EFI_PHYSICAL_ADDRESS  MtrrTable;
   //
   // Physical address of an array of CPU_REGISTER_TABLE structures, with
-  // NumberOfCpus entries.  This array must be allocated below 4GB from memory
-  // of type EfiACPIMemoryNVS.  If a register table is not required, then the
+  // NumberOfCpus entries.  If a register table is not required, then the
   // TableLength and AllocatedSize fields of CPU_REGISTER_TABLE are set to 0.
   // If TableLength is > 0, then elements of RegisterTableEntry are used to
   // initialize the CPU that matches InitialApicId, during an ACPI S3 resume,
@@ -134,8 +126,7 @@ typedef struct {
   EFI_PHYSICAL_ADDRESS  PreSmmInitRegisterTable;
   //
   // Physical address of an array of CPU_REGISTER_TABLE structures, with
-  // NumberOfCpus entries.  This array must be allocated below 4GB from memory
-  // of type EfiACPIMemoryNVS.  If a register table is not required, then the
+  // NumberOfCpus entries.  If a register table is not required, then the
   // TableLength and AllocatedSize fields of CPU_REGISTER_TABLE are set to 0.
   // If TableLength is > 0, then elements of RegisterTableEntry are used to
   // initialize the CPU that matches InitialApicId, during an ACPI S3 resume,
@@ -144,8 +135,7 @@ typedef struct {
   EFI_PHYSICAL_ADDRESS  RegisterTable;
   //
   // Physical address of a buffer that contains the machine check handler that
-  // is used during an ACPI S3 Resume.  This buffer must be allocated below 4GB
-  // from memory of type EfiACPIMemoryNVS.  In order for this machine check
+  // is used during an ACPI S3 Resume.  In order for this machine check
   // handler to be active on an AP during an ACPI S3 resume, the machine check
   // vector in the IDT provided by IdtrProfile must be initialized to transfer
   // control to this physical address.
-- 
2.15.0.windows.1

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

[Eric Dong]

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

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

[Ni, Ruiyu]

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

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

[Laszlo Ersek]

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
> 

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

[Marvin Häuser]

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

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

[Laszlo Ersek]

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

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

[Dong, Eric]

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

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

[Laszlo Ersek]

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

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

[Marvin Häuser]

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

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

[Dong, Eric]

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

[Patch v4 4/5] UefiCpuPkg/CpuS3DataDxe: Remove below 4G limitation.

[Eric Dong]

Because PrepareApStartupVector() stores StackAddress to
"mExchangeInfo->StackStart" (which has type (VOID*)), and because
"UefiCpuPkg/PiSmmCpuDxeSmm/X64/MpFuncs.nasm" reads the latter with:

         add  edi, StackStartAddressLocation
         add  rax, qword [edi]
         mov  rsp, rax
         mov  qword [edi], rax

in long-mode code. So code can remove below 4G limitation.

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>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com>
---
 UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c b/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
index 3e8c8b383c..ef98239844 100644
--- a/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
+++ b/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
@@ -47,9 +47,7 @@ typedef struct {
 } ACPI_CPU_DATA_EX;
 
 /**
-  Allocate EfiACPIMemoryNVS below 4G memory address.
-
-  This function allocates EfiACPIMemoryNVS below 4G memory address.
+  Allocate EfiACPIMemoryNVS memory.
 
   @param[in] Size   Size of memory to allocate.
 
@@ -57,7 +55,7 @@ typedef struct {
 
 **/
 VOID *
-AllocateAcpiNvsMemoryBelow4G (
+AllocateAcpiNvsMemory (
   IN UINTN  Size
   )
 {
@@ -65,9 +63,8 @@ AllocateAcpiNvsMemoryBelow4G (
   EFI_STATUS            Status;
   VOID                  *Buffer;
 
-  Address = BASE_4GB - 1;
   Status  = gBS->AllocatePages (
-                   AllocateMaxAddress,
+                   AllocateAnyPages,
                    EfiACPIMemoryNVS,
                    EFI_SIZE_TO_PAGES (Size),
                    &Address
@@ -230,9 +227,13 @@ CpuS3DataInitialize (
   AcpiCpuData->MtrrTable    = (EFI_PHYSICAL_ADDRESS)(UINTN)&AcpiCpuDataEx->MtrrTable;
 
   //
-  // Allocate stack space for all CPUs
+  // Allocate stack space for all CPUs.
+  // Use ACPI NVS memory type because this data will be directly used by APs
+  // in S3 resume phase in long mode. Also during S3 resume, the stack buffer
+  // will only be used as scratch space. i.e. we won't read anything from it
+  // before we write to it, in PiSmmCpuDxeSmm.
   //
-  Stack = AllocateAcpiNvsMemoryBelow4G (NumberOfCpus * AcpiCpuData->StackSize);
+  Stack = AllocateAcpiNvsMemory (NumberOfCpus * AcpiCpuData->StackSize);
   ASSERT (Stack != NULL);
   AcpiCpuData->StackAddress = (EFI_PHYSICAL_ADDRESS)(UINTN)Stack;
 
-- 
2.15.0.windows.1

[Patch v4 5/5] UefiCpuPkg/RegisterCpuFeaturesLib: Combine implementation.

[Eric Dong]

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.

V3 changes:
> Remove function definition in header file.
> Add STATIC in function implementation.

Pass OS boot and resume from S3 test.

Bugz: https://bugzilla.tianocore.org/show_bug.cgi?id=959

Reported-by: Marvin Häuser <Marvin.Haeuser@outlook.com>
Suggested-by: Fan Jeff <vanjeff_919@hotmail.com>
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>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com>
---
 .../DxeRegisterCpuFeaturesLib.c                    |  67 -----------
 .../PeiRegisterCpuFeaturesLib.c                    | 131 ---------------------
 .../RegisterCpuFeaturesLib/RegisterCpuFeatures.h   |  20 ----
 .../RegisterCpuFeaturesLib.c                       |  92 +++++++++++++++
 4 files changed, 92 insertions(+), 218 deletions(-)

diff --git a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/DxeRegisterCpuFeaturesLib.c b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/DxeRegisterCpuFeaturesLib.c
index 902a339529..1f34a3f489 100644
--- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/DxeRegisterCpuFeaturesLib.c
+++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/DxeRegisterCpuFeaturesLib.c
@@ -197,70 +197,3 @@ GetNumberOfProcessor (
   ASSERT_EFI_ERROR (Status);
 }
 
-/**
-  Allocates ACPI NVS memory to save ACPI_CPU_DATA.
-
-  @return  Pointer to allocated ACPI_CPU_DATA.
-**/
-ACPI_CPU_DATA *
-AllocateAcpiCpuData (
-  VOID
-  )
-{
-  //
-  // CpuS3DataDxe will do it.
-  //
-  ASSERT (FALSE);
-  return NULL;
-}
-
-/**
-  Enlarges CPU register table for each processor.
-
-  @param[in, out]  RegisterTable   Pointer processor's CPU register table
-**/
-VOID
-EnlargeRegisterTable (
-  IN OUT CPU_REGISTER_TABLE            *RegisterTable
-  )
-{
-  EFI_STATUS            Status;
-  EFI_PHYSICAL_ADDRESS  Address;
-  UINTN                 AllocatePages;
-
-  Address = BASE_4GB - 1;
-  AllocatePages = RegisterTable->AllocatedSize / EFI_PAGE_SIZE;
-  Status  = gBS->AllocatePages (
-                   AllocateMaxAddress,
-                   EfiACPIMemoryNVS,
-                   AllocatePages + 1,
-                   &Address
-                   );
-  ASSERT_EFI_ERROR (Status);
-
-  //
-  // If there are records existing in the register table, then copy its contents
-  // to new region and free the old one.
-  //
-  if (RegisterTable->AllocatedSize > 0) {
-    CopyMem (
-      (VOID *) (UINTN) Address,
-      (VOID *) (UINTN) RegisterTable->RegisterTableEntry,
-      RegisterTable->AllocatedSize
-      );
-    //
-    // RegisterTableEntry is allocated by gBS->AllocatePages() service.
-    // So, gBS->FreePages() service is used to free it.
-    //
-    gBS->FreePages (
-      RegisterTable->RegisterTableEntry,
-      AllocatePages
-      );
-  }
-
-  //
-  // Adjust the allocated size and register table base address.
-  //
-  RegisterTable->AllocatedSize     += EFI_PAGE_SIZE;
-  RegisterTable->RegisterTableEntry = Address;
-}
diff --git a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/PeiRegisterCpuFeaturesLib.c b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/PeiRegisterCpuFeaturesLib.c
index 6804eddf65..82fe268812 100644
--- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/PeiRegisterCpuFeaturesLib.c
+++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/PeiRegisterCpuFeaturesLib.c
@@ -257,134 +257,3 @@ GetNumberOfProcessor (
                          );
   ASSERT_EFI_ERROR (Status);
 }
-
-/**
-  Allocates ACPI NVS memory to save ACPI_CPU_DATA.
-
-  @return  Pointer to allocated ACPI_CPU_DATA.
-**/
-ACPI_CPU_DATA *
-AllocateAcpiCpuData (
-  VOID
-  )
-{
-  EFI_STATUS                           Status;
-  EFI_PEI_MP_SERVICES_PPI              *CpuMpPpi;
-  UINTN                                NumberOfCpus;
-  UINTN                                NumberOfEnabledProcessors;
-  ACPI_CPU_DATA                        *AcpiCpuData;
-  EFI_PHYSICAL_ADDRESS                 Address;
-  UINTN                                TableSize;
-  CPU_REGISTER_TABLE                   *RegisterTable;
-  UINTN                                Index;
-  EFI_PROCESSOR_INFORMATION            ProcessorInfoBuffer;
-
-  Status = PeiServicesAllocatePages (
-             EfiACPIMemoryNVS,
-             EFI_SIZE_TO_PAGES (sizeof (ACPI_CPU_DATA)),
-             &Address
-             );
-  ASSERT_EFI_ERROR (Status);
-  AcpiCpuData = (ACPI_CPU_DATA *) (UINTN) Address;
-  ASSERT (AcpiCpuData != NULL);
-
-  //
-  // Get MP Services Protocol
-  //
-  Status = PeiServicesLocatePpi (
-             &gEfiPeiMpServicesPpiGuid,
-             0,
-             NULL,
-             (VOID **)&CpuMpPpi
-             );
-  ASSERT_EFI_ERROR (Status);
-
-  //
-  // Get the number of CPUs
-  //
-  Status = CpuMpPpi->GetNumberOfProcessors (
-                         GetPeiServicesTablePointer (),
-                         CpuMpPpi,
-                         &NumberOfCpus,
-                         &NumberOfEnabledProcessors
-                         );
-  ASSERT_EFI_ERROR (Status);
-  AcpiCpuData->NumberOfCpus = (UINT32)NumberOfCpus;
-
-  //
-  // Allocate buffer for empty RegisterTable and PreSmmInitRegisterTable for all CPUs
-  //
-  TableSize = 2 * NumberOfCpus * sizeof (CPU_REGISTER_TABLE);
-  Status = PeiServicesAllocatePages (
-             EfiACPIMemoryNVS,
-             EFI_SIZE_TO_PAGES (TableSize),
-             &Address
-             );
-  ASSERT_EFI_ERROR (Status);
-  RegisterTable = (CPU_REGISTER_TABLE *) (UINTN) Address;
-
-  for (Index = 0; Index < NumberOfCpus; Index++) {
-    Status = CpuMpPpi->GetProcessorInfo (
-                         GetPeiServicesTablePointer (),
-                         CpuMpPpi,
-                         Index,
-                         &ProcessorInfoBuffer
-                         );
-    ASSERT_EFI_ERROR (Status);
-
-    RegisterTable[Index].InitialApicId      = (UINT32)ProcessorInfoBuffer.ProcessorId;
-    RegisterTable[Index].TableLength        = 0;
-    RegisterTable[Index].AllocatedSize      = 0;
-    RegisterTable[Index].RegisterTableEntry = 0;
-
-    RegisterTable[NumberOfCpus + Index].InitialApicId      = (UINT32)ProcessorInfoBuffer.ProcessorId;
-    RegisterTable[NumberOfCpus + Index].TableLength        = 0;
-    RegisterTable[NumberOfCpus + Index].AllocatedSize      = 0;
-    RegisterTable[NumberOfCpus + Index].RegisterTableEntry = 0;
-  }
-  AcpiCpuData->RegisterTable           = (EFI_PHYSICAL_ADDRESS)(UINTN)RegisterTable;
-  AcpiCpuData->PreSmmInitRegisterTable = (EFI_PHYSICAL_ADDRESS)(UINTN)(RegisterTable + NumberOfCpus);
-
-  return AcpiCpuData;
-}
-
-/**
-  Enlarges CPU register table for each processor.
-
-  @param[in, out]  RegisterTable   Pointer processor's CPU register table
-**/
-VOID
-EnlargeRegisterTable (
-  IN OUT CPU_REGISTER_TABLE            *RegisterTable
-  )
-{
-  EFI_STATUS                           Status;
-  EFI_PHYSICAL_ADDRESS                 Address;
-  UINTN                                AllocatePages;
-
-  AllocatePages = RegisterTable->AllocatedSize / EFI_PAGE_SIZE;
-  Status = PeiServicesAllocatePages (
-             EfiACPIMemoryNVS,
-             AllocatePages + 1,
-             &Address
-             );
-  ASSERT_EFI_ERROR (Status);
-
-  //
-  // If there are records existing in the register table, then copy its contents
-  // to new region and free the old one.
-  //
-  if (RegisterTable->AllocatedSize > 0) {
-    CopyMem (
-      (VOID *) (UINTN) Address,
-      (VOID *) (UINTN) RegisterTable->RegisterTableEntry,
-      RegisterTable->AllocatedSize
-      );
-  }
-
-  //
-  // Adjust the allocated size and register table base address.
-  //
-  RegisterTable->AllocatedSize += EFI_PAGE_SIZE;
-  RegisterTable->RegisterTableEntry = Address;
-}
diff --git a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeatures.h b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeatures.h
index 69b412172a..edd266934f 100644
--- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeatures.h
+++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeatures.h
@@ -87,26 +87,6 @@ GetCpuFeaturesData (
   VOID
   );
 
-/**
-  Enlarges CPU register table for each processor.
-
-  @param[in, out]  RegisterTable   Pointer processor's CPU register table
-**/
-VOID
-EnlargeRegisterTable (
-  IN OUT CPU_REGISTER_TABLE            *RegisterTable
-  );
-
-/**
-  Allocates ACPI NVS memory to save ACPI_CPU_DATA.
-
-  @return  Pointer to allocated ACPI_CPU_DATA.
-**/
-ACPI_CPU_DATA *
-AllocateAcpiCpuData (
-  VOID
-  );
-
 /**
   Worker function to return processor index.
 
diff --git a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
index dd6a82be7a..4143ee4bb1 100644
--- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
+++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
@@ -488,6 +488,98 @@ RegisterCpuFeature (
   return RETURN_SUCCESS;
 }
 
+/**
+  Allocates boot service data to save ACPI_CPU_DATA.
+
+  @return  Pointer to allocated ACPI_CPU_DATA.
+**/
+STATIC
+ACPI_CPU_DATA *
+AllocateAcpiCpuData (
+  VOID
+  )
+{
+  EFI_STATUS                           Status;
+  UINTN                                NumberOfCpus;
+  UINTN                                NumberOfEnabledProcessors;
+  ACPI_CPU_DATA                        *AcpiCpuData;
+  UINTN                                TableSize;
+  CPU_REGISTER_TABLE                   *RegisterTable;
+  UINTN                                Index;
+  EFI_PROCESSOR_INFORMATION            ProcessorInfoBuffer;
+
+  AcpiCpuData  = AllocatePages (EFI_SIZE_TO_PAGES (sizeof (ACPI_CPU_DATA)));
+  ASSERT (AcpiCpuData != NULL);
+
+  GetNumberOfProcessor (&NumberOfCpus, &NumberOfEnabledProcessors);
+  AcpiCpuData->NumberOfCpus = (UINT32)NumberOfCpus;
+
+  //
+  // Allocate buffer for empty RegisterTable and PreSmmInitRegisterTable for all CPUs
+  //  
+  TableSize = 2 * NumberOfCpus * sizeof (CPU_REGISTER_TABLE);
+  RegisterTable  = AllocatePages (EFI_SIZE_TO_PAGES (TableSize));
+  ASSERT (RegisterTable != NULL);
+
+  for (Index = 0; Index < NumberOfCpus; Index++) {
+    Status = GetProcessorInformation (Index, &ProcessorInfoBuffer);
+    ASSERT_EFI_ERROR (Status);
+
+    RegisterTable[Index].InitialApicId      = (UINT32)ProcessorInfoBuffer.ProcessorId;
+    RegisterTable[Index].TableLength        = 0;
+    RegisterTable[Index].AllocatedSize      = 0;
+    RegisterTable[Index].RegisterTableEntry = 0;
+
+    RegisterTable[NumberOfCpus + Index].InitialApicId      = (UINT32)ProcessorInfoBuffer.ProcessorId;
+    RegisterTable[NumberOfCpus + Index].TableLength        = 0;
+    RegisterTable[NumberOfCpus + Index].AllocatedSize      = 0;
+    RegisterTable[NumberOfCpus + Index].RegisterTableEntry = 0;
+  }
+  AcpiCpuData->RegisterTable           = (EFI_PHYSICAL_ADDRESS)(UINTN)RegisterTable;
+  AcpiCpuData->PreSmmInitRegisterTable = (EFI_PHYSICAL_ADDRESS)(UINTN)(RegisterTable + NumberOfCpus);
+
+  return AcpiCpuData;
+}
+
+/**
+  Enlarges CPU register table for each processor.
+
+  @param[in, out]  RegisterTable   Pointer processor's CPU register table
+**/
+STATIC
+VOID
+EnlargeRegisterTable (
+  IN OUT CPU_REGISTER_TABLE            *RegisterTable
+  )
+{
+  EFI_PHYSICAL_ADDRESS  Address;
+  UINTN                 UsedPages;
+
+  UsedPages = RegisterTable->AllocatedSize / EFI_PAGE_SIZE;
+  Address  = (UINTN)AllocatePages (UsedPages + 1);
+  ASSERT (Address != 0);
+
+  //
+  // If there are records existing in the register table, then copy its contents
+  // to new region and free the old one.
+  //
+  if (RegisterTable->AllocatedSize > 0) {
+    CopyMem (
+      (VOID *) (UINTN) Address,
+      (VOID *) (UINTN) RegisterTable->RegisterTableEntry,
+      RegisterTable->AllocatedSize
+      );
+
+    FreePages ((VOID *)(UINTN)RegisterTable->RegisterTableEntry, UsedPages);
+  }
+
+  //
+  // Adjust the allocated size and register table base address.
+  //
+  RegisterTable->AllocatedSize     += EFI_PAGE_SIZE;
+  RegisterTable->RegisterTableEntry = Address;
+}
+
 /**
   Add an entry in specified register table.
 
-- 
2.15.0.windows.1

Re: [Patch v4 0/5] Change CpuS3Data memory type and address limitation

[Laszlo Ersek]

On 08/15/18 04:14, Eric Dong wrote:
> Because CpuS3Data memory will be copy to smram at SmmReadToLock point by PiSmmCpuDxeSmm driver, 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.
> 
> 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>
> 
> Eric Dong (5):
>   UefiCpuPkg/PiSmmCpuDxeSmm: Use GDT/IDT saved in Smram.
>   UefiCpuPkg/AcpiCpuData.h: Remove AcpiNVS and Below 4G limitation.
>   UefiCpuPkg/CpuS3DataDxe: Change Memory Type and address limitation.
>   UefiCpuPkg/CpuS3DataDxe: Remove below 4G limitation.
>   UefiCpuPkg/RegisterCpuFeaturesLib: Combine implementation.
> 
>  UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c                |  51 +++++---
>  UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf           |   1 +
>  UefiCpuPkg/Include/AcpiCpuData.h                   |  34 ++----
>  .../DxeRegisterCpuFeaturesLib.c                    |  67 -----------
>  .../PeiRegisterCpuFeaturesLib.c                    | 131 ---------------------
>  .../RegisterCpuFeaturesLib/RegisterCpuFeatures.h   |  20 ----
>  .../RegisterCpuFeaturesLib.c                       |  92 +++++++++++++++
>  UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c                  |  31 +++--
>  8 files changed, 155 insertions(+), 272 deletions(-)
> 

Looks like this series is now fully reviewed.

I will now do some regression-testing.

Thanks!
Laszlo

Re: [Patch v4 0/5] Change CpuS3Data memory type and address limitation

[Laszlo Ersek]

On 08/15/18 15:14, Laszlo Ersek wrote:
> On 08/15/18 04:14, Eric Dong wrote:
>> Because CpuS3Data memory will be copy to smram at SmmReadToLock point by PiSmmCpuDxeSmm driver, 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.
>>
>> 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>
>>
>> Eric Dong (5):
>>   UefiCpuPkg/PiSmmCpuDxeSmm: Use GDT/IDT saved in Smram.
>>   UefiCpuPkg/AcpiCpuData.h: Remove AcpiNVS and Below 4G limitation.
>>   UefiCpuPkg/CpuS3DataDxe: Change Memory Type and address limitation.
>>   UefiCpuPkg/CpuS3DataDxe: Remove below 4G limitation.
>>   UefiCpuPkg/RegisterCpuFeaturesLib: Combine implementation.
>>
>>  UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c                |  51 +++++---
>>  UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf           |   1 +
>>  UefiCpuPkg/Include/AcpiCpuData.h                   |  34 ++----
>>  .../DxeRegisterCpuFeaturesLib.c                    |  67 -----------
>>  .../PeiRegisterCpuFeaturesLib.c                    | 131 ---------------------
>>  .../RegisterCpuFeaturesLib/RegisterCpuFeatures.h   |  20 ----
>>  .../RegisterCpuFeaturesLib.c                       |  92 +++++++++++++++
>>  UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c                  |  31 +++--
>>  8 files changed, 155 insertions(+), 272 deletions(-)
>>
> 
> Looks like this series is now fully reviewed.
> 
> I will now do some regression-testing.

Tested on the Q35 machine type, built into OVMF with -D SMM_REQUIRE, on
top of commit 22ec06c8aaa1.

I used the tests described here:
<https://github.com/tianocore/tianocore.github.io/wiki/Testing-SMM-with-QEMU,-KVM-and-libvirt>.

- with Linux guests:
  Fedora (IA32); Fedora (IA32X64); RHEL7 (IA32X64)

- with Windows guests (all IA32X64):
  7; Server 2008 R2; 8.1; Server 2012 R2; 10

For patches #1 through #4:
Tested-by: Laszlo Ersek <lersek@redhat.com>

(Patch #5 is not testable with OVMF, because no module in OVMF uses
RegisterCpuFeaturesLib -- the lib class isn't resolved under OvmfPkg/ to
any instance at all.)

Please hold off pushing the series until after the upcoming stable tag.

Thanks,
Laszlo