Re: [PATCH] UefiCpuPkg/MpInitLib: AP uses memory preceding IDT to store CpuMpData

[Laszlo Ersek <lersek@redhat.com>]

On 06/27/18 08:00, Ni, Ruiyu wrote:
> On 6/27/2018 2:57 AM, Laszlo Ersek wrote:
>> Second, even assuming that PushCpuMpData() and PopCpuMpData() are
>> atomic, the order in which APs complete the AP procedure is not
>> deterministic, and it need not be the exact reverse of the entry order.
>> We may have the following order, for example:
>>
>> - AP 1 writes CpuMpData, and saves the original PEI services pointer on
>>    its stack,
>> - AP 2 writes CpuMpData, and saves the same CpuMpData value (originally
>>    written by AP 1) on its stack,
>> - AP 1 completes the AP procedure and restores the original PEI services
>>    pointer from its stack,
>> - AP 2 completes the AP procedure, and overwrites the PEI services
>>    pointer, with the CpuMpData value from its stack, that was originally
>>    written by AP 1.
>>
> 
> Thanks for the analysis. It's a stupid bug that I should be aware of.
> That can also explain why I cannot reproduce it. It's random.
> 
>> Assuming I (remotely) understand what's going on, I'd (vaguely) suggest
>> three alternatives:
>>
>> - instead of the idea captured in this patch, we should use an APIC ID
>>    search similar to the one done initially in "MpFuncs.nasm",
> 
> Don't understand. Can you kindly explain more?

Roughly, I figured that we could set the pre-IDT pointer on the BSP,
*before* starting up the APs, to a mapping table. The mapping table
would consist of two columns, the first containing the APIC ID (of each
CPU), the second containing AP-specific pointer values. Then, after
starting up the APs, each AP could locate its own row (based on APIC ID)
in the table, and the context pointer that belongs to it.

But, this is likely not useful, as we want to expose just the same one
pointer value to all APs together.

> 
>>
>> - or else, we should stick with the current idea, but use atomic
>>    compare-and-swap operations, so that the original PEI services pointer
>>    value be restored ultimately, at the least,
> 
> I like this idea. Will generate the V2 patch.
> 
>>
>> - or else (possibly the simplest fix), allocate a separate IDT for each
>>    AP, including the UINTN that precedes the (now AP-specific) IDT. This
>>    means that the PEI services pointer*location*  would be separate for
>>    each AP, and no contention would occur.
> 
> I think it's the most complicated fix:)

It might take the most code, but I guess it would be the simplest to
reason about. No data sharing --> no locking necessary, and no races
possible.

Anyway, I saw your v2 (just the subject, and your note that we should
ignore v2 for now). I'll stay tuned for v3.

Meta-question: some of your emails are apparently addressed to the list
only, and not CC'd to people who commented earlier on the thread. Did
you drop CCs deliberately, or is it some kind of mail agent glitch?

Thanks!
Laszlo