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

Eric Dong posted 5 patches 5 years, 10 months ago
There is a newer version of this series
[edk2] [Patch v3 2/5] UefiCpuPkg/AcpiCpuData.h: Remove AcpiNVS and Below 4G limitation.
Posted by Eric Dong 5 years, 10 months ago
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

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [Patch v3 2/5] UefiCpuPkg/AcpiCpuData.h: Remove AcpiNVS and Below 4G limitation.
Posted by Ni, Ruiyu 5 years, 10 months ago
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

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [Patch v3 2/5] UefiCpuPkg/AcpiCpuData.h: Remove AcpiNVS and Below 4G limitation.
Posted by Laszlo Ersek 5 years, 10 months ago
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>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel