[edk2-devel] [PATCH v2 0/3] Get and use Get processor extended information

[edk2-devel] [PATCH v2 0/3] Get and use Get processor extended information

[Wu, Jiaxin]

The series patches are to complete commits 170d4ce8 & 1fadd18d,
then SMM cpu driver will use the processor extended information.

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: Jiaxin Wu <jiaxin.wu@intel.com>

Jiaxin Wu (3):
  UefiCpuPkg/BaseXApicLib: Fix CPUID_V2_EXTENDED_TOPOLOGY detection
  UefiCpuPkg/PiSmmCpuDxeSmm: Get processor extended information
  UefiCpuPkg/PiSmmCpuDxeSmm: Use processor extended information

 UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c | 21 ++++++++++++++++-----
 UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c         | 10 ++++++++++
 UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c          |  6 +++---
 3 files changed, 29 insertions(+), 8 deletions(-)

-- 
2.16.2.windows.1



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

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

[Wu, Jiaxin]

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 (#111257): https://edk2.groups.io/g/devel/message/111257
Mute This Topic: https://groups.io/mt/102602851/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Laszlo Ersek]

On 11/15/23 12:15, Jiaxin Wu wrote:
> 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) {

This synchronizes the GetProcessorLocationByApicId() and
GetProcessorLocation2ByApicId() functions in BaseXApicLib with those in
BaseXApicX2ApicLib.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111278): https://edk2.groups.io/g/devel/message/111278
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]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Ni, Ray]

[-- 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 --]

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

[Ni, Ray]

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

Thanks,
Ray
> -----Original Message-----
> From: Wu, Jiaxin <jiaxin.wu@intel.com>
> Sent: Wednesday, November 15, 2023 7:16 PM
> To: 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 (#111500): https://edk2.groups.io/g/devel/message/111500
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]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v2 2/3] UefiCpuPkg/PiSmmCpuDxeSmm: Get processor extended information

[Wu, Jiaxin]

This patch is to extend SmmAddProcessor function to get processor
extended information. It's to complete commit 1fadd18d.

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: Jiaxin Wu <jiaxin.wu@intel.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Regression-tested-by: Laszlo Ersek <lersek@redhat.com>
---
 UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c
index 391b64e9f2..c0485b0519 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c
@@ -169,10 +169,20 @@ SmmAddProcessor (
         &gSmmCpuPrivate->ProcessorInfo[Index].Location.Package,
         &gSmmCpuPrivate->ProcessorInfo[Index].Location.Core,
         &gSmmCpuPrivate->ProcessorInfo[Index].Location.Thread
         );
 
+      GetProcessorLocation2ByApicId (
+        (UINT32)ProcessorId,
+        &gSmmCpuPrivate->ProcessorInfo[Index].ExtendedInformation.Location2.Package,
+        &gSmmCpuPrivate->ProcessorInfo[Index].ExtendedInformation.Location2.Die,
+        &gSmmCpuPrivate->ProcessorInfo[Index].ExtendedInformation.Location2.Tile,
+        &gSmmCpuPrivate->ProcessorInfo[Index].ExtendedInformation.Location2.Module,
+        &gSmmCpuPrivate->ProcessorInfo[Index].ExtendedInformation.Location2.Core,
+        &gSmmCpuPrivate->ProcessorInfo[Index].ExtendedInformation.Location2.Thread
+        );
+
       *ProcessorNumber                 = Index;
       gSmmCpuPrivate->Operation[Index] = SmmCpuAdd;
       return EFI_SUCCESS;
     }
   }
-- 
2.16.2.windows.1



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

Re: [edk2-devel] [PATCH v2 2/3] UefiCpuPkg/PiSmmCpuDxeSmm: Get processor extended information

[Ni, Ray]

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

