Re: [RFC PATCH v3 06/13] powerpc/setup: Initialize sbm topology based on coregroup / NUMA topology

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] powerpc/setup: uninitialized num_sbm_instances in setup_sbm_topology()
- [High] powerpc/setup: unconditional dereference of smp_ops causes boot panic
- [Medium] powerpc/pseries: sizing SBM by num_online_nodes() breaks memory hotplug
--

commit a4d9dadc8cce1e3232c36f0087110489e579c9b4
Author: K Prateek Nayak <kprateek.nayak@amd.com>

powerpc/setup: Initialize sbm topology based on coregroup / NUMA topology

This commit initializes the sparsebitmap (sbm) topology based on coregroup
information, with each coregroup getting its own sparsemask leaf. For pSeries,
NUMA topology is used instead to account for CPU hotplug behavior.

> diff --git a/arch/powerpc/kernel/setup-common.c b/arch/powerpc/kernel/setup-common.c
> index 4afaba19b5867..4b57ad553172d 100644
> --- a/arch/powerpc/kernel/setup-common.c
> +++ b/arch/powerpc/kernel/setup-common.c
[ ... ]
> @@ -602,6 +603,87 @@ static __init int add_pcspkr(void)
>  device_initcall(add_pcspkr);
>  #endif	/* CONFIG_PCSPKR_PLATFORM */
>  
> +int arch_sbm_cpu_instance_id(int cpu)
> +{
> +	/*
> +	 * In case of pSeries processors, sbm masks are
> +	 * grouped by nodes where the cpuhotplug
> +	 * operations can remove and re-add same logical
> +	 * CPUs on different nodes.
> +	 *
> +	 * See comment in pseries_cpu_hotplug_init().
> +	 */
> +	if (smp_ops->cpu_disable)
> +		return cpu_to_node(cpu);

[Severity: High]
Is it possible for smp_ops to be NULL here?

If the platform does not initialize SMP (such as on UP platforms), evaluating
smp_ops->cpu_disable unconditionally can cause an immediate boot panic.

Additionally, on !CONFIG_SMP builds, struct smp_ops_t might be an incomplete
type, which can result in compilation errors.

> +
> +	return cpu_to_coregroup_id(cpu);
> +}
> +
> +static void __init setup_sbm_topology(void)
> +{
> +	int num_sbm_instances, max_threads_per_instance = 1;

[ ... ]

> +	/*
> +	 * If CPUs are disabled, they may pop up on any online node.
> +	 *
> +	 * XXX: Any implementation nuances that can help this?
> +	 * pSeries says only online nodes can be extended.
> +	 */
> +	if (disabled_cpus) {
> +		num_sbm_instances = num_sbm_instances + disabled_cpus;

[Severity: High]
Is num_sbm_instances uninitialized when it is used in this addition?

If disabled_cpus > 0, the calculation incorporates stack garbage. This
determines the SBM topology size during boot and can lead to uncontrolled
memory allocation sizing in sbm_init(), potentially causing out-of-bounds
array access or out-of-memory panics.

> +	} else {
> +		num_sbm_instances = cpumask_weight(cpu_sbm_setup_map);
> +	}
> +
> +	/*
> +	 * If disabled threads exists, assume the maximum threads per
> +	 * instance can extend by the number of disabled threads if they
> +	 * are all added to the same node.
> +	 */
> +	sbm_set_topology(num_sbm_instances,
> +			 max_threads_per_instance + disabled_cpus);
> +
> +	memblock_free(__node_thread_count, nr_cpu_ids * sizeof(int));
> +	memblock_free(cpu_sbm_setup_map, cpumask_size());
> +}
[ ... ]
> diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c b/arch/powerpc/platforms/pseries/hotplug-cpu.c
> index bc6926dbf1489..7c1c1ac3efde4 100644
> --- a/arch/powerpc/platforms/pseries/hotplug-cpu.c
> +++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c
[ ... ]
> @@ -870,6 +871,15 @@ void __init pseries_cpu_hotplug_init(void)
>  		return;
>  	}
>  
> +	/*
> +	 * find_cpu_id_range() only looks at online nodes.
> +	 *
> +	 * XXX: Is it possible for a CPU attached memory node to come
> +	 * online after this point? May need num_possbile_nodes() then
> +	 * unless there are platform nuances that can help optimize.
> +	 */
> +	sbm_set_topology(num_online_nodes(), num_possible_cpus());

[Severity: Medium]
Does sizing SBM by num_online_nodes() break dynamic node hotplug?

pSeries platforms support Dynamic LPAR, where entirely new NUMA nodes can be
onlined after boot. When a CPU is added to a new node, its requested topology
instance will exceed the boot-time pool bounded by num_online_nodes(). This
exhausts the SBM metadata index pool, causing alloc_sbm_index() to return
-ENOENT and potentially corrupting the scheduler's idle tracking mechanisms.

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