Re: [PATCH v2 2/2] UefiCpuPkg: Prevent from re-initializing CPU features during S3 resume

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

Reviewed-by: Ray Ni <ray.ni@Intel.com>

> -----Original Message-----
> From: Lou, Yun <yun.lou@intel.com>
> Sent: Thursday, September 16, 2021 5:27 PM
> To: devel@edk2.groups.io
> Cc: Lou, Yun <yun.lou@intel.com>; Ni, Ray <ray.ni@intel.com>; Dong, Eric <eric.dong@intel.com>; Laszlo Ersek
> <lersek@redhat.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>
> Subject: [PATCH v2 2/2] UefiCpuPkg: Prevent from re-initializing CPU features during S3 resume
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3621
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3631
> 
> Current CPU feature initialization design:
> During normal boot, CpuFeaturesPei module (inside FSP) initializes the
> CPU features. During S3 boot, CpuFeaturesPei module does nothing, and
> CpuSmm driver (in SMRAM) initializes CPU features instead.
> 
> This code change prevents CpuSmm driver from re-initializing CPU
> features during S3 resume if CpuFeaturesPei module has done the same
> initialization.
> 
> In addition, EDK2 contains DxeIpl PEIM that calls S3RestoreConfig2 PPI
> during S3 boot and this PPI eventually calls CpuSmm driver (in SMRAM) to
> initialize the CPU features, so "EDK2 + FSP" does not have the CPU
> feature initialization issue during S3 boot. But "coreboot" does not
> contain DxeIpl PEIM and the issue appears, unless
> "PcdCpuFeaturesInitOnS3Resume" is set to TRUE.
> 
> Signed-off-by: Jason Lou <yun.lou@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> ---
>  UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c            | 34 ++++++++++++--------
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf |  3 +-
>  2 files changed, 23 insertions(+), 14 deletions(-)
> 
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> index 2873cba083..2496abb392 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> @@ -1152,23 +1152,31 @@ GetAcpiCpuData (
>    mAcpiCpuData.ApMachineCheckHandlerBase = (EFI_PHYSICAL_ADDRESS)(UINTN)MachineCheckHandlerForAp;
> 
> 
> 
>    ZeroMem (&mAcpiCpuData.CpuFeatureInitData, sizeof (CPU_FEATURE_INIT_DATA));
> 
> -  CopyCpuFeatureInitDatatoSmram (&mAcpiCpuData.CpuFeatureInitData, &AcpiCpuData->CpuFeatureInitData);
> 
> 
> 
> -  CpuStatus = &mAcpiCpuData.CpuFeatureInitData.CpuStatus;
> 
> +  if (!PcdGetBool (PcdCpuFeaturesInitOnS3Resume)) {
> 
> +    //
> 
> +    // If the CPU features will not be initialized by CpuFeaturesPei module during
> 
> +    // next ACPI S3 resume, copy the CPU features initialization data into SMRAM,
> 
> +    // which will be consumed in SmmRestoreCpu during next S3 resume.
> 
> +    //
> 
> +    CopyCpuFeatureInitDatatoSmram (&mAcpiCpuData.CpuFeatureInitData, &AcpiCpuData->CpuFeatureInitData);
> 
> 
> 
> -  mCpuFlags.CoreSemaphoreCount = AllocateZeroPool (
> 
> -                                   sizeof (UINT32) * CpuStatus->PackageCount *
> 
> -                                   CpuStatus->MaxCoreCount * CpuStatus->MaxThreadCount
> 
> -                                   );
> 
> -  ASSERT (mCpuFlags.CoreSemaphoreCount != NULL);
> 
> +    CpuStatus = &mAcpiCpuData.CpuFeatureInitData.CpuStatus;
> 
> 
> 
> -  mCpuFlags.PackageSemaphoreCount = AllocateZeroPool (
> 
> -                                      sizeof (UINT32) * CpuStatus->PackageCount *
> 
> -                                      CpuStatus->MaxCoreCount * CpuStatus->MaxThreadCount
> 
> -                                      );
> 
> -  ASSERT (mCpuFlags.PackageSemaphoreCount != NULL);
> 
> +    mCpuFlags.CoreSemaphoreCount = AllocateZeroPool (
> 
> +                                     sizeof (UINT32) * CpuStatus->PackageCount *
> 
> +                                     CpuStatus->MaxCoreCount * CpuStatus->MaxThreadCount
> 
> +                                     );
> 
> +    ASSERT (mCpuFlags.CoreSemaphoreCount != NULL);
> 
> 
> 
> -  InitializeSpinLock((SPIN_LOCK*) &mCpuFlags.MemoryMappedLock);
> 
> +    mCpuFlags.PackageSemaphoreCount = AllocateZeroPool (
> 
> +                                        sizeof (UINT32) * CpuStatus->PackageCount *
> 
> +                                        CpuStatus->MaxCoreCount * CpuStatus->MaxThreadCount
> 
> +                                        );
> 
> +    ASSERT (mCpuFlags.PackageSemaphoreCount != NULL);
> 
> +
> 
> +    InitializeSpinLock((SPIN_LOCK*) &mCpuFlags.MemoryMappedLock);
> 
> +  }
> 
>  }
> 
> 
> 
>  /**
> 
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
> index 76b1462996..0e88071c70 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
> @@ -4,7 +4,7 @@
>  # This SMM driver performs SMM initialization, deploy SMM Entry Vector,
> 
>  # provides CPU specific services in SMM.
> 
>  #
> 
> -# Copyright (c) 2009 - 2019, Intel Corporation. All rights reserved.<BR>
> 
> +# Copyright (c) 2009 - 2021, Intel Corporation. All rights reserved.<BR>
> 
>  # Copyright (c) 2017, AMD Incorporated. All rights reserved.<BR>
> 
>  #
> 
>  # SPDX-License-Identifier: BSD-2-Clause-Patent
> 
> @@ -134,6 +134,7 @@
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmCodeAccessCheckEnable         ## CONSUMES
> 
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode                      ## CONSUMES
> 
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmShadowStackSize               ## SOMETIMES_CONSUMES
> 
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuFeaturesInitOnS3Resume           ## CONSUMES
> 
>    gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiS3Enable                   ## CONSUMES
> 
>    gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask    ## CONSUMES
> 
>    gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask    ## CONSUMES
> 
> --
> 2.28.0.windows.1