All of lore.kernel.org
 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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.