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

["Lohr, Paul A" <paul.a.lohr@intel.com>]

Hello,

Code to remove SMRAM = UC (line 650-ish) looks good.  I would suggest adding some debug comments in the area it was removed.  Thanks.

Per #4, I also "think" the second SMRAM = UC should be removed, in addition to the SMRAM = WB in the same area. But this is under investigation now.   Expect an update in 24-48 hours.

Per #5, I too do not like the "CPU AP" comment, as I initially thought it referred to Application Processor  😊  Yes, please clean that AP comment up!


Paul A. Lohr – Server Firmware Enabling
512.239.9073 (cell)
512.794.5044 (work)

-----Original Message-----
From: edk2-devel <edk2-devel-bounces@lists.01.org> On Behalf Of Laszlo Ersek
Sent: Monday, October 22, 2018 9:30 AM
To: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org
Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>
Subject: Re: [edk2] [PATCH] MdeModulePkg/PiSmmIpl: Do not reset SMRAM to UC when CPU driver runs

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.


(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

?


(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?


(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"?


... 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.

Thanks!
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel