All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 0/2] x86/amd: Refactor and fixup family17h cpu_core_id
@ 2017-07-22  2:00 Suravee Suthikulpanit
  2017-07-22  2:00 ` [PATCH v2 1/2] x86/amd: Refactor topology extension related code Suravee Suthikulpanit
  2017-07-22  2:00 ` [PATCH v2 2/2] x86/amd: Fixup cpu_core_id for family17h downcore configuration Suravee Suthikulpanit
  0 siblings, 2 replies; 7+ messages in thread
From: Suravee Suthikulpanit @ 2017-07-22  2:00 UTC (permalink / raw)
  To: linux-kernel, x86
  Cc: tglx, mingo, hpa, bp, peterz, Yazen.Ghannam, Suravee Suthikulpanit

Changes from V1 (https://lkml.org/lkml/2017/7/20/180)
  * Refactor topology extension logic into __get_topoext() (per Boris)

Suravee Suthikulpanit (2):
  x86/amd: Refactor topology extension related code
  x86/amd: Fixup cpu_core_id for family17h downcore configuration

 arch/x86/kernel/cpu/amd.c | 108 ++++++++++++++++++++++++++++++++--------------
 1 file changed, 76 insertions(+), 32 deletions(-)

-- 
2.7.4

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v2 1/2] x86/amd: Refactor topology extension related code
  2017-07-22  2:00 [PATCH v2 0/2] x86/amd: Refactor and fixup family17h cpu_core_id Suravee Suthikulpanit
@ 2017-07-22  2:00 ` Suravee Suthikulpanit
  2017-07-22 16:12   ` Borislav Petkov
  2017-07-23 19:25   ` kbuild test robot
  2017-07-22  2:00 ` [PATCH v2 2/2] x86/amd: Fixup cpu_core_id for family17h downcore configuration Suravee Suthikulpanit
  1 sibling, 2 replies; 7+ messages in thread
From: Suravee Suthikulpanit @ 2017-07-22  2:00 UTC (permalink / raw)
  To: linux-kernel, x86
  Cc: tglx, mingo, hpa, bp, peterz, Yazen.Ghannam, Suravee Suthikulpanit

Refactoring in preparation for subsequent changes.
There is no functional change.

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 arch/x86/kernel/cpu/amd.c | 79 ++++++++++++++++++++++++++---------------------
 1 file changed, 44 insertions(+), 35 deletions(-)

diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
index bb5abe8..74d8d7c 100644
--- a/arch/x86/kernel/cpu/amd.c
+++ b/arch/x86/kernel/cpu/amd.c
@@ -297,54 +297,63 @@ static int nearby_node(int apicid)
 #endif
 
 /*
- * Fixup core topology information for
- * (1) AMD multi-node processors
- *     Assumption: Number of cores in each internal node is the same.
- * (2) AMD processors supporting compute units
+ * Get topology information via X86_FEATURE_TOPOEXT
  */
-#ifdef CONFIG_SMP
-static void amd_get_topology(struct cpuinfo_x86 *c)
+static void __get_topoext(struct cpuinfo_x86 *c)
 {
-	u8 node_id;
+	u32 eax, ebx, ecx, edx;
 	int cpu = smp_processor_id();
 
-	/* get information required for multi-node processors */
-	if (boot_cpu_has(X86_FEATURE_TOPOEXT)) {
-		u32 eax, ebx, ecx, edx;
+	cpuid(0x8000001e, &eax, &ebx, &ecx, &edx);
 
-		cpuid(0x8000001e, &eax, &ebx, &ecx, &edx);
+	smp_num_siblings = ((ebx >> 8) & 0xff) + 1;
 
-		node_id  = ecx & 0xff;
-		smp_num_siblings = ((ebx >> 8) & 0xff) + 1;
+	if (c->x86 == 0x15)
+		c->cu_id = ebx & 0xff;
 
-		if (c->x86 == 0x15)
-			c->cu_id = ebx & 0xff;
+	if (c->x86 >= 0x17) {
+		c->cpu_core_id = ebx & 0xff;
 
-		if (c->x86 >= 0x17) {
-			c->cpu_core_id = ebx & 0xff;
+		if (smp_num_siblings > 1)
+			c->x86_max_cores /= smp_num_siblings;
+	}
 
-			if (smp_num_siblings > 1)
-				c->x86_max_cores /= smp_num_siblings;
-		}
+	/*
+	 * We may have multiple LLCs if L3 caches exist, so check if we
+	 * have an L3 cache by looking at the L3 cache CPUID leaf.
+	 */
+	if (cpuid_edx(0x80000006)) {
+		if (c->x86 == 0x17) {
+			/*
+			 * LLC is at the core complex level.
+			 * Core complex id is ApicId[3].
+			 */
+			per_cpu(cpu_llc_id, cpu) = c->apicid >> 3;
+		} else {
+			/* LLC is at the node level. */
+			u8 node_id = ecx & 0xff;
 
-		/*
-		 * We may have multiple LLCs if L3 caches exist, so check if we
-		 * have an L3 cache by looking at the L3 cache CPUID leaf.
-		 */
-		if (cpuid_edx(0x80000006)) {
-			if (c->x86 == 0x17) {
-				/*
-				 * LLC is at the core complex level.
-				 * Core complex id is ApicId[3].
-				 */
-				per_cpu(cpu_llc_id, cpu) = c->apicid >> 3;
-			} else {
-				/* LLC is at the node level. */
-				per_cpu(cpu_llc_id, cpu) = node_id;
-			}
+			per_cpu(cpu_llc_id, cpu) = node_id;
 		}
+	}
+}
+
+/*
+ * Fixup core topology information for
+ * (1) AMD multi-node processors
+ *     Assumption: Number of cores in each internal node is the same.
+ * (2) AMD processors supporting compute units
+ */
+#ifdef CONFIG_SMP
+static void amd_get_topology(struct cpuinfo_x86 *c)
+{
+	/* get information required for multi-node processors */
+	if (boot_cpu_has(X86_FEATURE_TOPOEXT)) {
+		__get_topoext(c);
 	} else if (cpu_has(c, X86_FEATURE_NODEID_MSR)) {
+		u8 node_id;
 		u64 value;
+		int cpu = smp_processor_id();
 
 		rdmsrl(MSR_FAM10H_NODE_ID, value);
 		node_id = value & 7;
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH v2 2/2] x86/amd: Fixup cpu_core_id for family17h downcore configuration
  2017-07-22  2:00 [PATCH v2 0/2] x86/amd: Refactor and fixup family17h cpu_core_id Suravee Suthikulpanit
  2017-07-22  2:00 ` [PATCH v2 1/2] x86/amd: Refactor topology extension related code Suravee Suthikulpanit
@ 2017-07-22  2:00 ` Suravee Suthikulpanit
  1 sibling, 0 replies; 7+ messages in thread
From: Suravee Suthikulpanit @ 2017-07-22  2:00 UTC (permalink / raw)
  To: linux-kernel, x86
  Cc: tglx, mingo, hpa, bp, peterz, Yazen.Ghannam, Suravee Suthikulpanit

For family17h, current cpu_core_id is directly taken from the value
CPUID_Fn8000001E_EBX[7:0] (CoreId), which is the physical ID of the
core within a die. However, on system with downcore configuration
(where not all physical cores within a die are available), this could
result in the case where cpu_core_id > (cores_per_node - 1).

Fix up the cpu_core_id by breaking down the bitfields of CoreId,
and calculate relative ID using available topology information.

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 arch/x86/kernel/cpu/amd.c | 73 +++++++++++++++++++++++++++++++++++------------
 1 file changed, 54 insertions(+), 19 deletions(-)

diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
index 74d8d7c..d2fbfdf 100644
--- a/arch/x86/kernel/cpu/amd.c
+++ b/arch/x86/kernel/cpu/amd.c
@@ -301,36 +301,71 @@ static int nearby_node(int apicid)
  */
 static void __get_topoext(struct cpuinfo_x86 *c)
 {
+	u16 l3_nshared = 0;
 	u32 eax, ebx, ecx, edx;
 	int cpu = smp_processor_id();
 
+	if (cpuid_edx(0x80000006)) {
+		cpuid_count(0x8000001d, 3, &eax, &ebx, &ecx, &edx);
+		l3_nshared = ((eax >> 14) & 0xfff) + 1;
+	}
+
 	cpuid(0x8000001e, &eax, &ebx, &ecx, &edx);
 
 	smp_num_siblings = ((ebx >> 8) & 0xff) + 1;
 
-	if (c->x86 == 0x15)
-		c->cu_id = ebx & 0xff;
-
-	if (c->x86 >= 0x17) {
-		c->cpu_core_id = ebx & 0xff;
-
-		if (smp_num_siblings > 1)
-			c->x86_max_cores /= smp_num_siblings;
-	}
+	switch (c->x86) {
+	case 0x17: {
+		u32 tmp, ccx_offset, cpu_offset;
 
-	/*
-	 * We may have multiple LLCs if L3 caches exist, so check if we
-	 * have an L3 cache by looking at the L3 cache CPUID leaf.
-	 */
-	if (cpuid_edx(0x80000006)) {
-		if (c->x86 == 0x17) {
+		/*
+		 * In family 17h, the CPUID_Fn8000001E_EBX[7:0] (CoreId)
+		 * is non-contiguous in downcore and non-SMT cases.
+		 * Fixup the cpu_core_id to be contiguous for cores within
+		 * the die.
+		 */
+		tmp = ebx & 0xff;
+		if (smp_num_siblings == 1) {
 			/*
-			 * LLC is at the core complex level.
-			 * Core complex id is ApicId[3].
+			 * CoreId bit-encoding for SMT-disabled
+			 * [7:4] : die
+			 * [3]   : ccx
+			 * [2:0] : core
 			 */
-			per_cpu(cpu_llc_id, cpu) = c->apicid >> 3;
+			ccx_offset = ((tmp >> 3) & 1) * l3_nshared;
+			cpu_offset = tmp & 7;
 		} else {
-			/* LLC is at the node level. */
+			/*
+			 * CoreId bit-encoding for SMT-enabled
+			 * [7:3] : die
+			 * [2]   : ccx
+			 * [1:0] : core
+			 */
+			ccx_offset = ((tmp >> 2) & 1) * l3_nshared /
+				       smp_num_siblings;
+			cpu_offset = tmp & 3;
+			c->x86_max_cores /= smp_num_siblings;
+
+		}
+		c->cpu_core_id = ccx_offset + cpu_offset;
+
+		/*
+		 * Family17h L3 cache (LLC) is at Core Complex (CCX).
+		 * There could be multiple CCXs in a node.
+		 * CCX ID is ApicId[3].
+		 */
+		per_cpu(cpu_llc_id, cpu) = c->apicid >> 3;
+
+		pr_debug("Fixup coreid:%#x to cpu_core_id:%#x\n",
+			 tmp, c->cpu_core_id);
+		break;
+	}
+	case 0x15:
+		c->cu_id = ebx & 0xff;
+		/* Follow through */
+	default:
+		/* LLC is default to L3, which generally per-node */
+		if (l3_nshared > 0) {
 			u8 node_id = ecx & 0xff;
 
 			per_cpu(cpu_llc_id, cpu) = node_id;
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH v2 1/2] x86/amd: Refactor topology extension related code
  2017-07-22  2:00 ` [PATCH v2 1/2] x86/amd: Refactor topology extension related code Suravee Suthikulpanit
@ 2017-07-22 16:12   ` Borislav Petkov
  2017-07-24  3:28     ` Suravee Suthikulpanit
  2017-07-23 19:25   ` kbuild test robot
  1 sibling, 1 reply; 7+ messages in thread
From: Borislav Petkov @ 2017-07-22 16:12 UTC (permalink / raw)
  To: Suravee Suthikulpanit
  Cc: linux-kernel, x86, tglx, mingo, hpa, peterz, Yazen.Ghannam

On Fri, Jul 21, 2017 at 09:00:38PM -0500, Suravee Suthikulpanit wrote:
> Refactoring in preparation for subsequent changes.
> There is no functional change.
> 
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> ---
>  arch/x86/kernel/cpu/amd.c | 79 ++++++++++++++++++++++++++---------------------
>  1 file changed, 44 insertions(+), 35 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
> index bb5abe8..74d8d7c 100644
> --- a/arch/x86/kernel/cpu/amd.c
> +++ b/arch/x86/kernel/cpu/amd.c
> @@ -297,54 +297,63 @@ static int nearby_node(int apicid)
>  #endif
>  
>  /*
> - * Fixup core topology information for
> - * (1) AMD multi-node processors
> - *     Assumption: Number of cores in each internal node is the same.
> - * (2) AMD processors supporting compute units
> + * Get topology information via X86_FEATURE_TOPOEXT
>   */
> -#ifdef CONFIG_SMP
> -static void amd_get_topology(struct cpuinfo_x86 *c)
> +static void __get_topoext(struct cpuinfo_x86 *c)
>  {
> -	u8 node_id;
> +	u32 eax, ebx, ecx, edx;
>  	int cpu = smp_processor_id();
>  
> -	/* get information required for multi-node processors */
> -	if (boot_cpu_has(X86_FEATURE_TOPOEXT)) {
> -		u32 eax, ebx, ecx, edx;
> +	cpuid(0x8000001e, &eax, &ebx, &ecx, &edx);
>  
> -		cpuid(0x8000001e, &eax, &ebx, &ecx, &edx);
> +	smp_num_siblings = ((ebx >> 8) & 0xff) + 1;
>  
> -		node_id  = ecx & 0xff;

When reviewers ask you about a preparatory cleanup patch, you don't
sneak in changes in it - you *only* *move* the code so that the change
is *absolutely* comprehensible. Ontop you do changes. Don't tell me you
didn't know that!

Try again.

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2 1/2] x86/amd: Refactor topology extension related code
  2017-07-22  2:00 ` [PATCH v2 1/2] x86/amd: Refactor topology extension related code Suravee Suthikulpanit
  2017-07-22 16:12   ` Borislav Petkov
@ 2017-07-23 19:25   ` kbuild test robot
  1 sibling, 0 replies; 7+ messages in thread
From: kbuild test robot @ 2017-07-23 19:25 UTC (permalink / raw)
  To: Suravee Suthikulpanit
  Cc: kbuild-all, linux-kernel, x86, tglx, mingo, hpa, bp, peterz,
	Yazen.Ghannam, Suravee Suthikulpanit

[-- Attachment #1: Type: text/plain, Size: 5541 bytes --]

Hi Suravee,

[auto build test ERROR on tip/x86/core]
[also build test ERROR on v4.13-rc1 next-20170721]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Suravee-Suthikulpanit/x86-amd-Refactor-topology-extension-related-code/20170724-024344
config: x86_64-randconfig-x007-201730 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   arch/x86/kernel/cpu/amd.c: In function '__get_topoext':
>> arch/x86/kernel/cpu/amd.c:309:19: error: lvalue required as left operand of assignment
     smp_num_siblings = ((ebx >> 8) & 0xff) + 1;
                      ^
   At top level:
   arch/x86/kernel/cpu/amd.c:302:13: warning: '__get_topoext' defined but not used [-Wunused-function]
    static void __get_topoext(struct cpuinfo_x86 *c)
                ^~~~~~~~~~~~~

vim +309 arch/x86/kernel/cpu/amd.c

^1da177e arch/i386/kernel/cpu/amd.c Linus Torvalds        2005-04-16  298  
11fdd252 arch/x86/kernel/cpu/amd.c  Yinghai Lu            2008-09-07  299  /*
1c2d1f62 arch/x86/kernel/cpu/amd.c  Suravee Suthikulpanit 2017-07-21  300   * Get topology information via X86_FEATURE_TOPOEXT
4a376ec3 arch/x86/kernel/cpu/amd.c  Andreas Herrmann      2009-09-03  301   */
1c2d1f62 arch/x86/kernel/cpu/amd.c  Suravee Suthikulpanit 2017-07-21  302  static void __get_topoext(struct cpuinfo_x86 *c)
4a376ec3 arch/x86/kernel/cpu/amd.c  Andreas Herrmann      2009-09-03  303  {
79a8b9aa arch/x86/kernel/cpu/amd.c  Borislav Petkov       2017-02-05  304  	u32 eax, ebx, ecx, edx;
1c2d1f62 arch/x86/kernel/cpu/amd.c  Suravee Suthikulpanit 2017-07-21  305  	int cpu = smp_processor_id();
6057b4d3 arch/x86/kernel/cpu/amd.c  Andreas Herrmann      2010-09-30  306  
79a8b9aa arch/x86/kernel/cpu/amd.c  Borislav Petkov       2017-02-05  307  	cpuid(0x8000001e, &eax, &ebx, &ecx, &edx);
79a8b9aa arch/x86/kernel/cpu/amd.c  Borislav Petkov       2017-02-05  308  
79a8b9aa arch/x86/kernel/cpu/amd.c  Borislav Petkov       2017-02-05 @309  	smp_num_siblings = ((ebx >> 8) & 0xff) + 1;
79a8b9aa arch/x86/kernel/cpu/amd.c  Borislav Petkov       2017-02-05  310  
79a8b9aa arch/x86/kernel/cpu/amd.c  Borislav Petkov       2017-02-05  311  	if (c->x86 == 0x15)
79a8b9aa arch/x86/kernel/cpu/amd.c  Borislav Petkov       2017-02-05  312  		c->cu_id = ebx & 0xff;
b6a50cdd arch/x86/kernel/cpu/amd.c  Yazen Ghannam         2016-11-08  313  
08b25963 arch/x86/kernel/cpu/amd.c  Yazen Ghannam         2017-02-05  314  	if (c->x86 >= 0x17) {
08b25963 arch/x86/kernel/cpu/amd.c  Yazen Ghannam         2017-02-05  315  		c->cpu_core_id = ebx & 0xff;
08b25963 arch/x86/kernel/cpu/amd.c  Yazen Ghannam         2017-02-05  316  
08b25963 arch/x86/kernel/cpu/amd.c  Yazen Ghannam         2017-02-05  317  		if (smp_num_siblings > 1)
08b25963 arch/x86/kernel/cpu/amd.c  Yazen Ghannam         2017-02-05  318  			c->x86_max_cores /= smp_num_siblings;
08b25963 arch/x86/kernel/cpu/amd.c  Yazen Ghannam         2017-02-05  319  	}
08b25963 arch/x86/kernel/cpu/amd.c  Yazen Ghannam         2017-02-05  320  
b6a50cdd arch/x86/kernel/cpu/amd.c  Yazen Ghannam         2016-11-08  321  	/*
b6a50cdd arch/x86/kernel/cpu/amd.c  Yazen Ghannam         2016-11-08  322  	 * We may have multiple LLCs if L3 caches exist, so check if we
b6a50cdd arch/x86/kernel/cpu/amd.c  Yazen Ghannam         2016-11-08  323  	 * have an L3 cache by looking at the L3 cache CPUID leaf.
b6a50cdd arch/x86/kernel/cpu/amd.c  Yazen Ghannam         2016-11-08  324  	 */
b6a50cdd arch/x86/kernel/cpu/amd.c  Yazen Ghannam         2016-11-08  325  	if (cpuid_edx(0x80000006)) {
b6a50cdd arch/x86/kernel/cpu/amd.c  Yazen Ghannam         2016-11-08  326  		if (c->x86 == 0x17) {
b6a50cdd arch/x86/kernel/cpu/amd.c  Yazen Ghannam         2016-11-08  327  			/*
b6a50cdd arch/x86/kernel/cpu/amd.c  Yazen Ghannam         2016-11-08  328  			 * LLC is at the core complex level.
b6a50cdd arch/x86/kernel/cpu/amd.c  Yazen Ghannam         2016-11-08  329  			 * Core complex id is ApicId[3].
b6a50cdd arch/x86/kernel/cpu/amd.c  Yazen Ghannam         2016-11-08  330  			 */
b6a50cdd arch/x86/kernel/cpu/amd.c  Yazen Ghannam         2016-11-08  331  			per_cpu(cpu_llc_id, cpu) = c->apicid >> 3;
b6a50cdd arch/x86/kernel/cpu/amd.c  Yazen Ghannam         2016-11-08  332  		} else {
b6a50cdd arch/x86/kernel/cpu/amd.c  Yazen Ghannam         2016-11-08  333  			/* LLC is at the node level. */
1c2d1f62 arch/x86/kernel/cpu/amd.c  Suravee Suthikulpanit 2017-07-21  334  			u8 node_id = ecx & 0xff;
1c2d1f62 arch/x86/kernel/cpu/amd.c  Suravee Suthikulpanit 2017-07-21  335  
b6a50cdd arch/x86/kernel/cpu/amd.c  Yazen Ghannam         2016-11-08  336  			per_cpu(cpu_llc_id, cpu) = node_id;
b6a50cdd arch/x86/kernel/cpu/amd.c  Yazen Ghannam         2016-11-08  337  		}
b6a50cdd arch/x86/kernel/cpu/amd.c  Yazen Ghannam         2016-11-08  338  	}
1c2d1f62 arch/x86/kernel/cpu/amd.c  Suravee Suthikulpanit 2017-07-21  339  }
1c2d1f62 arch/x86/kernel/cpu/amd.c  Suravee Suthikulpanit 2017-07-21  340  

:::::: The code at line 309 was first introduced by commit
:::::: 79a8b9aa388b0620cc1d525d7c0f0d9a8a85e08e x86/CPU/AMD: Bring back Compute Unit ID

:::::: TO: Borislav Petkov <bp@suse.de>
:::::: CC: Ingo Molnar <mingo@kernel.org>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 24471 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2 1/2] x86/amd: Refactor topology extension related code
  2017-07-22 16:12   ` Borislav Petkov
@ 2017-07-24  3:28     ` Suravee Suthikulpanit
  2017-07-24  7:15       ` Borislav Petkov
  0 siblings, 1 reply; 7+ messages in thread
From: Suravee Suthikulpanit @ 2017-07-24  3:28 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: linux-kernel, x86, tglx, mingo, hpa, peterz, Yazen.Ghannam

Boris,

On 7/22/17 23:12, Borislav Petkov wrote:
> On Fri, Jul 21, 2017 at 09:00:38PM -0500, Suravee Suthikulpanit wrote:
>> Refactoring in preparation for subsequent changes.
>> There is no functional change.
>>
>> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>> ---
>>  arch/x86/kernel/cpu/amd.c | 79 ++++++++++++++++++++++++++---------------------
>>  1 file changed, 44 insertions(+), 35 deletions(-)
>>
>> diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
>> index bb5abe8..74d8d7c 100644
>> --- a/arch/x86/kernel/cpu/amd.c
>> +++ b/arch/x86/kernel/cpu/amd.c
>> @@ -297,54 +297,63 @@ static int nearby_node(int apicid)
>>  #endif
>>
>>  /*
>> - * Fixup core topology information for
>> - * (1) AMD multi-node processors
>> - *     Assumption: Number of cores in each internal node is the same.
>> - * (2) AMD processors supporting compute units
>> + * Get topology information via X86_FEATURE_TOPOEXT
>>   */
>> -#ifdef CONFIG_SMP
>> -static void amd_get_topology(struct cpuinfo_x86 *c)
>> +static void __get_topoext(struct cpuinfo_x86 *c)
>>  {
>> -	u8 node_id;
>> +	u32 eax, ebx, ecx, edx;
>>  	int cpu = smp_processor_id();
>>
>> -	/* get information required for multi-node processors */
>> -	if (boot_cpu_has(X86_FEATURE_TOPOEXT)) {
>> -		u32 eax, ebx, ecx, edx;
>> +	cpuid(0x8000001e, &eax, &ebx, &ecx, &edx);
>>
>> -		cpuid(0x8000001e, &eax, &ebx, &ecx, &edx);
>> +	smp_num_siblings = ((ebx >> 8) & 0xff) + 1;
>>
>> -		node_id  = ecx & 0xff;
>
> When reviewers ask you about a preparatory cleanup patch, you don't
> sneak in changes in it - you *only* *move* the code so that the change
> is *absolutely* comprehensible. Ontop you do changes. Don't tell me you
> didn't know that!

I know that we should not sneak in change. I might have missed something here.

Are you referring to the part that I moved the "node_id = ecx & 0xff" from the 
top level of the function to inside the "if/else" logic where it is the only 
place that used within this new refactored __get_topoext() and there is nothing 
changed functionally? If that's really the case, I'll fix it.

Thanks,
Suravee

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2 1/2] x86/amd: Refactor topology extension related code
  2017-07-24  3:28     ` Suravee Suthikulpanit
@ 2017-07-24  7:15       ` Borislav Petkov
  0 siblings, 0 replies; 7+ messages in thread
From: Borislav Petkov @ 2017-07-24  7:15 UTC (permalink / raw)
  To: Suravee Suthikulpanit
  Cc: linux-kernel, x86, tglx, mingo, hpa, peterz, Yazen.Ghannam

On Mon, Jul 24, 2017 at 10:28:34AM +0700, Suravee Suthikulpanit wrote:
> Are you referring to the part that I moved the "node_id = ecx & 0xff" ...

Yes, that's what I'm referring to. Btw, I went and did it myself, now
look at my diff below. Now that diff is pretty straight-forward - stuff
is picked from one place and put somewhere else. Exactly like it should
be done.

Your diff is intermixing the new and old function because, *since*
you're touching stuff in there, the diff algorithm can't really detect
the move. Or maybe because you're using an old git but it doesn't look
like it because your patch applied gives the same "intermixed" diff
here.

Also, note that I've made the new function:

+static void __get_topoext(struct cpuinfo_x86 *c, int cpu)

so that you don't have to do smp_processor_id() twice.

Now, after you get it this way, you can start doing your cleanups and
improvements ontop because a diff like that is very easy to review.

Thanks.

diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
index 110ca5d2bb87..ee55f811b8c6 100644
--- a/arch/x86/kernel/cpu/amd.c
+++ b/arch/x86/kernel/cpu/amd.c
@@ -297,13 +297,55 @@ static int nearby_node(int apicid)
 }
 #endif
 
+#ifdef CONFIG_SMP
+
+/*
+ * Get topology information via X86_FEATURE_TOPOEXT.
+ */
+static void __get_topoext(struct cpuinfo_x86 *c, int cpu)
+{
+	u32 eax, ebx, ecx, edx;
+	u8 node_id;
+
+	cpuid(0x8000001e, &eax, &ebx, &ecx, &edx);
+
+	node_id  = ecx & 0xff;
+	smp_num_siblings = ((ebx >> 8) & 0xff) + 1;
+
+	if (c->x86 == 0x15)
+		c->cu_id = ebx & 0xff;
+
+	if (c->x86 >= 0x17) {
+		c->cpu_core_id = ebx & 0xff;
+
+		if (smp_num_siblings > 1)
+			c->x86_max_cores /= smp_num_siblings;
+	}
+
+	/*
+	 * We may have multiple LLCs if L3 caches exist, so check if we
+	 * have an L3 cache by looking at the L3 cache CPUID leaf.
+	 */
+	if (cpuid_edx(0x80000006)) {
+		if (c->x86 == 0x17) {
+			/*
+			 * LLC is at the core complex level.
+			 * Core complex id is ApicId[3].
+			 */
+			per_cpu(cpu_llc_id, cpu) = c->apicid >> 3;
+		} else {
+			/* LLC is at the node level. */
+			per_cpu(cpu_llc_id, cpu) = node_id;
+		}
+	}
+}
+
 /*
  * Fixup core topology information for
  * (1) AMD multi-node processors
  *     Assumption: Number of cores in each internal node is the same.
  * (2) AMD processors supporting compute units
  */
-#ifdef CONFIG_SMP
 static void amd_get_topology(struct cpuinfo_x86 *c)
 {
 	u8 node_id;
@@ -311,39 +353,7 @@ static void amd_get_topology(struct cpuinfo_x86 *c)
 
 	/* get information required for multi-node processors */
 	if (boot_cpu_has(X86_FEATURE_TOPOEXT)) {
-		u32 eax, ebx, ecx, edx;
-
-		cpuid(0x8000001e, &eax, &ebx, &ecx, &edx);
-
-		node_id  = ecx & 0xff;
-		smp_num_siblings = ((ebx >> 8) & 0xff) + 1;
-
-		if (c->x86 == 0x15)
-			c->cu_id = ebx & 0xff;
-
-		if (c->x86 >= 0x17) {
-			c->cpu_core_id = ebx & 0xff;
-
-			if (smp_num_siblings > 1)
-				c->x86_max_cores /= smp_num_siblings;
-		}
-
-		/*
-		 * We may have multiple LLCs if L3 caches exist, so check if we
-		 * have an L3 cache by looking at the L3 cache CPUID leaf.
-		 */
-		if (cpuid_edx(0x80000006)) {
-			if (c->x86 == 0x17) {
-				/*
-				 * LLC is at the core complex level.
-				 * Core complex id is ApicId[3].
-				 */
-				per_cpu(cpu_llc_id, cpu) = c->apicid >> 3;
-			} else {
-				/* LLC is at the node level. */
-				per_cpu(cpu_llc_id, cpu) = node_id;
-			}
-		}
+		__get_topoext(c, cpu);
 	} else if (cpu_has(c, X86_FEATURE_NODEID_MSR)) {
 		u64 value;


-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

^ permalink raw reply related	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2017-07-24  7:16 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-07-22  2:00 [PATCH v2 0/2] x86/amd: Refactor and fixup family17h cpu_core_id Suravee Suthikulpanit
2017-07-22  2:00 ` [PATCH v2 1/2] x86/amd: Refactor topology extension related code Suravee Suthikulpanit
2017-07-22 16:12   ` Borislav Petkov
2017-07-24  3:28     ` Suravee Suthikulpanit
2017-07-24  7:15       ` Borislav Petkov
2017-07-23 19:25   ` kbuild test robot
2017-07-22  2:00 ` [PATCH v2 2/2] x86/amd: Fixup cpu_core_id for family17h downcore configuration Suravee Suthikulpanit

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.