Re: [PATCH] UefiCpuPkg: Adding a new TSC library by using CPUID(0x15) TSC leaf

["Zeng, Star" <star.zeng@intel.com>]

Some comments below.

> -----Original Message-----
> From: Kuo, Donald
> Sent: Monday, August 12, 2019 1:57 PM
> To: devel@edk2.groups.io
> Cc: Ni, Ray <ray.ni@intel.com>; Zeng, Star <star.zeng@intel.com>; Dong, Eric
> <eric.dong@intel.com>
> Subject: [PATCH] UefiCpuPkg: Adding a new TSC library by using CPUID(0x15)
> TSC leaf
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1909
> 
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Signed-off-by: Donald Kuo <donald.kuo@intel.com>
> ---
>  .../Library/BaseCpuTimerLib/BaseCpuTimerLib.c      |  40 +++
>  .../Library/BaseCpuTimerLib/BaseCpuTimerLib.inf    |  35 +++
>  .../Library/BaseCpuTimerLib/BaseCpuTimerLib.uni    |  17 ++
>  UefiCpuPkg/Library/BaseCpuTimerLib/CpuTimerLib.c   | 290
> +++++++++++++++++++++
>  UefiCpuPkg/UefiCpuPkg.dec                          |   8 +
>  UefiCpuPkg/UefiCpuPkg.dsc                          |   4 +-
>  6 files changed, 393 insertions(+), 1 deletion(-)  create mode 100644
> UefiCpuPkg/Library/BaseCpuTimerLib/BaseCpuTimerLib.c
>  create mode 100644
> UefiCpuPkg/Library/BaseCpuTimerLib/BaseCpuTimerLib.inf
>  create mode 100644
> UefiCpuPkg/Library/BaseCpuTimerLib/BaseCpuTimerLib.uni
>  create mode 100644 UefiCpuPkg/Library/BaseCpuTimerLib/CpuTimerLib.c
> 


[Trimmed]


