From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) by mx.groups.io with SMTP id smtpd.web10.1830.1579251392801607912 for ; Fri, 17 Jan 2020 00:56:33 -0800 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 134.134.136.100, mailfrom: hao.a.wu@intel.com) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by orsmga105.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 17 Jan 2020 00:46:09 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.70,329,1574150400"; d="scan'208";a="261853675" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by fmsmga001.fm.intel.com with ESMTP; 17 Jan 2020 00:46:08 -0800 Received: from fmsmsx609.amr.corp.intel.com (10.18.126.89) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.439.0; Fri, 17 Jan 2020 00:46:08 -0800 Received: from fmsmsx609.amr.corp.intel.com (10.18.126.89) by fmsmsx609.amr.corp.intel.com (10.18.126.89) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.1713.5; Fri, 17 Jan 2020 00:46:08 -0800 Received: from shsmsx152.ccr.corp.intel.com (10.239.6.52) by fmsmsx609.amr.corp.intel.com (10.18.126.89) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256) id 15.1.1713.5 via Frontend Transport; Fri, 17 Jan 2020 00:46:07 -0800 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.197]) by SHSMSX152.ccr.corp.intel.com ([169.254.6.203]) with mapi id 14.03.0439.000; Fri, 17 Jan 2020 16:46:06 +0800 From: "Wu, Hao A" To: "devel@edk2.groups.io" , "lersek@redhat.com" CC: "Dong, Eric" , "Ni, Ray" , "Kinney, Michael D" Subject: Re: [edk2-devel] [PATCH v1] UefiCpuPkg/MpInitLib: Fix possible uninitialized 'InitFlag' field Thread-Topic: [edk2-devel] [PATCH v1] UefiCpuPkg/MpInitLib: Fix possible uninitialized 'InitFlag' field Thread-Index: AQHVzQNOf/XU9xrmnEqYwHefZX66RKfuA78AgACGoWA= Date: Fri, 17 Jan 2020 08:46:06 +0000 Message-ID: References: <20200117065638.9176-1-hao.a.wu@intel.com> <6e723520-b2ee-de9f-27f3-51d27f67f84e@redhat.com> In-Reply-To: <6e723520-b2ee-de9f-27f3-51d27f67f84e@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 > -----Original Message----- > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of > Laszlo Ersek > Sent: Friday, January 17, 2020 4:42 PM > To: Wu, Hao A; devel@edk2.groups.io > Cc: Dong, Eric; Ni, Ray; Kinney, Michael D > Subject: Re: [edk2-devel] [PATCH v1] UefiCpuPkg/MpInitLib: Fix possible > uninitialized 'InitFlag' field >=20 > On 01/17/20 07:56, 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 d= o > > some initialize sync: > > > > CpuMpData->InitFlag =3D ApInitReconfig; > > ... > > CpuMpData->InitFlag =3D ApInitDone; > > > > Under some cases (e.g. when variable OldCpuMpData is not NULL, which > means > > function CollectProcessorCount() will not be called), this will left t= he > > 'InitFlag' field being uninitialized with a value of 0, which is a inv= alid > > 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 > > --- > > UefiCpuPkg/Library/MpInitLib/MpLib.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c > b/UefiCpuPkg/Library/MpInitLib/MpLib.c > > index 6ec9b172b8..17e19395f2 100644 > > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c > > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c > > @@ -1775,11 +1775,12 @@ 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); > > 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 > It looks reasonable to me, but I was away while patch >=20 > "UefiCpuPkg/MpInitLib: Reduce the size when loading microcode patches" >=20 > was being reviewed, so I can't really say. >=20 > Can you explain (in the commit message) *why* commit d786a17232 > removed > these InitFlag assignments? I've now read the commit message on > d786a17232, and it's not obvious to me. >=20 Sure. I will update the commit message to add the information for such removal. >=20 > Also, it would be nice to reinstate the following comment: >=20 > // > // Wait for all APs finish initialization > // >=20 > just before the "while" statement. Yes, I will add back this comment in the next version of patch. Best Regards, Hao Wu >=20 > Thanks > Laszlo >=20 >=20 >=20