RE: [PATCH v9 1/4] xen/device-tree: Parse 'cpu-map' node for CPU topology exploration

[Hirokazu Takahashi <taka@xxxxxxxxxxxxx>]

Hi Michal,

Thank you for the feedback.

> -----Original Message-----
> From: Orzel, Michal <michal.orzel@xxxxxxx>
> Sent: Wednesday, September 9, 2026 12:14 AM
> To: Hirokazu Takahashi <taka@xxxxxxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Mykyta_Poturai@xxxxxxxx; Jan Beulich <jbeulich@xxxxxxxx>; Stefano
> Stabellini <sstabellini@xxxxxxxxxx>; Julien Grall <julien@xxxxxxx>; Bertrand
> Marquis <bertrand.marquis@xxxxxxx>; Volodymyr Babchuk
> <Volodymyr_Babchuk@xxxxxxxx>; Andrew Cooper
> <andrew.cooper3@xxxxxxxxxx>; Anthony PERARD
> <anthony.perard@xxxxxxxxxx>; Roger Pau Monné <roger@xxxxxxxxxxxxxx>
> Subject: Re: [PATCH v9 1/4] xen/device-tree: Parse 'cpu-map' node for CPU
> topology exploration
> 
> 
> 
> On 28-Jul-26 07:06, Hirokazu Takahashi wrote:
> > Parse the 'cpu-map' node in the Device Tree to extract CPU topology
> > information. If the 'cpu-map' node is absent, fall back to
> > generating the topology data from the NUMA information. This
> > generation assumes exactly one socket per NUMA node and that SMT
> > is unsupported.
> This does not seem to reflect the implementation. If there is no `cpu-map` 
> node,
> you just return error and free the table.

I forgot to update the description from an earlier implementation. I will fix
the commit message.

> > --- /dev/null
> > +++ b/xen/common/cpu-topology.c
> > @@ -0,0 +1,62 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> The main license for Xen is GPLv2-only. Any reason for GPLv2+ in the new
> files?
> I'm asking because if you don't care about the license and simply copied it 
> from
> other places, v2-only is a better fit for some organizations that cannot
> contribute to v2+.

Okay.

> > +void __init init_cpu_topology(void)
> > +{
> > +    unsigned int cpu;
> > +    int ret;
> > +
> > +    cpu_topology = xvzalloc_array(struct cpu_topology, nr_cpu_ids);
> You call it from `smp_init_cpus()` at which point `nr_cpu_ids` is not yet set
> and simply denotes `NR_CPUS`.

That is correct. I will move the call to init_cpu_topology() to a later point 
where
nr_cpu_ids is finalized.

> > +static int __init parse_core(const struct dt_device_node *core,
> > +                             unsigned int package_id,
> > +                             unsigned int cluster_id,
> > +                             unsigned int core_id)
> > +{
> > +    bool leaf = true;
> > +    unsigned int thread_id;
> > +    unsigned int cpu;
> > +
> > +    for ( thread_id = 0; ; thread_id++ )
> > +    {
> > +        const struct dt_device_node *thread;
> > +        char name[20];
> > +
> > +        snprintf(name, sizeof(name), "thread%u", thread_id);
> > +        thread = dt_find_child_node_by_name(core, name);
> > +
> > +        if ( !thread )
> > +            break;
> > +
> > +        leaf = false;
> > +        cpu = get_cpu_for_node(thread);
> > +
> > +        if ( cpu == INVALID_TOPO_ID )
> > +        {
> > +            printk(XENLOG_ERR
> > +                   "ERROR: %s: Can't get CPU for thread\n", 
> > dt_node_name(thread));
> > +            return -EINVAL;
> > +        }
> > +
> > +        ASSERT(cpu_map[cpu].package_id == INVALID_TOPO_ID);
> > +        ASSERT(cpu_map[cpu].cluster_id == INVALID_TOPO_ID);
> > +        ASSERT(cpu_map[cpu].core_id == INVALID_TOPO_ID);
> > +        ASSERT(cpu_map[cpu].thread_id == INVALID_TOPO_ID);
> This ASSERT block and the identical one below validate DT data, not Xen 
> internal
> invariant. Return error instead.

Okay.

> > +static int __init parse_cluster(const struct dt_device_node *cluster,
> > +                                unsigned int package_id,
> > +                                unsigned int cluster_id,
> > +                                unsigned int depth)
> > +{
> > +    bool leaf = true;
> > +    bool has_cores = false;
> > +    unsigned int core_id;
> > +    unsigned int child_cluster_id;
> > +
> > +    /*
> > +     * First check for child clusters; we currently ignore any
> > +     * information about the nesting of clusters and present the
> > +     * scheduler with a flat list of them.
> > +     */
> > +    for ( child_cluster_id = 0; ; child_cluster_id++ )
> > +    {
> > +        const struct dt_device_node *child_cluster;
> > +        char name[20];
> > +        int ret;
> > +
> > +        snprintf(name, sizeof(name), "cluster%u", child_cluster_id);
> > +        child_cluster = dt_find_child_node_by_name(cluster, name);
> > +
> > +        if ( !child_cluster )
> > +            break;
> > +
> > +        leaf = false;
> > +        ret = parse_cluster(child_cluster, package_id, child_cluster_id,
> > +                            depth + 1);
> > +        if ( depth > 0 )
> > +            printk(XENLOG_WARNING
> > +                   "WARNING: Topology for clusters of clusters not yet 
> > supported\n");
> > +        if ( ret != 0 )
> > +            return ret;
> > +    }
> > +
> > +    /* Now check for cores */
> > +    for ( core_id = 0; ; core_id++ )
> > +    {
> > +        const struct dt_device_node *core;
> > +        char name[20];
> > +        int ret;
> > +
> > +        snprintf(name, sizeof(name), "core%u", core_id);
> > +        core = dt_find_child_node_by_name(cluster, name);
> > +
> > +        if ( !core )
> > +            break;
> > +
> > +        has_cores = true;
> > +
> > +        if ( depth == 0 )
> > +        {
> > +            printk(XENLOG_ERR
> > +                   "ERROR: %s: cpu-map children should be clusters\n",
> > +                   dt_node_name(core));
> > +            return -EINVAL;
> > +        }
> > +
> > +        if ( leaf )
> > +        {
> > +            ret = parse_core(core, package_id, cluster_id, core_id);
> > +            if ( ret != 0 )
> > +                return ret;
> > +        }
> > +        else
> > +        {
> > +            printk(XENLOG_ERR "ERROR: %s: Non-leaf cluster with core %s\n",
> > +                   dt_node_name(cluster), name);
> > +            return -EINVAL;
> > +        }
> > +    }
> > +
> > +    if ( leaf && !has_cores )
> > +        printk(XENLOG_WARNING "WARNING: %s: empty cluster\n",
> > +               dt_node_name(cluster));
> > +
> > +    return 0;
> > +}
> > +
> > +static int __init parse_socket(const struct dt_device_node *socket)
> > +{
> > +    bool has_socket = false;
> > +    unsigned int package_id;
> > +    int ret;
> > +
> > +    for ( package_id = 0; ; package_id++ )
> > +    {
> > +        const struct dt_device_node *cluster;
> > +        char name[20];
> > +
> > +        snprintf(name, sizeof(name), "socket%u", package_id);
> > +        cluster = dt_find_child_node_by_name(socket, name);
> The names are one level off (I know you took it from Linux which suffers from
> the same problem): the parameter is the cpu-map node, not a socket, and the
> local is a socket node, not a cluster. parse_cluster() has the same problem.
> Please name the parameters after what they actually receive,
> e.g.parse_socket(cpu_map) with a local 'socket'. It makes it difficult to 
> parse
> the code and I'll wait with reviewing this file until this is fixed.

I had the exact same impression when porting this code from the Linux kernel.
I will rename the function parameters and local variables.

> > +
> > +int __init dt_init_cpu_topology(void)
> > +{
> > +    unsigned int cpu;
> > +    int ret;
> > +
> > +    BUG_ON(!acpi_disabled);
> > +    BUG_ON(!cpu_topology);
> ASSERTs are a better fit here, given that these are already validated by the
> sole caller.

Okay.

Thank you,
Hirokazu Takahashi.