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

["Laszlo Ersek" <lersek@redhat.com>]

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 <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> ---
> 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).<BR>\n"
>                                                                                              "19200000  -  Intel Atom processors based on Goldmont Microarchitecture with CPUID signature 06_5CH(19.2MHz).<BR>\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 <lersek@redhat.com>

Laszlo