Re: [PATCH] UefiCpuPkg/Cpuid: Dump leaf 1FH information correctly

["Dong, Eric" <eric.dong@intel.com>]

Hi Ray,

> -----Original Message-----
> From: Ni, Ray
> Sent: Friday, March 29, 2019 4:42 PM
> To: edk2-devel@lists.01.org
> Cc: Dong, Eric <eric.dong@intel.com>
> Subject: [PATCH] UefiCpuPkg/Cpuid: Dump leaf 1FH information correctly
> 
> Leaf 1FH is very similar to leaf 0BH. Both return the CPU topology information.
> Leaf 0BH returns 3-level (Package/Core/Thread) CPU topology info.
> Leaf 1FH returns 6-level (Package/Die/Tile/Module/Core/Thread) CPU
> topology info.
> The logic to enumerate the topology info is the same.
> 
> But today's logic to handle 1FH is completely wrong.
> The patch combines them together to fix the 1FH issue.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ray Ni <ray.ni@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> ---
>  UefiCpuPkg/Application/Cpuid/Cpuid.c | 83 ++++++++++------------------
>  1 file changed, 28 insertions(+), 55 deletions(-)
> 
> diff --git a/UefiCpuPkg/Application/Cpuid/Cpuid.c
> b/UefiCpuPkg/Application/Cpuid/Cpuid.c
> index 67cacf2714..3d242a0cbf 100644
> --- a/UefiCpuPkg/Application/Cpuid/Cpuid.c
> +++ b/UefiCpuPkg/Application/Cpuid/Cpuid.c
> @@ -1,7 +1,7 @@
>  /** @file
>    UEFI Application to display CPUID leaf information.
> 
> -  Copyright (c) 2016 - 2018, Intel Corporation. All rights reserved.<BR>
> +  Copyright (c) 2016 - 2019, Intel Corporation. All rights
> + reserved.<BR>
>    This program and the accompanying materials
>    are licensed and made available under the terms and conditions of the BSD
> License
>    which accompanies this distribution.  The full text of the license may be
> found at @@ -717,36 +717,42 @@
> CpuidArchitecturalPerformanceMonitoring (  **/  VOID
> CpuidExtendedTopology (
> -  VOID
> +  UINT32                       LeafFunction
>    )
>  {
> -  CPUID_EXTENDED_TOPOLOGY_EAX  Eax;
> -  CPUID_EXTENDED_TOPOLOGY_EBX  Ebx;
> -  CPUID_EXTENDED_TOPOLOGY_ECX  Ecx;
> -  UINT32                       Edx;
> -  UINT32                       LevelNumber;
> +  CPUID_V2_EXTENDED_TOPOLOGY_ENUMERATION_EAX  Eax;
> + CPUID_V2_EXTENDED_TOPOLOGY_ENUMERATION_EBX  Ebx;
> + CPUID_V2_EXTENDED_TOPOLOGY_ENUMERATION_ECX  Ecx;
> +  UINT32                                      Edx;
> +  UINT32                                      LevelNumber;
> 
> -  if (CPUID_EXTENDED_TOPOLOGY > gMaximumBasicFunction) {
> +  if (LeafFunction > gMaximumBasicFunction) {
> +    return;
> +  }
> +  if ((LeafFunction != CPUID_EXTENDED_TOPOLOGY) && (LeafFunction !=
> + CPUID_V2_EXTENDED_TOPOLOGY_ENUMERATION)) {
>      return;
>    }
> 
>    LevelNumber = 0;
> -  do {
> +  for (LevelNumber = 0; ; LevelNumber++) {
>      AsmCpuidEx (
> -      CPUID_EXTENDED_TOPOLOGY, LevelNumber,
> +      LeafFunction, LevelNumber,
>        &Eax.Uint32, &Ebx.Uint32, &Ecx.Uint32, &Edx
>        );
> -    if (Eax.Bits.ApicIdShift != 0) {
> -      Print (L"CPUID_EXTENDED_TOPOLOGY (Leaf %08x, Sub-Leaf %08x)\n",
> CPUID_EXTENDED_TOPOLOGY, LevelNumber);
> -      Print (L"  EAX:%08x  EBX:%08x  ECX:%08x  EDX:%08x\n", Eax.Uint32,
> Ebx.Uint32, Ecx.Uint32, Edx);
> -      PRINT_BIT_FIELD (Eax, ApicIdShift);
> -      PRINT_BIT_FIELD (Ebx, LogicalProcessors);
> -      PRINT_BIT_FIELD (Ecx, LevelNumber);
> -      PRINT_BIT_FIELD (Ecx, LevelType);
> -      PRINT_VALUE     (Edx, x2APIC_ID);
> +    if (Ecx.Bits.LevelType ==
> CPUID_V2_EXTENDED_TOPOLOGY_ENUMERATION_LEVEL_TYPE_INVALID) {
> +      break;
>      }
> -    LevelNumber++;
> -  } while (Eax.Bits.ApicIdShift != 0);
> +    Print (
> +      L"%a (Leaf %08x, Sub-Leaf %08x)\n",
> +      LeafFunction == CPUID_EXTENDED_TOPOLOGY ?
> "CPUID_EXTENDED_TOPOLOGY" :
> "CPUID_V2_EXTENDED_TOPOLOGY_ENUMERATION-2",
> +      LeafFunction, LevelNumber);
> +    Print (L"  EAX:%08x  EBX:%08x  ECX:%08x  EDX:%08x\n", Eax.Uint32,
> Ebx.Uint32, Ecx.Uint32, Edx);
> +    PRINT_BIT_FIELD (Eax, BitsNum);
> +    PRINT_BIT_FIELD (Ebx, ProcessorsNum);
> +    PRINT_BIT_FIELD (Ecx, LevelNum);
> +    PRINT_BIT_FIELD (Ecx, LevelType);
> +    PRINT_VALUE     (Edx, x2APIC_ID);

The name for above fields are different in CPUID_EXTENDED_TOPOLOGY and CPUID_V2_EXTENDED_TOPOLOGY_ENUMERATION,
So I think above print are not correct for CPUID_EXTENDED_TOPOLOGY, right?

Thanks,
Eric

> +  }
>  }
> 
>  /**
> @@ -1385,39 +1391,6 @@ CpuidDeterministicAddressTranslationParameters
> (
>    PRINT_BIT_FIELD (Edx, MaximumNum);
>  }
> 
> -/**
> -  Display CPUID_V2_EXTENDED_TOPOLOGY_ENUMERATION main leaf and
> sub-leafs.
> -
> -**/
> -VOID
> -CpuidV2ExtendedTopologyEnumeration (
> -  VOID
> -  )
> -{
> -  CPUID_V2_EXTENDED_TOPOLOGY_ENUMERATION_EAX  Eax;
> -  CPUID_V2_EXTENDED_TOPOLOGY_ENUMERATION_EBX  Ebx;
> -  CPUID_V2_EXTENDED_TOPOLOGY_ENUMERATION_ECX  Ecx;
> -  UINT32                                      Edx;
> -
> -  if (CPUID_V2_EXTENDED_TOPOLOGY_ENUMERATION >
> gMaximumBasicFunction) {
> -    return;
> -  }
> -
> -  AsmCpuidEx (
> -    CPUID_V2_EXTENDED_TOPOLOGY_ENUMERATION,
> -    CPUID_V2_EXTENDED_TOPOLOGY_ENUMERATION_MAIN_LEAF,
> -    &Eax.Uint32, &Ebx.Uint32, &Ecx.Uint32, &Edx
> -    );
> -  Print (L"CPUID_V2_EXTENDED_TOPOLOGY_ENUMERATION (Leaf %08x,
> Sub-Leaf %08x)\n", CPUID_V2_EXTENDED_TOPOLOGY_ENUMERATION,
> CPUID_V2_EXTENDED_TOPOLOGY_ENUMERATION_MAIN_LEAF);
> -  Print (L"  EAX:%08x  EBX:%08x  ECX:%08x  EDX:%08x\n", Eax.Uint32,
> Ebx.Uint32, Ecx.Uint32, Edx);
> -
> -  PRINT_BIT_FIELD (Eax, BitsNum);
> -  PRINT_BIT_FIELD (Ebx, ProcessorsNum);
> -  PRINT_BIT_FIELD (Ecx, LevelNum);
> -  PRINT_BIT_FIELD (Ecx, LevelType);
> -  PRINT_VALUE     (Edx, x2APICID);
> -}
> -
>  /**
>    Display CPUID_EXTENDED_FUNCTION leaf.
> 
> @@ -1619,7 +1592,7 @@ UefiMain (
>    CpuidStructuredExtendedFeatureFlags ();
>    CpuidDirectCacheAccessInfo();
>    CpuidArchitecturalPerformanceMonitoring ();
> -  CpuidExtendedTopology ();
> +  CpuidExtendedTopology (CPUID_EXTENDED_TOPOLOGY);
>    CpuidExtendedStateMainLeaf ();
>    CpuidIntelRdtMonitoringEnumerationSubLeaf ();
>    CpuidIntelRdtMonitoringL3CacheCapabilitySubLeaf (); @@ -1630,7 +1603,7
> @@ UefiMain (
>    CpuidProcessorFrequency ();
>    CpuidSocVendor ();
>    CpuidDeterministicAddressTranslationParameters ();
> -  CpuidV2ExtendedTopologyEnumeration ();
> +  CpuidExtendedTopology
> (CPUID_V2_EXTENDED_TOPOLOGY_ENUMERATION);
>    CpuidExtendedFunction ();
>    CpuidExtendedCpuSig ();
>    CpuidProcessorBrandString ();
> --
> 2.21.0.windows.1