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: 5691 bytes --]

Comments below starting with [Ray]

Thanks,
Ray

________________________________
From: Wu, Jiaxin <jiaxin.wu@intel.com>
Sent: Tuesday, April 16, 2024 20:58
To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io <devel@edk2.groups.io>
Cc: Zeng, Star <star.zeng@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>
Subject: RE: [PATCH v2 02/10] UefiCpuPkg/SmmRelocationLib: Add SmmRelocationLib library instance



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

[Ray] Original code should also pass uncrustify checker. I guess you added one blank line above so the alignment of "=" is not required.




[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?

[Ray] A PEIM could run in flash in post-mem phase. If you pass the values across routines through parameters, most of the global variables can be avoided.


+  UINTN                           SmramRanges;

[Ray.6] No need SmramRanges.

[Jiaxin.6] replace it with DescriptorBlock ->NumberOfSmmReservedRegions directly?

[Ray] yes.


+  //
+  // 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.

[Ray] As long as you remove local variable SmramRanges, I am fine.



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

[Ray] I just want to avoid unnecessary memory allocation.




+
+  //
+  // 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?

[Ray] If the code is inherited from PiSmmCpuDxeSmm driver, you can do that change in another patch.

+  //
+  // 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:

[Ray] My purpose is to avoid global variables.



+  //
+  // 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).

[Ray] I agree with you regarding not calculating values in more than 1 place. Please think about how to avoid that.





-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117893): https://edk2.groups.io/g/devel/message/117893
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: 16138 bytes --]