* [PATCH 0/2] Support kexec/kdump for clang built kernel
@ 2019-07-18 0:02 Vaibhav Rustagi
2019-07-18 0:02 ` [PATCH 1/2] x86/purgatory: add -mno-sse, -mno-mmx, -mno-sse2 to Makefile Vaibhav Rustagi
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Vaibhav Rustagi @ 2019-07-18 0:02 UTC (permalink / raw)
To: Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin
Cc: x86, linux-kernel, Vivek Goyal, Vaibhav Rustagi,
Nick Desaulniers, stable
This patch series includes the following:
1. Adding compiler options to not use XMM registers in the purgatory code.
2. Reuse the implementation of memcpy and memset instead of relying on
__builtin_memcpy and __builtin_memset as it causes infinite recursion
in clang.
Nick Desaulniers (1):
x86/purgatory: do not use __builtin_memcpy and __builtin_memset.
Vaibhav Rustagi (1):
x86/purgatory: add -mno-sse, -mno-mmx, -mno-sse2 to Makefile
arch/x86/purgatory/Makefile | 4 ++++
arch/x86/purgatory/purgatory.c | 6 ++++++
arch/x86/purgatory/string.c | 23 -----------------------
3 files changed, 10 insertions(+), 23 deletions(-)
delete mode 100644 arch/x86/purgatory/string.c
--
2.22.0.510.g264f2c817a-goog
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH 1/2] x86/purgatory: add -mno-sse, -mno-mmx, -mno-sse2 to Makefile 2019-07-18 0:02 [PATCH 0/2] Support kexec/kdump for clang built kernel Vaibhav Rustagi @ 2019-07-18 0:02 ` Vaibhav Rustagi 2019-07-18 0:47 ` Greg KH 2019-07-18 21:34 ` Nick Desaulniers 2019-07-18 0:02 ` [PATCH 2/2] x86/purgatory: do not use __builtin_memcpy and __builtin_memset Vaibhav Rustagi 2019-07-18 21:29 ` [PATCH 0/2] Support kexec/kdump for clang built kernel Nick Desaulniers 2 siblings, 2 replies; 10+ messages in thread From: Vaibhav Rustagi @ 2019-07-18 0:02 UTC (permalink / raw) To: Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin Cc: x86, linux-kernel, Vivek Goyal, Vaibhav Rustagi, Nick Desaulniers, stable Compiling the purgatory code with clang results in using of mmx registers. $ objdump -d arch/x86/purgatory/purgatory.ro | grep xmm 112: 0f 28 00 movaps (%rax),%xmm0 115: 0f 11 07 movups %xmm0,(%rdi) 122: 0f 28 00 movaps (%rax),%xmm0 125: 0f 11 47 10 movups %xmm0,0x10(%rdi) Add -mno-sse, -mno-mmx, -mno-sse2 to avoid generating SSE instructions. Signed-off-by: Vaibhav Rustagi <vaibhavrustagi@google.com> --- arch/x86/purgatory/Makefile | 1 + 1 file changed, 1 insertion(+) diff --git a/arch/x86/purgatory/Makefile b/arch/x86/purgatory/Makefile index 3cf302b26332..3589ec4a28c7 100644 --- a/arch/x86/purgatory/Makefile +++ b/arch/x86/purgatory/Makefile @@ -20,6 +20,7 @@ KCOV_INSTRUMENT := n # sure how to relocate those. Like kexec-tools, use custom flags. KBUILD_CFLAGS := -fno-strict-aliasing -Wall -Wstrict-prototypes -fno-zero-initialized-in-bss -fno-builtin -ffreestanding -c -Os -mcmodel=large +KBUILD_CFLAGS += -mno-mmx -mno-sse -mno-sse2 KBUILD_CFLAGS += -m$(BITS) KBUILD_CFLAGS += $(call cc-option,-fno-PIE) -- 2.22.0.510.g264f2c817a-goog ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] x86/purgatory: add -mno-sse, -mno-mmx, -mno-sse2 to Makefile 2019-07-18 0:02 ` [PATCH 1/2] x86/purgatory: add -mno-sse, -mno-mmx, -mno-sse2 to Makefile Vaibhav Rustagi @ 2019-07-18 0:47 ` Greg KH 2019-07-18 21:34 ` Nick Desaulniers 1 sibling, 0 replies; 10+ messages in thread From: Greg KH @ 2019-07-18 0:47 UTC (permalink / raw) To: Vaibhav Rustagi Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, x86, linux-kernel, Vivek Goyal, Nick Desaulniers, stable On Wed, Jul 17, 2019 at 05:02:05PM -0700, Vaibhav Rustagi wrote: > Compiling the purgatory code with clang results in using of mmx > registers. > > $ objdump -d arch/x86/purgatory/purgatory.ro | grep xmm > > 112: 0f 28 00 movaps (%rax),%xmm0 > 115: 0f 11 07 movups %xmm0,(%rdi) > 122: 0f 28 00 movaps (%rax),%xmm0 > 125: 0f 11 47 10 movups %xmm0,0x10(%rdi) > > Add -mno-sse, -mno-mmx, -mno-sse2 to avoid generating SSE instructions. > > Signed-off-by: Vaibhav Rustagi <vaibhavrustagi@google.com> > --- > arch/x86/purgatory/Makefile | 1 + > 1 file changed, 1 insertion(+) <formletter> This is not the correct way to submit patches for inclusion in the stable kernel tree. Please read: https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html for how to do this properly. </formletter> ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] x86/purgatory: add -mno-sse, -mno-mmx, -mno-sse2 to Makefile 2019-07-18 0:02 ` [PATCH 1/2] x86/purgatory: add -mno-sse, -mno-mmx, -mno-sse2 to Makefile Vaibhav Rustagi 2019-07-18 0:47 ` Greg KH @ 2019-07-18 21:34 ` Nick Desaulniers 2019-07-19 8:17 ` Peter Zijlstra 1 sibling, 1 reply; 10+ messages in thread From: Nick Desaulniers @ 2019-07-18 21:34 UTC (permalink / raw) To: Vaibhav Rustagi Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT), LKML, Vivek Goyal, # 3.4.x On Wed, Jul 17, 2019 at 5:02 PM Vaibhav Rustagi <vaibhavrustagi@google.com> wrote: > > Compiling the purgatory code with clang results in using of mmx > registers. > > $ objdump -d arch/x86/purgatory/purgatory.ro | grep xmm > > 112: 0f 28 00 movaps (%rax),%xmm0 > 115: 0f 11 07 movups %xmm0,(%rdi) > 122: 0f 28 00 movaps (%rax),%xmm0 > 125: 0f 11 47 10 movups %xmm0,0x10(%rdi) > > Add -mno-sse, -mno-mmx, -mno-sse2 to avoid generating SSE instructions. > > Signed-off-by: Vaibhav Rustagi <vaibhavrustagi@google.com> > --- > arch/x86/purgatory/Makefile | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/arch/x86/purgatory/Makefile b/arch/x86/purgatory/Makefile > index 3cf302b26332..3589ec4a28c7 100644 > --- a/arch/x86/purgatory/Makefile > +++ b/arch/x86/purgatory/Makefile > @@ -20,6 +20,7 @@ KCOV_INSTRUMENT := n > # sure how to relocate those. Like kexec-tools, use custom flags. > > KBUILD_CFLAGS := -fno-strict-aliasing -Wall -Wstrict-prototypes -fno-zero-initialized-in-bss -fno-builtin -ffreestanding -c -Os -mcmodel=large > +KBUILD_CFLAGS += -mno-mmx -mno-sse -mno-sse2 Yep, this is a commonly recurring bug in the kernel, observed again and again for Clang builds. The top level Makefile carefully sets KBUILD_CFLAGS, then lower subdirs in the kernel wipe them away with `:=` assignment. Invariably important flags don't always get re-added. In this case, these flags are used in arch/x86/Makefile, but not here and should be IMO. Thanks for the patch. Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> (Note that arch/x86/Makefile additionally sets -mno-3dnow and -mno-avx (if supported by the compiler). Not sure if the maintainers would like a v2 with those added, and we don't strictly need them yet, but we may someday). -- Thanks, ~Nick Desaulniers ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] x86/purgatory: add -mno-sse, -mno-mmx, -mno-sse2 to Makefile 2019-07-18 21:34 ` Nick Desaulniers @ 2019-07-19 8:17 ` Peter Zijlstra 2019-07-22 21:12 ` Nick Desaulniers 0 siblings, 1 reply; 10+ messages in thread From: Peter Zijlstra @ 2019-07-19 8:17 UTC (permalink / raw) To: Nick Desaulniers Cc: Vaibhav Rustagi, Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT), LKML, Vivek Goyal, # 3.4.x On Thu, Jul 18, 2019 at 02:34:44PM -0700, Nick Desaulniers wrote: > On Wed, Jul 17, 2019 at 5:02 PM Vaibhav Rustagi > <vaibhavrustagi@google.com> wrote: > > > > Compiling the purgatory code with clang results in using of mmx > > registers. > > > > $ objdump -d arch/x86/purgatory/purgatory.ro | grep xmm > > > > 112: 0f 28 00 movaps (%rax),%xmm0 > > 115: 0f 11 07 movups %xmm0,(%rdi) > > 122: 0f 28 00 movaps (%rax),%xmm0 > > 125: 0f 11 47 10 movups %xmm0,0x10(%rdi) > > > > Add -mno-sse, -mno-mmx, -mno-sse2 to avoid generating SSE instructions. > > > > Signed-off-by: Vaibhav Rustagi <vaibhavrustagi@google.com> > > --- > > arch/x86/purgatory/Makefile | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/arch/x86/purgatory/Makefile b/arch/x86/purgatory/Makefile > > index 3cf302b26332..3589ec4a28c7 100644 > > --- a/arch/x86/purgatory/Makefile > > +++ b/arch/x86/purgatory/Makefile > > @@ -20,6 +20,7 @@ KCOV_INSTRUMENT := n > > # sure how to relocate those. Like kexec-tools, use custom flags. > > > > KBUILD_CFLAGS := -fno-strict-aliasing -Wall -Wstrict-prototypes -fno-zero-initialized-in-bss -fno-builtin -ffreestanding -c -Os -mcmodel=large > > +KBUILD_CFLAGS += -mno-mmx -mno-sse -mno-sse2 > > Yep, this is a commonly recurring bug in the kernel, observed again > and again for Clang builds. The top level Makefile carefully sets > KBUILD_CFLAGS, then lower subdirs in the kernel wipe them away with > `:=` assignment. Invariably important flags don't always get re-added. > In this case, these flags are used in arch/x86/Makefile, but not here > and should be IMO. Thanks for the patch. Should we then not fix/remove these := assignments? ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] x86/purgatory: add -mno-sse, -mno-mmx, -mno-sse2 to Makefile 2019-07-19 8:17 ` Peter Zijlstra @ 2019-07-22 21:12 ` Nick Desaulniers 0 siblings, 0 replies; 10+ messages in thread From: Nick Desaulniers @ 2019-07-22 21:12 UTC (permalink / raw) To: Peter Zijlstra Cc: Vaibhav Rustagi, Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT), LKML, Vivek Goyal, # 3.4.x On Fri, Jul 19, 2019 at 1:17 AM Peter Zijlstra <peterz@infradead.org> wrote: > > On Thu, Jul 18, 2019 at 02:34:44PM -0700, Nick Desaulniers wrote: > > On Wed, Jul 17, 2019 at 5:02 PM Vaibhav Rustagi > > <vaibhavrustagi@google.com> wrote: > > > > > > Compiling the purgatory code with clang results in using of mmx > > > registers. > > > > > > $ objdump -d arch/x86/purgatory/purgatory.ro | grep xmm > > > > > > 112: 0f 28 00 movaps (%rax),%xmm0 > > > 115: 0f 11 07 movups %xmm0,(%rdi) > > > 122: 0f 28 00 movaps (%rax),%xmm0 > > > 125: 0f 11 47 10 movups %xmm0,0x10(%rdi) > > > > > > Add -mno-sse, -mno-mmx, -mno-sse2 to avoid generating SSE instructions. > > > > > > Signed-off-by: Vaibhav Rustagi <vaibhavrustagi@google.com> > > > --- > > > arch/x86/purgatory/Makefile | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/arch/x86/purgatory/Makefile b/arch/x86/purgatory/Makefile > > > index 3cf302b26332..3589ec4a28c7 100644 > > > --- a/arch/x86/purgatory/Makefile > > > +++ b/arch/x86/purgatory/Makefile > > > @@ -20,6 +20,7 @@ KCOV_INSTRUMENT := n > > > # sure how to relocate those. Like kexec-tools, use custom flags. > > > > > > KBUILD_CFLAGS := -fno-strict-aliasing -Wall -Wstrict-prototypes -fno-zero-initialized-in-bss -fno-builtin -ffreestanding -c -Os -mcmodel=large > > > +KBUILD_CFLAGS += -mno-mmx -mno-sse -mno-sse2 > > > > Yep, this is a commonly recurring bug in the kernel, observed again > > and again for Clang builds. The top level Makefile carefully sets > > KBUILD_CFLAGS, then lower subdirs in the kernel wipe them away with > > `:=` assignment. Invariably important flags don't always get re-added. > > In this case, these flags are used in arch/x86/Makefile, but not here > > and should be IMO. Thanks for the patch. > > Should we then not fix/remove these := assignments? Good point, it's actually pretty straightforward to do so. It just will invert the order of patches in the series, as then the memcpy/memset infinite recursion is now guaranteed with CONFIG_CC_OPTIMIZE_FOR_PERFORMANCE=y (without the other patch in this series). Did the x86 maintainers have thoughts on their favorite implementation of memset/memcpy for me to use from the thread from the other patch in the series? I'll just resend with this fix and maybe we can discuss there and spin a v3 if needed. -- Thanks, ~Nick Desaulniers ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 2/2] x86/purgatory: do not use __builtin_memcpy and __builtin_memset. 2019-07-18 0:02 [PATCH 0/2] Support kexec/kdump for clang built kernel Vaibhav Rustagi 2019-07-18 0:02 ` [PATCH 1/2] x86/purgatory: add -mno-sse, -mno-mmx, -mno-sse2 to Makefile Vaibhav Rustagi @ 2019-07-18 0:02 ` Vaibhav Rustagi 2019-07-18 0:47 ` Greg KH 2019-07-18 21:56 ` Nick Desaulniers 2019-07-18 21:29 ` [PATCH 0/2] Support kexec/kdump for clang built kernel Nick Desaulniers 2 siblings, 2 replies; 10+ messages in thread From: Vaibhav Rustagi @ 2019-07-18 0:02 UTC (permalink / raw) To: Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin Cc: x86, linux-kernel, Vivek Goyal, Vaibhav Rustagi, Nick Desaulniers, stable, Manoj Gupta, Alistair Delva From: Nick Desaulniers <ndesaulniers@google.com> Implementing memcpy and memset in terms of __builtin_memcpy and __builtin_memset is problematic. GCC at -O2 will replace calls to the builtins with calls to memcpy and memset (but will generate an inline implementation at -Os). Clang will replace the builtins with these calls regardless of optimization level. $ llvm-objdump -dr arch/x86/purgatory/string.o | tail 0000000000000339 memcpy: 339: 48 b8 00 00 00 00 00 00 00 00 movabsq $0, %rax 000000000000033b: R_X86_64_64 memcpy 343: ff e0 jmpq *%rax 0000000000000345 memset: 345: 48 b8 00 00 00 00 00 00 00 00 movabsq $0, %rax 0000000000000347: R_X86_64_64 memset 34f: ff e0 Such code results in infinite recursion at runtime. This is observed when doing kexec. Instead, reuse an implementation from arch/x86/boot/compressed/string.c if we define warn as a symbol. Link: https://bugs.chromium.org/p/chromium/issues/detail?id=984056 Reported-by: Vaibhav Rustagi <vaibhavrustagi@google.com> Tested-by: Vaibhav Rustagi <vaibhavrustagi@google.com> Debugged-by: Vaibhav Rustagi <vaibhavrustagi@google.com> Debugged-by: Manoj Gupta <manojgupta@google.com> Suggested-by: Alistair Delva <adelva@google.com> Signed-off-by: Vaibhav Rustagi <vaibhavrustagi@google.com> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> --- arch/x86/purgatory/Makefile | 3 +++ arch/x86/purgatory/purgatory.c | 6 ++++++ arch/x86/purgatory/string.c | 23 ----------------------- 3 files changed, 9 insertions(+), 23 deletions(-) delete mode 100644 arch/x86/purgatory/string.c diff --git a/arch/x86/purgatory/Makefile b/arch/x86/purgatory/Makefile index 3589ec4a28c7..84b8314ddb2d 100644 --- a/arch/x86/purgatory/Makefile +++ b/arch/x86/purgatory/Makefile @@ -6,6 +6,9 @@ purgatory-y := purgatory.o stack.o setup-x86_$(BITS).o sha256.o entry64.o string targets += $(purgatory-y) PURGATORY_OBJS = $(addprefix $(obj)/,$(purgatory-y)) +$(obj)/string.o: $(srctree)/arch/x86/boot/compressed/string.c FORCE + $(call if_changed_rule,cc_o_c) + $(obj)/sha256.o: $(srctree)/lib/sha256.c FORCE $(call if_changed_rule,cc_o_c) diff --git a/arch/x86/purgatory/purgatory.c b/arch/x86/purgatory/purgatory.c index 6d8d5a34c377..b607bda786f6 100644 --- a/arch/x86/purgatory/purgatory.c +++ b/arch/x86/purgatory/purgatory.c @@ -68,3 +68,9 @@ void purgatory(void) } copy_backup_region(); } + +/* + * Defined in order to reuse memcpy() and memset() from + * arch/x86/boot/compressed/string.c + */ +void warn(const char *msg) {} diff --git a/arch/x86/purgatory/string.c b/arch/x86/purgatory/string.c deleted file mode 100644 index 01ad43873ad9..000000000000 --- a/arch/x86/purgatory/string.c +++ /dev/null @@ -1,23 +0,0 @@ -// SPDX-License-Identifier: GPL-2.0-only -/* - * Simple string functions. - * - * Copyright (C) 2014 Red Hat Inc. - * - * Author: - * Vivek Goyal <vgoyal@redhat.com> - */ - -#include <linux/types.h> - -#include "../boot/string.c" - -void *memcpy(void *dst, const void *src, size_t len) -{ - return __builtin_memcpy(dst, src, len); -} - -void *memset(void *dst, int c, size_t len) -{ - return __builtin_memset(dst, c, len); -} -- 2.22.0.510.g264f2c817a-goog ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] x86/purgatory: do not use __builtin_memcpy and __builtin_memset. 2019-07-18 0:02 ` [PATCH 2/2] x86/purgatory: do not use __builtin_memcpy and __builtin_memset Vaibhav Rustagi @ 2019-07-18 0:47 ` Greg KH 2019-07-18 21:56 ` Nick Desaulniers 1 sibling, 0 replies; 10+ messages in thread From: Greg KH @ 2019-07-18 0:47 UTC (permalink / raw) To: Vaibhav Rustagi Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, x86, linux-kernel, Vivek Goyal, Nick Desaulniers, stable, Manoj Gupta, Alistair Delva On Wed, Jul 17, 2019 at 05:02:06PM -0700, Vaibhav Rustagi wrote: > From: Nick Desaulniers <ndesaulniers@google.com> > > Implementing memcpy and memset in terms of __builtin_memcpy and > __builtin_memset is problematic. > > GCC at -O2 will replace calls to the builtins with calls to memcpy and > memset (but will generate an inline implementation at -Os). Clang will > replace the builtins with these calls regardless of optimization level. > > $ llvm-objdump -dr arch/x86/purgatory/string.o | tail > > 0000000000000339 memcpy: > 339: 48 b8 00 00 00 00 00 00 00 00 movabsq $0, %rax > 000000000000033b: R_X86_64_64 memcpy > 343: ff e0 jmpq *%rax > > 0000000000000345 memset: > 345: 48 b8 00 00 00 00 00 00 00 00 movabsq $0, %rax > 0000000000000347: R_X86_64_64 memset > 34f: ff e0 > > Such code results in infinite recursion at runtime. This is observed > when doing kexec. > > Instead, reuse an implementation from arch/x86/boot/compressed/string.c > if we define warn as a symbol. > > Link: https://bugs.chromium.org/p/chromium/issues/detail?id=984056 > Reported-by: Vaibhav Rustagi <vaibhavrustagi@google.com> > Tested-by: Vaibhav Rustagi <vaibhavrustagi@google.com> > Debugged-by: Vaibhav Rustagi <vaibhavrustagi@google.com> > Debugged-by: Manoj Gupta <manojgupta@google.com> > Suggested-by: Alistair Delva <adelva@google.com> > Signed-off-by: Vaibhav Rustagi <vaibhavrustagi@google.com> > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> > --- > arch/x86/purgatory/Makefile | 3 +++ > arch/x86/purgatory/purgatory.c | 6 ++++++ > arch/x86/purgatory/string.c | 23 ----------------------- > 3 files changed, 9 insertions(+), 23 deletions(-) > delete mode 100644 arch/x86/purgatory/string.c <formletter> This is not the correct way to submit patches for inclusion in the stable kernel tree. Please read: https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html for how to do this properly. </formletter> ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] x86/purgatory: do not use __builtin_memcpy and __builtin_memset. 2019-07-18 0:02 ` [PATCH 2/2] x86/purgatory: do not use __builtin_memcpy and __builtin_memset Vaibhav Rustagi 2019-07-18 0:47 ` Greg KH @ 2019-07-18 21:56 ` Nick Desaulniers 1 sibling, 0 replies; 10+ messages in thread From: Nick Desaulniers @ 2019-07-18 21:56 UTC (permalink / raw) To: Thomas Gleixner, Borislav Petkov, Ingo Molnar, H. Peter Anvin, Kees Cook, Vivek Goyal Cc: maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT), LKML, Vaibhav Rustagi, # 3.4.x, Manoj Gupta, Alistair Delva, Hans Boehm, Alexander Potapenko On Wed, Jul 17, 2019 at 5:02 PM Vaibhav Rustagi <vaibhavrustagi@google.com> wrote: > > From: Nick Desaulniers <ndesaulniers@google.com> > > Implementing memcpy and memset in terms of __builtin_memcpy and > __builtin_memset is problematic. > > GCC at -O2 will replace calls to the builtins with calls to memcpy and > memset (but will generate an inline implementation at -Os). Clang will > replace the builtins with these calls regardless of optimization level. > > $ llvm-objdump -dr arch/x86/purgatory/string.o | tail > > 0000000000000339 memcpy: > 339: 48 b8 00 00 00 00 00 00 00 00 movabsq $0, %rax > 000000000000033b: R_X86_64_64 memcpy > 343: ff e0 jmpq *%rax > > 0000000000000345 memset: > 345: 48 b8 00 00 00 00 00 00 00 00 movabsq $0, %rax > 0000000000000347: R_X86_64_64 memset > 34f: ff e0 > > Such code results in infinite recursion at runtime. This is observed > when doing kexec. Just so it's crystal clear to other reviewers, consider this codegen between compilers and optimization levels: https://godbolt.org/z/jcfKsw So I'd imagine the commit that introduced these implementations very much relied on being compiled at -Os to work. > > Instead, reuse an implementation from arch/x86/boot/compressed/string.c > if we define warn as a symbol. Alternatively, I was getting fancy trying to match what GCC lowers __builtin_memcpy/__builtin_memset to: diff --git a/arch/x86/purgatory/string.c b/arch/x86/purgatory/string.c index 795ca4f..e055f65 100644 --- a/arch/x86/purgatory/string.c +++ b/arch/x86/purgatory/string.c @@ -16,10 +16,23 @@ void *memcpy(void *dst, const void *src, size_t len) { - return __builtin_memcpy(dst, src, len); + asm( + "movq %0, %%rax\n\t" + "movq %2, %%rcx\n\t" + "rep movsb\n\t" + : "=r"(dst) : "r"(src), "ri"(len) : "rax", "rcx"); + return dst; } void *memset(void *dst, int c, size_t len) { - return __builtin_memset(dst, c, len); + void* ret; + asm( + "movq %1, %%r8\n\t" + "movl %2, %%eax\n\t" + "movq %3, %%rcx\n\t" + "rep stosb\n\t" + "movq %%r8, %0" + : "=r"(ret) : "r"(dst), "ri"(c), "ri"(len) : "r8", "eax", "rcx"); + return ret; } but then Alistair pointed out that we have a proliferation of memcpy+memest definitions in the kernel, and we should probably just reuse an existing one rather than add to the arms race. > > Link: https://bugs.chromium.org/p/chromium/issues/detail?id=984056 > Reported-by: Vaibhav Rustagi <vaibhavrustagi@google.com> > Tested-by: Vaibhav Rustagi <vaibhavrustagi@google.com> > Debugged-by: Vaibhav Rustagi <vaibhavrustagi@google.com> > Debugged-by: Manoj Gupta <manojgupta@google.com> > Suggested-by: Alistair Delva <adelva@google.com> > Signed-off-by: Vaibhav Rustagi <vaibhavrustagi@google.com> > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> > --- > arch/x86/purgatory/Makefile | 3 +++ > arch/x86/purgatory/purgatory.c | 6 ++++++ > arch/x86/purgatory/string.c | 23 ----------------------- > 3 files changed, 9 insertions(+), 23 deletions(-) > delete mode 100644 arch/x86/purgatory/string.c > > diff --git a/arch/x86/purgatory/Makefile b/arch/x86/purgatory/Makefile > index 3589ec4a28c7..84b8314ddb2d 100644 > --- a/arch/x86/purgatory/Makefile > +++ b/arch/x86/purgatory/Makefile > @@ -6,6 +6,9 @@ purgatory-y := purgatory.o stack.o setup-x86_$(BITS).o sha256.o entry64.o string > targets += $(purgatory-y) > PURGATORY_OBJS = $(addprefix $(obj)/,$(purgatory-y)) > > +$(obj)/string.o: $(srctree)/arch/x86/boot/compressed/string.c FORCE > + $(call if_changed_rule,cc_o_c) > + > $(obj)/sha256.o: $(srctree)/lib/sha256.c FORCE > $(call if_changed_rule,cc_o_c) > > diff --git a/arch/x86/purgatory/purgatory.c b/arch/x86/purgatory/purgatory.c > index 6d8d5a34c377..b607bda786f6 100644 > --- a/arch/x86/purgatory/purgatory.c > +++ b/arch/x86/purgatory/purgatory.c > @@ -68,3 +68,9 @@ void purgatory(void) > } > copy_backup_region(); > } > + > +/* > + * Defined in order to reuse memcpy() and memset() from > + * arch/x86/boot/compressed/string.c > + */ > +void warn(const char *msg) {} This is the one part I feel bad about; memcpy() in arch/x86/boot/compressed/string.c calls warn() which would result in an undefined symbol in purgatory.ro. Maybe there's a preferred solution, or this is ok for purgatory/kexec? There's other x86 memsets+memcpys, but IMO this is the smallest incision without playing the satisfy-the-symbol-dependencies game. If the maintainers are ok with this, then the series looks ready to go to me. Thanks for debugging/sending Vaibhav. Orthogonally, I showed Hans Boehm the pointer comparisons+subtraction in arch/x86/boot/compressed/string.c's memcpy asking about pointer provenance issues (https://wiki.sei.cmu.edu/confluence/display/c/ARR36-C.+Do+not+subtract+or+compare+two+pointers+that+do+not+refer+to+the+same+array, http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2090.htm) introduced in commit 00ec2c37031e ("x86/boot: Warn on future overlapping memcpy() use") and he started cursing in Spanish (I don't think he speaks Spanish) and performed the sign of the cross. Y'all need <strikethrough>Jesus</strikethrough>[u]intptr_t. > diff --git a/arch/x86/purgatory/string.c b/arch/x86/purgatory/string.c > deleted file mode 100644 > index 01ad43873ad9..000000000000 > --- a/arch/x86/purgatory/string.c > +++ /dev/null > @@ -1,23 +0,0 @@ > -// SPDX-License-Identifier: GPL-2.0-only > -/* > - * Simple string functions. > - * > - * Copyright (C) 2014 Red Hat Inc. > - * > - * Author: > - * Vivek Goyal <vgoyal@redhat.com> > - */ > - > -#include <linux/types.h> > - > -#include "../boot/string.c" > - > -void *memcpy(void *dst, const void *src, size_t len) > -{ > - return __builtin_memcpy(dst, src, len); > -} > - > -void *memset(void *dst, int c, size_t len) > -{ > - return __builtin_memset(dst, c, len); > -} > -- > 2.22.0.510.g264f2c817a-goog > -- Thanks, ~Nick Desaulniers ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 0/2] Support kexec/kdump for clang built kernel 2019-07-18 0:02 [PATCH 0/2] Support kexec/kdump for clang built kernel Vaibhav Rustagi 2019-07-18 0:02 ` [PATCH 1/2] x86/purgatory: add -mno-sse, -mno-mmx, -mno-sse2 to Makefile Vaibhav Rustagi 2019-07-18 0:02 ` [PATCH 2/2] x86/purgatory: do not use __builtin_memcpy and __builtin_memset Vaibhav Rustagi @ 2019-07-18 21:29 ` Nick Desaulniers 2 siblings, 0 replies; 10+ messages in thread From: Nick Desaulniers @ 2019-07-18 21:29 UTC (permalink / raw) To: Vaibhav Rustagi Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT), LKML, Vivek Goyal, # 3.4.x On Wed, Jul 17, 2019 at 5:02 PM Vaibhav Rustagi <vaibhavrustagi@google.com> wrote: > > This patch series includes the following: > > 1. Adding compiler options to not use XMM registers in the purgatory code. > 2. Reuse the implementation of memcpy and memset instead of relying on > __builtin_memcpy and __builtin_memset as it causes infinite recursion > in clang. Thanks for the series, and debugging and finding the issue. These would explain why I couldn't get kexec to work with Clang built kernels. Comments/reviews inbound on the individual patches. > > Nick Desaulniers (1): > x86/purgatory: do not use __builtin_memcpy and __builtin_memset. > > Vaibhav Rustagi (1): > x86/purgatory: add -mno-sse, -mno-mmx, -mno-sse2 to Makefile > > arch/x86/purgatory/Makefile | 4 ++++ > arch/x86/purgatory/purgatory.c | 6 ++++++ > arch/x86/purgatory/string.c | 23 ----------------------- > 3 files changed, 10 insertions(+), 23 deletions(-) > delete mode 100644 arch/x86/purgatory/string.c > > -- > 2.22.0.510.g264f2c817a-goog > -- Thanks, ~Nick Desaulniers ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-07-22 21:12 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-07-18 0:02 [PATCH 0/2] Support kexec/kdump for clang built kernel Vaibhav Rustagi 2019-07-18 0:02 ` [PATCH 1/2] x86/purgatory: add -mno-sse, -mno-mmx, -mno-sse2 to Makefile Vaibhav Rustagi 2019-07-18 0:47 ` Greg KH 2019-07-18 21:34 ` Nick Desaulniers 2019-07-19 8:17 ` Peter Zijlstra 2019-07-22 21:12 ` Nick Desaulniers 2019-07-18 0:02 ` [PATCH 2/2] x86/purgatory: do not use __builtin_memcpy and __builtin_memset Vaibhav Rustagi 2019-07-18 0:47 ` Greg KH 2019-07-18 21:56 ` Nick Desaulniers 2019-07-18 21:29 ` [PATCH 0/2] Support kexec/kdump for clang built kernel Nick Desaulniers
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).