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

["Orzel, Michal" <michal.orzel@xxxxxxx>]

On 13-Oct-26 12:36, Hirokazu Takahashi wrote:
> Parse the 'cpu-map' node in the Device Tree to extract CPU topology
> information. If the 'cpu-map' node is absent, simply ignore it.
> 
> Signed-off-by: Hirokazu Takahashi <taka@xxxxxxxxxxxxx>
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx> # common, acpi
> ---
> Changes in v11:
>  - When parsing the Device Tree, ignore CPU nodes if the corresponding
>    physical CPU failed to boot, rather than treating it as an error.
>  - Drop the dependency on UNSUPPORTED for CPU topology support.
>  - Print an error message if memory allocation fails in init_cpu_topology().
>  - Fix a compilation error involving cpumask_set_cpu().
>  - Call map_cpu_to_dt_node() to record the CPU ID to DT node mapping only
>    when arch_cpu_init() succeeds, or if the CPU is the boot CPU (treated
>    as successful since the boot CPU is already up and running).
>  - Use XENLOG_ERR as the log level for error messages emitted when DT CPU
>    topology parsing fails.
>  - Add ASSERT(cpu_topology) to acpi_init_cpu_topology() for consistency.
>  - Code cleanups:
>    * Use dt_node_name_is_equal() instead of dt_node_cmp().
>    * Drop redundant "ERROR:" and "WARNING:" prefixes from log messages.
>    * Move header file includes into #ifdef / #else blocks where they are
>      actually needed.
>    * Adjust spacing to comply with the Xen coding style.
> 
>  xen/arch/arm/Kconfig                  |   1 +
>  xen/arch/arm/setup.c                  |   3 +
>  xen/arch/arm/smpboot.c                |   6 +
>  xen/common/Kconfig                    |  22 ++
>  xen/common/Makefile                   |   1 +
>  xen/common/cpu-topology.c             |  69 +++++
>  xen/common/cpu.c                      |   5 +
>  xen/common/device-tree/Makefile       |   1 +
>  xen/common/device-tree/cpu-topology.c | 352 ++++++++++++++++++++++++++
>  xen/drivers/acpi/Makefile             |   1 +
>  xen/drivers/acpi/topology.c           |  43 ++++
>  xen/include/xen/acpi.h                |  13 +
>  xen/include/xen/cpu-topology.h        |  34 +++
>  xen/include/xen/dt-cpu-topology.h     |  35 +++
>  14 files changed, 586 insertions(+)
>  create mode 100644 xen/common/cpu-topology.c
>  create mode 100644 xen/common/device-tree/cpu-topology.c
>  create mode 100644 xen/drivers/acpi/topology.c
>  create mode 100644 xen/include/xen/cpu-topology.h
>  create mode 100644 xen/include/xen/dt-cpu-topology.h
> 
> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> index b9c5c40678..3d0f689220 100644
> --- a/xen/arch/arm/Kconfig
> +++ b/xen/arch/arm/Kconfig
> @@ -19,6 +19,7 @@ config ARM
>       select HAS_ALTERNATIVE if HAS_VMAP
>       select HAS_DEVICE_TREE_DISCOVERY
>       select HAS_DOM0LESS
> +     select HAS_GENERIC_CPU_TOPOLOGY
>       select HAS_GRANT_CACHE_FLUSH if GRANT_TABLE
>       select HAS_SHARED_INFO
>       select HAS_STACK_PROTECTOR
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 2cd1c021b5..c1aa06bd31 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -10,6 +10,7 @@
>  
>  #include <xen/bootinfo.h>
>  #include <xen/compile.h>
> +#include <xen/cpu-topology.h>
>  #include <xen/device_tree.h>
>  #include <xen/dom0less-build.h>
>  #include <xen/domain_page.h>
> @@ -375,6 +376,8 @@ void asmlinkage __init noreturn start_xen(unsigned long 
> fdt_paddr)
>      nr_cpu_ids = smp_get_max_cpus();
>      printk(XENLOG_INFO "SMP: Allowing %u CPUs\n", nr_cpu_ids);
>  
> +    init_cpu_topology();
> +
>      /*
>       * Some errata relies on SMCCC version which is detected by psci_init()
>       * (called from smp_init_cpus()).
> diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
> index 8c9072f3c1..089c997f38 100644
> --- a/xen/arch/arm/smpboot.c
> +++ b/xen/arch/arm/smpboot.c
> @@ -9,10 +9,12 @@
>  
>  #include <xen/acpi.h>
>  #include <xen/cpu.h>
> +#include <xen/cpu-topology.h>
>  #include <xen/cpumask.h>
>  #include <xen/delay.h>
>  #include <xen/device_tree.h>
>  #include <xen/domain_page.h>
> +#include <xen/dt-cpu-topology.h>
>  #include <xen/errno.h>
>  #include <xen/init.h>
>  #include <xen/mm.h>
> @@ -245,6 +247,10 @@ static void __init dt_smp_init_cpus(void)
>          }
>          else
>              tmp_map[i] = hwid;
> +
> +        if ( (rc >= 0) || (i == 0) )
Please add a comment explaining the second part i.e. boot CPU may lack enable
method.

> +            /* Pass the info to dt_init_cpu_topology() */
> +            map_cpu_to_dt_node(i, cpu);
>      }
>  
>      if ( !bootcpu_valid )
> diff --git a/xen/common/Kconfig b/xen/common/Kconfig
> index 5b289e444f..fb42f0aa8b 100644
> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -140,6 +140,9 @@ config HAS_EX_TABLE
>  config HAS_FAST_MULTIPLY
>       bool
>  
> +config HAS_GENERIC_CPU_TOPOLOGY
> +     bool
> +
>  config HAS_IOPORTS
>       bool
>  
> @@ -194,6 +197,25 @@ config VM_EVENT
>  config NEEDS_LIBELF
>       bool
>  
> +config GENERIC_CPU_TOPOLOGY
> +     bool
> +
> +config DT_CPU_TOPOLOGY
> +     bool "Device tree based CPU topology support"
> +     depends on HAS_GENERIC_CPU_TOPOLOGY && DEVICE_TREE_PARSE
> +     select GENERIC_CPU_TOPOLOGY
> +     help
> +       Retrieve CPU topology information from the device tree to optimize
> +       vCPU scheduling.
> +
> +config ACPI_CPU_TOPOLOGY
> +     bool "ACPI based CPU topology support"
> +     depends on HAS_GENERIC_CPU_TOPOLOGY && ACPI
> +     select GENERIC_CPU_TOPOLOGY
> +     help
> +       Retrieve CPU topology information from the ACPI PPTT to optimize
> +       vCPU scheduling.
> +
>  config NUMA
>       bool
>  
> diff --git a/xen/common/Makefile b/xen/common/Makefile
> index f69d47d189..d0eb39a633 100644
> --- a/xen/common/Makefile
> +++ b/xen/common/Makefile
> @@ -5,6 +5,7 @@ obj-$(CONFIG_GENERIC_BUG_FRAME) += bug.o
>  obj-$(CONFIG_HYPFS_CONFIG) += config_data.o
>  obj-$(CONFIG_CORE_PARKING) += core_parking.o
>  obj-y += cpu.o
> +obj-$(CONFIG_GENERIC_CPU_TOPOLOGY) += cpu-topology.init.o
>  obj-$(CONFIG_DEBUG_TRACE) += debugtrace.o
>  obj-$(CONFIG_HAS_DEVICE_TREE_DISCOVERY) += device.o
>  obj-$(filter-out $(CONFIG_X86),$(CONFIG_ACPI)) += device.o
> diff --git a/xen/common/cpu-topology.c b/xen/common/cpu-topology.c
> new file mode 100644
> index 0000000000..ce50408858
> --- /dev/null
> +++ b/xen/common/cpu-topology.c
> @@ -0,0 +1,69 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#include <xen/acpi.h>
> +#include <xen/cpu-topology.h>
> +#include <xen/cpumask.h>
> +#include <xen/dt-cpu-topology.h>
> +#include <xen/init.h>
> +#include <xen/xvmalloc.h>
> +
> +static void __init free_topology_table(void)
> +{
> +    unsigned int cpu;
> +
> +    for ( cpu = 0; cpu < nr_cpu_ids; cpu++ )
> +    {
> +        free_cpumask_var(cpu_topology[cpu].thread_sibling);
> +        free_cpumask_var(cpu_topology[cpu].core_sibling);
> +        free_cpumask_var(cpu_topology[cpu].cluster_sibling);
> +    }
> +
> +    XVFREE(cpu_topology);
> +}
> +
> +void __init init_cpu_topology(void)
> +{
> +    unsigned int cpu;
> +    int ret;
> +
> +    if ( num_possible_cpus() == 1 )
> +        return;
> +
> +    cpu_topology = xvzalloc_array(struct cpu_topology, nr_cpu_ids);
> +    if ( !cpu_topology )
> +    {
> +        printk(XENLOG_WARNING "CPU topology table allocation failed.\n");
> +        return;
> +    }
> +
> +    for ( cpu = 0; cpu < nr_cpu_ids; cpu++ )
> +    {
> +        if ( !zalloc_cpumask_var(&cpu_topology[cpu].thread_sibling) ||
> +             !zalloc_cpumask_var(&cpu_topology[cpu].core_sibling) ||
> +             !zalloc_cpumask_var(&cpu_topology[cpu].cluster_sibling) )
> +        {
> +            free_topology_table();
> +            printk(XENLOG_WARNING "CPU topology table allocation failed.\n");
> +            return;
> +        }
> +    }
> +
> +    if ( acpi_disabled )
> +        ret = dt_init_cpu_topology();
> +    else
> +        ret = acpi_init_cpu_topology();
> +
> +    /* Free the CPU topology table if initialization fails. */
> +    if ( ret != 0 )
> +        free_topology_table();
In a previous revision I asked to print an error (except for expected errors
like -EOPNOTSUPP).

> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/common/cpu.c b/xen/common/cpu.c
> index f09af0444b..9591ada60a 100644
> --- a/xen/common/cpu.c
> +++ b/xen/common/cpu.c
> @@ -1,5 +1,6 @@
>  #include <xen/cpumask.h>
>  #include <xen/cpu.h>
> +#include <xen/cpu-topology.h>
>  #include <xen/event.h>
>  #include <xen/init.h>
>  #include <xen/sched.h>
> @@ -46,6 +47,10 @@ const unsigned long 
> cpu_bit_bitmap[BITS_PER_LONG+1][BITS_TO_LONGS(NR_CPUS)] = {
>  #undef MASK_DECLARE_2
>  #undef MASK_DECLARE_1
>  
> +#ifdef CONFIG_GENERIC_CPU_TOPOLOGY
> +struct cpu_topology *__ro_after_init cpu_topology;
> +#endif /* CONFIG_GENERIC_CPU_TOPOLOGY */
> +
>  static DEFINE_RWLOCK(cpu_add_remove_lock);
>  
>  bool get_cpu_maps(void)
> diff --git a/xen/common/device-tree/Makefile b/xen/common/device-tree/Makefile
> index 9036e455d6..6ee670b5f4 100644
> --- a/xen/common/device-tree/Makefile
> +++ b/xen/common/device-tree/Makefile
> @@ -1,6 +1,7 @@
>  obj-y += bootfdt.init.o
>  obj-$(CONFIG_HAS_DEVICE_TREE_DISCOVERY) += bootinfo-fdt.init.o
>  obj-$(CONFIG_HAS_DEVICE_TREE_DISCOVERY) += bootinfo.init.o
> +obj-$(CONFIG_DT_CPU_TOPOLOGY) += cpu-topology.init.o
>  obj-y += device-tree.o
>  obj-$(CONFIG_DOMAIN_BUILD_HELPERS) += domain-build.init.o
>  obj-$(filter $(CONFIG_DOM0LESS_BOOT),$(CONFIG_HAS_DEVICE_TREE_DISCOVERY)) += 
> dom0less-build.init.o
> diff --git a/xen/common/device-tree/cpu-topology.c 
> b/xen/common/device-tree/cpu-topology.c
> new file mode 100644
> index 0000000000..a62f65a41d
> --- /dev/null
> +++ b/xen/common/device-tree/cpu-topology.c
> @@ -0,0 +1,352 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * 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>
> +
> +#define INVALID_TOPO_ID (~0U)
> +
> +struct cpu_map {
> +    unsigned int thread_id;
> +    unsigned int core_id;
> +    unsigned int cluster_id;
> +    unsigned int socket_id;
> +};
> +
> +static struct cpu_map __initdata cpu_map[NR_CPUS] = {
> +    [0 ... NR_CPUS - 1] = {
> +        .thread_id = INVALID_TOPO_ID,
> +        .core_id = INVALID_TOPO_ID,
> +        .cluster_id = INVALID_TOPO_ID,
> +        .socket_id = INVALID_TOPO_ID,
> +    },
> +};
> +static struct dt_device_node *__initdata dt_cpu_table[NR_CPUS];
> +
> +static void __init setup_siblings_masks(unsigned int target_cpu)
> +{
> +    struct cpu_topology *target_topo = &cpu_topology[target_cpu];
> +    const struct cpu_map *target_map = &cpu_map[target_cpu];
> +    unsigned int cpu;
> +
> +    /* Update cluster, core and thread sibling masks */
> +    for_each_possible_cpu(cpu)
> +    {
> +        struct cpu_topology *cpu_topo = &cpu_topology[cpu];
> +        const struct cpu_map *map = &cpu_map[cpu];
> +
> +        if ( target_cpu > cpu )
> +            continue;
> +
> +        if ( target_map->socket_id != map->socket_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;
> +
> +        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 const struct dt_device_node *__init dt_find_child_node_by_name(
> +    const struct dt_device_node *dt,
> +    const char *name)
> +{
> +    const struct dt_device_node *np;
> +
> +    dt_for_each_child_node(dt, np)
> +        if ( np->name && dt_node_name_is_equal(np, name) )
> +            return np;
> +
> +    return NULL;
> +}
> +
> +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 %u exceeds the max cpus %zu\n",
> +               cpu, ARRAY_SIZE(dt_cpu_table));
> +}
> +
> +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 Xen cpu number of the DT node.
> + */
> +static unsigned int __init get_cpu_for_node(
> +    const struct dt_device_node *dt_node)
> +{
> +    const struct dt_device_node *cpu_node =
> +        dt_parse_phandle(dt_node, "cpu", 0);
> +
> +    if ( !cpu_node )
> +        return INVALID_TOPO_ID;
> +
> +    return cpu_node_to_id(cpu_node);
> +}
> +
> +static bool __init is_cpu_map_empty(unsigned int cpu)
> +{
> +    return (cpu_map[cpu].socket_id == INVALID_TOPO_ID) &&
> +           (cpu_map[cpu].cluster_id == INVALID_TOPO_ID) &&
> +           (cpu_map[cpu].core_id == INVALID_TOPO_ID) &&
> +           (cpu_map[cpu].thread_id == INVALID_TOPO_ID);
> +}
> +
> +static int __init parse_core(const struct dt_device_node *core,
> +                             unsigned int socket_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 )
get_cpu_for_node can return INVALID_TOPO_ID also on malformed DTs e.g. missing
phandle which you now accept. Shouldn't we return -EINVAL?

> +        {
> +            /*
> +             * Ignore this CPU node definition as the corresponding physical
> +             * CPU failed to boot.
> +             */
> +            printk(XENLOG_INFO
> +                   "%s: CPU for thread hasn't started\n", 
> dt_node_name(thread));
CPU topology is initialized before bringing CPU up, so this message is incorrect
and there are more than one scenarios we may end up with here. Maybe just "CPU
not used by Xen: ignoring".

> +            return 0;
This stops parsing other threads which I don't think is the right way to do it.
We should just continue.

> +        }
> +
> +        if ( !is_cpu_map_empty(cpu) )
> +        {
> +            printk(XENLOG_ERR
> +                   "Duplicate CPU definition for CPU%u\n", cpu);
> +            return -EINVAL;
> +        }
> +
> +        cpu_map[cpu].socket_id = socket_id;
> +        cpu_map[cpu].cluster_id = cluster_id;
> +        cpu_map[cpu].core_id = core_id;
> +        cpu_map[cpu].thread_id = thread_id;
> +    }
> +
> +    cpu = get_cpu_for_node(core);
> +
> +    if ( cpu != INVALID_TOPO_ID )
> +    {
> +        if ( !leaf )
> +        {
> +            printk(XENLOG_ERR "%s: Core has both threads and CPU\n",
> +                   dt_node_name(core));
> +            return -EINVAL;
> +        }
> +
> +        if ( !is_cpu_map_empty(cpu) )
> +        {
> +            printk(XENLOG_ERR "Duplicate CPU definition for CPU%u\n", cpu);
> +            return -EINVAL;
> +        }
> +
> +        cpu_map[cpu].socket_id = socket_id;
> +        cpu_map[cpu].cluster_id = cluster_id;
> +        cpu_map[cpu].core_id = core_id;
> +        cpu_map[cpu].thread_id = 0;
> +    }
> +    else if ( leaf )
> +    {
> +        /*
> +         * Ignore this CPU node definition as the corresponding physical
> +         * CPU failed to boot.
> +         */
> +        printk(XENLOG_INFO
> +               "%s: CPU for leaf core hasn't started\n", dt_node_name(core));
Same wording issue.

~Michal