* [PATCH v3] MdePkg/SynchronizationLib: fix Interlocked[De|In]crement return value
@ 2018-09-13 9:50 Ruiyu Ni
2018-09-21 7:45 ` Gao, Liming
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Ruiyu Ni @ 2018-09-13 9:50 UTC (permalink / raw)
To: edk2-devel; +Cc: Jiewen Yao, Liming Gao, Michael D Kinney
Today's InterlockedIncrement()/InterlockedDecrement() guarantees to
perform atomic increment/decrement but doesn't guarantee the return
value equals to the new value.
The patch fixes the behavior to use "XADD" instruction to guarantee
the return value equals to the new value.
The patch calls intrinsic functions for MSVC tool chain, calls the
NASM implementation for INTEL tool chain and calls GCC inline
assembly implementation (GccInline.c) for GCC tool chain.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
---
MdePkg/Include/Library/SynchronizationLib.h | 6 +--
.../BaseSynchronizationLib.inf | 18 ++++-----
.../BaseSynchronizationLibInternals.h | 6 +--
.../BaseSynchronizationLib/Ia32/GccInline.c | 18 ++++-----
.../Ia32/InterlockedDecrement.c | 42 ---------------------
.../Ia32/InterlockedDecrement.nasm | 10 ++---
.../Ia32/InterlockedIncrement.c | 43 ----------------------
.../Ia32/InterlockedIncrement.nasm | 7 ++--
...lockedDecrement.c => InterlockedDecrementMsc.c} | 4 +-
...lockedIncrement.c => InterlockedIncrementMsc.c} | 4 +-
.../BaseSynchronizationLib/Synchronization.c | 6 +--
.../BaseSynchronizationLib/SynchronizationGcc.c | 6 +--
.../BaseSynchronizationLib/SynchronizationMsc.c | 6 +--
.../Library/BaseSynchronizationLib/X64/GccInline.c | 19 +++++-----
.../X64/InterlockedDecrement.nasm | 8 ++--
.../X64/InterlockedIncrement.nasm | 5 ++-
16 files changed, 56 insertions(+), 152 deletions(-)
delete mode 100644 MdePkg/Library/BaseSynchronizationLib/Ia32/InterlockedDecrement.c
delete mode 100644 MdePkg/Library/BaseSynchronizationLib/Ia32/InterlockedIncrement.c
rename MdePkg/Library/BaseSynchronizationLib/{X64/InterlockedDecrement.c => InterlockedDecrementMsc.c} (87%)
rename MdePkg/Library/BaseSynchronizationLib/{X64/InterlockedIncrement.c => InterlockedIncrementMsc.c} (87%)
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/BaseSynchronizationLib.inf b/MdePkg/Library/BaseSynchronizationLib/BaseSynchronizationLib.inf
index 0be1d4331f..427e959f2e 100755
--- a/MdePkg/Library/BaseSynchronizationLib/BaseSynchronizationLib.inf
+++ b/MdePkg/Library/BaseSynchronizationLib/BaseSynchronizationLib.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
+ InterlockedIncrementMsc.c | MSFT
+ InterlockedDecrementMsc.c | 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
+ InterlockedIncrementMsc.c | MSFT
+ InterlockedDecrementMsc.c | 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/BaseSynchronizationLibInternals.h b/MdePkg/Library/BaseSynchronizationLib/BaseSynchronizationLibInternals.h
index 37edd7c188..8c363a8585 100644
--- a/MdePkg/Library/BaseSynchronizationLib/BaseSynchronizationLibInternals.h
+++ b/MdePkg/Library/BaseSynchronizationLib/BaseSynchronizationLibInternals.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/InterlockedDecrement.c b/MdePkg/Library/BaseSynchronizationLib/Ia32/InterlockedDecrement.c
deleted file mode 100644
index 354a0e7ab1..0000000000
--- a/MdePkg/Library/BaseSynchronizationLib/Ia32/InterlockedDecrement.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/InterlockedDecrement.nasm b/MdePkg/Library/BaseSynchronizationLib/Ia32/InterlockedDecrement.nasm
index 4c46041186..dd5a8de3ed 100644
--- a/MdePkg/Library/BaseSynchronizationLib/Ia32/InterlockedDecrement.nasm
+++ b/MdePkg/Library/BaseSynchronizationLib/Ia32/InterlockedDecrement.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/InterlockedIncrement.c b/MdePkg/Library/BaseSynchronizationLib/Ia32/InterlockedIncrement.c
deleted file mode 100644
index c61a550119..0000000000
--- a/MdePkg/Library/BaseSynchronizationLib/Ia32/InterlockedIncrement.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/InterlockedIncrement.nasm b/MdePkg/Library/BaseSynchronizationLib/Ia32/InterlockedIncrement.nasm
index 3902c73275..0677e4bf78 100644
--- a/MdePkg/Library/BaseSynchronizationLib/Ia32/InterlockedIncrement.nasm
+++ b/MdePkg/Library/BaseSynchronizationLib/Ia32/InterlockedIncrement.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/X64/InterlockedDecrement.c b/MdePkg/Library/BaseSynchronizationLib/InterlockedDecrementMsc.c
similarity index 87%
rename from MdePkg/Library/BaseSynchronizationLib/X64/InterlockedDecrement.c
rename to MdePkg/Library/BaseSynchronizationLib/InterlockedDecrementMsc.c
index da402cd4c6..6dc9502ea6 100644
--- a/MdePkg/Library/BaseSynchronizationLib/X64/InterlockedDecrement.c
+++ b/MdePkg/Library/BaseSynchronizationLib/InterlockedDecrementMsc.c
@@ -1,7 +1,7 @@
/** @file
InterlockedDecrement function
- 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
@@ -41,6 +41,6 @@ InternalSyncDecrement (
IN volatile UINT32 *Value
)
{
- return _InterlockedDecrement ((long *)(UINTN)(Value));
+ return _InterlockedDecrement ((long *)(Value));
}
diff --git a/MdePkg/Library/BaseSynchronizationLib/X64/InterlockedIncrement.c b/MdePkg/Library/BaseSynchronizationLib/InterlockedIncrementMsc.c
similarity index 87%
rename from MdePkg/Library/BaseSynchronizationLib/X64/InterlockedIncrement.c
rename to MdePkg/Library/BaseSynchronizationLib/InterlockedIncrementMsc.c
index bbd1384602..8327d81a87 100644
--- a/MdePkg/Library/BaseSynchronizationLib/X64/InterlockedIncrement.c
+++ b/MdePkg/Library/BaseSynchronizationLib/InterlockedIncrementMsc.c
@@ -1,7 +1,7 @@
/** @file
InterLockedIncrement function
- 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
@@ -41,6 +41,6 @@ InternalSyncIncrement (
IN volatile UINT32 *Value
)
{
- return _InterlockedIncrement ((long *)(UINTN)(Value));
+ return _InterlockedIncrement ((long *)(Value));
}
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/SynchronizationGcc.c b/MdePkg/Library/BaseSynchronizationLib/SynchronizationGcc.c
index 5ac548b19f..177739d3da 100644
--- a/MdePkg/Library/BaseSynchronizationLib/SynchronizationGcc.c
+++ b/MdePkg/Library/BaseSynchronizationLib/SynchronizationGcc.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/SynchronizationMsc.c b/MdePkg/Library/BaseSynchronizationLib/SynchronizationMsc.c
index e3298c8ab4..6ab2d71870 100644
--- a/MdePkg/Library/BaseSynchronizationLib/SynchronizationMsc.c
+++ b/MdePkg/Library/BaseSynchronizationLib/SynchronizationMsc.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/InterlockedDecrement.nasm b/MdePkg/Library/BaseSynchronizationLib/X64/InterlockedDecrement.nasm
index 60f43111fe..dabdca945e 100644
--- a/MdePkg/Library/BaseSynchronizationLib/X64/InterlockedDecrement.nasm
+++ b/MdePkg/Library/BaseSynchronizationLib/X64/InterlockedDecrement.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/InterlockedIncrement.nasm b/MdePkg/Library/BaseSynchronizationLib/X64/InterlockedIncrement.nasm
index 7f877b5774..77379d998e 100644
--- a/MdePkg/Library/BaseSynchronizationLib/X64/InterlockedIncrement.nasm
+++ b/MdePkg/Library/BaseSynchronizationLib/X64/InterlockedIncrement.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
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH v3] MdePkg/SynchronizationLib: fix Interlocked[De|In]crement return value
2018-09-13 9:50 [PATCH v3] MdePkg/SynchronizationLib: fix Interlocked[De|In]crement return value Ruiyu Ni
@ 2018-09-21 7:45 ` Gao, Liming
2018-09-25 14:08 ` Laszlo Ersek
2018-09-25 14:22 ` Laszlo Ersek
2 siblings, 0 replies; 8+ messages in thread
From: Gao, Liming @ 2018-09-21 7:45 UTC (permalink / raw)
To: Ni, Ruiyu, edk2-devel@lists.01.org; +Cc: Yao, Jiewen, Kinney, Michael D
The patch is good. Could you submit BZ to track this change?
Thanks
Liming
>-----Original Message-----
>From: Ni, Ruiyu
>Sent: Thursday, September 13, 2018 5:51 PM
>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 v3] 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.
>
>The patch calls intrinsic functions for MSVC tool chain, calls the
>NASM implementation for INTEL tool chain and calls GCC inline
>assembly implementation (GccInline.c) for GCC tool chain.
>
>Contributed-under: TianoCore Contribution Agreement 1.1
>Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
>Cc: Jiewen Yao <jiewen.yao@intel.com>
>Cc: Liming Gao <liming.gao@intel.com>
>Cc: Michael D Kinney <michael.d.kinney@intel.com>
>---
> MdePkg/Include/Library/SynchronizationLib.h | 6 +--
> .../BaseSynchronizationLib.inf | 18 ++++-----
> .../BaseSynchronizationLibInternals.h | 6 +--
> .../BaseSynchronizationLib/Ia32/GccInline.c | 18 ++++-----
> .../Ia32/InterlockedDecrement.c | 42 ---------------------
> .../Ia32/InterlockedDecrement.nasm | 10 ++---
> .../Ia32/InterlockedIncrement.c | 43 ----------------------
> .../Ia32/InterlockedIncrement.nasm | 7 ++--
> ...lockedDecrement.c => InterlockedDecrementMsc.c} | 4 +-
> ...lockedIncrement.c => InterlockedIncrementMsc.c} | 4 +-
> .../BaseSynchronizationLib/Synchronization.c | 6 +--
> .../BaseSynchronizationLib/SynchronizationGcc.c | 6 +--
> .../BaseSynchronizationLib/SynchronizationMsc.c | 6 +--
> .../Library/BaseSynchronizationLib/X64/GccInline.c | 19 +++++-----
> .../X64/InterlockedDecrement.nasm | 8 ++--
> .../X64/InterlockedIncrement.nasm | 5 ++-
> 16 files changed, 56 insertions(+), 152 deletions(-)
> delete mode 100644
>MdePkg/Library/BaseSynchronizationLib/Ia32/InterlockedDecrement.c
> delete mode 100644
>MdePkg/Library/BaseSynchronizationLib/Ia32/InterlockedIncrement.c
> rename
>MdePkg/Library/BaseSynchronizationLib/{X64/InterlockedDecrement.c =>
>InterlockedDecrementMsc.c} (87%)
> rename
>MdePkg/Library/BaseSynchronizationLib/{X64/InterlockedIncrement.c =>
>InterlockedIncrementMsc.c} (87%)
>
>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/BaseSynchronizationLib.inf
>b/MdePkg/Library/BaseSynchronizationLib/BaseSynchronizationLib.inf
>index 0be1d4331f..427e959f2e 100755
>--- a/MdePkg/Library/BaseSynchronizationLib/BaseSynchronizationLib.inf
>+++ b/MdePkg/Library/BaseSynchronizationLib/BaseSynchronizationLib.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
>+ InterlockedIncrementMsc.c | MSFT
>+ InterlockedDecrementMsc.c | 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
>+ InterlockedIncrementMsc.c | MSFT
>+ InterlockedDecrementMsc.c | 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/BaseSynchronizationLibInternals.h
>b/MdePkg/Library/BaseSynchronizationLib/BaseSynchronizationLibInternals.h
>index 37edd7c188..8c363a8585 100644
>---
>a/MdePkg/Library/BaseSynchronizationLib/BaseSynchronizationLibInternals.h
>+++
>b/MdePkg/Library/BaseSynchronizationLib/BaseSynchronizationLibInternals.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/InterlockedDecrement.c
>b/MdePkg/Library/BaseSynchronizationLib/Ia32/InterlockedDecrement.c
>deleted file mode 100644
>index 354a0e7ab1..0000000000
>--- a/MdePkg/Library/BaseSynchronizationLib/Ia32/InterlockedDecrement.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/InterlockedDecrement.nasm
>b/MdePkg/Library/BaseSynchronizationLib/Ia32/InterlockedDecrement.nasm
>index 4c46041186..dd5a8de3ed 100644
>---
>a/MdePkg/Library/BaseSynchronizationLib/Ia32/InterlockedDecrement.nasm
>+++
>b/MdePkg/Library/BaseSynchronizationLib/Ia32/InterlockedDecrement.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/InterlockedIncrement.c
>b/MdePkg/Library/BaseSynchronizationLib/Ia32/InterlockedIncrement.c
>deleted file mode 100644
>index c61a550119..0000000000
>--- a/MdePkg/Library/BaseSynchronizationLib/Ia32/InterlockedIncrement.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/InterlockedIncrement.nasm
>b/MdePkg/Library/BaseSynchronizationLib/Ia32/InterlockedIncrement.nasm
>index 3902c73275..0677e4bf78 100644
>---
>a/MdePkg/Library/BaseSynchronizationLib/Ia32/InterlockedIncrement.nasm
>+++
>b/MdePkg/Library/BaseSynchronizationLib/Ia32/InterlockedIncrement.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/X64/InterlockedDecrement.c
>b/MdePkg/Library/BaseSynchronizationLib/InterlockedDecrementMsc.c
>similarity index 87%
>rename from
>MdePkg/Library/BaseSynchronizationLib/X64/InterlockedDecrement.c
>rename to
>MdePkg/Library/BaseSynchronizationLib/InterlockedDecrementMsc.c
>index da402cd4c6..6dc9502ea6 100644
>--- a/MdePkg/Library/BaseSynchronizationLib/X64/InterlockedDecrement.c
>+++ b/MdePkg/Library/BaseSynchronizationLib/InterlockedDecrementMsc.c
>@@ -1,7 +1,7 @@
> /** @file
> InterlockedDecrement function
>
>- 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
>@@ -41,6 +41,6 @@ InternalSyncDecrement (
> IN volatile UINT32 *Value
> )
> {
>- return _InterlockedDecrement ((long *)(UINTN)(Value));
>+ return _InterlockedDecrement ((long *)(Value));
> }
>
>diff --git
>a/MdePkg/Library/BaseSynchronizationLib/X64/InterlockedIncrement.c
>b/MdePkg/Library/BaseSynchronizationLib/InterlockedIncrementMsc.c
>similarity index 87%
>rename from
>MdePkg/Library/BaseSynchronizationLib/X64/InterlockedIncrement.c
>rename to
>MdePkg/Library/BaseSynchronizationLib/InterlockedIncrementMsc.c
>index bbd1384602..8327d81a87 100644
>--- a/MdePkg/Library/BaseSynchronizationLib/X64/InterlockedIncrement.c
>+++ b/MdePkg/Library/BaseSynchronizationLib/InterlockedIncrementMsc.c
>@@ -1,7 +1,7 @@
> /** @file
> InterLockedIncrement function
>
>- 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
>@@ -41,6 +41,6 @@ InternalSyncIncrement (
> IN volatile UINT32 *Value
> )
> {
>- return _InterlockedIncrement ((long *)(UINTN)(Value));
>+ return _InterlockedIncrement ((long *)(Value));
> }
>
>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/SynchronizationGcc.c
>b/MdePkg/Library/BaseSynchronizationLib/SynchronizationGcc.c
>index 5ac548b19f..177739d3da 100644
>--- a/MdePkg/Library/BaseSynchronizationLib/SynchronizationGcc.c
>+++ b/MdePkg/Library/BaseSynchronizationLib/SynchronizationGcc.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/SynchronizationMsc.c
>b/MdePkg/Library/BaseSynchronizationLib/SynchronizationMsc.c
>index e3298c8ab4..6ab2d71870 100644
>--- a/MdePkg/Library/BaseSynchronizationLib/SynchronizationMsc.c
>+++ b/MdePkg/Library/BaseSynchronizationLib/SynchronizationMsc.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/InterlockedDecrement.nasm
>b/MdePkg/Library/BaseSynchronizationLib/X64/InterlockedDecrement.nasm
>index 60f43111fe..dabdca945e 100644
>---
>a/MdePkg/Library/BaseSynchronizationLib/X64/InterlockedDecrement.nasm
>+++
>b/MdePkg/Library/BaseSynchronizationLib/X64/InterlockedDecrement.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/InterlockedIncrement.nasm
>b/MdePkg/Library/BaseSynchronizationLib/X64/InterlockedIncrement.nasm
>index 7f877b5774..77379d998e 100644
>---
>a/MdePkg/Library/BaseSynchronizationLib/X64/InterlockedIncrement.nasm
>+++
>b/MdePkg/Library/BaseSynchronizationLib/X64/InterlockedIncrement.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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v3] MdePkg/SynchronizationLib: fix Interlocked[De|In]crement return value
2018-09-13 9:50 [PATCH v3] MdePkg/SynchronizationLib: fix Interlocked[De|In]crement return value Ruiyu Ni
2018-09-21 7:45 ` Gao, Liming
@ 2018-09-25 14:08 ` Laszlo Ersek
2018-09-25 14:22 ` Laszlo Ersek
2 siblings, 0 replies; 8+ messages in thread
From: Laszlo Ersek @ 2018-09-25 14:08 UTC (permalink / raw)
To: Ruiyu Ni, edk2-devel; +Cc: Michael D Kinney, Jiewen Yao, Liming Gao
Hi,
On 09/13/18 11:50, Ruiyu Ni wrote:
> Today's InterlockedIncrement()/InterlockedDecrement() guarantees to
> perform atomic increment/decrement but doesn't guarantee the return
> value equals to the new value.
>
> The patch fixes the behavior to use "XADD" instruction to guarantee
> the return value equals to the new value.
>
> The patch calls intrinsic functions for MSVC tool chain, calls the
> NASM implementation for INTEL tool chain and calls GCC inline
> assembly implementation (GccInline.c) for GCC tool chain.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> ---
> MdePkg/Include/Library/SynchronizationLib.h | 6 +--
> .../BaseSynchronizationLib.inf | 18 ++++-----
> .../BaseSynchronizationLibInternals.h | 6 +--
> .../BaseSynchronizationLib/Ia32/GccInline.c | 18 ++++-----
> .../Ia32/InterlockedDecrement.c | 42 ---------------------
> .../Ia32/InterlockedDecrement.nasm | 10 ++---
> .../Ia32/InterlockedIncrement.c | 43 ----------------------
> .../Ia32/InterlockedIncrement.nasm | 7 ++--
> ...lockedDecrement.c => InterlockedDecrementMsc.c} | 4 +-
> ...lockedIncrement.c => InterlockedIncrementMsc.c} | 4 +-
> .../BaseSynchronizationLib/Synchronization.c | 6 +--
> .../BaseSynchronizationLib/SynchronizationGcc.c | 6 +--
> .../BaseSynchronizationLib/SynchronizationMsc.c | 6 +--
> .../Library/BaseSynchronizationLib/X64/GccInline.c | 19 +++++-----
> .../X64/InterlockedDecrement.nasm | 8 ++--
> .../X64/InterlockedIncrement.nasm | 5 ++-
> 16 files changed, 56 insertions(+), 152 deletions(-)
> delete mode 100644 MdePkg/Library/BaseSynchronizationLib/Ia32/InterlockedDecrement.c
> delete mode 100644 MdePkg/Library/BaseSynchronizationLib/Ia32/InterlockedIncrement.c
> rename MdePkg/Library/BaseSynchronizationLib/{X64/InterlockedDecrement.c => InterlockedDecrementMsc.c} (87%)
> rename MdePkg/Library/BaseSynchronizationLib/{X64/InterlockedIncrement.c => InterlockedIncrementMsc.c} (87%)
This patch (commit 17634d026f96) has regressed OVMF on KVM. Here's the
bisection log:
> git bisect start
> # bad: [17634d026f968c404b039a8d8431b6389dd396ea] MdePkg/SynchronizationLib: fix Interlocked[De|In]crement return value
> git bisect bad 17634d026f968c404b039a8d8431b6389dd396ea
> # good: [997731e796f51df57c113dfd966e818622c3d4aa] UefiCpuPkg: Remove redundant library classes, Ppis and GUIDs
> git bisect good 997731e796f51df57c113dfd966e818622c3d4aa
> # good: [b602265d559b2f2ade4d09ba55652c23922c141f] MdeModulePkg RegularExpressionDxe: Update Oniguruma to 6.9.0
> git bisect good b602265d559b2f2ade4d09ba55652c23922c141f
> # good: [d5b28edd63f4d0fab087c5d6c9db773e5b5adc7d] MdePkg: Add a inf path in MdePkg.dsc
> git bisect good d5b28edd63f4d0fab087c5d6c9db773e5b5adc7d
> # good: [ca3e4f8ab82485edff2cfa7eeb87f71b4be38966] MdePkg UefiPciLibPciRootBridgeIo: Remove redundant dependency
> git bisect good ca3e4f8ab82485edff2cfa7eeb87f71b4be38966
> # first bad commit: [17634d026f968c404b039a8d8431b6389dd396ea] MdePkg/SynchronizationLib: fix Interlocked[De|In]crement return value
The symptom is that the boot gets stuck, with all VCPUs spinning, after
the following log is produced:
> Loading PEIM [CpuMpPei]
> Loading PEIM at 0x000BFEA8000 EntryPoint=0x000BFEADE86 CpuMpPei.efi
> Register PPI Notify: [EfiPeiMemoryDiscoveredPpi]
> Notify: PPI Guid: [EfiPeiMemoryDiscoveredPpi], Peim notify entry point: BFEB53B1
> AP Loop Mode is 1
> WakeupBufferStart = 9F000, WakeupBufferSize = 1000
MpInitLib uses SynchronizationLib:
> UefiCpuPkg/Library/MpInitLib/Microcode.c:241: AcquireSpinLock(&CpuMpData->MpLock);
> UefiCpuPkg/Library/MpInitLib/Microcode.c:244: ReleaseSpinLock(&CpuMpData->MpLock);
> UefiCpuPkg/Library/MpInitLib/MpLib.c:117: AcquireSpinLock (&CpuData->ApLock);
> UefiCpuPkg/Library/MpInitLib/MpLib.c:119: ReleaseSpinLock (&CpuData->ApLock);
> UefiCpuPkg/Library/MpInitLib/MpLib.c:555: AcquireSpinLock(&CpuMpData->MpLock);
> UefiCpuPkg/Library/MpInitLib/MpLib.c:557: ReleaseSpinLock(&CpuMpData->MpLock);
> UefiCpuPkg/Library/MpInitLib/MpLib.c:560: InitializeSpinLock(&CpuMpData->CpuData[ProcessorNumber].ApLock);
> UefiCpuPkg/Library/MpInitLib/MpLib.c:609: InterlockedIncrement ((UINT32 *) &CpuMpData->CpuCount);
> UefiCpuPkg/Library/MpInitLib/MpLib.c:637: InterlockedCompareExchange32 (
> UefiCpuPkg/Library/MpInitLib/MpLib.c:706: InterlockedIncrement ((UINT32 *) &CpuMpData->FinishedCount);
> UefiCpuPkg/Library/MpInitLib/MpLib.c:707: InterlockedDecrement ((UINT32 *) &CpuMpData->MpCpuExchangeInfo->NumApsExecuting);
> UefiCpuPkg/Library/MpInitLib/MpLib.c:775: while (InterlockedCompareExchange32 (
> UefiCpuPkg/Library/MpInitLib/MpLib.c:1645: InitializeSpinLock(&CpuMpData->MpLock);
> UefiCpuPkg/Library/MpInitLib/MpLib.c:1718: InitializeSpinLock(&CpuMpData->CpuData[Index].ApLock);
I'll try to collect more information.
Thanks,
Laszlo
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v3] MdePkg/SynchronizationLib: fix Interlocked[De|In]crement return value
2018-09-13 9:50 [PATCH v3] MdePkg/SynchronizationLib: fix Interlocked[De|In]crement return value Ruiyu Ni
2018-09-21 7:45 ` Gao, Liming
2018-09-25 14:08 ` Laszlo Ersek
@ 2018-09-25 14:22 ` Laszlo Ersek
2018-09-25 16:18 ` Laszlo Ersek
2 siblings, 1 reply; 8+ messages in thread
From: Laszlo Ersek @ 2018-09-25 14:22 UTC (permalink / raw)
To: Ruiyu Ni, edk2-devel; +Cc: Michael D Kinney, Jiewen Yao, Liming Gao
On 09/13/18 11:50, Ruiyu Ni wrote:
> 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
The operand list for the inline assembly is incorrect, I suspect. When I
build this code with GCC48 as part of OVMF, for the NOOPT target, then
disassemble "CpuMpPei.debug", I get:
> 0000000000004383 <InternalSyncIncrement>:
> UINT32
> EFIAPI
> InternalSyncIncrement (
> IN volatile UINT32 *Value
> )
> {
> 4383: 55 push %rbp
> 4384: 48 89 e5 mov %rsp,%rbp
> 4387: 48 83 ec 10 sub $0x10,%rsp
> 438b: 48 89 4d 10 mov %rcx,0x10(%rbp)
> UINT32 Result;
>
> __asm__ __volatile__ (
> 438f: 48 8b 55 10 mov 0x10(%rbp),%rdx
> 4393: 48 8b 45 10 mov 0x10(%rbp),%rax
> 4397: b8 01 00 00 00 mov $0x1,%eax
> 439c: f0 0f c1 00 lock xadd %eax,(%rax)
> 43a0: ff c0 inc %eax
> 43a2: 89 45 fc mov %eax,-0x4(%rbp)
> : "m" (*Value) // %2
> : "memory",
> "cc"
> );
>
> return Result;
> 43a5: 8b 45 fc mov -0x4(%rbp),%eax
> }
> 43a8: c9 leaveq
> 43a9: c3 retq
>
The generated code is trying to use EAX for both (a) the
exchange-and-add, i.e. as the source operand of the XADD instruction,
and (b) as (part of) the address of the (*Value) argument.
In effect, before we reach the XADD instruction, the least significant
dword of Value's address is overwritten with 0x0000_0001. The exchange
then happens against that memory location.
It is interesting that GCC first spills the parameter from RCX to the
stack, and then it loads the parameter from the stack to both RDX and
RAX.
I think the operand list used with the inline assembly should be updated
to prevent GCC from associating %2 with EAX.
I'll try to experiment some more.
Thanks
Laszlo
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v3] MdePkg/SynchronizationLib: fix Interlocked[De|In]crement return value
2018-09-25 14:22 ` Laszlo Ersek
@ 2018-09-25 16:18 ` Laszlo Ersek
2018-09-25 18:29 ` Laszlo Ersek
0 siblings, 1 reply; 8+ messages in thread
From: Laszlo Ersek @ 2018-09-25 16:18 UTC (permalink / raw)
To: Ruiyu Ni, edk2-devel; +Cc: Michael D Kinney, Jiewen Yao, Liming Gao
On 09/25/18 16:22, Laszlo Ersek wrote:
> On 09/13/18 11:50, Ruiyu Ni wrote:
>
>> 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
>
> The operand list for the inline assembly is incorrect, I suspect. When I
> build this code with GCC48 as part of OVMF, for the NOOPT target, then
> disassemble "CpuMpPei.debug", I get:
>
>> 0000000000004383 <InternalSyncIncrement>:
>> UINT32
>> EFIAPI
>> InternalSyncIncrement (
>> IN volatile UINT32 *Value
>> )
>> {
>> 4383: 55 push %rbp
>> 4384: 48 89 e5 mov %rsp,%rbp
>> 4387: 48 83 ec 10 sub $0x10,%rsp
>> 438b: 48 89 4d 10 mov %rcx,0x10(%rbp)
>> UINT32 Result;
>>
>> __asm__ __volatile__ (
>> 438f: 48 8b 55 10 mov 0x10(%rbp),%rdx
>> 4393: 48 8b 45 10 mov 0x10(%rbp),%rax
>> 4397: b8 01 00 00 00 mov $0x1,%eax
>> 439c: f0 0f c1 00 lock xadd %eax,(%rax)
>> 43a0: ff c0 inc %eax
>> 43a2: 89 45 fc mov %eax,-0x4(%rbp)
>> : "m" (*Value) // %2
>> : "memory",
>> "cc"
>> );
>>
>> return Result;
>> 43a5: 8b 45 fc mov -0x4(%rbp),%eax
>> }
>> 43a8: c9 leaveq
>> 43a9: c3 retq
>>
>
> The generated code is trying to use EAX for both (a) the
> exchange-and-add, i.e. as the source operand of the XADD instruction,
> and (b) as (part of) the address of the (*Value) argument.
>
> In effect, before we reach the XADD instruction, the least significant
> dword of Value's address is overwritten with 0x0000_0001. The exchange
> then happens against that memory location.
>
> It is interesting that GCC first spills the parameter from RCX to the
> stack, and then it loads the parameter from the stack to both RDX and
> RAX.
>
> I think the operand list used with the inline assembly should be updated
> to prevent GCC from associating %2 with EAX.
>
> I'll try to experiment some more.
The operand lists for almost all of the functions in
- MdePkg/Library/BaseSynchronizationLib/Ia32/GccInline.c
- MdePkg/Library/BaseSynchronizationLib/X64/GccInline.c
are wrong. The InternalSyncIncrement() and InternalSyncDecrement() issue
has now bitten us in practice. The InternalSyncCompareExchangeXx()
functions also have incorrect operand lists, they just haven't blown up yet.
The core problem is that input-output operands for the inline assembly
templates must be marked as such. The GCC documentation describes two
ways for that:
- List the r/w operand only in the output operand list, and use the "+"
(not "=") constraint.
- Alternatively, list the operand in both input and output lists, and
use the "=" constraint in the output list. However, in the input list,
the constraing must be "N" (a decimal number), where N stands for the
operand number in the output list. This is how GCC knows those operands
are actually the same. Using just the same *expression* to fill in the
two instances (= input and output) of the operand is not guaranteed to
work; the gcc documentation explicitly highlights this fact.
In addition, in the ASM template, operands can be referenced by "[name]"
instead of %N. This has been supported since at least gcc-3.1. This
makes the template a lot more readable.
I'm working on a patch set.
Thanks
Laszlo
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v3] MdePkg/SynchronizationLib: fix Interlocked[De|In]crement return value
2018-09-25 16:18 ` Laszlo Ersek
@ 2018-09-25 18:29 ` Laszlo Ersek
2018-09-25 19:25 ` Laszlo Ersek
0 siblings, 1 reply; 8+ messages in thread
From: Laszlo Ersek @ 2018-09-25 18:29 UTC (permalink / raw)
To: Ruiyu Ni, edk2-devel; +Cc: Michael D Kinney, Jiewen Yao, Liming Gao
On 09/25/18 18:18, Laszlo Ersek wrote:
> On 09/25/18 16:22, Laszlo Ersek wrote:
>> On 09/13/18 11:50, Ruiyu Ni wrote:
>>
>>> 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
>>
>> The operand list for the inline assembly is incorrect, I suspect. When I
>> build this code with GCC48 as part of OVMF, for the NOOPT target, then
>> disassemble "CpuMpPei.debug", I get:
>>
>>> 0000000000004383 <InternalSyncIncrement>:
>>> UINT32
>>> EFIAPI
>>> InternalSyncIncrement (
>>> IN volatile UINT32 *Value
>>> )
>>> {
>>> 4383: 55 push %rbp
>>> 4384: 48 89 e5 mov %rsp,%rbp
>>> 4387: 48 83 ec 10 sub $0x10,%rsp
>>> 438b: 48 89 4d 10 mov %rcx,0x10(%rbp)
>>> UINT32 Result;
>>>
>>> __asm__ __volatile__ (
>>> 438f: 48 8b 55 10 mov 0x10(%rbp),%rdx
>>> 4393: 48 8b 45 10 mov 0x10(%rbp),%rax
>>> 4397: b8 01 00 00 00 mov $0x1,%eax
>>> 439c: f0 0f c1 00 lock xadd %eax,(%rax)
>>> 43a0: ff c0 inc %eax
>>> 43a2: 89 45 fc mov %eax,-0x4(%rbp)
>>> : "m" (*Value) // %2
>>> : "memory",
>>> "cc"
>>> );
>>>
>>> return Result;
>>> 43a5: 8b 45 fc mov -0x4(%rbp),%eax
>>> }
>>> 43a8: c9 leaveq
>>> 43a9: c3 retq
>>>
>>
>> The generated code is trying to use EAX for both (a) the
>> exchange-and-add, i.e. as the source operand of the XADD instruction,
>> and (b) as (part of) the address of the (*Value) argument.
>>
>> In effect, before we reach the XADD instruction, the least significant
>> dword of Value's address is overwritten with 0x0000_0001. The exchange
>> then happens against that memory location.
>>
>> It is interesting that GCC first spills the parameter from RCX to the
>> stack, and then it loads the parameter from the stack to both RDX and
>> RAX.
>>
>> I think the operand list used with the inline assembly should be updated
>> to prevent GCC from associating %2 with EAX.
>>
>> I'll try to experiment some more.
>
> The operand lists for almost all of the functions in
>
> - MdePkg/Library/BaseSynchronizationLib/Ia32/GccInline.c
> - MdePkg/Library/BaseSynchronizationLib/X64/GccInline.c
>
> are wrong. The InternalSyncIncrement() and InternalSyncDecrement() issue
> has now bitten us in practice. The InternalSyncCompareExchangeXx()
> functions also have incorrect operand lists, they just haven't blown up yet.
>
> The core problem is that input-output operands for the inline assembly
> templates must be marked as such. The GCC documentation describes two
> ways for that:
>
> - List the r/w operand only in the output operand list, and use the "+"
> (not "=") constraint.
>
> - Alternatively, list the operand in both input and output lists, and
> use the "=" constraint in the output list. However, in the input list,
> the constraing must be "N" (a decimal number), where N stands for the
> operand number in the output list. This is how GCC knows those operands
> are actually the same. Using just the same *expression* to fill in the
> two instances (= input and output) of the operand is not guaranteed to
> work; the gcc documentation explicitly highlights this fact.
>
> In addition, in the ASM template, operands can be referenced by "[name]"
> instead of %N. This has been supported since at least gcc-3.1. This
> makes the template a lot more readable.
>
> I'm working on a patch set.
Apologies for conversing myself, but now I've managed to review more
background discussion from the mailing list.
In particular, in the following messages:
http://mid.mail-archive.com/4A89E2EF3DFEDB4C8BFDE51014F606A14E2F666F@SHSMSX104.ccr.corp.intel.com
http://mid.mail-archive.com/E92EE9817A31E24EB0585FDF735412F5B8AD687E@ORSMSX113.amr.corp.intel.com
an agreement was reached that ultimately *two* implementations would
remain: compiler intrinsics for the MSFT toolchain family, and the NASM
implementation for *everything else*, including *both* GCC and INTEL
toolchain families.
So, why was version 3 of the patch then accepted and pushed? Because now
we still have *three* implementations (excerpt):
[Sources.X64]
InterlockedIncrementMsc.c | MSFT
InterlockedDecrementMsc.c | MSFT
X64/InterlockedDecrement.nasm | INTEL
X64/InterlockedIncrement.nasm | INTEL
X64/GccInline.c | GCC
This is not in the spirit of the agreement on the list.
Why this is relevant to me: because now I realize I shouldn't *fix* the
GCC inline assembly. Instead, we should *remove* it, and consume the
NASM implementation.
So here's what I'm going to do. I will submit a standalone, "surgical"
patch, for fixing the regression introduced in 17634d026f96.
Additionally, I will file a TianoCore BZ about the *other* bugs in
GccInline.c, and suggest that we fix those issues by eliminating the
GCC-specific variant altogether. This way the regression is going to be
averted while we discuss that BZ.
Thanks
Laszlo
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v3] MdePkg/SynchronizationLib: fix Interlocked[De|In]crement return value
2018-09-25 18:29 ` Laszlo Ersek
@ 2018-09-25 19:25 ` Laszlo Ersek
2018-09-26 17:39 ` Kinney, Michael D
0 siblings, 1 reply; 8+ messages in thread
From: Laszlo Ersek @ 2018-09-25 19:25 UTC (permalink / raw)
To: Ruiyu Ni, edk2-devel; +Cc: Michael D Kinney, Jiewen Yao, Liming Gao
On 09/25/18 20:29, Laszlo Ersek wrote:
> So here's what I'm going to do. I will submit a standalone, "surgical"
> patch, for fixing the regression introduced in 17634d026f96.
Filed <https://bugzilla.tianocore.org/show_bug.cgi?id=1207> about that.
> Additionally, I will file a TianoCore BZ about the *other* bugs in
> GccInline.c, and suggest that we fix those issues by eliminating the
> GCC-specific variant altogether.
That one is <https://bugzilla.tianocore.org/show_bug.cgi?id=1208>.
Thanks
Laszlo
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3] MdePkg/SynchronizationLib: fix Interlocked[De|In]crement return value
2018-09-25 19:25 ` Laszlo Ersek
@ 2018-09-26 17:39 ` Kinney, Michael D
0 siblings, 0 replies; 8+ messages in thread
From: Kinney, Michael D @ 2018-09-26 17:39 UTC (permalink / raw)
To: Laszlo Ersek, Ni, Ruiyu, edk2-devel@lists.01.org; +Cc: Yao, Jiewen, Gao, Liming
Laszlo,
Thanks for filing the BZ. Converting inline assembly to
NASM source files is a really good direction to improve
the readability and maintainability of the code by
consolidating to one assembly source format.
Thanks,
Mike
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Tuesday, September 25, 2018 12:26 PM
> To: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-
> devel@lists.01.org
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Yao,
> Jiewen <jiewen.yao@intel.com>; Gao, Liming
> <liming.gao@intel.com>
> Subject: Re: [edk2] [PATCH v3]
> MdePkg/SynchronizationLib: fix Interlocked[De|In]crement
> return value
>
> On 09/25/18 20:29, Laszlo Ersek wrote:
>
> > So here's what I'm going to do. I will submit a
> standalone, "surgical"
> > patch, for fixing the regression introduced in
> 17634d026f96.
>
> Filed
> <https://bugzilla.tianocore.org/show_bug.cgi?id=1207>
> about that.
>
> > Additionally, I will file a TianoCore BZ about the
> *other* bugs in
> > GccInline.c, and suggest that we fix those issues by
> eliminating the
> > GCC-specific variant altogether.
>
> That one is
> <https://bugzilla.tianocore.org/show_bug.cgi?id=1208>.
>
> Thanks
> Laszlo
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-09-26 17:39 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-09-13 9:50 [PATCH v3] MdePkg/SynchronizationLib: fix Interlocked[De|In]crement return value Ruiyu Ni
2018-09-21 7:45 ` Gao, Liming
2018-09-25 14:08 ` Laszlo Ersek
2018-09-25 14:22 ` Laszlo Ersek
2018-09-25 16:18 ` Laszlo Ersek
2018-09-25 18:29 ` Laszlo Ersek
2018-09-25 19:25 ` Laszlo Ersek
2018-09-26 17:39 ` Kinney, Michael D
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox