[edk2-devel] [PATCH v4 2/3] ArmPkg: implement EFI_MP_SERVICES_PROTOCOL based on PSCI calls

[Rebecca Cran]

On 1/12/23 19:01, Kun Qin wrote:

>>> On 1/4/2023 7:37 AM, Rebecca Cran wrote:
>>>> +  if (WaitEvent != NULL) {
>>>> +    // Non Blocking
>>>> +    if (Finished != NULL) {
>>>> +      mCpuMpData.SingleApFinished = Finished;
>>>> +      *Finished                   = FALSE;
>>>> +    }
>>>> +
>>>> +    mCpuMpData.WaitEvent = WaitEvent;
> [KQ-3] Similar to the above [KQ] comment, for a wait event on the single 
> AP, I think there should be a designated
> wait event for each CPU available?

Yes! I've made that change in the v5 patch series.

>>>> +    Status = gBS->SetTimer (CpuData->CheckThisAPEvent, TimerCancel, 
>>>> 0);
> [KQ-3] Should we leave this timer keep checking the status of this AP 
> even the time is up? Otherwise, there will
> still be no mechanism to recover the CPU state to "Idle" if it ever 
> times out and this CPU is essentially unusable for
> the rest of this boot in UEFI.

I think I've fixed this by allowing CpuStateFinished as a starting 
state, or resetting CpuStateFinished back to CpuStateIdle (depending on 
the function being called).

>>>> +    if (mCpuMpData.WaitEvent != NULL) {
> [KQ-3] The same idea would apply that the event being signaled should be 
> per CPU specific, instead
> of the common event for "start all APs" call.
>>>> +      Status = gBS->SignalEvent (mCpuMpData.WaitEvent);
> [KQ-3] Then we probably want to set this to NULL after signalling this 
> event, just to be on the safe side.

Good point.

-- 
Rebecca Cran


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