linux-kernel - Re: [PATCH] x86/centaur: report correct CPU/cache topology

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>] [day] [month] [year] [list]

Message-ID: <000001d3dc6f$8bd810b0$a3883210$@zhaoxin.com>
Date:   Wed, 25 Apr 2018 16:29:36 +0800
From:   David Wang <davidwang@...oxin.com>
To:     'Thomas Gleixner' <tglx@...utronix.de>
CC:     <mingo@...hat.com>, <hpa@...or.com>, <mingo@...nel.org>,
        <x86@...nel.org>, <linux-kernel@...r.kernel.org>,
        <brucechang@...-alliance.com>, <cooperyan@...oxin.com>,
        <qiyuanwang@...oxin.com>, <benjaminpan@...tech.com>,
        <lukelin@...cpu.com>, <timguo@...oxin.com>
Subject: Re: [PATCH] x86/centaur: report correct CPU/cache topology



> -----Original Mail-----
>Sender: Thomas Gleixner [mailto:tglx@...utronix.de]
> Time: 2018年4月17日 18:16
> Receiver: David Wang <davidwang@...oxin.com>
> CC: mingo@...hat.com; hpa@...or.com; mingo@...nel.org;
> x86@...nel.org; linux-kernel@...r.kernel.org; brucechang@...-
> alliance.com; cooperyan@...oxin.com; qiyuanwang@...oxin.com;
> benjaminpan@...tech.com; lukelin@...cpu.com; timguo@...oxin.com
> Subject: Re: [PATCH] x86/centaur: report correct CPU/cache topology
> 
> On Wed, 4 Apr 2018, David Wang wrote:
> 
> > This patch is used to support multi-core Centaur CPU. After using this
> > patch, we can get correct CPU topology and correct cache topology.
> 
> David. This changelog is pretty useless. First of all, please do not use
'This
> patch ..'. We all know already that this is a patch.
> Documentation/process/submitting-patches.rst has a good explanation
> about writing changelogs.
> 
> The changelog should explain why it does something. Let me give you an
> example:
> 
>   Centaur CPUs enumerate the cache topology in the same way as Intel CPUs,
>   but the functionality is unused so far. The Centaur init code also
misses
>   to initialize x86_cpuinfo::max_cores so the CPU topology cannot be
>   desribed correctly,
> 
>   Initialize x86_cpuinfo::max_cores and invoke init_intel_cacheinfo() to
>   make CPU and cache topology information available and correct.
> 
> See? I'm neither using 'this patch' nor 'We/I' as I'm not impersonatimg
the
> code. It's all factual instead.

	OK. I got it.

> > Signed-off-by: David Wang <davidwang@...oxin.com>
> > ---
> >  arch/x86/kernel/cpu/centaur.c | 20 ++++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> >
> > diff --git a/arch/x86/kernel/cpu/centaur.c
> > b/arch/x86/kernel/cpu/centaur.c index e5ec0f1..713e4db 100644
> > --- a/arch/x86/kernel/cpu/centaur.c
> > +++ b/arch/x86/kernel/cpu/centaur.c
> > @@ -112,6 +112,19 @@ static void early_init_centaur(struct cpuinfo_x86
> *c)
> >  	}
> >  }
> >
> > +static int centaur_num_cpu_cores(struct cpuinfo_x86 *c) {
> > +	unsigned int eax, ebx, ecx, edx;
> > +
> > +	if (c->cpuid_level < 4)
> > +		return 1;
> > +	cpuid_count(4, 0, &eax, &ebx, &ecx, &edx);
> > +	if (eax & 0x1f)
> > +		return (eax >> 26) + 1;
> > +	else
> > +		return 1;
> 
> This is a bad copy of intel_num_cpu_cores(). See for the subtle
difference.
> Please rename the intel function and move it to common.c
> 
	OK. I will adjust.

> >  static void init_centaur(struct cpuinfo_x86 *c)  {  #ifdef
> > CONFIG_X86_32 @@ -128,6 +141,13 @@ static void init_centaur(struct
> > cpuinfo_x86 *c)
> >  	clear_cpu_cap(c, 0*32+31);
> >  #endif
> >  	early_init_centaur(c);
> > +
> > +	init_intel_cacheinfo(c);
> > +	c->x86_max_cores = centaur_num_cpu_cores(c); #ifdef
> CONFIG_X86_32
> > +	detect_ht(c);
> > +#endif
> 
> Can you please create a stub inline of detect_ht() for the !32bit case and
get
> rid of these #ifdefs in the code. That wants to be a separate patch which
also
> cleans up the existing call sites.

	The detect_ht() function will also be called by identify_cpu()
function for
	!32bit case. So, I think it means that all 64-bit CPUs can use
detect_ht(),
	but only some 32-bit CPUs can use detect_ht().
	Please correct me, if I'm wrong.
> 
> Thanks,
> 
> 	tglx

	Thanks,

---
David