Re: [edk2-devel] [PATCH v2 02/10] UefiCpuPkg/SmmRelocationLib: Add SmmRelocationLib library instance

["Ni, Ray" <ray.ni@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 10997 bytes --]

Comments below starting with [Ray.xx]

Thanks,
Ray


 /**
@@ -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.



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117838): https://edk2.groups.io/g/devel/message/117838
Mute This Topic: https://groups.io/mt/105535806/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: Type: text/html, Size: 27210 bytes --]