[PATCH 0/3] x86/cpufeatures: Re-arrange a few features and enumerate AVX512 BFLOAT16 intructions

[PATCH 0/3] x86/cpufeatures: Re-arrange a few features and enumerate AVX512 BFLOAT16 intructions

[Fenghua Yu]

To enumerate AVX512 BFLOAT16 feature CPUID.7.1:EAX[5] and other future
features in CPUID.7.1:EAX, Boris suggests to create a new pure feature
bits word.

Boris further suggests to re-define word 11 as scattered features word
and move the four X86_FEATURE_CQM_* features in existing word 11 and
word 12 into the new word 11. Then use word 12 to hold features in
CPUID.7.1:EAX including AVX512 BFLOAT16 instructions.

Also remove x86_cache_max_rmid and x86_cache_occ_scale in cpuinfo_x86
to save memory space because they are only read once by resctrl during
boot time. Get the info directly from CPUID in resctrl initialization.

Change Log:

Address all comments from Borislav Petkov:
- Re-define feature word 11 as a scattered features word and move the
  four X86_FEATURE_CQM_* features into the word
- Re-define feature word 12 to hold CPUID.7.1:EAX including BFLOAT16
- Name a dummy leaf 12 in cpuid_leafs in patch 0002 to avoid bisect
  error
- Simplify code to get number of rmid and monitoring scale in
  rdt_get_mon_l3_config()

Fenghua Yu (3):
  x86/resctrl: Get max rmid and occupancy scale directly from CPUID
    instead of cpuinfo_x86
  x86/cpufeatures: Combine word 11 and 12 into new scattered features
    word 11
  x86/cpufeatures: Enumerate new AVX512 BFLOAT16 instructions

 arch/x86/include/asm/cpufeature.h      |  4 +--
 arch/x86/include/asm/cpufeatures.h     | 18 +++++++----
 arch/x86/include/asm/processor.h       |  3 --
 arch/x86/kernel/cpu/common.c           | 45 ++------------------------
 arch/x86/kernel/cpu/cpuid-deps.c       |  4 +++
 arch/x86/kernel/cpu/resctrl/internal.h |  2 +-
 arch/x86/kernel/cpu/resctrl/monitor.c  | 13 ++++++--
 arch/x86/kernel/cpu/scattered.c        |  4 +++
 arch/x86/kvm/cpuid.h                   |  2 --
 9 files changed, 36 insertions(+), 59 deletions(-)

-- 
2.19.1

[PATCH 1/3] x86/resctrl: Get max rmid and occupancy scale directly from CPUID instead of cpuinfo_x86

[Fenghua Yu]

Although x86_cache_max_rmid and x86_cache_occ_scale are only read once
during resctrl initialization, they are always stored in cpuinfo_x86 for
each CPU during run time without usage. Even if resctrl is not
configured, they still occupy space in cpuinfo_x86.

To save cpuinfo_x86 space and make CPU and resctrl initialization simpler,
remove the two fields from cpuinfo_x86 and get max rmid and occupancy
scale directly from CPUID during resctrl initialization. And since each
known platform that supports resctrl has same max rmid on all CPUs, no
need to scan all CPUs to find minimum of max rmid values, i.e. getting
max rmid from CPUID on the current CPU is fine.

Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
---
 arch/x86/include/asm/processor.h       |  3 ---
 arch/x86/kernel/cpu/common.c           | 28 --------------------------
 arch/x86/kernel/cpu/resctrl/internal.h |  2 +-
 arch/x86/kernel/cpu/resctrl/monitor.c  | 13 +++++++++---
 4 files changed, 11 insertions(+), 35 deletions(-)

diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index c34a35c78618..27e875d4ca7d 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -99,9 +99,6 @@ struct cpuinfo_x86 {
 	/* in KB - valid for CPUS which support this call: */
 	unsigned int		x86_cache_size;
 	int			x86_cache_alignment;	/* In bytes */
-	/* Cache QoS architectural values: */
-	int			x86_cache_max_rmid;	/* max index */
-	int			x86_cache_occ_scale;	/* scale to bytes */
 	int			x86_power;
 	unsigned long		loops_per_jiffy;
 	/* cpuid returned max cores value: */
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 2c57fffebf9b..38e4b1a9005e 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -840,22 +840,9 @@ void get_cpu_cap(struct cpuinfo_x86 *c)
 		c->x86_capability[CPUID_F_0_EDX] = edx;
 
 		if (cpu_has(c, X86_FEATURE_CQM_LLC)) {
-			/* will be overridden if occupancy monitoring exists */
-			c->x86_cache_max_rmid = ebx;
-
 			/* QoS sub-leaf, EAX=0Fh, ECX=1 */
 			cpuid_count(0x0000000F, 1, &eax, &ebx, &ecx, &edx);
 			c->x86_capability[CPUID_F_1_EDX] = edx;
-
-			if ((cpu_has(c, X86_FEATURE_CQM_OCCUP_LLC)) ||
-			      ((cpu_has(c, X86_FEATURE_CQM_MBM_TOTAL)) ||
-			       (cpu_has(c, X86_FEATURE_CQM_MBM_LOCAL)))) {
-				c->x86_cache_max_rmid = ecx;
-				c->x86_cache_occ_scale = ebx;
-			}
-		} else {
-			c->x86_cache_max_rmid = -1;
-			c->x86_cache_occ_scale = -1;
 		}
 	}
 
@@ -1269,20 +1256,6 @@ static void generic_identify(struct cpuinfo_x86 *c)
 #endif
 }
 
-static void x86_init_cache_qos(struct cpuinfo_x86 *c)
-{
-	/*
-	 * The heavy lifting of max_rmid and cache_occ_scale are handled
-	 * in get_cpu_cap().  Here we just set the max_rmid for the boot_cpu
-	 * in case CQM bits really aren't there in this CPU.
-	 */
-	if (c != &boot_cpu_data) {
-		boot_cpu_data.x86_cache_max_rmid =
-			min(boot_cpu_data.x86_cache_max_rmid,
-			    c->x86_cache_max_rmid);
-	}
-}
-
 /*
  * Validate that ACPI/mptables have the same information about the
  * effective APIC id and update the package map.
@@ -1391,7 +1364,6 @@ static void identify_cpu(struct cpuinfo_x86 *c)
 #endif
 
 	x86_init_rdrand(c);
-	x86_init_cache_qos(c);
 	setup_pku(c);
 
 	/*
diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
index e49b77283924..474a7090d2dd 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h
+++ b/arch/x86/kernel/cpu/resctrl/internal.h
@@ -579,7 +579,7 @@ int closids_supported(void);
 void closid_free(int closid);
 int alloc_rmid(void);
 void free_rmid(u32 rmid);
-int rdt_get_mon_l3_config(struct rdt_resource *r);
+int __init rdt_get_mon_l3_config(struct rdt_resource *r);
 void mon_event_count(void *info);
 int rdtgroup_mondata_show(struct seq_file *m, void *arg);
 void rmdir_mondata_subdir_allrdtgrp(struct rdt_resource *r,
diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
index 1573a0a6b525..ea612dacf676 100644
--- a/arch/x86/kernel/cpu/resctrl/monitor.c
+++ b/arch/x86/kernel/cpu/resctrl/monitor.c
@@ -617,13 +617,20 @@ static void l3_mon_evt_init(struct rdt_resource *r)
 		list_add_tail(&mbm_local_event.list, &r->evt_list);
 }
 
-int rdt_get_mon_l3_config(struct rdt_resource *r)
+int __init rdt_get_mon_l3_config(struct rdt_resource *r)
 {
 	unsigned int cl_size = boot_cpu_data.x86_cache_size;
+	u32 eax, ebx, ecx, edx;
 	int ret;
 
-	r->mon_scale = boot_cpu_data.x86_cache_occ_scale;
-	r->num_rmid = boot_cpu_data.x86_cache_max_rmid + 1;
+	/*
+	 * At this point, CQM LLC and one of L3 occupancy, MBM total, and
+	 * MBM local monitoring features must be supported. So sub-leaf
+	 * (EAX=0xf, ECX=1) contains needed information for this resource.
+	 */
+	cpuid_count(0xf, 1, &eax, &ebx, &ecx, &edx);
+	r->num_rmid = ecx + 1;
+	r->mon_scale = ebx;
 
 	/*
 	 * A reasonable upper limit on the max threshold is the number
-- 
2.19.1

[PATCH 2/3] x86/cpufeatures: Combine word 11 and 12 into new scattered features word 11

[Fenghua Yu]

It's a waste for the four X86_FEATURE_CQM_* features to occupy two
pure feature bits words. To better utilize feature words, re-define
word 11 to host scattered features and move the four X86_FEATURE_CQM_*
features into Linux defined word 11. More scattered features can be added
in word 11 in the future.

Leaf 11 in cpuid_leafs is renamed as CPUID_LNX_4 to reflect it's a Linux
defined leaf.

Although word 12 doesn't have any feature now, leaf 12 in cpuid_leafs
still needs to be kept because cpuid_leafs must have NCAPINTS leafs.
Rename leaf 12 as CPUID_DUMMY which will be replaced by a meaningful name
in the next patch when CPUID.7.1:EAX occupies world 12.

KVM doesn't support resctrl now. So it's safe to move the
X86_FEATURE_CQM_* features to scattered features word 11 for KVM.

Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
---
 arch/x86/include/asm/cpufeature.h  |  4 ++--
 arch/x86/include/asm/cpufeatures.h | 17 ++++++++++-------
 arch/x86/kernel/cpu/common.c       | 14 --------------
 arch/x86/kernel/cpu/cpuid-deps.c   |  3 +++
 arch/x86/kernel/cpu/scattered.c    |  4 ++++
 arch/x86/kvm/cpuid.h               |  2 --
 6 files changed, 19 insertions(+), 25 deletions(-)

diff --git a/arch/x86/include/asm/cpufeature.h b/arch/x86/include/asm/cpufeature.h
index 1d337c51f7e6..403f70c2e431 100644
--- a/arch/x86/include/asm/cpufeature.h
+++ b/arch/x86/include/asm/cpufeature.h
@@ -22,8 +22,8 @@ enum cpuid_leafs
 	CPUID_LNX_3,
 	CPUID_7_0_EBX,
 	CPUID_D_1_EAX,
-	CPUID_F_0_EDX,
-	CPUID_F_1_EDX,
+	CPUID_LNX_4,
+	CPUID_DUMMY,
 	CPUID_8000_0008_EBX,
 	CPUID_6_EAX,
 	CPUID_8000_000A_EDX,
diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
index 75f27ee2c263..4f0a3d093794 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -269,13 +269,16 @@
 #define X86_FEATURE_XGETBV1		(10*32+ 2) /* XGETBV with ECX = 1 instruction */
 #define X86_FEATURE_XSAVES		(10*32+ 3) /* XSAVES/XRSTORS instructions */
 
-/* Intel-defined CPU QoS Sub-leaf, CPUID level 0x0000000F:0 (EDX), word 11 */
-#define X86_FEATURE_CQM_LLC		(11*32+ 1) /* LLC QoS if 1 */
-
-/* Intel-defined CPU QoS Sub-leaf, CPUID level 0x0000000F:1 (EDX), word 12 */
-#define X86_FEATURE_CQM_OCCUP_LLC	(12*32+ 0) /* LLC occupancy monitoring */
-#define X86_FEATURE_CQM_MBM_TOTAL	(12*32+ 1) /* LLC Total MBM monitoring */
-#define X86_FEATURE_CQM_MBM_LOCAL	(12*32+ 2) /* LLC Local MBM monitoring */
+/*
+ * Extended auxiliary flags: Linux defined - For features scattered in various
+ * CPUID levels like 0xf, word 11.
+ *
+ * Reuse free bits when adding new feature flags!
+ */
+#define X86_FEATURE_CQM_LLC		(11*32+ 0) /* LLC QoS if 1 */
+#define X86_FEATURE_CQM_OCCUP_LLC	(11*32+ 1) /* LLC occupancy monitoring */
+#define X86_FEATURE_CQM_MBM_TOTAL	(11*32+ 2) /* LLC Total MBM monitoring */
+#define X86_FEATURE_CQM_MBM_LOCAL	(11*32+ 3) /* LLC Local MBM monitoring */
 
 /* AMD-defined CPU features, CPUID level 0x80000008 (EBX), word 13 */
 #define X86_FEATURE_CLZERO		(13*32+ 0) /* CLZERO instruction */
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 38e4b1a9005e..5b0e9d869ce5 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -832,20 +832,6 @@ void get_cpu_cap(struct cpuinfo_x86 *c)
 		c->x86_capability[CPUID_D_1_EAX] = eax;
 	}
 
-	/* Additional Intel-defined flags: level 0x0000000F */
-	if (c->cpuid_level >= 0x0000000F) {
-
-		/* QoS sub-leaf, EAX=0Fh, ECX=0 */
-		cpuid_count(0x0000000F, 0, &eax, &ebx, &ecx, &edx);
-		c->x86_capability[CPUID_F_0_EDX] = edx;
-
-		if (cpu_has(c, X86_FEATURE_CQM_LLC)) {
-			/* QoS sub-leaf, EAX=0Fh, ECX=1 */
-			cpuid_count(0x0000000F, 1, &eax, &ebx, &ecx, &edx);
-			c->x86_capability[CPUID_F_1_EDX] = edx;
-		}
-	}
-
 	/* AMD-defined flags: level 0x80000001 */
 	eax = cpuid_eax(0x80000000);
 	c->extended_cpuid_level = eax;
