Re: [Patch V3 5/5] UefiCpuPkg: Eliminate the second INIT-SIPI-SIPI sequence.

["Yuanhao Xie" <yuanhao.xie@intel.com>]

Hi Ray,

Please check V4 patch series.
Thanks for the feedbacks.

Yuanhao


> +      OriginalValue         = InterlockedCompareExchange32 (
> +                                (UINT32 *)ApStartupSignalBuffer,
> +                                MP_HAND_OFF_SIGNAL,
> +                                0
> +                                );
> +      if (OriginalValue == MP_HAND_OFF_SIGNAL) {
> +        SetApState (&CpuMpData->CpuData[ProcessorNumber], CpuStateReady);
> +      }

1. I don't think InterlockedCompareExchange32() is needed. But it's consistent with the following code. Ok to me.
A: I keep this in V4 patch series.

> +
>        InterlockedCompareExchange32 (
>          (UINT32 *)ApStartupSignalBuffer,
>          WAKEUP_AP_SIGNAL,
> @@ -887,6 +897,32 @@ ApWakeupFunction (
>    }
>  }
> 

> +DxeApEntryPoint (
> +  CPU_MP_DATA  *CpuMpData
> +  )
> +{
> +  UINTN  ProcessorNumber;
> +
> +  GetProcessorNumber (CpuMpData, &ProcessorNumber);  
> + InterlockedIncrement ((UINT32 *)&CpuMpData->FinishedCount);  
> + RestoreVolatileRegisters (&CpuMpData->CpuData[0].VolatileRegisters, 
> + FALSE);  PlaceAPInMwaitLoopOrRunLoop (
> +    CpuMpData->ApLoopMode,
> +    CpuMpData->CpuData[ProcessorNumber].StartupApSignal,
> +    CpuMpData->ApTargetCState
> +    );

2. CpuMpData->ApLoopMode is set to PcdGet8 (PcdCpuApLoopMode) in DXE phase.
     It's possible that when building the Dxe instance of the library, PcdCpuApLoopMode is ApInHltLoop.
     Then above code should not expect the CpuMpData->ApLoopMode is either MwaitLoop or RunLoop.
     But, if the CPU runs here, PcdCpuApLoopMode in PEI phase should not be APInHltLoop.
     So the question becomes: Shall MpInitLib support different PcdCpuApLoopMode values?
     I prefer no for keeping the configuration simple.
     Then, can you please add an assertion before calling SwitchApContext()? (see comments below)

A: I added assertion in V4 patch series.

> +    if (MpHandOff->WaitLoopExecutionMode == sizeof (VOID *)) {

3. can you add "ASSERT (CpuMpData->ApLoopMode != APInHltLoop)" here?

> +      //
> +      // In scenarios where both the PEI and DXE phases run in the same
> +      // execution mode (32bit or 64bit), the BSP triggers
> +      // a start-up signal during the DXE phase to wake up the APs. This causes any
> +      // APs that are currently in a loop on the memory prepared during the PEI
> +      // phase to awaken and run the SwitchContextPerAp procedure. 
> + This
> procedure
> +      // enables the APs to switch to a different memory section and continue their
> +      // looping process there.
> +      //
> +      CpuMpData->FinishedCount = 0;
> +      CpuMpData->InitFlag      = ApInitDone;
> +      SaveCpuMpData (CpuMpData);
> +      SwitchApContext (MpHandOff);
> +      ASSERT (CpuMpData->FinishedCount == (CpuMpData->CpuCount - 1));