LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH v2] x86/centaur: report correct CPU/cache topology
@ 2018-04-25  8:48 David Wang
  2018-04-26  9:11 ` Thomas Gleixner
  0 siblings, 1 reply; 3+ messages in thread
From: David Wang @ 2018-04-25  8:48 UTC (permalink / raw)
  To: tglx, mingo, hpa, gregkh, x86, linux-kernel
  Cc: brucechang, cooperyan, qiyuanwang, benjaminpan, lukelin, timguo,
	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

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH v2] x86/centaur: report correct CPU/cache topology
  2018-04-25  8:48 [PATCH v2] x86/centaur: report correct CPU/cache topology David Wang
@ 2018-04-26  9:11 ` Thomas Gleixner
  0 siblings, 0 replies; 3+ messages in thread
From: Thomas Gleixner @ 2018-04-26  9:11 UTC (permalink / raw)
  To: David Wang
  Cc: mingo, hpa, gregkh, x86, linux-kernel, brucechang, cooperyan,
	qiyuanwang, benjaminpan, lukelin, timguo

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

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH v2] x86/centaur: report correct CPU/cache topology
@ 2018-04-26  9:52 David Wang
  0 siblings, 0 replies; 3+ messages in thread
From: David Wang @ 2018-04-26  9:52 UTC (permalink / raw)
  To: 'Thomas Gleixner'
  Cc: mingo, hpa, gregkh, x86, linux-kernel, brucechang, cooperyan,
	qiyuanwang, benjaminpan, lukelin, timguo



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

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2018-04-26  9:52 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-25  8:48 [PATCH v2] x86/centaur: report correct CPU/cache topology David Wang
2018-04-26  9:11 ` Thomas Gleixner
2018-04-26  9:52 David Wang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).