Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UefiCpuPkg/MpInitLib: avoid printing debug messages in AP #1498

Merged
merged 1 commit into from
Mar 17, 2021

Conversation

niruiyu
Copy link
Member

@niruiyu niruiyu commented Mar 17, 2021

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
Reviewed-by: Eric Dong eric.dong@intel.com
Acked-by: Laszlo Ersek lersek@redhat.com
Cc: Rahul Kumar rahul1.kumar@intel.com

@niruiyu niruiyu added the push Auto push patch series in PR if all checks pass label Mar 17, 2021
@mergify
Copy link

mergify bot commented Mar 17, 2021

PR can not be merged due to a PatchCheck failure. Please resolve and resubmit

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>
Reviewed-by: Eric Dong <eric.dong@intel.com>
Acked-by: Laszlo Ersek <lersek@redhat.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
@mergify mergify bot merged commit 030ba30 into tianocore:master Mar 17, 2021
@niruiyu niruiyu deleted the cpu/no_debug_message_ap branch April 22, 2021 08:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
push Auto push patch series in PR if all checks pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant