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
