Re: [edk2-devel] [PATCH v2] ArmPkg/CompilerIntrinsicsLib: provide atomics intrinsics

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 5/21/20 7:02 PM, Ard Biesheuvel wrote:
> On 5/21/20 6:59 PM, Philippe Mathieu-Daudé wrote:
>> On 5/21/20 6:45 PM, Ard Biesheuvel wrote:
>>> On 5/21/20 6:40 PM, Philippe Mathieu-Daudé via groups.io wrote:
>>>> On 5/20/20 2:37 PM, Philippe Mathieu-Daudé wrote:
>>>>> Hi Ard,
>>>>>
>>>>> On 5/20/20 1:44 PM, 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>
>>>>>> ---
>>>>>> 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
>>>>>
>>>>> Thanks, head hurts a bit less...
>>>>>
>>>>>>
>>>>>>   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.inf
>>>>>> @@ -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
>>>>>
>>>>> I see at the end \s is in {,b,h} range.
>>>>>
>>>>> Don't you need to use x15 on 64-bit?
>>>>
>>>> Ard, I expanded all macros and reviewed this patch, but I am still 
>>>> having hard time to figure why w15 temp is OK instead of x15. Any hint?
>>>>
>>>
>>> Why do you think it should be x15?
>>
>> I.e.:
>>
>> https://developer.arm.com/docs/100076/0100/instruction-set-reference/a64-data-transfer-instructions/ldaxr 
>>
>>
>> Syntax
>>
>> LDAXR Wt, [Xn|SP{,#0}] ; 32-bit
>> LDAXR Xt, [Xn|SP{,#0}] ; 64-bit
>>
> 
> That is the load part, where Wt and Xt map onto r0 in the code above.
> 
> https://developer.arm.com/docs/100076/0100/instruction-set-reference/a64-data-transfer-instructions/stlxr 

Grrrrrrrrr I had the both link open and looked at the wrong tab 
:facepalm: Sorry for the noise.

> 
> 
> gives the description for the store part, where the w15 register is 
> actually used.
> 

Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>