[edk2-devel] [Patch 1/1] UefiCpuPkg/Library/MpInitLib: Fix AP VolatileRegisters race condition

[Philippe Mathieu-Daudé]

Hi Michael,

On 1/23/21 3:02 AM, Laszlo Ersek wrote:
> +Tom, comments below
> 
> On 01/22/21 18:10, Michael D Kinney wrote:
>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3182
>>
>> Fix the order of operations in ApWakeupFunction() when PcdCpuApLoopMode
>> is set to HLT mode that uses INIT-SIPI-SIPI to wake APs.  In this mode,
>> volatile state is restored and saved each time a INIT-SIPI-SIPI is sent
>> to an AP to request a function to be executed on the AP.  When the
>> function is completed the volatile state of the AP is saved.  However,
>> the counters NumApsExecuting and FinishedCount are updated before
>> the volatile state is saved.  This allows for a race condition window
>> for the BSP that is waiting on these counters to request a new
>> INIT-SIPI-SIPI before all the APs have completely saved their volatile
>> state.  The fix is to save the AP volatile state before updating the
>> NumApsExecuting and FinishedCount counters.
>>
>> Cc: Eric Dong <eric.dong at intel.com>
>> Cc: Ray Ni <ray.ni at intel.com>
>> Cc: Laszlo Ersek <lersek at redhat.com>
>> Cc: Rahul Kumar <rahul1.kumar at intel.com>
>> Signed-off-by: Michael D Kinney <michael.d.kinney at intel.com>
>> ---
>>  UefiCpuPkg/Library/MpInitLib/MpLib.c | 31 ++++++++++++++++------------
>>  1 file changed, 18 insertions(+), 13 deletions(-)
...

> When we squash steps #1 through #3, we get *precisely* the posted patch.
> 
> A further improvement from the patch is that the relative order between
> incrementing FinishedCount, and decrementing NumApsExecuting, is now
> consistent between the SEV-ES-enabled and SEV-ES-disabled cases.
> Regardless of SEV-ES, we now incrementing FinishedCount first, and
> decrement NumApsExecuting second. Without the patch, the relative order
> between these two operations gets inverted upon negating the SEV-ES
> enablement.
> 
> I would prefer this patch to be broken up into the three steps that I
> reconstructed above (steps #1 and #2 being no-op refactorings), but I
> don't insist.

I agree with Laszlo, it was easier to understand your changes looking
at his explanation. Eventually adding Star's assert() on top.

Regards,

Phil.



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#70769): https://edk2.groups.io/g/devel/message/70769
Mute This Topic: https://groups.io/mt/80035510/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-