* [PATCH v2] UefiCpuPkg/MpInitLib: avoid printing debug messages in AP
@ 2021-03-16 1:52 Ni, Ray
2021-03-16 16:44 ` Laszlo Ersek
2021-03-17 4:55 ` Dong, Eric
0 siblings, 2 replies; 3+ messages in thread
From: Ni, Ray @ 2021-03-16 1:52 UTC (permalink / raw)
To: devel; +Cc: Eric Dong, Laszlo Ersek, Rahul Kumar
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;
//
--
2.27.0.windows.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2] UefiCpuPkg/MpInitLib: avoid printing debug messages in AP
2021-03-16 1:52 [PATCH v2] UefiCpuPkg/MpInitLib: avoid printing debug messages in AP Ni, Ray
@ 2021-03-16 16:44 ` Laszlo Ersek
2021-03-17 4:55 ` Dong, Eric
1 sibling, 0 replies; 3+ messages in thread
From: Laszlo Ersek @ 2021-03-16 16:44 UTC (permalink / raw)
To: Ray Ni, devel; +Cc: Eric Dong, Rahul Kumar
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>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2] UefiCpuPkg/MpInitLib: avoid printing debug messages in AP
2021-03-16 1:52 [PATCH v2] UefiCpuPkg/MpInitLib: avoid printing debug messages in AP Ni, Ray
2021-03-16 16:44 ` Laszlo Ersek
@ 2021-03-17 4:55 ` Dong, Eric
1 sibling, 0 replies; 3+ messages in thread
From: Dong, Eric @ 2021-03-17 4:55 UTC (permalink / raw)
To: Ni, Ray, devel@edk2.groups.io; +Cc: Laszlo Ersek, Kumar, Rahul1
Reviewed-by: Eric Dong <eric.dong@intel.com>
-----Original Message-----
From: Ni, Ray <ray.ni@intel.com>
Sent: Tuesday, March 16, 2021 9:52 AM
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 v2] 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 | 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;
//
--
2.27.0.windows.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-03-17 4:55 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-03-16 1:52 [PATCH v2] UefiCpuPkg/MpInitLib: avoid printing debug messages in AP Ni, Ray
2021-03-16 16:44 ` Laszlo Ersek
2021-03-17 4:55 ` Dong, Eric
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox