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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.06.2026 09:12, 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).

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.

> +    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.

> --- /dev/null
> +++ b/xen/common/device-tree/cpu-topology.c
> @@ -0,0 +1,343 @@
> +/* 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/cpumask.h>
> +#include <xen/device_tree.h>
> +#include <xen/cpu-topology.h>
> +#include <xen/numa.h>
> +#include <xen/errno.h>
> +#include <xen/init.h>
> +
> +struct cpu_map {
> +    unsigned int thread_id;
> +    unsigned int core_id;
> +    unsigned int cluster_id;
> +    unsigned int package_id;
> +};
> +
> +struct cpu_topology *cpu_topology;
> +static const unsigned int __initdata 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, 
> 0U}
> +};
> +static struct dt_device_node * __initdata dt_cpu_table[NR_CPUS];
> +
> +static void __init setup_siblings_masks(unsigned int cpuid)
> +{
> +    struct cpu_topology *cpuid_topo = &cpu_topology[cpuid];
> +    struct cpu_map *cpuid_map = &cpu_map[cpuid];
> +    unsigned int cpu;
> +
> +    /* Update core and thread sibling masks */
> +    for_each_possible_cpu(cpu)
> +    {
> +        struct cpu_topology *cpu_topo = &cpu_topology[cpu];
> +        struct cpu_map *map = &cpu_map[cpu];
> +
> +        if ( cpuid_map->package_id != map->package_id )
> +            continue;
> +
> +        cpumask_set_cpu(cpuid, &cpu_topo->core_sibling);
> +        cpumask_set_cpu(cpu, &cpuid_topo->core_sibling);
> +
> +        if ( cpuid_map->cluster_id != map->cluster_id )
> +            continue;
> +
> +        if ( cpuid_map->cluster_id != invalid_topo_id )
> +        {
> +            cpumask_set_cpu(cpu, &cpuid_topo->cluster_sibling);
> +            cpumask_set_cpu(cpuid, &cpu_topo->cluster_sibling);
> +        }
> +
> +        if ( cpuid_map->core_id != map->core_id )
> +            continue;
> +
> +        cpumask_set_cpu(cpuid, &cpu_topo->thread_sibling);
> +        cpumask_set_cpu(cpu, &cpuid_topo->thread_sibling);
> +    }
> +}
> +
> +static struct dt_device_node * __init dt_find_child_node_by_name(struct 
> dt_device_node *from, const char *name)

Nit: Overlong line here, and ...

> +static int __init parse_core(struct dt_device_node *core,
> +                unsigned int package_id, unsigned int cluster_id,
> +                unsigned int core_id)

... bogus indentation e.g. here. Please go though yourself to check style.

> --- /dev/null
> +++ b/xen/drivers/acpi/topology.c
> @@ -0,0 +1,38 @@
> +/* 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>
> +
> +/*
> + * ToDo: Populate the topology information by scanning the ACPI
> + *       PPTT (Processor Properties Topology Table).

Please can this be spelled "TODO:", to stand out more and to be a hit also
with case sensitive grep for "TODO"?

> --- a/xen/include/xen/acpi.h
> +++ b/xen/include/xen/acpi.h
> @@ -101,6 +101,8 @@ void acpi_table_print (struct acpi_table_header *header, 
> unsigned long phys_addr
>  void acpi_table_print_madt_entry (struct acpi_subtable_header *madt);
>  void acpi_table_print_srat_entry (struct acpi_subtable_header *srat);
>  
> +void acpi_init_cpu_topology(void);
> +
>  /* the following four functions are architecture-dependent */
>  void acpi_numa_slit_init (struct acpi_table_slit *slit);
>  void acpi_numa_processor_affinity_init(const struct acpi_srat_cpu_affinity 
> *);
> @@ -133,6 +135,8 @@ static inline int acpi_boot_table_init(void)
>       return 0;
>  }
>  
> +static inline void acpi_init_cpu_topology(void) {}

This shouldn't be needed. When ACPI=y, acpi_disabled is compile-time true,
and hence the compiler can and will DCE the call. All it needs to see is a
declaration, which therefore wants to move outside of the CONFIG_ACPI
conditional.

> --- /dev/null
> +++ b/xen/include/xen/cpu-topology.h
> @@ -0,0 +1,36 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#ifndef XEN_CPU_TOPOLOGY_H
> +#define XEN_CPU_TOPOLOGY_H
> +
> +#include <xen/types.h>
> +#include <xen/dt-cpu-topology.h>
> +
> +struct cpu_topology {
> +    cpumask_t thread_sibling;
> +    cpumask_t core_sibling;
> +    cpumask_t cluster_sibling;
> +};

With huge NR_CPUS this can be pretty large a struct (of which in 
init_cpu_topology()
you allocate an array). Imo you want to use cpumask_var_t here, with allocation 
added
as needed.

> +
> +

Nit: No double blank lines please.

Jan