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.web11.3580.1572513609832790747 for ; Thu, 31 Oct 2019 02:20:10 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=JSecwRBj; 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=1572513609; 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=2uQF7uVZc+TWHwin3zNBE3XAIlUZh7tAaouiRLuhkqY=; b=JSecwRBjBKDob/xs0ey8yPCQAIog7OOCU+s6lU2ziu/yktqrELVl2z9u3tFOZkXoV7ym6m S/EvecuSIW8hLMMR2KqUlSbghXzG67GP2uQdG0tu2TafCNkM8libHLRxXPa2p1AmfXZZII BMuEgLzQiwoWjYyijVuetbfRyAt6NTo= 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-210-gTRFgLW0OEqfSNk24kKJLg-1; Thu, 31 Oct 2019 05:20: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 AEF131005500; Thu, 31 Oct 2019 09:20: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 A28895D70D; Thu, 31 Oct 2019 09:20:03 +0000 (UTC) Subject: Re: [PATCH 2/2] UefiCpuPkg/MpInitLib: Remove global variable X2ApicEnable To: Ray Ni , devel@edk2.groups.io Cc: Eric Dong References: <20191030095233.565420-1-ray.ni@intel.com> <20191030095233.565420-3-ray.ni@intel.com> From: "Laszlo Ersek" Message-ID: <0a73f7c5-7ac3-bf35-9a39-8b576acb4656@redhat.com> Date: Thu, 31 Oct 2019 10:20: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-3-ray.ni@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-MC-Unique: gTRFgLW0OEqfSNk24kKJLg-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: > MpInitLib sets X2ApicEnable in two places. > 1. CollectProcessorCount() > This function is called when MpInitLibInitialize() hasn't been > called before. > It sets X2ApicEnable and later in the same function it configures > all CPUs to operate in X2 APIC mode. > 2. MpInitLibInitialize() > The X2ApicEnable setting happens when this function is called in > second time. But after that setting, no code consumes that flag. >=20 > With the above analysis and with the purpose of simplifying the code, > the X2ApicEnable in #1 is changed to local variable and the #2 can be > changed to remove the setting of X2ApicEnable. >=20 > Signed-off-by: Ray Ni > Cc: Eric Dong > Cc: Laszlo Ersek > --- > UefiCpuPkg/Library/MpInitLib/MpLib.c | 14 ++++++-------- > UefiCpuPkg/Library/MpInitLib/MpLib.h | 1 - > 2 files changed, 6 insertions(+), 9 deletions(-) >=20 > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/Mp= InitLib/MpLib.c > index 8f62a8d965..49be5d5385 100644 > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c > @@ -459,12 +459,12 @@ CollectProcessorCount ( > { > UINTN Index; > CPU_INFO_IN_HOB *CpuInfoInHob; > + BOOLEAN X2Apic; > =20 > // > // Send 1st broadcast IPI to APs to wakeup APs > // > - CpuMpData->InitFlag =3D ApInitConfig; > - CpuMpData->X2ApicEnable =3D FALSE; > + CpuMpData->InitFlag =3D ApInitConfig; > WakeUpAP (CpuMpData, TRUE, 0, NULL, NULL, TRUE); > CpuMpData->InitFlag =3D ApInitDone; > ASSERT (CpuMpData->CpuCount <=3D PcdGet32 (PcdCpuMaxLogicalProcessorNu= mber)); > @@ -481,22 +481,23 @@ CollectProcessorCount ( > // 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. > // > + X2Apic =3D FALSE; > if (CpuMpData->CpuCount > 255) { > // > // If there are more than 255 processor found, force to enable X2API= C > // > - CpuMpData->X2ApicEnable =3D TRUE; > + X2Apic =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; > + X2Apic =3D TRUE; > break; > } > } > } > =20 > - if (CpuMpData->X2ApicEnable) { > + if (X2Apic) { > DEBUG ((DEBUG_INFO, "Force x2APIC mode!\n")); > // > // Wakeup all APs to enable x2APIC mode > @@ -1780,9 +1781,6 @@ MpInitLibInitialize ( > CpuInfoInHob =3D (CPU_INFO_IN_HOB *) (UINTN) CpuMpData->CpuInfoInHob= ; > for (Index =3D 0; Index < CpuMpData->CpuCount; Index++) { > InitializeSpinLock(&CpuMpData->CpuData[Index].ApLock); > - if (CpuInfoInHob[Index].InitialApicId >=3D 255 || Index > 254) { > - CpuMpData->X2ApicEnable =3D TRUE; > - } > CpuMpData->CpuData[Index].CpuHealthy =3D (CpuInfoInHob[Index].Heal= th =3D=3D 0)? TRUE:FALSE; > CpuMpData->CpuData[Index].ApFunction =3D 0; > CopyMem (&CpuMpData->CpuData[Index].VolatileRegisters, &VolatileRe= gisters, sizeof (CPU_VOLATILE_REGISTERS)); > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/Mp= InitLib/MpLib.h > index 107872b367..8fa07b12c5 100644 > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h > @@ -227,7 +227,6 @@ struct _CPU_MP_DATA { > UINTN **FailedCpuList; > =20 > AP_INIT_STATE InitFlag; > - BOOLEAN X2ApicEnable; > BOOLEAN SwitchBspFlag; > UINTN NewBspNumber; > CPU_EXCHANGE_ROLE_INFO BSPInfo; >=20 Assuming the previous patch does the right thing in the series (and I think that's plausible), this patch looks correct as well. For a second I got concerned as to whether the field removal from CPU_MP_DATA would require updates to the offset constants in the assembly code. However, assembly code seems to know offsets into MP_CPU_EXCHANGE_INFO only, and doesn't care about CPU_MP_DATA. So the patch is OK I think. Reviewed-by: Laszlo Ersek Thanks Laszlo