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

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

Hi Laszlo,

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Thursday, July 12, 2018 5:26 PM
> To: Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>
> Subject: Re: [edk2] [Patch 1/3] UefiCpuPkg/MpInitLib: Relocate uCode to
> memory to save time.
> 
> 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.
> 

Yes, my original patch has PcdCpuMicrocodePatchRegionSize check, but after check I found if it is zero, all the allocate action will do nothing, so I removed it.

> (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.
> 

Agree.

> 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 for your sample code, I directly use it.  Also I think allocate memory failed is an abnormal case, so I add an ASSERT in the new patch.

> Thanks
> Laszlo