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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
