Re: [PATCH 17/17] Documentation/ABI: Update IFS ABI doc

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] platform/x86: intel/ifs: ifs_init() fails on gen0 due to cluster check
- [High] platform/x86: intel/ifs: do_core_test() updates grp_cpumask before checks
--

commit d3afda8246f048aa9feb139ad53fbb292e1d53d9
Author: Qiuxu Zhuo <qiuxu.zhuo@intel.com>

Documentation/ABI: Update IFS ABI doc

This series updates the Intel In-Field Scan (IFS) driver to support CPU
clusters sharing a scan test engine, validates cluster topology against
MSR_INTEGRITY_CAPS, tracks per-CPU scan results, and adds sysfs list
interfaces along with their documentation.

> diff --git a/drivers/platform/x86/intel/ifs/core.c b/drivers/platform/x86/intel/ifs/core.c
> index 3dafa28a10104..5c9c86e048371 100644
> --- a/drivers/platform/x86/intel/ifs/core.c
> +++ b/drivers/platform/x86/intel/ifs/core.c

[ ... ]

> +/*
> + * Test whether logical processors indicated by TID_BIT_SHIFT that share the
> + * same SCAN test engine match the cpumask from topology_cluster_cpumask().
> + */
> +static bool topology_cluster_matched(u32 msrval)
> +{
> +	u32 dist, shift = FIELD_GET(MSR_INTEGRITY_CAPS_TID_BIT_SHIFT_MASK, msrval);
> +	const cpumask_t *cpumask = topology_cluster_cpumask(0);
> +	u32 delta, step = 0, first = cpu_data(0).topo.apicid;
> +	unsigned int cpu, cpus;
> +
> +	/* Get the minimal APIC ID step among the CPUs in the same cluster. */
> +	for_each_cpu_andnot(cpu, cpumask, cpumask_of(0)) {
> +		delta = abs(cpu_data(cpu).topo.apicid - first);
> +
> +		if (!step) {
> +			step = delta;
> +			continue;
> +		}
> +
> +		if (step > delta)
> +			step = delta;
> +	}
> +
> +	/* A Hyper-Threading (HT) system with HT disabled. */
> +	if (!step)
> +		step = 2;
> +
> +	cpus = cpumask_weight(cpumask);
> +	dist = step * cpus;
> +
> +	if (dist != (1 << shift)) {
> +		pr_err_once("APICID step %u, group cpus %u, TID_BIT_SHIFT %u\n", step, cpus, shift);
> +		return false;
> +	}
> +
> +	return true;
> +}

[ ... ]

> @@ -134,11 +157,33 @@ static int __init ifs_init(void)
>  			continue;
>  		ifs_devices[i].rw_data.generation = FIELD_GET(MSR_INTEGRITY_CAPS_SAF_GEN_MASK,
>  							      msrval);
> +		/*
> +		 * For Sapphire Rapids and Emerald Rapids (SAF_GEN=0), the ALL_LP_JOIN
> +		 * indicator is unavailable. However, all the threads within a core still
> +		 * need to run IFS simultaneously, so the variable 'all_lp_join' is
> +		 * forced to true.
> +		 *
> +		 * For Granite Rapids and later generations (SAF_GEN=2), the ALL_LP_JOIN
> +		 * indicator is available, the variable 'all_lp_join' is determined by
> +		 * this indicator.
> +		 */
> +		ifs_devices[i].rw_data.all_lp_join = ifs_devices[i].rw_data.generation ?
> +						(msrval & MSR_INTEGRITY_CAPS_ALL_LP_JOIN) : true;
> +
> +		if (ifs_devices[i].rw_data.all_lp_join && !topology_cluster_matched(msrval)) {
> +			pr_err_once("TID_BIT_SHIFT mismatched topology_cluster_cpumask().\n");
> +			ret = -EINVAL;
> +			goto err_exit;
> +		}

[Severity: High]
Will ifs_init() -> topology_cluster_matched() cause a regression where driver
initialization unconditionally fails with -EINVAL on all Sapphire Rapids and
Emerald Rapids (generation == 0) processors?

In ifs_init(), when ifs_devices[i].rw_data.generation is 0, all_lp_join is
forced to true, and topology_cluster_matched() is then called without checking
whether generation is non-zero.

On generation 0 platforms, bits [20:16] (MSR_INTEGRITY_CAPS_TID_BIT_SHIFT_MASK)
of MSR_INTEGRITY_CAPS are reserved and read as 0, so shift is 0 and (1 << shift)
is 1. However, in topology_cluster_matched(), dist = step * cpus is always at
least 2 (when SMT is enabled, cpus >= 2 and step >= 1; when SMT is disabled,
cpus == 1 and step is set to 2). As a result, dist != (1 << shift) always
evaluates to true and ifs_init() aborts with -EINVAL.

