Re: [Patch 2/2] UefiCpuPkg/MpInitLib: Enhance waiting for AP initialization logic.

[Laszlo Ersek <lersek@redhat.com>]

Hi Eric,

On 10/25/17 07:42, Dong, Eric wrote:
> Hi Laszlo,
> 
> I think I have clear about your raised issues for Ovmf platform. In this case, I think your platform not suit for this code change.  How about I do below change based on the new code:
> 
> -      if (CpuMpData->CpuCount == 0) {
>         TimedWaitForApFinish (
>           CpuMpData,
>           PcdGet32 (PcdCpuMaxLogicalProcessorNumber) - 1,
>           PcdGet32 (PcdCpuApInitTimeOutInMicroSeconds)
>           );
> -      }
> 
> After this change, for Ovmf can still set
> PcdCpuApInitTimeOutInMicroSeconds to MAX_UINT32 to keep old solution.
> For the real platform, it can set a small value to follow the new
> solution. For this new change, it just wait one more
> PcdCpuApInitTimeOutInMicroSeconds timeout, should not have
> performance impact (This time may also been cost in later while
> (CpuMpData->MpCpuExchangeInfo->NumApsExecuting != 0) loop) or have
> litter performance impact (not cover by the later loop).
The suggested change will work for OVMF, thanks!

(Just please don't forget to un-indent the TimedWaitForApFinish() call
that will become unconditional again.)

--o--

Do you think Brian's concern should be investigated as well (perhaps in
a separate Bugzilla entry)?

Because, I believe, the scheduling pattern that I described earlier
could be possible on physical platforms as well, at least in theory:

>> (2) After at least one AP has started running, the logic expects
>> "NumApsExecuting" to monotonically grow for a while, and then to
>> monotonically decrease, back to zero. For example, if we have 1 BSP and
>> 7 APs, the BSP logic more or less expects the following values in
>> "NumApsExecuting":
>>
>> 1; 2; 3; 4; 5; 6; 7;
>> 6; 5; 4; 3; 2; 1; 0
>>
>>
>> While this may be a valid expectation for physical processors (which actually
>> run in parallel, in the physical world), in a virtual machine, it is not guaranteed.
>> Dependent on hypervisor scheduling artifacts, it is possible that, say, three
>> APs start up *and finish* before the remaining four APs start up *at all*. The
>> INIT-SIPI-SIPI marks all APs for execution / scheduling in the hypervisor, yes,
>> but the actual code execution may commence a lot later. For example, the
>> BSP may witness the following series of values in "NumApsExecuting":
>>
>> 1; 2; 3;
>> 2; 1; 0;
>> 1; 2; 3; 4;
>> 3; 2; 1; 0
>>
>> and the BSP could think that there are 3 APs only, when it sees the first 0
>> value.

I don't know if such a pattern is "likely", "unlikely", or "extremely
unlikely" on physical platforms. I think the pattern is "theoretically
possible" at least.

I suggest that we go ahead with the small patch for the OVMF use case
first, and then open a new BZ if Brian thinks there's a real safety
problem with the code.

... I should note that, with the small patch that's going to fix the
OVMF use case, the previous behavior will be restored for *all*
platforms that continue using their previous PCD values.

The cumulative effect of commit 0594ec417c89 and the upcoming patch will
be that a platform may significantly decrease
"PcdCpuApInitTimeOutInMicroSeconds", if it chooses to, and then the
"NumApsExecuting" loop will take the role of the preexisting
TimedWaitForApFinish() call. If a platform doesn't want that, then it
should keep its "PcdCpuApInitTimeOutInMicroSeconds" as high as before,
and then any implementation details in the "NumApsExecuting" loop will
be irrelevant.

Thanks!
Laszlo