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

[PATCH v2 0/4] 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-v2

Updates in v2:

- v1 patches have not received any updates, I've only picked up the
  feedback tags.

- Patch v2 #1 -- for RegisterCpuFeaturesLib -- is new; authored by Ray
  and updated slightly by myself. Star and/or Eric should please approve
  this patch.

v1 was posted at:

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

  Message-Id: <20210112191934.12459-1-lersek@redhat.com>
  https://edk2.groups.io/g/devel/message/70167
  https://www.redhat.com/archives/edk2-devel-archive/2021-January/msg00652.html

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

Ray Ni (1):
  UefiCpuPkg/CpuFeature: Don't assume CpuS3DataDxe alloc RegisterTable

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


base-commit: 83facfd184021874f95a6a34b2e47e0ebb34a762
-- 
2.19.1.3.g30247aa5d201

[PATCH v2 1/4] UefiCpuPkg/CpuFeature: Don't assume CpuS3DataDxe alloc RegisterTable

[Laszlo Ersek]

From: Ray Ni <ray.ni@intel.com>

There are lots of fields in ACPI_CPU_DATA structure while only
followings are accessed by CpuFeature infra:
* NumberOfCpus
* PreSmmInitRegisterTable // pointer
* RegisterTable  // pointer
* CpuStatus
* ApLocation  // pointer

So it's possible that an implementation of CpuS3DataDxe doesn't
allocate memory for PreSmmInitRegisterTable/RegisterTable/ApLocation.

This patch handles the case when CpuS3DataDxe doesn't allocate
memory for PreSmmInitRegisterTable/RegisterTable.

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: Ray Ni <ray.ni@intel.com>
[lersek@redhat.com: update CC list, add BZ reference, add my S-o-b]
[lersek@redhat.com: deal with RegisterTable and PreSmmInitRegisterTable
 being zero independently of each other; replacing the ASSERT()]
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---

Notes:
    v2:
    
    - new patch, cherry-picked from
      <https://github.com/niruiyu/edk2/commit/7091aa50b9d8>
    
    - Update as follows: do not assume that (RegisterTable == 0) implies
      (PreSmmInitRegisterTable == 0). The ACPI_CPU_DATA documentation update
      in the next patch permits each field to be zero in separation, and
      updating Ray's patch for accommodating that is not difficult. Some
      normal RAM may be wasted if exactly one of the "RegisterTable" and
      "PreSmmInitRegisterTable" fields is zero (the memory is allocated with
      the PEI or DXE instance of MemoryAllocationLib).
    
    - this patch is easiest to review with "git show -b -W"

 UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c | 80 +++++++++++---------
 1 file changed, 43 insertions(+), 37 deletions(-)

diff --git a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
index 4063d45760ad..7bb92404027f 100644
--- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
+++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
@@ -1,7 +1,7 @@
 /** @file
   CPU Register Table Library functions.
 
-  Copyright (c) 2017 - 2020, Intel Corporation. All rights reserved.<BR>
+  Copyright (c) 2017 - 2021, Intel Corporation. All rights reserved.<BR>
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
 **/
