linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [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

* [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 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 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 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

* 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 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 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

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).