[PATCH v4] UefiCpuPkg/PiSmmCpuDxeSmm: Avoid allocate Token every time

[PATCH v4] UefiCpuPkg/PiSmmCpuDxeSmm: Avoid allocate Token every time

[Dong, Eric]

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 <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
---
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                    |  4 ++
 5 files changed, 89 insertions(+), 4 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
index 0685637c2b..bfcf1ba397 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 = 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 (PcdTokenCountPerChunk);
+
+  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 (PcdTokenCountPerChunk);
+  ASSERT_EFI_ERROR (TokenCountPerChunk != 0);
+  if (TokenCountPerChunk == 0) {
+    DEBUG ((DEBUG_ERROR, "PcdTokenCountPerChunk should not be Zero!\n"));
+    CpuDeadLoop ();
+  }
+  DEBUG ((DEBUG_INFO, "CpuSmm: SpinLock Size = 0x%x, PreAllocateTokenNum = 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..a5607590ce 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 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..aa2f97e143 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
@@ -140,6 +140,9 @@
   gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPropertyMask               ## CONSUMES
   gEfiMdePkgTokenSpaceGuid.PcdControlFlowEnforcementPropertyMask        ## CONSUMES
 
+[FixedPcd]
+  gUefiCpuPkgTokenSpaceGuid.PcdTokenCountPerChunk                       ## CONSUMES
+
 [Pcd.X64]
   gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmRestrictedMemoryAccess        ## CONSUMES
 
diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec
index 12f4413ea5..349100371e 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
 
+  ## Size of pre allocated token count per chunk.
+  # @Prompt Specify the size of pre allocated token count per chunk.
+  gUefiCpuPkgTokenSpaceGuid.PcdTokenCountPerChunk|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..800b3ee6f7 100644
--- a/UefiCpuPkg/UefiCpuPkg.uni
+++ b/UefiCpuPkg/UefiCpuPkg.uni
@@ -272,3 +272,7 @@
                                                                                             "24000000  -  6th and 7th generation Intel Core processors and Intel Xeon W Processor Family(24MHz).<BR>\n"
                                                                                             "19200000  -  Intel Atom processors based on Goldmont Microarchitecture with CPUID signature 06_5CH(19.2MHz).<BR>\n"
 
+#string STR_gUefiCpuPkgTokenSpaceGuid_PcdTokenCountPerChunk_PROMPT  #language en-US "Specify the size of pre allocated token count per chunk.\n"
+
+#string STR_gUefiCpuPkgTokenSpaceGuid_PcdTokenCountPerChunk_HELP    #language en-US "This value used to specify the size of pre allocated token count per chunk.\n"
+                                                                                    "The Token is used for MM Mp Protocol.\n"
\ No newline at end of file
-- 
2.23.0.windows.1

Re: [PATCH v4] UefiCpuPkg/PiSmmCpuDxeSmm: Avoid allocate Token every time

[Ni, Ray]

Some comments.
> +  //
> 
> +  // 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.
1.  Can you reword to below?
"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."

> +  ASSERT_EFI_ERROR (TokenCountPerChunk != 0);

2. This is not right. Should use ASSERT().

> +  DEBUG ((DEBUG_INFO, "CpuSmm: SpinLock Size = 0x%x,
> PreAllocateTokenNum = 0x%x\n", SpinLockSize, TokenCountPerChunk));

3. "PreAllocateTokenNum" -> "PcdTokenCountPerChunk". Please directly use the PCD name in debug log.

> 
> +  UINT8                           *CurrentTokenBuf;
> 
> +  UINTN                           UsedTokenNum;     // Only record tokens used in
> CurrentTokenBuf.

4. I see a mismatch between types of UsedTokenNum and the PCD. One is UINTN, the other is UINT32.
Better to use UINT32 here.

> gUefiCpuPkgTokenSpaceGuid.PcdTokenCountPerChunk|64|UINT32|0x30002002

5. The PCD name is too generic. It's hard to tell when/where the PCD is used by reading
the name only.
Maybe "PcdCpuSmmMpTokenCountPerChunk"?

> +#string
> STR_gUefiCpuPkgTokenSpaceGuid_PcdTokenCountPerChunk_PROMPT
> #language en-US "Specify the size of pre allocated token count per chunk.\n"

6. "Size of pre allocated token count per chunk" is a bit confusing.
How about "Count of pre allocated SMM MP tokens"?
Then maybe the PCD name can be "PcdCpuSmmMpPreAllocateTokenCount"?

Thanks,
Ray

Re: [PATCH v4] UefiCpuPkg/PiSmmCpuDxeSmm: Avoid allocate Token every time

[Dong, Eric]

Hi Ray,

> -----Original Message-----
> From: Ni, Ray
> Sent: Thursday, December 5, 2019 1:25 PM
> To: Dong, Eric <eric.dong@intel.com>; devel@edk2.groups.io
> Cc: Laszlo Ersek <lersek@redhat.com>
> Subject: RE: [PATCH v4] UefiCpuPkg/PiSmmCpuDxeSmm: Avoid allocate
> Token every time
> 
> Some comments.
> > +  //
> >
> > +  // 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.
> 1.  Can you reword to below?
> "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."
[[Eric]] will change to this in next version.

> 
> > +  ASSERT_EFI_ERROR (TokenCountPerChunk != 0);
> 
> 2. This is not right. Should use ASSERT().
[[Eric]] Will update in next version.

> 
> > +  DEBUG ((DEBUG_INFO, "CpuSmm: SpinLock Size = 0x%x,
> > PreAllocateTokenNum = 0x%x\n", SpinLockSize, TokenCountPerChunk));
> 
> 3. "PreAllocateTokenNum" -> "PcdTokenCountPerChunk". Please directly
> use the PCD name in debug log.
[[Eric]] will update it in next version.

> 
> >
> > +  UINT8                           *CurrentTokenBuf;
> >
> > +  UINTN                           UsedTokenNum;     // Only record tokens used in
> > CurrentTokenBuf.
> 
> 4. I see a mismatch between types of UsedTokenNum and the PCD. One is
> UINTN, the other is UINT32.
> Better to use UINT32 here.
[[Eric]] will use UINT32 in next version.

> 
> >
> gUefiCpuPkgTokenSpaceGuid.PcdTokenCountPerChunk|64|UINT32|0x3000
> 2002
> 
> 5. The PCD name is too generic. It's hard to tell when/where the PCD is used
> by reading the name only.
> Maybe "PcdCpuSmmMpTokenCountPerChunk"?
[[Eric]] will use this name in next version.

> 
> > +#string
> > STR_gUefiCpuPkgTokenSpaceGuid_PcdTokenCountPerChunk_PROMPT
> > #language en-US "Specify the size of pre allocated token count per
> chunk.\n"
> 
> 6. "Size of pre allocated token count per chunk" is a bit confusing.
> How about "Count of pre allocated SMM MP tokens"?
[[Eric]] will update to "Count of pre allocated SMM MP tokens per chunk "


> Then maybe the PCD name can be "PcdCpuSmmMpPreAllocateTokenCount"?
[[Eric]] I think " PcdCpuSmmMpTokenCountPerChunk " is better and will use it.

Thanks,
Eric
> 
> Thanks,
> Ray