1. Patchew
  2. EDK2
  3. View series

[edk2-devel] [Patch] UefiCpuPkg/MpInitLib: Avoid copy twice.

Dong, Eric posted 1 patch 4 years, 8 months ago
Failed in applying to current master (apply log)
UefiCpuPkg/Library/MpInitLib/MpLib.c | 62 +++++++++++++++-------------
1 file changed, 33 insertions(+), 29 deletions(-)
[edk2-devel] [Patch] UefiCpuPkg/MpInitLib: Avoid copy twice.
Posted by Dong, Eric 4 years, 8 months ago 
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);
 
   //
-- 
2.21.0.windows.1


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#44673): https://edk2.groups.io/g/devel/message/44673
Mute This Topic: https://groups.io/mt/32664555/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [Patch] UefiCpuPkg/MpInitLib: Avoid copy twice.
Posted by Laszlo Ersek 4 years, 8 months ago 
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

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#44684): https://edk2.groups.io/g/devel/message/44684
Mute This Topic: https://groups.io/mt/32664555/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [Patch] UefiCpuPkg/MpInitLib: Avoid copy twice.
Posted by Ni, Ray 4 years, 8 months ago 
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

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#44741): https://edk2.groups.io/g/devel/message/44741
Mute This Topic: https://groups.io/mt/32664555/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.