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

[Patch v3 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                |  60 +++++++---
 UefiCpuPkg/Include/AcpiCpuData.h                   |  34 ++----
 .../DxeRegisterCpuFeaturesLib.c                    |  67 -----------
 .../PeiRegisterCpuFeaturesLib.c                    | 131 ---------------------
 .../RegisterCpuFeaturesLib/RegisterCpuFeatures.h   |  20 ----
 .../RegisterCpuFeaturesLib.c                       |  92 +++++++++++++++
 UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c                  |  11 +-
 7 files changed, 153 insertions(+), 262 deletions(-)

-- 
2.15.0.windows.1

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

[Eric Dong]

Current implementation copies GDT/IDT at SmmReadyToLock point
from ACPI NVS memory to Smram. Later at S3 resume phase, it restores
memory saved in Smram to ACPI NVS. Driver can directly use GDT/IDT
saved in Smram instead of restore the original ACPI NVS memory.

This patch do this change.

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 | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
index 0b8ef70359..764d8276d3 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
@@ -448,13 +448,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;
@@ -901,6 +894,10 @@ GetAcpiCpuData (
   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)mGdtForAp;
+  Idtr->Base = (UINTN)mIdtForAp;
+  mAcpiCpuData.ApMachineCheckHandlerBase = (EFI_PHYSICAL_ADDRESS)(UINTN)mMachineCheckHandlerForAp;
 }
 
 /**
-- 
2.15.0.windows.1

[Patch v3 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>
---
 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 v3 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.

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 | 43 +++++++++++++++++++++++++++++++------
 1 file changed, 36 insertions(+), 7 deletions(-)

diff --git a/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c b/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
index dccb406b8d..5b99a6e759 100644
--- a/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
+++ b/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
@@ -81,6 +81,38 @@ AllocateAcpiNvsMemoryBelow4G (
   return Buffer;
 }
 
+/**
+  Allocate EfiBootService memory.
+
+  @param[in] Size   Size of memory to allocate.
+
+  @return       Allocated address for output.
+
+**/
+VOID *
+AllocateBootServiceMemory (
+  IN UINTN  Size
+  )
+{
+  EFI_PHYSICAL_ADDRESS  Address;
+  EFI_STATUS            Status;
+  VOID                  *Buffer;
+
+  Status  = gBS->AllocatePages (
+                   AllocateAnyPages,
+                   EfiBootServicesData,
+                   EFI_SIZE_TO_PAGES (Size),
+                   &Address
+                   );
+  if (EFI_ERROR (Status)) {
+    return NULL;
+  }
+
+  Buffer = (VOID *)(UINTN)Address;
+  ZeroMem (Buffer, Size);
+
+  return Buffer;
+}
 /**
   Callback function executed when the EndOfDxe event group is signaled.
 
@@ -171,10 +203,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 = AllocateBootServiceMemory (sizeof (ACPI_CPU_DATA_EX));
   ASSERT (AcpiCpuDataEx != NULL);
   AcpiCpuData = &AcpiCpuDataEx->AcpiCpuData;
 
@@ -223,11 +252,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 = AllocateBootServiceMemory (GdtSize + IdtSize);
   ASSERT (Gdt != NULL);
   Idt = (VOID *)((UINTN)Gdt + GdtSize);
   CopyMem (Gdt, (VOID *)AcpiCpuDataEx->GdtrProfile.Base, GdtSize);
@@ -243,7 +272,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 *)AllocateBootServiceMemory (TableSize);
     ASSERT (RegisterTable != NULL);
 
     for (Index = 0; Index < NumberOfCpus; Index++) {
-- 
2.15.0.windows.1

[Patch v3 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>
---
 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 5b99a6e759..d18f33a5b8 100644
--- a/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
+++ b/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
@@ -46,9 +46,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.
 
@@ -56,7 +54,7 @@ typedef struct {
 
 **/
 VOID *
