From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by mx.groups.io with SMTP id smtpd.web12.674.1579407341948018836 for ; Sat, 18 Jan 2020 20:15:42 -0800 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 192.55.52.88, mailfrom: hao.a.wu@intel.com) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 18 Jan 2020 20:15:41 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.70,336,1574150400"; d="scan'208";a="426412969" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by fmsmga006.fm.intel.com with ESMTP; 18 Jan 2020 20:15:41 -0800 Received: from fmsmsx102.amr.corp.intel.com (10.18.124.200) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.439.0; Sat, 18 Jan 2020 20:15:41 -0800 Received: from shsmsx103.ccr.corp.intel.com (10.239.4.69) by FMSMSX102.amr.corp.intel.com (10.18.124.200) with Microsoft SMTP Server (TLS) id 14.3.439.0; Sat, 18 Jan 2020 20:15:40 -0800 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.197]) by SHSMSX103.ccr.corp.intel.com ([169.254.4.245]) with mapi id 14.03.0439.000; Sun, 19 Jan 2020 12:15:39 +0800 From: "Wu, Hao A" To: "Ni, Ray" , Laszlo Ersek , "devel@edk2.groups.io" CC: "Dong, Eric" , "Kinney, Michael D" Subject: Re: [PATCH v2] UefiCpuPkg/MpInitLib: Fix possible uninitialized 'InitFlag' field Thread-Topic: [PATCH v2] UefiCpuPkg/MpInitLib: Fix possible uninitialized 'InitFlag' field Thread-Index: AQHVzSpCnjZd4Ln0sUqHLFyyz02ZlqfuO+0AgAMnwfA= Date: Sun, 19 Jan 2020 04:15:38 +0000 Message-ID: References: <20200117113525.4768-1-hao.a.wu@intel.com> <557c365d-8288-6a65-065f-2d63e6954591@redhat.com> In-Reply-To: <557c365d-8288-6a65-065f-2d63e6954591@redhat.com> Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Return-Path: hao.a.wu@intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Thanks all, Patch has been pushed via commit 18fcb37598. Best Regards, Hao Wu > -----Original Message----- > From: Laszlo Ersek [mailto:lersek@redhat.com] > Sent: Friday, January 17, 2020 8:04 PM > To: Wu, Hao A; devel@edk2.groups.io > Cc: Dong, Eric; Ni, Ray; Kinney, Michael D > Subject: Re: [PATCH v2] UefiCpuPkg/MpInitLib: Fix possible uninitialized > 'InitFlag' field >=20 > On 01/17/20 12:35, Hao A Wu wrote: > > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=3D2474 > > > > Previous commit d786a17232: > > UefiCpuPkg/MpInitLib: Reduce the size when loading microcode patches > > > > Removed the below assignments for the 'InitFlag' field of CPU_MP_DATA > > structure in function MpInitLibInitialize() when APs are waken up to do > > some initialize sync: > > > > CpuMpData->InitFlag =3D ApInitReconfig; > > ... > > CpuMpData->InitFlag =3D ApInitDone; > > > > The above commit mistakenly assumed the 'InitFlag' field will have a va= lue > > of 'ApInitDone' when the APs have been successfully waken up before. > And > > since there is no explicit comparision for the 'InitFlag' field with th= e > > 'ApInitReconfig' value. The commit removed those assignments. > > > > However, under some cases (e.g. when variable OldCpuMpData is not > NULL, > > which means function CollectProcessorCount() will not be called), remov= ing > > the above assignments will left the 'InitFlag' field being uninitialize= d > > with a value of 0, which is a invalid value for the type of 'InitFlag' > > (AP_INIT_STATE). > > > > It may potentially cause the WakeUpAP() function to run some > unnecessary > > codes when the APs have been successfully waken up before: > > > > if (CpuMpData->WakeUpByInitSipiSipi || > > CpuMpData->InitFlag !=3D ApInitDone) { > > ResetVectorRequired =3D TRUE; > > AllocateResetVector (CpuMpData); > > FillExchangeInfoData (CpuMpData); > > SaveLocalApicTimerSetting (CpuMpData); > > } > > > > This commit will address the above-mentioned issue. > > > > Test done: > > * OS boot on a real platform with multi processors > > > > Cc: Eric Dong > > Cc: Ray Ni > > Cc: Laszlo Ersek > > Cc: Michael D Kinney > > Signed-off-by: Hao A Wu > > Reviewed-by: Ray Ni > > --- > > UefiCpuPkg/Library/MpInitLib/MpLib.c | 6 +++++- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c > b/UefiCpuPkg/Library/MpInitLib/MpLib.c > > index 6ec9b172b8..855d37ba3e 100644 > > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c > > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c > > @@ -1775,11 +1775,15 @@ MpInitLibInitialize ( > > // Wakeup APs to do some AP initialize sync (Microcode & MTRR) > > // > > if (CpuMpData->CpuCount > 1) { > > + CpuMpData->InitFlag =3D ApInitReconfig; > > WakeUpAP (CpuMpData, TRUE, 0, ApInitializeSync, CpuMpData, TRUE); > > + // > > + // Wait for all APs finished initialization > > + // > > while (CpuMpData->FinishedCount < (CpuMpData->CpuCount - 1)) { > > CpuPause (); > > } > > - > > + CpuMpData->InitFlag =3D ApInitDone; > > for (Index =3D 0; Index < CpuMpData->CpuCount; Index++) { > > SetApState (&CpuMpData->CpuData[Index], CpuStateIdle); > > } > > >=20 > Acked-by: Laszlo Ersek