[Patch v2 0/2] amd: uncore: Fix for AMD Uncore driver

[Patch v2 0/2] amd: uncore: Fix for AMD Uncore driver

[Janakarajan Natarajan]

The following series provides fixes for the AMD Uncore driver.

* Renamed cache counters cpufeatures macro to better reflect different
  last level cache for different families
* Get correct information about number of cores sharing last level of
  cache

v1->v2
* Replaced while(1) with finite for loop based on feedback
  from Peter Zijlstra and Borislav Petkov.

Janakarajan Natarajan (2):
  amd: uncore: Rename cpufeatures macro for cache counters
  amd: uncore: Get correct number of cores sharing last level cache

 arch/x86/events/amd/uncore.c       | 19 +++++++++++++++----
 arch/x86/include/asm/cpufeatures.h |  2 +-
 2 files changed, 16 insertions(+), 5 deletions(-)

-- 
2.7.4

[Patch v2 1/2] amd: uncore: Rename cpufeatures macro for cache counters

[Janakarajan Natarajan]

In Family 17h, L3 is the last level cache as opposed to L2 in previous
families. Avoid this name confusion and rename X86_FEATURE_PERFCTR_L2 to
X86_FEATURE_PERFCTR_LLC to indicate the performance counter on the last
level of cache

Signed-off-by: Janakarajan Natarajan <Janakarajan.Natarajan@amd.com>
---
 arch/x86/events/amd/uncore.c       | 2 +-
 arch/x86/include/asm/cpufeatures.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/events/amd/uncore.c b/arch/x86/events/amd/uncore.c
index ad44af0..e34f8a6 100644
--- a/arch/x86/events/amd/uncore.c
+++ b/arch/x86/events/amd/uncore.c
@@ -555,7 +555,7 @@ static int __init amd_uncore_init(void)
 		ret = 0;
 	}
 
