Re: [Patch v3 2/3] UefiCpuPkg/MpInitLib: Remove StartCount and volatile definition.

[Laszlo Ersek <lersek@redhat.com>]

On 07/25/18 14:09, Dong, Eric wrote:
> Hi Laszlo,
> 
> 
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Wednesday, July 25, 2018 7:47 PM
>> To: Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org
>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>
>> Subject: Re: [edk2] [Patch v3 2/3] UefiCpuPkg/MpInitLib: Remove StartCount
>> and volatile definition.
>>
>> Hi Eric,
>>
>> On 07/25/18 09:50, Eric Dong wrote:
>>> The StartCount is duplicated with RunningCount, replace it with
>>> RunningCount. Also the volatile for RunningCount is not needed.
>>
>> after staring at this patch for a long time, I think it is correct.
>> However, I suggest that we improve the commit message, because this patch
>> actually does three things:
>>
>> (1) It removes "volatile" from RunningCount.
>>
>> [This is OK, because only the BSP modifies it.]
>>
>> (2) [This is the tricky part.]
>>
>> When we detect a timeout in CheckAllAPs(), and collect the list of failed CPUs,
>> the size of the list is derived from the following difference, before the patch:
>>
>>   StartCount - FinishedCount
>>
>> where "StartCount" is set by the BSP at startup, and FinishedCount is
>> incremented by the APs themselves.
>>
> 
> I think FinishedCount used here is a typo. What the logic from the code context here is want to get the failed Aps and return them. So it should use RunningCount here, right? Also the FinishedCount may be bigger than StartCount if many Aps has been disabled. Right? 

I agree FinishedCount looks questionable / out of place in the original
code.

Thanks
Laszlo