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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 03.07.2026 11:15, Hirokazu Takahashi wrote:
> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -188,6 +188,24 @@ config VM_EVENT
>  config NEEDS_LIBELF
>       bool
>  
> +config HAS_GENERIC_CPU_TOPOLOGY
> +     bool

A few lines up from here you'll find a long, sorted list of HAS_*. Please
insert this one at the appropriate spot there.

> +config DT_CPU_TOPOLOGY
> +     bool
> +
> +config GENERIC_CPU_TOPOLOGY
> +     bool "CPU topology support (UNSUPPORTED)" if UNSUPPORTED
> +     depends on HAS_GENERIC_CPU_TOPOLOGY
> +     select DT_CPU_TOPOLOGY if DEVICE_TREE_PARSE
> +     select ACPI_CPU_TOPOLOGY if ACPI

I wonder what others think here; to me this looks backwards. "Generic" ought
to be selected by the more specific variants.

> +     help
> +       Retrieve CPU topology information from the device tree to optimize
> +       virtual CPU scheduling.

This is DT help text, not generic one. Altogether something like the below?

config GENERIC_CPU_TOPOLOGY
        bool

config DT_CPU_TOPOLOGY
        bool "CPU topology support (UNSUPPORTED)" if UNSUPPORTED
        depends on HAS_GENERIC_CPU_TOPOLOGY && DEVICE_TREE_PARSE
        select GENERIC_CPU_TOPOLOGY
        help
          Retrieve CPU topology information from the device tree to optimize
          virtual CPU scheduling.

(And then similarly for ACPI.)

> --- /dev/null
> +++ b/xen/common/cpu-topology.c
> @@ -0,0 +1,62 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +
> +#include <xen/acpi.h>
> +#include <xen/cpu-topology.h>
> +#include <xen/cpumask.h>
> +#include <xen/dt-cpu-topology.h>
> +#include <xen/init.h>
> +
> +struct cpu_topology *__ro_after_init cpu_topology;

