Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1541546 > unrolled thread

Re: [RFC, PATCHv1 15/28] x86: detect 5-level paging support

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [RFC, PATCHv1 15/28] x86: detect 5-level paging support "H. Peter Anvin" <hpa@zytor.com> - 2016-12-13 23:50 +0100
    Re: [RFC, PATCHv1 15/28] x86: detect 5-level paging support Boris Petkov <bp@alien8.de> - 2016-12-14 00:10 +0100
      Re: [RFC, PATCHv1 15/28] x86: detect 5-level paging support Borislav Petkov <bp@alien8.de> - 2016-12-15 15:40 +0100
        Re: [RFC, PATCHv1 15/28] x86: detect 5-level paging support hpa@zytor.com - 2016-12-15 19:00 +0100
          Re: [RFC, PATCHv1 15/28] x86: detect 5-level paging support Borislav Petkov <bp@alien8.de> - 2016-12-15 20:10 +0100
            Re: [RFC, PATCHv1 15/28] x86: detect 5-level paging support hpa@zytor.com - 2016-12-15 22:00 +0100
            Re: [RFC, PATCHv1 15/28] x86: detect 5-level paging support hpa@zytor.com - 2016-12-15 22:20 +0100
            Re: [RFC, PATCHv1 15/28] x86: detect 5-level paging support Andi Kleen <ak@linux.intel.com> - 2016-12-15 23:20 +0100

#1541546 — Re: [RFC, PATCHv1 15/28] x86: detect 5-level paging support

On 12/08/16 12:20, Borislav Petkov wrote:
> On Thu, Dec 08, 2016 at 12:08:53PM -0800, Linus Torvalds wrote:
>> Especially since that's some of the ugliest inline asm ever due to the
>> nasty BX handling.
> 
> Yeah, about that: why doesn't gcc handle that for us like it would
> handle a clobbered register? I mean, it *should* know that BX is live
> when building with -fPIC... The .ifnc thing looks really silly.
> 

When compiling with -fPIC gcc treats ebx as a "fixed register".  A fixed
register can't be spilled, and so a clobber of a fixed register is a
fatal error.

Like it or not, it's how it works.

	-hpa

[toc] | [next] | [standalone]

#1541553

On December 13, 2016 11:44:06 PM GMT+01:00, "H. Peter Anvin" <hpa@zytor.com> wrote:
>When compiling with -fPIC gcc treats ebx as a "fixed register".  A
>fixed
>register can't be spilled, and so a clobber of a fixed register is a
>fatal error.
>
>Like it or not, it's how it works.
>
>	-hpa

In the meantime I talked to my gcc guy and here's the deal:

There are gcc versions (4.x and earlier) which do not save/restore the PIC register around an inline asm even if it is one of the registers that the inline asm clobbers. Therefore the saving/restoring needs to be done by the inline asm itself.

5.x and later handle that fine.

Thus I was thinking of adding a build-time check for the gcc version but that might turn out to be more code in the end than those ugly ifnc clauses. 
-- 
Sent from a small device: formatting sux and brevity is inevitable. 

[toc] | [prev] | [next] | [standalone]

#1542774

On Wed, Dec 14, 2016 at 12:07:54AM +0100, Boris Petkov wrote:
> Thus I was thinking of adding a build-time check for the gcc version
> but that might turn out to be more code in the end than those ugly
> ifnc clauses.

IOW, something like this. I did this just to try to see whether it is
doable. And it does work - gcc 4.8 and 4.9 -m32 cannot preserve the PIC
register - actually the inline asm fails building due to impossible
constraints.

However, so many lines changed just to save the ifnc, meh, I dunno...

---
 arch/x86/boot/compressed/Makefile |  8 ++++++
 arch/x86/boot/cpuflags.c          | 14 ++++++++--
 scripts/gcc-clobber-pic.sh        | 58 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 77 insertions(+), 3 deletions(-)
 create mode 100755 scripts/gcc-clobber-pic.sh