> +
> diff --git a/UefiCpuPkg/Library/BaseCpuTimerLib/CpuTimerLib.c
> b/UefiCpuPkg/Library/BaseCpuTimerLib/CpuTimerLib.c
> new file mode 100644
> index 0000000000..5ed01146cf
> --- /dev/null
> +++ b/UefiCpuPkg/Library/BaseCpuTimerLib/CpuTimerLib.c
> @@ -0,0 +1,290 @@
> +/** @file
> +  CPUID Leaf 0x15 for Core Crystal Clock frequency instance of Timer Library.
> +
> +  Copyright (c) 2019 Intel Corporation. All rights reserved.<BR>
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#include <Base.h>
> +#include <Library/TimerLib.h>
> +#include <Library/BaseLib.h>
> +#include <Library/PcdLib.h>
> +#include <Library/DebugLib.h>
> +#include <Register/Cpuid.h>
> +
> +/**
> +  Internal function to retrieves the 64-bit frequency in Hz.
> +
> +  Internal function to retrieves the 64-bit frequency in Hz.
> +
> +  @return The frequency in Hz.
> +
> +**/
> +UINT64
> +InternalGetPerformanceCounterFrequency (
> +  VOID
> +  );
> +
> +/**
> +  CPUID Leaf 0x15 for Core Crystal Clock Frequency.
> +
> +  The TSC counting frequency is determined by using CPUID leaf 0x15.
> Frequency in MHz = Core XTAL frequency * EBX/EAX.
> +  In newer flavors of the CPU, core xtal frequency is returned in ECX or 0 if
> not supported.
> +  @return The number of TSC counts per second.
> +
> +**/
> +UINT64
> +CpuidCoreClockCalculateTscFrequency (
> +  VOID
> +  )
> +{
> +  CPUID_VERSION_INFO_EAX Eax;
> +  UINT64                 TscFrequency;
> +  UINT64                 CoreXtalFrequency;
> +  UINT32                 RegEax;
> +  UINT32                 RegEbx;
> +  UINT32                 RegEcx;
> +
> +  //
> +  // Display CPU FAMILY / MODEL / STEPPING ID Info  //  AsmCpuid
> + (CPUID_VERSION_INFO, &Eax.Uint32, NULL, NULL, NULL);  DEBUG
> + ((DEBUG_INFO, "CPUID = %X\n", (Eax.Uint32 & 0x0FFF0FFF)));

Suggest removing this debugging code block.

> +
> +  //
> +  // Use CPUID leaf 0x15 Time Stamp Counter and Nominal Core Crystal
> + Clock Information  // EBX returns 0 if not supported. ECX, if non zero,
> provides Core Xtal Frequency in hertz.
> +  // TSC frequency = (ECX, Core Xtal Frequency) * EBX/EAX.
> +  //
> +  AsmCpuid (CPUID_TIME_STAMP_COUNTER, &RegEax, &RegEbx, &RegEcx,
> NULL);
> + DEBUG ((DEBUG_INFO, "Denominator of the TSC ratio = %d\n", RegEax));
> + DEBUG ((DEBUG_INFO, "Numerator of the TSC ratio = %d\n", RegEbx));
> + DEBUG ((DEBUG_INFO, "Nominal frequency (hertz) = %d\n", RegEcx));

Suggest removing this debug message codes as the timerlib may be used by AP.

> +
> +  //
> +  // If EBX returns 0, the XTAL ratio is not enumerated.
> +  //
> +  if (RegEbx == 0) {
> +    DEBUG ((DEBUG_ERROR, "The CPU is not capble for Core Crystal Clock
> Frequency !!\n"));

Suggest removing this debug message codes as the timerlib may be used by AP.
Then the if condition can be also removed.

> +    ASSERT (RegEbx != 0);
> +  }
> +  //
> +  // If ECX returns 0, the XTAL frequency is not enumerated.
> +  //
> +  if (RegEcx == 0) {
> +    DEBUG ((DEBUG_ERROR, "The CPU is not capble for Core Crystal Clock
> Frequency !!\n"));

Suggest removing this debug message codes as the timerlib may be used by AP.

> +    CoreXtalFrequency = PcdGet64 (PcdCpuCoreCrystalClockFrequency);
> +    DEBUG ((DEBUG_INFO, "CoreXtalFrequency (hertz) from PCD = %d\n",
> CoreXtalFrequency));

Suggest removing this debug message codes as the timerlib may be used by AP.

> +    //ASSERT (RegEcx != 0);

Suggest removing this line

> +  } else {
> +    CoreXtalFrequency = (UINT64) RegEcx;  }
> +
> +  //
> +  // Calculate TSC frequency = (ECX, Core Xtal Frequency) * EBX/EAX  //
> + TscFrequency = DivU64x32 (MultU64x32 (CoreXtalFrequency, RegEbx) +
> + (UINT64)(RegEax >> 1), RegEax);
> +
> +  return TscFrequency;
> +}
> +

[Trimmed]

> --- a/UefiCpuPkg/UefiCpuPkg.dsc
> +++ b/UefiCpuPkg/UefiCpuPkg.dsc
> @@ -42,7 +42,7 @@
>    PeimEntryPoint|MdePkg/Library/PeimEntryPoint/PeimEntryPoint.inf
>    PeiServicesLib|MdePkg/Library/PeiServicesLib/PeiServicesLib.inf
> 
> PerformanceLib|MdePkg/Library/BasePerformanceLibNull/BasePerformanc
> eLibNull.inf
> -
> TimerLib|MdePkg/Library/BaseTimerLibNullTemplate/BaseTimerLibNullTem
> plate.inf
> +#
> +TimerLib|MdePkg/Library/BaseTimerLibNullTemplate/BaseTimerLibNullTe
> mpla
> +te.inf

Suggest removing this line.

Thanks,
Star

> 
> DebugAgentLib|MdeModulePkg/Library/DebugAgentLibNull/DebugAgentLi
> bNull.inf
> 
> LocalApicLib|UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.in
> f
> 
> ReportStatusCodeLib|MdePkg/Library/BaseReportStatusCodeLibNull/BaseR
> eportStatusCodeLibNull.inf
> @@ -56,6 +56,7 @@
> 
> PeCoffGetEntryPointLib|MdePkg/Library/BasePeCoffGetEntryPointLib/Base
> PeCoffGetEntryPointLib.inf
> 
> PeCoffExtraActionLib|MdePkg/Library/BasePeCoffExtraActionLibNull/BaseP
> eCoffExtraActionLibNull.inf
> 
> TpmMeasurementLib|MdeModulePkg/Library/TpmMeasurementLibNull/Tp
> mMeasurementLibNull.inf
> +  TimerLib|UefiCpuPkg/Library/BaseCpuTimerLib/BaseCpuTimerLib.inf
> 
>  [LibraryClasses.common.SEC]
> 
> PlatformSecLib|UefiCpuPkg/Library/PlatformSecLibNull/PlatformSecLibNull.i
> nf
> @@ -143,6 +144,7 @@
> 
> SmmCpuFeaturesLib|UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFea
> turesLibStm.inf
>    }
>    UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
> +  UefiCpuPkg/Library/BaseCpuTimerLib/BaseCpuTimerLib.inf
> 
>  [BuildOptions]
>    *_*_*_CC_FLAGS = -D DISABLE_NEW_DEPRECATED_INTERFACES
> --
> 2.14.2.windows.3