From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.132.183.28; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 8A00D21157410 for ; Tue, 25 Sep 2018 07:08:26 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id EE7FF30001E2; Tue, 25 Sep 2018 14:08:25 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-71.rdu2.redhat.com [10.10.120.71]) by smtp.corp.redhat.com (Postfix) with ESMTP id 5A2715D995; Tue, 25 Sep 2018 14:08:23 +0000 (UTC) To: Ruiyu Ni , edk2-devel@lists.01.org Cc: Michael D Kinney , Jiewen Yao , Liming Gao References: <20180913095046.9480-1-ruiyu.ni@intel.com> From: Laszlo Ersek Message-ID: <09dfe482-b329-9059-e1cd-d1a0b99bc2dc@redhat.com> Date: Tue, 25 Sep 2018 16:08:22 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20180913095046.9480-1-ruiyu.ni@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.40]); Tue, 25 Sep 2018 14:08:26 +0000 (UTC) Subject: Re: [PATCH v3] MdePkg/SynchronizationLib: fix Interlocked[De|In]crement return value X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 25 Sep 2018 14:08:27 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 > Cc: Jiewen Yao > Cc: Liming Gao > Cc: Michael D Kinney > --- > 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