Re: [PATCH] MdeModulePkg/PiSmmIpl: Do not reset SMRAM to UC when CPU driver runs

["Ni, Ruiyu" <ruiyu.ni@Intel.com>]

On 10/22/2018 10:30 PM, Laszlo Ersek wrote:
> On 10/22/18 11:03, Ruiyu Ni wrote:
>> Today's PiSmmIpl implementation initially sets SMRAM to WB to speed
>> up the SMM core/modules loading before SMM CPU driver runs.
>> When SMM CPU driver runs, PiSmmIpl resets the SMRAM to UC. It's done
>> in SmmIplDxeDispatchEventNotify(). COMM_BUFFER_SMM_DISPATCH_RESTART
>> is returned from SMM core that SMM CPU driver is just dispatched.
>>
>> Since now the SMRR is widely used to control the SMRAM cache setting.
>> It's not needed to reset the SMRAM to UC anymore.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>> Cc: Michael Kinney <michael.d.kinney@intel.com>
>> ---
>>   MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c | 13 -------------
>>   1 file changed, 13 deletions(-)
>>
>> diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
>> index f8cbe1704b..dc0d9a70b0 100644
>> --- a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
>> +++ b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
>> @@ -672,19 +672,6 @@ SmmIplDxeDispatchEventNotify (
>>         return;
>>       }
>>   
>> -    //
>> -    // Attempt to reset SMRAM cacheability to UC
>> -    // Assume CPU AP is available at this time
>> -    //
>> -    Status = gDS->SetMemorySpaceAttributes(
>> -                    mSmramCacheBase,
>> -                    mSmramCacheSize,
>> -                    EFI_MEMORY_UC
>> -                    );
>> -    if (EFI_ERROR (Status)) {
>> -      DEBUG ((DEBUG_WARN, "SMM IPL failed to reset SMRAM window to EFI_MEMORY_UC\n"));
>> -    }
>> -
>>       //
>>       // Close all SMRAM ranges to protect SMRAM
>>       //
>>
> 
> I vaguely remember this code from commit b07ea4c198a4 ("MdeModulePkg:
> SmmIplEntry(): don't suppress SMM core startup failure", 2015-05-12). So
> here I'm asking just out of curiosity -- because SMRR is not necessary
> to handle on QEMU/KVM.
> 
> I assume that the original UC setting was part of the SMRAM protection
> that starts at the next line -- the comment is visible in the context,
> saying "Close all SMRAM ranges to protect SMRAM". The commit message
> suggests that explicit SMRR management has now taken that role.
> 
> 
> (1) Can you add more details to the commit message where that explicit
> management happens? I.e., what modules call what library interfaces? I
> believe the relevant APIs are from SmmCpuFeaturesLib.

Yes. SmmCpuFeaturesInitializeProcessor() from SmmCpuFeaturesLib enables 
SMRR

> 
> 
> (2) Also, can you elaborate on "widely"? Does that mean
> 
>    in most edk2 modules for which SMRAM caching is relevant
> 
> or
> 
>    in most edk2-based platform firmware that is shipped in production
> 
> ?

Most (ALL maybe) edk2-based platform firmware that is shipped in production.

> 
> 
> (3) You forgot to mention the bugzilla in the commit message:
> 
>    https://bugzilla.tianocore.org/show_bug.cgi?id=1268
> 
> 
> (4) From commit b07ea4c198a4 that I mention above, it seems that we have
> another instance of this UC setting in the SMM IPL runtime DXE driver;
> namely in the SmmIplEntry() function. Should we remove that too?
> 
> Now, I understand that the situation at that site is different, because
> in that case, we are resetting the cacheability after *failure* to
> launch the SMM Core, so there may have been no chance for any module to
> manage SMRR. And so we have to roll back our own initial WB setting. Do
> I understand that right?

Yes. I don't want to change that piece of code.

> 
> (5) I like that this patch removes a "CPU AP" comment, because the
> comment is misleading. In this context, "CPU AP" means "CPU
> Architectural Protocol" (which underlies
> gDS->SetMemorySpaceAttributes()), and not "Application Processor". It's
> good that the patch removes such a confusing comment.
> 
> However, other potential abuses of the shorthand "CPU AP" remain in the
> code. Would you consider submitting a separate patch that replaces all
> remaining instances of "CPU AP" with "CPU Arch Protocol", in
> "MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c"?

Sure the "AP" also confused me in the beginning. I will post a separate 
patch to change it.

> 
> 
> ... I originally intended to test this patch at once, but given my
> question (4), you might want to modify the code as well, not just the
> commit message, and so I figured I'd postpone the testing until your answer.

Let me check Paul's mail then decide what to do next.

> 
> Thanks!
> Laszlo
> 


-- 
Thanks,
Ray