-	if (boot_cpu_has(X86_FEATURE_PERFCTR_L2)) {
+	if (boot_cpu_has(X86_FEATURE_PERFCTR_LLC)) {
 		amd_uncore_llc = alloc_percpu(struct amd_uncore *);
 		if (!amd_uncore_llc) {
 			ret = -ENOMEM;
diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
index 2701e5f..e3f3d0c 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -177,7 +177,7 @@
 #define X86_FEATURE_PERFCTR_NB  ( 6*32+24) /* NB performance counter extensions */
 #define X86_FEATURE_BPEXT	(6*32+26) /* data breakpoint extension */
 #define X86_FEATURE_PTSC	( 6*32+27) /* performance time-stamp counter */
-#define X86_FEATURE_PERFCTR_L2	( 6*32+28) /* L2 performance counter extensions */
+#define X86_FEATURE_PERFCTR_LLC	( 6*32+28) /* Last Level Cache performance counter extensions */
 #define X86_FEATURE_MWAITX	( 6*32+29) /* MWAIT extension (MONITORX/MWAITX) */
 
 /*
-- 
2.7.4

[Patch v2 2/2] amd: uncore: Get correct number of cores sharing last level cache

[Janakarajan Natarajan]

In Family 17h, the number of cores sharing a cache level is obtained
from the Cache Properties CPUID leaf (0x8000001d) by passing in the
cache level in ECX. In prior families, a cache level of 2 was used to
determine this information.

To get the right information, irrespective of Family, iterate over
the cache levels using CPUID 0x8000001d. The last level cache is the
last value to return a non-zero value in EAX.

Signed-off-by: Janakarajan Natarajan <Janakarajan.Natarajan@amd.com>
---
 arch/x86/events/amd/uncore.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/arch/x86/events/amd/uncore.c b/arch/x86/events/amd/uncore.c
index e34f8a6..6952262 100644
--- a/arch/x86/events/amd/uncore.c
+++ b/arch/x86/events/amd/uncore.c
@@ -400,11 +400,22 @@ static int amd_uncore_cpu_starting(unsigned int cpu)
 
 	if (amd_uncore_llc) {
 		unsigned int apicid = cpu_data(cpu).apicid;
-		unsigned int nshared;
+		unsigned int nshared, cache_topology, prev_eax = 0;
 
 		uncore = *per_cpu_ptr(amd_uncore_llc, cpu);
-		cpuid_count(0x8000001d, 2, &eax, &ebx, &ecx, &edx);
-		nshared = ((eax >> 14) & 0xfff) + 1;
+		/*
+		 * Iterate over Cache Topology Definition leaves until no
+		 * more cache descriptions are available
+		 */
+		for(cache_topology = 0; cache_topology < 5; cache_topology++) {
+			cpuid_count(0x8000001d, cache_topology, &eax, &ebx, &ecx, &edx);
+
+			if ((eax & 0x1f) == 0) /* EAX[0:4] gives type of cache */
+				break;
+
+			prev_eax = eax;
+		}
+		nshared = ((prev_eax >> 14) & 0xfff) + 1;
 		uncore->id = apicid - (apicid % nshared);
 
 		uncore = amd_uncore_find_online_sibling(uncore, amd_uncore_llc);
-- 
2.7.4

Re: [Patch v2 1/2] amd: uncore: Rename cpufeatures macro for cache counters

[Borislav Petkov]

On Thu, Jun 08, 2017 at 02:48:23PM -0500, Janakarajan Natarajan wrote:
> In Family 17h, L3 is the last level cache as opposed to L2 in previous
> families. Avoid this name confusion and rename X86_FEATURE_PERFCTR_L2 to
> X86_FEATURE_PERFCTR_LLC to indicate the performance counter on the last
> level of cache
		. (fullstop)

> 
> Signed-off-by: Janakarajan Natarajan <Janakarajan.Natarajan@amd.com>
> ---
>  arch/x86/events/amd/uncore.c       | 2 +-
>  arch/x86/include/asm/cpufeatures.h | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Borislav Petkov <bp@suse.de>

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [Patch v2 2/2] amd: uncore: Get correct number of cores sharing last level cache

[Borislav Petkov]

On Thu, Jun 08, 2017 at 02:48:24PM -0500, Janakarajan Natarajan wrote:
> In Family 17h, the number of cores sharing a cache level is obtained
> from the Cache Properties CPUID leaf (0x8000001d) by passing in the
> cache level in ECX. In prior families, a cache level of 2 was used to
> determine this information.
> 
> To get the right information, irrespective of Family, iterate over
> the cache levels using CPUID 0x8000001d. The last level cache is the
> last value to return a non-zero value in EAX.
> 
> Signed-off-by: Janakarajan Natarajan <Janakarajan.Natarajan@amd.com>
> ---
>  arch/x86/events/amd/uncore.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/events/amd/uncore.c b/arch/x86/events/amd/uncore.c
> index e34f8a6..6952262 100644
> --- a/arch/x86/events/amd/uncore.c
> +++ b/arch/x86/events/amd/uncore.c
> @@ -400,11 +400,22 @@ static int amd_uncore_cpu_starting(unsigned int cpu)
>  
>  	if (amd_uncore_llc) {
>  		unsigned int apicid = cpu_data(cpu).apicid;
> -		unsigned int nshared;
> +		unsigned int nshared, cache_topology, prev_eax = 0;

cache_topology is too long and not really what it is. Simply call it
subleaf.

>  		uncore = *per_cpu_ptr(amd_uncore_llc, cpu);
> -		cpuid_count(0x8000001d, 2, &eax, &ebx, &ecx, &edx);
> -		nshared = ((eax >> 14) & 0xfff) + 1;

<---- newline here.

> +		/*
> +		 * Iterate over Cache Topology Definition leaves until no
> +		 * more cache descriptions are available
							^
							|
All sentences need to end with a fullstop.--------------+

> +		 */
> +		for(cache_topology = 0; cache_topology < 5; cache_topology++) {
> +			cpuid_count(0x8000001d, cache_topology, &eax, &ebx, &ecx, &edx);
> +
> +			if ((eax & 0x1f) == 0) /* EAX[0:4] gives type of cache */

No side comments, put it over the if-line.

Also, we generally do 0-tests this way:

			if (!(eax & 0x1f))

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.