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

["Rebecca Cran" <quic_rcran@quicinc.com>]

Sorry, I missed this originally. I've added my replies inline.][

On 9/29/22 12:45, Kun Qin wrote:
> Hi Rebecca,
> 
> Thanks for sending this patch. I have a few questions inline "[KQ]". 
> Could you please help me to
> understand the patch better? Thanks in advance.
> 
> Regards,
> Kun
> 
> On 8/29/2022 8:59 AM, Rebecca Cran wrote:

>> +#define GET_MPIDR_AFFINITY_BITS(x)  ((x) & 0xFF00FFFFFF)
>> +
>> +#define MPIDR_MT_BIT  BIT24
> [KQ]: Is there any reason why this is not defined in a more formal 
> place? I think that will allow the platform to use as well.

That's a good point. I can certainly move it somewhere more proper.

>> +  BOOLEAN     IsBsp;
> [KQ]: Can we add a check to see if BSP is found during this routine? I 
> think failing to identify a BSP might cause other issues in the 
> following services?

We could certainly log an error and abort if IsBsp is never set to TRUE.

>> +  for (Index = 0; Index < mCpuMpData.NumberOfProcessors; Index++) {
>> +    if (GET_MPIDR_AFFINITY_BITS (ArmReadMpidr ()) == 
>> CoreInfo[Index].Mpidr) {
> [KQ]: Does this mean BSP should never set the `MPIDR_MT_BIT` in the 
> gArmMpCoreInfoGuid HOB data? Otherwise, this logic will never be able to 
> find the BSP?

No, here we assume we're running on the BSP, and the code is looking for 
the entry in the CoreInfo array that matches the BSP's affinity fields.

>> +      switch (GetApState (CpuData)) {
>> +        case CpuStateReady:
>> +          SetApProcedure (CpuData, Procedure, ProcedureArgument);
>> +          Status = DispatchCpu (Index);
>> +          if (EFI_ERROR (Status)) {
>> +            CpuData->State = CpuStateIdle;
>> +            Status         = EFI_NOT_READY;
>> +            goto Done;
> [KQ]: Is it really necessary to use "goto Done"? This might cause all 
> the CPUs after this failing index to not reset the CPU state.
> That way, all the subsequent "StartupAllAps" will fail due to 
> "CheckAllCpusReady" returning FALSE on those "dirty cores". Please
> let me know if that is by design or not.

Good point. I'll fix it.


-- 
Rebecca Cran