Laszlo, Thanks your suggestion. I will add the code in commit log.
Jeff -----Original Message----- From: Laszlo Ersek [mailto:[email protected]] Sent: Thursday, October 29, 2015 10:43 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 14:17, Fan, Jeff wrote: > 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)); I don't understand how that CopyMem() call is supposed to set gSmst->CurrentlyExecutingCpu. - gSmst points to an EFI_SMM_SYSTEM_TABLE2 object. - gSmmCoreSmst is an object of the same type. In the EFI_SMM_SYSTEM_TABLE2 structure, "SmmStartupThisAp" and "CurrentlyExecutingCpu" are *sibling* fields. Neither of both is contained in the other. Therefore I don't understand how the above CopyMem() can set the CurrentlyExecutingCpu field. ... OMG... The CopyMem() actually *overflows* the SmmStartupThisAp field. That field is only used as a start offset into gSmmCoreSmst. EFI_SMM_ENTRY_CONTEXT contains a *subset* of the EFI_SMM_SYSTEM_TABLE2 fields, in the same order, and with the same sizes: - SmmStartupThisAp - CurrentlyExecutingCpu - NumberOfCpus - CpuSaveStateSize - ... Which is why the CopyMem() works. I'm sorry to say, but I think this is one of the ugliest pieces of code I've seen in my life. :( Anyway, what we have now is the following order: BSPHandler() gSmmCpuPrivate->SmmCoreEntryContext.CurrentlyExecutingCpu = CpuIndex; gSmmCpuPrivate->SmmCoreEntry (&gSmmCpuPrivate->SmmCoreEntryContext); CopyMem (&gSmmCoreSmst.SmmStartupThisAp, SmmEntryContext, sizeof (EFI_SMM_ENTRY_CONTEXT)); PerformRemainingTasks() [UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c] ConfigSmmCodeAccessCheck() ... gSmst->CurrentlyExecutingCpu ... So, I don't understand how the bug is possible. In the commit message you say, "ConfigSmmCodeAccessCheck() maybe invoked before executing SmmCoreEntry()". How? Ah! Now I understand, there is another ConfigSmmCodeAccessCheck() call in BSPHandler(), *before* SmmCoreEntry. It depends on "mRestoreSmmConfigurationInS3". Can you please add the following to the commit message: ----------- BSPHandler() gSmmCpuPrivate->SmmCoreEntryContext.CurrentlyExecutingCpu = CpuIndex; // // when mRestoreSmmConfigurationInS3 is set: // ConfigSmmCodeAccessCheck() // // reads gSmst->CurrentlyExecutingCpu to early // gSmmCpuPrivate->SmmCoreEntry ( &gSmmCpuPrivate->SmmCoreEntryContext) // // sets gSmst->CurrentlyExecutingCpu with CopyMem() too late // CopyMem (&gSmmCoreSmst.SmmStartupThisAp, SmmEntryContext, sizeof (EFI_SMM_ENTRY_CONTEXT)); ----------- With that change: Reviewed-by: Laszlo Ersek <[email protected]> Thanks Laszlo > > 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
