Re: [PATCH v2 1/3] xen/device-tree: Parse 'cpu-map' node for CPU topology exploration

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.06.2026 11:05, Hirokazu Takahashi wrote:
>>> --- /dev/null
>>> +++ b/xen/common/cpu-topology.c
>>> @@ -0,0 +1,30 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>>> +
>>> +#include <xen/cpumask.h>
>>> +#include <xen/cpu-topology.h>
>>> +#include <xen/init.h>
>>> +#include <xen/acpi.h>
>>> +
>>> +void __init init_cpu_topology(void)
>>> +{
>>> +    const unsigned int nr_cpus = cpumask_last(&cpu_possible_map) +
>> 1U;
>>> +
>>> +    cpu_topology = xzalloc_array(struct cpu_topology, nr_cpus);
>>
>> cpu_topology exists as a global variable only when DT is in use. I think the
>> definition needs to move here (from common/device-tree/cpu-topology.c).
> 
> Okay
> 
>> As to the size of the array, it's not quite clear to me whether by doing it
>> this way (instead of using nr_cpu_ids) we're not setting ourselves up for
>> trouble.
> 
> On ARM64 Xen, nr_cpu_ids represents the total number of populated/available 
> CPUs, but unfortunately it cannot be relied upon as the maximum CPU ID. 
> 
> For instance, if a CPU node in the Device Tree has an invalid 'enable-method' 
> property, that CPU ID slot is still consumed during the initial parsing, but 
> the CPU is not counted towards nr_cpu_ids. This can result in a sparse CPU ID 
> allocation where the maximum CPU ID actually exceeds. 
> 
> If we were to use nr_cpu_ids as the array size here, we would risk an 
> out-of-bounds access under such faulty Device Tree configurations. This is 
> why I used "cpumask_last(&cpu_possible_map) + 1U" to ensure the array is 
> large enough to cover the highest allocated CPU ID.
> 
> Consequently, there might actually be potential bugs in other parts of Xen 
> where nr_cpu_ids is incorrectly assumed to be the upper bound for CPU ID 
> indexing on ARM.

Specifically cpumask_var_t allocations are dimensioned by nr_cpu_ids, and
all cpumask{,_var}_t accesses (including the cpumask_last() you use above)
also have bounds checks against nr_cpu_ids (sometimes only in debug builds).
IOW if there is an issue as you describe it, and if that can happen in
practice, then this urgently needs fixing on the Arm side. This cannot be an
excuse to not do the sane thing here.

>>> +    if ( !cpu_topology )
>>> +        panic("Failed to allocate memory for cpu_topology array\n");
>>
>> I question such uses of panic(): Surely we can do without any NUMA info,
>> it's only performance which is going to suffer.
> 
> Okay, I will replace the panic() with a XENLOG_WARNING printk.

Which of course you understand isn't all that needs changing then.

Jan