[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZbpEnDK3U/O24ng0@e133380.arm.com>
Date: Wed, 31 Jan 2024 13:01:16 +0000
From: Dave Martin <Dave.Martin@....com>
To: Ard Biesheuvel <ardb@...nel.org>
Cc: Fangrui Song <maskray@...gle.com>,
Catalin Marinas <catalin.marinas@....com>,
Will Deacon <will@...nel.org>, linux-arm-kernel@...ts.infradead.org,
Jisheng Zhang <jszhang@...nel.org>, llvm@...ts.linux.dev,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] arm64: jump_label: use constraint "S" instead of "i"
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@...gle.com> 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@...gle.com>
> > 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
Powered by blists - more mailing lists