[PATCH V4 0/4] x86/topology: Fix CPUID.1F handling

[PATCH V4 0/4] x86/topology: Fix CPUID.1F handling

[Zhang Rui]

Handling for CPUID.1F is introduced by commit 7745f03eb395 ("x86/topology:
Add CPUID.1F multi-die/package support").
And only SMT/Core/Die level types are supported at that time.

On Intel AlderLake-N platforms where there are Ecores only, the Ecore
Module topology is enumerated via CPUID.1F Module level.

This exposes two bugs in the 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.

Patch 3/4 and 4/4 fixes these two problems in CPUID.1F handling code, and
patch 1/4 and 2/4 are needed to avoid potential regressions.

thanks,
-rui

---
Changes in V4:
 - Drop a fix for a potential issue, as well as document cleanups/updates.
   So this patch series only contains bug fixes (stable candidates).
 - Integrate the patch subject/changelog improvements from Dave.
 - Add Fixes tags.

Changes in V3:
 - changelog improvements based on Peter' feedback
 - Remove combined tags

Changes in V2:
 - fix/improve changelog/comment wording issues
 - reorder the patches to eliminate bisection breakage window
 - add a new patch for coretemp driver variable renaming
 - update coretemp driver patch to fix a case of ida_free(&ida, -2)

[PATCH V4 1/4] hwmon/coretemp: Rename indx to index

[Zhang Rui]

Use variable name 'index' instead of 'indx' for the index in the
core_data[] array.

No functional change expected.

Cc: stable@vger.kernel.org
Suggested-by: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/hwmon/coretemp.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/hwmon/coretemp.c b/drivers/hwmon/coretemp.c
index ccf0af5b988a..bfdcfe8ccb34 100644
--- a/drivers/hwmon/coretemp.c
+++ b/drivers/hwmon/coretemp.c
@@ -515,15 +515,15 @@ coretemp_add_core(struct platform_device *pdev, unsigned int cpu, int pkg_flag)
 		dev_err(&pdev->dev, "Adding Core %u failed\n", cpu);
 }
 
-static void coretemp_remove_core(struct platform_data *pdata, int indx)
+static void coretemp_remove_core(struct platform_data *pdata, int index)
 {
-	struct temp_data *tdata = pdata->core_data[indx];
+	struct temp_data *tdata = pdata->core_data[index];
 
 	/* Remove the sysfs attributes */
 	sysfs_remove_group(&pdata->hwmon_dev->kobj, &tdata->attr_group);
 
-	kfree(pdata->core_data[indx]);
-	pdata->core_data[indx] = NULL;
+	kfree(pdata->core_data[index]);
+	pdata->core_data[index] = NULL;
 }
 
 static int coretemp_probe(struct platform_device *pdev)
@@ -647,7 +647,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 index, target;
 
 	/*
 	 * Don't execute this on suspend as the device remove locks
@@ -661,12 +661,12 @@ static int coretemp_cpu_offline(unsigned int cpu)
 		return 0;
 
 	/* The core id is too big, just return */
-	indx = TO_ATTR_NO(cpu);
-	if (indx > MAX_CORE_DATA - 1)
+	index = TO_ATTR_NO(cpu);
+	if (index > MAX_CORE_DATA - 1)
 		return 0;
 
 	pd = platform_get_drvdata(pdev);
-	tdata = pd->core_data[indx];
+	tdata = pd->core_data[index];
 
 	cpumask_clear_cpu(cpu, &pd->cpumask);
 
@@ -677,7 +677,7 @@ static int coretemp_cpu_offline(unsigned int cpu)
 	 */
 	target = cpumask_any_and(&pd->cpumask, topology_sibling_cpumask(cpu));
 	if (target >= nr_cpu_ids) {
-		coretemp_remove_core(pd, indx);
+		coretemp_remove_core(pd, index);
 	} else if (tdata && tdata->cpu == cpu) {
 		mutex_lock(&tdata->update_lock);
 		tdata->cpu = target;
-- 
2.25.1

Re: [PATCH V4 1/4] hwmon/coretemp: Rename indx to index

[Guenter Roeck]

On 10/14/22 02:01, Zhang Rui wrote:
> Use variable name 'index' instead of 'indx' for the index in the
> core_data[] array.
> 
> No functional change expected.
> 
> Cc: stable@vger.kernel.org
> Suggested-by: Ingo Molnar <mingo@kernel.org>
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>

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

> ---
>   drivers/hwmon/coretemp.c | 18 +++++++++---------
>   1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/hwmon/coretemp.c b/drivers/hwmon/coretemp.c
> index ccf0af5b988a..bfdcfe8ccb34 100644
> --- a/drivers/hwmon/coretemp.c
> +++ b/drivers/hwmon/coretemp.c
> @@ -515,15 +515,15 @@ coretemp_add_core(struct platform_device *pdev, unsigned int cpu, int pkg_flag)
>   		dev_err(&pdev->dev, "Adding Core %u failed\n", cpu);
>   }
>   
> -static void coretemp_remove_core(struct platform_data *pdata, int indx)
> +static void coretemp_remove_core(struct platform_data *pdata, int index)
>   {
> -	struct temp_data *tdata = pdata->core_data[indx];
> +	struct temp_data *tdata = pdata->core_data[index];
>   
>   	/* Remove the sysfs attributes */
>   	sysfs_remove_group(&pdata->hwmon_dev->kobj, &tdata->attr_group);
>   
> -	kfree(pdata->core_data[indx]);
> -	pdata->core_data[indx] = NULL;
> +	kfree(pdata->core_data[index]);
> +	pdata->core_data[index] = NULL;
>   }
>   
>   static int coretemp_probe(struct platform_device *pdev)
> @@ -647,7 +647,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 index, target;
>   
>   	/*
>   	 * Don't execute this on suspend as the device remove locks
> @@ -661,12 +661,12 @@ static int coretemp_cpu_offline(unsigned int cpu)
>   		return 0;
>   
>   	/* The core id is too big, just return */
> -	indx = TO_ATTR_NO(cpu);
> -	if (indx > MAX_CORE_DATA - 1)
> +	index = TO_ATTR_NO(cpu);
> +	if (index > MAX_CORE_DATA - 1)
>   		return 0;
>   
>   	pd = platform_get_drvdata(pdev);
> -	tdata = pd->core_data[indx];
> +	tdata = pd->core_data[index];
>   
>   	cpumask_clear_cpu(cpu, &pd->cpumask);
>   
> @@ -677,7 +677,7 @@ static int coretemp_cpu_offline(unsigned int cpu)
>   	 */
>   	target = cpumask_any_and(&pd->cpumask, topology_sibling_cpumask(cpu));
>   	if (target >= nr_cpu_ids) {
> -		coretemp_remove_core(pd, indx);
> +		coretemp_remove_core(pd, index);
>   	} else if (tdata && tdata->cpu == cpu) {
>   		mutex_lock(&tdata->update_lock);
>   		tdata->cpu = target;

Re: [PATCH V4 1/4] hwmon/coretemp: Rename indx to index

[Dave Hansen]

On 10/14/22 02:01, Zhang Rui wrote:
> Use variable name 'index' instead of 'indx' for the index in the
> core_data[] array.
> 
> No functional change expected.
> 
> Cc: stable@vger.kernel.org
> Suggested-by: Ingo Molnar <mingo@kernel.org>
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>

Hi,

Thanks for paring this series down.

I think I'm also going to pull this patch out of the series before I
apply it and just rework 2/4 to apply cleanly without it.  I just can't
put this in our "urgent" fixes pile and keep the stable@ tag on such a
trivial rename and keep a straight face.

Re: [PATCH V4 1/4] hwmon/coretemp: Rename indx to index

[Guenter Roeck]

On 10/14/22 10:12, Dave Hansen wrote:
> On 10/14/22 02:01, Zhang Rui wrote:
>> Use variable name 'index' instead of 'indx' for the index in the
>> core_data[] array.
>>
>> No functional change expected.
>>
>> Cc: stable@vger.kernel.org
>> Suggested-by: Ingo Molnar <mingo@kernel.org>
>> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> 
> Hi,
> 
> Thanks for paring this series down.
> 
> I think I'm also going to pull this patch out of the series before I
> apply it and just rework 2/4 to apply cleanly without it.  I just can't
> put this in our "urgent" fixes pile and keep the stable@ tag on such a
> trivial rename and keep a straight face.
> 

To be fair, this patch was only submitted as a prerequisite to the next
patch in the series because someone had objected to the use of both 'indx'
and 'index' in the driver.

Guenter

Re: [PATCH V4 1/4] hwmon/coretemp: Rename indx to index

[Zhang Rui]

Hi, Dave,

Thanks for taking care of this.
That totally works for me.

thanks,
rui


On Fri, 2022-10-14 at 10:20 -0700, Guenter Roeck wrote:
> On 10/14/22 10:12, Dave Hansen wrote:
> > On 10/14/22 02:01, Zhang Rui wrote:
> > > Use variable name 'index' instead of 'indx' for the index in the
> > > core_data[] array.
> > > 
> > > No functional change expected.
> > > 
> > > Cc: stable@vger.kernel.org
> > > Suggested-by: Ingo Molnar <mingo@kernel.org>
> > > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > 
> > Hi,
> > 
> > Thanks for paring this series down.
> > 
> > I think I'm also going to pull this patch out of the series before
> > I
> > apply it and just rework 2/4 to apply cleanly without it.  I just
> > can't
> > put this in our "urgent" fixes pile and keep the stable@ tag on
> > such a
> > trivial rename and keep a straight face.
> > 
> 
> To be fair, this patch was only submitted as a prerequisite to the
> next
> patch in the series because someone had objected to the use of both
> 'indx'
> and 'index' in the driver.
> 
> Guenter
> 

[PATCH V4 2/4] 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 ID's.

Cc: stable@vger.kernel.org
Signed-off-by: Zhang Rui <rui.zhang@intel.com>
Acked-by: Len Brown <len.brown@intel.com>
Acked-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/coretemp.c | 56 +++++++++++++++++++++++++++++-----------
 1 file changed, 41 insertions(+), 15 deletions(-)

diff --git a/drivers/hwmon/coretemp.c b/drivers/hwmon/coretemp.c
index bfdcfe8ccb34..291566aeb703 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,9 @@ static void coretemp_remove_core(struct platform_data *pdata, int index)
 
 	kfree(pdata->core_data[index]);
 	pdata->core_data[index] = NULL;
+
+	if (index >= BASE_SYSFS_ATTR_NO)
+		ida_free(&pdata->ida, index - BASE_SYSFS_ATTR_NO);
 }
 
 static int coretemp_probe(struct platform_device *pdev)
@@ -537,6 +554,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 +571,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 +666,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 index, target;
+	int i, index = -1, target;
 
 	/*
 	 * Don't execute this on suspend as the device remove locks
@@ -660,12 +679,19 @@ static int coretemp_cpu_offline(unsigned int cpu)
 	if (!pdev)
 		return 0;
 
-	/* The core id is too big, just return */
-	index = TO_ATTR_NO(cpu);
-	if (index > 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)) {
+			index = i + BASE_SYSFS_ATTR_NO;
+			break;
+		}
+	}
+
+	/* Too many cores and this core is not populated, just return */
+	if (index < 0)
 		return 0;
 
-	pd = platform_get_drvdata(pdev);
 	tdata = pd->core_data[index];
 
 	cpumask_clear_cpu(cpu, &pd->cpumask);
-- 
2.25.1

[PATCH V4 3/4] 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
APIC-ID bits above the enumerated levels are assumed to be package ID
bits.

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

For example, on a single package AlderLake-N, there are 2 Ecore Modules
with 4 atom cores in each module.  Linux does not support the 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 APIC-ID bits above all the CPUID.1F enumerated levels
as package ID.

Fixes: 7745f03eb395 ("x86/topology: Add CPUID.1F multi-die/package support")
Cc: stable@vger.kernel.org
Suggested-by: Len Brown <len.brown@intel.com>
Signed-off-by: Zhang Rui <rui.zhang@intel.com>
Reviewed-by: Len Brown <len.brown@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.25.1

[PATCH V4 4/4] 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, naively using them for array indexes can be problematic.

Fixes: 7745f03eb395 ("x86/topology: Add CPUID.1F multi-die/package support")
Cc: stable@vger.kernel.org
Suggested-by: Len Brown <len.brown@intel.com>
Signed-off-by: Zhang Rui <rui.zhang@intel.com>
Reviewed-by: Len Brown <len.brown@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.25.1