From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by mx.groups.io with SMTP id smtpd.web12.3854.1579144550898560812 for ; Wed, 15 Jan 2020 19:15:51 -0800 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 192.55.52.115, mailfrom: eric.dong@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 fmsmga103.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 15 Jan 2020 19:15:50 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.70,324,1574150400"; d="scan'208";a="255993374" Received: from fmsmsx107.amr.corp.intel.com ([10.18.124.205]) by fmsmga001.fm.intel.com with ESMTP; 15 Jan 2020 19:15:49 -0800 Received: from fmsmsx120.amr.corp.intel.com (10.18.124.208) by fmsmsx107.amr.corp.intel.com (10.18.124.205) with Microsoft SMTP Server (TLS) id 14.3.439.0; Wed, 15 Jan 2020 19:15:45 -0800 Received: from shsmsx154.ccr.corp.intel.com (10.239.6.54) by fmsmsx120.amr.corp.intel.com (10.18.124.208) with Microsoft SMTP Server (TLS) id 14.3.439.0; Wed, 15 Jan 2020 19:15:45 -0800 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.202]) by SHSMSX154.ccr.corp.intel.com ([169.254.7.132]) with mapi id 14.03.0439.000; Thu, 16 Jan 2020 11:15:43 +0800 From: "Dong, Eric" To: Laszlo Ersek , "devel@edk2.groups.io" CC: "Ni, Ray" Subject: Re: [PATCH] UefiCpuPkg/Library/MpInitLib: Remove BSP index == 0 Assumption. Thread-Topic: [PATCH] UefiCpuPkg/Library/MpInitLib: Remove BSP index == 0 Assumption. Thread-Index: AQHVy4s1Ohd/H9Zzr0S1YoV07S2me6fsnbCg Date: Thu, 16 Jan 2020 03:15:42 +0000 Message-ID: References: <20200115060642.1707-1-eric.dong@intel.com> <15007a75-a43e-203d-86f1-8b6a46ca30c9@redhat.com> In-Reply-To: <15007a75-a43e-203d-86f1-8b6a46ca30c9@redhat.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 Hi Laszlo, > -----Original Message----- > From: Laszlo Ersek [mailto:lersek@redhat.com] > Sent: Wednesday, January 15, 2020 6:05 PM > To: Dong, Eric ; devel@edk2.groups.io > Cc: Ni, Ray > Subject: Re: [PATCH] UefiCpuPkg/Library/MpInitLib: Remove BSP index =3D= =3D 0 > Assumption. >=20 > On 01/15/20 07:06, Eric Dong wrote: > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D2392 > > > > Current code implementation assumes BSP index is 0 at the begin. > > This code change removes this assumption. It get BSP index from the > > saved data structure if it existed. > > > > Cc: Ray Ni > > Cc: Laszlo Ersek > > Signed-off-by: Eric Dong > > --- > > UefiCpuPkg/Library/MpInitLib/MpLib.c | 10 ++++++---- > > 1 file changed, 6 insertions(+), 4 deletions(-) > > > > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c > > b/UefiCpuPkg/Library/MpInitLib/MpLib.c > > index 6ec9b172b8..922c87b766 100644 > > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c > > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c > > @@ -636,7 +636,7 @@ ApWakeupFunction ( > > // to initialize AP in InitConfig 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); > > + RestoreVolatileRegisters > > + (&CpuMpData->CpuData[CpuMpData->BspNumber].VolatileRegisters, > > + FALSE); > > InitializeApData (CpuMpData, ProcessorNumber, BistData, > ApTopOfStack); > > ApStartupSignalBuffer =3D > > CpuMpData->CpuData[ProcessorNumber].StartupApSignal; > > > > @@ -1615,6 +1615,7 @@ MpInitLibInitialize ( > > UINTN ApResetVectorSize; > > UINTN BackupBufferAddr; > > UINTN ApIdtBase; > > + UINT64 BspTopOfStack; > > > > OldCpuMpData =3D GetCpuMpDataFromGuidedHob (); > > if (OldCpuMpData =3D=3D NULL) { > > @@ -1677,7 +1678,7 @@ MpInitLibInitialize ( > > CpuMpData->BackupBufferSize =3D ApResetVectorSize; > > CpuMpData->WakeupBuffer =3D (UINTN) -1; > > CpuMpData->CpuCount =3D 1; > > - CpuMpData->BspNumber =3D 0; > > + CpuMpData->BspNumber =3D OldCpuMpData !=3D NULL ? > OldCpuMpData->BspNumber : 0; > > CpuMpData->WaitEvent =3D NULL; > > CpuMpData->SwitchBspFlag =3D FALSE; > > CpuMpData->CpuData =3D (CPU_AP_DATA *) (CpuMpData + 1); > > @@ -1704,11 +1705,12 @@ MpInitLibInitialize ( > > // Don't pass BSP's TR to APs to avoid AP init failure. > > // > > VolatileRegisters.Tr =3D 0; > > - CopyMem (&CpuMpData->CpuData[0].VolatileRegisters, > > &VolatileRegisters, sizeof (VolatileRegisters)); > > + CopyMem > > + (&CpuMpData->CpuData[CpuMpData->BspNumber].VolatileRegisters, > > + &VolatileRegisters, sizeof (VolatileRegisters)); > > // > > // Set BSP basic information > > // > > - InitializeApData (CpuMpData, 0, 0, CpuMpData->Buffer + > > ApStackSize); > > + BspTopOfStack =3D CpuMpData->Buffer + (CpuMpData->BspNumber + 1) > * > > + CpuMpData->CpuApStackSize; InitializeApData (CpuMpData, > > + CpuMpData->BspNumber, 0, BspTopOfStack); > > // > > // Save assembly code information > > // > > >=20 > The patch seems reasonable to me (although I have not tried verifying tha= t > all necessary spots are updated). >=20 > However, there is one thing I certainly don't understand, and the commit > message doesn't explain it. In the "BspTopOfStack" calculation, why do we > change the *second* factor, when we change the multiplication from: >=20 > (0 + 1) * ApStackSize >=20 > (where the (0 + 1) is implied in the old code), to: >=20 > (CpuMpData->BspNumber + 1) * CpuMpData->CpuApStackSize >=20 > ? >=20 > I understand why the *first* factor is changed -- we basically replace "0= " with > "CpuMpData->BspNumber" --; what I don't understand is why we replace > "ApStackSize" with "CpuMpData->CpuApStackSize", in the second factor. >=20 > ... Higher up in the code, we have: >=20 > CpuMpData->CpuApStackSize =3D ApStackSize; >=20 > so this part of the patch might actually have no effect. But, even then, = I think > it makes the patch harder to understand. So in that case, I'd suggest sti= cking > with "ApStackSize", just for keeping the patch simpler. >=20 [[Eric]] driver has two places to call InitializeApData (). Here is one and= the other in ApWakeupFunction(). InitializeApData (CpuMpData, ProcessorNumber, BistData, ApTopOfStack)= ; At that function, it calculates the ApTopOfStack like below: ApTopOfStack =3D CpuMpData->Buffer + (ProcessorNumber + 1) * CpuMpDa= ta->CpuApStackSize; I update new code to follow this coding style. I think after this change, t= he exit two code pieces are follow the same coding style. So I think we can keep my original change. Thanks, Eric > Thanks > Laszlo