Re: [edk2-devel] [PATCH v2 3/4] UefiCpuPkg/SmmCpuFeaturesLib: Skip to configure SMBASE

["Chang, Abner" <abner.chang@amd.com>]

[AMD Official Use Only - General]

Hi Jia Xin,
One comment in below,

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Wu,
> Jiaxin via groups.io
> Sent: Friday, January 13, 2023 11:31 PM
> To: devel@edk2.groups.io
> Cc: Eric Dong <eric.dong@intel.com>; Ray Ni <ray.ni@intel.com>; Zeng Star
> <star.zeng@intel.com>; Laszlo Ersek <lersek@redhat.com>; Gerd Hoffmann
> <kraxel@redhat.com>; Rahul Kumar <rahul1.kumar@intel.com>
> Subject: [edk2-devel] [PATCH v2 3/4] UefiCpuPkg/SmmCpuFeaturesLib: Skip
> to configure SMBASE
> 
> Caution: This message originated from an External Source. Use proper
> caution when opening attachments, clicking links, or responding.
> 
> 
> This patch is to avoid configure SMBASE if SmBase relocation has been done.
> If gSmmBaseHobGuid found, means SmBase info has been relocated and
> recorded in the SmBase array. No need to do the relocation in
> SmmCpuFeaturesInitializeProcessor().
> 
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Zeng Star <star.zeng@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Signed-off-by: Jiaxin Wu <jiaxin.wu@intel.com>
> ---
>  .../Library/SmmCpuFeaturesLib/CpuFeaturesLib.h     |  2 ++
>  .../SmmCpuFeaturesLib/IntelSmmCpuFeaturesLib.c     | 23
> +++++++++++++++++++---
>  .../SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf        |  4 ++++
>  .../SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf     |  1 +
>  UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmStm.c      |  1 -
>  .../StandaloneMmCpuFeaturesLib.inf                 |  4 ++++
>  6 files changed, 31 insertions(+), 4 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/CpuFeaturesLib.h
> b/UefiCpuPkg/Library/SmmCpuFeaturesLib/CpuFeaturesLib.h
> index fd3e902547..c2e4fbe96b 100644
> --- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/CpuFeaturesLib.h
> +++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/CpuFeaturesLib.h
> @@ -7,15 +7,17 @@
>  **/
> 
>  #ifndef CPU_FEATURES_LIB_H_
>  #define CPU_FEATURES_LIB_H_
> 
> +#include <Guid/SmmBaseHob.h>
>  #include <Library/SmmCpuFeaturesLib.h>
>  #include <Library/BaseLib.h>
>  #include <Library/PcdLib.h>
>  #include <Library/MemoryAllocationLib.h>  #include <Library/DebugLib.h>
> +#include <Library/HobLib.h>
> 
>  /**
>    Performs library initialization.
> 
>    This initialization function contains common functionality shared betwen all
> diff --git
> a/UefiCpuPkg/Library/SmmCpuFeaturesLib/IntelSmmCpuFeaturesLib.c
> b/UefiCpuPkg/Library/SmmCpuFeaturesLib/IntelSmmCpuFeaturesLib.c
> index d5eaaa7a99..7c3836286b 100644
> --- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/IntelSmmCpuFeaturesLib.c
> +++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/IntelSmmCpuFeaturesLib.c
> @@ -36,10 +36,16 @@ SPDX-License-Identifier: BSD-2-Clause-Patent  // Set
> default value to assume IA-32 Architectural MSRs are used  //
>  UINT32  mSmrrPhysBaseMsr = SMM_FEATURES_LIB_IA32_SMRR_PHYSBASE;
>  UINT32  mSmrrPhysMaskMsr =
> SMM_FEATURES_LIB_IA32_SMRR_PHYSMASK;
> 
> +//
> +// Indicate SmBase for each Processors has been relocated or not. If
> +TRUE, // means no need to do the relocation in
> SmmCpuFeaturesInitializeProcessor().
> +//
> +BOOLEAN  mSmmCpuFeaturesSmmRelocated;
> +
>  //
>  // Set default value to assume MTRRs need to be configured on each SMI  //
> BOOLEAN  mNeedConfigureMtrrs = TRUE;
> 
> @@ -142,10 +148,16 @@ CpuFeaturesLibInitialization (
>    //
>    // Allocate array for state of SMRR enable on all CPUs
>    //
>    mSmrrEnabled = (BOOLEAN *)AllocatePool (sizeof (BOOLEAN) *
> GetCpuMaxLogicalProcessorNumber ());
>    ASSERT (mSmrrEnabled != NULL);
> +
> +  //
> +  // If gSmmBaseHobGuid found, means SmBase info has been relocated
> and
> + recorded  // in the SmBase array.
> +  //
> +  mSmmCpuFeaturesSmmRelocated = (BOOLEAN)(GetFirstGuidHob
> + (&gSmmBaseHobGuid) != NULL);
We can have this code and mSmmCpuFeaturesSmmRelocated in the SmmCpuFeaturesLibConstructor so this mechanism can be leverage by other vendors.
Thanks
Abner

>  }
> 
>  /**
>    Called during the very first SMI into System Management Mode to initialize
>    CPU features, including SMBASE, for the currently executing CPU.  Since
> this @@ -185,14 +197,19 @@ SmmCpuFeaturesInitializeProcessor (
>    UINT32                RegEdx;
>    UINTN                 FamilyId;
>    UINTN                 ModelId;
> 
>    //
> -  // Configure SMBASE.
> +  // No need to configure SMBASE if SmBase relocation has been done.
>    //
> -  CpuState             = (SMRAM_SAVE_STATE_MAP
> *)(UINTN)(SMM_DEFAULT_SMBASE + SMRAM_SAVE_STATE_MAP_OFFSET);
> -  CpuState->x86.SMBASE = (UINT32)CpuHotPlugData->SmBase[CpuIndex];
> +  if (!mSmmCpuFeaturesSmmRelocated) {
> +    //
> +    // Configure SMBASE.
> +    //
> +    CpuState             = (SMRAM_SAVE_STATE_MAP
> *)(UINTN)(SMM_DEFAULT_SMBASE + SMRAM_SAVE_STATE_MAP_OFFSET);
> +    CpuState->x86.SMBASE = (UINT32)CpuHotPlugData->SmBase[CpuIndex];
> +  }
> 
>    //
>    // Intel(R) 64 and IA-32 Architectures Software Developer's Manual
>    // Volume 3C, Section 35.2 MSRs in the Intel(R) Core(TM) 2 Processor
> Family
>    //
> diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
> b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
> index 9ac7dde78f..280a4b8b39 100644
> --- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
> +++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
> @@ -31,10 +31,14 @@
>  [LibraryClasses]
>    BaseLib
>    PcdLib
>    MemoryAllocationLib
>    DebugLib
> +  HobLib
> +
> +[Guids]
> +  gSmmBaseHobGuid                ## CONSUMES
> 
>  [Pcd]
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber        ##
> SOMETIMES_CONSUMES
> 
>  [FeaturePcd]
> diff --git
> a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf
> b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf
> index 86d367e0a0..4bb045244b 100644
> --- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf
> +++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf
> @@ -62,10 +62,11 @@
> 
>  [Guids]
>    gMsegSmramGuid                           ## SOMETIMES_CONSUMES ## HOB
>    gEfiAcpi20TableGuid                      ## SOMETIMES_CONSUMES ##
> SystemTable
>    gEfiAcpi10TableGuid                      ## SOMETIMES_CONSUMES ##
> SystemTable
> +  gSmmBaseHobGuid                          ## CONSUMES
> 
>  [Pcd]
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber        ##
> SOMETIMES_CONSUMES
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuMsegSize                         ##
> SOMETIMES_CONSUMES
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStmExceptionStackSize         ##
> SOMETIMES_CONSUMES
> diff --git a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmStm.c
> b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmStm.c
> index 3cf162ada0..455fe83991 100644
> --- a/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmStm.c
> +++ b/UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmStm.c
> @@ -6,11 +6,10 @@
> 
>  **/
> 
>  #include <PiMm.h>
>  #include <Library/BaseMemoryLib.h>
> -#include <Library/HobLib.h>
>  #include <Library/UefiBootServicesTableLib.h>
>  #include <Library/SmmServicesTableLib.h>  #include
> <Library/TpmMeasurementLib.h>  #include <Register/Intel/Cpuid.h>
> #include <Register/Intel/ArchitecturalMsr.h>
> diff --git
> a/UefiCpuPkg/Library/SmmCpuFeaturesLib/StandaloneMmCpuFeaturesLib.i
> nf
> b/UefiCpuPkg/Library/SmmCpuFeaturesLib/StandaloneMmCpuFeaturesLib.i
> nf
> index b1f60a5505..63259e44e7 100644
> ---
> a/UefiCpuPkg/Library/SmmCpuFeaturesLib/StandaloneMmCpuFeaturesLib.i
> nf
> +++
> b/UefiCpuPkg/Library/SmmCpuFeaturesLib/StandaloneMmCpuFeaturesLib.i
> n
> +++ f
> @@ -32,10 +32,14 @@
>  [LibraryClasses]
>    BaseLib
>    DebugLib
>    MemoryAllocationLib
>    PcdLib
> +  HobLib
> +
> +[Guids]
> +  gSmmBaseHobGuid                ## CONSUMES
> 
>  [FixedPcd]
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber        ##
> SOMETIMES_CONSUMES
> 
>  [FeaturePcd]
> --
> 2.16.2.windows.1
> 
> 
> 
> 
>