From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga17.intel.com (mga17.intel.com [192.55.52.151]) by mx.groups.io with SMTP id smtpd.web12.2547.1587543766126144363 for ; Wed, 22 Apr 2020 01:22:46 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 192.55.52.151, mailfrom: eric.dong@intel.com) IronPort-SDR: dOaOFQflkydg49soUPrDswngaV6DZd4skyYAg10oI3FEmNZBoD4HLW/4r2QEeftUxHHMLVGfXf HMcor2w1gplA== X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by fmsmga107.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 22 Apr 2020 01:22:45 -0700 IronPort-SDR: OXrhWi/QMwjJqYosGXgbNJT+JBuCf4LPsgyVW31F3PK9c2v8gRzOuu311vAD94NSCP86i3875b namgFz6fVXMg== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.72,413,1580803200"; d="scan'208";a="290754343" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by fmsmga002.fm.intel.com with ESMTP; 22 Apr 2020 01:22:45 -0700 Received: from FMSMSX110.amr.corp.intel.com (10.18.116.10) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.439.0; Wed, 22 Apr 2020 01:22:44 -0700 Received: from shsmsx104.ccr.corp.intel.com (10.239.4.70) by fmsmsx110.amr.corp.intel.com (10.18.116.10) with Microsoft SMTP Server (TLS) id 14.3.439.0; Wed, 22 Apr 2020 01:22:44 -0700 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.138]) by SHSMSX104.ccr.corp.intel.com ([169.254.5.225]) with mapi id 14.03.0439.000; Wed, 22 Apr 2020 16:22:42 +0800 From: "Dong, Eric" To: "Ni, Ray" , "devel@edk2.groups.io" CC: Laszlo Ersek , "Kumar, Chandana C" Subject: Re: [PATCH 1/2] UefiCpuPkg/MpInitLib: Restore IDT context for APs. Thread-Topic: [PATCH 1/2] UefiCpuPkg/MpInitLib: Restore IDT context for APs. Thread-Index: AQHWGEZCRcbLgwRijUOYdPxGAtsV/6iEs/swgAAYBaA= Date: Wed, 22 Apr 2020 08:22:42 +0000 Message-ID: References: <20200422013456.1053-1-eric.dong@intel.com> <20200422013456.1053-2-eric.dong@intel.com> <734D49CCEBEEF84792F5B80ED585239D5C508113@SHSMSX104.ccr.corp.intel.com> In-Reply-To: <734D49CCEBEEF84792F5B80ED585239D5C508113@SHSMSX104.ccr.corp.intel.com> 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: 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 AP= s. >=20 > Please check my comments in below. >=20 > > -----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 APs. > > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D2683 > > > > > > > > This patch used to fix a ASSERT because AP can't find the CpuMpData. >=20 > 1. This patch fixes an assertion because AP can't find the CpuMpData. >=20 > > > > > > 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. >=20 > 2. When AP is waken up through Init-Sipi-Sipi, AP's IDT should be restore= d 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 i= s > ApInitConfig but misses the logic when CpuMpData->InitFlag is > ApInitReconfig. > This patch fixes this gap. >=20 >=20 >=20 > > > > 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].VolatileRegiste= rs, > > 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 base= d on > > BSP environment, > > > > + // to initialize AP in InitConfig/ApInitReconfig path. >=20 > 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. >=20 > > > > + // NOTE: IDTR.BASE stored in CpuMpData- > > >CpuData[0].VolatileRegisters points to a > > > > + // different IDT shared by all APs. > > > > + // > > > > + RestoreVolatileRegisters (&CpuMpData- > >CpuData[0].VolatileRegisters, > > FALSE); >=20 > 4. Is it possible to combine this function call with the restoration code= below? >=20 New logic changes code flow when CpuMpData->InitFlag =3D=3D ApInitReconfig.= 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 no= t change this code flow. Thanks, Eric > if (CpuMpData->InitFlag =3D=3D ApInitReconfig) { > // > // 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. > // In either case, use the volatile registers value derived from = BSP. > // NOTE: IDTR.BASE stored in CpuMpData->CpuData[0].VolatileRegist= ers > points to a > // different IDT shared by all APs. > // > RestoreVolatileRegisters (&CpuMpData->CpuData[0].VolatileRegister= s, > 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 upd= ating > // 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