Messages in this thread | | | Date | Wed, 31 Jan 2024 13:01:16 +0000 | From | Dave Martin <> | 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@google.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@google.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
| |