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

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

On 19-Oct-26 03:57, 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>
> ---
> Changes in v12:
>  - Error reporting for memory allocation failures in init_cpu_topology()
>    into a single error path.
>  - Emit an error message from init_cpu_topology() when
>    dt_init_cpu_topology() or acpi_init_cpu_topology() returns an error,
>    explicitly indicating that CPU topology initialization has failed.
>  - Drop the Reviewed-by tag from patch 0001 due to changes in
>    common/cpu-topology.c.
>  - In dt_smp_init_cpus(), add a comment explaining that the boot core
>    remains valid even if arch_cpu_init() fails.
>  - dt_init_cpu_topology() returns -EINVAL if a leaf node lacks the cpu
>    phandle property or if the target node is not a valid CPU node.
>  - dt_init_cpu_topology() continues parsing subsequent threads instead
>    of returning early when a referenced CPU is not tracked by Xen.
>  - Update the log message when skipping an unmapped CPU to reflect
>    that secondary CPUs have not yet been brought up at this stage.
>  - Remove trailing periods from log messages.
>  - Drop the Reviewed-by tag from patch 0001 due to changes in
>    common/cpu-topology.c.
> 
>  xen/arch/arm/Kconfig                  |   1 +
>  xen/arch/arm/setup.c                  |   3 +
>  xen/arch/arm/smpboot.c                |  10 +
>  xen/common/Kconfig                    |  22 ++
>  xen/common/Makefile                   |   1 +
>  xen/common/cpu-topology.c             |  73 +++++
>  xen/common/cpu.c                      |   5 +
>  xen/common/device-tree/Makefile       |   1 +
>  xen/common/device-tree/cpu-topology.c | 370 ++++++++++++++++++++++++++
>  xen/drivers/acpi/Makefile             |   1 +
>  xen/drivers/acpi/topology.c           |  43 +++
Shouldn't this file be called cpu-topology like the DT equivalent?

>  xen/include/xen/acpi.h                |  13 +
>  xen/include/xen/cpu-topology.h        |  34 +++
>  xen/include/xen/dt-cpu-topology.h     |  35 +++
>  14 files changed, 612 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..d34396f9b1 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>
Nothing in this file seems to use this header

>  #include <xen/errno.h>
>  #include <xen/init.h>
>  #include <xen/mm.h>
> @@ -245,6 +247,14 @@ static void __init dt_smp_init_cpus(void)
>          }
>          else
>              tmp_map[i] = hwid;
> +
> +        /*
> +         * Because the boot CPU (i == 0) is already running, treat it as 
> valid
> +         * even if arch_cpu_init() fails (e.g. due to a missing 
> enable-method).
> +         */
> +        if ( (rc >= 0) || (i == 0) )
> +            /* 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..8c64532e28
> --- /dev/null
> +++ b/xen/common/cpu-topology.c
> @@ -0,0 +1,73 @@
> +/* 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;
> +
> +    if ( !cpu_topology )
> +        return;
> +
> +    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 = 0;
> +
> +    if ( num_possible_cpus() == 1 )
> +        return;
> +
> +    cpu_topology = xvzalloc_array(struct cpu_topology, nr_cpu_ids);
> +
> +    if ( cpu_topology )
> +    {
> +        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) )
> +                ret = -ENOMEM;
The loop continues allocating after the first error which is incorrect.

> +        }
> +    }
> +    else
> +        ret = -ENOMEM;
> +
> +    if ( ret != 0 )
> +        printk(XENLOG_WARNING "CPU topology table allocation failed\n");
After printing this one we will also print the "Failed to initialize..." one.
Please goto the error.

> +    else 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 )
In v11 I asked you to print error on real failure, not all the time e.g. on -
EOPNOTSUPP or ENOENT as a result of not finding cpu-map.

> +    {
> +        printk("Failed to initialize cpu topology (error %d)\n", ret);
XENLOG_ERR/WARNING?

> +        free_topology_table();
> +    }
> +}
> +
> +/*
> + * 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 730d202349..b15b3382d2 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..bfcf153cc4
> --- /dev/null
> +++ b/xen/common/device-tree/cpu-topology.c
> @@ -0,0 +1,370 @@
> +/* 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 int __init get_cpu_for_node(
> +    const struct dt_device_node *dt_node, unsigned int *cpu)
> +{
> +    const struct dt_device_node *cpu_node =
> +        dt_parse_phandle(dt_node, "cpu", 0);
> +
> +    if ( !cpu_node || !dt_device_type_is_equal(cpu_node, "cpu") )
> +    {
> +        *cpu = INVALID_TOPO_ID;
> +        return -EINVAL;
> +    }
> +
> +    *cpu = cpu_node_to_id(cpu_node);
> +
> +    return 0;
> +}
> +
> +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;
> +    int ret;
> +
> +    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;
> +        ret = get_cpu_for_node(thread, &cpu);
> +        if ( ret )
> +        {
> +            printk(XENLOG_ERR
> +                   "%s: No CPU found for thread\n", dt_node_name(thread));
Use dt_node_full_name(). Otherwise the printed name can be ambiguous.

> +            return ret;
> +        }
> +
> +        if ( cpu == INVALID_TOPO_ID )
> +        {
> +            /*
> +             * Ignore this CPU node definition as the corresponding physical
> +             * CPU failed to boot.
You updated the printk message but not the comment. Please fix.

~Michal