diff --git a/arch/x86/kernel/cpu/cpuid-deps.c b/arch/x86/kernel/cpu/cpuid-deps.c
index 2c0bd38a44ab..fa07a224e7b9 100644
--- a/arch/x86/kernel/cpu/cpuid-deps.c
+++ b/arch/x86/kernel/cpu/cpuid-deps.c
@@ -59,6 +59,9 @@ static const struct cpuid_dep cpuid_deps[] = {
 	{ X86_FEATURE_AVX512_4VNNIW,	X86_FEATURE_AVX512F   },
 	{ X86_FEATURE_AVX512_4FMAPS,	X86_FEATURE_AVX512F   },
 	{ X86_FEATURE_AVX512_VPOPCNTDQ, X86_FEATURE_AVX512F   },
+	{ X86_FEATURE_CQM_OCCUP_LLC,	X86_FEATURE_CQM_LLC   },
+	{ X86_FEATURE_CQM_MBM_TOTAL,	X86_FEATURE_CQM_LLC   },
+	{ X86_FEATURE_CQM_MBM_LOCAL,	X86_FEATURE_CQM_LLC   },
 	{}
 };
 
diff --git a/arch/x86/kernel/cpu/scattered.c b/arch/x86/kernel/cpu/scattered.c
index 94aa1c72ca98..adf9b71386ef 100644
--- a/arch/x86/kernel/cpu/scattered.c
+++ b/arch/x86/kernel/cpu/scattered.c
@@ -26,6 +26,10 @@ struct cpuid_bit {
 static const struct cpuid_bit cpuid_bits[] = {
 	{ X86_FEATURE_APERFMPERF,       CPUID_ECX,  0, 0x00000006, 0 },
 	{ X86_FEATURE_EPB,		CPUID_ECX,  3, 0x00000006, 0 },
+	{ X86_FEATURE_CQM_LLC,		CPUID_EDX,  1, 0x0000000f, 0 },
+	{ X86_FEATURE_CQM_OCCUP_LLC,	CPUID_EDX,  0, 0x0000000f, 1 },
+	{ X86_FEATURE_CQM_MBM_TOTAL,	CPUID_EDX,  1, 0x0000000f, 1 },
+	{ X86_FEATURE_CQM_MBM_LOCAL,	CPUID_EDX,  2, 0x0000000f, 1 },
 	{ X86_FEATURE_CAT_L3,		CPUID_EBX,  1, 0x00000010, 0 },
 	{ X86_FEATURE_CAT_L2,		CPUID_EBX,  2, 0x00000010, 0 },
 	{ X86_FEATURE_CDP_L3,		CPUID_ECX,  2, 0x00000010, 1 },
diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h
index 9a327d5b6d1f..d78a61408243 100644
--- a/arch/x86/kvm/cpuid.h
+++ b/arch/x86/kvm/cpuid.h
@@ -47,8 +47,6 @@ static const struct cpuid_reg reverse_cpuid[] = {
 	[CPUID_8000_0001_ECX] = {0x80000001, 0, CPUID_ECX},
 	[CPUID_7_0_EBX]       = {         7, 0, CPUID_EBX},
 	[CPUID_D_1_EAX]       = {       0xd, 1, CPUID_EAX},
-	[CPUID_F_0_EDX]       = {       0xf, 0, CPUID_EDX},
-	[CPUID_F_1_EDX]       = {       0xf, 1, CPUID_EDX},
 	[CPUID_8000_0008_EBX] = {0x80000008, 0, CPUID_EBX},
 	[CPUID_6_EAX]         = {         6, 0, CPUID_EAX},
 	[CPUID_8000_000A_EDX] = {0x8000000a, 0, CPUID_EDX},
-- 
2.19.1

[PATCH 3/3] x86/cpufeatures: Enumerate new AVX512 BFLOAT16 instructions

[Fenghua Yu]

AVX512 Vector Neural Network Instructions (VNNI) in Intel Deep Learning
Boost support BFLOAT16 format (BF16). BF16 is a short version of FP32 and
has several advantages over FP16. BF16 offers more than enough range for
deep learning training tasks and doesn't need to handle hardware exception
as this is a performance optimization. FP32 accumulation after the
multiply is essential to achieve sufficient numerical behavior on an
application level.

AVX512 BFLOAT16 instructions can be enumerated by:
	CPUID.7.1:EAX[bit 5] AVX512_BF16

Use word 12, which is empty now, to hold features in CPUID.7.1:EAX
including AVX512_BF16. Leaf CPUID_DUMMY is renamed as CPUID_7_1_EAX.

Detailed information of the CPUID bit and AVX512 BFLOAT16 instructions
can be found in the latest Intel Architecture Instruction Set Extensions
and Future Features Programming Reference.

Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
---
 arch/x86/include/asm/cpufeature.h  | 2 +-
 arch/x86/include/asm/cpufeatures.h | 3 +++
 arch/x86/kernel/cpu/common.c       | 3 +++
 arch/x86/kernel/cpu/cpuid-deps.c   | 1 +
 4 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/cpufeature.h b/arch/x86/include/asm/cpufeature.h
index 403f70c2e431..58acda503817 100644
--- a/arch/x86/include/asm/cpufeature.h
+++ b/arch/x86/include/asm/cpufeature.h
@@ -23,7 +23,7 @@ enum cpuid_leafs
 	CPUID_7_0_EBX,
 	CPUID_D_1_EAX,
 	CPUID_LNX_4,
-	CPUID_DUMMY,
+	CPUID_7_1_EAX,
 	CPUID_8000_0008_EBX,
 	CPUID_6_EAX,
 	CPUID_8000_000A_EDX,
diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
index 4f0a3d093794..625191ceb214 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -280,6 +280,9 @@
 #define X86_FEATURE_CQM_MBM_TOTAL	(11*32+ 2) /* LLC Total MBM monitoring */
 #define X86_FEATURE_CQM_MBM_LOCAL	(11*32+ 3) /* LLC Local MBM monitoring */
 
+/* Intel-defined CPU features, CPUID level 0x00000007:1 (EAX), word 12 */
+#define X86_FEATURE_AVX512_BF16		(12*32+ 5) /* AVX512 BFLOAT16 instructions */
+
 /* AMD-defined CPU features, CPUID level 0x80000008 (EBX), word 13 */
 #define X86_FEATURE_CLZERO		(13*32+ 0) /* CLZERO instruction */
 #define X86_FEATURE_IRPERF		(13*32+ 1) /* Instructions Retired Count */
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 5b0e9d869ce5..6a4e948c7580 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -823,6 +823,9 @@ void get_cpu_cap(struct cpuinfo_x86 *c)
 		c->x86_capability[CPUID_7_0_EBX] = ebx;
 		c->x86_capability[CPUID_7_ECX] = ecx;
 		c->x86_capability[CPUID_7_EDX] = edx;
+
+		cpuid_count(0x00000007, 1, &eax, &ebx, &ecx, &edx);
+		c->x86_capability[CPUID_7_1_EAX] = eax;
 	}
 
 	/* Extended state features: level 0x0000000d */
diff --git a/arch/x86/kernel/cpu/cpuid-deps.c b/arch/x86/kernel/cpu/cpuid-deps.c
index fa07a224e7b9..a444028d8145 100644
--- a/arch/x86/kernel/cpu/cpuid-deps.c
+++ b/arch/x86/kernel/cpu/cpuid-deps.c
@@ -62,6 +62,7 @@ static const struct cpuid_dep cpuid_deps[] = {
 	{ X86_FEATURE_CQM_OCCUP_LLC,	X86_FEATURE_CQM_LLC   },
 	{ X86_FEATURE_CQM_MBM_TOTAL,	X86_FEATURE_CQM_LLC   },
 	{ X86_FEATURE_CQM_MBM_LOCAL,	X86_FEATURE_CQM_LLC   },
+	{ X86_FEATURE_AVX512_BF16,	X86_FEATURE_AVX512VL  },
 	{}
 };
 
-- 
2.19.1

Re: [PATCH 1/3] x86/resctrl: Get max rmid and occupancy scale directly from CPUID instead of cpuinfo_x86

[Borislav Petkov]

On Sun, Jun 16, 2019 at 10:14:08AM -0700, Fenghua Yu wrote:
> @@ -617,13 +617,20 @@ static void l3_mon_evt_init(struct rdt_resource *r)
>  		list_add_tail(&mbm_local_event.list, &r->evt_list);
>  }
>  
> -int rdt_get_mon_l3_config(struct rdt_resource *r)
> +int __init rdt_get_mon_l3_config(struct rdt_resource *r)
>  {
>  	unsigned int cl_size = boot_cpu_data.x86_cache_size;
> +	u32 eax, ebx, ecx, edx;
>  	int ret;
>  
> -	r->mon_scale = boot_cpu_data.x86_cache_occ_scale;
> -	r->num_rmid = boot_cpu_data.x86_cache_max_rmid + 1;
> +	/*
> +	 * At this point, CQM LLC and one of L3 occupancy, MBM total, and
> +	 * MBM local monitoring features must be supported. So sub-leaf
> +	 * (EAX=0xf, ECX=1) contains needed information for this resource.
> +	 */
> +	cpuid_count(0xf, 1, &eax, &ebx, &ecx, &edx);
> +	r->num_rmid = ecx + 1;
> +	r->mon_scale = ebx;
>  
>  	/*
>  	 * A reasonable upper limit on the max threshold is the number

This is simpler than that:

https://lkml.kernel.org/r/20190614174959.GF198207@romley-ivt3.sc.intel.com

Why?


-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH 1/3] x86/resctrl: Get max rmid and occupancy scale directly from CPUID instead of cpuinfo_x86

[Thomas Gleixner]

On Sun, 16 Jun 2019, Fenghua Yu wrote:

> Although x86_cache_max_rmid and x86_cache_occ_scale are only read once
> during resctrl initialization, they are always stored in cpuinfo_x86 for
> each CPU during run time without usage. Even if resctrl is not
> configured, they still occupy space in cpuinfo_x86.

And that's a problem because?

> To save cpuinfo_x86 space and make CPU and resctrl initialization simpler,
> remove the two fields from cpuinfo_x86 and get max rmid and occupancy

What is simpler? The fact that more code fiddles with CPUID? That's exactly
the wrong direction.

The storage size of struct cpuinfo is largely uninteresting especially as
long as we keep num_online_cpus copies of the same information around.

Just grep for places which invoke CPUID and then look how many of them do
it over and over and even the code in arch/x86/kernel/cpu/ is an
unpenetrable mess for exactly this reason.

The right thing to do is to have one instance of the CPUID information
which is

  - a proper data structure with named fields and named bits

  - a single master instance which can be mapped to all CPUs

This data structure is filled in in one go by reading out all leaves and
not by the maze we have today which puts together selected parts piecewise
and never exposes a full and consistent picture.

This allows to remove all these custom copies of CPUID leaf readouts and
allows proper filtering/disabling at a central place.

Making it a proper data structure with fields and bits gets rid of all that
hex masking/shifting nonsense which is used to decode parts of those
fields.

That's not a performance issue because all performance critical code should
use static_cpu_has() anyway. For non critical code boot_cpu_has() is
sufficient.

Upcoming secondary CPUs would do a sanity check on their CPUID content to
check whether everything is symmetric. We should do that actually today
because not detecting asymetric features early leads to exactly the issue
which was fixed recently with loading the micro code earlier than perf.
But we can't because the information retrieval is done in a gazillion of
places.
  
Now you might argue that the upcoming asymetric processors (SIGH!) will
require per CPU instances of the feature leafs. Sure that needs some
thought, but it needs thought even with the current code and I'm absolutely
not interested to duct tape that stuff into the current code.

The solution for this with the above scheme is to utilize the feature
mismatch detection and have a proper classification which features are
allowed to deviate and which are not. For those which can deviate, we can
provide separate storage as this information needs to be propagated to
other entities (fault handlers, placement code, xsave variants etc.). But
that's a limited amount of information and the bulk will still be the same.

This mismatch detection is essential for dealing with future asymetric
CPUs proper. When the kernel detects it, it can disable mismatching
features which are not yet handled by code which has to be aware of them.

And disabling them means that with that scheme we can actually trap CPUID
in userspace and provide it consistent and filtered information instead
of having the mismatch between kernel view and user space view.

Borislav has experimented with that already, but thanks to the marvelous
security features built into Intel (and other) CPUs this is still mostly a
drawing board exercise.

Just for the record. Before this cleanup takes place, I'm not even looking
at any patches which attempt to support asymetric processors. The current
supply of duct tape engineering horrors is sufficient for bad mood. No need
for more of that.

Thanks,

	tglx

Re: [PATCH 1/3] x86/resctrl: Get max rmid and occupancy scale directly from CPUID instead of cpuinfo_x86

[Fenghua Yu]

On Sun, Jun 16, 2019 at 10:24:13PM +0200, Thomas Gleixner wrote:
> On Sun, 16 Jun 2019, Fenghua Yu wrote:
> 
> > Although x86_cache_max_rmid and x86_cache_occ_scale are only read once
> > during resctrl initialization, they are always stored in cpuinfo_x86 for
> > each CPU during run time without usage. Even if resctrl is not
> > configured, they still occupy space in cpuinfo_x86.
> 
> And that's a problem because?
> 
> > To save cpuinfo_x86 space and make CPU and resctrl initialization simpler,
> > remove the two fields from cpuinfo_x86 and get max rmid and occupancy
> 
> What is simpler? The fact that more code fiddles with CPUID? That's exactly
> the wrong direction.
> 
> The storage size of struct cpuinfo is largely uninteresting especially as
> long as we keep num_online_cpus copies of the same information around.
> 
> Just grep for places which invoke CPUID and then look how many of them do
> it over and over and even the code in arch/x86/kernel/cpu/ is an
> unpenetrable mess for exactly this reason.
> 
> The right thing to do is to have one instance of the CPUID information
> which is
> 
>   - a proper data structure with named fields and named bits
> 
>   - a single master instance which can be mapped to all CPUs
> 
> This data structure is filled in in one go by reading out all leaves and
> not by the maze we have today which puts together selected parts piecewise
> and never exposes a full and consistent picture.
> 
> This allows to remove all these custom copies of CPUID leaf readouts and
> allows proper filtering/disabling at a central place.
> 
> Making it a proper data structure with fields and bits gets rid of all that
> hex masking/shifting nonsense which is used to decode parts of those
> fields.
> 
> That's not a performance issue because all performance critical code should
> use static_cpu_has() anyway. For non critical code boot_cpu_has() is
> sufficient.
> 
> Upcoming secondary CPUs would do a sanity check on their CPUID content to
> check whether everything is symmetric. We should do that actually today
> because not detecting asymetric features early leads to exactly the issue
> which was fixed recently with loading the micro code earlier than perf.
> But we can't because the information retrieval is done in a gazillion of
> places.
>   
> Now you might argue that the upcoming asymetric processors (SIGH!) will
> require per CPU instances of the feature leafs. Sure that needs some
> thought, but it needs thought even with the current code and I'm absolutely
> not interested to duct tape that stuff into the current code.
> 
> The solution for this with the above scheme is to utilize the feature
> mismatch detection and have a proper classification which features are
> allowed to deviate and which are not. For those which can deviate, we can
> provide separate storage as this information needs to be propagated to
> other entities (fault handlers, placement code, xsave variants etc.). But
> that's a limited amount of information and the bulk will still be the same.
> 
> This mismatch detection is essential for dealing with future asymetric
> CPUs proper. When the kernel detects it, it can disable mismatching
> features which are not yet handled by code which has to be aware of them.
> 
> And disabling them means that with that scheme we can actually trap CPUID
> in userspace and provide it consistent and filtered information instead
> of having the mismatch between kernel view and user space view.
> 
> Borislav has experimented with that already, but thanks to the marvelous
> security features built into Intel (and other) CPUs this is still mostly a
> drawing board exercise.
> 
> Just for the record. Before this cleanup takes place, I'm not even looking
> at any patches which attempt to support asymetric processors. The current
> supply of duct tape engineering horrors is sufficient for bad mood. No need
> for more of that.

I see. Then this patch #1 doesn't make sense.

So in the next version, can I remove the patch #1, change the patch #2 as
follows, and keep the patch #3?

It's a waste for the four X86_FEATURE_CQM_* features to occupy two
pure feature bits words. To better utilize feature words, re-define
word 11 to host scattered features and move the four X86_FEATURE_CQM_*
features into Linux defined word 11. More scattered features can be added
in word 11 in the future.

Leaf 11 in cpuid_leafs is renamed as CPUID_LNX_4 to reflect it's a Linux
defined leaf.

Although word 12 doesn't have any feature now, leaf 12 in cpuid_leafs
still needs to be kept because cpuid_leafs must have NCAPINTS leafs.
Rename leaf 12 as CPUID_DUMMY which will be replaced by a meaningful name
in the next patch when CPUID.7.1:EAX occupies world 12.

KVM doesn't support resctrl now. So it's safe to move the
X86_FEATURE_CQM_* features to scattered features word 11 for KVM.

Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
---
 arch/x86/include/asm/cpufeature.h  |  4 +--
 arch/x86/include/asm/cpufeatures.h | 17 ++++++----
 arch/x86/kernel/cpu/common.c       | 53 +++++++++++++++---------------
 arch/x86/kernel/cpu/cpuid-deps.c   |  3 ++
 arch/x86/kernel/cpu/scattered.c    |  4 +++
 arch/x86/kvm/cpuid.h               |  2 --
 6 files changed, 45 insertions(+), 38 deletions(-)

diff --git a/arch/x86/include/asm/cpufeature.h b/arch/x86/include/asm/cpufeature.h
index 1d337c51f7e6..403f70c2e431 100644
--- a/arch/x86/include/asm/cpufeature.h
+++ b/arch/x86/include/asm/cpufeature.h
@@ -22,8 +22,8 @@ enum cpuid_leafs
 	CPUID_LNX_3,
 	CPUID_7_0_EBX,
 	CPUID_D_1_EAX,
-	CPUID_F_0_EDX,
-	CPUID_F_1_EDX,
+	CPUID_LNX_4,
+	CPUID_DUMMY,
 	CPUID_8000_0008_EBX,
 	CPUID_6_EAX,
 	CPUID_8000_000A_EDX,
diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
index 75f27ee2c263..4f0a3d093794 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -269,13 +269,16 @@
 #define X86_FEATURE_XGETBV1		(10*32+ 2) /* XGETBV with ECX = 1 instruction */
 #define X86_FEATURE_XSAVES		(10*32+ 3) /* XSAVES/XRSTORS instructions */
 
-/* Intel-defined CPU QoS Sub-leaf, CPUID level 0x0000000F:0 (EDX), word 11 */
-#define X86_FEATURE_CQM_LLC		(11*32+ 1) /* LLC QoS if 1 */
-
-/* Intel-defined CPU QoS Sub-leaf, CPUID level 0x0000000F:1 (EDX), word 12 */
-#define X86_FEATURE_CQM_OCCUP_LLC	(12*32+ 0) /* LLC occupancy monitoring */
-#define X86_FEATURE_CQM_MBM_TOTAL	(12*32+ 1) /* LLC Total MBM monitoring */
-#define X86_FEATURE_CQM_MBM_LOCAL	(12*32+ 2) /* LLC Local MBM monitoring */
+/*
+ * Extended auxiliary flags: Linux defined - For features scattered in various
+ * CPUID levels like 0xf, word 11.
+ *
+ * Reuse free bits when adding new feature flags!
+ */
+#define X86_FEATURE_CQM_LLC		(11*32+ 0) /* LLC QoS if 1 */
+#define X86_FEATURE_CQM_OCCUP_LLC	(11*32+ 1) /* LLC occupancy monitoring */
+#define X86_FEATURE_CQM_MBM_TOTAL	(11*32+ 2) /* LLC Total MBM monitoring */
+#define X86_FEATURE_CQM_MBM_LOCAL	(11*32+ 3) /* LLC Local MBM monitoring */
 
 /* AMD-defined CPU features, CPUID level 0x80000008 (EBX), word 13 */
 #define X86_FEATURE_CLZERO		(13*32+ 0) /* CLZERO instruction */
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 2c57fffebf9b..f080be35da41 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -801,6 +801,31 @@ static void init_speculation_control(struct cpuinfo_x86 *c)
 	}
 }
 
