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

[Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx>]

On 23 February 2015 at 15:16, Anthony PERARD <anthony.perard@xxxxxxxxxx> 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 <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.
>

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

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