Re: [edk2-devel] [PATCH v2 1/3] UefiCpuPkg/BaseXApicLib: Fix CPUID_V2_EXTENDED_TOPOLOGY detection

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

[-- Attachment #1: Type: text/plain, Size: 4631 bytes --]

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

Thanks,
Ray
________________________________
From: Wu, Jiaxin <jiaxin.wu@intel.com>
Sent: Wednesday, November 15, 2023 7:15 PM
To: devel@edk2.groups.io <devel@edk2.groups.io>
Cc: Laszlo Ersek <lersek@redhat.com>; Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; Zeng, Star <star.zeng@intel.com>
Subject: [PATCH v2 1/3] UefiCpuPkg/BaseXApicLib: Fix CPUID_V2_EXTENDED_TOPOLOGY detection

This patch is to complete 170d4ce8, sync the change to BaseXApicLib.

Checking the max cpuid leaf is not enough to figure whenever
CPUID_V2_EXTENDED_TOPOLOGY is supported.  Intel SDM says:

Software must detect the presence of CPUID leaf 1FH by verifying
(a) the highest leaf index supported by CPUID is >= 1FH, and
(b) CPUID.1FH:EBX[15:0] reports a non-zero value.

The same is true for CPUID leaf 0BH.

This patch adds the EBX check to GetProcessorLocation2ByApicId().  The
patch also fixes the existing check in GetProcessorLocationByApicId() to
be in line with the spec by looking at bits 15:0.  The comments are
updated with a quote from the Intel SDM.

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Star Zeng <star.zeng@intel.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Jiaxin Wu <jiaxin.wu@intel.com>
---
 UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c b/UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c
index d56c6275cc..efb9d71ca1 100644
--- a/UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c
+++ b/UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c
@@ -1053,15 +1053,16 @@ GetProcessorLocationByApicId (
       &ExtendedTopologyEbx.Uint32,
       &ExtendedTopologyEcx.Uint32,
       NULL
       );
     //
-    // If CPUID.(EAX=0BH, ECX=0H):EBX returns zero and maximum input value for
-    // basic CPUID information is greater than 0BH, then CPUID.0BH leaf is not
-    // supported on that processor.
+    // Quoting Intel SDM:
+    // Software must detect the presence of CPUID leaf 0BH by
+    // verifying (a) the highest leaf index supported by CPUID is >=
+    // 0BH, and (b) CPUID.0BH:EBX[15:0] reports a non-zero value.
     //
-    if (ExtendedTopologyEbx.Uint32 != 0) {
+    if (ExtendedTopologyEbx.Bits.LogicalProcessors != 0) {
       TopologyLeafSupported = TRUE;

       //
       // Sub-leaf index 0 (ECX= 0 as input) provides enumeration parameters to extract
       // the SMT sub-field of x2APIC ID.
@@ -1183,10 +1184,11 @@ GetProcessorLocation2ByApicId (
   OUT UINT32  *Core     OPTIONAL,
   OUT UINT32  *Thread   OPTIONAL
   )
 {
   CPUID_EXTENDED_TOPOLOGY_EAX  ExtendedTopologyEax;
+  CPUID_EXTENDED_TOPOLOGY_EBX  ExtendedTopologyEbx;
   CPUID_EXTENDED_TOPOLOGY_ECX  ExtendedTopologyEcx;
   UINT32                       MaxStandardCpuIdIndex;
   UINT32                       Index;
   UINTN                        LevelType;
   UINT32                       Bits[CPUID_V2_EXTENDED_TOPOLOGY_LEVEL_TYPE_DIE + 2];
@@ -1195,14 +1197,23 @@ GetProcessorLocation2ByApicId (
   for (LevelType = 0; LevelType < ARRAY_SIZE (Bits); LevelType++) {
     Bits[LevelType] = 0;
   }

   //
-  // Get max index of CPUID
+  // Quoting Intel SDM:
+  // Software must detect the presence of CPUID leaf 1FH by verifying
+  // (a) the highest leaf index supported by CPUID is >= 1FH, and (b)
+  // CPUID.1FH:EBX[15:0] reports a non-zero value.
   //
   AsmCpuid (CPUID_SIGNATURE, &MaxStandardCpuIdIndex, NULL, NULL, NULL);
   if (MaxStandardCpuIdIndex < CPUID_V2_EXTENDED_TOPOLOGY) {
+    ExtendedTopologyEbx.Bits.LogicalProcessors = 0;
+  } else {
+    AsmCpuidEx (CPUID_V2_EXTENDED_TOPOLOGY, 0, NULL, &ExtendedTopologyEbx.Uint32, NULL, NULL);
+  }
+
+  if (ExtendedTopologyEbx.Bits.LogicalProcessors == 0) {
     if (Die != NULL) {
       *Die = 0;
     }

     if (Tile != NULL) {
--
2.16.2.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111296): https://edk2.groups.io/g/devel/message/111296
Mute This Topic: https://groups.io/mt/102602851/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: Type: text/html, Size: 8266 bytes --]