Received-SPF: pass (google.com: domain of linux-kernel+bounces-71086-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Precedence: bulk
MIME-Version: 1.0
References: <20240206074552.541154-1-maskray@google.com>
In-Reply-To: <20240206074552.541154-1-maskray@google.com>
From: Geert Uytterhoeven <geert@linux-m68k.org>
Date: Mon, 19 Feb 2024 11:03:39 +0100
Message-ID: <CAMuHMdVOoxVFohcwwoQtY0KgZEvbjgJJ6mib8Fabo97P0krEqw@mail.gmail.com>
Subject: Re: [PATCH v2] arm64: jump_label: use constraints "Si" instead of "i"
To: Fangrui Song <maskray@google.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>, Will Deacon <will@kernel.org>, 
	linux-arm-kernel@lists.infradead.org, Jisheng Zhang <jszhang@kernel.org>, 
	Dave Martin <Dave.Martin@arm.com>, Ard Biesheuvel <ardb@kernel.org>, Peter Smith <peter.smith@arm.com>, 
	llvm@lists.linux.dev, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Fangrui,

On Tue, Feb 6, 2024 at 8:46=E2=80=AFAM Fangrui Song <maskray@google.com> wr=
ote:
> The generic constraint "i" seems to be copied from x86 or arm (and with
> a redundant generic operand modifier "c"). It works with -fno-PIE but
> not with -fPIE/-fPIC in GCC's aarch64 port.

Thanks for your patch, which is now commit f9daab0ad01cf9d1 ("arm64:
jump_label: use constraints "Si" instead of "i"") in v6.8-rc5.

> The machine constraint "S", which denotes a symbol or label reference
> with a constant offset, supports PIC and has been available in GCC since
> 2012 and in Clang since 7.0. However, Clang before 19 does not support
> "S" on a symbol with a constant offset [1] (e.g.
> `static_key_false(&nf_hooks_needed[pf][hook])` in
> include/linux/netfilter.h), so we use "i" as a fallback.

https://gcc.gnu.org/releases.html says gcc-5 was released in 2015,
i.e. after 2012 ...

> Suggested-by: Ard Biesheuvel <ardb@kernel.org>
> Signed-off-by: Fangrui Song <maskray@google.com>
> Link: https://github.com/llvm/llvm-project/pull/80255 [1]
>
> ---
> Changes from
> arm64: jump_label: use constraint "S" instead of "i" (https://lore.kernel=
org/all/20240131065322.1126831-1-maskray@google.com/)
>
> * Use "Si" as Ard suggested to support Clang<19
> * Make branch a separate operand
>
> Changes from v1:
>
> * Use asmSymbolicName for readability

But it still fails on gcc-5:

    arch/arm64/include/asm/jump_label.h:25:2: error: invalid 'asm':
invalid operand
      asm goto(
      ^

http://kisskb.ellerman.id.au/kisskb/buildresult/15129281/

> --- a/arch/arm64/include/asm/jump_label.h
> +++ b/arch/arm64/include/asm/jump_label.h
> @@ -15,6 +15,10 @@
>
>  #define JUMP_LABEL_NOP_SIZE            AARCH64_INSN_SIZE
>
> +/*
> + * Prefer the constraint "S" to support PIC with GCC. Clang before 19 do=
es not
> + * support "S" on a symbol with a constant offset, so we use "i" as a fa=
llback.
> + */
>  static __always_inline bool arch_static_branch(struct static_key * const=
 key,
>                                                const bool branch)
>  {
> @@ -23,9 +27,9 @@ static __always_inline bool arch_static_branch(struct s=
tatic_key * const key,
>                  "      .pushsection    __jump_table, \"aw\"    \n\t"
>                  "      .align          3                       \n\t"
>                  "      .long           1b - ., %l[l_yes] - .   \n\t"
> -                "      .quad           %c0 - .                 \n\t"
> +                "      .quad           (%[key] - .) + %[bit0]  \n\t"
>                  "      .popsection                             \n\t"
> -                :  :  "i"(&((char *)key)[branch]) :  : l_yes);
> +                :  :  [key]"Si"(key), [bit0]"i"(branch) :  : l_yes);
>
>         return false;
>  l_yes:
> @@ -40,9 +44,9 @@ static __always_inline bool arch_static_branch_jump(str=
uct static_key * const ke
>                  "      .pushsection    __jump_table, \"aw\"    \n\t"
>                  "      .align          3                       \n\t"
>                  "      .long           1b - ., %l[l_yes] - .   \n\t"
> -                "      .quad           %c0 - .                 \n\t"
> +                "      .quad           (%[key] - .) + %[bit0]  \n\t"
>                  "      .popsection                             \n\t"
> -                :  :  "i"(&((char *)key)[branch]) :  : l_yes);
> +                :  :  [key]"Si"(key), [bit0]"i"(branch) :  : l_yes);
>
>         return false;
>  l_yes:

Gr{oetje,eeting}s,

                        Geert

--=20
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k=
org

In personal conversations with technical people, I call myself a hacker. Bu=
t
when I'm talking to journalists I just say "programmer" or something like t=
hat.
                                -- Linus Torvalds