[PATCH 0/7] x86/topology: Improve CPUID.1F handling

[PATCH 0/7] x86/topology: Improve CPUID.1F handling

[Zhang Rui]

On Intel AlderLake-N platforms where there are Ecores only, the Ecore
Module topology is enumerated via CPUID.1F Module level, which has not
been supported by Linux kernel yet.

This exposes two issues in current CPUID.1F handling code.
1. Linux interprets the Module id bits as package id and erroneously
   reports a multi module system as a multi-package system.
2. Linux excludes the unknown Module id bits from the core_id, and results
   in duplicate core_id’s shown in a package after the first issue solved.

Plus that, a third problem is observed on Intel Hybrid ADL-S/P platforms.
The return value of CPUID.1F SMT level EBX (number of siblings) differs on
Pcore CPUs and Ecore CPUs, and results in inconsistent smp_num_siblings
value based on the Pcore/Ecore CPU enumeration order. This could bring
some potential issues although we have not observed any functionalities
issues so far.

Patch 1/7 and 2/7 fix the first two issues. And at the same time, it
reveals a reality that the core_id could be sparse on platforms with
CPUID.1F support.
Patch 3/7 improves coretemp driver code to be able to handle sparse core
id, which is the only driver that uses core_id as array index and run on
platforms with CPUID.1F support.

Patch 4/7 to 7/7 propose a fix for the third problem and update the
related Documents.

The patch series have been tested on Intel ICL/CLX servers, SKL/KBL/ADL
clients.

thanks,
-rui

[PATCH 1/7] x86/topology: Fix multiple packages shown on a single-package system

[Zhang Rui]

CPUID.1F/B does not emumerate Package level explicitly, instead, all the
APICID bits above the enumerated levels are assumed to be package id bits.

Current code gets package id by shifting out all the APICID bits that
Linux supports, rather than shifting out all the APICID bits that CPUID.1F
enumerates. This introduces problems when CPUID.1F enumerates a level that
Linux does not support.

For example, on an AlderLake-N platform, there are 2 Ecore Modules, which
has 4 atom cores in each module, in a single package.
Linux does not support Module level and interprets the Module id bits as
package id and erroneously reports a multi module system as a multi-package
system.

Fix this by using APICID bits above all the CPUID.1F enumerated levels as
package id.

Suggested-and-reviewed-by: Len Brown <len.brown@intel.com>
Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 arch/x86/kernel/cpu/topology.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/cpu/topology.c b/arch/x86/kernel/cpu/topology.c
index 132a2de44d2f..f7592814e5d5 100644
--- a/arch/x86/kernel/cpu/topology.c
+++ b/arch/x86/kernel/cpu/topology.c
@@ -96,6 +96,7 @@ int detect_extended_topology(struct cpuinfo_x86 *c)
 	unsigned int ht_mask_width, core_plus_mask_width, die_plus_mask_width;
 	unsigned int core_select_mask, core_level_siblings;
 	unsigned int die_select_mask, die_level_siblings;
+	unsigned int pkg_mask_width;
 	bool die_level_present = false;
 	int leaf;
 
@@ -111,10 +112,10 @@ int detect_extended_topology(struct cpuinfo_x86 *c)
 	core_level_siblings = smp_num_siblings = LEVEL_MAX_SIBLINGS(ebx);
 	core_plus_mask_width = ht_mask_width = BITS_SHIFT_NEXT_LEVEL(eax);
 	die_level_siblings = LEVEL_MAX_SIBLINGS(ebx);
-	die_plus_mask_width = BITS_SHIFT_NEXT_LEVEL(eax);
+	pkg_mask_width = die_plus_mask_width = BITS_SHIFT_NEXT_LEVEL(eax);
 
 	sub_index = 1;
