arm64: write_sysreg asm illegal for aarch32

PATCH In review Mark Salyzyn 4 participants 8 messages Nov 1, 2017 – Nov 2, 2017 arm64

Share & tools

Mark Salyzyn

author

2017-11-01 16:58 UTC #0 lore raw

PATCH 1/1 arm64: write_sysreg asm illegal for aarch32

Cross compiling to aarch32 (for vdso32) using clang correctly identifies that (the unused) write_sysreg inline asm directive is illegal in that architectural context:

arch/arm64/include/asm/arch_timer.h: error: invalid input constraint 'rZ' in asm

        write_sysreg(cntkctl, cntkctl_el1);
        ^
arch/arm64/include/asm/sysreg.h: note: expanded from macro 'write_sysreg'
                     : : "rZ" (__val));
                         ^

GCC normally checks for correctness everywhere. But uniquely for unused asm, will optimize out and suppress the error report.

Signed-off-by Mark Salyzyn <[email protected]>

Cc Catalin Marinas <[email protected]>

Cc Will Deacon <[email protected]>

Cc Christoffer Dall <[email protected]>

Cc Mark Rutland <[email protected]>

Cc Marc Zyngier <[email protected]>

Cc Suzuki K Poulose <[email protected]>

Cc Stefan Traby <[email protected]>

Cc Dave Martin <[email protected]>

Cc linux-arm-kernel at lists.infradead.org

Cc linux-kernel at vger.kernel.org

---
 arch/arm64/include/asm/sysreg.h | 4 ++++
 1 file changed, 4 insertions(+)

arch/arm64/include/asm/sysreg.h +4 -0

index f707fed5886f..a7b61c9327db 100644
@@ -492,11 +492,15 @@ asm(
  * The "Z" constraint normally means a zero immediate, but when combined with
  * the "%x0" template means XZR.
  */
+#if defined(__aarch64__)
 #define write_sysreg(v, r) do {					\
 	u64 __val = (u64)(v);					\
 	asm volatile("msr " __stringify(r) ", %x0"		\
 		     : : "rZ" (__val));				\
 } while (0)
+#else
+#define write_sysreg(v, r) BUG()
+#endif
 
 /*
  * For registers without architectural names, or simply unsupported by

Signature · 1 line

2.15.0.403.gc27cc4dac6-goog

Robin Murphy

2017-11-01 17:14 UTC #1 lore raw

Quoted 5 lines · Mark Salyzyn

Cross compiling to aarch32 (for vdso32) using clang correctly identifies that (the unused) write_sysreg inline asm directive is illegal in that architectural context:
arch/arm64/include/asm/arch_timer.h: error: invalid input constraint 'rZ' in asm

Quoted 8 lines

        write_sysreg(cntkctl, cntkctl_el1);
        ^
arch/arm64/include/asm/sysreg.h: note: expanded from macro 'write_sysreg'
                     : : "rZ" (__val));
                         ^
GCC normally checks for correctness everywhere. But uniquely for unused asm, will optimize out and suppress the error report.

It sounds more like some paths are wrong in the compat vDSO build if it's pulling in this header in the first place - nothing in this file is relevant to AArch32.

Robin.

Quoted 36 lines

Signed-off-by Mark Salyzyn <[email protected]>
Cc Catalin Marinas <[email protected]>
Cc Will Deacon <[email protected]>
Cc Christoffer Dall <[email protected]>
Cc Mark Rutland <[email protected]>
Cc Marc Zyngier <[email protected]>
Cc Suzuki K Poulose <[email protected]>
Cc Stefan Traby <[email protected]>
Cc Dave Martin <[email protected]>
Cc linux-arm-kernel at lists.infradead.org
Cc linux-kernel at vger.kernel.org
---
 arch/arm64/include/asm/sysreg.h | 4 ++++
 1 file changed, 4 insertions(+)
arch/arm64/include/asm/sysreg.h +4 -0
index f707fed5886f..a7b61c9327db 100644
@@ -492,11 +492,15 @@ asm(
  * The "Z" constraint normally means a zero immediate, but when combined with
  * the "%x0" template means XZR.
  */
+#if defined(__aarch64__)
 #define write_sysreg(v, r) do {					\
 	u64 __val = (u64)(v);					\
 	asm volatile("msr " __stringify(r) ", %x0"		\
 		     : : "rZ" (__val));				\
 } while (0)
