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

["Yao, Jiewen" <jiewen.yao@intel.com>]

Thanks. SMRR is used to prevent cache poisoning attack. IMHO, the assumption is valid.

Reviewed-by: Jiewen.yao@intel.com


> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ni,
> Ruiyu
> Sent: Tuesday, October 30, 2018 10:51 AM
> To: edk2-devel@lists.01.org
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Yao, Jiewen
> <jiewen.yao@intel.com>
> Subject: [edk2] [PATCH v3] MdeModulePkg/PiSmmIpl: Do not reset SMRAM
> to UC when CPU driver runs
> 
> 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 | 15 ++-------------
>  1 file changed, 2 insertions(+), 13 deletions(-)
> 
> diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
> b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
> index f8cbe1704b..2fb877127b 100644
> --- a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
> +++ b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
> @@ -672,21 +672,10 @@ 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
> +    // NOTE: SMRR is enabled by CPU SMM driver by calling
> SmmCpuFeaturesInitializeProcessor() from SmmCpuFeaturesLib
> +    //       so no need to reset the SMRAM to UC in MTRR.
>      //
>      Status = mSmmAccess->Close (mSmmAccess);
>      ASSERT_EFI_ERROR (Status);
> --
> 2.16.1.windows.1
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel