Re: [PATCH v2 1/2] x86/amd: Refactor topology extension related code

From: Suravee Suthikulpanit
Date: Sun Jul 23 2017 - 23:28:44 EST


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@xxxxxxx>
---
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