+static void get_cqm_info(struct cpuinfo_x86 *c)
+{
+	if (cpu_has(c, X86_FEATURE_CQM_LLC)) {
+		u32 eax, ebx, ecx, edx;
+
+		/* QoS sub-leaf, EAX=0Fh, ECX=0 */
+		cpuid_count(0x0000000F, 0, &eax, &ebx, &ecx, &edx);
+		/* will be overridden if occupancy monitoring exists */
+		c->x86_cache_max_rmid = ebx;
+
+		if (cpu_has(c, X86_FEATURE_CQM_OCCUP_LLC) ||
+		    cpu_has(c, X86_FEATURE_CQM_MBM_TOTAL) ||
+		    cpu_has(c, X86_FEATURE_CQM_MBM_LOCAL)) {
+			/* QoS sub-leaf, EAX=0Fh, ECX=1 */
+			cpuid_count(0x0000000F, 1, &eax, &ebx, &ecx, &edx);
+
+			c->x86_cache_max_rmid = ecx;
+			c->x86_cache_occ_scale = ebx;
+		}
+	} else {
+		c->x86_cache_max_rmid = -1;
+		c->x86_cache_occ_scale = -1;
+	}
+}
+
 void get_cpu_cap(struct cpuinfo_x86 *c)
 {
 	u32 eax, ebx, ecx, edx;
@@ -832,33 +857,6 @@ void get_cpu_cap(struct cpuinfo_x86 *c)
 		c->x86_capability[CPUID_D_1_EAX] = eax;
 	}
 
