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

["Wu, Jiaxin" <jiaxin.wu@intel.com>]

[-- Attachment #1.1: Type: text/plain, Size: 13385 bytes --]



 /**
@@ -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.
[Jiaxin.1] it's not only move the code to the new lib, but also do some adaptation. For example here: the code style change to PASS the CI.
If no this change, can't pass the CI check.

+;-------------------------------------------------------------------------------
+
+%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?

[Jiaxin.2] I just rename below 2, all others are same as original and all has been moved to this lib, no definition in the smm cpu driver anymore, see below diff:

gcSmiIdtr -> gcSmmInitIdtr
gcSmiInitGdtr -> gcSmmInitGdtr

[cid:image001.png@01DA9037.9567A690]


+
+**/
+
+#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?
[Jiaxin.3] Except below 2, all others are needed (at least checked in OVMF).
#include <PiPei.h>
#include <Library/BaseLib.h>

+
+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.
[Jiaxin.4] The Lib needs to depend on the MP service PPI, it shall be called during post-mem phase, global variables can't be used?

+**/
+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.
[Jiaxin.5] Agree. I will change in the next version patches.

+  UINTN                           SmramRanges;
[Ray.6] No need SmramRanges.
[Jiaxin.6] replace it with DescriptorBlock ->NumberOfSmmReservedRegions directly?


+
+  NewDescriptorBlock = NULL;
[Ray.7] No need of this line.
[Jiaxin.7] Agree.


+
+  //
+  // 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."
[Jiaxin.8] Agree.

+  //
+  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++;
[Jiaxin.9] Agree since we copied the DescriptorBlock to NewDescriptorBlock first. But I still think original is more easy to understand.

+
+  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.
[Jiaxin.10] Agree

+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"?
[Jiaxin.11] Agree.

+
+  //
+  // 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.
[Jiaxin.12] do you mean calculate the value here? --> (UINTN)(Cpu0SmBase)+ Index * TileSize - SMM_HANDLER_OFFSET ?
I init the smbase value in the function of InitSmBaseForAllCpus(), the value will be used in both ConfigureSmBase & CreateSmmBaseHob.
How about the ConfigureSmBase function during SmmRelocateBases()? What's reason you think SmBaseForAllCpus is not good?


+      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.
[Jiaxin.13] i will align with above to %d, and change the %08x to 0x%08x.

+
+  //
+  // 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.
[Jiaxin.14] same as [Jiaxin.4]


+
+  //
+  // 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.
[Jiaxin.15] agree, do you prefer this change as another patch or I just update the hard code value directly in this patch?

+  //
+  // 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.
[Jiaxin.16] why need such behavior? I only allocate one stack for all cpus with existing design. The reason see below as I explained:

  //
  // 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)));
  SmmStacks    = (UINT8 *)AllocatePages (EFI_SIZE_TO_PAGES (SmmStackSize));
  if (SmmStacks == NULL) {
    Status = EFI_OUT_OF_RESOURCES;
    goto ON_EXIT;
  }


+  //
+  // 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.
[Jiaxin.17] let me remove the mSmBaseForAllCpus global variable. But I still think it's not good to calculate value in different 2 places again and again (ConfigureSmBase & CreateSmmBaseHob).

+++ 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.
[Jiaxin] agree.



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117880): https://edk2.groups.io/g/devel/message/117880
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 #1.2: Type: text/html, Size: 31904 bytes --]

[-- Attachment #2: image001.png --]
[-- Type: image/png, Size: 87383 bytes --]