Thanks,
Ray
> -----Original Message-----
> From: Wu, Jiaxin <jiaxin.wu@intel.com>
> Sent: Wednesday, November 15, 2023 7:16 PM
> To: 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 2/3] UefiCpuPkg/PiSmmCpuDxeSmm: Get processor
> extended information
> 
> This patch is to extend SmmAddProcessor function to get processor
> extended information. It's to complete commit 1fadd18d.
> 
> 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: Jiaxin Wu <jiaxin.wu@intel.com>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> Regression-tested-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c
> index 391b64e9f2..c0485b0519 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c
> @@ -169,10 +169,20 @@ SmmAddProcessor (
>          &gSmmCpuPrivate->ProcessorInfo[Index].Location.Package,
>          &gSmmCpuPrivate->ProcessorInfo[Index].Location.Core,
>          &gSmmCpuPrivate->ProcessorInfo[Index].Location.Thread
>          );
> 
> +      GetProcessorLocation2ByApicId (
> +        (UINT32)ProcessorId,
> +
> &gSmmCpuPrivate->ProcessorInfo[Index].ExtendedInformation.Location2.Pac
> kage,
> +
> &gSmmCpuPrivate->ProcessorInfo[Index].ExtendedInformation.Location2.Die
> ,
> +
> &gSmmCpuPrivate->ProcessorInfo[Index].ExtendedInformation.Location2.Tile
> ,
> +
> &gSmmCpuPrivate->ProcessorInfo[Index].ExtendedInformation.Location2.Mo
> dule,
> +
> &gSmmCpuPrivate->ProcessorInfo[Index].ExtendedInformation.Location2.Cor
> e,
> +
> &gSmmCpuPrivate->ProcessorInfo[Index].ExtendedInformation.Location2.Thr
> ead
> +        );
> +
>        *ProcessorNumber                 = Index;
>        gSmmCpuPrivate->Operation[Index] = SmmCpuAdd;
>        return EFI_SUCCESS;
>      }
>    }
> --
> 2.16.2.windows.1



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

[edk2-devel] [PATCH v2 3/3] UefiCpuPkg/PiSmmCpuDxeSmm: Use processor extended information

[Wu, Jiaxin]

This patch changes to use the processor extended information,
which can reflect the value from CPUID leaf 1FH.

The reasons are listed as below:
1. The processor package ID retrieved from CPUID 0x0Bh may be
not correct/accurate if CPU has the module & die info, it depends
on the CPUID implementation. See SDM statement:

EAX Bits 04 - 00: Number of bits to shift right on x2APIC ID to
get a unique topology ID of the next level type.
ECX Bits 15 - 08: level type

Level type field has the following encoding:
0: Invalid.
1: SMT.
2: Core.
3-255: Reserved

So, if level type returned from ECX Bits 15 - 08 is 2 (Core),
then it's not clear about the next level. It can be Module or
Die or Package. SDM doesn't has explanation for the next level
of Core. If so, the value will be decided by implementation.
The value can be package info for compatibility consideration,
but it's not standardized.

2. According SDM declaration, "CPUID leaf 1FH is a preferred
superset to leaf 0BH. Intel recommends first checking for the
existence of CPUID leaf 1FH before using leaf 0BH." This is
perfect match the existing GetProcessorLocation2ByApicId()
implementation.

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: Jiaxin Wu <jiaxin.wu@intel.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Regression-tested-by: Laszlo Ersek <lersek@redhat.com>
---
 UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