-	/* Additional Intel-defined flags: level 0x0000000F */
-	if (c->cpuid_level >= 0x0000000F) {
-
-		/* QoS sub-leaf, EAX=0Fh, ECX=0 */
-		cpuid_count(0x0000000F, 0, &eax, &ebx, &ecx, &edx);
-		c->x86_capability[CPUID_F_0_EDX] = edx;
-
-		if (cpu_has(c, X86_FEATURE_CQM_LLC)) {
-			/* will be overridden if occupancy monitoring exists */
-			c->x86_cache_max_rmid = ebx;
-
-			/* QoS sub-leaf, EAX=0Fh, ECX=1 */
-			cpuid_count(0x0000000F, 1, &eax, &ebx, &ecx, &edx);
-			c->x86_capability[CPUID_F_1_EDX] = edx;
-
-			if ((cpu_has(c, X86_FEATURE_CQM_OCCUP_LLC)) ||
-			      ((cpu_has(c, X86_FEATURE_CQM_MBM_TOTAL)) ||
-			       (cpu_has(c, X86_FEATURE_CQM_MBM_LOCAL)))) {
-				c->x86_cache_max_rmid = ecx;
-				c->x86_cache_occ_scale = ebx;
-			}
-		} else {
-			c->x86_cache_max_rmid = -1;
-			c->x86_cache_occ_scale = -1;
-		}
-	}
-
 	/* AMD-defined flags: level 0x80000001 */
 	eax = cpuid_eax(0x80000000);
 	c->extended_cpuid_level = eax;
