[PATCH 0/3] UefiCpuPkg, OvmfPkg: do not allocate useless register tables for S3 resume

[PATCH 0/3] UefiCpuPkg, OvmfPkg: do not allocate useless register tables for S3 resume

[Laszlo Ersek]

Ref:    https://bugzilla.tianocore.org/show_bug.cgi?id=3159
Repo:   https://pagure.io/lersek/edk2.git
Branch: remove-cpu-reg-table-alloc-3159

OvmfPkg/CpuS3DataDxe and UefiCpuPkg/CpuS3DataDxe allocate the
RegisterTable and PreSmmInitRegisterTable arrays in ACPI_CPU_DATA just
so every processor in the system can have its own empty register table,
matched by APIC ID, at S3 resume. This has never been useful.

Given recent commit e992cc3f4859 ("UefiCpuPkg PiSmmCpuDxeSmm: Reduce
SMRAM consumption in CpuS3.c", 2021-01-11), i.e., the NULL-check in the
IsRegisterTableEmpty() function, we should now set both RegisterTable
and PreSmmInitRegisterTable in ACPI_CPU_DATA to NULL, in the
CpuS3DataDxe drivers. That will simplify the drivers, save some normal
RAM in them, and accordingly, save some SMRAM in PiSmmCpuDxeSmm too.

The first patch in the series also updates the documentation of
ACPI_CPU_DATA (which, arguably, should have been done in commit
e992cc3f4859).

Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>

Thanks
Laszlo

Laszlo Ersek (3):
  UefiCpuPkg/AcpiCpuData: update comments on register table fields
  UefiCpuPkg/CpuS3DataDxe: do not allocate useless register tables
  OvmfPkg/CpuS3DataDxe: do not allocate useless register tables

 OvmfPkg/CpuS3DataDxe/CpuS3Data.c      | 70 +-------------------
 OvmfPkg/CpuS3DataDxe/CpuS3DataDxe.inf |  1 -
 UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c   | 32 ---------
 UefiCpuPkg/Include/AcpiCpuData.h      |  4 ++
 4 files changed, 5 insertions(+), 102 deletions(-)


base-commit: ebfe2d3eb5ac7fd92d74011edb31303a181920c7
-- 
2.19.1.3.g30247aa5d201

[PATCH 1/3] UefiCpuPkg/AcpiCpuData: update comments on register table fields

[Laszlo Ersek]

After commit e992cc3f4859 ("UefiCpuPkg PiSmmCpuDxeSmm: Reduce SMRAM
consumption in CpuS3.c", 2021-01-11), it is valid for a CPU S3 Data DXE
Driver to set "ACPI_CPU_DATA.PreSmmInitRegisterTable" and/or
"ACPI_CPU_DATA.RegisterTable" to 0, in case none of the CPUs needs a
register table of the corresponding kind, during S3 resume.

Document this fact in the "UefiCpuPkg/Include/AcpiCpuData.h" header file.

Cc: Eric Dong <eric.dong@intel.com>
Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3159
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 UefiCpuPkg/Include/AcpiCpuData.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/UefiCpuPkg/Include/AcpiCpuData.h b/UefiCpuPkg/Include/AcpiCpuData.h
index b5a69ad80c88..62a01b2c6bac 100644
--- a/UefiCpuPkg/Include/AcpiCpuData.h
+++ b/UefiCpuPkg/Include/AcpiCpuData.h
@@ -178,6 +178,8 @@ typedef struct {
   // If TableLength is > 0, then elements of RegisterTableEntry are used to
   // initialize the CPU that matches InitialApicId, during an ACPI S3 resume,
   // before SMBASE relocation is performed.
+  // If a register table is not required for any one of the CPUs, then
+  // PreSmmInitRegisterTable may be set to 0.
   //
   EFI_PHYSICAL_ADDRESS  PreSmmInitRegisterTable;
   //
@@ -187,6 +189,8 @@ typedef struct {
   // If TableLength is > 0, then elements of RegisterTableEntry are used to
   // initialize the CPU that matches InitialApicId, during an ACPI S3 resume,
   // after SMBASE relocation is performed.
+  // If a register table is not required for any one of the CPUs, then
+  // RegisterTable may be set to 0.
   //
   EFI_PHYSICAL_ADDRESS  RegisterTable;
   //
-- 
2.19.1.3.g30247aa5d201

Re: [PATCH 1/3] UefiCpuPkg/AcpiCpuData: update comments on register table fields

[Ni, Ray]

Reviewed-by: Ray Ni <ray.ni@intel.com>

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Wednesday, January 13, 2021 3:20 AM
> To: devel@edk2.groups.io
> Cc: Dong, Eric <eric.dong@intel.com>; Philippe Mathieu-Daudé
> <philmd@redhat.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>; Ni, Ray
> <ray.ni@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: [PATCH 1/3] UefiCpuPkg/AcpiCpuData: update comments on register
> table fields
> 
> After commit e992cc3f4859 ("UefiCpuPkg PiSmmCpuDxeSmm: Reduce SMRAM
> consumption in CpuS3.c", 2021-01-11), it is valid for a CPU S3 Data DXE
> Driver to set "ACPI_CPU_DATA.PreSmmInitRegisterTable" and/or
> "ACPI_CPU_DATA.RegisterTable" to 0, in case none of the CPUs needs a
> register table of the corresponding kind, during S3 resume.
> 
> Document this fact in the "UefiCpuPkg/Include/AcpiCpuData.h" header file.
> 
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3159
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  UefiCpuPkg/Include/AcpiCpuData.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/UefiCpuPkg/Include/AcpiCpuData.h
> b/UefiCpuPkg/Include/AcpiCpuData.h
> index b5a69ad80c88..62a01b2c6bac 100644
> --- a/UefiCpuPkg/Include/AcpiCpuData.h
> +++ b/UefiCpuPkg/Include/AcpiCpuData.h
> @@ -178,6 +178,8 @@ typedef struct {
>    // If TableLength is > 0, then elements of RegisterTableEntry are used to
>    // initialize the CPU that matches InitialApicId, during an ACPI S3 resume,
>    // before SMBASE relocation is performed.
> +  // If a register table is not required for any one of the CPUs, then
> +  // PreSmmInitRegisterTable may be set to 0.
>    //
>    EFI_PHYSICAL_ADDRESS  PreSmmInitRegisterTable;
>    //
> @@ -187,6 +189,8 @@ typedef struct {
>    // If TableLength is > 0, then elements of RegisterTableEntry are used to
>    // initialize the CPU that matches InitialApicId, during an ACPI S3 resume,
>    // after SMBASE relocation is performed.
> +  // If a register table is not required for any one of the CPUs, then
> +  // RegisterTable may be set to 0.
>    //
>    EFI_PHYSICAL_ADDRESS  RegisterTable;
>    //
> --
> 2.19.1.3.g30247aa5d201
> 

Re: [PATCH 1/3] UefiCpuPkg/AcpiCpuData: update comments on register table fields

[Philippe Mathieu-Daudé]

On 1/12/21 8:19 PM, Laszlo Ersek wrote:
> After commit e992cc3f4859 ("UefiCpuPkg PiSmmCpuDxeSmm: Reduce SMRAM
> consumption in CpuS3.c", 2021-01-11), it is valid for a CPU S3 Data DXE
> Driver to set "ACPI_CPU_DATA.PreSmmInitRegisterTable" and/or
> "ACPI_CPU_DATA.RegisterTable" to 0, in case none of the CPUs needs a
> register table of the corresponding kind, during S3 resume.
> 
> Document this fact in the "UefiCpuPkg/Include/AcpiCpuData.h" header file.
> 
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3159
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  UefiCpuPkg/Include/AcpiCpuData.h | 4 ++++
>  1 file changed, 4 insertions(+)

Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>

Re: [PATCH 1/3] UefiCpuPkg/AcpiCpuData: update comments on register table fields

[Laszlo Ersek]

On 01/15/21 10:28, Philippe Mathieu-Daudé wrote:
> On 1/12/21 8:19 PM, Laszlo Ersek wrote:
>> After commit e992cc3f4859 ("UefiCpuPkg PiSmmCpuDxeSmm: Reduce SMRAM
>> consumption in CpuS3.c", 2021-01-11), it is valid for a CPU S3 Data DXE
>> Driver to set "ACPI_CPU_DATA.PreSmmInitRegisterTable" and/or
>> "ACPI_CPU_DATA.RegisterTable" to 0, in case none of the CPUs needs a
>> register table of the corresponding kind, during S3 resume.
>>
>> Document this fact in the "UefiCpuPkg/Include/AcpiCpuData.h" header file.
>>
>> Cc: Eric Dong <eric.dong@intel.com>
>> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
>> Cc: Rahul Kumar <rahul1.kumar@intel.com>
>> Cc: Ray Ni <ray.ni@intel.com>
>> Cc: Star Zeng <star.zeng@intel.com>
>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3159
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>>  UefiCpuPkg/Include/AcpiCpuData.h | 4 ++++
>>  1 file changed, 4 insertions(+)
> 
> Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>
> 

Thank you!
Laszlo

[PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate useless register tables

[Laszlo Ersek]

CpuS3DataDxe allocates the "RegisterTable" and "PreSmmInitRegisterTable"
arrays in ACPI_CPU_DATA just so every processor in the system can have its
own empty register table, matched by APIC ID. This has never been useful
in practice.

Given commit e992cc3f4859 ("UefiCpuPkg PiSmmCpuDxeSmm: Reduce SMRAM
consumption in CpuS3.c", 2021-01-11), simply leave both
"AcpiCpuData->RegisterTable" and "AcpiCpuData->PreSmmInitRegisterTable"
initialized to the zero address. This simplifies the driver, and saves
both normal RAM (boot services data type memory) and -- in PiSmmCpuDxeSmm
-- SMRAM.

Cc: Eric Dong <eric.dong@intel.com>
Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3159
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---

Notes:
    Tested by temporarily replacing OvmfPkgPkg/CpuS3DataDxe in the OVMF IA32
    and IA32X64 platforms with this driver -- this driver works OK in OVMF
    as long as no CPUs are hot-plugged.

 UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c | 32 --------------------
 1 file changed, 32 deletions(-)

diff --git a/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c b/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
index 2be335d91903..078af36cfb41 100644
--- a/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
+++ b/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
@@ -165,10 +165,6 @@ CpuS3DataInitialize (
   UINTN                      NumberOfCpus;
   UINTN                      NumberOfEnabledProcessors;
   VOID                       *Stack;
-  UINTN                      TableSize;
-  CPU_REGISTER_TABLE         *RegisterTable;
-  UINTN                      Index;
-  EFI_PROCESSOR_INFORMATION  ProcessorInfoBuffer;
   UINTN                      GdtSize;
   UINTN                      IdtSize;
   VOID                       *Gdt;
@@ -255,34 +251,6 @@ CpuS3DataInitialize (
     AcpiCpuData->PreSmmInitRegisterTable = OldAcpiCpuData->PreSmmInitRegisterTable;
     AcpiCpuData->ApLocation              = OldAcpiCpuData->ApLocation;
     CopyMem (&AcpiCpuData->CpuStatus, &OldAcpiCpuData->CpuStatus, sizeof (CPU_STATUS_INFORMATION));
-  } else {
-    //
-    // Allocate buffer for empty RegisterTable and PreSmmInitRegisterTable for all CPUs
-    //
-    TableSize = 2 * NumberOfCpus * sizeof (CPU_REGISTER_TABLE);
-    RegisterTable = (CPU_REGISTER_TABLE *)AllocateZeroPages (TableSize);
-    ASSERT (RegisterTable != NULL);
-
-    for (Index = 0; Index < NumberOfCpus; Index++) {
-      Status = MpServices->GetProcessorInfo (
-                           MpServices,
-                           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);
   }
 
   //
-- 
2.19.1.3.g30247aa5d201

Re: [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate useless register tables

[Ni, Ray]

Reviewed-by: Ray Ni <ray.ni@intel.com>

(I guess you don't want to put Redhat copyright in the patch 1&2 so the copyright year is not updated.
Since it's a simple change that make the logic more neat, I am ok with that.)

Will you create a pull request to merge all 3 patches together?

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Wednesday, January 13, 2021 3:20 AM
> To: devel@edk2.groups.io
> Cc: Dong, Eric <eric.dong@intel.com>; Philippe Mathieu-Daudé
> <philmd@redhat.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>; Ni, Ray
> <ray.ni@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate useless
> register tables
> 
> CpuS3DataDxe allocates the "RegisterTable" and "PreSmmInitRegisterTable"
> arrays in ACPI_CPU_DATA just so every processor in the system can have its
> own empty register table, matched by APIC ID. This has never been useful
> in practice.
> 
> Given commit e992cc3f4859 ("UefiCpuPkg PiSmmCpuDxeSmm: Reduce
> SMRAM
> consumption in CpuS3.c", 2021-01-11), simply leave both
> "AcpiCpuData->RegisterTable" and "AcpiCpuData->PreSmmInitRegisterTable"
> initialized to the zero address. This simplifies the driver, and saves
> both normal RAM (boot services data type memory) and -- in
> PiSmmCpuDxeSmm
> -- SMRAM.
> 
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3159
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
> 
> Notes:
>     Tested by temporarily replacing OvmfPkgPkg/CpuS3DataDxe in the OVMF
> IA32
>     and IA32X64 platforms with this driver -- this driver works OK in OVMF
>     as long as no CPUs are hot-plugged.
> 
>  UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c | 32 --------------------
>  1 file changed, 32 deletions(-)
> 
> diff --git a/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
> b/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
> index 2be335d91903..078af36cfb41 100644
> --- a/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
> +++ b/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
> @@ -165,10 +165,6 @@ CpuS3DataInitialize (
>    UINTN                      NumberOfCpus;
>    UINTN                      NumberOfEnabledProcessors;
>    VOID                       *Stack;
> -  UINTN                      TableSize;
> -  CPU_REGISTER_TABLE         *RegisterTable;
> -  UINTN                      Index;
> -  EFI_PROCESSOR_INFORMATION  ProcessorInfoBuffer;
>    UINTN                      GdtSize;
>    UINTN                      IdtSize;
>    VOID                       *Gdt;
> @@ -255,34 +251,6 @@ CpuS3DataInitialize (
>      AcpiCpuData->PreSmmInitRegisterTable = OldAcpiCpuData-
> >PreSmmInitRegisterTable;
>      AcpiCpuData->ApLocation              = OldAcpiCpuData->ApLocation;
>      CopyMem (&AcpiCpuData->CpuStatus, &OldAcpiCpuData->CpuStatus, sizeof
> (CPU_STATUS_INFORMATION));
> -  } else {
> -    //
> -    // Allocate buffer for empty RegisterTable and PreSmmInitRegisterTable for
> all CPUs
> -    //
> -    TableSize = 2 * NumberOfCpus * sizeof (CPU_REGISTER_TABLE);
> -    RegisterTable = (CPU_REGISTER_TABLE *)AllocateZeroPages (TableSize);
> -    ASSERT (RegisterTable != NULL);
> -
> -    for (Index = 0; Index < NumberOfCpus; Index++) {
> -      Status = MpServices->GetProcessorInfo (
> -                           MpServices,
> -                           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);
>    }
> 
>    //
> --
> 2.19.1.3.g30247aa5d201
> 

Re: [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate useless register tables

[Zeng, Star]

DxeRegisterCpuFeaturesLib still has some execution sequence dependency to UefiCpuPkg CpuS3DataDxe.
There is ASSERT to happen at  https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c#L1059 when CpuS3DataDxe with this patch runs before DxeRegisterCpuFeaturesLib.

Thanks,
Star
-----Original Message-----
From: Ni, Ray <ray.ni@intel.com> 
Sent: Wednesday, January 13, 2021 2:12 PM
To: Laszlo Ersek <lersek@redhat.com>; devel@edk2.groups.io
Cc: Dong, Eric <eric.dong@intel.com>; Philippe Mathieu-Daudé <philmd@redhat.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>; Zeng, Star <star.zeng@intel.com>
Subject: RE: [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate useless register tables

Reviewed-by: Ray Ni <ray.ni@intel.com>

(I guess you don't want to put Redhat copyright in the patch 1&2 so the copyright year is not updated.
Since it's a simple change that make the logic more neat, I am ok with that.)

Will you create a pull request to merge all 3 patches together?

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Wednesday, January 13, 2021 3:20 AM
> To: devel@edk2.groups.io
> Cc: Dong, Eric <eric.dong@intel.com>; Philippe Mathieu-Daudé 
> <philmd@redhat.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>; Ni, Ray 
> <ray.ni@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate useless 
> register tables
> 
> CpuS3DataDxe allocates the "RegisterTable" and "PreSmmInitRegisterTable"
> arrays in ACPI_CPU_DATA just so every processor in the system can have 
> its own empty register table, matched by APIC ID. This has never been 
> useful in practice.
> 
> Given commit e992cc3f4859 ("UefiCpuPkg PiSmmCpuDxeSmm: Reduce SMRAM 
> consumption in CpuS3.c", 2021-01-11), simply leave both 
> "AcpiCpuData->RegisterTable" and "AcpiCpuData->PreSmmInitRegisterTable"
> initialized to the zero address. This simplifies the driver, and saves 
> both normal RAM (boot services data type memory) and -- in 
> PiSmmCpuDxeSmm
> -- SMRAM.
> 
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3159
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
> 
> Notes:
>     Tested by temporarily replacing OvmfPkgPkg/CpuS3DataDxe in the 
> OVMF
> IA32
>     and IA32X64 platforms with this driver -- this driver works OK in OVMF
>     as long as no CPUs are hot-plugged.
> 
>  UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c | 32 --------------------
>  1 file changed, 32 deletions(-)
> 
> diff --git a/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
> b/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
> index 2be335d91903..078af36cfb41 100644
> --- a/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
> +++ b/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
> @@ -165,10 +165,6 @@ CpuS3DataInitialize (
>    UINTN                      NumberOfCpus;
>    UINTN                      NumberOfEnabledProcessors;
>    VOID                       *Stack;
> -  UINTN                      TableSize;
> -  CPU_REGISTER_TABLE         *RegisterTable;
> -  UINTN                      Index;
> -  EFI_PROCESSOR_INFORMATION  ProcessorInfoBuffer;
>    UINTN                      GdtSize;
>    UINTN                      IdtSize;
>    VOID                       *Gdt;
> @@ -255,34 +251,6 @@ CpuS3DataInitialize (
>      AcpiCpuData->PreSmmInitRegisterTable = OldAcpiCpuData-
> >PreSmmInitRegisterTable;
>      AcpiCpuData->ApLocation              = OldAcpiCpuData->ApLocation;
>      CopyMem (&AcpiCpuData->CpuStatus, &OldAcpiCpuData->CpuStatus, 
> sizeof (CPU_STATUS_INFORMATION));
> -  } else {
> -    //
> -    // Allocate buffer for empty RegisterTable and PreSmmInitRegisterTable for
> all CPUs
> -    //
> -    TableSize = 2 * NumberOfCpus * sizeof (CPU_REGISTER_TABLE);
> -    RegisterTable = (CPU_REGISTER_TABLE *)AllocateZeroPages (TableSize);
> -    ASSERT (RegisterTable != NULL);
> -
> -    for (Index = 0; Index < NumberOfCpus; Index++) {
> -      Status = MpServices->GetProcessorInfo (
> -                           MpServices,
> -                           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);
>    }
> 
>    //
> --
> 2.19.1.3.g30247aa5d201
> 

Re: [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate useless register tables

[Ni, Ray]

Star,
Thanks for pointing that.
RegisterCpuFeaturesLib assumes [PreSmmInit]RegisterTable array is allocated but CpuS3Data
doesn't do that.

Can following change fix the problem?

--- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
+++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
@@ -937,21 +937,19 @@ GetAcpiCpuData (
   EFI_PROCESSOR_INFORMATION            ProcessorInfoBuffer;
 
   AcpiCpuData = (ACPI_CPU_DATA *) (UINTN) PcdGet64 (PcdCpuS3DataAddress);
-  if (AcpiCpuData != NULL) {
-    return AcpiCpuData;
-  }
-
-  AcpiCpuData  = AllocatePages (EFI_SIZE_TO_PAGES (sizeof (ACPI_CPU_DATA)));
-  ASSERT (AcpiCpuData != NULL);
+  if (AcpiCpuData == NULL) {
+    AcpiCpuData  = AllocatePages (EFI_SIZE_TO_PAGES (sizeof (ACPI_CPU_DATA)));
+    ASSERT (AcpiCpuData != NULL);
 
-  //
-  // Set PcdCpuS3DataAddress to the base address of the ACPI_CPU_DATA structure
-  //
-  Status = PcdSet64S (PcdCpuS3DataAddress, (UINT64)(UINTN)AcpiCpuData);
-  ASSERT_EFI_ERROR (Status);
+    //
+    // Set PcdCpuS3DataAddress to the base address of the ACPI_CPU_DATA structure
+    //
+    Status = PcdSet64S (PcdCpuS3DataAddress, (UINT64)(UINTN)AcpiCpuData);
+    ASSERT_EFI_ERROR (Status);
 
-  GetNumberOfProcessor (&NumberOfCpus, &NumberOfEnabledProcessors);
-  AcpiCpuData->NumberOfCpus = (UINT32)NumberOfCpus;
+    GetNumberOfProcessor (&NumberOfCpus, &NumberOfEnabledProcessors);
+    AcpiCpuData->NumberOfCpus = (UINT32)NumberOfCpus;
+  }
 
   //
   // Allocate buffer for empty RegisterTable and PreSmmInitRegisterTable for all CPUs

Thanks,
Ray

> -----Original Message-----
> From: Zeng, Star <star.zeng@intel.com>
> Sent: Wednesday, January 13, 2021 3:17 PM
> To: Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>;
> devel@edk2.groups.io
> Cc: Dong, Eric <eric.dong@intel.com>; Philippe Mathieu-Daudé
> <philmd@redhat.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>; Zeng, Star
> <star.zeng@intel.com>
> Subject: RE: [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate useless
> register tables
> 
> DxeRegisterCpuFeaturesLib still has some execution sequence dependency to
> UefiCpuPkg CpuS3DataDxe.
> There is ASSERT to happen at
> https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/Library/RegisterC
> puFeaturesLib/RegisterCpuFeaturesLib.c#L1059 when CpuS3DataDxe with this
> patch runs before DxeRegisterCpuFeaturesLib.
> 
> Thanks,
> Star
> -----Original Message-----
> From: Ni, Ray <ray.ni@intel.com>
> Sent: Wednesday, January 13, 2021 2:12 PM
> To: Laszlo Ersek <lersek@redhat.com>; devel@edk2.groups.io
> Cc: Dong, Eric <eric.dong@intel.com>; Philippe Mathieu-Daudé
> <philmd@redhat.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>; Zeng, Star
> <star.zeng@intel.com>
> Subject: RE: [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate useless
> register tables
> 
> Reviewed-by: Ray Ni <ray.ni@intel.com>
> 
> (I guess you don't want to put Redhat copyright in the patch 1&2 so the
> copyright year is not updated.
> Since it's a simple change that make the logic more neat, I am ok with that.)
> 
> Will you create a pull request to merge all 3 patches together?
> 
> > -----Original Message-----
> > From: Laszlo Ersek <lersek@redhat.com>
> > Sent: Wednesday, January 13, 2021 3:20 AM
> > To: devel@edk2.groups.io
> > Cc: Dong, Eric <eric.dong@intel.com>; Philippe Mathieu-Daudé
> > <philmd@redhat.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>; Ni, Ray
> > <ray.ni@intel.com>; Zeng, Star <star.zeng@intel.com>
> > Subject: [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate useless
> > register tables
> >
> > CpuS3DataDxe allocates the "RegisterTable" and "PreSmmInitRegisterTable"
> > arrays in ACPI_CPU_DATA just so every processor in the system can have
> > its own empty register table, matched by APIC ID. This has never been
> > useful in practice.
> >
> > Given commit e992cc3f4859 ("UefiCpuPkg PiSmmCpuDxeSmm: Reduce
> SMRAM
> > consumption in CpuS3.c", 2021-01-11), simply leave both
> > "AcpiCpuData->RegisterTable" and "AcpiCpuData->PreSmmInitRegisterTable"
> > initialized to the zero address. This simplifies the driver, and saves
> > both normal RAM (boot services data type memory) and -- in
> > PiSmmCpuDxeSmm
> > -- SMRAM.
> >
> > Cc: Eric Dong <eric.dong@intel.com>
> > Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> > Cc: Rahul Kumar <rahul1.kumar@intel.com>
> > Cc: Ray Ni <ray.ni@intel.com>
> > Cc: Star Zeng <star.zeng@intel.com>
> > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3159
> > Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> > ---
> >
> > Notes:
> >     Tested by temporarily replacing OvmfPkgPkg/CpuS3DataDxe in the
> > OVMF
> > IA32
> >     and IA32X64 platforms with this driver -- this driver works OK in OVMF
> >     as long as no CPUs are hot-plugged.
> >
> >  UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c | 32 --------------------
> >  1 file changed, 32 deletions(-)
> >
> > diff --git a/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
> > b/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
> > index 2be335d91903..078af36cfb41 100644
> > --- a/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
> > +++ b/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
> > @@ -165,10 +165,6 @@ CpuS3DataInitialize (
> >    UINTN                      NumberOfCpus;
> >    UINTN                      NumberOfEnabledProcessors;
> >    VOID                       *Stack;
> > -  UINTN                      TableSize;
> > -  CPU_REGISTER_TABLE         *RegisterTable;
> > -  UINTN                      Index;
> > -  EFI_PROCESSOR_INFORMATION  ProcessorInfoBuffer;
> >    UINTN                      GdtSize;
> >    UINTN                      IdtSize;
> >    VOID                       *Gdt;
> > @@ -255,34 +251,6 @@ CpuS3DataInitialize (
> >      AcpiCpuData->PreSmmInitRegisterTable = OldAcpiCpuData-
> > >PreSmmInitRegisterTable;
> >      AcpiCpuData->ApLocation              = OldAcpiCpuData->ApLocation;
> >      CopyMem (&AcpiCpuData->CpuStatus, &OldAcpiCpuData->CpuStatus,
> > sizeof (CPU_STATUS_INFORMATION));
> > -  } else {
> > -    //
> > -    // Allocate buffer for empty RegisterTable and PreSmmInitRegisterTable
> for
> > all CPUs
> > -    //
> > -    TableSize = 2 * NumberOfCpus * sizeof (CPU_REGISTER_TABLE);
> > -    RegisterTable = (CPU_REGISTER_TABLE *)AllocateZeroPages (TableSize);
> > -    ASSERT (RegisterTable != NULL);
> > -
> > -    for (Index = 0; Index < NumberOfCpus; Index++) {
> > -      Status = MpServices->GetProcessorInfo (
> > -                           MpServices,
> > -                           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);
> >    }
> >
> >    //
> > --
> > 2.19.1.3.g30247aa5d201
> >

Re: [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate useless register tables

[Zeng, Star]

It should work.

Thanks,
Star
-----Original Message-----
From: Ni, Ray <ray.ni@intel.com> 
Sent: Wednesday, January 13, 2021 4:12 PM
To: Zeng, Star <star.zeng@intel.com>; Laszlo Ersek <lersek@redhat.com>; devel@edk2.groups.io
Cc: Dong, Eric <eric.dong@intel.com>; Philippe Mathieu-Daudé <philmd@redhat.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>
Subject: RE: [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate useless register tables

Star,
Thanks for pointing that.
RegisterCpuFeaturesLib assumes [PreSmmInit]RegisterTable array is allocated but CpuS3Data doesn't do that.

Can following change fix the problem?

--- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
+++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
@@ -937,21 +937,19 @@ GetAcpiCpuData (
   EFI_PROCESSOR_INFORMATION            ProcessorInfoBuffer;
 
   AcpiCpuData = (ACPI_CPU_DATA *) (UINTN) PcdGet64 (PcdCpuS3DataAddress);
-  if (AcpiCpuData != NULL) {
-    return AcpiCpuData;
-  }
-
-  AcpiCpuData  = AllocatePages (EFI_SIZE_TO_PAGES (sizeof (ACPI_CPU_DATA)));
-  ASSERT (AcpiCpuData != NULL);
+  if (AcpiCpuData == NULL) {
+    AcpiCpuData  = AllocatePages (EFI_SIZE_TO_PAGES (sizeof (ACPI_CPU_DATA)));
+    ASSERT (AcpiCpuData != NULL);
 
-  //
-  // Set PcdCpuS3DataAddress to the base address of the ACPI_CPU_DATA structure
-  //
-  Status = PcdSet64S (PcdCpuS3DataAddress, (UINT64)(UINTN)AcpiCpuData);
-  ASSERT_EFI_ERROR (Status);
+    //
+    // Set PcdCpuS3DataAddress to the base address of the ACPI_CPU_DATA structure
+    //
+    Status = PcdSet64S (PcdCpuS3DataAddress, (UINT64)(UINTN)AcpiCpuData);
+    ASSERT_EFI_ERROR (Status);
 
-  GetNumberOfProcessor (&NumberOfCpus, &NumberOfEnabledProcessors);
-  AcpiCpuData->NumberOfCpus = (UINT32)NumberOfCpus;
+    GetNumberOfProcessor (&NumberOfCpus, &NumberOfEnabledProcessors);
+    AcpiCpuData->NumberOfCpus = (UINT32)NumberOfCpus;  }
 
   //
   // Allocate buffer for empty RegisterTable and PreSmmInitRegisterTable for all CPUs

Thanks,
Ray

> -----Original Message-----
> From: Zeng, Star <star.zeng@intel.com>
> Sent: Wednesday, January 13, 2021 3:17 PM
> To: Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>; 
> devel@edk2.groups.io
> Cc: Dong, Eric <eric.dong@intel.com>; Philippe Mathieu-Daudé 
> <philmd@redhat.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>; Zeng, 
> Star <star.zeng@intel.com>
> Subject: RE: [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate 
> useless register tables
> 
> DxeRegisterCpuFeaturesLib still has some execution sequence dependency 
> to UefiCpuPkg CpuS3DataDxe.
> There is ASSERT to happen at
> https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/Library/Regis
> terC
> puFeaturesLib/RegisterCpuFeaturesLib.c#L1059 when CpuS3DataDxe with 
> this patch runs before DxeRegisterCpuFeaturesLib.
> 
> Thanks,
> Star
> -----Original Message-----
> From: Ni, Ray <ray.ni@intel.com>
> Sent: Wednesday, January 13, 2021 2:12 PM
> To: Laszlo Ersek <lersek@redhat.com>; devel@edk2.groups.io
> Cc: Dong, Eric <eric.dong@intel.com>; Philippe Mathieu-Daudé 
> <philmd@redhat.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>; Zeng, 
> Star <star.zeng@intel.com>
> Subject: RE: [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate 
> useless register tables
> 
> Reviewed-by: Ray Ni <ray.ni@intel.com>
> 
> (I guess you don't want to put Redhat copyright in the patch 1&2 so 
> the copyright year is not updated.
> Since it's a simple change that make the logic more neat, I am ok with 
> that.)
> 
> Will you create a pull request to merge all 3 patches together?
> 
> > -----Original Message-----
> > From: Laszlo Ersek <lersek@redhat.com>
> > Sent: Wednesday, January 13, 2021 3:20 AM
> > To: devel@edk2.groups.io
> > Cc: Dong, Eric <eric.dong@intel.com>; Philippe Mathieu-Daudé 
> > <philmd@redhat.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>; Ni, Ray 
> > <ray.ni@intel.com>; Zeng, Star <star.zeng@intel.com>
> > Subject: [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate 
> > useless register tables
> >
> > CpuS3DataDxe allocates the "RegisterTable" and "PreSmmInitRegisterTable"
> > arrays in ACPI_CPU_DATA just so every processor in the system can 
> > have its own empty register table, matched by APIC ID. This has 
> > never been useful in practice.
> >
> > Given commit e992cc3f4859 ("UefiCpuPkg PiSmmCpuDxeSmm: Reduce
> SMRAM
> > consumption in CpuS3.c", 2021-01-11), simply leave both 
> > "AcpiCpuData->RegisterTable" and "AcpiCpuData->PreSmmInitRegisterTable"
> > initialized to the zero address. This simplifies the driver, and 
> > saves both normal RAM (boot services data type memory) and -- in 
> > PiSmmCpuDxeSmm
> > -- SMRAM.
> >
> > Cc: Eric Dong <eric.dong@intel.com>
> > Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> > Cc: Rahul Kumar <rahul1.kumar@intel.com>
> > Cc: Ray Ni <ray.ni@intel.com>
> > Cc: Star Zeng <star.zeng@intel.com>
> > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3159
> > Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> > ---
> >
> > Notes:
> >     Tested by temporarily replacing OvmfPkgPkg/CpuS3DataDxe in the 
> > OVMF
> > IA32
> >     and IA32X64 platforms with this driver -- this driver works OK in OVMF
> >     as long as no CPUs are hot-plugged.
> >
> >  UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c | 32 --------------------
> >  1 file changed, 32 deletions(-)
> >
> > diff --git a/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
> > b/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
> > index 2be335d91903..078af36cfb41 100644
> > --- a/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
> > +++ b/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
> > @@ -165,10 +165,6 @@ CpuS3DataInitialize (
> >    UINTN                      NumberOfCpus;
> >    UINTN                      NumberOfEnabledProcessors;
> >    VOID                       *Stack;
> > -  UINTN                      TableSize;
> > -  CPU_REGISTER_TABLE         *RegisterTable;
> > -  UINTN                      Index;
> > -  EFI_PROCESSOR_INFORMATION  ProcessorInfoBuffer;
> >    UINTN                      GdtSize;
> >    UINTN                      IdtSize;
> >    VOID                       *Gdt;
> > @@ -255,34 +251,6 @@ CpuS3DataInitialize (
> >      AcpiCpuData->PreSmmInitRegisterTable = OldAcpiCpuData-
> > >PreSmmInitRegisterTable;
> >      AcpiCpuData->ApLocation              = OldAcpiCpuData->ApLocation;
> >      CopyMem (&AcpiCpuData->CpuStatus, &OldAcpiCpuData->CpuStatus, 
> > sizeof (CPU_STATUS_INFORMATION));
> > -  } else {
> > -    //
> > -    // Allocate buffer for empty RegisterTable and PreSmmInitRegisterTable
> for
> > all CPUs
> > -    //
> > -    TableSize = 2 * NumberOfCpus * sizeof (CPU_REGISTER_TABLE);
> > -    RegisterTable = (CPU_REGISTER_TABLE *)AllocateZeroPages (TableSize);
> > -    ASSERT (RegisterTable != NULL);
> > -
> > -    for (Index = 0; Index < NumberOfCpus; Index++) {
> > -      Status = MpServices->GetProcessorInfo (
> > -                           MpServices,
> > -                           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);
> >    }
> >
> >    //
> > --
> > 2.19.1.3.g30247aa5d201
> >

Re: [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate useless register tables

[Laszlo Ersek]

On 01/13/21 09:12, Ni, Ray wrote:
> Star,
> Thanks for pointing that.
> RegisterCpuFeaturesLib assumes [PreSmmInit]RegisterTable array is allocated but CpuS3Data
> doesn't do that.

Sorry that I missed this; I did grep the tree for
[PreSmmInit]RegisterTable, but apparently didn't pay enough attention to
RegisterCpuFeaturesLib. OVMF does not use that library.

Ray: can you please post your fix as a standalone patch?

Or else, if it's more convenient, please just push the fix somewhere (as
a standalone patch) where I can fetch it from. It would be great if you
could write a commit message too.

Then, I will post a v2 of this series, including your fix for
RegisterCpuFeaturesLib (as a patch under your authorship).

Star and Eric can then review your patch on the list -- neither I nor
you can review your patch, as you are the author of it, and I'll be
posting it.

Thanks!
Laszlo

> 
> Can following change fix the problem?
> 
> --- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> @@ -937,21 +937,19 @@ GetAcpiCpuData (
>    EFI_PROCESSOR_INFORMATION            ProcessorInfoBuffer;
>  
>    AcpiCpuData = (ACPI_CPU_DATA *) (UINTN) PcdGet64 (PcdCpuS3DataAddress);
> -  if (AcpiCpuData != NULL) {
> -    return AcpiCpuData;
> -  }
> -
> -  AcpiCpuData  = AllocatePages (EFI_SIZE_TO_PAGES (sizeof (ACPI_CPU_DATA)));
> -  ASSERT (AcpiCpuData != NULL);
> +  if (AcpiCpuData == NULL) {
> +    AcpiCpuData  = AllocatePages (EFI_SIZE_TO_PAGES (sizeof (ACPI_CPU_DATA)));
> +    ASSERT (AcpiCpuData != NULL);
>  
> -  //
> -  // Set PcdCpuS3DataAddress to the base address of the ACPI_CPU_DATA structure
> -  //
> -  Status = PcdSet64S (PcdCpuS3DataAddress, (UINT64)(UINTN)AcpiCpuData);
> -  ASSERT_EFI_ERROR (Status);
> +    //
> +    // Set PcdCpuS3DataAddress to the base address of the ACPI_CPU_DATA structure
> +    //
> +    Status = PcdSet64S (PcdCpuS3DataAddress, (UINT64)(UINTN)AcpiCpuData);
> +    ASSERT_EFI_ERROR (Status);
>  
> -  GetNumberOfProcessor (&NumberOfCpus, &NumberOfEnabledProcessors);
> -  AcpiCpuData->NumberOfCpus = (UINT32)NumberOfCpus;
> +    GetNumberOfProcessor (&NumberOfCpus, &NumberOfEnabledProcessors);
> +    AcpiCpuData->NumberOfCpus = (UINT32)NumberOfCpus;
> +  }
>  
>    //
>    // Allocate buffer for empty RegisterTable and PreSmmInitRegisterTable for all CPUs
> 
> Thanks,
> Ray
> 
>> -----Original Message-----
>> From: Zeng, Star <star.zeng@intel.com>
>> Sent: Wednesday, January 13, 2021 3:17 PM
>> To: Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>;
>> devel@edk2.groups.io
>> Cc: Dong, Eric <eric.dong@intel.com>; Philippe Mathieu-Daudé
>> <philmd@redhat.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>; Zeng, Star
>> <star.zeng@intel.com>
>> Subject: RE: [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate useless
>> register tables
>>
>> DxeRegisterCpuFeaturesLib still has some execution sequence dependency to
>> UefiCpuPkg CpuS3DataDxe.
>> There is ASSERT to happen at
>> https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/Library/RegisterC
>> puFeaturesLib/RegisterCpuFeaturesLib.c#L1059 when CpuS3DataDxe with this
>> patch runs before DxeRegisterCpuFeaturesLib.
>>
>> Thanks,
>> Star
>> -----Original Message-----
>> From: Ni, Ray <ray.ni@intel.com>
>> Sent: Wednesday, January 13, 2021 2:12 PM
>> To: Laszlo Ersek <lersek@redhat.com>; devel@edk2.groups.io
>> Cc: Dong, Eric <eric.dong@intel.com>; Philippe Mathieu-Daudé
>> <philmd@redhat.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>; Zeng, Star
>> <star.zeng@intel.com>
>> Subject: RE: [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate useless
>> register tables
>>
>> Reviewed-by: Ray Ni <ray.ni@intel.com>
>>
>> (I guess you don't want to put Redhat copyright in the patch 1&2 so the
>> copyright year is not updated.
>> Since it's a simple change that make the logic more neat, I am ok with that.)
>>
>> Will you create a pull request to merge all 3 patches together?
>>
>>> -----Original Message-----
>>> From: Laszlo Ersek <lersek@redhat.com>
>>> Sent: Wednesday, January 13, 2021 3:20 AM
>>> To: devel@edk2.groups.io
>>> Cc: Dong, Eric <eric.dong@intel.com>; Philippe Mathieu-Daudé
>>> <philmd@redhat.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>; Ni, Ray
>>> <ray.ni@intel.com>; Zeng, Star <star.zeng@intel.com>
>>> Subject: [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate useless
>>> register tables
>>>
>>> CpuS3DataDxe allocates the "RegisterTable" and "PreSmmInitRegisterTable"
>>> arrays in ACPI_CPU_DATA just so every processor in the system can have
>>> its own empty register table, matched by APIC ID. This has never been
>>> useful in practice.
>>>
>>> Given commit e992cc3f4859 ("UefiCpuPkg PiSmmCpuDxeSmm: Reduce
>> SMRAM
>>> consumption in CpuS3.c", 2021-01-11), simply leave both
>>> "AcpiCpuData->RegisterTable" and "AcpiCpuData->PreSmmInitRegisterTable"
>>> initialized to the zero address. This simplifies the driver, and saves
>>> both normal RAM (boot services data type memory) and -- in
>>> PiSmmCpuDxeSmm
>>> -- SMRAM.
>>>
>>> Cc: Eric Dong <eric.dong@intel.com>
>>> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
>>> Cc: Rahul Kumar <rahul1.kumar@intel.com>
>>> Cc: Ray Ni <ray.ni@intel.com>
>>> Cc: Star Zeng <star.zeng@intel.com>
>>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3159
>>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>>> ---
>>>
>>> Notes:
>>>     Tested by temporarily replacing OvmfPkgPkg/CpuS3DataDxe in the
>>> OVMF
>>> IA32
>>>     and IA32X64 platforms with this driver -- this driver works OK in OVMF
>>>     as long as no CPUs are hot-plugged.
>>>
>>>  UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c | 32 --------------------
>>>  1 file changed, 32 deletions(-)
>>>
>>> diff --git a/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
>>> b/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
>>> index 2be335d91903..078af36cfb41 100644
>>> --- a/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
>>> +++ b/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
>>> @@ -165,10 +165,6 @@ CpuS3DataInitialize (
>>>    UINTN                      NumberOfCpus;
>>>    UINTN                      NumberOfEnabledProcessors;
>>>    VOID                       *Stack;
>>> -  UINTN                      TableSize;
>>> -  CPU_REGISTER_TABLE         *RegisterTable;
>>> -  UINTN                      Index;
>>> -  EFI_PROCESSOR_INFORMATION  ProcessorInfoBuffer;
>>>    UINTN                      GdtSize;
>>>    UINTN                      IdtSize;
>>>    VOID                       *Gdt;
>>> @@ -255,34 +251,6 @@ CpuS3DataInitialize (
>>>      AcpiCpuData->PreSmmInitRegisterTable = OldAcpiCpuData-
>>>> PreSmmInitRegisterTable;
>>>      AcpiCpuData->ApLocation              = OldAcpiCpuData->ApLocation;
>>>      CopyMem (&AcpiCpuData->CpuStatus, &OldAcpiCpuData->CpuStatus,
>>> sizeof (CPU_STATUS_INFORMATION));
>>> -  } else {
>>> -    //
>>> -    // Allocate buffer for empty RegisterTable and PreSmmInitRegisterTable
>> for
>>> all CPUs
>>> -    //
>>> -    TableSize = 2 * NumberOfCpus * sizeof (CPU_REGISTER_TABLE);
>>> -    RegisterTable = (CPU_REGISTER_TABLE *)AllocateZeroPages (TableSize);
>>> -    ASSERT (RegisterTable != NULL);
>>> -
>>> -    for (Index = 0; Index < NumberOfCpus; Index++) {
>>> -      Status = MpServices->GetProcessorInfo (
>>> -                           MpServices,
>>> -                           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);
>>>    }
>>>
>>>    //
>>> --
>>> 2.19.1.3.g30247aa5d201
>>>
> 

Re: [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate useless register tables

[Zeng, Star]

Just think more, the change below needs a minor enhancement as below, otherwise the table will be overridden by the function's second call.

> -----Original Message-----
> From: Ni, Ray <ray.ni@intel.com>
> Sent: Wednesday, January 13, 2021 4:12 PM
> To: Zeng, Star <star.zeng@intel.com>; Laszlo Ersek <lersek@redhat.com>;
> devel@edk2.groups.io
> Cc: Dong, Eric <eric.dong@intel.com>; Philippe Mathieu-Daudé
> <philmd@redhat.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>
> Subject: RE: [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate useless
> register tables
> 
> Star,
> Thanks for pointing that.
> RegisterCpuFeaturesLib assumes [PreSmmInit]RegisterTable array is
> allocated but CpuS3Data
> doesn't do that.
> 
> Can following change fix the problem?
> 
> --- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> @@ -937,21 +937,19 @@ GetAcpiCpuData (
>    EFI_PROCESSOR_INFORMATION            ProcessorInfoBuffer;
> 
>    AcpiCpuData = (ACPI_CPU_DATA *) (UINTN) PcdGet64
> (PcdCpuS3DataAddress);
> -  if (AcpiCpuData != NULL) {
> -    return AcpiCpuData;
> -  }
> -
> -  AcpiCpuData  = AllocatePages (EFI_SIZE_TO_PAGES (sizeof
> (ACPI_CPU_DATA)));
> -  ASSERT (AcpiCpuData != NULL);
> +  if (AcpiCpuData == NULL) {
> +    AcpiCpuData  = AllocatePages (EFI_SIZE_TO_PAGES (sizeof
> (ACPI_CPU_DATA)));
> +    ASSERT (AcpiCpuData != NULL);
> 
> -  //
> -  // Set PcdCpuS3DataAddress to the base address of the ACPI_CPU_DATA
> structure
> -  //
> -  Status = PcdSet64S (PcdCpuS3DataAddress, (UINT64)(UINTN)AcpiCpuData);
> -  ASSERT_EFI_ERROR (Status);
> +    //
> +    // Set PcdCpuS3DataAddress to the base address of the ACPI_CPU_DATA
> structure
> +    //
> +    Status = PcdSet64S (PcdCpuS3DataAddress,
> (UINT64)(UINTN)AcpiCpuData);
> +    ASSERT_EFI_ERROR (Status);
> 
> -  GetNumberOfProcessor (&NumberOfCpus, &NumberOfEnabledProcessors);
> -  AcpiCpuData->NumberOfCpus = (UINT32)NumberOfCpus;
> +    GetNumberOfProcessor (&NumberOfCpus,
> &NumberOfEnabledProcessors);
> +    AcpiCpuData->NumberOfCpus = (UINT32)NumberOfCpus;
> +  }
> 

Add check as below here.

if (AcpiCpuData->RegisterTable == 0) {

>    //
>    // Allocate buffer for empty RegisterTable and PreSmmInitRegisterTable
> for all CPUs
> 
> Thanks,
> Ray
> 
> > -----Original Message-----
> > From: Zeng, Star <star.zeng@intel.com>
> > Sent: Wednesday, January 13, 2021 3:17 PM
> > To: Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>;
> > devel@edk2.groups.io
> > Cc: Dong, Eric <eric.dong@intel.com>; Philippe Mathieu-Daudé
> > <philmd@redhat.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>; Zeng,
> Star
> > <star.zeng@intel.com>
> > Subject: RE: [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate
> useless
> > register tables
> >
> > DxeRegisterCpuFeaturesLib still has some execution sequence dependency
> to
> > UefiCpuPkg CpuS3DataDxe.
> > There is ASSERT to happen at
> >
> https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/Library/Regist
> erC
> > puFeaturesLib/RegisterCpuFeaturesLib.c#L1059 when CpuS3DataDxe with
> this
> > patch runs before DxeRegisterCpuFeaturesLib.
> >
> > Thanks,
> > Star
> > -----Original Message-----
> > From: Ni, Ray <ray.ni@intel.com>
> > Sent: Wednesday, January 13, 2021 2:12 PM
> > To: Laszlo Ersek <lersek@redhat.com>; devel@edk2.groups.io
> > Cc: Dong, Eric <eric.dong@intel.com>; Philippe Mathieu-Daudé
> > <philmd@redhat.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>; Zeng,
> Star
> > <star.zeng@intel.com>
> > Subject: RE: [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate
> useless
> > register tables
> >
> > Reviewed-by: Ray Ni <ray.ni@intel.com>
> >
> > (I guess you don't want to put Redhat copyright in the patch 1&2 so the
> > copyright year is not updated.
> > Since it's a simple change that make the logic more neat, I am ok with that.)
> >
> > Will you create a pull request to merge all 3 patches together?
> >
> > > -----Original Message-----
> > > From: Laszlo Ersek <lersek@redhat.com>
> > > Sent: Wednesday, January 13, 2021 3:20 AM
> > > To: devel@edk2.groups.io
> > > Cc: Dong, Eric <eric.dong@intel.com>; Philippe Mathieu-Daudé
> > > <philmd@redhat.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>; Ni,
> Ray
> > > <ray.ni@intel.com>; Zeng, Star <star.zeng@intel.com>
> > > Subject: [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate useless
> > > register tables
> > >
> > > CpuS3DataDxe allocates the "RegisterTable" and
> "PreSmmInitRegisterTable"
> > > arrays in ACPI_CPU_DATA just so every processor in the system can have
> > > its own empty register table, matched by APIC ID. This has never been
> > > useful in practice.
> > >
> > > Given commit e992cc3f4859 ("UefiCpuPkg PiSmmCpuDxeSmm: Reduce
> > SMRAM
> > > consumption in CpuS3.c", 2021-01-11), simply leave both
> > > "AcpiCpuData->RegisterTable" and "AcpiCpuData-
> >PreSmmInitRegisterTable"
> > > initialized to the zero address. This simplifies the driver, and saves
> > > both normal RAM (boot services data type memory) and -- in
> > > PiSmmCpuDxeSmm
> > > -- SMRAM.
> > >
> > > Cc: Eric Dong <eric.dong@intel.com>
> > > Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> > > Cc: Rahul Kumar <rahul1.kumar@intel.com>
> > > Cc: Ray Ni <ray.ni@intel.com>
> > > Cc: Star Zeng <star.zeng@intel.com>
> > > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3159
> > > Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> > > ---
> > >
> > > Notes:
> > >     Tested by temporarily replacing OvmfPkgPkg/CpuS3DataDxe in the
> > > OVMF
> > > IA32
> > >     and IA32X64 platforms with this driver -- this driver works OK in OVMF
> > >     as long as no CPUs are hot-plugged.
> > >
> > >  UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c | 32 --------------------
> > >  1 file changed, 32 deletions(-)
> > >
> > > diff --git a/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
> > > b/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
> > > index 2be335d91903..078af36cfb41 100644
> > > --- a/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
> > > +++ b/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
> > > @@ -165,10 +165,6 @@ CpuS3DataInitialize (
> > >    UINTN                      NumberOfCpus;
> > >    UINTN                      NumberOfEnabledProcessors;
> > >    VOID                       *Stack;
> > > -  UINTN                      TableSize;
> > > -  CPU_REGISTER_TABLE         *RegisterTable;
> > > -  UINTN                      Index;
> > > -  EFI_PROCESSOR_INFORMATION  ProcessorInfoBuffer;
> > >    UINTN                      GdtSize;
> > >    UINTN                      IdtSize;
> > >    VOID                       *Gdt;
> > > @@ -255,34 +251,6 @@ CpuS3DataInitialize (
> > >      AcpiCpuData->PreSmmInitRegisterTable = OldAcpiCpuData-
> > > >PreSmmInitRegisterTable;
> > >      AcpiCpuData->ApLocation              = OldAcpiCpuData->ApLocation;
> > >      CopyMem (&AcpiCpuData->CpuStatus, &OldAcpiCpuData->CpuStatus,
> > > sizeof (CPU_STATUS_INFORMATION));
> > > -  } else {
> > > -    //
> > > -    // Allocate buffer for empty RegisterTable and
> PreSmmInitRegisterTable
> > for
> > > all CPUs
> > > -    //
> > > -    TableSize = 2 * NumberOfCpus * sizeof (CPU_REGISTER_TABLE);
> > > -    RegisterTable = (CPU_REGISTER_TABLE *)AllocateZeroPages
> (TableSize);
> > > -    ASSERT (RegisterTable != NULL);
> > > -
> > > -    for (Index = 0; Index < NumberOfCpus; Index++) {
> > > -      Status = MpServices->GetProcessorInfo (
> > > -                           MpServices,
> > > -                           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);
> > >    }
> > >
> > >    //
> > > --
> > > 2.19.1.3.g30247aa5d201
> > >

Re: [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate useless register tables

[Laszlo Ersek]

Hi Star,

On 01/14/21 02:55, Zeng, Star wrote:
> Just think more, the change below needs a minor enhancement as below, otherwise the table will be overridden by the function's second call.

thank you for following up here -- could you or Ray please propose an
actual patch for RegisterCpuFeaturesLib, as I requested before?

Posting the patch is not necessary; I only need to fetch it from your
personal repo(s) -- you can even send me the repo / branch reference
off-list. I would include the RegisterCpuFeaturesLib patch in my v2
posting of this series.

Thanks!
Laszlo

> 
>> -----Original Message-----
>> From: Ni, Ray <ray.ni@intel.com>
>> Sent: Wednesday, January 13, 2021 4:12 PM
>> To: Zeng, Star <star.zeng@intel.com>; Laszlo Ersek <lersek@redhat.com>;
>> devel@edk2.groups.io
>> Cc: Dong, Eric <eric.dong@intel.com>; Philippe Mathieu-Daudé
>> <philmd@redhat.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>
>> Subject: RE: [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate useless
>> register tables
>>
>> Star,
>> Thanks for pointing that.
>> RegisterCpuFeaturesLib assumes [PreSmmInit]RegisterTable array is
>> allocated but CpuS3Data
>> doesn't do that.
>>
>> Can following change fix the problem?
>>
>> --- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
>> +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
>> @@ -937,21 +937,19 @@ GetAcpiCpuData (
>>    EFI_PROCESSOR_INFORMATION            ProcessorInfoBuffer;
>>
>>    AcpiCpuData = (ACPI_CPU_DATA *) (UINTN) PcdGet64
>> (PcdCpuS3DataAddress);
>> -  if (AcpiCpuData != NULL) {
>> -    return AcpiCpuData;
>> -  }
>> -
>> -  AcpiCpuData  = AllocatePages (EFI_SIZE_TO_PAGES (sizeof
>> (ACPI_CPU_DATA)));
>> -  ASSERT (AcpiCpuData != NULL);
>> +  if (AcpiCpuData == NULL) {
>> +    AcpiCpuData  = AllocatePages (EFI_SIZE_TO_PAGES (sizeof
>> (ACPI_CPU_DATA)));
>> +    ASSERT (AcpiCpuData != NULL);
>>
>> -  //
>> -  // Set PcdCpuS3DataAddress to the base address of the ACPI_CPU_DATA
>> structure
>> -  //
>> -  Status = PcdSet64S (PcdCpuS3DataAddress, (UINT64)(UINTN)AcpiCpuData);
>> -  ASSERT_EFI_ERROR (Status);
>> +    //
>> +    // Set PcdCpuS3DataAddress to the base address of the ACPI_CPU_DATA
>> structure
>> +    //
>> +    Status = PcdSet64S (PcdCpuS3DataAddress,
>> (UINT64)(UINTN)AcpiCpuData);
>> +    ASSERT_EFI_ERROR (Status);
>>
>> -  GetNumberOfProcessor (&NumberOfCpus, &NumberOfEnabledProcessors);
>> -  AcpiCpuData->NumberOfCpus = (UINT32)NumberOfCpus;
>> +    GetNumberOfProcessor (&NumberOfCpus,
>> &NumberOfEnabledProcessors);
>> +    AcpiCpuData->NumberOfCpus = (UINT32)NumberOfCpus;
>> +  }
>>
> 
> Add check as below here.
> 
> if (AcpiCpuData->RegisterTable == 0) {
> 
>>    //
>>    // Allocate buffer for empty RegisterTable and PreSmmInitRegisterTable
>> for all CPUs
>>
>> Thanks,
>> Ray
>>
>>> -----Original Message-----
>>> From: Zeng, Star <star.zeng@intel.com>
>>> Sent: Wednesday, January 13, 2021 3:17 PM
>>> To: Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>;
>>> devel@edk2.groups.io
>>> Cc: Dong, Eric <eric.dong@intel.com>; Philippe Mathieu-Daudé
>>> <philmd@redhat.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>; Zeng,
>> Star
>>> <star.zeng@intel.com>
>>> Subject: RE: [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate
>> useless
>>> register tables
>>>
>>> DxeRegisterCpuFeaturesLib still has some execution sequence dependency
>> to
>>> UefiCpuPkg CpuS3DataDxe.
>>> There is ASSERT to happen at
>>>
>> https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/Library/Regist
>> erC
>>> puFeaturesLib/RegisterCpuFeaturesLib.c#L1059 when CpuS3DataDxe with
>> this
>>> patch runs before DxeRegisterCpuFeaturesLib.
>>>
>>> Thanks,
>>> Star
>>> -----Original Message-----
>>> From: Ni, Ray <ray.ni@intel.com>
>>> Sent: Wednesday, January 13, 2021 2:12 PM
>>> To: Laszlo Ersek <lersek@redhat.com>; devel@edk2.groups.io
>>> Cc: Dong, Eric <eric.dong@intel.com>; Philippe Mathieu-Daudé
>>> <philmd@redhat.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>; Zeng,
>> Star
>>> <star.zeng@intel.com>
>>> Subject: RE: [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate
>> useless
>>> register tables
>>>
>>> Reviewed-by: Ray Ni <ray.ni@intel.com>
>>>
>>> (I guess you don't want to put Redhat copyright in the patch 1&2 so the
>>> copyright year is not updated.
>>> Since it's a simple change that make the logic more neat, I am ok with that.)
>>>
>>> Will you create a pull request to merge all 3 patches together?
>>>
>>>> -----Original Message-----
>>>> From: Laszlo Ersek <lersek@redhat.com>
>>>> Sent: Wednesday, January 13, 2021 3:20 AM
>>>> To: devel@edk2.groups.io
>>>> Cc: Dong, Eric <eric.dong@intel.com>; Philippe Mathieu-Daudé
>>>> <philmd@redhat.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>; Ni,
>> Ray
>>>> <ray.ni@intel.com>; Zeng, Star <star.zeng@intel.com>
>>>> Subject: [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate useless
>>>> register tables
>>>>
>>>> CpuS3DataDxe allocates the "RegisterTable" and
>> "PreSmmInitRegisterTable"
>>>> arrays in ACPI_CPU_DATA just so every processor in the system can have
>>>> its own empty register table, matched by APIC ID. This has never been
>>>> useful in practice.
>>>>
>>>> Given commit e992cc3f4859 ("UefiCpuPkg PiSmmCpuDxeSmm: Reduce
>>> SMRAM
>>>> consumption in CpuS3.c", 2021-01-11), simply leave both
>>>> "AcpiCpuData->RegisterTable" and "AcpiCpuData-
>>> PreSmmInitRegisterTable"
>>>> initialized to the zero address. This simplifies the driver, and saves
>>>> both normal RAM (boot services data type memory) and -- in
>>>> PiSmmCpuDxeSmm
>>>> -- SMRAM.
>>>>
>>>> Cc: Eric Dong <eric.dong@intel.com>
>>>> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
>>>> Cc: Rahul Kumar <rahul1.kumar@intel.com>
>>>> Cc: Ray Ni <ray.ni@intel.com>
>>>> Cc: Star Zeng <star.zeng@intel.com>
>>>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3159
>>>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>>>> ---
>>>>
>>>> Notes:
>>>>     Tested by temporarily replacing OvmfPkgPkg/CpuS3DataDxe in the
>>>> OVMF
>>>> IA32
>>>>     and IA32X64 platforms with this driver -- this driver works OK in OVMF
>>>>     as long as no CPUs are hot-plugged.
>>>>
>>>>  UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c | 32 --------------------
>>>>  1 file changed, 32 deletions(-)
>>>>
>>>> diff --git a/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
>>>> b/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
>>>> index 2be335d91903..078af36cfb41 100644
>>>> --- a/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
>>>> +++ b/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
>>>> @@ -165,10 +165,6 @@ CpuS3DataInitialize (
>>>>    UINTN                      NumberOfCpus;
>>>>    UINTN                      NumberOfEnabledProcessors;
>>>>    VOID                       *Stack;
>>>> -  UINTN                      TableSize;
>>>> -  CPU_REGISTER_TABLE         *RegisterTable;
>>>> -  UINTN                      Index;
>>>> -  EFI_PROCESSOR_INFORMATION  ProcessorInfoBuffer;
>>>>    UINTN                      GdtSize;
>>>>    UINTN                      IdtSize;
>>>>    VOID                       *Gdt;
>>>> @@ -255,34 +251,6 @@ CpuS3DataInitialize (
>>>>      AcpiCpuData->PreSmmInitRegisterTable = OldAcpiCpuData-
>>>>> PreSmmInitRegisterTable;
>>>>      AcpiCpuData->ApLocation              = OldAcpiCpuData->ApLocation;
>>>>      CopyMem (&AcpiCpuData->CpuStatus, &OldAcpiCpuData->CpuStatus,
>>>> sizeof (CPU_STATUS_INFORMATION));
>>>> -  } else {
>>>> -    //
>>>> -    // Allocate buffer for empty RegisterTable and
>> PreSmmInitRegisterTable
>>> for
>>>> all CPUs
>>>> -    //
>>>> -    TableSize = 2 * NumberOfCpus * sizeof (CPU_REGISTER_TABLE);
>>>> -    RegisterTable = (CPU_REGISTER_TABLE *)AllocateZeroPages
>> (TableSize);
>>>> -    ASSERT (RegisterTable != NULL);
>>>> -
>>>> -    for (Index = 0; Index < NumberOfCpus; Index++) {
>>>> -      Status = MpServices->GetProcessorInfo (
>>>> -                           MpServices,
>>>> -                           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);
>>>>    }
>>>>
>>>>    //
>>>> --
>>>> 2.19.1.3.g30247aa5d201
>>>>
> 

Re: [edk2-devel] [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate useless register tables

[Ni, Ray]

Laszlo,
I will test my patches internally and find a way to give you the patch to be included in your V2.

Thanks,
Ray

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek
> Sent: Friday, January 15, 2021 1:36 AM
> To: Zeng, Star <star.zeng@intel.com>; Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io
> Cc: Dong, Eric <eric.dong@intel.com>; Philippe Mathieu-Daudé <philmd@redhat.com>; Kumar, Rahul1
> <rahul1.kumar@intel.com>
> Subject: Re: [edk2-devel] [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate useless register tables
> 
> Hi Star,
> 
> On 01/14/21 02:55, Zeng, Star wrote:
> > Just think more, the change below needs a minor enhancement as below, otherwise the table will be overridden by the
> function's second call.
> 
> thank you for following up here -- could you or Ray please propose an
> actual patch for RegisterCpuFeaturesLib, as I requested before?
> 
> Posting the patch is not necessary; I only need to fetch it from your
> personal repo(s) -- you can even send me the repo / branch reference
> off-list. I would include the RegisterCpuFeaturesLib patch in my v2
> posting of this series.
> 
> Thanks!
> Laszlo
> 
> >
> >> -----Original Message-----
> >> From: Ni, Ray <ray.ni@intel.com>
> >> Sent: Wednesday, January 13, 2021 4:12 PM
> >> To: Zeng, Star <star.zeng@intel.com>; Laszlo Ersek <lersek@redhat.com>;
> >> devel@edk2.groups.io
> >> Cc: Dong, Eric <eric.dong@intel.com>; Philippe Mathieu-Daudé
> >> <philmd@redhat.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>
> >> Subject: RE: [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate useless
> >> register tables
> >>
> >> Star,
> >> Thanks for pointing that.
> >> RegisterCpuFeaturesLib assumes [PreSmmInit]RegisterTable array is
> >> allocated but CpuS3Data
> >> doesn't do that.
> >>
> >> Can following change fix the problem?
> >>
> >> --- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> >> +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> >> @@ -937,21 +937,19 @@ GetAcpiCpuData (
> >>    EFI_PROCESSOR_INFORMATION            ProcessorInfoBuffer;
> >>
> >>    AcpiCpuData = (ACPI_CPU_DATA *) (UINTN) PcdGet64
> >> (PcdCpuS3DataAddress);
> >> -  if (AcpiCpuData != NULL) {
> >> -    return AcpiCpuData;
> >> -  }
> >> -
> >> -  AcpiCpuData  = AllocatePages (EFI_SIZE_TO_PAGES (sizeof
> >> (ACPI_CPU_DATA)));
> >> -  ASSERT (AcpiCpuData != NULL);
> >> +  if (AcpiCpuData == NULL) {
> >> +    AcpiCpuData  = AllocatePages (EFI_SIZE_TO_PAGES (sizeof
> >> (ACPI_CPU_DATA)));
> >> +    ASSERT (AcpiCpuData != NULL);
> >>
> >> -  //
> >> -  // Set PcdCpuS3DataAddress to the base address of the ACPI_CPU_DATA
> >> structure
> >> -  //
> >> -  Status = PcdSet64S (PcdCpuS3DataAddress, (UINT64)(UINTN)AcpiCpuData);
> >> -  ASSERT_EFI_ERROR (Status);
> >> +    //
> >> +    // Set PcdCpuS3DataAddress to the base address of the ACPI_CPU_DATA
> >> structure
> >> +    //
> >> +    Status = PcdSet64S (PcdCpuS3DataAddress,
> >> (UINT64)(UINTN)AcpiCpuData);
> >> +    ASSERT_EFI_ERROR (Status);
> >>
> >> -  GetNumberOfProcessor (&NumberOfCpus, &NumberOfEnabledProcessors);
> >> -  AcpiCpuData->NumberOfCpus = (UINT32)NumberOfCpus;
> >> +    GetNumberOfProcessor (&NumberOfCpus,
> >> &NumberOfEnabledProcessors);
> >> +    AcpiCpuData->NumberOfCpus = (UINT32)NumberOfCpus;
> >> +  }
> >>
> >
> > Add check as below here.
> >
> > if (AcpiCpuData->RegisterTable == 0) {
> >
> >>    //
> >>    // Allocate buffer for empty RegisterTable and PreSmmInitRegisterTable
> >> for all CPUs
> >>
> >> Thanks,
> >> Ray
> >>
> >>> -----Original Message-----
> >>> From: Zeng, Star <star.zeng@intel.com>
> >>> Sent: Wednesday, January 13, 2021 3:17 PM
> >>> To: Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>;
> >>> devel@edk2.groups.io
> >>> Cc: Dong, Eric <eric.dong@intel.com>; Philippe Mathieu-Daudé
> >>> <philmd@redhat.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>; Zeng,
> >> Star
> >>> <star.zeng@intel.com>
> >>> Subject: RE: [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate
> >> useless
> >>> register tables
> >>>
> >>> DxeRegisterCpuFeaturesLib still has some execution sequence dependency
> >> to
> >>> UefiCpuPkg CpuS3DataDxe.
> >>> There is ASSERT to happen at
> >>>
> >> https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/Library/Regist
> >> erC
> >>> puFeaturesLib/RegisterCpuFeaturesLib.c#L1059 when CpuS3DataDxe with
> >> this
> >>> patch runs before DxeRegisterCpuFeaturesLib.
> >>>
> >>> Thanks,
> >>> Star
> >>> -----Original Message-----
> >>> From: Ni, Ray <ray.ni@intel.com>
> >>> Sent: Wednesday, January 13, 2021 2:12 PM
> >>> To: Laszlo Ersek <lersek@redhat.com>; devel@edk2.groups.io
> >>> Cc: Dong, Eric <eric.dong@intel.com>; Philippe Mathieu-Daudé
> >>> <philmd@redhat.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>; Zeng,
> >> Star
> >>> <star.zeng@intel.com>
> >>> Subject: RE: [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate
> >> useless
> >>> register tables
> >>>
> >>> Reviewed-by: Ray Ni <ray.ni@intel.com>
> >>>
> >>> (I guess you don't want to put Redhat copyright in the patch 1&2 so the
> >>> copyright year is not updated.
> >>> Since it's a simple change that make the logic more neat, I am ok with that.)
> >>>
> >>> Will you create a pull request to merge all 3 patches together?
> >>>
> >>>> -----Original Message-----
> >>>> From: Laszlo Ersek <lersek@redhat.com>
> >>>> Sent: Wednesday, January 13, 2021 3:20 AM
> >>>> To: devel@edk2.groups.io
> >>>> Cc: Dong, Eric <eric.dong@intel.com>; Philippe Mathieu-Daudé
> >>>> <philmd@redhat.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>; Ni,
> >> Ray
> >>>> <ray.ni@intel.com>; Zeng, Star <star.zeng@intel.com>
> >>>> Subject: [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate useless
> >>>> register tables
> >>>>
> >>>> CpuS3DataDxe allocates the "RegisterTable" and
> >> "PreSmmInitRegisterTable"
> >>>> arrays in ACPI_CPU_DATA just so every processor in the system can have
> >>>> its own empty register table, matched by APIC ID. This has never been
> >>>> useful in practice.
> >>>>
> >>>> Given commit e992cc3f4859 ("UefiCpuPkg PiSmmCpuDxeSmm: Reduce
> >>> SMRAM
> >>>> consumption in CpuS3.c", 2021-01-11), simply leave both
> >>>> "AcpiCpuData->RegisterTable" and "AcpiCpuData-
> >>> PreSmmInitRegisterTable"
> >>>> initialized to the zero address. This simplifies the driver, and saves
> >>>> both normal RAM (boot services data type memory) and -- in
> >>>> PiSmmCpuDxeSmm
> >>>> -- SMRAM.
> >>>>
> >>>> Cc: Eric Dong <eric.dong@intel.com>
> >>>> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> >>>> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> >>>> Cc: Ray Ni <ray.ni@intel.com>
> >>>> Cc: Star Zeng <star.zeng@intel.com>
> >>>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3159
> >>>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> >>>> ---
> >>>>
> >>>> Notes:
> >>>>     Tested by temporarily replacing OvmfPkgPkg/CpuS3DataDxe in the
> >>>> OVMF
> >>>> IA32
> >>>>     and IA32X64 platforms with this driver -- this driver works OK in OVMF
> >>>>     as long as no CPUs are hot-plugged.
> >>>>
> >>>>  UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c | 32 --------------------
> >>>>  1 file changed, 32 deletions(-)
> >>>>
> >>>> diff --git a/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
> >>>> b/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
> >>>> index 2be335d91903..078af36cfb41 100644
> >>>> --- a/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
> >>>> +++ b/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
> >>>> @@ -165,10 +165,6 @@ CpuS3DataInitialize (
> >>>>    UINTN                      NumberOfCpus;
> >>>>    UINTN                      NumberOfEnabledProcessors;
> >>>>    VOID                       *Stack;
> >>>> -  UINTN                      TableSize;
> >>>> -  CPU_REGISTER_TABLE         *RegisterTable;
> >>>> -  UINTN                      Index;
> >>>> -  EFI_PROCESSOR_INFORMATION  ProcessorInfoBuffer;
> >>>>    UINTN                      GdtSize;
> >>>>    UINTN                      IdtSize;
> >>>>    VOID                       *Gdt;
> >>>> @@ -255,34 +251,6 @@ CpuS3DataInitialize (
> >>>>      AcpiCpuData->PreSmmInitRegisterTable = OldAcpiCpuData-
> >>>>> PreSmmInitRegisterTable;
> >>>>      AcpiCpuData->ApLocation              = OldAcpiCpuData->ApLocation;
> >>>>      CopyMem (&AcpiCpuData->CpuStatus, &OldAcpiCpuData->CpuStatus,
> >>>> sizeof (CPU_STATUS_INFORMATION));
> >>>> -  } else {
> >>>> -    //
> >>>> -    // Allocate buffer for empty RegisterTable and
> >> PreSmmInitRegisterTable
> >>> for
> >>>> all CPUs
> >>>> -    //
> >>>> -    TableSize = 2 * NumberOfCpus * sizeof (CPU_REGISTER_TABLE);
> >>>> -    RegisterTable = (CPU_REGISTER_TABLE *)AllocateZeroPages
> >> (TableSize);
> >>>> -    ASSERT (RegisterTable != NULL);
> >>>> -
> >>>> -    for (Index = 0; Index < NumberOfCpus; Index++) {
> >>>> -      Status = MpServices->GetProcessorInfo (
> >>>> -                           MpServices,
> >>>> -                           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);
> >>>>    }
> >>>>
> >>>>    //
> >>>> --
> >>>> 2.19.1.3.g30247aa5d201
> >>>>
> >
> 
> 
> 
> 
> 

Re: [edk2-devel] [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate useless register tables

[Laszlo Ersek]

On 01/15/21 09:33, Ni, Ray wrote:
> Laszlo,
> I will test my patches internally and find a way to give you the patch to be included in your V2.

Thank you!
Laszlo

> 
> Thanks,
> Ray
> 
>> -----Original Message-----
>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek
>> Sent: Friday, January 15, 2021 1:36 AM
>> To: Zeng, Star <star.zeng@intel.com>; Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io
>> Cc: Dong, Eric <eric.dong@intel.com>; Philippe Mathieu-Daudé <philmd@redhat.com>; Kumar, Rahul1
>> <rahul1.kumar@intel.com>
>> Subject: Re: [edk2-devel] [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate useless register tables
>>
>> Hi Star,
>>
>> On 01/14/21 02:55, Zeng, Star wrote:
>>> Just think more, the change below needs a minor enhancement as below, otherwise the table will be overridden by the
>> function's second call.
>>
>> thank you for following up here -- could you or Ray please propose an
>> actual patch for RegisterCpuFeaturesLib, as I requested before?
>>
>> Posting the patch is not necessary; I only need to fetch it from your
>> personal repo(s) -- you can even send me the repo / branch reference
>> off-list. I would include the RegisterCpuFeaturesLib patch in my v2
>> posting of this series.
>>
>> Thanks!
>> Laszlo
>>
>>>
>>>> -----Original Message-----
>>>> From: Ni, Ray <ray.ni@intel.com>
>>>> Sent: Wednesday, January 13, 2021 4:12 PM
>>>> To: Zeng, Star <star.zeng@intel.com>; Laszlo Ersek <lersek@redhat.com>;
>>>> devel@edk2.groups.io
>>>> Cc: Dong, Eric <eric.dong@intel.com>; Philippe Mathieu-Daudé
>>>> <philmd@redhat.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>
>>>> Subject: RE: [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate useless
>>>> register tables
>>>>
>>>> Star,
>>>> Thanks for pointing that.
>>>> RegisterCpuFeaturesLib assumes [PreSmmInit]RegisterTable array is
>>>> allocated but CpuS3Data
>>>> doesn't do that.
>>>>
>>>> Can following change fix the problem?
>>>>
>>>> --- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
>>>> +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
>>>> @@ -937,21 +937,19 @@ GetAcpiCpuData (
>>>>    EFI_PROCESSOR_INFORMATION            ProcessorInfoBuffer;
>>>>
>>>>    AcpiCpuData = (ACPI_CPU_DATA *) (UINTN) PcdGet64
>>>> (PcdCpuS3DataAddress);
>>>> -  if (AcpiCpuData != NULL) {
>>>> -    return AcpiCpuData;
>>>> -  }
>>>> -
>>>> -  AcpiCpuData  = AllocatePages (EFI_SIZE_TO_PAGES (sizeof
>>>> (ACPI_CPU_DATA)));
>>>> -  ASSERT (AcpiCpuData != NULL);
>>>> +  if (AcpiCpuData == NULL) {
>>>> +    AcpiCpuData  = AllocatePages (EFI_SIZE_TO_PAGES (sizeof
>>>> (ACPI_CPU_DATA)));
>>>> +    ASSERT (AcpiCpuData != NULL);
>>>>
>>>> -  //
>>>> -  // Set PcdCpuS3DataAddress to the base address of the ACPI_CPU_DATA
>>>> structure
>>>> -  //
>>>> -  Status = PcdSet64S (PcdCpuS3DataAddress, (UINT64)(UINTN)AcpiCpuData);
>>>> -  ASSERT_EFI_ERROR (Status);
>>>> +    //
>>>> +    // Set PcdCpuS3DataAddress to the base address of the ACPI_CPU_DATA
>>>> structure
>>>> +    //
>>>> +    Status = PcdSet64S (PcdCpuS3DataAddress,
>>>> (UINT64)(UINTN)AcpiCpuData);
>>>> +    ASSERT_EFI_ERROR (Status);
>>>>
>>>> -  GetNumberOfProcessor (&NumberOfCpus, &NumberOfEnabledProcessors);
>>>> -  AcpiCpuData->NumberOfCpus = (UINT32)NumberOfCpus;
>>>> +    GetNumberOfProcessor (&NumberOfCpus,
>>>> &NumberOfEnabledProcessors);
>>>> +    AcpiCpuData->NumberOfCpus = (UINT32)NumberOfCpus;
>>>> +  }
>>>>
>>>
>>> Add check as below here.
>>>
>>> if (AcpiCpuData->RegisterTable == 0) {
>>>
>>>>    //
>>>>    // Allocate buffer for empty RegisterTable and PreSmmInitRegisterTable
>>>> for all CPUs
>>>>
>>>> Thanks,
>>>> Ray
>>>>
>>>>> -----Original Message-----
>>>>> From: Zeng, Star <star.zeng@intel.com>
>>>>> Sent: Wednesday, January 13, 2021 3:17 PM
>>>>> To: Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>;
>>>>> devel@edk2.groups.io
>>>>> Cc: Dong, Eric <eric.dong@intel.com>; Philippe Mathieu-Daudé
>>>>> <philmd@redhat.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>; Zeng,
>>>> Star
>>>>> <star.zeng@intel.com>
>>>>> Subject: RE: [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate
>>>> useless
>>>>> register tables
>>>>>
>>>>> DxeRegisterCpuFeaturesLib still has some execution sequence dependency
>>>> to
>>>>> UefiCpuPkg CpuS3DataDxe.
>>>>> There is ASSERT to happen at
>>>>>
>>>> https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/Library/Regist
>>>> erC
>>>>> puFeaturesLib/RegisterCpuFeaturesLib.c#L1059 when CpuS3DataDxe with
>>>> this
>>>>> patch runs before DxeRegisterCpuFeaturesLib.
>>>>>
>>>>> Thanks,
>>>>> Star
>>>>> -----Original Message-----
>>>>> From: Ni, Ray <ray.ni@intel.com>
>>>>> Sent: Wednesday, January 13, 2021 2:12 PM
>>>>> To: Laszlo Ersek <lersek@redhat.com>; devel@edk2.groups.io
>>>>> Cc: Dong, Eric <eric.dong@intel.com>; Philippe Mathieu-Daudé
>>>>> <philmd@redhat.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>; Zeng,
>>>> Star
>>>>> <star.zeng@intel.com>
>>>>> Subject: RE: [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate
>>>> useless
>>>>> register tables
>>>>>
>>>>> Reviewed-by: Ray Ni <ray.ni@intel.com>
>>>>>
>>>>> (I guess you don't want to put Redhat copyright in the patch 1&2 so the
>>>>> copyright year is not updated.
>>>>> Since it's a simple change that make the logic more neat, I am ok with that.)
>>>>>
>>>>> Will you create a pull request to merge all 3 patches together?
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Laszlo Ersek <lersek@redhat.com>
>>>>>> Sent: Wednesday, January 13, 2021 3:20 AM
>>>>>> To: devel@edk2.groups.io
>>>>>> Cc: Dong, Eric <eric.dong@intel.com>; Philippe Mathieu-Daudé
>>>>>> <philmd@redhat.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>; Ni,
>>>> Ray
>>>>>> <ray.ni@intel.com>; Zeng, Star <star.zeng@intel.com>
>>>>>> Subject: [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate useless
>>>>>> register tables
>>>>>>
>>>>>> CpuS3DataDxe allocates the "RegisterTable" and
>>>> "PreSmmInitRegisterTable"
>>>>>> arrays in ACPI_CPU_DATA just so every processor in the system can have
>>>>>> its own empty register table, matched by APIC ID. This has never been
>>>>>> useful in practice.
>>>>>>
>>>>>> Given commit e992cc3f4859 ("UefiCpuPkg PiSmmCpuDxeSmm: Reduce
>>>>> SMRAM
>>>>>> consumption in CpuS3.c", 2021-01-11), simply leave both
>>>>>> "AcpiCpuData->RegisterTable" and "AcpiCpuData-
>>>>> PreSmmInitRegisterTable"
>>>>>> initialized to the zero address. This simplifies the driver, and saves
>>>>>> both normal RAM (boot services data type memory) and -- in
>>>>>> PiSmmCpuDxeSmm
>>>>>> -- SMRAM.
>>>>>>
>>>>>> Cc: Eric Dong <eric.dong@intel.com>
>>>>>> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
>>>>>> Cc: Rahul Kumar <rahul1.kumar@intel.com>
>>>>>> Cc: Ray Ni <ray.ni@intel.com>
>>>>>> Cc: Star Zeng <star.zeng@intel.com>
>>>>>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3159
>>>>>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>>>>>> ---
>>>>>>
>>>>>> Notes:
>>>>>>     Tested by temporarily replacing OvmfPkgPkg/CpuS3DataDxe in the
>>>>>> OVMF
>>>>>> IA32
>>>>>>     and IA32X64 platforms with this driver -- this driver works OK in OVMF
>>>>>>     as long as no CPUs are hot-plugged.
>>>>>>
>>>>>>  UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c | 32 --------------------
>>>>>>  1 file changed, 32 deletions(-)
>>>>>>
>>>>>> diff --git a/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
>>>>>> b/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
>>>>>> index 2be335d91903..078af36cfb41 100644
>>>>>> --- a/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
>>>>>> +++ b/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
>>>>>> @@ -165,10 +165,6 @@ CpuS3DataInitialize (
>>>>>>    UINTN                      NumberOfCpus;
>>>>>>    UINTN                      NumberOfEnabledProcessors;
>>>>>>    VOID                       *Stack;
>>>>>> -  UINTN                      TableSize;
>>>>>> -  CPU_REGISTER_TABLE         *RegisterTable;
>>>>>> -  UINTN                      Index;
>>>>>> -  EFI_PROCESSOR_INFORMATION  ProcessorInfoBuffer;
>>>>>>    UINTN                      GdtSize;
>>>>>>    UINTN                      IdtSize;
>>>>>>    VOID                       *Gdt;
>>>>>> @@ -255,34 +251,6 @@ CpuS3DataInitialize (
>>>>>>      AcpiCpuData->PreSmmInitRegisterTable = OldAcpiCpuData-
>>>>>>> PreSmmInitRegisterTable;
>>>>>>      AcpiCpuData->ApLocation              = OldAcpiCpuData->ApLocation;
>>>>>>      CopyMem (&AcpiCpuData->CpuStatus, &OldAcpiCpuData->CpuStatus,
>>>>>> sizeof (CPU_STATUS_INFORMATION));
>>>>>> -  } else {
>>>>>> -    //
>>>>>> -    // Allocate buffer for empty RegisterTable and
>>>> PreSmmInitRegisterTable
>>>>> for
>>>>>> all CPUs
>>>>>> -    //
>>>>>> -    TableSize = 2 * NumberOfCpus * sizeof (CPU_REGISTER_TABLE);
>>>>>> -    RegisterTable = (CPU_REGISTER_TABLE *)AllocateZeroPages
>>>> (TableSize);
>>>>>> -    ASSERT (RegisterTable != NULL);
>>>>>> -
>>>>>> -    for (Index = 0; Index < NumberOfCpus; Index++) {
>>>>>> -      Status = MpServices->GetProcessorInfo (
>>>>>> -                           MpServices,
>>>>>> -                           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);
>>>>>>    }
>>>>>>
>>>>>>    //
>>>>>> --
>>>>>> 2.19.1.3.g30247aa5d201
>>>>>>
>>>
>>
>>
>>
>> 
>>
> 

Re: [edk2-devel] [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate useless register tables

[Ni, Ray]

https://github.com/niruiyu/edk2/commit/7091aa50b9d87240b14e5a74398eab010ffc4d3d
Laszlo, Star,
Please check this code change. I verified S3 boot in an internal platform.

Thanks,
Ray

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek
> Sent: Friday, January 15, 2021 4:37 PM
> To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io; Zeng, Star <star.zeng@intel.com>
> Cc: Dong, Eric <eric.dong@intel.com>; Philippe Mathieu-Daudé <philmd@redhat.com>; Kumar, Rahul1
> <rahul1.kumar@intel.com>
> Subject: Re: [edk2-devel] [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate useless register tables
> 
> On 01/15/21 09:33, Ni, Ray wrote:
> > Laszlo,
> > I will test my patches internally and find a way to give you the patch to be included in your V2.
> 
> Thank you!
> Laszlo
> 
> >
> > Thanks,
> > Ray
> >
> >> -----Original Message-----
> >> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek
> >> Sent: Friday, January 15, 2021 1:36 AM
> >> To: Zeng, Star <star.zeng@intel.com>; Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io
> >> Cc: Dong, Eric <eric.dong@intel.com>; Philippe Mathieu-Daudé <philmd@redhat.com>; Kumar, Rahul1
> >> <rahul1.kumar@intel.com>
> >> Subject: Re: [edk2-devel] [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate useless register tables
> >>
> >> Hi Star,
> >>
> >> On 01/14/21 02:55, Zeng, Star wrote:
> >>> Just think more, the change below needs a minor enhancement as below, otherwise the table will be overridden by
> the
> >> function's second call.
> >>
> >> thank you for following up here -- could you or Ray please propose an
> >> actual patch for RegisterCpuFeaturesLib, as I requested before?
> >>
> >> Posting the patch is not necessary; I only need to fetch it from your
> >> personal repo(s) -- you can even send me the repo / branch reference
> >> off-list. I would include the RegisterCpuFeaturesLib patch in my v2
> >> posting of this series.
> >>
> >> Thanks!
> >> Laszlo
> >>
> >>>
> >>>> -----Original Message-----
> >>>> From: Ni, Ray <ray.ni@intel.com>
> >>>> Sent: Wednesday, January 13, 2021 4:12 PM
> >>>> To: Zeng, Star <star.zeng@intel.com>; Laszlo Ersek <lersek@redhat.com>;
> >>>> devel@edk2.groups.io
> >>>> Cc: Dong, Eric <eric.dong@intel.com>; Philippe Mathieu-Daudé
> >>>> <philmd@redhat.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>
> >>>> Subject: RE: [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate useless
> >>>> register tables
> >>>>
> >>>> Star,
> >>>> Thanks for pointing that.
> >>>> RegisterCpuFeaturesLib assumes [PreSmmInit]RegisterTable array is
> >>>> allocated but CpuS3Data
> >>>> doesn't do that.
> >>>>
> >>>> Can following change fix the problem?
> >>>>
> >>>> --- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> >>>> +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> >>>> @@ -937,21 +937,19 @@ GetAcpiCpuData (
> >>>>    EFI_PROCESSOR_INFORMATION            ProcessorInfoBuffer;
> >>>>
> >>>>    AcpiCpuData = (ACPI_CPU_DATA *) (UINTN) PcdGet64
> >>>> (PcdCpuS3DataAddress);
> >>>> -  if (AcpiCpuData != NULL) {
> >>>> -    return AcpiCpuData;
> >>>> -  }
> >>>> -
> >>>> -  AcpiCpuData  = AllocatePages (EFI_SIZE_TO_PAGES (sizeof
> >>>> (ACPI_CPU_DATA)));
> >>>> -  ASSERT (AcpiCpuData != NULL);
> >>>> +  if (AcpiCpuData == NULL) {
> >>>> +    AcpiCpuData  = AllocatePages (EFI_SIZE_TO_PAGES (sizeof
> >>>> (ACPI_CPU_DATA)));
> >>>> +    ASSERT (AcpiCpuData != NULL);
> >>>>
> >>>> -  //
> >>>> -  // Set PcdCpuS3DataAddress to the base address of the ACPI_CPU_DATA
> >>>> structure
> >>>> -  //
> >>>> -  Status = PcdSet64S (PcdCpuS3DataAddress, (UINT64)(UINTN)AcpiCpuData);
> >>>> -  ASSERT_EFI_ERROR (Status);
> >>>> +    //
> >>>> +    // Set PcdCpuS3DataAddress to the base address of the ACPI_CPU_DATA
> >>>> structure
> >>>> +    //
> >>>> +    Status = PcdSet64S (PcdCpuS3DataAddress,
> >>>> (UINT64)(UINTN)AcpiCpuData);
> >>>> +    ASSERT_EFI_ERROR (Status);
> >>>>
> >>>> -  GetNumberOfProcessor (&NumberOfCpus, &NumberOfEnabledProcessors);
> >>>> -  AcpiCpuData->NumberOfCpus = (UINT32)NumberOfCpus;
> >>>> +    GetNumberOfProcessor (&NumberOfCpus,
> >>>> &NumberOfEnabledProcessors);
> >>>> +    AcpiCpuData->NumberOfCpus = (UINT32)NumberOfCpus;
> >>>> +  }
> >>>>
> >>>
> >>> Add check as below here.
> >>>
> >>> if (AcpiCpuData->RegisterTable == 0) {
> >>>
> >>>>    //
> >>>>    // Allocate buffer for empty RegisterTable and PreSmmInitRegisterTable
> >>>> for all CPUs
> >>>>
> >>>> Thanks,
> >>>> Ray
> >>>>
> >>>>> -----Original Message-----
> >>>>> From: Zeng, Star <star.zeng@intel.com>
> >>>>> Sent: Wednesday, January 13, 2021 3:17 PM
> >>>>> To: Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>;
> >>>>> devel@edk2.groups.io
> >>>>> Cc: Dong, Eric <eric.dong@intel.com>; Philippe Mathieu-Daudé
> >>>>> <philmd@redhat.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>; Zeng,
> >>>> Star
> >>>>> <star.zeng@intel.com>
> >>>>> Subject: RE: [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate
> >>>> useless
> >>>>> register tables
> >>>>>
> >>>>> DxeRegisterCpuFeaturesLib still has some execution sequence dependency
> >>>> to
> >>>>> UefiCpuPkg CpuS3DataDxe.
> >>>>> There is ASSERT to happen at
> >>>>>
> >>>> https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/Library/Regist
> >>>> erC
> >>>>> puFeaturesLib/RegisterCpuFeaturesLib.c#L1059 when CpuS3DataDxe with
> >>>> this
> >>>>> patch runs before DxeRegisterCpuFeaturesLib.
> >>>>>
> >>>>> Thanks,
> >>>>> Star
> >>>>> -----Original Message-----
> >>>>> From: Ni, Ray <ray.ni@intel.com>
> >>>>> Sent: Wednesday, January 13, 2021 2:12 PM
> >>>>> To: Laszlo Ersek <lersek@redhat.com>; devel@edk2.groups.io
> >>>>> Cc: Dong, Eric <eric.dong@intel.com>; Philippe Mathieu-Daudé
> >>>>> <philmd@redhat.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>; Zeng,
> >>>> Star
> >>>>> <star.zeng@intel.com>
> >>>>> Subject: RE: [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate
> >>>> useless
> >>>>> register tables
> >>>>>
> >>>>> Reviewed-by: Ray Ni <ray.ni@intel.com>
> >>>>>
> >>>>> (I guess you don't want to put Redhat copyright in the patch 1&2 so the
> >>>>> copyright year is not updated.
> >>>>> Since it's a simple change that make the logic more neat, I am ok with that.)
> >>>>>
> >>>>> Will you create a pull request to merge all 3 patches together?
> >>>>>
> >>>>>> -----Original Message-----
> >>>>>> From: Laszlo Ersek <lersek@redhat.com>
> >>>>>> Sent: Wednesday, January 13, 2021 3:20 AM
> >>>>>> To: devel@edk2.groups.io
> >>>>>> Cc: Dong, Eric <eric.dong@intel.com>; Philippe Mathieu-Daudé
> >>>>>> <philmd@redhat.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>; Ni,
> >>>> Ray
> >>>>>> <ray.ni@intel.com>; Zeng, Star <star.zeng@intel.com>
> >>>>>> Subject: [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate useless
> >>>>>> register tables
> >>>>>>
> >>>>>> CpuS3DataDxe allocates the "RegisterTable" and
> >>>> "PreSmmInitRegisterTable"
> >>>>>> arrays in ACPI_CPU_DATA just so every processor in the system can have
> >>>>>> its own empty register table, matched by APIC ID. This has never been
> >>>>>> useful in practice.
> >>>>>>
> >>>>>> Given commit e992cc3f4859 ("UefiCpuPkg PiSmmCpuDxeSmm: Reduce
> >>>>> SMRAM
> >>>>>> consumption in CpuS3.c", 2021-01-11), simply leave both
> >>>>>> "AcpiCpuData->RegisterTable" and "AcpiCpuData-
> >>>>> PreSmmInitRegisterTable"
> >>>>>> initialized to the zero address. This simplifies the driver, and saves
> >>>>>> both normal RAM (boot services data type memory) and -- in
> >>>>>> PiSmmCpuDxeSmm
> >>>>>> -- SMRAM.
> >>>>>>
> >>>>>> Cc: Eric Dong <eric.dong@intel.com>
> >>>>>> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> >>>>>> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> >>>>>> Cc: Ray Ni <ray.ni@intel.com>
> >>>>>> Cc: Star Zeng <star.zeng@intel.com>
> >>>>>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3159
> >>>>>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> >>>>>> ---
> >>>>>>
> >>>>>> Notes:
> >>>>>>     Tested by temporarily replacing OvmfPkgPkg/CpuS3DataDxe in the
> >>>>>> OVMF
> >>>>>> IA32
> >>>>>>     and IA32X64 platforms with this driver -- this driver works OK in OVMF
> >>>>>>     as long as no CPUs are hot-plugged.
> >>>>>>
> >>>>>>  UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c | 32 --------------------
> >>>>>>  1 file changed, 32 deletions(-)
> >>>>>>
> >>>>>> diff --git a/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
> >>>>>> b/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
> >>>>>> index 2be335d91903..078af36cfb41 100644
> >>>>>> --- a/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
> >>>>>> +++ b/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
> >>>>>> @@ -165,10 +165,6 @@ CpuS3DataInitialize (
> >>>>>>    UINTN                      NumberOfCpus;
> >>>>>>    UINTN                      NumberOfEnabledProcessors;
> >>>>>>    VOID                       *Stack;
> >>>>>> -  UINTN                      TableSize;
> >>>>>> -  CPU_REGISTER_TABLE         *RegisterTable;
> >>>>>> -  UINTN                      Index;
> >>>>>> -  EFI_PROCESSOR_INFORMATION  ProcessorInfoBuffer;
> >>>>>>    UINTN                      GdtSize;
> >>>>>>    UINTN                      IdtSize;
> >>>>>>    VOID                       *Gdt;
> >>>>>> @@ -255,34 +251,6 @@ CpuS3DataInitialize (
> >>>>>>      AcpiCpuData->PreSmmInitRegisterTable = OldAcpiCpuData-
> >>>>>>> PreSmmInitRegisterTable;
> >>>>>>      AcpiCpuData->ApLocation              = OldAcpiCpuData->ApLocation;
> >>>>>>      CopyMem (&AcpiCpuData->CpuStatus, &OldAcpiCpuData->CpuStatus,
> >>>>>> sizeof (CPU_STATUS_INFORMATION));
> >>>>>> -  } else {
> >>>>>> -    //
> >>>>>> -    // Allocate buffer for empty RegisterTable and
> >>>> PreSmmInitRegisterTable
> >>>>> for
> >>>>>> all CPUs
> >>>>>> -    //
> >>>>>> -    TableSize = 2 * NumberOfCpus * sizeof (CPU_REGISTER_TABLE);
> >>>>>> -    RegisterTable = (CPU_REGISTER_TABLE *)AllocateZeroPages
> >>>> (TableSize);
> >>>>>> -    ASSERT (RegisterTable != NULL);
> >>>>>> -
> >>>>>> -    for (Index = 0; Index < NumberOfCpus; Index++) {
> >>>>>> -      Status = MpServices->GetProcessorInfo (
> >>>>>> -                           MpServices,
> >>>>>> -                           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);
> >>>>>>    }
> >>>>>>
> >>>>>>    //
> >>>>>> --
> >>>>>> 2.19.1.3.g30247aa5d201
> >>>>>>
> >>>
> >>
> >>
> >>
> >>
> >>
> >
> 
> 
> 
> 
> 

Re: [edk2-devel] [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate useless register tables

[Laszlo Ersek]

On 01/19/21 13:52, Ni, Ray wrote:
> https://github.com/niruiyu/edk2/commit/7091aa50b9d87240b14e5a74398eab010ffc4d3d
> Laszlo, Star,
> Please check this code change. I verified S3 boot in an internal platform.

Thanks -- I've picked this commit to my local v2 branch; I'm going to
submit the v2 series soon. I'd like Star to review the new patch on the
list, as part of the v2 series.

Thanks
Laszlo

Re: [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate useless register tables

[Laszlo Ersek]

On 01/13/21 07:12, Ni, Ray wrote:
> Reviewed-by: Ray Ni <ray.ni@intel.com>
> 
> (I guess you don't want to put Redhat copyright in the patch 1&2 so the copyright year is not updated.
> Since it's a simple change that make the logic more neat, I am ok with that.)

I don't care about updating existent (C) notices; the git history is
there for that. (Unless the package maintainer insists, of course.)

At Red Hat, we add copyright notices when we create new files.

> Will you create a pull request to merge all 3 patches together?

Sure, I'll be happy to.

Thanks!
Laszlo

> 
>> -----Original Message-----
>> From: Laszlo Ersek <lersek@redhat.com>
>> Sent: Wednesday, January 13, 2021 3:20 AM
>> To: devel@edk2.groups.io
>> Cc: Dong, Eric <eric.dong@intel.com>; Philippe Mathieu-Daudé
>> <philmd@redhat.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>; Ni, Ray
>> <ray.ni@intel.com>; Zeng, Star <star.zeng@intel.com>
>> Subject: [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate useless
>> register tables
>>
>> CpuS3DataDxe allocates the "RegisterTable" and "PreSmmInitRegisterTable"
>> arrays in ACPI_CPU_DATA just so every processor in the system can have its
>> own empty register table, matched by APIC ID. This has never been useful
>> in practice.
>>
>> Given commit e992cc3f4859 ("UefiCpuPkg PiSmmCpuDxeSmm: Reduce
>> SMRAM
>> consumption in CpuS3.c", 2021-01-11), simply leave both
>> "AcpiCpuData->RegisterTable" and "AcpiCpuData->PreSmmInitRegisterTable"
>> initialized to the zero address. This simplifies the driver, and saves
>> both normal RAM (boot services data type memory) and -- in
>> PiSmmCpuDxeSmm
>> -- SMRAM.
>>
>> Cc: Eric Dong <eric.dong@intel.com>
>> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
>> Cc: Rahul Kumar <rahul1.kumar@intel.com>
>> Cc: Ray Ni <ray.ni@intel.com>
>> Cc: Star Zeng <star.zeng@intel.com>
>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3159
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>>
>> Notes:
>>     Tested by temporarily replacing OvmfPkgPkg/CpuS3DataDxe in the OVMF
>> IA32
>>     and IA32X64 platforms with this driver -- this driver works OK in OVMF
>>     as long as no CPUs are hot-plugged.
>>
>>  UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c | 32 --------------------
>>  1 file changed, 32 deletions(-)
>>
>> diff --git a/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
>> b/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
>> index 2be335d91903..078af36cfb41 100644
>> --- a/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
>> +++ b/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
>> @@ -165,10 +165,6 @@ CpuS3DataInitialize (
>>    UINTN                      NumberOfCpus;
>>    UINTN                      NumberOfEnabledProcessors;
>>    VOID                       *Stack;
>> -  UINTN                      TableSize;
>> -  CPU_REGISTER_TABLE         *RegisterTable;
>> -  UINTN                      Index;
>> -  EFI_PROCESSOR_INFORMATION  ProcessorInfoBuffer;
>>    UINTN                      GdtSize;
>>    UINTN                      IdtSize;
>>    VOID                       *Gdt;
>> @@ -255,34 +251,6 @@ CpuS3DataInitialize (
>>      AcpiCpuData->PreSmmInitRegisterTable = OldAcpiCpuData-
>>> PreSmmInitRegisterTable;
>>      AcpiCpuData->ApLocation              = OldAcpiCpuData->ApLocation;
>>      CopyMem (&AcpiCpuData->CpuStatus, &OldAcpiCpuData->CpuStatus, sizeof
>> (CPU_STATUS_INFORMATION));
>> -  } else {
>> -    //
>> -    // Allocate buffer for empty RegisterTable and PreSmmInitRegisterTable for
>> all CPUs
>> -    //
>> -    TableSize = 2 * NumberOfCpus * sizeof (CPU_REGISTER_TABLE);
>> -    RegisterTable = (CPU_REGISTER_TABLE *)AllocateZeroPages (TableSize);
>> -    ASSERT (RegisterTable != NULL);
>> -
>> -    for (Index = 0; Index < NumberOfCpus; Index++) {
>> -      Status = MpServices->GetProcessorInfo (
>> -                           MpServices,
>> -                           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);
>>    }
>>
>>    //
>> --
>> 2.19.1.3.g30247aa5d201
>>
> 

[PATCH 3/3] OvmfPkg/CpuS3DataDxe: do not allocate useless register tables

[Laszlo Ersek]

CpuS3DataDxe allocates the "RegisterTable" and "PreSmmInitRegisterTable"
arrays in ACPI_CPU_DATA just so every processor in the system can have its
own empty register table, matched by APIC ID. This has never been useful
in practice.

Given commit e992cc3f4859 ("UefiCpuPkg PiSmmCpuDxeSmm: Reduce SMRAM
consumption in CpuS3.c", 2021-01-11), simply leave both
"AcpiCpuData->RegisterTable" and "AcpiCpuData->PreSmmInitRegisterTable"
initialized to the zero address. This simplifies the driver, and saves
both normal RAM (boot services data type memory) and -- in PiSmmCpuDxeSmm
-- SMRAM.

(This simplification backs out a good chunk of commit 1158fc8e2c7b
("OvmfPkg/CpuS3DataDxe: enable S3 resume after CPU hotplug", 2020-03-04).
But CpuS3DataDxe still differs between UefiCpuPkg and OvmfPkg, due to the
latter supporting CPU hotplug; thus, we can't remove OvmfPkg/CpuS3DataDxe
altogether.)

Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3159
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/CpuS3DataDxe/CpuS3DataDxe.inf |  1 -
 OvmfPkg/CpuS3DataDxe/CpuS3Data.c      | 70 +-------------------
 2 files changed, 1 insertion(+), 70 deletions(-)

diff --git a/OvmfPkg/CpuS3DataDxe/CpuS3DataDxe.inf b/OvmfPkg/CpuS3DataDxe/CpuS3DataDxe.inf
index ceae1d4078c7..228d5ae1b260 100644
--- a/OvmfPkg/CpuS3DataDxe/CpuS3DataDxe.inf
+++ b/OvmfPkg/CpuS3DataDxe/CpuS3DataDxe.inf
@@ -42,7 +42,6 @@ [LibraryClasses]
   BaseLib
   BaseMemoryLib
   DebugLib
-  IoLib
   MemoryAllocationLib
   MtrrLib
   UefiBootServicesTableLib
diff --git a/OvmfPkg/CpuS3DataDxe/CpuS3Data.c b/OvmfPkg/CpuS3DataDxe/CpuS3Data.c
index bac7285aa2f3..5ffe1f3cd74e 100644
--- a/OvmfPkg/CpuS3DataDxe/CpuS3Data.c
+++ b/OvmfPkg/CpuS3DataDxe/CpuS3Data.c
@@ -23,7 +23,6 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 #include <Library/BaseLib.h>
 #include <Library/BaseMemoryLib.h>
 #include <Library/DebugLib.h>
-#include <Library/IoLib.h>
 #include <Library/MemoryAllocationLib.h>
 #include <Library/MtrrLib.h>
 #include <Library/UefiBootServicesTableLib.h>
@@ -31,9 +30,6 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 #include <Protocol/MpService.h>
 #include <Guid/EventGroup.h>
 
-#include <IndustryStandard/Q35MchIch9.h>
-#include <IndustryStandard/QemuCpuHotplug.h>
-
 //
 // Data structure used to allocate ACPI_CPU_DATA and its supporting structures
 //
@@ -168,17 +164,12 @@ CpuS3DataInitialize (
   EFI_MP_SERVICES_PROTOCOL   *MpServices;
   UINTN                      NumberOfCpus;
   VOID                       *Stack;
-  UINTN                      TableSize;
-  CPU_REGISTER_TABLE         *RegisterTable;
-  UINTN                      Index;
-  EFI_PROCESSOR_INFORMATION  ProcessorInfoBuffer;
   UINTN                      GdtSize;
   UINTN                      IdtSize;
   VOID                       *Gdt;
   VOID                       *Idt;
   EFI_EVENT                  Event;
   ACPI_CPU_DATA              *OldAcpiCpuData;
-  BOOLEAN                    FetchPossibleApicIds;
 
   if (!PcdGetBool (PcdAcpiS3Enable)) {
     return EFI_UNSUPPORTED;
@@ -193,13 +184,7 @@ CpuS3DataInitialize (
   ASSERT (AcpiCpuDataEx != NULL);
   AcpiCpuData = &AcpiCpuDataEx->AcpiCpuData;
 
-  //
-  // The "SMRAM at default SMBASE" feature guarantees that
-  // QEMU_CPUHP_CMD_GET_ARCH_ID too is available.
-  //
-  FetchPossibleApicIds = PcdGetBool (PcdQ35SmramAtDefaultSmbase);
-
-  if (FetchPossibleApicIds) {
+  if (PcdGetBool (PcdQ35SmramAtDefaultSmbase)) {
     NumberOfCpus = PcdGet32 (PcdCpuMaxLogicalProcessorNumber);
   } else {
     UINTN NumberOfEnabledProcessors;
@@ -271,59 +256,6 @@ CpuS3DataInitialize (
     AcpiCpuData->PreSmmInitRegisterTable = OldAcpiCpuData->PreSmmInitRegisterTable;
     AcpiCpuData->ApLocation              = OldAcpiCpuData->ApLocation;
     CopyMem (&AcpiCpuData->CpuStatus, &OldAcpiCpuData->CpuStatus, sizeof (CPU_STATUS_INFORMATION));
-  } else {
-    //
-    // Allocate buffer for empty RegisterTable and PreSmmInitRegisterTable for all CPUs
-    //
-    TableSize = 2 * NumberOfCpus * sizeof (CPU_REGISTER_TABLE);
-    RegisterTable = (CPU_REGISTER_TABLE *)AllocateZeroPages (TableSize);
-    ASSERT (RegisterTable != NULL);
-
-    if (FetchPossibleApicIds) {
-      //
-      // Write a valid selector so that other hotplug registers can be
-      // accessed.
-      //
-      IoWrite32 (ICH9_CPU_HOTPLUG_BASE + QEMU_CPUHP_W_CPU_SEL, 0);
-      //
-      // We'll be fetching the APIC IDs.
-      //
-      IoWrite8 (ICH9_CPU_HOTPLUG_BASE + QEMU_CPUHP_W_CMD,
-        QEMU_CPUHP_CMD_GET_ARCH_ID);
-    }
-    for (Index = 0; Index < NumberOfCpus; Index++) {
-      UINT32 InitialApicId;
-
-      if (FetchPossibleApicIds) {
-        IoWrite32 (ICH9_CPU_HOTPLUG_BASE + QEMU_CPUHP_W_CPU_SEL,
-          (UINT32)Index);
-        InitialApicId = IoRead32 (
-                          ICH9_CPU_HOTPLUG_BASE + QEMU_CPUHP_RW_CMD_DATA);
-      } else {
-        Status = MpServices->GetProcessorInfo (
-                             MpServices,
-                             Index,
-                             &ProcessorInfoBuffer
-                             );
-        ASSERT_EFI_ERROR (Status);
-        InitialApicId = (UINT32)ProcessorInfoBuffer.ProcessorId;
-      }
-
-      DEBUG ((DEBUG_VERBOSE, "%a: Index=%05Lu ApicId=0x%08x\n", __FUNCTION__,
-        (UINT64)Index, InitialApicId));
-
-      RegisterTable[Index].InitialApicId      = InitialApicId;
-      RegisterTable[Index].TableLength        = 0;
-      RegisterTable[Index].AllocatedSize      = 0;
-      RegisterTable[Index].RegisterTableEntry = 0;
-
-      RegisterTable[NumberOfCpus + Index].InitialApicId      = InitialApicId;
-      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);
   }
 
   //
-- 
2.19.1.3.g30247aa5d201

Re: [PATCH 3/3] OvmfPkg/CpuS3DataDxe: do not allocate useless register tables

[Ard Biesheuvel]

On 1/12/21 8:19 PM, Laszlo Ersek wrote:
> CpuS3DataDxe allocates the "RegisterTable" and "PreSmmInitRegisterTable"
> arrays in ACPI_CPU_DATA just so every processor in the system can have its
> own empty register table, matched by APIC ID. This has never been useful
> in practice.
> 
> Given commit e992cc3f4859 ("UefiCpuPkg PiSmmCpuDxeSmm: Reduce SMRAM
> consumption in CpuS3.c", 2021-01-11), simply leave both
> "AcpiCpuData->RegisterTable" and "AcpiCpuData->PreSmmInitRegisterTable"
> initialized to the zero address. This simplifies the driver, and saves
> both normal RAM (boot services data type memory) and -- in PiSmmCpuDxeSmm
> -- SMRAM.
> 
> (This simplification backs out a good chunk of commit 1158fc8e2c7b
> ("OvmfPkg/CpuS3DataDxe: enable S3 resume after CPU hotplug", 2020-03-04).
> But CpuS3DataDxe still differs between UefiCpuPkg and OvmfPkg, due to the
> latter supporting CPU hotplug; thus, we can't remove OvmfPkg/CpuS3DataDxe
> altogether.)
> 
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3159
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>

Acked-by: Ard Biesheuvel <ard.biesheuvel@arm.com>

> ---
>  OvmfPkg/CpuS3DataDxe/CpuS3DataDxe.inf |  1 -
>  OvmfPkg/CpuS3DataDxe/CpuS3Data.c      | 70 +-------------------
>  2 files changed, 1 insertion(+), 70 deletions(-)
> 
> diff --git a/OvmfPkg/CpuS3DataDxe/CpuS3DataDxe.inf b/OvmfPkg/CpuS3DataDxe/CpuS3DataDxe.inf
> index ceae1d4078c7..228d5ae1b260 100644
> --- a/OvmfPkg/CpuS3DataDxe/CpuS3DataDxe.inf
> +++ b/OvmfPkg/CpuS3DataDxe/CpuS3DataDxe.inf
> @@ -42,7 +42,6 @@ [LibraryClasses]
>    BaseLib
>    BaseMemoryLib
>    DebugLib
> -  IoLib
>    MemoryAllocationLib
>    MtrrLib
>    UefiBootServicesTableLib
> diff --git a/OvmfPkg/CpuS3DataDxe/CpuS3Data.c b/OvmfPkg/CpuS3DataDxe/CpuS3Data.c
> index bac7285aa2f3..5ffe1f3cd74e 100644
> --- a/OvmfPkg/CpuS3DataDxe/CpuS3Data.c
> +++ b/OvmfPkg/CpuS3DataDxe/CpuS3Data.c
> @@ -23,7 +23,6 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>  #include <Library/BaseLib.h>
>  #include <Library/BaseMemoryLib.h>
>  #include <Library/DebugLib.h>
> -#include <Library/IoLib.h>
>  #include <Library/MemoryAllocationLib.h>
>  #include <Library/MtrrLib.h>
>  #include <Library/UefiBootServicesTableLib.h>
> @@ -31,9 +30,6 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>  #include <Protocol/MpService.h>
>  #include <Guid/EventGroup.h>
>  
> -#include <IndustryStandard/Q35MchIch9.h>
> -#include <IndustryStandard/QemuCpuHotplug.h>
> -
>  //
>  // Data structure used to allocate ACPI_CPU_DATA and its supporting structures
>  //
> @@ -168,17 +164,12 @@ CpuS3DataInitialize (
>    EFI_MP_SERVICES_PROTOCOL   *MpServices;
>    UINTN                      NumberOfCpus;
>    VOID                       *Stack;
> -  UINTN                      TableSize;
> -  CPU_REGISTER_TABLE         *RegisterTable;
> -  UINTN                      Index;
> -  EFI_PROCESSOR_INFORMATION  ProcessorInfoBuffer;
>    UINTN                      GdtSize;
>    UINTN                      IdtSize;
>    VOID                       *Gdt;
>    VOID                       *Idt;
>    EFI_EVENT                  Event;
>    ACPI_CPU_DATA              *OldAcpiCpuData;
> -  BOOLEAN                    FetchPossibleApicIds;
>  
>    if (!PcdGetBool (PcdAcpiS3Enable)) {
>      return EFI_UNSUPPORTED;
> @@ -193,13 +184,7 @@ CpuS3DataInitialize (
>    ASSERT (AcpiCpuDataEx != NULL);
>    AcpiCpuData = &AcpiCpuDataEx->AcpiCpuData;
>  
> -  //
> -  // The "SMRAM at default SMBASE" feature guarantees that
> -  // QEMU_CPUHP_CMD_GET_ARCH_ID too is available.
> -  //
> -  FetchPossibleApicIds = PcdGetBool (PcdQ35SmramAtDefaultSmbase);
> -
> -  if (FetchPossibleApicIds) {
> +  if (PcdGetBool (PcdQ35SmramAtDefaultSmbase)) {
>      NumberOfCpus = PcdGet32 (PcdCpuMaxLogicalProcessorNumber);
>    } else {
>      UINTN NumberOfEnabledProcessors;
> @@ -271,59 +256,6 @@ CpuS3DataInitialize (
>      AcpiCpuData->PreSmmInitRegisterTable = OldAcpiCpuData->PreSmmInitRegisterTable;
>      AcpiCpuData->ApLocation              = OldAcpiCpuData->ApLocation;
>      CopyMem (&AcpiCpuData->CpuStatus, &OldAcpiCpuData->CpuStatus, sizeof (CPU_STATUS_INFORMATION));
> -  } else {
> -    //
> -    // Allocate buffer for empty RegisterTable and PreSmmInitRegisterTable for all CPUs
> -    //
> -    TableSize = 2 * NumberOfCpus * sizeof (CPU_REGISTER_TABLE);
> -    RegisterTable = (CPU_REGISTER_TABLE *)AllocateZeroPages (TableSize);
> -    ASSERT (RegisterTable != NULL);
> -
> -    if (FetchPossibleApicIds) {
> -      //
> -      // Write a valid selector so that other hotplug registers can be
> -      // accessed.
> -      //
> -      IoWrite32 (ICH9_CPU_HOTPLUG_BASE + QEMU_CPUHP_W_CPU_SEL, 0);
> -      //
> -      // We'll be fetching the APIC IDs.
> -      //
> -      IoWrite8 (ICH9_CPU_HOTPLUG_BASE + QEMU_CPUHP_W_CMD,
> -        QEMU_CPUHP_CMD_GET_ARCH_ID);
> -    }
> -    for (Index = 0; Index < NumberOfCpus; Index++) {
> -      UINT32 InitialApicId;
> -
> -      if (FetchPossibleApicIds) {
> -        IoWrite32 (ICH9_CPU_HOTPLUG_BASE + QEMU_CPUHP_W_CPU_SEL,
> -          (UINT32)Index);
> -        InitialApicId = IoRead32 (
> -                          ICH9_CPU_HOTPLUG_BASE + QEMU_CPUHP_RW_CMD_DATA);
> -      } else {
> -        Status = MpServices->GetProcessorInfo (
> -                             MpServices,
> -                             Index,
> -                             &ProcessorInfoBuffer
> -                             );
> -        ASSERT_EFI_ERROR (Status);
> -        InitialApicId = (UINT32)ProcessorInfoBuffer.ProcessorId;
> -      }
> -
> -      DEBUG ((DEBUG_VERBOSE, "%a: Index=%05Lu ApicId=0x%08x\n", __FUNCTION__,
> -        (UINT64)Index, InitialApicId));
> -
> -      RegisterTable[Index].InitialApicId      = InitialApicId;
> -      RegisterTable[Index].TableLength        = 0;
> -      RegisterTable[Index].AllocatedSize      = 0;
> -      RegisterTable[Index].RegisterTableEntry = 0;
> -
> -      RegisterTable[NumberOfCpus + Index].InitialApicId      = InitialApicId;
> -      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);
>    }
>  
>    //
>