linux-kernel - RE: [PATCH 03/11] x86 topology: Add CPUID.1F multi-die/package support

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: <1A7043D5F58CCB44A599DFD55ED4C9485C5BA396@FMSMSX126.amr.corp.intel.com>
Date:   Mon, 25 Feb 2019 08:08:24 +0000
From:   "Brown, Len" <len.brown@...el.com>
To:     Brice Goglin <Brice.Goglin@...ia.fr>, Len Brown <lenb@...nel.org>,
        "x86@...nel.org" <x86@...nel.org>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
        Like Xu <like.xu@...ux.intel.com>
Subject: RE: [PATCH 03/11] x86 topology: Add CPUID.1F multi-die/package
 support

Hi Brice,

Yes, you re-discovered the bug that Kan Liang pointed out last week.

I have updated this patch set, and the latest for testing is in my git tree on kernel.org or

https://github.com/lenb/linux.git x86

Note that I took your advice and left the core_siblings with its original definition,
And created package_threads as a synonym.  I will e-mail out the patch set again
When I do 2 more things:

1. add core_threads map to sysfs
2. replace unique_die_id with logical_die_id -- turns out it is useful for same reason as logical_package_id.

Thanks,
-Len




-----Original Message-----
From: Brice Goglin [mailto:Brice.Goglin@...ia.fr] 
Sent: Sunday, February 24, 2019 5:04 AM
To: Len Brown <lenb@...nel.org>; x86@...nel.org
Cc: linux-kernel@...r.kernel.org; Brown, Len <len.brown@...el.com>; linux-doc@...r.kernel.org; Like Xu <like.xu@...ux.intel.com>
Subject: Re: [PATCH 03/11] x86 topology: Add CPUID.1F multi-die/package support

Le 19/02/2019 à 04:40, Len Brown a écrit :
> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c 
> index ccd1f2a8e557..4250a87f57db 100644
> --- a/arch/x86/kernel/smpboot.c
> +++ b/arch/x86/kernel/smpboot.c
> @@ -393,6 +393,7 @@ static bool match_smt(struct cpuinfo_x86 *c, struct cpuinfo_x86 *o)
>  		int cpu1 = c->cpu_index, cpu2 = o->cpu_index;
>  
>  		if (c->phys_proc_id == o->phys_proc_id &&
> +		    c->cpu_die_id == o->cpu_die_id &&
>  		    per_cpu(cpu_llc_id, cpu1) == per_cpu(cpu_llc_id, cpu2)) {
>  			if (c->cpu_core_id == o->cpu_core_id)
>  				return topology_sane(c, o, "smt"); @@ -404,6 +405,7 @@ static 
> bool match_smt(struct cpuinfo_x86 *c, struct cpuinfo_x86 *o)
>  		}
>  
>  	} else if (c->phys_proc_id == o->phys_proc_id &&
> +		   c->cpu_die_id == o->cpu_die_id &&
>  		   c->cpu_core_id == o->cpu_core_id) {
>  		return topology_sane(c, o, "smt");
>  	}
> @@ -461,7 +463,7 @@ static bool match_llc(struct cpuinfo_x86 *c, struct cpuinfo_x86 *o)
>   */
>  static bool match_die(struct cpuinfo_x86 *c, struct cpuinfo_x86 *o)  
> {
> -	if (c->phys_proc_id == o->phys_proc_id)
> +	if (c->cpu_die_id == o->cpu_die_id)
>  		return true;
>  	return false;
>  }

Hello Len,

I am testing your patches in a VM with CPUID.1f QEMU patches (author Like Xu is CC'ed), booted to get 2 packages with 1 NUMA node each and 2 dies each:

-smp cpus=16,threads=2,cores=2,dies=2,sockets=2 -numa node,cpus=0-7,nodeid=0 -numa node,cpus=8-15,nodeid=1

sysfs files expose wrong information:
core_siblings contains threads of the local die AND of die with same number in other processors, eg cpu 0-3 and 8-11 instead of 0-3 only.

The issue is that you seem to assume that die ids will always be unique across multiple packages.
Is this a valid assumption? If so, QEMU patches should be fixed.
If not, I fixed the issue by changing match_die() to check both phys_proc_id and cpu_die_id:

static bool match_die(struct cpuinfo_x86 *c, struct cpuinfo_x86 *o) {
	if (c->phys_proc_id == o->phys_proc_id && c->cpu_die_id == o->cpu_die_id)
 		return true;
	return false;
}

Thanks
Brice