Re: [PATCH] arm64: jump_label: use constraint "S" instead of "i"

[Dave Martin]

On Wed, Jan 31, 2024 at 08:16:04AM +0100, Ard Biesheuvel wrote:
> Hello Fangrui,
> 
> On Wed, 31 Jan 2024 at 07:53, Fangrui Song <maskray@xxxxxxxxxx> wrote:
> >
> > The constraint "i" seems to be copied from x86 (and with a redundant
> > modifier "c"). It works with -fno-PIE but not with -fPIE/-fPIC in GCC's
> > aarch64 port.

(I'm not sure of the exact history, but the "c" may be inherited from
arm, where an output modifier was needed to suppress the "#" that
prefixes immediates in the traditional asm syntax.  This does not
actually seem to be required for AArch64: rather while a # is allowed
and still considered good style in handwritten asm code, the syntax
doesn't require it, and the compiler doesn't emit it for "i" arguments,
AFAICT.)

> > The constraint "S", which denotes a symbol reference (e.g. function,
> > global variable) or label reference, is more appropriate, and has been
> > available in GCC since 2012 and in Clang since 7.0.
> >
> > Signed-off-by: Fangrui Song <maskray@xxxxxxxxxx>
> > Link: https://maskray.me/blog/2024-01-30-raw-symbol-names-in-inline-assembly
> > ---
> >  arch/arm64/include/asm/jump_label.h | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/arm64/include/asm/jump_label.h b/arch/arm64/include/asm/jump_label.h
> > index 48ddc0f45d22..31862b3bb33d 100644
> > --- a/arch/arm64/include/asm/jump_label.h
> > +++ b/arch/arm64/include/asm/jump_label.h
> > @@ -23,9 +23,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 - .                  \n\t"
> >                  "      .popsection                             \n\t"
> > -                :  :  "i"(&((char *)key)[branch]) :  : l_yes);
> > +                :  :  "S"(&((char *)key)[branch]) :  : l_yes);
> 
> 'key' is not used as a raw symbol name. We should make this
> 
> "    .quad   %0 + %1 - ."
> 
> and
> 
> ::  "S"(key), "i"(branch) :: l_yes);
>
> if we want to really clean this up.

This hides more logic in the asm so it's arguably more cryptic
(although the code is fairly cryptic to begin with -- I don't really
see why the argument wasn't written as the equivalent
(char *)key + branch...)

Anyway, I don't think the "i" versys "S" distinction makes any
difference without -fpic or equivalent, so it is not really relevant
for the kernel (except that "S" breaks compatibility with older
compilers...)


I think the main advantage of "S" is that it stops you accidentally
emitting undesirable relocations from asm code that is not written for
the -fpic case.

But just changing "i" to "S" is not sufficient to port asms to -fpic:
the asms still need to be reviewed.


So unless the asm has been reviewed for position-independence, it may
anyway be better to stick with "i" so that the compiler actually chokes
if someone tries to build the code with -fpic.

Since we are not trying to run arbitraily many running kernels in a
common address space (and not likely to do that), I'm not sure that we
would ever build the kernel with -fpic except for a few special-case
bits like the EFI stub and vDSO... unless I've missed something?

If there's another reason why "S" is advantageous though, I'm happy to
be corrected.

[...]

Cheers
---Dave