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-boun...@lists.01.org> On Behalf Of Laszlo Ersek
Sent: Monday, October 22, 2018 9:30 AM
To: Ni, Ruiyu <ruiyu...@intel.com>; edk2-devel@lists.01.org
Cc: Kinney, Michael D <michael.d.kin...@intel.com>; Yao, Jiewen
<jiewen....@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...@intel.com>
> Cc: Jiewen Yao <jiewen....@intel.com>
> Cc: Michael Kinney <michael.d.kin...@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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
