x86, AMD: Correct F15h IC aliasing issue

PATCH In review Borislav Petkov 3 participants 17 messages Aug 5, 2011 – Aug 6, 2011

Subsystems (4)

Documentation x86 exec and binfmt x86 MM

lore

Share & tools

lore

Borislav Petkov

author

2011-08-05 13:15 UTC #0 lore raw

cover x86, AMD: Correct F15h IC aliasing issue

From: Borislav Petkov <[email protected]>

Hi,

a small refinement of the patchset from yesterday per hpa's comments:

* put mask and flags into a single cacheline and make it __read_mostly

* change alignment computation back to clearing bits [14:12] so that a mask of 0x0 can have no effect on the address.

Please take a look and apply, if no objections.

Thanks.

---
Changelog:

v3:
here's an updated and revised patchset addressing all comments from last time:

* saturate bits [14:12] instead of clearing them
* calculate the mask from the CPUID 0x8000_0005 IC identifier instead of hardcoding it

v2:
here's the second version of this patch which actually turned into a small patchset. As Ingo suggested, the initial patch stays first to ease backporting and the following 3 patches address (hopefully) all review comments from the initial submission. The patchset has been tested with Debian's old stable lenny (i.e. 5.0) distro in a 32-bit environment and all worked as expected.

Below some performance data to show that there is no noticeable performance degradation introduced by the changeset.

So please, do take a look again and let me know.

Thanks.

VA alignment enabled.
====================

Performance counter stats for './build.sh' (10 runs):

    3187047.935990 task-clock                #   24.001 CPUs utilized            ( +-  1.37% )
           510,888 context-switches          #    0.000 M/sec                    ( +-  0.44% )
            60,712 CPU-migrations            #    0.000 M/sec                    ( +-  0.51% )
        26,046,891 page-faults               #    0.008 M/sec                    ( +-  0.00% )
 1,841,068,123,735 cycles                    #    0.578 GHz                      ( +-  1.10% ) [63.39%]
   560,044,437,348 stalled-cycles-frontend   #   30.42% frontend cycles idle     ( +-  1.13% ) [64.65%]
   436,165,228,465 stalled-cycles-backend    #   23.69% backend  cycles idle     ( +-  1.19% ) [67.21%]
 1,461,854,088,667 instructions              #    0.79  insns per cycle
                                             #    0.38  stalled cycles per insn  ( +-  0.77% ) [70.31%]
   334,169,452,362 branches                  #  104.852 M/sec                    ( +-  1.20% ) [69.43%]
    21,485,007,982 branch-misses             #    6.43% of all branches          ( +-  0.68% ) [65.01%]

132.787483539 seconds time elapsed ( +- 1.37% )

VA alignment disabled
=====================

Performance counter stats for './build.sh' (10 runs):

    3173688.887193 task-clock                #   24.001 CPUs utilized            ( +-  1.37% )
           511,425 context-switches          #    0.000 M/sec                    ( +-  0.28% )
            60,522 CPU-migrations            #    0.000 M/sec                    ( +-  0.60% )
        26,046,902 page-faults               #    0.008 M/sec                    ( +-  0.00% )
 1,832,825,813,094 cycles                    #    0.578 GHz                      ( +-  0.96% ) [63.60%]
   563,123,451,900 stalled-cycles-frontend   #   30.72% frontend cycles idle     ( +-  0.96% ) [63.97%]
   439,565,070,106 stalled-cycles-backend    #   23.98% backend  cycles idle     ( +-  1.23% ) [66.69%]
 1,465,314,643,020 instructions              #    0.80  insns per cycle
                                             #    0.38  stalled cycles per insn  ( +-  0.74% ) [70.11%]
   332,416,669,982 branches                  #  104.741 M/sec                    ( +-  0.85% ) [69.71%]
    21,181,821,204 branch-misses             #    6.37% of all branches          ( +-  0.97% ) [65.93%]

132.230903628 seconds time elapsed ( +- 1.37% )

stock 3.0
=========

Performance counter stats for './build.sh' (10 runs):

    3369707.240439 task-clock                #   24.001 CPUs utilized            ( +-  1.18% )
           510,450 context-switches          #    0.000 M/sec                    ( +-  0.29% )
            58,906 CPU-migrations            #    0.000 M/sec                    ( +-  0.35% )
        26,057,272 page-faults               #    0.008 M/sec                    ( +-  0.00% )
 1,836,326,075,063 cycles                    #    0.545 GHz                      ( +-  1.05% ) [63.51%]
   561,850,647,545 stalled-cycles-frontend   #   30.60% frontend cycles idle     ( +-  1.03% ) [64.17%]
   439,923,021,200 stalled-cycles-backend    #   23.96% backend  cycles idle     ( +-  1.10% ) [66.64%]
 1,467,236,934,265 instructions              #    0.80  insns per cycle
                                             #    0.38  stalled cycles per insn  ( +-  0.87% ) [70.06%]
   331,937,054,120 branches                  #   98.506 M/sec                    ( +-  0.81% ) [69.83%]
    21,228,553,080 branch-misses             #    6.40% of all branches          ( +-  0.87% ) [65.79%]

140.398317711 seconds time elapsed ( +- 1.18% )

Borislav Petkov

author

2011-08-05 13:15 UTC #1 lore raw

PATCH 1/3 x86, AMD: Correct F15h IC aliasing issue

From: Borislav Petkov <[email protected]>

This patch provides performance tuning for the "Bulldozer" CPU. With its shared instruction cache there is a chance of generating an excessive number of cache cross-invalidates when running specific workloads on the cores of a compute module.

This excessive amount of cross-invalidations can be observed if cache lines backed by shared physical memory alias in bits [14:12] of their virtual addresses, as those bits are used for the index generation.

This patch addresses the issue by clearing all the bits in the [14:12] slice of the file mapping's virtual address at generation time, thus forcing those bits the same for all mappings of a single shared library across processes and, in doing so, avoids instruction cache aliases.

