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

[Laszlo Ersek <lersek@redhat.com>]

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