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.4184.1574996528752414449 for ; Thu, 28 Nov 2019 19:02:08 -0800 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 134.134.136.24, mailfrom: eric.dong@intel.com) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga003.jf.intel.com ([10.7.209.27]) by orsmga102.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 28 Nov 2019 19:02:08 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.69,255,1571727600"; d="scan'208,217";a="212163437" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by orsmga003.jf.intel.com with ESMTP; 28 Nov 2019 19:02:07 -0800 Received: from fmsmsx111.amr.corp.intel.com (10.18.116.5) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.439.0; Thu, 28 Nov 2019 19:02:07 -0800 Received: from shsmsx107.ccr.corp.intel.com (10.239.4.96) by fmsmsx111.amr.corp.intel.com (10.18.116.5) with Microsoft SMTP Server (TLS) id 14.3.439.0; Thu, 28 Nov 2019 19:02:06 -0800 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.108]) by SHSMSX107.ccr.corp.intel.com ([169.254.9.63]) with mapi id 14.03.0439.000; Fri, 29 Nov 2019 11:02:05 +0800 From: "Dong, Eric" To: "devel@edk2.groups.io" , "lersek@redhat.com" CC: "Ni, Ray" , "Gao, Liming" Subject: Re: [edk2-devel] [PATCH v2] UefiCpuPkg/PiSmmCpuDxeSmm: Avoid allocate Token every time. Thread-Topic: [edk2-devel] [PATCH v2] UefiCpuPkg/PiSmmCpuDxeSmm: Avoid allocate Token every time. Thread-Index: AQHVpfO/hNyhzSbZ/keB9lij2XaGfKehXNHg Date: Fri, 29 Nov 2019 03:02:04 +0000 Message-ID: References: <20191128061716.196-1-eric.dong@intel.com> <7ac1f712-5da3-e6b2-f24e-936feb81daa3@redhat.com> In-Reply-To: <7ac1f712-5da3-e6b2-f24e-936feb81daa3@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: multipart/alternative; boundary="_000_ED077930C258884BBCB450DB737E662259F4608Ashsmsx102ccrcor_" --_000_ED077930C258884BBCB450DB737E662259F4608Ashsmsx102ccrcor_ Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Hi Laszlo, From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Lasz= lo Ersek Sent: Thursday, November 28, 2019 9:57 PM To: Dong, Eric ; devel@edk2.groups.io Cc: Ni, Ray ; Gao, Liming Subject: Re: [edk2-devel] [PATCH v2] UefiCpuPkg/PiSmmCpuDxeSmm: Avoid allo= cate Token every time. On 11/28/19 07:17, Eric Dong wrote: > v2 changes: > Minor update based on comments. > > v1 changes: > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D2388 > > Current logic allocate Token every time when need to use it. (1) Can you please clarify, in the *commit message*, what service the token allocation is needed for? To me it seems to be related to the SMM MP protocol, which was added for . Is that correct? [Eric] yes, it's a SMI latency issue occurred only in some special case. = It's a regression issue caused by SMM MP protocol change. (For example, the CreateToken() function was introduced in commit 51dd408ae102, "UefiCpuPkg/PiSmmCpuDxeSmm: Enable MM MP Protocol", 2019-07-16.) (2) If the problem indeed depends on MM MP protocol usage, then please update Bugzilla 2388 too, with that information ("performance regression from BZ#1937"). If a platform does not use the MM MP protocol, then highlighting this information helps platforms determine that they are not affected. (I read Bugzilla 2388 soon after it was filed, and I was surprised, because I didn't remember any "human visible" SMI handling slowdown. Maybe it's not on the "human visible" scale; I'm not sure.) [Eric] Yes, it's a special case. I check the basic SMI (write data to B2 a= nd collect the performance data) latency data and not found this issue. I found you already add note in this bugz. Thanks. > The logic > caused SMI latency raised to very high. Update logic to allocate Token > buffer at driver's entry point. Later use the token from the allocated > token buffer. Only when all the buffer have been used, then need to > allocate new buffer. > > Signed-off-by: Eric Dong > > Cc: Ray Ni > > Cc: Laszlo Ersek > > --- > UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 56 ++++++++++++++++++++-- > UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 16 +++++++ > 2 files changed, 68 insertions(+), 4 deletions(-) commenting on the header file changes first: [Eric] what's this sentence means? Follow above comments to update the com= ment message? > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h b/UefiCpuPkg/PiS= mmCpuDxeSmm/PiSmmCpuDxeSmm.h > index 8c29f1a558..36fd0dae92 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > @@ -198,6 +198,7 @@ typedef UINT32 SMM_CPU_= ARRIVAL_EXCEPTIONS; > #define ARRIVAL_EXCEPTION_DELAYED 0x2 > #define ARRIVAL_EXCEPTION_SMI_DISABLED 0x4 > > +#define MAX_PRE_RESERVE_TOKEN_COUNT 0x512 (3) This is a very strange constant, 0x512: it's decimal 1298. Is that your intent? [Eric] add "0x" by mistake, will use PCD and remove this code. (4) Should we make this a global (SMRAM) variable instead, and initialize it from a PCD? [Eric] yes, will use PCD for this value. I'm not sure how large tokens are, but if a platform has no use for the [Eric] The value is 0x40 for the platform I used. MM MP protocol, then pre-allocating tokens for the MM MP protocol just wastes SMRAM. If I grep edk2 (at bd85bf54c268), and edk2-platforms (at 04889ec1198b), for "gEfiMmMpProtocolGuid", the only reported module is "UefiCpuPkg/PiSmmCpuDxeSmm" itself. In other words, there is no open source consumer for the new protocol. I don't think we should waste SMRAM on platforms that don't consume this protocol. Furthermore, I've applied the following patch, and rebuilt OVMF: > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c b/UefiCpuPkg/PiSmmCpu= DxeSmm/MpService.c > index 0685637c2b51..5874195ba96e 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > @@ -1119,6 +1119,8 @@ CreateToken ( > SPIN_LOCK *CpuToken; > UINTN SpinLockSize; > > + ASSERT (FALSE); > + > SpinLockSize =3D GetSpinLockProperties (); > CpuToken =3D AllocatePool (SpinLockSize); > ASSERT (CpuToken !=3D NULL); then I've re-run my usual tests. The ASSERT() has not been reached, and so I think that the new SMRAM allocation would be pure loss (=3D unused) for OVMF. I suggest that we introduce a new PCD for "token count per chunk", and set the default value to 1, in the DEC file. (And we should copy the PCD into a global variable, in InitializeDataForMmMp().) This way, if a platform suddenly starts consuming tokens, it will work (there will always be a single available token). If they want to improve performance (using a chunked allocation style), they can set the PCD as they see fit. Back to the patch: On 11/28/19 07:17, Eric Dong wrote: > // > // Wrapper used to convert EFI_AP_PROCEDURE2 and EFI_AP_PROCEDURE. > // > @@ -217,6 +218,17 @@ typedef struct { > > #define PROCEDURE_TOKEN_FROM_LINK(a) CR (a, PROCEDURE_TOKEN, Link, PRO= CEDURE_TOKEN_SIGNATURE) > > +#define TOKEN_BUFFER_SIGNATURE SIGNATURE_32 ('T', 'K', 'B', 'S') > + > +typedef struct { > + UINTN Signature; > + LIST_ENTRY Link; > + > + UINT8 *Buffer; > +} TOKEN_BUFFER; > + > +#define TOKEN_BUFFER_FROM_LINK(a) CR (a, TOKEN_BUFFER, Link, TOKEN_BUF= FER_SIGNATURE) > + > // > // Private structure for the SMM CPU module that is stored in DXE Runti= me memory > // Contains the SMM Configuration Protocols that is produced. > @@ -243,6 +255,10 @@ typedef struct { > PROCEDURE_WRAPPER *ApWrapperFunc; > LIST_ENTRY TokenList; > > + LIST_ENTRY OldTokenBufList; > + > + UINT8 *CurrentTokenBuf; > + UINTN UsedTokenNum; // Only record toke= ns used in CurrentTokenBuf. > } SMM_CPU_PRIVATE_DATA; > > extern SMM_CPU_PRIVATE_DATA *gSmmCpuPrivate; > (5) There are two new lines above that are overlong. Did "PatchCheck.py" not complain? [Eric] I think PatchCheck tool not check the code width, only check the co= mmit message width. Also edk2 coding style spec not has restrict rule for it, just a general r= ule. Content like below: 5.1 General Rules 5.1.1 Lines shall be 120 columns, or less https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Specification= s > > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c b/UefiCpuPkg/PiSmmCpu= DxeSmm/MpService.c > index d8d2b6f444..4632e5b0c2 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > @@ -492,6 +492,23 @@ FreeTokens ( > { > LIST_ENTRY *Link; > PROCEDURE_TOKEN *ProcToken; > + TOKEN_BUFFER *TokenBuf; > + > + // > + // Not free the buffer, just clear the UsedTokenNum. In order to > + // avoid later trig allcate action again when need to use token. > + // > + gSmmCpuPrivate->UsedTokenNum =3D 0; (6) Here we do not zero out the current token buffer, but in CreateToken() and InitializeDataForMmMp(), we use AllocateZeroPool(). This is an inconsistency, we should call either ZeroMem() here (if zeroing matters), or AllocatePool() in the other two places (if zeroing does not matter). [Eric] Not catch your meaning here? Why can't use "=3D0" here? > + > + Link =3D GetFirstNode (&gSmmCpuPrivate->OldTokenBufList); > + while (!IsNull (&gSmmCpuPrivate->OldTokenBufList, Link)) { > + TokenBuf =3D TOKEN_BUFFER_FROM_LINK (Link); > + > + Link =3D RemoveEntryList (&TokenBuf->Link); > + > + FreePool (TokenBuf->Buffer); > + FreePool (TokenBuf); > + } > > while (!IsListEmpty (&gSmmCpuPrivate->TokenList)) { > Link =3D GetFirstNode (&gSmmCpuPrivate->TokenList); > @@ -499,7 +516,6 @@ FreeTokens ( > > RemoveEntryList (&ProcToken->Link); > > - FreePool ((VOID *)ProcToken->ProcedureToken); > FreePool (ProcToken); > } > } > @@ -1115,13 +1131,35 @@ CreateToken ( > VOID > ) > { > - PROCEDURE_TOKEN *ProcToken; > + PROCEDURE_TOKEN *ProcToken; > SPIN_LOCK *CpuToken; > UINTN SpinLockSize; > + TOKEN_BUFFER *TokenBuf; > > SpinLockSize =3D GetSpinLockProperties (); > - CpuToken =3D AllocatePool (SpinLockSize); > - ASSERT (CpuToken !=3D NULL); > + > + if (gSmmCpuPrivate->UsedTokenNum =3D=3D MAX_PRE_RESERVE_TOKEN_COUNT) = { > + DEBUG((DEBUG_INFO, "CpuSmm: No free token buffer, allocate new buff= er!\n")); (7) This is an expected case, and not too much a corner case at that. Furthermore, the DEBUG message is in a performance-sensitive path. [Eric] this code is called by the caller. I don't think it's performance = sensitive. What's your rule for "performance-sensitive path" ? I add this debug message because I= want to know how often the pre allocate buffer is not enough. We can enlarge the buffe= r size to get better performance. Therefore, the message should be logged with the DEBUG_VERBOSE mask, and not with the DEBUG_INFO mask. (8) In addition, the DEBUG line is too long. [Eric] Same with [5], I think not break edk2 rules. (9) The DEBUG line also misses a space character. [Eric] will update in next version. > + > + // > + // Record current token buffer for later free action usage. > + // Current used token buffer not in this list. > + // > + TokenBuf =3D AllocatePool (sizeof (TOKEN_BUFFER)); > + ASSERT (TokenBuf !=3D NULL); > + TokenBuf->Signature =3D TOKEN_BUFFER_SIGNATURE; > + TokenBuf->Buffer =3D gSmmCpuPrivate->CurrentTokenBuf; > + > + InsertTailList (&gSmmCpuPrivate->OldTokenBufList, &TokenBuf->Link); > + > + gSmmCpuPrivate->CurrentTokenBuf =3D AllocateZeroPool (SpinLockSiz= e * MAX_PRE_RESERVE_TOKEN_COUNT); > + ASSERT (gSmmCpuPrivate->CurrentTokenBuf !=3D NULL); > + gSmmCpuPrivate->UsedTokenNum =3D 0; > + } > + > + CpuToken =3D (SPIN_LOCK *)(gSmmCpuPrivate->CurrentTokenBuf + SpinLock= Size * gSmmCpuPrivate->UsedTokenNum); (10) Two lines are overlong. [Eric] Same with [5], I think not break edk2 rules. > + gSmmCpuPrivate->UsedTokenNum++; > + > InitializeSpinLock (CpuToken); > AcquireSpinLock (CpuToken); > > @@ -1737,10 +1775,20 @@ InitializeDataForMmMp ( > VOID > ) > { > + UINTN SpinLockSize; > + > + SpinLockSize =3D GetSpinLockProperties (); > + DEBUG((DEBUG_INFO, "CpuSmm: SpinLock Size =3D 0x%x\n", SpinLockSize))= ; (11) For consistency with (7), you might want to downgrade this to DEBUG_VERBOSE too. But, in this case, I won't insist. [Eric] This value impact the final used buffer size, want to know this inf= o in normal boot. So I use INFO level. (12) Space character missing after "DEBUG". [Eric] Will update it in next patch. (13) Are you proposing this patch for edk2-stable201911? (CC Liming) [Eric] it's too later, will not propose in this stable tag. Thanks Laszlo > + > + gSmmCpuPrivate->CurrentTokenBuf =3D AllocateZeroPool (SpinLockSize * = MAX_PRE_RESERVE_TOKEN_COUNT); > + ASSERT (gSmmCpuPrivate->CurrentTokenBuf !=3D NULL); > + gSmmCpuPrivate->UsedTokenNum =3D 0; > + > gSmmCpuPrivate->ApWrapperFunc =3D AllocatePool (sizeof (PROCEDURE_WRA= PPER) * gSmmCpuPrivate->SmmCoreEntryContext.NumberOfCpus); > ASSERT (gSmmCpuPrivate->ApWrapperFunc !=3D NULL); > > InitializeListHead (&gSmmCpuPrivate->TokenList); > + InitializeListHead (&gSmmCpuPrivate->OldTokenBufList); > } > > /** --_000_ED077930C258884BBCB450DB737E662259F4608Ashsmsx102ccrcor_ Content-Type: text/html; charset="us-ascii" Content-Transfer-Encoding: quoted-printable

Hi Laszlo,

 

From: devel@edk2.groups.io [mailto:= devel@edk2.groups.io] On Behalf Of Laszlo Ersek
Sent: Thursday, November 28, 2019 9:57 PM
To: Dong, Eric <eric.dong@intel.com>; devel@edk2.groups.io Cc: Ni, Ray <ray.ni@intel.com>; Gao, Liming <liming.gao@in= tel.com>
Subject: Re: [edk2-devel] [PATCH v2] UefiCpuPkg/PiSmmCpuDxeSmm: Avo= id allocate Token every time.

 

On 11/28/19 07:17, Eri= c Dong wrote:
> v2 changes:
>   Minor update = based on comments.
>
> v1 changes:
> REF: https://bugzilla.ti= anocore.org/show_bug.cgi?id=3D2388
>
> Current logic allocate&nb= sp;Token every time when need to use it.=

(1) Can you please clarify= , in the *commit message*, what service = the
token allocation is needed = ;for?

To me it seems to be&= nbsp;related to the SMM MP protocol, which&nb= sp;was added for
<https://bugzilla.tianocore.org/show_= bug.cgi?id=3D1937>. Is that corre= ct?
[Eric] yes, it’s a SMI latency is= sue occurred only in some special case.  It’s a regression issue= caused by SMM MP protocol change.

 


(For example, the CreateToken()=  function was introduced in commit
51dd408ae102, "UefiCpuPkg/PiSmmCpuDx= eSmm: Enable MM MP Protocol",
2019-07-16.)


(2) If the problem indeed&= nbsp;depends on MM MP protocol usage, then&nb= sp;please
update Bugzilla 2388 too, = with that information ("performance regression
from BZ#1937"). If a = platform does not use the MM MP protocol= , then
highlighting this information h= elps platforms determine that they are
not affected.

(I read Bugzilla 2388 soon=  after it was filed, and I was surp= rised,
because I didn't remember = any "human visible" SMI handling slowdow= n.

Maybe it's not on the = ;"human visible" scale; I'm not sure.)

[Eric] Yes, it’s a special cas= e. I check the basic SMI (write data to B2 and collect the performance data) latency data and not found this issue.

I found you already add note in this= bugz. Thanks.

>       = ;            &n= bsp;            = ;            &n= bsp;            = ;     The logic
> caused SMI latency r= aised to very high. Update logic to allo= cate Token
> buffer at driver's e= ntry point. Later use the token from the=  allocated
> token buffer. Only w= hen all the buffer have been used, then&= nbsp;need to
> allocate new buffer.
>
> Signed-off-by: Eric Dong&= nbsp;<eric.dong= @intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <= ;lersek@redhat.com>
> ---
>  UefiCpuPkg/PiSmmCpuDxeSmm/MpS= ervice.c      | 56 +++&= #43;++++++++++++++&= #43;+--
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiS= mmCpuDxeSmm.h | 16 +++++++
>  2 files changed,&nb= sp;68 insertions(+), 4 deletions(-)

commenting on the header f= ile changes first:=

[Eric] what’s this sentence me= ans? Follow above comments to update the comment message?=



> diff --git a/UefiCpuPkg/P= iSmmCpuDxeSmm/PiSmmCpuDxeSmm.h b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxe= Smm.h
> index 8c29f1a558..36fd0dae92&n= bsp;100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSm= m/PiSmmCpuDxeSmm.h
> +++ b/UefiCpuPkg/P= iSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> @@ -198,6 +198,7 = ;@@ typedef UINT32        = ;            &n= bsp;         SMM_CPU_ARRIVAL_E= XCEPTIONS;
>  #define ARRIVAL_EXCEPTIO= N_DELAYED           = 0x2
>  #define ARRIVAL_EXCEPTIO= N_SMI_DISABLED      0x4
>
> +#define MAX_PRE_RESERVE_T= OKEN_COUNT           = ;          0x512
(3) This is a very st= range constant, 0x512: it's decimal 1298. Is&= nbsp;that
your intent?
[Eric] add “0x” by mistake, will= use PCD and remove this code.



(4) Should we make this&nb= sp;a global (SMRAM) variable instead, and initialize it from a PCD?<= /span>

[Eric] yes, will use PCD for this va= lue.



I'm not sure how large&nbs= p;tokens are, but if a platform has no&n= bsp;use for the

[Eric] The value is 0x40 for the pla= tform I used.


MM MP protocol, then pre-a= llocating tokens for the MM MP protocol = just
wastes SMRAM.

If I grep edk2 (at bd= 85bf54c268), and edk2-platforms (at 04889ec1198b),
for "gEfiMmMpProtocolGuid",&nbs= p;the only reported module is
"UefiCpuPkg/PiSmmCpuDxeSmm" its= elf. In other words, there is no open
source consumer for the ne= w protocol. I don't think we should wast= e
SMRAM on platforms that do= n't consume this protocol.

Furthermore, I've applied the&n= bsp;following patch, and rebuilt OVMF:

> diff --git a/UefiCpuPkg/P= iSmmCpuDxeSmm/MpService.c b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> index 0685637c2b51..5874195ba9= 6e 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSm= m/MpService.c
> +++ b/UefiCpuPkg/P= iSmmCpuDxeSmm/MpService.c
> @@ -1119,6 +1119,8&nb= sp;@@ CreateToken (
>    SPIN_LOCK &n= bsp;         *CpuToken;=
>    UINTN  =             &nb= sp;SpinLockSize;
>
> +  ASSERT (FALSE= );
> +
>    SpinLockSize = ;=3D GetSpinLockProperties ();
>    CpuToken =3D=  AllocatePool (SpinLockSize);
>    ASSERT (CpuT= oken !=3D NULL);

then I've re-run my usual&= nbsp;tests. The ASSERT() has not been reached= , and
so I think that the n= ew SMRAM allocation would be pure loss (= = =3D unused)
for OVMF.

I suggest that we introduc= e a new PCD for "token count per&nb= sp;chunk", and
set the default value to&n= bsp;1, in the DEC file. (And we should&n= bsp;copy the PCD
into a global variable, in=  InitializeDataForMmMp().)

This way, if a platform&nb= sp;suddenly starts consuming tokens, it will = work
(there will always be a&nb= sp;single available token). If they want to&n= bsp;improve
performance (using a chunked&nb= sp;allocation style), they can set the PCD&nb= sp;as
they see fit.

Back to the patch:

On 11/28/19 07:17, Eric D= ong wrote:
>  //
>  // Wrapper used&nbs= p;to convert EFI_AP_PROCEDURE2 and EFI_AP_PROCEDURE.
>  //
> @@ -217,6 +218,17&nbs= p;@@ typedef struct {
>
>  #define PROCEDURE_TOKEN_= FROM_LINK(a)  CR (a, PROCEDURE_TOKEN, Link, P= ROCEDURE_TOKEN_SIGNATURE)
>
> +#define TOKEN_BUFFER_SIGN= ATURE  SIGNATURE_32 ('T', 'K', 'B', 'S')
> +
> +typedef struct {
> +  UINTN  &= nbsp;           &nbs= p;    Signature;
> +  LIST_ENTRY &n= bsp;            = ;Link;
> +
> +  UINT8  &= nbsp;           &nbs= p;    *Buffer;
> +} TOKEN_BUFFER; > +
> +#define TOKEN_BUFFER_FROM= _LINK(a)  CR (a, TOKEN_BUFFER, Link, TOKEN_BU= FFER_SIGNATURE)
> +
>  //
>  // Private structur= e for the SMM CPU module that is st= ored in DXE Runtime memory
>  // Contains the&nbs= p;SMM Configuration Protocols that is produced.
> @@ -243,6 +255,10&nbs= p;@@ typedef struct {
>    PROCEDURE_WRAPPER=             &nb= sp;  *ApWrapperFunc;
>    LIST_ENTRY &= nbsp;           &nbs= p;        TokenList;
>
> +  LIST_ENTRY &n= bsp;            = ;        OldTokenBufList; > +
> +  UINT8  &= nbsp;           &nbs= p;            *= CurrentTokenBuf;
> +  UINTN  &= nbsp;           &nbs= p;            U= sedTokenNum;     // Only record tok= ens used in CurrentTokenBuf.
>  } SMM_CPU_PRIVATE_DATA;<= /span>
>
>  extern SMM_CPU_PRIVATE_D= ATA  *gSmmCpuPrivate;
>

(5) There are two new = ;lines above that are overlong.
Did "PatchCheck.py" not&nb= sp;complain?

[Eric] I think PatchCheck tool not c= heck the code width, only check the commit message width.

Also edk2 coding style spec not has = restrict rule for it, just a general rule. Content like below:

5.1 General Rules

5.1.1 Lines shall be 120 columns, or= less

https://github.com/tianocore/tianocore.github.i= o/wiki/EDK-II-Specifications



>
> diff --git a/UefiCpuPkg/P= iSmmCpuDxeSmm/MpService.c b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> index d8d2b6f444..4632e5b0c2&n= bsp;100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSm= m/MpService.c
> +++ b/UefiCpuPkg/P= iSmmCpuDxeSmm/MpService.c
> @@ -492,6 +492,23&nbs= p;@@ FreeTokens (
>  {
>    LIST_ENTRY &= nbsp;          *Link;
>    PROCEDURE_TOKEN&n= bsp;      *ProcToken;
> +  TOKEN_BUFFER =          *TokenBuf;
> +
> +  //
> +  // Not f= ree the buffer, just clear the UsedTokenNum.&= nbsp;In order to
> +  // avoid = ;later trig allcate action again when need&nb= sp;to use token.
> +  //
> +  gSmmCpuPrivate->= ;UsedTokenNum =3D 0;

(6) Here we do not ze= ro out the current token buffer, but in<= /span>
CreateToken() and InitializeDataForM= mMp(), we use AllocateZeroPool().

This is an inconsistency, = we should call either ZeroMem() here (if
zeroing matters), or AllocatePo= ol() in the other two places (if zeroing=
does not matter).<= /span>

[Eric] Not catch your meaning here? = Why can’t use  “=3D0” here?


> +
> +  Link =3D = ;GetFirstNode (&gSmmCpuPrivate->OldTokenBufList);
> +  while (!IsNul= l (&gSmmCpuPrivate->OldTokenBufList, Link)) {<= br> > +    TokenB= uf =3D TOKEN_BUFFER_FROM_LINK (Link);
> +
> +    Link&n= bsp;=3D RemoveEntryList (&TokenBuf->Link);
> +
> +    FreePo= ol (TokenBuf->Buffer);
> +    FreePo= ol (TokenBuf);
> +  }
>
>    while (!IsLi= stEmpty (&gSmmCpuPrivate->TokenList)) {
>      Link&= nbsp;=3D GetFirstNode (&gSmmCpuPrivate->TokenList);=
> @@ -499,7 +516,6 = ;@@ FreeTokens (
>
>      Remov= eEntryList (&ProcToken->Link);
>
> -    FreePool&n= bsp;((VOID *)ProcToken->ProcedureToken);
>      FreeP= ool (ProcToken);
>    }
>  }
> @@ -1115,13 +1131,35&= nbsp;@@ CreateToken (
>    VOID
>    )
>  {
> -  PROCEDURE_TOKEN &= nbsp;  *ProcToken;
> +  PROCEDURE_TOKEN&nb= sp;    *ProcToken;
>    SPIN_LOCK &n= bsp;         *CpuToken;=
>    UINTN  =             &nb= sp;SpinLockSize;
> +  TOKEN_BUFFER =        *TokenBuf;
>
>    SpinLockSize = ;=3D GetSpinLockProperties ();
> -  CpuToken =3D = ;AllocatePool (SpinLockSize);
> -  ASSERT (CpuToken&= nbsp;!=3D NULL);
> +
> +  if (gSmmCpuPr= ivate->UsedTokenNum =3D=3D MAX_PRE_RESERVE_TOKEN_COUNT) {=
> +    DEBUG(= (DEBUG_INFO, "CpuSmm: No free token buffer,&n= bsp;allocate new buffer!\n"));

(7) This is an expected&nb= sp;case, and not too much a corner case&= nbsp;at that.
Furthermore, the DEBUG message&= nbsp;is in a performance-sensitive path.

[Eric] this code is called by the ca= ller.  I don’t think it’s performance sensitive. What’s your

rule for “performance-sensitiv= e path” ? I add this debug message because I want to know

how often the pre allocate buffer is= not enough.  We can enlarge the buffer size to get

better performance.


Therefore, the message should&n= bsp;be logged with the DEBUG_VERBOSE mask, an= d
not with the DEBUG_INFO ma= sk.

(8) In addition, the DEBUG=  line is too long.

[Eric] Same with [5], I think not br= eak edk2 rules.


(9) The DEBUG line also&nb= sp;misses a space character.

[Eric] will update  in next ver= sion.


> +
> +    //
> +    //&nbs= p;Record current token buffer for later free&= nbsp;action usage.
> +    //&nbs= p;Current used token buffer not in this = list.
> +    //
> +    TokenB= uf =3D AllocatePool (sizeof (TOKEN_BUFFER));
> +    ASSERT=  (TokenBuf !=3D NULL);
> +    TokenB= uf->Signature =3D TOKEN_BUFFER_SIGNATURE;
> +    TokenB= uf->Buffer  =3D gSmmCpuPrivate->CurrentTokenBuf;
> +
> +    Insert= TailList (&gSmmCpuPrivate->OldTokenBufList, &TokenBuf-= >Link);
> +
> +    gSmmCp= uPrivate->CurrentTokenBuf   =3D AllocateZeroPool&nbs= p;(SpinLockSize * MAX_PRE_RESERVE_TOKEN_COUNT);
> +    ASSERT=  (gSmmCpuPrivate->CurrentTokenBuf !=3D NULL);
> +    gSmmCp= uPrivate->UsedTokenNum =3D 0;
> +  }
> +
> +  CpuToken =3D&= nbsp;(SPIN_LOCK *)(gSmmCpuPrivate->CurrentTokenBuf + = SpinLockSize * gSmmCpuPrivate->UsedTokenNum);

(10) Two lines are overlon= g.
[Eric] Same with [5], I think not break edk2= rules.



> +  gSmmCpuPrivate->= ;UsedTokenNum++;
> +
>    InitializeSpinLoc= k (CpuToken);
>    AcquireSpinLock&n= bsp;(CpuToken);
>
> @@ -1737,10 +1775,20&= nbsp;@@ InitializeDataForMmMp (
>    VOID
>    )
>  {
> +  UINTN  &= nbsp;           Spin= LockSize;
> +
> +  SpinLockSize = = =3D GetSpinLockProperties ();
> +  DEBUG((DEBUG_INFO,=  "CpuSmm: SpinLock Size =3D 0x%x\n",&nbs= p;SpinLockSize));

(11) For consistency with = (7), you might want to downgrade this to=
DEBUG_VERBOSE too. But, in = ;this case, I won't insist.
[Eric] This value impact the final used buff= er size, want to know this info in normal boot. So I use INFO level.


(12) Space character missing&nb= sp;after "DEBUG".

[Eric] Will update it in next patch.=

(13) Are you proposing thi= s patch for edk2-stable201911? (CC Liming)<= br> [Eric] it’s too later, will not propos= e in this stable tag.


Thanks
Laszlo


> +
> +  gSmmCpuPrivate->= ;CurrentTokenBuf =3D AllocateZeroPool (SpinLockSize *&n= bsp;MAX_PRE_RESERVE_TOKEN_COUNT);
> +  ASSERT (gSmmC= puPrivate->CurrentTokenBuf !=3D NULL);
> +  gSmmCpuPrivate->= ;UsedTokenNum =3D 0;
> +
>    gSmmCpuPrivate-&g= t;ApWrapperFunc =3D AllocatePool (sizeof (PROCEDURE_WRA= PPER) * gSmmCpuPrivate->SmmCoreEntryContext.NumberOfCpus);
>    ASSERT (gSmm= CpuPrivate->ApWrapperFunc !=3D NULL);
>
>    InitializeListHea= d (&gSmmCpuPrivate->TokenList);
> +  InitializeListHead=  (&gSmmCpuPrivate->OldTokenBufList);
>  }
>
>  /**


--_000_ED077930C258884BBCB450DB737E662259F4608Ashsmsx102ccrcor_--