It also adds the command line option "align_va_addr=(32|64|on|off)" with which virtual address alignment can be enabled for 32-bit or 64-bit x86 individually, or both, or be completely disabled.

This change leaves virtual region address allocation on other families and/or vendors unaffected.

Signed-off-by Borislav Petkov <[email protected]>

---
 Documentation/kernel-parameters.txt |   13 ++++++
 arch/x86/include/asm/elf.h          |   31 +++++++++++++
 arch/x86/kernel/cpu/amd.c           |   13 ++++++
 arch/x86/kernel/sys_x86_64.c        |   81 +++++++++++++++++++++++++++++++++-
 arch/x86/mm/mmap.c                  |   15 ------
 arch/x86/vdso/vma.c                 |    9 ++++
 6 files changed, 144 insertions(+), 18 deletions(-)

Documentation/kernel-parameters.txt +13 -0

index aa47be7..af73c03 100644
@@ -299,6 +299,19 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 			behaviour to be specified.  Bit 0 enables warnings,
 			bit 1 enables fixups, and bit 2 sends a segfault.
 
+	align_va_addr=	[X86-64]
+			Align virtual addresses by clearing slice [14:12] when
+			allocating a VMA at process creation time. This option
+			gives you up to 3% performance improvement on AMD F15h
+			machines (where it is enabled by default) for a
+			CPU-intensive style benchmark, and it can vary highly in
+			a microbenchmark depending on workload and compiler.
+
+			1: only for 32-bit processes
+			2: only for 64-bit processes
+			on: enable for both 32- and 64-bit processes
+			off: disable for both 32- and 64-bit processes
+
 	amd_iommu=	[HW,X86-84]
 			Pass parameters to the AMD IOMMU driver in the system.
 			Possible values are:

arch/x86/include/asm/elf.h +31 -0

index f2ad216..5f962df 100644
@@ -4,6 +4,7 @@
 /*
  * ELF register definitions..
  */
+#include <linux/thread_info.h>
 
 #include <asm/ptrace.h>
 #include <asm/user.h>

@@ -320,4 +321,34 @@ extern int syscall32_setup_pages(struct linux_binprm *, int exstack);
 extern unsigned long arch_randomize_brk(struct mm_struct *mm);
 #define arch_randomize_brk arch_randomize_brk
 
+/*
+ * True on X86_32 or when emulating IA32 on X86_64
+ */
+static inline int mmap_is_ia32(void)
+{
+#ifdef CONFIG_X86_32
+	return 1;
+#endif
+#ifdef CONFIG_IA32_EMULATION
+	if (test_thread_flag(TIF_IA32))
+		return 1;
+#endif
+	return 0;
+}
+
+/* The first two values are special, do not change. See align_addr() */
+enum align_flags {
+	ALIGN_VA_32	= BIT(0),
+	ALIGN_VA_64	= BIT(1),
+	ALIGN_VDSO	= BIT(2),
+	ALIGN_TOPDOWN	= BIT(3),
+};
+
+struct va_alignment {
+	int flags;
+	unsigned long mask;
+} ____cacheline_aligned;
+
+extern struct va_alignment va_align;
+extern unsigned long align_addr(unsigned long, struct file *, enum align_flags);
 #endif /* _ASM_X86_ELF_H */

arch/x86/kernel/cpu/amd.c +13 -0

index b13ed39..b0234bc 100644
@@ -458,6 +458,19 @@ static void __cpuinit early_init_amd(struct cpuinfo_x86 *c)
 					"with P0 frequency!\n");
 		}
 	}
+
+	if (c->x86 == 0x15) {
+		unsigned long upperbit;
+		u32 cpuid, assoc;
+
+		cpuid	 = cpuid_edx(0x80000005);
+		assoc	 = cpuid >> 16 & 0xff;
+		upperbit = ((cpuid >> 24) << 10) / assoc;
+
+		va_align.mask	  = (upperbit - 1) & PAGE_MASK;
+		va_align.flags    = ALIGN_VA_32 | ALIGN_VA_64;
+
+	}
 }
 
 static void __cpuinit init_amd(struct cpuinfo_x86 *c)

arch/x86/kernel/sys_x86_64.c +78 -3

112 lines not on this page · raw message

arch/x86/mm/mmap.c +0 ~~-15~~

index 1dab519..d4c0736 100644
@@ -51,21 +51,6 @@ static unsigned int stack_maxrandom_size(void)
 #define MIN_GAP (128*1024*1024UL + stack_maxrandom_size())
 #define MAX_GAP (TASK_SIZE/6*5)
 
-/*
- * True on X86_32 or when emulating IA32 on X86_64
- */
-static int mmap_is_ia32(void)
-{
-#ifdef CONFIG_X86_32
-	return 1;
-#endif
-#ifdef CONFIG_IA32_EMULATION
-	if (test_thread_flag(TIF_IA32))
-		return 1;
-#endif
-	return 0;
-}
-
 static int mmap_is_legacy(void)
 {
 	if (current->personality & ADDR_COMPAT_LAYOUT)

arch/x86/vdso/vma.c +9 -0

index 7abd2be..caa42ce 100644
@@ -69,6 +69,15 @@ static unsigned long vdso_addr(unsigned long start, unsigned len)
 	addr = start + (offset << PAGE_SHIFT);
 	if (addr >= end)
 		addr = end;
+
+	/*
+	 * page-align it here so that get_unmapped_area doesn't
+	 * align it wrongfully again to the next page. addr can come in 4K
+	 * unaligned here as a result of stack start randomization.
+	 */
+	addr = PAGE_ALIGN(addr);
+	addr = align_addr(addr, NULL, ALIGN_VDSO);
+
 	return addr;
 }

Signature · 1 line

1.7.4.rc2

tip-bot for Borislav Petkov

2011-08-05 22:58 UTC #2 lore raw

[tip:x86/cpu] x86, amd: Avoid cache aliasing penalties on AMD family 15h

Commit-ID:  dfb09f9b7ab03fd367740e541a5caf830ed56726
Gitweb:     http://git.kernel.org/tip/dfb09f9b7ab03fd367740e541a5caf830ed56726
Author:     Borislav Petkov <[email protected]>
AuthorDate: Fri, 5 Aug 2011 15:15:08 +0200
Committer:  H. Peter Anvin <[email protected]>
CommitDate: Fri, 5 Aug 2011 12:26:44 -0700

x86, amd: Avoid cache aliasing penalties on AMD family 15h

It also adds the command line option "align_va_addr=(32|64|on|off)" with which virtual address alignment can be enabled for 32-bit or 64-bit x86 individually, or both, or be completely disabled.

This change leaves virtual region address allocation on other families and/or vendors unaffected.

Signed-off-by Borislav Petkov <[email protected]>

Link http://lkml.kernel.org/r/[email protected] open here

Signed-off-by H. Peter Anvin <[email protected]>

---
 Documentation/kernel-parameters.txt |   13 ++++++
 arch/x86/include/asm/elf.h          |   31 +++++++++++++
 arch/x86/kernel/cpu/amd.c           |   13 ++++++
 arch/x86/kernel/sys_x86_64.c        |   81 +++++++++++++++++++++++++++++++++-
 arch/x86/mm/mmap.c                  |   15 ------
 arch/x86/vdso/vma.c                 |    9 ++++
 6 files changed, 144 insertions(+), 18 deletions(-)

Documentation/kernel-parameters.txt +13 -0

index aa47be7..af73c03 100644
@@ -299,6 +299,19 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 			behaviour to be specified.  Bit 0 enables warnings,
 			bit 1 enables fixups, and bit 2 sends a segfault.
 
+	align_va_addr=	[X86-64]
+			Align virtual addresses by clearing slice [14:12] when
+			allocating a VMA at process creation time. This option
+			gives you up to 3% performance improvement on AMD F15h
+			machines (where it is enabled by default) for a
+			CPU-intensive style benchmark, and it can vary highly in
+			a microbenchmark depending on workload and compiler.
+
+			1: only for 32-bit processes
+			2: only for 64-bit processes
+			on: enable for both 32- and 64-bit processes
+			off: disable for both 32- and 64-bit processes
+
 	amd_iommu=	[HW,X86-84]
 			Pass parameters to the AMD IOMMU driver in the system.
 			Possible values are:

arch/x86/include/asm/elf.h +31 -0

index f2ad216..5f962df 100644
@@ -4,6 +4,7 @@
 /*
  * ELF register definitions..
  */
+#include <linux/thread_info.h>
 
 #include <asm/ptrace.h>
 #include <asm/user.h>

@@ -320,4 +321,34 @@ extern int syscall32_setup_pages(struct linux_binprm *, int exstack);
 extern unsigned long arch_randomize_brk(struct mm_struct *mm);
 #define arch_randomize_brk arch_randomize_brk
 
+/*
+ * True on X86_32 or when emulating IA32 on X86_64
+ */
+static inline int mmap_is_ia32(void)
+{
+#ifdef CONFIG_X86_32
+	return 1;
+#endif
+#ifdef CONFIG_IA32_EMULATION
+	if (test_thread_flag(TIF_IA32))
+		return 1;
+#endif
+	return 0;
+}
+
+/* The first two values are special, do not change. See align_addr() */
+enum align_flags {
+	ALIGN_VA_32	= BIT(0),
+	ALIGN_VA_64	= BIT(1),
+	ALIGN_VDSO	= BIT(2),
+	ALIGN_TOPDOWN	= BIT(3),
+};
+
+struct va_alignment {
+	int flags;
+	unsigned long mask;
+} ____cacheline_aligned;
+
+extern struct va_alignment va_align;
+extern unsigned long align_addr(unsigned long, struct file *, enum align_flags);
 #endif /* _ASM_X86_ELF_H */

arch/x86/kernel/cpu/amd.c +13 -0

index b13ed39..b0234bc 100644
@@ -458,6 +458,19 @@ static void __cpuinit early_init_amd(struct cpuinfo_x86 *c)
 					"with P0 frequency!\n");
 		}
 	}
+
+	if (c->x86 == 0x15) {
+		unsigned long upperbit;
+		u32 cpuid, assoc;
+
+		cpuid	 = cpuid_edx(0x80000005);
+		assoc	 = cpuid >> 16 & 0xff;
+		upperbit = ((cpuid >> 24) << 10) / assoc;
+
+		va_align.mask	  = (upperbit - 1) & PAGE_MASK;
+		va_align.flags    = ALIGN_VA_32 | ALIGN_VA_64;
+
+	}
 }
 
 static void __cpuinit init_amd(struct cpuinfo_x86 *c)

arch/x86/kernel/sys_x86_64.c +78 -3

112 lines not on this page · raw message

arch/x86/mm/mmap.c +0 ~~-15~~

index 1dab519..d4c0736 100644
@@ -51,21 +51,6 @@ static unsigned int stack_maxrandom_size(void)
 #define MIN_GAP (128*1024*1024UL + stack_maxrandom_size())
 #define MAX_GAP (TASK_SIZE/6*5)
 
-/*
- * True on X86_32 or when emulating IA32 on X86_64
- */
-static int mmap_is_ia32(void)
-{
-#ifdef CONFIG_X86_32
-	return 1;
-#endif
-#ifdef CONFIG_IA32_EMULATION
-	if (test_thread_flag(TIF_IA32))
-		return 1;
-#endif
-	return 0;
-}
-
 static int mmap_is_legacy(void)
 {
 	if (current->personality & ADDR_COMPAT_LAYOUT)

arch/x86/vdso/vma.c +9 -0

index 7abd2be..caa42ce 100644
@@ -69,6 +69,15 @@ static unsigned long vdso_addr(unsigned long start, unsigned len)
 	addr = start + (offset << PAGE_SHIFT);
 	if (addr >= end)
 		addr = end;
+
+	/*
+	 * page-align it here so that get_unmapped_area doesn't
+	 * align it wrongfully again to the next page. addr can come in 4K
+	 * unaligned here as a result of stack start randomization.
+	 */
+	addr = PAGE_ALIGN(addr);
+	addr = align_addr(addr, NULL, ALIGN_VDSO);
+
 	return addr;
 }

H. Peter Anvin

reviewer

2011-08-06 00:22 UTC #3 lore raw

Re: [tip:x86/cpu] x86, amd: Avoid cache aliasing penalties on AMD family 15h

Quoted 14 lines · tip-bot for Borislav Petkov

arch/x86/kernel/cpu/amd.c +6 -0

index b13ed39..b0234bc 100644
@@ -458,6 +458,19 @@ static void __cpuinit early_init_amd(struct cpuinfo_x86 *c)
 					"with P0 frequency!\n");
 		}
 	}
+
+	if (c->x86 == 0x15) {
+		unsigned long upperbit;
+		u32 cpuid, assoc;
+
+		cpuid	 = cpuid_edx(0x80000005);

arch/x86/kernel/cpu/amd.c static void __cpuinit early_init_amd(struct cpuinfo_x86 *c) · line 476

Quoted 10 lines

arch/x86/kernel/cpu/amd.c +7 -0

+		assoc	 = cpuid >> 16 & 0xff;
+		upperbit = ((cpuid >> 24) << 10) / assoc;
+
+		va_align.mask	  = (upperbit - 1) & PAGE_MASK;
+		va_align.flags    = ALIGN_VA_32 | ALIGN_VA_64;
+
+	}
 }
 
 static void __cpuinit init_amd(struct cpuinfo_x86 *c)

Breaks all i386 builds:

/home/hpa/kernel/linux-tip.cpu/arch/x86/kernel/cpu/amd.c:437: undefined reference to va_align' /home/hpa/kernel/linux-tip.cpu/arch/x86/kernel/cpu/amd.c:436: undefined reference to va_align'

[the line numbers refer to the entire patchset]

-hpa

Borislav Petkov

author

2011-08-06 12:31 UTC #4 lore raw

PATCH 1/1 x86, AMD: Fix 32-bit build after cache aliasing patch

From: Borislav Petkov <[email protected]>

hpa reported that dfb09f9b7ab03fd367740e541a5caf830ed56726 breaks 32-bit builds with the following error message:

/home/hpa/kernel/linux-tip.cpu/arch/x86/kernel/cpu/amd.c:437: undefined reference to va_align' /home/hpa/kernel/linux-tip.cpu/arch/x86/kernel/cpu/amd.c:436: undefined reference to va_align'

This is due to the fact that va_align is a global in a 64-bit only compilation unit. Move it to mmap.c where it is visible to both subarches.

Signed-off-by Borislav Petkov <[email protected]>

---
 arch/x86/kernel/sys_x86_64.c |    4 ----
 arch/x86/mm/mmap.c           |    5 ++++-
 2 files changed, 4 insertions(+), 5 deletions(-)

arch/x86/kernel/sys_x86_64.c +0 -4

index aaa8d09..fe7d2da 100644
@@ -18,10 +18,6 @@
 #include <asm/ia32.h>
 #include <asm/syscalls.h>
 
-struct __read_mostly va_alignment va_align = {
-	.flags = -1,
-};
-
 /*
  * Align a virtual address to avoid aliasing in the I$ on AMD F15h.
  *

arch/x86/mm/mmap.c +4 -1

index d4c0736..4b5ba85 100644
@@ -31,6 +31,10 @@
 #include <linux/sched.h>
 #include <asm/elf.h>
 
+struct __read_mostly va_alignment va_align = {
+	.flags = -1,
+};
+
 static unsigned int stack_maxrandom_size(void)
 {
 	unsigned int max = 0;

@@ -42,7 +46,6 @@ static unsigned int stack_maxrandom_size(void)
 	return max;
 }
 
-
 /*
  * Top of mmap area (just below the process stack).
  *

Signature · 1 line

1.7.4.rc2

tip-bot for Borislav Petkov

2011-08-06 23:22 UTC #5 lore raw

[tip:x86/cpu] x86-32, amd: Move va_align definition to unbreak 32-bit build

Commit-ID:  9387f774d61b01ab71bade85e6d0bfab0b3419bd
Gitweb:     http://git.kernel.org/tip/9387f774d61b01ab71bade85e6d0bfab0b3419bd
Author:     Borislav Petkov <[email protected]>
AuthorDate: Sat, 6 Aug 2011 14:31:38 +0200
Committer:  H. Peter Anvin <[email protected]>
CommitDate: Sat, 6 Aug 2011 11:44:57 -0700

x86-32, amd: Move va_align definition to unbreak 32-bit build

hpa reported that dfb09f9b7ab03fd367740e541a5caf830ed56726 breaks 32-bit builds with the following error message:

/home/hpa/kernel/linux-tip.cpu/arch/x86/kernel/cpu/amd.c:437: undefined reference to va_align' /home/hpa/kernel/linux-tip.cpu/arch/x86/kernel/cpu/amd.c:436: undefined reference to va_align'

This is due to the fact that va_align is a global in a 64-bit only compilation unit. Move it to mmap.c where it is visible to both subarches.

Signed-off-by Borislav Petkov <[email protected]>

Link http://lkml.kernel.org/r/[email protected] open here

Signed-off-by H. Peter Anvin <[email protected]>

---
 arch/x86/kernel/sys_x86_64.c |    4 ----
 arch/x86/mm/mmap.c           |    5 ++++-
 2 files changed, 4 insertions(+), 5 deletions(-)

arch/x86/kernel/sys_x86_64.c +0 -4

index aaa8d09..fe7d2da 100644
@@ -18,10 +18,6 @@
 #include <asm/ia32.h>
 #include <asm/syscalls.h>
 
-struct __read_mostly va_alignment va_align = {
-	.flags = -1,
-};
-
 /*
  * Align a virtual address to avoid aliasing in the I$ on AMD F15h.
  *

arch/x86/mm/mmap.c +4 -1

index d4c0736..4b5ba85 100644
@@ -31,6 +31,10 @@
 #include <linux/sched.h>
 #include <asm/elf.h>
 
+struct __read_mostly va_alignment va_align = {
+	.flags = -1,
+};
+
 static unsigned int stack_maxrandom_size(void)
 {
 	unsigned int max = 0;

@@ -42,7 +46,6 @@ static unsigned int stack_maxrandom_size(void)
 	return max;
 }
 
-
 /*
  * Top of mmap area (just below the process stack).
  *

Borislav Petkov

author

2011-08-05 13:15 UTC #6 lore raw

PATCH 2/3 x86: Add a BSP cpuinit helper

In reply to Borislav Petkov

From: Borislav Petkov <[email protected]>

Add a function ptr to struct x86_cpuinit_ops which is destined to be run only once on the BSP during boot.

Signed-off-by Borislav Petkov <[email protected]>

---
 arch/x86/include/asm/x86_init.h |    1 +
 arch/x86/kernel/cpu/common.c    |    2 ++
 arch/x86/kernel/x86_init.c      |    1 +
 3 files changed, 4 insertions(+), 0 deletions(-)

arch/x86/include/asm/x86_init.h +1 -0

index d3d8590..08994a0 100644
@@ -147,6 +147,7 @@ struct x86_init_ops {
  */
 struct x86_cpuinit_ops {
 	void (*setup_percpu_clockev)(void);
+	void (*run_on_bsp)(void);
 };
 
 /**

arch/x86/kernel/cpu/common.c +2 -0

index 22a073d..465e633 100644
@@ -681,6 +681,8 @@ static void __init early_identify_cpu(struct cpuinfo_x86 *c)
 	filter_cpuid_features(c, false);
 
 	setup_smep(c);
+
+	x86_cpuinit.run_on_bsp();
 }
 
 void __init early_cpu_init(void)

arch/x86/kernel/x86_init.c +1 -0

index 6f164bd..76b37ed 100644
@@ -90,6 +90,7 @@ struct x86_init_ops x86_init __initdata = {
 
 struct x86_cpuinit_ops x86_cpuinit __cpuinitdata = {
 	.setup_percpu_clockev		= setup_secondary_APIC_clock,
+	.run_on_bsp			= x86_init_noop,
 };
 
 static void default_nmi_init(void) { };

Signature · 1 line

1.7.4.rc2

Borislav Petkov

author

2011-08-05 13:15 UTC #7 lore raw

PATCH 3/3 x86, AMD: Move BSP code to cpuinit helper

In reply to Borislav Petkov

From: Borislav Petkov <[email protected]>

Move code which is run once on the BSP during boot into the cpuinit helper.

Signed-off-by Borislav Petkov <[email protected]>

---
 arch/x86/kernel/cpu/amd.c |   60 +++++++++++++++++++++++---------------------
 1 files changed, 31 insertions(+), 29 deletions(-)

arch/x86/kernel/cpu/amd.c +31 ~~-29~~

index b0234bc..16939b8 100644
@@ -410,6 +410,36 @@ static void __cpuinit early_init_amd_mc(struct cpuinfo_x86 *c)
 #endif
 }
 
+static void __cpuinit amd_run_on_bsp(void)
+{
+	struct cpuinfo_x86 *c = &boot_cpu_data;
+
+	if (static_cpu_has(X86_FEATURE_CONSTANT_TSC)) {
+
+		if (c->x86 > 0x10 ||
+		    (c->x86 == 0x10 && c->x86_model >= 0x2)) {
+			u64 val;
+
+			rdmsrl(MSR_K7_HWCR, val);
+			if (!(val & BIT(24)))
+				printk(KERN_WARNING FW_BUG "TSC doesn't count "
+					"with P0 frequency!\n");
+		}
+	}
+
+	if (c->x86 == 0x15) {
+		unsigned long upperbit;
+		u32 cpuid, assoc;
+
+		cpuid	 = cpuid_edx(0x80000005);
+		assoc	 = cpuid >> 16 & 0xff;
+		upperbit = ((cpuid >> 24) << 10) / assoc;
+
+		va_align.mask	  = (upperbit - 1) & PAGE_MASK;
+		va_align.flags    = ALIGN_VA_32 | ALIGN_VA_64;
+	}
+}
+
 static void __cpuinit early_init_amd(struct cpuinfo_x86 *c)
 {
 	early_init_amd_mc(c);

@@ -442,35 +472,7 @@ static void __cpuinit early_init_amd(struct cpuinfo_x86 *c)
 	}
 #endif
 
-	/* We need to do the following only once */
-	if (c != &boot_cpu_data)
-		return;
-
-	if (cpu_has(c, X86_FEATURE_CONSTANT_TSC)) {
-
-		if (c->x86 > 0x10 ||
-		    (c->x86 == 0x10 && c->x86_model >= 0x2)) {
-			u64 val;
-
-			rdmsrl(MSR_K7_HWCR, val);
-			if (!(val & BIT(24)))
-				printk(KERN_WARNING FW_BUG "TSC doesn't count "
-					"with P0 frequency!\n");
-		}
-	}
-
-	if (c->x86 == 0x15) {
-		unsigned long upperbit;
-		u32 cpuid, assoc;
-
-		cpuid	 = cpuid_edx(0x80000005);
-		assoc	 = cpuid >> 16 & 0xff;
-		upperbit = ((cpuid >> 24) << 10) / assoc;
-
-		va_align.mask	  = (upperbit - 1) & PAGE_MASK;
-		va_align.flags    = ALIGN_VA_32 | ALIGN_VA_64;
-
-	}
+	x86_cpuinit.run_on_bsp = amd_run_on_bsp;
 }
 
 static void __cpuinit init_amd(struct cpuinfo_x86 *c)

Signature · 1 line

1.7.4.rc2

H. Peter Anvin

reviewer

2011-08-05 17:11 UTC #8 lore raw

In reply to Borislav Petkov

Quoted 4 lines · Borislav Petkov

From: Borislav Petkov <[email protected]>
Hi,

Quoted 9 lines

