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

On 14.07.2026 12:44, Hirokazu Takahashi wrote:
> --- 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

You've got indentation right here and ...

> @@ -191,6 +194,25 @@ config VM_EVENT
>  config NEEDS_LIBELF
>  	bool
>  
> +config GENERIC_CPU_TOPOLOGY
> +	bool

... here. Why not ...

> +config DT_CPU_TOPOLOGY
> +    bool "Device tree based 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.
> +
> +config ACPI_CPU_TOPOLOGY
> +    bool "ACPI based CPU topology support (UNSUPPORTED)" if UNSUPPORTED
> +    depends on HAS_GENERIC_CPU_TOPOLOGY && ACPI
> +    select GENERIC_CPU_TOPOLOGY
> +    help
> +      Retrieve CPU topology information from the ACPI PPTT to optimize
> +      virtual CPU scheduling.

... throughout here?

I'm also a little puzzled by the "if UNSUPPORTED" on the prompts. Imo that
would better be normal "depends on UNSUPPORTED". The situation is different
in e.g. common/sched/Kconfig, where the default value may be Y (with the
prompt being invisible making it impossible to turn off the option).

Also may I suggest s/virtual CPU/vCPU/ ?

> --- /dev/null
> +++ b/xen/common/cpu-topology.c
> @@ -0,0 +1,65 @@
> +/* 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>
> +
> +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);
> +    }
> +
> +    XFREE(cpu_topology);

XVFREE(), as it wants to be ...

> +}
> +
> +void __init init_cpu_topology(void)
> +{
> +    unsigned int cpu;
> +    int ret;
> +
> +    cpu_topology = xzalloc_array(struct cpu_topology, nr_cpu_ids);

--- xvzalloc_array() here.

> +    if ( !cpu_topology )
> +    {
> +        printk(XENLOG_ERR "Failed to allocate memory for cpu_topology table\n");

Is this really an error (irrespective of whether this then also needs
logging)? There may not be any topology information to retrieve, in which
case the allocation failure is benign.

> +        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_ERR "Failed to allocate memory for cpu_topology table\n");

Same here then obviously. Also please respect line length constraints. Don't
split format strings, but splitting XENLOG_* off the string literal is fine.

> --- a/xen/common/cpu.c
> +++ b/xen/common/cpu.c
> @@ -46,6 +46,11 @@ 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;

Why would this be needed?

> +struct cpu_topology *__ro_after_init cpu_topology;

This includes the same effect.

> --- /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/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 package_id;
> +};
> +
> +static struct cpu_map __initdata cpu_map[NR_CPUS] = {
> +    [0 ... NR_CPUS - 1] = {INVALID_TOPO_ID, INVALID_TOPO_ID,
> +                           INVALID_TOPO_ID, INVALID_TOPO_ID}

I think it would be nice if this properly used designated initializers
throughout. A trailing comma (twice) would also be nice.

> +static struct dt_device_node *__init dt_find_child_node_by_name(
> +    const struct dt_device_node *dt,
> +    const char *name)
> +{
> +    struct dt_device_node *np;

Is there a reason this cannot be pointer-to-const?

> --- a/xen/drivers/acpi/Makefile
> +++ b/xen/drivers/acpi/Makefile
> @@ -10,3 +10,4 @@ obj-$(CONFIG_PM_OP) += pm-op.o
>  
>  obj-$(CONFIG_X86) += hwregs.o
>  obj-$(CONFIG_X86) += reboot.o
> +obj-$(CONFIG_ACPI_CPU_TOPOLOGY) += topology.init.o

As before, I'd prefer if this was appended to the earlier group of objects.

Jan