From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by mx.groups.io with SMTP id smtpd.web09.1979.1575505513586388604 for ; Wed, 04 Dec 2019 16:25:13 -0800 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 192.55.52.88, mailfrom: eric.dong@intel.com) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 04 Dec 2019 16:25:13 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.69,279,1571727600"; d="scan'208";a="236483985" Received: from fmsmsx107.amr.corp.intel.com ([10.18.124.205]) by fmsmga004.fm.intel.com with ESMTP; 04 Dec 2019 16:25:13 -0800 Received: from fmsmsx118.amr.corp.intel.com (10.18.116.18) by fmsmsx107.amr.corp.intel.com (10.18.124.205) with Microsoft SMTP Server (TLS) id 14.3.439.0; Wed, 4 Dec 2019 16:25:12 -0800 Received: from shsmsx153.ccr.corp.intel.com (10.239.6.53) by fmsmsx118.amr.corp.intel.com (10.18.116.18) with Microsoft SMTP Server (TLS) id 14.3.439.0; Wed, 4 Dec 2019 16:25:12 -0800 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.109]) by SHSMSX153.ccr.corp.intel.com ([169.254.12.195]) with mapi id 14.03.0439.000; Thu, 5 Dec 2019 08:25:07 +0800 From: "Dong, Eric" To: Laszlo Ersek , "devel@edk2.groups.io" CC: "Ni, Ray" Subject: Re: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Avoid allocate Token every time Thread-Topic: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Avoid allocate Token every time Thread-Index: AQHVqoQbU+nHSfNzlUqAiiWdRa/j8Keqreog Date: Thu, 5 Dec 2019 00:25:07 +0000 Message-ID: References: <20191204080507.1316-1-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 Hi Laszlo, > -----Original Message----- > From: Laszlo Ersek [mailto:lersek@redhat.com] > Sent: Wednesday, December 4, 2019 5:20 PM > To: Dong, Eric ; devel@edk2.groups.io > Cc: Ni, Ray > Subject: Re: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Avoid allocate Token > every time >=20 > Hi Eric, >=20 > I have three comments: >=20 > On 12/04/19 09:05, Eric Dong wrote: > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D2388 > > > > Token is new introduced by MM MP Protocol. Current logic allocate > > Token every time when need to use it. 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 > > --- > > V3 changes: > > Introduce PCD to control the pre allocated toke buffer size. > > > > v2 changes: > > Minor update based on comments. > > > > UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 67 > ++++++++++++++++++-- > > UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 15 +++++ > > UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf | 1 + > > UefiCpuPkg/UefiCpuPkg.dec | 4 ++ > > UefiCpuPkg/UefiCpuPkg.uni | 1 + > > 5 files changed, 84 insertions(+), 4 deletions(-) > > > > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > > b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > > index d8d2b6f444..33aad3f3e9 100644 > > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > > @@ -492,6 +492,24 @@ FreeTokens ( > > { > > LIST_ENTRY *Link; > > PROCEDURE_TOKEN *ProcToken; > > + TOKEN_BUFFER *TokenBuf; > > + > > + // > > + // Not free the buffer, just clear the UsedTokenNum. In order to > > + // avoid triggering allocate action when we need to use the token, > > + // do not free the buffer. > > + // > > + gSmmCpuPrivate->UsedTokenNum =3D 0; > > + > > + 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 > > +517,6 @@ FreeTokens ( > > > > RemoveEntryList (&ProcToken->Link); > > > > - FreePool ((VOID *)ProcToken->ProcedureToken); > > FreePool (ProcToken); > > } > > } > > @@ -1115,13 +1132,37 @@ CreateToken ( > > VOID > > ) > > { > > - PROCEDURE_TOKEN *ProcToken; > > + PROCEDURE_TOKEN *ProcToken; > > SPIN_LOCK *CpuToken; > > UINTN SpinLockSize; > > + TOKEN_BUFFER *TokenBuf; > > + UINT32 TokenCountPerChunk; > > > > SpinLockSize =3D GetSpinLockProperties (); > > - CpuToken =3D AllocatePool (SpinLockSize); > > - ASSERT (CpuToken !=3D NULL); > > + TokenCountPerChunk =3D PcdGet32 (PcdTokenCountPerChunk); >=20 > (1) I understand that the DEC file introduces the PCD as [PcdsFixedAtBuil= d] > *only*. So, at this specific time, the code above is safe. >=20 > But I'm worried that at a later time, we might relax that to a dynamic PC= D, > and then the above code will behave really badly. Therefore, please updat= e > the patch in one of two ways: >=20 > (1a) please use a static global variable in the entry point function to c= ache the > PCD, and then use that variable in place of the PCD everywhere, >=20 > (1b) alternatively, please update the INF file, and the PcdGet32() calls = too, to > [FixedPcd] and FixedPcdGet32(). Then, if we happen to relax the PCD > declaration to dynamic, and a platform actually tries to use that, they w= ill get > a build error. (And then they'll be forced to imlement > (1a) above.) >=20 > The idea is to make this as robust / safe as possible. [[Eric]] Good suggestion. I will use 1b as the final solution for it. =20 >=20 > > + > > + if (gSmmCpuPrivate->UsedTokenNum =3D=3D TokenCountPerChunk) { > > + DEBUG ((DEBUG_VERBOSE, "CpuSmm: No free token buffer, allocate > > + new buffer!\n")); > > + > > + // > > + // 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 AllocatePool (SpinLockSize * > TokenCountPerChunk); > > + ASSERT (gSmmCpuPrivate->CurrentTokenBuf !=3D NULL); > > + gSmmCpuPrivate->UsedTokenNum =3D 0; } > > + > > + CpuToken =3D (SPIN_LOCK *)(gSmmCpuPrivate->CurrentTokenBuf + > > + SpinLockSize * gSmmCpuPrivate->UsedTokenNum); > > + gSmmCpuPrivate->UsedTokenNum++; > > + > > InitializeSpinLock (CpuToken); > > AcquireSpinLock (CpuToken); > > > > @@ -1737,10 +1778,28 @@ InitializeDataForMmMp ( > > VOID > > ) > > { > > + UINTN SpinLockSize; > > + UINT32 TokenCountPerChunk; > > + > > + SpinLockSize =3D GetSpinLockProperties (); TokenCountPerChunk =3D > > + PcdGet32 (PcdTokenCountPerChunk); ASSERT_EFI_ERROR > > + (TokenCountPerChunk !=3D 0); if (TokenCountPerChunk =3D=3D 0) { > > + DEBUG ((EFI_D_ERROR, "PcdTokenCountPerChunk should not be > > + Zero!\n")); >=20 > (2) This is a good check, but we should use DEBUG_ERROR, not EFI_D_ERROR. [[Eric]] good catch, it's copy paste issue :). I will update it when I chec= k in the code. >=20 > > + CpuDeadLoop (); > > + } > > + DEBUG ((DEBUG_INFO, "CpuSmm: SpinLock Size =3D 0x%x, > > + PreAllocateTokenNum =3D 0x%x\n", SpinLockSize, TokenCountPerChunk)); > > + > > + gSmmCpuPrivate->CurrentTokenBuf =3D AllocatePool (SpinLockSize * > > + TokenCountPerChunk); ASSERT (gSmmCpuPrivate->CurrentTokenBuf !=3D > > + NULL); > > + > > + gSmmCpuPrivate->UsedTokenNum =3D 0; > > + > > gSmmCpuPrivate->ApWrapperFunc =3D AllocatePool (sizeof > (PROCEDURE_WRAPPER) * gSmmCpuPrivate- > >SmmCoreEntryContext.NumberOfCpus); > > ASSERT (gSmmCpuPrivate->ApWrapperFunc !=3D NULL); > > > > InitializeListHead (&gSmmCpuPrivate->TokenList); > > + InitializeListHead (&gSmmCpuPrivate->OldTokenBufList); > > } > > > > /** > > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > > b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > > index 8c29f1a558..08ef8d2e15 100644 > > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > > @@ -217,6 +217,17 @@ typedef struct { > > > > #define PROCEDURE_TOKEN_FROM_LINK(a) CR (a, PROCEDURE_TOKEN, > Link, > > PROCEDURE_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_BUFFER_SIGNATURE) > > + > > // > > // Private structure for the SMM CPU module that is stored in DXE > > Runtime memory // Contains the SMM Configuration Protocols that is > produced. > > @@ -243,6 +254,10 @@ typedef struct { > > PROCEDURE_WRAPPER *ApWrapperFunc; > > LIST_ENTRY TokenList; > > > > + LIST_ENTRY OldTokenBufList; > > + > > + UINT8 *CurrentTokenBuf; > > + UINTN UsedTokenNum; // Only record tok= ens used in > CurrentTokenBuf. > > } SMM_CPU_PRIVATE_DATA; > > > > extern SMM_CPU_PRIVATE_DATA *gSmmCpuPrivate; diff --git > > a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf > > b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf > > index 851a8cb258..8b6c71b697 100644 > > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf > > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf > > @@ -140,6 +140,7 @@ > > > gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask > ## CONSUMES > > gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPropertyMask > ## CONSUMES > > gEfiMdePkgTokenSpaceGuid.PcdControlFlowEnforcementPropertyMask > ## CONSUMES > > + gUefiCpuPkgTokenSpaceGuid.PcdTokenCountPerChunk = ## > CONSUMES > > > > [Depex] > > gEfiMpServiceProtocolGuid > > diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec > > index 83acd33612..8ec2153459 100644 > > --- a/UefiCpuPkg/UefiCpuPkg.dec > > +++ b/UefiCpuPkg/UefiCpuPkg.dec > > @@ -147,6 +147,10 @@ > > # @Prompt Specify size of good stack of exception which need switchi= ng > stack. > > > > > gUefiCpuPkgTokenSpaceGuid.PcdCpuKnownGoodStackSize|2048|UINT32|0 > x30002 > > 001 > > > > + ## Size of pre allocated token count per chunk. > > + # @Prompt Specify the size of pre allocated token count per chunk. > > + > > + > gUefiCpuPkgTokenSpaceGuid.PcdTokenCountPerChunk|64|UINT32|0x3000 > 2002 > > + > > [PcdsFixedAtBuild, PcdsPatchableInModule] > > ## This value is the CPU Local APIC base address, which aligns the a= ddress > on a 4-KByte boundary. > > # @Prompt Configure base address of CPU Local APIC diff --git > > a/UefiCpuPkg/UefiCpuPkg.uni b/UefiCpuPkg/UefiCpuPkg.uni index > > fbf7680726..3bb951cc72 100644 > > --- a/UefiCpuPkg/UefiCpuPkg.uni > > +++ b/UefiCpuPkg/UefiCpuPkg.uni > > @@ -252,3 +252,4 @@ > > = "24000000 - 6th and 7th > generation Intel Core processors and Intel Xeon W Processor > Family(24MHz).
\n" > > = "19200000 - Intel Atom > processors based on Goldmont Microarchitecture with CPUID signature > 06_5CH(19.2MHz).
\n" > > > > +#string > STR_gUefiCpuPkgTokenSpaceGuid_PcdTokenCountPerChunk_PROMPT > #language en-US "Specify the size of pre allocated token count per chunk.= \n" > > \ No newline at end of file > > >=20 > (3) I think this would be the first PCD in the UNI file that does not hav= e _HELP, > only _PROMPT. Is that OK? It seems like an inconsistency. > But, I'll let Ray decide. [[Eric]] I missed the _HELP, will add it when I check in the code. Thanks, Eric >=20 > With (1) fixed (either (1a) or (1b)): >=20 > Acked-by: Laszlo Ersek >=20 > Thanks > Laszlo