a small refinement of the patchset from yesterday per hpa's comments:
* put mask and flags into a single cacheline and make it __read_mostly
* change alignment computation back to clearing bits [14:12] so that a mask of 0x0 can have no effect on the address.
Please take a look and apply, if no objections.

Patch 1 looks good now.

Patch 2 I'm going to object to because it puts your run_on_bsp method into a different structure where all the existing methods for this already are in a way that looks totally gratuitous to me. Why not just have a c_bsp_init on struct cpu_dev like all the other methods?

-hpa

Signature · 2 lines

H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Borislav Petkov

author

2011-08-05 17:55 UTC #9 lore raw

Quoted 4 lines · H. Peter Anvin

Patch 2 I'm going to object to because it puts your run_on_bsp method into a different structure where all the existing methods for this already are in a way that looks totally gratuitous to me. Why not just have a c_bsp_init on struct cpu_dev like all the other methods?

Indeed, cpu_dev is actually the only logical place to put them, thanks. I'm sending updated versions as a reply to this message.

Signature · 8 lines

Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

Borislav Petkov

author

2011-08-05 18:01 UTC #10 lore raw

PATCH 2/3 x86: Add a BSP cpu_dev helper

Add a function ptr to struct cpu_dev which is destined to be run only once on the BSP during boot.

Signed-off-by Borislav Petkov <[email protected]>

---
 arch/x86/kernel/cpu/common.c |    3 +++
 arch/x86/kernel/cpu/cpu.h    |    1 +
 2 files changed, 4 insertions(+), 0 deletions(-)

arch/x86/kernel/cpu/common.c +3 -0

index 22a073d..8ed394a 100644
@@ -681,6 +681,9 @@ static void __init early_identify_cpu(struct cpuinfo_x86 *c)
 	filter_cpuid_features(c, false);
 
 	setup_smep(c);
+
+	if (this_cpu->c_bsp_init)
+		this_cpu->c_bsp_init(c);
 }
 
 void __init early_cpu_init(void)

arch/x86/kernel/cpu/cpu.h +1 -0

index e765633..1b22dcc 100644
@@ -18,6 +18,7 @@ struct cpu_dev {
 	struct		cpu_model_info c_models[4];
 
 	void            (*c_early_init)(struct cpuinfo_x86 *);
+	void		(*c_bsp_init)(struct cpuinfo_x86 *);
 	void		(*c_init)(struct cpuinfo_x86 *);
 	void		(*c_identify)(struct cpuinfo_x86 *);
 	unsigned int	(*c_size_cache)(struct cpuinfo_x86 *, unsigned int);

Signature · 1 line

1.7.4.rc2

tip-bot for Borislav Petkov

author

2011-08-05 22:59 UTC #11 lore raw

[tip:x86/cpu] x86: Add a BSP cpu_dev helper

Commit-ID:  a110b5ec7371592eac856ac5c22dc7b518952d44
Gitweb:     http://git.kernel.org/tip/a110b5ec7371592eac856ac5c22dc7b518952d44
Author:     Borislav Petkov <[email protected]>
AuthorDate: Fri, 5 Aug 2011 20:01:16 +0200
Committer:  H. Peter Anvin <[email protected]>
CommitDate: Fri, 5 Aug 2011 12:26:49 -0700

x86: Add a BSP cpu_dev helper

Add a function ptr to struct cpu_dev which is destined to be run only once on the BSP during boot.

Signed-off-by Borislav Petkov <[email protected]>

Link http://lkml.kernel.org/r/20110805180116.GB26217@aftab open here

Signed-off-by H. Peter Anvin <[email protected]>

---
 arch/x86/kernel/cpu/common.c |    3 +++
 arch/x86/kernel/cpu/cpu.h    |    1 +
 2 files changed, 4 insertions(+), 0 deletions(-)

arch/x86/kernel/cpu/common.c +3 -0

index 22a073d..8ed394a 100644
@@ -681,6 +681,9 @@ static void __init early_identify_cpu(struct cpuinfo_x86 *c)
 	filter_cpuid_features(c, false);
 
 	setup_smep(c);
+
+	if (this_cpu->c_bsp_init)
+		this_cpu->c_bsp_init(c);
 }
 
 void __init early_cpu_init(void)

arch/x86/kernel/cpu/cpu.h +1 -0

index e765633..1b22dcc 100644
@@ -18,6 +18,7 @@ struct cpu_dev {
 	struct		cpu_model_info c_models[4];
 
 	void            (*c_early_init)(struct cpuinfo_x86 *);
+	void		(*c_bsp_init)(struct cpuinfo_x86 *);
 	void		(*c_init)(struct cpuinfo_x86 *);
 	void		(*c_identify)(struct cpuinfo_x86 *);
 	unsigned int	(*c_size_cache)(struct cpuinfo_x86 *, unsigned int);

Borislav Petkov

author

2011-08-05 18:04 UTC #12 lore raw

PATCH 3/3 x86, AMD: Move BSP code to cpu_dev helper

In reply to Borislav Petkov

Move code which is run once on the BSP during boot into the cpu_dev helper.

Signed-off-by Borislav Petkov <[email protected]>

---
 arch/x86/kernel/cpu/amd.c |   59 ++++++++++++++++++++++-----------------------
 1 files changed, 29 insertions(+), 30 deletions(-)

arch/x86/kernel/cpu/amd.c +29 ~~-30~~

index b0234bc..53d96f5 100644
@@ -410,6 +410,34 @@ static void __cpuinit early_init_amd_mc(struct cpuinfo_x86 *c)
 #endif
 }
 