@@ -889,6 +887,7 @@ void get_cpu_cap(struct cpuinfo_x86 *c)
 
 	init_scattered_cpuid_features(c);
 	init_speculation_control(c);
+	get_cqm_info(c);
 
 	/*
 	 * Clear/Set all flags overridden by options, after probe.
diff --git a/arch/x86/kernel/cpu/cpuid-deps.c b/arch/x86/kernel/cpu/cpuid-deps.c
index 2c0bd38a44ab..fa07a224e7b9 100644
--- a/arch/x86/kernel/cpu/cpuid-deps.c
+++ b/arch/x86/kernel/cpu/cpuid-deps.c
@@ -59,6 +59,9 @@ static const struct cpuid_dep cpuid_deps[] = {
 	{ X86_FEATURE_AVX512_4VNNIW,	X86_FEATURE_AVX512F   },
 	{ X86_FEATURE_AVX512_4FMAPS,	X86_FEATURE_AVX512F   },
 	{ X86_FEATURE_AVX512_VPOPCNTDQ, X86_FEATURE_AVX512F   },
+	{ X86_FEATURE_CQM_OCCUP_LLC,	X86_FEATURE_CQM_LLC   },
+	{ X86_FEATURE_CQM_MBM_TOTAL,	X86_FEATURE_CQM_LLC   },
+	{ X86_FEATURE_CQM_MBM_LOCAL,	X86_FEATURE_CQM_LLC   },
 	{}
 };
 
diff --git a/arch/x86/kernel/cpu/scattered.c b/arch/x86/kernel/cpu/scattered.c
index 94aa1c72ca98..adf9b71386ef 100644
--- a/arch/x86/kernel/cpu/scattered.c
+++ b/arch/x86/kernel/cpu/scattered.c
@@ -26,6 +26,10 @@ struct cpuid_bit {
 static const struct cpuid_bit cpuid_bits[] = {
 	{ X86_FEATURE_APERFMPERF,       CPUID_ECX,  0, 0x00000006, 0 },
 	{ X86_FEATURE_EPB,		CPUID_ECX,  3, 0x00000006, 0 },
+	{ X86_FEATURE_CQM_LLC,		CPUID_EDX,  1, 0x0000000f, 0 },
+	{ X86_FEATURE_CQM_OCCUP_LLC,	CPUID_EDX,  0, 0x0000000f, 1 },
+	{ X86_FEATURE_CQM_MBM_TOTAL,	CPUID_EDX,  1, 0x0000000f, 1 },
+	{ X86_FEATURE_CQM_MBM_LOCAL,	CPUID_EDX,  2, 0x0000000f, 1 },
 	{ X86_FEATURE_CAT_L3,		CPUID_EBX,  1, 0x00000010, 0 },
 	{ X86_FEATURE_CAT_L2,		CPUID_EBX,  2, 0x00000010, 0 },
 	{ X86_FEATURE_CDP_L3,		CPUID_ECX,  2, 0x00000010, 1 },
diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h
index 9a327d5b6d1f..d78a61408243 100644
--- a/arch/x86/kvm/cpuid.h
+++ b/arch/x86/kvm/cpuid.h
@@ -47,8 +47,6 @@ static const struct cpuid_reg reverse_cpuid[] = {
 	[CPUID_8000_0001_ECX] = {0x80000001, 0, CPUID_ECX},
 	[CPUID_7_0_EBX]       = {         7, 0, CPUID_EBX},
 	[CPUID_D_1_EAX]       = {       0xd, 1, CPUID_EAX},
-	[CPUID_F_0_EDX]       = {       0xf, 0, CPUID_EDX},
-	[CPUID_F_1_EDX]       = {       0xf, 1, CPUID_EDX},
 	[CPUID_8000_0008_EBX] = {0x80000008, 0, CPUID_EBX},
 	[CPUID_6_EAX]         = {         6, 0, CPUID_EAX},
 	[CPUID_8000_000A_EDX] = {0x8000000a, 0, CPUID_EDX},
-- 
2.19.1


Thanks.

-Fenghua

Re: [PATCH 1/3] x86/resctrl: Get max rmid and occupancy scale directly from CPUID instead of cpuinfo_x86

[Fenghua Yu]

On Sun, Jun 16, 2019 at 07:52:33PM +0200, Borislav Petkov wrote:
> On Sun, Jun 16, 2019 at 10:14:08AM -0700, Fenghua Yu wrote:
> > @@ -617,13 +617,20 @@ static void l3_mon_evt_init(struct rdt_resource *r)
> >  		list_add_tail(&mbm_local_event.list, &r->evt_list);
> >  }
> >  
> > -int rdt_get_mon_l3_config(struct rdt_resource *r)
> > +int __init rdt_get_mon_l3_config(struct rdt_resource *r)
> >  {
> >  	unsigned int cl_size = boot_cpu_data.x86_cache_size;
> > +	u32 eax, ebx, ecx, edx;
> >  	int ret;
> >  
> > -	r->mon_scale = boot_cpu_data.x86_cache_occ_scale;
> > -	r->num_rmid = boot_cpu_data.x86_cache_max_rmid + 1;
> > +	/*
> > +	 * At this point, CQM LLC and one of L3 occupancy, MBM total, and
> > +	 * MBM local monitoring features must be supported. So sub-leaf
> > +	 * (EAX=0xf, ECX=1) contains needed information for this resource.
> > +	 */
> > +	cpuid_count(0xf, 1, &eax, &ebx, &ecx, &edx);
> > +	r->num_rmid = ecx + 1;
> > +	r->mon_scale = ebx;
> >  
> >  	/*
> >  	 * A reasonable upper limit on the max threshold is the number
> 
> This is simpler than that:
> 
> https://lkml.kernel.org/r/20190614174959.GF198207@romley-ivt3.sc.intel.com
> 
> Why?

After think this code again, ecx and ebx in sub-leaf CPUID.f.1 actually
contains the number of rmid and monitoring scale. The two variables are
always valid if any of L3 occupancy, MBM total, and MBM local monitoring
features is supported. So there is no need to check the features to get
the info.

But seems this patch is not needed according to Thomas?

Should I do the following changes in the next version of patch set?

1. Remove patch #1
2. Change patch #2 to the patch in https://lkml.org/lkml/2019/6/16/274
3. Keep patch #3

Please advice.

Thanks.

-Fenghua

Re: [PATCH 1/3] x86/resctrl: Get max rmid and occupancy scale directly from CPUID instead of cpuinfo_x86

[Borislav Petkov]

On Sun, Jun 16, 2019 at 08:18:09PM -0700, Fenghua Yu wrote:
> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> index 2c57fffebf9b..f080be35da41 100644
> --- a/arch/x86/kernel/cpu/common.c
> +++ b/arch/x86/kernel/cpu/common.c
> @@ -801,6 +801,31 @@ static void init_speculation_control(struct cpuinfo_x86 *c)
>  	}
>  }
>  
> +static void get_cqm_info(struct cpuinfo_x86 *c)
> +{
> +	if (cpu_has(c, X86_FEATURE_CQM_LLC)) {
> +		u32 eax, ebx, ecx, edx;
> +
> +		/* QoS sub-leaf, EAX=0Fh, ECX=0 */
> +		cpuid_count(0x0000000F, 0, &eax, &ebx, &ecx, &edx);
> +		/* will be overridden if occupancy monitoring exists */
> +		c->x86_cache_max_rmid = ebx;
> +
> +		if (cpu_has(c, X86_FEATURE_CQM_OCCUP_LLC) ||
> +		    cpu_has(c, X86_FEATURE_CQM_MBM_TOTAL) ||
> +		    cpu_has(c, X86_FEATURE_CQM_MBM_LOCAL)) {
> +			/* QoS sub-leaf, EAX=0Fh, ECX=1 */
> +			cpuid_count(0x0000000F, 1, &eax, &ebx, &ecx, &edx);
> +
> +			c->x86_cache_max_rmid = ecx;
> +			c->x86_cache_occ_scale = ebx;
> +		}
> +	} else {
> +		c->x86_cache_max_rmid = -1;
> +		c->x86_cache_occ_scale = -1;
> +	}
> +}
> +
>  void get_cpu_cap(struct cpuinfo_x86 *c)
>  {
>  	u32 eax, ebx, ecx, edx;
> @@ -832,33 +857,6 @@ void get_cpu_cap(struct cpuinfo_x86 *c)
>  		c->x86_capability[CPUID_D_1_EAX] = eax;
>  	}
>  
> -	/* Additional Intel-defined flags: level 0x0000000F */
> -	if (c->cpuid_level >= 0x0000000F) {
> -
> -		/* QoS sub-leaf, EAX=0Fh, ECX=0 */
> -		cpuid_count(0x0000000F, 0, &eax, &ebx, &ecx, &edx);
> -		c->x86_capability[CPUID_F_0_EDX] = edx;
> -
> -		if (cpu_has(c, X86_FEATURE_CQM_LLC)) {
> -			/* will be overridden if occupancy monitoring exists */
> -			c->x86_cache_max_rmid = ebx;
> -
> -			/* QoS sub-leaf, EAX=0Fh, ECX=1 */
> -			cpuid_count(0x0000000F, 1, &eax, &ebx, &ecx, &edx);
> -			c->x86_capability[CPUID_F_1_EDX] = edx;
> -
> -			if ((cpu_has(c, X86_FEATURE_CQM_OCCUP_LLC)) ||
> -			      ((cpu_has(c, X86_FEATURE_CQM_MBM_TOTAL)) ||
> -			       (cpu_has(c, X86_FEATURE_CQM_MBM_LOCAL)))) {
> -				c->x86_cache_max_rmid = ecx;
> -				c->x86_cache_occ_scale = ebx;
> -			}
> -		} else {
> -			c->x86_cache_max_rmid = -1;
> -			c->x86_cache_occ_scale = -1;
> -		}
> -	}

