From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by mx.groups.io with SMTP id smtpd.web11.2550.1588162897143510534 for ; Wed, 29 Apr 2020 05:21:37 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 134.134.136.24, mailfrom: eric.dong@intel.com) IronPort-SDR: 6Ds8yy7HVyoQqHVP6FVU3R+CKjUAzA/LQMZcIQV2hVOtvcNczVuEhFGPQgPJMQwWVmBogZZk62 amtQ+4R4742w== X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga102.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 29 Apr 2020 05:21:34 -0700 IronPort-SDR: gS7xRLg/PuelTmQZBHXQKgyl0vklsogWQjMa6SYaG68nGXam5AVLITIJpHNkoK/KKDrSg8k3Hw +0rpljFADOpA== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.73,332,1583222400"; d="scan'208";a="276154656" Received: from fmsmsx105.amr.corp.intel.com ([10.18.124.203]) by orsmga002.jf.intel.com with ESMTP; 29 Apr 2020 05:21:34 -0700 Received: from fmsmsx156.amr.corp.intel.com (10.18.116.74) by FMSMSX105.amr.corp.intel.com (10.18.124.203) with Microsoft SMTP Server (TLS) id 14.3.439.0; Wed, 29 Apr 2020 05:21:34 -0700 Received: from shsmsx153.ccr.corp.intel.com (10.239.6.53) by fmsmsx156.amr.corp.intel.com (10.18.116.74) with Microsoft SMTP Server (TLS) id 14.3.439.0; Wed, 29 Apr 2020 05:21:33 -0700 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.138]) by SHSMSX153.ccr.corp.intel.com ([169.254.12.89]) with mapi id 14.03.0439.000; Wed, 29 Apr 2020 20:21:31 +0800 From: "Dong, Eric" To: Laszlo Ersek , "devel@edk2.groups.io" CC: "Ni, Ray" , "Kumar, Chandana C" Subject: Re: [edk2-devel] [PATCH v3 1/2] UefiCpuPkg/MpInitLib: Restore IDT context for APs. Thread-Topic: [edk2-devel] [PATCH v3 1/2] UefiCpuPkg/MpInitLib: Restore IDT context for APs. Thread-Index: AQHWHhU4QvtL6WtPkk+aWntu8qVpqKiQAxTg Date: Wed, 29 Apr 2020 12:21:30 +0000 Message-ID: References: <20200424084716.877-1-eric.dong@intel.com> <20200424084716.877-2-eric.dong@intel.com> In-Reply-To: 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: Laszlo Ersek [mailto:lersek@redhat.com] > Sent: Wednesday, April 29, 2020 6:59 PM > To: devel@edk2.groups.io; Dong, Eric > Cc: Ni, Ray ; Kumar, Chandana C > > Subject: Re: [edk2-devel] [PATCH v3 1/2] UefiCpuPkg/MpInitLib: Restore ID= T > context for APs. >=20 > Hi Eric, >=20 > (1) on the email address list of this patch set, there is a strange item: >=20 > 00000000 16 64 65 76 65 6c 40 65 64 6b 32 2e 67 72 6f 75 |.devel@edk2.= grou| > 00000010 70 73 2e 69 6f |ps.io| >=20 > The first character is U+0016, which seems to stand for SYNCHRONOUS IDLE > . I think it must have be= en > a typo. >=20 I copied the mail address from outlook, maybe some invisible characters cau= sed this error. =20 > (The set was otherwise posted correctly to the list -- the correct list a= ddress is > present, too.) >=20 > Pointing this out just for future postings. >=20 > On 04/24/20 10:47, Dong, Eric wrote: > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D2683 > > > > This patch fixes an assertion because AP can't find the CpuMpData. > > 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 > > --- > > V3: > > Remove invalid save volatile registers process. Refine restore > > volatile registers process. > > > > V2: > > Enhance code to remove CpuMpData->ApLoopMode =3D=3D ApInHltLoop > check. > > > > UefiCpuPkg/Library/MpInitLib/MpLib.c | 34 > > +++++++++++++++++++--------- > > 1 file changed, 23 insertions(+), 11 deletions(-) > > > > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c > > b/UefiCpuPkg/Library/MpInitLib/MpLib.c > > index 64a4c3546e..7fd757b428 100644 > > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c > > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c > > @@ -686,18 +686,31 @@ ApWakeupFunction ( > > WAKEUP_AP_SIGNAL, > > 0 > > ); > > - if (CpuMpData->ApLoopMode =3D=3D ApInHltLoop) { > > - // > > - // Restore AP's volatile registers saved > > - // > > - RestoreVolatileRegisters (&CpuMpData- > >CpuData[ProcessorNumber].VolatileRegisters, TRUE); > > - } else { > > + > > + if (CpuMpData->InitFlag =3D=3D ApInitReconfig) { > > // > > - // The CPU driver might not flush TLB for APs on spot after up= dating > > - // page attributes. AP in mwait loop mode needs to take care o= f it > when > > - // woken up. > > + // 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. >=20 > (2) git-am complains that the line above has a trailing space character. = Please > remove it before you push the set. Got it. Will remove it when merge the code. >=20 > > + // 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. > > // > > - CpuFlushTlb (); > > + 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 (); > > + } > > } > > > > if (GetApState (&CpuMpData->CpuData[ProcessorNumber]) =3D=3D > > CpuStateReady) { @@ -1780,7 +1793,6 @@ MpInitLibInitialize ( > > InitializeSpinLock(&CpuMpData->CpuData[Index].ApLock); > > CpuMpData->CpuData[Index].CpuHealthy =3D > (CpuInfoInHob[Index].Health =3D=3D 0)? TRUE:FALSE; > > CpuMpData->CpuData[Index].ApFunction =3D 0; > > - CopyMem (&CpuMpData->CpuData[Index].VolatileRegisters, > &VolatileRegisters, sizeof (CPU_VOLATILE_REGISTERS)); > > } > > } > > > > >=20 > For the full series: >=20 > Regression-tested-by: Laszlo Ersek >=20 > (For the testing, I used OVMF, with/without SMM, and normal boot and S3. > With SMM, I also tested CPU hotplug. OVMF doesn't support "microcode > patching", so maybe it didn't make sense to perform these regression-test= s. > But, "these patches should be harmless regarding ", are famous > last words... So I rather wanted to regression-test them, under my use ca= se.) Thanks for your test! Very appreciate your supports. >=20 > Thanks, > Laszlo