From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by mx.groups.io with SMTP id smtpd.web09.435.1577084334922838205 for ; Sun, 22 Dec 2019 22:58:55 -0800 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 134.134.136.20, mailfrom: eric.dong@intel.com) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga007.jf.intel.com ([10.7.209.58]) by orsmga101.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 22 Dec 2019 22:58:54 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.69,346,1571727600"; d="scan'208,217";a="207175064" Received: from fmsmsx105.amr.corp.intel.com ([10.18.124.203]) by orsmga007.jf.intel.com with ESMTP; 22 Dec 2019 22:58:54 -0800 Received: from FMSMSX109.amr.corp.intel.com (10.18.116.9) by FMSMSX105.amr.corp.intel.com (10.18.124.203) with Microsoft SMTP Server (TLS) id 14.3.439.0; Sun, 22 Dec 2019 22:58:54 -0800 Received: from shsmsx154.ccr.corp.intel.com (10.239.6.54) by fmsmsx109.amr.corp.intel.com (10.18.116.9) with Microsoft SMTP Server (TLS) id 14.3.439.0; Sun, 22 Dec 2019 22:58:53 -0800 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.109]) by SHSMSX154.ccr.corp.intel.com ([169.254.7.71]) with mapi id 14.03.0439.000; Mon, 23 Dec 2019 14:58:51 +0800 From: "Dong, Eric" To: "Ni, Ray" , "devel@edk2.groups.io" CC: Laszlo Ersek Subject: Re: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Remove dependence between APs. Thread-Topic: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Remove dependence between APs. Thread-Index: AQHVtvc2AinzdE11/hKbpegOQFMGRKfCh8iQgAAI6aA= Date: Mon, 23 Dec 2019 06:58:51 +0000 Message-ID: References: <20191220053446.1532-1-eric.dong@intel.com> <734D49CCEBEEF84792F5B80ED585239D5C3A3FA1@SHSMSX104.ccr.corp.intel.com> In-Reply-To: <734D49CCEBEEF84792F5B80ED585239D5C3A3FA1@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: multipart/alternative; boundary="_000_ED077930C258884BBCB450DB737E662259F6BBC2shsmsx102ccrcor_" --_000_ED077930C258884BBCB450DB737E662259F6BBC2shsmsx102ccrcor_ Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Hi Ray, > -----Original Message----- > From: Ni, Ray > Sent: Friday, December 20, 2019 2:15 PM > To: Dong, Eric >; devel@e= dk2.groups.io > Cc: Laszlo Ersek > > Subject: RE: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Remove dependence > between APs. > > > > > + if (!Token->SingleAp) { > > > > + ReleaseSemaphore (&Token->FinishedApCount); > > 1. If the FinishedApCount is renamed to RunningApCount and > InterlockedDecrement() is called for it. > > SingleAp flag is unneeded. > > For StartupAllAps(), RunningApCount =3D mMaxNumberOfCpus - 1; For > StartupThisAps(), RunningApCount =3D 1; > > When RunningApCount =3D=3D 0, the spinlock is released. > [[Eric]] good idea, will update the logic. > > + if (mSmmMpSyncData->CpuData[CpuIndex].Token !=3D NULL) { > > > > + ReleaseToken (CpuIndex); > > 2. Can you directly pass in mSmmMpSyncData->CpuData[CpuIndex].Token? > It simplifies the ReleaseToken() and also make people understand that > ReleaseToken() will only modifies the Token but other states in > CpuData[Index] is NOT changed. > [[Eric]] ReleaseToken also set mSmmMpSyncData->CpuData[CpuIndex].Token to N= ULL at the end. So can't directly input Token. > > > > @@ -1170,10 +1120,12 @@ CreateToken ( > > 3. With the comment #1, CreateToken() can carry additional parameter whic= h > specifies the RunningApCount. > [[Eric]] yes, will update the logic. > > ASSERT (ProcToken !=3D NULL); > > > > ProcToken->Signature =3D PROCEDURE_TOKEN_SIGNATURE; > > > > ProcToken->ProcedureToken =3D CpuToken; > > 4. ProcToken->ProcedureToken looks a bit strange. > Can we use "ProcToken->Spinlock"? [[Eric]] yes, will update the name. Thanks, Eric > > > > > + *Token =3D (MM_COMPLETION) mSmmMpSyncData- > > >CpuData[CpuIndex].Token->ProcedureToken; > > 5. It will become > *Token =3D (MM_COMPLETION) mSmmMpSyncData- > >CpuData[CpuIndex].Token->Spinlock; > > > > > + ReleaseSemaphore (&ProcToken->FinishedApCount); > > 6. I can now understand why "FinishedApCount is directly compared against > mMaxNumberOfCpus because the FinishedApCount is already increased for > BSP. It's not a comment for code change. > > > Thanks, > Ray --_000_ED077930C258884BBCB450DB737E662259F6BBC2shsmsx102ccrcor_ Content-Type: text/html; charset="us-ascii" Content-Transfer-Encoding: quoted-printable

Hi Ray,

> -----Original Message-----
> From: Ni, Ray
> Sent: Friday, December&nbs= p;20, 2019 2:15 PM
> To: Dong, Eric <eric.dong@intel.com>; devel@edk2.groups.io
> Cc: Laszlo Ersek <= lersek@redhat.com>
> Subject: RE: [PATCH] = UefiCpuPkg/PiSmmCpuDxeSmm: Remove dependence
> between APs.
> 
> >
> > +  if (!T= oken->SingleAp) {
> >
> > +    = ;ReleaseSemaphore (&Token->FinishedApCount);
> 
> 1. If the FinishedApC= ount is renamed to RunningApCount and
> InterlockedDecrement() is = called for it.
> 
> SingleAp flag is unne= eded.
> 
> For StartupAllAps(), Runni= ngApCount =3D mMaxNumberOfCpus - 1; For
> StartupThisAps(), RunningApCoun= t =3D 1;
> 
> When RunningApCount =3D=3D=  0, the spinlock is released.
> 
[[Eric]] good idea, will up= date the logic.


> > +    = ;if (mSmmMpSyncData->CpuData[CpuIndex].Token !=3D NULL)&n= bsp;{
> >
> > +    = ;  ReleaseToken (CpuIndex);
> 
> 2. Can you directly&n= bsp;pass in mSmmMpSyncData->CpuData[CpuIndex].Token? > It simplifies the Rel= easeToken() and also make people understand t= hat
> ReleaseToken() will only&n= bsp;modifies the Token but other states in
> CpuData[Index] is NOT = ;changed.
> 
[[Eric]] ReleaseToken also set&n= bsp;mSmmMpSyncData->CpuData[CpuIndex].Token to NULL
at the end. So can't d= irectly input Token.

> >
> > @@ -1170,10 +1= 120,12 @@ CreateToken (
> 
> 3. With the comment&n= bsp;#1, CreateToken() can carry additional paramet= er which
> specifies the RunningApCou= nt.
> 
[[Eric]] yes, will update t= he logic.

> >    ASSERT&nbs= p;(ProcToken !=3D NULL);
> >
> >    ProcToken-= >Signature =3D PROCEDURE_TOKEN_SIGNATURE;
> >
> >    ProcToken-= >ProcedureToken =3D CpuToken;
> 
> 4. ProcToken->ProcedureToken=  looks a bit strange.
> Can we use "Proc= Token->Spinlock"?
[[Eric]] yes, will update t= he name.

Thanks,
Eric
> 
> >
> > +    = ;*Token =3D (MM_COMPLETION) mSmmMpSyncData-
> > >CpuData[CpuIndex].Token= ->ProcedureToken;
> 
> 5. It will become
> *Token =3D (MM_COMPLETION)=  mSmmMpSyncData-
> >CpuData[CpuIndex].Token->Spinlo= ck;
> 
> >
> > +    = ;  ReleaseSemaphore (&ProcToken->FinishedApCount);
> 
> 6. I can now und= erstand why "FinishedApCount is directly comp= ared against
> mMaxNumberOfCpus because t= he FinishedApCount is already increased for=
> BSP. It's not a = comment for code change.
> 
> 
> Thanks,
> Ray

--_000_ED077930C258884BBCB450DB737E662259F6BBC2shsmsx102ccrcor_--