Message ID | 20180322212705.141175-1-mka@chromium.org (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
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
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.
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.
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.
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?
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.
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__)
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(-)