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
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 | 22 ++++++++++++++++++----
arch/x86/include/asm/cpufeatures.h | 2 +-
2 files changed, 19 insertions(+), 5 deletions(-)
--
1.8.3.1
In Family 17h, L3 is the last level cache as opposed to L2 in previous
families. Avoid this name confusion and rename X86_FEATURE_PERFCT_L2 to
X86_FEATURE_PERFCTR_LLC to indicate the performance counter on the last
level of cache.
Signed-off-by: Janakarajan Natarajan <[email protected]>
---
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 4d1f7f2d..8fea711 100644
--- a/arch/x86/events/amd/uncore.c
+++ b/arch/x86/events/amd/uncore.c
@@ -569,7 +569,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 b04bb6d..da422d6 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) */
/*
--
1.8.3.1
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 <[email protected]>
---
arch/x86/events/amd/uncore.c | 20 +++++++++++++++++---
1 file changed, 17 insertions(+), 3 deletions(-)
diff --git a/arch/x86/events/amd/uncore.c b/arch/x86/events/amd/uncore.c
index 8fea711..e5458a9 100644
--- a/arch/x86/events/amd/uncore.c
+++ b/arch/x86/events/amd/uncore.c
@@ -397,11 +397,25 @@ 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_level = 0, 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
+ */
+ while (1) {
+ cpuid_count(0x8000001d, cache_level,
+ &eax, &ebx, &ecx, &edx);
+ /*
+ * EAX[0:4] gives type of cache. 0 = No more caches
+ */
+ if ((eax & 0x1f) == 0)
+ break;
+ cache_level++;
+ prev_eax = eax;
+ }
+ nshared = ((prev_eax >> 14) & 0xfff) + 1;
uncore->id = apicid - (apicid % nshared);
uncore = amd_uncore_find_online_sibling(uncore, amd_uncore_llc);
--
1.8.3.1
On Mon, Jun 05, 2017 at 11:13:19AM -0500, Janakarajan Natarajan wrote:
> + /*
> + * Iterate over Cache Topology Definition leaves until no
> + * more cache descriptions are available
> + */
> + while (1) {
> + cpuid_count(0x8000001d, cache_level,
> + &eax, &ebx, &ecx, &edx);
> + /*
> + * EAX[0:4] gives type of cache. 0 = No more caches
> + */
> + if ((eax & 0x1f) == 0)
> + break;
> + cache_level++;
> + prev_eax = eax;
> + }
> + nshared = ((prev_eax >> 14) & 0xfff) + 1;
Egads, could we pretty please write that in a less horrible fashion?
Maybe something like:
for (cache_level = 0; cache_level < 3; cache_level++) {
cpuid_count(0x8000001d, cache_level, &eax, &ebx, &ecx, &edx);
if ((eax & 0x1f) == 0) /* EAX[0:4] gives cache type */
break;
prev_eax = eax;
}
That way we'll not run off into the woods if CPUID goes funny (never
trust a BIOS/virt monkey).
On Mon, Jun 05, 2017 at 11:13:18AM -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_PERFCT_L2 to
> X86_FEATURE_PERFCTR_LLC to indicate the performance counter on the last
> level of cache.
>
> Signed-off-by: Janakarajan Natarajan <[email protected]>
> ---
> 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 4d1f7f2d..8fea711 100644
> --- a/arch/x86/events/amd/uncore.c
> +++ b/arch/x86/events/amd/uncore.c
> @@ -569,7 +569,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 b04bb6d..da422d6 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 */
Bah, the Zen PPR calls this PerfCtrExtL3 now. And F16h BKDG calls it
PerfCtrExtL2I.
Can this be fixed to be PerfCtrExtLLC in the docs so that there is no
more confusion?
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
On Tue, Jun 06, 2017 at 12:15:30PM +0200, Peter Zijlstra wrote:
> Maybe something like:
>
> for (cache_level = 0; cache_level < 3; cache_level++) {
> cpuid_count(0x8000001d, cache_level, &eax, &ebx, &ecx, &edx);
>
> if ((eax & 0x1f) == 0) /* EAX[0:4] gives cache type */
if (!(eax & 0x1f))
> break;
>
> prev_eax = eax;
> }
>
> That way we'll not run off into the woods if CPUID goes funny (never
> trust a BIOS/virt monkey).
Yap, especially if there are CPUID functions with subleafs with holes in
them.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
On 06/06/2017 12:45 PM, Borislav Petkov wrote:
> On Tue, Jun 06, 2017 at 12:15:30PM +0200, Peter Zijlstra wrote:
>> Maybe something like:
>>
>> for (cache_level = 0; cache_level < 3; cache_level++) {
>> cpuid_count(0x8000001d, cache_level, &eax, &ebx, &ecx, &edx);
>>
>> if ((eax & 0x1f) == 0) /* EAX[0:4] gives cache type */
> if (!(eax & 0x1f))
>
>> break;
>>
>> prev_eax = eax;
>> }
>>
>> That way we'll not run off into the woods if CPUID goes funny (never
>> trust a BIOS/virt monkey).
> Yap, especially if there are CPUID functions with subleafs with holes in
> them.
Okay. I can send a v2 with the changes.
>
On 6/6/2017 11:31 AM, Borislav Petkov wrote:
> On Mon, Jun 05, 2017 at 11:13:18AM -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_PERFCT_L2 to
>> X86_FEATURE_PERFCTR_LLC to indicate the performance counter on the last
>> level of cache.
>>
>> Signed-off-by: Janakarajan Natarajan <[email protected]>
>> ---
>> 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 4d1f7f2d..8fea711 100644
>> --- a/arch/x86/events/amd/uncore.c
>> +++ b/arch/x86/events/amd/uncore.c
>> @@ -569,7 +569,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 b04bb6d..da422d6 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 */
> Bah, the Zen PPR calls this PerfCtrExtL3 now. And F16h BKDG calls it
> PerfCtrExtL2I.
>
> Can this be fixed to be PerfCtrExtLLC in the docs so that there is no
> more confusion?
The PerfCtrExtL3 will be changed to PerfCtrExtLLC in the next PPR update.