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

["Laszlo Ersek" <lersek@redhat.com>]

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