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

[Anthony PERARD <anthony.perard@citrix.com>]

On Mon, Feb 23, 2015 at 05:54:03PM +0000, Ard Biesheuvel wrote:
> On 23 February 2015 at 15:16, Anthony PERARD <anthony.perard@citrix.com> 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@linaro.org>
> >> ---
> >>  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?

Yes, that looks correct now.
Thanks,

> 
> >> +      return 1;
> >> +    }
> >> +  }
> >> +  return 0;
> >> +}

-- 
Anthony PERARD