+static void __cpuinit bsp_init_amd(struct cpuinfo_x86 *c)
+{
+	if (static_cpu_has(X86_FEATURE_CONSTANT_TSC)) {
+
+		if (c->x86 > 0x10 ||
+		    (c->x86 == 0x10 && c->x86_model >= 0x2)) {
+			u64 val;
+
+			rdmsrl(MSR_K7_HWCR, val);
+			if (!(val & BIT(24)))
+				printk(KERN_WARNING FW_BUG "TSC doesn't count "
+					"with P0 frequency!\n");
+		}
+	}
+
+	if (c->x86 == 0x15) {
+		unsigned long upperbit;
+		u32 cpuid, assoc;
+
+		cpuid	 = cpuid_edx(0x80000005);
+		assoc	 = cpuid >> 16 & 0xff;
+		upperbit = ((cpuid >> 24) << 10) / assoc;
+
+		va_align.mask	  = (upperbit - 1) & PAGE_MASK;
+		va_align.flags    = ALIGN_VA_32 | ALIGN_VA_64;
+	}
+}
+
 static void __cpuinit early_init_amd(struct cpuinfo_x86 *c)
 {
 	early_init_amd_mc(c);

@@ -441,36 +469,6 @@ static void __cpuinit early_init_amd(struct cpuinfo_x86 *c)
 			set_cpu_cap(c, X86_FEATURE_EXTD_APICID);
 	}
 #endif
-
-	/* We need to do the following only once */
-	if (c != &boot_cpu_data)
-		return;
-
-	if (cpu_has(c, X86_FEATURE_CONSTANT_TSC)) {
-
-		if (c->x86 > 0x10 ||
-		    (c->x86 == 0x10 && c->x86_model >= 0x2)) {
-			u64 val;
-
-			rdmsrl(MSR_K7_HWCR, val);
-			if (!(val & BIT(24)))
-				printk(KERN_WARNING FW_BUG "TSC doesn't count "
-					"with P0 frequency!\n");
-		}
-	}
-
-	if (c->x86 == 0x15) {
-		unsigned long upperbit;
-		u32 cpuid, assoc;
-
-		cpuid	 = cpuid_edx(0x80000005);
-		assoc	 = cpuid >> 16 & 0xff;
-		upperbit = ((cpuid >> 24) << 10) / assoc;
-
-		va_align.mask	  = (upperbit - 1) & PAGE_MASK;
-		va_align.flags    = ALIGN_VA_32 | ALIGN_VA_64;
-
-	}
 }
 
 static void __cpuinit init_amd(struct cpuinfo_x86 *c)

@@ -692,6 +690,7 @@ static const struct cpu_dev __cpuinitconst amd_cpu_dev = {
 	.c_size_cache	= amd_size_cache,
 #endif
 	.c_early_init   = early_init_amd,
+	.c_bsp_init	= bsp_init_amd,
 	.c_init		= init_amd,
 	.c_x86_vendor	= X86_VENDOR_AMD,
 };

Signature · 1 line

1.7.4.rc2

H. Peter Anvin

reviewer

2011-08-05 20:08 UTC #13 lore raw

Re: [PATCH -v3.2 3/3] x86, AMD: Move BSP code to cpu_dev helper

arch/x86/kernel/cpu/amd.c static void __cpuinit early_init_amd_mc(struct cpuinfo_x86 *c) · line 416

Quoted 6 lines · Borislav Petkov