index 25d058c5b9..c61562c867 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
@@ -177,11 +177,11 @@ IsPackageFirstThread (
   IN UINTN  CpuIndex
   )
 {
   UINT32  PackageIndex;
 
-  PackageIndex =  gSmmCpuPrivate->ProcessorInfo[CpuIndex].Location.Package;
+  PackageIndex =  gSmmCpuPrivate->ProcessorInfo[CpuIndex].ExtendedInformation.Location2.Package;
 
   ASSERT (mPackageFirstThreadIndex != NULL);
 
   //
   // Set the value of mPackageFirstThreadIndex[PackageIndex].
@@ -1834,12 +1834,12 @@ InitPackageFirstThreadIndexInfo (
 
   //
   // Count the number of package, set to max PackageId + 1
   //
   for (Index = 0; Index < mNumberOfCpus; Index++) {
-    if (PackageId < gSmmCpuPrivate->ProcessorInfo[Index].Location.Package) {
-      PackageId = gSmmCpuPrivate->ProcessorInfo[Index].Location.Package;
+    if (PackageId < gSmmCpuPrivate->ProcessorInfo[Index].ExtendedInformation.Location2.Package) {
+      PackageId = gSmmCpuPrivate->ProcessorInfo[Index].ExtendedInformation.Location2.Package;
     }
   }
 
   PackageCount = PackageId + 1;
 
-- 
2.16.2.windows.1



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

Re: [edk2-devel] [PATCH v2 3/3] UefiCpuPkg/PiSmmCpuDxeSmm: Use processor extended information

[Ni, Ray]

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

I cannot remember if CPUID.0B and CPUID.1F return the same value for package ID.

And I am not sure about the benefit if we get the package id from location2.

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 3/3] UefiCpuPkg/PiSmmCpuDxeSmm: Use processor extended information

This patch changes to use the processor extended information,
which can reflect the value from CPUID leaf 1FH.

The reasons are listed as below:
1. The processor package ID retrieved from CPUID 0x0Bh may be
not correct/accurate if CPU has the module & die info, it depends
on the CPUID implementation. See SDM statement:

EAX Bits 04 - 00: Number of bits to shift right on x2APIC ID to
get a unique topology ID of the next level type.
ECX Bits 15 - 08: level type

Level type field has the following encoding:
0: Invalid.
1: SMT.
2: Core.
3-255: Reserved

So, if level type returned from ECX Bits 15 - 08 is 2 (Core),
then it's not clear about the next level. It can be Module or
Die or Package. SDM doesn't has explanation for the next level
of Core. If so, the value will be decided by implementation.
The value can be package info for compatibility consideration,
but it's not standardized.

2. According SDM declaration, "CPUID leaf 1FH is a preferred
superset to leaf 0BH. Intel recommends first checking for the
existence of CPUID leaf 1FH before using leaf 0BH." This is
perfect match the existing GetProcessorLocation2ByApicId()
implementation.

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: Jiaxin Wu <jiaxin.wu@intel.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Regression-tested-by: Laszlo Ersek <lersek@redhat.com>
---
 UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
index 25d058c5b9..c61562c867 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
@@ -177,11 +177,11 @@ IsPackageFirstThread (
   IN UINTN  CpuIndex
   )
 {
   UINT32  PackageIndex;

-  PackageIndex =  gSmmCpuPrivate->ProcessorInfo[CpuIndex].Location.Package;
+  PackageIndex =  gSmmCpuPrivate->ProcessorInfo[CpuIndex].ExtendedInformation.Location2.Package;

   ASSERT (mPackageFirstThreadIndex != NULL);

   //
   // Set the value of mPackageFirstThreadIndex[PackageIndex].
@@ -1834,12 +1834,12 @@ InitPackageFirstThreadIndexInfo (

   //
   // Count the number of package, set to max PackageId + 1
   //
   for (Index = 0; Index < mNumberOfCpus; Index++) {
-    if (PackageId < gSmmCpuPrivate->ProcessorInfo[Index].Location.Package) {
-      PackageId = gSmmCpuPrivate->ProcessorInfo[Index].Location.Package;
+    if (PackageId < gSmmCpuPrivate->ProcessorInfo[Index].ExtendedInformation.Location2.Package) {
+      PackageId = gSmmCpuPrivate->ProcessorInfo[Index].ExtendedInformation.Location2.Package;
     }
   }

   PackageCount = PackageId + 1;

--
2.16.2.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111297): https://edk2.groups.io/g/devel/message/111297
Mute This Topic: https://groups.io/mt/102602853/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: 6790 bytes --]

Re: [edk2-devel] [PATCH v2 3/3] UefiCpuPkg/PiSmmCpuDxeSmm: Use processor extended information

[Laszlo Ersek]

On 11/16/23 02:30, Ni, Ray wrote:
> I cannot remember if CPUID.0B and CPUID.1F return the same value for
> package ID.
> 
> And I am not sure about the benefit if we get the package id from location2.

Isn't the benefit that Location2 / CPUID leaf 1F is fully specified,
while leaf 0B isn't? From the commit message it seems we should always
prefer leaf 1F and Location2, even if we're not aware of concrete
problems with leaf 0B.

Do you think we should only merge patches #1 and #2?

Thanks,
Laszlo



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

Re: [edk2-devel] [PATCH v2 3/3] UefiCpuPkg/PiSmmCpuDxeSmm: Use processor extended information

[Ni, Ray]

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

let me add more to explain:

1. CPUID.0B.PackageId == CPUID.1F.PackageId

SDM clearly states the scope of every MSR (public): package, core, or thread.
But SDM doesn't emphasize that if a MSR is package scope, it's within the package defined by CPUID.0B or CPUID.1F.
That implies, CPUID.0B and CPUID.1F should return the same value for package ID.

Also, SDM has following statement to explain result of EAX for CPUID.0B and CPUID.1F:
    Bits 04-00: The number of bits that the x2APIC ID must be shifted to the right to address instances of the "next higher-scoped"​ domain.

That means when CPUID.0B returns the EAX[04:00], it returns the total bits of "thread", "core", "module", "tie", "die" because "package" is
the next higher-scoped domain.

That also supports the idea: CPUID.0B.PackageId == CPUID.1F.PackageId.

2. CPU Feature Initialization

In UefiCpuPkg/Include/RegisterCpuFeaturesLib.h, the following macros were added to support consumers of RegisterCpuFeaturesLib specify
dependencies among different features.
For example, when feature #a PACKAGE_BEFORE feature #b, #b is performed in one thread of a package and after all threads have performed #a.
That means internally multi-thread-sync is used to guarantee the dependencies.
#define CPU_FEATURE_THREAD_BEFORE   BIT25
#define CPU_FEATURE_THREAD_AFTER    BIT26
#define CPU_FEATURE_CORE_BEFORE     BIT27
#define CPU_FEATURE_CORE_AFTER      BIT28
#define CPU_FEATURE_PACKAGE_BEFORE  BIT29
#define CPU_FEATURE_PACKAGE_AFTER   BIT30

But above 3 sets of macro only define the dependencies between 3 scopes: thread, core and package.
Other scopes were not supported as there is no MSR which belongs to other scopes at that moment, even today.
So, the cpu features library implementation also only depends on CPUID.0B.
If we update the code to get package id from CPUID.1F, to be consistent, we should also get the core id from CPUID.1F.
But if we do that, the number of cores which belong to the same domain could be less in CPUID.1F. As CPUID.1F returns
the number of cores per module, instead of per package.
That will break the MP sync logic which depends on the number of cores per each domain.

Conclusion: we should not update code to use CPUID.1F as it will break the MP-sync logic in RegisterCpuFeaturesLib which is not aware of more than 3 layers of scopes.

Thanks,
Ray

________________________________
From: Laszlo Ersek <lersek@redhat.com>
Sent: Saturday, November 18, 2023 5:05 AM
To: devel@edk2.groups.io <devel@edk2.groups.io>; Ni, Ray <ray.ni@intel.com>; Wu, Jiaxin <jiaxin.wu@intel.com>
Cc: Dong, Eric <eric.dong@intel.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; Zeng, Star <star.zeng@intel.com>
Subject: Re: [edk2-devel] [PATCH v2 3/3] UefiCpuPkg/PiSmmCpuDxeSmm: Use processor extended information

On 11/16/23 02:30, Ni, Ray wrote:
> I cannot remember if CPUID.0B and CPUID.1F return the same value for
> package ID.
>
> And I am not sure about the benefit if we get the package id from location2.

Isn't the benefit that Location2 / CPUID leaf 1F is fully specified,
while leaf 0B isn't? From the commit message it seems we should always
prefer leaf 1F and Location2, even if we're not aware of concrete
problems with leaf 0B.

Do you think we should only merge patches #1 and #2?

Thanks,
Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111432): https://edk2.groups.io/g/devel/message/111432
Mute This Topic: https://groups.io/mt/102602853/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: 16875 bytes --]

Re: [edk2-devel] [PATCH v2 3/3] UefiCpuPkg/PiSmmCpuDxeSmm: Use processor extended information

[Wu, Jiaxin]

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

For core id in cpu features library, I agree it should be not easy or simple change to 0x1f.

But in SMM CPU, there is no usage case depends on the number of cores retrieved from cupid 0x0b return value, only PackageId will be used. So, this patch doesn’t do bad things, should no regression issue. I agree with Ray’s explanation that  “CPUID.0B.PackageId == CPUID.1F.PackageId”, thus no need update for the PackageId update.

I checked the latest SDM:

“The sub-leaves of CPUID leaf 0BH describe an ordered hierarchy of logical processors starting from the smallest-scoped domain of a Logical Processor (sub-leaf index 0) to the Core domain (sub-leaf index 1) to the largest-scoped domain (the last valid sub-leaf index) *that is implicitly subordinate to the unenumerated highest-scoped domain of the processor package (socket)*”

Looks it already updated to indicate the largest-scoped domain is package.

With all above, I agree to drop this path, but other 2 patches in this set should be ok. Thanks Ray help clarify this.

Thanks,
Jiaxin

From: Ni, Ray <ray.ni@intel.com>
Sent: Monday, November 20, 2023 9:45 AM
To: Laszlo Ersek <lersek@redhat.com>; devel@edk2.groups.io; Wu, Jiaxin <jiaxin.wu@intel.com>
Cc: Dong, Eric <eric.dong@intel.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; Zeng, Star <star.zeng@intel.com>
Subject: Re: [edk2-devel] [PATCH v2 3/3] UefiCpuPkg/PiSmmCpuDxeSmm: Use processor extended information

let me add more to explain:

1. CPUID.0B.PackageId == CPUID.1F.PackageId

SDM clearly states the scope of every MSR (public): package, core, or thread.
But SDM doesn't emphasize that if a MSR is package scope, it's within the package defined by CPUID.0B or CPUID.1F.
That implies, CPUID.0B and CPUID.1F should return the same value for package ID.

Also, SDM has following statement to explain result of EAX for CPUID.0B and CPUID.1F:
    Bits 04-00: The number of bits that the x2APIC ID must be shifted to the right to address instances of the "next higher-scoped"​ domain.

That means when CPUID.0B returns the EAX[04:00], it returns the total bits of "thread", "core", "module", "tie", "die" because "package" is
the next higher-scoped domain.

That also supports the idea: CPUID.0B.PackageId == CPUID.1F.PackageId.

2. CPU Feature Initialization

In UefiCpuPkg/Include/RegisterCpuFeaturesLib.h, the following macros were added to support consumers of RegisterCpuFeaturesLib specify
dependencies among different features.
For example, when feature #a PACKAGE_BEFORE feature #b, #b is performed in one thread of a package and after all threads have performed #a.
That means internally multi-thread-sync is used to guarantee the dependencies.
#define CPU_FEATURE_THREAD_BEFORE   BIT25
#define CPU_FEATURE_THREAD_AFTER    BIT26
#define CPU_FEATURE_CORE_BEFORE     BIT27
#define CPU_FEATURE_CORE_AFTER      BIT28
#define CPU_FEATURE_PACKAGE_BEFORE  BIT29
#define CPU_FEATURE_PACKAGE_AFTER   BIT30

But above 3 sets of macro only define the dependencies between 3 scopes: thread, core and package.
Other scopes were not supported as there is no MSR which belongs to other scopes at that moment, even today.
So, the cpu features library implementation also only depends on CPUID.0B.
If we update the code to get package id from CPUID.1F, to be consistent, we should also get the core id from CPUID.1F.
But if we do that, the number of cores which belong to the same domain could be less in CPUID.1F. As CPUID.1F returns
the number of cores per module, instead of per package.
That will break the MP sync logic which depends on the number of cores per each domain.

Conclusion: we should not update code to use CPUID.1F as it will break the MP-sync logic in RegisterCpuFeaturesLib which is not aware of more than 3 layers of scopes.

Thanks,
Ray

________________________________
From: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
Sent: Saturday, November 18, 2023 5:05 AM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>>; Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Wu, Jiaxin <jiaxin.wu@intel.com<mailto:jiaxin.wu@intel.com>>
Cc: Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>; Kumar, Rahul R <rahul.r.kumar@intel.com<mailto:rahul.r.kumar@intel.com>>; Gerd Hoffmann <kraxel@redhat.com<mailto:kraxel@redhat.com>>; Zeng, Star <star.zeng@intel.com<mailto:star.zeng@intel.com>>
Subject: Re: [edk2-devel] [PATCH v2 3/3] UefiCpuPkg/PiSmmCpuDxeSmm: Use processor extended information

On 11/16/23 02:30, Ni, Ray wrote:
> I cannot remember if CPUID.0B and CPUID.1F return the same value for
> package ID.
>
> And I am not sure about the benefit if we get the package id from location2.

Isn't the benefit that Location2 / CPUID leaf 1F is fully specified,
while leaf 0B isn't? From the commit message it seems we should always
prefer leaf 1F and Location2, even if we're not aware of concrete
problems with leaf 0B.

Do you think we should only merge patches #1 and #2?

Thanks,
Laszlo


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



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

Re: [edk2-devel] [PATCH v2 3/3] UefiCpuPkg/PiSmmCpuDxeSmm: Use processor extended information

[Laszlo Ersek]

On 11/20/23 13:42, Wu, Jiaxin wrote:
> For core id in cpu features library, I agree it should be not easy or
> simple change to 0x1f.
> 
>  
> 
> But in SMM CPU, there is no usage case depends on the number of cores
> retrieved from cupid 0x0b return value, only PackageId will be used. So,
> this patch doesn’t do bad things, should no regression issue. I agree
> with Ray’s explanation that  “CPUID.0B.PackageId == CPUID.1F.PackageId”,
> thus no need update for the PackageId update.
> 
>  
> 
> I checked the latest SDM:
> 
>  
> 
> “The sub-leaves of CPUID leaf 0BH describe an ordered hierarchy of
> logical processors starting from the smallest-scoped domain of a Logical
> Processor (sub-leaf index 0) to the Core domain (sub-leaf index 1) to
> the largest-scoped domain (the last valid sub-leaf index) **that is
> implicitly subordinate to the unenumerated highest-scoped domain of the
> processor package (socket)**”
> 
>  
> 
> Looks it already updated to indicate the largest-scoped domain is package.
> 
>  
> 
> With all above, I agree to drop this path, but other 2 patches in this
> set should be ok. Thanks Ray help clarify this.

This is precisely the reason why I originally requested the now-last
patch to be split off from the rest. I certainly didn't / couldn't go
into such depths of CPUID.0B versus CPUID.1F discussion, but still that
change looked very distinct from *populating* Location2 in the
SMM-add-processor protocol member function (upon CPU hotplug). So, FWIW,
I'm fine if the last patch in the series gets dropped.

Thanks
Laszlo

> 
>  
> 
> Thanks,
> 
> Jiaxin
> 
>  
> 
> *From:* Ni, Ray <ray.ni@intel.com>
> *Sent:* Monday, November 20, 2023 9:45 AM
> *To:* Laszlo Ersek <lersek@redhat.com>; devel@edk2.groups.io; Wu, Jiaxin
> <jiaxin.wu@intel.com>
> *Cc:* Dong, Eric <eric.dong@intel.com>; Kumar, Rahul R
> <rahul.r.kumar@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; Zeng, Star
> <star.zeng@intel.com>
> *Subject:* Re: [edk2-devel] [PATCH v2 3/3] UefiCpuPkg/PiSmmCpuDxeSmm:
> Use processor extended information
> 
>  
> 
> let me add more to explain:
> 
>  
> 
> 1. CPUID.0B.PackageId == CPUID.1F.PackageId
> 
>  
> 
> SDM clearly states the scope of every MSR (public): package, core, or
> thread.
> 
> But SDM doesn't emphasize that if a MSR is package scope, it's within
> the package defined by CPUID.0B or CPUID.1F.
> 
> That implies, CPUID.0B and CPUID.1F should return the same value for
> package ID.
> 
>  
> 
> Also, SDM has following statement to explain result of EAX for CPUID.0B
> and CPUID.1F:
> 
>     Bits 04-00: The number of bits that the x2APIC ID must be shifted to
> the right to address instances of the "*next higher-scoped"*​ domain.
> 
>  
> 
> That means when CPUID.0B returns the EAX[04:00], it returns the total
> bits of "thread", "core", "module", "tie", "die" because "package" is
> the next higher-scoped domain.
> 
>  
> 
> That also supports the idea: CPUID.0B.PackageId == CPUID.1F.PackageId.
> 
>  
> 
> 2. CPU Feature Initialization
> 
>  
> 
> In UefiCpuPkg/Include/RegisterCpuFeaturesLib.h, the following macros
> were added to support consumers of RegisterCpuFeaturesLib specify
> dependencies among different features.
> 
> For example, when feature #a PACKAGE_BEFORE feature #b, #b is performed
> in one thread of a package and after all threads have performed #a.
> 
> That means internally multi-thread-sync is used to guarantee the
> dependencies.
> 
> #define CPU_FEATURE_THREAD_BEFORE   BIT25
> 
> #define CPU_FEATURE_THREAD_AFTER    BIT26
> 
> #define CPU_FEATURE_CORE_BEFORE     BIT27
> 
> #define CPU_FEATURE_CORE_AFTER      BIT28
> 
> #define CPU_FEATURE_PACKAGE_BEFORE  BIT29
> 
> #define CPU_FEATURE_PACKAGE_AFTER   BIT30
> 
>  
> 
> But above 3 sets of macro only define the dependencies between 3 scopes:
> thread, core and package.
> 
> Other scopes were not supported as there is no MSR which belongs to
> other scopes at that moment, even today.
> 
> So, the cpu features library implementation also only depends on CPUID.0B.
> 
> If we update the code to get package id from CPUID.1F, to be consistent,
> we should also get the core id from CPUID.1F.
> 
> But if we do that, the number of cores which belong to the same domain
> could be less in CPUID.1F. As CPUID.1F returns
> the number of cores per module, instead of per package.
> 
> That will break the MP sync logic which depends on the number of cores
> per each domain.
> 
>  
> 
> Conclusion: we should not update code to use CPUID.1F as it will break
> the MP-sync logic in RegisterCpuFeaturesLib which is not aware of more
> than 3 layers of scopes.
> 
>  
> 
> Thanks,
> 
> Ray
> 
>  
> 
> ------------------------------------------------------------------------
> 
> *From:* Laszlo Ersek <lersek@redhat.com <mailto:lersek@redhat.com>>
> *Sent:* Saturday, November 18, 2023 5:05 AM
> *To:* devel@edk2.groups.io
> <mailto:devel@edk2.groups.io><devel@edk2.groups.io
> <mailto:devel@edk2.groups.io>>; Ni, Ray <ray.ni@intel.com
> <mailto:ray.ni@intel.com>>; Wu, Jiaxin <jiaxin.wu@intel.com
> <mailto:jiaxin.wu@intel.com>>
> *Cc:* Dong, Eric <eric.dong@intel.com <mailto:eric.dong@intel.com>>;
> Kumar, Rahul R <rahul.r.kumar@intel.com
> <mailto:rahul.r.kumar@intel.com>>; Gerd Hoffmann <kraxel@redhat.com
> <mailto:kraxel@redhat.com>>; Zeng, Star <star.zeng@intel.com
> <mailto:star.zeng@intel.com>>
> *Subject:* Re: [edk2-devel] [PATCH v2 3/3] UefiCpuPkg/PiSmmCpuDxeSmm:
> Use processor extended information
> 
>  
> 
> On 11/16/23 02:30, Ni, Ray wrote:
>> I cannot remember if CPUID.0B and CPUID.1F return the same value for
>> package ID.
>>
>> And I am not sure about the benefit if we get the package id from
> location2.
> 
> Isn't the benefit that Location2 / CPUID leaf 1F is fully specified,
> while leaf 0B isn't? From the commit message it seems we should always
> prefer leaf 1F and Location2, even if we're not aware of concrete
> problems with leaf 0B.
> 
> Do you think we should only merge patches #1 and #2?
> 
> Thanks,
> Laszlo
> 



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

Re: [edk2-devel] [PATCH v2 3/3] UefiCpuPkg/PiSmmCpuDxeSmm: Use processor extended information

[Laszlo Ersek]

On 11/20/23 13:42, Wu, Jiaxin wrote:
> For core id in cpu features library, I agree it should be not easy or
> simple change to 0x1f.
> 
>  
> 
> But in SMM CPU, there is no usage case depends on the number of cores
> retrieved from cupid 0x0b return value, only PackageId will be used. So,
> this patch doesn’t do bad things, should no regression issue. I agree
> with Ray’s explanation that  “CPUID.0B.PackageId == CPUID.1F.PackageId”,
> thus no need update for the PackageId update.
> 
>  
> 
> I checked the latest SDM:
> 
>  
> 
> “The sub-leaves of CPUID leaf 0BH describe an ordered hierarchy of
> logical processors starting from the smallest-scoped domain of a Logical
> Processor (sub-leaf index 0) to the Core domain (sub-leaf index 1) to
> the largest-scoped domain (the last valid sub-leaf index) **that is
> implicitly subordinate to the unenumerated highest-scoped domain of the
> processor package (socket)**”
> 
>  
> 
> Looks it already updated to indicate the largest-scoped domain is package.
> 
>  
> 
> With all above, I agree to drop this path, but other 2 patches in this
> set should be ok. Thanks Ray help clarify this.

Merged the first two patches in the series as commits
ad0b1cc144b56fcbd8d369eaff6eaf5f3020efe7 and
7eb504060787c9c37d5b3c33f5d65021d553ea3f, via
<https://github.com/tianocore/edk2/pull/5125>.

Laszlo



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