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

[Ni, Ray]

> +      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.

> +
>        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)


> +    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));



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#106419): https://edk2.groups.io/g/devel/message/106419
Mute This Topic: https://groups.io/mt/99782484/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3943202/1813853/130120423/xyzzy [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-