From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [205.139.110.120]) by mx.groups.io with SMTP id smtpd.web12.9057.1594381876921154599 for ; Fri, 10 Jul 2020 04:51:17 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=TG1gZoNt; spf=pass (domain: redhat.com, ip: 205.139.110.120, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1594381876; 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=pqXaxbPbY+STngFuaB2IuDL7RCAt4YbaD4PNVqcca/Y=; b=TG1gZoNtPN2IviAE8ZcvgDMwsZ6Wn9euUa6b7Yb+tv4m+SEVooFXg4fFzGsxH3kEEX6qDD ht2M5C/oGPAMniwZLPGFZk6WRgAC3WxXmP7/VfxmuzfLsIs8hYSnvo0YjsH7xxFSDKCwIO C/HIWFTVvxIRBMFe1p0W3WLvEdNzQt0= 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-492-eq5HJIxCNmKviaQnIZXJ6g-1; Fri, 10 Jul 2020 07:51:09 -0400 X-MC-Unique: eq5HJIxCNmKviaQnIZXJ6g-1 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id A02E980BCAA; Fri, 10 Jul 2020 11:51:07 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-114-173.ams2.redhat.com [10.36.114.173]) by smtp.corp.redhat.com (Postfix) with ESMTP id 5059F61982; Fri, 10 Jul 2020 11:51:06 +0000 (UTC) Subject: Re: [edk2: PATCH] UefiCpuPkg: To enable X2Apic by default From: "Laszlo Ersek" To: Ji-yunX Lu , devel@edk2.groups.io Cc: Eric Dong , Ray Ni , Rahul Kumar References: <20200710033123.2236-1-ji-yunx.lu@intel.com> <822cc92e-74f8-14b3-928f-49c0fd670ffc@redhat.com> Message-ID: <35d1394b-605f-9dc3-6ec1-73668229e74e@redhat.com> Date: Fri, 10 Jul 2020 13:51:05 +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: <822cc92e-74f8-14b3-928f-49c0fd670ffc@redhat.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 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 07:38, Laszlo Ersek wrote: > 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. If a PCD is considered overkill for this, then a new API could be declared in LocalApicLib. UINTN EFIAPI GetMaxApicMode ( VOID ); In BaseXApicLib, the function would return constant LOCAL_APIC_MODE_XAPIC. In BaseXApicX2ApicLib, the function would call the AsmCpuid() seen above in the patch, and return LOCAL_APIC_MODE_XAPIC or LOCAL_APIC_MODE_X2APIC, dependent on "VersionInfoEcx.Bits.x2APIC". And then this patch would call GetMaxApicMode(), rather than check CPUID. Thanks Laszlo