> -----Original Mail-----
>Sender: Thomas Gleixner [mailto:t...@linutronix.de]
> Time: 2018年4月17日 18:16
> Receiver: David Wang <davidw...@zhaoxin.com>
> CC: mi...@redhat.com; h...@zytor.com; mi...@kernel.org;
> x...@kernel.org; linux-kernel@vger.kernel.org; brucechang@via-
> alliance.com; cooper...@zhaoxin.com; qiyuanw...@zhaoxin.com;
> benjamin...@viatech.com; luke...@viacpu.com; tim...@zhaoxin.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 <davidw...@zhaoxin.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
