1. Patchew
  2. EDK2
  3. View series

[edk2-devel] [PATCH] UefiCpuPkg/MpInitLib: avoid printing debug messages in AP

Ni, Ray posted 1 patch 3 years, 1 month ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/edk2 tags/patchew/20210312114804.2-1-ray.ni@intel.com
There is a newer version of this series
UefiCpuPkg/Library/MpInitLib/Microcode.c | 11 +----------
UefiCpuPkg/Library/MpInitLib/MpLib.c     |  9 +++++++++
UefiCpuPkg/Library/MpInitLib/MpLib.h     |  1 +
3 files changed, 11 insertions(+), 10 deletions(-)
[edk2-devel] [PATCH] UefiCpuPkg/MpInitLib: avoid printing debug messages in AP
Posted by Ni, Ray 3 years, 1 month ago 
MpInitLib contains a function MicrocodeDetect() which is called by
all threads as an AP procedure.
Today this function contains below code:

    if (CurrentRevision != LatestRevision) {
      AcquireSpinLock(&CpuMpData->MpLock);
      DEBUG ((
        EFI_D_ERROR,
        "Updated microcode signature [0x%08x] does not match \
        loaded microcode signature [0x%08x]\n",
        CurrentRevision, LatestRevision
        ));
      ReleaseSpinLock(&CpuMpData->MpLock);
    }

When the if-check is passed, the code may call into PEI services:
1. AcquireSpinLock
   When the PcdSpinTimeout is not 0, TimerLib
   GetPerformanceCounterProperties() is called. And some of the
   TimerLib implementations would get the information cached in
   HOB. But AP procedure cannot call PEI services to retrieve the
   HOB list.

2. DEBUG
   Certain DebugLib relies on ReportStatusCode services and the
   ReportStatusCode PPI is retrieved through the PEI services.
   DebugLibSerialPort should be used.
   But when SerialPortLib is implemented to depend on PEI services,
   even using DebugLibSerialPort can still cause AP calls PEI
   services resulting hang.

It causes a lot of debugging effort on the platform side.

There are 2 options to fix the problem:
1. make sure platform DSC chooses the proper DebugLib and set the
   PcdSpinTimeout to 0. So that AcquireSpinLock and DEBUG don't call
   PEI services.
2. remove the AcquireSpinLock and DEBUG call from the procedure.

Option #2 is preferred because it's not practical to ask every
platform DSC to be written properly.

Following option #2, there are two sub-options:
2.A. Just remove the if-check.
2.B. Capture the CurrentRevision and ExpectedRevision in the memory
     for each AP and print them together from BSP.

The patch follows option 2.B.

Signed-off-by: Ray Ni <ray.ni@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
---
 UefiCpuPkg/Library/MpInitLib/Microcode.c | 11 +----------
 UefiCpuPkg/Library/MpInitLib/MpLib.c     |  9 +++++++++
 UefiCpuPkg/Library/MpInitLib/MpLib.h     |  1 +
 3 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/UefiCpuPkg/Library/MpInitLib/Microcode.c b/UefiCpuPkg/Library/MpInitLib/Microcode.c
index 15629591e2..297c2abcd1 100644
--- a/UefiCpuPkg/Library/MpInitLib/Microcode.c
+++ b/UefiCpuPkg/Library/MpInitLib/Microcode.c
@@ -315,17 +315,8 @@ Done:
         MSR_IA32_BIOS_UPDT_TRIG,
         (UINT64) (UINTN) MicrocodeData
         );
-    //
-    // Get and check new microcode signature
-    //
-    CurrentRevision = GetCurrentMicrocodeSignature ();
-    if (CurrentRevision != LatestRevision) {
-      AcquireSpinLock(&CpuMpData->MpLock);
-      DEBUG ((EFI_D_ERROR, "Updated microcode signature [0x%08x] does not match \
-                loaded microcode signature [0x%08x]\n", CurrentRevision, LatestRevision));
-      ReleaseSpinLock(&CpuMpData->MpLock);
-    }
   }
