Re: [edk2] [PATCH v3 21/27] Ovmf/Xen: add ARM and AArch64 support to XenBusDxe

[Jordan Justen <jordan.l.justen@intel.com>]

On 2015-02-05 01:56:04, Ard Biesheuvel wrote:
> On 4 February 2015 at 21:10, Jordan Justen <jordan.l.justen@intel.com> wrote:
> > On 2015-02-03 11:20:06, Ard Biesheuvel wrote:
> >> This patch adds support to XenBusDxe for executing on ARM and AArch64
> >> machines (the former only when built with GCC).
> >>
> >> Contributed-under: TianoCore Contribution Agreement 1.0
> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >> ---
> >>  OvmfPkg/XenBusDxe/AtomicsGcc.c  | 44 +++++++++++++++++++++++++++++++++++++++++
> >>  OvmfPkg/XenBusDxe/XenBusDxe.inf |  3 +++
> >>  2 files changed, 47 insertions(+)
> >>  create mode 100644 OvmfPkg/XenBusDxe/AtomicsGcc.c
> >>
> >> diff --git a/OvmfPkg/XenBusDxe/AtomicsGcc.c b/OvmfPkg/XenBusDxe/AtomicsGcc.c
> >> new file mode 100644
> >> index 000000000000..a0bdcbf67440
> >> --- /dev/null
> >> +++ b/OvmfPkg/XenBusDxe/AtomicsGcc.c
> >> @@ -0,0 +1,44 @@
> >> +/** @file
> >> +  Arch-independent implementations of XenBusDxe atomics using GCC __builtins
> >> +
> >> +  Copyright (C) 2014, 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.
> >> +
> >> +**/
> >> +
> >> +UINT16
> >> +EFIAPI
> >> +InternalSyncCompareExchange16 (
> >> +  IN      volatile UINT16           *Value,
> >> +  IN      UINT16                    CompareValue,
> >> +  IN      UINT16                    ExchangeValue
> >
> > Can you instead get InterlockedCompareExchange16 added to
> > BaseSynchronizationLib?
> >
> > http://permalink.gmane.org/gmane.comp.bios.tianocore.devel/10470
> >
> 
> That would seem the obvious choice, but sadly, BaseSynchronizationLib
> is not implemented at all for ARM and AArch64 (Some .c files are there
> but they are just unsynchronized C implementations)
> 
> So in order to do this properly, I would need to
> - implement the existing functions for ARM and AArch64

Would you? It appears the library is already used by ARM platforms as
is.

If you add your InterlockedCompareExchange16, it is at least an
improvement, right?

> - implement InterlockedCompareExchange16 () for all existing
> architectures, including IPF, which I don't have a clue about, and
> toolchains I don't have access to

Mike, you added the IPF InterlockedCompareExchange32.s and
InterlockedCompareExchange64.s files. Could you help to provide a
InterlockedCompareExchange16.s?

> - implement TestAndClearBit () for all architectures, or create a
> BaseBitopsLib and implement it there

I only meant to add InterlockedCompareExchange16 for now.

-Jordan

> I now understand how Anthony ended up doing it like this in the first place :-)
> 
> Anyway, we are all invested in improving the code base, so I am happy
> to do my part, but I would need help from others to implement the
> pieces that are really not feasible for me to take on.
> 
> -- 
> Ard.
> 
> >> +  )
> >> +{
> >> +  return __sync_val_compare_and_swap_2 (Value, CompareValue, ExchangeValue);
> >> +}
> >> +
> >> +INT32
> >> +EFIAPI
> >> +TestAndClearBit (
> >> +  IN INT32            Bit,
> >> +  IN volatile VOID    *Address
> >> +  )
> >> +{
> >> +  //
> >> +  // 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 >> 5) * sizeof(INT32);
> >> +  Bit &= 31;
> >> +
> >> +  return (__sync_fetch_and_and_4 (Address, ~(1U << Bit)) & (1U << Bit)) != 0;
> >> +}
> >> diff --git a/OvmfPkg/XenBusDxe/XenBusDxe.inf b/OvmfPkg/XenBusDxe/XenBusDxe.inf
> >> index 31553ac5a64a..949ec0a0c732 100644
> >> --- a/OvmfPkg/XenBusDxe/XenBusDxe.inf
> >> +++ b/OvmfPkg/XenBusDxe/XenBusDxe.inf
> >> @@ -54,6 +54,9 @@
> >>    X64/InterlockedCompareExchange16.nasm
> >>    X64/TestAndClearBit.nasm
> >>
> >> +[Sources.AARCH64, Sources.ARM]
> >> +  AtomicsGcc.c | GCC
> >> +
> >>  [LibraryClasses]
> >>    UefiDriverEntryPoint
> >>    UefiBootServicesTableLib
> >> --
> >> 1.8.3.2
> >>
> >>
> >> ------------------------------------------------------------------------------
> >> Dive into the World of Parallel Programming. The Go Parallel Website,
> >> sponsored by Intel and developed in partnership with Slashdot Media, is your
> >> hub for all things parallel software development, from weekly thought
> >> leadership blogs to news, videos, case studies, tutorials and more. Take a
> >> look and join the conversation now. http://goparallel.sourceforge.net/
> >> _______________________________________________
> >> edk2-devel mailing list
> >> edk2-devel@lists.sourceforge.net
> >> https://lists.sourceforge.net/lists/listinfo/edk2-devel