@@ -937,45 +937,51 @@ 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);
-
-  //
-  // 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;
-
-  //
-  // Allocate buffer for empty RegisterTable and PreSmmInitRegisterTable for all CPUs
-  //
-  TableSize = 2 * NumberOfCpus * sizeof (CPU_REGISTER_TABLE);
-  RegisterTable  = AllocatePages (EFI_SIZE_TO_PAGES (TableSize));
-  ASSERT (RegisterTable != NULL);
-
-  for (Index = 0; Index < NumberOfCpus; Index++) {
-    Status = GetProcessorInformation (Index, &ProcessorInfoBuffer);
+  if (AcpiCpuData == NULL) {
+    AcpiCpuData  = AllocatePages (EFI_SIZE_TO_PAGES (sizeof (ACPI_CPU_DATA)));
+    ASSERT (AcpiCpuData != NULL);
+    ZeroMem (AcpiCpuData, sizeof (ACPI_CPU_DATA));
+
+    //
+    // Set PcdCpuS3DataAddress to the base address of the ACPI_CPU_DATA structure
+    //
+    Status = PcdSet64S (PcdCpuS3DataAddress, (UINT64)(UINTN)AcpiCpuData);
     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;
+    GetNumberOfProcessor (&NumberOfCpus, &NumberOfEnabledProcessors);
+    AcpiCpuData->NumberOfCpus = (UINT32)NumberOfCpus;
+  }
+
+  if (AcpiCpuData->RegisterTable == 0 ||
+      AcpiCpuData->PreSmmInitRegisterTable == 0) {
+    //
+    // Allocate buffer for empty RegisterTable and PreSmmInitRegisterTable for all CPUs
+    //
+    TableSize = 2 * NumberOfCpus * sizeof (CPU_REGISTER_TABLE);
+    RegisterTable  = AllocatePages (EFI_SIZE_TO_PAGES (TableSize));
+    ASSERT (RegisterTable != NULL);
+
+    for (Index = 0; Index < NumberOfCpus; Index++) {
+      Status = GetProcessorInformation (Index, &ProcessorInfoBuffer);
+      ASSERT_EFI_ERROR (Status);
+
+      RegisterTable[Index].InitialApicId      = (UINT32)ProcessorInfoBuffer.ProcessorId;
+      RegisterTable[Index].TableLength        = 0;
+      RegisterTable[Index].AllocatedSize      = 0;
+      RegisterTable[Index].RegisterTableEntry = 0;
+
+      RegisterTable[NumberOfCpus + Index].InitialApicId      = (UINT32)ProcessorInfoBuffer.ProcessorId;
+      RegisterTable[NumberOfCpus + Index].TableLength        = 0;
+      RegisterTable[NumberOfCpus + Index].AllocatedSize      = 0;
+      RegisterTable[NumberOfCpus + Index].RegisterTableEntry = 0;
+    }
+    if (AcpiCpuData->RegisterTable == 0) {
+      AcpiCpuData->RegisterTable = (EFI_PHYSICAL_ADDRESS)(UINTN)RegisterTable;
+    }
+    if (AcpiCpuData->PreSmmInitRegisterTable == 0) {
+      AcpiCpuData->PreSmmInitRegisterTable = (EFI_PHYSICAL_ADDRESS)(UINTN)(RegisterTable + NumberOfCpus);
+    }
   }
-  AcpiCpuData->RegisterTable           = (EFI_PHYSICAL_ADDRESS)(UINTN)RegisterTable;
-  AcpiCpuData->PreSmmInitRegisterTable = (EFI_PHYSICAL_ADDRESS)(UINTN)(RegisterTable + NumberOfCpus);
 
   return AcpiCpuData;
 }
-- 
2.19.1.3.g30247aa5d201

[PATCH v2 2/4] 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>
Reviewed-by: Ray Ni <ray.ni@intel.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---

Notes:
    v2:
    
    - no changes, pick up feedback tags

 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

[PATCH v2 3/4] 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>
Reviewed-by: Ray Ni <ray.ni@intel.com>
---

Notes:
    v2:
    
    - no changes, pick up feedback tags
    
    v1:
    
    - 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 v2 4/4] 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>
Acked-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
---

Notes:
    v2:
    
    - no changes, pick up feedback tags

 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: [edk2-devel] [PATCH v2 0/4] UefiCpuPkg, OvmfPkg: do not allocate useless register tables for S3 resume

[Laszlo Ersek]

On 01/19/21 16:54, Laszlo Ersek wrote:
> Ref:    https://bugzilla.tianocore.org/show_bug.cgi?id=3159
> Repo:   https://pagure.io/lersek/edk2.git
> Branch: remove-cpu-reg-table-alloc-3159-v2
> 
> Updates in v2:
> 
> - v1 patches have not received any updates, I've only picked up the
>   feedback tags.

