Re: [Patch] UefiCpuPkg/MpInitLib: Avoid copy twice.

["Ni, Ray" <ray.ni@intel.com>]

Reviewed-by: Ray Ni <ray.ni@intel.com>

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Wednesday, July 31, 2019 11:08 PM
> To: Dong, Eric <eric.dong@intel.com>; devel@edk2.groups.io
> Cc: Ni, Ray <ray.ni@intel.com>
> Subject: Re: [Patch] UefiCpuPkg/MpInitLib: Avoid copy twice.
> 
> On 07/31/19 10:01, Eric Dong wrote:
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1982
> >
> > MpInitLibInitialize in MpLib.c will be invoked on both PEI and DXE CPU
> > code, MicrocodeDetect would be performed twice and copy Microcode
> from
> > flash to memory twice as well, which consider as duplicate work to
> > lead longer boot time.
> > This patch just use microcode memory copied in PEI phase if exist.
> >
> > Signed-off-by: Eric Dong <eric.dong@intel.com>
> > Cc: Ray Ni <ray.ni@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > ---
> >  UefiCpuPkg/Library/MpInitLib/MpLib.c | 62
> > +++++++++++++++-------------
> >  1 file changed, 33 insertions(+), 29 deletions(-)
> >
> > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> > b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> > index 572495ec36..a1ad665564 100644
> > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> > @@ -1607,38 +1607,42 @@ MpInitLibInitialize (
> >    CpuMpData->SwitchBspFlag    = FALSE;
> >    CpuMpData->CpuData          = (CPU_AP_DATA *) (CpuMpData + 1);
> >    CpuMpData->CpuInfoInHob     = (UINT64) (UINTN) (CpuMpData-
> >CpuData + MaxLogicalProcessorNumber);
> > -  CpuMpData->MicrocodePatchRegionSize = PcdGet64
> > (PcdCpuMicrocodePatchRegionSize);
> > -  //
> > -  // 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 {
> > +  if (OldCpuMpData == NULL) {
> > +    CpuMpData->MicrocodePatchRegionSize = PcdGet64
> > + (PcdCpuMicrocodePatchRegionSize);
> >      //
> > -    // there are multiple processors, and a microcode patch is available, and
> > -    // memory allocation succeeded
> > +    // If platform has more than one CPU, relocate microcode to memory to
> reduce
> > +    // loading microcode time.
> >      //
> > -    CopyMem (
> > -      MicrocodePatchInRam,
> > -      (VOID *)(UINTN)PcdGet64 (PcdCpuMicrocodePatchAddress),
> > -      (UINTN)CpuMpData->MicrocodePatchRegionSize
> > -      );
> > -    CpuMpData->MicrocodePatchAddress = (UINTN)MicrocodePatchInRam;
> > +    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;
> > +    }
> > +  }else {
> > +    CpuMpData->MicrocodePatchRegionSize = OldCpuMpData-
> >MicrocodePatchRegionSize;
> > +    CpuMpData->MicrocodePatchAddress    = OldCpuMpData-
> >MicrocodePatchAddress;
> >    }
> > -
> >    InitializeSpinLock(&CpuMpData->MpLock);
> >
> >    //
> >
> 
> I applied this patch locally and reviewed it with:
> 
>   git show -b -W
> 
> The change looks reasonable to me.
> 
> (1) Please clarify the subject line a bit. My suggestion is:
> 
>   UefiCpuPkg/MpInitLib: don't shadow the microcode patch twice
> 
> (this is 58 characters). Feel free to tweak this further if you wish; the point is
> that "copy" is too generic in itself.
> 
> 
> With (1) fixed:
> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> 
> Thanks
> Laszlo