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

["Dong, Eric" <eric.dong@intel.com>]

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