From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ard Biesheuvel Subject: Re: [PATCH v4 23/29] Ovmf/Xen: port XenBusDxe to other architectures Date: Mon, 23 Feb 2015 17:54:03 +0000 Message-ID: References: <1423739961-5945-1-git-send-email-ard.biesheuvel@linaro.org> <1423739961-5945-24-git-send-email-ard.biesheuvel@linaro.org> <20150223151617.GU1345@perard.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20150223151617.GU1345@perard.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Anthony PERARD Cc: "Tian, Feng" , Julien Grall , Ian Campbell , Olivier Martin , Stefano Stabellini , Jordan Justen , "edk2-devel@lists.sourceforge.net" , Leif Lindholm , xen-devel@lists.xen.org, Roy Franz , "Kinney, Michael D" , Laszlo Ersek List-Id: xen-devel@lists.xenproject.org On 23 February 2015 at 15:16, Anthony PERARD wrote: > On Thu, Feb 12, 2015 at 07:19:15PM +0800, Ard Biesheuvel wrote: >> This patch updates XenBusDxe to use the 16-bit compare and exchange >> function that was introduced for this purpose to the >> BaseSynchronizationLib. It also provides a new generic implementation >> of TestAndClearBit () using the same 16-bit compare and exchange, making >> this module fully architecture agnostic. >> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Ard Biesheuvel >> --- >> OvmfPkg/XenBusDxe/GrantTable.c | 2 +- >> OvmfPkg/XenBusDxe/Ia32/InterlockedCompareExchange16.nasm | 42 ------------------------------------------ >> OvmfPkg/XenBusDxe/Ia32/TestAndClearBit.nasm | 16 ---------------- >> OvmfPkg/XenBusDxe/InterlockedCompareExchange16.c | 33 --------------------------------- >> OvmfPkg/XenBusDxe/InterlockedCompareExchange16.h | 38 -------------------------------------- >> OvmfPkg/XenBusDxe/TestAndClearBit.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++ >> OvmfPkg/XenBusDxe/X64/InterlockedCompareExchange16.nasm | 41 ----------------------------------------- >> OvmfPkg/XenBusDxe/X64/TestAndClearBit.nasm | 15 --------------- >> OvmfPkg/XenBusDxe/XenBusDxe.h | 2 +- >> OvmfPkg/XenBusDxe/XenBusDxe.inf | 12 ++---------- >> 10 files changed, 50 insertions(+), 197 deletions(-) >> >> diff --git a/OvmfPkg/XenBusDxe/TestAndClearBit.c b/OvmfPkg/XenBusDxe/TestAndClearBit.c >> new file mode 100644 >> index 000000000000..e971b40a89ce >> --- /dev/null >> +++ b/OvmfPkg/XenBusDxe/TestAndClearBit.c >> @@ -0,0 +1,46 @@ >> +/** @file >> + Implementation of TestAndClearBit using compare-exchange primitive >> + >> + Copyright (C) 2015, Linaro Ltd. >> + >> + 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. >> + >> +**/ >> + >> +#include >> +#include >> + >> +INT32 >> +EFIAPI >> +TestAndClearBit ( >> + IN INT32 Bit, >> + IN VOID *Address >> + ) >> +{ >> + UINT16 Word; >> + UINT16 Mask; >> + >> + // >> + // Calculate the effective address relative to 'Address' based on the >> + // higher order bits of 'Bit'. Use signed shift instead of division to >> + // ensure we round towards -Inf, and end up with a positive shift in >> + // 'Bit', even if 'Bit' itself is negative. >> + // >> + Address += (Bit >> 4) * sizeof(UINT16); >> + >> + Word = *(UINT16 *)Address; >> + Mask = 1U << (Bit & 15); >> + >> + while (Word & Mask) { >> + if (Word == InterlockedCompareExchange16 (Address, Word, Word & ~Mask)) { > > I think there is an infinite loop here, if the value pointed by Address > change between "Word = *Address" and this call to > InterlockedCompareExchange16. > You're quite right. I need to re-read Word at every iteration, something like (with UINT16 Read added): for (Word = *(UINT16 *) Address; Word & Mask; Word = Read) { Read = InterlockedCompareExchange16 (Address, Word, Word & ~Mask); if (Read == Word) { return 1; } } perhaps? >> + return 1; >> + } >> + } >> + return 0; >> +} >> diff --git a/OvmfPkg/XenBusDxe/X64/TestAndClearBit.nasm b/OvmfPkg/XenBusDxe/X64/TestAndClearBit.nasm >> deleted file mode 100644 >> index a4859a62a250..000000000000 >> --- a/OvmfPkg/XenBusDxe/X64/TestAndClearBit.nasm >> +++ /dev/null >> @@ -1,15 +0,0 @@ >> -DEFAULT REL >> -SECTION .text >> - >> -; INT32 >> -; EFIAPI >> -; TestAndClearBit ( >> -; IN INT32 Bit, // rcx >> -; IN volatile VOID* Address // rdx >> -; ); >> -global ASM_PFX(TestAndClearBit) >> -ASM_PFX(TestAndClearBit): >> - lock btr [rdx], ecx >> - sbb eax, eax >> - ret >> - >> diff --git a/OvmfPkg/XenBusDxe/XenBusDxe.h b/OvmfPkg/XenBusDxe/XenBusDxe.h >> index 6c306e017b07..953e4b72e85e 100644 >> --- a/OvmfPkg/XenBusDxe/XenBusDxe.h >> +++ b/OvmfPkg/XenBusDxe/XenBusDxe.h >> @@ -122,7 +122,7 @@ INT32 >> EFIAPI >> TestAndClearBit ( >> IN INT32 Bit, >> - IN volatile VOID *Address >> + IN VOID *Address >> ); >> >> CHAR8* >> diff --git a/OvmfPkg/XenBusDxe/XenBusDxe.inf b/OvmfPkg/XenBusDxe/XenBusDxe.inf >> index 31553ac5a64a..f0c5db98b1f4 100644 >> --- a/OvmfPkg/XenBusDxe/XenBusDxe.inf >> +++ b/OvmfPkg/XenBusDxe/XenBusDxe.inf >> @@ -34,8 +34,6 @@ >> DriverBinding.h >> ComponentName.c >> ComponentName.h >> - InterlockedCompareExchange16.c >> - InterlockedCompareExchange16.h >> GrantTable.c >> GrantTable.h >> EventChannel.c >> @@ -45,14 +43,7 @@ >> XenBus.c >> XenBus.h >> Helpers.c >> - >> -[Sources.IA32] >> - Ia32/InterlockedCompareExchange16.nasm >> - Ia32/TestAndClearBit.nasm >> - >> -[Sources.X64] >> - X64/InterlockedCompareExchange16.nasm >> - X64/TestAndClearBit.nasm >> + TestAndClearBit.c >> >> [LibraryClasses] >> UefiDriverEntryPoint >> @@ -64,6 +55,7 @@ >> DevicePathLib >> DebugLib >> XenHypercallLib >> + SynchronizationLib >> >> [Protocols] >> gEfiDriverBindingProtocolGuid > > -- > Anthony PERARD