* [PATCH 0/3] UefiCpuPkg, OvmfPkg: do not allocate useless register tables for S3 resume
@ 2021-01-12 19:19 Laszlo Ersek
2021-01-12 19:19 ` [PATCH 1/3] UefiCpuPkg/AcpiCpuData: update comments on register table fields Laszlo Ersek
` (2 more replies)
0 siblings, 3 replies; 20+ messages in thread
From: Laszlo Ersek @ 2021-01-12 19:19 UTC (permalink / raw)
To: devel
Cc: Ard Biesheuvel, Eric Dong, Jordan Justen,
Philippe Mathieu-Daudé, Rahul Kumar, Ray Ni, Star Zeng
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3159
Repo: https://pagure.io/lersek/edk2.git
Branch: remove-cpu-reg-table-alloc-3159
OvmfPkg/CpuS3DataDxe and UefiCpuPkg/CpuS3DataDxe allocate the
RegisterTable and PreSmmInitRegisterTable arrays in ACPI_CPU_DATA just
so every processor in the system can have its own empty register table,
matched by APIC ID, at S3 resume. This has never been useful.
Given recent commit e992cc3f4859 ("UefiCpuPkg PiSmmCpuDxeSmm: Reduce
SMRAM consumption in CpuS3.c", 2021-01-11), i.e., the NULL-check in the
IsRegisterTableEmpty() function, we should now set both RegisterTable
and PreSmmInitRegisterTable in ACPI_CPU_DATA to NULL, in the
CpuS3DataDxe drivers. That will simplify the drivers, save some normal
RAM in them, and accordingly, save some SMRAM in PiSmmCpuDxeSmm too.
The first patch in the series also updates the documentation of
ACPI_CPU_DATA (which, arguably, should have been done in commit
e992cc3f4859).
Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Thanks
Laszlo
Laszlo Ersek (3):
UefiCpuPkg/AcpiCpuData: update comments on register table fields
UefiCpuPkg/CpuS3DataDxe: do not allocate useless register tables
OvmfPkg/CpuS3DataDxe: do not allocate useless register tables
OvmfPkg/CpuS3DataDxe/CpuS3Data.c | 70 +-------------------
OvmfPkg/CpuS3DataDxe/CpuS3DataDxe.inf | 1 -
UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c | 32 ---------
UefiCpuPkg/Include/AcpiCpuData.h | 4 ++
4 files changed, 5 insertions(+), 102 deletions(-)
base-commit: ebfe2d3eb5ac7fd92d74011edb31303a181920c7
--
2.19.1.3.g30247aa5d201
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 1/3] UefiCpuPkg/AcpiCpuData: update comments on register table fields
2021-01-12 19:19 [PATCH 0/3] UefiCpuPkg, OvmfPkg: do not allocate useless register tables for S3 resume Laszlo Ersek
@ 2021-01-12 19:19 ` Laszlo Ersek
2021-01-13 5:00 ` Ni, Ray
2021-01-15 9:28 ` Philippe Mathieu-Daudé
2021-01-12 19:19 ` [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate useless register tables Laszlo Ersek
2021-01-12 19:19 ` [PATCH 3/3] OvmfPkg/CpuS3DataDxe: " Laszlo Ersek
2 siblings, 2 replies; 20+ messages in thread
From: Laszlo Ersek @ 2021-01-12 19:19 UTC (permalink / raw)
To: devel
Cc: Eric Dong, Philippe Mathieu-Daudé, Rahul Kumar, Ray Ni,
Star Zeng
After commit e992cc3f4859 ("UefiCpuPkg PiSmmCpuDxeSmm: Reduce SMRAM
consumption in CpuS3.c", 2021-01-11), it is valid for a CPU S3 Data DXE
Driver to set "ACPI_CPU_DATA.PreSmmInitRegisterTable" and/or
"ACPI_CPU_DATA.RegisterTable" to 0, in case none of the CPUs needs a
register table of the corresponding kind, during S3 resume.
Document this fact in the "UefiCpuPkg/Include/AcpiCpuData.h" header file.
Cc: Eric Dong <eric.dong@intel.com>
Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3159
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
UefiCpuPkg/Include/AcpiCpuData.h | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/UefiCpuPkg/Include/AcpiCpuData.h b/UefiCpuPkg/Include/AcpiCpuData.h
index b5a69ad80c88..62a01b2c6bac 100644
--- a/UefiCpuPkg/Include/AcpiCpuData.h
+++ b/UefiCpuPkg/Include/AcpiCpuData.h
@@ -178,6 +178,8 @@ typedef struct {
// If TableLength is > 0, then elements of RegisterTableEntry are used to
// initialize the CPU that matches InitialApicId, during an ACPI S3 resume,
// before SMBASE relocation is performed.
+ // If a register table is not required for any one of the CPUs, then
+ // PreSmmInitRegisterTable may be set to 0.
//
EFI_PHYSICAL_ADDRESS PreSmmInitRegisterTable;
//
@@ -187,6 +189,8 @@ typedef struct {
// If TableLength is > 0, then elements of RegisterTableEntry are used to
// initialize the CPU that matches InitialApicId, during an ACPI S3 resume,
// after SMBASE relocation is performed.
+ // If a register table is not required for any one of the CPUs, then
+ // RegisterTable may be set to 0.
//
EFI_PHYSICAL_ADDRESS RegisterTable;
//
--
2.19.1.3.g30247aa5d201
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate useless register tables
2021-01-12 19:19 [PATCH 0/3] UefiCpuPkg, OvmfPkg: do not allocate useless register tables for S3 resume Laszlo Ersek
2021-01-12 19:19 ` [PATCH 1/3] UefiCpuPkg/AcpiCpuData: update comments on register table fields Laszlo Ersek
@ 2021-01-12 19:19 ` Laszlo Ersek
2021-01-13 6:12 ` Ni, Ray
2021-01-12 19:19 ` [PATCH 3/3] OvmfPkg/CpuS3DataDxe: " Laszlo Ersek
2 siblings, 1 reply; 20+ messages in thread
From: Laszlo Ersek @ 2021-01-12 19:19 UTC (permalink / raw)
To: devel
Cc: Eric Dong, Philippe Mathieu-Daudé, Rahul Kumar, Ray Ni,
Star Zeng
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
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 3/3] OvmfPkg/CpuS3DataDxe: do not allocate useless register tables
2021-01-12 19:19 [PATCH 0/3] UefiCpuPkg, OvmfPkg: do not allocate useless register tables for S3 resume Laszlo Ersek
2021-01-12 19:19 ` [PATCH 1/3] UefiCpuPkg/AcpiCpuData: update comments on register table fields Laszlo Ersek
2021-01-12 19:19 ` [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate useless register tables Laszlo Ersek
@ 2021-01-12 19:19 ` Laszlo Ersek
2021-01-14 12:53 ` Ard Biesheuvel
2 siblings, 1 reply; 20+ messages in thread
From: Laszlo Ersek @ 2021-01-12 19:19 UTC (permalink / raw)
To: devel; +Cc: Ard Biesheuvel, Jordan Justen, Philippe Mathieu-Daudé
CpuS3DataDxe allocates the "RegisterTable" and "PreSmmInitRegisterTable"
arrays in ACPI_CPU_DATA just so every processor in the system can have its
own empty register table, matched by APIC ID. This has never been useful
in practice.
Given commit e992cc3f4859 ("UefiCpuPkg PiSmmCpuDxeSmm: Reduce SMRAM
consumption in CpuS3.c", 2021-01-11), simply leave both
"AcpiCpuData->RegisterTable" and "AcpiCpuData->PreSmmInitRegisterTable"
initialized to the zero address. This simplifies the driver, and saves
both normal RAM (boot services data type memory) and -- in PiSmmCpuDxeSmm
-- SMRAM.
(This simplification backs out a good chunk of commit 1158fc8e2c7b
("OvmfPkg/CpuS3DataDxe: enable S3 resume after CPU hotplug", 2020-03-04).
But CpuS3DataDxe still differs between UefiCpuPkg and OvmfPkg, due to the
latter supporting CPU hotplug; thus, we can't remove OvmfPkg/CpuS3DataDxe
altogether.)
Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3159
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
OvmfPkg/CpuS3DataDxe/CpuS3DataDxe.inf | 1 -
OvmfPkg/CpuS3DataDxe/CpuS3Data.c | 70 +-------------------
2 files changed, 1 insertion(+), 70 deletions(-)
diff --git a/OvmfPkg/CpuS3DataDxe/CpuS3DataDxe.inf b/OvmfPkg/CpuS3DataDxe/CpuS3DataDxe.inf
index ceae1d4078c7..228d5ae1b260 100644
--- a/OvmfPkg/CpuS3DataDxe/CpuS3DataDxe.inf
+++ b/OvmfPkg/CpuS3DataDxe/CpuS3DataDxe.inf
@@ -42,7 +42,6 @@ [LibraryClasses]
BaseLib
BaseMemoryLib
DebugLib
- IoLib
MemoryAllocationLib
MtrrLib
UefiBootServicesTableLib
diff --git a/OvmfPkg/CpuS3DataDxe/CpuS3Data.c b/OvmfPkg/CpuS3DataDxe/CpuS3Data.c
index bac7285aa2f3..5ffe1f3cd74e 100644
--- a/OvmfPkg/CpuS3DataDxe/CpuS3Data.c
+++ b/OvmfPkg/CpuS3DataDxe/CpuS3Data.c
@@ -23,7 +23,6 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
#include <Library/BaseLib.h>
#include <Library/BaseMemoryLib.h>
#include <Library/DebugLib.h>
-#include <Library/IoLib.h>
#include <Library/MemoryAllocationLib.h>
#include <Library/MtrrLib.h>
#include <Library/UefiBootServicesTableLib.h>
@@ -31,9 +30,6 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
#include <Protocol/MpService.h>
#include <Guid/EventGroup.h>
-#include <IndustryStandard/Q35MchIch9.h>
-#include <IndustryStandard/QemuCpuHotplug.h>
-
//
// Data structure used to allocate ACPI_CPU_DATA and its supporting structures
//
@@ -168,17 +164,12 @@ CpuS3DataInitialize (
EFI_MP_SERVICES_PROTOCOL *MpServices;
UINTN NumberOfCpus;
VOID *Stack;
- UINTN TableSize;
- CPU_REGISTER_TABLE *RegisterTable;
- UINTN Index;
- EFI_PROCESSOR_INFORMATION ProcessorInfoBuffer;
UINTN GdtSize;
UINTN IdtSize;
VOID *Gdt;
VOID *Idt;
EFI_EVENT Event;
ACPI_CPU_DATA *OldAcpiCpuData;
- BOOLEAN FetchPossibleApicIds;
if (!PcdGetBool (PcdAcpiS3Enable)) {
return EFI_UNSUPPORTED;
@@ -193,13 +184,7 @@ CpuS3DataInitialize (
ASSERT (AcpiCpuDataEx != NULL);
AcpiCpuData = &AcpiCpuDataEx->AcpiCpuData;
- //
- // The "SMRAM at default SMBASE" feature guarantees that
- // QEMU_CPUHP_CMD_GET_ARCH_ID too is available.
- //
- FetchPossibleApicIds = PcdGetBool (PcdQ35SmramAtDefaultSmbase);
-
- if (FetchPossibleApicIds) {
+ if (PcdGetBool (PcdQ35SmramAtDefaultSmbase)) {
NumberOfCpus = PcdGet32 (PcdCpuMaxLogicalProcessorNumber);
} else {
UINTN NumberOfEnabledProcessors;
@@ -271,59 +256,6 @@ CpuS3DataInitialize (
AcpiCpuData->PreSmmInitRegisterTable = OldAcpiCpuData->PreSmmInitRegisterTable;
AcpiCpuData->ApLocation = OldAcpiCpuData->ApLocation;
CopyMem (&AcpiCpuData->CpuStatus, &OldAcpiCpuData->CpuStatus, sizeof (CPU_STATUS_INFORMATION));
- } else {
- //
- // Allocate buffer for empty RegisterTable and PreSmmInitRegisterTable for all CPUs
- //
- TableSize = 2 * NumberOfCpus * sizeof (CPU_REGISTER_TABLE);
- RegisterTable = (CPU_REGISTER_TABLE *)AllocateZeroPages (TableSize);
- ASSERT (RegisterTable != NULL);
-
- if (FetchPossibleApicIds) {
- //
- // Write a valid selector so that other hotplug registers can be
- // accessed.
- //
- IoWrite32 (ICH9_CPU_HOTPLUG_BASE + QEMU_CPUHP_W_CPU_SEL, 0);
- //
- // We'll be fetching the APIC IDs.
- //
- IoWrite8 (ICH9_CPU_HOTPLUG_BASE + QEMU_CPUHP_W_CMD,
- QEMU_CPUHP_CMD_GET_ARCH_ID);
- }
- for (Index = 0; Index < NumberOfCpus; Index++) {
- UINT32 InitialApicId;
-
- if (FetchPossibleApicIds) {
- IoWrite32 (ICH9_CPU_HOTPLUG_BASE + QEMU_CPUHP_W_CPU_SEL,
- (UINT32)Index);
- InitialApicId = IoRead32 (
- ICH9_CPU_HOTPLUG_BASE + QEMU_CPUHP_RW_CMD_DATA);
- } else {
- Status = MpServices->GetProcessorInfo (
- MpServices,
- Index,
- &ProcessorInfoBuffer
- );
- ASSERT_EFI_ERROR (Status);
- InitialApicId = (UINT32)ProcessorInfoBuffer.ProcessorId;
- }
-
- DEBUG ((DEBUG_VERBOSE, "%a: Index=%05Lu ApicId=0x%08x\n", __FUNCTION__,
- (UINT64)Index, InitialApicId));
-
- RegisterTable[Index].InitialApicId = InitialApicId;
- RegisterTable[Index].TableLength = 0;
- RegisterTable[Index].AllocatedSize = 0;
- RegisterTable[Index].RegisterTableEntry = 0;
-
- RegisterTable[NumberOfCpus + Index].InitialApicId = InitialApicId;
- RegisterTable[NumberOfCpus + Index].TableLength = 0;
- RegisterTable[NumberOfCpus + Index].AllocatedSize = 0;
- RegisterTable[NumberOfCpus + Index].RegisterTableEntry = 0;
- }
- AcpiCpuData->RegisterTable = (EFI_PHYSICAL_ADDRESS)(UINTN)RegisterTable;
- AcpiCpuData->PreSmmInitRegisterTable = (EFI_PHYSICAL_ADDRESS)(UINTN)(RegisterTable + NumberOfCpus);
}
//
--
2.19.1.3.g30247aa5d201
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 1/3] UefiCpuPkg/AcpiCpuData: update comments on register table fields
2021-01-12 19:19 ` [PATCH 1/3] UefiCpuPkg/AcpiCpuData: update comments on register table fields Laszlo Ersek
@ 2021-01-13 5:00 ` Ni, Ray
2021-01-15 9:28 ` Philippe Mathieu-Daudé
1 sibling, 0 replies; 20+ messages in thread
From: Ni, Ray @ 2021-01-13 5:00 UTC (permalink / raw)
To: Laszlo Ersek, devel@edk2.groups.io
Cc: Dong, Eric, Philippe Mathieu-Daudé, Kumar, Rahul1,
Zeng, Star
Reviewed-by: Ray Ni <ray.ni@intel.com>
> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Wednesday, January 13, 2021 3:20 AM
> To: devel@edk2.groups.io
> Cc: Dong, Eric <eric.dong@intel.com>; Philippe Mathieu-Daudé
> <philmd@redhat.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>; Ni, Ray
> <ray.ni@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: [PATCH 1/3] UefiCpuPkg/AcpiCpuData: update comments on register
> table fields
>
> After commit e992cc3f4859 ("UefiCpuPkg PiSmmCpuDxeSmm: Reduce SMRAM
> consumption in CpuS3.c", 2021-01-11), it is valid for a CPU S3 Data DXE
> Driver to set "ACPI_CPU_DATA.PreSmmInitRegisterTable" and/or
> "ACPI_CPU_DATA.RegisterTable" to 0, in case none of the CPUs needs a
> register table of the corresponding kind, during S3 resume.
>
> Document this fact in the "UefiCpuPkg/Include/AcpiCpuData.h" header file.
>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3159
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
> UefiCpuPkg/Include/AcpiCpuData.h | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/UefiCpuPkg/Include/AcpiCpuData.h
> b/UefiCpuPkg/Include/AcpiCpuData.h
> index b5a69ad80c88..62a01b2c6bac 100644
> --- a/UefiCpuPkg/Include/AcpiCpuData.h
> +++ b/UefiCpuPkg/Include/AcpiCpuData.h
> @@ -178,6 +178,8 @@ typedef struct {
> // If TableLength is > 0, then elements of RegisterTableEntry are used to
> // initialize the CPU that matches InitialApicId, during an ACPI S3 resume,
> // before SMBASE relocation is performed.
> + // If a register table is not required for any one of the CPUs, then
> + // PreSmmInitRegisterTable may be set to 0.
> //
> EFI_PHYSICAL_ADDRESS PreSmmInitRegisterTable;
> //
> @@ -187,6 +189,8 @@ typedef struct {
> // If TableLength is > 0, then elements of RegisterTableEntry are used to
> // initialize the CPU that matches InitialApicId, during an ACPI S3 resume,
> // after SMBASE relocation is performed.
> + // If a register table is not required for any one of the CPUs, then
> + // RegisterTable may be set to 0.
> //
> EFI_PHYSICAL_ADDRESS RegisterTable;
> //
> --
> 2.19.1.3.g30247aa5d201
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate useless register tables
2021-01-12 19:19 ` [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate useless register tables Laszlo Ersek
@ 2021-01-13 6:12 ` Ni, Ray
2021-01-13 7:16 ` Zeng, Star
2021-01-13 9:10 ` Laszlo Ersek
0 siblings, 2 replies; 20+ messages in thread
From: Ni, Ray @ 2021-01-13 6:12 UTC (permalink / raw)
To: Laszlo Ersek, devel@edk2.groups.io
Cc: Dong, Eric, Philippe Mathieu-Daudé, Kumar, Rahul1,
Zeng, Star
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
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate useless register tables
2021-01-13 6:12 ` Ni, Ray
@ 2021-01-13 7:16 ` Zeng, Star
2021-01-13 8:12 ` Ni, Ray
2021-01-13 9:10 ` Laszlo Ersek
1 sibling, 1 reply; 20+ messages in thread
From: Zeng, Star @ 2021-01-13 7:16 UTC (permalink / raw)
To: Ni, Ray, Laszlo Ersek, devel@edk2.groups.io
Cc: Dong, Eric, Philippe Mathieu-Daudé, Kumar, Rahul1,
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
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate useless register tables
2021-01-13 7:16 ` Zeng, Star
@ 2021-01-13 8:12 ` Ni, Ray
2021-01-13 8:40 ` Zeng, Star
` (2 more replies)
0 siblings, 3 replies; 20+ messages in thread
From: Ni, Ray @ 2021-01-13 8:12 UTC (permalink / raw)
To: Zeng, Star, Laszlo Ersek, devel@edk2.groups.io
Cc: Dong, Eric, Philippe Mathieu-Daudé, Kumar, Rahul1
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
> >
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate useless register tables
2021-01-13 8:12 ` Ni, Ray
@ 2021-01-13 8:40 ` Zeng, Star
2021-01-13 9:16 ` Laszlo Ersek
2021-01-14 1:55 ` Zeng, Star
2 siblings, 0 replies; 20+ messages in thread
From: Zeng, Star @ 2021-01-13 8:40 UTC (permalink / raw)
To: Ni, Ray, Laszlo Ersek, devel@edk2.groups.io
Cc: Dong, Eric, Philippe Mathieu-Daudé, Kumar, Rahul1,
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
> >
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate useless register tables
2021-01-13 6:12 ` Ni, Ray
2021-01-13 7:16 ` Zeng, Star
@ 2021-01-13 9:10 ` Laszlo Ersek
1 sibling, 0 replies; 20+ messages in thread
From: Laszlo Ersek @ 2021-01-13 9:10 UTC (permalink / raw)
To: Ni, Ray, devel@edk2.groups.io
Cc: Dong, Eric, Philippe Mathieu-Daudé, Kumar, Rahul1,
Zeng, Star
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
>>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate useless register tables
2021-01-13 8:12 ` Ni, Ray
2021-01-13 8:40 ` Zeng, Star
@ 2021-01-13 9:16 ` Laszlo Ersek
2021-01-14 1:55 ` Zeng, Star
2 siblings, 0 replies; 20+ messages in thread
From: Laszlo Ersek @ 2021-01-13 9:16 UTC (permalink / raw)
To: Ni, Ray, Zeng, Star, devel@edk2.groups.io
Cc: Dong, Eric, Philippe Mathieu-Daudé, Kumar, Rahul1
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
>>>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate useless register tables
2021-01-13 8:12 ` Ni, Ray
2021-01-13 8:40 ` Zeng, Star
2021-01-13 9:16 ` Laszlo Ersek
@ 2021-01-14 1:55 ` Zeng, Star
2021-01-14 17:35 ` Laszlo Ersek
2 siblings, 1 reply; 20+ messages in thread
From: Zeng, Star @ 2021-01-14 1:55 UTC (permalink / raw)
To: Ni, Ray, Laszlo Ersek, devel@edk2.groups.io
Cc: Dong, Eric, Philippe Mathieu-Daudé, Kumar, Rahul1,
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
> > >
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 3/3] OvmfPkg/CpuS3DataDxe: do not allocate useless register tables
2021-01-12 19:19 ` [PATCH 3/3] OvmfPkg/CpuS3DataDxe: " Laszlo Ersek
@ 2021-01-14 12:53 ` Ard Biesheuvel
0 siblings, 0 replies; 20+ messages in thread
From: Ard Biesheuvel @ 2021-01-14 12:53 UTC (permalink / raw)
To: Laszlo Ersek, devel; +Cc: Jordan Justen, Philippe Mathieu-Daudé
On 1/12/21 8:19 PM, Laszlo Ersek wrote:
> CpuS3DataDxe allocates the "RegisterTable" and "PreSmmInitRegisterTable"
> arrays in ACPI_CPU_DATA just so every processor in the system can have its
> own empty register table, matched by APIC ID. This has never been useful
> in practice.
>
> Given commit e992cc3f4859 ("UefiCpuPkg PiSmmCpuDxeSmm: Reduce SMRAM
> consumption in CpuS3.c", 2021-01-11), simply leave both
> "AcpiCpuData->RegisterTable" and "AcpiCpuData->PreSmmInitRegisterTable"
> initialized to the zero address. This simplifies the driver, and saves
> both normal RAM (boot services data type memory) and -- in PiSmmCpuDxeSmm
> -- SMRAM.
>
> (This simplification backs out a good chunk of commit 1158fc8e2c7b
> ("OvmfPkg/CpuS3DataDxe: enable S3 resume after CPU hotplug", 2020-03-04).
> But CpuS3DataDxe still differs between UefiCpuPkg and OvmfPkg, due to the
> latter supporting CPU hotplug; thus, we can't remove OvmfPkg/CpuS3DataDxe
> altogether.)
>
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3159
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Acked-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
> ---
> OvmfPkg/CpuS3DataDxe/CpuS3DataDxe.inf | 1 -
> OvmfPkg/CpuS3DataDxe/CpuS3Data.c | 70 +-------------------
> 2 files changed, 1 insertion(+), 70 deletions(-)
>
> diff --git a/OvmfPkg/CpuS3DataDxe/CpuS3DataDxe.inf b/OvmfPkg/CpuS3DataDxe/CpuS3DataDxe.inf
> index ceae1d4078c7..228d5ae1b260 100644
> --- a/OvmfPkg/CpuS3DataDxe/CpuS3DataDxe.inf
> +++ b/OvmfPkg/CpuS3DataDxe/CpuS3DataDxe.inf
> @@ -42,7 +42,6 @@ [LibraryClasses]
> BaseLib
> BaseMemoryLib
> DebugLib
> - IoLib
> MemoryAllocationLib
> MtrrLib
> UefiBootServicesTableLib
> diff --git a/OvmfPkg/CpuS3DataDxe/CpuS3Data.c b/OvmfPkg/CpuS3DataDxe/CpuS3Data.c
> index bac7285aa2f3..5ffe1f3cd74e 100644
> --- a/OvmfPkg/CpuS3DataDxe/CpuS3Data.c
> +++ b/OvmfPkg/CpuS3DataDxe/CpuS3Data.c
> @@ -23,7 +23,6 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> #include <Library/BaseLib.h>
> #include <Library/BaseMemoryLib.h>
> #include <Library/DebugLib.h>
> -#include <Library/IoLib.h>
> #include <Library/MemoryAllocationLib.h>
> #include <Library/MtrrLib.h>
> #include <Library/UefiBootServicesTableLib.h>
> @@ -31,9 +30,6 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> #include <Protocol/MpService.h>
> #include <Guid/EventGroup.h>
>
> -#include <IndustryStandard/Q35MchIch9.h>
> -#include <IndustryStandard/QemuCpuHotplug.h>
> -
> //
> // Data structure used to allocate ACPI_CPU_DATA and its supporting structures
> //
> @@ -168,17 +164,12 @@ CpuS3DataInitialize (
> EFI_MP_SERVICES_PROTOCOL *MpServices;
> UINTN NumberOfCpus;
> VOID *Stack;
> - UINTN TableSize;
> - CPU_REGISTER_TABLE *RegisterTable;
> - UINTN Index;
> - EFI_PROCESSOR_INFORMATION ProcessorInfoBuffer;
> UINTN GdtSize;
> UINTN IdtSize;
> VOID *Gdt;
> VOID *Idt;
> EFI_EVENT Event;
> ACPI_CPU_DATA *OldAcpiCpuData;
> - BOOLEAN FetchPossibleApicIds;
>
> if (!PcdGetBool (PcdAcpiS3Enable)) {
> return EFI_UNSUPPORTED;
> @@ -193,13 +184,7 @@ CpuS3DataInitialize (
> ASSERT (AcpiCpuDataEx != NULL);
> AcpiCpuData = &AcpiCpuDataEx->AcpiCpuData;
>
> - //
> - // The "SMRAM at default SMBASE" feature guarantees that
> - // QEMU_CPUHP_CMD_GET_ARCH_ID too is available.
> - //
> - FetchPossibleApicIds = PcdGetBool (PcdQ35SmramAtDefaultSmbase);
> -
> - if (FetchPossibleApicIds) {
> + if (PcdGetBool (PcdQ35SmramAtDefaultSmbase)) {
> NumberOfCpus = PcdGet32 (PcdCpuMaxLogicalProcessorNumber);
> } else {
> UINTN NumberOfEnabledProcessors;
> @@ -271,59 +256,6 @@ CpuS3DataInitialize (
> AcpiCpuData->PreSmmInitRegisterTable = OldAcpiCpuData->PreSmmInitRegisterTable;
> AcpiCpuData->ApLocation = OldAcpiCpuData->ApLocation;
> CopyMem (&AcpiCpuData->CpuStatus, &OldAcpiCpuData->CpuStatus, sizeof (CPU_STATUS_INFORMATION));
> - } else {
> - //
> - // Allocate buffer for empty RegisterTable and PreSmmInitRegisterTable for all CPUs
> - //
> - TableSize = 2 * NumberOfCpus * sizeof (CPU_REGISTER_TABLE);
> - RegisterTable = (CPU_REGISTER_TABLE *)AllocateZeroPages (TableSize);
> - ASSERT (RegisterTable != NULL);
> -
> - if (FetchPossibleApicIds) {
> - //
> - // Write a valid selector so that other hotplug registers can be
> - // accessed.
> - //
> - IoWrite32 (ICH9_CPU_HOTPLUG_BASE + QEMU_CPUHP_W_CPU_SEL, 0);
> - //
> - // We'll be fetching the APIC IDs.
> - //
> - IoWrite8 (ICH9_CPU_HOTPLUG_BASE + QEMU_CPUHP_W_CMD,
> - QEMU_CPUHP_CMD_GET_ARCH_ID);
> - }
> - for (Index = 0; Index < NumberOfCpus; Index++) {
> - UINT32 InitialApicId;
> -
> - if (FetchPossibleApicIds) {
> - IoWrite32 (ICH9_CPU_HOTPLUG_BASE + QEMU_CPUHP_W_CPU_SEL,
> - (UINT32)Index);
> - InitialApicId = IoRead32 (
> - ICH9_CPU_HOTPLUG_BASE + QEMU_CPUHP_RW_CMD_DATA);
> - } else {
> - Status = MpServices->GetProcessorInfo (
> - MpServices,
> - Index,
> - &ProcessorInfoBuffer
> - );
> - ASSERT_EFI_ERROR (Status);
> - InitialApicId = (UINT32)ProcessorInfoBuffer.ProcessorId;
> - }
> -
> - DEBUG ((DEBUG_VERBOSE, "%a: Index=%05Lu ApicId=0x%08x\n", __FUNCTION__,
> - (UINT64)Index, InitialApicId));
> -
> - RegisterTable[Index].InitialApicId = InitialApicId;
> - RegisterTable[Index].TableLength = 0;
> - RegisterTable[Index].AllocatedSize = 0;
> - RegisterTable[Index].RegisterTableEntry = 0;
> -
> - RegisterTable[NumberOfCpus + Index].InitialApicId = InitialApicId;
> - RegisterTable[NumberOfCpus + Index].TableLength = 0;
> - RegisterTable[NumberOfCpus + Index].AllocatedSize = 0;
> - RegisterTable[NumberOfCpus + Index].RegisterTableEntry = 0;
> - }
> - AcpiCpuData->RegisterTable = (EFI_PHYSICAL_ADDRESS)(UINTN)RegisterTable;
> - AcpiCpuData->PreSmmInitRegisterTable = (EFI_PHYSICAL_ADDRESS)(UINTN)(RegisterTable + NumberOfCpus);
> }
>
> //
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate useless register tables
2021-01-14 1:55 ` Zeng, Star
@ 2021-01-14 17:35 ` Laszlo Ersek
2021-01-15 8:33 ` [edk2-devel] " Ni, Ray
0 siblings, 1 reply; 20+ messages in thread
From: Laszlo Ersek @ 2021-01-14 17:35 UTC (permalink / raw)
To: Zeng, Star, Ni, Ray, devel@edk2.groups.io
Cc: Dong, Eric, Philippe Mathieu-Daudé, Kumar, Rahul1
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
>>>>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [edk2-devel] [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate useless register tables
2021-01-14 17:35 ` Laszlo Ersek
@ 2021-01-15 8:33 ` Ni, Ray
2021-01-15 8:37 ` Laszlo Ersek
0 siblings, 1 reply; 20+ messages in thread
From: Ni, Ray @ 2021-01-15 8:33 UTC (permalink / raw)
To: devel@edk2.groups.io, lersek@redhat.com, Zeng, Star
Cc: Dong, Eric, Philippe Mathieu-Daudé, Kumar, Rahul1
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
> >>>>
> >
>
>
>
>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [edk2-devel] [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate useless register tables
2021-01-15 8:33 ` [edk2-devel] " Ni, Ray
@ 2021-01-15 8:37 ` Laszlo Ersek
2021-01-19 12:52 ` Ni, Ray
0 siblings, 1 reply; 20+ messages in thread
From: Laszlo Ersek @ 2021-01-15 8:37 UTC (permalink / raw)
To: Ni, Ray, devel@edk2.groups.io, Zeng, Star
Cc: Dong, Eric, Philippe Mathieu-Daudé, Kumar, Rahul1
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
>>>>>>
>>>
>>
>>
>>
>>
>>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/3] UefiCpuPkg/AcpiCpuData: update comments on register table fields
2021-01-12 19:19 ` [PATCH 1/3] UefiCpuPkg/AcpiCpuData: update comments on register table fields Laszlo Ersek
2021-01-13 5:00 ` Ni, Ray
@ 2021-01-15 9:28 ` Philippe Mathieu-Daudé
2021-01-15 9:34 ` Laszlo Ersek
1 sibling, 1 reply; 20+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-01-15 9:28 UTC (permalink / raw)
To: Laszlo Ersek, devel; +Cc: Eric Dong, Rahul Kumar, Ray Ni, Star Zeng
On 1/12/21 8:19 PM, Laszlo Ersek wrote:
> After commit e992cc3f4859 ("UefiCpuPkg PiSmmCpuDxeSmm: Reduce SMRAM
> consumption in CpuS3.c", 2021-01-11), it is valid for a CPU S3 Data DXE
> Driver to set "ACPI_CPU_DATA.PreSmmInitRegisterTable" and/or
> "ACPI_CPU_DATA.RegisterTable" to 0, in case none of the CPUs needs a
> register table of the corresponding kind, during S3 resume.
>
> Document this fact in the "UefiCpuPkg/Include/AcpiCpuData.h" header file.
>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3159
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
> UefiCpuPkg/Include/AcpiCpuData.h | 4 ++++
> 1 file changed, 4 insertions(+)
Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/3] UefiCpuPkg/AcpiCpuData: update comments on register table fields
2021-01-15 9:28 ` Philippe Mathieu-Daudé
@ 2021-01-15 9:34 ` Laszlo Ersek
0 siblings, 0 replies; 20+ messages in thread
From: Laszlo Ersek @ 2021-01-15 9:34 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, devel
Cc: Eric Dong, Rahul Kumar, Ray Ni, Star Zeng
On 01/15/21 10:28, Philippe Mathieu-Daudé wrote:
> On 1/12/21 8:19 PM, Laszlo Ersek wrote:
>> After commit e992cc3f4859 ("UefiCpuPkg PiSmmCpuDxeSmm: Reduce SMRAM
>> consumption in CpuS3.c", 2021-01-11), it is valid for a CPU S3 Data DXE
>> Driver to set "ACPI_CPU_DATA.PreSmmInitRegisterTable" and/or
>> "ACPI_CPU_DATA.RegisterTable" to 0, in case none of the CPUs needs a
>> register table of the corresponding kind, during S3 resume.
>>
>> Document this fact in the "UefiCpuPkg/Include/AcpiCpuData.h" header file.
>>
>> Cc: Eric Dong <eric.dong@intel.com>
>> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
>> Cc: Rahul Kumar <rahul1.kumar@intel.com>
>> Cc: Ray Ni <ray.ni@intel.com>
>> Cc: Star Zeng <star.zeng@intel.com>
>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3159
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>> UefiCpuPkg/Include/AcpiCpuData.h | 4 ++++
>> 1 file changed, 4 insertions(+)
>
> Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>
>
Thank you!
Laszlo
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [edk2-devel] [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate useless register tables
2021-01-15 8:37 ` Laszlo Ersek
@ 2021-01-19 12:52 ` Ni, Ray
2021-01-19 13:48 ` Laszlo Ersek
0 siblings, 1 reply; 20+ messages in thread
From: Ni, Ray @ 2021-01-19 12:52 UTC (permalink / raw)
To: devel@edk2.groups.io, lersek@redhat.com, Zeng, Star
Cc: Dong, Eric, Philippe Mathieu-Daudé, Kumar, Rahul1
https://github.com/niruiyu/edk2/commit/7091aa50b9d87240b14e5a74398eab010ffc4d3d
Laszlo, Star,
Please check this code change. I verified S3 boot in an internal platform.
Thanks,
Ray
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek
> Sent: Friday, January 15, 2021 4:37 PM
> To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io; Zeng, Star <star.zeng@intel.com>
> Cc: Dong, Eric <eric.dong@intel.com>; Philippe Mathieu-Daudé <philmd@redhat.com>; Kumar, Rahul1
> <rahul1.kumar@intel.com>
> Subject: Re: [edk2-devel] [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate useless register tables
>
> On 01/15/21 09:33, Ni, Ray wrote:
> > Laszlo,
> > I will test my patches internally and find a way to give you the patch to be included in your V2.
>
> Thank you!
> Laszlo
>
> >
> > Thanks,
> > Ray
> >
> >> -----Original Message-----
> >> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek
> >> Sent: Friday, January 15, 2021 1:36 AM
> >> To: Zeng, Star <star.zeng@intel.com>; Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io
> >> Cc: Dong, Eric <eric.dong@intel.com>; Philippe Mathieu-Daudé <philmd@redhat.com>; Kumar, Rahul1
> >> <rahul1.kumar@intel.com>
> >> Subject: Re: [edk2-devel] [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate useless register tables
> >>
> >> Hi Star,
> >>
> >> On 01/14/21 02:55, Zeng, Star wrote:
> >>> Just think more, the change below needs a minor enhancement as below, otherwise the table will be overridden by
> the
> >> function's second call.
> >>
> >> thank you for following up here -- could you or Ray please propose an
> >> actual patch for RegisterCpuFeaturesLib, as I requested before?
> >>
> >> Posting the patch is not necessary; I only need to fetch it from your
> >> personal repo(s) -- you can even send me the repo / branch reference
> >> off-list. I would include the RegisterCpuFeaturesLib patch in my v2
> >> posting of this series.
> >>
> >> Thanks!
> >> Laszlo
> >>
> >>>
> >>>> -----Original Message-----
> >>>> From: Ni, Ray <ray.ni@intel.com>
> >>>> Sent: Wednesday, January 13, 2021 4:12 PM
> >>>> To: Zeng, Star <star.zeng@intel.com>; Laszlo Ersek <lersek@redhat.com>;
> >>>> devel@edk2.groups.io
> >>>> Cc: Dong, Eric <eric.dong@intel.com>; Philippe Mathieu-Daudé
> >>>> <philmd@redhat.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>
> >>>> Subject: RE: [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate useless
> >>>> register tables
> >>>>
> >>>> Star,
> >>>> Thanks for pointing that.
> >>>> RegisterCpuFeaturesLib assumes [PreSmmInit]RegisterTable array is
> >>>> allocated but CpuS3Data
> >>>> doesn't do that.
> >>>>
> >>>> Can following change fix the problem?
> >>>>
> >>>> --- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> >>>> +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> >>>> @@ -937,21 +937,19 @@ GetAcpiCpuData (
> >>>> EFI_PROCESSOR_INFORMATION ProcessorInfoBuffer;
> >>>>
> >>>> AcpiCpuData = (ACPI_CPU_DATA *) (UINTN) PcdGet64
> >>>> (PcdCpuS3DataAddress);
> >>>> - if (AcpiCpuData != NULL) {
> >>>> - return AcpiCpuData;
> >>>> - }
> >>>> -
> >>>> - AcpiCpuData = AllocatePages (EFI_SIZE_TO_PAGES (sizeof
> >>>> (ACPI_CPU_DATA)));
> >>>> - ASSERT (AcpiCpuData != NULL);
> >>>> + if (AcpiCpuData == NULL) {
> >>>> + AcpiCpuData = AllocatePages (EFI_SIZE_TO_PAGES (sizeof
> >>>> (ACPI_CPU_DATA)));
> >>>> + ASSERT (AcpiCpuData != NULL);
> >>>>
> >>>> - //
> >>>> - // Set PcdCpuS3DataAddress to the base address of the ACPI_CPU_DATA
> >>>> structure
> >>>> - //
> >>>> - Status = PcdSet64S (PcdCpuS3DataAddress, (UINT64)(UINTN)AcpiCpuData);
> >>>> - ASSERT_EFI_ERROR (Status);
> >>>> + //
> >>>> + // Set PcdCpuS3DataAddress to the base address of the ACPI_CPU_DATA
> >>>> structure
> >>>> + //
> >>>> + Status = PcdSet64S (PcdCpuS3DataAddress,
> >>>> (UINT64)(UINTN)AcpiCpuData);
> >>>> + ASSERT_EFI_ERROR (Status);
> >>>>
> >>>> - GetNumberOfProcessor (&NumberOfCpus, &NumberOfEnabledProcessors);
> >>>> - AcpiCpuData->NumberOfCpus = (UINT32)NumberOfCpus;
> >>>> + GetNumberOfProcessor (&NumberOfCpus,
> >>>> &NumberOfEnabledProcessors);
> >>>> + AcpiCpuData->NumberOfCpus = (UINT32)NumberOfCpus;
> >>>> + }
> >>>>
> >>>
> >>> Add check as below here.
> >>>
> >>> if (AcpiCpuData->RegisterTable == 0) {
> >>>
> >>>> //
> >>>> // Allocate buffer for empty RegisterTable and PreSmmInitRegisterTable
> >>>> for all CPUs
> >>>>
> >>>> Thanks,
> >>>> Ray
> >>>>
> >>>>> -----Original Message-----
> >>>>> From: Zeng, Star <star.zeng@intel.com>
> >>>>> Sent: Wednesday, January 13, 2021 3:17 PM
> >>>>> To: Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>;
> >>>>> devel@edk2.groups.io
> >>>>> Cc: Dong, Eric <eric.dong@intel.com>; Philippe Mathieu-Daudé
> >>>>> <philmd@redhat.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>; Zeng,
> >>>> Star
> >>>>> <star.zeng@intel.com>
> >>>>> Subject: RE: [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate
> >>>> useless
> >>>>> register tables
> >>>>>
> >>>>> DxeRegisterCpuFeaturesLib still has some execution sequence dependency
> >>>> to
> >>>>> UefiCpuPkg CpuS3DataDxe.
> >>>>> There is ASSERT to happen at
> >>>>>
> >>>> https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/Library/Regist
> >>>> erC
> >>>>> puFeaturesLib/RegisterCpuFeaturesLib.c#L1059 when CpuS3DataDxe with
> >>>> this
> >>>>> patch runs before DxeRegisterCpuFeaturesLib.
> >>>>>
> >>>>> Thanks,
> >>>>> Star
> >>>>> -----Original Message-----
> >>>>> From: Ni, Ray <ray.ni@intel.com>
> >>>>> Sent: Wednesday, January 13, 2021 2:12 PM
> >>>>> To: Laszlo Ersek <lersek@redhat.com>; devel@edk2.groups.io
> >>>>> Cc: Dong, Eric <eric.dong@intel.com>; Philippe Mathieu-Daudé
> >>>>> <philmd@redhat.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>; Zeng,
> >>>> Star
> >>>>> <star.zeng@intel.com>
> >>>>> Subject: RE: [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate
> >>>> useless
> >>>>> register tables
> >>>>>
> >>>>> Reviewed-by: Ray Ni <ray.ni@intel.com>
> >>>>>
> >>>>> (I guess you don't want to put Redhat copyright in the patch 1&2 so the
> >>>>> copyright year is not updated.
> >>>>> Since it's a simple change that make the logic more neat, I am ok with that.)
> >>>>>
> >>>>> Will you create a pull request to merge all 3 patches together?
> >>>>>
> >>>>>> -----Original Message-----
> >>>>>> From: Laszlo Ersek <lersek@redhat.com>
> >>>>>> Sent: Wednesday, January 13, 2021 3:20 AM
> >>>>>> To: devel@edk2.groups.io
> >>>>>> Cc: Dong, Eric <eric.dong@intel.com>; Philippe Mathieu-Daudé
> >>>>>> <philmd@redhat.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>; Ni,
> >>>> Ray
> >>>>>> <ray.ni@intel.com>; Zeng, Star <star.zeng@intel.com>
> >>>>>> Subject: [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate useless
> >>>>>> register tables
> >>>>>>
> >>>>>> CpuS3DataDxe allocates the "RegisterTable" and
> >>>> "PreSmmInitRegisterTable"
> >>>>>> arrays in ACPI_CPU_DATA just so every processor in the system can have
> >>>>>> its own empty register table, matched by APIC ID. This has never been
> >>>>>> useful in practice.
> >>>>>>
> >>>>>> Given commit e992cc3f4859 ("UefiCpuPkg PiSmmCpuDxeSmm: Reduce
> >>>>> SMRAM
> >>>>>> consumption in CpuS3.c", 2021-01-11), simply leave both
> >>>>>> "AcpiCpuData->RegisterTable" and "AcpiCpuData-
> >>>>> PreSmmInitRegisterTable"
> >>>>>> initialized to the zero address. This simplifies the driver, and saves
> >>>>>> both normal RAM (boot services data type memory) and -- in
> >>>>>> PiSmmCpuDxeSmm
> >>>>>> -- SMRAM.
> >>>>>>
> >>>>>> Cc: Eric Dong <eric.dong@intel.com>
> >>>>>> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> >>>>>> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> >>>>>> Cc: Ray Ni <ray.ni@intel.com>
> >>>>>> Cc: Star Zeng <star.zeng@intel.com>
> >>>>>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3159
> >>>>>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> >>>>>> ---
> >>>>>>
> >>>>>> Notes:
> >>>>>> Tested by temporarily replacing OvmfPkgPkg/CpuS3DataDxe in the
> >>>>>> OVMF
> >>>>>> IA32
> >>>>>> and IA32X64 platforms with this driver -- this driver works OK in OVMF
> >>>>>> as long as no CPUs are hot-plugged.
> >>>>>>
> >>>>>> UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c | 32 --------------------
> >>>>>> 1 file changed, 32 deletions(-)
> >>>>>>
> >>>>>> diff --git a/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
> >>>>>> b/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
> >>>>>> index 2be335d91903..078af36cfb41 100644
> >>>>>> --- a/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
> >>>>>> +++ b/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
> >>>>>> @@ -165,10 +165,6 @@ CpuS3DataInitialize (
> >>>>>> UINTN NumberOfCpus;
> >>>>>> UINTN NumberOfEnabledProcessors;
> >>>>>> VOID *Stack;
> >>>>>> - UINTN TableSize;
> >>>>>> - CPU_REGISTER_TABLE *RegisterTable;
> >>>>>> - UINTN Index;
> >>>>>> - EFI_PROCESSOR_INFORMATION ProcessorInfoBuffer;
> >>>>>> UINTN GdtSize;
> >>>>>> UINTN IdtSize;
> >>>>>> VOID *Gdt;
> >>>>>> @@ -255,34 +251,6 @@ CpuS3DataInitialize (
> >>>>>> AcpiCpuData->PreSmmInitRegisterTable = OldAcpiCpuData-
> >>>>>>> PreSmmInitRegisterTable;
> >>>>>> AcpiCpuData->ApLocation = OldAcpiCpuData->ApLocation;
> >>>>>> CopyMem (&AcpiCpuData->CpuStatus, &OldAcpiCpuData->CpuStatus,
> >>>>>> sizeof (CPU_STATUS_INFORMATION));
> >>>>>> - } else {
> >>>>>> - //
> >>>>>> - // Allocate buffer for empty RegisterTable and
> >>>> PreSmmInitRegisterTable
> >>>>> for
> >>>>>> all CPUs
> >>>>>> - //
> >>>>>> - TableSize = 2 * NumberOfCpus * sizeof (CPU_REGISTER_TABLE);
> >>>>>> - RegisterTable = (CPU_REGISTER_TABLE *)AllocateZeroPages
> >>>> (TableSize);
> >>>>>> - ASSERT (RegisterTable != NULL);
> >>>>>> -
> >>>>>> - for (Index = 0; Index < NumberOfCpus; Index++) {
> >>>>>> - Status = MpServices->GetProcessorInfo (
> >>>>>> - MpServices,
> >>>>>> - Index,
> >>>>>> - &ProcessorInfoBuffer
> >>>>>> - );
> >>>>>> - ASSERT_EFI_ERROR (Status);
> >>>>>> -
> >>>>>> - RegisterTable[Index].InitialApicId =
> >>>>>> (UINT32)ProcessorInfoBuffer.ProcessorId;
> >>>>>> - RegisterTable[Index].TableLength = 0;
> >>>>>> - RegisterTable[Index].AllocatedSize = 0;
> >>>>>> - RegisterTable[Index].RegisterTableEntry = 0;
> >>>>>> -
> >>>>>> - RegisterTable[NumberOfCpus + Index].InitialApicId =
> >>>>>> (UINT32)ProcessorInfoBuffer.ProcessorId;
> >>>>>> - RegisterTable[NumberOfCpus + Index].TableLength = 0;
> >>>>>> - RegisterTable[NumberOfCpus + Index].AllocatedSize = 0;
> >>>>>> - RegisterTable[NumberOfCpus + Index].RegisterTableEntry = 0;
> >>>>>> - }
> >>>>>> - AcpiCpuData->RegisterTable =
> >>>>>> (EFI_PHYSICAL_ADDRESS)(UINTN)RegisterTable;
> >>>>>> - AcpiCpuData->PreSmmInitRegisterTable =
> >>>>>> (EFI_PHYSICAL_ADDRESS)(UINTN)(RegisterTable + NumberOfCpus);
> >>>>>> }
> >>>>>>
> >>>>>> //
> >>>>>> --
> >>>>>> 2.19.1.3.g30247aa5d201
> >>>>>>
> >>>
> >>
> >>
> >>
> >>
> >>
> >
>
>
>
>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [edk2-devel] [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate useless register tables
2021-01-19 12:52 ` Ni, Ray
@ 2021-01-19 13:48 ` Laszlo Ersek
0 siblings, 0 replies; 20+ messages in thread
From: Laszlo Ersek @ 2021-01-19 13:48 UTC (permalink / raw)
To: devel, ray.ni, Zeng, Star
Cc: Dong, Eric, Philippe Mathieu-Daudé, Kumar, Rahul1
On 01/19/21 13:52, Ni, Ray wrote:
> https://github.com/niruiyu/edk2/commit/7091aa50b9d87240b14e5a74398eab010ffc4d3d
> Laszlo, Star,
> Please check this code change. I verified S3 boot in an internal platform.
Thanks -- I've picked this commit to my local v2 branch; I'm going to
submit the v2 series soon. I'd like Star to review the new patch on the
list, as part of the v2 series.
Thanks
Laszlo
^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2021-01-19 13:48 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-01-12 19:19 [PATCH 0/3] UefiCpuPkg, OvmfPkg: do not allocate useless register tables for S3 resume Laszlo Ersek
2021-01-12 19:19 ` [PATCH 1/3] UefiCpuPkg/AcpiCpuData: update comments on register table fields Laszlo Ersek
2021-01-13 5:00 ` Ni, Ray
2021-01-15 9:28 ` Philippe Mathieu-Daudé
2021-01-15 9:34 ` Laszlo Ersek
2021-01-12 19:19 ` [PATCH 2/3] UefiCpuPkg/CpuS3DataDxe: do not allocate useless register tables Laszlo Ersek
2021-01-13 6:12 ` Ni, Ray
2021-01-13 7:16 ` Zeng, Star
2021-01-13 8:12 ` Ni, Ray
2021-01-13 8:40 ` Zeng, Star
2021-01-13 9:16 ` Laszlo Ersek
2021-01-14 1:55 ` Zeng, Star
2021-01-14 17:35 ` Laszlo Ersek
2021-01-15 8:33 ` [edk2-devel] " Ni, Ray
2021-01-15 8:37 ` Laszlo Ersek
2021-01-19 12:52 ` Ni, Ray
2021-01-19 13:48 ` Laszlo Ersek
2021-01-13 9:10 ` Laszlo Ersek
2021-01-12 19:19 ` [PATCH 3/3] OvmfPkg/CpuS3DataDxe: " Laszlo Ersek
2021-01-14 12:53 ` Ard Biesheuvel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox