* Re: [edk2-devel] [PATCH 1/1] UefiCpuPkg/BaseXApicX2ApicLib: fix CPUID_V2_EXTENDED_TOPOLOGY detection
2023-10-11 10:06 [edk2-devel] [PATCH 1/1] UefiCpuPkg/BaseXApicX2ApicLib: fix CPUID_V2_EXTENDED_TOPOLOGY detection Gerd Hoffmann
@ 2023-10-13 20:12 ` Laszlo Ersek
2023-10-17 9:12 ` Gerd Hoffmann
0 siblings, 1 reply; 3+ messages in thread
From: Laszlo Ersek @ 2023-10-13 20:12 UTC (permalink / raw)
To: devel, kraxel; +Cc: Rahul Kumar, Ray Ni, Eric Dong, Oliver Steffen
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]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 3+ messages in thread