This patch is used to support multi-core Centaur CPU. After using this
patch, we can get correct CPU topology and correct cache topology.
Signed-off-by: David Wang <[email protected]>
---
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;
+}
+
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
+
switch (c->x86) {
#ifdef CONFIG_X86_32
case 5:
--
1.9.1
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.
> Signed-off-by: David Wang <[email protected]>
> ---
> 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
> 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.
Thanks,
tglx
> -----Original Mail-----
> Sender: Thomas Gleixner [mailto:[email protected]]
> Time : 2018/4/17 18:16
> Receiver: David Wang <[email protected]>
> CC: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; brucechang@via-
> alliance.com; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]
> 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.
> > Signed-off-by: David Wang <[email protected]>
> > ---
> > 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
>
> > 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.
>
> Thanks,
>
> tglx
I will send patch v2 to solve all problems you listed.
Thanks,
---
David
> -----Original Mail-----
>Sender: Thomas Gleixner [mailto:[email protected]]
> Time: 2018??4??17?? 18:16
> Receiver: David Wang <[email protected]>
> CC: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; brucechang@via-
> alliance.com; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]
> 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 <[email protected]>
> > ---
> > 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