[PATCH v2] x86/centaur: report correct CPU/cache topology

[PATCH v2] x86/centaur: report correct CPU/cache topology

[David Wang]

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_info::max_cores, so
the CPU topology can't be described correctly.

Initialize x86_cpuinfo:max_core and invoke init_intel_cacheinfo() to
make CPU and cache topology information avaliable and correct.

Signed-off-by: David Wang <davidwang@zhaoxin.com>

Changes from v1 to v2:
*1 replace centaur_num_cpu_cores with early_init_centaur_mc according to 
suggestions from tglx;
*2 call cpu_detect_cache_sizes when init_intel_cacheinfo returns 0. For
some very old Centaur CPUs can't support the mechanisms defined in init_
intel_cacheinfo.
 
---
 arch/x86/kernel/cpu/centaur.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/arch/x86/kernel/cpu/centaur.c b/arch/x86/kernel/cpu/centaur.c
index 80d5110..367540b 100644
--- a/arch/x86/kernel/cpu/centaur.c
+++ b/arch/x86/kernel/cpu/centaur.c
@@ -96,8 +96,25 @@ enum {
 		EAMD3D		= 1<<20,
 };
 
+static void early_init_centaur_mc(struct cpuinfo_x86 *c)
+{
+#ifdef CONFIG_SMP
+	unsigned int eax, ebx, ecx, edx;
+
+	if (c->cpuid_level < 4)
+		return;
+
+	cpuid_count(4, 0, &eax, &ebx, &ecx, &edx);
+	if (eax & 0x1f)
+		c->x86_max_cores = (eax >> 26) + 1;
+	else
+		return;
+#endif
+}
+
 static void early_init_centaur(struct cpuinfo_x86 *c)
 {
+	early_init_centaur_mc(c);
 	switch (c->x86) {
 #ifdef CONFIG_X86_32
 	case 5:
@@ -146,6 +163,7 @@ static void centaur_detect_vmx_virtcap(struct cpuinfo_x86 *c)
 
 static void init_centaur(struct cpuinfo_x86 *c)
 {
+	unsigned int l2 = 0;
 #ifdef CONFIG_X86_32
 	char *name;
 	u32  fcr_set = 0;
@@ -161,6 +179,17 @@ static void init_centaur(struct cpuinfo_x86 *c)
 #endif
 	early_init_centaur(c);
 
+	l2 = init_intel_cacheinfo(c);
+
+	/* Detect legacy cache sizes if init_intel_cacheinfo did not */
+	if (l2 == 0) {
+		cpu_detect_cache_sizes(c);
+	}
+
+#ifdef CONFIG_X86_32
+	detect_ht(c);
+#endif
+
 	if (c->cpuid_level > 9) {
 		unsigned int eax = cpuid_eax(10);
 
-- 
1.9.1

Re: [PATCH v2] x86/centaur: report correct CPU/cache topology

[Thomas Gleixner]

On Wed, 25 Apr 2018, David Wang wrote:
>  
> +static void early_init_centaur_mc(struct cpuinfo_x86 *c)
> +{
> +#ifdef CONFIG_SMP
> +	unsigned int eax, ebx, ecx, edx;
> +
> +	if (c->cpuid_level < 4)
> +		return;
> +
> +	cpuid_count(4, 0, &eax, &ebx, &ecx, &edx);
> +	if (eax & 0x1f)
> +		c->x86_max_cores = (eax >> 26) + 1;
> +	else
> +		return;
> +#endif
> +}

My review comment from last time still stands:

> > 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

In other words:

Make a patch which moves intel_num_cpu_cores() into common.c. Rename the
function into something like detect_num_cpu_cores() and fix up the call
site in intel.c.

Then add your patch and use the very same function.

> +
>  static void early_init_centaur(struct cpuinfo_x86 *c)
>  {
> +	early_init_centaur_mc(c);
>  	switch (c->x86) {
>  #ifdef CONFIG_X86_32
>  	case 5:
> @@ -146,6 +163,7 @@ static void centaur_detect_vmx_virtcap(struct cpuinfo_x86 *c)
>  
>  static void init_centaur(struct cpuinfo_x86 *c)
>  {
> +	unsigned int l2 = 0;
>  #ifdef CONFIG_X86_32
>  	char *name;
>  	u32  fcr_set = 0;
> @@ -161,6 +179,17 @@ static void init_centaur(struct cpuinfo_x86 *c)
>  #endif
>  	early_init_centaur(c);
>  
> +	l2 = init_intel_cacheinfo(c);
> +
> +	/* Detect legacy cache sizes if init_intel_cacheinfo did not */
> +	if (l2 == 0) {
> +		cpu_detect_cache_sizes(c);
> +	}

Aside of the pointless parentheses, this really wants to be cleaned up as
well.

init_intel_cacheinfo() is called from the intel init code and it does the
same silly thing.

So the right thing to do is in a separate patch first

--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -679,12 +679,6 @@ static void init_intel(struct cpuinfo_x8
 
 	l2 = init_intel_cacheinfo(c);
 
-	/* Detect legacy cache sizes if init_intel_cacheinfo did not */
-	if (l2 == 0) {
-		cpu_detect_cache_sizes(c);
-		l2 = c->x86_cache_size;
-	}
-
 	if (c->cpuid_level > 9) {
 		unsigned eax = cpuid_eax(10);
 		/* Check for version and the number of counters */
--- a/arch/x86/kernel/cpu/intel_cacheinfo.c
+++ b/arch/x86/kernel/cpu/intel_cacheinfo.c
@@ -802,6 +802,11 @@ unsigned int init_intel_cacheinfo(struct
 
 	c->x86_cache_size = l3 ? l3 : (l2 ? l2 : (l1i+l1d));
 
+	/* Detect legacy cache sizes if the above did not work */
+	if (!l2) {
+		cpu_detect_cache_sizes(c);
+		l2 = c->x86_cache_size;
+	}
 	return l2;
 }
 
Hmm?

	tglx

Re: [PATCH v2] x86/centaur: report correct CPU/cache topology

[David Wang]

> -----Original Mail-----
> Sender: Thomas Gleixner [mailto:tglx@linutronix.de]
> Time: 2018年4月26日 17:12
> Receiver: David Wang <davidwang@zhaoxin.com>
> CC: mingo@redhat.com; hpa@zytor.com; gregkh@linuxfoundation.org;
> x86@kernel.org; linux-kernel@vger.kernel.org; brucechang@via-
> alliance.com; cooperyan@zhaoxin.com; qiyuanwang@zhaoxin.com;
> benjaminpan@viatech.com; lukelin@viacpu.com; timguo@zhaoxin.com
> Subject: Re: [PATCH v2] x86/centaur: report correct CPU/cache topology
> 
> On Wed, 25 Apr 2018, David Wang wrote:
> >
> > +static void early_init_centaur_mc(struct cpuinfo_x86 *c) { #ifdef
> > +CONFIG_SMP
> > +	unsigned int eax, ebx, ecx, edx;
> > +
> > +	if (c->cpuid_level < 4)
> > +		return;
> > +
> > +	cpuid_count(4, 0, &eax, &ebx, &ecx, &edx);
> > +	if (eax & 0x1f)
> > +		c->x86_max_cores = (eax >> 26) + 1;
> > +	else
> > +		return;
> > +#endif
> > +}
> 
> My review comment from last time still stands:
> 
> > > 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
> 
> In other words:
> 
> Make a patch which moves intel_num_cpu_cores() into common.c. Rename
> the function into something like detect_num_cpu_cores() and fix up the
call
> site in intel.c.
> 
> Then add your patch and use the very same function.
> 

	OK. I got it.
> > +
> >  static void early_init_centaur(struct cpuinfo_x86 *c)  {
> > +	early_init_centaur_mc(c);
> >  	switch (c->x86) {
> >  #ifdef CONFIG_X86_32
> >  	case 5:
> > @@ -146,6 +163,7 @@ static void centaur_detect_vmx_virtcap(struct
> > cpuinfo_x86 *c)
> >
> >  static void init_centaur(struct cpuinfo_x86 *c)  {
> > +	unsigned int l2 = 0;
> >  #ifdef CONFIG_X86_32
> >  	char *name;
> >  	u32  fcr_set = 0;
> > @@ -161,6 +179,17 @@ static void init_centaur(struct cpuinfo_x86 *c)
> > #endif
> >  	early_init_centaur(c);
> >
> > +	l2 = init_intel_cacheinfo(c);
> > +
> > +	/* Detect legacy cache sizes if init_intel_cacheinfo did not */
> > +	if (l2 == 0) {
> > +		cpu_detect_cache_sizes(c);
> > +	}
> 
> Aside of the pointless parentheses, this really wants to be cleaned up as
well.
> 
> init_intel_cacheinfo() is called from the intel init code and it does the
same
> silly thing.
> 
> So the right thing to do is in a separate patch first
> 
> --- a/arch/x86/kernel/cpu/intel.c
> +++ b/arch/x86/kernel/cpu/intel.c
> @@ -679,12 +679,6 @@ static void init_intel(struct cpuinfo_x8
> 
>  	l2 = init_intel_cacheinfo(c);
> 
> -	/* Detect legacy cache sizes if init_intel_cacheinfo did not */
> -	if (l2 == 0) {
> -		cpu_detect_cache_sizes(c);
> -		l2 = c->x86_cache_size;
> -	}
> -
>  	if (c->cpuid_level > 9) {
>  		unsigned eax = cpuid_eax(10);
>  		/* Check for version and the number of counters */
> --- a/arch/x86/kernel/cpu/intel_cacheinfo.c
> +++ b/arch/x86/kernel/cpu/intel_cacheinfo.c
> @@ -802,6 +802,11 @@ unsigned int init_intel_cacheinfo(struct
> 
>  	c->x86_cache_size = l3 ? l3 : (l2 ? l2 : (l1i+l1d));
> 
> +	/* Detect legacy cache sizes if the above did not work */
> +	if (!l2) {
> +		cpu_detect_cache_sizes(c);
> +		l2 = c->x86_cache_size;
> +	}
>  	return l2;
>  }
> 
> Hmm?
> 
> 	tglx
> 
	OK. I got it.
	Patch v3 will be sent later.
	Thank you.
---
David