Additionally, on generation >= 2 systems, topology_cluster_matched() computes
cpus = cpumask_weight(topology_cluster_cpumask(0)) using only currently online
CPUs and assumes !step always means 2-way SMT disabled (step = 2). If any
sibling thread or core in CPU 0's cluster is offline when the module loads,
won't dist != (1 << shift) also fail initialization?

> diff --git a/drivers/platform/x86/intel/ifs/runtest.c b/drivers/platform/x86/intel/ifs/runtest.c
> index dfc119d7354d7..d5303dd722333 100644
> --- a/drivers/platform/x86/intel/ifs/runtest.c
> +++ b/drivers/platform/x86/intel/ifs/runtest.c

[ ... ]

> +static void build_cpugroup_mask(struct device *dev, int cpu)
>  {
> -	union ifs_sbaf_status status = (union ifs_sbaf_status)status_data;
> +	struct ifs_data *ifsd = ifs_get_data(dev);
>  

[ ... ]

> +	cpumask_clear(&ifsd->grp_cpumask);
>  
>  	/*
> -	 * This WRMSR will wait for other HT threads to also write
> -	 * to this MSR (at most for activate.delay cycles). Then it
> -	 * starts scan of each requested bundle. The core test happens
> -	 * during the "execution" of the WRMSR.
> +	 * Some CPUs, such as Sierra Forest and Clearwater Forest, have multiple cores
> +	 * sharing the same SCAN test engine, but they don't require a rendezvous.
>  	 */

[ ... ]

> +	if (!ifsd->all_lp_join) {
> +		cpumask_set_cpu(cpu, &ifsd->grp_cpumask);
> +		return;
>  	}
>  

[ ... ]

> +	cpumask_copy(&ifsd->grp_cpumask, topology_cluster_cpumask(cpu));
>  }
>  
>  /*
> - * Initiate per core test. It wakes up work queue threads on the target cpu and
> - * its sibling cpu. Once all sibling threads wake up, the scan test gets executed and
> + * Initiate per core test. It wakes up work queue threads on the target CPU and
> + * its sibling CPU. Once all sibling threads wake up, the scan test gets executed and
>   * wait for all sibling threads to finish the scan test.
>   */
>  int do_core_test(int cpu, struct device *dev)
> @@ -628,9 +493,10 @@ int do_core_test(int cpu, struct device *dev)
>  
>  	/* Prevent CPUs from being taken offline during the scan test */
>  	cpus_read_lock();
> +	build_cpugroup_mask(dev, cpu);
>  
>  	if (!cpu_online(cpu)) {
> -		dev_info(dev, "cannot test on the offline cpu %d\n", cpu);
> +		dev_info(dev, "cannot test on the offline CPU %d\n", cpu);
>  		ret = -EINVAL;
>  		goto out;
>  	}

[Severity: High]
Can calling build_cpugroup_mask() in do_core_test() before checking
cpu_online(cpu) and ifsd->loaded dereference a NULL cpumask pointer or corrupt
ifsd->grp_cpumask?

In drivers/platform/x86/intel/ifs/sysfs.c, run_test_store() only verifies
cpu < nr_cpu_ids before calling do_core_test(cpu, dev):

run_test_store()
  do_core_test(cpu, dev)
    build_cpugroup_mask(dev, cpu)

If user space writes a CPU number that is < nr_cpu_ids but not in
cpu_possible_mask:

1) When ifsd->all_lp_join is true, build_cpugroup_mask() calls
cpumask_copy(&ifsd->grp_cpumask, topology_cluster_cpumask(cpu)), which reads
per_cpu(cpu_l2c_shared_map, cpu). Because cpu_l2c_shared_map is only allocated
for possible CPUs in smp_prepare_cpus_common(), won't this dereference a NULL
pointer when CONFIG_CPUMASK_OFFSTACK=y?

2) When ifsd->all_lp_join is false, build_cpugroup_mask() sets the non-possible
CPU in ifsd->grp_cpumask before the !cpu_online(cpu) check returns -EINVAL.
Subsequent reads of status_list_show(), details_list_show(), and
addnl_details_list_show() in drivers/platform/x86/intel/ifs/sysfs.c will then
iterate over ifsd->grp_cpumask and call per_cpu_ptr(ifsd->result_ptr, lcpu) on
an unallocated per-CPU slot.

Even when cpu is possible, if cpu is offline (-EINVAL) or !ifsd->loaded (-EPERM
in the IFS_TYPE_SAF case), build_cpugroup_mask() has already overwritten
ifsd->grp_cpumask, causing cpus_ran_list, status_list, details_list, and
addnl_details_list to report stale or untested CPUs instead of "none" or the
previous test's group.

Should build_cpugroup_mask() be moved after the cpu_online(cpu) and
ifsd->loaded checks?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026183820.129459-1-ehab.ababneh@intel.com?part=17