gUefiCpuPkgTokenSpaceGuid.PcdCpuApTargetCstate definition confusion?

gUefiCpuPkgTokenSpaceGuid.PcdCpuApTargetCstate definition confusion?

[Andrew Fish]

[-- Attachment #1: Type: text/plain, Size: 2505 bytes --]

I was tracking down a Platform C state bug and I noticed the platform code was using this definition [1] for PcdCpuApTargetCstate and wrote that actual value used for Mwait. But the MpInitLIb seems to assume that PcdCpuApTargetCstate is eax[7:4] and it shifts the data left. This ends up with the value being 0 which means C1 for all calculated values. 

So my question is which definition is correct? As far as I can tell eax[3:0] are sub C states and thus being able to pass that value would be useful? 

There is other code in the MpLib [3] that is also doing the shift so it seems intestinal. But given the PCD definition I can see how the platform code set the wrong value. 

[1] PCD Definition
https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/UefiCpuPkg.dec <https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/UefiCpuPkg.dec>

  ## Specifies the AP target C-state for Mwait during POST phase.
  #  The default value 0 means C1 state.
  #  The value is defined as below.<BR><BR>
  # @Prompt The specified AP target C-state for Mwait.
  gUefiCpuPkgTokenSpaceGuid.PcdCpuApTargetCstate|0|UINT8|0x00000007

[2] UefiCpuPkg code
https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm <https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm>
MwaitLoop:
    cli
    mov        eax, esp           ; Set Monitor Address
    xor        ecx, ecx           ; ecx = 0
    xor        edx, edx           ; edx = 0
    monitor
    mov        eax, ebx           ; Mwait Cx, Target C-State per eax[7:4]
    shl        eax, 4
    mwait
    jmp        MwaitLoop


https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c <https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c>
  AsmRelocateApLoopFunc (
    MwaitSupport,
    CpuMpData->ApTargetCState,
    CpuMpData->PmCodeSegment,
    mReservedTopOfApStack - ProcessorNumber * AP_SAFE_STACK_SIZE,
    (UINTN) &mNumberToFinish
    );
[3] ApTargetCState shifting
https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/Library/MpInitLib/MpLib.c <https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/Library/MpInitLib/MpLib.c>
          //
          // Check AP start-up signal again.
          // If AP start-up signal is not set, place AP into
          // the specified C-state
          //
          AsmMwait (CpuMpData->ApTargetCState << 4, 0);

Thanks,

Andrew Fish

[-- Attachment #2: Type: text/html, Size: 35543 bytes --]