From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by mx.groups.io with SMTP id smtpd.web10.2174.1587542108400300453 for ; Wed, 22 Apr 2020 00:55:08 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 192.55.52.43, mailfrom: ray.ni@intel.com) IronPort-SDR: K+M+TtfBtZyANDOXtFrvFHxp1olcW2zVkL6NRP4WmpBHC8u18q8Jiw0ciH+4A57X8KjfwRHNe2 EhLuxos7PTGQ== X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga007.fm.intel.com ([10.253.24.52]) by fmsmga105.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 22 Apr 2020 00:55:07 -0700 IronPort-SDR: uoUTjPkSIstdMM2FQAgNYNnKU1IUny7Ym8F6c62klvyPt0cs7zS9CpwsUE8TL4WTotNdYNnmvY 0jaTie1A1MOg== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.72,412,1580803200"; d="scan'208";a="245910081" Received: from fmsmsx105.amr.corp.intel.com ([10.18.124.203]) by fmsmga007.fm.intel.com with ESMTP; 22 Apr 2020 00:55:07 -0700 Received: from fmsmsx605.amr.corp.intel.com (10.18.126.85) by FMSMSX105.amr.corp.intel.com (10.18.124.203) with Microsoft SMTP Server (TLS) id 14.3.439.0; Wed, 22 Apr 2020 00:55:07 -0700 Received: from fmsmsx605.amr.corp.intel.com (10.18.126.85) by fmsmsx605.amr.corp.intel.com (10.18.126.85) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.1713.5; Wed, 22 Apr 2020 00:55:07 -0700 Received: from shsmsx153.ccr.corp.intel.com (10.239.6.53) by fmsmsx605.amr.corp.intel.com (10.18.126.85) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256) id 15.1.1713.5 via Frontend Transport; Wed, 22 Apr 2020 00:55:07 -0700 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.225]) by SHSMSX153.ccr.corp.intel.com ([169.254.12.89]) with mapi id 14.03.0439.000; Wed, 22 Apr 2020 15:54:04 +0800 From: "Ni, Ray" 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. Thread-Topic: [PATCH 1/2] UefiCpuPkg/MpInitLib: Restore IDT context for APs. Thread-Index: AQHWGEZCRcbLgwRijUOYdPxGAtsV/6iEs/sw Date: Wed, 22 Apr 2020 07:54:04 +0000 Message-ID: <734D49CCEBEEF84792F5B80ED585239D5C508113@SHSMSX104.ccr.corp.intel.com> References: <20200422013456.1053-1-eric.dong@intel.com> <20200422013456.1053-2-eric.dong@intel.com> In-Reply-To: <20200422013456.1053-2-eric.dong@intel.com> Accept-Language: en-US, zh-CN X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Return-Path: ray.ni@intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable 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 APs. >=20 > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D2683 >=20 >=20 >=20 > 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. >=20 >=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. 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=20 when CpuMpData->InitFlag is ApInitReconfig. This patch fixes this gap. >=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(-) >=20 > 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)); >=20 > // >=20 > // CpuMpData->CpuData[0].VolatileRegisters is initialized based on= BSP > environment, >=20 > - // to initialize AP in InitConfig path. >=20 > - // NOTE: IDTR.BASE stored in CpuMpData->CpuData[0].VolatileRegiste= rs > points to a different IDT shared by all APs. >=20 > + // to initialize AP in InitConfig/ApInitReconfig path. >=20 > + // NOTE: IDTR.BASE stored in CpuMpData->CpuData[0].VolatileRegiste= rs > points to a >=20 > + // different IDT shared by all APs. >=20 > // >=20 > RestoreVolatileRegisters (&CpuMpData->CpuData[0].VolatileRegisters= , > FALSE); >=20 > InitializeApData (CpuMpData, ProcessorNumber, BistData, > ApTopOfStack); >=20 > @@ -673,6 +674,16 @@ ApWakeupFunction ( >=20 >=20 > InterlockedDecrement ((UINT32 *) &CpuMpData->MpCpuExchangeInfo- > >NumApsExecuting); >=20 > } else { >=20 > + if ((CpuMpData->InitFlag =3D=3D ApInitReconfig) && (CpuMpData- > >ApLoopMode !=3D ApInHltLoop)) { >=20 > + // >=20 > + // CpuMpData->CpuData[0].VolatileRegisters is initialized based = on > BSP environment, >=20 > + // 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. >=20 > + // NOTE: IDTR.BASE stored in CpuMpData- > >CpuData[0].VolatileRegisters points to a >=20 > + // different IDT shared by all APs. >=20 > + // >=20 > + RestoreVolatileRegisters (&CpuMpData->CpuData[0].VolatileRegiste= rs, > FALSE); 4. Is it possible to combine this function call with the restoration code b= elow? if (CpuMpData->InitFlag =3D=3D ApInitReconfig) { // // ApInitReconfig happens when: // 1. AP is re-enabled after it's disabled, in either PEI or DXE ph= ase. // 2. AP is initialized in DXE phase.=20 // In either case, use the volatile registers value derived from BS= P. // NOTE: IDTR.BASE stored in CpuMpData->CpuData[0].VolatileRegister= s 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].Vo= latileRegisters, TRUE); } else { // // The CPU driver might not flush TLB for APs on spot after updat= ing // page attributes. AP in mwait loop mode needs to take care of i= t when // woken up. // CpuFlushTlb (); } } >=20 > + } >=20 > + >=20 > // >=20 > // Execute AP function if AP is ready >=20 > // >=20 > -- > 2.23.0.windows.1