Re: [Xen-devel] [PATCH v4 23/29] Ovmf/Xen: port XenBusDxe to other architectures

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

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 <ard.biesheuvel@xxxxxxxxxx>
> ---
>  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 <Base.h>
> +#include <Library/SynchronizationLib.h>
> +
> +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.

> +      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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel