Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] platform/x86/intel/ifs: ifs_init() fails on SAF_GEN=0 CPUs - [Medium] platform/x86/intel/ifs: broken cluster validation in topology_cluster_matched() --
commit d52108059abf4fca7e3a463651949851cf65de59 Author: Qiuxu Zhuo <[email protected]> platform/x86/intel/ifs: Validate TID_BIT_SHIFT matches topology_cluster_cpumask() This commit validates during driver initialization that the logical processors indicated by TID_BIT_SHIFT in MSR_INTEGRITY_CAPS match topology_cluster_cpumask(), failing driver load if they do not match. > diff --git a/drivers/platform/x86/intel/ifs/core.c > b/drivers/platform/x86/intel/ifs/core.c > index f2d2caab04797..c830a02fe7b33 100644 > --- a/drivers/platform/x86/intel/ifs/core.c > +++ b/drivers/platform/x86/intel/ifs/core.c [ ... ] > @@ -69,6 +71,45 @@ static struct ifs_device ifs_devices[] = { > }, > }; > > +/* > + * 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; > +} [Severity: Medium] Can topology_cluster_matched() fail when CPUs in cluster 0 are offline or undergoing hotplug during module initialization? topology_cluster_cpumask(0) returns a pointer to per_cpu(cpu_l2c_shared_map, 0), which only tracks currently online CPUs and is updated in-place by set_cpu_sibling_map() and remove_siblinginfo() under the CPU hotplug lock: arch/x86/kernel/smpboot.c:remove_siblinginfo() { ... for_each_cpu(sibling, cpu_l2c_shared_mask(cpu)) cpumask_clear_cpu(cpu, cpu_l2c_shared_mask(sibling)); ... } Because CONFIG_INTEL_IFS is tristate, ifs_init() can run at module load time without holding cpus_read_lock(). topology_cluster_matched() iterates over cpumask in for_each_cpu_andnot() to compute step and then re-reads cpumask in cpumask_weight(cpumask) to compute cpus, so concurrent CPU hotplug on a cluster-0 sibling can mutate the mask between the two reads. Furthermore, if any CPU in cluster 0 is offline when the module is loaded (for example, when a core or thread in a Dual-Core Module is offline, or when booted with maxcpus=1), cpumask_weight(cpumask) and step will not reflect the full hardware cluster, causing dist != (1 << shift) and failing driver initialization with -EINVAL. In addition, when cpumask contains only CPU 0 (cpus == 1), the fallback: if (!step) step = 2; forces dist to 2, while when cpus >= 2, step >= 1 also makes dist >= 2. Can dist ever match (1 << shift) on a non-SMT single-CPU cluster where TID_BIT_SHIFT is 0? > + > #define IFS_NUMTESTS ARRAY_SIZE(ifs_devices) > > static void ifs_cleanup(void) > @@ -122,6 +163,13 @@ static int __init ifs_init(void) > */ > 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] Won't this check cause ifs_init() to unconditionally fail with -EINVAL on Sapphire Rapids and Emerald Rapids (SAF_GEN=0) processors? On SAF_GEN=0 CPUs, ifs_devices[i].rw_data.generation is 0, so ifs_devices[i].rw_data.all_lp_join is forced to true. However, bits 20:16 (TID_BIT_SHIFT) of MSR_INTEGRITY_CAPS were only introduced in SAF_GEN=2 and read as 0 on SAF_GEN=0 platforms. When ifs_init() calls topology_cluster_matched(msrval) with shift == 0, (1 << shift) is 1, whereas dist = step * cpus evaluates to 2 (both with Hyper-Threading enabled where cpus == 2 and step == 1, and with Hyper-Threading disabled where cpus == 1 and step is forced to 2). As a result, dist != (1 << shift) is always true and topology_cluster_matched() always returns false on SAF_GEN=0 hardware. Should the topology_cluster_matched() check also be gated on ifs_devices[i].rw_data.generation (or only performed when ALL_LP_JOIN is enumerated by hardware)? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=10
