From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com [207.211.31.81]) by mx.groups.io with SMTP id smtpd.web11.4846.1594359493531213349 for ; Thu, 09 Jul 2020 22:38:14 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=QJWlzqKt; spf=pass (domain: redhat.com, ip: 207.211.31.81, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1594359492; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=fQnJyCC6T8m6PwxuWXl2TlQEEgPY6DUTQdOmaNmy4FY=; b=QJWlzqKtS3VFRIdSLw7MPDGm0AZpUgNyQe0U6U7DDeNlXokj1jcbXZ2upxq6S3o/qxSLG3 YxDmJkvWoAUuhvTDXjVBfq6oMupbolFDl2iqOPi6NyM7n4O/Q+I4HEB8NpI30e6CGZH62S zF8xP8qmIZsuuTwHeVo99I2XRZ8skt0= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-23-7XQjyPqTNfi7L9T1Q3kakg-1; Fri, 10 Jul 2020 01:38:06 -0400 X-MC-Unique: 7XQjyPqTNfi7L9T1Q3kakg-1 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 07C46100AA21; Fri, 10 Jul 2020 05:38:05 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-97.ams2.redhat.com [10.36.112.97]) by smtp.corp.redhat.com (Postfix) with ESMTP id ABE023A0; Fri, 10 Jul 2020 05:38:03 +0000 (UTC) Subject: Re: [edk2: PATCH] UefiCpuPkg: To enable X2Apic by default To: Ji-yunX Lu , devel@edk2.groups.io Cc: Eric Dong , Ray Ni , Rahul Kumar References: <20200710033123.2236-1-ji-yunx.lu@intel.com> From: "Laszlo Ersek" Message-ID: <822cc92e-74f8-14b3-928f-49c0fd670ffc@redhat.com> Date: Fri, 10 Jul 2020 07:38:02 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Firefox/52.0 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20200710033123.2236-1-ji-yunx.lu@intel.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit On 07/10/20 05:31, Ji-yunX Lu wrote: > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2845 > Platform shall enable X2APIC by default to meet requirements for interrupt steering policy on Windows OS. > > Change-Id: Ia9e24bce79c91762c560fa3de6260716939f0b1b > Signed-off-by: Ji-yunX Lu > Cc: Eric Dong > Cc: Ray Ni > Cc: Laszlo Ersek > Cc: Rahul Kumar > --- > UefiCpuPkg/Library/MpInitLib/MpLib.c | 21 ++++----------------- > 1 file changed, 4 insertions(+), 17 deletions(-) > > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c > index ab7a8ed663..70bc5da195 100644 > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c > @@ -488,8 +488,8 @@ CollectProcessorCount ( > ) > { > UINTN Index; > - CPU_INFO_IN_HOB *CpuInfoInHob; > BOOLEAN X2Apic; > + CPUID_VERSION_INFO_ECX VersionInfoEcx; > > // > // Send 1st broadcast IPI to APs to wakeup APs > @@ -505,26 +505,13 @@ CollectProcessorCount ( > CpuPause (); > } > > - > - // > - // Enable x2APIC mode if > - // 1. Number of CPU is greater than 255; or > - // 2. There are any logical processors reporting an Initial APIC ID of 255 or greater. > - // > X2Apic = FALSE; > - if (CpuMpData->CpuCount > 255) { > + AsmCpuid (CPUID_VERSION_INFO, NULL, NULL, &VersionInfoEcx.Uint32, NULL); > + if (VersionInfoEcx.Bits.x2APIC == 1) { > // > - // If there are more than 255 processor found, force to enable X2APIC > + // Enable x2APIC mode if capable > // > X2Apic = TRUE; > - } else { > - CpuInfoInHob = (CPU_INFO_IN_HOB *) (UINTN) CpuMpData->CpuInfoInHob; > - for (Index = 0; Index < CpuMpData->CpuCount; Index++) { > - if (CpuInfoInHob[Index].InitialApicId >= 0xFF) { > - X2Apic = TRUE; > - break; > - } > - } > } > > if (X2Apic) { > (1) I think this would break platforms that resolve the LocalApicLib class to the "BaseXApicLib.inf" instance. Based on the message of my earlier commit decb365b0016 ("OvmfPkg: select LocalApicLib instance with x2apic support", 2015-11-30), it seems like the BaseXApicLib instance notices and trips an assert when the LAPIC is in X2APIC mode, at the next time a LocalApicLib API is used. The BaseXApicLib instance contains many ASSERTs like this: ASSERT (GetApicMode () == LOCAL_APIC_MODE_XAPIC); and the GetApicMode() function itself has the following ASSERT: ASSERT (ApicBaseMsr.Bits.EXTD == 0); So, the change proposed in this patch needs to be gated by a boolean or Feature PCD, and the PCD should default to FALSE. If the platform uses the BaseXApicX2ApicLib instance, then it can set the PCD to TRUE. In turn, for such platform DSCs that already use BaseXApicX2ApicLib exclusively, in edk2 and in edk2-platforms, please post patches that set the PCD to TRUE. This includes the OvmfPkg platform DSC files, for example. (2) Please drop the "Change-Id" tag from the commit message. (3) I think this patch would not pass PatchCheck.py; the commit message is not wrapped correctly. The first line is 105 characters long. (4) Please elaborate on the "requirements for interrupt steering policy on Windows OS", in the commit message. I don't understand what that means. If you have a link to MSDN or similar, please include that. (5) Please take better care of the associated BZ . Given that you posted a patch, (a) the BZ should have status IN_PROGRESS, (b) you should be set as the assignee, and (c) a BZ comment should link your patch from the mailing list archive. Thanks Laszlo