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...@intel.com>
> Cc: Eric Dong <eric.d...@intel.com>
> Cc: Laszlo Ersek <ler...@redhat.com>
> Cc: Rahul Kumar <rahul1.ku...@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 <ler...@redhat.com>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#72924): https://edk2.groups.io/g/devel/message/72924
Mute This Topic: https://groups.io/mt/81367127/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
