1. devel@edk2.groups.io
Topics

[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
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
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
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
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
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
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
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
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
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
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
1 - 11 of 11  
More
Home Messages Hashtags Subgroups Calendar About Features Pricing Updates Terms Help Twitter