diff --git a/arch/x86/boot/compressed/Makefile b/arch/x86/boot/compressed/Makefile
index 34d9e15857c3..705fc2ab3fd6 100644
--- a/arch/x86/boot/compressed/Makefile
+++ b/arch/x86/boot/compressed/Makefile
@@ -35,6 +35,14 @@ KBUILD_CFLAGS += -mno-mmx -mno-sse
 KBUILD_CFLAGS += $(call cc-option,-ffreestanding)
 KBUILD_CFLAGS += $(call cc-option,-fno-stack-protector)
 
+# check whether inline asm clobbers the PIC register
+ifeq ($(CONFIG_X86_32),y)
+ifeq ($(shell $(CONFIG_SHELL) $(srctree)/scripts/gcc-clobber-pic.sh $(CC) -m32),n)
+	KBUILD_CFLAGS += -DCC_PRESERVES_PIC
+	KBUILD_AFLAGS += -DCC_PRESERVES_PIC
+endif
+endif
+
 KBUILD_AFLAGS  := $(KBUILD_CFLAGS) -D__ASSEMBLY__
 GCOV_PROFILE := n
 UBSAN_SANITIZE :=n
diff --git a/arch/x86/boot/cpuflags.c b/arch/x86/boot/cpuflags.c
index 6687ab953257..913c3f5ab3a0 100644
--- a/arch/x86/boot/cpuflags.c
+++ b/arch/x86/boot/cpuflags.c
@@ -70,11 +70,19 @@ int has_eflag(unsigned long mask)
 # define EBX_REG "=b"
 #endif
 