With both functions below being __init, this is the sole reason why the
file cannot (also) be built into cpu-topology.init.o. Perhaps this would
better move into e.g. cpu.c (inside an #ifdef there, of course)?

> --- /dev/null
> +++ b/xen/common/device-tree/cpu-topology.c
> @@ -0,0 +1,355 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Derived from Linux kernel 7.0's $drivers/base/arch_topology.c
> + * Parse cpu topology information.
> + */
> +
> +#include <xen/acpi.h>
> +#include <xen/cpu-topology.h>
> +#include <xen/cpumask.h>
> +#include <xen/device_tree.h>
> +#include <xen/errno.h>
> +#include <xen/init.h>
> +#include <xen/numa.h>
> +
> +struct cpu_map {
> +    unsigned int thread_id;
> +    unsigned int core_id;
> +    unsigned int cluster_id;
> +    unsigned int package_id;
> +};
> +
> +static const unsigned int __initdata invalid_topo_id = ~0U;

To better fit with the rest of the codebase, I think this will want to be

#define INVALID_TOPO_ID (~0U)

> +static struct cpu_map __initdata cpu_map[NR_CPUS] = {
> +    [0 ... NR_CPUS - 1] = {invalid_topo_id, invalid_topo_id, 
> invalid_topo_id, 0}
> +};

Why would 0 need spelling out? And why is it 0 for package_id, but
invalid_topo_id for everything else? (IOW: There may want to be a comment
here.)

> +static struct dt_device_node *__initdata dt_cpu_table[NR_CPUS];
> +
> +static void __init setup_siblings_masks(unsigned int target_cpu)
> +{
> +    const struct cpu_topology *target_topo = &cpu_topology[target_cpu];
> +    const struct cpu_map *target_map = &cpu_map[target_cpu];
> +    unsigned int cpu;
> +
> +    /* Update core and thread sibling masks */
> +    for_each_possible_cpu(cpu)
> +    {
> +        const struct cpu_topology *cpu_topo = &cpu_topology[cpu];
> +        const struct cpu_map *map = &cpu_map[cpu];
> +
> +        if ( target_map->package_id != map->package_id )
> +            continue;
> +
> +        cpumask_set_cpu(target_cpu, cpu_topo->core_sibling);
> +        cpumask_set_cpu(cpu, target_topo->core_sibling);
> +
> +        if ( target_map->cluster_id != map->cluster_id )
> +            continue;
> +
> +        if ( target_map->cluster_id != invalid_topo_id )
> +        {
> +            cpumask_set_cpu(target_cpu, cpu_topo->cluster_sibling);
> +            cpumask_set_cpu(cpu, target_topo->cluster_sibling);
> +        }
> +
> +        if ( target_map->core_id != map->core_id )
> +            continue;
> +
> +        cpumask_set_cpu(target_cpu, cpu_topo->thread_sibling);
> +        cpumask_set_cpu(cpu, target_topo->thread_sibling);
> +    }
> +}
> +
> +static struct dt_device_node *__init
> +       dt_find_child_node_by_name(const struct dt_device_node *dt,

Bogus indentation (the function name wants to start at line start when
wrapped like this).

> +                                  const char *name)
> +{
> +    struct dt_device_node *np;
> +
> +    dt_for_each_child_node(dt, np)
> +        if ( np->name && (dt_node_cmp(np->name, name) == 0) )
> +            break;
> +
> +    return np;
> +}

I'm not a DT person, so I don't know how likely it is that the
implementation of dt_for_each_child_node(), to e.g. be similar to
list_for_each*(). Here you're assuming np to be NULL upon loop exit,
which however is an implementation detail of the macro.

> +void __init map_cpu_to_dt_node(unsigned int cpu,
> +                               struct dt_device_node *cpu_node)
> +{
> +    if ( cpu < ARRAY_SIZE(dt_cpu_table) )
> +        dt_cpu_table[cpu] = cpu_node;
> +    else
> +        printk(XENLOG_WARNING
> +               "cpu id %u exceeds the max cores %lu\n",
> +               cpu, ARRAY_SIZE(dt_cpu_table));

There's still "id" here, and "cores" also neither fits dt_cpu_table[],
nor is it an arch-independent synonym for "CPU" (and you use the term
for another purpose further down). %lu also isn't exactly the format to
use with size_t arguments.

> +static unsigned int __init cpu_node_to_id(const struct dt_device_node 
> *cpu_node)
> +{
> +    unsigned int cpu;
> +
> +    for_each_possible_cpu(cpu)
> +        if ( cpu_node == dt_cpu_table[cpu] )
> +            return cpu;
> +
> +    return invalid_topo_id;
> +}
> +
> +/*
> + * This function returns the logical cpu number of the DT node.
> + */

"logical" meaning what exactly here? To be unambiguous, maybe better "Xen
CPU number"?

> +static unsigned int __init
> +       get_cpu_for_node(const struct dt_device_node *dt_node)

Bad indentation again.

> +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 i = 0;
> +    unsigned int cpu;
> +
> +    do {
> +        const struct dt_device_node *t;
> +        char name[20];
> +
> +        snprintf(name, sizeof(name), "thread%u", i);
> +        t = dt_find_child_node_by_name(core, name);
> +
> +        if ( !t )
> +            break;
> +
> +        leaf = false;
> +        cpu = get_cpu_for_node(t);
> +        if ( cpu != invalid_topo_id )
> +        {
> +            cpu_map[cpu].package_id = package_id;
> +            cpu_map[cpu].cluster_id = cluster_id;
> +            cpu_map[cpu].core_id = core_id;
> +            cpu_map[cpu].thread_id = i;
> +        }
> +        else
> +        {
> +            printk(XENLOG_ERR
> +                   "ERROR: %s: Can't get CPU for thread\n", dt_node_name(t));
> +            return -EINVAL;
> +        }

Less indentation is needed with

        if ( cpu == invalid_topo_id )
        {
            printk(XENLOG_ERR
                   "ERROR: %s: Can't get CPU for thread\n", dt_node_name(t));
            return -EINVAL;
        }

        cpu_map[cpu].package_id = package_id;
        cpu_map[cpu].cluster_id = cluster_id;
        cpu_map[cpu].core_id = core_id;
        cpu_map[cpu].thread_id = i;

Also should you maybe assert up front that the slot is still vacant?

> +        i++;
> +    } while ( true );

Why not the more conventional

    for ( i = 0; ; ++i )

? Then it will also be possible to use "continue" inside the loop (if ever
needed). Same elsewhere.

> +/*
> + * Generate cpu topology information when cpu-map node doesn't exist.
> + * It assumes that the cpu doesn't have SMT and all CPUs on a NUMA
> + * node belong to the same socket.
> + */
> +static void __init fixup_topology(void)
> +{
> +    unsigned int cpu;
> +    unsigned int clid = 0;
> +    unsigned int pkgid = 0;
> +
> +    for_each_possible_cpu(cpu)
> +    {
> +        struct cpu_map *map = &cpu_map[cpu];
> +
> +        map->package_id = cpu_to_node(cpu);
> +        if ( map->package_id != pkgid )
> +        {
> +            pkgid = map->package_id;
> +            clid = 0;
> +        }
> +        map->cluster_id = clid++;
> +        map->core_id = 0;
> +        map->thread_id = 0;
> +    }
> +}

The comment ahead of the function talks of NUMA nodes, but the function
shows no NUMA-awareness.

> +int __init parse_dt_topology(void)

static?

> --- /dev/null
> +++ b/xen/drivers/acpi/topology.c
> @@ -0,0 +1,38 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +
> +#include <xen/acpi.h>
> +#include <xen/cpu-topology.h>
> +#include <xen/cpumask.h>
> +#include <xen/init.h>
> +
> +/*
> + * TODO: Populate the topology information by scanning the ACPI
> + *       PPTT (Processor Properties Topology Table).
> + */
> +void __init acpi_init_cpu_topology(void)
> +{
> +    unsigned int cpu;
> +
> +    /*
> +     * Generate temporary cpu topology information for now.
> +     * It assumes that the cpu doesn't have SMT and all CPUs
> +     * belong to the same socket.
> +     */
> +    for_each_possible_cpu(cpu)
> +    {
> +        struct cpu_topology *topo = &cpu_topology[cpu];
> +
> +        cpumask_set_cpu(cpu, topo->thread_sibling);
> +        cpumask_copy(topo->core_sibling, &cpu_possible_map);
> +    }

What about ->cluster_sibling? That can't remain all empty, can it?

> --- /dev/null
> +++ b/xen/include/xen/cpu-topology.h
> @@ -0,0 +1,35 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#ifndef XEN_CPU_TOPOLOGY_H
> +#define XEN_CPU_TOPOLOGY_H
> +
> +#include <xen/cpumask.h>
> +
> +#ifdef CONFIG_GENERIC_CPU_TOPOLOGY
> +
> +struct cpu_topology {
> +    cpumask_var_t thread_sibling;
> +    cpumask_var_t core_sibling;
> +    cpumask_var_t cluster_sibling;
> +};
> +
> +extern struct cpu_topology *cpu_topology;
> +void init_cpu_topology(void);
> +
> +#else /* CONFIG_GENERIC_CPU_TOPOLOGY */
> +
> +#define cpu_topology ((struct cpu_topology *)NULL)

As before: I consider it dangerous to expose an identifier which is
only ever NULL. The extern should be moved out of the #ifdef, and
DCE should be leveraged to eliminate all references when
GENERIC_CPU_TOPOLOGY=n.

Jan