[PATCH] arm64: relax jump label ASM constraints

[PATCH] arm64: relax jump label ASM constraints

[Rémi Denis-Courmont]

From: Remi Denis-Courmont <remi.denis.courmont@huawei.com>

The static key address is stored in the jump label table. It needs to
be a run-time constant. However, it does not need to be a constant
suitable for expansion as an immediate value, given that it is
expanded in a full 64-bits (.quad) statement.

Signed-off-by: Remi Denis-Courmont <remi.denis.courmont@huawei.com>
---
 arch/arm64/include/asm/jump_label.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/include/asm/jump_label.h b/arch/arm64/include/asm/jump_label.h
index cea441b6aa5d..dcc0a32f073b 100644
--- a/arch/arm64/include/asm/jump_label.h
+++ b/arch/arm64/include/asm/jump_label.h
@@ -25,7 +25,7 @@ static __always_inline bool arch_static_branch(struct static_key *key,
 		 "	.long		1b - ., %l[l_yes] - .	\n\t"
 		 "	.quad		%c0 - .			\n\t"
 		 "	.popsection				\n\t"
-		 :  :  "i"(&((char *)key)[branch]) :  : l_yes);
+		 :  :  "S"(&((char *)key)[branch]) :  : l_yes);
 
 	return false;
 l_yes:
@@ -42,7 +42,7 @@ static __always_inline bool arch_static_branch_jump(struct static_key *key,
 		 "	.long		1b - ., %l[l_yes] - .	\n\t"
 		 "	.quad		%c0 - .			\n\t"
 		 "	.popsection				\n\t"
-		 :  :  "i"(&((char *)key)[branch]) :  : l_yes);
+		 :  :  "S"(&((char *)key)[branch]) :  : l_yes);
 
 	return false;
 l_yes:
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: relax jump label ASM constraints

[Mark Rutland]

On Wed, Mar 04, 2020 at 11:34:19AM +0200, Rémi Denis-Courmont wrote:
> From: Remi Denis-Courmont <remi.denis.courmont@huawei.com>
> 
> The static key address is stored in the jump label table. It needs to
> be a run-time constant. However, it does not need to be a constant
> suitable for expansion as an immediate value, given that it is
> expanded in a full 64-bits (.quad) statement.

I'm not sure that's quite true, since it's used in an expression
evaluated by the assembler. IIRC the "%c0 - ." expression cannot be
represented by an AArch64 ELF relocation.

Looking at the gcc docs:

Under "Simple Constaints", 'i' is defined:

| An immediate integer operand (one with constant value) is allowed.
| This includes symbolic constants whose values will be known only at
| assembly time or later. 

Note that 'i' allows any constant integer value, unlike the
AArch64-specific 'I' constraint that only allows immediates suitable for
ADD instructions.

Under "Constraints for Particular Machines", "AArch64 family", 'S' is
defined:

| An absolute symbolic address or a label reference

... and IIUC either should work here.

Have I missed something that 'S' allows that 'i' does not, or is there a
functional problem today?

Thanks,
Mark.

> 
> Signed-off-by: Remi Denis-Courmont <remi.denis.courmont@huawei.com>
> ---
>  arch/arm64/include/asm/jump_label.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/jump_label.h b/arch/arm64/include/asm/jump_label.h
> index cea441b6aa5d..dcc0a32f073b 100644
> --- a/arch/arm64/include/asm/jump_label.h
> +++ b/arch/arm64/include/asm/jump_label.h
> @@ -25,7 +25,7 @@ static __always_inline bool arch_static_branch(struct static_key *key,
>  		 "	.long		1b - ., %l[l_yes] - .	\n\t"
>  		 "	.quad		%c0 - .			\n\t"
>  		 "	.popsection				\n\t"
> -		 :  :  "i"(&((char *)key)[branch]) :  : l_yes);
> +		 :  :  "S"(&((char *)key)[branch]) :  : l_yes);
>  
>  	return false;
>  l_yes:
> @@ -42,7 +42,7 @@ static __always_inline bool arch_static_branch_jump(struct static_key *key,
>  		 "	.long		1b - ., %l[l_yes] - .	\n\t"
>  		 "	.quad		%c0 - .			\n\t"
>  		 "	.popsection				\n\t"
> -		 :  :  "i"(&((char *)key)[branch]) :  : l_yes);
> +		 :  :  "S"(&((char *)key)[branch]) :  : l_yes);
>  
>  	return false;
>  l_yes:
> -- 
> 2.25.1
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: relax jump label ASM constraints

[Rémi Denis-Courmont]

Hi,

Le keskiviikkona 4. maaliskuuta 2020, 14.39.33 EET Mark Rutland a écrit :
> On Wed, Mar 04, 2020 at 11:34:19AM +0200, Rémi Denis-Courmont wrote:
> > From: Remi Denis-Courmont <remi.denis.courmont@huawei.com>
> > 
> > The static key address is stored in the jump label table. It needs to
> > be a run-time constant. However, it does not need to be a constant
> > suitable for expansion as an immediate value, given that it is
> > expanded in a full 64-bits (.quad) statement.
> 
> I'm not sure that's quite true,

It it indeed not true. Some time elapsed between writing the patch and 
describing it, and I'd forgetten the exact details in the mean time :$

What's actually happening is that the current constraint wants a constant, but 
in reality, either a constant or a symbol is acceptable. In the later case, 
the .quad will be a constant.

> since it's used in an expression
> evaluated by the assembler. IIRC the "%c0 - ." expression cannot be
> represented by an AArch64 ELF relocation.

Changing the constraint won't affect the ability or inability of the compiler 
and linker to generate a relocation. In this case, the value is essentially an 
offset and it just works either way.

> Have I missed something that 'S' allows that 'i' does not, or is there a
> functional problem today?

No. I don't remember exactly which compiler or compiler flags and kernel 
configuration I was trying to notice the inconsistency. But it's only a latent 
issue as far as I am aware.

-- 
Rémi Denis-Courmont
http://www.remlab.net/




_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel