[edk2-devel] [PATCH 1/1] UefiCpuPkg/BaseXApicX2ApicLib: fix CPUID_V2_EXTENDED_TOPOLOGY detection

[edk2-devel] [PATCH 1/1] UefiCpuPkg/BaseXApicX2ApicLib: fix CPUID_V2_EXTENDED_TOPOLOGY detection

[Gerd Hoffmann]

Checking the max cpuid leaf is not enough to figure whenever
CPUID_V2_EXTENDED_TOPOLOGY is supported.  Quoting a comment for
CPUID_EXTENDED_TOPOLOGY in GetProcessorLocationByApicId():

// 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.

Add a similar check for CPUID_V2_EXTENDED_TOPOLOGY to
GetProcessorLocation2ByApicId().  Without this fix OVMF triggers
an ASSERT when running in a kvm guest on latest (12th gen) intel
processors.

Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2241388
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c b/UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c
index aa4eb11181f6..eff7f1f2043b 100644
--- a/UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c
+++ b/UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c
@@ -1424,6 +1424,7 @@ GetProcessorLocation2ByApicId (
   )
 {
   CPUID_EXTENDED_TOPOLOGY_EAX  ExtendedTopologyEax;
+  CPUID_EXTENDED_TOPOLOGY_EBX  ExtendedTopologyEbx;
   CPUID_EXTENDED_TOPOLOGY_ECX  ExtendedTopologyEcx;
   UINT32                       MaxStandardCpuIdIndex;
   UINT32                       Index;
@@ -1440,6 +1441,12 @@ GetProcessorLocation2ByApicId (
   //
   AsmCpuid (CPUID_SIGNATURE, &MaxStandardCpuIdIndex, NULL, NULL, NULL);
   if (MaxStandardCpuIdIndex < CPUID_V2_EXTENDED_TOPOLOGY) {
+    ExtendedTopologyEbx.Uint32 = 0;
+  } else {
+    AsmCpuidEx (CPUID_V2_EXTENDED_TOPOLOGY, 0, NULL, &ExtendedTopologyEbx.Uint32, NULL, NULL);
+  }
+
+  if (ExtendedTopologyEbx.Uint32 == 0) {
     if (Die != NULL) {
       *Die = 0;
     }
-- 
2.41.0



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

Re: [edk2-devel] [PATCH 1/1] UefiCpuPkg/BaseXApicX2ApicLib: fix CPUID_V2_EXTENDED_TOPOLOGY detection

[Laszlo Ersek]

On 10/11/23 12:06, Gerd Hoffmann wrote:
> Checking the max cpuid leaf is not enough to figure whenever
> CPUID_V2_EXTENDED_TOPOLOGY is supported.  Quoting a comment for
> CPUID_EXTENDED_TOPOLOGY in GetProcessorLocationByApicId():
> 
> // 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.
> 
> Add a similar check for CPUID_V2_EXTENDED_TOPOLOGY to
> GetProcessorLocation2ByApicId().  Without this fix OVMF triggers
> an ASSERT when running in a kvm guest on latest (12th gen) intel
> processors.
> 
> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2241388
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c b/UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c
> index aa4eb11181f6..eff7f1f2043b 100644
> --- a/UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c
> +++ b/UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c
> @@ -1424,6 +1424,7 @@ GetProcessorLocation2ByApicId (
>    )
>  {
>    CPUID_EXTENDED_TOPOLOGY_EAX  ExtendedTopologyEax;
> +  CPUID_EXTENDED_TOPOLOGY_EBX  ExtendedTopologyEbx;
>    CPUID_EXTENDED_TOPOLOGY_ECX  ExtendedTopologyEcx;
>    UINT32                       MaxStandardCpuIdIndex;
>    UINT32                       Index;
> @@ -1440,6 +1441,12 @@ GetProcessorLocation2ByApicId (
>    //
>    AsmCpuid (CPUID_SIGNATURE, &MaxStandardCpuIdIndex, NULL, NULL, NULL);
>    if (MaxStandardCpuIdIndex < CPUID_V2_EXTENDED_TOPOLOGY) {
> +    ExtendedTopologyEbx.Uint32 = 0;
> +  } else {
> +    AsmCpuidEx (CPUID_V2_EXTENDED_TOPOLOGY, 0, NULL, &ExtendedTopologyEbx.Uint32, NULL, NULL);
> +  }
> +
> +  if (ExtendedTopologyEbx.Uint32 == 0) {
>      if (Die != NULL) {
>        *Die = 0;
>      }

This is a terrible *organization* bug in the Intel SDM.

I've downloaded the latest version (Order number 325462-081US, September
2023). Under the CPUID instruction, we have Table 3-8 (starts on page
814), which specifies the various leaves *in detail*.

Or, well, so you would think.

If you open page 833, still belonging to the same table, you find "V2
Extended Topology Enumeration Leaf (Initial EAX Value = 1FH)". It
specifies the CPUID functionality at hand, and it is *entirely silent*
on EBX bits 15-00 *possibly being zero*. This is what the table says
about EBX:

        Bits 15-00: The number of logical processors across all
        instances of this domain within the next higher-scoped domain
        relative to this current logical processor. (For example, in a
        processor socket/package comprising “M” dies of “N” cores each,
        where each core has “L” logical processors, the “die” domain
        subleaf value of this field would be M*N*L. In an asymmetric
        topology this would be the summation of the value across the
        lower domain level instances to create each upper domain level
        instance.) This number reflects configuration as shipped by
        Intel. Note, software must not use this field to enumerate
        processor topology*. Bits 31-16: Reserved.

We have an asterisk (footnote) there, which is expanded on page 834:

        * Software must not use the value of EBX[15:0] to enumerate
        processor topology of the system. The value is only intended for
        display and diagnostic purposes. The actual number of logical
        processors available to BIOS/OS/Applications may be different
        from the value of EBX[15:0], depending on software and platform
        hardware configurations.

Aha. "Display and diagnostic purposes."

Now scroll to the very end of Table 3-8 -- page 836. Look, there are a
bunch of *further* notes there -- without any references to them in the
Table of Contents!

So, keep scrolling... a whopping 14 pages later, on page 850, we find:

        INPUT EAX = 1FH: Returns V2 Extended Topology Information

        When CPUID executes with EAX set to 1FH, the processor returns
        information about extended topology enumeration data. 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. See Table 3-8.

Oh, so there's a condition (b) too! How very nice of the authors not to
mention that in the *actual specification* of EAX = 1FH, you know,
*inside* Table 3-8!

Remember: "The value [of EBX[15:0]] is only intended for display and
diagnostic purposes."

Incredible.

The patch is mostly good, but the commit message can be improved. We
shouldn't base the reasoning on the existent EBX check in
GetProcessorLocationByApicId(). Instead, we should quote this specific
passage from the SDM.

Furthermore, it's not the "ExtendedTopologyEbx.Uint32" union field that
we should compare against zero; we should compare
"ExtendedTopologyEbx.Bits.LogicalProcessors" -- that's what the spec says.

(I understand that the other word is reserved, so the effect is
currently the same, but let us stick with the wording of the spec.)

... I'm still amazed. (Not in a good way.)

Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109608): https://edk2.groups.io/g/devel/message/109608
Mute This Topic: https://groups.io/mt/101893551/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 1/1] UefiCpuPkg/BaseXApicX2ApicLib: fix CPUID_V2_EXTENDED_TOPOLOGY detection

[Gerd Hoffmann]

  Hi,

> This is a terrible *organization* bug in the Intel SDM.

[ ... ]

> So, keep scrolling... a whopping 14 pages later, on page 850, we find:
> 
>         INPUT EAX = 1FH: Returns V2 Extended Topology Information
> 
>         When CPUID executes with EAX set to 1FH, the processor returns
>         information about extended topology enumeration data. 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. See Table 3-8.

Wow.  Thanks for finding that well hidden reference.

An almost identical worded paragraph exists for "INPUT EAX = 0BH:
Returns Extended Topology Information".

> The patch is mostly good, but the commit message can be improved. We
> shouldn't base the reasoning on the existent EBX check in
> GetProcessorLocationByApicId(). Instead, we should quote this specific
> passage from the SDM.

Sure.

I didn't notice all the notes at the bottom of the table, so that
comment plus the fact that the 0x0b and 0x1f leafs are modeled in
a very similar way was the best reference I had at hand.

> Furthermore, it's not the "ExtendedTopologyEbx.Uint32" union field that
> we should compare against zero; we should compare
> "ExtendedTopologyEbx.Bits.LogicalProcessors" -- that's what the spec says.

OK.

take care,
  Gerd



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