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

[Olivier Martin <Olivier.Martin@xxxxxxx>]

Hi Ard,
I did an implementation of BaseSynchronizationLib for ARM(32) a couple years 
ago. I am not sure why I have never pushed this patch upstream.
Anyway, feel free to use it and push it upstream after reviewing it and adding 
support for AArch64.

    Contributed-under: TianoCore Contribution Agreement 1.0
    Signed-off-by: Olivier Martin <olivier.martin@xxxxxxx>

Thanks,
Olivier
________________________________________
From: Jordan Justen [jordan.l.justen@xxxxxxxxx]
Sent: 07 February 2015 22:00
To: Ard Biesheuvel; Kinney, Michael D
Cc: edk2-devel@xxxxxxxxxxxxxxxxxxxxx; Laszlo Ersek; Olivier Martin; Roy Franz; 
Leif Lindholm; Stefano Stabellini; Ian.Campbell@xxxxxxxxxx; Anthony PERARD; 
Christoffer Dall; xen-devel@xxxxxxxxxxxxx; Ilias Biris; Julien Grall
Subject: Re: [edk2] [PATCH v3 21/27] Ovmf/Xen: add ARM and AArch64 support to 
XenBusDxe

On 2015-02-05 01:56:04, Ard Biesheuvel wrote:
> On 4 February 2015 at 21:10, Jordan Justen <jordan.l.justen@xxxxxxxxx> 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@xxxxxxxxxx>
> >> ---
> >>  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@xxxxxxxxxxxxxxxxxxxxx
> >> https://lists.sourceforge.net/lists/listinfo/edk2-devel


-- IMPORTANT NOTICE: The contents of this email and any attachments are 
confidential and may also be privileged. If you are not the intended recipient, 
please notify the sender immediately and do not disclose the contents to any 
other person, use it for any purpose, or store or copy the information in any 
medium.  Thank you.

ARM Limited, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered 
in England & Wales, Company No:  2557590
ARM Holdings plc, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, 
Registered in England & Wales, Company No:  2548782

Attachment: MdePkg-BaseSynchronizationLib-Added-ARM-support.patch
Description: MdePkg-BaseSynchronizationLib-Added-ARM-support.patch

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