-AllocateAcpiNvsMemoryBelow4G (
+AllocateAcpiNvsMemory (
   IN UINTN  Size
   )
 {
@@ -64,9 +62,8 @@ AllocateAcpiNvsMemoryBelow4G (
   EFI_STATUS            Status;
   VOID                  *Buffer;
 
-  Address = BASE_4GB - 1;
   Status  = gBS->AllocatePages (
-                   AllocateMaxAddress,
+                   AllocateAnyPages,
                    EfiACPIMemoryNVS,
                    EFI_SIZE_TO_PAGES (Size),
                    &Address
@@ -239,9 +236,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 PiSmmCpuDxeSemm.
   //
-  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 v3 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>
---
 .../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 v3 1/5] UefiCpuPkg/PiSmmCpuDxeSmm: Use GDT/IDT saved in Smram.

[Laszlo Ersek]

On 08/10/18 06:19, Eric Dong wrote:
> Current implementation copies GDT/IDT at SmmReadyToLock point
> from ACPI NVS memory to Smram. Later at S3 resume phase, it restores
> memory saved in Smram to ACPI NVS. Driver can directly use GDT/IDT
> saved in Smram instead of restore the original ACPI NVS memory.
> 
> This patch do this change.
> 
> 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 | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> index 0b8ef70359..764d8276d3 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> @@ -448,13 +448,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;
> @@ -901,6 +894,10 @@ GetAcpiCpuData (
>    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)mGdtForAp;
> +  Idtr->Base = (UINTN)mIdtForAp;
> +  mAcpiCpuData.ApMachineCheckHandlerBase = (EFI_PHYSICAL_ADDRESS)(UINTN)mMachineCheckHandlerForAp;
>  }
>  
>  /**
> 

This patch looks good to me (I'm ready to give R-b), but I think we can
simplify the code further. Can we replace the mGdtForAp, mIdtForAp and
mMachineCheckHandlerForAp global variables, with GdtForAp, IdtForAp and
MachineCheckHandlerForAp local variables, used only in the
GetAcpiCpuData() function?

If you prefer to do that as an incremental patch, that's fine too.

Thanks,
Laszlo

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

[Laszlo Ersek]

On 08/10/18 06:19, Eric Dong wrote:
> 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>
> ---
>  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.
> 

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

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

[Laszlo Ersek]

On 08/10/18 06:19, Eric Dong wrote:
> Because CpuS3Data memory will be copy to smram at SmmReadyToLock point,
> the memory type no need to be ACPI NVS type, also the address not
> limit to below 4G.
> 
> This change remove the limit of ACPI NVS memory type and below 4G.
> 
> Pass OS boot and resume from S3 test.
> 
> 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 | 43 +++++++++++++++++++++++++++++++------
>  1 file changed, 36 insertions(+), 7 deletions(-)
> 
> diff --git a/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c b/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
> index dccb406b8d..5b99a6e759 100644
> --- a/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
> +++ b/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
> @@ -81,6 +81,38 @@ AllocateAcpiNvsMemoryBelow4G (
>    return Buffer;
>  }
>  
> +/**
> +  Allocate EfiBootService memory.
> +
> +  @param[in] Size   Size of memory to allocate.
> +
> +  @return       Allocated address for output.
> +
> +**/
> +VOID *
> +AllocateBootServiceMemory (
> +  IN UINTN  Size
> +  )
> +{
> +  EFI_PHYSICAL_ADDRESS  Address;
> +  EFI_STATUS            Status;
> +  VOID                  *Buffer;
> +
> +  Status  = gBS->AllocatePages (
> +                   AllocateAnyPages,
> +                   EfiBootServicesData,
> +                   EFI_SIZE_TO_PAGES (Size),
> +                   &Address
> +                   );
> +  if (EFI_ERROR (Status)) {
> +    return NULL;
> +  }
> +
> +  Buffer = (VOID *)(UINTN)Address;
> +  ZeroMem (Buffer, Size);
> +
> +  return Buffer;
> +}

(1) If I remember correctly, we discussed AllocateZeroPages() for this.
Why did you decide against it?

CpuS3DataDxe is a DXE_DRIVER, and the matching MemoryAllocationLib
instance would allocate Boot Services Data type memory.

>  /**
>    Callback function executed when the EndOfDxe event group is signaled.
>  
> @@ -171,10 +203,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 = AllocateBootServiceMemory (sizeof (ACPI_CPU_DATA_EX));
>    ASSERT (AcpiCpuDataEx != NULL);
>    AcpiCpuData = &AcpiCpuDataEx->AcpiCpuData;
>  
> @@ -223,11 +252,11 @@ CpuS3DataInitialize (
>    AsmReadIdtr (&AcpiCpuDataEx->IdtrProfile);
>  

(2) In the previous patch, we lifted the 4GB limitation on the stack
address (while preserving the memory type restriction as AcpiNVS).
However, you continue to allocate the stack with
AllocateAcpiNvsMemoryBelow4G().

I don't think that's consistent with the purpose of this patch set, or
with the documentation change in the previous patch. We should allocate
the stack as AcpiNVS without address limitation.

And then we can remove the AllocateAcpiNvsMemoryBelow4G() function
altogether.

>    //
> -  // 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 = AllocateBootServiceMemory (GdtSize + IdtSize);
>    ASSERT (Gdt != NULL);
>    Idt = (VOID *)((UINTN)Gdt + GdtSize);
>    CopyMem (Gdt, (VOID *)AcpiCpuDataEx->GdtrProfile.Base, GdtSize);
> @@ -243,7 +272,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 *)AllocateBootServiceMemory (TableSize);
>      ASSERT (RegisterTable != NULL);
>  
>      for (Index = 0; Index < NumberOfCpus; Index++) {
> 

Thanks,
Laszlo

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

[Laszlo Ersek]

On 08/10/18 18:12, Laszlo Ersek wrote:
> On 08/10/18 06:19, Eric Dong wrote:
>> Because CpuS3Data memory will be copy to smram at SmmReadyToLock point,
>> the memory type no need to be ACPI NVS type, also the address not
>> limit to below 4G.
>>
>> This change remove the limit of ACPI NVS memory type and below 4G.
>>
>> Pass OS boot and resume from S3 test.

[snip]

> (2) In the previous patch, we lifted the 4GB limitation on the stack
> address (while preserving the memory type restriction as AcpiNVS).
> However, you continue to allocate the stack with
> AllocateAcpiNvsMemoryBelow4G().
> 
> I don't think that's consistent with the purpose of this patch set, or
> with the documentation change in the previous patch. We should allocate
> the stack as AcpiNVS without address limitation.
> 
> And then we can remove the AllocateAcpiNvsMemoryBelow4G() function
> altogether.

Nevermind, I'm just seeing the next patch.

(You might want to add a hint about the next patch to the commit message
of this patch -- "we'll handle the stack in the next patch".)

So only my question (1) remains for this patch, i.e. why not use
AllocateZeroPages().

Thanks,
Laszlo

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

[Laszlo Ersek]

On 08/10/18 06:19, Eric Dong wrote:
> 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>
> ---
>  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 5b99a6e759..d18f33a5b8 100644
> --- a/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
> +++ b/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
> @@ -46,9 +46,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.
>  
> @@ -56,7 +54,7 @@ typedef struct {
>  
>  **/
>  VOID *
> -AllocateAcpiNvsMemoryBelow4G (
> +AllocateAcpiNvsMemory (
>    IN UINTN  Size
>    )
>  {
> @@ -64,9 +62,8 @@ AllocateAcpiNvsMemoryBelow4G (
>    EFI_STATUS            Status;
>    VOID                  *Buffer;
>  
> -  Address = BASE_4GB - 1;
>    Status  = gBS->AllocatePages (
> -                   AllocateMaxAddress,
> +                   AllocateAnyPages,
>                     EfiACPIMemoryNVS,
>                     EFI_SIZE_TO_PAGES (Size),
>                     &Address
> @@ -239,9 +236,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 PiSmmCpuDxeSemm.

Please fix the typo: "PiSmmCpuDxeSemm" --> "PiSmmCpuDxeSmm".

With that:

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

Thanks
Laszlo

>    //
> -  Stack = AllocateAcpiNvsMemoryBelow4G (NumberOfCpus * AcpiCpuData->StackSize);
> +  Stack = AllocateAcpiNvsMemory (NumberOfCpus * AcpiCpuData->StackSize);
>    ASSERT (Stack != NULL);
>    AcpiCpuData->StackAddress = (EFI_PHYSICAL_ADDRESS)(UINTN)Stack;
>  
> 

Re: [Patch v3 5/5] UefiCpuPkg/RegisterCpuFeaturesLib: Combine implementation.

[Laszlo Ersek]

On 08/10/18 06:19, Eric Dong wrote:
> V1 changes:
>> Current code logic can't confirm CpuS3DataDxe driver start before
>> CpuFeaturesDxe driver. So the assumption in CpuFeaturesDxe not valid.
>> Add implementation for AllocateAcpiCpuData function to remove this
>> assumption.
> 
> V2 changes:
>> Because CpuS3Data memory will be copy to smram at SmmReadToLock point,
>> so the memory type no need to be ACPI NVS type, also the address not
>> limit to below 4G.
>> This change remove the limit of ACPI NVS memory type and below 4G.
> 
> 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>
> ---
>  .../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.
>  
> 

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

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

[Laszlo Ersek]

Eric,

On 08/10/18 06:19, 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                |  60 +++++++---
>  UefiCpuPkg/Include/AcpiCpuData.h                   |  34 ++----
>  .../DxeRegisterCpuFeaturesLib.c                    |  67 -----------
>  .../PeiRegisterCpuFeaturesLib.c                    | 131 ---------------------
>  .../RegisterCpuFeaturesLib/RegisterCpuFeatures.h   |  20 ----
>  .../RegisterCpuFeaturesLib.c                       |  92 +++++++++++++++
>  UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c                  |  11 +-
>  7 files changed, 153 insertions(+), 262 deletions(-)
> 

Because patches #1 and #3 could still be updated, I'm not going to test
the series at this time. If you decide to replace the global variables
from patch #1 with local variables, using an incremental change, *and*
for patch #3, AllocateZeroPages() does not prove useful, then I'll test
this version. Otherwise I'd like to test the next version (v4).

Thanks!
Laszlo

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

[Dong, Eric]

Hi Laszlo,

I checked the code base and don't find the API AllocateZeroPages (), and I agree to Zero the memory before using it. So I think update the original function is the best way now.

Thanks,
Eric

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

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

[Dong, Eric]

Hi Laszlo,

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Laszlo Ersek
> Sent: Friday, August 10, 2018 11:40 PM
> To: Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>
> Subject: Re: [edk2] [Patch v3 1/5] UefiCpuPkg/PiSmmCpuDxeSmm: Use
> GDT/IDT saved in Smram.
> 
> On 08/10/18 06:19, Eric Dong wrote:
> > Current implementation copies GDT/IDT at SmmReadyToLock point from
> > ACPI NVS memory to Smram. Later at S3 resume phase, it restores memory
> > saved in Smram to ACPI NVS. Driver can directly use GDT/IDT saved in
> > Smram instead of restore the original ACPI NVS memory.
> >
> > This patch do this change.
> >
> > 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 | 11 ++++-------
> >  1 file changed, 4 insertions(+), 7 deletions(-)
> >
> > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> > b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> > index 0b8ef70359..764d8276d3 100644
> > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> > @@ -448,13 +448,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; @@ -901,6
> > +894,10 @@ GetAcpiCpuData (
> >    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)mGdtForAp;
> > +  Idtr->Base = (UINTN)mIdtForAp;
> > +  mAcpiCpuData.ApMachineCheckHandlerBase =
> > + (EFI_PHYSICAL_ADDRESS)(UINTN)mMachineCheckHandlerForAp;
> >  }
> >
> >  /**
> >
> 
> This patch looks good to me (I'm ready to give R-b), but I think we can simplify
> the code further. Can we replace the mGdtForAp, mIdtForAp and
> mMachineCheckHandlerForAp global variables, with GdtForAp, IdtForAp and
> MachineCheckHandlerForAp local variables, used only in the
> GetAcpiCpuData() function?
>

Agree to do this change in my next version change. Will send new patches after finishing AllocateZeroPages related discussion.

> If you prefer to do that as an incremental patch, that's fine too.
> 
> Thanks,
> Laszlo
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

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

[Ni, Ruiyu]

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

So the AllocateBootServiceMemory() is not needed.

Thanks/Ray

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

Re: [Patch v3 5/5] UefiCpuPkg/RegisterCpuFeaturesLib: Combine implementation.

[Ni, Ruiyu]

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

Thanks/Ray

> -----Original Message-----
> From: Dong, Eric
> Sent: Friday, August 10, 2018 12:19 PM
> 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>; Ni, Ruiyu
> <ruiyu.ni@intel.com>
> Subject: [Patch v3 5/5] UefiCpuPkg/RegisterCpuFeaturesLib: Combine
> implementation.
> 
> 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>
> ---
>  .../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/DxeRegisterCpuFeaturesLi
> +++ b.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/PeiRegisterCpuFeaturesLi
> +++ b.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 v3 4/5] UefiCpuPkg/CpuS3DataDxe: Remove below 4G limitation.

[Ni, Ruiyu]

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

Thanks/Ray

> -----Original Message-----
> From: Dong, Eric
> Sent: Friday, August 10, 2018 12:19 PM
> To: edk2-devel@lists.01.org
> Cc: Laszlo Ersek <lersek@redhat.com>; Ni, Ruiyu <ruiyu.ni@intel.com>
> Subject: [Patch v3 4/5] UefiCpuPkg/CpuS3DataDxe: Remove below 4G
> limitation.
> 
> 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>
> ---
>  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 5b99a6e759..d18f33a5b8 100644
> --- a/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
> +++ b/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
> @@ -46,9 +46,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.
> 
> @@ -56,7 +54,7 @@ typedef struct {
> 
>  **/
>  VOID *
> -AllocateAcpiNvsMemoryBelow4G (
> +AllocateAcpiNvsMemory (
>    IN UINTN  Size
>    )
>  {
> @@ -64,9 +62,8 @@ AllocateAcpiNvsMemoryBelow4G (
>    EFI_STATUS            Status;
>    VOID                  *Buffer;
> 
> -  Address = BASE_4GB - 1;
>    Status  = gBS->AllocatePages (
> -                   AllocateMaxAddress,
> +                   AllocateAnyPages,
>                     EfiACPIMemoryNVS,
>                     EFI_SIZE_TO_PAGES (Size),
>                     &Address
> @@ -239,9 +236,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 PiSmmCpuDxeSemm.
>    //
> -  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

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

[Ni, Ruiyu]

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

Thanks/Ray

> -----Original Message-----
> From: Dong, Eric
> Sent: Friday, August 10, 2018 12:19 PM
> 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>; Ni, Ruiyu
> <ruiyu.ni@intel.com>
> Subject: [Patch v3 2/5] UefiCpuPkg/AcpiCpuData.h: Remove AcpiNVS and
> Below 4G limitation.
> 
> 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>
> ---
>  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

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

[Ni, Ruiyu]

Thanks/Ray

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Friday, August 10, 2018 11:40 PM
> To: Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>
> Subject: Re: [Patch v3 1/5] UefiCpuPkg/PiSmmCpuDxeSmm: Use GDT/IDT
> saved in Smram.
> 
> On 08/10/18 06:19, Eric Dong wrote:
> > Current implementation copies GDT/IDT at SmmReadyToLock point from
> > ACPI NVS memory to Smram. Later at S3 resume phase, it restores memory
> > saved in Smram to ACPI NVS. Driver can directly use GDT/IDT saved in
> > Smram instead of restore the original ACPI NVS memory.
> >
> > This patch do this change.
> >
> > 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 | 11 ++++-------
> >  1 file changed, 4 insertions(+), 7 deletions(-)
> >
> > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> > b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> > index 0b8ef70359..764d8276d3 100644
> > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> > @@ -448,13 +448,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; @@ -901,6
> > +894,10 @@ GetAcpiCpuData (
> >    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)mGdtForAp;
> > +  Idtr->Base = (UINTN)mIdtForAp;
> > +  mAcpiCpuData.ApMachineCheckHandlerBase =
> > + (EFI_PHYSICAL_ADDRESS)(UINTN)mMachineCheckHandlerForAp;
> >  }
> >
> >  /**
> >
> 
> This patch looks good to me (I'm ready to give R-b), but I think we can
> simplify the code further. Can we replace the mGdtForAp, mIdtForAp and
> mMachineCheckHandlerForAp global variables, with GdtForAp, IdtForAp and
> MachineCheckHandlerForAp local variables, used only in the
> GetAcpiCpuData() function?
Great code cleanup to remove 3 global variables.
> 
> If you prefer to do that as an incremental patch, that's fine too.
> 
> Thanks,
> Laszlo

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

[Ni, Ruiyu]

Ignore my previous mail.

You could use AllocatePages() instead of directly calling gBS->AllocatePages().
Then rename the AllocateBootServiceMemory() to AllocateZeroPages() to better
reflect the function behavior.

Thanks/Ray

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

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

[Laszlo Ersek]

On 08/13/18 07:52, Ni, Ruiyu wrote:
> Ignore my previous mail.
> 
> You could use AllocatePages() instead of directly calling gBS->AllocatePages().
> Then rename the AllocateBootServiceMemory() to AllocateZeroPages() to better
> reflect the function behavior.
> 
> Thanks/Ray
> 
>> -----Original Message-----
>> From: edk2-devel <edk2-devel-bounces@lists.01.org> On Behalf Of Ni, Ruiyu
>> Sent: Monday, August 13, 2018 1:39 PM
>> To: Dong, Eric <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>;
>> edk2-devel@lists.01.org
>> Subject: Re: [edk2] [Patch v3 3/5] UefiCpuPkg/CpuS3DataDxe: Change
>> Memory Type and address limitation.
>>
>> Eric,
>> I think you could change:
>> AcpiCpuDataEx = AllocateBootServiceMemory (sizeof
>> (ACPI_CPU_DATA_EX));
>> ->
>> AcpiCpuDataEx = AllocatePages (EFI_SIZE_TO_PAGES (sizeof
>> (ACPI_CPU_DATA_EX)));
>>
>> So the AllocateBootServiceMemory() is not needed.
>>
>> Thanks/Ray
>>
>>> -----Original Message-----
>>> From: Dong, Eric
>>> Sent: Monday, August 13, 2018 9:51 AM
>>> To: Laszlo Ersek <lersek@redhat.com>; edk2-devel@lists.01.org
>>> Cc: Marvin Häuser <Marvin.Haeuser@outlook.com>; Fan Jeff
>>> <vanjeff_919@hotmail.com>; Ni, Ruiyu <ruiyu.ni@intel.com>
>>> Subject: RE: [Patch v3 3/5] UefiCpuPkg/CpuS3DataDxe: Change Memory
>>> Type and address limitation.
>>>
>>> Hi Laszlo,
>>>
>>> I checked the code base and don't find the API AllocateZeroPages (),

Sorry, I was misled by AllocateZeroPages() in
[IntelSiliconPkg/Feature/VTd/IntelVTdDxe/TranslationTable.c].

>>> and I agree to Zero the memory before using it. So I think update the
>>> original function is the best way now.

If you wouldn't like to do what Ray suggests, or you would like to do it
later / separately:

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

because both of my earlier questions for this patch have been explained.

Thanks
Laszlo

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

[Laszlo Ersek]

On 08/13/18 03:54, Dong, Eric wrote:
> Hi Laszlo,
> 
>> -----Original Message-----
>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
>> Laszlo Ersek
>> Sent: Friday, August 10, 2018 11:40 PM
>> To: Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org
>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>
>> Subject: Re: [edk2] [Patch v3 1/5] UefiCpuPkg/PiSmmCpuDxeSmm: Use
>> GDT/IDT saved in Smram.
>>
>> On 08/10/18 06:19, Eric Dong wrote:
>>> Current implementation copies GDT/IDT at SmmReadyToLock point from
>>> ACPI NVS memory to Smram. Later at S3 resume phase, it restores memory
>>> saved in Smram to ACPI NVS. Driver can directly use GDT/IDT saved in
>>> Smram instead of restore the original ACPI NVS memory.
>>>
>>> This patch do this change.
>>>
>>> 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 | 11 ++++-------
>>>  1 file changed, 4 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
>>> b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
>>> index 0b8ef70359..764d8276d3 100644
>>> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
>>> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
>>> @@ -448,13 +448,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; @@ -901,6
>>> +894,10 @@ GetAcpiCpuData (
>>>    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)mGdtForAp;
>>> +  Idtr->Base = (UINTN)mIdtForAp;
>>> +  mAcpiCpuData.ApMachineCheckHandlerBase =
>>> + (EFI_PHYSICAL_ADDRESS)(UINTN)mMachineCheckHandlerForAp;
>>>  }
>>>
>>>  /**
>>>
>>
>> This patch looks good to me (I'm ready to give R-b), but I think we can simplify
>> the code further. Can we replace the mGdtForAp, mIdtForAp and
>> mMachineCheckHandlerForAp global variables, with GdtForAp, IdtForAp and
>> MachineCheckHandlerForAp local variables, used only in the
>> GetAcpiCpuData() function?
>>
> 
> Agree to do this change in my next version change. Will send new patches after finishing AllocateZeroPages related discussion.

OK, cool, thanks! I will test that version then.

Laszlo

> 
>> If you prefer to do that as an incremental patch, that's fine too.
>>
>> Thanks,
>> Laszlo
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel