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

["Ni, Ruiyu" <ruiyu.ni@Intel.com>]

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?

> 
> - 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:)


-- 
Thanks,
Ray