Move code which is run once on the BSP during boot into the cpu_dev helper.
arch/x86/kernel/cpu/amd.c +4 -0
+static void __cpuinit bsp_init_amd(struct cpuinfo_x86 *c)
+{
+	if (static_cpu_has(X86_FEATURE_CONSTANT_TSC)) {
+

You can't use static_cpu_has() here, since this code runs before alternatives -- it will always be false. Furthermore, for code that only runs once, it is never a win to do patching.

Arguably bsp_init should be __init and not __cpuinit, but I don't know how to make that work with the machinery, and is something that can be fixed anyway.

-hpa

Borislav Petkov

author

2011-08-05 22:52 UTC #14 lore raw

Re: [PATCH -v3.2 3/3] x86, AMD: Move BSP code to cpu_dev helper

Quoted 11 lines · H. Peter Anvin

arch/x86/kernel/cpu/amd.c static void __cpuinit early_init_amd_mc(struct cpuinfo_x86 *c) · line 416
Quoted 6 lines · Borislav Petkov
Move code which is run once on the BSP during boot into the cpu_dev helper.
arch/x86/kernel/cpu/amd.c +4 -0
+static void __cpuinit bsp_init_amd(struct cpuinfo_x86 *c)
+{
+	if (static_cpu_has(X86_FEATURE_CONSTANT_TSC)) {
+
You can't use static_cpu_has() here, since this code runs before alternatives -- it will always be false. Furthermore, for code that only runs once, it is never a win to do patching.

Oh crap, this is a leftover from when run_on_bsp was struct x86_cpuinit_ops member with no args. And I f*cked it up even then although I went and got myself a pointer to boot_cpu_data:

+static void __cpuinit amd_run_on_bsp(void)
+{
+ struct cpuinfo_x86 *c = &boot_cpu_data;
+
+ if (static_cpu_has(X86_FEATURE_CONSTANT_TSC)) {

but forgot to use it. Good catch, will fix it tomorrow.

Quoted 3 lines

Arguably bsp_init should be __init and not __cpuinit, but I don't know how to make that work with the machinery, and is something that can be fixed anyway.

Yeah, how do we do that? struct cpu_dev is __cpuinitconst, x86_cpuinit_ops is __cpuinitdata.

We could add it to identify_boot_cpu() - there's already some per-vendor stuff like init_amd_e400_c1e_mask() which wouldn't hurt to be behind a vendor check. early_identify_cpu() does already the vendor check with get_cpu_vendor() so later, in identify_cpu() we could add a run_on_bsp() which is __init and switch/case on the ->x86_vendor inside.

Then we can collect all the run-once-on-the-BSP code in there.

Hmmm..

Signature · 8 lines

Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

H. Peter Anvin

reviewer

2011-08-05 22:57 UTC #15 lore raw

Re: [PATCH -v3.2 3/3] x86, AMD: Move BSP code to cpu_dev helper

Quoted 13 lines · Borislav Petkov

Quoted 11 lines · H. Peter Anvin
arch/x86/kernel/cpu/amd.c static void __cpuinit early_init_amd_mc(struct cpuinfo_x86 *c) · line 416
Quoted 6 lines · Borislav Petkov
Move code which is run once on the BSP during boot into the cpu_dev helper.
arch/x86/kernel/cpu/amd.c +4 -0
+static void __cpuinit bsp_init_amd(struct cpuinfo_x86 *c)
+{
+	if (static_cpu_has(X86_FEATURE_CONSTANT_TSC)) {
+
You can't use static_cpu_has() here, since this code runs before alternatives -- it will always be false. Furthermore, for code that only runs once, it is never a win to do patching.

Quoted 4 lines

Oh crap, this is a leftover from when run_on_bsp was struct x86_cpuinit_ops member with no args. And I f*cked it up even then although I went and got myself a pointer to boot_cpu_data:

I fixed it up directly.

Quoted 14 lines

Quoted 4 lines
Arguably bsp_init should be __init and not __cpuinit, but I don't know how to make that work with the machinery, and is something that can be fixed anyway.
Yeah, how do we do that? struct cpu_dev is __cpuinitconst, x86_cpuinit_ops is __cpuinitdata.
We could add it to identify_boot_cpu() - there's already some per-vendor stuff like init_amd_e400_c1e_mask() which wouldn't hurt to be behind a vendor check. early_identify_cpu() does already the vendor check with get_cpu_vendor() so later, in identify_cpu() we could add a run_on_bsp() which is __init and switch/case on the ->x86_vendor inside.

Quoted 4 lines

Then we can collect all the run-once-on-the-BSP code in there.
Hmmm..

As I said, we can fix this up incrementally.

-hpa

tip-bot for Borislav Petkov

author

2011-08-05 22:59 UTC #16 lore raw

[tip:x86/cpu] x86, amd: Move BSP code to cpu_dev helper

In reply to Borislav Petkov

Commit-ID:  8fa8b035085e7320c15875c1f6b03b290ca2dd66
Gitweb:     http://git.kernel.org/tip/8fa8b035085e7320c15875c1f6b03b290ca2dd66
Author:     Borislav Petkov <[email protected]>
AuthorDate: Fri, 5 Aug 2011 20:04:09 +0200
Committer:  H. Peter Anvin <[email protected]>
CommitDate: Fri, 5 Aug 2011 12:32:33 -0700

x86, amd: Move BSP code to cpu_dev helper

Move code which is run once on the BSP during boot into the cpu_dev helper.

[ hpa: removed bogus cpu_has -> static_cpu_has conversion ]

Signed-off-by Borislav Petkov <[email protected]>

Link http://lkml.kernel.org/r/20110805180409.GC26217@aftab open here

Signed-off-by H. Peter Anvin <[email protected]>

---
 arch/x86/kernel/cpu/amd.c |   59 ++++++++++++++++++++++-----------------------
 1 files changed, 29 insertions(+), 30 deletions(-)

arch/x86/kernel/cpu/amd.c +29 ~~-30~~

index b0234bc..b6e3e87 100644
@@ -410,6 +410,34 @@ static void __cpuinit early_init_amd_mc(struct cpuinfo_x86 *c)
 #endif
 }
 
+static void __cpuinit bsp_init_amd(struct cpuinfo_x86 *c)
+{
+	if (cpu_has(c, X86_FEATURE_CONSTANT_TSC)) {
+
+		if (c->x86 > 0x10 ||
+		    (c->x86 == 0x10 && c->x86_model >= 0x2)) {
+			u64 val;
+
+			rdmsrl(MSR_K7_HWCR, val);
+			if (!(val & BIT(24)))
+				printk(KERN_WARNING FW_BUG "TSC doesn't count "
+					"with P0 frequency!\n");
+		}
+	}
+
+	if (c->x86 == 0x15) {
+		unsigned long upperbit;
+		u32 cpuid, assoc;
+
+		cpuid	 = cpuid_edx(0x80000005);
+		assoc	 = cpuid >> 16 & 0xff;
+		upperbit = ((cpuid >> 24) << 10) / assoc;
+
+		va_align.mask	  = (upperbit - 1) & PAGE_MASK;
+		va_align.flags    = ALIGN_VA_32 | ALIGN_VA_64;
+	}
+}
+
 static void __cpuinit early_init_amd(struct cpuinfo_x86 *c)
 {
 	early_init_amd_mc(c);

@@ -441,36 +469,6 @@ static void __cpuinit early_init_amd(struct cpuinfo_x86 *c)
 			set_cpu_cap(c, X86_FEATURE_EXTD_APICID);
 	}
 #endif
-
-	/* We need to do the following only once */
-	if (c != &boot_cpu_data)
-		return;
-
-	if (cpu_has(c, X86_FEATURE_CONSTANT_TSC)) {
-
-		if (c->x86 > 0x10 ||
-		    (c->x86 == 0x10 && c->x86_model >= 0x2)) {
-			u64 val;
-
-			rdmsrl(MSR_K7_HWCR, val);
-			if (!(val & BIT(24)))
-				printk(KERN_WARNING FW_BUG "TSC doesn't count "
-					"with P0 frequency!\n");
-		}
-	}
-
-	if (c->x86 == 0x15) {
-		unsigned long upperbit;
-		u32 cpuid, assoc;
-
-		cpuid	 = cpuid_edx(0x80000005);
-		assoc	 = cpuid >> 16 & 0xff;
-		upperbit = ((cpuid >> 24) << 10) / assoc;
-
-		va_align.mask	  = (upperbit - 1) & PAGE_MASK;
-		va_align.flags    = ALIGN_VA_32 | ALIGN_VA_64;
-
-	}
 }
 
 static void __cpuinit init_amd(struct cpuinfo_x86 *c)

@@ -692,6 +690,7 @@ static const struct cpu_dev __cpuinitconst amd_cpu_dev = {
 	.c_size_cache	= amd_size_cache,
 #endif
 	.c_early_init   = early_init_amd,
+	.c_bsp_init	= bsp_init_amd,
 	.c_init		= init_amd,
 	.c_x86_vendor	= X86_VENDOR_AMD,
 };