| Message ID | 20220408084517.33082-5-luca.fancellu@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Boot time cpupools | expand |
On 08.04.2022 10:45, Luca Fancellu wrote: > Introduce a way to create different cpupools at boot time, this is > particularly useful on ARM big.LITTLE system where there might be the > need to have different cpupools for each type of core, but also > systems using NUMA can have different cpu pools for each node. > > The feature on arm relies on a specification of the cpupools from the > device tree to build pools and assign cpus to them. > > ACPI is not supported for this feature. > > Documentation is created to explain the feature. > > Signed-off-by: Luca Fancellu <luca.fancellu@arm.com> > Reviewed-by: Juergen Gross <jgross@suse.com> This looks to be not in line with ... > --- > Changes in v6: > - Changed docs, return if booted with ACPI in btcpupools_dtb_parse, > panic if /chosen does not exists. Changed commit message (Julien) > - Add Juergen R-by for the xen/common/sched part that didn't change ... what you say here. What's the scope of Jürgen's R-b? If it has restricted scope, you need to retain that restriction for committers to know. Jan
On 08.04.2022 10:45, Luca Fancellu wrote: > --- > docs/misc/arm/device-tree/cpupools.txt | 140 +++++++++++++++++ > xen/arch/arm/include/asm/smp.h | 3 + > xen/common/Kconfig | 7 + > xen/common/Makefile | 1 + > xen/common/boot_cpupools.c | 207 +++++++++++++++++++++++++ > xen/common/sched/cpupool.c | 12 +- > xen/include/xen/sched.h | 14 ++ > 7 files changed, 383 insertions(+), 1 deletion(-) > create mode 100644 docs/misc/arm/device-tree/cpupools.txt > create mode 100644 xen/common/boot_cpupools.c Under whose maintainership is the new file to fall? Without an addition to ./MAINTAINERS and without the file being placed in xen/common/sched/, it'll be REST maintainers, which I think would better be avoided. Would it perhaps make sense to have this as xen/common/sched/boot.c, allowing other boot-only code to potentially be moved there over time? This would then also avoid me asking about the underscore in the file name: Underscores are a somewhat artificial thing for use in places where dashes can't be used. Yet in the file system dashes are fine, and dashes are (slightly) easier to type. Jan
> On 8 Apr 2022, at 09:56, Jan Beulich <jbeulich@suse.com> wrote: > > On 08.04.2022 10:45, Luca Fancellu wrote: >> Introduce a way to create different cpupools at boot time, this is >> particularly useful on ARM big.LITTLE system where there might be the >> need to have different cpupools for each type of core, but also >> systems using NUMA can have different cpu pools for each node. >> >> The feature on arm relies on a specification of the cpupools from the >> device tree to build pools and assign cpus to them. >> >> ACPI is not supported for this feature. >> >> Documentation is created to explain the feature. >> >> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com> >> Reviewed-by: Juergen Gross <jgross@suse.com> > > This looks to be not in line with ... > >> --- >> Changes in v6: >> - Changed docs, return if booted with ACPI in btcpupools_dtb_parse, >> panic if /chosen does not exists. Changed commit message (Julien) >> - Add Juergen R-by for the xen/common/sched part that didn't change > > ... what you say here. What's the scope of Jürgen's R-b? If it has > restricted scope, you need to retain that restriction for committers > to know. Hi Jan, Sorry about that, I’ve just refreshed my memory with sending-patches.pandoc and I see I should have added Juergen's R-by with # area. It’s the first time I retain an R-by for just a part of the commit, I will remember it for the next time. Cheers, Luca > > Jan
> On 8 Apr 2022, at 10:01, Jan Beulich <jbeulich@suse.com> wrote: > > On 08.04.2022 10:45, Luca Fancellu wrote: >> --- >> docs/misc/arm/device-tree/cpupools.txt | 140 +++++++++++++++++ >> xen/arch/arm/include/asm/smp.h | 3 + >> xen/common/Kconfig | 7 + >> xen/common/Makefile | 1 + >> xen/common/boot_cpupools.c | 207 +++++++++++++++++++++++++ >> xen/common/sched/cpupool.c | 12 +- >> xen/include/xen/sched.h | 14 ++ >> 7 files changed, 383 insertions(+), 1 deletion(-) >> create mode 100644 docs/misc/arm/device-tree/cpupools.txt >> create mode 100644 xen/common/boot_cpupools.c > > Under whose maintainership is the new file to fall? Without an > addition to ./MAINTAINERS and without the file being placed in > xen/common/sched/, it'll be REST maintainers, which I think would > better be avoided. Would it perhaps make sense to have this as > xen/common/sched/boot.c, allowing other boot-only code to > potentially be moved there over time? This would then also avoid > me asking about the underscore in the file name: Underscores are > a somewhat artificial thing for use in places where dashes can't > be used. Yet in the file system dashes are fine, and dashes are > (slightly) easier to type. > Ok I can put the new file under xen/common/sched/ as boot.c, should this new file be under this section? CPU POOLS M: Juergen Gross <jgross@suse.com> M: Dario Faggioli <dfaggioli@suse.com> S: Supported F: xen/common/sched/cpupool.c + F: xen/common/sched/boot.c > Jan >
On 08.04.2022 13:37, Luca Fancellu wrote: > > >> On 8 Apr 2022, at 10:01, Jan Beulich <jbeulich@suse.com> wrote: >> >> On 08.04.2022 10:45, Luca Fancellu wrote: >>> --- >>> docs/misc/arm/device-tree/cpupools.txt | 140 +++++++++++++++++ >>> xen/arch/arm/include/asm/smp.h | 3 + >>> xen/common/Kconfig | 7 + >>> xen/common/Makefile | 1 + >>> xen/common/boot_cpupools.c | 207 +++++++++++++++++++++++++ >>> xen/common/sched/cpupool.c | 12 +- >>> xen/include/xen/sched.h | 14 ++ >>> 7 files changed, 383 insertions(+), 1 deletion(-) >>> create mode 100644 docs/misc/arm/device-tree/cpupools.txt >>> create mode 100644 xen/common/boot_cpupools.c >> >> Under whose maintainership is the new file to fall? Without an >> addition to ./MAINTAINERS and without the file being placed in >> xen/common/sched/, it'll be REST maintainers, which I think would >> better be avoided. Would it perhaps make sense to have this as >> xen/common/sched/boot.c, allowing other boot-only code to >> potentially be moved there over time? This would then also avoid >> me asking about the underscore in the file name: Underscores are >> a somewhat artificial thing for use in places where dashes can't >> be used. Yet in the file system dashes are fine, and dashes are >> (slightly) easier to type. >> > > Ok I can put the new file under xen/common/sched/ as boot.c, should this new > file be under this section? > > CPU POOLS > M: Juergen Gross <jgross@suse.com> > M: Dario Faggioli <dfaggioli@suse.com> > S: Supported > F: xen/common/sched/cpupool.c > + F: xen/common/sched/boot.c If it's to hold general scheduler code (which this shorter name would suggest), it shouldn't need any change to ./MAINTAINERS as the scheduler section would already cover it then. If it was to remain CPU-pools-specific, then you'd need to stick to the longer name and put it in the section you have reproduced above. Jan
Hi Luca, On 08/04/2022 09:45, Luca Fancellu wrote: > Introduce a way to create different cpupools at boot time, this is > particularly useful on ARM big.LITTLE system where there might be the > need to have different cpupools for each type of core, but also > systems using NUMA can have different cpu pools for each node. > > The feature on arm relies on a specification of the cpupools from the > device tree to build pools and assign cpus to them. > > ACPI is not supported for this feature. > > Documentation is created to explain the feature. > > Signed-off-by: Luca Fancellu <luca.fancellu@arm.com> > Reviewed-by: Juergen Gross <jgross@suse.com> > --- > Changes in v6: > - Changed docs, return if booted with ACPI in btcpupools_dtb_parse, > panic if /chosen does not exists. Changed commit message (Julien) I went through the changes and they LGTM. Stefano has paid closer attention to this series, so I will leave him to do the full review. Cheers,
On Fri, 8 Apr 2022, Luca Fancellu wrote: > Introduce a way to create different cpupools at boot time, this is > particularly useful on ARM big.LITTLE system where there might be the > need to have different cpupools for each type of core, but also > systems using NUMA can have different cpu pools for each node. > > The feature on arm relies on a specification of the cpupools from the > device tree to build pools and assign cpus to them. > > ACPI is not supported for this feature. > > Documentation is created to explain the feature. > > Signed-off-by: Luca Fancellu <luca.fancellu@arm.com> > Reviewed-by: Juergen Gross <jgross@suse.com> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > --- > Changes in v6: > - Changed docs, return if booted with ACPI in btcpupools_dtb_parse, > panic if /chosen does not exists. Changed commit message (Julien) > - Add Juergen R-by for the xen/common/sched part that didn't change > Changes in v5: > - Fixed wrong variable name, swapped schedulers, add scheduler info > in the printk (Stefano) > - introduce assert in cpupool_init and btcpupools_get_cpupool_id to > harden the code > Changes in v4: > - modify Makefile to put in *.init.o, fixed stubs and macro (Jan) > - fixed docs, fix brakets (Stefano) > - keep cpu0 in Pool-0 (Julien) > - moved printk from btcpupools_allocate_pools to > btcpupools_get_cpupool_id > - Add to docs constraint about cpu0 and Pool-0 > Changes in v3: > - Add newline to cpupools.txt and removed "default n" from Kconfig (Jan) > - Fixed comment, moved defines, used global cpu_online_map, use > HAS_DEVICE_TREE instead of ARM and place arch specific code in header > (Juergen) > - Fix brakets, x86 code only panic, get rid of scheduler dt node, don't > save pool pointer and look for it from the pool list (Stefano) > - Changed data structures to allow modification to the code. > Changes in v2: > - Move feature to common code (Juergen) > - Try to decouple dtb parse and cpupool creation to allow > more way to specify cpupools (for example command line) > - Created standalone dt node for the scheduler so it can > be used in future work to set scheduler specific > parameters > - Use only auto generated ids for cpupools > --- > docs/misc/arm/device-tree/cpupools.txt | 140 +++++++++++++++++ > xen/arch/arm/include/asm/smp.h | 3 + > xen/common/Kconfig | 7 + > xen/common/Makefile | 1 + > xen/common/boot_cpupools.c | 207 +++++++++++++++++++++++++ > xen/common/sched/cpupool.c | 12 +- > xen/include/xen/sched.h | 14 ++ > 7 files changed, 383 insertions(+), 1 deletion(-) > create mode 100644 docs/misc/arm/device-tree/cpupools.txt > create mode 100644 xen/common/boot_cpupools.c > > diff --git a/docs/misc/arm/device-tree/cpupools.txt b/docs/misc/arm/device-tree/cpupools.txt > new file mode 100644 > index 000000000000..40cc8135c66f > --- /dev/null > +++ b/docs/misc/arm/device-tree/cpupools.txt > @@ -0,0 +1,140 @@ > +Boot time cpupools > +================== > + > +When BOOT_TIME_CPUPOOLS is enabled in the Xen configuration, it is possible to > +create cpupools during boot phase by specifying them in the device tree. > +ACPI is not supported for this feature. > + > +Cpupools specification nodes shall be direct childs of /chosen node. > +Each cpupool node contains the following properties: > + > +- compatible (mandatory) > + > + Must always include the compatiblity string: "xen,cpupool". > + > +- cpupool-cpus (mandatory) > + > + Must be a list of device tree phandle to nodes describing cpus (e.g. having > + device_type = "cpu"), it can't be empty. > + > +- cpupool-sched (optional) > + > + Must be a string having the name of a Xen scheduler. Check the sched=<...> > + boot argument for allowed values [1]. When this property is omitted, the Xen > + default scheduler will be used. > + > + > +Constraints > +=========== > + > +If no cpupools are specified, all cpus will be assigned to one cpupool > +implicitly created (Pool-0). > + > +If cpupools node are specified, but not every cpu brought up by Xen is assigned, > +all the not assigned cpu will be assigned to an additional cpupool. > + > +If a cpu is assigned to a cpupool, but it's not brought up correctly, Xen will > +stop. > + > +The boot cpu must be assigned to Pool-0, so the cpupool containing that core > +will become Pool-0 automatically. > + > + > +Examples > +======== > + > +A system having two types of core, the following device tree specification will > +instruct Xen to have two cpupools: > + > +- The cpupool with id 0 will have 4 cpus assigned. > +- The cpupool with id 1 will have 2 cpus assigned. > + > +The following example can work only if hmp-unsafe=1 is passed to Xen boot > +arguments, otherwise not all cores will be brought up by Xen and the cpupool > +creation process will stop Xen. > + > + > +a72_1: cpu@0 { > + compatible = "arm,cortex-a72"; > + reg = <0x0 0x0>; > + device_type = "cpu"; > + [...] > +}; > + > +a72_2: cpu@1 { > + compatible = "arm,cortex-a72"; > + reg = <0x0 0x1>; > + device_type = "cpu"; > + [...] > +}; > + > +a53_1: cpu@100 { > + compatible = "arm,cortex-a53"; > + reg = <0x0 0x100>; > + device_type = "cpu"; > + [...] > +}; > + > +a53_2: cpu@101 { > + compatible = "arm,cortex-a53"; > + reg = <0x0 0x101>; > + device_type = "cpu"; > + [...] > +}; > + > +a53_3: cpu@102 { > + compatible = "arm,cortex-a53"; > + reg = <0x0 0x102>; > + device_type = "cpu"; > + [...] > +}; > + > +a53_4: cpu@103 { > + compatible = "arm,cortex-a53"; > + reg = <0x0 0x103>; > + device_type = "cpu"; > + [...] > +}; > + > +chosen { > + > + cpupool_a { > + compatible = "xen,cpupool"; > + cpupool-cpus = <&a53_1 &a53_2 &a53_3 &a53_4>; > + }; > + cpupool_b { > + compatible = "xen,cpupool"; > + cpupool-cpus = <&a72_1 &a72_2>; > + cpupool-sched = "credit2"; > + }; > + > + [...] > + > +}; > + > + > +A system having the cpupools specification below will instruct Xen to have three > +cpupools: > + > +- The cpupool Pool-0 will have 2 cpus assigned. > +- The cpupool Pool-1 will have 2 cpus assigned. > +- The cpupool Pool-2 will have 2 cpus assigned (created by Xen with all the not > + assigned cpus a53_3 and a53_4). > + > +chosen { > + > + cpupool_a { > + compatible = "xen,cpupool"; > + cpupool-cpus = <&a53_1 &a53_2>; > + }; > + cpupool_b { > + compatible = "xen,cpupool"; > + cpupool-cpus = <&a72_1 &a72_2>; > + cpupool-sched = "null"; > + }; > + > + [...] > + > +}; > + > +[1] docs/misc/xen-command-line.pandoc > diff --git a/xen/arch/arm/include/asm/smp.h b/xen/arch/arm/include/asm/smp.h > index af5a2fe65266..83c0cd69767b 100644 > --- a/xen/arch/arm/include/asm/smp.h > +++ b/xen/arch/arm/include/asm/smp.h > @@ -34,6 +34,9 @@ extern void init_secondary(void); > extern void smp_init_cpus(void); > extern void smp_clear_cpu_maps (void); > extern int smp_get_max_cpus (void); > + > +#define cpu_physical_id(cpu) cpu_logical_map(cpu) > + > #endif > > /* > diff --git a/xen/common/Kconfig b/xen/common/Kconfig > index d921c74d615e..70aac5220e75 100644 > --- a/xen/common/Kconfig > +++ b/xen/common/Kconfig > @@ -22,6 +22,13 @@ config GRANT_TABLE > > If unsure, say Y. > > +config BOOT_TIME_CPUPOOLS > + bool "Create cpupools at boot time" > + depends on HAS_DEVICE_TREE > + help > + Creates cpupools during boot time and assigns cpus to them. Cpupools > + options can be specified in the device tree. > + > config ALTERNATIVE_CALL > bool > > diff --git a/xen/common/Makefile b/xen/common/Makefile > index b1e076c30b81..218174ca8b6b 100644 > --- a/xen/common/Makefile > +++ b/xen/common/Makefile > @@ -1,5 +1,6 @@ > obj-$(CONFIG_ARGO) += argo.o > obj-y += bitmap.o > +obj-$(CONFIG_BOOT_TIME_CPUPOOLS) += boot_cpupools.init.o > obj-$(CONFIG_HYPFS_CONFIG) += config_data.o > obj-$(CONFIG_CORE_PARKING) += core_parking.o > obj-y += cpu.o > diff --git a/xen/common/boot_cpupools.c b/xen/common/boot_cpupools.c > new file mode 100644 > index 000000000000..9429a5025fc4 > --- /dev/null > +++ b/xen/common/boot_cpupools.c > @@ -0,0 +1,207 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * xen/common/boot_cpupools.c > + * > + * Code to create cpupools at boot time. > + * > + * Copyright (C) 2022 Arm Ltd. > + */ > + > +#include <xen/sched.h> > +#include <asm/acpi.h> > + > +/* > + * pool_cpu_map: Index is logical cpu number, content is cpupool id, (-1) for > + * unassigned. > + * pool_sched_map: Index is cpupool id, content is scheduler id, (-1) for > + * unassigned. > + */ > +static int __initdata pool_cpu_map[NR_CPUS] = { [0 ... NR_CPUS-1] = -1 }; > +static int __initdata pool_sched_map[NR_CPUS] = { [0 ... NR_CPUS-1] = -1 }; > +static unsigned int __initdata next_pool_id; > + > +#define BTCPUPOOLS_DT_NODE_NO_REG (-1) > +#define BTCPUPOOLS_DT_NODE_NO_LOG_CPU (-2) > + > +static int __init get_logical_cpu_from_hw_id(unsigned int hwid) > +{ > + unsigned int i; > + > + for ( i = 0; i < nr_cpu_ids; i++ ) > + { > + if ( cpu_physical_id(i) == hwid ) > + return i; > + } > + > + return -1; > +} > + > +static int __init > +get_logical_cpu_from_cpu_node(const struct dt_device_node *cpu_node) > +{ > + int cpu_num; > + const __be32 *prop; > + unsigned int cpu_reg; > + > + prop = dt_get_property(cpu_node, "reg", NULL); > + if ( !prop ) > + return BTCPUPOOLS_DT_NODE_NO_REG; > + > + cpu_reg = dt_read_number(prop, dt_n_addr_cells(cpu_node)); > + > + cpu_num = get_logical_cpu_from_hw_id(cpu_reg); > + if ( cpu_num < 0 ) > + return BTCPUPOOLS_DT_NODE_NO_LOG_CPU; > + > + return cpu_num; > +} > + > +static int __init check_and_get_sched_id(const char* scheduler_name) > +{ > + int sched_id = sched_get_id_by_name(scheduler_name); > + > + if ( sched_id < 0 ) > + panic("Scheduler %s does not exists!\n", scheduler_name); > + > + return sched_id; > +} > + > +void __init btcpupools_dtb_parse(void) > +{ > + const struct dt_device_node *chosen, *node; > + > + if ( !acpi_disabled ) > + return; > + > + chosen = dt_find_node_by_path("/chosen"); > + if ( !chosen ) > + panic("/chosen missing. Boot time cpupools can't be parsed from DT.\n"); > + > + dt_for_each_child_node(chosen, node) > + { > + const struct dt_device_node *phandle_node; > + int sched_id = -1; > + const char* scheduler_name; > + unsigned int i = 0; > + > + if ( !dt_device_is_compatible(node, "xen,cpupool") ) > + continue; > + > + if ( !dt_property_read_string(node, "cpupool-sched", &scheduler_name) ) > + sched_id = check_and_get_sched_id(scheduler_name); > + > + phandle_node = dt_parse_phandle(node, "cpupool-cpus", i++); > + if ( !phandle_node ) > + panic("Missing or empty cpupool-cpus property!\n"); > + > + while ( phandle_node ) > + { > + int cpu_num; > + > + cpu_num = get_logical_cpu_from_cpu_node(phandle_node); > + > + if ( cpu_num < 0 ) > + panic("Error retrieving logical cpu from node %s (%d)\n", > + dt_node_name(node), cpu_num); > + > + if ( pool_cpu_map[cpu_num] != -1 ) > + panic("Logical cpu %d already added to a cpupool!\n", cpu_num); > + > + pool_cpu_map[cpu_num] = next_pool_id; > + > + phandle_node = dt_parse_phandle(node, "cpupool-cpus", i++); > + } > + > + /* Save scheduler choice for this cpupool id */ > + pool_sched_map[next_pool_id] = sched_id; > + > + /* Let Xen generate pool ids */ > + next_pool_id++; > + } > +} > + > +void __init btcpupools_allocate_pools(void) > +{ > + unsigned int i; > + bool add_extra_cpupool = false; > + int swap_id = -1; > + > + /* > + * If there are no cpupools, the value of next_pool_id is zero, so the code > + * below will assign every cpu to cpupool0 as the default behavior. > + * When there are cpupools, the code below is assigning all the not > + * assigned cpu to a new pool (next_pool_id value is the last id + 1). > + * In the same loop we check if there is any assigned cpu that is not > + * online. > + */ > + for ( i = 0; i < nr_cpu_ids; i++ ) > + { > + if ( cpumask_test_cpu(i, &cpu_online_map) ) > + { > + /* Unassigned cpu gets next_pool_id pool id value */ > + if ( pool_cpu_map[i] < 0 ) > + { > + pool_cpu_map[i] = next_pool_id; > + add_extra_cpupool = true; > + } > + > + /* > + * Cpu0 must be in cpupool0, otherwise some operations like moving > + * cpus between cpupools, cpu hotplug, destroying cpupools, shutdown > + * of the host, might not work in a sane way. > + */ > + if ( !i && (pool_cpu_map[0] != 0) ) > + swap_id = pool_cpu_map[0]; > + > + if ( swap_id != -1 ) > + { > + if ( pool_cpu_map[i] == swap_id ) > + pool_cpu_map[i] = 0; > + else if ( pool_cpu_map[i] == 0 ) > + pool_cpu_map[i] = swap_id; > + } > + } > + else > + { > + if ( pool_cpu_map[i] >= 0 ) > + panic("Pool-%d contains cpu%u that is not online!\n", > + pool_cpu_map[i], i); > + } > + } > + > + /* A swap happened, swap schedulers between cpupool id 0 and the other */ > + if ( swap_id != -1 ) > + { > + int swap_sched = pool_sched_map[swap_id]; > + > + pool_sched_map[swap_id] = pool_sched_map[0]; > + pool_sched_map[0] = swap_sched; > + } > + > + if ( add_extra_cpupool ) > + next_pool_id++; > + > + /* Create cpupools with selected schedulers */ > + for ( i = 0; i < next_pool_id; i++ ) > + cpupool_create_pool(i, pool_sched_map[i]); > +} > + > +unsigned int __init btcpupools_get_cpupool_id(unsigned int cpu) > +{ > + ASSERT((cpu < NR_CPUS) && (pool_cpu_map[cpu] >= 0)); > + > + printk(XENLOG_INFO "Logical CPU %u in Pool-%d (Scheduler id: %d).\n", > + cpu, pool_cpu_map[cpu], pool_sched_map[pool_cpu_map[cpu]]); > + > + return pool_cpu_map[cpu]; > +} > + > +/* > + * 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/sched/cpupool.c b/xen/common/sched/cpupool.c > index 89a891af7076..86a175f99cd5 100644 > --- a/xen/common/sched/cpupool.c > +++ b/xen/common/sched/cpupool.c > @@ -1247,12 +1247,22 @@ static int __init cf_check cpupool_init(void) > cpupool_put(cpupool0); > register_cpu_notifier(&cpu_nfb); > > + btcpupools_dtb_parse(); > + > + btcpupools_allocate_pools(); > + > spin_lock(&cpupool_lock); > > cpumask_copy(&cpupool_free_cpus, &cpu_online_map); > > for_each_cpu ( cpu, &cpupool_free_cpus ) > - cpupool_assign_cpu_locked(cpupool0, cpu); > + { > + unsigned int pool_id = btcpupools_get_cpupool_id(cpu); > + struct cpupool *pool = cpupool_find_by_id(pool_id); > + > + ASSERT(pool); > + cpupool_assign_cpu_locked(pool, cpu); > + } > > spin_unlock(&cpupool_lock); > > diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h > index b527f141a1d3..453e98f1cba8 100644 > --- a/xen/include/xen/sched.h > +++ b/xen/include/xen/sched.h > @@ -1178,6 +1178,20 @@ extern void cf_check dump_runq(unsigned char key); > > void arch_do_physinfo(struct xen_sysctl_physinfo *pi); > > +#ifdef CONFIG_BOOT_TIME_CPUPOOLS > +void btcpupools_allocate_pools(void); > +unsigned int btcpupools_get_cpupool_id(unsigned int cpu); > +void btcpupools_dtb_parse(void); > + > +#else /* !CONFIG_BOOT_TIME_CPUPOOLS */ > +static inline void btcpupools_allocate_pools(void) {} > +static inline void btcpupools_dtb_parse(void) {} > +static inline unsigned int btcpupools_get_cpupool_id(unsigned int cpu) > +{ > + return 0; > +} > +#endif > + > #endif /* __SCHED_H__ */ > > /* > -- > 2.17.1 >
On Fri, 8 Apr 2022, Jan Beulich wrote: > On 08.04.2022 13:37, Luca Fancellu wrote: > > > > > >> On 8 Apr 2022, at 10:01, Jan Beulich <jbeulich@suse.com> wrote: > >> > >> On 08.04.2022 10:45, Luca Fancellu wrote: > >>> --- > >>> docs/misc/arm/device-tree/cpupools.txt | 140 +++++++++++++++++ > >>> xen/arch/arm/include/asm/smp.h | 3 + > >>> xen/common/Kconfig | 7 + > >>> xen/common/Makefile | 1 + > >>> xen/common/boot_cpupools.c | 207 +++++++++++++++++++++++++ > >>> xen/common/sched/cpupool.c | 12 +- > >>> xen/include/xen/sched.h | 14 ++ > >>> 7 files changed, 383 insertions(+), 1 deletion(-) > >>> create mode 100644 docs/misc/arm/device-tree/cpupools.txt > >>> create mode 100644 xen/common/boot_cpupools.c > >> > >> Under whose maintainership is the new file to fall? Without an > >> addition to ./MAINTAINERS and without the file being placed in > >> xen/common/sched/, it'll be REST maintainers, which I think would > >> better be avoided. Would it perhaps make sense to have this as > >> xen/common/sched/boot.c, allowing other boot-only code to > >> potentially be moved there over time? This would then also avoid > >> me asking about the underscore in the file name: Underscores are > >> a somewhat artificial thing for use in places where dashes can't > >> be used. Yet in the file system dashes are fine, and dashes are > >> (slightly) easier to type. > >> > > > > Ok I can put the new file under xen/common/sched/ as boot.c, should this new > > file be under this section? > > > > CPU POOLS > > M: Juergen Gross <jgross@suse.com> > > M: Dario Faggioli <dfaggioli@suse.com> > > S: Supported > > F: xen/common/sched/cpupool.c > > + F: xen/common/sched/boot.c > > If it's to hold general scheduler code (which this shorter name would > suggest), it shouldn't need any change to ./MAINTAINERS as the > scheduler section would already cover it then. If it was to remain > CPU-pools-specific, then you'd need to stick to the longer name and > put it in the section you have reproduced above. In my opinion it is best if the maintenance of boot_cpupools.c falls under "CPU POOLS". Luca, you can retain my reviewed-by when you add the change to MAINTAINERS or rename the file. I don't have an opinion if it should be called xen/common/boot_cpupools.c or xen/common/boot-cpupools.c
On 08.04.22 22:25, Stefano Stabellini wrote: > On Fri, 8 Apr 2022, Jan Beulich wrote: >> On 08.04.2022 13:37, Luca Fancellu wrote: >>> >>> >>>> On 8 Apr 2022, at 10:01, Jan Beulich <jbeulich@suse.com> wrote: >>>> >>>> On 08.04.2022 10:45, Luca Fancellu wrote: >>>>> --- >>>>> docs/misc/arm/device-tree/cpupools.txt | 140 +++++++++++++++++ >>>>> xen/arch/arm/include/asm/smp.h | 3 + >>>>> xen/common/Kconfig | 7 + >>>>> xen/common/Makefile | 1 + >>>>> xen/common/boot_cpupools.c | 207 +++++++++++++++++++++++++ >>>>> xen/common/sched/cpupool.c | 12 +- >>>>> xen/include/xen/sched.h | 14 ++ >>>>> 7 files changed, 383 insertions(+), 1 deletion(-) >>>>> create mode 100644 docs/misc/arm/device-tree/cpupools.txt >>>>> create mode 100644 xen/common/boot_cpupools.c >>>> >>>> Under whose maintainership is the new file to fall? Without an >>>> addition to ./MAINTAINERS and without the file being placed in >>>> xen/common/sched/, it'll be REST maintainers, which I think would >>>> better be avoided. Would it perhaps make sense to have this as >>>> xen/common/sched/boot.c, allowing other boot-only code to >>>> potentially be moved there over time? This would then also avoid >>>> me asking about the underscore in the file name: Underscores are >>>> a somewhat artificial thing for use in places where dashes can't >>>> be used. Yet in the file system dashes are fine, and dashes are >>>> (slightly) easier to type. >>>> >>> >>> Ok I can put the new file under xen/common/sched/ as boot.c, should this new >>> file be under this section? >>> >>> CPU POOLS >>> M: Juergen Gross <jgross@suse.com> >>> M: Dario Faggioli <dfaggioli@suse.com> >>> S: Supported >>> F: xen/common/sched/cpupool.c >>> + F: xen/common/sched/boot.c >> >> If it's to hold general scheduler code (which this shorter name would >> suggest), it shouldn't need any change to ./MAINTAINERS as the >> scheduler section would already cover it then. If it was to remain >> CPU-pools-specific, then you'd need to stick to the longer name and >> put it in the section you have reproduced above. > > In my opinion it is best if the maintenance of boot_cpupools.c falls > under "CPU POOLS". Luca, you can retain my reviewed-by when you add > the change to MAINTAINERS or rename the file. > > I don't have an opinion if it should be called > xen/common/boot_cpupools.c or xen/common/boot-cpupools.c > I'd go with xen/common/sched/boot-cpupool.c Juergen
On 08.04.2022 22:25, Stefano Stabellini wrote: > On Fri, 8 Apr 2022, Jan Beulich wrote: >> On 08.04.2022 13:37, Luca Fancellu wrote: >>> >>> >>>> On 8 Apr 2022, at 10:01, Jan Beulich <jbeulich@suse.com> wrote: >>>> >>>> On 08.04.2022 10:45, Luca Fancellu wrote: >>>>> --- >>>>> docs/misc/arm/device-tree/cpupools.txt | 140 +++++++++++++++++ >>>>> xen/arch/arm/include/asm/smp.h | 3 + >>>>> xen/common/Kconfig | 7 + >>>>> xen/common/Makefile | 1 + >>>>> xen/common/boot_cpupools.c | 207 +++++++++++++++++++++++++ >>>>> xen/common/sched/cpupool.c | 12 +- >>>>> xen/include/xen/sched.h | 14 ++ >>>>> 7 files changed, 383 insertions(+), 1 deletion(-) >>>>> create mode 100644 docs/misc/arm/device-tree/cpupools.txt >>>>> create mode 100644 xen/common/boot_cpupools.c >>>> >>>> Under whose maintainership is the new file to fall? Without an >>>> addition to ./MAINTAINERS and without the file being placed in >>>> xen/common/sched/, it'll be REST maintainers, which I think would >>>> better be avoided. Would it perhaps make sense to have this as >>>> xen/common/sched/boot.c, allowing other boot-only code to >>>> potentially be moved there over time? This would then also avoid >>>> me asking about the underscore in the file name: Underscores are >>>> a somewhat artificial thing for use in places where dashes can't >>>> be used. Yet in the file system dashes are fine, and dashes are >>>> (slightly) easier to type. >>>> >>> >>> Ok I can put the new file under xen/common/sched/ as boot.c, should this new >>> file be under this section? >>> >>> CPU POOLS >>> M: Juergen Gross <jgross@suse.com> >>> M: Dario Faggioli <dfaggioli@suse.com> >>> S: Supported >>> F: xen/common/sched/cpupool.c >>> + F: xen/common/sched/boot.c >> >> If it's to hold general scheduler code (which this shorter name would >> suggest), it shouldn't need any change to ./MAINTAINERS as the >> scheduler section would already cover it then. If it was to remain >> CPU-pools-specific, then you'd need to stick to the longer name and >> put it in the section you have reproduced above. > > In my opinion it is best if the maintenance of boot_cpupools.c falls > under "CPU POOLS". Luca, you can retain my reviewed-by when you add > the change to MAINTAINERS or rename the file. Yet even then, with cpupools.c living in sched/, ... > I don't have an opinion if it should be called > xen/common/boot_cpupools.c or xen/common/boot-cpupools.c > ... this one may want living there are well. Jan
> On 11 Apr 2022, at 07:15, Jan Beulich <jbeulich@suse.com> wrote: > > On 08.04.2022 22:25, Stefano Stabellini wrote: >> On Fri, 8 Apr 2022, Jan Beulich wrote: >>> On 08.04.2022 13:37, Luca Fancellu wrote: >>>> >>>> >>>>> On 8 Apr 2022, at 10:01, Jan Beulich <jbeulich@suse.com> wrote: >>>>> >>>>> On 08.04.2022 10:45, Luca Fancellu wrote: >>>>>> --- >>>>>> docs/misc/arm/device-tree/cpupools.txt | 140 +++++++++++++++++ >>>>>> xen/arch/arm/include/asm/smp.h | 3 + >>>>>> xen/common/Kconfig | 7 + >>>>>> xen/common/Makefile | 1 + >>>>>> xen/common/boot_cpupools.c | 207 +++++++++++++++++++++++++ >>>>>> xen/common/sched/cpupool.c | 12 +- >>>>>> xen/include/xen/sched.h | 14 ++ >>>>>> 7 files changed, 383 insertions(+), 1 deletion(-) >>>>>> create mode 100644 docs/misc/arm/device-tree/cpupools.txt >>>>>> create mode 100644 xen/common/boot_cpupools.c >>>>> >>>>> Under whose maintainership is the new file to fall? Without an >>>>> addition to ./MAINTAINERS and without the file being placed in >>>>> xen/common/sched/, it'll be REST maintainers, which I think would >>>>> better be avoided. Would it perhaps make sense to have this as >>>>> xen/common/sched/boot.c, allowing other boot-only code to >>>>> potentially be moved there over time? This would then also avoid >>>>> me asking about the underscore in the file name: Underscores are >>>>> a somewhat artificial thing for use in places where dashes can't >>>>> be used. Yet in the file system dashes are fine, and dashes are >>>>> (slightly) easier to type. >>>>> >>>> >>>> Ok I can put the new file under xen/common/sched/ as boot.c, should this new >>>> file be under this section? >>>> >>>> CPU POOLS >>>> M: Juergen Gross <jgross@suse.com> >>>> M: Dario Faggioli <dfaggioli@suse.com> >>>> S: Supported >>>> F: xen/common/sched/cpupool.c >>>> + F: xen/common/sched/boot.c >>> >>> If it's to hold general scheduler code (which this shorter name would >>> suggest), it shouldn't need any change to ./MAINTAINERS as the >>> scheduler section would already cover it then. If it was to remain >>> CPU-pools-specific, then you'd need to stick to the longer name and >>> put it in the section you have reproduced above. >> >> In my opinion it is best if the maintenance of boot_cpupools.c falls >> under "CPU POOLS". Luca, you can retain my reviewed-by when you add >> the change to MAINTAINERS or rename the file. > > Yet even then, with cpupools.c living in sched/, ... > >> I don't have an opinion if it should be called >> xen/common/boot_cpupools.c or xen/common/boot-cpupools.c >> > > ... this one may want living there are well. Yes I agree with you all, I will rename it to xen/common/sched/boot-cpupool.c and add it in MAINTAINERS. > > Jan
On Mon, 2022-04-11 at 08:29 +0000, Luca Fancellu wrote: > > On 11 Apr 2022, at 07:15, Jan Beulich <jbeulich@suse.com> wrote: > > On 08.04.2022 22:25, Stefano Stabellini wrote: > > > In my opinion it is best if the maintenance of boot_cpupools.c > > > falls > > > under "CPU POOLS". Luca, you can retain my reviewed-by when you > > > add > > > the change to MAINTAINERS or rename the file. > > > > Yet even then, with cpupools.c living in sched/, ... > > > > > I don't have an opinion if it should be called > > > xen/common/boot_cpupools.c or xen/common/boot-cpupools.c > > > > > > > ... this one may want living there are well. > > Yes I agree with you all, I will rename it to xen/common/sched/boot- > cpupool.c > and add it in MAINTAINERS. > FWIW, I agree as well. With something like this, IMO: CPU POOLS M: Juergen Gross <jgross@suse.com> M: Dario Faggioli <dfaggioli@suse.com> S: Supported F: xen/common/sched/*cpupool.c Regards
Hi Luca, On 08/04/2022 09:45, Luca Fancellu wrote: > diff --git a/docs/misc/arm/device-tree/cpupools.txt b/docs/misc/arm/device-tree/cpupools.txt > new file mode 100644 > index 000000000000..40cc8135c66f > --- /dev/null > +++ b/docs/misc/arm/device-tree/cpupools.txt > @@ -0,0 +1,140 @@ > +Boot time cpupools > +================== > + > +When BOOT_TIME_CPUPOOLS is enabled in the Xen configuration, it is possible to > +create cpupools during boot phase by specifying them in the device tree. > +ACPI is not supported for this feature. > + > +Cpupools specification nodes shall be direct childs of /chosen node. > +Each cpupool node contains the following properties: > + > +- compatible (mandatory) > + > + Must always include the compatiblity string: "xen,cpupool". > + > +- cpupool-cpus (mandatory) > + > + Must be a list of device tree phandle to nodes describing cpus (e.g. having > + device_type = "cpu"), it can't be empty. > + > +- cpupool-sched (optional) > + > + Must be a string having the name of a Xen scheduler. Check the sched=<...> > + boot argument for allowed values [1]. When this property is omitted, the Xen > + default scheduler will be used. > + > + > +Constraints > +=========== > + > +If no cpupools are specified, all cpus will be assigned to one cpupool > +implicitly created (Pool-0). > + > +If cpupools node are specified, but not every cpu brought up by Xen is assigned, > +all the not assigned cpu will be assigned to an additional cpupool. > + > +If a cpu is assigned to a cpupool, but it's not brought up correctly, Xen will > +stop. > + > +The boot cpu must be assigned to Pool-0, so the cpupool containing that core > +will become Pool-0 automatically. > + > + > +Examples > +======== > + > +A system having two types of core, the following device tree specification will > +instruct Xen to have two cpupools: > + > +- The cpupool with id 0 will have 4 cpus assigned. > +- The cpupool with id 1 will have 2 cpus assigned. AFAIK, there are no guarantee that Xen will parse cpupool_a first. So it would be possible that the ID are inverted here. This could happen if you want to keep the boot CPU in pool 0 and it is not cpu@0 (some bootloaders allows you to change the boot CPU). Also, here you write "The cpupool with id X" but ... > +A system having the cpupools specification below will instruct Xen to have three > +cpupools: > + > +- The cpupool Pool-0 will have 2 cpus assigned. > +- The cpupool Pool-1 will have 2 cpus assigned. > +- The cpupool Pool-2 will have 2 cpus assigned (created by Xen with all the not > + assigned cpus a53_3 and a53_4). here you write "The cpupool Pool-X". Can you be consistent? On a separate topic, I think dom0_max_vcpus() needs to be updated to by default (i.e when opt_dom0_max_vcpus == 0) the number of vCPUs match the number of vCPUs in the cpupool (I think 0) used to created dom0. Cheers,
> On 11 Apr 2022, at 11:58, Julien Grall <julien@xen.org> wrote: > > Hi Luca, > > On 08/04/2022 09:45, Luca Fancellu wrote: >> diff --git a/docs/misc/arm/device-tree/cpupools.txt b/docs/misc/arm/device-tree/cpupools.txt >> new file mode 100644 >> index 000000000000..40cc8135c66f >> --- /dev/null >> +++ b/docs/misc/arm/device-tree/cpupools.txt >> @@ -0,0 +1,140 @@ >> +Boot time cpupools >> +================== >> + >> +When BOOT_TIME_CPUPOOLS is enabled in the Xen configuration, it is possible to >> +create cpupools during boot phase by specifying them in the device tree. >> +ACPI is not supported for this feature. >> + >> +Cpupools specification nodes shall be direct childs of /chosen node. >> +Each cpupool node contains the following properties: >> + >> +- compatible (mandatory) >> + >> + Must always include the compatiblity string: "xen,cpupool". >> + >> +- cpupool-cpus (mandatory) >> + >> + Must be a list of device tree phandle to nodes describing cpus (e.g. having >> + device_type = "cpu"), it can't be empty. >> + >> +- cpupool-sched (optional) >> + >> + Must be a string having the name of a Xen scheduler. Check the sched=<...> >> + boot argument for allowed values [1]. When this property is omitted, the Xen >> + default scheduler will be used. >> + >> + >> +Constraints >> +=========== >> + >> +If no cpupools are specified, all cpus will be assigned to one cpupool >> +implicitly created (Pool-0). >> + >> +If cpupools node are specified, but not every cpu brought up by Xen is assigned, >> +all the not assigned cpu will be assigned to an additional cpupool. >> + >> +If a cpu is assigned to a cpupool, but it's not brought up correctly, Xen will >> +stop. >> + >> +The boot cpu must be assigned to Pool-0, so the cpupool containing that core >> +will become Pool-0 automatically. >> + >> + >> +Examples >> +======== >> + >> +A system having two types of core, the following device tree specification will >> +instruct Xen to have two cpupools: >> + >> +- The cpupool with id 0 will have 4 cpus assigned. >> +- The cpupool with id 1 will have 2 cpus assigned. > > AFAIK, there are no guarantee that Xen will parse cpupool_a first. So it would be possible that the ID are inverted here. > > This could happen if you want to keep the boot CPU in pool 0 and it is not cpu@0 (some bootloaders allows you to change the boot CPU). Hi Julien, Yes I will specify that the boot cpu is listed in cpupool_a, so that cpupool will have id 0 regardless of the parsing order. > > Also, here you write "The cpupool with id X" but ... > >> +A system having the cpupools specification below will instruct Xen to have three >> +cpupools: >> + >> +- The cpupool Pool-0 will have 2 cpus assigned. >> +- The cpupool Pool-1 will have 2 cpus assigned. >> +- The cpupool Pool-2 will have 2 cpus assigned (created by Xen with all the not >> + assigned cpus a53_3 and a53_4). > > here you write "The cpupool Pool-X". Can you be consistent? Sure, do you have a preference between “The cpupool with id X” and “Pool-X”? Otherwise I would go for Pool-X everywhere. > > On a separate topic, I think dom0_max_vcpus() needs to be updated to by default (i.e when opt_dom0_max_vcpus == 0) the number of vCPUs match the number of vCPUs in the cpupool (I think 0) used to created dom0. Yes right, I didn’t think about that, I think the change could be something like that: diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index 9c67a483d4a4..9787104c3d31 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -73,7 +73,10 @@ custom_param("dom0_mem", parse_dom0_mem); unsigned int __init dom0_max_vcpus(void) { if ( opt_dom0_max_vcpus == 0 ) - opt_dom0_max_vcpus = num_online_cpus(); + { + ASSERT(cpupool0); + opt_dom0_max_vcpus = cpumask_weight(cpupool_valid_cpus(cpupool0)); + } if ( opt_dom0_max_vcpus > MAX_VIRT_CPUS ) opt_dom0_max_vcpus = MAX_VIRT_CPUS; And if you agree I will include the changes for the v7. Cheers, Luca > > Cheers, > > -- > Julien Grall
Hi Luca, On 11/04/2022 12:30, Luca Fancellu wrote: >> On 11 Apr 2022, at 11:58, Julien Grall <julien@xen.org> wrote: >> >> Hi Luca, >> >> On 08/04/2022 09:45, Luca Fancellu wrote: >>> diff --git a/docs/misc/arm/device-tree/cpupools.txt b/docs/misc/arm/device-tree/cpupools.txt >>> new file mode 100644 >>> index 000000000000..40cc8135c66f >>> --- /dev/null >>> +++ b/docs/misc/arm/device-tree/cpupools.txt >>> @@ -0,0 +1,140 @@ >>> +Boot time cpupools >>> +================== >>> + >>> +When BOOT_TIME_CPUPOOLS is enabled in the Xen configuration, it is possible to >>> +create cpupools during boot phase by specifying them in the device tree. >>> +ACPI is not supported for this feature. >>> + >>> +Cpupools specification nodes shall be direct childs of /chosen node. >>> +Each cpupool node contains the following properties: >>> + >>> +- compatible (mandatory) >>> + >>> + Must always include the compatiblity string: "xen,cpupool". >>> + >>> +- cpupool-cpus (mandatory) >>> + >>> + Must be a list of device tree phandle to nodes describing cpus (e.g. having >>> + device_type = "cpu"), it can't be empty. >>> + >>> +- cpupool-sched (optional) >>> + >>> + Must be a string having the name of a Xen scheduler. Check the sched=<...> >>> + boot argument for allowed values [1]. When this property is omitted, the Xen >>> + default scheduler will be used. >>> + >>> + >>> +Constraints >>> +=========== >>> + >>> +If no cpupools are specified, all cpus will be assigned to one cpupool >>> +implicitly created (Pool-0). >>> + >>> +If cpupools node are specified, but not every cpu brought up by Xen is assigned, >>> +all the not assigned cpu will be assigned to an additional cpupool. >>> + >>> +If a cpu is assigned to a cpupool, but it's not brought up correctly, Xen will >>> +stop. >>> + >>> +The boot cpu must be assigned to Pool-0, so the cpupool containing that core >>> +will become Pool-0 automatically. >>> + >>> + >>> +Examples >>> +======== >>> + >>> +A system having two types of core, the following device tree specification will >>> +instruct Xen to have two cpupools: >>> + >>> +- The cpupool with id 0 will have 4 cpus assigned. >>> +- The cpupool with id 1 will have 2 cpus assigned. >> >> AFAIK, there are no guarantee that Xen will parse cpupool_a first. So it would be possible that the ID are inverted here. >> >> This could happen if you want to keep the boot CPU in pool 0 and it is not cpu@0 (some bootloaders allows you to change the boot CPU). > Yes I will specify that the boot cpu is listed in cpupool_a, so that cpupool will have id 0 regardless of the parsing order. This only covers the case where are two cpupools. AFAIK, there are no guarantee that Xen will parse the DT or the compiler will generate the DT the way you want. So for three cpupools, we still don't know which pool will be ID 1/2. See more below. > >> >> Also, here you write "The cpupool with id X" but ... >> >>> +A system having the cpupools specification below will instruct Xen to have three >>> +cpupools: >>> + >>> +- The cpupool Pool-0 will have 2 cpus assigned. >>> +- The cpupool Pool-1 will have 2 cpus assigned. >>> +- The cpupool Pool-2 will have 2 cpus assigned (created by Xen with all the not >>> + assigned cpus a53_3 and a53_4). >> >> here you write "The cpupool Pool-X". Can you be consistent? > > Sure, do you have a preference between “The cpupool with id X” and “Pool-X”? Otherwise I would go for Pool-X everywhere. Using "cpupool with ID 0" is definitely wrong. Pool-X is marginally better because an admin may think that this name will match what we have in Xen. So I think it would be better to use the node name and mention that there are no guarantee in which ID will used by Xen. > >> >> On a separate topic, I think dom0_max_vcpus() needs to be updated to by default (i.e when opt_dom0_max_vcpus == 0) the number of vCPUs match the number of vCPUs in the cpupool (I think 0) used to created dom0. > > Yes right, I didn’t think about that, I think the change could be something like that: > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > index 9c67a483d4a4..9787104c3d31 100644 > --- a/xen/arch/arm/domain_build.c > +++ b/xen/arch/arm/domain_build.c > @@ -73,7 +73,10 @@ custom_param("dom0_mem", parse_dom0_mem); > unsigned int __init dom0_max_vcpus(void) > { > if ( opt_dom0_max_vcpus == 0 ) > - opt_dom0_max_vcpus = num_online_cpus(); > + { > + ASSERT(cpupool0); > + opt_dom0_max_vcpus = cpumask_weight(cpupool_valid_cpus(cpupool0)); > + } > if ( opt_dom0_max_vcpus > MAX_VIRT_CPUS ) > opt_dom0_max_vcpus = MAX_VIRT_CPUS; > > And if you agree I will include the changes for the v7. This should work. Cheers,
diff --git a/docs/misc/arm/device-tree/cpupools.txt b/docs/misc/arm/device-tree/cpupools.txt new file mode 100644 index 000000000000..40cc8135c66f --- /dev/null +++ b/docs/misc/arm/device-tree/cpupools.txt @@ -0,0 +1,140 @@ +Boot time cpupools +================== + +When BOOT_TIME_CPUPOOLS is enabled in the Xen configuration, it is possible to +create cpupools during boot phase by specifying them in the device tree. +ACPI is not supported for this feature. + +Cpupools specification nodes shall be direct childs of /chosen node. +Each cpupool node contains the following properties: + +- compatible (mandatory) + + Must always include the compatiblity string: "xen,cpupool". + +- cpupool-cpus (mandatory) + + Must be a list of device tree phandle to nodes describing cpus (e.g. having + device_type = "cpu"), it can't be empty. + +- cpupool-sched (optional) + + Must be a string having the name of a Xen scheduler. Check the sched=<...> + boot argument for allowed values [1]. When this property is omitted, the Xen + default scheduler will be used. + + +Constraints +=========== + +If no cpupools are specified, all cpus will be assigned to one cpupool +implicitly created (Pool-0). + +If cpupools node are specified, but not every cpu brought up by Xen is assigned, +all the not assigned cpu will be assigned to an additional cpupool. + +If a cpu is assigned to a cpupool, but it's not brought up correctly, Xen will +stop. + +The boot cpu must be assigned to Pool-0, so the cpupool containing that core +will become Pool-0 automatically. + + +Examples +======== + +A system having two types of core, the following device tree specification will +instruct Xen to have two cpupools: + +- The cpupool with id 0 will have 4 cpus assigned. +- The cpupool with id 1 will have 2 cpus assigned. + +The following example can work only if hmp-unsafe=1 is passed to Xen boot +arguments, otherwise not all cores will be brought up by Xen and the cpupool +creation process will stop Xen. + + +a72_1: cpu@0 { + compatible = "arm,cortex-a72"; + reg = <0x0 0x0>; + device_type = "cpu"; + [...] +}; + +a72_2: cpu@1 { + compatible = "arm,cortex-a72"; + reg = <0x0 0x1>; + device_type = "cpu"; + [...] +}; + +a53_1: cpu@100 { + compatible = "arm,cortex-a53"; + reg = <0x0 0x100>; + device_type = "cpu"; + [...] +}; + +a53_2: cpu@101 { + compatible = "arm,cortex-a53"; + reg = <0x0 0x101>; + device_type = "cpu"; + [...] +}; + +a53_3: cpu@102 { + compatible = "arm,cortex-a53"; + reg = <0x0 0x102>; + device_type = "cpu"; + [...] +}; + +a53_4: cpu@103 { + compatible = "arm,cortex-a53"; + reg = <0x0 0x103>; + device_type = "cpu"; + [...] +}; + +chosen { + + cpupool_a { + compatible = "xen,cpupool"; + cpupool-cpus = <&a53_1 &a53_2 &a53_3 &a53_4>; + }; + cpupool_b { + compatible = "xen,cpupool"; + cpupool-cpus = <&a72_1 &a72_2>; + cpupool-sched = "credit2"; + }; + + [...] + +}; + + +A system having the cpupools specification below will instruct Xen to have three +cpupools: + +- The cpupool Pool-0 will have 2 cpus assigned. +- The cpupool Pool-1 will have 2 cpus assigned. +- The cpupool Pool-2 will have 2 cpus assigned (created by Xen with all the not + assigned cpus a53_3 and a53_4). + +chosen { + + cpupool_a { + compatible = "xen,cpupool"; + cpupool-cpus = <&a53_1 &a53_2>; + }; + cpupool_b { + compatible = "xen,cpupool"; + cpupool-cpus = <&a72_1 &a72_2>; + cpupool-sched = "null"; + }; + + [...] + +}; + +[1] docs/misc/xen-command-line.pandoc diff --git a/xen/arch/arm/include/asm/smp.h b/xen/arch/arm/include/asm/smp.h index af5a2fe65266..83c0cd69767b 100644 --- a/xen/arch/arm/include/asm/smp.h +++ b/xen/arch/arm/include/asm/smp.h @@ -34,6 +34,9 @@ extern void init_secondary(void); extern void smp_init_cpus(void); extern void smp_clear_cpu_maps (void); extern int smp_get_max_cpus (void); + +#define cpu_physical_id(cpu) cpu_logical_map(cpu) + #endif /* diff --git a/xen/common/Kconfig b/xen/common/Kconfig index d921c74d615e..70aac5220e75 100644 --- a/xen/common/Kconfig +++ b/xen/common/Kconfig @@ -22,6 +22,13 @@ config GRANT_TABLE If unsure, say Y. +config BOOT_TIME_CPUPOOLS + bool "Create cpupools at boot time" + depends on HAS_DEVICE_TREE + help + Creates cpupools during boot time and assigns cpus to them. Cpupools + options can be specified in the device tree. + config ALTERNATIVE_CALL bool diff --git a/xen/common/Makefile b/xen/common/Makefile index b1e076c30b81..218174ca8b6b 100644 --- a/xen/common/Makefile +++ b/xen/common/Makefile @@ -1,5 +1,6 @@ obj-$(CONFIG_ARGO) += argo.o obj-y += bitmap.o +obj-$(CONFIG_BOOT_TIME_CPUPOOLS) += boot_cpupools.init.o obj-$(CONFIG_HYPFS_CONFIG) += config_data.o obj-$(CONFIG_CORE_PARKING) += core_parking.o obj-y += cpu.o diff --git a/xen/common/boot_cpupools.c b/xen/common/boot_cpupools.c new file mode 100644 index 000000000000..9429a5025fc4 --- /dev/null +++ b/xen/common/boot_cpupools.c @@ -0,0 +1,207 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * xen/common/boot_cpupools.c + * + * Code to create cpupools at boot time. + * + * Copyright (C) 2022 Arm Ltd. + */ + +#include <xen/sched.h> +#include <asm/acpi.h> + +/* + * pool_cpu_map: Index is logical cpu number, content is cpupool id, (-1) for + * unassigned. + * pool_sched_map: Index is cpupool id, content is scheduler id, (-1) for + * unassigned. + */ +static int __initdata pool_cpu_map[NR_CPUS] = { [0 ... NR_CPUS-1] = -1 }; +static int __initdata pool_sched_map[NR_CPUS] = { [0 ... NR_CPUS-1] = -1 }; +static unsigned int __initdata next_pool_id; + +#define BTCPUPOOLS_DT_NODE_NO_REG (-1) +#define BTCPUPOOLS_DT_NODE_NO_LOG_CPU (-2) + +static int __init get_logical_cpu_from_hw_id(unsigned int hwid) +{ + unsigned int i; + + for ( i = 0; i < nr_cpu_ids; i++ ) + { + if ( cpu_physical_id(i) == hwid ) + return i; + } + + return -1; +} + +static int __init +get_logical_cpu_from_cpu_node(const struct dt_device_node *cpu_node) +{ + int cpu_num; + const __be32 *prop; + unsigned int cpu_reg; + + prop = dt_get_property(cpu_node, "reg", NULL); + if ( !prop ) + return BTCPUPOOLS_DT_NODE_NO_REG; + + cpu_reg = dt_read_number(prop, dt_n_addr_cells(cpu_node)); + + cpu_num = get_logical_cpu_from_hw_id(cpu_reg); + if ( cpu_num < 0 ) + return BTCPUPOOLS_DT_NODE_NO_LOG_CPU; + + return cpu_num; +} + +static int __init check_and_get_sched_id(const char* scheduler_name) +{ + int sched_id = sched_get_id_by_name(scheduler_name); + + if ( sched_id < 0 ) + panic("Scheduler %s does not exists!\n", scheduler_name); + + return sched_id; +} + +void __init btcpupools_dtb_parse(void) +{ + const struct dt_device_node *chosen, *node; + + if ( !acpi_disabled ) + return; + + chosen = dt_find_node_by_path("/chosen"); + if ( !chosen ) + panic("/chosen missing. Boot time cpupools can't be parsed from DT.\n"); + + dt_for_each_child_node(chosen, node) + { + const struct dt_device_node *phandle_node; + int sched_id = -1; + const char* scheduler_name; + unsigned int i = 0; + + if ( !dt_device_is_compatible(node, "xen,cpupool") ) + continue; + + if ( !dt_property_read_string(node, "cpupool-sched", &scheduler_name) ) + sched_id = check_and_get_sched_id(scheduler_name); + + phandle_node = dt_parse_phandle(node, "cpupool-cpus", i++); + if ( !phandle_node ) + panic("Missing or empty cpupool-cpus property!\n"); + + while ( phandle_node ) + { + int cpu_num; + + cpu_num = get_logical_cpu_from_cpu_node(phandle_node); + + if ( cpu_num < 0 ) + panic("Error retrieving logical cpu from node %s (%d)\n", + dt_node_name(node), cpu_num); + + if ( pool_cpu_map[cpu_num] != -1 ) + panic("Logical cpu %d already added to a cpupool!\n", cpu_num); + + pool_cpu_map[cpu_num] = next_pool_id; + + phandle_node = dt_parse_phandle(node, "cpupool-cpus", i++); + } + + /* Save scheduler choice for this cpupool id */ + pool_sched_map[next_pool_id] = sched_id; + + /* Let Xen generate pool ids */ + next_pool_id++; + } +} + +void __init btcpupools_allocate_pools(void) +{ + unsigned int i; + bool add_extra_cpupool = false; + int swap_id = -1; + + /* + * If there are no cpupools, the value of next_pool_id is zero, so the code + * below will assign every cpu to cpupool0 as the default behavior. + * When there are cpupools, the code below is assigning all the not + * assigned cpu to a new pool (next_pool_id value is the last id + 1). + * In the same loop we check if there is any assigned cpu that is not + * online. + */ + for ( i = 0; i < nr_cpu_ids; i++ ) + { + if ( cpumask_test_cpu(i, &cpu_online_map) ) + { + /* Unassigned cpu gets next_pool_id pool id value */ + if ( pool_cpu_map[i] < 0 ) + { + pool_cpu_map[i] = next_pool_id; + add_extra_cpupool = true; + } + + /* + * Cpu0 must be in cpupool0, otherwise some operations like moving + * cpus between cpupools, cpu hotplug, destroying cpupools, shutdown + * of the host, might not work in a sane way. + */ + if ( !i && (pool_cpu_map[0] != 0) ) + swap_id = pool_cpu_map[0]; + + if ( swap_id != -1 ) + { + if ( pool_cpu_map[i] == swap_id ) + pool_cpu_map[i] = 0; + else if ( pool_cpu_map[i] == 0 ) + pool_cpu_map[i] = swap_id; + } + } + else + { + if ( pool_cpu_map[i] >= 0 ) + panic("Pool-%d contains cpu%u that is not online!\n", + pool_cpu_map[i], i); + } + } + + /* A swap happened, swap schedulers between cpupool id 0 and the other */ + if ( swap_id != -1 ) + { + int swap_sched = pool_sched_map[swap_id]; + + pool_sched_map[swap_id] = pool_sched_map[0]; + pool_sched_map[0] = swap_sched; + } + + if ( add_extra_cpupool ) + next_pool_id++; + + /* Create cpupools with selected schedulers */ + for ( i = 0; i < next_pool_id; i++ ) + cpupool_create_pool(i, pool_sched_map[i]); +} + +unsigned int __init btcpupools_get_cpupool_id(unsigned int cpu) +{ + ASSERT((cpu < NR_CPUS) && (pool_cpu_map[cpu] >= 0)); + + printk(XENLOG_INFO "Logical CPU %u in Pool-%d (Scheduler id: %d).\n", + cpu, pool_cpu_map[cpu], pool_sched_map[pool_cpu_map[cpu]]); + + return pool_cpu_map[cpu]; +} + +/* + * 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/sched/cpupool.c b/xen/common/sched/cpupool.c index 89a891af7076..86a175f99cd5 100644 --- a/xen/common/sched/cpupool.c +++ b/xen/common/sched/cpupool.c @@ -1247,12 +1247,22 @@ static int __init cf_check cpupool_init(void) cpupool_put(cpupool0); register_cpu_notifier(&cpu_nfb); + btcpupools_dtb_parse(); + + btcpupools_allocate_pools(); + spin_lock(&cpupool_lock); cpumask_copy(&cpupool_free_cpus, &cpu_online_map); for_each_cpu ( cpu, &cpupool_free_cpus ) - cpupool_assign_cpu_locked(cpupool0, cpu); + { + unsigned int pool_id = btcpupools_get_cpupool_id(cpu); + struct cpupool *pool = cpupool_find_by_id(pool_id); + + ASSERT(pool); + cpupool_assign_cpu_locked(pool, cpu); + } spin_unlock(&cpupool_lock); diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h index b527f141a1d3..453e98f1cba8 100644 --- a/xen/include/xen/sched.h +++ b/xen/include/xen/sched.h @@ -1178,6 +1178,20 @@ extern void cf_check dump_runq(unsigned char key); void arch_do_physinfo(struct xen_sysctl_physinfo *pi); +#ifdef CONFIG_BOOT_TIME_CPUPOOLS +void btcpupools_allocate_pools(void); +unsigned int btcpupools_get_cpupool_id(unsigned int cpu); +void btcpupools_dtb_parse(void); + +#else /* !CONFIG_BOOT_TIME_CPUPOOLS */ +static inline void btcpupools_allocate_pools(void) {} +static inline void btcpupools_dtb_parse(void) {} +static inline unsigned int btcpupools_get_cpupool_id(unsigned int cpu) +{ + return 0; +} +#endif + #endif /* __SCHED_H__ */ /*