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

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

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