On 10/29/15 09:11, Jeff Fan wrote:
> In ConfigSmmCodeAccessCheck(), we used gSmst->CurrentlyExecutingCpu to get the
> current SMM BSP. But ConfigSmmCodeAccessCheck() maybe invoked before executing
> SmmCoreEntry() and gSmst->CurrentlyExecutingCpu hasn't been updated to the
> latest value. Instead, we should use
> gSmmCpuPrivate->SmmCoreEntryContext.CurrentlyExecutingCpu directly.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jeff Fan <[email protected]>
> Cc: Michael Kinney <[email protected]>
> Cc: Laszlo Ersek <[email protected]>
> ---
> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
> index e210c8d..c351875 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
> @@ -1386,7 +1386,7 @@ ConfigSmmCodeAccessCheck (
> //
> // Check to see if the Feature Control MSR is supported on this CPU
> //
> - Index = gSmst->CurrentlyExecutingCpu;
> + Index = gSmmCpuPrivate->SmmCoreEntryContext.CurrentlyExecutingCpu;
> if (!SmmCpuFeaturesIsSmmRegisterSupported (Index, SmmRegFeatureControl)) {
> mSmmCodeAccessCheckEnable = FALSE;
> return;
> @@ -1428,7 +1428,7 @@ ConfigSmmCodeAccessCheck (
> // Enable SMM Code Access Check feature for the APs.
> //
> for (Index = 0; Index < gSmst->NumberOfCpus; Index++) {
> - if (Index != gSmst->CurrentlyExecutingCpu) {
> + if (Index != gSmmCpuPrivate->SmmCoreEntryContext.CurrentlyExecutingCpu) {
>
> //
> // Acquire Config SMM Code Access Check spin lock. The AP will
> release the
>
I found precisely one location in the entire codebase where
"gSmmCpuPrivate->SmmCoreEntryContext.CurrentlyExecutingCpu" is set. The call
tree is:
BSPHandler() [UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c]
// sets field
gSmmCpuPrivate->SmmCoreEntryContext.CurrentlyExecutingCpu = CpuIndex;
PerformRemainingTasks() [UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c]
ConfigSmmCodeAccessCheck()
// reads the value here
>From that aspect this patch looks correct.
However... I have also found that gSmst->CurrentlyExecutingCpu is *never* set.
Is that a correct observation?
And if it is, shouldn't we rather (or additionally) fix *that*?
The commit message references SmmCoreEntry(). That function doesn't seem to
exist in edk2.
A field called "SmmCoreEntry" does exist in SMM_CPU_PRIVATE_DATA, and (IIRC) it
does point to SmmEntryPoint() [MdeModulePkg/Core/PiSmmCore/PiSmmCore.c], after
registration. But even SmmEntryPoint() does not seem to set
gSmst->CurrentlyExecutingCpu.
Thanks
Laszlo
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
