From: "Kinney, Michael D" <michael.d.kinney@intel.com>
To: "Ni, Ruiyu" <ruiyu.ni@intel.com>,
"edk2-devel@lists.01.org" <edk2-devel@lists.01.org>,
"Kinney, Michael D" <michael.d.kinney@intel.com>
Cc: "Yao, Jiewen" <jiewen.yao@intel.com>,
"Gao, Liming" <liming.gao@intel.com>
Subject: Re: [PATCH] MdePkg/SynchronizationLib: fix Interlocked[De|In]crement return value
Date: Mon, 10 Sep 2018 16:38:31 +0000 [thread overview]
Message-ID: <E92EE9817A31E24EB0585FDF735412F5B8AD5778@ORSMSX113.amr.corp.intel.com> (raw)
In-Reply-To: <20180910100600.276668-1-ruiyu.ni@intel.com>
Ray,
Why are we removing the use of intrinsics from
MSFT builds? We should keep those for more efficient
code generation if the intrinsic has the correct
behavior.
Thanks,
Mike
> -----Original Message-----
> From: Ni, Ruiyu
> Sent: Monday, September 10, 2018 3:06 AM
> To: edk2-devel@lists.01.org
> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Gao, Liming
> <liming.gao@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>
> Subject: [PATCH] MdePkg/SynchronizationLib: fix
> Interlocked[De|In]crement return value
>
> 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.
>
> 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
> ++--
> .../BaseSynchronizationLib/Synchronization.c | 6
> +--
> .../BaseSynchronizationLib/SynchronizationGcc.c | 6
> +--
> .../BaseSynchronizationLib/SynchronizationMsc.c | 6
> +--
> .../Library/BaseSynchronizationLib/X64/GccInline.c | 19
> +++++----
> .../X64/InterlockedDecrement.c | 46
> ----------------------
> .../X64/InterlockedDecrement.nasm | 8
> ++--
> .../X64/InterlockedIncrement.c | 46
> ----------------------
> .../X64/InterlockedIncrement.nasm | 5
> ++-
> 16 files changed, 52 insertions(+), 240 deletions(-)
> delete mode 100644
> MdePkg/Library/BaseSynchronizationLib/Ia32/InterlockedDe
> crement.c
> delete mode 100644
> MdePkg/Library/BaseSynchronizationLib/Ia32/InterlockedIn
> crement.c
> delete mode 100644
> MdePkg/Library/BaseSynchronizationLib/X64/InterlockedDec
> rement.c
> delete mode 100644
> MdePkg/Library/BaseSynchronizationLib/X64/InterlockedInc
> rement.c
>
> diff --git a/MdePkg/Include/Library/SynchronizationLib.h
> b/MdePkg/Include/Library/SynchronizationLib.h
> index da69f6ff5e..ce3bce04f5 100644
> --- a/MdePkg/Include/Library/SynchronizationLib.h
> +++ b/MdePkg/Include/Library/SynchronizationLib.h
> @@ -144,8 +144,7 @@ ReleaseSpinLock (
>
> Performs an atomic increment of the 32-bit unsigned
> integer specified by
> Value and returns the incremented value. The
> increment operation must be
> - performed using MP safe mechanisms. The state of the
> return value is not
> - guaranteed to be MP safe.
> + performed using MP safe mechanisms.
>
> If Value is NULL, then ASSERT().
>
> @@ -166,8 +165,7 @@ InterlockedIncrement (
>
> Performs an atomic decrement of the 32-bit unsigned
> integer specified by
> Value and returns the decremented value. The
> decrement operation must be
> - performed using MP safe mechanisms. The state of the
> return value is not
> - guaranteed to be MP safe.
> + performed using MP safe mechanisms.
>
> If Value is NULL, then ASSERT().
>
> diff --git
> a/MdePkg/Library/BaseSynchronizationLib/BaseSynchronizat
> ionLib.inf
> b/MdePkg/Library/BaseSynchronizationLib/BaseSynchronizat
> ionLib.inf
> index 0be1d4331f..b971cd138d 100755
> ---
> a/MdePkg/Library/BaseSynchronizationLib/BaseSynchronizat
> ionLib.inf
> +++
> b/MdePkg/Library/BaseSynchronizationLib/BaseSynchronizat
> ionLib.inf
> @@ -34,9 +34,9 @@ [Sources.IA32]
> Ia32/InterlockedCompareExchange64.c | MSFT
> Ia32/InterlockedCompareExchange32.c | MSFT
> Ia32/InterlockedCompareExchange16.c | MSFT
> - Ia32/InterlockedDecrement.c | MSFT
> - Ia32/InterlockedIncrement.c | MSFT
> - SynchronizationMsc.c | MSFT
> + Ia32/InterlockedDecrement.nasm | MSFT
> + Ia32/InterlockedIncrement.nasm | MSFT
> + SynchronizationMsc.c | MSFT
>
> Ia32/InterlockedCompareExchange64.nasm| INTEL
> Ia32/InterlockedCompareExchange32.nasm| INTEL
> @@ -54,17 +54,15 @@ [Sources.X64]
> X64/InterlockedCompareExchange64.c | MSFT
> X64/InterlockedCompareExchange32.c | MSFT
> X64/InterlockedCompareExchange16.c | MSFT
> + X64/InterlockedDecrement.nasm | MSFT
> + X64/InterlockedIncrement.nasm | MSFT
> + SynchronizationMsc.c | MSFT
>
> X64/InterlockedCompareExchange64.nasm| INTEL
> X64/InterlockedCompareExchange32.nasm| INTEL
> X64/InterlockedCompareExchange16.nasm| INTEL
> -
> - X64/InterlockedDecrement.c | MSFT
> - X64/InterlockedIncrement.c | MSFT
> - SynchronizationMsc.c | MSFT
> -
> - X64/InterlockedDecrement.nasm| INTEL
> - X64/InterlockedIncrement.nasm| INTEL
> + X64/InterlockedDecrement.nasm | INTEL
> + X64/InterlockedIncrement.nasm | INTEL
> Synchronization.c | INTEL
>
> Ia32/InternalGetSpinLockProperties.c | GCC
> diff --git
> a/MdePkg/Library/BaseSynchronizationLib/BaseSynchronizat
> ionLibInternals.h
> b/MdePkg/Library/BaseSynchronizationLib/BaseSynchronizat
> ionLibInternals.h
> index 37edd7c188..8c363a8585 100644
> ---
> a/MdePkg/Library/BaseSynchronizationLib/BaseSynchronizat
> ionLibInternals.h
> +++
> b/MdePkg/Library/BaseSynchronizationLib/BaseSynchronizat
> ionLibInternals.h
> @@ -27,8 +27,7 @@
>
> Performs an atomic increment of the 32-bit unsigned
> integer specified by
> Value and returns the incremented value. The
> increment operation must be
> - performed using MP safe mechanisms. The state of the
> return value is not
> - guaranteed to be MP safe.
> + performed using MP safe mechanisms.
>
> @param Value A pointer to the 32-bit value to
> increment.
>
> @@ -47,8 +46,7 @@ InternalSyncIncrement (
>
> Performs an atomic decrement of the 32-bit unsigned
> integer specified by
> Value and returns the decrement value. The decrement
> operation must be
> - performed using MP safe mechanisms. The state of the
> return value is not
> - guaranteed to be MP safe.
> + performed using MP safe mechanisms.
>
> @param Value A pointer to the 32-bit value to
> decrement.
>
> diff --git
> a/MdePkg/Library/BaseSynchronizationLib/Ia32/GccInline.c
> b/MdePkg/Library/BaseSynchronizationLib/Ia32/GccInline.c
> index 4ab293f243..d82e0205f5 100644
> ---
> a/MdePkg/Library/BaseSynchronizationLib/Ia32/GccInline.c
> +++
> b/MdePkg/Library/BaseSynchronizationLib/Ia32/GccInline.c
> @@ -20,8 +20,7 @@
>
> Performs an atomic increment of the 32-bit unsigned
> integer specified by
> Value and returns the incremented value. The
> increment operation must be
> - performed using MP safe mechanisms. The state of the
> return value is not
> - guaranteed to be MP safe.
> + performed using MP safe mechanisms.
>
> @param Value A pointer to the 32-bit value to
> increment.
>
> @@ -37,9 +36,10 @@ InternalSyncIncrement (
> UINT32 Result;
>
> __asm__ __volatile__ (
> + "movl $1, %%eax \n\t"
> "lock \n\t"
> - "incl %2 \n\t"
> - "movl %2, %%eax "
> + "xadd %%eax, %2 \n\t"
> + "inc %%eax "
> : "=a" (Result), // %0
> "=m" (*Value) // %1
> : "m" (*Value) // %2
> @@ -57,8 +57,7 @@ InternalSyncIncrement (
>
> Performs an atomic decrement of the 32-bit unsigned
> integer specified by
> Value and returns the decremented value. The
> decrement operation must be
> - performed using MP safe mechanisms. The state of the
> return value is not
> - guaranteed to be MP safe.
> + performed using MP safe mechanisms.
>
> @param Value A pointer to the 32-bit value to
> decrement.
>
> @@ -74,9 +73,10 @@ InternalSyncDecrement (
> UINT32 Result;
>
> __asm__ __volatile__ (
> - "lock \n\t"
> - "decl %2 \n\t"
> - "movl %2, %%eax "
> + "movl $-1, %%eax \n\t"
> + "lock \n\t"
> + "xadd %%eax, %2 \n\t"
> + "dec %%eax "
> : "=a" (Result), // %0
> "=m" (*Value) // %1
> : "m" (*Value) // %2
> diff --git
> a/MdePkg/Library/BaseSynchronizationLib/Ia32/Interlocked
> Decrement.c
> b/MdePkg/Library/BaseSynchronizationLib/Ia32/Interlocked
> Decrement.c
> deleted file mode 100644
> index 354a0e7ab1..0000000000
> ---
> a/MdePkg/Library/BaseSynchronizationLib/Ia32/Interlocked
> Decrement.c
> +++ /dev/null
> @@ -1,42 +0,0 @@
> -/** @file
> - InterlockedDecrement function
> -
> - Copyright (c) 2006 - 2016, Intel Corporation. All
> rights reserved.<BR>
> - This program and the accompanying materials
> - are licensed and made available under the terms and
> conditions of the BSD License
> - which accompanies this distribution. The full text
> of the license may be found at
> - http://opensource.org/licenses/bsd-license.php.
> -
> - THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON
> AN "AS IS" BASIS,
> - WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND,
> EITHER EXPRESS OR IMPLIED.
> -
> -**/
> -
> -
> -
> -
> -/**
> - Performs an atomic decrement of an 32-bit unsigned
> integer.
> -
> - Performs an atomic decrement of the 32-bit unsigned
> integer specified by
> - Value and returns the decrement value. The decrement
> operation must be
> - performed using MP safe mechanisms. The state of the
> return value is not
> - guaranteed to be MP safe.
> -
> - @param Value A pointer to the 32-bit value to
> decrement.
> -
> - @return The decrement value.
> -
> -**/
> -UINT32
> -EFIAPI
> -InternalSyncDecrement (
> - IN volatile UINT32 *Value
> - )
> -{
> - _asm {
> - mov eax, Value
> - lock dec dword ptr [eax]
> - mov eax, [eax]
> - }
> -}
> diff --git
> a/MdePkg/Library/BaseSynchronizationLib/Ia32/Interlocked
> Decrement.nasm
> b/MdePkg/Library/BaseSynchronizationLib/Ia32/Interlocked
> Decrement.nasm
> index 4c46041186..dd5a8de3ed 100644
> ---
> a/MdePkg/Library/BaseSynchronizationLib/Ia32/Interlocked
> Decrement.nasm
> +++
> b/MdePkg/Library/BaseSynchronizationLib/Ia32/Interlocked
> Decrement.nasm
> @@ -1,6 +1,6 @@
> ;------------------------------------------------------
> ------------------------
> ;
> -; Copyright (c) 2006 - 2016, Intel Corporation. All
> rights reserved.<BR>
> +; Copyright (c) 2006 - 2018, Intel Corporation. All
> rights reserved.<BR>
> ; This program and the accompanying materials
> ; are licensed and made available under the terms and
> conditions of the BSD License
> ; which accompanies this distribution. The full text
> of the license may be found at
> @@ -32,8 +32,8 @@
> ;------------------------------------------------------
> ------------------------
> global ASM_PFX(InternalSyncDecrement)
> ASM_PFX(InternalSyncDecrement):
> - mov eax, [esp + 4]
> - lock dec dword [eax]
> - mov eax, [eax]
> + mov ecx, [esp + 4]
> + mov eax, 0FFFFFFFFh
> + lock xadd dword [ecx], eax
> + dec eax
> ret
> -
> diff --git
> a/MdePkg/Library/BaseSynchronizationLib/Ia32/Interlocked
> Increment.c
> b/MdePkg/Library/BaseSynchronizationLib/Ia32/Interlocked
> Increment.c
> deleted file mode 100644
> index c61a550119..0000000000
> ---
> a/MdePkg/Library/BaseSynchronizationLib/Ia32/Interlocked
> Increment.c
> +++ /dev/null
> @@ -1,43 +0,0 @@
> -/** @file
> - InterLockedIncrement function
> -
> - Copyright (c) 2006 - 2016, Intel Corporation. All
> rights reserved.<BR>
> - This program and the accompanying materials
> - are licensed and made available under the terms and
> conditions of the BSD License
> - which accompanies this distribution. The full text
> of the license may be found at
> - http://opensource.org/licenses/bsd-license.php.
> -
> - THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON
> AN "AS IS" BASIS,
> - WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND,
> EITHER EXPRESS OR IMPLIED.
> -
> -**/
> -
> -
> -
> -
> -/**
> - Performs an atomic increment of an 32-bit unsigned
> integer.
> -
> - Performs an atomic increment of the 32-bit unsigned
> integer specified by
> - Value and returns the incremented value. The
> increment operation must be
> - performed using MP safe mechanisms. The state of the
> return value is not
> - guaranteed to be MP safe.
> -
> - @param Value A pointer to the 32-bit value to
> increment.
> -
> - @return The incremented value.
> -
> -**/
> -UINT32
> -EFIAPI
> -InternalSyncIncrement (
> - IN volatile UINT32 *Value
> - )
> -{
> - _asm {
> - mov eax, Value
> - lock inc dword ptr [eax]
> - mov eax, [eax]
> - }
> -}
> -
> diff --git
> a/MdePkg/Library/BaseSynchronizationLib/Ia32/Interlocked
> Increment.nasm
> b/MdePkg/Library/BaseSynchronizationLib/Ia32/Interlocked
> Increment.nasm
> index 3902c73275..0677e4bf78 100644
> ---
> a/MdePkg/Library/BaseSynchronizationLib/Ia32/Interlocked
> Increment.nasm
> +++
> b/MdePkg/Library/BaseSynchronizationLib/Ia32/Interlocked
> Increment.nasm
> @@ -32,8 +32,9 @@
> ;------------------------------------------------------
> ------------------------
> global ASM_PFX(InternalSyncIncrement)
> ASM_PFX(InternalSyncIncrement):
> - mov eax, [esp + 4]
> - lock inc dword [eax]
> - mov eax, [eax]
> + mov ecx, [esp + 4]
> + mov eax, 1
> + lock xadd dword [ecx], eax
> + inc eax
> ret
>
> diff --git
> a/MdePkg/Library/BaseSynchronizationLib/Synchronization.
> c
> b/MdePkg/Library/BaseSynchronizationLib/Synchronization.
> c
> index 76c5a1275c..0ab59ca702 100644
> ---
> a/MdePkg/Library/BaseSynchronizationLib/Synchronization.
> c
> +++
> b/MdePkg/Library/BaseSynchronizationLib/Synchronization.
> c
> @@ -231,8 +231,7 @@ ReleaseSpinLock (
>
> Performs an atomic increment of the 32-bit unsigned
> integer specified by
> Value and returns the incremented value. The
> increment operation must be
> - performed using MP safe mechanisms. The state of the
> return value is not
> - guaranteed to be MP safe.
> + performed using MP safe mechanisms.
>
> If Value is NULL, then ASSERT().
>
> @@ -256,8 +255,7 @@ InterlockedIncrement (
>
> Performs an atomic decrement of the 32-bit unsigned
> integer specified by
> Value and returns the decremented value. The
> decrement operation must be
> - performed using MP safe mechanisms. The state of the
> return value is not
> - guaranteed to be MP safe.
> + performed using MP safe mechanisms.
>
> If Value is NULL, then ASSERT().
>
> diff --git
> a/MdePkg/Library/BaseSynchronizationLib/SynchronizationG
> cc.c
> b/MdePkg/Library/BaseSynchronizationLib/SynchronizationG
> cc.c
> index 5ac548b19f..177739d3da 100644
> ---
> a/MdePkg/Library/BaseSynchronizationLib/SynchronizationG
> cc.c
> +++
> b/MdePkg/Library/BaseSynchronizationLib/SynchronizationG
> cc.c
> @@ -247,8 +247,7 @@ ReleaseSpinLock (
>
> Performs an atomic increment of the 32-bit unsigned
> integer specified by
> Value and returns the incremented value. The
> increment operation must be
> - performed using MP safe mechanisms. The state of the
> return value is not
> - guaranteed to be MP safe.
> + performed using MP safe mechanisms.
>
> If Value is NULL, then ASSERT().
>
> @@ -272,8 +271,7 @@ InterlockedIncrement (
>
> Performs an atomic decrement of the 32-bit unsigned
> integer specified by
> Value and returns the decremented value. The
> decrement operation must be
> - performed using MP safe mechanisms. The state of the
> return value is not
> - guaranteed to be MP safe.
> + performed using MP safe mechanisms.
>
> If Value is NULL, then ASSERT().
>
> diff --git
> a/MdePkg/Library/BaseSynchronizationLib/SynchronizationM
> sc.c
> b/MdePkg/Library/BaseSynchronizationLib/SynchronizationM
> sc.c
> index e3298c8ab4..6ab2d71870 100644
> ---
> a/MdePkg/Library/BaseSynchronizationLib/SynchronizationM
> sc.c
> +++
> b/MdePkg/Library/BaseSynchronizationLib/SynchronizationM
> sc.c
> @@ -249,8 +249,7 @@ ReleaseSpinLock (
>
> Performs an atomic increment of the 32-bit unsigned
> integer specified by
> Value and returns the incremented value. The
> increment operation must be
> - performed using MP safe mechanisms. The state of the
> return value is not
> - guaranteed to be MP safe.
> + performed using MP safe mechanisms.
>
> If Value is NULL, then ASSERT().
>
> @@ -274,8 +273,7 @@ InterlockedIncrement (
>
> Performs an atomic decrement of the 32-bit unsigned
> integer specified by
> Value and returns the decremented value. The
> decrement operation must be
> - performed using MP safe mechanisms. The state of the
> return value is not
> - guaranteed to be MP safe.
> + performed using MP safe mechanisms.
>
> If Value is NULL, then ASSERT().
>
> diff --git
> a/MdePkg/Library/BaseSynchronizationLib/X64/GccInline.c
> b/MdePkg/Library/BaseSynchronizationLib/X64/GccInline.c
> index 5224dd063f..4c4d6e3fc7 100644
> ---
> a/MdePkg/Library/BaseSynchronizationLib/X64/GccInline.c
> +++
> b/MdePkg/Library/BaseSynchronizationLib/X64/GccInline.c
> @@ -15,14 +15,12 @@
>
>
>
> -
> /**
> Performs an atomic increment of an 32-bit unsigned
> integer.
>
> Performs an atomic increment of the 32-bit unsigned
> integer specified by
> Value and returns the incremented value. The
> increment operation must be
> - performed using MP safe mechanisms. The state of the
> return value is not
> - guaranteed to be MP safe.
> + performed using MP safe mechanisms.
>
> @param Value A pointer to the 32-bit value to
> increment.
>
> @@ -38,9 +36,10 @@ InternalSyncIncrement (
> UINT32 Result;
>
> __asm__ __volatile__ (
> + "movl $1, %%eax \n\t"
> "lock \n\t"
> - "incl %2 \n\t"
> - "mov %2, %%eax "
> + "xadd %%eax, %2 \n\t"
> + "inc %%eax "
> : "=a" (Result), // %0
> "=m" (*Value) // %1
> : "m" (*Value) // %2
> @@ -57,8 +56,7 @@ InternalSyncIncrement (
>
> Performs an atomic decrement of the 32-bit unsigned
> integer specified by
> Value and returns the decremented value. The
> decrement operation must be
> - performed using MP safe mechanisms. The state of the
> return value is not
> - guaranteed to be MP safe.
> + performed using MP safe mechanisms.
>
> @param Value A pointer to the 32-bit value to
> decrement.
>
> @@ -74,9 +72,10 @@ InternalSyncDecrement (
> UINT32 Result;
>
> __asm__ __volatile__ (
> - "lock \n\t"
> - "decl %2 \n\t"
> - "mov %2, %%eax "
> + "movl $-1, %%eax \n\t"
> + "lock \n\t"
> + "xadd %%eax, %2 \n\t"
> + "dec %%eax "
> : "=a" (Result), // %0
> "=m" (*Value) // %1
> : "m" (*Value) // %2
> diff --git
> a/MdePkg/Library/BaseSynchronizationLib/X64/InterlockedD
> ecrement.c
> b/MdePkg/Library/BaseSynchronizationLib/X64/InterlockedD
> ecrement.c
> deleted file mode 100644
> index da402cd4c6..0000000000
> ---
> a/MdePkg/Library/BaseSynchronizationLib/X64/InterlockedD
> ecrement.c
> +++ /dev/null
> @@ -1,46 +0,0 @@
> -/** @file
> - InterlockedDecrement function
> -
> - Copyright (c) 2006 - 2016, Intel Corporation. All
> rights reserved.<BR>
> - This program and the accompanying materials
> - are licensed and made available under the terms and
> conditions of the BSD License
> - which accompanies this distribution. The full text
> of the license may be found at
> - http://opensource.org/licenses/bsd-license.php.
> -
> - THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON
> AN "AS IS" BASIS,
> - WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND,
> EITHER EXPRESS OR IMPLIED.
> -
> -**/
> -
> -/**
> - Microsoft Visual Studio 7.1 Function Prototypes for
> I/O Intrinsics.
> -**/
> -
> -long _InterlockedDecrement(
> - long * lpAddend
> -);
> -
> -#pragma intrinsic(_InterlockedDecrement)
> -
> -/**
> - Performs an atomic decrement of an 32-bit unsigned
> integer.
> -
> - Performs an atomic decrement of the 32-bit unsigned
> integer specified by
> - Value and returns the decrement value. The decrement
> operation must be
> - performed using MP safe mechanisms. The state of the
> return value is not
> - guaranteed to be MP safe.
> -
> - @param Value A pointer to the 32-bit value to
> decrement.
> -
> - @return The decrement value.
> -
> -**/
> -UINT32
> -EFIAPI
> -InternalSyncDecrement (
> - IN volatile UINT32 *Value
> - )
> -{
> - return _InterlockedDecrement ((long
> *)(UINTN)(Value));
> -}
> -
> diff --git
> a/MdePkg/Library/BaseSynchronizationLib/X64/InterlockedD
> ecrement.nasm
> b/MdePkg/Library/BaseSynchronizationLib/X64/InterlockedD
> ecrement.nasm
> index 60f43111fe..dabdca945e 100644
> ---
> a/MdePkg/Library/BaseSynchronizationLib/X64/InterlockedD
> ecrement.nasm
> +++
> b/MdePkg/Library/BaseSynchronizationLib/X64/InterlockedD
> ecrement.nasm
> @@ -1,6 +1,6 @@
> ;------------------------------------------------------
> ------------------------
> ;
> -; Copyright (c) 2006 - 2016, Intel Corporation. All
> rights reserved.<BR>
> +; Copyright (c) 2006 - 2018, Intel Corporation. All
> rights reserved.<BR>
> ; This program and the accompanying materials
> ; are licensed and made available under the terms and
> conditions of the BSD License
> ; which accompanies this distribution. The full text
> of the license may be found at
> @@ -33,7 +33,7 @@
> ;------------------------------------------------------
> ------------------------
> global ASM_PFX(InternalSyncDecrement)
> ASM_PFX(InternalSyncDecrement):
> - lock dec dword [rcx]
> - mov eax, [rcx]
> + mov eax, 0FFFFFFFFh
> + lock xadd dword [rcx], eax
> + dec eax
> ret
> -
> diff --git
> a/MdePkg/Library/BaseSynchronizationLib/X64/InterlockedI
> ncrement.c
> b/MdePkg/Library/BaseSynchronizationLib/X64/InterlockedI
> ncrement.c
> deleted file mode 100644
> index bbd1384602..0000000000
> ---
> a/MdePkg/Library/BaseSynchronizationLib/X64/InterlockedI
> ncrement.c
> +++ /dev/null
> @@ -1,46 +0,0 @@
> -/** @file
> - InterLockedIncrement function
> -
> - Copyright (c) 2006 - 2016, Intel Corporation. All
> rights reserved.<BR>
> - This program and the accompanying materials
> - are licensed and made available under the terms and
> conditions of the BSD License
> - which accompanies this distribution. The full text
> of the license may be found at
> - http://opensource.org/licenses/bsd-license.php.
> -
> - THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON
> AN "AS IS" BASIS,
> - WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND,
> EITHER EXPRESS OR IMPLIED.
> -
> -**/
> -
> -/**
> - Microsoft Visual Studio 7.1 Function Prototypes for
> I/O Intrinsics.
> -**/
> -
> -long _InterlockedIncrement(
> - long * lpAddend
> -);
> -
> -#pragma intrinsic(_InterlockedIncrement)
> -
> -/**
> - Performs an atomic increment of an 32-bit unsigned
> integer.
> -
> - Performs an atomic increment of the 32-bit unsigned
> integer specified by
> - Value and returns the incremented value. The
> increment operation must be
> - performed using MP safe mechanisms. The state of the
> return value is not
> - guaranteed to be MP safe.
> -
> - @param Value A pointer to the 32-bit value to
> increment.
> -
> - @return The incremented value.
> -
> -**/
> -UINT32
> -EFIAPI
> -InternalSyncIncrement (
> - IN volatile UINT32 *Value
> - )
> -{
> - return _InterlockedIncrement ((long
> *)(UINTN)(Value));
> -}
> -
> diff --git
> a/MdePkg/Library/BaseSynchronizationLib/X64/InterlockedI
> ncrement.nasm
> b/MdePkg/Library/BaseSynchronizationLib/X64/InterlockedI
> ncrement.nasm
> index 7f877b5774..77379d998e 100644
> ---
> a/MdePkg/Library/BaseSynchronizationLib/X64/InterlockedI
> ncrement.nasm
> +++
> b/MdePkg/Library/BaseSynchronizationLib/X64/InterlockedI
> ncrement.nasm
> @@ -33,7 +33,8 @@
> ;------------------------------------------------------
> ------------------------
> global ASM_PFX(InternalSyncIncrement)
> ASM_PFX(InternalSyncIncrement):
> - lock inc dword [rcx]
> - mov eax, [rcx]
> + mov eax, 1
> + lock xadd dword [rcx], eax
> + inc eax
> ret
>
> --
> 2.16.1.windows.1
next prev parent reply other threads:[~2018-09-10 16:38 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-09-10 10:06 [PATCH] MdePkg/SynchronizationLib: fix Interlocked[De|In]crement return value Ruiyu Ni
2018-09-10 11:37 ` Yao, Jiewen
2018-09-10 14:59 ` Ni, Ruiyu
2018-09-10 15:17 ` Yao, Jiewen
2018-09-10 16:38 ` Kinney, Michael D [this message]
2018-09-11 2:25 ` Ni, Ruiyu
2018-09-11 2:26 ` Ni, Ruiyu
2018-09-12 2:16 ` Ni, Ruiyu
2018-09-12 14:53 ` Kinney, Michael D
2018-09-12 15:08 ` Gao, Liming
2018-09-12 15:12 ` Kinney, Michael D
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=E92EE9817A31E24EB0585FDF735412F5B8AD5778@ORSMSX113.amr.corp.intel.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox