Message-ID: <CAMj1kXGN-nKSzxJoyM9peBTDevuPkH-+P2UzH746P-F913Dg-g@mail.gmail.com>
Date: Sat, 3 Feb 2024 10:50:44 +0100
From: Ard Biesheuvel <ardb@...nel.org>
To: Fangrui Song <maskray@...gle.com>
Cc: Dave Martin <Dave.Martin@....com>, Catalin Marinas <catalin.marinas@....com>, 
	Will Deacon <will@...nel.org>, linux-arm-kernel@...ts.infradead.org, 
	Jisheng Zhang <jszhang@...nel.org>, Peter Smith <peter.smith@....com>, llvm@...ts.linux.dev, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] arm64: jump_label: use constraints "Si" instead of "i"

On Fri, 2 Feb 2024 at 23:51, Fangrui Song <maskray@...gle.com> wrote:
>
> On 2024-02-02, Dave Martin wrote:
> >On Fri, Feb 02, 2024 at 05:32:33PM +0100, Ard Biesheuvel wrote:
> >> On Fri, 2 Feb 2024 at 16:56, Dave Martin <Dave.Martin@....com> wrote:
> >> >
> >> > On Thu, Feb 01, 2024 at 02:35:45PM -0800, Fangrui Song wrote:
> >> > > 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.
> >> > >
> >> > > 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.
> >> > >
> >> > > Suggested-by: Ard Biesheuvel <ardb@...nel.org>
> >> > > Signed-off-by: Fangrui Song <maskray@...gle.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
> >> > > ---
> >> > >  arch/arm64/include/asm/jump_label.h | 12 ++++++++----
> >> > >  1 file changed, 8 insertions(+), 4 deletions(-)
> >> > >
> >> > > diff --git a/arch/arm64/include/asm/jump_label.h b/arch/arm64/include/asm/jump_label.h
> >> > > index 48ddc0f45d22..1f7d529be608 100644
> >> > > --- 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 does not
> >> > > + * support "S" on a symbol with a constant offset, so we use "i" as a fallback.
> >> > > + */
> >> > >  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 static_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           %0 + %1 - .             \n\t"
> >> > >                "      .popsection                             \n\t"
> >> > > -              :  :  "i"(&((char *)key)[branch]) :  : l_yes);
> >> > > +              :  :  "Si"(key), "i"(branch) :  : l_yes);
> >> >
> >> > Is the meaning of multi-alternatives well defined if different arguments
> >> > specify different numbers of alternatives?  The GCC documentation says:
> >> >
> >> > https://gcc.gnu.org/onlinedocs/gcc/Multi-Alternative.html:
> >> >
> >> > -8<-
> >> >
> >> > [...] All operands for a single instruction must have the same number of
> >> > alternatives.
> >> >
> >> > ->8-
> >> >
> >>
> >> AIUI that does not apply here. That reasons about multiple arguments
> >> having more than one constraint, where not all combinations of those
> >> constraints are supported by the instruction.
> >
> >Ah, scratch that: I'm confusing multi-alternatives with simple
> >constraints: all arguments must have the same number of comma-separated
> >lists of constraint letters, but each list can contain as many or as few
> >letters as are needed to match the operand.
> >
> >So "Si"(), "i"() is fine.
>
> "Si" is fine for GCC and Clang.
> "i" is fine for Clang but not for GCC PIC.
>
>      https://maskray.me/blog/2024-01-30-raw-symbol-names-in-inline-assembly#aarch64
>
>      In gcc/config/aarch64, LEGITIMATE_PIC_OPERAND_P(X) disallows any symbol
>      reference, which means that "i" and "s" cannot be used for PIC. Instead,
>      the constraint "S" has been supported since the initial port (2012) to
>      reference a symbol or label.
>
> I am also not familiar with
> https://gcc.gnu.org/onlinedocs/gcc/Multi-Alternative.html (comma in a
> constraint string). Thankfully we don't need this powerful construct:)
>
> >> > Also, I still think it may be redundant to move the addition inside the
> >> > asm, so long as Clang is happy with the symbol having an offset.
> >> >
> >>
> >> Older Clang is not happy with the symbol having an offset.
> >>
> >> And given that the key pointer and the 'branch' boolean are two
> >> distinct inputs to this function, I struggle to understand why you
> >> feel it is better to combine them in the argument. 'branch' is used to
> >> decide whether or not to set bit 0, independently of the value of key.
> >> Using array indexing to combine those values together to avoid an
> >> addition in the asm obfuscates the code.
> >
> >This was more about not making changes that don't need to be made,
> >unless it clearly makes the code better.
> >
> >But if some verions of Clang accept "S" but choke if there's an offset,
> >then moving the addition into the asm seems the way to go.
> >
> >(I may have misread the commit message on this point.)
> >
> >[...]
> >
> >Cheers
> >---Dave
>
> I am convinced by Ard' argument that two inputs (key, branch) deserve
> two operands.
> The existing "i"(&((char *)key)[branch]) is kinda ugly and also longer:)

If it helps clarify things, we might do something like

".quad  (%[key]  - .) + %[bit0]"

: : [key]"Si"(key), [bit0]"i"(branch) :  : l_yes);

Powered by blists - more mailing lists