Re: [Patch 1/3] UefiCpuPkg/MpInitLib: Relocate uCode to memory to save time.

[Laszlo Ersek <lersek@redhat.com>]

Hi Eric,

On 07/11/18 13:07, Eric Dong wrote:
> Read uCode from memory has better performance than from flash.
> But it needs extra effort to let BSP copy uCode from flash to
> memory. Also BSP already enable cache in SEC phase, so it use
> less time to relocate uCode from flash to memory. After
> verification, if system has more than one processor, it will
> reduce some time if load uCode from memory.
>
> This change enable this optimization.
>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Eric Dong <eric.dong@intel.com>
> ---
>  UefiCpuPkg/Library/MpInitLib/MpLib.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index eec178b419..8b458a4a3a 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -1560,8 +1560,19 @@ MpInitLibInitialize (
>    CpuMpData->SwitchBspFlag    = FALSE;
>    CpuMpData->CpuData          = (CPU_AP_DATA *) (CpuMpData + 1);
>    CpuMpData->CpuInfoInHob     = (UINT64) (UINTN) (CpuMpData->CpuData + MaxLogicalProcessorNumber);
> -  CpuMpData->MicrocodePatchAddress    = PcdGet64 (PcdCpuMicrocodePatchAddress);
>    CpuMpData->MicrocodePatchRegionSize = PcdGet64 (PcdCpuMicrocodePatchRegionSize);
> +  //
> +  // if platform has more than one CPU, relocate microcode to memory to reduce loading microcode time.
> +  //
> +  if (MaxLogicalProcessorNumber > 1) {
> +    CpuMpData->MicrocodePatchAddress = (UINT64) (UINTN) AllocatePages (EFI_SIZE_TO_PAGES ((UINTN)CpuMpData->MicrocodePatchRegionSize));
> +    if (CpuMpData->MicrocodePatchAddress != 0) {
> +      CopyMem ((VOID *) (UINTN)CpuMpData->MicrocodePatchAddress, (VOID *)(UINTN)(PcdGet64 (PcdCpuMicrocodePatchAddress)), (UINTN)CpuMpData->MicrocodePatchRegionSize);
> +    }
> +  } else {
> +    CpuMpData->MicrocodePatchAddress = PcdGet64 (PcdCpuMicrocodePatchAddress);
> +  }
> +
>    InitializeSpinLock(&CpuMpData->MpLock);
>    //
>    // Save BSP's Control registers to APs
>

(1) Can you please break up the added lines to multiple lines? They are
extremely long, and difficult for me to handle. It should be possible to
break up both AllocatePages() and CopyMem(), for example.

(2) The code appears to handle the case well when
PcdCpuMicrocodePatchRegionSize is zero. In that case,
EFI_SIZE_TO_PAGES(...) evaluates to zero, and -- I checked --
AllocatePages() returns NULL. In this case, allocation and copying will
not take place, and that's fine -- there is nothing to copy and no
microcode to install. So, OK.

(3) However, if there is a microcode update available, but we can't
allocate memory, things will go wrong. The region size is nonzero, thus
MicrocodeDetect() will not exit early. But MicrocodePatchAddress will be
set to 0.

So, I suggest the following instead:

---------
  VOID *MicrocodePatchInRam;

  //
  // If platform has more than one CPU, relocate microcode to memory to reduce
  // loading microcode time.
  //
  MicrocodePatchInRam = NULL;
  if (MaxLogicalProcessorNumber > 1) {
    MicrocodePatchInRam = AllocatePages (
                            EFI_SIZE_TO_PAGES (
                              (UINTN)CpuMpData->MicrocodePatchRegionSize
                              )
                            );
  }
  if (MicrocodePatchInRam == NULL) {
    //
    // there is only one processor, or no microcode patch is available, or
    // memory allocation failed
    //
    CpuMpData->MicrocodePatchAddress = PcdGet64 (PcdCpuMicrocodePatchAddress);
  } else {
    //
    // there are multiple processors, and a microcode patch is available, and
    // memory allocation succeeded
    //
    CopyMem (
      MicrocodePatchInRam,
      (VOID *)(UINTN)PcdGet64 (PcdCpuMicrocodePatchAddress),
      (UINTN)CpuMpData->MicrocodePatchRegionSize
      );
    CpuMpData->MicrocodePatchAddress = (UINTN)MicrocodePatchInRam;
  }
---------

Thanks
Laszlo