Re: [RFC PATCH v3 02/13] drivers/base/arch_topology: Add support for initializing sbm topology

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] topology: 64-bit fw_token truncated to 32-bit int causing ID collisions
- [High] topology: hardcoded 'cpu == 0' fails initialization when CPU 0 is not possible
- [Medium] topology: memory leak of 'count' on xa_store() failure
- [Medium] topology: topology uniformity check silently ignores the first CPU
- [Medium] topology: mismatched cache type filtering causes inconsistent SBM domains
--

commit 3748179aae956b072baf4016333e144175bba72f
Author: K Prateek Nayak <kprateek.nayak@amd.com>
Subject: drivers/base/arch_topology: Add support for initializing sbm topology

This commit introduces support for initializing sparsebitmap (sbm) topology
for architectures supporting GENERIC_ARCH_TOPOLOGY. It counts the number of
unique LLCs or packages and the maximum threads per instance to set the sbm
processor configuration.

> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
> index 8c5e47c28d9a3..f55745a932986 100644
> --- a/drivers/base/arch_topology.c
> +++ b/drivers/base/arch_topology.c

[ ... ]

> @@ -930,6 +931,114 @@ __weak int __init parse_acpi_topology(void)
>  int arch_sbm_cpu_instance_id(int cpu)
>  {
>  	if (last_level_cache_is_valid(cpu)) {
>  		struct cacheinfo *llc_info = get_cpu_cacheinfo_llc(cpu);
>  
>  		if (!llc_info)
>  			goto out;
>  
>  		if (llc_info->attributes & CACHE_ID)
>  			return llc_info->id;
>  
>  		/*
>  		 * XXX: fw_token be truncated from cast
>  		 * when the value is returned as an int.
>  		 */
>  		return (int)((long)llc_info->fw_token);

[Severity: High]
Does this explicit truncation risk SBM topology corruption?

On 64-bit architectures using ACPI PPTT for cache topology, fw_token contains
a 64-bit pointer. By casting it down to a 32-bit int, the upper 32 bits are
dropped. If two different cache structures collide in their lower 32 bits,
CPUs that do not share an LLC might be incorrectly grouped into the same
instance ID.

>  	}
>  out:
>  	return cpu_topology[cpu].package_id;
>  }

[ ... ]

>  static void __init init_sbm_topology(void)
>  {
>  	int num_sbm_instances = 0, max_threads_per_instance = -1;
>  	bool has_cache = false, has_package = false;
>  	cpumask_var_t unique_cpus;
>  	struct xarray instances;
>  	unsigned long cpu, *count;

[ ... ]

>  	for_each_possible_cpu(cpu) {
>  		bool found = false;
>  		int unique_cpu;
>  
>  		for_each_cpu(unique_cpu, unique_cpus) {

[Severity: Medium]
Will this silently ignore the first CPU for the topology uniformity check?

When the first CPU is processed, unique_cpus is empty, so this inner
for_each_cpu loop is completely skipped. Since has_cache and has_package
are only updated inside this loop, the cache state for the first CPU is
never recorded.

If a subsequent CPU has a conflicting topology, won't the has_cache &&
has_package check fail to detect the mismatch?

>  			/*
>  			 * XXX: Assumes last_level_cache_is_valid() is uniformly true
>  			 * across the entire system if it is true for one CPU.
>  			 */
>  			if (last_level_cache_is_valid(cpu)) {

[Severity: Medium]
Are we using mismatched cache type filtering rules?

Here in init_sbm_topology(), last_level_cache_is_valid() is used, which does
not filter out instruction caches. However, arch_sbm_cpu_instance_id() uses
get_cpu_cacheinfo_llc(), which strictly filters out instruction caches and
falls back to package_id.

If the LLC is an instruction cache, won't the domain counting diverge from ID
assignment, leading to inconsistent SBM domains?

>  				has_cache = true;
>  				if (last_level_cache_is_shared(cpu, unique_cpu)) {
>  					found = true;
>  					break;
>  				}

[ ... ]

>  		if (!found) {
>  			count = kzalloc_obj(*count);
>  			if (!count)
>  				goto out;
>  
>  			cpumask_set_cpu(cpu, unique_cpus);
>  			*count += 1;
>  
>  			xa_store(&instances, cpu, count, GFP_KERNEL);
>  			continue;

[Severity: Medium]
Does this code leak the newly allocated count object if xa_store() fails?

The return value of xa_store() can be an error pointer if internal memory
allocation fails. If xa_store() fails here, count is not added to the xarray,
but we continue the loop without freeing it, resulting in a memory leak.

>  		}

[ ... ]

>  void __init init_cpu_topology(void)
>  {
>  	int cpu, ret;

[ ... ]

>  	/*
>  	 * If fetch_cache_info() fails for first CPU,
>  	 * init_cpu_sbm_topology() will use pacakge_id instead.
>  	 *
>  	 * Uniform cache topology is a necessary since implementation
>  	 * assumes last_level_cache_is_valid() gives same result for
>  	 * all possible CPUs
>  	 */
>  	if (!ret || (cpu == 0 && ret == -ENOENT))
>  		init_sbm_topology();

[Severity: High]
Can this hardcoded check for cpu == 0 silently bypass SBM initialization if
CPU 0 is offline or not possible?

If fetch_cache_info() fails with -ENOENT for the first possible CPU, the
loop preceding this block breaks and leaves that CPU in the cpu variable.
If the first possible CPU is greater than 0, cpu == 0 evaluates to false,
and init_sbm_topology() will never be called.

Should this dynamically resolve the first valid CPU via
cpumask_first(cpu_possible_mask) instead?

>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261001192849.74788-1-kprateek.nayak@amd.com?part=2