I didn't regenerate the CC's for the v1-originated patches, so the CC
list still uses Ard's @arm.com email address. I don't think that's a
problem for this specific posting (and I expect to merge this series
once Eric or Star approves patch v2 #1).

Thanks
Laszlo

> 
> - Patch v2 #1 -- for RegisterCpuFeaturesLib -- is new; authored by Ray
>   and updated slightly by myself. Star and/or Eric should please approve
>   this patch.
> 
> v1 was posted at:
> 
>   [edk2-devel] [PATCH 0/3] UefiCpuPkg, OvmfPkg: do not allocate useless
>   register tables for S3 resume
> 
>   Message-Id: <20210112191934.12459-1-lersek@redhat.com>
>   https://edk2.groups.io/g/devel/message/70167
>   https://www.redhat.com/archives/edk2-devel-archive/2021-January/msg00652.html
> 
> 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
> 
> Ray Ni (1):
>   UefiCpuPkg/CpuFeature: Don't assume CpuS3DataDxe alloc RegisterTable
> 
>  OvmfPkg/CpuS3DataDxe/CpuS3Data.c                                   | 70 +----------------
>  OvmfPkg/CpuS3DataDxe/CpuS3DataDxe.inf                              |  1 -
>  UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c                                | 32 --------
>  UefiCpuPkg/Include/AcpiCpuData.h                                   |  4 +
>  UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c | 80 +++++++++++---------
>  5 files changed, 48 insertions(+), 139 deletions(-)
> 
> 
> base-commit: 83facfd184021874f95a6a34b2e47e0ebb34a762
> 

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

[Zeng, Star]

Series Reviewed-by: Star Zeng <star.zeng@intel.com>

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Tuesday, January 19, 2021 11:55 PM
> To: edk2-devel-groups-io <devel@edk2.groups.io>
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>; Dong, Eric
> <eric.dong@intel.com>; Justen, Jordan L <jordan.l.justen@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 v2 0/4] UefiCpuPkg, OvmfPkg: do not allocate useless
> register tables for S3 resume
> 
> Ref:    https://bugzilla.tianocore.org/show_bug.cgi?id=3159
> Repo:   https://pagure.io/lersek/edk2.git
> Branch: remove-cpu-reg-table-alloc-3159-v2
> 
> Updates in v2:
> 
> - v1 patches have not received any updates, I've only picked up the
>   feedback tags.
> 
> - Patch v2 #1 -- for RegisterCpuFeaturesLib -- is new; authored by Ray
>   and updated slightly by myself. Star and/or Eric should please approve
>   this patch.
> 
> v1 was posted at:
> 
>   [edk2-devel] [PATCH 0/3] UefiCpuPkg, OvmfPkg: do not allocate useless
>   register tables for S3 resume
> 
>   Message-Id: <20210112191934.12459-1-lersek@redhat.com>
>   https://edk2.groups.io/g/devel/message/70167
>   https://www.redhat.com/archives/edk2-devel-archive/2021-
> January/msg00652.html
> 
> 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
> 
> Ray Ni (1):
>   UefiCpuPkg/CpuFeature: Don't assume CpuS3DataDxe alloc RegisterTable
> 
>  OvmfPkg/CpuS3DataDxe/CpuS3Data.c                                   | 70 +---------------
> -
>  OvmfPkg/CpuS3DataDxe/CpuS3DataDxe.inf                              |  1 -
>  UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c                                | 32 --------
>  UefiCpuPkg/Include/AcpiCpuData.h                                   |  4 +
>  UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c | 80
> +++++++++++---------
>  5 files changed, 48 insertions(+), 139 deletions(-)
> 
> 
> base-commit: 83facfd184021874f95a6a34b2e47e0ebb34a762
> --
> 2.19.1.3.g30247aa5d201

Re: [edk2-devel] [PATCH v2 0/4] UefiCpuPkg, OvmfPkg: do not allocate useless register tables for S3 resume

[Laszlo Ersek]

On 01/20/21 10:28, Zeng, Star wrote:
> Series Reviewed-by: Star Zeng <star.zeng@intel.com>

Series merged as commit range e843a21e23ea..339371ef78eb, via
<https://github.com/tianocore/edk2/pull/1373>.

Thanks!
Laszlo

> 
>> -----Original Message-----
>> From: Laszlo Ersek <lersek@redhat.com>
>> Sent: Tuesday, January 19, 2021 11:55 PM
>> To: edk2-devel-groups-io <devel@edk2.groups.io>
>> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>; Dong, Eric
>> <eric.dong@intel.com>; Justen, Jordan L <jordan.l.justen@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 v2 0/4] UefiCpuPkg, OvmfPkg: do not allocate useless
>> register tables for S3 resume
>>
>> Ref:    https://bugzilla.tianocore.org/show_bug.cgi?id=3159
>> Repo:   https://pagure.io/lersek/edk2.git
>> Branch: remove-cpu-reg-table-alloc-3159-v2
>>
>> Updates in v2:
>>
>> - v1 patches have not received any updates, I've only picked up the
>>   feedback tags.
>>
>> - Patch v2 #1 -- for RegisterCpuFeaturesLib -- is new; authored by Ray
>>   and updated slightly by myself. Star and/or Eric should please approve
>>   this patch.
>>
>> v1 was posted at:
>>
>>   [edk2-devel] [PATCH 0/3] UefiCpuPkg, OvmfPkg: do not allocate useless
>>   register tables for S3 resume
>>
>>   Message-Id: <20210112191934.12459-1-lersek@redhat.com>
>>   https://edk2.groups.io/g/devel/message/70167
>>   https://www.redhat.com/archives/edk2-devel-archive/2021-
>> January/msg00652.html
>>
>> 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
>>
>> Ray Ni (1):
>>   UefiCpuPkg/CpuFeature: Don't assume CpuS3DataDxe alloc RegisterTable
>>
>>  OvmfPkg/CpuS3DataDxe/CpuS3Data.c                                   | 70 +---------------
>> -
>>  OvmfPkg/CpuS3DataDxe/CpuS3DataDxe.inf                              |  1 -
>>  UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c                                | 32 --------
>>  UefiCpuPkg/Include/AcpiCpuData.h                                   |  4 +
>>  UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c | 80
>> +++++++++++---------
>>  5 files changed, 48 insertions(+), 139 deletions(-)
>>
>>
>> base-commit: 83facfd184021874f95a6a34b2e47e0ebb34a762
>> --
>> 2.19.1.3.g30247aa5d201
> 
> 
> 
> 
> 
>