+#if defined(__i386__) && defined(__PIC__) && !defined(CC_PRESERVES_PIC)
+# define SAVE_PIC ".ifnc %%ebx, %3;  movl %%ebx, %3; .endif\n\t"
+# define SWAP_PIC ".ifnc %%ebx, %3; xchgl %%ebx, %3; .endif\n\t"
+#else
+# define SAVE_PIC
+# define SWAP_PIC
+#endif
+
 static inline void cpuid(u32 id, u32 *a, u32 *b, u32 *c, u32 *d)
 {
-	asm volatile(".ifnc %%ebx,%3 ; movl  %%ebx,%3 ; .endif	\n\t"
-		     "cpuid					\n\t"
-		     ".ifnc %%ebx,%3 ; xchgl %%ebx,%3 ; .endif	\n\t"
+	asm volatile(SAVE_PIC
+		     "cpuid\n\t"
+		     SWAP_PIC
 		    : "=a" (*a), "=c" (*c), "=d" (*d), EBX_REG (*b)
 		    : "a" (id)
 	);
diff --git a/scripts/gcc-clobber-pic.sh b/scripts/gcc-clobber-pic.sh
new file mode 100755
index 000000000000..7ff10edf9b08
--- /dev/null
+++ b/scripts/gcc-clobber-pic.sh
@@ -0,0 +1,58 @@
+#!/bin/bash -x
+err=0
+O=$(mktemp)
+cat << "END" | $@ -fPIC -x c - -o $O >/dev/null 2>&1 || err=1
+int some_global_var, some_other_global_var;
+
+typedef unsigned int u32;
+
+void __attribute__((noinline)) foo(void)
+{
+	asm volatile("# some crap just so that we don't get optimized away");
+
+	some_other_global_var = 43;
+}
+
+static inline void cpuid(u32 id, u32 *a, u32 *b, u32 *c, u32 *d)
+{
+        asm volatile("cpuid"
+                    : "=a" (*a), "=b" (*b), "=c" (*c), "=d" (*d)
+                    : "a" (id), "2" (*c)
+		    : "si", "di"
+        );
+
+	some_global_var = 42;
+	foo();
+}
+
+int main(void)
+{
+	u32 a, b, c = 0, d;
+
+	cpuid(0x1, &a, &b, &c, &d);
+
+	/*
+	 * Make sure foo() gets actually called and not optimized away due to
+	 * miscompilation.
+	 */
+	if (some_global_var == 42 && some_other_global_var == 43)
+		return 0;
+	else
+		return 1;
+}
+END
+
+if (( $err ));
+then
+	exit 1
+fi
+
+chmod u+x $O
+$O
+
+if ! (( $? ));
+then
+	echo "n"
+fi
+
+rm -f $O
-- 
2.11.0

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[toc] | [prev] | [next] | [standalone]

#1542948

On December 15, 2016 6:39:44 AM PST, Borislav Petkov <bp@alien8.de> wrote:
>On Wed, Dec 14, 2016 at 12:07:54AM +0100, Boris Petkov wrote:
>> Thus I was thinking of adding a build-time check for the gcc version
>> but that might turn out to be more code in the end than those ugly
>> ifnc clauses.
>
>IOW, something like this. I did this just to try to see whether it is
>doable. And it does work - gcc 4.8 and 4.9 -m32 cannot preserve the PIC
>register - actually the inline asm fails building due to impossible
>constraints.
>
>However, so many lines changed just to save the ifnc, meh, I dunno...
>
>---
> arch/x86/boot/compressed/Makefile |  8 ++++++
> arch/x86/boot/cpuflags.c          | 14 ++++++++--
>scripts/gcc-clobber-pic.sh        | 58
>+++++++++++++++++++++++++++++++++++++++
> 3 files changed, 77 insertions(+), 3 deletions(-)
> create mode 100755 scripts/gcc-clobber-pic.sh
>
>diff --git a/arch/x86/boot/compressed/Makefile
>b/arch/x86/boot/compressed/Makefile
>index 34d9e15857c3..705fc2ab3fd6 100644
>--- a/arch/x86/boot/compressed/Makefile
>+++ b/arch/x86/boot/compressed/Makefile
>@@ -35,6 +35,14 @@ KBUILD_CFLAGS += -mno-mmx -mno-sse
> KBUILD_CFLAGS += $(call cc-option,-ffreestanding)
> KBUILD_CFLAGS += $(call cc-option,-fno-stack-protector)
> 
>+# check whether inline asm clobbers the PIC register
>+ifeq ($(CONFIG_X86_32),y)
>+ifeq ($(shell $(CONFIG_SHELL) $(srctree)/scripts/gcc-clobber-pic.sh
>$(CC) -m32),n)
>+	KBUILD_CFLAGS += -DCC_PRESERVES_PIC
>+	KBUILD_AFLAGS += -DCC_PRESERVES_PIC
>+endif
>+endif
>+
> KBUILD_AFLAGS  := $(KBUILD_CFLAGS) -D__ASSEMBLY__
> GCOV_PROFILE := n
> UBSAN_SANITIZE :=n
>diff --git a/arch/x86/boot/cpuflags.c b/arch/x86/boot/cpuflags.c
>index 6687ab953257..913c3f5ab3a0 100644
>--- a/arch/x86/boot/cpuflags.c
>+++ b/arch/x86/boot/cpuflags.c
>@@ -70,11 +70,19 @@ int has_eflag(unsigned long mask)
> # define EBX_REG "=b"
> #endif
> 
>+#if defined(__i386__) && defined(__PIC__) &&
>!defined(CC_PRESERVES_PIC)
>+# define SAVE_PIC ".ifnc %%ebx, %3;  movl %%ebx, %3; .endif\n\t"
>+# define SWAP_PIC ".ifnc %%ebx, %3; xchgl %%ebx, %3; .endif\n\t"
>+#else
>+# define SAVE_PIC
>+# define SWAP_PIC
>+#endif
>+
> static inline void cpuid(u32 id, u32 *a, u32 *b, u32 *c, u32 *d)
> {
>-	asm volatile(".ifnc %%ebx,%3 ; movl  %%ebx,%3 ; .endif	\n\t"
>-		     "cpuid					\n\t"
>-		     ".ifnc %%ebx,%3 ; xchgl %%ebx,%3 ; .endif	\n\t"
>+	asm volatile(SAVE_PIC
>+		     "cpuid\n\t"
>+		     SWAP_PIC
> 		    : "=a" (*a), "=c" (*c), "=d" (*d), EBX_REG (*b)
> 		    : "a" (id)
> 	);
>diff --git a/scripts/gcc-clobber-pic.sh b/scripts/gcc-clobber-pic.sh
>new file mode 100755
>index 000000000000..7ff10edf9b08
>--- /dev/null
>+++ b/scripts/gcc-clobber-pic.sh
>@@ -0,0 +1,58 @@
>+#!/bin/bash -x
>+err=0
>+O=$(mktemp)
>+cat << "END" | $@ -fPIC -x c - -o $O >/dev/null 2>&1 || err=1
>+int some_global_var, some_other_global_var;
>+
>+typedef unsigned int u32;
>+
>+void __attribute__((noinline)) foo(void)
>+{
>+	asm volatile("# some crap just so that we don't get optimized away");
>+
>+	some_other_global_var = 43;
>+}
>+
>+static inline void cpuid(u32 id, u32 *a, u32 *b, u32 *c, u32 *d)
>+{
>+        asm volatile("cpuid"
>+                    : "=a" (*a), "=b" (*b), "=c" (*c), "=d" (*d)
>+                    : "a" (id), "2" (*c)
>+		    : "si", "di"
>+        );
>+
>+	some_global_var = 42;
>+	foo();
>+}
>+
>+int main(void)
>+{
>+	u32 a, b, c = 0, d;
>+
>+	cpuid(0x1, &a, &b, &c, &d);
>+
>+	/*
>+	 * Make sure foo() gets actually called and not optimized away due to
>+	 * miscompilation.
>+	 */
>+	if (some_global_var == 42 && some_other_global_var == 43)
>+		return 0;
>+	else
>+		return 1;
>+}
>+END
>+
>+if (( $err ));
>+then
>+	exit 1
>+fi
>+
>+chmod u+x $O
>+$O
>+
>+if ! (( $? ));
>+then
>+	echo "n"
>+fi
>+
>+rm -f $O

This really is only worthwhile if it ends up producing better code, but I doubt it.
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

[toc] | [prev] | [next] | [standalone]

#1542988

On Thu, Dec 15, 2016 at 09:52:12AM -0800, hpa@zytor.com wrote:
> This really is only worthwhile if it ends up producing better code,
> but I doubt it.

Nah, the most it does is drops those ifnc lines in there on newer gccs.

They will appear only on
		gcc-4 and earlier and
		if we're -fPIC and
		if we're -m32 and
		if we have enough register pressure to force gcc to use the PIC	register

It was a good exercise for me to see in detail how would I go about
doing a gcc-specific workaround.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[toc] | [prev] | [next] | [standalone]

#1543066

On December 15, 2016 11:20:17 AM PST, Andi Kleen <ak@linux.intel.com> wrote:
>
>The code is not calling CPUID in any performance critical path, only
>at initialization. So any discussion about saving a few instructions
>is a complete waste of time.
>
>-Andi

Sort of.  The BIOS boot code is very space-constrained for certain legacy bootloaders to continue to work.  The BIOS boot code proper does not need PIC.

However, the existing .ifnc solution already takes care of it, so it doesn't matter.
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

[toc] | [prev] | [next] | [standalone]

#1543069

On December 15, 2016 11:20:17 AM PST, Andi Kleen <ak@linux.intel.com> wrote:
>
>The code is not calling CPUID in any performance critical path, only
>at initialization. So any discussion about saving a few instructions
>is a complete waste of time.
>
>-Andi

NB: the chief offender is Loadlin, which is still used in some manufacturing flows that depends on a mix of legacy DOS and Linux applications; however, older versions of LILO also have this problem.
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

[toc] | [prev] | [next] | [standalone]

#1543095

The code is not calling CPUID in any performance critical path, only
at initialization. So any discussion about saving a few instructions
is a complete waste of time.

-Andi

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web