From: "Gao, Liming" <liming.gao@intel.com>
To: "Kinney, Michael D" <michael.d.kinney@intel.com>,
"Ni, Ruiyu" <ruiyu.ni@intel.com>,
"'edk2-devel@lists.01.org'" <edk2-devel@lists.01.org>
Cc: "Yao, Jiewen" <jiewen.yao@intel.com>
Subject: Re: [PATCH] MdePkg/SynchronizationLib: fix Interlocked[De|In]crement return value
Date: Wed, 12 Sep 2018 15:08:19 +0000 [thread overview]
Message-ID: <4A89E2EF3DFEDB4C8BFDE51014F606A14E2F666F@SHSMSX104.ccr.corp.intel.com> (raw)
In-Reply-To: <E92EE9817A31E24EB0585FDF735412F5B8AD67F1@ORSMSX113.amr.corp.intel.com>
Mike:
For this case InternalSyncDecrement, there are three implementations in SynchronizationLib library.
One is C inline assembly for GCC, another is intrinsic function for MSFT, last one is nasm implementation for INTEL compiler. We would like to keep one implementation. If intrinsic one is required to be kept, that means at least two implementations are required.
Thanks
Liming
> -----Original Message-----
> From: Kinney, Michael D
> Sent: Wednesday, September 12, 2018 10:53 PM
> 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
>
> Ray,
>
> Yes. This provides best size with optimizations enabled.
>
> Mike
>
> > -----Original Message-----
> > From: Ni, Ruiyu
> > Sent: Tuesday, September 11, 2018 7:16 PM
> > To: Ni, Ruiyu <ruiyu.ni@intel.com>; 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
> >
> > Mike,
> > Do you require to still use MSVC intrinsic function?
> >
> > > -----Original Message-----
> > > From: edk2-devel [mailto:edk2-devel-
> > bounces@lists.01.org] On Behalf Of Ni,
> > > Ruiyu
> > > Sent: Tuesday, September 11, 2018 10:27 AM
> > > 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: [edk2] [PATCH] MdePkg/SynchronizationLib:
> > fix
> > > Interlocked[De|In]crement return value
> > >
> > > The reason I didn't remove the GCC version is because
> > Liming told me that there
> > > is a XCODE issue which prevents using NASM for
> > library.
> > >
> > > Thanks/Ray
> > >
> > > > -----Original Message-----
> > > > From: Ni, Ruiyu
> > > > Sent: Tuesday, September 11, 2018 10:25 AM
> > > > To: Kinney, Michael D <michael.d.kinney@intel.com>;
> > 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
> > > >
> > > > 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
> > >
> > > _______________________________________________
> > > edk2-devel mailing list
> > > edk2-devel@lists.01.org
> > > https://lists.01.org/mailman/listinfo/edk2-devel
next prev parent reply other threads:[~2018-09-12 15:08 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
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 [this message]
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=4A89E2EF3DFEDB4C8BFDE51014F606A14E2F666F@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