Re: [PATCH v2] UefiCpuPkg/MpInitLib: avoid printing debug messages in AP

["Laszlo Ersek" <lersek@redhat.com>]

On 03/16/21 02:52, 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     | 25 ++++++++++++++++++++++++
>  UefiCpuPkg/Library/MpInitLib/MpLib.h     |  1 +
>  3 files changed, 27 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..3d945972a0 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -2135,6 +2135,31 @@ MpInitLibInitialize (
>      }
>    }
>  
> +  //
> +  // Dump the microcode revision for each core.
> +  //
> +  DEBUG_CODE (
> +    UINT32 ThreadId;
> +    UINT32 ExpectedMicrocodeRevision;
> +    CpuInfoInHob = (CPU_INFO_IN_HOB *) (UINTN) CpuMpData->CpuInfoInHob;
> +    for (Index = 0; Index < CpuMpData->CpuCount; Index++) {
> +      GetProcessorLocationByApicId (CpuInfoInHob[Index].InitialApicId, NULL, NULL, &ThreadId);
> +      if (ThreadId == 0) {
> +        //
> +        // MicrocodeDetect() loads microcode in first thread of each core, so,
> +        // CpuMpData->CpuData[Index].MicrocodeEntryAddr is initialized only for first thread of each core.
> +        //
> +        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
> +          ));
> +      }
> +    }
> +  );
>    //
>    // Initialize global data for MP support
>    //
> 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>