|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v9 1/4] xen/device-tree: Parse 'cpu-map' node for CPU topology exploration
On 28-Jul-26 07:06, Hirokazu Takahashi wrote:
> Parse the 'cpu-map' node in the Device Tree to extract CPU topology
> information. If the 'cpu-map' node is absent, fall back to
> generating the topology data from the NUMA information. This
> generation assumes exactly one socket per NUMA node and that SMT
> is unsupported.
This does not seem to reflect the implementation. If there is no `cpu-map` node,
you just return error and free the table.
>
> Signed-off-by: Hirokazu Takahashi <taka@xxxxxxxxxxxxx>
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx> # common, acpi
> ---
> xen/arch/arm/Kconfig | 1 +
> xen/arch/arm/smpboot.c | 7 +
> xen/common/Kconfig | 22 ++
> xen/common/Makefile | 1 +
> xen/common/cpu-topology.c | 62 +++++
> xen/common/cpu.c | 5 +
> xen/common/device-tree/Makefile | 1 +
> xen/common/device-tree/cpu-topology.c | 344 ++++++++++++++++++++++++++
> xen/drivers/acpi/Makefile | 1 +
> xen/drivers/acpi/topology.c | 41 +++
> xen/include/xen/acpi.h | 13 +
> xen/include/xen/cpu-topology.h | 34 +++
> xen/include/xen/dt-cpu-topology.h | 35 +++
> 13 files changed, 567 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 843a43897e..1e0fd4957e 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_STACK_PROTECTOR
> select HAS_STATIC_MEMORY
> diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
> index ba5fd2dd52..d957553a44 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>
> @@ -244,6 +246,9 @@ static void __init dt_smp_init_cpus(void)
> }
> else
> tmp_map[i] = hwid;
> +
> + /* Pass the info to dt_init_cpu_topology() */
> + map_cpu_to_dt_node(i, cpu);
You should call this only after successful `arch_cpu_init()`.
> }
>
> if ( !bootcpu_valid )
> @@ -280,6 +285,8 @@ void __init smp_init_cpus(void)
> else
> acpi_smp_init_cpus();
>
> + init_cpu_topology();
This is not a good placement. See below.
> +
> if ( opt_hmp_unsafe )
> warning_add("WARNING: HMP COMPUTING HAS BEEN ENABLED.\n"
> "It has implications on the security and stability of
> the system,\n"
> diff --git a/xen/common/Kconfig b/xen/common/Kconfig
> index da80fdba84..29879a9131 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
>
> @@ -191,6 +194,25 @@ config VM_EVENT
> config NEEDS_LIBELF
> bool
>
> +config GENERIC_CPU_TOPOLOGY
> + bool
> +
> +config DT_CPU_TOPOLOGY
> + bool "Device tree based CPU topology support (UNSUPPORTED)"
> + depends on HAS_GENERIC_CPU_TOPOLOGY && DEVICE_TREE_PARSE && UNSUPPORTED
> + 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 (UNSUPPORTED)"
> + depends on HAS_GENERIC_CPU_TOPOLOGY && ACPI && UNSUPPORTED
> + 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 6018e25614..901bb37925 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..52e31ef518
> --- /dev/null
> +++ b/xen/common/cpu-topology.c
> @@ -0,0 +1,62 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
The main license for Xen is GPLv2-only. Any reason for GPLv2+ in the new files?
I'm asking because if you don't care about the license and simply copied it from
other places, v2-only is a better fit for some organizations that cannot
contribute to v2+.
> +
> +#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;
> +
> + cpu_topology = xvzalloc_array(struct cpu_topology, nr_cpu_ids);
You call it from `smp_init_cpus()` at which point `nr_cpu_ids` is not yet set
and simply denotes `NR_CPUS`.
> + if ( !cpu_topology )
> + 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();
> + 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();
> +}
> +
> +/*
> + * 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..9259be73bc
> --- /dev/null
> +++ b/xen/common/device-tree/cpu-topology.c
> @@ -0,0 +1,344 @@
> +/* 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] = {
> + .thread_id = INVALID_TOPO_ID,
> + .core_id = INVALID_TOPO_ID,
> + .cluster_id = INVALID_TOPO_ID,
> + .package_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)
> +{
> + const 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)
> + {
> + const struct cpu_topology *cpu_topo = &cpu_topology[cpu];
> + const struct cpu_map *map = &cpu_map[cpu];
> +
> + if ( target_cpu > cpu )
> + continue;
> +
> + 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 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_cmp(np->name, name) == 0) )
> + 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 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 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 )
> + {
> + printk(XENLOG_ERR
> + "ERROR: %s: Can't get CPU for thread\n",
> dt_node_name(thread));
> + return -EINVAL;
> + }
> +
> + ASSERT(cpu_map[cpu].package_id == INVALID_TOPO_ID);
> + ASSERT(cpu_map[cpu].cluster_id == INVALID_TOPO_ID);
> + ASSERT(cpu_map[cpu].core_id == INVALID_TOPO_ID);
> + ASSERT(cpu_map[cpu].thread_id == INVALID_TOPO_ID);
This ASSERT block and the identical one below validate DT data, not Xen internal
invariant. Return error instead.
> +
> + 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 = thread_id;
> + }
> +
> + cpu = get_cpu_for_node(core);
> +
> + if ( cpu != INVALID_TOPO_ID )
> + {
> + if ( !leaf )
> + {
> + printk(XENLOG_ERR "ERROR: %s: Core has both threads and CPU\n",
> + dt_node_name(core));
> + return -EINVAL;
> + }
> +
> + ASSERT(cpu_map[cpu].package_id == INVALID_TOPO_ID);
> + ASSERT(cpu_map[cpu].cluster_id == INVALID_TOPO_ID);
> + ASSERT(cpu_map[cpu].core_id == INVALID_TOPO_ID);
> + ASSERT(cpu_map[cpu].thread_id == 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 = 0;
> + }
> + else if ( leaf )
> + {
> + printk(XENLOG_ERR
> + "ERROR: %s: Can't get CPU for leaf core\n",
> dt_node_name(core));
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static int __init parse_cluster(const struct dt_device_node *cluster,
> + unsigned int package_id,
> + unsigned int cluster_id,
> + unsigned int depth)
> +{
> + bool leaf = true;
> + bool has_cores = false;
> + unsigned int core_id;
> + unsigned int child_cluster_id;
> +
> + /*
> + * First check for child clusters; we currently ignore any
> + * information about the nesting of clusters and present the
> + * scheduler with a flat list of them.
> + */
> + for ( child_cluster_id = 0; ; child_cluster_id++ )
> + {
> + const struct dt_device_node *child_cluster;
> + char name[20];
> + int ret;
> +
> + snprintf(name, sizeof(name), "cluster%u", child_cluster_id);
> + child_cluster = dt_find_child_node_by_name(cluster, name);
> +
> + if ( !child_cluster )
> + break;
> +
> + leaf = false;
> + ret = parse_cluster(child_cluster, package_id, child_cluster_id,
> + depth + 1);
> + if ( depth > 0 )
> + printk(XENLOG_WARNING
> + "WARNING: Topology for clusters of clusters not yet
> supported\n");
> + if ( ret != 0 )
> + return ret;
> + }
> +
> + /* Now check for cores */
> + for ( core_id = 0; ; core_id++ )
> + {
> + const struct dt_device_node *core;
> + char name[20];
> + int ret;
> +
> + snprintf(name, sizeof(name), "core%u", core_id);
> + core = dt_find_child_node_by_name(cluster, name);
> +
> + if ( !core )
> + break;
> +
> + has_cores = true;
> +
> + if ( depth == 0 )
> + {
> + printk(XENLOG_ERR
> + "ERROR: %s: cpu-map children should be clusters\n",
> + dt_node_name(core));
> + return -EINVAL;
> + }
> +
> + if ( leaf )
> + {
> + ret = parse_core(core, package_id, cluster_id, core_id);
> + if ( ret != 0 )
> + return ret;
> + }
> + else
> + {
> + printk(XENLOG_ERR "ERROR: %s: Non-leaf cluster with core %s\n",
> + dt_node_name(cluster), name);
> + return -EINVAL;
> + }
> + }
> +
> + if ( leaf && !has_cores )
> + printk(XENLOG_WARNING "WARNING: %s: empty cluster\n",
> + dt_node_name(cluster));
> +
> + return 0;
> +}
> +
> +static int __init parse_socket(const struct dt_device_node *socket)
> +{
> + bool has_socket = false;
> + unsigned int package_id;
> + int ret;
> +
> + for ( package_id = 0; ; package_id++ )
> + {
> + const struct dt_device_node *cluster;
> + char name[20];
> +
> + snprintf(name, sizeof(name), "socket%u", package_id);
> + cluster = dt_find_child_node_by_name(socket, name);
The names are one level off (I know you took it from Linux which suffers from
the same problem): the parameter is the cpu-map node, not a socket, and the
local is a socket node, not a cluster. parse_cluster() has the same problem.
Please name the parameters after what they actually receive,
e.g.parse_socket(cpu_map) with a local 'socket'. It makes it difficult to parse
the code and I'll wait with reviewing this file until this is fixed.
> +
> + if ( !cluster )
> + break;
> +
> + has_socket = true;
> + ret = parse_cluster(cluster, package_id, INVALID_TOPO_ID, 0);
> + if ( ret != 0 )
> + return ret;
> + }
> +
> + if ( !has_socket )
> + ret = parse_cluster(socket, 0, INVALID_TOPO_ID, 0);
> +
> + return ret;
> +}
> +
> +static int __init parse_dt_topology(void)
> +{
> + const struct dt_device_node *cpus;
> + const struct dt_device_node *map;
> +
> + cpus = dt_find_node_by_path("/cpus");
> + if ( !cpus )
> + return -ENOENT;
> +
> + map = dt_find_child_node_by_name(cpus, "cpu-map");
> + if ( !map )
> + return -ENOENT;
> +
> + return parse_socket(map);
> +}
> +
> +int __init dt_init_cpu_topology(void)
> +{
> + unsigned int cpu;
> + int ret;
> +
> + BUG_ON(!acpi_disabled);
> + BUG_ON(!cpu_topology);
ASSERTs are a better fit here, given that these are already validated by the
sole caller.
~Michal
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |