Re: [PATCH v5 4/6] xen/cpupool: Create different cpupools at boot time

[Julien Grall <julien@xxxxxxx>]

Hi Luca,

On 05/04/2022 09:57, 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.
How will this work for ACPI? Note that I am not suggesting to add suport 
right now. However, we probably want to clarify that this is not yet 
supported.
[...]

> diff --git a/docs/misc/arm/device-tree/cpupools.txt 
> b/docs/misc/arm/device-tree/cpupools.txt
> new file mode 100644
> index 000000000000..5dac2b1384e0
> --- /dev/null
> +++ b/docs/misc/arm/device-tree/cpupools.txt
> @@ -0,0 +1,136 @@
> +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.
How about ACPI?

> +
> +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.
I would clarify what would be the scheduler if cpupool-sched is not 
specified.
Also, I would give a pointer to xen-command-line.pandoc so it is easier 
to know where 'sched' is described.
[...]

> +void __init btcpupools_dtb_parse(void)
> +{
> +    const struct dt_device_node *chosen, *node;
> +
> +    chosen = dt_find_node_by_path("/chosen");
> +    if ( !chosen )
> +        return;
Aside when using ACPI, the chosen node should always be there. So I 
think we should throw/print an error if chosen is not present.
Also, I would check that we haven't booted using ACPI rather than 
relying on dt_find_node_by_path("/chosen") to return NULL.
Cheers,

--
Julien Grall