Why are you doing this carving out into a separate function since you're
keeping the cpuinfo_x86 members?

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH 1/3] x86/resctrl: Get max rmid and occupancy scale directly from CPUID instead of cpuinfo_x86

[Fenghua Yu]

On Mon, Jun 17, 2019 at 09:52:14AM +0200, Borislav Petkov wrote:
> On Sun, Jun 16, 2019 at 08:18:09PM -0700, Fenghua Yu wrote:
> > diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> > index 2c57fffebf9b..f080be35da41 100644
> > --- a/arch/x86/kernel/cpu/common.c
> > +++ b/arch/x86/kernel/cpu/common.c
> > @@ -801,6 +801,31 @@ static void init_speculation_control(struct cpuinfo_x86 *c)
> >  	}
> >  }
> >  
> > +static void get_cqm_info(struct cpuinfo_x86 *c)
> > +{
> > +	if (cpu_has(c, X86_FEATURE_CQM_LLC)) {
> > +		u32 eax, ebx, ecx, edx;
> > +
> > +		/* QoS sub-leaf, EAX=0Fh, ECX=0 */
> > +		cpuid_count(0x0000000F, 0, &eax, &ebx, &ecx, &edx);
> > +		/* will be overridden if occupancy monitoring exists */
> > +		c->x86_cache_max_rmid = ebx;
> > +
> > +		if (cpu_has(c, X86_FEATURE_CQM_OCCUP_LLC) ||
> > +		    cpu_has(c, X86_FEATURE_CQM_MBM_TOTAL) ||
> > +		    cpu_has(c, X86_FEATURE_CQM_MBM_LOCAL)) {
> > +			/* QoS sub-leaf, EAX=0Fh, ECX=1 */
> > +			cpuid_count(0x0000000F, 1, &eax, &ebx, &ecx, &edx);
> > +
> > +			c->x86_cache_max_rmid = ecx;
> > +			c->x86_cache_occ_scale = ebx;
> > +		}
> > +	} else {
> > +		c->x86_cache_max_rmid = -1;
> > +		c->x86_cache_occ_scale = -1;
> > +	}
> > +}
> > +
> >  void get_cpu_cap(struct cpuinfo_x86 *c)
> >  {
> >  	u32 eax, ebx, ecx, edx;
> > @@ -832,33 +857,6 @@ void get_cpu_cap(struct cpuinfo_x86 *c)
> >  		c->x86_capability[CPUID_D_1_EAX] = eax;
> >  	}
> >  
> > -	/* Additional Intel-defined flags: level 0x0000000F */
> > -	if (c->cpuid_level >= 0x0000000F) {
> > -
> > -		/* QoS sub-leaf, EAX=0Fh, ECX=0 */
> > -		cpuid_count(0x0000000F, 0, &eax, &ebx, &ecx, &edx);
> > -		c->x86_capability[CPUID_F_0_EDX] = edx;
> > -
> > -		if (cpu_has(c, X86_FEATURE_CQM_LLC)) {
> > -			/* will be overridden if occupancy monitoring exists */
> > -			c->x86_cache_max_rmid = ebx;
> > -
> > -			/* QoS sub-leaf, EAX=0Fh, ECX=1 */
> > -			cpuid_count(0x0000000F, 1, &eax, &ebx, &ecx, &edx);
> > -			c->x86_capability[CPUID_F_1_EDX] = edx;
> > -
> > -			if ((cpu_has(c, X86_FEATURE_CQM_OCCUP_LLC)) ||
> > -			      ((cpu_has(c, X86_FEATURE_CQM_MBM_TOTAL)) ||
> > -			       (cpu_has(c, X86_FEATURE_CQM_MBM_LOCAL)))) {
> > -				c->x86_cache_max_rmid = ecx;
> > -				c->x86_cache_occ_scale = ebx;
> > -			}
> > -		} else {
> > -			c->x86_cache_max_rmid = -1;
> > -			c->x86_cache_occ_scale = -1;
> > -		}
> > -	}
> 
> Why are you doing this carving out into a separate function since you're
> keeping the cpuinfo_x86 members?

