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

["Rebecca Cran" <rebecca@bsdio.com>]

On 1/6/23 15:11, Kun Qin wrote:

> On 1/4/2023 7:37 AM, Rebecca Cran wrote:
>> +  mCpuMpData.Timeout       = TimeoutInMicroseconds;
>> +  mCpuMpData.TimeTaken     = 0;
>> +  mCpuMpData.TimeoutActive = (BOOLEAN)(TimeoutInMicroseconds != 0);
> 
> [KQ] Adding a timeout active flag is correct. However, I think each AP 
> should have its own timeout related
> data (that is Timeout, TimeTaken and TimeoutActive). Because i.e. if 
> this StartupThisAp call is used on AP 1 in
> a non-blocking mode, then a subsequent call on AP 2 is blocking, the 
> common flag and timeout values will
> impact both cores, and create unintended timeout events.

Good catch! Thanks, I've fixed it by adding per-CPU fields for 
StartupThisAP.

>> +  if (mCpuMpData.TimeoutActive && (mCpuMpData.TimeTaken > 
>> mCpuMpData.Timeout)) {
> [KQ] Similar to the other comment, this is probably better using a 
> per-core data to track elapsed time.
> On a side note, if this timeout ever occurs, this core will never be 
> usable in next StartupThisAp calls due
> to this AP state will not be set to Idle even if the AP procedure is 
> complete. Is this the intended behavior?

No, it wasn't. I've added code to StartupThisAP and StartupAllAPs to 
accept CpuStateFinished or reset CPUs in that state back to Idle.

-- 
Rebecca Cran