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

[Like Xu]

On 2019/2/24 18:04, Brice Goglin wrote:
> 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.

I don't think the intel MCP topolohgy has this global uniqueness 
assumption for die_id according to CPUID.1F SDM.

> 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