From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [207.211.31.120]) by mx.groups.io with SMTP id smtpd.web09.3575.1572513069706647497 for ; Thu, 31 Oct 2019 02:11:09 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=cZlrtvD6; spf=pass (domain: redhat.com, ip: 207.211.31.120, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1572513068; 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=ka1rOD/LP12Cq0HQSp7pGxL00N/cyAUtZraety2Ig44=; b=cZlrtvD6aV567txSAOoTjtVWAiV+/Rk1yWIzVPEtXbWlJBu0NFVEDVTbqvXgPOv6uHoJFC kEtRGqQOT3EsM2vGu7OWtXn8Kqx4E3EI7GcpTOrgDYPzbdzOlrynGfAgPTZE1jCvNMIoWl 2Wq8+1aiH0w1oe/eFkaxOxYA5e1oAnc= 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-123-UhYpK7EuN2SJ3wRLqKxMGw-1; Thu, 31 Oct 2019 05:11:05 -0400 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 87EA21800D55; Thu, 31 Oct 2019 09:11:04 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-117-6.ams2.redhat.com [10.36.117.6]) by smtp.corp.redhat.com (Postfix) with ESMTP id A907F5D6D6; Thu, 31 Oct 2019 09:11:03 +0000 (UTC) Subject: Re: [PATCH 1/2] UefiCpuPkg/MpInitLib: Set X2ApicEnable flag from BSP To: Ray Ni , devel@edk2.groups.io Cc: Eric Dong References: <20191030095233.565420-1-ray.ni@intel.com> <20191030095233.565420-2-ray.ni@intel.com> From: "Laszlo Ersek" Message-ID: <4af5a9e3-55e9-09fd-f015-a81d318b7dbd@redhat.com> Date: Thu, 31 Oct 2019 10:11:02 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20191030095233.565420-2-ray.ni@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-MC-Unique: UhYpK7EuN2SJ3wRLqKxMGw-1 X-Mimecast-Spam-Score: 0 Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 10/30/19 10:52, Ray Ni wrote: > Today's logic sets X2ApicEnable flag in each AP's initialization > path when InitFlag =3D=3D ApInitConfig. > Since all CPUs update the same global data, a spin-lock is used > to avoid modifications from multiple CPUs happen at the same time. > The spin-lock causes two problems: > 1. Potential performance downgrade. > 2. Undefined behavior when improper timer lib is used. > For example we saw certain platforms used AcpiTimerLib from > PcAtChipsetPkg and that library depends on retrieving PeiServices > from idtr. But in fact AP's (idtr - 4) doesn't point to > PeiServices. >=20 > The patch simplifies the code to let BSP set the X2ApicEnable flag so > the spin-lock acquisition from AP is not needed any more. >=20 > Signed-off-by: Ray Ni > Cc: Eric Dong > Cc: Laszlo Ersek > --- > UefiCpuPkg/Library/MpInitLib/MpLib.c | 25 ++++++++++++++++--------- > 1 file changed, 16 insertions(+), 9 deletions(-) >=20 > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/Mp= InitLib/MpLib.c > index 622b70ca3c..8f62a8d965 100644 > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c > @@ -458,6 +458,7 @@ CollectProcessorCount ( > ) > { > UINTN Index; > + CPU_INFO_IN_HOB *CpuInfoInHob; > =20 > // > // Send 1st broadcast IPI to APs to wakeup APs > @@ -474,12 +475,27 @@ CollectProcessorCount ( > CpuPause (); > } > =20 > + =20 > + // > + // Enable x2APIC mode if > + // 1. Number of CPU is greater than 255; or > + // 2. There are any logical processors reporting an Initial APIC ID o= f 255 or greater. > + // > if (CpuMpData->CpuCount > 255) { > // > // If there are more than 255 processor found, force to enable X2API= C > // > CpuMpData->X2ApicEnable =3D TRUE; > + } else { > + CpuInfoInHob =3D (CPU_INFO_IN_HOB *) (UINTN) CpuMpData->CpuInfoInHob= ; > + for (Index =3D 0; Index < CpuMpData->CpuCount; Index++) { > + if (CpuInfoInHob[Index].InitialApicId >=3D 0xFF) { > + CpuMpData->X2ApicEnable =3D TRUE; > + break; > + } > + } > } > + > if (CpuMpData->X2ApicEnable) { > DEBUG ((DEBUG_INFO, "Force x2APIC mode!\n")); > // > @@ -541,15 +557,6 @@ InitializeApData ( > =20 > CpuMpData->CpuData[ProcessorNumber].Waiting =3D FALSE; > CpuMpData->CpuData[ProcessorNumber].CpuHealthy =3D (BistData =3D=3D 0)= ? TRUE : FALSE; > - if (CpuInfoInHob[ProcessorNumber].InitialApicId >=3D 0xFF) { > - // > - // Set x2APIC mode if there are any logical processor reporting > - // an Initial APIC ID of 255 or greater. > - // > - AcquireSpinLock(&CpuMpData->MpLock); > - CpuMpData->X2ApicEnable =3D TRUE; > - ReleaseSpinLock(&CpuMpData->MpLock); > - } > =20 > InitializeSpinLock(&CpuMpData->CpuData[ProcessorNumber].ApLock); > SetApState (&CpuMpData->CpuData[ProcessorNumber], CpuStateIdle); >=20 I would have suggested "InterlockedCompareExchange8" as an alternative, but (a) maybe there's really no reason for updating that field from APs, (b) there doesn't seem to be a single byte version of that function. So, the patch looks plausible. Got no time to test it now, alas. Acked-by: Laszlo Ersek Thanks Laszlo