-	do {
+	while (true) {
 		cpuid_count(leaf, sub_index, &eax, &ebx, &ecx, &edx);
 
 		/*
@@ -132,8 +133,13 @@ int detect_extended_topology(struct cpuinfo_x86 *c)
 			die_plus_mask_width = BITS_SHIFT_NEXT_LEVEL(eax);
 		}
 
+		if (LEAFB_SUBTYPE(ecx) != INVALID_TYPE)
+			pkg_mask_width = BITS_SHIFT_NEXT_LEVEL(eax);
+		else
+			break;
+
 		sub_index++;
-	} while (LEAFB_SUBTYPE(ecx) != INVALID_TYPE);
+	}
 
 	core_select_mask = (~(-1 << core_plus_mask_width)) >> ht_mask_width;
 	die_select_mask = (~(-1 << die_plus_mask_width)) >>
@@ -148,7 +154,7 @@ int detect_extended_topology(struct cpuinfo_x86 *c)
 	}
 
 	c->phys_proc_id = apic->phys_pkg_id(c->initial_apicid,
-				die_plus_mask_width);
+				pkg_mask_width);
 	/*
 	 * Reinit the apicid, now that we have extended initial_apicid.
 	 */
-- 
2.34.1

[PATCH 2/7] x86/topology: Fix duplicated core_id within a package

[Zhang Rui]

Today, core_id is assumed to be unique within each package.

But an AlderLake-N platform adds a Module level between core and package,
Linux excludes the unknown modules bits from the core_id, resulting in
duplicate core_id's.

To keep core_id unique within a package, Linux must include all APIC-id
bits for known or un-known levels above the core and below the package
in the core_id.

It is important to understand that core_id's have always come directly
from the APIC-id encoding, which comes from the BIOS. Thus there is no
guarantee that they start at 0, or that they are contiguous.
As such, using them for array indexes can be problematic.

Suggested-and-reviewed-by: Len Brown <len.brown@intel.com>
Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 arch/x86/kernel/cpu/topology.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/topology.c b/arch/x86/kernel/cpu/topology.c
index f7592814e5d5..5e868b62a7c4 100644
--- a/arch/x86/kernel/cpu/topology.c
+++ b/arch/x86/kernel/cpu/topology.c
@@ -141,7 +141,7 @@ int detect_extended_topology(struct cpuinfo_x86 *c)
 		sub_index++;
 	}
 
-	core_select_mask = (~(-1 << core_plus_mask_width)) >> ht_mask_width;
+	core_select_mask = (~(-1 << pkg_mask_width)) >> ht_mask_width;
 	die_select_mask = (~(-1 << die_plus_mask_width)) >>
 				core_plus_mask_width;
 
-- 
2.34.1

[PATCH 3/7] hwmon/coretemp: Handle large core id value

[Zhang Rui]

The coretemp driver supports up to a hard-coded limit of 128 cores.

Today, the driver can not support a core with an id above that limit.
Yet, the encoding of core_id's is arbitrary (BIOS APIC-id) and so they
may be sparse and they may be large.

Update the driver to map arbitrary core_id numbers into appropriate
array indexes so that 128 cores can be supported, no matter the encoding
of core_ids's.

Acked-by: Len Brown <len.brown@intel.com>
Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/hwmon/coretemp.c | 55 +++++++++++++++++++++++++++++-----------
 1 file changed, 40 insertions(+), 15 deletions(-)

diff --git a/drivers/hwmon/coretemp.c b/drivers/hwmon/coretemp.c
index ccf0af5b988a..3f0f7d7612ae 100644
--- a/drivers/hwmon/coretemp.c
+++ b/drivers/hwmon/coretemp.c
@@ -46,9 +46,6 @@ MODULE_PARM_DESC(tjmax, "TjMax value in degrees Celsius");
 #define TOTAL_ATTRS		(MAX_CORE_ATTRS + 1)
 #define MAX_CORE_DATA		(NUM_REAL_CORES + BASE_SYSFS_ATTR_NO)
 
-#define TO_CORE_ID(cpu)		(cpu_data(cpu).cpu_core_id)
-#define TO_ATTR_NO(cpu)		(TO_CORE_ID(cpu) + BASE_SYSFS_ATTR_NO)
-
 #ifdef CONFIG_SMP
 #define for_each_sibling(i, cpu) \
 	for_each_cpu(i, topology_sibling_cpumask(cpu))
@@ -91,6 +88,8 @@ struct temp_data {
 struct platform_data {
 	struct device		*hwmon_dev;
 	u16			pkg_id;
+	u16			cpu_map[NUM_REAL_CORES];
+	struct ida		ida;
 	struct cpumask		cpumask;
 	struct temp_data	*core_data[MAX_CORE_DATA];
 	struct device_attribute name_attr;
@@ -441,7 +440,7 @@ static struct temp_data *init_temp_data(unsigned int cpu, int pkg_flag)
 							MSR_IA32_THERM_STATUS;
 	tdata->is_pkg_data = pkg_flag;
 	tdata->cpu = cpu;
-	tdata->cpu_core_id = TO_CORE_ID(cpu);
+	tdata->cpu_core_id = topology_core_id(cpu);
 	tdata->attr_size = MAX_CORE_ATTRS;
 	mutex_init(&tdata->update_lock);
 	return tdata;
@@ -454,7 +453,7 @@ static int create_core_data(struct platform_device *pdev, unsigned int cpu,
 	struct platform_data *pdata = platform_get_drvdata(pdev);
 	struct cpuinfo_x86 *c = &cpu_data(cpu);
 	u32 eax, edx;
-	int err, attr_no;
+	int err, index, attr_no;
 
 	/*
 	 * Find attr number for sysfs:
@@ -462,14 +461,26 @@ static int create_core_data(struct platform_device *pdev, unsigned int cpu,
 	 * The attr number is always core id + 2
 	 * The Pkgtemp will always show up as temp1_*, if available
 	 */
-	attr_no = pkg_flag ? PKG_SYSFS_ATTR_NO : TO_ATTR_NO(cpu);
+	if (pkg_flag)
+		attr_no = PKG_SYSFS_ATTR_NO;
+	else {
+		index = ida_alloc(&pdata->ida, GFP_KERNEL);
+		if (index < 0)
+			return index;
+		pdata->cpu_map[index] = topology_core_id(cpu);
+		attr_no = index + BASE_SYSFS_ATTR_NO;
+	}
 
-	if (attr_no > MAX_CORE_DATA - 1)
-		return -ERANGE;
+	if (attr_no > MAX_CORE_DATA - 1) {
+		err = -ERANGE;
+		goto ida_free;
+	}
 
 	tdata = init_temp_data(cpu, pkg_flag);
-	if (!tdata)
-		return -ENOMEM;
+	if (!tdata) {
+		err = -ENOMEM;
+		goto ida_free;
+	}
 
 	/* Test if we can access the status register */
 	err = rdmsr_safe_on_cpu(cpu, tdata->status_reg, &eax, &edx);
@@ -505,6 +516,9 @@ static int create_core_data(struct platform_device *pdev, unsigned int cpu,
 exit_free:
 	pdata->core_data[attr_no] = NULL;
 	kfree(tdata);
+ida_free:
+	if (!pkg_flag)
+		ida_free(&pdata->ida, index);
 	return err;
 }
 
@@ -524,6 +538,8 @@ static void coretemp_remove_core(struct platform_data *pdata, int indx)
 
 	kfree(pdata->core_data[indx]);
 	pdata->core_data[indx] = NULL;
+
+	ida_free(&pdata->ida, indx - BASE_SYSFS_ATTR_NO);
 }
 
 static int coretemp_probe(struct platform_device *pdev)
@@ -537,6 +553,7 @@ static int coretemp_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	pdata->pkg_id = pdev->id;
+	ida_init(&pdata->ida);
 	platform_set_drvdata(pdev, pdata);
 
 	pdata->hwmon_dev = devm_hwmon_device_register_with_groups(dev, DRVNAME,
@@ -553,6 +570,7 @@ static int coretemp_remove(struct platform_device *pdev)
 		if (pdata->core_data[i])
 			coretemp_remove_core(pdata, i);
 
+	ida_destroy(&pdata->ida);
 	return 0;
 }
 
@@ -647,7 +665,7 @@ static int coretemp_cpu_offline(unsigned int cpu)
 	struct platform_device *pdev = coretemp_get_pdev(cpu);
 	struct platform_data *pd;
 	struct temp_data *tdata;
-	int indx, target;
+	int i, indx = -1, target;
 
 	/*
 	 * Don't execute this on suspend as the device remove locks
@@ -660,12 +678,19 @@ static int coretemp_cpu_offline(unsigned int cpu)
 	if (!pdev)
 		return 0;
 
-	/* The core id is too big, just return */
-	indx = TO_ATTR_NO(cpu);
-	if (indx > MAX_CORE_DATA - 1)
+	pd = platform_get_drvdata(pdev);
+
+	for (i = 0; i < NUM_REAL_CORES; i++) {
+		if (pd->cpu_map[i] == topology_core_id(cpu)) {
+			indx = i + BASE_SYSFS_ATTR_NO;
+			break;
+		}
+	}
+
+	/* Too many cores and this core is not pupolated, just return */
+	if (indx < 0)
 		return 0;
 
-	pd = platform_get_drvdata(pdev);
 	tdata = pd->core_data[indx];
 
 	cpumask_clear_cpu(cpu, &pd->cpumask);
-- 
2.34.1

[PATCH 4/7] x86/topology: Fix max_siblings calculation

[Zhang Rui]

The max siblings value returned by CPUID.1F SMT level EBX differs among
CPUs on Intel Hybrid platforms like ADL-S/P.
It returns 2 for Pcore CPUs which have HT sibling and 1 for Ecore CPUs
which do not.

Today, CPUID SMT level EBX sets the global variable smp_num_siblings.
Thus, smp_num_siblings is overridden to different values based on the CPU
Pcore/Ecore enumeration order.

For example,

[    0.201005] detect_extended_topology: CPU APICID 0x0, smp_num_siblings 2, x86_max_cores 10
[    0.201117] start_kernel->check_bugs->cpu_smt_check_topology: smp_num_siblings 2
...
[    0.010146] detect_extended_topology: CPU APICID 0x8, smp_num_siblings 2, x86_max_cores 10
...
[    0.010146] detect_extended_topology: CPU APICID 0x39, smp_num_siblings 2, x86_max_cores 10
[    0.010146] detect_extended_topology: CPU APICID 0x48, smp_num_siblings 1, x86_max_cores 20
...
[    0.010146] detect_extended_topology: CPU APICID 0x4e, smp_num_siblings 1, x86_max_cores 20
[    2.583800] sched_set_itmt_core_prio: smp_num_siblings 1

This inconsistency brings several potential issues:
1. some kernel configuration like cpu_smt_control, as set in
   start_kernel()->check_bugs()->cpu_smt_check_topology(), depends on
   smp_num_siblings set by cpu0.
   It is pure luck that all the current hybrid platforms use Pcore as cpu0
   and hide this problem.
2. some per CPU data like cpuinfo_x86.x86_max_cores that depends on
   smp_num_siblings becomes inconsistent and bogus.
3. the final smp_num_siblings value after boot depends on the last CPU
   enumerated, which could either be Pcore or Ecore CPU.

The solution is to use CPUID EAX bits_shift to get the maximum number of
addressable logical processors, and use this to determin max siblings.
Because:
1. the CPUID EAX bits_shift values are consistent among CPUs as far as
   observed.
2. some code already uses smp_num_siblings value to isolate the SMT ID
   bits in APIC-ID, like apic_id_is_primary_thread().

Suggested-and-reviewed-by: Len Brown <len.brown@intel.com>
Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 arch/x86/kernel/cpu/topology.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kernel/cpu/topology.c b/arch/x86/kernel/cpu/topology.c
index 5e868b62a7c4..2a88f2fa5756 100644
--- a/arch/x86/kernel/cpu/topology.c
+++ b/arch/x86/kernel/cpu/topology.c
@@ -23,7 +23,12 @@
 
 #define LEAFB_SUBTYPE(ecx)		(((ecx) >> 8) & 0xff)
 #define BITS_SHIFT_NEXT_LEVEL(eax)	((eax) & 0x1f)
-#define LEVEL_MAX_SIBLINGS(ebx)		((ebx) & 0xffff)
+
+/*
+ * Use EAX bit_shift to calculate the maximum number of addressable logical
+ * processors sharing the current level.
+ */
+#define LEVEL_MAX_SIBLINGS(eax)		(1 << BITS_SHIFT_NEXT_LEVEL(eax))
 
 unsigned int __max_die_per_package __read_mostly = 1;
 EXPORT_SYMBOL(__max_die_per_package);
@@ -79,7 +84,7 @@ int detect_extended_topology_early(struct cpuinfo_x86 *c)
 	 * initial apic id, which also represents 32-bit extended x2apic id.
 	 */
 	c->initial_apicid = edx;
-	smp_num_siblings = LEVEL_MAX_SIBLINGS(ebx);
+	smp_num_siblings = LEVEL_MAX_SIBLINGS(eax);
 #endif
 	return 0;
 }
@@ -109,9 +114,9 @@ int detect_extended_topology(struct cpuinfo_x86 *c)
 	 */
 	cpuid_count(leaf, SMT_LEVEL, &eax, &ebx, &ecx, &edx);
 	c->initial_apicid = edx;
-	core_level_siblings = smp_num_siblings = LEVEL_MAX_SIBLINGS(ebx);
+	core_level_siblings = smp_num_siblings = LEVEL_MAX_SIBLINGS(eax);
 	core_plus_mask_width = ht_mask_width = BITS_SHIFT_NEXT_LEVEL(eax);
-	die_level_siblings = LEVEL_MAX_SIBLINGS(ebx);
+	die_level_siblings = LEVEL_MAX_SIBLINGS(eax);
 	pkg_mask_width = die_plus_mask_width = BITS_SHIFT_NEXT_LEVEL(eax);
 
 	sub_index = 1;
@@ -122,14 +127,14 @@ int detect_extended_topology(struct cpuinfo_x86 *c)
 		 * Check for the Core type in the implemented sub leaves.
 		 */
 		if (LEAFB_SUBTYPE(ecx) == CORE_TYPE) {
-			core_level_siblings = LEVEL_MAX_SIBLINGS(ebx);
+			core_level_siblings = LEVEL_MAX_SIBLINGS(eax);
 			core_plus_mask_width = BITS_SHIFT_NEXT_LEVEL(eax);
 			die_level_siblings = core_level_siblings;
 			die_plus_mask_width = BITS_SHIFT_NEXT_LEVEL(eax);
 		}
 		if (LEAFB_SUBTYPE(ecx) == DIE_TYPE) {
 			die_level_present = true;
-			die_level_siblings = LEVEL_MAX_SIBLINGS(ebx);
+			die_level_siblings = LEVEL_MAX_SIBLINGS(eax);
 			die_plus_mask_width = BITS_SHIFT_NEXT_LEVEL(eax);
 		}
 
-- 
2.34.1

[PATCH 5/7] Documentation: x86: Update smp_num_siblings/x86_max_cores description

[Zhang Rui]

smp_num_siblings/cpuinfo_x86.x86_max_cores are retrieved via CPUID EAX
bit_shift value, and they represent the maximum possible number of threads
in a core, and the maximum possible number of cores in a package.

Update the smp_num_siblings/cpuinfo_x86.x86_max_cores description in the
documentation.

Reviewed-by: Len Brown <len.brown@intel.com>
Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 Documentation/x86/topology.rst | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/Documentation/x86/topology.rst b/Documentation/x86/topology.rst
index 7f58010ea86a..c5eb5bc42380 100644
--- a/Documentation/x86/topology.rst
+++ b/Documentation/x86/topology.rst
@@ -49,7 +49,8 @@ AMD nomenclature for package is 'Node'.
 
   - cpuinfo_x86.x86_max_cores:
 
-    The number of cores in a package. This information is retrieved via CPUID.
+    The maximum possible number of cores in a package. This information is
+    retrieved via CPUID.
 
   - cpuinfo_x86.x86_max_dies:
 
@@ -102,10 +103,10 @@ AMDs nomenclature for a CMT core is "Compute Unit". The kernel always uses
 
   - smp_num_siblings:
 
-    The number of threads in a core. The number of threads in a package can be
-    calculated by::
+    The maximum possible number of threads in a core. The maximum possible
+    number of threads in a package can be calculated by::
 
-	threads_per_package = cpuinfo_x86.x86_max_cores * smp_num_siblings
+	maximum_threads_per_package = cpuinfo_x86.x86_max_cores * smp_num_siblings
 
 
 Threads
-- 
2.34.1

[PATCH 6/7] Documentation: x86: Remove obsolete x86_max_dies description

[Zhang Rui]

cpuinfo_x86.x86_max_dies is introduced in commit 7745f03eb395
("x86/topology: Add CPUID.1F multi-die/package support") and then
removed in commit 14d96d6c06b5
("x86/topology: Create topology_max_die_per_package()").

Remove the obsolete cpuinfo_x86.x86_max_dies description.

Fixes: 14d96d6c06b5 ("x86/topology: Create topology_max_die_per_package()")
Reviewed-by: Len Brown <len.brown@intel.com>
Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 Documentation/x86/topology.rst | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/Documentation/x86/topology.rst b/Documentation/x86/topology.rst
index c5eb5bc42380..fbef91b1ee5e 100644
--- a/Documentation/x86/topology.rst
+++ b/Documentation/x86/topology.rst
@@ -52,10 +52,6 @@ AMD nomenclature for package is 'Node'.
     The maximum possible number of cores in a package. This information is
     retrieved via CPUID.
 
-  - cpuinfo_x86.x86_max_dies:
-
-    The number of dies in a package. This information is retrieved via CPUID.
-
   - cpuinfo_x86.cpu_die_id:
 
     The physical ID of the die. This information is retrieved via CPUID.
-- 
2.34.1

[PATCH 7/7] perf/x86/intel/P4: Fix smp_num_siblings usage

[Zhang Rui]

smp_num_siblings can be larger than 2.

Any value larger than 1 suggests HT is supported.

Reviewed-by: Len Brown <len.brown@intel.com>
Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 arch/x86/include/asm/perf_event_p4.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/perf_event_p4.h b/arch/x86/include/asm/perf_event_p4.h
index 94de1a05aeba..b14e9a20a7c0 100644
--- a/arch/x86/include/asm/perf_event_p4.h
+++ b/arch/x86/include/asm/perf_event_p4.h
@@ -189,7 +189,7 @@ static inline int p4_ht_active(void)
 static inline int p4_ht_thread(int cpu)
 {
 #ifdef CONFIG_SMP
-	if (smp_num_siblings == 2)
+	if (smp_num_siblings > 1)
 		return cpu != cpumask_first(this_cpu_cpumask_var_ptr(cpu_sibling_map));
 #endif
 	return 0;
-- 
2.34.1

Re: [PATCH 3/7] hwmon/coretemp: Handle large core id value

[Guenter Roeck]

On Sat, Aug 13, 2022 at 12:41:40AM +0800, Zhang Rui wrote:
> The coretemp driver supports up to a hard-coded limit of 128 cores.
> 
> Today, the driver can not support a core with an id above that limit.
> Yet, the encoding of core_id's is arbitrary (BIOS APIC-id) and so they
> may be sparse and they may be large.
> 
> Update the driver to map arbitrary core_id numbers into appropriate
> array indexes so that 128 cores can be supported, no matter the encoding
> of core_ids's.
> 
> Acked-by: Len Brown <len.brown@intel.com>
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> ---
>  drivers/hwmon/coretemp.c | 55 +++++++++++++++++++++++++++++-----------
>  1 file changed, 40 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/hwmon/coretemp.c b/drivers/hwmon/coretemp.c
> index ccf0af5b988a..3f0f7d7612ae 100644
> --- a/drivers/hwmon/coretemp.c
> +++ b/drivers/hwmon/coretemp.c
> @@ -46,9 +46,6 @@ MODULE_PARM_DESC(tjmax, "TjMax value in degrees Celsius");
>  #define TOTAL_ATTRS		(MAX_CORE_ATTRS + 1)
>  #define MAX_CORE_DATA		(NUM_REAL_CORES + BASE_SYSFS_ATTR_NO)
>  
> -#define TO_CORE_ID(cpu)		(cpu_data(cpu).cpu_core_id)
> -#define TO_ATTR_NO(cpu)		(TO_CORE_ID(cpu) + BASE_SYSFS_ATTR_NO)
> -
>  #ifdef CONFIG_SMP
>  #define for_each_sibling(i, cpu) \
>  	for_each_cpu(i, topology_sibling_cpumask(cpu))
> @@ -91,6 +88,8 @@ struct temp_data {
>  struct platform_data {
>  	struct device		*hwmon_dev;
>  	u16			pkg_id;
> +	u16			cpu_map[NUM_REAL_CORES];
> +	struct ida		ida;
>  	struct cpumask		cpumask;
>  	struct temp_data	*core_data[MAX_CORE_DATA];
>  	struct device_attribute name_attr;
> @@ -441,7 +440,7 @@ static struct temp_data *init_temp_data(unsigned int cpu, int pkg_flag)
>  							MSR_IA32_THERM_STATUS;
>  	tdata->is_pkg_data = pkg_flag;
>  	tdata->cpu = cpu;
> -	tdata->cpu_core_id = TO_CORE_ID(cpu);
> +	tdata->cpu_core_id = topology_core_id(cpu);
>  	tdata->attr_size = MAX_CORE_ATTRS;
>  	mutex_init(&tdata->update_lock);
>  	return tdata;
> @@ -454,7 +453,7 @@ static int create_core_data(struct platform_device *pdev, unsigned int cpu,
>  	struct platform_data *pdata = platform_get_drvdata(pdev);
>  	struct cpuinfo_x86 *c = &cpu_data(cpu);
>  	u32 eax, edx;
> -	int err, attr_no;
> +	int err, index, attr_no;
>  
>  	/*
>  	 * Find attr number for sysfs:
> @@ -462,14 +461,26 @@ static int create_core_data(struct platform_device *pdev, unsigned int cpu,
>  	 * The attr number is always core id + 2
>  	 * The Pkgtemp will always show up as temp1_*, if available
>  	 */
> -	attr_no = pkg_flag ? PKG_SYSFS_ATTR_NO : TO_ATTR_NO(cpu);
> +	if (pkg_flag)
> +		attr_no = PKG_SYSFS_ATTR_NO;
> +	else {
> +		index = ida_alloc(&pdata->ida, GFP_KERNEL);
> +		if (index < 0)
> +			return index;
> +		pdata->cpu_map[index] = topology_core_id(cpu);
> +		attr_no = index + BASE_SYSFS_ATTR_NO;
> +	}
>  
> -	if (attr_no > MAX_CORE_DATA - 1)
> -		return -ERANGE;
> +	if (attr_no > MAX_CORE_DATA - 1) {
> +		err = -ERANGE;
> +		goto ida_free;
> +	}
>  
>  	tdata = init_temp_data(cpu, pkg_flag);
> -	if (!tdata)
> -		return -ENOMEM;
> +	if (!tdata) {
> +		err = -ENOMEM;
> +		goto ida_free;
> +	}
>  
>  	/* Test if we can access the status register */
>  	err = rdmsr_safe_on_cpu(cpu, tdata->status_reg, &eax, &edx);
> @@ -505,6 +516,9 @@ static int create_core_data(struct platform_device *pdev, unsigned int cpu,
>  exit_free:
>  	pdata->core_data[attr_no] = NULL;
>  	kfree(tdata);
> +ida_free:
> +	if (!pkg_flag)
> +		ida_free(&pdata->ida, index);
>  	return err;
>  }
>  
> @@ -524,6 +538,8 @@ static void coretemp_remove_core(struct platform_data *pdata, int indx)
>  
>  	kfree(pdata->core_data[indx]);
>  	pdata->core_data[indx] = NULL;
> +
> +	ida_free(&pdata->ida, indx - BASE_SYSFS_ATTR_NO);
>  }
>  
>  static int coretemp_probe(struct platform_device *pdev)
> @@ -537,6 +553,7 @@ static int coretemp_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  
>  	pdata->pkg_id = pdev->id;
> +	ida_init(&pdata->ida);
>  	platform_set_drvdata(pdev, pdata);
>  
>  	pdata->hwmon_dev = devm_hwmon_device_register_with_groups(dev, DRVNAME,
> @@ -553,6 +570,7 @@ static int coretemp_remove(struct platform_device *pdev)
>  		if (pdata->core_data[i])
>  			coretemp_remove_core(pdata, i);
>  
> +	ida_destroy(&pdata->ida);
>  	return 0;
>  }
>  
> @@ -647,7 +665,7 @@ static int coretemp_cpu_offline(unsigned int cpu)
>  	struct platform_device *pdev = coretemp_get_pdev(cpu);
>  	struct platform_data *pd;
>  	struct temp_data *tdata;
> -	int indx, target;
> +	int i, indx = -1, target;
>  
>  	/*
>  	 * Don't execute this on suspend as the device remove locks
> @@ -660,12 +678,19 @@ static int coretemp_cpu_offline(unsigned int cpu)
>  	if (!pdev)
>  		return 0;
>  
> -	/* The core id is too big, just return */
> -	indx = TO_ATTR_NO(cpu);
> -	if (indx > MAX_CORE_DATA - 1)
> +	pd = platform_get_drvdata(pdev);
> +
> +	for (i = 0; i < NUM_REAL_CORES; i++) {
> +		if (pd->cpu_map[i] == topology_core_id(cpu)) {
> +			indx = i + BASE_SYSFS_ATTR_NO;
> +			break;
> +		}
> +	}
> +
> +	/* Too many cores and this core is not pupolated, just return */

populated

Other than that looks good.

Acked-by: Guenter Roeck <linux@roeck-us.net>

> +	if (indx < 0)
>  		return 0;
>  
> -	pd = platform_get_drvdata(pdev);
>  	tdata = pd->core_data[indx];
>  
>  	cpumask_clear_cpu(cpu, &pd->cpumask);
> -- 
> 2.34.1
> 

Re: [PATCH 0/7] x86/topology: Improve CPUID.1F handling

[Ingo Molnar]

* Zhang Rui <rui.zhang@intel.com> wrote:

> On Intel AlderLake-N platforms where there are Ecores only, the Ecore
> Module topology is enumerated via CPUID.1F Module level, which has not
> been supported by Linux kernel yet.
> 
> This exposes two issues in current CPUID.1F handling code.
> 1. Linux interprets the Module id bits as package id and erroneously
>    reports a multi module system as a multi-package system.
> 2. Linux excludes the unknown Module id bits from the core_id, and results
>    in duplicate core_id’s shown in a package after the first issue solved.
> 
> Plus that, a third problem is observed on Intel Hybrid ADL-S/P platforms.
> The return value of CPUID.1F SMT level EBX (number of siblings) differs on
> Pcore CPUs and Ecore CPUs, and results in inconsistent smp_num_siblings
> value based on the Pcore/Ecore CPU enumeration order. This could bring
> some potential issues although we have not observed any functionalities
> issues so far.
> 
> Patch 1/7 and 2/7 fix the first two issues. And at the same time, it
> reveals a reality that the core_id could be sparse on platforms with
> CPUID.1F support.
> Patch 3/7 improves coretemp driver code to be able to handle sparse core
> id, which is the only driver that uses core_id as array index and run on
> platforms with CPUID.1F support.
> 
> Patch 4/7 to 7/7 propose a fix for the third problem and update the
> related Documents.

Yeah, so patch 3/7 probably needs to come first - otherwise there's a 
window for bisection breakage.

Thanks,

	Ingo

Re: [PATCH 3/7] hwmon/coretemp: Handle large core id value

[Ingo Molnar]

* Zhang Rui <rui.zhang@intel.com> wrote:

> The coretemp driver supports up to a hard-coded limit of 128 cores.
> 
> Today, the driver can not support a core with an id above that limit.
> Yet, the encoding of core_id's is arbitrary (BIOS APIC-id) and so they
> may be sparse and they may be large.
> 
> Update the driver to map arbitrary core_id numbers into appropriate
> array indexes so that 128 cores can be supported, no matter the encoding
> of core_ids's.

Please capitalize 'ID' consistently throughout the series.

> -	attr_no = pkg_flag ? PKG_SYSFS_ATTR_NO : TO_ATTR_NO(cpu);
> +	if (pkg_flag)
> +		attr_no = PKG_SYSFS_ATTR_NO;
> +	else {
> +		index = ida_alloc(&pdata->ida, GFP_KERNEL);
> +		if (index < 0)
> +			return index;
> +		pdata->cpu_map[index] = topology_core_id(cpu);
> +		attr_no = index + BASE_SYSFS_ATTR_NO;
> +	}

Unbalanced curly braces.

> -	int err, attr_no;
> +	int err, index, attr_no;

So it's 'index' here.

> @@ -524,6 +538,8 @@ static void coretemp_remove_core(struct platform_data *pdata, int indx)

But 'indx' here.

> -	int indx, target;
> +	int i, indx = -1, target;

And 'indx' again. Did we run out of the letter 'e'? Either use 'index' 
naming consistently, or 'idx' if it has to be abbreviated.

Thanks,

	Ingo

Re: [PATCH 7/7] perf/x86/intel/P4: Fix smp_num_siblings usage

[Ingo Molnar]

* Zhang Rui <rui.zhang@intel.com> wrote:

> smp_num_siblings can be larger than 2.
> 
> Any value larger than 1 suggests HT is supported.
> 
> Reviewed-by: Len Brown <len.brown@intel.com>
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> ---
>  arch/x86/include/asm/perf_event_p4.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/include/asm/perf_event_p4.h b/arch/x86/include/asm/perf_event_p4.h
> index 94de1a05aeba..b14e9a20a7c0 100644
> --- a/arch/x86/include/asm/perf_event_p4.h
> +++ b/arch/x86/include/asm/perf_event_p4.h
> @@ -189,7 +189,7 @@ static inline int p4_ht_active(void)
>  static inline int p4_ht_thread(int cpu)
>  {
>  #ifdef CONFIG_SMP
> -	if (smp_num_siblings == 2)
> +	if (smp_num_siblings > 1)
>  		return cpu != cpumask_first(this_cpu_cpumask_var_ptr(cpu_sibling_map));

This fix too should probably come before all the other changes.

(Not that Pentium 4 code is expected to ever see such SMT thread values.)

Thanks,

	Ingo

Re: [PATCH 3/7] hwmon/coretemp: Handle large core id value

[Zhang Rui]

On Sat, 2022-08-13 at 12:48 +0200, Ingo Molnar wrote:
> 
> * Zhang Rui <rui.zhang@intel.com> wrote:
> 
> > The coretemp driver supports up to a hard-coded limit of 128 cores.
> > 
> > Today, the driver can not support a core with an id above that
> > limit.
> > Yet, the encoding of core_id's is arbitrary (BIOS APIC-id) and so
> > they
> > may be sparse and they may be large.
> > 
> > Update the driver to map arbitrary core_id numbers into appropriate
> > array indexes so that 128 cores can be supported, no matter the
> > encoding
> > of core_ids's.
> 
> Please capitalize 'ID' consistently throughout the series.
> 
> > -       attr_no = pkg_flag ? PKG_SYSFS_ATTR_NO : TO_ATTR_NO(cpu);
> > +       if (pkg_flag)
> > +               attr_no = PKG_SYSFS_ATTR_NO;
> > +       else {
> > +               index = ida_alloc(&pdata->ida, GFP_KERNEL);
> > +               if (index < 0)
> > +                       return index;
> > +               pdata->cpu_map[index] = topology_core_id(cpu);
> > +               attr_no = index + BASE_SYSFS_ATTR_NO;
> > +       }
> 
> Unbalanced curly braces.

Sure, will fix these two issues in next version.

> 
> > -       int err, attr_no;
> > +       int err, index, attr_no;
> 
> So it's 'index' here.
> 
> > @@ -524,6 +538,8 @@ static void coretemp_remove_core(struct
> > platform_data *pdata, int indx)
> 
> But 'indx' here.
> 
> > -       int indx, target;
> > +       int i, indx = -1, target;
> 
> And 'indx' again. Did we run out of the letter 'e'? Either use
> 'index' 
> naming consistently, or 'idx' if it has to be abbreviated.

I'd prefer 'index', but here, this 'indx' is from previous code and
this patch just initializes it to -1.

thanks,
rui
> 
> Thanks,
> 
>         Ingo

Re: [PATCH 0/7] x86/topology: Improve CPUID.1F handling

[Zhang Rui]

Hi, Ingo,

Thanks for reviewing this patch series.

On Sat, 2022-08-13 at 12:44 +0200, Ingo Molnar wrote:
> 
> * Zhang Rui <rui.zhang@intel.com> wrote:
> 
> > On Intel AlderLake-N platforms where there are Ecores only, the
> > Ecore
> > Module topology is enumerated via CPUID.1F Module level, which has
> > not
> > been supported by Linux kernel yet.
> > 
> > This exposes two issues in current CPUID.1F handling code.
> > 1. Linux interprets the Module id bits as package id and
> > erroneously
> >    reports a multi module system as a multi-package system.
> > 2. Linux excludes the unknown Module id bits from the core_id, and
> > results
> >    in duplicate core_id’s shown in a package after the first issue
> > solved.
> > 
> > Plus that, a third problem is observed on Intel Hybrid ADL-S/P
> > platforms.
> > The return value of CPUID.1F SMT level EBX (number of siblings)
> > differs on
> > Pcore CPUs and Ecore CPUs, and results in inconsistent
> > smp_num_siblings
> > value based on the Pcore/Ecore CPU enumeration order. This could
> > bring
> > some potential issues although we have not observed any
> > functionalities
> > issues so far.
> > 
> > Patch 1/7 and 2/7 fix the first two issues. And at the same time,
> > it
> > reveals a reality that the core_id could be sparse on platforms
> > with
> > CPUID.1F support.
> > Patch 3/7 improves coretemp driver code to be able to handle sparse
> > core
> > id, which is the only driver that uses core_id as array index and
> > run on
> > platforms with CPUID.1F support.
> > 
> > Patch 4/7 to 7/7 propose a fix for the third problem and update the
> > related Documents.
> 
> Yeah, so patch 3/7 probably needs to come first - otherwise there's a
> window for bisection breakage.

Sure, I will re-arrange this.


thanks,
rui

Re: [PATCH 3/7] hwmon/coretemp: Handle large core id value

[Zhang Rui]

Hi, Guenter,

On Fri, 2022-08-12 at 10:16 -0700, Guenter Roeck wrote:
> 
> > -       /* The core id is too big, just return */
> > -       indx = TO_ATTR_NO(cpu);
> > -       if (indx > MAX_CORE_DATA - 1)
> > +       pd = platform_get_drvdata(pdev);
> > +
> > +       for (i = 0; i < NUM_REAL_CORES; i++) {
> > +               if (pd->cpu_map[i] == topology_core_id(cpu)) {
> > +                       indx = i + BASE_SYSFS_ATTR_NO;
> > +                       break;
> > +               }
> > +       }
> > +
> > +       /* Too many cores and this core is not pupolated, just
> > return */
> 
> populated
> 
> Other than that looks good.
> 
> Acked-by: Guenter Roeck <linux@roeck-us.net>

Thanks for reviewing, will fix in next version.

thanks,
rui

Re: [PATCH 7/7] perf/x86/intel/P4: Fix smp_num_siblings usage

[Zhang Rui]

Hi, Ingo,

On Sat, 2022-08-13 at 12:50 +0200, Ingo Molnar wrote:
> 
> * Zhang Rui <rui.zhang@intel.com> wrote:
> 
> > smp_num_siblings can be larger than 2.
> > 
> > Any value larger than 1 suggests HT is supported.
> > 
> > Reviewed-by: Len Brown <len.brown@intel.com>
> > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > ---
> >  arch/x86/include/asm/perf_event_p4.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/arch/x86/include/asm/perf_event_p4.h
> > b/arch/x86/include/asm/perf_event_p4.h
> > index 94de1a05aeba..b14e9a20a7c0 100644
> > --- a/arch/x86/include/asm/perf_event_p4.h
> > +++ b/arch/x86/include/asm/perf_event_p4.h
> > @@ -189,7 +189,7 @@ static inline int p4_ht_active(void)
> >  static inline int p4_ht_thread(int cpu)
> >  {
> >  #ifdef CONFIG_SMP
> > -       if (smp_num_siblings == 2)
> > +       if (smp_num_siblings > 1)
> >                 return cpu !=
> > cpumask_first(this_cpu_cpumask_var_ptr(cpu_sibling_map));
> 
> This fix too should probably come before all the other changes.
> 
> (Not that Pentium 4 code is expected to ever see such SMT thread
> values.)
> 
Do you mean that this is a clean fix, and there is no reason for this
patch to be blocked by any of the other patches in this series?

thanks,
rui

Re: [PATCH 3/7] hwmon/coretemp: Handle large core id value

[Ingo Molnar]

* Zhang Rui <rui.zhang@intel.com> wrote:

> On Sat, 2022-08-13 at 12:48 +0200, Ingo Molnar wrote:
> > 
> > * Zhang Rui <rui.zhang@intel.com> wrote:
> > 
> > > The coretemp driver supports up to a hard-coded limit of 128 cores.
> > > 
> > > Today, the driver can not support a core with an id above that
> > > limit.
> > > Yet, the encoding of core_id's is arbitrary (BIOS APIC-id) and so
> > > they
> > > may be sparse and they may be large.
> > > 
> > > Update the driver to map arbitrary core_id numbers into appropriate
> > > array indexes so that 128 cores can be supported, no matter the
> > > encoding
> > > of core_ids's.
> > 
> > Please capitalize 'ID' consistently throughout the series.
> > 
> > > -       attr_no = pkg_flag ? PKG_SYSFS_ATTR_NO : TO_ATTR_NO(cpu);
> > > +       if (pkg_flag)
> > > +               attr_no = PKG_SYSFS_ATTR_NO;
> > > +       else {
> > > +               index = ida_alloc(&pdata->ida, GFP_KERNEL);
> > > +               if (index < 0)
> > > +                       return index;
> > > +               pdata->cpu_map[index] = topology_core_id(cpu);
> > > +               attr_no = index + BASE_SYSFS_ATTR_NO;
> > > +       }
> > 
> > Unbalanced curly braces.
> 
> Sure, will fix these two issues in next version.
> 
> > 
> > > -       int err, attr_no;
> > > +       int err, index, attr_no;
> > 
> > So it's 'index' here.
> > 
> > > @@ -524,6 +538,8 @@ static void coretemp_remove_core(struct
> > > platform_data *pdata, int indx)
> > 
> > But 'indx' here.
> > 
> > > -       int indx, target;
> > > +       int i, indx = -1, target;
> > 
> > And 'indx' again. Did we run out of the letter 'e'? Either use
> > 'index' 
> > naming consistently, or 'idx' if it has to be abbreviated.
> 
> I'd prefer 'index', but here, this 'indx' is from previous code and
> this patch just initializes it to -1.

Then queue up a cleanup patch as patch #1 - but idiosyncratic noise like 
that makes review harder.

Thanks,

	Ingo

Re: [PATCH 7/7] perf/x86/intel/P4: Fix smp_num_siblings usage

[Peter Zijlstra]

On Sat, Aug 13, 2022 at 12:41:44AM +0800, Zhang Rui wrote:
> smp_num_siblings can be larger than 2.

Not on a P4 it can't ;-)

> 
> Any value larger than 1 suggests HT is supported.
> 
> Reviewed-by: Len Brown <len.brown@intel.com>
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> ---
>  arch/x86/include/asm/perf_event_p4.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/include/asm/perf_event_p4.h b/arch/x86/include/asm/perf_event_p4.h
> index 94de1a05aeba..b14e9a20a7c0 100644
> --- a/arch/x86/include/asm/perf_event_p4.h
> +++ b/arch/x86/include/asm/perf_event_p4.h
> @@ -189,7 +189,7 @@ static inline int p4_ht_active(void)
>  static inline int p4_ht_thread(int cpu)
>  {
>  #ifdef CONFIG_SMP
> -	if (smp_num_siblings == 2)
> +	if (smp_num_siblings > 1)
>  		return cpu != cpumask_first(this_cpu_cpumask_var_ptr(cpu_sibling_map));
>  #endif
>  	return 0;

Unless Intel plans to respin an P4 with extra siblings on, I don't think
this qualifies for the word 'fix'.

Re: [PATCH 7/7] perf/x86/intel/P4: Fix smp_num_siblings usage

[Zhang Rui]

On Mon, 2022-08-15 at 11:11 +0200, Peter Zijlstra wrote:
> On Sat, Aug 13, 2022 at 12:41:44AM +0800, Zhang Rui wrote:
> > smp_num_siblings can be larger than 2.
> 
> Not on a P4 it can't ;-)

Kernel code doesn't prevent this from happening, so it just depends on
how SMT ID is encoded in APICID.
Checking for smp_num_sibling > 1 is the right logic to detect HT
support, which is followed by all other kernel code except this one. :)

thanks,
rui
> 
> > Any value larger than 1 suggests HT is supported.
> > 
> > Reviewed-by: Len Brown <len.brown@intel.com>
> > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > ---
> >  arch/x86/include/asm/perf_event_p4.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/arch/x86/include/asm/perf_event_p4.h
> > b/arch/x86/include/asm/perf_event_p4.h
> > index 94de1a05aeba..b14e9a20a7c0 100644
> > --- a/arch/x86/include/asm/perf_event_p4.h
> > +++ b/arch/x86/include/asm/perf_event_p4.h
> > @@ -189,7 +189,7 @@ static inline int p4_ht_active(void)
> >  static inline int p4_ht_thread(int cpu)
> >  {
> >  #ifdef CONFIG_SMP
> > -	if (smp_num_siblings == 2)
> > +	if (smp_num_siblings > 1)
> >  		return cpu !=
> > cpumask_first(this_cpu_cpumask_var_ptr(cpu_sibling_map));
> >  #endif
> >  	return 0;
> 
> Unless Intel plans to respin an P4 with extra siblings on, I don't
> think
> this qualifies for the word 'fix'.

Re: [PATCH 7/7] perf/x86/intel/P4: Fix smp_num_siblings usage

[Peter Zijlstra]

On Tue, Aug 16, 2022 at 10:26:19AM +0800, Zhang Rui wrote:
> On Mon, 2022-08-15 at 11:11 +0200, Peter Zijlstra wrote:
> > On Sat, Aug 13, 2022 at 12:41:44AM +0800, Zhang Rui wrote:
> > > smp_num_siblings can be larger than 2.
> > 
> > Not on a P4 it can't ;-)
> 
> Kernel code doesn't prevent this from happening, so it just depends on
> how SMT ID is encoded in APICID.

No P4 ever encountered had it different and since no P4 will ever be
made again (I sincerely hope) we have a complete case.

> Checking for smp_num_sibling > 1 is the right logic to detect HT
> support, which is followed by all other kernel code except this one. :)

I'm fine with making it consistent, I'm arguing with the subject calling
this a fix, it is not, it's a functional no-op.