I just keep the code a bit uniform around the calling area where
a few functions are called. So get_cqm_info() makes the code a bit more
readable.

        init_scattered_cpuid_features(c);
        init_speculation_control(c);
+       get_cqm_info(c);

        /*
         * Clear/Set all flags overridden by options, after probe.
         * This needs to happen each time we re-probe, which may happen
         * several times during CPU initialization.
         */
        apply_forced_caps(c);
}

Maybe not? If the function is not good, I can directly put the code here?

Thanks.

-Fenghua

Re: [PATCH 1/3] x86/resctrl: Get max rmid and occupancy scale directly from CPUID instead of cpuinfo_x86

[Borislav Petkov]

On Mon, Jun 17, 2019 at 01:09:09AM -0700, Fenghua Yu wrote:
> I just keep the code a bit uniform around the calling area where
> a few functions are called. So get_cqm_info() makes the code a bit more
> readable.
> 
>         init_scattered_cpuid_features(c);
>         init_speculation_control(c);
> +       get_cqm_info(c);
> 
>         /*
>          * Clear/Set all flags overridden by options, after probe.
>          * This needs to happen each time we re-probe, which may happen
>          * several times during CPU initialization.
>          */
>         apply_forced_caps(c);
> }
> 
> Maybe not? If the function is not good, I can directly put the code here?

If you want to have it cleaner, make that a separate patch and say so in
the commit message. Patches should do one logical thing and not mix up
different changes which makes review harder.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH 1/3] x86/resctrl: Get max rmid and occupancy scale directly from CPUID instead of cpuinfo_x86

[Fenghua Yu]

On Mon, Jun 17, 2019 at 10:30:48AM +0200, Borislav Petkov wrote:
> On Mon, Jun 17, 2019 at 01:09:09AM -0700, Fenghua Yu wrote:
> > I just keep the code a bit uniform around the calling area where
> > a few functions are called. So get_cqm_info() makes the code a bit more
> > readable.
> > 
> >         init_scattered_cpuid_features(c);
> >         init_speculation_control(c);
> > +       get_cqm_info(c);
> > 
> >         /*
> >          * Clear/Set all flags overridden by options, after probe.
> >          * This needs to happen each time we re-probe, which may happen
> >          * several times during CPU initialization.
> >          */
> >         apply_forced_caps(c);
> > }
> > 
> > Maybe not? If the function is not good, I can directly put the code here?
> 
> If you want to have it cleaner, make that a separate patch and say so in
> the commit message. Patches should do one logical thing and not mix up
> different changes which makes review harder.

So in patch 0001, move the code of getting CQM info from before
calling init_scattered_cpuid_features(c) to after calling the function.

Then in patch 0002, carve out the code of getting CQM info into a
helper function get_cqm_info(c) for cleaner code.

Is this OK? Or the patch 0002 is unnecessary?

Thanks.

-Fenghua

Re: [PATCH 1/3] x86/resctrl: Get max rmid and occupancy scale directly from CPUID instead of cpuinfo_x86

[Borislav Petkov]

On Mon, Jun 17, 2019 at 01:35:02AM -0700, Fenghua Yu wrote:
> Is this OK? Or the patch 0002 is unnecessary?

One patch: carve out and move it where you think it should belong.

-- 
Regards/Gruss,
    Boris.

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