From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by mx.groups.io with SMTP id smtpd.web10.2744.1587546045039775395 for ; Wed, 22 Apr 2020 02:00:45 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 134.134.136.31, mailfrom: eric.dong@intel.com) IronPort-SDR: eij4SZleIfI//XlvTmmJChxXG4QxUB3WrDLoyS+TaD1NEWbSFBgfK0qhH3IDf18QL+scFlcDL7 UVrnbLTDI/GQ== X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga006.jf.intel.com ([10.7.209.51]) by orsmga104.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 22 Apr 2020 02:00:43 -0700 IronPort-SDR: 6fdEUzpNLqaKGjqGy8pq7dPQJ7UtZX/dhL3dYB5PT+Y67zih2Ge04t77Vf1JAcJajK0gPlpmui KoOLihwqGIcQ== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.72,413,1580803200"; d="scan'208";a="259006387" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by orsmga006.jf.intel.com with ESMTP; 22 Apr 2020 02:00:43 -0700 Received: from fmsmsx151.amr.corp.intel.com (10.18.125.4) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.439.0; Wed, 22 Apr 2020 02:00:43 -0700 Received: from shsmsx101.ccr.corp.intel.com (10.239.4.153) by FMSMSX151.amr.corp.intel.com (10.18.125.4) with Microsoft SMTP Server (TLS) id 14.3.439.0; Wed, 22 Apr 2020 02:00:43 -0700 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.138]) by SHSMSX101.ccr.corp.intel.com ([169.254.1.129]) with mapi id 14.03.0439.000; Wed, 22 Apr 2020 17:00:40 +0800 From: "Dong, Eric" To: "devel@edk2.groups.io" , "Dong, Eric" , "Ni, Ray" CC: Laszlo Ersek , "Kumar, Chandana C" Subject: Re: [edk2-devel] [PATCH 1/2] UefiCpuPkg/MpInitLib: Restore IDT context for APs. Thread-Topic: [edk2-devel] [PATCH 1/2] UefiCpuPkg/MpInitLib: Restore IDT context for APs. Thread-Index: AQHWGEZCRcbLgwRijUOYdPxGAtsV/6iEs/swgAAYBaCAAAvKQA== Date: Wed, 22 Apr 2020 09:00:40 +0000 Message-ID: References: <20200422013456.1053-1-eric.dong@intel.com> <20200422013456.1053-2-eric.dong@intel.com> <734D49CCEBEEF84792F5B80ED585239D5C508113@SHSMSX104.ccr.corp.intel.com> <160816A4F32AAED9.20374@groups.io> In-Reply-To: <160816A4F32AAED9.20374@groups.io> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Return-Path: eric.dong@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 > Dong, Eric > Sent: Wednesday, April 22, 2020 4:23 PM > To: Ni, Ray ; devel@edk2.groups.io > Cc: Laszlo Ersek ; Kumar, Chandana C > > Subject: Re: [edk2-devel] [PATCH 1/2] UefiCpuPkg/MpInitLib: Restore IDT > context for APs. >=20 > > -----Original Message----- > > From: Ni, Ray > > Sent: Wednesday, April 22, 2020 3:54 PM > > To: Dong, Eric ; devel@edk2.groups.io > > Cc: Laszlo Ersek ; Kumar, Chandana C > > > > Subject: RE: [PATCH 1/2] UefiCpuPkg/MpInitLib: Restore IDT context for > APs. > > > > Please check my comments in below. > > > > > -----Original Message----- > > > From: Dong, Eric > > > Sent: Wednesday, April 22, 2020 9:35 AM > > > To: devel@edk2.groups.io > > > Cc: Ni, Ray ; Laszlo Ersek ; > > > Kumar, Chandana C > > > Subject: [PATCH 1/2] UefiCpuPkg/MpInitLib: Restore IDT context for A= Ps. > > > > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D2683 > > > > > > > > > > > > This patch used to fix a ASSERT because AP can't find the CpuMpData. > > > > 1. This patch fixes an assertion because AP can't find the CpuMpData. > > > > > > > > > > > When AP been waked up through Init-Sipi-Sipi, the IDT should been > > > restore to preallcated buffer to make it can get the CpuMpData > > > through IDT base address. > > > Not when CpuMpData->InitFlag is ApInitReconfig or ApInitConfig, AP > > > will be wake up through Init-Sipi-Sipi. Current code already has > > > logic to handle ApInitConfig, but miss the handler for > > > ApInitReconfig. This patch fixes this gap. > > > > 2. When AP is waken up through Init-Sipi-Sipi, AP's IDT should be > > restored to pre-allocated buffer so AP can get the CpuMpData through > > the IDT base address. > > Current code already has logic to handle this when CpuMpData-> > > InitFlag is ApInitConfig but misses the logic when CpuMpData->InitFlag > > is ApInitReconfig. > > This patch fixes this gap. > > > > > > > > > > > > Cc: Ray Ni > > > Cc: Laszlo Ersek > > > Cc: Chandana Kumar > > > Signed-off-by: Eric Dong > > > --- > > > UefiCpuPkg/Library/MpInitLib/MpLib.c | 15 +++++++++++++-- > > > 1 file changed, 13 insertions(+), 2 deletions(-) > > > > > > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c > > > b/UefiCpuPkg/Library/MpInitLib/MpLib.c > > > index 64a4c3546e..ac7f92fd48 100644 > > > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c > > > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c > > > @@ -664,8 +664,9 @@ ApWakeupFunction ( > > > BistData =3D *(UINT32 *) ((UINTN) ApTopOfStack - sizeof > > > (UINTN)); > > > > > > // > > > > > > // CpuMpData->CpuData[0].VolatileRegisters is initialized > > > based on BSP environment, > > > > > > - // to initialize AP in InitConfig path. > > > > > > - // NOTE: IDTR.BASE stored in CpuMpData- > > >CpuData[0].VolatileRegisters > > > points to a different IDT shared by all APs. > > > > > > + // to initialize AP in InitConfig/ApInitReconfig path. > > > > > > + // NOTE: IDTR.BASE stored in CpuMpData- > > >CpuData[0].VolatileRegisters > > > points to a > > > > > > + // different IDT shared by all APs. > > > > > > // > > > > > > RestoreVolatileRegisters > > > (&CpuMpData->CpuData[0].VolatileRegisters, > > > FALSE); > > > > > > InitializeApData (CpuMpData, ProcessorNumber, BistData, > > > ApTopOfStack); > > > > > > @@ -673,6 +674,16 @@ ApWakeupFunction ( > > > > > > > > > InterlockedDecrement ((UINT32 *) &CpuMpData- > > >MpCpuExchangeInfo- > > > >NumApsExecuting); > > > > > > } else { > > > > > > + if ((CpuMpData->InitFlag =3D=3D ApInitReconfig) && (CpuMpData= - > > > >ApLoopMode !=3D ApInHltLoop)) { > > > > > > + // > > > > > > + // CpuMpData->CpuData[0].VolatileRegisters is initialized > > > + based on > > > BSP environment, > > > > > > + // to initialize AP in InitConfig/ApInitReconfig path. > > > > 3. > > Initialize AP volatile registers in ApInitReconfig path. > > ApInitReconfig happens when: > > 1. AP is re-enabled after it's disabled, in either PEI or DXE phase. > > 2. AP is initialized in DXE phase. > > > > > > > > + // NOTE: IDTR.BASE stored in CpuMpData- > > > >CpuData[0].VolatileRegisters points to a > > > > > > + // different IDT shared by all APs. > > > > > > + // > > > > > > + RestoreVolatileRegisters (&CpuMpData- > > >CpuData[0].VolatileRegisters, > > > FALSE); > > > > 4. Is it possible to combine this function call with the restoration c= ode > below? > > >=20 I move the code into the else of " if (CpuMpData->ApLoopMode =3D=3D ApInHl= tLoop)", it's truly better than original. Please see my v2 change. Thanks, Eric > New logic changes code flow when CpuMpData->InitFlag =3D=3D ApInitReconf= ig. > In original flow, the code does RestoreVolatileRegisters and CpuFlushTlb= , but > new code only does CpuFlushTlb. > Is CpuFlushTlb not needs if wake up through Init-Sipi-Sipi? I prefer to= not > change this code flow. >=20 > Thanks, > Eric >=20 > > if (CpuMpData->InitFlag =3D=3D ApInitReconfig) { > > // > > // ApInitReconfig happens when: > > // 1. AP is re-enabled after it's disabled, in either PEI or D= XE phase. > > // 2. AP is initialized in DXE phase. > > // In either case, use the volatile registers value derived fr= om BSP. > > // NOTE: IDTR.BASE stored in > > CpuMpData->CpuData[0].VolatileRegisters > > points to a > > // different IDT shared by all APs. > > // > > RestoreVolatileRegisters > > (&CpuMpData->CpuData[0].VolatileRegisters, > > FALSE); > > } else { > > if (CpuMpData->ApLoopMode =3D=3D ApInHltLoop) { > > // > > // Restore AP's volatile registers saved before AP is halted > > // > > RestoreVolatileRegisters (&CpuMpData- > > >CpuData[ProcessorNumber].VolatileRegisters, TRUE); > > } else { > > // > > // The CPU driver might not flush TLB for APs on spot after = updating > > // page attributes. AP in mwait loop mode needs to take care= of it > when > > // woken up. > > // > > CpuFlushTlb (); > > } > > } > > > > > > + } > > > > > > + > > > > > > // > > > > > > // Execute AP function if AP is ready > > > > > > // > > > > > > -- > > > 2.23.0.windows.1 >=20 >=20