| Message ID | 20200608205711.109418-1-ndesaulniers@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] arm64: vdso32: add CONFIG_THUMB2_COMPAT_VDSO | expand |
On Mon, Jun 08, 2020 at 01:57:08PM -0700, Nick Desaulniers wrote: > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig > index 7f9d38444d6d..fe9e6b231cac 100644 > --- a/arch/arm64/Kconfig > +++ b/arch/arm64/Kconfig > @@ -1299,6 +1299,14 @@ config COMPAT_VDSO > You must have a 32-bit build of glibc 2.22 or later for programs > to seamlessly take advantage of this. > > +config THUMB2_COMPAT_VDSO > + bool "Compile the vDSO in THUMB2 mode" > + depends on COMPAT_VDSO > + default y > + help > + Compile the compat vDSO with -mthumb -fomit-frame-pointer if y, otherwise > + as -marm. Now that we understood the issue (I think), do we actually need this choice? Why not going for -mthumb -fomit-frame-pointer always for the compat vdso?
On Tue, Jun 9, 2020 at 1:35 PM Catalin Marinas <catalin.marinas@arm.com> wrote: > > On Mon, Jun 08, 2020 at 01:57:08PM -0700, Nick Desaulniers wrote: > > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig > > index 7f9d38444d6d..fe9e6b231cac 100644 > > --- a/arch/arm64/Kconfig > > +++ b/arch/arm64/Kconfig > > @@ -1299,6 +1299,14 @@ config COMPAT_VDSO > > You must have a 32-bit build of glibc 2.22 or later for programs > > to seamlessly take advantage of this. > > > > +config THUMB2_COMPAT_VDSO > > + bool "Compile the vDSO in THUMB2 mode" > > + depends on COMPAT_VDSO > > + default y > > + help > > + Compile the compat vDSO with -mthumb -fomit-frame-pointer if y, otherwise > > + as -marm. > > Now that we understood the issue (I think), do we actually need this > choice? Why not going for -mthumb -fomit-frame-pointer always for the > compat vdso? "Why should the compat vdso be configurable?" is a fair question. I don't have an answer, but maybe some of the folks on thread do? Our problem is more so "if the vdso is built as thumb, we need it also explicitly built with -fomit-frame-pointer." Whether it should be built as thumb, arm, or configurable (and which default to pick in that case) are still an open questions. Will asked for it to be configurable, so I sent a patch making it configurable. I'm not sure what the tradeoffs would be for a A32 vs T32 compat vdso image. Is it possible to have hardware that's A64+A32 but not T32, or A64+T32 but not A32? (I suspect not). I'm not sure whether userspace cares about frame pointer based unwinding through the vdso, but if it's built as THUMB, then that likely doesn't work for binaries with A32/T32 interworking. Whether the functions in the vdso are faster when built as A32 or T32 I cannot say.
On Tue, Jun 09, 2020 at 04:55:13PM -0700, Nick Desaulniers wrote: > On Tue, Jun 9, 2020 at 1:35 PM Catalin Marinas <catalin.marinas@arm.com> wrote: > > > > On Mon, Jun 08, 2020 at 01:57:08PM -0700, Nick Desaulniers wrote: > > > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig > > > index 7f9d38444d6d..fe9e6b231cac 100644 > > > --- a/arch/arm64/Kconfig > > > +++ b/arch/arm64/Kconfig > > > @@ -1299,6 +1299,14 @@ config COMPAT_VDSO > > > You must have a 32-bit build of glibc 2.22 or later for programs > > > to seamlessly take advantage of this. > > > > > > +config THUMB2_COMPAT_VDSO > > > + bool "Compile the vDSO in THUMB2 mode" > > > + depends on COMPAT_VDSO > > > + default y > > > + help > > > + Compile the compat vDSO with -mthumb -fomit-frame-pointer if y, otherwise > > > + as -marm. > > > > Now that we understood the issue (I think), do we actually need this > > choice? Why not going for -mthumb -fomit-frame-pointer always for the > > compat vdso? > > "Why should the compat vdso be configurable?" is a fair question. I > don't have an answer, but maybe some of the folks on thread do? > > Our problem is more so "if the vdso is built as thumb, we need it also > explicitly built with -fomit-frame-pointer." Whether it should be > built as thumb, arm, or configurable (and which default to pick in > that case) are still an open questions. Will asked for it to be > configurable, so I sent a patch making it configurable. It's configurable for 32-bit arm, so I was just following that as it's hardly a maintenance burden to support both. I suppose you could have a toolchain that only supports one or the other, but it does seem a little esoteric if you're building a kernel for an arm64 CPU. Will
On Wed, Jun 10, 2020 at 09:47:55AM +0100, Will Deacon wrote: > On Tue, Jun 09, 2020 at 04:55:13PM -0700, Nick Desaulniers wrote: > > On Tue, Jun 9, 2020 at 1:35 PM Catalin Marinas <catalin.marinas@arm.com> wrote: > > > On Mon, Jun 08, 2020 at 01:57:08PM -0700, Nick Desaulniers wrote: > > > > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig > > > > index 7f9d38444d6d..fe9e6b231cac 100644 > > > > --- a/arch/arm64/Kconfig > > > > +++ b/arch/arm64/Kconfig > > > > @@ -1299,6 +1299,14 @@ config COMPAT_VDSO > > > > You must have a 32-bit build of glibc 2.22 or later for programs > > > > to seamlessly take advantage of this. > > > > > > > > +config THUMB2_COMPAT_VDSO > > > > + bool "Compile the vDSO in THUMB2 mode" > > > > + depends on COMPAT_VDSO > > > > + default y > > > > + help > > > > + Compile the compat vDSO with -mthumb -fomit-frame-pointer if y, otherwise > > > > + as -marm. > > > > > > Now that we understood the issue (I think), do we actually need this > > > choice? Why not going for -mthumb -fomit-frame-pointer always for the > > > compat vdso? > > > > "Why should the compat vdso be configurable?" is a fair question. I > > don't have an answer, but maybe some of the folks on thread do? > > > > Our problem is more so "if the vdso is built as thumb, we need it also > > explicitly built with -fomit-frame-pointer." Whether it should be > > built as thumb, arm, or configurable (and which default to pick in > > that case) are still an open questions. Will asked for it to be > > configurable, so I sent a patch making it configurable. > > It's configurable for 32-bit arm, On 32-bit, the vdso mode is a side-effect of how we build the kernel image. I guess we haven't put much thought into whether we want to keep the vdso in Thumb-2 or ARM mode. > so I was just following that as it's > hardly a maintenance burden to support both. I suppose you could have > a toolchain that only supports one or the other, but it does seem a little > esoteric if you're building a kernel for an arm64 CPU. We could leave the config option in if we ever need to change the compat vdso mode. But as not to confuse others with too many options, maybe add: bool "Compile the vDSO in THUMB2 mode" if EXPERT Either way: Acked-by: Catalin Marinas <catalin.marinas@arm.com>
On Wed, Jun 10, 2020 at 11:29:17AM +0100, Catalin Marinas wrote: > On Wed, Jun 10, 2020 at 09:47:55AM +0100, Will Deacon wrote: > > On Tue, Jun 09, 2020 at 04:55:13PM -0700, Nick Desaulniers wrote: > > > On Tue, Jun 9, 2020 at 1:35 PM Catalin Marinas <catalin.marinas@arm.com> wrote: > > > > On Mon, Jun 08, 2020 at 01:57:08PM -0700, Nick Desaulniers wrote: > > > > > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig > > > > > index 7f9d38444d6d..fe9e6b231cac 100644 > > > > > --- a/arch/arm64/Kconfig > > > > > +++ b/arch/arm64/Kconfig > > > > > @@ -1299,6 +1299,14 @@ config COMPAT_VDSO > > > > > You must have a 32-bit build of glibc 2.22 or later for programs > > > > > to seamlessly take advantage of this. > > > > > > > > > > +config THUMB2_COMPAT_VDSO > > > > > + bool "Compile the vDSO in THUMB2 mode" > > > > > + depends on COMPAT_VDSO > > > > > + default y > > > > > + help > > > > > + Compile the compat vDSO with -mthumb -fomit-frame-pointer if y, otherwise > > > > > + as -marm. > > > > > > > > Now that we understood the issue (I think), do we actually need this > > > > choice? Why not going for -mthumb -fomit-frame-pointer always for the > > > > compat vdso? > > > > > > "Why should the compat vdso be configurable?" is a fair question. I > > > don't have an answer, but maybe some of the folks on thread do? > > > > > > Our problem is more so "if the vdso is built as thumb, we need it also > > > explicitly built with -fomit-frame-pointer." Whether it should be > > > built as thumb, arm, or configurable (and which default to pick in > > > that case) are still an open questions. Will asked for it to be > > > configurable, so I sent a patch making it configurable. > > > > It's configurable for 32-bit arm, > > On 32-bit, the vdso mode is a side-effect of how we build the kernel > image. I guess we haven't put much thought into whether we want to keep > the vdso in Thumb-2 or ARM mode. I think your guess is correct! > > so I was just following that as it's > > hardly a maintenance burden to support both. I suppose you could have > > a toolchain that only supports one or the other, but it does seem a little > > esoteric if you're building a kernel for an arm64 CPU. > > We could leave the config option in if we ever need to change the compat > vdso mode. But as not to confuse others with too many options, maybe > add: > > bool "Compile the vDSO in THUMB2 mode" if EXPERT > > Either way: > > Acked-by: Catalin Marinas <catalin.marinas@arm.com> Cheers, I'll add the dependency on EXPERT since I agree it's probably not something most people would want to touch. I'll also change the text to say "compat vDSO" not just vDSO. Will
On Mon, 8 Jun 2020 13:57:08 -0700, Nick Desaulniers wrote: > Allow the compat vdso (32b) to be compiled as either THUMB2 (default) or > ARM. > > For THUMB2, the register r7 is reserved for the frame pointer, but > code in arch/arm64/include/asm/vdso/compat_gettimeofday.h > uses r7. Explicitly set -fomit-frame-pointer, since unwinding through > interworked THUMB2 and ARM is unreliable anyways. See also how > CONFIG_UNWINDER_FRAME_POINTER cannot be selected for > CONFIG_THUMB2_KERNEL for ARCH=arm. > > [...] Applied to arm64 (for-next/core), thanks! [1/1] arm64: vdso32: add CONFIG_THUMB2_COMPAT_VDSO https://git.kernel.org/arm64/c/625412c210fb Cheers,
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig index 7f9d38444d6d..fe9e6b231cac 100644 --- a/arch/arm64/Kconfig +++ b/arch/arm64/Kconfig @@ -1299,6 +1299,14 @@ config COMPAT_VDSO You must have a 32-bit build of glibc 2.22 or later for programs to seamlessly take advantage of this. +config THUMB2_COMPAT_VDSO + bool "Compile the vDSO in THUMB2 mode" + depends on COMPAT_VDSO + default y + help + Compile the compat vDSO with -mthumb -fomit-frame-pointer if y, otherwise + as -marm. + menuconfig ARMV8_DEPRECATED bool "Emulate deprecated/obsolete ARMv8 instructions" depends on SYSCTL diff --git a/arch/arm64/kernel/vdso32/Makefile b/arch/arm64/kernel/vdso32/Makefile index 3964738ebbde..7ea1e827e505 100644 --- a/arch/arm64/kernel/vdso32/Makefile +++ b/arch/arm64/kernel/vdso32/Makefile @@ -105,6 +105,14 @@ VDSO_CFLAGS += -D__uint128_t='void*' VDSO_CFLAGS += $(call cc32-disable-warning,shift-count-overflow) VDSO_CFLAGS += -Wno-int-to-pointer-cast +# Compile as THUMB2 or ARM. Unwinding via frame-pointers in THUMB2 is +# unreliable. +ifeq ($(CONFIG_THUMB2_COMPAT_VDSO), y) +VDSO_CFLAGS += -mthumb -fomit-frame-pointer +else +VDSO_CFLAGS += -marm +endif + VDSO_AFLAGS := $(VDSO_CAFLAGS) VDSO_AFLAGS += -D__ASSEMBLY__
Allow the compat vdso (32b) to be compiled as either THUMB2 (default) or ARM. For THUMB2, the register r7 is reserved for the frame pointer, but code in arch/arm64/include/asm/vdso/compat_gettimeofday.h uses r7. Explicitly set -fomit-frame-pointer, since unwinding through interworked THUMB2 and ARM is unreliable anyways. See also how CONFIG_UNWINDER_FRAME_POINTER cannot be selected for CONFIG_THUMB2_KERNEL for ARCH=arm. This also helps toolchains that differ in their implicit value if the choice of -f{no-}omit-frame-pointer is left unspecified, to not error on the use of r7. 2019 Q4 ARM AAPCS seeks to standardize the use of r11 as the reserved frame pointer register, but no production compiler that can compile the Linux kernel currently implements this. We're actively discussing such a transition with ARM toolchain developers currently. Link: https://static.docs.arm.com/ihi0042/i/aapcs32.pdf Link: https://bugs.chromium.org/p/chromium/issues/detail?id=1084372 Cc: Stephen Boyd <swboyd@google.com> Cc: Robin Murphy <robin.murphy@arm.com> Cc: Dave Martin <Dave.Martin@arm.com> Reported-by: Luis Lozano <llozano@google.com> Tested-by: Manoj Gupta <manojgupta@google.com> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> --- Changes V1 -> V2: * add THUMB2_COMPAT_VDSO config, making -mthumb/-marm configurable rather than hard coding. * Fixed https://reviews.llvm.org/D80828 in Clang, but still an issue. Not due to implicit state of -marm vs -mthumb, but actually -f{no-}omit-frame-pointer due to https://source.chromium.org/chromiumos/chromiumos/codesearch/+/master:src/third_party/toolchain-utils/compiler_wrapper/config.go;l=110, which prefixes -fno-omit-frame-pointer for all arches and projects. Projects that don't set -f{no-}omit-frame-pointer thus don't overwrite the prefixed -fno-omit-frame-pointer, which is an issue when inline asm compiled as -mthumb uses r7. * I don't have a strong preference on the default state of this config. arch/arm64/Kconfig | 8 ++++++++ arch/arm64/kernel/vdso32/Makefile | 8 ++++++++ 2 files changed, 16 insertions(+)