Comments below starting with [Ray.xx]
/**
@@ -30,11 +30,12 @@ SemaphoreHook (
{
SMRAM_SAVE_STATE_MAP *CpuState;
mRebasedFlag = RebasedFlag;
- CpuState = (SMRAM_SAVE_STATE_MAP *)(UINTN)(SMM_DEFAULT_SMBASE + SMRAM_SAVE_STATE_MAP_OFFSET);
+ CpuState = (SMRAM_SAVE_STATE_MAP *)(UINTN)(SMM_DEFAULT_SMBASE + SMRAM_SAVE_STATE_MAP_OFFSET);
[Ray.1] This change is unnecessary.
+;-------------------------------------------------------------------------------
+
+%include "StuffRsbNasm.inc"
+
+global ASM_PFX(gcSmmInitIdtr)
+global ASM_PFX(gcSmmInitGdtr)
+
+extern ASM_PFX(SmmInitHandler)
+extern ASM_PFX(mRebasedFlag)
+extern ASM_PFX(mSmmRelocationOriginalAddress)
+
+global ASM_PFX(gPatchSmmCr3)
+global ASM_PFX(gPatchSmmCr4)
+global ASM_PFX(gPatchSmmCr0)
+global ASM_PFX(gPatchSmmInitStack)
+global ASM_PFX(gcSmmInitSize)
+global ASM_PFX(gcSmmInitTemplate)
[Ray.2] Can you create another patch after this patch to rename the global variables/functions to avoid using the same as PiSmmCpuDxeSmm driver?
+
+**/
+
+#ifndef INTERNAL_SMM_RELOCATION_LIB_H_
+#define INTERNAL_SMM_RELOCATION_LIB_H_
+
+#include <PiPei.h>
+#include <Library/BaseLib.h>
+#include <Library/BaseMemoryLib.h>
+#include <Library/CpuExceptionHandlerLib.h>
+#include <Library/DebugLib.h>
+#include <Library/HobLib.h>
+#include <Library/LocalApicLib.h>
+#include <Library/MemoryAllocationLib.h>
+#include <Library/PcdLib.h>
+#include <Library/PeimEntryPoint.h>
+#include <Library/PeiServicesLib.h>
+#include <Library/SmmRelocationLib.h>
+#include <Guid/SmramMemoryReserve.h>
+#include <Guid/SmmBaseHob.h>
+#include <Register/Intel/Cpuid.h>
+#include <Register/Intel/SmramSaveStateMap.h>
+#include <Protocol/MmCpu.h>
[Ray.3] Can you double confirm if all above headers are needed?
+
+extern UINT64 *mSmBaseForAllCpus;
+
+extern IA32_DESCRIPTOR gcSmmInitGdtr;
+extern IA32_DESCRIPTOR gcSmmInitIdtr;
+extern CONST UINT16 gcSmmInitSize;
+extern CONST UINT8 gcSmmInitTemplate[];
+
+X86_ASSEMBLY_PATCH_LABEL gPatchSmmCr0;
+X86_ASSEMBLY_PATCH_LABEL gPatchSmmCr3;
+X86_ASSEMBLY_PATCH_LABEL gPatchSmmCr4;
+X86_ASSEMBLY_PATCH_LABEL gPatchSmmInitStack;
[Ray.4] Can you evaluate what extra changes are required if allowing the lib runs in flash area? Basically all global variables cannot be modified at runtime.
+**/
+EFI_STATUS
+SplitSmramHobForSmmRelocation (
+ IN UINT64 SmmRelocationSize,
+ IN OUT EFI_PHYSICAL_ADDRESS *SmmRelocationStart
+ )
+{
+ EFI_HOB_GUID_TYPE *GuidHob;
+ EFI_SMRAM_HOB_DESCRIPTOR_BLOCK *DescriptorBlock;
+ EFI_SMRAM_HOB_DESCRIPTOR_BLOCK *NewDescriptorBlock;
+ UINTN BufferSize;
[Ray.5] How about Block, NewBlock, NewBlockSize? It's simpler and easy to understand.
+ UINTN SmramRanges;
[Ray.6] No need SmramRanges.
+
+ NewDescriptorBlock = NULL;
[Ray.7] No need of this line.
+
+ //
+ // Retrieve the GUID HOB data that contains the set of SMRAM descriptors
+ //
+ GuidHob = GetFirstGuidHob (&gEfiSmmSmramMemoryGuid);
+ if (GuidHob == NULL) {
+ return EFI_NOT_FOUND;
+ }
+
+ DescriptorBlock = (EFI_SMRAM_HOB_DESCRIPTOR_BLOCK *)GET_GUID_HOB_DATA (GuidHob);
+
+ //
+ // Allocate one extra EFI_SMRAM_DESCRIPTOR to describe SMRAM memory that contains a pointer
+ // to the Smm relocated memory.
[Ray.8] "pointer" is a bit confusing.
"Allocate one extra EFI_SMRAM_DESCRIPTOR to describe smram carved out for all SMBASE."
+ //
+ SmramRanges = DescriptorBlock->NumberOfSmmReservedRegions;
+ BufferSize = sizeof (EFI_SMRAM_HOB_DESCRIPTOR_BLOCK) + (SmramRanges * sizeof (EFI_SMRAM_DESCRIPTOR));
+
+ NewDescriptorBlock = (EFI_SMRAM_HOB_DESCRIPTOR_BLOCK *)BuildGuidHob (
+ &gEfiSmmSmramMemoryGuid,
+ BufferSize
+ );
+ ASSERT (NewDescriptorBlock != NULL);
+ if (NewDescriptorBlock == NULL) {
+ return EFI_DEVICE_ERROR;
+ }
+
+ //
+ // Copy old EFI_SMRAM_HOB_DESCRIPTOR_BLOCK to new allocated region
+ //
+ CopyMem ((VOID *)NewDescriptorBlock, DescriptorBlock, BufferSize - sizeof (EFI_SMRAM_DESCRIPTOR));
+
+ //
+ // Increase the number of SMRAM descriptors by 1 to make room for the ALLOCATED descriptor of size EFI_PAGE_SIZE
+ //
+ NewDescriptorBlock->NumberOfSmmReservedRegions = (UINT32)(SmramRanges + 1);
[Ray.9] NewBlock->NumberOfSmmReservedRegions++;
+
+ ASSERT (SmramRanges >= 1);
+ //
+ // Copy last entry to the end - we assume TSEG is last entry.
+ //
+ CopyMem (&NewDescriptorBlock->Descriptor[SmramRanges], &NewDescriptorBlock->Descriptor[SmramRanges - 1], sizeof (EFI_SMRAM_DESCRIPTOR));
+
+ //
+ // Update the entry in the array with a size of SmmRelocationSize and put into the ALLOCATED state
+ //
+ NewDescriptorBlock->Descriptor[SmramRanges - 1].PhysicalSize = SmmRelocationSize;
+ NewDescriptorBlock->Descriptor[SmramRanges - 1].RegionState |= EFI_ALLOCATED;
+
+ //
+ // Return the start address of Smm relocated memory in SMRAM.
+ //
+ if (SmmRelocationStart != NULL) {
[Ray.10] It's a local function and we know SmmRelocationStart is never NULL. No need the if-check.
+EFI_STATUS
+CreateSmmBaseHob (
+ IN UINT64 *SmBaseForAllCpus
+ )
+{
+ UINTN Index;
+ SMM_BASE_HOB_DATA *SmmBaseHobData;
+ UINT32 CpuCount;
+ UINT32 NumberOfProcessorsInHob;
+ UINT32 MaxCapOfProcessorsInHob;
+ UINT32 HobCount;
+
+ SmmBaseHobData = NULL;
+ CpuCount = 0;
+ NumberOfProcessorsInHob = 0;
+ MaxCapOfProcessorsInHob = 0;
+ HobCount = 0;
+
+ //
+ // Count the HOB instance maximum capacity of CPU (MaxCapOfProcessorsInHob) since the max HobLength is 0xFFF8.
+ //
+ MaxCapOfProcessorsInHob = (0xFFF8 - sizeof (EFI_HOB_GUID_TYPE) - sizeof (SMM_BASE_HOB_DATA)) / sizeof (UINT64) + 1;
+ DEBUG ((DEBUG_INFO, "CreateSmmBaseHob - MaxCapOfProcessorsInHob: %03x\n", MaxCapOfProcessorsInHob));
[Ray.11] "%03x" is confusing. Log readers cannot know "10" means 10 or 16. How about "%d"?
+
+ //
+ // Create Guided SMM Base HOB Instances.
+ //
+ while (CpuCount != mMaxNumberOfCpus) {
+ NumberOfProcessorsInHob = MIN ((UINT32)mMaxNumberOfCpus - CpuCount, MaxCapOfProcessorsInHob);
+
+ SmmBaseHobData = BuildGuidHob (
+ &gSmmBaseHobGuid,
+ sizeof (SMM_BASE_HOB_DATA) + sizeof (UINT64) * NumberOfProcessorsInHob
+ );
+ if (SmmBaseHobData == NULL) {
+ return EFI_OUT_OF_RESOURCES;
+ }
+
+ SmmBaseHobData->ProcessorIndex = CpuCount;
+ SmmBaseHobData->NumberOfProcessors = NumberOfProcessorsInHob;
+
+ DEBUG ((DEBUG_INFO, "CreateSmmBaseHob - SmmBaseHobData[%d]->ProcessorIndex: %03x\n", HobCount, SmmBaseHobData->ProcessorIndex));
+ DEBUG ((DEBUG_INFO, "CreateSmmBaseHob - SmmBaseHobData[%d]->NumberOfProcessors: %03x\n", HobCount, SmmBaseHobData->NumberOfProcessors));
+ for (Index = 0; Index < SmmBaseHobData->NumberOfProcessors; Index++) {
+ //
+ // Calculate the new SMBASE address
+ //
+ SmmBaseHobData->SmBase[Index] = SmBaseForAllCpus[Index + CpuCount];
[Ray.12] Please re-organize the code so that SmBaseForAllCpus array is not needed. What we need is only the Cpu0SmBase and TileSize.
+ DEBUG ((DEBUG_INFO, "CreateSmmBaseHob - SmmBaseHobData[%d]->SmBase[%03x]: %08x\n", HobCount, Index, SmmBaseHobData->SmBase[Index]));
[Ray.13] Same comments as above. Please use "0x" prefix if you prints hex otherwise the log is hard to understand.
+
+ //
+ // Patch ASM code template with current CR0, CR3, and CR4 values
+ //
+ PatchInstructionX86 (gPatchSmmCr0, AsmReadCr0 (), 4);
+ PatchInstructionX86 (gPatchSmmCr3, AsmReadCr3 (), 4);
+ PatchInstructionX86 (gPatchSmmCr4, AsmReadCr4 () & (~CR4_CET_ENABLE), 4);
[Ray.14] Similar question: Please try to remove the assumption that the lib runs in physical memory.
+
+ //
+ // Patch SMI stack for SMM base relocation
+ // Note: No need allocate stack for all CPUs since the relocation
+ // occurs serially for each CPU
+ //
+ SmmStackSize = EFI_PAGES_TO_SIZE (EFI_SIZE_TO_PAGES (PcdGet32 (PcdCpuSmmStackSize)));
[Ray.15] PcdCpuSmmStackSize is configured by platform as only platform knows what kind of SMI handlers will run in SMM env.
But in this case, all code is provided by this lib and stack size should be fixed, maybe simply 1 page is enough.
+ //
+ // Get the number of processors
+ //
+ Status = MpServices2->GetNumberOfProcessors (
+ MpServices2,
+ &mNumberOfCpus,
+ &NumberOfEnabledCpus
+ );
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
+ if (FeaturePcdGet (PcdCpuHotPlugSupport)) {
+ mMaxNumberOfCpus = PcdGet32 (PcdCpuMaxLogicalProcessorNumber);
+ } else {
+ mMaxNumberOfCpus = mNumberOfCpus;
+ }
+
+ //
+ // Retrieve the Processor Info for all CPUs
+ //
+ mProcessorInfo = (EFI_PROCESSOR_INFORMATION *)AllocatePool (sizeof (EFI_PROCESSOR_INFORMATION) * mMaxNumberOfCpus);
[Ray.16] mProcessorInfo is needed when programming the new SMBASE. Then can you just put the new SMBASE for every CPU in the stack top, or just patch the value in the code region?
You can do that in a new patch.
+ //
+ // Initialize the SmBase for all CPUs
+ //
+ Status = InitSmBaseForAllCpus (&mSmBaseForAllCpus);
[Ray.17] mSmBaseForAllCpus global variable is not needed. We only need Cpu0Smbase and TileSize and the two can be local variables and passed to further routines through parameters.
+++ b/UefiCpuPkg/Library/SmmRelocationLib/SmramSaveStateConfig.c
@@ -0,0 +1,139 @@
+/** @file
+ Config SMRAM Save State for SmmBases Relocation.
+
+ Copyright (c) 2024, Intel Corporation. All rights reserved.<BR>
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+#include "InternalSmmRelocationLib.h"
+#include <Library/CpuLib.h>
+
+/**
+ Determine the mode of the CPU at the time an SMI occurs
+
+ @retval EFI_MM_SAVE_STATE_REGISTER_LMA_32BIT 32 bit.
+ @retval EFI_MM_SAVE_STATE_REGISTER_LMA_64BIT 64 bit.
+
+**/
+UINT8
+CheckMmSaveStateRegisterLma (
[Ray.18] GetMmSaveStateRegisterLma(). I recommend to never use "check" in function name.