* [PATCH v1 1/2] UefiCpuPkg: Refactor initialization of CPU features during S3 resume
@ 2021-09-16 2:01 Jason Lou
2021-09-16 2:01 ` [PATCH v1 2/2] UefiCpuPkg: Prevent from re-initializing " Jason Lou
2021-09-16 6:14 ` [PATCH v1 1/2] UefiCpuPkg: Refactor initialization of " Ni, Ray
0 siblings, 2 replies; 4+ messages in thread
From: Jason Lou @ 2021-09-16 2:01 UTC (permalink / raw)
To: devel; +Cc: Jason, Ray Ni, Eric Dong, Laszlo Ersek, Rahul Kumar
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3621
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3631
Refactor initialization of CPU features during S3 resume.
In addition, the macro ACPI_CPU_DATA_STRUCTURE_UPDATE is used to fix
incompatibility issue caused by ACPI_CPU_DATA structure update. It will
be removed after all the platform code uses new ACPI_CPU_DATA structure.
Signed-off-by: Jason Lou <yun.lou@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
---
OvmfPkg/CpuS3DataDxe/CpuS3Data.c | 7 +-
UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c | 7 +-
UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c | 12 +-
UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c | 18 +--
UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c | 163 ++++++++++++--------
UefiCpuPkg/Include/AcpiCpuData.h | 91 ++++++-----
6 files changed, 170 insertions(+), 128 deletions(-)
diff --git a/OvmfPkg/CpuS3DataDxe/CpuS3Data.c b/OvmfPkg/CpuS3DataDxe/CpuS3Data.c
index 5ffe1f3cd7..de20d87567 100644
--- a/OvmfPkg/CpuS3DataDxe/CpuS3Data.c
+++ b/OvmfPkg/CpuS3DataDxe/CpuS3Data.c
@@ -9,7 +9,7 @@ number of CPUs reported by the MP Services Protocol, so this module does not
support hot plug CPUs. This module can be copied into a CPU specific package
and customized if these additional features are required.
-Copyright (c) 2013 - 2017, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2013 - 2021, Intel Corporation. All rights reserved.<BR>
Copyright (c) 2015 - 2020, Red Hat, Inc.
SPDX-License-Identifier: BSD-2-Clause-Patent
@@ -252,10 +252,7 @@ CpuS3DataInitialize (
AcpiCpuDataEx->IdtrProfile.Base = (UINTN)Idt;
if (OldAcpiCpuData != NULL) {
- AcpiCpuData->RegisterTable = OldAcpiCpuData->RegisterTable;
- AcpiCpuData->PreSmmInitRegisterTable = OldAcpiCpuData->PreSmmInitRegisterTable;
- AcpiCpuData->ApLocation = OldAcpiCpuData->ApLocation;
- CopyMem (&AcpiCpuData->CpuStatus, &OldAcpiCpuData->CpuStatus, sizeof (CPU_STATUS_INFORMATION));
+ CopyMem (&AcpiCpuData->CpuFeatureInitData, &OldAcpiCpuData->CpuFeatureInitData, sizeof (CPU_FEATURE_INIT_DATA));
}
//
diff --git a/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c b/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
index 078af36cfb..61ec7c44b2 100644
--- a/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
+++ b/UefiCpuPkg/CpuS3DataDxe/CpuS3Data.c
@@ -9,7 +9,7 @@ number of CPUs reported by the MP Services Protocol, so this module does not
support hot plug CPUs. This module can be copied into a CPU specific package
and customized if these additional features are required.
-Copyright (c) 2013 - 2017, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2013 - 2021, Intel Corporation. All rights reserved.<BR>
Copyright (c) 2015, Red Hat, Inc.
SPDX-License-Identifier: BSD-2-Clause-Patent
@@ -247,10 +247,7 @@ CpuS3DataInitialize (
AcpiCpuDataEx->IdtrProfile.Base = (UINTN)Idt;
if (OldAcpiCpuData != NULL) {
- AcpiCpuData->RegisterTable = OldAcpiCpuData->RegisterTable;
- AcpiCpuData->PreSmmInitRegisterTable = OldAcpiCpuData->PreSmmInitRegisterTable;
- AcpiCpuData->ApLocation = OldAcpiCpuData->ApLocation;
- CopyMem (&AcpiCpuData->CpuStatus, &OldAcpiCpuData->CpuStatus, sizeof (CPU_STATUS_INFORMATION));
+ CopyMem (&AcpiCpuData->CpuFeatureInitData, &OldAcpiCpuData->CpuFeatureInitData, sizeof (CPU_FEATURE_INIT_DATA));
}
//
diff --git a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
index 57511c4efa..6e2ab79518 100644
--- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
+++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
@@ -1,7 +1,7 @@
/** @file
CPU Features Initialize functions.
- Copyright (c) 2017 - 2020, Intel Corporation. All rights reserved.<BR>
+ Copyright (c) 2017 - 2021, Intel Corporation. All rights reserved.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent
**/
@@ -152,10 +152,10 @@ CpuInitDataInitialize (
ASSERT (AcpiCpuData != NULL);
CpuFeaturesData->AcpiCpuData= AcpiCpuData;
- CpuStatus = &AcpiCpuData->CpuStatus;
+ CpuStatus = &AcpiCpuData->CpuFeatureInitData.CpuStatus;
Location = AllocateZeroPool (sizeof (EFI_CPU_PHYSICAL_LOCATION) * NumberOfCpus);
ASSERT (Location != NULL);
- AcpiCpuData->ApLocation = (EFI_PHYSICAL_ADDRESS)(UINTN)Location;
+ AcpiCpuData->CpuFeatureInitData.ApLocation = (EFI_PHYSICAL_ADDRESS)(UINTN)Location;
for (ProcessorNumber = 0; ProcessorNumber < NumberOfCpus; ProcessorNumber++) {
InitOrder = &CpuFeaturesData->InitOrder[ProcessorNumber];
@@ -1131,7 +1131,7 @@ SetProcessorRegister (
CpuFeaturesData = (CPU_FEATURES_DATA *) Buffer;
AcpiCpuData = CpuFeaturesData->AcpiCpuData;
- RegisterTables = (CPU_REGISTER_TABLE *)(UINTN)AcpiCpuData->RegisterTable;
+ RegisterTables = (CPU_REGISTER_TABLE *)(UINTN)AcpiCpuData->CpuFeatureInitData.RegisterTable;
InitApicId = GetInitialApicId ();
RegisterTable = NULL;
@@ -1147,8 +1147,8 @@ SetProcessorRegister (
ProgramProcessorRegister (
RegisterTable,
- (EFI_CPU_PHYSICAL_LOCATION *)(UINTN)AcpiCpuData->ApLocation + ProcIndex,
- &AcpiCpuData->CpuStatus,
+ (EFI_CPU_PHYSICAL_LOCATION *)(UINTN)AcpiCpuData->CpuFeatureInitData.ApLocation + ProcIndex,
+ &AcpiCpuData->CpuFeatureInitData.CpuStatus,
&CpuFeaturesData->CpuFlags
);
}
diff --git a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
index 60daa5cc87..e6ef9c602d 100644
--- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
+++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
@@ -952,8 +952,8 @@ GetAcpiCpuData (
AcpiCpuData->NumberOfCpus = (UINT32)NumberOfCpus;
}
- if (AcpiCpuData->RegisterTable == 0 ||
- AcpiCpuData->PreSmmInitRegisterTable == 0) {
+ if (AcpiCpuData->CpuFeatureInitData.RegisterTable == 0 ||
+ AcpiCpuData->CpuFeatureInitData.PreSmmInitRegisterTable == 0) {
//
// Allocate buffer for empty RegisterTable and PreSmmInitRegisterTable for all CPUs
//
@@ -976,11 +976,11 @@ GetAcpiCpuData (
RegisterTable[NumberOfCpus + Index].AllocatedSize = 0;
RegisterTable[NumberOfCpus + Index].RegisterTableEntry = 0;
}
- if (AcpiCpuData->RegisterTable == 0) {
- AcpiCpuData->RegisterTable = (EFI_PHYSICAL_ADDRESS)(UINTN)RegisterTable;
+ if (AcpiCpuData->CpuFeatureInitData.RegisterTable == 0) {
+ AcpiCpuData->CpuFeatureInitData.RegisterTable = (EFI_PHYSICAL_ADDRESS)(UINTN)RegisterTable;
}
- if (AcpiCpuData->PreSmmInitRegisterTable == 0) {
- AcpiCpuData->PreSmmInitRegisterTable = (EFI_PHYSICAL_ADDRESS)(UINTN)(RegisterTable + NumberOfCpus);
+ if (AcpiCpuData->CpuFeatureInitData.PreSmmInitRegisterTable == 0) {
+ AcpiCpuData->CpuFeatureInitData.PreSmmInitRegisterTable = (EFI_PHYSICAL_ADDRESS)(UINTN)(RegisterTable + NumberOfCpus);
}
}
@@ -1063,9 +1063,9 @@ CpuRegisterTableWriteWorker (
CpuFeaturesData = GetCpuFeaturesData ();
if (CpuFeaturesData->RegisterTable == NULL) {
AcpiCpuData = GetAcpiCpuData ();
- ASSERT ((AcpiCpuData != NULL) && (AcpiCpuData->RegisterTable != 0));
- CpuFeaturesData->RegisterTable = (CPU_REGISTER_TABLE *) (UINTN) AcpiCpuData->RegisterTable;
- CpuFeaturesData->PreSmmRegisterTable = (CPU_REGISTER_TABLE *) (UINTN) AcpiCpuData->PreSmmInitRegisterTable;
+ ASSERT ((AcpiCpuData != NULL) && (AcpiCpuData->CpuFeatureInitData.RegisterTable != 0));
+ CpuFeaturesData->RegisterTable = (CPU_REGISTER_TABLE *) (UINTN) AcpiCpuData->CpuFeatureInitData.RegisterTable;
+ CpuFeaturesData->PreSmmRegisterTable = (CPU_REGISTER_TABLE *) (UINTN) AcpiCpuData->CpuFeatureInitData.PreSmmInitRegisterTable;
}
if (PreSmmFlag) {
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
index ab7f39aa2b..1270992f38 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
@@ -476,16 +476,22 @@ SetRegister (
IN BOOLEAN PreSmmRegisterTable
)
{
+ CPU_FEATURE_INIT_DATA *FeatureInitData;
CPU_REGISTER_TABLE *RegisterTable;
CPU_REGISTER_TABLE *RegisterTables;
UINT32 InitApicId;
UINTN ProcIndex;
UINTN Index;
+ FeatureInitData = &mAcpiCpuData.CpuFeatureInitData;
+ if (FeatureInitData == NULL) {
+ return;
+ }
+
if (PreSmmRegisterTable) {
- RegisterTables = (CPU_REGISTER_TABLE *)(UINTN)mAcpiCpuData.PreSmmInitRegisterTable;
+ RegisterTables = (CPU_REGISTER_TABLE *)(UINTN)FeatureInitData->PreSmmInitRegisterTable;
} else {
- RegisterTables = (CPU_REGISTER_TABLE *)(UINTN)mAcpiCpuData.RegisterTable;
+ RegisterTables = (CPU_REGISTER_TABLE *)(UINTN)FeatureInitData->RegisterTable;
}
if (RegisterTables == NULL) {
return;
@@ -503,18 +509,18 @@ SetRegister (
}
ASSERT (RegisterTable != NULL);
- if (mAcpiCpuData.ApLocation != 0) {
+ if (FeatureInitData->ApLocation != 0) {
ProgramProcessorRegister (
RegisterTable,
- (EFI_CPU_PHYSICAL_LOCATION *)(UINTN)mAcpiCpuData.ApLocation + ProcIndex,
- &mAcpiCpuData.CpuStatus,
+ (EFI_CPU_PHYSICAL_LOCATION *)(UINTN)FeatureInitData->ApLocation + ProcIndex,
+ &FeatureInitData->CpuStatus,
&mCpuFlags
);
} else {
ProgramProcessorRegister (
RegisterTable,
NULL,
- &mAcpiCpuData.CpuStatus,
+ &FeatureInitData->CpuStatus,
&mCpuFlags
);
}
@@ -1010,6 +1016,71 @@ IsRegisterTableEmpty (
return TRUE;
}
+/**
+ Copy the data used to initialize processor register into SMRAM.
+
+ @param[in,out] CpuFeatureInitDataDst Pointer to the destination CPU_FEATURE_INIT_DATA structure.
+ @param[in] CpuFeatureInitDataSrc Pointer to the source CPU_FEATURE_INIT_DATA structure.
+
+**/
+VOID
+CopyCpuFeatureInitDatatoSmram (
+ IN OUT CPU_FEATURE_INIT_DATA *CpuFeatureInitDataDst,
+ IN CPU_FEATURE_INIT_DATA *CpuFeatureInitDataSrc
+ )
+{
+ CPU_STATUS_INFORMATION *CpuStatus;
+
+ if (!IsRegisterTableEmpty ((CPU_REGISTER_TABLE *)(UINTN)CpuFeatureInitDataSrc->PreSmmInitRegisterTable, mAcpiCpuData.NumberOfCpus)) {
+ CpuFeatureInitDataDst->PreSmmInitRegisterTable = (EFI_PHYSICAL_ADDRESS)(UINTN)AllocatePool (mAcpiCpuData.NumberOfCpus * sizeof (CPU_REGISTER_TABLE));
+ ASSERT (CpuFeatureInitDataDst->PreSmmInitRegisterTable != 0);
+
+ CopyRegisterTable (
+ (CPU_REGISTER_TABLE *)(UINTN)CpuFeatureInitDataDst->PreSmmInitRegisterTable,
+ (CPU_REGISTER_TABLE *)(UINTN)CpuFeatureInitDataSrc->PreSmmInitRegisterTable,
+ mAcpiCpuData.NumberOfCpus
+ );
+ }
+
+ if (!IsRegisterTableEmpty ((CPU_REGISTER_TABLE *)(UINTN)CpuFeatureInitDataSrc->RegisterTable, mAcpiCpuData.NumberOfCpus)) {
+ CpuFeatureInitDataDst->RegisterTable = (EFI_PHYSICAL_ADDRESS)(UINTN)AllocatePool (mAcpiCpuData.NumberOfCpus * sizeof (CPU_REGISTER_TABLE));
+ ASSERT (CpuFeatureInitDataDst->RegisterTable != 0);
+
+ CopyRegisterTable (
+ (CPU_REGISTER_TABLE *)(UINTN)CpuFeatureInitDataDst->RegisterTable,
+ (CPU_REGISTER_TABLE *)(UINTN)CpuFeatureInitDataSrc->RegisterTable,
+ mAcpiCpuData.NumberOfCpus
+ );
+ }
+
+ CpuStatus = &CpuFeatureInitDataDst->CpuStatus;
+ CopyMem (CpuStatus, &CpuFeatureInitDataSrc->CpuStatus, sizeof (CPU_STATUS_INFORMATION));
+
+ if (CpuFeatureInitDataSrc->CpuStatus.ThreadCountPerPackage != 0) {
+ CpuStatus->ThreadCountPerPackage = (EFI_PHYSICAL_ADDRESS)(UINTN)AllocateCopyPool (
+ sizeof (UINT32) * CpuStatus->PackageCount,
+ (UINT32 *)(UINTN)CpuFeatureInitDataSrc->CpuStatus.ThreadCountPerPackage
+ );
+ ASSERT (CpuStatus->ThreadCountPerPackage != 0);
+ }
+
+ if (CpuFeatureInitDataSrc->CpuStatus.ThreadCountPerCore != 0) {
+ CpuStatus->ThreadCountPerCore = (EFI_PHYSICAL_ADDRESS)(UINTN)AllocateCopyPool (
+ sizeof (UINT8) * (CpuStatus->PackageCount * CpuStatus->MaxCoreCount),
+ (UINT32 *)(UINTN)CpuFeatureInitDataSrc->CpuStatus.ThreadCountPerCore
+ );
+ ASSERT (CpuStatus->ThreadCountPerCore != 0);
+ }
+
+ if (CpuFeatureInitDataSrc->ApLocation != 0) {
+ CpuFeatureInitDataDst->ApLocation = (EFI_PHYSICAL_ADDRESS)(UINTN)AllocateCopyPool (
+ mAcpiCpuData.NumberOfCpus * sizeof (EFI_CPU_PHYSICAL_LOCATION),
+ (EFI_CPU_PHYSICAL_LOCATION *)(UINTN)CpuFeatureInitDataSrc->ApLocation
+ );
+ ASSERT (CpuFeatureInitDataDst->ApLocation != 0);
+ }
+}
+
/**
Get ACPI CPU data.
@@ -1064,39 +1135,13 @@ GetAcpiCpuData (
CopyMem ((VOID *)(UINTN)mAcpiCpuData.IdtrProfile, (VOID *)(UINTN)AcpiCpuData->IdtrProfile, sizeof (IA32_DESCRIPTOR));
- if (!IsRegisterTableEmpty ((CPU_REGISTER_TABLE *)(UINTN)AcpiCpuData->PreSmmInitRegisterTable, mAcpiCpuData.NumberOfCpus)) {
- mAcpiCpuData.PreSmmInitRegisterTable = (EFI_PHYSICAL_ADDRESS)(UINTN)AllocatePool (mAcpiCpuData.NumberOfCpus * sizeof (CPU_REGISTER_TABLE));
- ASSERT (mAcpiCpuData.PreSmmInitRegisterTable != 0);
-
- CopyRegisterTable (
- (CPU_REGISTER_TABLE *)(UINTN)mAcpiCpuData.PreSmmInitRegisterTable,
- (CPU_REGISTER_TABLE *)(UINTN)AcpiCpuData->PreSmmInitRegisterTable,
- mAcpiCpuData.NumberOfCpus
- );
- } else {
- mAcpiCpuData.PreSmmInitRegisterTable = 0;
- }
-
- if (!IsRegisterTableEmpty ((CPU_REGISTER_TABLE *)(UINTN)AcpiCpuData->RegisterTable, mAcpiCpuData.NumberOfCpus)) {
- mAcpiCpuData.RegisterTable = (EFI_PHYSICAL_ADDRESS)(UINTN)AllocatePool (mAcpiCpuData.NumberOfCpus * sizeof (CPU_REGISTER_TABLE));
- ASSERT (mAcpiCpuData.RegisterTable != 0);
-
- CopyRegisterTable (
- (CPU_REGISTER_TABLE *)(UINTN)mAcpiCpuData.RegisterTable,
- (CPU_REGISTER_TABLE *)(UINTN)AcpiCpuData->RegisterTable,
- mAcpiCpuData.NumberOfCpus
- );
- } else {
- mAcpiCpuData.RegisterTable = 0;
- }
-
//
// Copy AP's GDT, IDT and Machine Check handler into SMRAM.
//
Gdtr = (IA32_DESCRIPTOR *)(UINTN)mAcpiCpuData.GdtrProfile;
Idtr = (IA32_DESCRIPTOR *)(UINTN)mAcpiCpuData.IdtrProfile;
- GdtForAp = AllocatePool ((Gdtr->Limit + 1) + (Idtr->Limit + 1) + mAcpiCpuData.ApMachineCheckHandlerSize);
+ GdtForAp = AllocatePool ((Gdtr->Limit + 1) + (Idtr->Limit + 1) + mAcpiCpuData.ApMachineCheckHandlerSize);
ASSERT (GdtForAp != NULL);
IdtForAp = (VOID *) ((UINTN)GdtForAp + (Gdtr->Limit + 1));
MachineCheckHandlerForAp = (VOID *) ((UINTN)IdtForAp + (Idtr->Limit + 1));
@@ -1109,41 +1154,23 @@ GetAcpiCpuData (
Idtr->Base = (UINTN)IdtForAp;
mAcpiCpuData.ApMachineCheckHandlerBase = (EFI_PHYSICAL_ADDRESS)(UINTN)MachineCheckHandlerForAp;
- CpuStatus = &mAcpiCpuData.CpuStatus;
- CopyMem (CpuStatus, &AcpiCpuData->CpuStatus, sizeof (CPU_STATUS_INFORMATION));
- if (AcpiCpuData->CpuStatus.ThreadCountPerPackage != 0) {
- CpuStatus->ThreadCountPerPackage = (EFI_PHYSICAL_ADDRESS)(UINTN)AllocateCopyPool (
- sizeof (UINT32) * CpuStatus->PackageCount,
- (UINT32 *)(UINTN)AcpiCpuData->CpuStatus.ThreadCountPerPackage
- );
- ASSERT (CpuStatus->ThreadCountPerPackage != 0);
- }
- if (AcpiCpuData->CpuStatus.ThreadCountPerCore != 0) {
- CpuStatus->ThreadCountPerCore = (EFI_PHYSICAL_ADDRESS)(UINTN)AllocateCopyPool (
- sizeof (UINT8) * (CpuStatus->PackageCount * CpuStatus->MaxCoreCount),
- (UINT32 *)(UINTN)AcpiCpuData->CpuStatus.ThreadCountPerCore
- );
- ASSERT (CpuStatus->ThreadCountPerCore != 0);
- }
- if (AcpiCpuData->ApLocation != 0) {
- mAcpiCpuData.ApLocation = (EFI_PHYSICAL_ADDRESS)(UINTN)AllocateCopyPool (
- mAcpiCpuData.NumberOfCpus * sizeof (EFI_CPU_PHYSICAL_LOCATION),
- (EFI_CPU_PHYSICAL_LOCATION *)(UINTN)AcpiCpuData->ApLocation
- );
- ASSERT (mAcpiCpuData.ApLocation != 0);
- }
- if (CpuStatus->PackageCount != 0) {
- mCpuFlags.CoreSemaphoreCount = AllocateZeroPool (
- sizeof (UINT32) * CpuStatus->PackageCount *
- CpuStatus->MaxCoreCount * CpuStatus->MaxThreadCount
- );
- ASSERT (mCpuFlags.CoreSemaphoreCount != NULL);
- mCpuFlags.PackageSemaphoreCount = AllocateZeroPool (
- sizeof (UINT32) * CpuStatus->PackageCount *
- CpuStatus->MaxCoreCount * CpuStatus->MaxThreadCount
- );
- ASSERT (mCpuFlags.PackageSemaphoreCount != NULL);
- }
+ ZeroMem (&mAcpiCpuData.CpuFeatureInitData, sizeof (CPU_FEATURE_INIT_DATA));
+ CopyCpuFeatureInitDatatoSmram (&mAcpiCpuData.CpuFeatureInitData, &AcpiCpuData->CpuFeatureInitData);
+
+ CpuStatus = &mAcpiCpuData.CpuFeatureInitData.CpuStatus;
+
+ mCpuFlags.CoreSemaphoreCount = AllocateZeroPool (
+ sizeof (UINT32) * CpuStatus->PackageCount *
+ CpuStatus->MaxCoreCount * CpuStatus->MaxThreadCount
+ );
+ ASSERT (mCpuFlags.CoreSemaphoreCount != NULL);
+
+ mCpuFlags.PackageSemaphoreCount = AllocateZeroPool (
+ sizeof (UINT32) * CpuStatus->PackageCount *
+ CpuStatus->MaxCoreCount * CpuStatus->MaxThreadCount
+ );
+ ASSERT (mCpuFlags.PackageSemaphoreCount != NULL);
+
InitializeSpinLock((SPIN_LOCK*) &mCpuFlags.MemoryMappedLock);
}
diff --git a/UefiCpuPkg/Include/AcpiCpuData.h b/UefiCpuPkg/Include/AcpiCpuData.h
index 62a01b2c6b..2fa8801d1f 100644
--- a/UefiCpuPkg/Include/AcpiCpuData.h
+++ b/UefiCpuPkg/Include/AcpiCpuData.h
@@ -1,7 +1,7 @@
/** @file
Definitions for CPU S3 data.
-Copyright (c) 2013 - 2020, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2013 - 2021, Intel Corporation. All rights reserved.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent
**/
@@ -9,6 +9,15 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
#ifndef _ACPI_CPU_DATA_H_
#define _ACPI_CPU_DATA_H_
+//
+// This macro definition is used to fix incompatibility issue caused by
+// ACPI_CPU_DATA structure update. It will be removed after all the platform
+// code uses new ACPI_CPU_DATA structure.
+//
+#ifndef ACPI_CPU_DATA_STRUCTURE_UPDATE
+#define ACPI_CPU_DATA_STRUCTURE_UPDATE
+#endif
+
//
// Register types in register table
//
@@ -118,6 +127,49 @@ typedef struct {
EFI_PHYSICAL_ADDRESS RegisterTableEntry;
} CPU_REGISTER_TABLE;
+//
+// Data structure that is used for CPU feature initialization during ACPI S3
+// resume.
+//
+typedef struct {
+ //
+ // Physical address of an array of CPU_REGISTER_TABLE structures, with
+ // NumberOfCpus entries. If a register table is not required, then the
+ // TableLength and AllocatedSize fields of CPU_REGISTER_TABLE are set to 0.
+ // If TableLength is > 0, then elements of RegisterTableEntry are used to
+ // initialize the CPU that matches InitialApicId, during an ACPI S3 resume,
+ // 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;
+ //
+ // Physical address of an array of CPU_REGISTER_TABLE structures, with
+ // NumberOfCpus entries. If a register table is not required, then the
+ // TableLength and AllocatedSize fields of CPU_REGISTER_TABLE are set to 0.
+ // If TableLength is > 0, then elements of RegisterTableEntry are used to
+ // initialize the CPU that matches InitialApicId, during an ACPI S3 resume,
+ // 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;
+ //
+ // CPU information which is required when set the register table.
+ //
+ CPU_STATUS_INFORMATION CpuStatus;
+ //
+ // Location info for each AP.
+ // It points to an array which saves all APs location info.
+ // The array count is the AP count in this CPU.
+ //
+ // If the platform does not support MSR setting at S3 resume, and
+ // therefore it doesn't need the dependency semaphores, it should set
+ // this field to 0.
+ //
+ EFI_PHYSICAL_ADDRESS ApLocation;
+} CPU_FEATURE_INIT_DATA;
+
//
// Data structure that is required for ACPI S3 resume. The PCD
// PcdCpuS3DataAddress must be set to the physical address where this structure
@@ -172,28 +224,6 @@ typedef struct {
//
EFI_PHYSICAL_ADDRESS MtrrTable;
//
- // Physical address of an array of CPU_REGISTER_TABLE structures, with
- // NumberOfCpus entries. If a register table is not required, then the
- // TableLength and AllocatedSize fields of CPU_REGISTER_TABLE are set to 0.
- // If TableLength is > 0, then elements of RegisterTableEntry are used to
- // initialize the CPU that matches InitialApicId, during an ACPI S3 resume,
- // 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;
- //
- // Physical address of an array of CPU_REGISTER_TABLE structures, with
- // NumberOfCpus entries. If a register table is not required, then the
- // TableLength and AllocatedSize fields of CPU_REGISTER_TABLE are set to 0.
- // If TableLength is > 0, then elements of RegisterTableEntry are used to
- // initialize the CPU that matches InitialApicId, during an ACPI S3 resume,
- // 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;
- //
// Physical address of a buffer that contains the machine check handler that
// is used during an ACPI S3 Resume. In order for this machine check
// handler to be active on an AP during an ACPI S3 resume, the machine check
@@ -208,19 +238,10 @@ typedef struct {
//
UINT32 ApMachineCheckHandlerSize;
//
- // CPU information which is required when set the register table.
- //
- CPU_STATUS_INFORMATION CpuStatus;
- //
- // Location info for each AP.
- // It points to an array which saves all APs location info.
- // The array count is the AP count in this CPU.
- //
- // If the platform does not support MSR setting at S3 resume, and
- // therefore it doesn't need the dependency semaphores, it should set
- // this field to 0.
+ // Data structure that is used for CPU feature initialization during ACPI S3
+ // resume.
//
- EFI_PHYSICAL_ADDRESS ApLocation;
+ CPU_FEATURE_INIT_DATA CpuFeatureInitData;
} ACPI_CPU_DATA;
#endif
--
2.28.0.windows.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH v1 2/2] UefiCpuPkg: Prevent from re-initializing CPU features during S3 resume
2021-09-16 2:01 [PATCH v1 1/2] UefiCpuPkg: Refactor initialization of CPU features during S3 resume Jason Lou
@ 2021-09-16 2:01 ` Jason Lou
2021-09-16 6:14 ` [PATCH v1 1/2] UefiCpuPkg: Refactor initialization of " Ni, Ray
1 sibling, 0 replies; 4+ messages in thread
From: Jason Lou @ 2021-09-16 2:01 UTC (permalink / raw)
To: devel; +Cc: Jason, Ray Ni, Eric Dong, Laszlo Ersek, Rahul Kumar
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3621
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3631
Current CPU feature initialization design:
During normal boot, CpuFeaturesPei module (inside FSP) initializes the
CPU features. During S3 boot, CpuFeaturesPei module does nothing, and
CpuSmm driver (in SMRAM) initializes CPU features instead.
This code change prevents CpuSmm driver from re-initializing CPU
features during S3 resume if CpuFeaturesPei module has done the same
initialization.
In addition, EDK2 contains DxeIpl PEIM that calls S3RestoreConfig2 PPI
during S3 boot and this PPI eventually calls CpuSmm driver (in SMRAM) to
initialize the CPU features, so "EDK2 + FSP" does not have the CPU
feature initialization issue during S3 boot. But "coreboot" does not
contain DxeIpl PEIM and the issue appears, unless
"PcdCpuFeaturesInitOnS3Resume" is set to TRUE.
Signed-off-by: Jason Lou <yun.lou@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
---
UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c | 34 ++++++++++++--------
UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf | 3 +-
2 files changed, 23 insertions(+), 14 deletions(-)
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
index 1270992f38..fece75266d 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
@@ -1155,23 +1155,31 @@ GetAcpiCpuData (
mAcpiCpuData.ApMachineCheckHandlerBase = (EFI_PHYSICAL_ADDRESS)(UINTN)MachineCheckHandlerForAp;
ZeroMem (&mAcpiCpuData.CpuFeatureInitData, sizeof (CPU_FEATURE_INIT_DATA));
- CopyCpuFeatureInitDatatoSmram (&mAcpiCpuData.CpuFeatureInitData, &AcpiCpuData->CpuFeatureInitData);
- CpuStatus = &mAcpiCpuData.CpuFeatureInitData.CpuStatus;
+ if (!PcdGetBool (PcdCpuFeaturesInitOnS3Resume)) {
+ //
+ // If the CPU features will not be initialized by CpuFeaturesPei module during
+ // next ACPI S3 resume, copy the CPU features initialization data into SMRAM,
+ // which will be consumed in SmmRestoreCpu during next S3 resume.
+ //
+ CopyCpuFeatureInitDatatoSmram (&mAcpiCpuData.CpuFeatureInitData, &AcpiCpuData->CpuFeatureInitData);
- mCpuFlags.CoreSemaphoreCount = AllocateZeroPool (
- sizeof (UINT32) * CpuStatus->PackageCount *
- CpuStatus->MaxCoreCount * CpuStatus->MaxThreadCount
- );
- ASSERT (mCpuFlags.CoreSemaphoreCount != NULL);
+ CpuStatus = &mAcpiCpuData.CpuFeatureInitData.CpuStatus;
- mCpuFlags.PackageSemaphoreCount = AllocateZeroPool (
- sizeof (UINT32) * CpuStatus->PackageCount *
- CpuStatus->MaxCoreCount * CpuStatus->MaxThreadCount
- );
- ASSERT (mCpuFlags.PackageSemaphoreCount != NULL);
+ mCpuFlags.CoreSemaphoreCount = AllocateZeroPool (
+ sizeof (UINT32) * CpuStatus->PackageCount *
+ CpuStatus->MaxCoreCount * CpuStatus->MaxThreadCount
+ );
+ ASSERT (mCpuFlags.CoreSemaphoreCount != NULL);
- InitializeSpinLock((SPIN_LOCK*) &mCpuFlags.MemoryMappedLock);
+ mCpuFlags.PackageSemaphoreCount = AllocateZeroPool (
+ sizeof (UINT32) * CpuStatus->PackageCount *
+ CpuStatus->MaxCoreCount * CpuStatus->MaxThreadCount
+ );
+ ASSERT (mCpuFlags.PackageSemaphoreCount != NULL);
+
+ InitializeSpinLock((SPIN_LOCK*) &mCpuFlags.MemoryMappedLock);
+ }
}
/**
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
index 76b1462996..0e88071c70 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
@@ -4,7 +4,7 @@
# This SMM driver performs SMM initialization, deploy SMM Entry Vector,
# provides CPU specific services in SMM.
#
-# Copyright (c) 2009 - 2019, Intel Corporation. All rights reserved.<BR>
+# Copyright (c) 2009 - 2021, Intel Corporation. All rights reserved.<BR>
# Copyright (c) 2017, AMD Incorporated. All rights reserved.<BR>
#
# SPDX-License-Identifier: BSD-2-Clause-Patent
@@ -134,6 +134,7 @@
gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmCodeAccessCheckEnable ## CONSUMES
gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode ## CONSUMES
gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmShadowStackSize ## SOMETIMES_CONSUMES
+ gUefiCpuPkgTokenSpaceGuid.PcdCpuFeaturesInitOnS3Resume ## CONSUMES
gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiS3Enable ## CONSUMES
gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask ## CONSUMES
gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask ## CONSUMES
--
2.28.0.windows.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v1 1/2] UefiCpuPkg: Refactor initialization of CPU features during S3 resume
2021-09-16 2:01 [PATCH v1 1/2] UefiCpuPkg: Refactor initialization of CPU features during S3 resume Jason Lou
2021-09-16 2:01 ` [PATCH v1 2/2] UefiCpuPkg: Prevent from re-initializing " Jason Lou
@ 2021-09-16 6:14 ` Ni, Ray
2021-09-16 6:24 ` [edk2-devel] " Jason Lou
1 sibling, 1 reply; 4+ messages in thread
From: Ni, Ray @ 2021-09-16 6:14 UTC (permalink / raw)
To: Lou, Yun, devel@edk2.groups.io; +Cc: Dong, Eric, Laszlo Ersek, Kumar, Rahul1
+ FeatureInitData = &mAcpiCpuData.CpuFeatureInitData;
+ if (FeatureInitData == NULL) {
1. It's impossible for FeatureInitData == NULL.
With this check removed, Reviewed-by: Ray Ni <ray.ni@intel.com>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [edk2-devel] [PATCH v1 1/2] UefiCpuPkg: Refactor initialization of CPU features during S3 resume
2021-09-16 6:14 ` [PATCH v1 1/2] UefiCpuPkg: Refactor initialization of " Ni, Ray
@ 2021-09-16 6:24 ` Jason Lou
0 siblings, 0 replies; 4+ messages in thread
From: Jason Lou @ 2021-09-16 6:24 UTC (permalink / raw)
To: Ni, Ray, devel
[-- Attachment #1: Type: text/plain, Size: 232 bytes --]
On Thu, Sep 16, 2021 at 02:14 PM, Ni, Ray wrote:
>
> FeatureInitData = &mAcpiCpuData.CpuFeatureInitData;
Hi Ray,
Yes, agree with you, I will remove the code blow.
>
> if (FeatureInitData == NULL) {
> return;
> }
>
[-- Attachment #2: Type: text/html, Size: 342 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-09-16 6:24 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-09-16 2:01 [PATCH v1 1/2] UefiCpuPkg: Refactor initialization of CPU features during S3 resume Jason Lou
2021-09-16 2:01 ` [PATCH v1 2/2] UefiCpuPkg: Prevent from re-initializing " Jason Lou
2021-09-16 6:14 ` [PATCH v1 1/2] UefiCpuPkg: Refactor initialization of " Ni, Ray
2021-09-16 6:24 ` [edk2-devel] " Jason Lou
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox