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
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 <[email protected]>
---
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
From: Nick Desaulniers <[email protected]>
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 <[email protected]>
Tested-by: Vaibhav Rustagi <[email protected]>
Debugged-by: Vaibhav Rustagi <[email protected]>
Debugged-by: Manoj Gupta <[email protected]>
Suggested-by: Alistair Delva <[email protected]>
Signed-off-by: Vaibhav Rustagi <[email protected]>
Signed-off-by: Nick Desaulniers <[email protected]>
---
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 <[email protected]>
- */
-
-#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
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 <[email protected]>
> ---
> 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>
On Wed, Jul 17, 2019 at 05:02:06PM -0700, Vaibhav Rustagi wrote:
> From: Nick Desaulniers <[email protected]>
>
> 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 <[email protected]>
> Tested-by: Vaibhav Rustagi <[email protected]>
> Debugged-by: Vaibhav Rustagi <[email protected]>
> Debugged-by: Manoj Gupta <[email protected]>
> Suggested-by: Alistair Delva <[email protected]>
> Signed-off-by: Vaibhav Rustagi <[email protected]>
> Signed-off-by: Nick Desaulniers <[email protected]>
> ---
> 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>
On Wed, Jul 17, 2019 at 5:02 PM Vaibhav Rustagi
<[email protected]> 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
On Wed, Jul 17, 2019 at 5:02 PM Vaibhav Rustagi
<[email protected]> 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 <[email protected]>
> ---
> 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 <[email protected]>
(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
On Wed, Jul 17, 2019 at 5:02 PM Vaibhav Rustagi
<[email protected]> wrote:
>
> From: Nick Desaulniers <[email protected]>
>
> 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 <[email protected]>
> Tested-by: Vaibhav Rustagi <[email protected]>
> Debugged-by: Vaibhav Rustagi <[email protected]>
> Debugged-by: Manoj Gupta <[email protected]>
> Suggested-by: Alistair Delva <[email protected]>
> Signed-off-by: Vaibhav Rustagi <[email protected]>
> Signed-off-by: Nick Desaulniers <[email protected]>
> ---
> 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 <[email protected]>
> - */
> -
> -#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
On Thu, Jul 18, 2019 at 02:34:44PM -0700, Nick Desaulniers wrote:
> On Wed, Jul 17, 2019 at 5:02 PM Vaibhav Rustagi
> <[email protected]> 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 <[email protected]>
> > ---
> > 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?
On Fri, Jul 19, 2019 at 1:17 AM Peter Zijlstra <[email protected]> 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
> > <[email protected]> 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 <[email protected]>
> > > ---
> > > 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