+  CpuMpData->CpuData[ProcessorNumber].MicrocodeRevision = GetCurrentMicrocodeSignature ();
 }
 
 /**
diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
index 5040053dad..e4baeff894 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
@@ -1947,6 +1947,7 @@ MpInitLibInitialize (
   UINTN                    ApResetVectorSize;
   UINTN                    BackupBufferAddr;
   UINTN                    ApIdtBase;
+  UINT32                   ExpectedMicrocodeRevision;
 
   OldCpuMpData = GetCpuMpDataFromGuidedHob ();
   if (OldCpuMpData == NULL) {
@@ -2131,6 +2132,14 @@ MpInitLibInitialize (
       CpuMpData->InitFlag = ApInitDone;
     }
     for (Index = 0; Index < CpuMpData->CpuCount; Index++) {
+      ExpectedMicrocodeRevision = 0;
+      if (CpuMpData->CpuData[Index].MicrocodeEntryAddr != 0) {
+        ExpectedMicrocodeRevision = ((CPU_MICROCODE_HEADER *)(UINTN)CpuMpData->CpuData[Index].MicrocodeEntryAddr)->UpdateRevision;
+      }
+      DEBUG ((
+        DEBUG_INFO, "CPU[%04d]: Microcode revision = %08x, expected = %08x\n",
+        Index, CpuMpData->CpuData[Index].MicrocodeRevision, ExpectedMicrocodeRevision
+        ));
       SetApState (&CpuMpData->CpuData[Index], CpuStateIdle);
     }
   }
diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h
index 0bd60388b1..66f9eb2304 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
@@ -144,6 +144,7 @@ typedef struct {
   UINT32                         ProcessorSignature;
   UINT8                          PlatformId;
   UINT64                         MicrocodeEntryAddr;
+  UINT32                         MicrocodeRevision;
 } CPU_AP_DATA;
 
 //
-- 
2.27.0.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#72714): https://edk2.groups.io/g/devel/message/72714
Mute This Topic: https://groups.io/mt/81276903/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 printing debug messages in AP
Posted by Dong, Eric 3 years, 1 month ago 
Reviewed-by: Eric Dong <eric.dong@intel.com>

-----Original Message-----
From: Ni, Ray <ray.ni@intel.com> 
Sent: Friday, March 12, 2021 7:48 PM
To: devel@edk2.groups.io
Cc: Dong, Eric <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>
Subject: [PATCH] UefiCpuPkg/MpInitLib: avoid printing debug messages in AP

MpInitLib contains a function MicrocodeDetect() which is called by
all threads as an AP procedure.
Today this function contains below code:

    if (CurrentRevision != LatestRevision) {
      AcquireSpinLock(&CpuMpData->MpLock);
      DEBUG ((
        EFI_D_ERROR,
        "Updated microcode signature [0x%08x] does not match \
        loaded microcode signature [0x%08x]\n",
        CurrentRevision, LatestRevision
        ));
      ReleaseSpinLock(&CpuMpData->MpLock);
    }

When the if-check is passed, the code may call into PEI services:
1. AcquireSpinLock
   When the PcdSpinTimeout is not 0, TimerLib
   GetPerformanceCounterProperties() is called. And some of the
   TimerLib implementations would get the information cached in
   HOB. But AP procedure cannot call PEI services to retrieve the
   HOB list.

2. DEBUG
   Certain DebugLib relies on ReportStatusCode services and the
   ReportStatusCode PPI is retrieved through the PEI services.
   DebugLibSerialPort should be used.
   But when SerialPortLib is implemented to depend on PEI services,
   even using DebugLibSerialPort can still cause AP calls PEI
   services resulting hang.

It causes a lot of debugging effort on the platform side.

There are 2 options to fix the problem:
1. make sure platform DSC chooses the proper DebugLib and set the
   PcdSpinTimeout to 0. So that AcquireSpinLock and DEBUG don't call
   PEI services.
2. remove the AcquireSpinLock and DEBUG call from the procedure.

Option #2 is preferred because it's not practical to ask every
platform DSC to be written properly.

Following option #2, there are two sub-options:
2.A. Just remove the if-check.
2.B. Capture the CurrentRevision and ExpectedRevision in the memory
     for each AP and print them together from BSP.

The patch follows option 2.B.

Signed-off-by: Ray Ni <ray.ni@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
---
 UefiCpuPkg/Library/MpInitLib/Microcode.c | 11 +----------
 UefiCpuPkg/Library/MpInitLib/MpLib.c     |  9 +++++++++
 UefiCpuPkg/Library/MpInitLib/MpLib.h     |  1 +
 3 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/UefiCpuPkg/Library/MpInitLib/Microcode.c b/UefiCpuPkg/Library/MpInitLib/Microcode.c
index 15629591e2..297c2abcd1 100644
--- a/UefiCpuPkg/Library/MpInitLib/Microcode.c
+++ b/UefiCpuPkg/Library/MpInitLib/Microcode.c
@@ -315,17 +315,8 @@ Done:
         MSR_IA32_BIOS_UPDT_TRIG,

         (UINT64) (UINTN) MicrocodeData

         );

-    //

-    // Get and check new microcode signature

-    //

-    CurrentRevision = GetCurrentMicrocodeSignature ();

-    if (CurrentRevision != LatestRevision) {

-      AcquireSpinLock(&CpuMpData->MpLock);

-      DEBUG ((EFI_D_ERROR, "Updated microcode signature [0x%08x] does not match \

-                loaded microcode signature [0x%08x]\n", CurrentRevision, LatestRevision));

-      ReleaseSpinLock(&CpuMpData->MpLock);

-    }

   }

+  CpuMpData->CpuData[ProcessorNumber].MicrocodeRevision = GetCurrentMicrocodeSignature ();

 }

 

 /**

diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
index 5040053dad..e4baeff894 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
@@ -1947,6 +1947,7 @@ MpInitLibInitialize (
   UINTN                    ApResetVectorSize;

   UINTN                    BackupBufferAddr;

   UINTN                    ApIdtBase;

+  UINT32                   ExpectedMicrocodeRevision;

 

   OldCpuMpData = GetCpuMpDataFromGuidedHob ();

   if (OldCpuMpData == NULL) {

@@ -2131,6 +2132,14 @@ MpInitLibInitialize (
       CpuMpData->InitFlag = ApInitDone;

     }

     for (Index = 0; Index < CpuMpData->CpuCount; Index++) {

+      ExpectedMicrocodeRevision = 0;

+      if (CpuMpData->CpuData[Index].MicrocodeEntryAddr != 0) {

+        ExpectedMicrocodeRevision = ((CPU_MICROCODE_HEADER *)(UINTN)CpuMpData->CpuData[Index].MicrocodeEntryAddr)->UpdateRevision;

+      }

+      DEBUG ((

+        DEBUG_INFO, "CPU[%04d]: Microcode revision = %08x, expected = %08x\n",

+        Index, CpuMpData->CpuData[Index].MicrocodeRevision, ExpectedMicrocodeRevision

+        ));

       SetApState (&CpuMpData->CpuData[Index], CpuStateIdle);

     }

   }

diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h
index 0bd60388b1..66f9eb2304 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
@@ -144,6 +144,7 @@ typedef struct {
   UINT32                         ProcessorSignature;

   UINT8                          PlatformId;

   UINT64                         MicrocodeEntryAddr;

+  UINT32                         MicrocodeRevision;

 } CPU_AP_DATA;

 

 //

-- 
2.27.0.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#72715): https://edk2.groups.io/g/devel/message/72715
Mute This Topic: https://groups.io/mt/81276903/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 printing debug messages in AP
Posted by Laszlo Ersek 3 years, 1 month ago 
On 03/12/21 12:48, Ray Ni wrote:
> MpInitLib contains a function MicrocodeDetect() which is called by
> all threads as an AP procedure.
> Today this function contains below code:
> 
>     if (CurrentRevision != LatestRevision) {
>       AcquireSpinLock(&CpuMpData->MpLock);
>       DEBUG ((
>         EFI_D_ERROR,
>         "Updated microcode signature [0x%08x] does not match \
>         loaded microcode signature [0x%08x]\n",
>         CurrentRevision, LatestRevision
>         ));
>       ReleaseSpinLock(&CpuMpData->MpLock);
>     }
> 
> When the if-check is passed, the code may call into PEI services:
> 1. AcquireSpinLock
>    When the PcdSpinTimeout is not 0, TimerLib
>    GetPerformanceCounterProperties() is called. And some of the
>    TimerLib implementations would get the information cached in
>    HOB. But AP procedure cannot call PEI services to retrieve the
>    HOB list.
> 
> 2. DEBUG
>    Certain DebugLib relies on ReportStatusCode services and the
>    ReportStatusCode PPI is retrieved through the PEI services.
>    DebugLibSerialPort should be used.
>    But when SerialPortLib is implemented to depend on PEI services,
>    even using DebugLibSerialPort can still cause AP calls PEI
>    services resulting hang.
> 
> It causes a lot of debugging effort on the platform side.
> 
> There are 2 options to fix the problem:
> 1. make sure platform DSC chooses the proper DebugLib and set the
>    PcdSpinTimeout to 0. So that AcquireSpinLock and DEBUG don't call
>    PEI services.
> 2. remove the AcquireSpinLock and DEBUG call from the procedure.
> 
> Option #2 is preferred because it's not practical to ask every
> platform DSC to be written properly.
> 
> Following option #2, there are two sub-options:
> 2.A. Just remove the if-check.
> 2.B. Capture the CurrentRevision and ExpectedRevision in the memory
>      for each AP and print them together from BSP.
> 
> The patch follows option 2.B.
> 
> Signed-off-by: Ray Ni <ray.ni@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> ---
>  UefiCpuPkg/Library/MpInitLib/Microcode.c | 11 +----------
>  UefiCpuPkg/Library/MpInitLib/MpLib.c     |  9 +++++++++
>  UefiCpuPkg/Library/MpInitLib/MpLib.h     |  1 +
>  3 files changed, 11 insertions(+), 10 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/Microcode.c b/UefiCpuPkg/Library/MpInitLib/Microcode.c
> index 15629591e2..297c2abcd1 100644
> --- a/UefiCpuPkg/Library/MpInitLib/Microcode.c
> +++ b/UefiCpuPkg/Library/MpInitLib/Microcode.c
> @@ -315,17 +315,8 @@ Done:
>          MSR_IA32_BIOS_UPDT_TRIG,
>          (UINT64) (UINTN) MicrocodeData
>          );
> -    //
> -    // Get and check new microcode signature
> -    //
> -    CurrentRevision = GetCurrentMicrocodeSignature ();
> -    if (CurrentRevision != LatestRevision) {
> -      AcquireSpinLock(&CpuMpData->MpLock);
> -      DEBUG ((EFI_D_ERROR, "Updated microcode signature [0x%08x] does not match \
> -                loaded microcode signature [0x%08x]\n", CurrentRevision, LatestRevision));
> -      ReleaseSpinLock(&CpuMpData->MpLock);
> -    }
>    }
> +  CpuMpData->CpuData[ProcessorNumber].MicrocodeRevision = GetCurrentMicrocodeSignature ();
>  }
>  
>  /**
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index 5040053dad..e4baeff894 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -1947,6 +1947,7 @@ MpInitLibInitialize (
>    UINTN                    ApResetVectorSize;
>    UINTN                    BackupBufferAddr;
>    UINTN                    ApIdtBase;
> +  UINT32                   ExpectedMicrocodeRevision;
>  
>    OldCpuMpData = GetCpuMpDataFromGuidedHob ();
>    if (OldCpuMpData == NULL) {
> @@ -2131,6 +2132,14 @@ MpInitLibInitialize (
>        CpuMpData->InitFlag = ApInitDone;
>      }
>      for (Index = 0; Index < CpuMpData->CpuCount; Index++) {
> +      ExpectedMicrocodeRevision = 0;
> +      if (CpuMpData->CpuData[Index].MicrocodeEntryAddr != 0) {
> +        ExpectedMicrocodeRevision = ((CPU_MICROCODE_HEADER *)(UINTN)CpuMpData->CpuData[Index].MicrocodeEntryAddr)->UpdateRevision;
> +      }
> +      DEBUG ((
> +        DEBUG_INFO, "CPU[%04d]: Microcode revision = %08x, expected = %08x\n",
> +        Index, CpuMpData->CpuData[Index].MicrocodeRevision, ExpectedMicrocodeRevision
> +        ));
>        SetApState (&CpuMpData->CpuData[Index], CpuStateIdle);
>      }
>    }
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> index 0bd60388b1..66f9eb2304 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> @@ -144,6 +144,7 @@ typedef struct {
>    UINT32                         ProcessorSignature;
>    UINT8                          PlatformId;
>    UINT64                         MicrocodeEntryAddr;
> +  UINT32                         MicrocodeRevision;
>  } CPU_AP_DATA;
>  
>  //
> 

Acked-by: Laszlo Ersek <lersek@redhat.com>



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#72725): https://edk2.groups.io/g/devel/message/72725
Mute This Topic: https://groups.io/mt/81276903/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.