+#else
+#define write_sysreg(v, r) BUG()
+#endif
 
 /*
  * For registers without architectural names, or simply unsupported by

Mark Salyzyn

author

2017-11-01 17:49 UTC #2 lore raw

Quoted 12 lines · Robin Murphy

Quoted 11 lines · Mark Salyzyn
Cross compiling to aarch32 (for vdso32) using clang correctly identifies that (the unused) write_sysreg inline asm directive is illegal in that architectural context:
arch/arm64/include/asm/arch_timer.h: error: invalid input constraint 'rZ' in asm
         write_sysreg(cntkctl, cntkctl_el1);
         ^
arch/arm64/include/asm/sysreg.h: note: expanded from macro 'write_sysreg'
                      : : "rZ" (__val));
                          ^

Quoted 8 lines

Quoted 2 lines
GCC normally checks for correctness everywhere. But uniquely for unused asm, will optimize out and suppress the error report.
It sounds more like some paths are wrong in the compat vDSO build if it's pulling in this header in the first place - nothing in this file is relevant to AArch32.
Robin.

And yet, when you CROSS_COMPILE_ARM32 a vdso32, you have no choice but to utilize the arm64 headers since they contain all the relevant kernel structures and environment.

asm/arch_timer.h (remember we are using arm instructions to access arch64 timers)

linux/time.h (really only for struct timespec())

asm/processor.h (eg: cpu_relax())

pull in a _lot_ of architectural related cruft that always somehow picks up asm/sysreg.h somewhere in the multitude of includes to fulfill some unused inline's needs.

-- Mark

Mark Rutland

reviewer

2017-11-01 17:56 UTC #3 lore raw

Quoted 19 lines · Mark Salyzyn

Quoted 18 lines · Robin Murphy
Quoted 5 lines · Mark Salyzyn
Cross compiling to aarch32 (for vdso32) using clang correctly identifies that (the unused) write_sysreg inline asm directive is illegal in that architectural context:
arch/arm64/include/asm/arch_timer.h: error: invalid input constraint 'rZ' in asm
Quoted 8 lines
         write_sysreg(cntkctl, cntkctl_el1);
         ^
arch/arm64/include/asm/sysreg.h: note: expanded from macro 'write_sysreg'
                      : : "rZ" (__val));
                          ^
GCC normally checks for correctness everywhere. But uniquely for unused asm, will optimize out and suppress the error report.
It sounds more like some paths are wrong in the compat vDSO build if it's pulling in this header in the first place - nothing in this file is relevant to AArch32.

Quoted 5 lines

Quoted 2 lines
Robin.
And yet, when you CROSS_COMPILE_ARM32 a vdso32, you have no choice but to utilize the arm64 headers since they contain all the relevant kernel structures and environment.

This itself is the underlying issue.

When building the compat VDSO, we must ensure that we only include headers that make sense for 32-bit arm.

If the build system can't do that today, we should rework it so that it can. Anything else cannot be a complete fix.

Quoted 10 lines

asm/arch_timer.h (remember we are using arm instructions to access arch64 timers)
linux/time.h (really only for struct timespec())
asm/processor.h (eg: cpu_relax())
pull in a _lot_ of architectural related cruft that always somehow picks up asm/sysreg.h somewhere in the multitude of includes to fulfill some unused inline's needs.

... these are just the particular symptoms this problem results in today.

Thanks,
Mark.

Mark Salyzyn

author

2017-11-01 18:16 UTC #4 lore raw

Quoted 40 lines · Mark Rutland

Quoted 19 lines · Mark Salyzyn
Quoted 18 lines · Robin Murphy
Quoted 5 lines · Mark Salyzyn
Cross compiling to aarch32 (for vdso32) using clang correctly identifies that (the unused) write_sysreg inline asm directive is illegal in that architectural context:
arch/arm64/include/asm/arch_timer.h: error: invalid input constraint 'rZ' in asm
Quoted 8 lines
          write_sysreg(cntkctl, cntkctl_el1);
          ^
arch/arm64/include/asm/sysreg.h: note: expanded from macro 'write_sysreg'
                       : : "rZ" (__val));
                           ^
GCC normally checks for correctness everywhere. But uniquely for unused asm, will optimize out and suppress the error report.
It sounds more like some paths are wrong in the compat vDSO build if it's pulling in this header in the first place - nothing in this file is relevant to AArch32.
Quoted 5 lines
Quoted 2 lines
Robin.
And yet, when you CROSS_COMPILE_ARM32 a vdso32, you have no choice but to utilize the arm64 headers since they contain all the relevant kernel structures and environment.
This itself is the underlying issue.
When building the compat VDSO, we must ensure that we only include headers that make sense for 32-bit arm.
If the build system can't do that today, we should rework it so that it can. Anything else cannot be a complete fix.
Quoted 7 lines
asm/arch_timer.h (remember we are using arm instructions to access arch64 timers)
linux/time.h (really only for struct timespec())
asm/processor.h (eg: cpu_relax())

Quoted 8 lines

Quoted 3 lines
pull in a _lot_ of architectural related cruft that always somehow picks up asm/sysreg.h somewhere in the multitude of includes to fulfill some unused inline's needs.
... these are just the particular symptoms this problem results in today.
Thanks,
Mark.

Ok, will attack it and see just how bad the scale is...

These ones trouble me ... there was no intent to pick up the wrong pieces.

In file included from ./include/linux/time.h:4:
In file included from ./include/linux/cache.h:5:
In file included from ./arch/arm64/include/asm/cache.h:19:
In file included from ./arch/arm64/include/asm/cachetype.h:19:
In file included from ./arch/arm64/include/asm/cputype.h:94:
./arch/arm64/include/asm/sysreg.h:303:2: error: invalid input constraint 'rZ' in asm

In file included from ./include/linux/hrtimer.h:18:
In file included from ./include/linux/rbtree.h:34:
In file included from ./include/linux/rcupdate.h:47:
In file included from ./include/linux/ktime.h:25:
In file included from ./include/linux/jiffies.h:8:
In file included from ./include/linux/timex.h:65:
In file included from ./arch/arm64/include/asm/timex.h:19:
./arch/arm64/include/asm/arch_timer.h:83:4: error: invalid input constraint 'rZ' in asm

??????? write_sysreg(cntkctl, cntkctl_el1);
??????? ^

./arch/arm64/include/asm/sysreg.h:278:12: note: expanded from macro 'write_sysreg'
???????????????????? : : "rZ" (__val));

-- Mark

Mark Salyzyn

author

2017-11-01 20:32 UTC #5 lore raw

Quoted 54 lines · Mark Salyzyn

Quoted 53 lines · Mark Rutland
Quoted 22 lines · Mark Salyzyn
Quoted 21 lines · Robin Murphy
Quoted 13 lines · Mark Salyzyn
Cross compiling to aarch32 (for vdso32) using clang correctly identifies that (the unused) write_sysreg inline asm directive is illegal in that architectural context:
arch/arm64/include/asm/arch_timer.h: error: invalid input constraint 'rZ' in asm
????????? write_sysreg(cntkctl, cntkctl_el1); ????????? ^
arch/arm64/include/asm/sysreg.h: note: expanded from macro 'write_sysreg'
?????????????????????? : : "rZ" (__val)); ?????????????????????????? ^
Quoted 2 lines
GCC normally checks for correctness everywhere. But uniquely for unused asm, will optimize out and suppress the error report.
It sounds more like some paths are wrong in the compat vDSO build if it's pulling in this header in the first place - nothing in this file is
relevant to AArch32.
Quoted 6 lines
Quoted 2 lines
Robin.
And yet, when you CROSS_COMPILE_ARM32 a vdso32, you have no choice but to
utilize the arm64 headers since they contain all the relevant kernel structures and environment.
This itself is the underlying issue.
When building the compat VDSO, we must ensure that we only include headers that make sense for 32-bit arm.
If the build system can't do that today, we should rework it so that it can. Anything else cannot be a complete fix.
Quoted 6 lines
asm/arch_timer.h (remember we are using arm instructions to access arch64
timers)
linux/time.h (really only for struct timespec())
Quoted 7 lines
asm/processor.h (eg: cpu_relax())
pull in a _lot_ of architectural related cruft that always somehow picks up
asm/sysreg.h somewhere in the multitude of includes to fulfill some unused
inline's needs.
... these are just the particular symptoms this problem results in today.

Quoted 10 lines

Quoted 2 lines
Thanks,
Mark.
Ok, will attack it and see just how bad the scale is...
. . .
-- Mark

Scoped, not as bad as I thought, but there is some open-coded evilness to fix:

1) linux/jiffies.h can not be included, replace with open coding:

#include <asm/param.h>

#define TICK_NSEC TICK_NSEC ((NSEC_PER_SEC+HZ/2)/HZ)

2) linux/hrtimer.h can not be included, replace with open coding (must have above to work):

#define LOW_RES_NSEC??? ??? TICK_NSEC
#ifdef CONFIG_HIGH_RES_TIMERS

# define HIGH_RES_NSEC??? ??? 1
# define MONOTONIC_RES_NSEC??? HIGH_RES_NSEC
#else
# define MONOTONIC_RES_NSEC??? LOW_RES_NSEC
#endif

3) asm/processor.h can not be included, replace with open coding:

static inline void cpu_relax(void)
{
??? asm volatile("yield" ::: "memory");
}

4) linux/time.h can not be included, replace with open coding:

#include <linux/compiler.h>

#include <linux/math64.h>

#include <uapi/linux/time.h>

#define NSEC_PER_SEC 1000000000L

static __always_inline void timespec_add_ns(struct timespec *a, u64 ns) {
??? a->tv_sec += __iter_div_u64_rem(a->tv_nsec + ns, NSEC_PER_SEC, &ns); ??? a->tv_nsec = ns;
}

I am at a loss to determine if there is an acceptable way to split off the open-coding. For instance asm-generic/processor.h (for cpu_relax()), uapi/linux/hrtimer.h and uapi/linux/jiffies.h for the #defines (uapi is a bad choice, flipping coin?). The time open-coding is probably OK given that they have not changed since near the limits of git history.

-- Mark

Mark Rutland

reviewer

2017-11-02 10:08 UTC #6 lore raw

Quoted 45 lines · Mark Salyzyn

Quoted 32 lines · Mark Salyzyn
Quoted 31 lines · Mark Rutland
Quoted 22 lines · Mark Salyzyn
Quoted 21 lines · Robin Murphy
Quoted 13 lines · Mark Salyzyn
Cross compiling to aarch32 (for vdso32) using clang correctly identifies that (the unused) write_sysreg inline asm directive is illegal in that architectural context:
arch/arm64/include/asm/arch_timer.h: error: invalid input constraint 'rZ' in asm
????????? write_sysreg(cntkctl, cntkctl_el1); ????????? ^
arch/arm64/include/asm/sysreg.h: note: expanded from macro 'write_sysreg'
?????????????????????? : : "rZ" (__val)); ?????????????????????????? ^
Quoted 2 lines
GCC normally checks for correctness everywhere. But uniquely for unused asm, will optimize out and suppress the error report.
It sounds more like some paths are wrong in the compat vDSO build if it's pulling in this header in the first place - nothing in this file is
relevant to AArch32.
Quoted 6 lines
Quoted 2 lines
Robin.
And yet, when you CROSS_COMPILE_ARM32 a vdso32, you have no choice but to
utilize the arm64 headers since they contain all the relevant kernel structures and environment.
This itself is the underlying issue.
Quoted 7 lines
Quoted 5 lines
When building the compat VDSO, we must ensure that we only include headers that make sense for 32-bit arm.
If the build system can't do that today, we should rework it so that it can. Anything else cannot be a complete fix.
Ok, will attack it and see just how bad the scale is...
Scoped, not as bad as I thought, but there is some open-coded evilness to fix:
1) linux/jiffies.h can not be included, replace with open coding:

[...]

Quoted 2 lines

2) linux/hrtimer.h can not be included, replace with open coding (must have above to work):

[...]

Quoted 1 line

3) asm/processor.h can not be included, replace with open coding:

[...]

Quoted 1 line

4) linux/time.h can not be included, replace with open coding:

[...]

Evidently I was not sufficiently clear.

I don't think that we should try to bodge the #include directives to try to avoid including 64-bit headers. The above changes might work today, but they're *extremely* fragile.

When I said that we should rework the build system above, I mean we should rework the Makefile logic to pass the 32-bit include paths into the compat vdso. That way, we can include the usual set of headers, as we'll pick up the 32-bit headers.

We might have to have separate native/compat vdso data pages to make sure that the types align, but that's not the end of the world...

Thanks,
Mark.

Marc Zyngier

2017-11-02 10:05 UTC #7 lore raw

In reply to Mark Salyzyn

Quoted 5 lines · Mark Salyzyn

Cross compiling to aarch32 (for vdso32) using clang correctly identifies that (the unused) write_sysreg inline asm directive is illegal in that architectural context:
arch/arm64/include/asm/arch_timer.h: error: invalid input constraint 'rZ' in asm

Quoted 8 lines

        write_sysreg(cntkctl, cntkctl_el1);
        ^
arch/arm64/include/asm/sysreg.h: note: expanded from macro 'write_sysreg'
                     : : "rZ" (__val));
                         ^
GCC normally checks for correctness everywhere. But uniquely for unused asm, will optimize out and suppress the error report.

-ENOTABUG.

Please fix the build system instead (why on Earth are you using arm64 headers for an Aarch32 binary?), and don't introduce stuff that is making understanding the kernel more difficult to understand and maintain.

Thanks,

M.

Signature · 1 line

Jazz is not dead. It just smells funny...