|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v3 1/3] xen/device-tree: Parse 'cpu-map' node for CPU topology exploration
On 29.06.2026 23:58, Hirokazu Takahashi wrote:
> --- a/xen/arch/arm/Kconfig
> +++ b/xen/arch/arm/Kconfig
> @@ -101,6 +101,16 @@ endchoice
>
> source "arch/Kconfig"
>
> +config ARM_CPU_TOPOLOGY
> + bool "CPU topology support (UNSUPPORTED)" if UNSUPPORTED
> + select CPU_TOPOLOGY
> + help
> + Retrieve CPU topology information from the device tree to optimize
> + virtual CPU scheduling.
> +
> + Note: Implementation for parsing CPU topology from the ACPI PPTT
> + is currently missing.
This option isn't itself used anywhere; it exists solely for its "select"
effect. Since topology is an arch-independent concept, I'd suggest
CPU_TOPOLOGY to have the prompt and help text, and there being a per-arch
HAS_CPU_TOPOLOGY (or maybe HAS_GENERIC_CPU_TOPOLOGY, to avoid the false
impression that x86 doesn't deal with topology) which CPU_TOPOLOGY then
depends on.
> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -188,6 +188,14 @@ config VM_EVENT
> config NEEDS_LIBELF
> bool
>
> +config DT_CPU_TOPOLOGY
> + bool
> +
> +config CPU_TOPOLOGY
> + bool
> + select DT_CPU_TOPOLOGY if DEVICE_TREE_PARSE
> + select ACPI_CPU_TOPOLOGY if ACPI
As per part of what I said above, this may also want to be GENERIC_CPU_TOPOLOGY
or some such. Other maintainers' input may be wanted here.
> --- 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_CPU_TOPOLOGY) += cpu-topology.o
As least for now this may want to be
obj-bin-$(CONFIG_CPU_TOPOLOGY) += cpu-topology.init.o
seeing that ...
> --- /dev/null
> +++ b/xen/common/cpu-topology.c
> @@ -0,0 +1,59 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +
> +#include <xen/acpi.h>
> +#include <xen/cpu-topology.h>
> +#include <xen/cpumask.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);
> +}
> +
> +void __init init_cpu_topology(void)
> +{
> + unsigned int cpu;
> +
> + cpu_topology = xzalloc_array(struct cpu_topology, nr_cpu_ids);
> + if ( !cpu_topology )
> + {
> + printk(XENLOG_ERR "Failed to allocate memory for cpu_topology
> table\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_ERR "Failed to allocate memory for cpu_topology
> table\n");
> + return;
> + }
> + }
> +
> + if ( acpi_disabled )
> + dt_init_cpu_topology();
> + else
> + acpi_init_cpu_topology();
> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
... there are only __init functions here.
> --- 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.o
Same here, albeit requiring ...
> --- /dev/null
> +++ b/xen/common/device-tree/cpu-topology.c
> @@ -0,0 +1,352 @@
> +/* 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>
> +#include <xen/numa.h>
> +#include <xen/xvmalloc.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;
... this (which ought to be __ro_after_init anyway) to be moved elsewhere.
> +static const unsigned int __initdata invalid_topo_id = (~0U);
Nit: What use are the parentheses here? This may want to be a #define (with
the identifier all uppercase), in which case the parentheses need keeping.
> +void __init map_cpuid_to_node(unsigned int cpuid,
> + struct dt_device_node *cpu_node)
> +{
> + if ( cpuid < NR_CPUS )
Better use ARRAY_SIZE() when ...
> + dt_cpu_table[cpuid] = cpu_node;
... an array access is guarded.
Just for my own understanding: It is deliberate for cpuid >= NR_CPUS to go
entirely silently here?
As to "cpuid" - please can this be "cpu", as you have it elsewhere. "cpuid"
is a misleading term here when x86 comes into play.
> +static unsigned int __init cpu_node_to_id(struct dt_device_node *cpu_node)
> +{
> + unsigned int cpu;
> + bool found = false;
Pointless initializer; in fact ...
> + for_each_possible_cpu(cpu)
> + {
> + found = (cpu_node == dt_cpu_table[cpu]);
... the declaration could move here, or - better yet - could be omitted
altogether, as the variable is used ...
> + if ( found )
... exacly once.
> +/*
> + * This function returns the logic cpu number of the node.
Nit: "logical"? Also "of the node" is misleading (towards NUMA), "of the DT
node" would be unambiguous.
> + */
> +static unsigned int __init get_cpu_for_node(struct dt_device_node *node)
Pointer-to-const?
> +{
> + struct dt_device_node *cpu_node = dt_parse_phandle(node, "cpu", 0);
Again? Generally everywhere that it is possible and sensible.
> + if ( !cpu_node )
> + return invalid_topo_id;
> +
> + return cpu_node_to_id(cpu_node);
> +}
> +
> +static int __init parse_core(struct dt_device_node *core,
> + unsigned int package_id,
> + unsigned int cluster_id,
> + unsigned int core_id)
> +{
> + char name[20];
Move to the more narrow scope it's used in? (Again please take as a
general remark.)
> + bool leaf = true;
> + unsigned int i = 0;
> + unsigned int cpu;
> +
> + do {
> + struct dt_device_node *t;
> +
> + snprintf(name, sizeof(name), "thread%u", i);
> + t = dt_find_child_node_by_name(core, name);
> +
> + if ( !t )
> + break;
> +
> + leaf = false;
> + cpu = get_cpu_for_node(t);
> + if ( cpu != 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 = i;
> + }
> + else
> + {
> + printk(XENLOG_ERR "ERROR: %pOF: Can't get CPU for thread\n", t);
I don't think we support %pOF (just yet).
> --- a/xen/drivers/acpi/Makefile
> +++ b/xen/drivers/acpi/Makefile
> @@ -10,3 +10,5 @@ obj-$(CONFIG_PM_OP) += pm-op.o
>
> obj-$(CONFIG_X86) += hwregs.o
> obj-$(CONFIG_X86) += reboot.o
> +
> +obj-$(CONFIG_ACPI_CPU_TOPOLOGY) += topology.o
See earlier remarks as to object containing only __init code. Also this may
better be appended (without a blank line) to the earlier block of objects.
> --- /dev/null
> +++ b/xen/include/xen/cpu-topology.h
> @@ -0,0 +1,35 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#ifndef XEN_CPU_TOPOLOGY_H
> +#define XEN_CPU_TOPOLOGY_H
> +
> +#include <xen/dt-cpu-topology.h>
> +
> +struct cpu_topology {
> + cpumask_var_t thread_sibling;
> + cpumask_var_t core_sibling;
> + cpumask_var_t cluster_sibling;
For these to compile independent of what #include-s occurred earlier in the
top-level .c file, don't you need to also "#include <xen/cpumask.h>"? Otoh
I can't quite spot why you #include xen/dt-cpu-topology.h here.
Further shouldn't this move ...
> +};
> +
> +#ifdef CONFIG_CPU_TOPOLOGY
... here?
> +extern struct cpu_topology *cpu_topology;
> +void init_cpu_topology(void);
> +
> +#else /* CONFIG_CPU_TOPOLOGY */
> +
> +#define cpu_topology ((struct cpu_topology *)NULL)
Why exactly is this needed? It very much looks as if its presence may hide
bugs.
> --- /dev/null
> +++ b/xen/include/xen/dt-cpu-topology.h
> @@ -0,0 +1,29 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#ifndef XEN_DT_CPU_TOPOLOGY_H
> +#define XEN_DT_CPU_TOPOLOGY_H
> +
> +#include <xen/device_tree.h>
Again I can't quite see why this would be needed. You need a forward-decl
of ...
> +#ifdef CONFIG_DT_CPU_TOPOLOGY
> +
> +void map_cpuid_to_node(unsigned int cpuid, struct dt_device_node *cpu_node);
... struct dt_device_node, yes, but that's all.
Jan
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |