Comments below starting with [Ray]
Thanks,
Ray
________________________________
From: Wu, Jiaxin <jiaxin...@intel.com>
Sent: Tuesday, April 16, 2024 20:58
To: Ni, Ray <ray...@intel.com>; devel@edk2.groups.io <devel@edk2.groups.io>
Cc: Zeng, Star <star.z...@intel.com>; Gerd Hoffmann <kra...@redhat.com>; Kumar,
Rahul R <rahul.r.ku...@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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
