From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id 7A58B941DEB for ; Fri, 13 Oct 2023 20:13:02 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=j3ZDLUaY2moHFkpBV37QDNStN4SFtVKYfBVTOZiA50E=; c=relaxed/simple; d=groups.io; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From:In-Reply-To:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Language:Content-Type:Content-Transfer-Encoding; s=20140610; t=1697227981; v=1; b=K0ejWBK6SIY5MBsRDeIpF4XXri54d50hXH0uZuU+2ukqrAD+YcMhN+/BeJcfqgsGSSPTAMrL AvRkeWgyHmvpECSY9n900olArg9XdBvmeHgU1dCekeJQY3MafBen/jCHjfJYFCn+Row8IfzP7Mz wQutKP5rjnxVFdQt8Kci+EMo= X-Received: by 127.0.0.2 with SMTP id 2bDYYY7687511xNapkTkDG2J; Fri, 13 Oct 2023 13:13:01 -0700 X-Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by mx.groups.io with SMTP id smtpd.web11.48852.1697227980354375844 for ; Fri, 13 Oct 2023 13:13:00 -0700 X-Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-499-Xa-28VODPACU1dn1V5MkUg-1; Fri, 13 Oct 2023 16:12:58 -0400 X-MC-Unique: Xa-28VODPACU1dn1V5MkUg-1 X-Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.rdu2.redhat.com [10.11.54.3]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id AD46C81D9ED; Fri, 13 Oct 2023 20:12:57 +0000 (UTC) X-Received: from [10.39.192.13] (unknown [10.39.192.13]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 7332C1102E14; Fri, 13 Oct 2023 20:12:56 +0000 (UTC) Message-ID: <96d0233f-035d-628a-f2d4-c8456937650a@redhat.com> Date: Fri, 13 Oct 2023 22:12:54 +0200 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH 1/1] UefiCpuPkg/BaseXApicX2ApicLib: fix CPUID_V2_EXTENDED_TOPOLOGY detection To: devel@edk2.groups.io, kraxel@redhat.com Cc: Rahul Kumar , Ray Ni , Eric Dong , Oliver Steffen References: <20231011100654.3313309-1-kraxel@redhat.com> From: "Laszlo Ersek" In-Reply-To: <20231011100654.3313309-1-kraxel@redhat.com> X-Scanned-By: MIMEDefang 3.1 on 10.11.54.3 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,lersek@redhat.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: FxZfmoEx5KW8bVFd9an2lJwax7686176AA= Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=K0ejWBK6; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=redhat.com (policy=none); spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io 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 > --- > 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] -=-=-=-=-=-=-=-=-=-=-=-