Re: [PATCH v3] MdePkg/SynchronizationLib: fix Interlocked[De|In]crement return value

[Laszlo Ersek <lersek@redhat.com>]

Hi,

On 09/13/18 11:50, Ruiyu Ni wrote:
> Today's InterlockedIncrement()/InterlockedDecrement() guarantees to
> perform atomic increment/decrement but doesn't guarantee the return
> value equals to the new value.
>
> The patch fixes the behavior to use "XADD" instruction to guarantee
> the return value equals to the new value.
>
> The patch calls intrinsic functions for MSVC tool chain, calls the
> NASM implementation for INTEL tool chain and calls GCC inline
> assembly implementation (GccInline.c) for GCC tool chain.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> ---
>  MdePkg/Include/Library/SynchronizationLib.h        |  6 +--
>  .../BaseSynchronizationLib.inf                     | 18 ++++-----
>  .../BaseSynchronizationLibInternals.h              |  6 +--
>  .../BaseSynchronizationLib/Ia32/GccInline.c        | 18 ++++-----
>  .../Ia32/InterlockedDecrement.c                    | 42 ---------------------
>  .../Ia32/InterlockedDecrement.nasm                 | 10 ++---
>  .../Ia32/InterlockedIncrement.c                    | 43 ----------------------
>  .../Ia32/InterlockedIncrement.nasm                 |  7 ++--
>  ...lockedDecrement.c => InterlockedDecrementMsc.c} |  4 +-
>  ...lockedIncrement.c => InterlockedIncrementMsc.c} |  4 +-
>  .../BaseSynchronizationLib/Synchronization.c       |  6 +--
>  .../BaseSynchronizationLib/SynchronizationGcc.c    |  6 +--
>  .../BaseSynchronizationLib/SynchronizationMsc.c    |  6 +--
>  .../Library/BaseSynchronizationLib/X64/GccInline.c | 19 +++++-----
>  .../X64/InterlockedDecrement.nasm                  |  8 ++--
>  .../X64/InterlockedIncrement.nasm                  |  5 ++-
>  16 files changed, 56 insertions(+), 152 deletions(-)
>  delete mode 100644 MdePkg/Library/BaseSynchronizationLib/Ia32/InterlockedDecrement.c
>  delete mode 100644 MdePkg/Library/BaseSynchronizationLib/Ia32/InterlockedIncrement.c
>  rename MdePkg/Library/BaseSynchronizationLib/{X64/InterlockedDecrement.c => InterlockedDecrementMsc.c} (87%)
>  rename MdePkg/Library/BaseSynchronizationLib/{X64/InterlockedIncrement.c => InterlockedIncrementMsc.c} (87%)

This patch (commit 17634d026f96) has regressed OVMF on KVM. Here's the
bisection log:

> git bisect start
> # bad: [17634d026f968c404b039a8d8431b6389dd396ea] MdePkg/SynchronizationLib: fix Interlocked[De|In]crement return value
> git bisect bad 17634d026f968c404b039a8d8431b6389dd396ea
> # good: [997731e796f51df57c113dfd966e818622c3d4aa] UefiCpuPkg: Remove redundant library classes, Ppis and GUIDs
> git bisect good 997731e796f51df57c113dfd966e818622c3d4aa
> # good: [b602265d559b2f2ade4d09ba55652c23922c141f] MdeModulePkg RegularExpressionDxe: Update Oniguruma to 6.9.0
> git bisect good b602265d559b2f2ade4d09ba55652c23922c141f
> # good: [d5b28edd63f4d0fab087c5d6c9db773e5b5adc7d] MdePkg: Add a inf path in MdePkg.dsc
> git bisect good d5b28edd63f4d0fab087c5d6c9db773e5b5adc7d
> # good: [ca3e4f8ab82485edff2cfa7eeb87f71b4be38966] MdePkg UefiPciLibPciRootBridgeIo: Remove redundant dependency
> git bisect good ca3e4f8ab82485edff2cfa7eeb87f71b4be38966
> # first bad commit: [17634d026f968c404b039a8d8431b6389dd396ea] MdePkg/SynchronizationLib: fix Interlocked[De|In]crement return value

The symptom is that the boot gets stuck, with all VCPUs spinning, after
the following log is produced:

> Loading PEIM [CpuMpPei]
> Loading PEIM at 0x000BFEA8000 EntryPoint=0x000BFEADE86 CpuMpPei.efi
> Register PPI Notify: [EfiPeiMemoryDiscoveredPpi]
> Notify: PPI Guid: [EfiPeiMemoryDiscoveredPpi], Peim notify entry point: BFEB53B1
> AP Loop Mode is 1
> WakeupBufferStart = 9F000, WakeupBufferSize = 1000

MpInitLib uses SynchronizationLib:

> UefiCpuPkg/Library/MpInitLib/Microcode.c:241:      AcquireSpinLock(&CpuMpData->MpLock);
> UefiCpuPkg/Library/MpInitLib/Microcode.c:244:      ReleaseSpinLock(&CpuMpData->MpLock);
> UefiCpuPkg/Library/MpInitLib/MpLib.c:117:  AcquireSpinLock (&CpuData->ApLock);
> UefiCpuPkg/Library/MpInitLib/MpLib.c:119:  ReleaseSpinLock (&CpuData->ApLock);
> UefiCpuPkg/Library/MpInitLib/MpLib.c:555:    AcquireSpinLock(&CpuMpData->MpLock);
> UefiCpuPkg/Library/MpInitLib/MpLib.c:557:    ReleaseSpinLock(&CpuMpData->MpLock);
> UefiCpuPkg/Library/MpInitLib/MpLib.c:560:  InitializeSpinLock(&CpuMpData->CpuData[ProcessorNumber].ApLock);
> UefiCpuPkg/Library/MpInitLib/MpLib.c:609:      InterlockedIncrement ((UINT32 *) &CpuMpData->CpuCount);
> UefiCpuPkg/Library/MpInitLib/MpLib.c:637:      InterlockedCompareExchange32 (
> UefiCpuPkg/Library/MpInitLib/MpLib.c:706:    InterlockedIncrement ((UINT32 *) &CpuMpData->FinishedCount);
> UefiCpuPkg/Library/MpInitLib/MpLib.c:707:    InterlockedDecrement ((UINT32 *) &CpuMpData->MpCpuExchangeInfo->NumApsExecuting);
> UefiCpuPkg/Library/MpInitLib/MpLib.c:775:  while (InterlockedCompareExchange32 (
> UefiCpuPkg/Library/MpInitLib/MpLib.c:1645:  InitializeSpinLock(&CpuMpData->MpLock);
> UefiCpuPkg/Library/MpInitLib/MpLib.c:1718:      InitializeSpinLock(&CpuMpData->CpuData[Index].ApLock);

I'll try to collect more information.

Thanks,
Laszlo