From: "Liming Gao" <liming.gao@intel.com>
To: "devel@edk2.groups.io" <devel@edk2.groups.io>,
"lersek@redhat.com" <lersek@redhat.com>,
Leif Lindholm <leif@nuviainc.com>, Gary Lin <glin@suse.com>,
"afish@apple.com" <afish@apple.com>,
"Kinney, Michael D" <michael.d.kinney@intel.com>
Cc: "ard.biesheuvel@arm.com" <ard.biesheuvel@arm.com>
Subject: Re: [edk2-devel] [PATCH v2] ArmPkg/CompilerIntrinsicsLib: provide atomics intrinsics
Date: Fri, 29 May 2020 03:04:14 +0000 [thread overview]
Message-ID: <SN6PR11MB3197B505F3BE9C1F0FCF815B808F0@SN6PR11MB3197.namprd11.prod.outlook.com> (raw)
In-Reply-To: <bba944c2-e4bb-ee23-4c04-898881656b71@redhat.com>
Laszlo:
I agree the proposal to delay few days to collect more feedbacks. I just send the mail to highlight this request in Hard Feature Freeze https://edk2.groups.io/g/devel/message/60421.
Thanks
Liming
-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek
Sent: 2020年5月29日 4:09
To: Gao, Liming <liming.gao@intel.com>; Leif Lindholm <leif@nuviainc.com>; Gary Lin <glin@suse.com>; afish@apple.com; Kinney, Michael D <michael.d.kinney@intel.com>
Cc: devel@edk2.groups.io; ard.biesheuvel@arm.com
Subject: Re: [edk2-devel] [PATCH v2] ArmPkg/CompilerIntrinsicsLib: provide atomics intrinsics
On 05/28/20 17:48, Gao, Liming wrote:
> Leif:
> BZ 2723 is a tiano feature request. This patch adds support for new GCC10. I agree with Laszlo that this change is like the enhancement instead of the critical bug fix. So, I suggest to delay it after this stable tag.
In <https://bugzilla.tianocore.org/show_bug.cgi?id=2723#c22> I mentioned "If there's strong disagreement, we can revert these BZ field changes", and in another mailing list thread Leif pointed out that the patch only affects builds that would otherwise fail.
So at the moment I'm neutral on this work; I'm fine either way (merged or postponed).
Liming: would you consider merging if we delayed the stable tag by a few days (mid next week)?
Anyway, let me fade into the background on this topic.
Thanks
Laszlo
>> -----Original Message-----
>> From: Leif Lindholm <leif@nuviainc.com>
>> Sent: Thursday, May 28, 2020 5:49 PM
>> To: Gary Lin <glin@suse.com>
>> Cc: devel@edk2.groups.io; ard.biesheuvel@arm.com; lersek@redhat.com;
>> Gao, Liming <liming.gao@intel.com>
>> Subject: Re: [edk2-devel] [PATCH v2] ArmPkg/CompilerIntrinsicsLib:
>> provide atomics intrinsics
>>
>> On Thu, May 28, 2020 at 09:36:33 +0800, Gary Lin wrote:
>>> On Wed, May 20, 2020 at 01:44:48PM +0200, Ard Biesheuvel wrote:
>>>> Gary reports the GCC 10 will emit calls to atomics intrinsics
>>>> routines unless -mno-outline-atomics is specified. This means
>>>> GCC-10 introduces new intrinsics, and even though it would be
>>>> possible to work around this by specifying the command line option,
>>>> this would require a new GCC10 toolchain profile to be created, which we prefer to avoid.
>>>>
>>>> So instead, add the new intrinsics to our library so they are
>>>> provided when necessary.
>>>>
>>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
>>> After applying this patch, gcc 10 can build ArmVirtPkg without the
>>> linking error.
>>>
>>> Tested-by: Gary Lin <glin@suse.com>
>>
>> Thanks Gary.
>>
>> Liming, can we consider this patch for stable tag please?
>>
>> With an added link to
>> https://bugzilla.tianocore.org/show_bug.cgi?id=2723 in the commit message:
>> Reviewed-by: Leif Lindholm <leif@nuviainc.com>
>>
>> /
>> Leif
>>
>>>> ---
>>>> v2:
>>>> - add missing .globl to export the functions from the object file
>>>> - add function end markers so the size of each is visible in the
>>>> ELF metadata
>>>> - add some comments to describe what is going on
>>>>
>>>> ArmPkg/Library/CompilerIntrinsicsLib/CompilerIntrinsicsLib.inf | 3 +
>>>> ArmPkg/Library/CompilerIntrinsicsLib/AArch64/Atomics.S | 142 ++++++++++++++++++++
>>>> 2 files changed, 145 insertions(+)
>>>>
>>>> diff --git
>>>> a/ArmPkg/Library/CompilerIntrinsicsLib/CompilerIntrinsicsLib.inf
>> b/ArmPkg/Library/CompilerIntrinsicsLib/CompilerIntrinsicsLib.inf
>>>> index d5bad9467758..fcf48c678119 100644
>>>> ---
>>>> a/ArmPkg/Library/CompilerIntrinsicsLib/CompilerIntrinsicsLib.inf
>>>> +++ b/ArmPkg/Library/CompilerIntrinsicsLib/CompilerIntrinsicsLib.in
>>>> +++ f
>>>> @@ -79,6 +79,9 @@ [Sources.ARM]
>>>> Arm/ldivmod.asm | MSFT
>>>> Arm/llsr.asm | MSFT
>>>>
>>>> +[Sources.AARCH64]
>>>> + AArch64/Atomics.S | GCC
>>>> +
>>>> [Packages]
>>>> MdePkg/MdePkg.dec
>>>> ArmPkg/ArmPkg.dec
>>>> diff --git a/ArmPkg/Library/CompilerIntrinsicsLib/AArch64/Atomics.S
>>>> b/ArmPkg/Library/CompilerIntrinsicsLib/AArch64/Atomics.S
>>>> new file mode 100644
>>>> index 000000000000..dc61d6bb8e52
>>>> --- /dev/null
>>>> +++ b/ArmPkg/Library/CompilerIntrinsicsLib/AArch64/Atomics.S
>>>> @@ -0,0 +1,142 @@
>>>> +#-----------------------------------------------------------------
>>>> +-------------
>>>> +#
>>>> +# Copyright (c) 2020, Arm, Limited. All rights reserved.<BR> # #
>>>> +SPDX-License-Identifier: BSD-2-Clause-Patent #
>>>> +#-----------------------------------------------------------------
>>>> +-------------
>>>> +
>>>> + /*
>>>> + * Provide the GCC intrinsics that are required when using GCC 9 or
>>>> + * later with the -moutline-atomics options (which became the default
>>>> + * in GCC 10)
>>>> + */
>>>> + .arch armv8-a
>>>> +
>>>> + .macro reg_alias, pfx, sz
>>>> + r0_\sz .req \pfx\()0
>>>> + r1_\sz .req \pfx\()1
>>>> + tmp0_\sz .req \pfx\()16
>>>> + tmp1_\sz .req \pfx\()17
>>>> + .endm
>>>> +
>>>> + /*
>>>> + * Define register aliases of the right type for each size
>>>> + * (xN for 8 bytes, wN for everything smaller)
>>>> + */
>>>> + reg_alias w, 1
>>>> + reg_alias w, 2
>>>> + reg_alias w, 4
>>>> + reg_alias x, 8
>>>> +
>>>> + .macro fn_start, name:req
>>>> + .section .text.\name
>>>> + .globl \name
>>>> + .type \name, %function
>>>> +\name\():
>>>> + .endm
>>>> +
>>>> + .macro fn_end, name:req
>>>> + .size \name, . - \name
>>>> + .endm
>>>> +
>>>> + /*
>>>> + * Emit an atomic helper for \model with operands of size \sz, using
>>>> + * the operation specified by \insn (which is the LSE name), and which
>>>> + * can be implemented using the generic load-locked/store-conditional
>>>> + * (LL/SC) sequence below, using the arithmetic operation given by
>>>> + * \opc.
>>>> + */
>>>> + .macro emit_ld_sz, sz:req, insn:req, opc:req, model:req, s, a, l
>>>> + fn_start __aarch64_\insn\()\sz\()\model
>>>> + mov tmp0_\sz, r0_\sz
>>>> +0: ld\a\()xr\s r0_\sz, [x1]
>>>> + .ifnc \insn, swp
>>>> + \opc tmp1_\sz, r0_\sz, tmp0_\sz
>>>> + .else
>>>> + \opc tmp1_\sz, tmp0_\sz
>>>> + .endif
>>>> + st\l\()xr\s w15, tmp1_\sz, [x1]
>>>> + cbnz w15, 0b
>>>> + ret
>>>> + fn_end __aarch64_\insn\()\sz\()\model
>>>> + .endm
>>>> +
>>>> + /*
>>>> + * Emit atomic helpers for \model for operand sizes in the
>>>> + * set {1, 2, 4, 8}, for the instruction pattern given by
>>>> + * \insn. (This is the LSE name, but this implementation uses
>>>> + * the generic LL/SC sequence using \opc as the arithmetic
>>>> + * operation on the target.)
>>>> + */
>>>> + .macro emit_ld, insn:req, opc:req, model:req, a, l
>>>> + emit_ld_sz 1, \insn, \opc, \model, b, \a, \l
>>>> + emit_ld_sz 2, \insn, \opc, \model, h, \a, \l
>>>> + emit_ld_sz 4, \insn, \opc, \model, , \a, \l
>>>> + emit_ld_sz 8, \insn, \opc, \model, , \a, \l
>>>> + .endm
>>>> +
>>>> + /*
>>>> + * Emit the compare and swap helper for \model and size \sz
>>>> + * using LL/SC instructions.
>>>> + */
>>>> + .macro emit_cas_sz, sz:req, model:req, uxt:req, s, a, l
>>>> + fn_start __aarch64_cas\sz\()\model
>>>> + \uxt tmp0_\sz, r0_\sz
>>>> +0: ld\a\()xr\s r0_\sz, [x2]
>>>> + cmp r0_\sz, tmp0_\sz
>>>> + bne 1f
>>>> + st\l\()xr\s w15, r1_\sz, [x2]
>>>> + cbnz w15, 0b
>>>> +1: ret
>>>> + fn_end __aarch64_cas\sz\()\model
>>>> + .endm
>>>> +
>>>> + /*
>>>> + * Emit compare-and-swap helpers for \model for operand sizes in the
>>>> + * set {1, 2, 4, 8, 16}.
>>>> + */
>>>> + .macro emit_cas, model:req, a, l
>>>> + emit_cas_sz 1, \model, uxtb, b, \a, \l
>>>> + emit_cas_sz 2, \model, uxth, h, \a, \l
>>>> + emit_cas_sz 4, \model, mov , , \a, \l
>>>> + emit_cas_sz 8, \model, mov , , \a, \l
>>>> +
>>>> + /*
>>>> + * We cannot use the parameterized sequence for 16 byte CAS, so we
>>>> + * need to define it explicitly.
>>>> + */
>>>> + fn_start __aarch64_cas16\model
>>>> + mov x16, x0
>>>> + mov x17, x1
>>>> +0: ld\a\()xp x0, x1, [x4]
>>>> + cmp x0, x16
>>>> + ccmp x1, x17, #0, eq
>>>> + bne 1f
>>>> + st\l\()xp w15, x16, x17, [x4]
>>>> + cbnz w15, 0b
>>>> +1: ret
>>>> + fn_end __aarch64_cas16\model
>>>> + .endm
>>>> +
>>>> + /*
>>>> + * Emit the set of GCC outline atomic helper functions for
>>>> + * the memory ordering model given by \model:
>>>> + * - relax unordered loads and stores
>>>> + * - acq load-acquire, unordered store
>>>> + * - rel unordered load, store-release
>>>> + * - acq_rel load-acquire, store-release
>>>> + */
>>>> + .macro emit_model, model:req, a, l
>>>> + emit_ld ldadd, add, \model, \a, \l
>>>> + emit_ld ldclr, bic, \model, \a, \l
>>>> + emit_ld ldeor, eor, \model, \a, \l
>>>> + emit_ld ldset, orr, \model, \a, \l
>>>> + emit_ld swp, mov, \model, \a, \l
>>>> + emit_cas \model, \a, \l
>>>> + .endm
>>>> +
>>>> + emit_model _relax
>>>> + emit_model _acq, a
>>>> + emit_model _rel,, l
>>>> + emit_model _acq_rel, a, l
>>>> --
>>>> 2.17.1
>>>>
>>>>
>>>>
>>>>
>
prev parent reply other threads:[~2020-05-29 3:04 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-05-20 11:44 [PATCH v2] ArmPkg/CompilerIntrinsicsLib: provide atomics intrinsics Ard Biesheuvel
2020-05-20 12:37 ` [edk2-devel] " Philippe Mathieu-Daudé
2020-05-21 16:40 ` Philippe Mathieu-Daudé
2020-05-21 16:45 ` Ard Biesheuvel
2020-05-21 16:59 ` Philippe Mathieu-Daudé
2020-05-21 17:02 ` Ard Biesheuvel
2020-05-21 17:07 ` Philippe Mathieu-Daudé
2020-05-20 15:59 ` Laszlo Ersek
2020-05-21 11:23 ` Leif Lindholm
2020-05-21 12:58 ` [edk2-devel] " Ard Biesheuvel
2020-05-21 13:16 ` Leif Lindholm
2020-05-21 13:31 ` Ard Biesheuvel
2020-05-21 14:16 ` Leif Lindholm
2020-05-21 20:22 ` Laszlo Ersek
2020-05-22 8:16 ` Ard Biesheuvel
2020-05-22 10:54 ` Leif Lindholm
2020-05-22 13:27 ` Ard Biesheuvel
2020-05-22 19:04 ` Laszlo Ersek
2020-06-02 0:50 ` Liming Gao
2020-06-02 7:09 ` Ard Biesheuvel
2020-05-28 1:36 ` Gary Lin
2020-05-28 9:49 ` Leif Lindholm
2020-05-28 15:48 ` Liming Gao
2020-05-28 20:09 ` Laszlo Ersek
2020-05-29 3:04 ` Liming Gao [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=SN6PR11MB3197B505F3BE9C1F0FCF815B808F0@SN6PR11MB3197.namprd11.prod.outlook.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox