From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com [207.211.31.81]) by mx.groups.io with SMTP id smtpd.web09.640.1575567842943423404 for ; Thu, 05 Dec 2019 09:44:03 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=Geki4vQo; spf=pass (domain: redhat.com, ip: 207.211.31.81, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1575567842; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=ceFFfljdT1CEqHkjyq7mTfEmpjCMfvM0HrIAJZAHsW4=; b=Geki4vQoUTiUCzUPxIjyhZGddG5qU8pjO39gVdrz/cP/vi10tDRxhS1Fv3tGawAgqZbyu4 9dGsHW6vWHB/NJEq/FYwPfQrDL09uhVChSzhbG69fiwimS29kADUySQcZUgbZNXjssA0hX w12oLxQtwFSDmz5qlWyWKFtfWOdfp+M= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-105-14UFEyd_NvGXD0VOU5HWRQ-1; Thu, 05 Dec 2019 12:43:58 -0500 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 53F40DBA5; Thu, 5 Dec 2019 17:43:57 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-62.ams2.redhat.com [10.36.116.62]) by smtp.corp.redhat.com (Postfix) with ESMTP id 4D3825D6AE; Thu, 5 Dec 2019 17:43:56 +0000 (UTC) Subject: Re: [PATCH v5] UefiCpuPkg/PiSmmCpuDxeSmm: Avoid allocate Token every time To: Eric Dong , devel@edk2.groups.io Cc: Ray Ni References: <20191205070805.1951-1-eric.dong@intel.com> From: "Laszlo Ersek" Message-ID: <70438f62-69ea-449e-6292-9e18017bfeba@redhat.com> Date: Thu, 5 Dec 2019 18:43:55 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20191205070805.1951-1-eric.dong@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-MC-Unique: 14UFEyd_NvGXD0VOU5HWRQ-1 X-Mimecast-Spam-Score: 0 Content-Language: en-US Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit On 12/05/19 08:08, Eric Dong wrote: > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2388 > > 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 > --- > V5 changes: > Refine PCD names and some code comments. > V4 changes: > Specify PCD type to FixedPcd in code. > 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 | 3 + > UefiCpuPkg/UefiCpuPkg.dec | 4 ++ > UefiCpuPkg/UefiCpuPkg.uni | 3 + > 5 files changed, 88 insertions(+), 4 deletions(-) > > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > index 0685637c2b..757f1056f7 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; > + > + // > + // Only free the token buffer recorded in the OldTOkenBufList > + // upon exiting SMI. Current token buffer stays allocated so > + // next SMI doesn't need to re-allocate. > + // > + gSmmCpuPrivate->UsedTokenNum = 0; > + > + Link = GetFirstNode (&gSmmCpuPrivate->OldTokenBufList); > + while (!IsNull (&gSmmCpuPrivate->OldTokenBufList, Link)) { > + TokenBuf = TOKEN_BUFFER_FROM_LINK (Link); > + > + Link = RemoveEntryList (&TokenBuf->Link); > + > + FreePool (TokenBuf->Buffer); > + FreePool (TokenBuf); > + } > > while (!IsListEmpty (&gSmmCpuPrivate->TokenList)) { > Link = 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 = GetSpinLockProperties (); > - CpuToken = AllocatePool (SpinLockSize); > - ASSERT (CpuToken != NULL); > + TokenCountPerChunk = FixedPcdGet32 (PcdCpuSmmMpTokenCountPerChunk); > + > + if (gSmmCpuPrivate->UsedTokenNum == 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 = AllocatePool (sizeof (TOKEN_BUFFER)); > + ASSERT (TokenBuf != NULL); > + TokenBuf->Signature = TOKEN_BUFFER_SIGNATURE; > + TokenBuf->Buffer = gSmmCpuPrivate->CurrentTokenBuf; > + > + InsertTailList (&gSmmCpuPrivate->OldTokenBufList, &TokenBuf->Link); > + > + gSmmCpuPrivate->CurrentTokenBuf = AllocatePool (SpinLockSize * TokenCountPerChunk); > + ASSERT (gSmmCpuPrivate->CurrentTokenBuf != NULL); > + gSmmCpuPrivate->UsedTokenNum = 0; > + } > + > + CpuToken = (SPIN_LOCK *)(gSmmCpuPrivate->CurrentTokenBuf + SpinLockSize * gSmmCpuPrivate->UsedTokenNum); > + gSmmCpuPrivate->UsedTokenNum++; > + > InitializeSpinLock (CpuToken); > AcquireSpinLock (CpuToken); > > @@ -1732,10 +1773,28 @@ InitializeDataForMmMp ( > VOID > ) > { > + UINTN SpinLockSize; > + UINT32 TokenCountPerChunk; > + > + SpinLockSize = GetSpinLockProperties (); > + TokenCountPerChunk = FixedPcdGet32 (PcdCpuSmmMpTokenCountPerChunk); > + ASSERT (TokenCountPerChunk != 0); > + if (TokenCountPerChunk == 0) { > + DEBUG ((DEBUG_ERROR, "PcdCpuSmmMpTokenCountPerChunk should not be Zero!\n")); > + CpuDeadLoop (); > + } > + DEBUG ((DEBUG_INFO, "CpuSmm: SpinLock Size = 0x%x, PcdCpuSmmMpTokenCountPerChunk = 0x%x\n", SpinLockSize, TokenCountPerChunk)); > + > + gSmmCpuPrivate->CurrentTokenBuf = AllocatePool (SpinLockSize * TokenCountPerChunk); > + ASSERT (gSmmCpuPrivate->CurrentTokenBuf != NULL); > + > + gSmmCpuPrivate->UsedTokenNum = 0; > + > gSmmCpuPrivate->ApWrapperFunc = AllocatePool (sizeof (PROCEDURE_WRAPPER) * gSmmCpuPrivate->SmmCoreEntryContext.NumberOfCpus); > ASSERT (gSmmCpuPrivate->ApWrapperFunc != NULL); > > InitializeListHead (&gSmmCpuPrivate->TokenList); > + InitializeListHead (&gSmmCpuPrivate->OldTokenBufList); > } > > /** > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > index 7e7c73f27f..5c1a01e42b 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; > + UINT32 UsedTokenNum; // Only record tokens 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 b12b2691f8..76b1462996 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf > @@ -140,6 +140,9 @@ > gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPropertyMask ## CONSUMES > gEfiMdePkgTokenSpaceGuid.PcdControlFlowEnforcementPropertyMask ## CONSUMES > > +[FixedPcd] > + gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmMpTokenCountPerChunk ## CONSUMES > + > [Pcd.X64] > gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmRestrictedMemoryAccess ## CONSUMES > > diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec > index 12f4413ea5..797f948631 100644 > --- a/UefiCpuPkg/UefiCpuPkg.dec > +++ b/UefiCpuPkg/UefiCpuPkg.dec > @@ -148,6 +148,10 @@ > # @Prompt Specify size of good stack of exception which need switching stack. > gUefiCpuPkgTokenSpaceGuid.PcdCpuKnownGoodStackSize|2048|UINT32|0x30002001 > > + ## Count of pre allocated SMM MP tokens per chunk. > + # @Prompt Specify the count of pre allocated SMM MP tokens per chunk. > + gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmMpTokenCountPerChunk|64|UINT32|0x30002002 > + > [PcdsFixedAtBuild, PcdsPatchableInModule] > ## This value is the CPU Local APIC base address, which aligns the address on a 4-KByte boundary. > # @Prompt Configure base address of CPU Local APIC > diff --git a/UefiCpuPkg/UefiCpuPkg.uni b/UefiCpuPkg/UefiCpuPkg.uni > index bfd696f48c..442ce5cb85 100644 > --- a/UefiCpuPkg/UefiCpuPkg.uni > +++ b/UefiCpuPkg/UefiCpuPkg.uni > @@ -272,3 +272,6 @@ > "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_PcdCpuSmmMpTokenCountPerChunk_PROMPT #language en-US "Specify the count of pre allocated SMM MP tokens per chunk.\n" > + > +#string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuSmmMpTokenCountPerChunk_HELP #language en-US "This value used to specify the count of pre allocated SMM MP tokens per chunk.\n" > \ No newline at end of file > Thank you for the updates! Acked-by: Laszlo Ersek Laszlo