Laszlo, SmmCoreEntry() is from the following code, and it is just PiSmmCore's SmmEntryPoint(). // // Invoke SMM Foundation EntryPoint with the processor information context. // gSmmCpuPrivate->SmmCoreEntry (&gSmmCpuPrivate->SmmCoreEntryContext);
Yes. gSmst->CurrentlyExecutingCpu is not set in PiSmmCpuDxeSmm driver. It is set in PiSmmCore driver by the following code. // // Update SMST using the context // CopyMem (&gSmmCoreSmst.SmmStartupThisAp, SmmEntryContext, sizeof (EFI_SMM_ENTRY_CONTEXT)); Thanks! Jeff -----Original Message----- From: Laszlo Ersek [mailto:[email protected]] Sent: Thursday, October 29, 2015 8:46 PM To: Fan, Jeff; [email protected] Cc: Kinney, Michael D Subject: Re: [edk2] [Patch] UefiCpuPkg/PiSmmCpuDxeSmm: Shouldn't use gSmst->CurrentlyExecutingCpu 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
