public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Ni, Ruiyu" <ruiyu.ni@intel.com>
To: "Kinney, Michael D" <michael.d.kinney@intel.com>,
	"edk2-devel@lists.01.org" <edk2-devel@lists.01.org>
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: Tue, 11 Sep 2018 02:25:11 +0000	[thread overview]
Message-ID: <734D49CCEBEEF84792F5B80ED585239D5BE028E7@SHSMSX104.ccr.corp.intel.com> (raw)
In-Reply-To: <E92EE9817A31E24EB0585FDF735412F5B8AD5778@ORSMSX113.amr.corp.intel.com>

The NASM does the exactly same as the MSFT intrinsic.
I'd like to remove them because I was almost lost in so many versions of Interlocked[De|In]crement.
I'd like to merge them to one version.

Thanks/Ray

> -----Original Message-----
> From: Kinney, Michael D
> Sent: Tuesday, September 11, 2018 12:39 AM
> To: Ni, Ruiyu <ruiyu.ni@intel.com>; 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
> 
> 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



  reply	other threads:[~2018-09-11  2:25 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
2018-09-11  2:25   ` Ni, Ruiyu [this message]
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=734D49CCEBEEF84792F5B80ED585239D5BE028E7@SHSMSX104.ccr.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