linux-kernel - Re: [PATCH 2/3] x86/topology: Fix AMD core count

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20160318164101.GG7817@pd.tnic>
Date:	Fri, 18 Mar 2016 17:41:01 +0100
From:	Borislav Petkov <bp@...en8.de>
To:	Peter Zijlstra <peterz@...radead.org>,
	Rui Huang <ray.huang@....com>
Cc:	tglx@...utronix.de, linux-kernel@...r.kernel.org, mingo@...nel.org,
	aherrmann@...e.com, jencce.kernel@...il.com
Subject: Re: [PATCH 2/3] x86/topology: Fix AMD core count

On Fri, Mar 18, 2016 at 04:03:47PM +0100, Peter Zijlstra wrote:
> It turns out AMD gets x86_max_cores wrong when there are compute
> units.
> 
> The issue is that Linux assumes:
> 
> 	nr_logical_cpus = nr_cores * nr_siblings
> 
> But AMD reports its CU unit as 2 cores, but then sets num_smp_siblings
> to 2 as well.
> 
> Cc: Ingo Molnar <mingo@...nel.org>
> Cc: Borislav Petkov <bp@...en8.de>
> Cc: Thomas Gleixner <tglx@...utronix.de>
> Cc: Andreas Herrmann <aherrmann@...e.com>
> Reported-by: Xiong Zhou <jencce.kernel@...il.com>
> Fixes: 1f12e32f4cd5 ("x86/topology: Create logical package id")
> Signed-off-by: Peter Zijlstra (Intel) <peterz@...radead.org>
> Link: http://lkml.kernel.org/r/20160317095220.GO6344@twins.programming.kicks-ass.net
> ---
>  arch/x86/kernel/cpu/amd.c |    8 ++++----
>  arch/x86/kernel/smpboot.c |   11 ++++++-----
>  2 files changed, 10 insertions(+), 9 deletions(-)
> 
> --- a/arch/x86/kernel/cpu/amd.c
> +++ b/arch/x86/kernel/cpu/amd.c
> @@ -313,9 +313,9 @@ static void amd_get_topology(struct cpui
>  		node_id = ecx & 7;
>  
>  		/* get compute unit information */
> -		smp_num_siblings = ((ebx >> 8) & 3) + 1;
> +		cores_per_cu = smp_num_siblings = ((ebx >> 8) & 3) + 1;
> +		c->x86_max_cores /= smp_num_siblings;
>  		c->compute_unit_id = ebx & 0xff;
> -		cores_per_cu += ((ebx >> 8) & 3);
>  	} else if (cpu_has(c, X86_FEATURE_NODEID_MSR)) {
>  		u64 value;
>  
> @@ -331,8 +331,8 @@ static void amd_get_topology(struct cpui
>  		u32 cus_per_node;
>  
>  		set_cpu_cap(c, X86_FEATURE_AMD_DCM);
> -		cores_per_node = c->x86_max_cores / nodes_per_socket;
> -		cus_per_node = cores_per_node / cores_per_cu;
> +		cus_per_node = c->x86_max_cores / nodes_per_socket;
> +		cores_per_node = cus_per_node * cores_per_cu;
>  
>  		/* store NodeID, use llc_shared_map to store sibling info */
>  		per_cpu(cpu_llc_id, cpu) = node_id;

Looks ok to me, however it probably would be prudent if AMD tested it on
a bunch of machines just to make sure we don't break anything else. I'm
thinking F15h and F16h, something big...

Rui, can you find some time to run this one please?

Look at before/after info in /proc/cpuinfo, topology in sysfs and dmesg
before and after might be useful too.

Thanks.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.