Re: [PATCH v4 4/5] UefiCpuPkg/SmmCpuFeaturesLib: Skip SMBASE configuration

["Ni, Ray" <ray.ni@intel.com>]

Reviewed-by: Ray Ni <ray.ni@intel.com>
  -- but please update the copyright year.

> -----Original Message-----
> From: Wu, Jiaxin <jiaxin.wu@intel.com>
> Sent: Friday, February 10, 2023 2:05 PM
> To: devel@edk2.groups.io
> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Zeng, Star
> <star.zeng@intel.com>; Laszlo Ersek <lersek@redhat.com>; Gerd Hoffmann
> <kraxel@redhat.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>
> Subject: [PATCH v4 4/5] UefiCpuPkg/SmmCpuFeaturesLib: Skip SMBASE
> configuration
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4337
> 
> 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);
>  }
> 
>  /**
>    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.inf
> b/UefiCpuPkg/Library/SmmCpuFeaturesLib/StandaloneMmCpuFeaturesLib.inf
> index b1f60a5505..63259e44e7 100644
> ---
> a/UefiCpuPkg/Library/SmmCpuFeaturesLib/StandaloneMmCpuFeaturesLib.inf
> +++
> b/UefiCpuPkg/Library/SmmCpuFeaturesLib/StandaloneMmCpuFeaturesLib.inf
> @@ -32,10 +32,14 @@
>  [LibraryClasses]
>    BaseLib
>    DebugLib
>    MemoryAllocationLib
>    PcdLib
> +  HobLib
> +
> +[Guids]
> +  gSmmBaseHobGuid                ## CONSUMES
> 
>  [FixedPcd]
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber        ##
> SOMETIMES_CONSUMES
> 
>  [FeaturePcd]
> --
> 2.16.2.windows.1