* [PATCH v2 1/4] UefiCpuPkg/SmmBaseHob.h: Add SMM Base HOB Data
2023-01-13 15:30 [PATCH v2 0/4] Simplify SMM Relocated Process Wu, Jiaxin
@ 2023-01-13 15:30 ` Wu, Jiaxin
2023-01-16 1:39 ` Ni, Ray
2023-01-13 15:30 ` [PATCH v2 2/4] UefiCpuPkg/PiSmmCpuDxeSmm: Consume SMM Base Hob for SmBase info Wu, Jiaxin
` (2 subsequent siblings)
3 siblings, 1 reply; 12+ messages in thread
From: Wu, Jiaxin @ 2023-01-13 15:30 UTC (permalink / raw)
To: devel
Cc: Eric Dong, Ray Ni, Zeng Star, Laszlo Ersek, Gerd Hoffmann,
Rahul Kumar
The default SMBASE for the x86 processor is 0x30000. When
SMI happens, CPU runs the SMI handler at SMBASE+0x8000.
Also, the SMM save state area is within SMBASE+0x10000.
One of the SMM initialization from CPU perspective is to
program the new SMBASE (in TSEG range) for each CPU thread.
When the SMBASE relocated happens in one PEI module ahead
of the PiSmmCpuDxeSmm, the PEI module shall produce the
SMM_BASE_HOB in HOB database which tells the PiSmmCpuDxeSmm
driver which runs at a later phase about the new SMBASE for
each CPU thread. PiSmmCpuDxeSmm driver shall install the
SMI handler at the SMM_BASE_HOB.SmBase[Index]+0x8000 for
CPU thread Index. When the HOB doesn't exist,
PiSmmCpuDxeSmm driver shall program the new SMBASE itself.
This patch adds the SMM Base HOB, which can be produced
by any PEI module to do the SmBase relocation ahead of
PiSmmCpuDxeSmm driver and store the relocated SmBase
address in array for reach Processors.
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Zeng Star <star.zeng@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Signed-off-by: Jiaxin Wu <jiaxin.wu@intel.com>
---
UefiCpuPkg/Include/Guid/SmmBaseHob.h | 49 ++++++++++++++++++++++++++++++++++++
UefiCpuPkg/UefiCpuPkg.dec | 3 +++
2 files changed, 52 insertions(+)
create mode 100644 UefiCpuPkg/Include/Guid/SmmBaseHob.h
diff --git a/UefiCpuPkg/Include/Guid/SmmBaseHob.h b/UefiCpuPkg/Include/Guid/SmmBaseHob.h
new file mode 100644
index 0000000000..6ed32481dc
--- /dev/null
+++ b/UefiCpuPkg/Include/Guid/SmmBaseHob.h
@@ -0,0 +1,49 @@
+/** @file
+ The Smm Base HOB is used to store the information of:
+ * The relocated SmBase in array for each Processors.
+
+ The default SMBASE for the x86 processor is 0x30000. When SMI happens,
+ CPU runs the SMI handler at SMBASE+0x8000. Also, the SMM save state
+ area is within SMBASE+0x10000.
+
+ One of the SMM initialization from CPU perspective is to program the
+ new SMBASE (in TSEG range) for each CPU thread. When the SMBASE update
+ happens in a PEI module, the PEI module shall produce the SMM_BASE_HOB
+ in HOB database which tells the PiSmmCpuDxeSmm driver which runs at a
+ later phase about the new SMBASE for each CPU thread. PiSmmCpuDxeSmm
+ driver installs the SMI handler at the SMM_BASE_HOB.SmBase[Index]+0x8000
+ for CPU thread Index. When the HOB doesn't exist, PiSmmCpuDxeSmm driver
+ shall program the new SMBASE itself.
+
+ Copyright (c) 2023, Intel Corporation. All rights reserved.<BR>
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#ifndef SMM_BASE_HOB_H_
+#define SMM_BASE_HOB_H_
+
+#include <Protocol/MpService.h>
+#include <PiPei.h>
+
+#define SMM_BASE_HOB_DATA_GUID \
+ { \
+ 0xc2217ba7, 0x03bb, 0x4f63, {0xa6, 0x47, 0x7c, 0x25, 0xc5, 0xfc, 0x9d, 0x73} \
+ }
+
+#pragma pack(1)
+typedef struct {
+ ///
+ /// Describes the Number of all max supported processors.
+ ///
+ UINT64 NumberOfProcessors;
+ ///
+ /// Pointer to SmBase address for each Processors.
+ ///
+ UINT64 SmBase[];
+} SMM_BASE_HOB_DATA;
+#pragma pack()
+
+extern EFI_GUID gSmmBaseHobGuid;
+
+#endif
diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec
index cff239d528..2afd08cdd2 100644
--- a/UefiCpuPkg/UefiCpuPkg.dec
+++ b/UefiCpuPkg/UefiCpuPkg.dec
@@ -76,10 +76,13 @@
gEdkiiCpuFeaturesInitDoneGuid = { 0xc77c3a41, 0x61ab, 0x4143, { 0x98, 0x3e, 0x33, 0x39, 0x28, 0x6, 0x28, 0xe5 }}
## Include/Guid/MicrocodePatchHob.h
gEdkiiMicrocodePatchHobGuid = { 0xd178f11d, 0x8716, 0x418e, { 0xa1, 0x31, 0x96, 0x7d, 0x2a, 0xc4, 0x28, 0x43 }}
+ ## Include/Guid/SmmBaseHob.h
+ gSmmBaseHobGuid = { 0xc2217ba7, 0x03bb, 0x4f63, {0xa6, 0x47, 0x7c, 0x25, 0xc5, 0xfc, 0x9d, 0x73 }}
+
[Protocols]
## Include/Protocol/SmmCpuService.h
gEfiSmmCpuServiceProtocolGuid = { 0x1d202cab, 0xc8ab, 0x4d5c, { 0x94, 0xf7, 0x3c, 0xfc, 0xc0, 0xd3, 0xd3, 0x35 }}
gEdkiiSmmCpuRendezvousProtocolGuid = { 0xaa00d50b, 0x4911, 0x428f, { 0xb9, 0x1a, 0xa5, 0x9d, 0xdb, 0x13, 0xe2, 0x4c }}
--
2.16.2.windows.1
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH v2 1/4] UefiCpuPkg/SmmBaseHob.h: Add SMM Base HOB Data
2023-01-13 15:30 ` [PATCH v2 1/4] UefiCpuPkg/SmmBaseHob.h: Add SMM Base HOB Data Wu, Jiaxin
@ 2023-01-16 1:39 ` Ni, Ray
0 siblings, 0 replies; 12+ messages in thread
From: Ni, Ray @ 2023-01-16 1:39 UTC (permalink / raw)
To: Wu, Jiaxin, devel@edk2.groups.io
Cc: Dong, Eric, Zeng, Star, Laszlo Ersek, Gerd Hoffmann,
Kumar, Rahul R
> +
> +#include <Protocol/MpService.h>
> +#include <PiPei.h>
The Two headers can be removed.
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 2/4] UefiCpuPkg/PiSmmCpuDxeSmm: Consume SMM Base Hob for SmBase info
2023-01-13 15:30 [PATCH v2 0/4] Simplify SMM Relocated Process Wu, Jiaxin
2023-01-13 15:30 ` [PATCH v2 1/4] UefiCpuPkg/SmmBaseHob.h: Add SMM Base HOB Data Wu, Jiaxin
@ 2023-01-13 15:30 ` Wu, Jiaxin
2023-01-16 5:21 ` Ni, Ray
2023-01-13 15:30 ` [PATCH v2 3/4] UefiCpuPkg/SmmCpuFeaturesLib: Skip to configure SMBASE Wu, Jiaxin
2023-01-13 15:30 ` [PATCH v2 4/4] OvmfPkg/SmmCpuFeaturesLib: " Wu, Jiaxin
3 siblings, 1 reply; 12+ messages in thread
From: Wu, Jiaxin @ 2023-01-13 15:30 UTC (permalink / raw)
To: devel
Cc: Eric Dong, Ray Ni, Zeng Star, Laszlo Ersek, Gerd Hoffmann,
Rahul Kumar
PiSmmCpuDxeSmm will retrieve the SMBASE addresses from SMM Base Hob
and installs the SMI handler at [SMBASE+8000h] for each processor
instead of relocating SMM Base addresses from SMRAM again.
With SMM Base Hob, PiSmmCpuDxeSmm does not need the RSM
instruction to reload the SMBASE register with the new allocated
SMBASE each time when it exits SMM. SMBASE Register for each
processors have already been programmed and all SMBASE address
have recorded in SMM Base Hob. So the same default SMBASE Address
(0x30000) will not be used, thus the CPUs over-writing
each other's SMM Save State Area will not happen. This way makes
the first SMI init can be executed in parallel and save boot
time on multi-core system.
Mainly changes as below:
*Combine 2 SMIs (gcSmmInitTemplate & gcSmiHandlerTemplate) into one
(gcSmiHandlerTemplate), the new SMI handler needs to run to 2 paths:
one to SmmCpuFeaturesInitializeProcessor(), the other to SMM Core
Entry Point.
*Issue SMI IPI (All Excluding Self SMM IPI + BSP SMM IPI) for SMM
init before normal SMI sources happen.
*Call SmmCpuFeaturesInitializeProcessor() in parallel.
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Zeng Star <star.zeng@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Signed-off-by: Jiaxin Wu <jiaxin.wu@intel.com>
---
UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c | 21 ++-
UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 29 ++++-
UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c | 185 +++++++++++++++++++++------
UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 31 ++++-
UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf | 1 +
5 files changed, 217 insertions(+), 50 deletions(-)
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
index fb4a44eab6..66b05e74ed 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
@@ -822,13 +822,30 @@ SmmRestoreCpu (
//
InitializeCpuBeforeRebase ();
}
//
- // Restore SMBASE for BSP and all APs
+ // Retrive the allocated SmmBase from gSmmBaseHobGuid. If found,
+ // means the SmBase relocation has been done.
//
- SmmRelocateBases ();
+ mSmmRelocated = (BOOLEAN)(GetFirstGuidHob (&gSmmBaseHobGuid) != NULL);
+
+ //
+ // Check whether Smm Relocation is done or not.
+ // If not, will do the SmmBases Relocation here!!!
+ //
+ if (!mSmmRelocated) {
+ //
+ // Restore SMBASE for BSP and all APs
+ //
+ SmmRelocateBases ();
+ } else {
+ //
+ // Issue SMI IPI (All Excluding Self SMM IPI + BSP SMM IPI) to execute first SMI init.
+ //
+ ExecuteFirstSmiInit ();
+ }
//
// Skip initialization if mAcpiCpuData is not valid
//
if (mAcpiCpuData.NumberOfCpus > 0) {
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
index a0967eb69c..b64efd327c 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
@@ -1721,17 +1721,40 @@ SmiRendezvous (
UINTN Index;
UINTN Cr2;
ASSERT (CpuIndex < mMaxNumberOfCpus);
+ if (mSmmRelocated) {
+ ASSERT (mSmmInitialized != NULL);
+ }
+
//
// Save Cr2 because Page Fault exception in SMM may override its value,
// when using on-demand paging for above 4G memory.
//
Cr2 = 0;
SaveCr2 (&Cr2);
+ if (mSmmRelocated && !mSmmInitialized[CpuIndex]) {
+ //
+ // Perform SmmInitHandler for CpuIndex
+ //
+ SmmInitHandler ();
+
+ //
+ // Restore Cr2
+ //
+ RestoreCr2 (Cr2);
+
+ //
+ // Mark the first SMI init for CpuIndex has been done so as to avoid the reentry.
+ //
+ mSmmInitialized[CpuIndex] = TRUE;
+
+ return;
+ }
+
//
// Call the user register Startup function first.
//
if (mSmmMpSyncData->StartupProcedure != NULL) {
mSmmMpSyncData->StartupProcedure (mSmmMpSyncData->StartupProcArgs);
@@ -1882,13 +1905,13 @@ Exit:
//
RestoreCr2 (Cr2);
}
/**
- Initialize PackageBsp Info. Processor specified by mPackageFirstThreadIndex[PackageIndex]
- will do the package-scope register programming. Set default CpuIndex to (UINT32)-1, which
- means not specified yet.
+ Initialize mPackageFirstThreadIndex Info. Processor specified by mPackageFirstThreadIndex[PackageIndex]
+ will do the package-scope register programming. Set default CpuIndex to (UINT32)-1, which means not
+ specified yet.
**/
VOID
InitPackageFirstThreadIndexInfo (
VOID
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
index 655175a2c6..2ec423355f 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
@@ -57,11 +57,10 @@ SMM_CPU_PRIVATE_DATA *gSmmCpuPrivate = &mSmmCpuPrivateData;
//
// SMM Relocation variables
//
volatile BOOLEAN *mRebased;
-volatile BOOLEAN mIsBsp;
///
/// Handle for the SMM CPU Protocol
///
EFI_HANDLE mSmmCpuHandle = NULL;
@@ -83,10 +82,14 @@ EDKII_SMM_MEMORY_ATTRIBUTE_PROTOCOL mSmmMemoryAttribute = {
EdkiiSmmClearMemoryAttributes
};
EFI_CPU_INTERRUPT_HANDLER mExternalVectorTable[EXCEPTION_VECTOR_NUMBER];
+BOOLEAN mSmmRelocated = FALSE;
+BOOLEAN *mSmmInitialized = NULL;
+UINT32 mBspApicId = 0;
+
//
// SMM stack information
//
UINTN mSmmStackArrayBase;
UINTN mSmmStackArrayEnd;
@@ -341,60 +344,109 @@ VOID
EFIAPI
SmmInitHandler (
VOID
)
{
- UINT32 ApicId;
- UINTN Index;
+ UINT32 ApicId;
+ UINTN Index;
+ BOOLEAN IsBsp;
//
// Update SMM IDT entries' code segment and load IDT
//
AsmWriteIdtr (&gcSmiIdtr);
ApicId = GetApicId ();
+ IsBsp = (BOOLEAN)(mBspApicId == ApicId);
+
ASSERT (mNumberOfCpus <= mMaxNumberOfCpus);
for (Index = 0; Index < mNumberOfCpus; Index++) {
if (ApicId == (UINT32)gSmmCpuPrivate->ProcessorInfo[Index].ProcessorId) {
//
// Initialize SMM specific features on the currently executing CPU
//
SmmCpuFeaturesInitializeProcessor (
Index,
- mIsBsp,
+ IsBsp,
gSmmCpuPrivate->ProcessorInfo,
&mCpuHotPlugData
);
if (!mSmmS3Flag) {
//
// Check XD and BTS features on each processor on normal boot
//
CheckFeatureSupported ();
- }
-
- if (mIsBsp) {
+ } else if (IsBsp) {
//
// BSP rebase is already done above.
// Initialize private data during S3 resume
//
InitializeMpSyncData ();
}
- //
- // Hook return after RSM to set SMM re-based flag
- //
- SemaphoreHook (Index, &mRebased[Index]);
+ if (!mSmmRelocated) {
+ //
+ // Hook return after RSM to set SMM re-based flag
+ //
+ SemaphoreHook (Index, &mRebased[Index]);
+ }
return;
}
}
ASSERT (FALSE);
}
+/**
+ Issue SMI IPI (All Excluding Self SMM IPI + BSP SMM IPI) to execute first SMI init.
+
+**/
+VOID
+EFIAPI
+ExecuteFirstSmiInit (
+ VOID
+ )
+{
+ UINTN Index;
+
+ //
+ // Reset the mSmmInitialized to false.
+ //
+ if (mSmmInitialized == NULL) {
+ mSmmInitialized = (BOOLEAN *)AllocatePool (sizeof (BOOLEAN) * mMaxNumberOfCpus);
+ }
+
+ ASSERT (mSmmInitialized != NULL);
+ if (mSmmInitialized == NULL) {
+ return;
+ }
+
+ ZeroMem (mSmmInitialized, sizeof (BOOLEAN) * mMaxNumberOfCpus);
+
+ //
+ // Get the BSP ApicId.
+ //
+ mBspApicId = GetApicId ();
+
+ //
+ // Issue SMI IPI (All Excluding Self SMM IPI + BSP SMM IPI) for SMM init
+ //
+ SendSmiIpi (mBspApicId);
+ SendSmiIpiAllExcludingSelf ();
+
+ //
+ // Wait for all processors to finish its 1st SMI
+ //
+ for (Index = 0; Index < mNumberOfCpus; Index++) {
+ while (mSmmInitialized[Index] == FALSE) {
+ }
+ }
+}
+
/**
Relocate SmmBases for each processor.
Execute on first boot and all S3 resumes
@@ -407,11 +459,10 @@ SmmRelocateBases (
{
UINT8 BakBuf[BACK_BUF_SIZE];
SMRAM_SAVE_STATE_MAP BakBuf2;
SMRAM_SAVE_STATE_MAP *CpuStatePtr;
UINT8 *U8Ptr;
- UINT32 ApicId;
UINTN Index;
UINTN BspIndex;
//
// Make sure the reserved size is large enough for procedure SmmInitTemplate.
@@ -448,21 +499,20 @@ SmmRelocateBases (
CopyMem (U8Ptr, gcSmmInitTemplate, gcSmmInitSize);
//
// Retrieve the local APIC ID of current processor
//
- ApicId = GetApicId ();
+ mBspApicId = GetApicId ();
//
// Relocate SM bases for all APs
// This is APs' 1st SMI - rebase will be done here, and APs' default SMI handler will be overridden by gcSmmInitTemplate
//
- mIsBsp = FALSE;
BspIndex = (UINTN)-1;
for (Index = 0; Index < mNumberOfCpus; Index++) {
mRebased[Index] = FALSE;
- if (ApicId != (UINT32)gSmmCpuPrivate->ProcessorInfo[Index].ProcessorId) {
+ if (mBspApicId != (UINT32)gSmmCpuPrivate->ProcessorInfo[Index].ProcessorId) {
SendSmiIpi ((UINT32)gSmmCpuPrivate->ProcessorInfo[Index].ProcessorId);
//
// Wait for this AP to finish its 1st SMI
//
while (!mRebased[Index]) {
@@ -477,12 +527,11 @@ SmmRelocateBases (
//
// Relocate BSP's SMM base
//
ASSERT (BspIndex != (UINTN)-1);
- mIsBsp = TRUE;
- SendSmiIpi (ApicId);
+ SendSmiIpi (mBspApicId);
//
// Wait for the BSP to finish its 1st SMI
//
while (!mRebased[BspIndex]) {
}
@@ -561,10 +610,15 @@ PiCpuSmmEntry (
UINT32 RegEcx;
UINT32 RegEdx;
UINTN FamilyId;
UINTN ModelId;
UINT32 Cr3;
+ EFI_HOB_GUID_TYPE *GuidHob;
+ SMM_BASE_HOB_DATA *SmmBaseHobData;
+
+ GuidHob = NULL;
+ SmmBaseHobData = NULL;
//
// Initialize address fixup
//
PiSmmCpuSmmInitFixupAddress ();
@@ -789,30 +843,55 @@ PiCpuSmmEntry (
// context must be reduced.
//
ASSERT (TileSize <= (SMRAM_SAVE_STATE_MAP_OFFSET + sizeof (SMRAM_SAVE_STATE_MAP) - SMM_HANDLER_OFFSET));
//
- // Allocate buffer for all of the tiles.
+ // Check whether the Required TileSize is enough.
//
- // Intel(R) 64 and IA-32 Architectures Software Developer's Manual
- // Volume 3C, Section 34.11 SMBASE Relocation
- // For Pentium and Intel486 processors, the SMBASE values must be
- // aligned on a 32-KByte boundary or the processor will enter shutdown
- // state during the execution of a RSM instruction.
+ if (TileSize > SIZE_8KB) {
+ DEBUG ((DEBUG_ERROR, "The Range of Smbase in SMRAM is not enough -- Required TileSize = 0x%08x, Actual TileSize = 0x%08x\n", TileSize, SIZE_8KB));
+ CpuDeadLoop ();
+ return RETURN_BUFFER_TOO_SMALL;
+ }
+
//
- // Intel486 processors: FamilyId is 4
- // Pentium processors : FamilyId is 5
+ // Retrive the allocated SmmBase from gSmmBaseHobGuid. If found,
+ // means the SmBase relocation has been done.
//
- BufferPages = EFI_SIZE_TO_PAGES (SIZE_32KB + TileSize * (mMaxNumberOfCpus - 1));
- if ((FamilyId == 4) || (FamilyId == 5)) {
- Buffer = AllocateAlignedCodePages (BufferPages, SIZE_32KB);
+ GuidHob = GetFirstGuidHob (&gSmmBaseHobGuid);
+ if (GuidHob != NULL) {
+ SmmBaseHobData = GET_GUID_HOB_DATA (GuidHob);
+
+ ASSERT (SmmBaseHobData->NumberOfProcessors == mMaxNumberOfCpus);
+ mSmmRelocated = TRUE;
} else {
- Buffer = AllocateAlignedCodePages (BufferPages, SIZE_4KB);
- }
+ //
+ // When the HOB doesn't exist, allocate new SMBASE itself.
+ //
+ DEBUG ((DEBUG_INFO, "PiCpuSmmEntry: gSmmBaseHobGuid not found!\n"));
+ //
+ // Allocate buffer for all of the tiles.
+ //
+ // Intel(R) 64 and IA-32 Architectures Software Developer's Manual
+ // Volume 3C, Section 34.11 SMBASE Relocation
+ // For Pentium and Intel486 processors, the SMBASE values must be
+ // aligned on a 32-KByte boundary or the processor will enter shutdown
+ // state during the execution of a RSM instruction.
+ //
+ // Intel486 processors: FamilyId is 4
+ // Pentium processors : FamilyId is 5
+ //
+ BufferPages = EFI_SIZE_TO_PAGES (SIZE_32KB + TileSize * (mMaxNumberOfCpus - 1));
+ if ((FamilyId == 4) || (FamilyId == 5)) {
+ Buffer = AllocateAlignedCodePages (BufferPages, SIZE_32KB);
+ } else {
+ Buffer = AllocateAlignedCodePages (BufferPages, SIZE_4KB);
+ }
- ASSERT (Buffer != NULL);
- DEBUG ((DEBUG_INFO, "SMRAM SaveState Buffer (0x%08x, 0x%08x)\n", Buffer, EFI_PAGES_TO_SIZE (BufferPages)));
+ ASSERT (Buffer != NULL);
+ DEBUG ((DEBUG_INFO, "New Allcoated SMRAM SaveState Buffer (0x%08x, 0x%08x)\n", Buffer, EFI_PAGES_TO_SIZE (BufferPages)));
+ }
//
// Allocate buffer for pointers to array in SMM_CPU_PRIVATE_DATA.
//
gSmmCpuPrivate->ProcessorInfo = (EFI_PROCESSOR_INFORMATION *)AllocatePool (sizeof (EFI_PROCESSOR_INFORMATION) * mMaxNumberOfCpus);
@@ -843,11 +922,12 @@ PiCpuSmmEntry (
// Retrieve APIC ID of each enabled processor from the MP Services protocol.
// Also compute the SMBASE address, CPU Save State address, and CPU Save state
// size for each CPU in the platform
//
for (Index = 0; Index < mMaxNumberOfCpus; Index++) {
- mCpuHotPlugData.SmBase[Index] = (UINTN)Buffer + Index * TileSize - SMM_HANDLER_OFFSET;
+ mCpuHotPlugData.SmBase[Index] = mSmmRelocated ? (UINTN)SmmBaseHobData->SmBase[Index] : (UINTN)Buffer + Index * TileSize - SMM_HANDLER_OFFSET;
+
gSmmCpuPrivate->CpuSaveStateSize[Index] = sizeof (SMRAM_SAVE_STATE_MAP);
gSmmCpuPrivate->CpuSaveState[Index] = (VOID *)(mCpuHotPlugData.SmBase[Index] + SMRAM_SAVE_STATE_MAP_OFFSET);
gSmmCpuPrivate->Operation[Index] = SmmCpuNone;
if (Index < mNumberOfCpus) {
@@ -956,21 +1036,27 @@ PiCpuSmmEntry (
// Initialize IDT
//
InitializeSmmIdt ();
//
- // Relocate SMM Base addresses to the ones allocated from SMRAM
+ // Check whether Smm Relocation is done or not.
+ // If not, will do the SmmBases Relocation here!!!
//
- mRebased = (BOOLEAN *)AllocateZeroPool (sizeof (BOOLEAN) * mMaxNumberOfCpus);
- ASSERT (mRebased != NULL);
- SmmRelocateBases ();
+ if (!mSmmRelocated) {
+ //
+ // Relocate SMM Base addresses to the ones allocated from SMRAM
+ //
+ mRebased = (BOOLEAN *)AllocateZeroPool (sizeof (BOOLEAN) * mMaxNumberOfCpus);
+ ASSERT (mRebased != NULL);
+ SmmRelocateBases ();
- //
- // Call hook for BSP to perform extra actions in normal mode after all
- // SMM base addresses have been relocated on all CPUs
- //
- SmmCpuFeaturesSmmRelocationComplete ();
+ //
+ // Call hook for BSP to perform extra actions in normal mode after all
+ // SMM base addresses have been relocated on all CPUs
+ //
+ SmmCpuFeaturesSmmRelocationComplete ();
+ }
DEBUG ((DEBUG_INFO, "mXdSupported - 0x%x\n", mXdSupported));
//
// SMM Time initialization
@@ -997,10 +1083,25 @@ PiCpuSmmEntry (
);
}
}
}
+ //
+ // For relocated SMBASE, some MSRs & CSRs are still required to be configured in SMM Mode for SMM Initialization.
+ // Those MSRs & CSRs must be configured before normal SMI sources happen.
+ // So, here is to issue SMI IPI (All Excluding Self SMM IPI + BSP SMM IPI) to execute first SMI init.
+ //
+ if (mSmmRelocated) {
+ ExecuteFirstSmiInit ();
+
+ //
+ // Call hook for BSP to perform extra actions in normal mode after all
+ // SMM base addresses have been relocated on all CPUs
+ //
+ SmmCpuFeaturesSmmRelocationComplete ();
+ }
+
//
// Fill in SMM Reserved Regions
//
gSmmCpuPrivate->SmmReservedSmramRegion[0].SmramReservedStart = 0;
gSmmCpuPrivate->SmmReservedSmramRegion[0].SmramReservedSize = 0;
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
index 5f0a38e400..bddd0b5798 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
@@ -23,10 +23,11 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
#include <Protocol/MmMp.h>
#include <Guid/AcpiS3Context.h>
#include <Guid/MemoryAttributesTable.h>
#include <Guid/PiSmmMemoryAttributesTable.h>
+#include <Guid/SmmBaseHob.h>
#include <Library/BaseLib.h>
#include <Library/IoLib.h>
#include <Library/TimerLib.h>
#include <Library/SynchronizationLib.h>
@@ -346,10 +347,30 @@ SmmWriteSaveState (
IN EFI_SMM_SAVE_STATE_REGISTER Register,
IN UINTN CpuIndex,
IN CONST VOID *Buffer
);
+/**
+ C function for SMI handler. To change all processor's SMMBase Register.
+
+**/
+VOID
+EFIAPI
+SmmInitHandler (
+ VOID
+ );
+
+/**
+ Issue SMI IPI (All Excluding Self SMM IPI + BSP SMM IPI) to execute first SMI init.
+
+**/
+VOID
+EFIAPI
+ExecuteFirstSmiInit (
+ VOID
+ );
+
/**
Read a CPU Save State register on the target processor.
This function abstracts the differences that whether the CPU Save State register is in the
IA32 CPU Save State Map or X64 CPU Save State Map.
@@ -400,10 +421,14 @@ WriteSaveStateRegister (
IN EFI_SMM_SAVE_STATE_REGISTER Register,
IN UINTN Width,
IN CONST VOID *Buffer
);
+extern BOOLEAN mSmmRelocated;
+extern BOOLEAN *mSmmInitialized;
+extern UINT32 mBspApicId;
+
extern CONST UINT8 gcSmmInitTemplate[];
extern CONST UINT16 gcSmmInitSize;
X86_ASSEMBLY_PATCH_LABEL gPatchSmmCr0;
extern UINT32 mSmmCr0;
X86_ASSEMBLY_PATCH_LABEL gPatchSmmCr3;
@@ -1486,13 +1511,13 @@ RegisterStartupProcedure (
IN EFI_AP_PROCEDURE Procedure,
IN OUT VOID *ProcedureArguments OPTIONAL
);
/**
- Initialize PackageBsp Info. Processor specified by mPackageFirstThreadIndex[PackageIndex]
- will do the package-scope register programming. Set default CpuIndex to (UINT32)-1, which
- means not specified yet.
+ Initialize mPackageFirstThreadIndex Info. Processor specified by mPackageFirstThreadIndex[PackageIndex]
+ will do the package-scope register programming. Set default CpuIndex to (UINT32)-1, which means not
+ specified yet.
**/
VOID
InitPackageFirstThreadIndexInfo (
VOID
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
index b4b327f60c..6dbed17b96 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
@@ -112,10 +112,11 @@
[Guids]
gEfiAcpiVariableGuid ## SOMETIMES_CONSUMES ## HOB # it is used for S3 boot.
gEdkiiPiSmmMemoryAttributesTableGuid ## CONSUMES ## SystemTable
gEfiMemoryAttributesTableGuid ## CONSUMES ## SystemTable
+ gSmmBaseHobGuid ## CONSUMES
[FeaturePcd]
gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmDebug ## CONSUMES
gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmBlockStartupThisAp ## CONSUMES
gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmEnableBspElection ## CONSUMES
--
2.16.2.windows.1
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH v2 2/4] UefiCpuPkg/PiSmmCpuDxeSmm: Consume SMM Base Hob for SmBase info
2023-01-13 15:30 ` [PATCH v2 2/4] UefiCpuPkg/PiSmmCpuDxeSmm: Consume SMM Base Hob for SmBase info Wu, Jiaxin
@ 2023-01-16 5:21 ` Ni, Ray
2023-01-16 5:52 ` Wu, Jiaxin
0 siblings, 1 reply; 12+ messages in thread
From: Ni, Ray @ 2023-01-16 5:21 UTC (permalink / raw)
To: Wu, Jiaxin, devel@edk2.groups.io
Cc: Dong, Eric, Zeng, Star, Laszlo Ersek, Gerd Hoffmann,
Kumar, Rahul R
> //
> - SmmRelocateBases ();
> + mSmmRelocated = (BOOLEAN)(GetFirstGuidHob (&gSmmBaseHobGuid) != NULL);
Do we support the case when the HOB is produced in normal boot but is not in S3 boot?
To keep code flow simple and not to support unnecessary configuration combinations, I prefer no.
Then, since mSmmRelocated is initialized in normal boot already. How about only adding
check here to make sure the HOB existence status is the same between normal and s3 boot.
This also helps to catch platform bugs that the HOB is only produced in normal boot.
ASSERT (mSmmRelocated == (BOOLEAN)(GetFirstGuidHob (&gSmmBaseHobGuid) != NULL));
If (mSmmRelocated != (BOOLEAN)(GetFirstGuidHob (&gSmmBaseHobGuid) != NULL)) {
DEBUG ((
DEBUG_ERROR,
"SmmBase HOB %a produced in normal boot but %a in S3 boot",
mSmmRelocated ? "is" : "is not",
mSmmRelocated ? "is not" : "is"
));
CpuDeadLoop ();
}
>
> /**
> - Initialize PackageBsp Info. Processor specified by mPackageFirstThreadIndex[PackageIndex]
> - will do the package-scope register programming. Set default CpuIndex to (UINT32)-1, which
> - means not specified yet.
> + Initialize mPackageFirstThreadIndex Info. Processor specified by mPackageFirstThreadIndex[PackageIndex]
> + will do the package-scope register programming. Set default CpuIndex to (UINT32)-1, which means not
> + specified yet.
The comments update is good. Can you separate in another patch?
> - if (mIsBsp) {
> + } else if (IsBsp) {
I agree with this change. But it changes the old logic.
Old logic calls below function even in non-S3 path.
If we want to change the behavior, can we separate in a new patch?
> +VOID
> +EFIAPI
> +ExecuteFirstSmiInit (
Can you remove the EFIAPI?
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v2 2/4] UefiCpuPkg/PiSmmCpuDxeSmm: Consume SMM Base Hob for SmBase info
2023-01-16 5:21 ` Ni, Ray
@ 2023-01-16 5:52 ` Wu, Jiaxin
0 siblings, 0 replies; 12+ messages in thread
From: Wu, Jiaxin @ 2023-01-16 5:52 UTC (permalink / raw)
To: Ni, Ray, devel@edk2.groups.io
Cc: Dong, Eric, Zeng, Star, Laszlo Ersek, Gerd Hoffmann,
Kumar, Rahul R
>
> > //
> > - SmmRelocateBases ();
> > + mSmmRelocated = (BOOLEAN)(GetFirstGuidHob
> (&gSmmBaseHobGuid) != NULL);
>
> Do we support the case when the HOB is produced in normal boot but is not
> in S3 boot?
> To keep code flow simple and not to support unnecessary configuration
> combinations, I prefer no.
> Then, since mSmmRelocated is initialized in normal boot already. How about
> only adding
> check here to make sure the HOB existence status is the same between
> normal and s3 boot.
> This also helps to catch platform bugs that the HOB is only produced in
> normal boot.
>
> ASSERT (mSmmRelocated == (BOOLEAN)(GetFirstGuidHob
> (&gSmmBaseHobGuid) != NULL));
> If (mSmmRelocated != (BOOLEAN)(GetFirstGuidHob
> (&gSmmBaseHobGuid) != NULL)) {
> DEBUG ((
> DEBUG_ERROR,
> "SmmBase HOB %a produced in normal boot but %a in S3 boot",
> mSmmRelocated ? "is" : "is not",
> mSmmRelocated ? "is not" : "is"
> ));
> CpuDeadLoop ();
> }
> >
Agree, thanks Ray. Will refine the code in next patch.
> > /**
> > - Initialize PackageBsp Info. Processor specified by
> mPackageFirstThreadIndex[PackageIndex]
> > - will do the package-scope register programming. Set default CpuIndex to
> (UINT32)-1, which
> > - means not specified yet.
> > + Initialize mPackageFirstThreadIndex Info. Processor specified by
> mPackageFirstThreadIndex[PackageIndex]
> > + will do the package-scope register programming. Set default CpuIndex to
> (UINT32)-1, which means not
> > + specified yet.
>
> The comments update is good. Can you separate in another patch?
Okay, thanks Ray, I should never add any smuggled items even it's very simple change:).
>
> > - if (mIsBsp) {
> > + } else if (IsBsp) {
> I agree with this change. But it changes the old logic.
> Old logic calls below function even in non-S3 path.
> If we want to change the behavior, can we separate in a new patch?
>
> > +VOID
> > +EFIAPI
> > +ExecuteFirstSmiInit (
>
> Can you remove the EFIAPI?
>
Okay
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 3/4] UefiCpuPkg/SmmCpuFeaturesLib: Skip to configure SMBASE
2023-01-13 15:30 [PATCH v2 0/4] Simplify SMM Relocated Process Wu, Jiaxin
2023-01-13 15:30 ` [PATCH v2 1/4] UefiCpuPkg/SmmBaseHob.h: Add SMM Base HOB Data Wu, Jiaxin
2023-01-13 15:30 ` [PATCH v2 2/4] UefiCpuPkg/PiSmmCpuDxeSmm: Consume SMM Base Hob for SmBase info Wu, Jiaxin
@ 2023-01-13 15:30 ` Wu, Jiaxin
2023-01-16 5:23 ` [edk2-devel] " Chang, Abner
2023-01-13 15:30 ` [PATCH v2 4/4] OvmfPkg/SmmCpuFeaturesLib: " Wu, Jiaxin
3 siblings, 1 reply; 12+ messages in thread
From: Wu, Jiaxin @ 2023-01-13 15:30 UTC (permalink / raw)
To: devel
Cc: Eric Dong, Ray Ni, Zeng Star, Laszlo Ersek, Gerd Hoffmann,
Rahul Kumar
This patch is to avoid configure SMBASE if SmBase relocation has been
done. If gSmmBaseHobGuid found, means SmBase info has been relocated
and recorded in the SmBase array. No need to do the relocation in
SmmCpuFeaturesInitializeProcessor().
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Zeng Star <star.zeng@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Signed-off-by: Jiaxin Wu <jiaxin.wu@intel.com>
---
.../Library/SmmCpuFeaturesLib/CpuFeaturesLib.h | 2 ++
.../SmmCpuFeaturesLib/IntelSmmCpuFeaturesLib.c | 23 +++++++++++++++++++---
.../SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf | 4 ++++
.../SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf | 1 +
UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmStm.c | 1 -
.../StandaloneMmCpuFeaturesLib.inf | 4 ++++
6 files changed, 31 insertions(+), 4 deletions(-)
diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/CpuFeaturesLib.h b/UefiCpuPkg/Library/SmmCpuFeaturesLib/CpuFeaturesLib.h
index fd3e902547..c2e4fbe96b 100644
--- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/CpuFeaturesLib.h
+++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/CpuFeaturesLib.h
@@ -7,15 +7,17 @@
**/
#ifndef CPU_FEATURES_LIB_H_
#define CPU_FEATURES_LIB_H_
+#include <Guid/SmmBaseHob.h>
#include <Library/SmmCpuFeaturesLib.h>
#include <Library/BaseLib.h>
#include <Library/PcdLib.h>
#include <Library/MemoryAllocationLib.h>
#include <Library/DebugLib.h>
+#include <Library/HobLib.h>
/**
Performs library initialization.
This initialization function contains common functionality shared betwen all
diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/IntelSmmCpuFeaturesLib.c b/UefiCpuPkg/Library/SmmCpuFeaturesLib/IntelSmmCpuFeaturesLib.c
index d5eaaa7a99..7c3836286b 100644
--- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/IntelSmmCpuFeaturesLib.c
+++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/IntelSmmCpuFeaturesLib.c
@@ -36,10 +36,16 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
// Set default value to assume IA-32 Architectural MSRs are used
//
UINT32 mSmrrPhysBaseMsr = SMM_FEATURES_LIB_IA32_SMRR_PHYSBASE;
UINT32 mSmrrPhysMaskMsr = SMM_FEATURES_LIB_IA32_SMRR_PHYSMASK;
+//
+// Indicate SmBase for each Processors has been relocated or not. If TRUE,
+// means no need to do the relocation in SmmCpuFeaturesInitializeProcessor().
+//
+BOOLEAN mSmmCpuFeaturesSmmRelocated;
+
//
// Set default value to assume MTRRs need to be configured on each SMI
//
BOOLEAN mNeedConfigureMtrrs = TRUE;
@@ -142,10 +148,16 @@ CpuFeaturesLibInitialization (
//
// Allocate array for state of SMRR enable on all CPUs
//
mSmrrEnabled = (BOOLEAN *)AllocatePool (sizeof (BOOLEAN) * GetCpuMaxLogicalProcessorNumber ());
ASSERT (mSmrrEnabled != NULL);
+
+ //
+ // If gSmmBaseHobGuid found, means SmBase info has been relocated and recorded
+ // in the SmBase array.
+ //
+ mSmmCpuFeaturesSmmRelocated = (BOOLEAN)(GetFirstGuidHob (&gSmmBaseHobGuid) != NULL);
}
/**
Called during the very first SMI into System Management Mode to initialize
CPU features, including SMBASE, for the currently executing CPU. Since this
@@ -185,14 +197,19 @@ SmmCpuFeaturesInitializeProcessor (
UINT32 RegEdx;
UINTN FamilyId;
UINTN ModelId;
//
- // Configure SMBASE.
+ // No need to configure SMBASE if SmBase relocation has been done.
//
- CpuState = (SMRAM_SAVE_STATE_MAP *)(UINTN)(SMM_DEFAULT_SMBASE + SMRAM_SAVE_STATE_MAP_OFFSET);
- CpuState->x86.SMBASE = (UINT32)CpuHotPlugData->SmBase[CpuIndex];
+ if (!mSmmCpuFeaturesSmmRelocated) {
+ //
+ // Configure SMBASE.
+ //
+ CpuState = (SMRAM_SAVE_STATE_MAP *)(UINTN)(SMM_DEFAULT_SMBASE + SMRAM_SAVE_STATE_MAP_OFFSET);
+ CpuState->x86.SMBASE = (UINT32)CpuHotPlugData->SmBase[CpuIndex];
+ }
//
// Intel(R) 64 and IA-32 Architectures Software Developer's Manual
// Volume 3C, Section 35.2 MSRs in the Intel(R) Core(TM) 2 Processor Family
//
diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
index 9ac7dde78f..280a4b8b39 100644
--- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
+++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
@@ -31,10 +31,14 @@
[LibraryClasses]
BaseLib
PcdLib
MemoryAllocationLib
DebugLib
+ HobLib
+
+[Guids]
+ gSmmBaseHobGuid ## CONSUMES
[Pcd]
gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber ## SOMETIMES_CONSUMES
[FeaturePcd]
diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf
index 86d367e0a0..4bb045244b 100644
--- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf
+++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf
@@ -62,10 +62,11 @@
[Guids]
gMsegSmramGuid ## SOMETIMES_CONSUMES ## HOB
gEfiAcpi20TableGuid ## SOMETIMES_CONSUMES ## SystemTable
gEfiAcpi10TableGuid ## SOMETIMES_CONSUMES ## SystemTable
+ gSmmBaseHobGuid ## CONSUMES
[Pcd]
gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber ## SOMETIMES_CONSUMES
gUefiCpuPkgTokenSpaceGuid.PcdCpuMsegSize ## SOMETIMES_CONSUMES
gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStmExceptionStackSize ## SOMETIMES_CONSUMES
diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmStm.c b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmStm.c
index 3cf162ada0..455fe83991 100644
--- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmStm.c
+++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmStm.c
@@ -6,11 +6,10 @@
**/
#include <PiMm.h>
#include <Library/BaseMemoryLib.h>
-#include <Library/HobLib.h>
#include <Library/UefiBootServicesTableLib.h>
#include <Library/SmmServicesTableLib.h>
#include <Library/TpmMeasurementLib.h>
#include <Register/Intel/Cpuid.h>
#include <Register/Intel/ArchitecturalMsr.h>
diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/StandaloneMmCpuFeaturesLib.inf b/UefiCpuPkg/Library/SmmCpuFeaturesLib/StandaloneMmCpuFeaturesLib.inf
index b1f60a5505..63259e44e7 100644
--- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/StandaloneMmCpuFeaturesLib.inf
+++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/StandaloneMmCpuFeaturesLib.inf
@@ -32,10 +32,14 @@
[LibraryClasses]
BaseLib
DebugLib
MemoryAllocationLib
PcdLib
+ HobLib
+
+[Guids]
+ gSmmBaseHobGuid ## CONSUMES
[FixedPcd]
gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber ## SOMETIMES_CONSUMES
[FeaturePcd]
--
2.16.2.windows.1
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [edk2-devel] [PATCH v2 3/4] UefiCpuPkg/SmmCpuFeaturesLib: Skip to configure SMBASE
2023-01-13 15:30 ` [PATCH v2 3/4] UefiCpuPkg/SmmCpuFeaturesLib: Skip to configure SMBASE Wu, Jiaxin
@ 2023-01-16 5:23 ` Chang, Abner
2023-01-16 5:38 ` Ni, Ray
0 siblings, 1 reply; 12+ messages in thread
From: Chang, Abner @ 2023-01-16 5:23 UTC (permalink / raw)
To: devel@edk2.groups.io, jiaxin.wu@intel.com
Cc: Eric Dong, Ray Ni, Zeng Star, Laszlo Ersek, Gerd Hoffmann,
Rahul Kumar
[AMD Official Use Only - General]
Hi Jia Xin,
One comment in below,
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Wu,
> Jiaxin via groups.io
> Sent: Friday, January 13, 2023 11:31 PM
> To: devel@edk2.groups.io
> Cc: Eric Dong <eric.dong@intel.com>; Ray Ni <ray.ni@intel.com>; Zeng Star
> <star.zeng@intel.com>; Laszlo Ersek <lersek@redhat.com>; Gerd Hoffmann
> <kraxel@redhat.com>; Rahul Kumar <rahul1.kumar@intel.com>
> Subject: [edk2-devel] [PATCH v2 3/4] UefiCpuPkg/SmmCpuFeaturesLib: Skip
> to configure SMBASE
>
> Caution: This message originated from an External Source. Use proper
> caution when opening attachments, clicking links, or responding.
>
>
> This patch is to avoid configure SMBASE if SmBase relocation has been done.
> If gSmmBaseHobGuid found, means SmBase info has been relocated and
> recorded in the SmBase array. No need to do the relocation in
> SmmCpuFeaturesInitializeProcessor().
>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Zeng Star <star.zeng@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Signed-off-by: Jiaxin Wu <jiaxin.wu@intel.com>
> ---
> .../Library/SmmCpuFeaturesLib/CpuFeaturesLib.h | 2 ++
> .../SmmCpuFeaturesLib/IntelSmmCpuFeaturesLib.c | 23
> +++++++++++++++++++---
> .../SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf | 4 ++++
> .../SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf | 1 +
> UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmStm.c | 1 -
> .../StandaloneMmCpuFeaturesLib.inf | 4 ++++
> 6 files changed, 31 insertions(+), 4 deletions(-)
>
> diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/CpuFeaturesLib.h
> b/UefiCpuPkg/Library/SmmCpuFeaturesLib/CpuFeaturesLib.h
> index fd3e902547..c2e4fbe96b 100644
> --- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/CpuFeaturesLib.h
> +++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/CpuFeaturesLib.h
> @@ -7,15 +7,17 @@
> **/
>
> #ifndef CPU_FEATURES_LIB_H_
> #define CPU_FEATURES_LIB_H_
>
> +#include <Guid/SmmBaseHob.h>
> #include <Library/SmmCpuFeaturesLib.h>
> #include <Library/BaseLib.h>
> #include <Library/PcdLib.h>
> #include <Library/MemoryAllocationLib.h> #include <Library/DebugLib.h>
> +#include <Library/HobLib.h>
>
> /**
> Performs library initialization.
>
> This initialization function contains common functionality shared betwen all
> diff --git
> a/UefiCpuPkg/Library/SmmCpuFeaturesLib/IntelSmmCpuFeaturesLib.c
> b/UefiCpuPkg/Library/SmmCpuFeaturesLib/IntelSmmCpuFeaturesLib.c
> index d5eaaa7a99..7c3836286b 100644
> --- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/IntelSmmCpuFeaturesLib.c
> +++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/IntelSmmCpuFeaturesLib.c
> @@ -36,10 +36,16 @@ SPDX-License-Identifier: BSD-2-Clause-Patent // Set
> default value to assume IA-32 Architectural MSRs are used //
> UINT32 mSmrrPhysBaseMsr = SMM_FEATURES_LIB_IA32_SMRR_PHYSBASE;
> UINT32 mSmrrPhysMaskMsr =
> SMM_FEATURES_LIB_IA32_SMRR_PHYSMASK;
>
> +//
> +// Indicate SmBase for each Processors has been relocated or not. If
> +TRUE, // means no need to do the relocation in
> SmmCpuFeaturesInitializeProcessor().
> +//
> +BOOLEAN mSmmCpuFeaturesSmmRelocated;
> +
> //
> // Set default value to assume MTRRs need to be configured on each SMI //
> BOOLEAN mNeedConfigureMtrrs = TRUE;
>
> @@ -142,10 +148,16 @@ CpuFeaturesLibInitialization (
> //
> // Allocate array for state of SMRR enable on all CPUs
> //
> mSmrrEnabled = (BOOLEAN *)AllocatePool (sizeof (BOOLEAN) *
> GetCpuMaxLogicalProcessorNumber ());
> ASSERT (mSmrrEnabled != NULL);
> +
> + //
> + // If gSmmBaseHobGuid found, means SmBase info has been relocated
> and
> + recorded // in the SmBase array.
> + //
> + mSmmCpuFeaturesSmmRelocated = (BOOLEAN)(GetFirstGuidHob
> + (&gSmmBaseHobGuid) != NULL);
We can have this code and mSmmCpuFeaturesSmmRelocated in the SmmCpuFeaturesLibConstructor so this mechanism can be leverage by other vendors.
Thanks
Abner
> }
>
> /**
> Called during the very first SMI into System Management Mode to initialize
> CPU features, including SMBASE, for the currently executing CPU. Since
> this @@ -185,14 +197,19 @@ SmmCpuFeaturesInitializeProcessor (
> UINT32 RegEdx;
> UINTN FamilyId;
> UINTN ModelId;
>
> //
> - // Configure SMBASE.
> + // No need to configure SMBASE if SmBase relocation has been done.
> //
> - CpuState = (SMRAM_SAVE_STATE_MAP
> *)(UINTN)(SMM_DEFAULT_SMBASE + SMRAM_SAVE_STATE_MAP_OFFSET);
> - CpuState->x86.SMBASE = (UINT32)CpuHotPlugData->SmBase[CpuIndex];
> + if (!mSmmCpuFeaturesSmmRelocated) {
> + //
> + // Configure SMBASE.
> + //
> + CpuState = (SMRAM_SAVE_STATE_MAP
> *)(UINTN)(SMM_DEFAULT_SMBASE + SMRAM_SAVE_STATE_MAP_OFFSET);
> + CpuState->x86.SMBASE = (UINT32)CpuHotPlugData->SmBase[CpuIndex];
> + }
>
> //
> // Intel(R) 64 and IA-32 Architectures Software Developer's Manual
> // Volume 3C, Section 35.2 MSRs in the Intel(R) Core(TM) 2 Processor
> Family
> //
> diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
> b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
> index 9ac7dde78f..280a4b8b39 100644
> --- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
> +++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
> @@ -31,10 +31,14 @@
> [LibraryClasses]
> BaseLib
> PcdLib
> MemoryAllocationLib
> DebugLib
> + HobLib
> +
> +[Guids]
> + gSmmBaseHobGuid ## CONSUMES
>
> [Pcd]
> gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber ##
> SOMETIMES_CONSUMES
>
> [FeaturePcd]
> diff --git
> a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf
> b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf
> index 86d367e0a0..4bb045244b 100644
> --- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf
> +++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf
> @@ -62,10 +62,11 @@
>
> [Guids]
> gMsegSmramGuid ## SOMETIMES_CONSUMES ## HOB
> gEfiAcpi20TableGuid ## SOMETIMES_CONSUMES ##
> SystemTable
> gEfiAcpi10TableGuid ## SOMETIMES_CONSUMES ##
> SystemTable
> + gSmmBaseHobGuid ## CONSUMES
>
> [Pcd]
> gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber ##
> SOMETIMES_CONSUMES
> gUefiCpuPkgTokenSpaceGuid.PcdCpuMsegSize ##
> SOMETIMES_CONSUMES
> gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStmExceptionStackSize ##
> SOMETIMES_CONSUMES
> diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmStm.c
> b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmStm.c
> index 3cf162ada0..455fe83991 100644
> --- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmStm.c
> +++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmStm.c
> @@ -6,11 +6,10 @@
>
> **/
>
> #include <PiMm.h>
> #include <Library/BaseMemoryLib.h>
> -#include <Library/HobLib.h>
> #include <Library/UefiBootServicesTableLib.h>
> #include <Library/SmmServicesTableLib.h> #include
> <Library/TpmMeasurementLib.h> #include <Register/Intel/Cpuid.h>
> #include <Register/Intel/ArchitecturalMsr.h>
> diff --git
> a/UefiCpuPkg/Library/SmmCpuFeaturesLib/StandaloneMmCpuFeaturesLib.i
> nf
> b/UefiCpuPkg/Library/SmmCpuFeaturesLib/StandaloneMmCpuFeaturesLib.i
> nf
> index b1f60a5505..63259e44e7 100644
> ---
> a/UefiCpuPkg/Library/SmmCpuFeaturesLib/StandaloneMmCpuFeaturesLib.i
> nf
> +++
> b/UefiCpuPkg/Library/SmmCpuFeaturesLib/StandaloneMmCpuFeaturesLib.i
> n
> +++ f
> @@ -32,10 +32,14 @@
> [LibraryClasses]
> BaseLib
> DebugLib
> MemoryAllocationLib
> PcdLib
> + HobLib
> +
> +[Guids]
> + gSmmBaseHobGuid ## CONSUMES
>
> [FixedPcd]
> gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber ##
> SOMETIMES_CONSUMES
>
> [FeaturePcd]
> --
> 2.16.2.windows.1
>
>
>
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [edk2-devel] [PATCH v2 3/4] UefiCpuPkg/SmmCpuFeaturesLib: Skip to configure SMBASE
2023-01-16 5:23 ` [edk2-devel] " Chang, Abner
@ 2023-01-16 5:38 ` Ni, Ray
2023-01-16 5:46 ` Chang, Abner
0 siblings, 1 reply; 12+ messages in thread
From: Ni, Ray @ 2023-01-16 5:38 UTC (permalink / raw)
To: devel@edk2.groups.io, abner.chang@amd.com, Wu, Jiaxin
Cc: Dong, Eric, Zeng, Star, Laszlo Ersek, Gerd Hoffmann,
Kumar, Rahul R
> > + //
> > + // If gSmmBaseHobGuid found, means SmBase info has been relocated
> > and
> > + recorded // in the SmBase array.
> > + //
> > + mSmmCpuFeaturesSmmRelocated = (BOOLEAN)(GetFirstGuidHob
> > + (&gSmmBaseHobGuid) != NULL);
> We can have this code and mSmmCpuFeaturesSmmRelocated in the SmmCpuFeaturesLibConstructor so this mechanism
> can be leverage by other vendors.
Abner,
I guess you found this logic is useful in AMD flow.
Let's keep the current change as is and you could refactor the code later to present a full picture how AMD flow is like.
Thanks,
Ray
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [edk2-devel] [PATCH v2 3/4] UefiCpuPkg/SmmCpuFeaturesLib: Skip to configure SMBASE
2023-01-16 5:38 ` Ni, Ray
@ 2023-01-16 5:46 ` Chang, Abner
0 siblings, 0 replies; 12+ messages in thread
From: Chang, Abner @ 2023-01-16 5:46 UTC (permalink / raw)
To: Ni, Ray, devel@edk2.groups.io, Wu, Jiaxin
Cc: Dong, Eric, Zeng, Star, Laszlo Ersek, Gerd Hoffmann,
Kumar, Rahul R
[AMD Official Use Only - General]
> -----Original Message-----
> From: Ni, Ray <ray.ni@intel.com>
> Sent: Monday, January 16, 2023 1:38 PM
> To: devel@edk2.groups.io; Chang, Abner <Abner.Chang@amd.com>; Wu,
> Jiaxin <jiaxin.wu@intel.com>
> Cc: Dong, Eric <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>;
> Laszlo Ersek <lersek@redhat.com>; Gerd Hoffmann <kraxel@redhat.com>;
> Kumar, Rahul R <rahul.r.kumar@intel.com>
> Subject: RE: [edk2-devel] [PATCH v2 3/4] UefiCpuPkg/SmmCpuFeaturesLib:
> Skip to configure SMBASE
>
> Caution: This message originated from an External Source. Use proper
> caution when opening attachments, clicking links, or responding.
>
>
> > > + //
> > > + // If gSmmBaseHobGuid found, means SmBase info has been relocated
> > > and
> > > + recorded // in the SmBase array.
> > > + //
> > > + mSmmCpuFeaturesSmmRelocated = (BOOLEAN)(GetFirstGuidHob
> > > + (&gSmmBaseHobGuid) != NULL);
> > We can have this code and mSmmCpuFeaturesSmmRelocated in the
> > SmmCpuFeaturesLibConstructor so this mechanism can be leverage by
> other vendors.
>
> Abner,
> I guess you found this logic is useful in AMD flow.
> Let's keep the current change as is and you could refactor the code later to
> present a full picture how AMD flow is like.
This is also fine to me.
Thanks
Abner
>
> Thanks,
> Ray
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 4/4] OvmfPkg/SmmCpuFeaturesLib: Skip to configure SMBASE
2023-01-13 15:30 [PATCH v2 0/4] Simplify SMM Relocated Process Wu, Jiaxin
` (2 preceding siblings ...)
2023-01-13 15:30 ` [PATCH v2 3/4] UefiCpuPkg/SmmCpuFeaturesLib: Skip to configure SMBASE Wu, Jiaxin
@ 2023-01-13 15:30 ` Wu, Jiaxin
2023-01-16 8:12 ` Gerd Hoffmann
3 siblings, 1 reply; 12+ messages in thread
From: Wu, Jiaxin @ 2023-01-13 15:30 UTC (permalink / raw)
To: devel
Cc: Eric Dong, Ray Ni, Zeng Star, Laszlo Ersek, Gerd Hoffmann,
Rahul Kumar
This patch is to avoid configure SMBASE if SmBase relocation has been
done. If gSmmBaseHobGuid found, means SmBase info has been relocated
and recorded in the SmBase array. No need to do the relocation in
SmmCpuFeaturesInitializeProcessor().
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Zeng Star <star.zeng@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Signed-off-by: Jiaxin Wu <jiaxin.wu@intel.com>
---
.../Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c | 37 ++++++++++++++++------
.../SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf | 4 +++
2 files changed, 32 insertions(+), 9 deletions(-)
diff --git a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
index 6693666d04..d2841af6a4 100644
--- a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
+++ b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
@@ -15,20 +15,28 @@
#include <Library/PcdLib.h>
#include <Library/SafeIntLib.h>
#include <Library/SmmCpuFeaturesLib.h>
#include <Library/SmmServicesTableLib.h>
#include <Library/UefiBootServicesTableLib.h>
+#include <Library/HobLib.h>
#include <Pcd/CpuHotEjectData.h>
#include <PiSmm.h>
#include <Register/Intel/SmramSaveStateMap.h>
#include <Register/QemuSmramSaveStateMap.h>
+#include <Guid/SmmBaseHob.h>
//
// EFER register LMA bit
//
#define LMA BIT10
+//
+// Indicate SmBase for each Processors has been relocated or not. If TRUE,
+// means no need to do the relocation in SmmCpuFeaturesInitializeProcessor().
+//
+BOOLEAN mSmmCpuFeaturesSmmRelocated;
+
/**
The constructor function
@param[in] ImageHandle The firmware allocated handle for the EFI image.
@param[in] SystemTable A pointer to the EFI System Table.
@@ -41,10 +49,16 @@ EFIAPI
SmmCpuFeaturesLibConstructor (
IN EFI_HANDLE ImageHandle,
IN EFI_SYSTEM_TABLE *SystemTable
)
{
+ //
+ // If gSmmBaseHobGuid found, means SmBase info has been relocated and recorded
+ // in the SmBase array.
+ //
+ mSmmCpuFeaturesSmmRelocated = (BOOLEAN)(GetFirstGuidHob (&gSmmBaseHobGuid) != NULL);
+
//
// No need to program SMRRs on our virtual platform.
//
return EFI_SUCCESS;
}
@@ -83,20 +97,25 @@ SmmCpuFeaturesInitializeProcessor (
)
{
QEMU_SMRAM_SAVE_STATE_MAP *CpuState;
//
- // Configure SMBASE.
+ // No need to configure SMBASE if SmBase relocation has been done.
//
- CpuState = (QEMU_SMRAM_SAVE_STATE_MAP *)(UINTN)(
- SMM_DEFAULT_SMBASE +
- SMRAM_SAVE_STATE_MAP_OFFSET
- );
- if ((CpuState->x86.SMMRevId & 0xFFFF) == 0) {
- CpuState->x86.SMBASE = (UINT32)CpuHotPlugData->SmBase[CpuIndex];
- } else {
- CpuState->x64.SMBASE = (UINT32)CpuHotPlugData->SmBase[CpuIndex];
+ if (!mSmmCpuFeaturesSmmRelocated) {
+ //
+ // Configure SMBASE.
+ //
+ CpuState = (QEMU_SMRAM_SAVE_STATE_MAP *)(UINTN)(
+ SMM_DEFAULT_SMBASE +
+ SMRAM_SAVE_STATE_MAP_OFFSET
+ );
+ if ((CpuState->x86.SMMRevId & 0xFFFF) == 0) {
+ CpuState->x86.SMBASE = (UINT32)CpuHotPlugData->SmBase[CpuIndex];
+ } else {
+ CpuState->x64.SMBASE = (UINT32)CpuHotPlugData->SmBase[CpuIndex];
+ }
}
//
// No need to program SMRRs on our virtual platform.
//
diff --git a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
index 8a426a4c10..6a281518f5 100644
--- a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
+++ b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
@@ -33,10 +33,14 @@
MemoryAllocationLib
PcdLib
SafeIntLib
SmmServicesTableLib
UefiBootServicesTableLib
+ HobLib
+
+[Guids]
+ gSmmBaseHobGuid ## CONSUMES
[Pcd]
gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber
gUefiOvmfPkgTokenSpaceGuid.PcdCpuHotEjectDataAddress
gUefiOvmfPkgTokenSpaceGuid.PcdQ35SmramAtDefaultSmbase
--
2.16.2.windows.1
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH v2 4/4] OvmfPkg/SmmCpuFeaturesLib: Skip to configure SMBASE
2023-01-13 15:30 ` [PATCH v2 4/4] OvmfPkg/SmmCpuFeaturesLib: " Wu, Jiaxin
@ 2023-01-16 8:12 ` Gerd Hoffmann
0 siblings, 0 replies; 12+ messages in thread
From: Gerd Hoffmann @ 2023-01-16 8:12 UTC (permalink / raw)
To: Jiaxin Wu; +Cc: devel, Eric Dong, Ray Ni, Zeng Star, Laszlo Ersek, Rahul Kumar
On Fri, Jan 13, 2023 at 11:30:45PM +0800, Jiaxin Wu wrote:
> This patch is to avoid configure SMBASE if SmBase relocation has been
> done. If gSmmBaseHobGuid found, means SmBase info has been relocated
> and recorded in the SmBase array. No need to do the relocation in
> SmmCpuFeaturesInitializeProcessor().
You will not find a gSmmBaseHobGuid HOB if you don't produce it
somewhere.
take care,
Gerd
^ permalink raw reply [flat|nested] 12+ messages in thread