[PATCH] arm/arm64: smccc: Use xN for arm64 register constraints with clang

[PATCH] arm/arm64: smccc: Use xN for arm64 register constraints with clang

[Matthias Kaehlcke]

The SMCCC v1.1 inline primitive code added by commit f2d3b2e8759a
("arm/arm64: smccc: Implement SMCCC v1.1 inline primitive") uses
register names r0, r1, ... for constraints. This breaks clang builds
for arm64, since clang currently only accepts x0, x1, ... in this
context. Use xN names for register constrains when building for arm64
with clang.

Fixes: f2d3b2e8759a ("arm/arm64: smccc: Implement SMCCC v1.1 inline primitive")
Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---
Note: Support for rN in future versions of clang is underway:
  https://bugs.llvm.org/show_bug.cgi?id=36862

 include/linux/arm-smccc.h | 54 ++++++++++++++++++++++-----------------
 1 file changed, 30 insertions(+), 24 deletions(-)

diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h
index a031897fca76..5c55d9e8c2ef 100644
--- a/include/linux/arm-smccc.h
+++ b/include/linux/arm-smccc.h
@@ -155,6 +155,11 @@ asmlinkage void __arm_smccc_hvc(unsigned long a0, unsigned long a1,
 
 #define SMCCC_SMC_INST	"smc	#0"
 #define SMCCC_HVC_INST	"hvc	#0"
+#ifndef __clang__
+#define __reg__ "r"
+#else
+#define __reg__ "x"
+#endif
 
 #elif defined(CONFIG_ARM)
 #include <asm/opcodes-sec.h>
@@ -162,6 +167,7 @@ asmlinkage void __arm_smccc_hvc(unsigned long a0, unsigned long a1,
 
 #define SMCCC_SMC_INST	__SMC(0)
 #define SMCCC_HVC_INST	__HVC(0)
+#define __reg__ "r"
 
 #endif
 
@@ -187,54 +193,54 @@ asmlinkage void __arm_smccc_hvc(unsigned long a0, unsigned long a1,
 #define __constraint_read_1
 #define __constraint_read_2
 #define __constraint_read_3
-#define __constraint_read_4	"r" (r4)
-#define __constraint_read_5	__constraint_read_4, "r" (r5)
-#define __constraint_read_6	__constraint_read_5, "r" (r6)
-#define __constraint_read_7	__constraint_read_6, "r" (r7)
+#define __constraint_read_4	__reg__ (r4)
+#define __constraint_read_5	__constraint_read_4, __reg__ (r5)
+#define __constraint_read_6	__constraint_read_5, __reg__ (r6)
+#define __constraint_read_7	__constraint_read_6, __reg__ (r7)
 
 #define __declare_arg_0(a0, res)					\
 	struct arm_smccc_res   *___res = res;				\
-	register u32           r0 asm("r0") = a0;			\
-	register unsigned long r1 asm("r1");				\
-	register unsigned long r2 asm("r2");				\
-	register unsigned long r3 asm("r3")
+	register u32           r0 asm(__reg__"0") = a0;			\
+	register unsigned long r1 asm(__reg__"1");			\
+	register unsigned long r2 asm(__reg__"2");			\
+	register unsigned long r3 asm(__reg__"3")
 
 #define __declare_arg_1(a0, a1, res)					\
 	struct arm_smccc_res   *___res = res;				\
-	register u32           r0 asm("r0") = a0;			\
-	register typeof(a1)    r1 asm("r1") = a1;			\
-	register unsigned long r2 asm("r2");				\
-	register unsigned long r3 asm("r3")
+	register u32           r0 asm(__reg__"0") = a0;			\
+	register typeof(a1)    r1 asm(__reg__"1") = a1;			\
+	register unsigned long r2 asm(__reg__"2");			\
+	register unsigned long r3 asm(__reg__"3")
 
 #define __declare_arg_2(a0, a1, a2, res)				\
 	struct arm_smccc_res   *___res = res;				\
-	register u32           r0 asm("r0") = a0;			\
-	register typeof(a1)    r1 asm("r1") = a1;			\
-	register typeof(a2)    r2 asm("r2") = a2;			\
-	register unsigned long r3 asm("r3")
+	register u32           r0 asm(__reg__"0") = a0;			\
+	register typeof(a1)    r1 asm(__reg__"1") = a1;			\
+	register typeof(a2)    r2 asm(__reg__"2") = a2;			\
+	register unsigned long r3 asm(__reg__"3")
 
 #define __declare_arg_3(a0, a1, a2, a3, res)				\
 	struct arm_smccc_res   *___res = res;				\
-	register u32           r0 asm("r0") = a0;			\
-	register typeof(a1)    r1 asm("r1") = a1;			\
-	register typeof(a2)    r2 asm("r2") = a2;			\
-	register typeof(a3)    r3 asm("r3") = a3
+	register u32           r0 asm(__reg__"0") = a0;			\
+	register typeof(a1)    r1 asm(__reg__"1") = a1;			\
+	register typeof(a2)    r2 asm(__reg__"2") = a2;			\
+	register typeof(a3)    r3 asm(__reg__"3") = a3
 
 #define __declare_arg_4(a0, a1, a2, a3, a4, res)			\
 	__declare_arg_3(a0, a1, a2, a3, res);				\
-	register typeof(a4) r4 asm("r4") = a4
+	register typeof(a4) r4 asm(__reg__"4") = a4
 
 #define __declare_arg_5(a0, a1, a2, a3, a4, a5, res)			\
 	__declare_arg_4(a0, a1, a2, a3, a4, res);			\
-	register typeof(a5) r5 asm("r5") = a5
+	register typeof(a5) r5 asm(__reg__"5") = a5
 
 #define __declare_arg_6(a0, a1, a2, a3, a4, a5, a6, res)		\
 	__declare_arg_5(a0, a1, a2, a3, a4, a5, res);			\
-	register typeof(a6) r6 asm("r6") = a6
+	register typeof(a6) r6 asm(__reg__"6") = a6
 
 #define __declare_arg_7(a0, a1, a2, a3, a4, a5, a6, a7, res)		\
 	__declare_arg_6(a0, a1, a2, a3, a4, a5, a6, res);		\
-	register typeof(a7) r7 asm("r7") = a7
+	register typeof(a7) r7 asm(__reg__"7") = a7
 
 #define ___declare_args(count, ...) __declare_arg_ ## count(__VA_ARGS__)
 #define __declare_args(count, ...)  ___declare_args(count, __VA_ARGS__)
-- 
2.17.0.rc0.231.g781580f067-goog

[PATCH] arm/arm64: smccc: Use xN for arm64 register constraints with clang

[Nick Desaulniers]

Note that a patch in this form has previously been implemented by:

Andrey Konovalov <andreyknvl@google.com>:
https://gist.github.com/xairy/ee11682ea86044a45c0291c528cd936f

and another by:

Greg Hackmann <ghackmann@google.com>:
https://android-review.googlesource.com/c/kernel/common/+/645181

If you used either as a reference, you may want to credit them with a
`Suggested-by:` in the commit message.

On Thu, Mar 22, 2018 at 2:28 PM Matthias Kaehlcke <mka@chromium.org> wrote:
> +#ifndef __clang__
> +#define __reg__ "r"
> +#else
> +#define __reg__ "x"
> +#endif

Can this be flipped to #ifdef __clang__ ?  having an if...else where the
conditional negated is kind of funny.

--
Thanks,
~Nick Desaulniers

[PATCH] arm/arm64: smccc: Use xN for arm64 register constraints with clang

[Matthias Kaehlcke]

El Thu, Mar 22, 2018 at 10:26:18PM +0000 Nick Desaulniers ha dit:

> Note that a patch in this form has previously been implemented by:
> 
> Andrey Konovalov <andreyknvl@google.com>:
> https://gist.github.com/xairy/ee11682ea86044a45c0291c528cd936f
> 
> and another by:
> 
> Greg Hackmann <ghackmann@google.com>:
> https://android-review.googlesource.com/c/kernel/common/+/645181
> 
> If you used either as a reference, you may want to credit them with a
> `Suggested-by:` in the commit message.

Not really, but I think I prefer Greg's version over mine and might
use it in a respin if nobody raises objections.

> On Thu, Mar 22, 2018 at 2:28 PM Matthias Kaehlcke <mka@chromium.org> wrote:
> > +#ifndef __clang__
> > +#define __reg__ "r"
> > +#else
> > +#define __reg__ "x"
> > +#endif
> 
> Can this be flipped to #ifdef __clang__ ?  having an if...else where the
> conditional negated is kind of funny.

Sure, my thought was "common case first", but I agree that the negated
condition isn't ideal either.

[PATCH] arm/arm64: smccc: Use xN for arm64 register constraints with clang

[Greg Hackmann]

On 03/22/2018 03:44 PM, Matthias Kaehlcke wrote:
> El Thu, Mar 22, 2018 at 10:26:18PM +0000 Nick Desaulniers ha dit:
> 
>> Note that a patch in this form has previously been implemented by:
>>
>> Andrey Konovalov <andreyknvl@google.com>:
>> https://gist.github.com/xairy/ee11682ea86044a45c0291c528cd936f
>>
>> and another by:
>>
>> Greg Hackmann <ghackmann@google.com>:
>> https://android-review.googlesource.com/c/kernel/common/+/645181
>>
>> If you used either as a reference, you may want to credit them with a
>> `Suggested-by:` in the commit message.
> 
> Not really, but I think I prefer Greg's version over mine and might
> use it in a respin if nobody raises objections.

NAK.  There's a reason I didn't send my change upstream.

As Marc pointed out (https://lkml.org/lkml/2018/3/16/987), the "r"
prefix tells gcc to pick the appropriate register width.  "x" makes it
unconditionally use the entire 64-bit register width.  Just swapping out
one for the other changes the macro's semantics.

Unfortunately since this was breaking builds in android-4.14 and we
didn't have an immediate-term fix, I bit the bullet and added the above
commit -- but *only* as a short-term workaround.  For the one caller we
currently have in 4.14.y, gcc was using the entire 64-bit width for all
its inputs anyway, so "r" vs. "x" didn't make a difference.  But that
might not be true if/when someone introduces other SMCCC 1.1 callers.

Unfortunately I don't see a better way to deal with this than waiting
for clang to support "r"-style constraints on ARM64.

[PATCH] arm/arm64: smccc: Use xN for arm64 register constraints with clang

[Matthias Kaehlcke]

El Thu, Mar 22, 2018 at 04:19:42PM -0700 Greg Hackmann ha dit:

> On 03/22/2018 03:44 PM, Matthias Kaehlcke wrote:
> > El Thu, Mar 22, 2018 at 10:26:18PM +0000 Nick Desaulniers ha dit:
> > 
> >> Note that a patch in this form has previously been implemented by:
> >>
> >> Andrey Konovalov <andreyknvl@google.com>:
> >> https://gist.github.com/xairy/ee11682ea86044a45c0291c528cd936f
> >>
> >> and another by:
> >>
> >> Greg Hackmann <ghackmann@google.com>:
> >> https://android-review.googlesource.com/c/kernel/common/+/645181
> >>
> >> If you used either as a reference, you may want to credit them with a
> >> `Suggested-by:` in the commit message.
> > 
> > Not really, but I think I prefer Greg's version over mine and might
> > use it in a respin if nobody raises objections.
> 
> NAK.  There's a reason I didn't send my change upstream.
> 
> As Marc pointed out (https://lkml.org/lkml/2018/3/16/987), the "r"
> prefix tells gcc to pick the appropriate register width.  "x" makes it
> unconditionally use the entire 64-bit register width.  Just swapping out
> one for the other changes the macro's semantics.
> 
> Unfortunately since this was breaking builds in android-4.14 and we
> didn't have an immediate-term fix, I bit the bullet and added the above
> commit -- but *only* as a short-term workaround.  For the one caller we
> currently have in 4.14.y, gcc was using the entire 64-bit width for all
> its inputs anyway, so "r" vs. "x" didn't make a difference.  But that
> might not be true if/when someone introduces other SMCCC 1.1 callers.
> 
> Unfortunately I don't see a better way to deal with this than waiting
> for clang to support "r"-style constraints on ARM64.

Thanks for the clarification! From the other thread
(https://lkml.org/lkml/2018/3/1/268) I had the impression that ARM
folks saw the option of a mergeable fix.

Given the fact that clang support for kernel builds is still
recent/WIP I guess it's not the end of the world if we have to raise
the minimum clang version to 7.x for newer kernels.

[PATCH] arm/arm64: smccc: Use xN for arm64 register constraints with clang

[Nick Desaulniers]

On Thu, Mar 22, 2018 at 4:58 PM Matthias Kaehlcke <mka@chromium.org> wrote:

> El Thu, Mar 22, 2018 at 04:19:42PM -0700 Greg Hackmann ha dit:
> > NAK.  There's a reason I didn't send my change upstream.
> >
> > As Marc pointed out (https://lkml.org/lkml/2018/3/16/987), the "r"
> > prefix tells gcc to pick the appropriate register width.  "x" makes it
> > unconditionally use the entire 64-bit register width.  Just swapping out
> > one for the other changes the macro's semantics.
> >
> > Unfortunately since this was breaking builds in android-4.14 and we
> > didn't have an immediate-term fix, I bit the bullet and added the above
> > commit -- but *only* as a short-term workaround.  For the one caller we
> > currently have in 4.14.y, gcc was using the entire 64-bit width for all
> > its inputs anyway, so "r" vs. "x" didn't make a difference.  But that
> > might not be true if/when someone introduces other SMCCC 1.1 callers.
> >
> > Unfortunately I don't see a better way to deal with this than waiting
> > for clang to support "r"-style constraints on ARM64.

> Thanks for the clarification! From the other thread
> (https://lkml.org/lkml/2018/3/1/268) I had the impression that ARM
> folks saw the option of a mergeable fix.

> Given the fact that clang support for kernel builds is still
> recent/WIP I guess it's not the end of the world if we have to raise
> the minimum clang version to 7.x for newer kernels.


Manoj fixed this in:
https://reviews.llvm.org/rL328829
https://bugs.llvm.org/show_bug.cgi?id=36862

Looks set to ride the Clang 6.0 train.  mka@ if you're planning another
state of the union email, it would be good to note the clang 6.0
requirement for arm64.

Is there anything left to do here?
-- 
Thanks,
~Nick Desaulniers

[PATCH] arm/arm64: smccc: Use xN for arm64 register constraints with clang

[Matthias Kaehlcke]

On Thu, Apr 05, 2018 at 06:43:05PM +0000, Nick Desaulniers wrote:
> On Thu, Mar 22, 2018 at 4:58 PM Matthias Kaehlcke <mka@chromium.org> wrote:
> 
> > El Thu, Mar 22, 2018 at 04:19:42PM -0700 Greg Hackmann ha dit:
> > > NAK.  There's a reason I didn't send my change upstream.
> > >
> > > As Marc pointed out (https://lkml.org/lkml/2018/3/16/987), the "r"
> > > prefix tells gcc to pick the appropriate register width.  "x" makes it
> > > unconditionally use the entire 64-bit register width.  Just swapping out
> > > one for the other changes the macro's semantics.
> > >
> > > Unfortunately since this was breaking builds in android-4.14 and we
> > > didn't have an immediate-term fix, I bit the bullet and added the above
> > > commit -- but *only* as a short-term workaround.  For the one caller we
> > > currently have in 4.14.y, gcc was using the entire 64-bit width for all
> > > its inputs anyway, so "r" vs. "x" didn't make a difference.  But that
> > > might not be true if/when someone introduces other SMCCC 1.1 callers.
> > >
> > > Unfortunately I don't see a better way to deal with this than waiting
> > > for clang to support "r"-style constraints on ARM64.
> 
> > Thanks for the clarification! From the other thread
> > (https://lkml.org/lkml/2018/3/1/268) I had the impression that ARM
> > folks saw the option of a mergeable fix.
> 
> > Given the fact that clang support for kernel builds is still
> > recent/WIP I guess it's not the end of the world if we have to raise
> > the minimum clang version to 7.x for newer kernels.
> 
> 
> Manoj fixed this in:
> https://reviews.llvm.org/rL328829
> https://bugs.llvm.org/show_bug.cgi?id=36862
> 
> Looks set to ride the Clang 6.0 train.  mka@ if you're planning another
> state of the union email, it would be good to note the clang 6.0
> requirement for arm64.
> 
> Is there anything left to do here?

We should be good, unless somebody wants to look into a patch that
fixes clang pre-6.0.1 builds and doesn't look too ugly.